Repository navigation
Exclude archived candidates from the rendered video - #199
Conversation
Fixes SM-1 (HIGH) and SM-17 (LOW).
When a user archives the selected storyboard candidate, the candidate card
is hidden from the storyboard UI, but its index previously remained in
scene.selectedCandidateIndex. Because resolveSceneRenderClip lacked an
archive check, the hidden candidate was still fed into the combine workflow
and rendered into the exported video.
This implements a two-layer defense:
1. In Storyboard.toggleArchive (storyboard.ts), when archiving the candidate
currently pointed to by scene.selectedCandidateIndex, clear the selection
by setting scene.selectedCandidateIndex = undefined. Additionally, evict
candidate.referenceImage?.preview?.path along with the high quality
thumbnail and reference image paths from the thumbnail cache (SM-17).
2. As a defense-in-depth safety net, in resolveSceneRenderClip (config.ts),
check candidate.isArchived and return {state: 'not-selected'}. Returning
'not-selected' drops the unselected/archived scene from the rendered
composition without disabling the Render button for the entire project,
which returning 'invalid' would do.
Added regression tests in archived-candidate-render.spec.ts covering clip
resolution states, combine workflow submission filtering, and toggleArchive
selection clearing and cache eviction.
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
1 similar comment
|
Still need readability approvals from:
|
Review: merge after small fixes — but the live half of the bug is still openMulti-agent review (reviewer → independent critique agent re-verifying each claim against the code). The 12-line production guard is well-placed. Important The archived candidate can still be re-selected — and re-selecting it mutates saved scene state
It's worse than a silent drop. The One-line fix at the source: if (scene.candidates![index].isArchived) return;That makes the template The spec file: recommend deleting it and folding ~40 lines into the existing suites
Suggested shape: keep ~3 Explicitly not recommended
Verified, no action needed
One caveat on method: this review is by inspection, not execution — I did not run the suite. |
Consolidated ReviewVerdict: Merge after one small fix. This note reconciles the 19:50 comment on this PR against head Do before merge
Optional, does not block
Rejected or superseded, do not re-litigate
Evidence
Way forward
|
Follow-up on Consolidated ReviewI addressed the archived-selection blocker from the Consolidated Review on head
Verification: the focused storyboard, dictation, and archived-candidate render suites passed with 95 tests; lint and spec typecheck passed. The regression reproduced before the guard and passed afterward, including render exclusion and enabled Restore coverage. The archived drawer was checked through Angular component rendering; no live-cloud browser scenario was run. Current-head GitHub CI is complete: Python 3.11/3.12/3.13, UI build/lint/tests, deploy checks and security scans passed. Conditional zizmor jobs were skipped. |
| if (candidate.isArchived && scene.selectedCandidateIndex === index) { | ||
| scene.selectedCandidateIndex = undefined; | ||
| } | ||
| this.updateScenes(); |
There was a problem hiding this comment.
Setting scene.selectedCandidateIndex = undefined here has three issues that need to be addressed together:
- Unpersisted / Reverted Selection (
this.updateScenes()withoutscene):toggleArchivenow mutates a top-level property onscene(scene.selectedCandidateIndex = undefined), but line 1162 callsthis.updateScenes()with no arguments. InsideupdateScenes(scene?)(storyboard.ts:1125),const updatedScene = scene || this.selectedScene()falls back tothis.selectedScene(), which returns a shallow clone ({...s}) of the scene fromprojectConfig(storyboard.ts:244)—ornullifselectedSceneId()is unset. Ifscenepassed totoggleArchiveis not the exact memoized clone instance cached byselectedScene(),this.updateScenes()overwrites the storyboard entry with the stale clone whereselectedCandidateIndexis stillindex(or drops the save whenselectedScene()isnull). Passthis.updateScenes(scene)likeselectCandidateandupdateScenePromptdo. - Stale Playback / Trim Duration State: When
selectedCandidateIndexbecomesundefinedand<video #mainVideo>leaves the DOM,trimEnd()(storyboard.ts:515) falls back tothis.videoDuration(), leaving the Trim End input displaying the archived candidate's duration unlessisVideoPlaying,videoDuration, andcurrentPlaybackTimeare reset. - Ripple Effect in
RemixEngineService.attachCandidates(ui/src/app/services/remix-engine/remix-engine.ts:1799-1806) &Storyboard.selectedCandidate(storyboard.ts:250-259): When a user archivescandidates[0](selectedCandidateIndexbecomesundefined) and then clicks Generate Candidates or Edit with a prompt to generatecandidates[1],attachCandidatesevaluates:BecauseselectedCandidateIndex: selectedCandidateIndex !== undefined && Number.isInteger(selectedCandidateIndex) && selectedCandidateIndex >= 0 && selectedCandidateIndex < candidates.length ? selectedCandidateIndex : 0,
s.selectedCandidateIndexis nowundefined,attachCandidatesfalls back to: 0— re-selecting the archivedcandidates[0]instead of the newly generatedcandidates[1]! BecauseStoryboard.selectedCandidate(storyboard.ts:258) returnsscene.candidates[scene.selectedCandidateIndex]without checkingcandidate.isArchived, the Storyboard preview player immediately loads the archivedcandidates[0]whileresolveSceneRenderClip(config.ts:472) seescandidates[0].isArchived === trueand excludes the scene from the Composition playlist and render.- In
remix-engine.ts:1799-1806, verify!candidates[selectedCandidateIndex]?.isArchivedand fall back tocandidates.findIndex(c => !c.isArchived)(orundefinedif all are archived). - In
storyboard.ts:258(selectedCandidate), returncandidate?.isArchived ? undefined : candidate.
- In
| if (candidate.isArchived && scene.selectedCandidateIndex === index) { | |
| scene.selectedCandidateIndex = undefined; | |
| } | |
| this.updateScenes(); | |
| if (candidate.isArchived && scene.selectedCandidateIndex === index) { | |
| scene.selectedCandidateIndex = undefined; | |
| this.isVideoPlaying.set(false); | |
| this.videoDuration.set(0); | |
| this.currentPlaybackTime.set(0); | |
| } | |
| this.updateScenes(scene); |
There was a problem hiding this comment.
When candidates are created (remix-engine.ts:1724), newCandidate.referenceImage is initialized as a shallow clone of scene.referenceImage ({...params.referenceImage}). As a result, candidate.referenceImage?.path and candidate.referenceImage?.preview?.path share the exact same GCS object paths as:
- The scene's own
scene.referenceImage(which remains active on the scene in the Right Sidebar and Top Filmstrip when a candidate is archived), and - Other active (non-archived) candidates generated in the same run.
Unconditionally calling thumbnailCache.invalidateCandidate(projectId, path) for candidate.referenceImage?.path and candidate.referenceImage?.preview?.path deletes those entries from CacheStorage (scene-machine-thumbnails-v1) and bumps candidateVersions (media-cache-engine.ts:153-166) while scene.referenceImage and active sibling candidates are still using them. Filtering out paths that remain referenced by scene.referenceImage or any non-archived candidate in scene.candidates ensures only unreferenced thumbnails/previews are evicted.
| const projectId = this.config.projectConfig.value().id; | |
| const activePaths = new Set<string>(); | |
| if (scene.referenceImage?.path) { | |
| activePaths.add(scene.referenceImage.path); | |
| } | |
| if (scene.referenceImage?.preview?.path) { | |
| activePaths.add(scene.referenceImage.preview.path); | |
| } | |
| for (const c of scene.candidates) { | |
| if (!c.isArchived) { | |
| if (c.highQualityThumbnail?.path) { | |
| activePaths.add(c.highQualityThumbnail.path); | |
| } | |
| if (c.referenceImage?.path) { | |
| activePaths.add(c.referenceImage.path); | |
| } | |
| if (c.referenceImage?.preview?.path) { | |
| activePaths.add(c.referenceImage.preview.path); | |
| } | |
| } | |
| } | |
| for (const path of [ | |
| candidate.highQualityThumbnail?.path, | |
| candidate.referenceImage?.path, | |
| candidate.referenceImage?.preview?.path, | |
| ]) { | |
| if (path && !activePaths.has(path)) | |
| void this.thumbnailCache.invalidateCandidate(projectId, path); | |
| } | |
| } |
|
Still need readability approvals from:
|
d51226f to
4397a7b
Compare
…round empty scenes
| private sceneRenderClips = computed(() => | ||
| this.scenes().map(scene => ({ | ||
| scene, | ||
| resolution: resolveSceneRenderClip(scene), | ||
| })), | ||
| this.scenes().map((scene, index, allScenes) => { | ||
| const resolution = resolveSceneRenderClip(scene); | ||
| const prevScene = index > 0 ? allScenes[index - 1] : undefined; | ||
| const prevNotReady = | ||
| prevScene !== undefined && | ||
| resolveSceneRenderClip(prevScene).state !== 'ready'; | ||
| const effectiveScene = | ||
| prevNotReady && | ||
| (scene.transition !== undefined || | ||
| scene.transitionOverlap !== undefined) | ||
| ? { | ||
| ...scene, | ||
| transition: undefined, | ||
| transitionOverlap: undefined, | ||
| } | ||
| : scene; | ||
| return { | ||
| scene: effectiveScene, | ||
| resolution, | ||
| }; | ||
| }), | ||
| ); | ||
|
|
||
| filmstripScenes = computed(() => | ||
| this.sceneRenderClips() | ||
| .filter(({resolution}) => resolution.state === 'ready') | ||
| .map(({scene}) => scene), | ||
| .map(({scene}, index) => | ||
| index === 0 && | ||
| (scene.transition || scene.transitionOverlap !== undefined) | ||
| ? { | ||
| ...scene, | ||
| transition: undefined, | ||
| transitionOverlap: undefined, | ||
| } | ||
| : scene, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
In sceneRenderClips, effectiveScene strips transition and transitionOverlap whenever the immediately preceding scene in this.scenes() (allScenes[index - 1]) is not 'ready' (prevNotReady === true).
However, filmstripScenes filters out non-ready scenes, and composition.html:269 (@if (i > 0)) still renders a clickable .transition-icon ((click)="openTransitionModal(i)") between every adjacent pair in filmstripScenes(). If the storyboard is [Scene 0 (ready), Scene 1 (empty / archived), Scene 2 (ready)]:
filmstripScenes()renders[Scene 0, Scene 2]with a+transition button between them.- Clicking
+and saving a transition inonTransitionSelected(composition.ts:783-794) writestransitionandtransitionOverlapontoScene 2inprojectConfig. sceneRenderClipsimmediately stripstransition: undefinedfromScene 2becauseScene 1is'not-selected', so the button in the UI remains+(add_circle_outline) as if Save failed, while the hidden transition remains persisted in Firestore and unexpectedly activates ifScene 1later generates a candidate.
Exposing canTransitionFromPrev: index > 0 && !prevNotReady on filmstripScenes (and checking @if (i > 0 && scene.canTransitionFromPrev) in composition.html:269) keeps the Composition transition controls aligned with prevNotReady.
| private sceneRenderClips = computed(() => | |
| this.scenes().map(scene => ({ | |
| scene, | |
| resolution: resolveSceneRenderClip(scene), | |
| })), | |
| this.scenes().map((scene, index, allScenes) => { | |
| const resolution = resolveSceneRenderClip(scene); | |
| const prevScene = index > 0 ? allScenes[index - 1] : undefined; | |
| const prevNotReady = | |
| prevScene !== undefined && | |
| resolveSceneRenderClip(prevScene).state !== 'ready'; | |
| const effectiveScene = | |
| prevNotReady && | |
| (scene.transition !== undefined || | |
| scene.transitionOverlap !== undefined) | |
| ? { | |
| ...scene, | |
| transition: undefined, | |
| transitionOverlap: undefined, | |
| } | |
| : scene; | |
| return { | |
| scene: effectiveScene, | |
| resolution, | |
| }; | |
| }), | |
| ); | |
| filmstripScenes = computed(() => | |
| this.sceneRenderClips() | |
| .filter(({resolution}) => resolution.state === 'ready') | |
| .map(({scene}) => scene), | |
| .map(({scene}, index) => | |
| index === 0 && | |
| (scene.transition || scene.transitionOverlap !== undefined) | |
| ? { | |
| ...scene, | |
| transition: undefined, | |
| transitionOverlap: undefined, | |
| } | |
| : scene, | |
| ), | |
| ); | |
| private sceneRenderClips = computed(() => | |
| this.scenes().map((scene, index, allScenes) => { | |
| const resolution = resolveSceneRenderClip(scene); | |
| const prevScene = index > 0 ? allScenes[index - 1] : undefined; | |
| const prevNotReady = | |
| prevScene !== undefined && | |
| resolveSceneRenderClip(prevScene).state !== 'ready'; | |
| const canTransitionFromPrev = index > 0 && !prevNotReady; | |
| const effectiveScene = | |
| !canTransitionFromPrev && | |
| (scene.transition !== undefined || | |
| scene.transitionOverlap !== undefined) | |
| ? { | |
| ...scene, | |
| transition: undefined, | |
| transitionOverlap: undefined, | |
| } | |
| : scene; | |
| return { | |
| scene: effectiveScene, | |
| resolution, | |
| canTransitionFromPrev, | |
| }; | |
| }), | |
| ); | |
| filmstripScenes = computed(() => | |
| this.sceneRenderClips() | |
| .filter(({resolution}) => resolution.state === 'ready') | |
| .map(({scene, canTransitionFromPrev}, index) => | |
| index === 0 && | |
| (scene.transition || scene.transitionOverlap !== undefined) | |
| ? { | |
| ...scene, | |
| transition: undefined, | |
| transitionOverlap: undefined, | |
| canTransitionFromPrev: false, | |
| } | |
| : { | |
| ...scene, | |
| canTransitionFromPrev: index > 0 && canTransitionFromPrev, | |
| }, | |
| ), | |
| ); |
There was a problem hiding this comment.
Fixed in 51d3b95 — exposed canTransitionFromPrev on filmstripScenes() and guarded the .transition-icon button in composition.html with @if (i > 0 && scene.canTransitionFromPrev).
| const projectId = this.config.projectConfig.value().id; | ||
| for (const path of [ | ||
| candidate.highQualityThumbnail?.path, | ||
| candidate.referenceImage?.path, | ||
| candidate.referenceImage?.preview?.path, | ||
| ]) { | ||
| if (path) { | ||
| void this.thumbnailCache.invalidateCandidate(projectId, path); | ||
| } | ||
| } |
There was a problem hiding this comment.
When multiple candidates are generated in the same run (numberOfCandidates > 1) or edited from an existing candidate (editCandidate), every candidate receives a shallow clone of the same referenceImage ({...params.referenceImage} in remix-engine.ts:1724).
When the user archives candidates[0] and line 1256 auto-selects candidates[1] via this.selectCandidate(scene, nextActiveIndex), scene.referenceImage and candidates[1].referenceImage still reference the exact same referenceImage.path and referenceImage.preview.path. Running thumbnailCache.invalidateCandidate(projectId, path) unconditionally at lines 1280–1289 deletes the cached blob from CacheStorage (scene-machine-thumbnails-v1) and bumps candidateVersions for the reference image that scene.referenceImage and candidates[1] are actively displaying.
| const projectId = this.config.projectConfig.value().id; | |
| for (const path of [ | |
| candidate.highQualityThumbnail?.path, | |
| candidate.referenceImage?.path, | |
| candidate.referenceImage?.preview?.path, | |
| ]) { | |
| if (path) { | |
| void this.thumbnailCache.invalidateCandidate(projectId, path); | |
| } | |
| } | |
| const projectId = this.config.projectConfig.value().id; | |
| const activePaths = new Set<string>(); | |
| if (scene.referenceImage?.path) { | |
| activePaths.add(scene.referenceImage.path); | |
| } | |
| if (scene.referenceImage?.preview?.path) { | |
| activePaths.add(scene.referenceImage.preview.path); | |
| } | |
| for (const c of scene.candidates) { | |
| if (!c.isArchived) { | |
| if (c.highQualityThumbnail?.path) { | |
| activePaths.add(c.highQualityThumbnail.path); | |
| } | |
| if (c.referenceImage?.path) { | |
| activePaths.add(c.referenceImage.path); | |
| } | |
| if (c.referenceImage?.preview?.path) { | |
| activePaths.add(c.referenceImage.preview.path); | |
| } | |
| } | |
| } | |
| for (const path of [ | |
| candidate.highQualityThumbnail?.path, | |
| candidate.referenceImage?.path, | |
| candidate.referenceImage?.preview?.path, | |
| ]) { | |
| if (path && !activePaths.has(path)) { | |
| void this.thumbnailCache.invalidateCandidate(projectId, path); | |
| } | |
| } |
There was a problem hiding this comment.
Fixed in 51d3b95 — collected activePaths across scene.referenceImage and unarchived candidates (!c.isArchived) before invalidating thumbnail paths in toggleArchive.
| if (scene.selectedCandidateIndex === undefined) { | ||
| return scene; | ||
| } | ||
| const selected = scene.candidates?.[scene.selectedCandidateIndex]; | ||
| if (selected && !selected.isArchived) { | ||
| return scene; | ||
| } | ||
| const nextActiveIndex = | ||
| scene.candidates?.findIndex(c => !c.isArchived) ?? -1; | ||
| const updatedScene: GeneratedScene = {...scene}; | ||
| if (nextActiveIndex >= 0 && scene.candidates) { | ||
| const activeCandidate = scene.candidates[nextActiveIndex]; | ||
| updatedScene.selectedCandidateIndex = nextActiveIndex; | ||
| updatedScene.prompt = activeCandidate.prompt; | ||
| if (activeCandidate.referenceImage) { | ||
| updatedScene.referenceImage = activeCandidate.referenceImage; | ||
| } else { | ||
| delete updatedScene.referenceImage; | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Returning early whenever scene.selectedCandidateIndex === undefined (line 1056) skips normalization for scenes that already have scene.candidates (for example, projects saved when archiving the selected candidate set selectedCandidateIndex = undefined without selecting nextActiveIndex or clearing scene ingredients):
- If
scene.candidatescontains an active (!c.isArchived) candidate,selectedCandidateIndexremainsundefined(state: 'not-selected') and the loop at lines 1086–1099 deletesscene's andscene[i + 1]'s transitions. - If
scene.candidatescontains only archived candidates, staleprompt,referenceImage,lowQualityThumbnail, andhighQualityThumbnailfields are left onscene.
Only scenes with no candidates ((scene.candidates?.length ?? 0) === 0, i.e., draft scenes before candidate generation) should return early when selectedCandidateIndex === undefined.
| if (scene.selectedCandidateIndex === undefined) { | |
| return scene; | |
| } | |
| const selected = scene.candidates?.[scene.selectedCandidateIndex]; | |
| if (selected && !selected.isArchived) { | |
| return scene; | |
| } | |
| const nextActiveIndex = | |
| scene.candidates?.findIndex(c => !c.isArchived) ?? -1; | |
| const updatedScene: GeneratedScene = {...scene}; | |
| if (nextActiveIndex >= 0 && scene.candidates) { | |
| const activeCandidate = scene.candidates[nextActiveIndex]; | |
| updatedScene.selectedCandidateIndex = nextActiveIndex; | |
| updatedScene.prompt = activeCandidate.prompt; | |
| if (activeCandidate.referenceImage) { | |
| updatedScene.referenceImage = activeCandidate.referenceImage; | |
| } else { | |
| delete updatedScene.referenceImage; | |
| } | |
| } else { | |
| const hasCandidates = (scene.candidates?.length ?? 0) > 0; | |
| if (scene.selectedCandidateIndex === undefined && !hasCandidates) { | |
| return scene; | |
| } | |
| const selected = | |
| scene.selectedCandidateIndex !== undefined | |
| ? scene.candidates?.[scene.selectedCandidateIndex] | |
| : undefined; | |
| if (selected && !selected.isArchived) { | |
| return scene; | |
| } | |
| const nextActiveIndex = | |
| scene.candidates?.findIndex(c => !c.isArchived) ?? -1; | |
| const updatedScene: GeneratedScene = {...scene}; | |
| if (nextActiveIndex >= 0 && scene.candidates) { | |
| const activeCandidate = scene.candidates[nextActiveIndex]; | |
| updatedScene.selectedCandidateIndex = nextActiveIndex; | |
| updatedScene.prompt = activeCandidate.prompt; | |
| if (activeCandidate.referenceImage) { | |
| updatedScene.referenceImage = activeCandidate.referenceImage; | |
| } else { | |
| delete updatedScene.referenceImage; | |
| } | |
| delete updatedScene.lowQualityThumbnail; | |
| delete updatedScene.highQualityThumbnail; |
There was a problem hiding this comment.
Fixed in 51d3b95 — normalizeLoadedProject now only returns early when selectedCandidateIndex === undefined && !hasCandidates, normalizing scenes that have candidates while clearing scene-level thumbnails when an active candidate is selected.
| thumbnailPersistForScene( | ||
| scene: GeneratedScene | ProvidedVideoScene, | ||
| ): boolean { | ||
| if (!this.config.isGeneratedScene(scene)) return true; | ||
| const selected = scene.candidates?.[scene.selectedCandidateIndex ?? 0]; | ||
| return !selected?.isArchived; | ||
| if (scene.selectedCandidateIndex === undefined) return false; | ||
| const selected = scene.candidates?.[scene.selectedCandidateIndex]; | ||
| return !!selected && !selected.isArchived; | ||
| } |
There was a problem hiding this comment.
thumbnailPersistForScene(scene) is bound to [thumbnailImagePersist]="thumbnailPersistForScene(scene)" on the Right Sidebar Reference Image preview (storyboard.html:873).
Returning false unconditionally whenever scene.selectedCandidateIndex === undefined (line 514) disables CacheStorage persistence for every newly generated storyboard scene with a product referenceImage and every scene where a user uploads a reference image before generating video candidates. Because archiving or moving the last active candidate deletes scene.referenceImage, returning !!scene.referenceImage when selectedCandidateIndex === undefined keeps empty/all-archived scenes unpersisted while allowing active reference images on ungenerated scenes to use CacheStorage.
| thumbnailPersistForScene( | |
| scene: GeneratedScene | ProvidedVideoScene, | |
| ): boolean { | |
| if (!this.config.isGeneratedScene(scene)) return true; | |
| const selected = scene.candidates?.[scene.selectedCandidateIndex ?? 0]; | |
| return !selected?.isArchived; | |
| if (scene.selectedCandidateIndex === undefined) return false; | |
| const selected = scene.candidates?.[scene.selectedCandidateIndex]; | |
| return !!selected && !selected.isArchived; | |
| } | |
| thumbnailPersistForScene( | |
| scene: GeneratedScene | ProvidedVideoScene, | |
| ): boolean { | |
| if (!this.config.isGeneratedScene(scene)) return true; | |
| if (scene.selectedCandidateIndex === undefined) { | |
| return !!scene.referenceImage; | |
| } | |
| const selected = scene.candidates?.[scene.selectedCandidateIndex]; | |
| return !!selected && !selected.isArchived; | |
| } |
There was a problem hiding this comment.
Fixed in 51d3b95 — thumbnailPersistForScene now returns !!scene.referenceImage when selectedCandidateIndex === undefined.
… load normalization
|
Readability approvals granted:
Still need readability approvals from:
|
2 participants
Summary
Archived candidates cannot be selected or included in rendered output. Archiving the selected candidate clears its selection; attempting to select an archived candidate leaves the scene's selection, prompt and reference image unchanged and does not persist an update. Archived drawer cards no longer advertise selection through their click handler or selected styling; Restore remains available.
The shared render-clip resolver returns
not-selectedfor an archived candidate, so the same protection covers rendering and composition. Archive cleanup also evicts the candidate reference preview from the thumbnail cache (SM-17).Validation