[APP-16229] add play button preview to camera widget#51
Conversation
🦋 Changeset detectedLatest commit: 1cd4ce1 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@claude review |
|
Claude finished @DTCurrie's task in 2m 34s —— View job PR Review
The approach is clean and minimal — gating the feed behind a "Start feed" button is a sensible solution for this use case. Two items worth addressing before merge: Medium —
|
|
@claude review |
|
Claude finished @gmulz's task in 2m 36s —— View job PR Review
The previous review's two blocking items (missing Medium —
|
| let isPlaying = $state(false) | ||
| $effect(() => { | ||
| // Reset the play gate whenever the camera identity changes so a swapped | ||
| // resource doesn't auto-start using the previous instance's state. | ||
| void resourceName | ||
| void partID | ||
| isPlaying = false | ||
| }) | ||
| let isShowingPointcloud = $state(false) | ||
| let selectedSource = $state('') | ||
| let sourceNames = $state<string[]>([]) |
There was a problem hiding this comment.
The prop-change $effect resets isPlaying but not sourceNames or selectedSource. This causes a stale-state bug on camera swap:
- Camera A starts →
sourceNames = ['rgb', 'depth'] - Props switch to camera B
isPlayingresets → feed stops- User clicks "Start feed" for camera B
- Camera B's data arrives, but the guard at line 67 (
sourceNames.length === 0) prevents the update — camera A's source names stay displayed indefinitely
Moving the selectedSource/sourceNames declarations before the effect and resetting them there fixes both the TDZ ordering issue and the stale-state bug:
| let isPlaying = $state(false) | |
| $effect(() => { | |
| // Reset the play gate whenever the camera identity changes so a swapped | |
| // resource doesn't auto-start using the previous instance's state. | |
| void resourceName | |
| void partID | |
| isPlaying = false | |
| }) | |
| let isShowingPointcloud = $state(false) | |
| let selectedSource = $state('') | |
| let sourceNames = $state<string[]>([]) | |
| let isPlaying = $state(false) | |
| let selectedSource = $state('') | |
| let sourceNames = $state<string[]>([]) | |
| $effect(() => { | |
| // Reset state whenever the camera identity changes so a swapped | |
| // resource doesn't auto-start using the previous instance's state. | |
| void resourceName | |
| void partID | |
| isPlaying = false | |
| sourceNames = [] | |
| selectedSource = '' | |
| }) | |
| let isShowingPointcloud = $state(false) |
There was a problem hiding this comment.
i dont think this matters because sourceNames and selectedSource are already set in an $effect based on the results of the call to GetImages
adds a small preview gate in front of the camera widget. we had discussed broader change to the refetch controller, but i think this is a quick and dirty approach to get something through for the camera card
i experimented with having an actual blurred image but i think it looks dumb. maybe if it were more blurry
