Conversation
…frame GPU transitions (glitch, fade, etc.) render via WebGPU shaders only through the composition renderer canvas overlay. When paused on a transition frame without GPU effects on items, the overlay was hidden and the DOM Player showed the raw video frame — missing the transition entirely. Add targeted paused-on-transition handling: pin the transition session, use currentFrame as the render target, and bypass the overlay guard so the composition renderer draws the GPU transition. Also fix prevTargetFrame calculation to use the previous state's transition check so seeking away from a transition correctly hides the overlay. Separately, wire useGpuEffectsOverlay to detect live gizmo effect previews so the overlay activates when previewing GPU effects from the panel.
…range The subscription cleared item selection whenever the playhead moved outside the selected clips' time range during playback. This was disruptive — users lost their selection unexpectedly while playing through the timeline.
Replace the separate eye (visibility) and volume (mute) buttons on track headers with a single Power/PowerOff toggle. Video tracks toggle visibility, audio tracks toggle mute, and mixed tracks toggle both. This simplifies the track header UI from two buttons to one context-aware control.
Add draggable fade handles for video clips, mirroring the existing audio fade handle UX. Includes SVG fade overlay, drag-to-adjust, double-click to reset, and live preview during edits.
…igibility Replace hardcoded `item.type === 'video'` checks with a `supportsVisualFadeControls` predicate so fade handles can be extended to image, shape, and other visual item types in one place.
Multiply fadeIn/fadeOut opacity into the resolved transform for video and composition items so exported renders honour clip fades. Overlapping fades use a midpoint crossover to avoid clamping artifacts. Also fix corner-pin rendering when opacity < 1 — flatten the warped image onto a temp canvas first so globalAlpha applies uniformly.
…cold starts Lower concurrent extraction limits and frame budgets so dropping multiple clips into a fresh timeline does not fan out into a large parallel decode burst. Prefer breadth (one worker per clip) over depth when there is an extraction backlog, keeping the UI more responsive.
When the first video is dropped into an empty project, show a dialog offering to match the project resolution and/or frame rate to the media. Canvas panel changes now go through undo/redo history so resizing and background color edits are reversible. FPS options are dynamic and include the source media rate when it falls outside the standard presets.
…space Wrap the transform node in a full-canvas mask container so clip-path and SVG masks are authored relative to the composition dimensions instead of the item's own bounding box. Fixes masks drifting with item position.
…e to stale mask refs reuseStableMaskInfos compared masks by shape.id (string) instead of shape reference, so maskType/maskFeather changes were silently dropped when the transform was unchanged. Switch to reference equality so any shape property update propagates immediately. Also: force clip masks to zero feather in the visual state hook, default new masks to feather 0, sync maskFeather when toggling mask type in the dropdown, and prefer the Player path during mask gizmo/vertex drags so hard-edge clips don't soften on effected items.
The fast-scrub renderer's maskFrameIndex captured ShapeItem references at creation time. When maskType/maskFeather changed in the store, the index still held stale shapes — re-renders after cache invalidation read old mask data, showing feathered masks during scrub even after switching to clip mode. Pass getLiveMaskItem into getActiveMasksForFrame so each frame render resolves the current store snapshot, matching the existing getCurrentItem pattern used for non-mask items.
On mousedown on a mask shape in the preview, the fast-scrub canvas overlay was activating for non-GPU-effect clips, switching from DOM Player rendering (browser video seek, ±1 frame imprecise) to mediabunny (frame-accurate decode). The decoder mismatch caused a visible frame shift, most noticeable with soft-edge alpha masks where the blur amplifies edge-area content differences. Fix: detect when the active gizmo is on a mask shape (isMask item) and skip overlay activation for non-GPU clips. The DOM Player handles mask transform previews fine through React. GPU-effected clips already have the overlay active so no path switch occurs — forceFastScrubOverlay is never suppressed. Also removes the scrub-time toggle between rasterized and inline SVG mask rendering (isInteractivePreviewScrub guard) which caused a separate visual flash when scrub started. The rasterized mask path has an LRU cache so it's free during scrub.
…or non-GPU clips The overlay was activating on any gizmo interaction (video, mask, text) for clips without GPU effects. This switched from DOM Player (browser video seek, ±1 frame imprecise) to the canvas renderer (mediabunny, frame-accurate), producing a visible frame shift — especially noticeable at high resolutions (3840x1920) and with soft-edge masks. Remove the gizmo-triggered overlay branch entirely: non-GPU clips now stay on the DOM Player path for all gizmo interactions. GPU-effected clips already have forceFastScrubOverlay active, so no change there. Also switches the mask feather control from NumberInput to SliderInput to match the Opacity slider UX.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds project-media matching flow for imported videos, constrains clip masks to hard edges, restructures mask DOM/rasterization, introduces visual video-fade UI, centralizes project metadata commits with undo, extends preview/export mask/live-item plumbing, and numerous tests and utility modules to support these flows. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TimelineUI as Timeline UI
participant DropHandler as Drop Handler
participant Preflight as preflightFirstTimelineVideoProjectMatch
participant MediaProcessor as MediaProcessorService
participant Dialog as ProjectMediaMatchDialog
participant ProjectStore as Project Store
participant TimelineStore as Timeline Store
User->>TimelineUI: drop video file
TimelineUI->>DropHandler: handleDrop(file)
DropHandler->>Preflight: preflightFirstTimelineVideoProjectMatch(entries)
Preflight->>ProjectStore: read current project & mediaItems
alt no existing project video
Preflight->>MediaProcessor: processMedia(file, mimeType, generateThumbnail=false)
MediaProcessor-->>Preflight: metadata {type:'video', width, height, fps}
Preflight->>Dialog: requestProjectMediaMatch(projectId, candidate)
Dialog-->>User: show dialog (match options)
User->>Dialog: choose option
Dialog->>ProjectStore: commitProjectMetadataChange(update)
Dialog-->>Preflight: resolves choice
Preflight-->>DropHandler: ok to proceed
else has existing video
Preflight-->>DropHandler: skip matching
end
DropHandler->>TimelineStore: addItem(placedItem)
TimelineStore-->>User: item placed with computed sizing
sequenceDiagram
participant Editor as Editor UI
participant ClipPanel as Clip Properties
participant ShapeModel as Shape Data
participant CompositionRuntime as Composition Runtime
participant Renderer as Rendering Engine
Editor->>ClipPanel: toggle "Use as Mask" / change mask type
ClipPanel->>ShapeModel: set maskType, set maskFeather (0 for clip, computed for alpha)
ShapeModel->>CompositionRuntime: provide mask info (shape, maskType, maskFeather)
CompositionRuntime->>CompositionRuntime: use-item-visual-state resolves feather:
alt maskType === 'alpha'
CompositionRuntime->>Renderer: apply feather = (shape.maskFeather * uniformScale)
else
CompositionRuntime->>Renderer: force feather = 0 (hard edge)
end
Renderer-->>Editor: display masked item (SVG mask or CSS clip-path as chosen)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+191
to
+195
| resolveProjectMediaMatch(choice); | ||
| } catch (error) { | ||
| toast.error('Failed to update project settings', { | ||
| description: error instanceof Error ? error.message : 'Please try again.', | ||
| }); |
There was a problem hiding this comment.
The promise returned by requestProjectMediaMatch never resolves if commitProjectMetadataChange throws an error. This will cause any caller awaiting the match choice to hang indefinitely.
catch (error) {
toast.error('Failed to update project settings', {
description: error instanceof Error ? error.message : 'Please try again.',
});
resolveProjectMediaMatch('keep-current'); // Add this line
}The resolveProjectMediaMatch call must be added to the catch block to ensure the promise is resolved even when the update fails.
Suggested change
| resolveProjectMediaMatch(choice); | |
| } catch (error) { | |
| toast.error('Failed to update project settings', { | |
| description: error instanceof Error ? error.message : 'Please try again.', | |
| }); | |
| resolveProjectMediaMatch(choice); | |
| } catch (error) { | |
| toast.error('Failed to update project settings', { | |
| description: error instanceof Error ? error.message : 'Please try again.', | |
| }); | |
| resolveProjectMediaMatch('keep-current'); |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a81618336f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+2333
to
+2337
| if (supportsVisualFadeControls(contentPreviewItem) && videoFadeEdit !== null) { | ||
| return { | ||
| ...contentPreviewItem, | ||
| fadeIn: videoFadeEdit.previewFadeIn, | ||
| fadeOut: videoFadeEdit.previewFadeOut, |
There was a problem hiding this comment.
This new preview override depends on videoFadeEdit being cleared after updateTimelineItem, but the custom memo comparator at the bottom of TimelineItem still does not compare item.fadeIn/item.fadeOut. When a fade handle commit only changes those fields, React can skip the prop update, leaving videoFadeEdit stuck and this branch continuing to override the item with stale preview fades (e.g., after undo/redo or subsequent edits). Include fade props in the comparator so committed fade updates propagate.
Useful? React with 👍 / 👎.
Comment on lines
+369
to
+371
| const mediaState = useMediaLibraryStore.getState(); | ||
| const currentProjectId = mediaState.currentProjectId; | ||
| const hasExistingProjectVideo = mediaState.mediaItems.some((item) => item.mimeType.startsWith('video/')); |
There was a problem hiding this comment.
This first-video detection runs from mediaItems without checking whether the media library has finished loading. If a user drops a file immediately after opening a project that already contains videos, mediaItems can still be temporarily empty and the flow will incorrectly prompt to match project settings, risking unintended canvas/FPS changes. Gate this logic on a loaded media state before treating the project as having no existing video.
Useful? React with 👍 / 👎.
Comment on lines
+17
to
+20
| const hasExistingProjectVideo = useMediaLibraryStore | ||
| .getState() | ||
| .mediaItems | ||
| .some((item) => item.mimeType.startsWith('video/')); |
There was a problem hiding this comment.
The timeline drop preflight also infers "first video" from mediaItems alone. During initial project load, this can be empty even when the project already has videos, so a drop in that window can incorrectly trigger project-matching for non-first media. Add an isLoading check (or equivalent ready signal) before using this emptiness check to decide whether to prompt.
Useful? React with 👍 / 👎.
Contributor
Greptile SummaryThis PR addresses four distinct preview rendering regressions related to mask handling, gizmo interaction, and fast-scrub stability, while also landing two feature additions (visual fade opacity in the export path and canvas resize undo/redo). Bug fixes:
Structural changes:
Minor concerns (no merge blockers):
Confidence Score: 5/5Safe to merge — all findings are P2 style suggestions with no correctness blockers. The four core bug fixes are each implemented correctly and backed by new unit tests. The structural DOM change in item-visual-wrapper is intentional and architecturally sound. The three P2 comments (canvas pool leak on rare throw, extra wrapper div, blend-mode stacking context) are minor edge cases that do not affect the primary user paths described in the test plan. src/features/export/utils/canvas-item-renderer.ts (canvas pool release ordering) and src/features/composition-runtime/components/item-visual-wrapper.tsx (blend-mode + mask combination) Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as ShapeSection / CanvasPanel
participant Store as ItemsStore / ProjectStore
participant Snap as captureSnapshot()
participant Undo as TimelineCommandStore
participant Renderer as ClientRenderEngine (preview)
participant MaskIdx as MaskFrameIndex (cached)
participant LiveStore as useItemsStore (live)
Note over UI,Store: Mask type change (clip → alpha)
UI->>Store: updateShapeItems({ maskType, maskFeather })
Store-->>Renderer: shape ref changes (new object)
Renderer->>MaskIdx: getActiveMasksForFrame(..., getLiveMaskItem)
MaskIdx->>LiveStore: getLiveMaskItem(mask.id)
LiveStore-->>MaskIdx: current ShapeItem (fresh ref)
MaskIdx-->>Renderer: resolved masks with live maskType/maskFeather
Note over UI,Undo: Canvas resize (undo-aware)
UI->>Snap: captureSnapshot() → beforeSnapshot
UI->>Store: updateProject(id, { width, height }) [async]
Store-->>Store: ProjectStore updated
UI->>Undo: addUndoEntry(UPDATE_PROJECT_METADATA, beforeSnapshot)
Note over Undo,Store: Undo
Undo->>Snap: restoreSnapshot(beforeSnapshot)
Snap->>Store: useProjectStore.setState(old metadata)
Snap->>Store: updateProject(id, old metadata) [IndexedDB]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+169
to
+180
| const maskContainerStyle = useMemo((): React.CSSProperties => { | ||
| if (state.maskType === null) { | ||
| return {}; | ||
| } | ||
|
|
||
| return { | ||
| position: 'absolute', | ||
| left: 0, | ||
| top: 0, | ||
| width: '100%', | ||
| height: '100%', | ||
| ...maskStyle, |
Contributor
There was a problem hiding this comment.
When state.maskType === null, maskContainerStyle returns {}, so the outer <div style={{}}> is an empty, unstyled DOM node that wraps every item even when no mask is active. The original code had no such wrapper in the no-mask case.
For compositions with many items this adds measurable DOM overhead. Consider short-circuiting the wrapper when there is no mask:
if (state.maskType === null) {
return (
<>
{svgMaskDefs}
<div style={{ ...state.transformStyle, overflow: ..., mixBlendMode: ... }}>
{/* corner pin + inner divs */}
</div>
</>
);
}Or, if the unified structure is intentional for simplicity, the current approach is acceptable — just worth noting the extra DOM node per item.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/composition-runtime/components/item-visual-wrapper.tsx
Line: 169-180
Comment:
**Extra wrapper div rendered for every unmasked item**
When `state.maskType === null`, `maskContainerStyle` returns `{}`, so the outer `<div style={{}}>` is an empty, unstyled DOM node that wraps every item even when no mask is active. The original code had no such wrapper in the no-mask case.
For compositions with many items this adds measurable DOM overhead. Consider short-circuiting the wrapper when there is no mask:
```tsx
if (state.maskType === null) {
return (
<>
{svgMaskDefs}
<div style={{ ...state.transformStyle, overflow: ..., mixBlendMode: ... }}>
{/* corner pin + inner divs */}
</div>
</>
);
}
```
Or, if the unified structure is intentional for simplicity, the current approach is acceptable — just worth noting the extra DOM node per item.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+188
to
+203
|
|
||
| {/* Outer: Transform + Mask + Blend Mode */} | ||
| <div | ||
| style={{ | ||
| ...state.transformStyle, | ||
| ...maskStyle, | ||
| overflow: state.transform.cornerRadius > 0 && !cornerPinStyle ? 'hidden' : undefined, | ||
| mixBlendMode: item.blendMode && item.blendMode !== 'normal' | ||
| ? BLEND_MODE_CSS[item.blendMode] | ||
| : undefined, | ||
| }} | ||
| > | ||
| {/* Corner Pin wrapper (only when active) */} | ||
| {/* When corner pin is active, will-change + backfaceVisibility force Chrome | ||
| to composite through the CSS pipeline instead of video hardware overlay, | ||
| which would otherwise ignore the matrix3d transform. */} | ||
| {/* Masks are authored in composition space, so they must be applied on a | ||
| full-canvas wrapper instead of the item-sized transform node. */} | ||
| <div style={maskContainerStyle}> | ||
| <div | ||
| style={cornerPinStyle ? { | ||
| width: '100%', | ||
| height: '100%', | ||
| ...cornerPinStyle, | ||
| willChange: 'transform', | ||
| backfaceVisibility: 'hidden' as const, | ||
| overflow: state.transform.cornerRadius > 0 ? 'hidden' : undefined, | ||
| } : { | ||
| width: '100%', | ||
| height: '100%', | ||
| style={{ | ||
| ...state.transformStyle, | ||
| overflow: state.transform.cornerRadius > 0 && !cornerPinStyle ? 'hidden' : undefined, | ||
| mixBlendMode: item.blendMode && item.blendMode !== 'normal' | ||
| ? BLEND_MODE_CSS[item.blendMode] | ||
| : undefined, | ||
| }} | ||
| > | ||
| {/* Inner: Effects + Content */} | ||
| {/* Corner Pin wrapper (only when active) */} | ||
| {/* When corner pin is active, will-change + backfaceVisibility force Chrome | ||
| to composite through the CSS pipeline instead of video hardware overlay, |
Contributor
There was a problem hiding this comment.
Previously mixBlendMode lived on the same element as the clip-path / mask style. Now it is on the inner transform <div>, one level inside the full-canvas positioned wrapper that owns the clip-path.
clip-path on the outer div creates a new stacking context in all browsers. mix-blend-mode on a child of a stacking context blends against that context's background (transparent here) rather than the canvas content below. In practice the outer div has no background so the composited result should look identical, but it's worth verifying with a blend-mode + mask combination (e.g., multiply blend mode on a masked video item) to confirm no visual regression before merge.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/composition-runtime/components/item-visual-wrapper.tsx
Line: 188-203
Comment:
**mix-blend-mode moved from clip-path element to inner transform element**
Previously `mixBlendMode` lived on the same element as the `clip-path` / mask style. Now it is on the inner transform `<div>`, one level inside the full-canvas positioned wrapper that owns the `clip-path`.
`clip-path` on the outer div creates a new stacking context in all browsers. `mix-blend-mode` on a child of a stacking context blends against that context's background (transparent here) rather than the canvas content below. In practice the outer div has no background so the composited result should look identical, but it's worth verifying with a blend-mode + mask combination (e.g., `multiply` blend mode on a masked video item) to confirm no visual regression before merge.
How can I resolve this? If you propose a fix, please make it concise.
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/features/composition-runtime/utils/mask-info.test.ts (1)
54-68:⚠️ Potential issue | 🟡 MinorTests pass for the wrong reason — shape references differ before transform/trackOrder is checked.
Since
createMaskInfowithout an explicitshapecreates a new object each call,previous[0].shape !== next[0].shapedue to reference inequality. The tests pass, but not because of the transform or trackOrder change — they pass because the shape references already differ.To properly validate transform and trackOrder comparisons, pass the same shape reference:
🧪 Proposed fix to test the intended behavior
it('returns a new array when any mask transform changes', () => { - const previous = [createMaskInfo('mask-1')]; - const next = [createMaskInfo('mask-1', 0, { x: 42 })]; + const shape = { id: 'mask-1' } as MaskInfo['shape']; + const previous = [createMaskInfo('mask-1', 0, {}, shape)]; + const next = [createMaskInfo('mask-1', 0, { x: 42 }, shape)]; expect(reuseStableMaskInfos(previous, next)).toEqual(next); expect(reuseStableMaskInfos(previous, next)).not.toBe(previous); }); it('returns a new array when any mask track order changes', () => { - const previous = [createMaskInfo('mask-1', 0)]; - const next = [createMaskInfo('mask-1', 1)]; + const shape = { id: 'mask-1' } as MaskInfo['shape']; + const previous = [createMaskInfo('mask-1', 0, {}, shape)]; + const next = [createMaskInfo('mask-1', 1, {}, shape)]; expect(reuseStableMaskInfos(previous, next)).toEqual(next); expect(reuseStableMaskInfos(previous, next)).not.toBe(previous); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/composition-runtime/utils/mask-info.test.ts` around lines 54 - 68, The tests are passing due to differing shape object references rather than the intended transform/trackOrder differences; update the two failing specs to reuse the same shape object when calling createMaskInfo so only transform or trackOrder differs: create a shared shape constant and pass it as the shape argument to createMaskInfo in both previous and next for the 'returns a new array when any mask transform changes' and 'returns a new array when any mask track order changes' tests, then assert reuseStableMaskInfos(previous, next) equals next and is not the same reference as previous.
🧹 Nitpick comments (11)
src/features/export/utils/canvas-item-renderer.corner-pin.test.ts (1)
129-196: Consider adding assertion forglobalAlphaassignment.The test thoroughly verifies the flattening mechanics (acquire → draw corner-pin to flattened canvas → drawImage to main ctx → release), but doesn't assert that
ctx.globalAlphawas set to0.35beforedrawImage. This would catch regressions where flattening happens but opacity isn't applied.💡 Optional: Add globalAlpha verification
await renderItem(ctx, item, transform, 0, rctx); + // Verify opacity was applied before drawing flattened result + expect(ctx.globalAlpha).toBe(0.35); expect(canvasPool.acquire).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/export/utils/canvas-item-renderer.corner-pin.test.ts` around lines 129 - 196, Add an assertion that the canvas context globalAlpha is set to the transform opacity before drawImage: after calling await renderItem(...) and before checking ctx.drawImage, assert ctx.globalAlpha (or mocked ctx.globalAlpha value) equals 0.35 (use toBe or toBeCloseTo for floating precision). Locate this in the test 'flattens faded corner pin output before applying opacity' in canvas-item-renderer.corner-pin.test.ts around the existing expectations for canvasPool.acquire, mockFns.drawCornerPinImageMock, and ctx.drawImage so it fails if renderItem (and its call sites) do not apply transform.opacity to ctx.globalAlpha.src/features/timeline/components/timeline-content.test.tsx (1)
74-97: Use fresh fixture objects per test to avoid hidden cross-test state leaks.Line 74 and Line 87 define module-level objects reused by
resetStores(). If any code mutates these objects, later tests may inherit mutated state and become flaky. Prefer creating freshTimelineTrack/VideoIteminstances insideresetStores()(or via factory helpers).Also applies to: 117-177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/components/timeline-content.test.tsx` around lines 74 - 97, The module-level fixtures VIDEO_TRACK and VIDEO_ITEM are reused across tests causing possible cross-test state leaks; change resetStores() (or create a factory helper used by resetStores()) to construct fresh TimelineTrack and VideoItem objects each time instead of referencing the module constants—replace uses of VIDEO_TRACK and VIDEO_ITEM in resetStores() with newly created objects (or call the factory) and update any test setup that mutates track/items to use these fresh instances; apply the same change for other fixtures in the 117-177 range to ensure tests receive clean copies.src/features/timeline/hooks/use-timeline-tracks.ts (1)
4-4: Use path alias for the newgetTrackKindimport.Please switch this import to the
@/*alias form to match repo conventions.As per coding guidelines, "Use path alias
@/*to import fromsrc/*".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/hooks/use-timeline-tracks.ts` at line 4, Replace the relative import of getTrackKind with the project path-alias form: update the import in use-timeline-tracks.ts that currently references "../utils/classic-tracks" to use the "@/..." alias (importing getTrackKind from the corresponding src path via the `@/` alias) so it follows the repo convention for imports.src/features/timeline/components/track-header.tsx (1)
15-15: Use@/*alias for the newgetTrackKindimport.Please switch this relative import to the src alias path.
As per coding guidelines, "Use path alias
@/*to import fromsrc/*".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/components/track-header.tsx` at line 15, Replace the relative import for getTrackKind with the project src alias: change the import of getTrackKind (used in track-header.tsx) from the relative path '../utils/classic-tracks' to the alias path '@/features/timeline/utils/classic-tracks' so it follows the "Use path alias `@/`* for src/*" guideline.src/features/timeline/components/track-header.test.tsx (1)
73-79: Consider asserting click behavior for the audio case too.Adding a click +
toHaveBeenCalledTimes(1)check here would keep parity with the video-path regression coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/components/track-header.test.tsx` around lines 73 - 79, The test currently asserts presence/absence of buttons for a muted audio track but doesn't verify the click behavior; update the test using the same click/assert pattern used in the video-path test: render the header with renderTrackHeader(makeTrack({ id: 'track-2', name: 'A1', kind: 'audio', muted: true })) while supplying the same mock handler used elsewhere (e.g., the onEnable/onToggle mock passed into renderTrackHeader), simulate a click on screen.getByRole('button', { name: 'Enable track' }) (using userEvent or fireEvent), and add an assertion expect(mockHandler).toHaveBeenCalledTimes(1) to confirm the enable action is invoked once.src/features/export/utils/canvas-keyframes.ts (1)
27-42: Consider guarding against frames outside the item's bounds.The function computes
relativeFrame = frame - item.frombut doesn't explicitly check ifframeis within[item.from, item.from + durationInFrames). WhileinterpolateLinearusesclamp01which handles out-of-range progress values gracefully, the semantic intent may be clearer with an explicit guard:
- If
frame < item.from: opacity should be 0 (clip not yet visible)- If
frame >= item.from + durationInFrames: opacity should be 0 (clip ended)The current implementation will return values based on extrapolated progress, which
clamp01will clamp to 0 or 1. This likely works correctly in practice since the export renderer only calls this for visible items, but explicit bounds checking would make the contract clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/export/utils/canvas-keyframes.ts` around lines 27 - 42, The getVisualFadeOpacity function should explicitly guard frames outside the item's bounds: before computing relativeFrame (and before using fadeInFrames/fadeOutFrames), check if frame < item.from or frame >= item.from + item.durationInFrames and return 0 in those cases; keep the existing fade logic (using fadeInFrames, fadeOutFrames, fadeOutStart, and interpolateLinear) for in-range frames so semantics are clear and not left to clamping inside interpolateLinear.src/features/preview/hooks/use-gpu-effects-overlay.ts (1)
4-4: Use@/*alias foruseGizmoStoreimport.Line 4 should use the project alias import style instead of a relative path for consistency with repo standards.
♻️ Proposed change
-import { useGizmoStore } from '../stores/gizmo-store'; +import { useGizmoStore } from '@/features/preview/stores/gizmo-store';As per coding guidelines, "
Use path alias@/* to import from src/*".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/preview/hooks/use-gpu-effects-overlay.ts` at line 4, Replace the relative import of useGizmoStore with the project path-alias style: locate the import statement that pulls in useGizmoStore (symbol: useGizmoStore) and change it from a relative path (../stores/gizmo-store) to the `@/`* alias form so it imports from the matching src/* module via the @ prefix (i.e., use the '@/...' alias instead of the relative path).src/features/preview/components/video-preview.sync.test.tsx (1)
849-1260: Consider extracting shared GPU clip/track fixtures to test helpers.The new cases are valuable, but setup duplication is high; a small helper factory would make future edits safer and shorter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/preview/components/video-preview.sync.test.tsx` around lines 849 - 1260, Many tests duplicate GPU clip/track setup; extract small test helpers to reduce repetition and make edits safer. Create helpers like createTrackFixture (returns the track object used with useItemsStore.getState().setTracks), createVideoItem({id, src, from, durationInFrames, effects?}) to build timeline items, and createGpuEffect({id, gpuEffectType, params}) to build effect objects; also add renderVideoPreview(props) to wrap the common render(<VideoPreview ... />) call. Replace repeated calls to useItemsStore.getState().setTracks, useItemsStore.getState().setItems, act(() => usePlaybackStore.getState().setCurrentFrame(...)), useGizmoStore.getState().setEffectsPreviewNew, and repeated container/canvas queries with these helpers (refer to createCompositionRendererMock, rendererMockState, and scrubCanvas usages to keep tests behavior unchanged).src/features/timeline/stores/commands/labels.test.ts (1)
32-39: Expand tests for remaining metadata-label branches.
This case validates only the resize path. Please add cases forfps,backgroundColor, and unknown-field fallback to protect the new branching logic from regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/stores/commands/labels.test.ts` around lines 32 - 39, Add unit tests in labels.test.ts for the other branches of formatTimelineCommandLabel: add a case where payload.fields includes 'fps' and assert the label equals 'Change frame rate'; add a case where fields includes 'backgroundColor' and assert the label equals 'Change background color'; and add a case with an unknown field (e.g., 'foo') or an empty fields array to assert the fallback label (e.g., 'Update project metadata' or whatever the function returns for unknown fields). Locate tests around the existing UPDATE_PROJECT_METADATA test and use formatTimelineCommandLabel as the target function to exercise each branch.src/features/projects/utils/validation.ts (1)
155-157: Theas constassertion has no effect when spreading a variable.The
as conston line 157 only affects array mutability, not the inferred element types, sinceDEFAULT_PROJECT_FPS_OPTIONSis already typed. If the goal is to ensureFPS_PRESETSis a readonly tuple with literal types,DEFAULT_PROJECT_FPS_OPTIONSitself must be declaredas const.♻️ Suggested cleanup
If
DEFAULT_PROJECT_FPS_OPTIONSis alreadyreadonlyoras const, you can simplify:export const FPS_PRESETS = [ ...DEFAULT_PROJECT_FPS_OPTIONS, -] as const; +];Or, if you need a direct readonly reference:
-export const FPS_PRESETS = [ - ...DEFAULT_PROJECT_FPS_OPTIONS, -] as const; +export const FPS_PRESETS = DEFAULT_PROJECT_FPS_OPTIONS;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/projects/utils/validation.ts` around lines 155 - 157, The `as const` on FPS_PRESETS has no effect because you spread DEFAULT_PROJECT_FPS_OPTIONS; either remove the redundant `as const` from the FPS_PRESETS declaration or make DEFAULT_PROJECT_FPS_OPTIONS itself a readonly literal (declare DEFAULT_PROJECT_FPS_OPTIONS `as const`) so that FPS_PRESETS becomes a readonly tuple with literal types; update the declaration for DEFAULT_PROJECT_FPS_OPTIONS or simplify FPS_PRESETS accordingly (refer to DEFAULT_PROJECT_FPS_OPTIONS and FPS_PRESETS).src/features/timeline/components/timeline-track.tsx (1)
43-43: Consider extracting shared drop logic.
timeline-track.tsxandtimeline-media-drop-zone.tsxnow share near-identical implementations for:
getCurrentCanvasSize()callbackresolveTimelineItemsForEntries()patternbuildTimelineTemplateItem()with canvas size- Preflight + error handling
This is acceptable for now, but consider extracting shared utilities in a future refactor to reduce duplication.
Also applies to: 202-288, 383-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/components/timeline-track.tsx` at line 43, There’s duplicated drop handling across timeline-track.tsx and timeline-media-drop-zone.tsx (notably getCurrentCanvasSize, resolveTimelineItemsForEntries, buildTimelineTemplateItem and the preflight + error handling using preflightFirstTimelineVideoProjectMatch); extract these into a shared utility module (e.g., timeline-drop-utils) that exports getCurrentCanvasSize, resolveTimelineItemsForEntries, buildTimelineTemplateItem and a preflight wrapper that calls preflightFirstTimelineVideoProjectMatch and centralizes error handling, then replace the inline implementations in both timeline-track.tsx and timeline-media-drop-zone.tsx to import and use those shared helpers (ensure function signatures and any callbacks/events used by the components remain consistent).
🤖 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/features/editor/components/project-media-match-dialog.tsx`:
- Around line 83-94: The code currently picks the newest clip by sorting
newVideos descending; change selection to the earliest imported video so the
dialog uses the first-added clip. Locate the selection logic around firstVideo
(where newVideos is sorted and requestProjectMediaMatch called) and either sort
ascending by createdAt or find the minimal createdAt (e.g., use .sort((a,b) =>
a.createdAt - b.createdAt)[0] or a reduce to pick the oldest), then pass that
oldest video's fileName/width/height/fps into requestProjectMediaMatch; keep
awaitingAutoPromptRef.current and the surrounding call/finally intact.
In `@src/features/preview/components/video-preview.tsx`:
- Around line 3313-3323: The paused-transition overlay logic currently checks
getTransitionWindowForFrame() which includes post-transition cooldown and causes
overlays and live-preview invalidators (unsubscribeCornerPin,
unsubscribeMaskEditor) to mis-handle non-active frames; define a new predicate
isPausedTransitionOverlayActive(frame, playbackState) that returns true only
when a transition window exists and the frame is within the active span
(startFrame <= frame < endFrame) and previewFrame === null and
forceFastScrubOverlay === false, then replace the existing uses (the
isPausedOnTransitionFrame expression and the branches around
shouldShowPlaybackTransitionOverlay/forceFastScrubOverlay/shouldShowFastScrubOverlay
and the checks at the unsubscribeCornerPin and unsubscribeMaskEditor early
returns) to use isPausedTransitionOverlayActive so the overlay and unsubscribes
consistently reflect only active transition frames.
In `@src/features/preview/hooks/use-canvas-media-drop.ts`:
- Around line 375-403: The code currently calls
useProjectMediaMatchDialogStore.getState().requestProjectMediaMatch(...) (via
requestProjectMediaMatch) before importHandlesForPlacement runs, which can
mutate project width/height/fps even if import fails; to fix, defer calling
requestProjectMediaMatch until after importHandlesForPlacement completes
successfully (or alternatively perform the match and only commit the project
mutation after importHandlesForPlacement returns), i.e., move the
requestProjectMediaMatch/shouldPreserveInitialPlacement/getMatchedProjectSize
logic so it runs after importHandlesForPlacement succeeds and use the existing
preserveInitialPlacement/placementProjectSize variables thereafter.
In `@src/features/projects/utils/project-fps.ts`:
- Around line 24-27: AUTO_MATCH_PROJECT_FPS_VALUES currently builds from
DEFAULT_PROJECT_FPS_OPTIONS and explicitly includes 120, so 240 (present in
LEGACY_PROJECT_FPS_OPTIONS) is never considered by resolveAutoMatchProjectFps;
update the auto-match list generation to include all candidate fps values that
should be auto-matched (e.g., merge values from LEGACY_PROJECT_FPS_OPTIONS or
explicitly add 240) so resolveAutoMatchProjectFps can return 240 for first-clip
matches; modify the constant AUTO_MATCH_PROJECT_FPS_VALUES (or its construction)
to include the missing 240 and ensure DEFAULT_PROJECT_FPS_OPTIONS,
LEGACY_PROJECT_FPS_OPTIONS and resolveAutoMatchProjectFps remain consistent.
In `@src/features/timeline/components/timeline-item/index.tsx`:
- Around line 1657-1662: The memo equality function used for TimelineItem (the
custom prop comparator passed to React.memo near the block comparing prevProps
and nextProps) is missing item.fadeIn and item.fadeOut, so updates to
displayedVideoFadeIn/displayedVideoFadeOut can be skipped; update that
comparator (the same whitelist logic used by
StableVideoSequence/areGroupPropsEqual) to include comparisons for
prevProps.item.fadeIn !== nextProps.item.fadeIn and prevProps.item.fadeOut !==
nextProps.item.fadeOut (also add these checks in the other similar comparator
around the 1684-1716 range) so committed fade updates correctly trigger
re-renders of TimelineItem, overlay, and handles.
- Around line 1863-1870: handleVideoFadeHandleMouseDown starts edit mode for any
mouse button; add an early check to ignore non-primary presses by returning
immediately when the mouse event's button is not the primary button (e.g. if
(e.button !== 0) return;). Update the handler (handleVideoFadeHandleMouseDown)
to perform this check before calling preventDefault/stopPropagation or starting
the drag/edit logic so right-clicks or other buttons don't enter fade-edit mode
for AudioFadeHandle.
In `@src/features/timeline/components/timeline-item/video-fade-handles.tsx`:
- Around line 40-56: The pointer-only fade handles are still keyboard-focusable;
update the handle elements produced using getHandleClassName /
handleVisibilityClass so they are removed from the tab order and not exposed to
assistive tech: add tabIndex={-1} and aria-hidden="true" (or conditionally set
tabIndex to -1 whenever isEditing/isSelected is false and the handle is hidden)
on the handle elements, and remove the ineffective focus styling
(focus-visible:outline-none) if you later reintroduce keyboard interactions;
ensure any required keyboard accessibility is implemented elsewhere (e.g.,
separate keyboard-accessible controls) rather than leaving these pointer-only
elements focusable.
In `@src/features/timeline/stores/commands/labels.ts`:
- Around line 62-68: When generating the history label, handle the combined
update before single-field checks: add a conditional that checks if
fields.includes('fps') AND (fields.includes('width') ||
fields.includes('height')) and return a combined label (e.g., "Resize canvas and
change frame rate") prior to the existing checks that return "Resize canvas" or
"Change frame rate"; update the code around the existing fields.includes checks
so the combined case is evaluated first and individual cases remain unchanged.
---
Outside diff comments:
In `@src/features/composition-runtime/utils/mask-info.test.ts`:
- Around line 54-68: The tests are passing due to differing shape object
references rather than the intended transform/trackOrder differences; update the
two failing specs to reuse the same shape object when calling createMaskInfo so
only transform or trackOrder differs: create a shared shape constant and pass it
as the shape argument to createMaskInfo in both previous and next for the
'returns a new array when any mask transform changes' and 'returns a new array
when any mask track order changes' tests, then assert
reuseStableMaskInfos(previous, next) equals next and is not the same reference
as previous.
---
Nitpick comments:
In `@src/features/export/utils/canvas-item-renderer.corner-pin.test.ts`:
- Around line 129-196: Add an assertion that the canvas context globalAlpha is
set to the transform opacity before drawImage: after calling await
renderItem(...) and before checking ctx.drawImage, assert ctx.globalAlpha (or
mocked ctx.globalAlpha value) equals 0.35 (use toBe or toBeCloseTo for floating
precision). Locate this in the test 'flattens faded corner pin output before
applying opacity' in canvas-item-renderer.corner-pin.test.ts around the existing
expectations for canvasPool.acquire, mockFns.drawCornerPinImageMock, and
ctx.drawImage so it fails if renderItem (and its call sites) do not apply
transform.opacity to ctx.globalAlpha.
In `@src/features/export/utils/canvas-keyframes.ts`:
- Around line 27-42: The getVisualFadeOpacity function should explicitly guard
frames outside the item's bounds: before computing relativeFrame (and before
using fadeInFrames/fadeOutFrames), check if frame < item.from or frame >=
item.from + item.durationInFrames and return 0 in those cases; keep the existing
fade logic (using fadeInFrames, fadeOutFrames, fadeOutStart, and
interpolateLinear) for in-range frames so semantics are clear and not left to
clamping inside interpolateLinear.
In `@src/features/preview/components/video-preview.sync.test.tsx`:
- Around line 849-1260: Many tests duplicate GPU clip/track setup; extract small
test helpers to reduce repetition and make edits safer. Create helpers like
createTrackFixture (returns the track object used with
useItemsStore.getState().setTracks), createVideoItem({id, src, from,
durationInFrames, effects?}) to build timeline items, and createGpuEffect({id,
gpuEffectType, params}) to build effect objects; also add
renderVideoPreview(props) to wrap the common render(<VideoPreview ... />) call.
Replace repeated calls to useItemsStore.getState().setTracks,
useItemsStore.getState().setItems, act(() =>
usePlaybackStore.getState().setCurrentFrame(...)),
useGizmoStore.getState().setEffectsPreviewNew, and repeated container/canvas
queries with these helpers (refer to createCompositionRendererMock,
rendererMockState, and scrubCanvas usages to keep tests behavior unchanged).
In `@src/features/preview/hooks/use-gpu-effects-overlay.ts`:
- Line 4: Replace the relative import of useGizmoStore with the project
path-alias style: locate the import statement that pulls in useGizmoStore
(symbol: useGizmoStore) and change it from a relative path
(../stores/gizmo-store) to the `@/`* alias form so it imports from the matching
src/* module via the @ prefix (i.e., use the '@/...' alias instead of the
relative path).
In `@src/features/projects/utils/validation.ts`:
- Around line 155-157: The `as const` on FPS_PRESETS has no effect because you
spread DEFAULT_PROJECT_FPS_OPTIONS; either remove the redundant `as const` from
the FPS_PRESETS declaration or make DEFAULT_PROJECT_FPS_OPTIONS itself a
readonly literal (declare DEFAULT_PROJECT_FPS_OPTIONS `as const`) so that
FPS_PRESETS becomes a readonly tuple with literal types; update the declaration
for DEFAULT_PROJECT_FPS_OPTIONS or simplify FPS_PRESETS accordingly (refer to
DEFAULT_PROJECT_FPS_OPTIONS and FPS_PRESETS).
In `@src/features/timeline/components/timeline-content.test.tsx`:
- Around line 74-97: The module-level fixtures VIDEO_TRACK and VIDEO_ITEM are
reused across tests causing possible cross-test state leaks; change
resetStores() (or create a factory helper used by resetStores()) to construct
fresh TimelineTrack and VideoItem objects each time instead of referencing the
module constants—replace uses of VIDEO_TRACK and VIDEO_ITEM in resetStores()
with newly created objects (or call the factory) and update any test setup that
mutates track/items to use these fresh instances; apply the same change for
other fixtures in the 117-177 range to ensure tests receive clean copies.
In `@src/features/timeline/components/timeline-track.tsx`:
- Line 43: There’s duplicated drop handling across timeline-track.tsx and
timeline-media-drop-zone.tsx (notably getCurrentCanvasSize,
resolveTimelineItemsForEntries, buildTimelineTemplateItem and the preflight +
error handling using preflightFirstTimelineVideoProjectMatch); extract these
into a shared utility module (e.g., timeline-drop-utils) that exports
getCurrentCanvasSize, resolveTimelineItemsForEntries, buildTimelineTemplateItem
and a preflight wrapper that calls preflightFirstTimelineVideoProjectMatch and
centralizes error handling, then replace the inline implementations in both
timeline-track.tsx and timeline-media-drop-zone.tsx to import and use those
shared helpers (ensure function signatures and any callbacks/events used by the
components remain consistent).
In `@src/features/timeline/components/track-header.test.tsx`:
- Around line 73-79: The test currently asserts presence/absence of buttons for
a muted audio track but doesn't verify the click behavior; update the test using
the same click/assert pattern used in the video-path test: render the header
with renderTrackHeader(makeTrack({ id: 'track-2', name: 'A1', kind: 'audio',
muted: true })) while supplying the same mock handler used elsewhere (e.g., the
onEnable/onToggle mock passed into renderTrackHeader), simulate a click on
screen.getByRole('button', { name: 'Enable track' }) (using userEvent or
fireEvent), and add an assertion expect(mockHandler).toHaveBeenCalledTimes(1) to
confirm the enable action is invoked once.
In `@src/features/timeline/components/track-header.tsx`:
- Line 15: Replace the relative import for getTrackKind with the project src
alias: change the import of getTrackKind (used in track-header.tsx) from the
relative path '../utils/classic-tracks' to the alias path
'@/features/timeline/utils/classic-tracks' so it follows the "Use path alias `@/`*
for src/*" guideline.
In `@src/features/timeline/hooks/use-timeline-tracks.ts`:
- Line 4: Replace the relative import of getTrackKind with the project
path-alias form: update the import in use-timeline-tracks.ts that currently
references "../utils/classic-tracks" to use the "@/..." alias (importing
getTrackKind from the corresponding src path via the `@/` alias) so it follows the
repo convention for imports.
In `@src/features/timeline/stores/commands/labels.test.ts`:
- Around line 32-39: Add unit tests in labels.test.ts for the other branches of
formatTimelineCommandLabel: add a case where payload.fields includes 'fps' and
assert the label equals 'Change frame rate'; add a case where fields includes
'backgroundColor' and assert the label equals 'Change background color'; and add
a case with an unknown field (e.g., 'foo') or an empty fields array to assert
the fallback label (e.g., 'Update project metadata' or whatever the function
returns for unknown fields). Locate tests around the existing
UPDATE_PROJECT_METADATA test and use formatTimelineCommandLabel as the target
function to exercise each branch.
🪄 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
Run ID: 3c2482e4-84aa-4c90-b963-702cdd327cb8
📒 Files selected for processing (60)
src/features/composition-runtime/components/composition-content.masks.test.tsxsrc/features/composition-runtime/components/hooks/use-item-visual-state.tssrc/features/composition-runtime/components/item-visual-wrapper.tsxsrc/features/composition-runtime/utils/mask-info.test.tssrc/features/composition-runtime/utils/mask-info.tssrc/features/editor/components/editor.test.tsxsrc/features/editor/components/editor.tsxsrc/features/editor/components/project-media-match-dialog.test.tsxsrc/features/editor/components/project-media-match-dialog.tsxsrc/features/editor/components/properties-sidebar/canvas-panel/index.tsxsrc/features/editor/components/properties-sidebar/clip-panel/shape-section.tsxsrc/features/editor/deps/projects-contract.tssrc/features/editor/deps/timeline-store.tssrc/features/editor/utils/project-media-match.tssrc/features/editor/utils/project-metadata-history.tssrc/features/export/utils/canvas-item-renderer.corner-pin.test.tssrc/features/export/utils/canvas-item-renderer.tssrc/features/export/utils/canvas-keyframes.test.tssrc/features/export/utils/canvas-keyframes.tssrc/features/export/utils/canvas-masks.tssrc/features/export/utils/client-render-engine.tssrc/features/media-library/contracts/timeline.tssrc/features/media-library/services/media-processor-service.tssrc/features/media-library/workers/media-processor.worker.tssrc/features/preview/components/video-preview.sync.test.tsxsrc/features/preview/components/video-preview.tsxsrc/features/preview/deps/media-library-contract.tssrc/features/preview/hooks/use-canvas-media-drop.test.tsxsrc/features/preview/hooks/use-canvas-media-drop.tssrc/features/preview/hooks/use-gpu-effects-overlay.test.tssrc/features/preview/hooks/use-gpu-effects-overlay.tssrc/features/projects/components/project-form.tsxsrc/features/projects/utils/project-fps.test.tssrc/features/projects/utils/project-fps.tssrc/features/projects/utils/validation.tssrc/features/timeline/components/timeline-content.test.tsxsrc/features/timeline/components/timeline-content.tsxsrc/features/timeline/components/timeline-item/index.tsxsrc/features/timeline/components/timeline-item/video-fade-handles.test.tsxsrc/features/timeline/components/timeline-item/video-fade-handles.tsxsrc/features/timeline/components/timeline-item/visual-fade-items.test.tssrc/features/timeline/components/timeline-item/visual-fade-items.tssrc/features/timeline/components/timeline-media-drop-zone.tsxsrc/features/timeline/components/timeline-track.tsxsrc/features/timeline/components/timeline.tsxsrc/features/timeline/components/track-header.test.tsxsrc/features/timeline/components/track-header.tsxsrc/features/timeline/contracts/editor.tssrc/features/timeline/deps/media-library-resolver.tssrc/features/timeline/hooks/use-timeline-tracks.tssrc/features/timeline/services/filmstrip-cache.tssrc/features/timeline/stores/commands/labels.test.tssrc/features/timeline/stores/commands/labels.tssrc/features/timeline/stores/commands/snapshot.tssrc/features/timeline/stores/commands/types.tssrc/features/timeline/stores/timeline-store-facade.test.tssrc/features/timeline/utils/external-file-project-match.test.tssrc/features/timeline/utils/external-file-project-match.tssrc/shared/state/project-media-match-dialog/index.tssrc/shared/state/project-media-match-dialog/store.ts
💤 Files with no reviewable changes (1)
- src/features/timeline/components/timeline-content.tsx
Comment on lines
+375
to
+403
| if (entry.mediaType === 'video' && currentProjectId && !hasExistingProjectVideo) { | ||
| try { | ||
| const mimeType = getMimeType(entry.file); | ||
| const { metadata } = await mediaProcessorService.processMedia(entry.file, mimeType, { | ||
| generateThumbnail: false, | ||
| }); | ||
|
|
||
| if (metadata.type !== 'video') { | ||
| toast.error('Unable to inspect dropped video.'); | ||
| return; | ||
| } | ||
|
|
||
| const matchChoice = await useProjectMediaMatchDialogStore.getState().requestProjectMediaMatch(currentProjectId, { | ||
| fileName: entry.file.name, | ||
| width: metadata.width, | ||
| height: metadata.height, | ||
| fps: metadata.fps, | ||
| }); | ||
|
|
||
| preserveInitialPlacement = shouldPreserveInitialPlacement(matchChoice); | ||
| if (preserveInitialPlacement) { | ||
| placementProjectSize = getMatchedProjectSize(metadata) ?? undefined; | ||
| } | ||
| } catch (error) { | ||
| toast.error('Unable to inspect dropped file.', { | ||
| description: error instanceof Error ? error.message : 'Please try again.', | ||
| }); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Defer project matching until the import succeeds.
requestProjectMediaMatch() can commit width/height/fps changes before importHandlesForPlacement() runs. If the import then fails on Lines 410-413, the project stays mutated even though the dropped clip never made it into the library or timeline.
Also applies to: 406-413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/preview/hooks/use-canvas-media-drop.ts` around lines 375 - 403,
The code currently calls
useProjectMediaMatchDialogStore.getState().requestProjectMediaMatch(...) (via
requestProjectMediaMatch) before importHandlesForPlacement runs, which can
mutate project width/height/fps even if import fails; to fix, defer calling
requestProjectMediaMatch until after importHandlesForPlacement completes
successfully (or alternatively perform the match and only commit the project
mutation after importHandlesForPlacement returns), i.e., move the
requestProjectMediaMatch/shouldPreserveInitialPlacement/getMatchedProjectSize
logic so it runs after importHandlesForPlacement succeeds and use the existing
preserveInitialPlacement/placementProjectSize variables thereafter.
Comment on lines
+24
to
+27
| const AUTO_MATCH_PROJECT_FPS_VALUES = [ | ||
| ...DEFAULT_PROJECT_FPS_OPTIONS.map((option) => option.value), | ||
| 120, | ||
| ] as const; |
Contributor
There was a problem hiding this comment.
240 fps can never be auto-matched with the current candidate list.
LEGACY_PROJECT_FPS_OPTIONS exposes 240 as a valid project FPS, but resolveAutoMatchProjectFps() only searches AUTO_MATCH_PROJECT_FPS_VALUES, which omits it. A 240 fps first clip will therefore auto-suggest 120 fps and update the project incorrectly.
Possible fix
const AUTO_MATCH_PROJECT_FPS_VALUES = [
...DEFAULT_PROJECT_FPS_OPTIONS.map((option) => option.value),
120,
+ 240,
] as const;Also applies to: 60-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/projects/utils/project-fps.ts` around lines 24 - 27,
AUTO_MATCH_PROJECT_FPS_VALUES currently builds from DEFAULT_PROJECT_FPS_OPTIONS
and explicitly includes 120, so 240 (present in LEGACY_PROJECT_FPS_OPTIONS) is
never considered by resolveAutoMatchProjectFps; update the auto-match list
generation to include all candidate fps values that should be auto-matched
(e.g., merge values from LEGACY_PROJECT_FPS_OPTIONS or explicitly add 240) so
resolveAutoMatchProjectFps can return 240 for first-clip matches; modify the
constant AUTO_MATCH_PROJECT_FPS_VALUES (or its construction) to include the
missing 240 and ensure DEFAULT_PROJECT_FPS_OPTIONS, LEGACY_PROJECT_FPS_OPTIONS
and resolveAutoMatchProjectFps remain consistent.
Comment on lines
+1657
to
+1662
| const displayedVideoFadeIn = isVisualFadeItem | ||
| ? (videoFadeEdit?.previewFadeIn ?? item.fadeIn ?? 0) | ||
| : 0; | ||
| const displayedVideoFadeOut = isVisualFadeItem | ||
| ? (videoFadeEdit?.previewFadeOut ?? item.fadeOut ?? 0) | ||
| : 0; |
Contributor
There was a problem hiding this comment.
Include fadeIn and fadeOut in the memo prop whitelist.
These derived values now render directly from item.fadeIn / item.fadeOut, but the custom equality check near Lines 3109-3134 still ignores both fields. A committed fade update can therefore be skipped, leaving the overlay and handles stale until some unrelated prop changes. Please add both comparisons there.
Based on learnings: StableVideoSequence's areGroupPropsEqual in stable-video-sequence.tsx whitelists item properties for React.memo comparison. When adding new visual properties to TimelineItem, add them to this comparison — missing properties cause stale renders during playback.
Also applies to: 1684-1716
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/features/timeline/components/timeline-item/index.tsx` around lines 1657 -
1662, The memo equality function used for TimelineItem (the custom prop
comparator passed to React.memo near the block comparing prevProps and
nextProps) is missing item.fadeIn and item.fadeOut, so updates to
displayedVideoFadeIn/displayedVideoFadeOut can be skipped; update that
comparator (the same whitelist logic used by
StableVideoSequence/areGroupPropsEqual) to include comparisons for
prevProps.item.fadeIn !== nextProps.item.fadeIn and prevProps.item.fadeOut !==
nextProps.item.fadeOut (also add these checks in the other similar comparator
around the 1684-1716 range) so committed fade updates correctly trigger
re-renders of TimelineItem, overlay, and handles.
Code fixes:
- Fix canvas pool leak in corner-pin renderer (acquire inside try/finally)
- Skip extra wrapper div for unmasked items in ItemVisualWrapper
- Move mixBlendMode to mask container div (correct stacking context)
- Pick earliest imported video for project match, not newest
- Defer requestProjectMediaMatch until import succeeds
- Remove 120/240 from auto-match FPS (snap to 60 instead)
- Add combined "Resize canvas and change frame rate" history label
- Use isPausedTransitionOverlayActive predicate (no cooldown bleed)
- Add fadeIn/fadeOut to TimelineItem memo comparator
- Ignore non-primary mouse button on fade handle mousedown
- Add tabIndex={-1} and aria-hidden to pointer-only fade handles
- Fix mask-info tests to use shared shape refs
Nitpicks:
- Add globalAlpha assertion in corner-pin test
- Add out-of-bounds guard in getVisualFadeOpacity
- Replace relative imports with @/ aliases (3 files)
- Remove redundant `as const` on FPS_PRESETS
- Add test coverage for label branches and track header click
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.
Summary
reuseStableMaskInfosused string ID comparison instead of reference equality, returning stale mask data whenmaskType/maskFeatherchangedmaskFrameIndexcached shape refs at creation time; addedgetLiveMaskItemto resolve current store snapshots during preview renderingisInteractivePreviewScrubtoggle that switched between rasterized PNG and inline SVG mask renderingTest plan
Summary by CodeRabbit
New Features
Improvements