Skip to content

[APP-16229] add play button preview to camera widget#51

Open
gmulz wants to merge 3 commits intomainfrom
gmulitz-camera-card-preview-ui
Open

[APP-16229] add play button preview to camera widget#51
gmulz wants to merge 3 commits intomainfrom
gmulitz-camera-card-preview-ui

Conversation

@gmulz
Copy link
Copy Markdown
Member

@gmulz gmulz commented Apr 30, 2026

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

image

i experimented with having an actual blurred image but i think it looks dumb. maybe if it were more blurry
image

@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 30, 2026

🦋 Changeset detected

Latest 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

@gmulz gmulz requested review from DTCurrie and micheal-parks April 30, 2026 16:21
@DTCurrie
Copy link
Copy Markdown
Member

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented Apr 30, 2026

Claude finished @DTCurrie's task in 2m 34s —— View job


PR Review

  • Read CLAUDE.md and gather context
  • Read modified files (camera.svelte, live-or-polling-video.svelte)
  • Run git diff
  • Post inline comments
  • Post review summary

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 — isPlaying not reset on prop change

See inline comment on line 33. If the widget is reused with a different resourceName (prop swap rather than remount), isPlaying stays true and the new camera's feed starts immediately, bypassing the preview gate. A $effect that reads resourceName and partID to reset isPlaying = false would fix this.

Minor — Invalid aria-label usage

See inline comment on line 149. aria-label on a plain <div> without a role is ignored by screen readers, so the "feed paused" state isn't communicated accessibly. Since the Button already has "Start feed" as its accessible name, the simplest fix is removing the aria-label entirely.

Nit — No tests for the new gate behavior

There are no component tests for camera.svelte at all (only get-source-names.spec.ts), so this isn't a regression, but the new isPlaying flow is a good candidate for a test:

  • Initial render shows "Start feed" button and no LiveOrPollingVideo
  • Clicking "Start feed" renders the video component and controls

Pre-existing — slot="input" (Svelte 4 syntax)

Lines 122 and 212 still use slot="input" (Svelte 4). Carried over as-is from before this PR, so out of scope here, but worth a follow-up.

Comment thread src/lib/components/widgets/camera/camera.svelte
Comment thread src/lib/components/widgets/camera/camera.svelte Outdated
@gmulz
Copy link
Copy Markdown
Member Author

gmulz commented May 1, 2026

@claude review

@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 1, 2026

Claude finished @gmulz's task in 2m 36s —— View job


PR Review

  • Read CLAUDE.md and gather context
  • Read modified files
  • Run git diff
  • Post inline comments
  • Post review summary

The previous review's two blocking items (missing isPlaying reset on prop change, and the invalid aria-label) have both been addressed. The gate logic and the $effect comment are clean. One new medium issue found:

Medium — sourceNames not reset on prop change

See inline comment on lines 33–43. The prop-change $effect resets isPlaying but not sourceNames or selectedSource. The source-population guard at line 67 (sourceNames.length === 0) means stale source names from the previous camera persist after a swap — camera B will show camera A's sources indefinitely. The suggestion in the inline comment fixes it and also resolves a minor TDZ ordering concern (declarations referenced in the effect appear after it). Fix this →

Nit — No way to stop the feed once started

Once "Start feed" is clicked there's no way back to the idle state short of remounting. Presumably intentional for this PR, but worth a follow-up if the camera card needs a stop/pause control.

Pre-existing — Svelte 4 patterns

slot="input" (lines 130, 216) and on:change / on:input (lines 129, 219) are Svelte 4 syntax — carried over from before this PR, so out of scope here.

Comment on lines +33 to 43
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[]>([])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prop-change $effect resets isPlaying but not sourceNames or selectedSource. This causes a stale-state bug on camera swap:

  1. Camera A starts → sourceNames = ['rgb', 'depth']
  2. Props switch to camera B
  3. isPlaying resets → feed stops
  4. User clicks "Start feed" for camera B
  5. 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:

Suggested change
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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants