Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughSplit and refactor large editor, dopesheet, mask-editor, media-library, timeline, and cache modules into focused helpers/services/hooks; introduce lazy-loaded editor dialogs; centralize media/file helpers and persistence; add memory-aware cache abstractions; add dopesheet/keyframe and mask-editor utilities and unit tests. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
Contributor
Greptile SummaryThis PR is a large-scale refactoring that extracts helpers from monolithic files into dedicated modules across timeline, media-library, and editor features. It also lazy-loads three additional editor dialogs ( Confidence Score: 5/5Safe to merge — this is a clean extract-refactor with faithful logic preservation and new unit tests for every extracted helper. All extracted functions are unit-tested and verified against the original inline implementations. The only findings are P2: a Suspense boundary scoping suggestion and a logger name nit. No behavioral regressions introduced. src/features/editor/components/editor.tsx — lazy dialogs share the existing Suspense boundary, which could cause a brief flicker on first load if an export dialog is concurrently visible.
|
| Filename | Overview |
|---|---|
| src/features/editor/components/editor.tsx | Three dialogs moved inside the existing Suspense boundary and converted to lazy imports; first-load suspension could briefly hide other dialog content in the same boundary. |
| src/features/media-library/services/media-asset-helpers.ts | New module extracted from media-library-service.ts; includes correct OPFS/DB/thumbnail rollback logic and dimension helpers. |
| src/features/media-library/services/file-access.ts | New module extracting FileAccessError and ensureFileHandlePermission; logger name intentionally reuses 'MediaLibraryService' for log grouping. |
| src/features/timeline/components/timeline-item/use-timeline-item-actions.ts | New hook extracting all action handlers from the 2700-line timeline-item component; logic is faithfully preserved with correct dependency arrays. |
| src/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.ts | New hook extracting all drag/drop handlers for transitions and effects; preserves readDraggedTransitionDescriptor and effect drop preview logic correctly. |
| src/features/keyframes/components/dopesheet-editor/selection-frame-actions.ts | Extracted buildSelectionFramePreview/commitSelectionFramePreview/duplicateSelectionFramePreview; logic matches original exactly, with new tests confirming behaviour. |
| src/features/keyframes/components/dopesheet-editor/header-frame-input-actions.ts | New module for frame-input commit planning; unit tests verify local/global commit logic and null-safety paths. |
| src/features/preview/components/mask-editor-overlay.tsx | Mask editor overlay shrunk significantly by delegating to new drawing/hit-testing/utils modules; all logic faithfully delegated. |
| src/features/media-library/stores/media-library-store.test.ts | Test updated to mock loggerEventMocks (for startEvent return value) and createOperationId; fixes previously broken structured logging assertions. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph editor["editor.tsx (lazy dialogs)"]
ED["LoadedEditor"]
S["<Suspense>"]
LD1["LazyClearKeyframesDialog\n(clearKeyframesDialogOpen)"]
LD2["LazyProjectMediaMatchDialog\n(projectMediaMatchDialogOpen)"]
LD3["LazyTtsGenerateDialog\n(ttsGenerateDialogOpen)"]
LD4["LazyExportDialog / LazyBundleExportDialog\n(pre-existing)"]
ED --> S
S --> LD4
S --> LD1
S --> LD2
S --> LD3
end
subgraph timeline_item["timeline-item/index.tsx (extracted hooks)"]
TI["TimelineItem"]
ACT["useTimelineItemActions\n(join, delete, captions, scenes, TTS...)"]
DROP["useTimelineItemDropHandlers\n(transition + effect DnD)"]
TI --> ACT
TI --> DROP
end
subgraph dopesheet["dopesheet-editor (extracted helpers)"]
DS["DopesheetEditor"]
FU["frame-utils.ts\nclampFrame, clampToAvoidBlockedRanges"]
HFA["header-frame-input-actions.ts\nplanLocal/GlobalHeaderFrameCommit"]
SFA["selection-frame-actions.ts\nbuildSelectionFramePreview"]
RAH["row-action-helpers.ts\nbuildRowKeyframeRefs, removeSelectionIds"]
SFG["sheet-preview-frame-groups.ts\ngetDisplayedGroupFrameGroups"]
DS --> FU & HFA & SFA & RAH & SFG
end
subgraph mask["mask-editor (extracted helpers)"]
MEO["MaskEditorOverlay"]
MED["mask-editor-drawing.ts\ndrawMaskSegment, drawMaskVertexWithHandles"]
MEH["mask-editor-hit-testing.ts\nhitTestMaskVertices, hitTestPenVertices"]
MEU["mask-editor-overlay-utils.ts\ncubicPointAt, isPointInPolygon..."]
MEO --> MED & MEH & MEU
end
subgraph media["media-library/services (extracted helpers)"]
MLS["media-library-service.ts"]
MAH["media-asset-helpers.ts\npersistGeneratedMediaAsset, getThumbnailDimensions"]
FA["file-access.ts\nFileAccessError, ensureFileHandlePermission"]
MLS -->|re-exports FileAccessError| FA
MLS --> MAH
end
Comments Outside Diff (1)
-
src/features/editor/components/editor.tsx, line 67-75 (link)Lazy dialogs share the existing Suspense boundary
LazyClearKeyframesDialog,LazyProjectMediaMatchDialog, andLazyTtsGenerateDialogare now placed inside the same<Suspense>block as the export/bundle-export dialogs. The first time any of these new lazy chunks loads, React suspends the entire boundary — which would briefly unmount the export dialog if it happens to be visible at the same moment.In practice this race is unlikely (users rarely open the clear-keyframes or TTS dialogs while an export is in progress), but giving each group of dialogs its own small Suspense boundary would eliminate the risk entirely:
Prompt To Fix With AI
This is a comment left during a code review. Path: src/features/editor/components/editor.tsx Line: 67-75 Comment: **Lazy dialogs share the existing Suspense boundary** `LazyClearKeyframesDialog`, `LazyProjectMediaMatchDialog`, and `LazyTtsGenerateDialog` are now placed inside the same `<Suspense>` block as the export/bundle-export dialogs. The first time any of these new lazy chunks loads, React suspends the entire boundary — which would briefly unmount the export dialog if it happens to be visible at the same moment. In practice this race is unlikely (users rarely open the clear-keyframes or TTS dialogs while an export is in progress), but giving each group of dialogs its own small Suspense boundary would eliminate the risk entirely: How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/features/editor/components/editor.tsx
Line: 67-75
Comment:
**Lazy dialogs share the existing Suspense boundary**
`LazyClearKeyframesDialog`, `LazyProjectMediaMatchDialog`, and `LazyTtsGenerateDialog` are now placed inside the same `<Suspense>` block as the export/bundle-export dialogs. The first time any of these new lazy chunks loads, React suspends the entire boundary — which would briefly unmount the export dialog if it happens to be visible at the same moment.
In practice this race is unlikely (users rarely open the clear-keyframes or TTS dialogs while an export is in progress), but giving each group of dialogs its own small Suspense boundary would eliminate the risk entirely:
```suggestion
{clearKeyframesDialogOpen && (
<Suspense fallback={null}>
<LazyClearKeyframesDialog />
</Suspense>
)}
{projectMediaMatchDialogOpen && (
<Suspense fallback={null}>
<LazyProjectMediaMatchDialog projectId={projectId} />
</Suspense>
)}
{ttsGenerateDialogOpen && (
<Suspense fallback={null}>
<LazyTtsGenerateDialog />
</Suspense>
)}
</Suspense>
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/features/media-library/services/file-access.ts
Line: 5
Comment:
**Logger name doesn't match the module**
`createLogger('MediaLibraryService')` was the correct name in the monolithic service, but this new dedicated file is `file-access.ts`. Using a more specific name makes it easier to filter logs to exactly the permission-check path:
```suggestion
const logger = createLogger('FileAccess');
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Refactor dopesheet frame and row action ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const logger = createLogger('MediaLibraryService'); | ||
|
|
||
| /** |
Contributor
There was a problem hiding this comment.
createLogger('MediaLibraryService') was the correct name in the monolithic service, but this new dedicated file is file-access.ts. Using a more specific name makes it easier to filter logs to exactly the permission-check path:
Suggested change
| /** | |
| const logger = createLogger('FileAccess'); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/media-library/services/file-access.ts
Line: 5
Comment:
**Logger name doesn't match the module**
`createLogger('MediaLibraryService')` was the correct name in the monolithic service, but this new dedicated file is `file-access.ts`. Using a more specific name makes it easier to filter logs to exactly the permission-check path:
```suggestion
const logger = createLogger('FileAccess');
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![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: 13
🧹 Nitpick comments (4)
src/features/timeline/components/timeline-item/use-timeline-item-actions.ts (1)
297-299: Consider narrowing theitemdependency inhandleEnterComposition.Line 313 passes the entire
itemobject toenterComposition, but onlyitem.compositionId,item.label, anditem.idare used. Including the fullitemin the dependency array (line 313) means the callback regenerates on any item property change.♻️ Narrower dependencies
const handleEnterComposition = useCallback(() => { if (!isCompositionItem || !item.compositionId) { return; } useCompositionNavigationStore.getState().enterComposition(item.compositionId, item.label, item.id); - }, [isCompositionItem, item]); + }, [isCompositionItem, item.compositionId, item.label, item.id]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/components/timeline-item/use-timeline-item-actions.ts` around lines 297 - 299, The handleEnterComposition callback currently depends on the entire item object causing unnecessary re-creations; update handleEnterComposition to only use and depend on the specific properties passed to enterComposition (item.compositionId, item.label, item.id) instead of the full item, and change its dependency array to include those three values (and enterComposition) so the callback only regenerates when those specific fields change.src/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.ts (1)
64-68: Consider simplifying the redundant descriptor check.The condition checks both
dragDescriptor(freshly read) anddraggedTransition(from hook params/render). SincereadDraggedTransitionDescriptoralready checks the store state first, the!draggedTransitioncheck is redundant but harmless as a defensive measure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.ts` around lines 64 - 68, The guard in handleTransitionCutDragOver redundantly checks both the freshly read dragDescriptor and the external draggedTransition; remove the redundant "!draggedTransition" condition so the early-return only checks readDraggedTransitionDescriptor(e) (and trackLocked) — keep readDraggedTransitionDescriptor, handleTransitionCutDragOver, trackLocked and draggedTransition references intact but eliminate the extra draggedTransition check to simplify the logic.src/features/timeline/stores/actions/item-edit-actions.ts (1)
1025-1033: Inconsistent indentation inrollingTrimItems.The
if/elseblock at lines 1025-1033 has inconsistent indentation compared to the rest of the function. The closing brace at line 1033 and subsequent code at lines 1035+ appear to be at the wrong indentation level.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/stores/actions/item-edit-actions.ts` around lines 1025 - 1033, The if/else inside rollingTrimItems has inconsistent indentation and a misaligned closing brace; adjust the block so the inner statements and the closing brace align with the surrounding function scope. Specifically, inside rollingTrimItems ensure the if branch calls itemsStore._trimItemStart(rightId, editPointDelta) then itemsStore._trimItemEnd(leftId, editPointDelta) and the else branch calls itemsStore._trimItemEnd(leftId, editPointDelta) then itemsStore._trimItemStart(rightId, editPointDelta), and move the closing brace so it is indented to the same level as the opening of the if/else and the rest of rollingTrimItems code.src/features/media-library/services/media-asset-helpers.ts (1)
94-159: Wrap generated-asset persistence in one structured event.This helper now spans OPFS save, thumbnail persistence, IndexedDB create, project association, and rollback, but the logging is still a few uncorrelated cleanup warnings. Please emit one
startEvent(...).success()/failure()withprojectId,mediaMetadata.id,mediaMetadata.mimeType, and rollback status so failures can be traced as one operation.As per coding guidelines "Use wide event pattern for multi-step operations:
log.startEvent(name, opId)accumulates context, emits one structured event via.success()/.failure(). UsecreateOperationId()for correlation. Include business context (project ID, item counts, codec, resolution) in events".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/media-library/services/media-asset-helpers.ts` around lines 94 - 159, In persistGeneratedMediaAsset wrap the whole multi-step flow in a single structured event: call createOperationId() and pass it to log.startEvent('persistGeneratedMediaAsset', opId) before any work; add projectId, mediaMetadata.id, and mediaMetadata.mimeType to the startEvent context. Track rollback flags (metadataCreated, thumbnailSaved, opfsDeleted) and on success call .success() with the same context; on error call .failure() and include the error plus final rollback-status flags. Keep existing cleanup attempts (opfsService.saveFile, saveThumbnailDB, createMediaDB, associateMediaWithProject, deleteMediaDB, deleteThumbnailsByMediaId, opfsService.deleteFile) but remove or simplify independent logger.warn calls and instead surface their outcomes into the failure event context so the operation emits one structured success()/failure() for the entire persistGeneratedMediaAsset operation.
🤖 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/keyframes/components/dopesheet-editor/frame-utils.ts`:
- Around line 14-20: The current loop in frame-utils.ts returns immediately on
the first blocked range hit (using blockedRanges, frame, initialFrame,
range.start, range.end), which can yield a candidate that still falls into a
subsequent/adjacent blocked range; instead either merge/sort blockedRanges up
front (coalescing overlapping or touching ranges) or, when you compute a
candidate escape (range.start - 1 or range.end), update frame to that candidate
and continue scanning all ranges (do not return immediately) until the candidate
is outside every blocked range; implement one of these approaches inside the
function that iterates blockedRanges so final returned frame is guaranteed not
to lie within any range.
In
`@src/features/keyframes/components/dopesheet-editor/header-frame-input-actions.ts`:
- Around line 1-2: The two import statements using relative paths should be
converted to repo path-alias imports that begin with '@/'; locate the imports
that bring in BlockedFrameRange and the functions clampFrame and
clampToAvoidBlockedRanges and replace their module specifiers with the
equivalent '@/...' aliased module paths (keeping the imported symbols unchanged)
so the file follows the repo convention of using the `@/`* alias instead of
relative paths.
In
`@src/features/keyframes/components/dopesheet-editor/selection-frame-actions.ts`:
- Around line 77-101: The loop currently calls clampToAvoidBlockedRanges per
keyframe and yields different per-keyframe deltas; instead compute a single
blocked-safe delta for the whole movableSelection: for each id in
movableSelectionIds use keyframeMetaById.get(id) to get initialFrame and compute
allowedDelta = clampToAvoidBlockedRanges(initialFrame + constrainedDeltaFrames,
initialFrame, transitionBlockedRanges) - initialFrame, collect all
allowedDeltas, then pick one commonDelta = (constrainedDeltaFrames > 0 ?
Math.min(...allowedDeltas) : Math.max(...allowedDeltas)) (or 0 if no ids) and
finally set nextPreviewFrames[id] = initialFrame + commonDelta for every
movableSelectionId and set appliedDeltaFrames = commonDelta so previewFrames and
appliedDeltaFrames are consistent across the selection (update uses of
movableSelectionIds, clampToAvoidBlockedRanges, constrainedDeltaFrames,
nextPreviewFrames, appliedDeltaFrames).
In `@src/features/media-library/services/file-access.ts`:
- Around line 22-35: The catch in ensureFileHandlePermission currently swallows
all exceptions and returns false; change it to throw a typed FileAccessError
with kind 'unknown' (including the original error) instead of returning false so
callers can distinguish unexpected failures from permission denial, and update
requestPermission (which currently returns
ensureFileHandlePermission(media.fileHandle)) to either await/propagate that
exception or wrap the call in a try/catch that rethrows the FileAccessError
unchanged—ensure both ensureFileHandlePermission and requestPermission
consistently propagate FileAccessError('unknown') for unexpected exceptions and
only return/resolve boolean on explicit permission outcomes.
In `@src/features/media-library/services/media-asset-helpers.ts`:
- Around line 21-39: getThumbnailDimensions currently sanitizes width/height but
uses maxSize raw; coerce and clamp maxSize into a safe integer (e.g. const
safeMax = Math.max(1, Math.round(Number(maxSize) || 0))) and then use safeMax
instead of maxSize in the returned width/height calculations (replace maxSize
with safeMax where used), so negative, zero or NaN inputs produce valid
thumbnail dimensions; keep existing safeWidth/safeHeight logic and only add the
new safeMax variable inside getThumbnailDimensions.
In `@src/features/timeline/components/timeline-item/use-timeline-item-actions.ts`:
- Around line 332-375: The run function creates an AbortController and a video
element but doesn't clean them up on unmount or on error; make the
AbortController and video persistent via refs (e.g., abortControllerRef and
videoRef) or at least store the controller where the component's cleanup effect
can access it, call abortControllerRef.current.abort() in a useEffect cleanup to
cancel detectScenes on unmount, and move video.src = '' plus removing event
handlers into a finally block (and revoke any object URL if used) so the video
is always cleared whether detectScenes succeeds or throws; ensure the same
AbortController instance is passed to detectScenes and that the cleanup abort is
wired to the component lifecycle.
In `@src/features/timeline/services/sized-accessed-memory-cache.ts`:
- Around line 22-35: The add method in sized-accessed-memory-cache.ts can insert
an entry whose sizeBytes exceeds this.maxSizeBytes, causing the cache to
permanently exceed its budget; fix by checking at the start of add(key: string,
entry: TEntry) if entry.sizeBytes > this.maxSizeBytes and, if so, do not insert
(early return or drop) instead of attempting evictions and set operations —
leave existing logic for updating currentSizeBytes, entries, and evictOldest
unchanged for normal-sized entries.
In `@src/features/timeline/stores/actions/item-edit-actions.ts`:
- Line 129: Replace the garbled UTF-8 sequences in comments in
src/features/timeline/stores/actions/item-edit-actions.ts (e.g., the comment
mentioning "_splitItem") with the proper characters (for example change "â€"" to
an em dash "—" or the intended punctuation) and update the other occurrence near
the later comment (the one around the second mention) so both comments read
correctly and consistently; search for the garbled sequences and correct them to
standard ASCII/Unicode punctuation.
- Around line 800-806: The execute('INSERT_FREEZE_FRAME', ...) callback can
return early with undefined when useItemsStore.getState()._splitItem(itemId,
playheadFrame) fails, causing the outer action to still return true; update the
callback so that on split failure it returns false (e.g., replace the early
return with "return false" after logging via
getLogger().error('[insertFreezeFrame] Split failed') ), or move the split call
out of execute and only call execute if splitResult is truthy, ensuring the
function returns false on failure instead of falling through to true; reference
the execute('INSERT_FREEZE_FRAME', ...), useItemsStore.getState()._splitItem,
and getLogger().error symbols when making the change.
In `@src/features/timeline/stores/actions/item-placement.ts`:
- Around line 45-67: The placement is non-deterministic because items are
processed in input order; group incoming items by trackId and for each track
sort that group's items by their from value before the loop that calls
findNextAvailableSpaceOnTrack so placement is deterministic per track. Update
the logic around occupiedRangesByTrack/trackItems and the loop that uses
findNextAvailableSpaceOnTrack and placedItems so you iterate sorted
track-specific arrays (preserving other fields) and then push/update trackItems
and placedItems in that sorted order.
In `@src/features/timeline/stores/timeline-persistence.ts`:
- Around line 867-878: When loading a project, the code only sets orphaned clips
when orphans.length > 0 so any previous orphan state remains; update the block
after validateProjectMediaReferences to handle the empty case: if orphans.length
> 0 keep the existing behavior (logger.warn,
useMediaLibraryStore.getState().setOrphanedClips(orphans),
openOrphanedClipsDialog()), otherwise call
useMediaLibraryStore.getState().setOrphanedClips([]) and ensure the orphan
dialog is closed (e.g. call a closeOrphanedClipsDialog/closeDialog method on
useMediaLibraryStore or otherwise hide it) so a clean project clears prior
orphan state; reference symbols: validateProjectMediaReferences, useItemsStore,
useCompositionsStore, useMediaLibraryStore, setOrphanedClips,
openOrphanedClipsDialog.
- Around line 279-294: When converting a visual composition wrapper to an audio
item in the audio-only repair path (the block using hasVisualWrapper,
hasOwnedAudio, wrapperTrackKind, existingAudioCompanion, ensureTrackOfKindNear
and assigning into items[wrapperIndex] as AudioItem), also remove any existing
composition-audio companion (existingAudioCompanion) from items to avoid leaving
a duplicate overlapping audio item; locate existingAudioCompanion by its trackId
or index and splice it out of items (or otherwise delete it), ensure you still
set changed = true, and then continue as before.
---
Nitpick comments:
In `@src/features/media-library/services/media-asset-helpers.ts`:
- Around line 94-159: In persistGeneratedMediaAsset wrap the whole multi-step
flow in a single structured event: call createOperationId() and pass it to
log.startEvent('persistGeneratedMediaAsset', opId) before any work; add
projectId, mediaMetadata.id, and mediaMetadata.mimeType to the startEvent
context. Track rollback flags (metadataCreated, thumbnailSaved, opfsDeleted) and
on success call .success() with the same context; on error call .failure() and
include the error plus final rollback-status flags. Keep existing cleanup
attempts (opfsService.saveFile, saveThumbnailDB, createMediaDB,
associateMediaWithProject, deleteMediaDB, deleteThumbnailsByMediaId,
opfsService.deleteFile) but remove or simplify independent logger.warn calls and
instead surface their outcomes into the failure event context so the operation
emits one structured success()/failure() for the entire
persistGeneratedMediaAsset operation.
In `@src/features/timeline/components/timeline-item/use-timeline-item-actions.ts`:
- Around line 297-299: The handleEnterComposition callback currently depends on
the entire item object causing unnecessary re-creations; update
handleEnterComposition to only use and depend on the specific properties passed
to enterComposition (item.compositionId, item.label, item.id) instead of the
full item, and change its dependency array to include those three values (and
enterComposition) so the callback only regenerates when those specific fields
change.
In
`@src/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.ts`:
- Around line 64-68: The guard in handleTransitionCutDragOver redundantly checks
both the freshly read dragDescriptor and the external draggedTransition; remove
the redundant "!draggedTransition" condition so the early-return only checks
readDraggedTransitionDescriptor(e) (and trackLocked) — keep
readDraggedTransitionDescriptor, handleTransitionCutDragOver, trackLocked and
draggedTransition references intact but eliminate the extra draggedTransition
check to simplify the logic.
In `@src/features/timeline/stores/actions/item-edit-actions.ts`:
- Around line 1025-1033: The if/else inside rollingTrimItems has inconsistent
indentation and a misaligned closing brace; adjust the block so the inner
statements and the closing brace align with the surrounding function scope.
Specifically, inside rollingTrimItems ensure the if branch calls
itemsStore._trimItemStart(rightId, editPointDelta) then
itemsStore._trimItemEnd(leftId, editPointDelta) and the else branch calls
itemsStore._trimItemEnd(leftId, editPointDelta) then
itemsStore._trimItemStart(rightId, editPointDelta), and move the closing brace
so it is indented to the same level as the opening of the if/else and the rest
of rollingTrimItems code.
🪄 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: 926011f3-aaca-462c-ad97-28822055e1af
📒 Files selected for processing (32)
src/features/editor/components/editor.tsxsrc/features/keyframes/components/dopesheet-editor/frame-utils.tssrc/features/keyframes/components/dopesheet-editor/header-frame-input-actions.test.tssrc/features/keyframes/components/dopesheet-editor/header-frame-input-actions.tssrc/features/keyframes/components/dopesheet-editor/index.tsxsrc/features/keyframes/components/dopesheet-editor/row-action-helpers.test.tssrc/features/keyframes/components/dopesheet-editor/row-action-helpers.tssrc/features/keyframes/components/dopesheet-editor/selection-frame-actions.test.tssrc/features/keyframes/components/dopesheet-editor/selection-frame-actions.tssrc/features/keyframes/components/dopesheet-editor/sheet-preview-frame-groups.test.tssrc/features/keyframes/components/dopesheet-editor/sheet-preview-frame-groups.tssrc/features/media-library/services/compound-clip-thumbnail-service.tssrc/features/media-library/services/file-access.tssrc/features/media-library/services/media-asset-helpers.tssrc/features/media-library/services/media-library-service.tssrc/features/media-library/stores/media-library-store.test.tssrc/features/preview/components/mask-editor-drawing.tssrc/features/preview/components/mask-editor-hit-testing.tssrc/features/preview/components/mask-editor-overlay-utils.tssrc/features/preview/components/mask-editor-overlay.tsxsrc/features/timeline/components/timeline-item/index.tsxsrc/features/timeline/components/timeline-item/use-timeline-item-actions.tssrc/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.tssrc/features/timeline/services/filmstrip-cache.tssrc/features/timeline/services/filmstrip-memory-state.tssrc/features/timeline/services/sized-accessed-memory-cache.tssrc/features/timeline/services/waveform-cache.tssrc/features/timeline/stores/actions/item-actions.tssrc/features/timeline/stores/actions/item-edit-actions.tssrc/features/timeline/stores/actions/item-placement.tssrc/features/timeline/stores/timeline-persistence.tssrc/features/timeline/stores/timeline-store-facade.ts
Comment on lines
+506
to
+712
| export async function saveTimeline(projectId: string): Promise<void> { | ||
| // If currently editing a sub-composition, navigate back to root to save | ||
| // the main timeline data, then restore the full breadcrumb path after save completes. | ||
| const navStore = useCompositionNavigationStore.getState(); | ||
| const previousBreadcrumbs = navStore.breadcrumbs | ||
| .filter((breadcrumb) => breadcrumb.compositionId !== null) | ||
| .map((breadcrumb) => ({ | ||
| compositionId: breadcrumb.compositionId!, | ||
| label: breadcrumb.label, | ||
| entryItemId: breadcrumb.entryItemId, | ||
| })); | ||
| if (previousBreadcrumbs.length > 0) { | ||
| navStore.resetToRoot(); | ||
| } | ||
|
|
||
| const restoreCompositionPath = () => { | ||
| for (const breadcrumb of previousBreadcrumbs) { | ||
| useCompositionNavigationStore.getState().enterComposition( | ||
| breadcrumb.compositionId, | ||
| breadcrumb.label, | ||
| breadcrumb.entryItemId, | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| // Read directly from domain stores | ||
| const itemsState = useItemsStore.getState(); | ||
| const transitionsState = useTransitionsStore.getState(); | ||
| const keyframesState = useKeyframesStore.getState(); | ||
| const markersState = useMarkersStore.getState(); | ||
| const currentFrame = usePlaybackStore.getState().currentFrame; | ||
| const zoomLevel = useZoomStore.getState().level; | ||
|
|
||
| try { | ||
| const project = await getProject(projectId); | ||
| if (!project) { | ||
| throw new Error(`Project not found: ${projectId}`); | ||
| } | ||
|
|
||
| const settingsState = useTimelineSettingsStore.getState(); | ||
|
|
||
| // Build timeline data (fps is stored in project.metadata, not timeline) | ||
| const timeline: ProjectTimeline = { | ||
| tracks: itemsState.tracks as ProjectTimeline['tracks'], | ||
| items: itemsState.items as ProjectTimeline['items'], | ||
| currentFrame, | ||
| zoomLevel, | ||
| scrollPosition: settingsState.scrollPosition, | ||
| ...(markersState.inPoint !== null && { inPoint: markersState.inPoint }), | ||
| ...(markersState.outPoint !== null && { outPoint: markersState.outPoint }), | ||
| ...(markersState.markers.length > 0 && { | ||
| markers: markersState.markers.map((m) => ({ | ||
| id: m.id, | ||
| frame: m.frame, | ||
| color: m.color, | ||
| ...(m.label && { label: m.label }), | ||
| })), | ||
| }), | ||
| ...(transitionsState.transitions.length > 0 && { | ||
| transitions: transitionsState.transitions.map(cloneTransitionForProject), | ||
| }), | ||
| ...(keyframesState.keyframes.length > 0 && { | ||
| keyframes: keyframesState.keyframes.map((ik) => ({ | ||
| itemId: ik.itemId, | ||
| properties: ik.properties.map((pk) => ({ | ||
| property: pk.property, | ||
| keyframes: pk.keyframes.map((k) => ({ | ||
| id: k.id, | ||
| frame: k.frame, | ||
| value: k.value, | ||
| easing: k.easing, | ||
| ...(k.easingConfig && { easingConfig: k.easingConfig }), | ||
| })), | ||
| })), | ||
| })), | ||
| }), | ||
| // Sub-compositions (pre-comps) | ||
| ...(() => { | ||
| const comps = useCompositionsStore.getState().compositions; | ||
| if (comps.length === 0) return {}; | ||
| return { | ||
| compositions: comps.map((c) => ({ | ||
| id: c.id, | ||
| name: c.name, | ||
| items: c.items as ProjectTimeline['items'], | ||
| tracks: c.tracks as ProjectTimeline['tracks'], | ||
| ...(c.transitions?.length && { | ||
| transitions: c.transitions.map(cloneTransitionForProject) as ProjectTimeline['transitions'], | ||
| }), | ||
| ...(c.keyframes?.length && { keyframes: c.keyframes as ProjectTimeline['keyframes'] }), | ||
| fps: c.fps, | ||
| width: c.width, | ||
| height: c.height, | ||
| durationInFrames: c.durationInFrames, | ||
| ...(c.backgroundColor && { backgroundColor: c.backgroundColor }), | ||
| })), | ||
| }; | ||
| })(), | ||
| }; | ||
|
|
||
| // Generate thumbnail — prefer capturing the existing preview canvas | ||
| // (near-free: reuses the already-initialized scrub renderer with cached | ||
| // media + GPU pipeline) and fall back to a full renderSingleFrame only | ||
| // when the preview capture path is unavailable. | ||
| let thumbnailId: string | undefined; | ||
| if (itemsState.items.length > 0) { | ||
| try { | ||
| const width = project.metadata?.width || 1920; | ||
| const height = project.metadata?.height || 1080; | ||
|
|
||
| // Calculate thumbnail dimensions preserving project aspect ratio | ||
| const maxThumbWidth = 320; | ||
| const maxThumbHeight = 180; | ||
| const projectAspectRatio = width / height; | ||
| const targetAspectRatio = maxThumbWidth / maxThumbHeight; | ||
|
|
||
| let thumbWidth: number; | ||
| let thumbHeight: number; | ||
| if (projectAspectRatio > targetAspectRatio) { | ||
| thumbWidth = maxThumbWidth; | ||
| thumbHeight = Math.round(maxThumbWidth / projectAspectRatio); | ||
| } else { | ||
| thumbHeight = maxThumbHeight; | ||
| thumbWidth = Math.round(maxThumbHeight * projectAspectRatio); | ||
| } | ||
|
|
||
| let thumbnailBlob: Blob | null = null; | ||
|
|
||
| // Fast path: capture from existing preview renderer (avoids full re-init) | ||
| const captureCanvasSource = usePreviewBridgeStore.getState().captureCanvasSource; | ||
| if (captureCanvasSource) { | ||
| try { | ||
| const sourceCanvas = await captureCanvasSource(); | ||
| if (sourceCanvas) { | ||
| thumbnailBlob = await scaleCanvasToBlob(sourceCanvas, thumbWidth, thumbHeight, 0.85); | ||
| } | ||
| } catch { | ||
| // Fall through to slow path | ||
| } | ||
| } | ||
|
|
||
| // Slow path: full render from scratch (when preview isn't available) | ||
| if (!thumbnailBlob) { | ||
| const fps = project.metadata?.fps || 30; | ||
| const backgroundColor = project.metadata?.backgroundColor; | ||
| const composition = convertTimelineToComposition( | ||
| itemsState.tracks, | ||
| itemsState.items, | ||
| transitionsState.transitions, | ||
| fps, | ||
| width, | ||
| height, | ||
| null, null, | ||
| keyframesState.keyframes, | ||
| backgroundColor | ||
| ); | ||
| const resolvedTracks = await resolveMediaUrls(composition.tracks); | ||
| const resolvedComposition = { ...composition, tracks: resolvedTracks }; | ||
| thumbnailBlob = await renderSingleFrame({ | ||
| composition: resolvedComposition, | ||
| frame: currentFrame, | ||
| width: thumbWidth, | ||
| height: thumbHeight, | ||
| quality: 0.85, | ||
| format: 'image/jpeg', | ||
| }); | ||
| } | ||
|
|
||
| // Save thumbnail to IndexedDB | ||
| thumbnailId = `project:${projectId}:cover`; | ||
| await saveThumbnail({ | ||
| id: thumbnailId, | ||
| mediaId: projectId, | ||
| blob: thumbnailBlob, | ||
| timestamp: Date.now(), | ||
| width: thumbWidth, | ||
| height: thumbHeight, | ||
| }); | ||
| } catch (thumbError) { | ||
| // Thumbnail generation failure shouldn't block save | ||
| logger.warn('Failed to generate thumbnail:', thumbError); | ||
| } | ||
| } | ||
|
|
||
| // Update project | ||
| // Clear deprecated thumbnail field when using thumbnailId to save space | ||
| await updateProject(projectId, { | ||
| timeline, | ||
| ...(thumbnailId && { thumbnailId, thumbnail: undefined }), | ||
| updatedAt: Date.now(), | ||
| }); | ||
|
|
||
| // Mark as clean after successful save | ||
| useTimelineSettingsStore.getState().markClean(); | ||
|
|
||
| // Re-enter the sub-composition the user was editing before save | ||
| if (previousBreadcrumbs.length > 0) { | ||
| restoreCompositionPath(); | ||
| } | ||
| } catch (error) { | ||
| logger.error('Failed to save timeline:', error); | ||
| // Re-enter even on failure so user doesn't lose their editing context | ||
| if (previousBreadcrumbs.length > 0) { | ||
| restoreCompositionPath(); | ||
| } | ||
| throw error; | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use one correlated structured event for the save/load workflows.
These functions are multi-step operations, but the new module still emits scattered debug/info/warn/error calls instead of a single correlated event with accumulated context. That makes save/load failures much harder to trace across migration, thumbnailing, validation, and persistence steps.
As per coding guidelines "Use wide event pattern for multi-step operations: log.startEvent(name, opId) accumulates context, emits one structured event via .success() / .failure(). Use createOperationId() for correlation. Include business context (project ID, item counts, codec, resolution) in events".
Also applies to: 726-888
…timeline - Isolate lazy dialog Suspense boundaries to prevent cross-unmount - Fix clampToAvoidBlockedRanges to re-scan all ranges until settled - Use unified blocked-safe delta for multi-keyframe selection moves - Convert dopesheet imports to @/ alias convention - Throw FileAccessError on unexpected permission failures - Sanitize maxSize in getThumbnailDimensions - Fix logger names for extracted modules (FileAccess, MediaAssetHelpers) - Clean up scene detection abort controller and video on unmount - Remove redundant draggedTransition guard in drop handler - Reject oversized entries in SizedAccessedMemoryCache - Fix garbled UTF-8 em dashes and indentation in item-edit-actions - Return false from execute callback on freeze-frame split failure - Deterministic item placement by sorting per-track groups - Clear orphan state when project has no orphaned clips - Remove duplicate audio companion in audio-only comp repair
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
src/features/timeline/stores/timeline-persistence.ts (2)
881-887:⚠️ Potential issue | 🟠 MajorExplicitly close orphan dialog when no orphans are found.
Lines 885-887 clear orphaned clips but do not close the dialog in this module. If the dialog was opened by a prior load, it can remain stale unless
setOrphanedClips([])implicitly closes it.Please verify store behavior and wire an explicit close action if needed:
#!/bin/bash set -euo pipefail # Inspect orphan dialog state/actions in media library store implementation. fd -i 'media-library-store.*' src | while read -r file; do echo "=== $file ===" rg -n -C3 'orphan|openOrphanedClipsDialog|closeOrphanedClipsDialog|close.*Dialog|setOrphanedClips' "$file" doneExpected result: there is a close action that gets called in the
elsebranch aftersetOrphanedClips([]).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/stores/timeline-persistence.ts` around lines 881 - 887, The else branch currently clears orphaned clips but does not explicitly close the orphan dialog; update the logic in timeline-persistence where useMediaLibraryStore.getState().setOrphanedClips([]) is called to also call the store's dialog-close action (e.g., useMediaLibraryStore.getState().closeOrphanedClipsDialog()) so the dialog is not left open from a prior load; if the store uses a different close action name, call that instead (verify existence of closeOrphanedClipsDialog or equivalent on useMediaLibraryStore.getState()) and then setOrphanedClips([]).
513-719:⚠️ Potential issue | 🟠 MajorUse a single correlated wide event for each save/load operation.
Lines 513-719 and 733-897 still emit scattered
debug/info/warn/errorcalls instead of one correlated operation event with accumulated context. This makes multi-step failures harder to trace.Suggested refactor pattern
+import { createOperationId } from '@/shared/logging/logger'; ... export async function saveTimeline(projectId: string): Promise<void> { + const opId = createOperationId(); + const evt = logger.startEvent('timeline.save', opId).context({ projectId }); try { ... - logger.warn('Failed to generate thumbnail:', thumbError); + evt.context({ thumbnailFailed: true, thumbnailError: String(thumbError) }); ... + evt.success({ itemsCount: itemsState.items.length, tracksCount: itemsState.tracks.length }); } catch (error) { - logger.error('Failed to save timeline:', error); + evt.failure(error, { projectId }); throw error; } }As per coding guidelines "Use wide event pattern for multi-step operations:
log.startEvent(name, opId)accumulates context, emits one structured event via.success()/.failure(). UsecreateOperationId()for correlation. Include business context (project ID, item counts, codec, resolution) in events".Also applies to: 733-897
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/stores/timeline-persistence.ts` around lines 513 - 719, saveTimeline emits scattered logs instead of a single correlated wide event; refactor to use createOperationId() and log.startEvent(name, opId) to accumulate context and call .success() or .failure() once. In saveTimeline wrap the whole save flow (project fetch, timeline build, thumbnail generation, updateProject) in a single operation: create an opId via createOperationId(), call log.startEvent('saveTimeline', opId) with initial context (projectId, itemsState.items.length, tracks length, fps/resolution from project.metadata, etc.), add context as you progress (e.g., thumbnail path/size, fallback render used, errors) to the same event, replace scattered logger.warn/logger.error calls (inside thumbnail try/catch and main catch) with event.addContext or event.log and finish with event.success({updatedAt,...}) on success or event.failure({error}) on failure; ensure references to functions/variables like saveTimeline, createOperationId, log.startEvent, updateProject, renderSingleFrame, saveThumbnail, and thumbnailId are updated accordingly.
🧹 Nitpick comments (3)
src/features/editor/components/editor.tsx (1)
53-67: Switch the new lazy imports to@/aliases.These three new dynamic imports use relative paths, which drifts from the repo import convention for
src/*modules.♻️ Proposed fix
const LazyClearKeyframesDialog = lazy(() => - import('./clear-keyframes-dialog').then((module) => ({ + import('@/features/editor/components/clear-keyframes-dialog').then((module) => ({ default: module.ClearKeyframesDialog, })) ); const LazyTtsGenerateDialog = lazy(() => - import('./tts-generate-dialog').then((module) => ({ + import('@/features/editor/components/tts-generate-dialog').then((module) => ({ default: module.TtsGenerateDialog, })) ); const LazyProjectMediaMatchDialog = lazy(() => - import('./project-media-match-dialog').then((module) => ({ + import('@/features/editor/components/project-media-match-dialog').then((module) => ({ default: module.ProjectMediaMatchDialog, })) );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/editor/components/editor.tsx` around lines 53 - 67, Update the three dynamic imports to use the repo path alias instead of relative paths: replace './clear-keyframes-dialog', './tts-generate-dialog', and './project-media-match-dialog' with '@/features/editor/components/clear-keyframes-dialog', '@/features/editor/components/tts-generate-dialog', and '@/features/editor/components/project-media-match-dialog' respectively in the lazy definitions for LazyClearKeyframesDialog, LazyTtsGenerateDialog, and LazyProjectMediaMatchDialog so they follow the `@/*` import convention.src/features/media-library/services/media-asset-helpers.ts (2)
10-10: Prefer@/alias import foropfs-service.Use the project alias style here for consistency with the repository import convention.
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/media-library/services/media-asset-helpers.ts` at line 10, In media-asset-helpers.ts replace the relative import of opfsService with the project path-alias form: change the import statement that currently references './opfs-service' to use the '@/...' alias (importing opfsService via the repository alias) so the module import follows the codebase convention; ensure the imported symbol name remains opfsService and that any export default/named import style matches the opfs-service module.
95-159: Use a single wide operation event for this transactional workflow.This is a multi-step operation (OPFS + thumbnails + media record + association + rollback), but logging is currently scattered warnings only. Please wrap it with
startEvent(..., createOperationId())and emit one.success()/.failure()event with context (projectId,mediaId, rollback outcome).As per coding guidelines, "Use wide event pattern for multi-step operations:
log.startEvent(name, opId)accumulates context, emits one structured event via.success()/.failure(). UsecreateOperationId()for correlation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/media-library/services/media-asset-helpers.ts` around lines 95 - 159, Wrap the entire persistGeneratedMediaAsset workflow with a wide operation event: create an opId with createOperationId(), call log.startEvent('persistGeneratedMediaAsset', opId) at the top and keep the returned event object; on successful completion call event.success({ projectId, mediaId: mediaMetadata.id, opId }); in the catch block call event.failure(error, { projectId, mediaId: mediaMetadata?.id, opId, rollback: { metadataDeleted: metadataCreated ? 'attempted' : 'none', thumbnailDeleted: thumbnailSaved ? 'attempted' : 'none', opfsDeleted: mediaMetadata?.opfsPath ? 'attempted' : 'none' } }) after performing the existing rollback attempts; reference the function persistGeneratedMediaAsset and the flags metadataCreated and thumbnailSaved so the event includes those rollback outcomes for correlation.
🤖 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/media-library/services/file-access.ts`:
- Around line 25-39: Wrap the multi-step permission flow in a wide-event:
generate an operation id via createOperationId() at the start of the function,
call log.startEvent('file.permission.check', opId) before invoking
handle.queryPermission/handle.requestPermission, then on any final outcome emit
either .success({ permission }) with the resolved permission state or .failure({
error }) when catching an exception; replace the direct logger.error call with
the event.failure call and still throw the existing FileAccessError (preserving
the current message construction), and ensure the event includes the operation
id and relevant context (e.g., which step returned which permission) so the
entire flow is traceable (referencing handle.queryPermission,
handle.requestPermission, logger.error, FileAccessError, createOperationId, and
log.startEvent/.success/.failure).
In `@src/features/media-library/services/media-asset-helpers.ts`:
- Around line 113-123: Thumbnail dimension checks currently allow negative
values and treat 0 as falsy; compute sanitized integer dimensions before
persisting: derive sanitizedWidth and sanitizedHeight from
thumbnailWidth/thumbnailHeight by coercing to numbers, taking absolute value,
flooring to integers, and clamping to a minimum of 1 (e.g., Math.max(1,
Math.floor(Math.abs(...)))) and only call saveThumbnailDB if thumbnailBlob is
present and both sanitized dimensions are valid; pass these
sanitizedWidth/sanitizedHeight in the object sent to saveThumbnailDB (keep use
of crypto.randomUUID(), mediaMetadata.id, timestamp, and blob).
- Around line 46-51: The createImageBitmap branch (call to createImageBitmap and
use of getImageDimensionsFromBitmap) must be wrapped in a try/catch so a
rejection doesn't abort the function and prevents the Image-based fallback from
running; call createImageBitmap(file) inside try, extract dimensions and close
the bitmap on success, and ensure bitmap.close() is called in a finally if
bitmap was created, while the catch should swallow or log the error and allow
execution to continue to the Image fallback logic (do not rethrow).
In `@src/features/timeline/components/timeline-item/use-timeline-item-actions.ts`:
- Around line 367-398: In use-timeline-item-actions (scene detection path)
you're mixing source-native frames with project frames: cut.frame, clipDuration,
and clipFrom are in project FPS but sourceStart is in the media's native FPS;
convert sourceStart to project frames (or convert all to seconds) before
subtracting. Retrieve the media asset fps from the media library store
(media.fps) and convert sourceStart via seconds = sourceStart / mediaFps then
projectFrame = Math.round(seconds * projectFps) (or do the inverse for cut/frame
math), then use that converted value in the map/filter that builds splitFrames
so splits are computed in a consistent FPS domain.
- Around line 342-425: The run function's metadata wait and finally cleanup
aren't abort-aware so an earlier run can toast errors and remove overlays from a
later run; make the video metadata await respect sceneDetectionAbortRef by
creating the AbortController (abortController) and wiring its signal into the
metadata Promise (listen for signal.abort and reject with an AbortError), catch
AbortError early and return without toasting, and in the finally block only call
useTimelineItemOverlayStore.getState().removeOverlay(clipId,
SCENE_DETECTION_OVERLAY_ID) if sceneDetectionAbortRef.current ===
abortController (i.e., this run still owns the controller) so stale runs don't
remove overlays for newer runs.
In
`@src/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.ts`:
- Around line 182-193: The current filter builds lockedTrackIds from raw
track.locked, missing locks inherited from groups; replace that logic to call
resolveEffectiveTrackStates() (from group-utils.ts) on
useTimelineStore.getState().tracks and collect ids where the effective
state.locked is true, assign to lockedTrackIds, then use that set in the
existing filter that removes targets from resolveEffectDropTargetIds({ ... });
update the variable name if needed but keep the rest of the return expression
intact.
- Around line 165-172: resolveDirectEffectDropTemplate and
resolveEffectDropTargetIds currently allow audio clips from the selection to be
returned as valid effect targets (because only the hovered item is checked), so
filter out audio items when building effect templates/target ID lists: after
calling getTemplateEffectsForDirectApplication(payload) or when computing target
IDs in resolveEffectDropTargetIds, remove any effects/IDs that correspond to
items with type === 'audio' (use the same item.type checks or selection lookup
used elsewhere) so addEffects never receives non-visual (audio) targets; update
both resolveDirectEffectDropTemplate and resolveEffectDropTargetIds to apply
this filtering consistently before returning.
In `@src/features/timeline/stores/timeline-persistence.ts`:
- Around line 129-153: The code uses fallbackDurationInFrames (in
timeline/project FPS) directly for sourceDuration which can mix units; in
normalizeCompoundWrapperSourceFields compute inferredSourceDuration using
timelineToSourceFrames (already present) and use that as the fallback for
sourceDuration instead of fallbackDurationInFrames so sourceDuration is in
source-native frames; update the return to set sourceDuration:
item.sourceDuration ?? inferredSourceDuration while keeping
sourceStart/sourceEnd/sourceFps/speed logic unchanged.
---
Duplicate comments:
In `@src/features/timeline/stores/timeline-persistence.ts`:
- Around line 881-887: The else branch currently clears orphaned clips but does
not explicitly close the orphan dialog; update the logic in timeline-persistence
where useMediaLibraryStore.getState().setOrphanedClips([]) is called to also
call the store's dialog-close action (e.g.,
useMediaLibraryStore.getState().closeOrphanedClipsDialog()) so the dialog is not
left open from a prior load; if the store uses a different close action name,
call that instead (verify existence of closeOrphanedClipsDialog or equivalent on
useMediaLibraryStore.getState()) and then setOrphanedClips([]).
- Around line 513-719: saveTimeline emits scattered logs instead of a single
correlated wide event; refactor to use createOperationId() and
log.startEvent(name, opId) to accumulate context and call .success() or
.failure() once. In saveTimeline wrap the whole save flow (project fetch,
timeline build, thumbnail generation, updateProject) in a single operation:
create an opId via createOperationId(), call log.startEvent('saveTimeline',
opId) with initial context (projectId, itemsState.items.length, tracks length,
fps/resolution from project.metadata, etc.), add context as you progress (e.g.,
thumbnail path/size, fallback render used, errors) to the same event, replace
scattered logger.warn/logger.error calls (inside thumbnail try/catch and main
catch) with event.addContext or event.log and finish with
event.success({updatedAt,...}) on success or event.failure({error}) on failure;
ensure references to functions/variables like saveTimeline, createOperationId,
log.startEvent, updateProject, renderSingleFrame, saveThumbnail, and thumbnailId
are updated accordingly.
---
Nitpick comments:
In `@src/features/editor/components/editor.tsx`:
- Around line 53-67: Update the three dynamic imports to use the repo path alias
instead of relative paths: replace './clear-keyframes-dialog',
'./tts-generate-dialog', and './project-media-match-dialog' with
'@/features/editor/components/clear-keyframes-dialog',
'@/features/editor/components/tts-generate-dialog', and
'@/features/editor/components/project-media-match-dialog' respectively in the
lazy definitions for LazyClearKeyframesDialog, LazyTtsGenerateDialog, and
LazyProjectMediaMatchDialog so they follow the `@/*` import convention.
In `@src/features/media-library/services/media-asset-helpers.ts`:
- Line 10: In media-asset-helpers.ts replace the relative import of opfsService
with the project path-alias form: change the import statement that currently
references './opfs-service' to use the '@/...' alias (importing opfsService via
the repository alias) so the module import follows the codebase convention;
ensure the imported symbol name remains opfsService and that any export
default/named import style matches the opfs-service module.
- Around line 95-159: Wrap the entire persistGeneratedMediaAsset workflow with a
wide operation event: create an opId with createOperationId(), call
log.startEvent('persistGeneratedMediaAsset', opId) at the top and keep the
returned event object; on successful completion call event.success({ projectId,
mediaId: mediaMetadata.id, opId }); in the catch block call event.failure(error,
{ projectId, mediaId: mediaMetadata?.id, opId, rollback: { metadataDeleted:
metadataCreated ? 'attempted' : 'none', thumbnailDeleted: thumbnailSaved ?
'attempted' : 'none', opfsDeleted: mediaMetadata?.opfsPath ? 'attempted' :
'none' } }) after performing the existing rollback attempts; reference the
function persistGeneratedMediaAsset and the flags metadataCreated and
thumbnailSaved so the event includes those rollback outcomes for correlation.
🪄 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: 3af5f856-df9e-4ba0-96a6-657ee6ed244b
📒 Files selected for processing (12)
src/features/editor/components/editor.tsxsrc/features/keyframes/components/dopesheet-editor/frame-utils.tssrc/features/keyframes/components/dopesheet-editor/header-frame-input-actions.tssrc/features/keyframes/components/dopesheet-editor/selection-frame-actions.tssrc/features/media-library/services/file-access.tssrc/features/media-library/services/media-asset-helpers.tssrc/features/timeline/components/timeline-item/use-timeline-item-actions.tssrc/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.tssrc/features/timeline/services/sized-accessed-memory-cache.tssrc/features/timeline/stores/actions/item-edit-actions.tssrc/features/timeline/stores/actions/item-placement.tssrc/features/timeline/stores/timeline-persistence.ts
✅ Files skipped from review due to trivial changes (2)
- src/features/keyframes/components/dopesheet-editor/header-frame-input-actions.ts
- src/features/timeline/stores/actions/item-edit-actions.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/features/timeline/services/sized-accessed-memory-cache.ts
- src/features/timeline/stores/actions/item-placement.ts
- src/features/keyframes/components/dopesheet-editor/frame-utils.ts
- src/features/keyframes/components/dopesheet-editor/selection-frame-actions.ts
Comment on lines
+165
to
+172
| const resolveDirectEffectDropTemplate = useCallback((payload: unknown) => { | ||
| const effects = getTemplateEffectsForDirectApplication(payload); | ||
| if (!effects || trackLocked || item.type === 'audio') { | ||
| return null; | ||
| } | ||
|
|
||
| return effects; | ||
| }, [item.type, trackLocked]); |
Contributor
There was a problem hiding this comment.
Exclude audio clips from multi-select effect targets.
This only blocks the hovered item when it is audio. If the current selection also contains audio clips, resolveEffectDropTargetIds() can still return those IDs here, so the preview path looks valid and addEffects() receives non-visual targets.
Suggested fix
- return resolveEffectDropTargetIds({
+ return resolveEffectDropTargetIds({
hoveredItemId: item.id,
items,
selectedItemIds,
- }).filter((itemId) => !lockedTrackIds.has(itemById.get(itemId)?.trackId ?? ''));
+ }).filter((itemId) => {
+ const targetItem = itemById.get(itemId);
+ return (
+ targetItem != null &&
+ targetItem.type !== 'audio' &&
+ !lockedTrackIds.has(targetItem.trackId)
+ );
+ });Also applies to: 174-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.ts`
around lines 165 - 172, resolveDirectEffectDropTemplate and
resolveEffectDropTargetIds currently allow audio clips from the selection to be
returned as valid effect targets (because only the hovered item is checked), so
filter out audio items when building effect templates/target ID lists: after
calling getTemplateEffectsForDirectApplication(payload) or when computing target
IDs in resolveEffectDropTargetIds, remove any effects/IDs that correspond to
items with type === 'audio' (use the same item.type checks or selection lookup
used elsewhere) so addEffects never receives non-visual (audio) targets; update
both resolveDirectEffectDropTemplate and resolveEffectDropTargetIds to apply
this filtering consistently before returning.
…ersistence - Wrap file permission flow and persistGeneratedMediaAsset in wide events - Wrap createImageBitmap in try/catch to allow Image fallback on rejection - Sanitize thumbnail dimensions to positive integers before persisting - Fix FPS domain mixing in scene detection (convert sourceStart to project FPS) - Make scene detection abort-aware with signal-wired metadata wait and guarded cleanup - Use resolveEffectiveTrackStates for group-aware locked track filtering in effect drops - Fix sourceDuration fallback to use source-native frames via inferredSourceDuration - Close orphaned clips dialog when no orphans found on timeline load - Replace scattered saveTimeline logs with single correlated wide event - Use @/* path alias for lazy dialog imports in editor
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.ts (1)
166-194:⚠️ Potential issue | 🟠 MajorFilter selected audio clips out of direct-effect targets.
This still only rejects the hovered item. If the current selection includes audio clips,
resolveEffectDropTargetIds()can return those IDs here andaddEffects()will receive non-visual targets.Suggested fix
return resolveEffectDropTargetIds({ hoveredItemId: item.id, items, selectedItemIds, - }).filter((itemId) => !lockedTrackIds.has(itemById.get(itemId)?.trackId ?? '')); + }).filter((itemId) => { + const targetItem = itemById.get(itemId); + return ( + targetItem != null && + targetItem.type !== 'audio' && + !lockedTrackIds.has(targetItem.trackId) + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.ts` around lines 166 - 194, resolveEffectDropTargets currently filters out locked-track targets but not selected audio clips, so resolveEffectDropTargetIds can return audio item IDs leading to non-visual targets for addEffects; update resolveEffectDropTargets to additionally exclude any item IDs whose corresponding item (from useItemsStore.getState().items / itemById map) has type === 'audio' before returning, ensuring you still respect hoveredItemId/selectedItemIds and lockedTrackIds while removing audio items from the final array.
🧹 Nitpick comments (1)
src/features/editor/components/editor.tsx (1)
183-187: Consider moving the dialog wiring into a smallEditorDialogHost.The new open-state selectors plus the conditional mounts add more dialog-specific branching to
LoadedEditor. Pulling this block into a leaf component would keep the editor shell focused on layout/load/save concerns and make future dialog additions cheaper.Also applies to: 529-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/editor/components/editor.tsx` around lines 183 - 187, Extract the dialog open-state selectors and conditional mounts out of LoadedEditor into a new leaf component (e.g., EditorDialogHost) that encapsulates the useClearKeyframesDialogStore, useTtsGenerateDialogStore, and useProjectMediaMatchDialogStore selectors (including the projectId-specific condition), and renders the dialog components; then replace the current selector block in LoadedEditor (the clearKeyframesDialogOpen, ttsGenerateDialogOpen, projectMediaMatchDialogOpen logic) with a single <EditorDialogHost ...> invocation (pass projectId or any needed props) so LoadedEditor only handles layout/load/save concerns while EditorDialogHost owns dialog wiring and mounts.
🤖 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/media-library/services/media-asset-helpers.ts`:
- Around line 123-127: The code currently sanitizes
thumbnailWidth/thumbnailHeight to 1 when missing, causing any present
thumbnailBlob to be saved as a synthetic 1x1; change the logic in
media-asset-helpers (the block that computes sanitizedWidth/sanitizedHeight and
uses thumbnailBlob) to first validate raw thumbnailWidth and thumbnailHeight
(e.g., ensure they are numeric and > 0) and only proceed to sanitize/accept
thumbnailBlob if the raw values are valid; keep using
sanitizedWidth/sanitizedHeight (Math.abs/Math.floor/Math.max) after that
validation and bail out (skip persisting thumbnail) when raw dimensions are
invalid or missing to avoid implicit 1x1 thumbnails.
- Around line 21-29: getThumbnailDimensions can still produce NaN/Infinity
because Math.round/Math.max alone don't coerce non-finite inputs; update the
function to first coerce width, height, and maxSize to numeric finite values
(e.g., let w = Number(width); if (!Number.isFinite(w)) w = 1) before rounding
and clamping, then compute safeWidth/safeHeight/safeMax from those finite values
and continue returning the rounded, clamped dimensions; reference
variables/function: getThumbnailDimensions, safeWidth, safeHeight, safeMax.
---
Duplicate comments:
In
`@src/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.ts`:
- Around line 166-194: resolveEffectDropTargets currently filters out
locked-track targets but not selected audio clips, so resolveEffectDropTargetIds
can return audio item IDs leading to non-visual targets for addEffects; update
resolveEffectDropTargets to additionally exclude any item IDs whose
corresponding item (from useItemsStore.getState().items / itemById map) has type
=== 'audio' before returning, ensuring you still respect
hoveredItemId/selectedItemIds and lockedTrackIds while removing audio items from
the final array.
---
Nitpick comments:
In `@src/features/editor/components/editor.tsx`:
- Around line 183-187: Extract the dialog open-state selectors and conditional
mounts out of LoadedEditor into a new leaf component (e.g., EditorDialogHost)
that encapsulates the useClearKeyframesDialogStore, useTtsGenerateDialogStore,
and useProjectMediaMatchDialogStore selectors (including the projectId-specific
condition), and renders the dialog components; then replace the current selector
block in LoadedEditor (the clearKeyframesDialogOpen, ttsGenerateDialogOpen,
projectMediaMatchDialogOpen logic) with a single <EditorDialogHost ...>
invocation (pass projectId or any needed props) so LoadedEditor only handles
layout/load/save concerns while EditorDialogHost owns dialog wiring and mounts.
🪄 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: 3cf123f5-ff1d-41de-8bdd-8e7bc56e4caf
📒 Files selected for processing (6)
src/features/editor/components/editor.tsxsrc/features/media-library/services/file-access.tssrc/features/media-library/services/media-asset-helpers.tssrc/features/timeline/components/timeline-item/use-timeline-item-actions.tssrc/features/timeline/components/timeline-item/use-timeline-item-drop-handlers.tssrc/features/timeline/stores/timeline-persistence.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/features/media-library/services/file-access.ts
- src/features/timeline/stores/timeline-persistence.ts
- Validate raw thumbnail dimensions as finite positive numbers before sanitizing; skip persist entirely when missing/invalid instead of saving synthetic 1×1 thumbnails - Coerce getThumbnailDimensions inputs to finite numbers to prevent NaN/Infinity propagation - Extract EditorDialogHost leaf component to isolate dialog store selectors from LoadedEditor re-renders
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/features/media-library/services/media-asset-helpers.ts (1)
117-119: Add richer business context to the operation event.Line 118 only merges
projectIdandmediaId. For this multi-step operation, include context like file size/type and thumbnail/resolution inputs so success/failure events are diagnostic enough.Suggested patch
const opId = createOperationId(); const event = logger.startEvent('persistGeneratedMediaAsset', opId); - event.merge({ projectId, mediaId: mediaMetadata.id }); + event.merge({ + projectId, + mediaId: mediaMetadata.id, + mimeType: file.type, + fileSizeBytes: file.size, + hasThumbnail: Boolean(thumbnailBlob), + thumbnailWidth: thumbnailWidth ?? null, + thumbnailHeight: thumbnailHeight ?? null, + });As per coding guidelines "Use wide event pattern for multi-step operations:
log.startEvent(name, opId)accumulates context, emits one structured event via.success()/.failure(). UsecreateOperationId()for correlation. Include business context (project ID, item counts, codec, resolution) in events".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/features/media-library/services/media-asset-helpers.ts` around lines 117 - 119, The event started with logger.startEvent('persistGeneratedMediaAsset', opId) only merges projectId and mediaId (mediaMetadata.id); enhance the merged context to include business details such as mediaMetadata.size (file size), mediaMetadata.mimeType or codec, primary resolution (width/height), thumbnail specs (thumbnail count/sizes or thumbnailInputs), and any processing flags/inputs used for generation so that subsequent event.success() / event.failure() calls are diagnostic; update the event.merge call for persistGeneratedMediaAsset to include these fields (projectId, mediaId, file size, file type/codec, resolution, thumbnail/resolution inputs, and any relevant operation inputs) to follow the wide-event pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/features/media-library/services/media-asset-helpers.ts`:
- Around line 117-119: The event started with
logger.startEvent('persistGeneratedMediaAsset', opId) only merges projectId and
mediaId (mediaMetadata.id); enhance the merged context to include business
details such as mediaMetadata.size (file size), mediaMetadata.mimeType or codec,
primary resolution (width/height), thumbnail specs (thumbnail count/sizes or
thumbnailInputs), and any processing flags/inputs used for generation so that
subsequent event.success() / event.failure() calls are diagnostic; update the
event.merge call for persistGeneratedMediaAsset to include these fields
(projectId, mediaId, file size, file type/codec, resolution,
thumbnail/resolution inputs, and any relevant operation inputs) to follow the
wide-event pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2efe70d-4acd-46cc-8644-b6640ea94e2f
📒 Files selected for processing (2)
src/features/editor/components/editor.tsxsrc/features/media-library/services/media-asset-helpers.ts
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
Test plan
npm run test:run)Summary by CodeRabbit
New Features
Performance
Improvements
Tests
Refactor