Optimize timeline waveform rendering and markings #1460
Conversation
WalkthroughThe changes introduce performance optimizations to the timeline editor by implementing viewport-aware waveform virtualization, RAF-based batching for zoom/scroll interactions, time-based sampling for waveform rendering, and dynamic marking calculations. Waveforms now render only visible segments based on canvas width thresholds, and wheel events are coalesced via scheduled RAF callbacks rather than immediate updates. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/desktop/src/routes/editor/Timeline/index.tsx (2)
218-267: RAF-based batching implementation looks solid for coalescing rapid wheel events.The pattern correctly accumulates deltas and schedules a single RAF callback. However, there's a subtle issue:
pendingZoomOriginalways overwrites to the latest origin rather than keeping the first origin of the batch. For zoom operations, the origin point matters - the first touch point in a gesture is typically the intended zoom anchor.Consider preserving the first origin in a batch:
🔎 Suggested improvement
function scheduleZoomUpdate(delta: number, origin: number) { pendingZoomDelta += delta; - pendingZoomOrigin = origin; + if (pendingZoomOrigin === null) { + pendingZoomOrigin = origin; + } if (zoomRafId === null) { zoomRafId = requestAnimationFrame(flushPendingZoom); } }
613-637: Switch fromFortoIndexis appropriate for stable indices, but the array creation pattern can be optimized.Using
Indexis correct here since the array items have no identity - only their indices matter. However,Array.from({ length: markingCount() })creates a new array on every reactive update, which triggers unnecessary re-renders.🔎 Use a number range signal instead
Consider creating a simple range accessor that doesn't allocate:
-<Index each={Array.from({ length: markingCount() })}> - {(_, index) => { +<Index each={new Array(markingCount())}> + {(_, index) => {Or better yet, since
Indextracks by index anyway, you could use a keyedForwith a stable range memo:const markingIndices = createMemo(() => Array.from({ length: markingCount() }, (_, i) => i) ); // Then in JSX: <For each={markingIndices()}>This memoizes the array creation, avoiding allocations on every render cycle.
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (2)
72-82: TheamplitudeAtfunction uses a hardcoded sample rate assumption.The index calculation
Math.floor(time * 10)assumes waveform data is sampled at 10 samples per second (matchingWAVEFORM_SAMPLE_STEP = 0.1). This coupling is implicit and could break if the sample step changes.🔎 Consider making the sample rate explicit
+ const samplesPerSecond = 1 / WAVEFORM_SAMPLE_STEP; + const amplitudeAt = (time: number) => { - const index = Math.floor(time * 10); + const index = Math.floor(time * samplesPerSecond); const sample = waveform[index];
912-927: Same array allocation pattern as Timeline/index.tsx - consider memoizing.Similar to the
TimelineMarkingscomponent,Array.from({ length: markingParams().count })creates a new array on each reactive update.🔎 Memoize the marking indices
const markingIndices = createMemo(() => Array.from({ length: markingParams().count }, (_, i) => i) ); // Then in JSX: <For each={markingIndices()}>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(7 hunks)apps/desktop/src/routes/editor/Timeline/index.tsx(4 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use React Query hooks with Server Actions for mutations and perform precise cache updates using setQueryData/setQueriesData instead of broad invalidations
Files:
apps/desktop/src/routes/editor/Timeline/index.tsxapps/desktop/src/routes/editor/Timeline/ClipTrack.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use strict TypeScript; avoid any type; leverage shared types from @cap/* packages
Follow camelCase naming for variables and functions; PascalCase for components; hooks must start with 'use' prefix
Use Biome for linting and formatting; match existing formatting conventions in the codebase
Use Tailwind CSS for styling in web components; stay consistent with spacing and tokens
Use static skeletons for loading states that mirror content; avoid bouncing animations
Memoize expensive work, code-split naturally, and use Next/Image for remote assets
**/*.{ts,tsx,js,jsx}: Use 2-space indent for TypeScript files; format with Biome usingpnpm format
Use Biome for code formatting and linting; runpnpm formatregularly
Use kebab-case for file names (e.g.,user-menu.tsx); use PascalCase for components
Files:
apps/desktop/src/routes/editor/Timeline/index.tsxapps/desktop/src/routes/editor/Timeline/ClipTrack.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Never add any form of comments to code (single-line //, multi-line /* /, JSDoc /* */, or any other comment syntax); code must be self-explanatory through naming, types, and structure
Directory naming must use lowercase-dashed convention
Files:
apps/desktop/src/routes/editor/Timeline/index.tsxapps/desktop/src/routes/editor/Timeline/ClipTrack.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (AGENTS.md)
Never add comments to code (
//,/* */,///,//!,#, etc.); code must be self-explanatory through naming, types, and structure
Files:
apps/desktop/src/routes/editor/Timeline/index.tsxapps/desktop/src/routes/editor/Timeline/ClipTrack.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to apps/web/**/*.tsx : Minimize useEffect usage in React components; compute during render, handle logic in event handlers, and ensure cleanups for subscriptions/timers
Applied to files:
apps/desktop/src/routes/editor/Timeline/index.tsx
📚 Learning: 2025-12-07T14:29:19.180Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-07T14:29:19.180Z
Learning: Applies to apps/desktop/**/*.ts : Use tanstack/solid-query for server state management in SolidJS components
Applied to files:
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx
🧬 Code graph analysis (1)
apps/desktop/src/routes/editor/Timeline/index.tsx (1)
apps/desktop/src/routes/editor/utils.ts (1)
formatTime(1-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
apps/desktop/src/routes/editor/Timeline/index.tsx (2)
448-464: Wheel event handling correctly routes to RAF-based schedulers.The logic properly distinguishes between ctrl+wheel (zoom) and regular wheel (scroll), with platform-specific handling for macOS. The delta calculations are correctly passed to the scheduling functions.
600-606: Marking calculation functions are clean and correctly compute time-based positions.The
markingCount,markingOffset, andgetMarkingTimefunctions properly calculate the number of markings needed and their time positions based on the current viewport transform.apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (6)
47-65: Time-based sampling logic is well-designed with appropriate bounds.The
numSamplescalculation correctly bounds between a minimum of 50, the target samples, the native sample count, and the global maximum. This prevents both under-sampling and excessive computation.
129-135: RAF tracking variables are correctly scoped for lifecycle management.The
rafIdandlastRenderKeyvariables properly track pending animation frames and prevent redundant renders.
157-203: Virtualization logic correctly calculates visible segment bounds.The viewport intersection calculation properly determines which portion of the segment is visible and computes appropriate canvas dimensions. The early return for non-visible segments (line 169-174) is a good optimization.
254-270: The createEffect has many dependencies but no explicit dependency array - this is intentional for SolidJS.The effect correctly tracks all reactive values that should trigger a re-render. The RAF scheduling inside the effect properly cancels any pending frame before scheduling a new one, preventing frame accumulation.
640-640: PassingsegmentOffsettoWaveformCanvascorrectly enables viewport-aware virtualization.The
prevDuration()value provides the necessary context for the canvas to calculate its position relative to the overall timeline viewport.
896-909: Marking calculation correctly scopes visibility to the segment bounds.The
markingParamsfunction properly calculates the visible range within the segment context, andgetMarkingTimereturns absolute times for each marking index.
| onMount(() => { | ||
| setTimeout(() => { | ||
| lastRenderKey = ""; | ||
| if (rafId !== null) { | ||
| cancelAnimationFrame(rafId); | ||
| } | ||
| rafId = requestAnimationFrame(renderCanvas); | ||
| }, 300); | ||
| }); |
There was a problem hiding this comment.
The setTimeout in onMount introduces a potential race condition.
The 300ms delay with lastRenderKey = "" forces a re-render after mount, but if the component unmounts within that window, the timeout callback will still execute. While onCleanup cancels the RAF, it doesn't cancel this timeout.
🔎 Clear the timeout on cleanup
+ let mountTimeoutId: number | null = null;
+
onMount(() => {
- setTimeout(() => {
+ mountTimeoutId = window.setTimeout(() => {
lastRenderKey = "";
if (rafId !== null) {
cancelAnimationFrame(rafId);
}
rafId = requestAnimationFrame(renderCanvas);
}, 300);
});
onCleanup(() => {
+ if (mountTimeoutId !== null) {
+ clearTimeout(mountTimeoutId);
+ }
if (rafId !== null) {
cancelAnimationFrame(rafId);
}
});🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx around lines 272 to
280, the onMount uses setTimeout which can fire after the component unmounts
causing a race; store the timeout id (e.g. let mountTimer: number | null),
assign mountTimer = window.setTimeout(...), and in onCleanup call
clearTimeout(mountTimer) (and set mountTimer = null) in addition to the existing
cancelAnimationFrame logic so the delayed callback cannot run after unmount.
The changes focus on virtualizing waveform rendering to handle large segments efficiently, batching timeline zoom and scroll updates for smoother user experience, and optimizing how timeline and segment markings are generated and displayed.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.