Conversation
…lat background Convert full-height major tick lines to short bottom-anchored marks, flatten the background gradient to a single tone, drop the edge vignettes, and soften the label shadow for a calmer ruler that doesn't compete with the playhead/clip grid.
planTrackMediaDropPlacements always returned a freshly-cloned tracks array, so applyResolvedTimelineDrop always saw a "change" and called setTracks; setTracks then ran every track through normalizeTrack (which always allocates), giving every track a new reference and breaking TimelineTrack's identity-based memo. Result: every track row re-rendered on every drop. - planTrackMediaDropPlacements now returns the original tracks reference when no track was added/changed, so the redundant setTracks is skipped. - setTracks now preserves per-track object identity (and the array reference) for unchanged tracks, which also benefits mute/rename/ reorder/resize callers.
…a new track Moving a clip across tracks remounts its TimelineItem (each track renders its own keyed items), which re-initializes the waveform. Two gaps caused a loading-skeleton flash on remount: - The waveform memory cache budget was 20MB, and SizedAccessedMemoryCache rejects any item larger than the budget. A long clip's full-resolution peaks (1000 samples/sec stereo ~= 28.8MB/hour) exceeded that, so it was never cached and every (re)mount triggered an async OPFS reload + shimmer. Raised the budget to 128MB so realistic long-clip waveforms stay resident and remounts hit the sync cache. - ClipWaveform measured its height in a post-paint useEffect, so a remount showed a height=0 skeleton frame even with cached peaks. Moved the measurement to useLayoutEffect so it commits before paint.
The 128MB budget bump fixed the reported case but left the underlying footgun: SizedAccessedMemoryCache silently dropped any entry larger than the budget, so a clip whose full-resolution peaks exceed the budget (~4.5h stereo) would still never be cached and would reload with a skeleton flash on every remount. Remove the oversized-reject guard so the existing LRU eviction stores the entry once the rest of the working set is evicted (briefly exceeding the budget, reclaimed on the next add). This makes the waveform fix size-independent rather than dependent on the magic 128MB number. Adds unit tests for LRU eviction, size accounting, and oversized retention.
The timeline previously held full-resolution peaks (1000 samples/sec) in memory for every visible audio clip, so a long clip pinned tens of MB and display depended on the full-res cache staying resident. Render from the persisted OPFS multi-resolution levels instead, choosing the coarsest level that still has >=1 sample per pixel for the current zoom (chooseDisplayLevelForZoom). When zoomed out this is a small fraction of the data; full detail (level 0) is used only when zoomed in past ~200 px/s, matching the old look. A small dedicated level cache makes remounts (e.g. dragging a clip to another track) and zoom-back instant with no skeleton flash. Falls back to the full-resolution generate/load path while a waveform is still generating or has no persisted multi-resolution file. The wider, density-aware level selection avoids the blocky waveforms a naive zoom->level mapping (chooseLevelForZoom) produced when zoomed in.
The density-aware level selection still picked the 200/sec level in the 50–200 px/s range, which visibly lost transient detail vs the old always-1000/sec rendering. Render full resolution (level 0) at any editing zoom (>=16 px/s) and only step down to a coarse level at overview zoom, where the whole clip is on screen, the detail is imperceptible, and the memory savings for long clips are largest.
Tile tick geometry is drawn relative to canvasHeight, so a ruler-height change (e.g. a new editor-density preset) would otherwise reuse stale cached tiles.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds zoom-aware waveform display-level caching and selection, updates useWaveform to prefer persisted downsampled levels via pixelsPerSecond, integrates pre-paint height measurement in ClipWaveform, refines TimelineMarkers rendering and caching keys, and preserves track reference identity to avoid unnecessary re-renders. ChangesWaveform zoom rendering and optimizations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/timeline/components/timeline-markers.tsx`:
- Around line 463-466: The cache key used for tile redraw short-circuiting
(built as cacheKey from quantizedPPS, fps, and canvasHeight) ignores the ruler's
rendered width, so canvas.dataset.ck can falsely indicate a match when only
display width changes; update the key construction (the cacheKey string) to
include displayWidth (or the last tile's effective width) and ensure the same
augmented key is written to canvas.dataset.ck so width-only changes force a
redraw (refer to cacheKey, quantizedPPS.toFixed, canvasHeight, fps, and
canvas.dataset.ck in timeline-markers.tsx).
In `@src/features/timeline/hooks/use-waveform.ts`:
- Around line 144-157: The async display-level load guard only checks mediaId,
so slow responses for prior zooms can still call setDisplayLevel; capture the
requested level index/token when starting the request (e.g. const
requestLevelIndex = levelIndex or create a combined requestToken) and compare it
against a persistent ref that tracks the latest requested level (add
lastLevelIndexRef or lastDisplayRequestRef similar to lastMediaIdRef) inside the
then() callback; if cancelled or lastMediaIdRef.current !== requestMediaId or
lastLevelIndexRef.current !== requestLevelIndex then return, otherwise call
setDisplayLevel(...) and setLevelProbed(true). Ensure you update the ref
whenever a new request is started.
In `@src/features/timeline/services/waveform-cache.ts`:
- Around line 307-325: The async load in request() can cache a level after
clearMedia()/clearAll() runs, resurrecting deleted media; add a per-media
invalidation token/version (e.g., a map keyed by mediaId incremented by
clearMedia/clearAll) and read the current token before calling
waveformOPFSStorage.getLevel(), capture it, then before calling
this.levelCache.add(key, result) verify the token still matches (drop the result
if it changed) so late OPFS completions don’t reinsert stale levels; update
clearMedia()/clearAll() to bump the token for the affected media.
🪄 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: 473acb96-32fb-4f2b-bcdb-bb6f37d1a8b4
📒 Files selected for processing (9)
src/features/timeline/components/clip-waveform/index.tsxsrc/features/timeline/components/timeline-markers.tsxsrc/features/timeline/hooks/use-waveform.tssrc/features/timeline/services/sized-accessed-memory-cache.test.tssrc/features/timeline/services/sized-accessed-memory-cache.tssrc/features/timeline/services/waveform-cache.tssrc/features/timeline/services/waveform-opfs-storage.tssrc/features/timeline/stores/items-store.tssrc/features/timeline/utils/track-media-drop.ts
Comment on lines
+144
to
+157
| let cancelled = false | ||
| const requestMediaId = mediaId | ||
| waveformCache | ||
| .getDisplayLevel(mediaId, levelIndex) | ||
| .then((level) => { | ||
| if (cancelled || lastMediaIdRef.current !== requestMediaId) return | ||
| if (level) { | ||
| setDisplayLevel(level) | ||
| } | ||
| // Mark probed regardless: a null result means no persisted level, so | ||
| // the generation effect should take over. Keep any previously-shown | ||
| // level on null so a transient miss doesn't blank the clip. | ||
| setLevelProbed(true) | ||
| }) |
Contributor
There was a problem hiding this comment.
Ignore stale display-level loads after zoom changes.
This guard only checks mediaId, so a slower request for an older levelIndex can still call setDisplayLevel after the user has zoomed again. That leaves the clip rendering the wrong resolution until another threshold change happens. Track the latest requested level/token and discard late completions that no longer match it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/features/timeline/hooks/use-waveform.ts` around lines 144 - 157, The
async display-level load guard only checks mediaId, so slow responses for prior
zooms can still call setDisplayLevel; capture the requested level index/token
when starting the request (e.g. const requestLevelIndex = levelIndex or create a
combined requestToken) and compare it against a persistent ref that tracks the
latest requested level (add lastLevelIndexRef or lastDisplayRequestRef similar
to lastMediaIdRef) inside the then() callback; if cancelled or
lastMediaIdRef.current !== requestMediaId or lastLevelIndexRef.current !==
requestLevelIndex then return, otherwise call setDisplayLevel(...) and
setLevelProbed(true). Ensure you update the ref whenever a new request is
started.
Contributor
Greptile SummaryThis PR improves timeline waveform rendering and ruler performance by introducing zoom-appropriate OPFS resolution levels, fixing skeleton flash on clip remount, and reducing unnecessary re-renders across several code paths.
Confidence Score: 3/5Generally safe to merge for the UI improvements, but two data-consistency issues in the waveform level cache should be resolved before shipping to production. The new
Important Files Changed
Sequence DiagramsequenceDiagram
participant CW as ClipWaveform
participant UW as useWaveform
participant WC as WaveformCacheService
participant OPFS as WaveformOPFSStorage
participant MC as SizedAccessedMemoryCache
CW->>UW: pixelsPerSecond (zoom)
UW->>UW: chooseDisplayLevelForZoom(pps) → levelIndex
UW->>WC: getDisplayLevelSync(mediaId, levelIndex)
WC->>MC: get(key) → CachedWaveformLevel or null
alt Level in memory cache
WC-->>UW: CachedWaveformLevel (sync)
UW-->>CW: render downsampled peaks immediately
else Not in memory
UW->>WC: getDisplayLevel(mediaId, levelIndex)
WC->>OPFS: getLevel(mediaId, levelIndex)
OPFS-->>WC: Float32Array peaks
WC->>MC: add(key, CachedWaveformLevel)
WC-->>UW: CachedWaveformLevel
UW-->>CW: render downsampled peaks
else OPFS has no file (null)
WC-->>UW: null, setLevelProbed(true)
UW->>WC: getCachedWaveform / getWaveform (full-res generation)
WC-->>UW: CachedWaveform (full-res)
UW-->>CW: render full-res peaks
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+197
to
200
| const needsFullRes = !useLevels || (levelProbed && !displayLevel) | ||
| useEffect(() => { | ||
| if (!enabled) { | ||
| if (!enabled || !needsFullRes) { | ||
| return |
Contributor
There was a problem hiding this comment.
displayLevel suppresses full-res generation when levelIndex changes but OPFS level is unavailable
needsFullRes is false whenever displayLevel is non-null. When levelIndex changes (zoom crosses a threshold), the old level's data stays in displayLevel until the new level loads — intentional for smooth transitions. However, if the async getDisplayLevel for the new levelIndex returns null (OPFS file absent, e.g. cleared between a zoom change and the probe completing), the then branch skips setDisplayLevel, leaving displayLevel holding the stale coarser level. With levelProbed = true and displayLevel non-null, needsFullRes stays false indefinitely, so full-resolution generation is never triggered and the clip is permanently stuck rendering the wrong level's peaks.
In practice the OPFS file stores all levels atomically, so this scenario is unlikely under normal conditions; the concern becomes real if clearMedia races with a zoom transition.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/timeline/hooks/use-waveform.ts
Line: 197-200
Comment:
**Stale `displayLevel` suppresses full-res generation when `levelIndex` changes but OPFS level is unavailable**
`needsFullRes` is `false` whenever `displayLevel` is non-null. When `levelIndex` changes (zoom crosses a threshold), the old level's data stays in `displayLevel` until the new level loads — intentional for smooth transitions. However, if the async `getDisplayLevel` for the new `levelIndex` returns `null` (OPFS file absent, e.g. cleared between a zoom change and the probe completing), the `then` branch skips `setDisplayLevel`, leaving `displayLevel` holding the stale coarser level. With `levelProbed = true` and `displayLevel` non-null, `needsFullRes` stays `false` indefinitely, so full-resolution generation is never triggered and the clip is permanently stuck rendering the wrong level's peaks.
In practice the OPFS file stores all levels atomically, so this scenario is unlikely under normal conditions; the concern becomes real if `clearMedia` races with a zoom transition.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+40
to
+54
| // Memory cache budget — the working-set ceiling for resident waveforms. | ||
| // Full-resolution peaks are ~28.8MB/hour (1000 samples/sec, stereo), so the | ||
| // old 20MB held under an hour total and a single long clip evicted everything | ||
| // else. 128MB keeps several hours of waveform resident so the clips around the | ||
| // viewport stay cached and remounts (e.g. dragging a clip to another track) hit | ||
| // the sync cache instead of reloading with a skeleton flash. | ||
| // Note: SizedAccessedMemoryCache retains entries larger than this budget rather | ||
| // than dropping them, so a single clip longer than ~4.5h is still cached (it | ||
| // just evicts the rest of the working set while resident). | ||
| const MAX_CACHE_SIZE_BYTES = 128 * 1024 * 1024 // 128MB | ||
| // Separate, smaller budget for downsampled display levels (see getDisplayLevel). | ||
| // These are what the timeline renders: a zoom-appropriate resolution level | ||
| // (e.g. 10–50 samples/sec when zoomed out) is a fraction of the full-res peaks, | ||
| // so the working set of visible clips stays tiny regardless of clip length. | ||
| const MAX_LEVEL_CACHE_SIZE_BYTES = 64 * 1024 * 1024 // 64MB |
Contributor
There was a problem hiding this comment.
The combined resident ceiling is now 192 MB for waveform data alone, before any other allocations. On low-memory mobile or mid-range Android browsers the tab may be OOM-killed when several hours of waveform peaks are simultaneously resident. The comment acknowledges the calculation (~28.8 MB/hr stereo) but 128 MB allows ~4.5 hours of full-res peaks, which could be the whole project timeline. Consider whether a lower default (e.g. 64 MB) with graceful degradation to on-demand OPFS reads is more appropriate for constrained devices.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/features/timeline/services/waveform-cache.ts
Line: 40-54
Comment:
**Memory budget jump from 20 MB → 128 MB (plus new 64 MB level cache)**
The combined resident ceiling is now 192 MB for waveform data alone, before any other allocations. On low-memory mobile or mid-range Android browsers the tab may be OOM-killed when several hours of waveform peaks are simultaneously resident. The comment acknowledges the calculation (~28.8 MB/hr stereo) but 128 MB allows ~4.5 hours of full-res peaks, which could be the whole project timeline. Consider whether a lower default (e.g. 64 MB) with graceful degradation to on-demand OPFS reads is more appropriate for constrained devices.
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!
…ale display levels - Guard getDisplayLevel with per-media/global generation tokens so a late OPFS read can't re-insert a level after clearMedia/clearAll - Clear a stale display level on a null probe so full-res generation can take over instead of stranding the clip on the wrong level's peaks - Include per-tile rendered width in the ruler tile cache key so a duration or viewport change at fixed zoom redraws the last partial tile
Contributor
|
Actionable comments posted: 0 |
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
canvasHeightin the ruler tile cache key so re-renders pick up height changesTest plan
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Style
Tests