Fix video wallpaper sync and streamline updater UI#366
Merged
webadderall merged 9 commits intomainfrom Apr 27, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors video wallpaper URL resolution and playback sync, updates exporter to treat video wallpapers as first-class media, removes informational update dialogs, and adds a dev-only update-toast preview option. Changes
Sequence Diagram(s)sequenceDiagram
participant Timeline
participant VideoPlayback
participant SyncRate as getMediaSyncPlaybackRate
participant BgVideo as BackgroundVideoElement
Timeline->>VideoPlayback: timeline time / play state update
VideoPlayback->>VideoPlayback: compute targetTime, select SpeedRegion
VideoPlayback->>SyncRate: request interpolated playback rate
SyncRate-->>VideoPlayback: interpolated playbackRate
VideoPlayback->>BgVideo: smoothly set playbackRate
alt jump or drift > threshold
VideoPlayback->>BgVideo: snap currentTime to targetTime
end
alt isPlaying toggled
VideoPlayback->>BgVideo: play() / pause()
end
BgVideo-->>VideoPlayback: render frames (synchronized)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
webadderall
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/launch/LaunchWindow.tsx (1)
1476-1486: Handle possible IPC rejection when triggering preview toast.At Line 1480, the fire-and-forget call can produce an unhandled rejection if the invoke fails; worth handling explicitly for cleaner renderer logs.
Suggested tweak
onClick={() => { - void window.electronAPI.previewUpdateToast(); + void window.electronAPI.previewUpdateToast().catch((error) => { + console.warn("Failed to preview update toast:", error); + }); setActiveDropdown("none"); }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/launch/LaunchWindow.tsx` around lines 1476 - 1486, The fire-and-forget call to window.electronAPI.previewUpdateToast inside the SHOW_DEV_UPDATE_PREVIEW DropdownItem can produce an unhandled rejection; change the onClick to call previewUpdateToast() and explicitly handle failures (e.g. window.electronAPI.previewUpdateToast().catch(err => console.error(...))) so errors are logged, and ensure setActiveDropdown("none") still runs regardless of the promise outcome; locate the handler around the DropdownItem using SHOW_DEV_UPDATE_PREVIEW, window.electronAPI.previewUpdateToast, and setActiveDropdown to implement the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/video-editor/SettingsPanel.tsx`:
- Around line 158-177: When src changes the effect currently waits for
getRenderableVideoUrl to finish and leaves the previous resolvedSrc leading to
stale preview frames; update the useEffect (the effect that references src,
getRenderableVideoUrl, setResolvedSrc and the cancelled flag) to immediately
reset resolvedSrc (call setResolvedSrc(undefined or empty value appropriate for
your state type) at the start of the effect) before starting the async
resolution so the UI clears the old preview while the new URL is being resolved;
keep the existing try/catch and cancelled logic for updating resolvedSrc with
the resolved value or fallback.
In `@src/components/video-editor/VideoPlayback.tsx`:
- Around line 1017-1019: The jump-detection logic uses the wrapped targetTime
(computed with currentTime % videoDuration) which drops from ~videoDuration to
~0 at loop boundaries and causes false positives when compared to
previousTimelineTime; change the detection to use an unwrapped timeline position
for comparison (e.g., track a monotonicTimeline or rawCurrentTime variable
derived from currentTime before modulo) or explicitly detect and ignore
wrap-around by checking if videoDuration is truthy and the difference crosses
the boundary, then only compute the display/seek target using targetTime
(currentTime % videoDuration or clampMediaTimeToDuration) while using the
unwrapped value (rawCurrentTime or monotonicTimeline) for comparing against
previousTimelineTime in the jump-detection block.
---
Nitpick comments:
In `@src/components/launch/LaunchWindow.tsx`:
- Around line 1476-1486: The fire-and-forget call to
window.electronAPI.previewUpdateToast inside the SHOW_DEV_UPDATE_PREVIEW
DropdownItem can produce an unhandled rejection; change the onClick to call
previewUpdateToast() and explicitly handle failures (e.g.
window.electronAPI.previewUpdateToast().catch(err => console.error(...))) so
errors are logged, and ensure setActiveDropdown("none") still runs regardless of
the promise outcome; locate the handler around the DropdownItem using
SHOW_DEV_UPDATE_PREVIEW, window.electronAPI.previewUpdateToast, and
setActiveDropdown to implement the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e858cf40-2f7e-450f-99d8-1a89d8860d33
📒 Files selected for processing (8)
electron/updater.tssrc/components/launch/LaunchWindow.tsxsrc/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/VideoPlayback.tsxsrc/lib/assetPath.test.tssrc/lib/assetPath.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/modernFrameRenderer.ts
💤 Files with no reviewable changes (1)
- electron/updater.ts
Comment on lines
+158
to
+177
| useEffect(() => { | ||
| let cancelled = false; | ||
|
|
||
| void (async () => { | ||
| try { | ||
| const nextSrc = await getRenderableVideoUrl(src); | ||
| if (!cancelled) { | ||
| setResolvedSrc(nextSrc); | ||
| } | ||
| } catch { | ||
| if (!cancelled) { | ||
| setResolvedSrc(src); | ||
| } | ||
| } | ||
| })(); | ||
|
|
||
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [src]); |
Contributor
There was a problem hiding this comment.
Reset resolvedSrc immediately on src changes to avoid stale preview frames.
Line 156/Line 158 currently keeps the old resolved URL until async resolution finishes, which can briefly show the wrong tile video after list updates/reordering.
Suggested patch
function WallpaperVideoPreview({ src }: { src: string }) {
const [resolvedSrc, setResolvedSrc] = useState(src);
useEffect(() => {
let cancelled = false;
+ setResolvedSrc(src);
void (async () => {
try {
const nextSrc = await getRenderableVideoUrl(src);
if (!cancelled) {
setResolvedSrc(nextSrc);
}
} catch {
if (!cancelled) {
setResolvedSrc(src);
}
}
})();
return () => {
cancelled = true;
};
}, [src]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| useEffect(() => { | |
| let cancelled = false; | |
| void (async () => { | |
| try { | |
| const nextSrc = await getRenderableVideoUrl(src); | |
| if (!cancelled) { | |
| setResolvedSrc(nextSrc); | |
| } | |
| } catch { | |
| if (!cancelled) { | |
| setResolvedSrc(src); | |
| } | |
| } | |
| })(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [src]); | |
| useEffect(() => { | |
| let cancelled = false; | |
| setResolvedSrc(src); | |
| void (async () => { | |
| try { | |
| const nextSrc = await getRenderableVideoUrl(src); | |
| if (!cancelled) { | |
| setResolvedSrc(nextSrc); | |
| } | |
| } catch { | |
| if (!cancelled) { | |
| setResolvedSrc(src); | |
| } | |
| } | |
| })(); | |
| return () => { | |
| cancelled = true; | |
| }; | |
| }, [src]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/video-editor/SettingsPanel.tsx` around lines 158 - 177, When
src changes the effect currently waits for getRenderableVideoUrl to finish and
leaves the previous resolvedSrc leading to stale preview frames; update the
useEffect (the effect that references src, getRenderableVideoUrl, setResolvedSrc
and the cancelled flag) to immediately reset resolvedSrc (call
setResolvedSrc(undefined or empty value appropriate for your state type) at the
start of the effect) before starting the async resolution so the UI clears the
old preview while the new URL is being resolved; keep the existing try/catch and
cancelled logic for updating resolvedSrc with the resolved value or fallback.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/exporter/modernFrameRenderer.ts (1)
339-353:⚠️ Potential issue | 🟠 MajorMissing destroy-time cleanup for new background resources
Line 339 and Line 352-353 add long-lived background resources, but
destroy()does not revokecleanupBackgroundSourceor clear the background staging canvas/context. This can leak blob URLs/memory across exporter instances.💡 Suggested fix
destroy(): void { + this.cleanupBackgroundSource?.(); + this.cleanupBackgroundSource = null; + this.closeBackgroundDecodedFrame(); this.backgroundForwardFrameSource?.cancel(); void this.backgroundForwardFrameSource?.destroy(); this.backgroundForwardFrameSource = null; @@ this.sceneVideoFrameStagingCanvas = null; this.sceneVideoFrameStagingCtx = null; + this.backgroundVideoFrameStagingCanvas = null; + this.backgroundVideoFrameStagingCtx = null; this.webcamVideoFrameStagingCanvas = null; this.webcamVideoFrameStagingCtx = null;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/modernFrameRenderer.ts` around lines 339 - 353, The new background resources cleanupBackgroundSource, backgroundVideoFrameStagingCanvas and backgroundVideoFrameStagingCtx must be released in destroy(): if cleanupBackgroundSource is set, call it and then set cleanupBackgroundSource = null; if backgroundVideoFrameStagingCanvas exists, clear it (e.g., set width/height = 0 or clearRect) and set backgroundVideoFrameStagingCtx = null and backgroundVideoFrameStagingCanvas = null; ensure any related blob URLs or video-related sources are revoked in that same block to prevent memory/URL leaks; add these steps to the existing destroy() method next to other resource teardown so the exporter fully releases background resources.
🧹 Nitpick comments (1)
src/lib/exporter/modernFrameRenderer.ts (1)
1477-1485: Consider handling background startup-staging mode transitions like scene/webcamLine 1477-1485 now stages background frames, but background texture updates do not track staging mode transitions (canvas ↔ decoded frame) the way scene/webcam do. This asymmetry risks stale/incorrect texture updates when startup staging turns off.
♻️ Suggested direction
+ private backgroundTextureUsesStartupStaging = false; @@ - const resolvedBackgroundSource = this.stageVideoFrameForTexture( + const resolvedBackgroundSource = this.stageVideoFrameForTexture( decodedFrame, "background", decodedFrame.displayWidth, decodedFrame.displayHeight, ); + const usesStartupStaging = resolvedBackgroundSource !== decodedFrame; this.ensureBackgroundSprite( resolvedBackgroundSource, decodedFrame.displayWidth, decodedFrame.displayHeight, + usesStartupStaging, );And mirror the scene/webcam swap logic inside
ensureBackgroundSprite(...)(replace texture when mode flips).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/modernFrameRenderer.ts` around lines 1477 - 1485, The background path now stages frames via stageVideoFrameForTexture and calls ensureBackgroundSprite, but ensureBackgroundSprite does not detect or handle startup staging mode transitions (canvas ↔ decoded frame) the way the scene/webcam code does, which can leave an old texture in place; update ensureBackgroundSprite to mirror the scene/webcam swap logic: detect when the resolvedBackgroundSource type/mode has changed compared to the current background sprite's texture source and replace the sprite's texture (dispose old texture if needed) when a flip between canvas and decoded frame occurs, ensuring stageVideoFrameForTexture+ensureBackgroundSprite together perform the same mode-aware texture swap behavior as scene/webcam.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/exporter/modernFrameRenderer.ts`:
- Around line 339-353: The new background resources cleanupBackgroundSource,
backgroundVideoFrameStagingCanvas and backgroundVideoFrameStagingCtx must be
released in destroy(): if cleanupBackgroundSource is set, call it and then set
cleanupBackgroundSource = null; if backgroundVideoFrameStagingCanvas exists,
clear it (e.g., set width/height = 0 or clearRect) and set
backgroundVideoFrameStagingCtx = null and backgroundVideoFrameStagingCanvas =
null; ensure any related blob URLs or video-related sources are revoked in that
same block to prevent memory/URL leaks; add these steps to the existing
destroy() method next to other resource teardown so the exporter fully releases
background resources.
---
Nitpick comments:
In `@src/lib/exporter/modernFrameRenderer.ts`:
- Around line 1477-1485: The background path now stages frames via
stageVideoFrameForTexture and calls ensureBackgroundSprite, but
ensureBackgroundSprite does not detect or handle startup staging mode
transitions (canvas ↔ decoded frame) the way the scene/webcam code does, which
can leave an old texture in place; update ensureBackgroundSprite to mirror the
scene/webcam swap logic: detect when the resolvedBackgroundSource type/mode has
changed compared to the current background sprite's texture source and replace
the sprite's texture (dispose old texture if needed) when a flip between canvas
and decoded frame occurs, ensuring
stageVideoFrameForTexture+ensureBackgroundSprite together perform the same
mode-aware texture swap behavior as scene/webcam.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e7f5c1eb-53c8-46c6-b15d-bf46b1cad21a
📒 Files selected for processing (6)
src/lib/assetPath.test.tssrc/lib/assetPath.tssrc/lib/exporter/frameRenderer.test.tssrc/lib/exporter/frameRenderer.tssrc/lib/exporter/modernFrameRenderer.test.tssrc/lib/exporter/modernFrameRenderer.ts
✅ Files skipped from review due to trivial changes (2)
- src/lib/exporter/frameRenderer.test.ts
- src/lib/exporter/modernFrameRenderer.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/lib/assetPath.test.ts
- src/lib/exporter/frameRenderer.ts
- src/lib/assetPath.ts
Collaborator
Author
|
@CodeRabbit Review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/exporter/modernFrameRenderer.ts (2)
339-353:⚠️ Potential issue | 🟠 MajorRevoke the new background resources on every teardown path.
Line 865 stores a revoker for local wallpaper blob URLs, but today that callback is only consumed from
resetBackgroundLayer(). If the initial load falls into the fallback path, or the renderer goes straight todestroy(), the blob URL stays alive. The new background staging canvas/context at Lines 352-353 also never get cleared indestroy(), unlike the scene/webcam staging buffers. This will accumulate memory across exports with local video wallpapers.♻️ Suggested cleanup
} catch (error) { + this.cleanupBackgroundSource?.(); + this.cleanupBackgroundSource = null; console.error("[FrameRenderer] Error setting up background, using fallback:", error); const fallback = document.createElement("canvas"); fallback.width = this.config.width; @@ if (this.backgroundVideoElement) { this.backgroundVideoElement.pause(); this.backgroundVideoElement.src = ""; this.backgroundVideoElement.load(); this.backgroundVideoElement = null; } + this.cleanupBackgroundSource?.(); + this.cleanupBackgroundSource = null; @@ this.sceneVideoFrameStagingCanvas = null; this.sceneVideoFrameStagingCtx = null; + this.backgroundVideoFrameStagingCanvas = null; + this.backgroundVideoFrameStagingCtx = null; this.webcamVideoFrameStagingCanvas = null; this.webcamVideoFrameStagingCtx = null;Also applies to: 833-834, 865-867
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/modernFrameRenderer.ts` around lines 339 - 353, The cleanupBackgroundSource revoker and the background staging buffers are not always torn down — ensure any stored revoker in cleanupBackgroundSource is invoked and cleared on every teardown path (including destroy() and any background-load fallback paths), and null out/cleanup backgroundVideoFrameStagingCanvas and backgroundVideoFrameStagingCtx just like the scene/webcam staging buffers; update resetBackgroundLayer(), destroy(), and any fallback branch that sets the local wallpaper to call the revoker (if present) and then set cleanupBackgroundSource = null, and set backgroundVideoFrameStagingCanvas = null and backgroundVideoFrameStagingCtx = null to prevent leaked blob URLs and canvases.
859-898:⚠️ Potential issue | 🟠 MajorSynchronize background wallpaper frames using the same presented-frame barrier as webcams.
Lines 859-898 create an
HTMLVideoElementfor video wallpapers, routed tosyncBackgroundFrame(). However, this function (lines 1477-1501) waits only for theseekedevent before callingensureBackgroundSprite(). Theseekedevent does not guarantee the new frame is ready to draw—around seeks and speed transitions, this can export the previous wallpaper frame for one tick.
syncWebcamFrame()(lines 1921-1960+) already addresses this by callingwaitForPresentedFrame()after seek completion, which usesrequestVideoFrameCallbackandrequestAnimationFrameto ensure the frame is presented before proceeding. Apply the same pattern tosyncBackgroundFrame()for consistency and correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/exporter/modernFrameRenderer.ts` around lines 859 - 898, syncBackgroundFrame currently proceeds after the video `seeked`/loaded event which can still yield the previous frame; update syncBackgroundFrame to mirror syncWebcamFrame by calling the existing waitForPresentedFrame(video) (the helper that uses requestVideoFrameCallback + requestAnimationFrame) after seek completion and before calling ensureBackgroundSprite(), so the wallpaper frame is guaranteed presented; locate syncBackgroundFrame and replace the post-seek/loaded completion path to await waitForPresentedFrame(video) (or equivalent) rather than relying solely on the seeked event, ensuring ensureBackgroundSprite() runs only after waitForPresentedFrame resolves.
🧹 Nitpick comments (2)
src/components/video-editor/timeline/timelineLayout.test.ts (1)
34-40: Consider pinning normalization edge-cases in stretch-factor tests.Since the helper internally normalizes input, adding assertions for negative/
NaN/fractional values would lock intended behavior and reduce regressions around Line 34.Proposed test additions
it("stretches content height to keep a two-row viewport", () => { expect(TIMELINE_VISIBLE_ROW_COUNT).toBe(2); expect(getTimelineViewportStretchFactor(2)).toBe(1); expect(getTimelineViewportStretchFactor(4)).toBe(2); expect(getTimelineViewportStretchFactor(5)).toBe(2.5); expect(getTimelineViewportStretchFactor(0)).toBe(1); + expect(getTimelineViewportStretchFactor(-1)).toBe(1); + expect(getTimelineViewportStretchFactor(Number.NaN)).toBe(1); + expect(getTimelineViewportStretchFactor(2.9)).toBe(1); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/timelineLayout.test.ts` around lines 34 - 40, Add assertions that pin the normalization behavior of getTimelineViewportStretchFactor for edge inputs: include tests verifying negative numbers (e.g., -1), NaN, and fractional values (e.g., 2.3 or 0.5) return the expected normalized stretch factor (consistent with TIMELINE_VISIBLE_ROW_COUNT behavior and existing cases). Update the test block that references TIMELINE_VISIBLE_ROW_COUNT and getTimelineViewportStretchFactor to explicitly assert handling of negative, NaN, and fractional inputs so the intended normalization logic is locked in and prevents regressions.src/components/video-editor/timeline/TimelineEditor.tsx (1)
721-721: Extract the height expression to a named memoized value for readability.Line 721 is doing the right math, but inlining this full CSS expression makes future edits riskier.
Proposed refactor
const timelineRowsMinHeightPx = getTimelineRowsMinHeightPx(timelineRowCount); const timelineContentMinHeightPx = getTimelineContentMinHeightPx(timelineRowCount); const timelineViewportStretchFactor = getTimelineViewportStretchFactor(timelineRowCount); + const timelineHeight = useMemo( + () => + `max(100%, ${timelineContentMinHeightPx}px, calc(${TIMELINE_AXIS_HEIGHT_PX}px + (100% - ${TIMELINE_AXIS_HEIGHT_PX}px) * ${timelineViewportStretchFactor}))`, + [timelineContentMinHeightPx, timelineViewportStretchFactor], + ); return ( <div ref={setRefs} style={{ ...style, - height: `max(100%, ${timelineContentMinHeightPx}px, calc(${TIMELINE_AXIS_HEIGHT_PX}px + (100% - ${TIMELINE_AXIS_HEIGHT_PX}px) * ${timelineViewportStretchFactor}))`, + height: timelineHeight, }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/video-editor/timeline/TimelineEditor.tsx` at line 721, Extract the complex CSS height string into a named memoized value to improve readability and prevent accidental breakage: inside the TimelineEditor component compute a const (e.g., computedTimelineHeight or timelineHeightStyle) using useMemo that returns the template string `max(100%, ${timelineContentMinHeightPx}px, calc(${TIMELINE_AXIS_HEIGHT_PX}px + (100% - ${TIMELINE_AXIS_HEIGHT_PX}px) * ${timelineViewportStretchFactor}))`, then replace the inline expression at the height prop with that constant; reference the existing symbols TIMELINE_AXIS_HEIGHT_PX, timelineContentMinHeightPx, and timelineViewportStretchFactor when building the memo so it only recalculates when those change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/exporter/modernFrameRenderer.ts`:
- Around line 339-353: The cleanupBackgroundSource revoker and the background
staging buffers are not always torn down — ensure any stored revoker in
cleanupBackgroundSource is invoked and cleared on every teardown path (including
destroy() and any background-load fallback paths), and null out/cleanup
backgroundVideoFrameStagingCanvas and backgroundVideoFrameStagingCtx just like
the scene/webcam staging buffers; update resetBackgroundLayer(), destroy(), and
any fallback branch that sets the local wallpaper to call the revoker (if
present) and then set cleanupBackgroundSource = null, and set
backgroundVideoFrameStagingCanvas = null and backgroundVideoFrameStagingCtx =
null to prevent leaked blob URLs and canvases.
- Around line 859-898: syncBackgroundFrame currently proceeds after the video
`seeked`/loaded event which can still yield the previous frame; update
syncBackgroundFrame to mirror syncWebcamFrame by calling the existing
waitForPresentedFrame(video) (the helper that uses requestVideoFrameCallback +
requestAnimationFrame) after seek completion and before calling
ensureBackgroundSprite(), so the wallpaper frame is guaranteed presented; locate
syncBackgroundFrame and replace the post-seek/loaded completion path to await
waitForPresentedFrame(video) (or equivalent) rather than relying solely on the
seeked event, ensuring ensureBackgroundSprite() runs only after
waitForPresentedFrame resolves.
---
Nitpick comments:
In `@src/components/video-editor/timeline/TimelineEditor.tsx`:
- Line 721: Extract the complex CSS height string into a named memoized value to
improve readability and prevent accidental breakage: inside the TimelineEditor
component compute a const (e.g., computedTimelineHeight or timelineHeightStyle)
using useMemo that returns the template string `max(100%,
${timelineContentMinHeightPx}px, calc(${TIMELINE_AXIS_HEIGHT_PX}px + (100% -
${TIMELINE_AXIS_HEIGHT_PX}px) * ${timelineViewportStretchFactor}))`, then
replace the inline expression at the height prop with that constant; reference
the existing symbols TIMELINE_AXIS_HEIGHT_PX, timelineContentMinHeightPx, and
timelineViewportStretchFactor when building the memo so it only recalculates
when those change.
In `@src/components/video-editor/timeline/timelineLayout.test.ts`:
- Around line 34-40: Add assertions that pin the normalization behavior of
getTimelineViewportStretchFactor for edge inputs: include tests verifying
negative numbers (e.g., -1), NaN, and fractional values (e.g., 2.3 or 0.5)
return the expected normalized stretch factor (consistent with
TIMELINE_VISIBLE_ROW_COUNT behavior and existing cases). Update the test block
that references TIMELINE_VISIBLE_ROW_COUNT and getTimelineViewportStretchFactor
to explicitly assert handling of negative, NaN, and fractional inputs so the
intended normalization logic is locked in and prevents regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e7a924f-70ca-4185-b53e-58ab03e72790
📒 Files selected for processing (6)
src/components/video-editor/SettingsPanel.tsxsrc/components/video-editor/timeline/TimelineEditor.tsxsrc/components/video-editor/timeline/TimelineWrapper.tsxsrc/components/video-editor/timeline/timelineLayout.test.tssrc/components/video-editor/timeline/timelineLayout.tssrc/lib/exporter/modernFrameRenderer.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/video-editor/timeline/TimelineWrapper.tsx
This was referenced Apr 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
Preview Update UIlauncher action to development builds onlyWhy
Custom video wallpapers were being routed through image-oriented asset handling, which could break preview/export behavior and let the wallpaper drift away from the main clip timeline. The updater flow also surfaced non-actionable UI during manual checks.
Impact
Root cause
Video wallpaper sources were not consistently treated as video resources end-to-end, and preview sync only hard-corrected wallpaper time during paused scrubbing instead of continuously following the clip timeline.
Validation
./node_modules/.bin/tsc --noEmit@rollup/rollup-darwin-arm64) before tests executeSummary by CodeRabbit
New Features
Improvements