desktop: set initial zoom level #1210
Conversation
WalkthroughThe Timeline component's onMount lifecycle now includes a runtime check that enforces a minimum segment width of 80 pixels by adjusting the zoom level. If timeline bounds are available, it computes desired zoom based on segment count; if not, it retries after 10 milliseconds. Changes
Sequence DiagramsequenceDiagram
participant React as React Lifecycle
participant Mount as Timeline.onMount
participant DOM as DOM/timelineBounds
participant State as Transform State
React->>Mount: Component mounting
Mount->>Mount: Call checkBounds()
Mount->>DOM: Check if timelineBounds.width exists
alt Width available and > 0
DOM-->>Mount: width > 0
Mount->>Mount: Compute desiredZoom<br/>(80px min per segment)
alt Current zoom exceeds desired
Mount->>State: Update transform.zoom
State-->>Mount: Zoom adjusted
else Current zoom acceptable
Mount-->>Mount: No adjustment needed
end
else Width not available yet
DOM-->>Mount: width = 0 or undefined
Mount->>Mount: Schedule retry<br/>(setTimeout 10ms)
Mount->>Mount: Call checkBounds() again
end
Mount-->>React: onMount complete
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The change is localized to a single file with straightforward conditional logic for zoom adjustment and retry scheduling. Review focuses on validating the minimum segment width calculation, timeout handling, and potential edge cases with DOM readiness. 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 (1)
apps/desktop/src/routes/editor/Timeline/index.tsx (1)
58-60: Calculation doesn't consider segment count as mentioned in summary.The AI-generated summary states that the code "computes desired zoom based on segment count," but the implementation only uses a fixed
minSegmentPixels = 80constant. This means:
- All segments will be 80 pixels wide regardless of how many segments exist
- For recordings with many segments, the timeline could become excessively wide
- For recordings with few segments, the zoom level might not be optimal
Consider incorporating the actual segment count into the calculation:
- const minSegmentPixels = 80; - const secondsPerPixel = 1 / minSegmentPixels; - const desiredZoom = timelineBounds.width * secondsPerPixel; + const MIN_SEGMENT_PIXELS = 80; + const segmentCount = project.timeline?.zoomSegments?.length || 1; + const desiredZoom = (timelineBounds.width / segmentCount) * (1 / MIN_SEGMENT_PIXELS);Also, extract the magic number
80as a module-level constant for better maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/Timeline/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.Use strict TypeScript and avoid any; leverage shared types
Files:
apps/desktop/src/routes/editor/Timeline/index.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/editor/Timeline/index.tsx
apps/desktop/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/src/**/*.{ts,tsx}: Desktop icons are auto-imported (unplugin-icons); do not import icons manually
Desktop IPC: Call generated tauri_specta commands/events; listen to generated events and use typed interfaces
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/editor/Timeline/index.tsx
⏰ 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). (4)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (javascript-typescript)
| const checkBounds = () => { | ||
| if (timelineBounds.width && timelineBounds.width > 0) { | ||
| const minSegmentPixels = 80; | ||
| const secondsPerPixel = 1 / minSegmentPixels; | ||
| const desiredZoom = timelineBounds.width * secondsPerPixel; | ||
|
|
||
| if (transform().zoom > desiredZoom) { | ||
| transform().updateZoom(desiredZoom, 0); | ||
| } | ||
| } else { | ||
| setTimeout(checkBounds, 10); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Add safeguards to prevent infinite retry loop.
The checkBounds function will retry indefinitely if timelineBounds.width never becomes available (e.g., if the element is hidden or removed). Additionally, if the component unmounts while a retry is scheduled, the timeout will continue executing, potentially causing a memory leak.
Consider adding:
- A maximum retry count or timeout duration
- A cleanup mechanism in
onCleanupto cancel pending timeouts - Store the timeout ID to enable cancellation
+ const [checkBoundsTimeoutId, setCheckBoundsTimeoutId] = createSignal<number | null>(null);
onMount(() => {
if (!project.timeline) {
const resume = projectHistory.pause();
setProject("timeline", {
segments: [
{
timescale: 1,
start: 0,
end: duration(),
},
],
});
resume();
}
+ let retryCount = 0;
+ const MAX_RETRIES = 100; // 1 second total
+
const checkBounds = () => {
if (timelineBounds.width && timelineBounds.width > 0) {
const minSegmentPixels = 80;
const secondsPerPixel = 1 / minSegmentPixels;
const desiredZoom = timelineBounds.width * secondsPerPixel;
if (transform().zoom > desiredZoom) {
transform().updateZoom(desiredZoom, 0);
}
- } else {
- setTimeout(checkBounds, 10);
+ } else if (retryCount < MAX_RETRIES) {
+ retryCount++;
+ const timeoutId = setTimeout(checkBounds, 10);
+ setCheckBoundsTimeoutId(timeoutId);
}
};
checkBounds();
+
+ onCleanup(() => {
+ const timeoutId = checkBoundsTimeoutId();
+ if (timeoutId !== null) {
+ clearTimeout(timeoutId);
+ }
+ });
});🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/index.tsx around lines 56 to 68,
checkBounds currently schedules infinite retries via setTimeout and never
cancels them if the component unmounts; add a max retry count (or deadline) and
track the timeout ID so retries stop after N attempts or a timeout duration,
store that ID in a ref/local variable, and clear it in the component's cleanup
(onCleanup) to avoid leaks; also guard scheduling so you only call setTimeout
when the component is still mounted (or use an isMounted flag) and prevent
further retries once width becomes available or max retries are reached.
1 participant
This PR: If you open or record a long or short recording, the zoom level will automatically be set on mount (initially) just enough so you can see your zoom segment well enough, whether adding or already for existing ones.
This deals with the pain of having to manually zoom in initially so you can add and control your newly added zoom segment properly or modify existing ones.
Summary by CodeRabbit