Refactor new timeline layout to "scene" segments #948
Conversation
Replaces 'layoutSegments' with 'sceneSegments' throughout the codebase, including type and variable renames in the editor and timeline components. Removes LayoutTrack and introduces SceneTrack in the timeline. Updates selection handling and config components to use scene segments instead of layout segments.
|
Caution Review failedThe pull request is closed. WalkthroughRenames timeline "layout" concepts to "scene" across crates and the desktop app, replaces the old layout interpolation with a new scene interpolation module, updates types/fields/imports/UI/and editor actions to use Scene* and Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Editor UI
participant Ctx as EditorContext
participant Proj as Project Timeline
participant Rend as Renderer
participant Scene as InterpolatedScene
UI->>Ctx: deleteSceneSegment(index)
Ctx->>Proj: mutate timeline.sceneSegments
UI->>Rend: request frame(time)
Rend->>Proj: read timeline.scene_segments
Rend->>Scene: SceneSegmentsCursor::new(time, segments)
Rend->>Scene: InterpolatedScene::new(cursor)
Scene-->>Rend: {camera_opacity, screen_opacity, ...}
Rend-->>UI: composed frame (screen & camera passes)
rect rgba(200,230,255,0.20)
note over Scene: New scene-based interpolation replaces former layout logic
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (15)
apps/desktop/src/utils/tauri.ts (3)
462-468: Value import for Channel is acceptable here.TAURI_CHANNEL is imported as a value. That’s fine for generated code and avoids type-only pitfalls with tooling. Just noting that bundlers may retain the import even if only used in types; do not hand-edit this file to “optimize” it.
428-443: Scene segments migration complete; address remaining null-safety spots.Thanks for adding
SceneMode/SceneSegmentand wiring upTimelineConfiguration.sceneSegments. A quick scan shows:
No more references to the old
LayoutSegment/layoutSegmentstypes (the ripgrep search returned zero hits)All UI now reads from
sceneSegments…but there are a couple of unguarded spots:• In
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsxaround line 203:const sceneSegments = () => project.timeline!.sceneSegments!;This uses non-null assertions on both
timelineandsceneSegments; it’ll throw for legacy projects withoutsceneSegments.• Elsewhere you default to
project.timeline?.sceneSegments || [], which is safe—but the!-based accessor bypasses that guard.Please replace the non-null assertions with a safe fallback or early-return. For example:
- const sceneSegments = () => project.timeline!.sceneSegments!; + const sceneSegments = () => project.timeline?.sceneSegments ?? [];That way, any legacy project (where
sceneSegmentsis undefined) will fall back to an empty array instead of crashing.
271-313: Switch all raw Tauri event imports/calls to the generatedevents.*APIWe’ve added a typed
eventsobject inapps/desktop/src/utils/tauri.tsvia__makeEvents__, so we should no longer import or call the low-level@tauri-apps/api/eventAPI directly. Please refactor the following locations to useevents.<eventName>.listen(...)orevents.<eventName>.emit(...)instead:• apps/desktop/src/utils/auth.ts
– Remove
ts import { listen } from "@tauri-apps/api/event"; … const unlisten = listen("authentication-invalid", handler);
– Replace with
ts import { events } from "src/utils/tauri"; … const unlisten = events.authenticationInvalid.listen(handler);• apps/desktop/src/routes/target-select-overlay.tsx
– Remove
ts import { emit } from "@tauri-apps/api/event"; … emit("target-under-cursor", payload);
– Replace with
ts import { events } from "src/utils/tauri"; … events.targetUnderCursor.emit(payload);• apps/desktop/src/routes/(window-chrome)/new-main/index.tsx
– Remove
ts import { listen } from "@tauri-apps/api/event"; … const cleanup = listen("start-sign-in", () => { … });
– Replace with
ts import { events } from "src/utils/tauri"; … const cleanup = events.startSignIn.listen(() => { … });• apps/desktop/src/routes/(window-chrome)/upgrade.tsx
– Similarly swap outimport { listen }…+ rawlisten("…")calls for the correspondingeventsAPI.• apps/desktop/src/utils/titlebar-state.ts
– Instead of
ts import type { UnlistenFn } from "@tauri-apps/api/event";
– Import the unlisten type from your generated API (e.g.import type { UnlistenFn } from "src/utils/tauri";) so handlers remain typed alongside the rest of your IPC.Using the generated
events.*API everywhere ensures full type-safety and consistency across the desktop frontend.apps/desktop/src/routes/editor/context.ts (1)
149-161: Harden deleteSceneSegment with bounds checks (and keep invariants safe)We splice without validating
segmentIndex. Add a simple in-bounds guard to avoid silent no-ops or unexpected deletes if an invalid index gets through.deleteSceneSegment: (segmentIndex: number) => { batch(() => { setProject( "timeline", - "sceneSegments", + "sceneSegments", produce((s) => { - if (!s) return; - s.splice(segmentIndex, 1); + if (!s) return; + if (!Number.isInteger(segmentIndex) || segmentIndex < 0 || segmentIndex >= s.length) return; + s.splice(segmentIndex, 1); }), ); setEditorState("timeline", "selection", null); }); },apps/desktop/src/routes/editor/Timeline/index.tsx (2)
41-54: InitializesceneSegmentsalongsidesegmentsto avoid scattered undefined checksYou lazily create
sceneSegmentselsewhere; initializing here keeps the timeline shape consistent and removes the need for repeated nullish coalescing.onMount(() => { if (!project.timeline) { const resume = projectHistory.pause(); setProject("timeline", { segments: [ { timescale: 1, start: 0, end: duration(), }, ], + sceneSegments: [], }); resume(); } });
60-74: Consolidate default timeline initialization (addsceneSegmentshere too or dedupe)This second initialization block ensures
zoomSegmentsbut omitssceneSegments. Either addsceneSegments: []here as well or refactor both places into a singleensureTimelineDefaults()helper to avoid duplication.setProject( produce((project) => { project.timeline ??= { segments: [ { start: 0, end: duration(), timescale: 1, }, ], zoomSegments: [], + sceneSegments: [], }; }) );crates/project/src/configuration.rs (1)
471-476: Consider documenting the default for SceneSegment.modeSince mode defaults to SceneMode::Default when omitted in JSON, consider a short doc comment here to avoid confusion for API consumers.
pub struct SceneSegment { pub start: f64, pub end: f64, #[serde(default)] - pub mode: SceneMode, + /// Defaults to `SceneMode::Default` if not provided in project-config.json + pub mode: SceneMode, }crates/rendering/src/lib.rs (2)
606-641: Use scene.screen_opacity/blur to gate screen work, not an unconditional pathRight now display.opacity is driven by scene.screen_opacity, but we still do the work every frame. For camera-only sections this is wasteful. At minimum, gate display/cursor passes on a predicate tied to scene.screen_opacity (or a small epsilon) so we skip when fully hidden.
- CompositeVideoFrameUniforms { + let display_uniforms = CompositeVideoFrameUniforms { ... - motion_blur_amount: (motion_blur_amount + scene.screen_blur as f32 * 0.8).min(1.0), + motion_blur_amount: (motion_blur_amount + scene.screen_blur as f32 * 0.8).min(1.0), ... - opacity: scene.screen_opacity as f32, + opacity: scene.screen_opacity as f32, _padding: [0.0; 3], - } + };And below in the render path (see Lines 1012-1031), use
scene.screen_opacity > 0.01instead ofshould_render_screen()(which currently always returns true; see separate comment in scene.rs).
752-813: Camera-only layer math looks solid; minor micro-optimizations availableTarget size fills the output and crop is computed to preserve aspect. Opacity/blur are tied to scene.* fields. If desired, we can early-return None when
scene.camera_only_transition_opacity()is ~0 to skip building uniforms entirely.- let camera_only = options + let camera_only = (scene.camera_only_transition_opacity() > 0.01) + .then_some(()) + .and(options .camera_size .filter(|_| !project.camera.hide && scene.is_transitioning_camera_only()) .map(|camera_size| { ... }));apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (4)
58-79: Type mode precisely using shared SceneMode instead of stringAvoid stringly-typed mode. Use the generated SceneMode union to align with backend and prevent typos.
- const getSceneIcon = (mode: string | undefined) => { + import type { SceneMode } from "~/utils/tauri"; + const getSceneIcon = (mode: SceneMode | undefined) => { switch (mode) { case "cameraOnly": return <IconLucideVideo class="size-3.5" />; case "hideCamera": return <IconLucideEyeOff class="size-3.5" />; default: return <IconLucideMonitor class="size-3.5" />; } }; - const getSceneLabel = (mode: string | undefined) => { + const getSceneLabel = (mode: SceneMode | undefined) => { switch (mode) { case "cameraOnly": return "Camera Only"; case "hideCamera": return "Hide Camera"; default: return "Default"; } };
308-347: Unify minimum segment duration logic (start handle uses 1s, elsewhere 0.5s)You use a 0.5s threshold in hover logic but enforce a 1s minimum in resizing. Define a single MIN_SEGMENT_DURATION (e.g., 0.5) to keep UX consistent.
- const maxValue = segment.end - 1; + const MIN_SEGMENT_DURATION = 0.5; + const maxValue = segment.end - MIN_SEGMENT_DURATION;
415-449: Same: end handle enforces 1s minimum; align with a shared constantMatch the start-handle change for consistency.
- const minValue = segment.start + 1; + const MIN_SEGMENT_DURATION = 0.5; + const minValue = segment.start + MIN_SEGMENT_DURATION;
273-283: Selection resolution by value can be unstable after editsYou compute selection by matching start/end pairs, then compare with selection.index. After edits and resorting, equality by float values can fail due to precision. Consider using the iteration index
i()directly when setting selection, or introduce stable IDs on segments.Would you like me to add a stable id to SceneSegment in the project configuration and plumb it through the UI to remove index-by-float comparisons?
crates/rendering/src/scene.rs (2)
66-82: Remove unnecessary .clone() on SceneMode (it’s Copy)Clippy flags multiple clone_on_copy warnings. Replace
.clone()with direct copies to silence warnings and reduce noise.- scene_mode: mode.clone(), + scene_mode: mode, transition_progress: 1.0, - from_mode: mode.clone(), + from_mode: mode, - to_mode: mode, + to_mode: mode,- return InterpolatedScene::from_single_mode(segment.mode.clone()); + return InterpolatedScene::from_single_mode(segment.mode);- prev_seg.mode.clone() + prev_seg.mode- segment.mode.clone(), + segment.mode,- (segment.mode.clone(), segment.mode.clone(), 1.0) + (segment.mode, segment.mode, 1.0)- scene_mode: if transition_progress > 0.5 { - next_mode.clone() + scene_mode: if transition_progress > 0.5 { + next_mode } else { - current_mode.clone() + current_mode },Also applies to: 273-277, 103-116, 131-131
287-294: Consider distinct scene values for CameraOnly (e.g., lower screen_opacity)Right now
get_scene_valuesreturns(1.0, 1.0, 1.0)even for CameraOnly, meaning the screen is fully “visible” and only covered by the camera_only pass. If you want the screen to truly disappear in CameraOnly, setscreen_opacityto 0.0 for that mode.- SceneMode::Default => (1.0, 1.0, 1.0), - SceneMode::CameraOnly => (1.0, 1.0, 1.0), - SceneMode::HideCamera => (0.0, 1.0, 1.0), + SceneMode::Default => (1.0, 1.0, 1.0), // camera visible, screen visible + SceneMode::CameraOnly => (1.0, 0.0, 1.0), // camera visible, screen hidden + SceneMode::HideCamera => (0.0, 1.0, 1.0), // camera hidden, screen visibleIf you adopt this, ensure the gating/opacity logic in lib.rs is consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
apps/desktop/src-tauri/src/recording.rs(1 hunks)apps/desktop/src/routes/(window-chrome)/(main).tsx(3 hunks)apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx(0 hunks)apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx(1 hunks)apps/desktop/src/routes/editor/Timeline/index.tsx(1 hunks)apps/desktop/src/routes/editor/context.ts(2 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)crates/project/src/configuration.rs(3 hunks)crates/rendering/src/layout.rs(0 hunks)crates/rendering/src/lib.rs(11 hunks)crates/rendering/src/scene.rs(1 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx
- crates/rendering/src/layout.rs
🧰 Additional context used
📓 Path-based instructions (5)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/(window-chrome)/(main).tsxapps/desktop/src/routes/editor/context.tsapps/desktop/src/routes/editor/Timeline/SceneTrack.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/editor/Timeline/index.tsx
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable
Files:
apps/desktop/src/routes/(window-chrome)/(main).tsxapps/desktop/src/routes/editor/context.tspackages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/editor/Timeline/SceneTrack.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/editor/Timeline/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/desktop/src/routes/(window-chrome)/(main).tsxapps/desktop/src/routes/editor/context.tspackages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/editor/Timeline/SceneTrack.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/editor/Timeline/index.tsx
apps/desktop/src-tauri/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
Use tauri_specta on the Rust side for IPC (typed commands/events) and emit events via generated types
Files:
apps/desktop/src-tauri/src/recording.rs
**/tauri.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated IPC bindings file: tauri.ts
Files:
apps/desktop/src/utils/tauri.ts
🧠 Learnings (2)
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Applied to files:
packages/ui-solid/src/auto-imports.d.ts
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Applied to files:
apps/desktop/src/utils/tauri.ts
🧬 Code graph analysis (6)
crates/project/src/configuration.rs (1)
apps/desktop/src/utils/tauri.ts (2)
SceneMode(428-428)SceneSegment(429-429)
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (1)
apps/desktop/src/routes/editor/Timeline/Track.tsx (4)
TrackRoot(12-26)SegmentRoot(49-87)SegmentHandle(103-125)SegmentContent(89-101)
apps/desktop/src-tauri/src/recording.rs (1)
crates/rendering/src/lib.rs (6)
new(63-133)new(298-335)new(516-827)new(843-848)new(895-905)new(1045-1075)
crates/rendering/src/scene.rs (2)
apps/desktop/src/utils/tauri.ts (2)
SceneMode(428-428)SceneSegment(429-429)crates/rendering/src/lib.rs (6)
new(63-133)new(298-335)new(516-827)new(843-848)new(895-905)new(1045-1075)
apps/desktop/src/utils/tauri.ts (4)
apps/desktop/src-tauri/src/windows.rs (1)
id(664-690)apps/desktop/src-tauri/src/recording.rs (3)
id(147-152)inputs(65-70)mode(125-130)apps/desktop/src/routes/editor/Header.tsx (1)
ExportEstimates(40-44)apps/desktop/src/store/captions.ts (1)
CaptionSettings(12-25)
apps/desktop/src/routes/editor/Timeline/index.tsx (3)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (2)
ZoomSegmentDragState(26-29)ZoomTrack(31-521)apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (2)
SceneSegmentDragState(27-30)SceneTrack(32-479)apps/desktop/src/routes/editor/Timeline/context.ts (1)
TimelineContextProvider(14-36)
🪛 GitHub Check: Clippy
crates/rendering/src/scene.rs
[warning] 131-131: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:131:48
|
131 | (segment.mode.clone(), segment.mode.clone(), 1.0)
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 131-131: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:131:26
|
131 | (segment.mode.clone(), segment.mode.clone(), 1.0)
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 115-115: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:115:21
|
115 | segment.mode.clone(),
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 107-107: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:107:25
|
107 | prev_seg.mode.clone()
| ^^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: prev_seg.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 103-103: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:103:68
|
103 | return InterpolatedScene::from_single_mode(segment.mode.clone());
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 76-76: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:76:24
|
76 | from_mode: mode.clone(),
| ^^^^^^^^^^^^ help: try removing the clone call: mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 74-74: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:74:25
|
74 | scene_mode: mode.clone(),
| ^^^^^^^^^^^^ help: try removing the clone call: mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
= note: #[warn(clippy::clone_on_copy)] on by default
⏰ 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). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (15)
apps/desktop/src/utils/tauri.ts (7)
2-2: Auto-generated file — do not hand-edit. Confirm regeneration source.Per coding guidelines, never edit apps/desktop/src/utils/tauri.ts directly. Please confirm these diffs were produced by re-running tauri-specta after backend/type changes rather than manual edits.
Would you confirm the generator command you ran (e.g., cargo tauri specta or equivalent) and that no manual edits were made to this file?
128-133: Tuple return type parentheses are a no-op (safe change).Promise<[string, T][]> vs Promise<([string, T])[]> are equivalent; no functional or typing impact. Existing destructuring like for (const [path, meta] of listRecordings) continues to work.
490-493: Callable event properties typing: semantics unchanged, stronger inference.The type change to a callable signature property preserves both forms events.foo.listen(cb) and events.foo(window).listen(cb). Good tightening without runtime impact.
345-348: Captions types match the app store shape.CaptionSettings here matches apps/desktop/src/store/captions.ts (fields and types), which should prevent drift in caption settings serialization.
59-61:getCurrentRecordingWrapper Unwrapped inqueries.ts— No Action RequiredThe only usage of
commands.getCurrentRecording()inapps/desktop/src/utils/queries.tsunwraps theJsonValuetuple viaqueryFn: () => commands.getCurrentRecording().then(d => d[0])so no raw
[value]tuple ever propagates to call sites. There are no other unwrapped calls in the codebase.
435-435: No changes needed: CaptureArea payload shape matches Rust definitionThe TypeScript
ShowCapWindowunion defines{ CaptureArea: { screen_id: DisplayId } }and the Rust enum in
apps/desktop/src-tauri/src/windows.rsdeclares theCaptureAreavariant with the same field name:// … around line 681 in windows.rs ShowCapWindow::CaptureArea { screen_id: screen_id.clone(), … } … ShowCapWindow::CaptureArea { .. } => CapWindowId::CaptureArea,Since both sides use
screen_id: DisplayId, the payload shape is consistent and no changes are required.
62-64: No action required: exportVideo progress channel usage is correctly implementedThe
export.tswrapper consistently importsChannelfrom@tauri-apps/api/coreand instantiates it for every call tocommands.exportVideo. All UI components (recordings-overlay.tsx,ExportDialog.tsx,ShareButton.tsx, etc.) invoke this wrapper rather than callingcommands.exportVideodirectly, ensuring the correctChannel<FramesRendered>is always passed.
– apps/desktop/src/utils/export.ts: importsChannelfrom@tauri-apps/api/coreand wraps callers’ callbacks in anew Channel<FramesRendered>(…).
– No direct usages ofcommands.exportVideowere found outside this wrapper.apps/desktop/src/routes/editor/context.ts (2)
260-260: Selection discriminant rename aligns with the new scene modelThe switch to
{ type: "scene"; index }is consistent with Timeline/SceneTrack and the project schema changes.
149-161: No stale layout-segment references detectedI ran a repo-wide search for the legacy identifiers (
layoutSegments,deleteLayoutSegment,LayoutTrack,LayoutSegment) and found zero occurrences. All layout-segment names have been fully renamed to the new scene-based equivalents.packages/ui-solid/src/auto-imports.d.ts (1)
7-91: Auto-generated types: no manual edits; quoting differences are benignThis file is generator-owned (unplugin-auto-import). The single vs double quotes inconsistency is harmless and shouldn’t be hand-edited. If it keeps flipping in diffs, consider pinning generator version or config to stabilize output.
Note: We validated elsewhere in the desktop app that icons are auto-imported (per our learning for apps/desktop) and not manually imported. ✔
apps/desktop/src-tauri/src/recording.rs (1)
1012-1013: LGTM: timeline initialization now usesscene_segmentsMatches the rendering path (InterpolatedScene/SceneSegmentsCursor) and new ProjectConfiguration field; safe default as empty vec.
crates/project/src/configuration.rs (2)
462-476: Rename looks consistent; serde + specta mapping aligns with desktop typesSceneMode is correctly camelCased and Default is annotated. Matches the TS union in apps/desktop/src/utils/tauri.ts, and SceneSegment.mode has a sensible default via #[serde(default)] so the optional field on the TS side is safe to omit. Looks good.
484-485: Field rename to scene_segments is backwards-compatible with serde defaultAdding #[serde(default)] prevents deserialization failures on older configs missing this field. Good defensive change.
crates/rendering/src/lib.rs (2)
568-575: Switch to SceneSegments wiring looks correctInterpolatedScene::new(SceneSegmentsCursor::new(...t.scene_segments)) is the right substitution for the layout path and correctly handles the absent-timeline case with unwrap_or(&[]).
645-662: Camera sizing blends scene.camera_scale correctly; verify extreme scalesWe multiply the blended zoom by scene.camera_scale. If upstream interpolation ever emits values < 0.2 or > 2, we could end up with odd padding results. Consider clamping
scene.camera_scalein InterpolatedScene or here.- let zoomed_size = zoomed_size * scene.camera_scale as f32; + let zoomed_size = zoomed_size * (scene.camera_scale as f32).clamp(0.25, 3.0);Would you like me to propagate a clamp inside InterpolatedScene instead, so all users benefit?
| screen = _screens()?.find((s: any) => s.id === screenId) ?? _screens()?.[0]; | ||
| } else if (rawOptions.captureTarget.variant === "area") { | ||
| const screenId = rawOptions.captureTarget.screen; | ||
| screen = _screens()?.find((s) => s.id === screenId) ?? _screens()?.[0]; | ||
| screen = _screens()?.find((s: any) => s.id === screenId) ?? _screens()?.[0]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any with concrete types in .find callbacks
Avoid any per our strict TS guideline; you already have solid types available in this file. This keeps narrowing/auto-complete intact and prevents accidental shape mismatches.
Apply:
- screen = _screens()?.find((s: any) => s.id === screenId) ?? _screens()?.[0];
+ screen = _screens()?.find((s: CaptureDisplay) => s.id === screenId) ?? _screens()?.[0];
- screen = _screens()?.find((s: any) => s.id === screenId) ?? _screens()?.[0];
+ screen = _screens()?.find((s: CaptureDisplay) => s.id === screenId) ?? _screens()?.[0];
- win = _windows()?.find((s: any) => s.id === windowId) ?? _windows()?.[0];
+ win = _windows()?.find((w: CaptureWindow) => w.id === windowId) ?? _windows()?.[0];
- micName: () => mics.data?.find((name: any) => name === rawOptions.micName),
+ micName: () => mics.data?.find((name: string) => name === rawOptions.micName),Also applies to: 164-164, 176-176
🤖 Prompt for AI Agents
In apps/desktop/src/routes/(window-chrome)/(main).tsx around lines 150-154 (and
also at lines 164 and 176), the .find callbacks use the unsafe any type; replace
any with the concrete screen type used in this file (e.g., Screen or ScreenInfo
or the exact interface defined nearby) so the callbacks become (s: Screen) =>
s.id === screenId, and similarly update the array indexing types if needed;
ensure imports/types are referenced correctly and run TypeScript to fix any
remaining narrowings.
| createRoot((dispose) => { | ||
| createEventListener(e.currentTarget, "mouseup", () => { | ||
| handleUpdatePlayhead(e); | ||
| if (zoomSegmentDragState.type === "idle") { | ||
| setEditorState("timeline", "selection", null); | ||
| } | ||
| }); | ||
| createEventListener(window, "mouseup", () => { | ||
| dispose(); | ||
| }); | ||
| }); | ||
| }} |
There was a problem hiding this comment.
Don’t clear selection while a scene segment is being dragged
You already gate playhead updates on both zoom and scene drag states; apply the same guard when clearing selection to avoid clearing mid-drag of a scene segment.
- createEventListener(e.currentTarget, "mouseup", () => {
+ createEventListener(e.currentTarget, "mouseup", () => {
handleUpdatePlayhead(e);
- if (zoomSegmentDragState.type === "idle") {
+ if (
+ zoomSegmentDragState.type === "idle" &&
+ sceneSegmentDragState.type === "idle"
+ ) {
setEditorState("timeline", "selection", null);
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| createRoot((dispose) => { | |
| createEventListener(e.currentTarget, "mouseup", () => { | |
| handleUpdatePlayhead(e); | |
| if (zoomSegmentDragState.type === "idle") { | |
| setEditorState("timeline", "selection", null); | |
| } | |
| }); | |
| createEventListener(window, "mouseup", () => { | |
| dispose(); | |
| }); | |
| }); | |
| }} | |
| createRoot((dispose) => { | |
| createEventListener(e.currentTarget, "mouseup", () => { | |
| handleUpdatePlayhead(e); | |
| if ( | |
| zoomSegmentDragState.type === "idle" && | |
| sceneSegmentDragState.type === "idle" | |
| ) { | |
| setEditorState("timeline", "selection", null); | |
| } | |
| }); | |
| createEventListener(window, "mouseup", () => { | |
| dispose(); | |
| }); | |
| }); | |
| }} |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/index.tsx around lines 134 to 145,
the code clears the timeline selection whenever zoomSegmentDragState is idle,
but it doesn't guard against an ongoing scene segment drag; change the condition
that calls setEditorState("timeline", "selection", null) so it only clears
selection when both zoomSegmentDragState.type === "idle" AND
sceneSegmentDragState.type === "idle"; keep the existing playhead update and
event listener disposal logic unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
1-523: Remove allanyannotations in.findcallbacks and use proper typesThe following callback parameters are still typed as
anyand should be updated to their correct types:
apps/desktop/src/routes/(window-chrome)/(main).tsx
• Line 151 and 155:_screens()?.find((s: any) => s.id === screenId)→ replace with
_screens()?.find((screen: CaptureDisplay) => screen.id === screenId)• Line 166:
_windows()?.find((s: any) => s.id === windowId)→ replace with
_windows()?.find((win: CaptureWindow) => win.id === windowId)• Line 179:
mics.data?.find((name: any) => name === rawOptions.micName)→ replace with
mics.data?.find((deviceName: string) => deviceName === rawOptions.micName)Additionally, for consistency and clarity, consider annotating the
cameras.findcallback parameter:// current cameras.find((c) => { … }) // with explicit type cameras.find((device: CameraInfo) => { const { cameraID } = rawOptions; … })These changes will eliminate untyped
anyusage and ensure the callbacks are strongly typed.
♻️ Duplicate comments (7)
apps/desktop/src/routes/(window-chrome)/(main).tsx (3)
150-156: Replace any with concrete CaptureDisplay type in .find callbacksPer strict TS guidelines, avoid any. Use CaptureDisplay so narrowing and autocomplete stay intact.
- screen = - _screens()?.find((s: any) => s.id === screenId) ?? _screens()?.[0]; + screen = + _screens()?.find((s: CaptureDisplay) => s.id === screenId) ?? _screens()?.[0];- screen = - _screens()?.find((s: any) => s.id === screenId) ?? _screens()?.[0]; + screen = + _screens()?.find((s: CaptureDisplay) => s.id === screenId) ?? _screens()?.[0];
165-166: Strongly type window lookupSame here; type the item as CaptureWindow.
- win = - _windows()?.find((s: any) => s.id === windowId) ?? _windows()?.[0]; + win = + _windows()?.find((w: CaptureWindow) => w.id === windowId) ?? _windows()?.[0];
179-179: Strongly type microphone nameslistAudioDevices returns string[]. Replace any with string.
- micName: () => mics.data?.find((name: any) => name === rawOptions.micName), + micName: () => mics.data?.find((name: string) => name === rawOptions.micName),crates/rendering/src/scene.rs (2)
306-308: Gate screen rendering; avoid doing work when the screen is invisibleReturning true defeats render gating. Use opacity/blur thresholds like the camera check.
- pub fn should_render_screen(&self) -> bool { - true - } + pub fn should_render_screen(&self) -> bool { + self.screen_opacity > 0.01 || self.screen_blur > 0.01 + }
87-118: Unreachable “transition into segment” branch; start-of-segment transitions never runInside if let Some(segment) = cursor.segment, time ∈ [segment.start, segment.end). The check cursor.time < segment.start is unreachable, so the “transition into segment” code never executes.
Apply this to handle the first SCENE_TRANSITION_DURATION inside the segment and move “transition out” to the final window:
- let (current_mode, next_mode, transition_progress) = if let Some(segment) = cursor.segment { - let transition_start = segment.start - SCENE_TRANSITION_DURATION; - let transition_end = segment.end - SCENE_TRANSITION_DURATION; - - if cursor.time < segment.start && cursor.time >= transition_start { + let (current_mode, next_mode, transition_progress) = if let Some(segment) = cursor.segment { + let transition_in_end = segment.start + SCENE_TRANSITION_DURATION; + let transition_out_start = segment.end - SCENE_TRANSITION_DURATION; + + // Transitioning into this segment (first window) + if cursor.time >= segment.start && cursor.time < transition_in_end { // Check if we should skip transition for small gaps let prev_mode = if let Some(prev_seg) = cursor.prev_segment { - let gap = segment.start - prev_seg.end; + let gap = segment.start - prev_seg.end; let same_mode = matches!( (&prev_seg.mode, &segment.mode), (SceneMode::CameraOnly, SceneMode::CameraOnly) | (SceneMode::Default, SceneMode::Default) | (SceneMode::HideCamera, SceneMode::HideCamera) ); if gap < MIN_GAP_FOR_TRANSITION && same_mode { - // Small gap between same modes, no transition needed - return InterpolatedScene::from_single_mode(segment.mode.clone()); + // Small gap between same modes, no transition needed + return InterpolatedScene::from_single_mode(segment.mode); } else if gap > 0.01 { SceneMode::Default } else { - prev_seg.mode.clone() + prev_seg.mode } } else { SceneMode::Default }; - let progress = (cursor.time - transition_start) / SCENE_TRANSITION_DURATION; + let progress = ((cursor.time - segment.start) / SCENE_TRANSITION_DURATION).min(1.0); ( prev_mode, - segment.mode.clone(), + segment.mode, ease_in_out(progress as f32) as f64, ) - } else if cursor.time >= transition_end && cursor.time < segment.end { + // Transitioning out of this segment (final window) + } else if cursor.time >= transition_out_start && cursor.time < segment.end { if let Some(next_seg) = cursor.next_segment() { - let gap = next_seg.start - segment.end; + let gap = next_seg.start - segment.end; // For small gaps between same-mode segments, don't transition let same_mode = matches!( (&segment.mode, &next_seg.mode), (SceneMode::CameraOnly, SceneMode::CameraOnly) | (SceneMode::Default, SceneMode::Default) | (SceneMode::HideCamera, SceneMode::HideCamera) ); if gap < MIN_GAP_FOR_TRANSITION && same_mode { // Keep the current mode without transitioning - (segment.mode.clone(), segment.mode.clone(), 1.0) + (segment.mode, segment.mode, 1.0) } else if gap > 0.01 { // There's a significant gap, so transition to default scene - let progress = - ((cursor.time - transition_end) / SCENE_TRANSITION_DURATION).min(1.0); + let progress = + ((cursor.time - transition_out_start) / SCENE_TRANSITION_DURATION).min(1.0); ( - segment.mode.clone(), + segment.mode, SceneMode::Default, ease_in_out(progress as f32) as f64, ) } else { // No gap, segments are back-to-back, transition directly if modes differ - let progress = - ((cursor.time - transition_end) / SCENE_TRANSITION_DURATION).min(1.0); + let progress = + ((cursor.time - transition_out_start) / SCENE_TRANSITION_DURATION).min(1.0); ( - segment.mode.clone(), - next_seg.mode.clone(), + segment.mode, + next_seg.mode, ease_in_out(progress as f32) as f64, ) } } else { // No next segment, transition to default - let progress = - ((cursor.time - transition_end) / SCENE_TRANSITION_DURATION).min(1.0); + let progress = + ((cursor.time - transition_out_start) / SCENE_TRANSITION_DURATION).min(1.0); ( - segment.mode.clone(), + segment.mode, SceneMode::Default, ease_in_out(progress as f32) as f64, ) } } else { - (segment.mode.clone(), segment.mode.clone(), 1.0) + (segment.mode, segment.mode, 1.0) }apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (1)
16-18: Do not import icons manually in desktop app; rely on unplugin-icons auto-importsPer apps/desktop guideline, remove manual lucide imports and use <IconLucide*> directly.
-import IconLucideEyeOff from "~icons/lucide/eye-off"; -import IconLucideMonitor from "~icons/lucide/monitor"; -import IconLucideVideo from "~icons/lucide/video";apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
52-54: Remove manual lucide icon imports; rely on auto-importsThis file is under apps/desktop; let unplugin-icons provide IconLucideMonitor/Sparkles automatically.
-import IconLucideMonitor from "~icons/lucide/monitor"; -import IconLucideSparkles from "~icons/lucide/sparkles";
🧹 Nitpick comments (4)
apps/desktop/src/routes/(window-chrome)/(main).tsx (1)
145-180: Optional: de-duplicate screen selection logicThe two screen selections are identical except for where screenId comes from. Consider a tiny helper to avoid duplication and future drift.
+ const findScreenById = (id: string | undefined) => + id ? _screens()?.find((s: CaptureDisplay) => s.id === id) ?? _screens()?.[0] : _screens()?.[0]; ... - if (rawOptions.captureTarget.variant === "display") { - const screenId = rawOptions.captureTarget.id; - screen = - _screens()?.find((s: CaptureDisplay) => s.id === screenId) ?? _screens()?.[0]; - } else if (rawOptions.captureTarget.variant === "area") { - const screenId = rawOptions.captureTarget.screen; - screen = - _screens()?.find((s: CaptureDisplay) => s.id === screenId) ?? _screens()?.[0]; - } + if (rawOptions.captureTarget.variant === "display") { + screen = findScreenById(rawOptions.captureTarget.id); + } else if (rawOptions.captureTarget.variant === "area") { + screen = findScreenById(rawOptions.captureTarget.screen); + }crates/rendering/src/scene.rs (1)
74-82: Remove unnecessary clone() on Copy types (SceneMode)SceneMode implements Copy; clone() triggers clippy::clone_on_copy warnings and adds noise.
- scene_mode: mode.clone(), + scene_mode: mode, ... - from_mode: mode.clone(), + from_mode: mode, ... - return InterpolatedScene::from_single_mode(segment.mode.clone()); + return InterpolatedScene::from_single_mode(segment.mode); ... - prev_seg.mode.clone() + prev_seg.mode ... - segment.mode.clone(), + segment.mode, ... - (segment.mode.clone(), segment.mode.clone(), 1.0) + (segment.mode, segment.mode, 1.0) ... - segment.mode.clone(), + segment.mode, ... - next_seg.mode.clone(), + next_seg.mode, ... - scene_mode: if transition_progress > 0.5 { next_mode.clone() } else { current_mode.clone() }, + scene_mode: if transition_progress > 0.5 { next_mode } else { current_mode },Also applies to: 103-107, 115-116, 131-131, 156-160, 176-185, 277-280
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (1)
58-79: Tighten types: avoid stringly-typed modeUse the shared type to keep exhaustiveness and autocomplete. This also flags typos at compile time.
- const getSceneIcon = (mode: string | undefined) => { + const getSceneIcon = (mode: SceneSegment["mode"]) => { switch (mode) { case "cameraOnly": return <IconLucideVideo class="size-3.5" />; case "hideCamera": return <IconLucideEyeOff class="size-3.5" />; default: return <IconLucideMonitor class="size-3.5" />; } }; ... - const getSceneLabel = (mode: string | undefined) => { + const getSceneLabel = (mode: SceneSegment["mode"]) => { switch (mode) { case "cameraOnly": return "Camera Only"; case "hideCamera": return "Hide Camera"; default: return "Default"; } };apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
2319-2325: Prefer SceneMode type over re-declared string unionAvoid duplicating the union literals; use the source-of-truth type from tauri bindings.
- v as "default" | "cameraOnly" | "hideCamera", + v as import("~/utils/tauri").SceneMode,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
apps/desktop/src/routes/(window-chrome)/(main).tsx(3 hunks)apps/desktop/src/routes/editor/ConfigSidebar.tsx(6 hunks)apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx(18 hunks)apps/desktop/src/routes/editor/Timeline/index.tsx(4 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)crates/rendering/src/scene.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/routes/editor/Timeline/index.tsx
🧰 Additional context used
📓 Path-based instructions (4)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/(window-chrome)/(main).tsxapps/desktop/src/routes/editor/Timeline/SceneTrack.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsxapps/desktop/src/utils/tauri.ts
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable
Files:
apps/desktop/src/routes/(window-chrome)/(main).tsxapps/desktop/src/routes/editor/Timeline/SceneTrack.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsxapps/desktop/src/utils/tauri.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/desktop/src/routes/(window-chrome)/(main).tsxapps/desktop/src/routes/editor/Timeline/SceneTrack.tsxapps/desktop/src/routes/editor/ConfigSidebar.tsxapps/desktop/src/utils/tauri.ts
**/tauri.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Never edit auto-generated IPC bindings file: tauri.ts
Files:
apps/desktop/src/utils/tauri.ts
🧠 Learnings (1)
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Applied to files:
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx
🧬 Code graph analysis (2)
crates/rendering/src/scene.rs (2)
apps/desktop/src/utils/tauri.ts (2)
SceneMode(693-693)SceneSegment(694-694)crates/rendering/src/lib.rs (6)
new(63-133)new(298-335)new(516-827)new(843-848)new(895-905)new(1045-1075)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
apps/desktop/src/utils/tauri.ts (1)
SceneSegment(694-694)
🪛 GitHub Check: Clippy
crates/rendering/src/scene.rs
[warning] 131-131: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:131:48
|
131 | (segment.mode.clone(), segment.mode.clone(), 1.0)
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 131-131: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:131:26
|
131 | (segment.mode.clone(), segment.mode.clone(), 1.0)
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 115-115: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:115:21
|
115 | segment.mode.clone(),
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 107-107: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:107:25
|
107 | prev_seg.mode.clone()
| ^^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: prev_seg.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 103-103: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:103:68
|
103 | return InterpolatedScene::from_single_mode(segment.mode.clone());
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 76-76: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:76:24
|
76 | from_mode: mode.clone(),
| ^^^^^^^^^^^^ help: try removing the clone call: mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 74-74: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:74:25
|
74 | scene_mode: mode.clone(),
| ^^^^^^^^^^^^ help: try removing the clone call: mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
= note: #[warn(clippy::clone_on_copy)] on by default
⏰ 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). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
apps/desktop/src/utils/tauri.ts (1)
693-695: Types aligned with sceneSegments migration; no manual edits neededSceneMode/SceneSegment additions and TimelineConfiguration.sceneSegments look consistent with the PR-wide rename. Given this file is auto-generated by tauri-specta, no action required as long as it wasn’t hand-edited.
If this wasn’t regenerated, please re-run the tauri-specta generation step to avoid drift between Rust and TS bindings.
Also applies to: 751-754
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (5)
27-35: Rename to SceneSegmentDragState: SGTMProp and state rename read well and align with “scene” terminology.
52-52: Hover reset uses sceneSegments: SGTMCorrectly updates hover state when sceneSegments are cleared.
190-198: UX copy update: nice“Click to add scene segment” aligns with the new terminology.
161-183: Insertion logic and sorting look correctPre-seeding sceneSegments, computing insertion index from start time, inserting, and then sorting in resizers is consistent.
277-282: Selection path updated to scene: SGTMSelection resolution uses sceneSegments index; matches rest of PR.
apps/desktop/src/routes/editor/ConfigSidebar.tsx (3)
47-51: Switch to SceneSegment import: SGTMUsing SceneSegment from tauri bindings keeps UI types in sync with backend.
675-682: Selection plumbing updated to sceneSegments: SGTMIndexing into project.timeline.sceneSegments matches the new model.
2287-2290: SceneSegmentConfig props look correctTies the config panel directly to a typed SceneSegment and its index.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/rendering/src/scene.rs (1)
87-118: Fix unreachable code and remove unnecessary clones on Copy typesBased on past review feedback, this code has an unreachable branch (lines 91-117) and multiple unnecessary
.clone()calls onSceneModewhich implements theCopytrait. The conditioncursor.time < segment.startis impossible whencursor.segmentis Some because the segment was selected precisely whentime >= segment.start.The unreachable branch should be replaced with an early-in-segment transition check, and all
.clone()calls onSceneModeshould be removed.
🧹 Nitpick comments (4)
crates/rendering/src/scene.rs (4)
67-82: Optimize Copy trait usage in helper methodThe
from_single_modemethod has unnecessary.clone()calls onSceneModewhich implementsCopy.Apply this fix:
- scene_mode: mode.clone(), + scene_mode: mode, transition_progress: 1.0, - from_mode: mode.clone(), + from_mode: mode, to_mode: mode,
118-163: Complex transition logic with scattered Copy trait issuesThis section handles transitions from inside segments to next segments or defaults. The logic appears sound but contains multiple unnecessary
.clone()calls onSceneModethroughout.Remove all
.clone()calls onSceneModethroughout this section, for example:- (segment.mode.clone(), segment.mode.clone(), 1.0) + (segment.mode, segment.mode, 1.0) - segment.mode.clone(), + segment.mode, - next_seg.mode.clone(), + next_seg.mode,
164-211: Gap transition handling needs Copy trait optimizationThe gap transition logic correctly handles small gaps between same-mode segments and transitions into upcoming segments. However, it also contains unnecessary
.clone()calls.Remove
.clone()calls onSceneMode:- (prev_seg.mode.clone(), prev_seg.mode.clone(), 1.0) + (prev_seg.mode, prev_seg.mode, 1.0) - prev_seg.mode.clone() + prev_seg.mode - next_segment.mode.clone(), + next_segment.mode,
272-287: Scene mode selection and Copy trait issues in constructorThe final scene construction has good transition logic but still contains unnecessary
.clone()calls.Remove
.clone()calls:scene_mode: if transition_progress > 0.5 { - next_mode.clone() + next_mode } else { - current_mode.clone() + current_mode },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
crates/rendering/src/scene.rs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/rendering/src/scene.rs (2)
apps/desktop/src/utils/tauri.ts (2)
SceneMode(693-693)SceneSegment(694-694)crates/rendering/src/lib.rs (6)
new(63-133)new(298-335)new(516-827)new(843-848)new(895-905)new(1045-1075)
🪛 GitHub Check: Clippy
crates/rendering/src/scene.rs
[warning] 147-147: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:147:29
|
147 | ... next_seg.mode.clone(),
| ^^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: next_seg.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 146-146: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:146:29
|
146 | ... segment.mode.clone(),
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 137-137: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:137:29
|
137 | ... segment.mode.clone(),
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 131-131: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:131:48
|
131 | (segment.mode.clone(), segment.mode.clone(), 1.0)
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 131-131: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:131:26
|
131 | (segment.mode.clone(), segment.mode.clone(), 1.0)
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 115-115: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:115:21
|
115 | segment.mode.clone(),
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 107-107: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:107:25
|
107 | prev_seg.mode.clone()
| ^^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: prev_seg.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 103-103: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:103:68
|
103 | return InterpolatedScene::from_single_mode(segment.mode.clone());
| ^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 76-76: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:76:24
|
76 | from_mode: mode.clone(),
| ^^^^^^^^^^^^ help: try removing the clone call: mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 74-74: using clone on type SceneMode which implements the Copy trait
warning: using clone on type SceneMode which implements the Copy trait
--> crates/rendering/src/scene.rs:74:25
|
74 | scene_mode: mode.clone(),
| ^^^^^^^^^^^^ help: try removing the clone call: mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
= note: #[warn(clippy::clone_on_copy)] on by default
⏰ 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). (3)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (6)
crates/rendering/src/scene.rs (6)
306-308: Good implementation of meaningful render gatingThe
should_render_screen()method correctly implements render gating based on both opacity and blur thresholds, which will optimize rendering performance by skipping display passes when the screen is effectively invisible.
1-4: Well-defined constants for transition timingThe constants
SCENE_TRANSITION_DURATIONandMIN_GAP_FOR_TRANSITIONprovide clear, configurable parameters for the scene transition system. The 0.3 second transition duration and 0.5 second minimum gap threshold are reasonable defaults.
15-44: Solid cursor implementation with proper segment trackingThe
SceneSegmentsCursorconstructor correctly handles both cases: finding an active segment at the given time, and tracking the previous segment when between segments. The logic for identifying previous segments uses proper reverse iteration.
46-49: Simple and effective next segment lookupThe
next_segment()method efficiently finds the next segment after the current time using iterator operations.
290-296: Clean scene value mapping implementationThe
get_scene_valuesmethod provides a clear mapping from scene modes to their respective opacity and scale values. The values are appropriate for the intended functionality.
315-351: Well-structured transition opacity calculationsThe
camera_only_transition_opacityandregular_camera_transition_opacitymethods properly handle the complex opacity calculations needed for smooth transitions between camera-only and regular modes, including the fast fade effects.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (3)
56-66: Icons: auto-import compliance OK; mark decorative icons as aria-hiddenThis file correctly relies on unplugin-icons auto-imports (per desktop guidelines). To avoid redundant announcements by screen readers, mark these icons as decorative.
Apply:
- return <IconLucideVideo class="size-3.5" />; + return <IconLucideVideo class="size-3.5" aria-hidden="true" />; ... - return <IconLucideEyeOff class="size-3.5" />; + return <IconLucideEyeOff class="size-3.5" aria-hidden="true" />; ... - return <IconLucideMonitor class="size-3.5" />; + return <IconLucideMonitor class="size-3.5" aria-hidden="true" />;
86-93: Use currentTarget for bounds and clamp negative timeUsing
e.targetcan select a child node and yield incorrect bounds; also guard against negative time when panning left of 0.Apply:
- const bounds = e.target.getBoundingClientRect()!; + const bounds = (e.currentTarget as HTMLElement).getBoundingClientRect(); let time = (e.clientX - bounds.left) * secsPerPixel() + editorState.timeline.transform.position; + + // Prevent negative timeline positions + time = Math.max(0, time);
149-185: Mouseup should be attached to window to catch releases outside the trackIf the pointer leaves the track before mouseup, the handler on
currentTargetwon’t fire and the segment won’t be created. Attach the listener towindow.Apply:
- createEventListener(e.currentTarget, "mouseup", (e) => { + createEventListener(window, "mouseup", (e) => { dispose(); const time = hoveredTime(); const maxDuration = maxAvailableDuration();
🧹 Nitpick comments (6)
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (6)
67-76: Type the mode with shared SceneMode instead of stringAvoid
string | undefinedhere; prefer the shared union type (e.g.,SceneMode) to catch typos at compile time and align with repo-wide types.Example:
-const getSceneLabel = (mode: string | undefined) => { +const getSceneLabel = (mode?: SceneMode) => {If
SceneModeis generated or exported from a shared package, import it here.
201-202: Avoid non-null assertions; provide safe fallbackThis lives under a
<For each={project.timeline?.sceneSegments}>, but removing!s improves local robustness.Apply:
- const sceneSegments = () => project.timeline!.sceneSegments!; + const sceneSegments = () => project.timeline?.sceneSegments ?? [];
310-316: Avoid shadowing outersegmentbinding in loopsShadowing the
segmentidentifier reduces readability. Rename inner variable.Apply:
- for (let i = sceneSegments().length - 1; i >= 0; i--) { - const segment = sceneSegments()[i]!; - if (segment.end <= start) { - minValue = segment.end; + for (let i = sceneSegments().length - 1; i >= 0; i--) { + const seg = sceneSegments()[i]!; + if (seg.end <= start) { + minValue = seg.end; break; } }
336-344: Repeated sort during drag can be costly; consider deferring or local swapSorting
sceneSegmentson every mousemove can thrash for long lists. Consider only sorting when a boundary is crossed, or deferring sort tofinish()after drag ends with a single stable reinsert.
393-401: Labels/icons render well; consider i18n hook for copy“Scene”, “Camera Only”, “Hide Camera”, “Default” could come from an i18n source to match the rest of the app.
417-423: Avoid shadowing here as wellSame nit as above; rename inner
segmenttosegfor consistency.Apply:
- for (let i = 0; i < sceneSegments().length; i++) { - const segment = sceneSegments()[i]!; - if (segment.start > end) { - maxValue = segment.start; + for (let i = 0; i < sceneSegments().length; i++) { + const seg = sceneSegments()[i]!; + if (seg.start > end) { + maxValue = seg.start; break; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx(19 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/desktop/**/*.{ts,tsx}: In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Use generated tauri_specta commands/events (commands, events) in the desktop frontend; listen to generated events directly
Use @tanstack/solid-query for server state in the desktop app
Files:
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx
{apps/desktop,packages/ui-solid}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Component naming (Solid): components in PascalCase; hooks/utilities in camelCase starting with 'use' where applicable
Files:
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; prefer shared types from packages
Files:
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx
🧠 Learnings (1)
📚 Learning: 2025-08-25T10:58:06.134Z
Learnt from: CR
PR: CapSoftware/Cap#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:58:06.134Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : In the desktop app, rely on unplugin-icons auto-imports; do not manually import icon modules
Applied to files:
apps/desktop/src/routes/editor/Timeline/SceneTrack.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). (3)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (7)
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (7)
25-33: Rename and typing for Scene drag state look goodNew
SceneSegmentDragStateand theSceneTrackprop signature align with the PR’s scene refactor and keep the API explicit.
49-54: Hover reset effect correctly targets sceneSegmentsSwitching the dependency to
project.timeline?.sceneSegmentsprevents stale hover when segments are removed. Good catch.
188-196: Copy and empty state align with scene terminology“Click to add scene segment” and helper text read well and match the refactor.
354-359: Prev/next lookup is clear; confirm edges on duration changesThis correctly constrains movement between neighbors and timeline duration. If duration can shrink during a drag, you may need to re-evaluate
maxEndmid-drag.
380-384: Atomic segment move looks goodUpdating the whole object for center-drag keeps invariants intact. Nice.
433-451: End-handle drag update + post-sort are consistentConstraining with min/max and re-sorting ensures a valid, ordered array post-update.
271-279: SceneSegment has noidfield—remove unusable suggestionThe
SceneSegmenttype is defined inapps/desktop/src/utils/tauri.tsas:export type SceneSegment = { start: number; end: number; mode?: SceneMode };There is no
idproperty available, so the recommendation to “use a stable identifier (e.g.id)” cannot be implemented without first extending the type.Next steps:
- Either extend
SceneSegmentwith a uniqueidand propagate it throughproject.timeline.sceneSegments, then compare byid- Or simplify the selection logic in
apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx(lines 271–279) by passing the segment’s array index into the component (via.map((segment, index) => …)) and comparing directly toselection.indexinstead of matching onstart/end.apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx (271–279):
- const segmentIndex = project.timeline?.sceneSegments?.findIndex( - (s) => s.start === segment.start && s.end === segment.end, - ); + // Prefer using the index passed from the map callback for stable selection + // const segmentIndex = project.timeline?.sceneSegments?.findIndex(…);Likely an incorrect or invalid review comment.
| setProject( | ||
| "timeline", | ||
| "layoutSegments", | ||
| produce((layoutSegments) => { | ||
| layoutSegments ??= []; | ||
| "sceneSegments", | ||
| produce((sceneSegments) => { | ||
| sceneSegments ??= []; | ||
|
|
||
| let index = layoutSegments.length; | ||
| let index = sceneSegments.length; | ||
|
|
||
| for (let i = layoutSegments.length - 1; i >= 0; i--) { | ||
| if (layoutSegments[i].start > time) { | ||
| for (let i = sceneSegments.length - 1; i >= 0; i--) { | ||
| if (sceneSegments[i].start > time) { | ||
| index = i; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| layoutSegments.splice(index, 0, { | ||
| sceneSegments.splice(index, 0, { | ||
| start: time, |
There was a problem hiding this comment.
Insertion index search is incorrect; can insert after the wrong neighbor
The backward scan picks the highest index with start > time, which inserts after segments that also satisfy the condition (e.g., time=4 with starts [2,5,8] inserts at 2, before 8 but after 5). You want the first index where start > time.
Apply:
- let index = sceneSegments.length;
-
- for (let i = sceneSegments.length - 1; i >= 0; i--) {
- if (sceneSegments[i].start > time) {
- index = i;
- break;
- }
- }
+ let index = sceneSegments.findIndex((s) => s.start > time);
+ if (index === -1) index = sceneSegments.length;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setProject( | |
| "timeline", | |
| "layoutSegments", | |
| produce((layoutSegments) => { | |
| layoutSegments ??= []; | |
| "sceneSegments", | |
| produce((sceneSegments) => { | |
| sceneSegments ??= []; | |
| let index = layoutSegments.length; | |
| let index = sceneSegments.length; | |
| for (let i = layoutSegments.length - 1; i >= 0; i--) { | |
| if (layoutSegments[i].start > time) { | |
| for (let i = sceneSegments.length - 1; i >= 0; i--) { | |
| if (sceneSegments[i].start > time) { | |
| index = i; | |
| break; | |
| } | |
| } | |
| layoutSegments.splice(index, 0, { | |
| sceneSegments.splice(index, 0, { | |
| start: time, | |
| setProject( | |
| "timeline", | |
| "sceneSegments", | |
| produce((sceneSegments) => { | |
| sceneSegments ??= []; | |
| let index = sceneSegments.findIndex((s) => s.start > time); | |
| if (index === -1) index = sceneSegments.length; | |
| sceneSegments.splice(index, 0, { | |
| start: time, | |
| // … | |
| }); | |
| }) | |
| ); |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/SceneTrack.tsx around lines 160–176,
the backward loop currently finds the highest index with start > time (causing
incorrect insertion after the wrong neighbor); change the search to find the
first index where sceneSegments[i].start > time — for example, iterate forward
from i=0..len-1, set index = i and break when sceneSegments[i].start > time
(keep default index = sceneSegments.length), or alternatively adjust the
backward scan to detect the last start <= time and set index = i+1; then splice
at that computed index.
Renaming layout segments to scene segments
Summary by CodeRabbit
New Features
Refactor
UX
Chores