Conversation
WalkthroughAdds layout-segment support end-to-end: types (TS/Rust), editor UI (LayoutTrack, ConfigSidebar), editor actions/state, recording init, rendering interpolation and camera-only path, shader opacity, zoom-preview robustness, ambient icons, and repository guidance (CLAUDE.md). No public API removals. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant LayoutTrack
participant EditorContext
participant ProjectState
participant TauriBridge
participant Renderer
User->>LayoutTrack: click/drag to create or modify layout segment
LayoutTrack->>EditorContext: create/move/resize/select requests
EditorContext->>ProjectState: immutably update timeline.layoutSegments (sorted)
ProjectState->>TauriBridge: expose updated TimelineConfiguration (layout_segments)
TauriBridge->>Renderer: supply layout per-frame
Renderer->>Renderer: build InterpolatedLayout -> decide layers/opacities
Renderer-->>User: composited frame (screen / camera / camera_only)
sequenceDiagram
autonumber
participant Timeline
participant InterpolatedLayout
participant RendererLayers
participant WGSL
Timeline->>InterpolatedLayout: segments + current time
InterpolatedLayout-->>RendererLayers: camera/screen opacities & mode
RendererLayers->>RendererLayers: render screen if allowed
RendererLayers->>RendererLayers: render camera or camera_only per guards
RendererLayers->>WGSL: send uniforms (including opacity)
WGSL-->>RendererLayers: shaded/blended frame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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. ✨ 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 (
|
richiemcilroy
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (13)
crates/rendering/src/composite_frame.rs (1)
106-117: Avoid per-call sampler creation in bind_groupA new
Sampleris created for everybind_groupcall. Samplers are immutable and cheap to reuse. Consider storing one onCompositeVideoFramePipelineand reusing it.Possible refactor:
- Add
pub sampler: wgpu::Sampleron the pipeline struct.- Create it once in
new().- Use
&self.samplerhere and remove this block.crates/rendering/src/shaders/composite-video-frame.wgsl (1)
85-86: Clarify whether opacity should also fade the shadowCurrently
opacityonly affects the content alpha; the shadow remains fully visible. If the intended behavior is to fade the entire composite (content + shadow), multiply the shadow alpha byuniforms.opacitytoo.Proposed change:
- let shadow_color = vec4<f32>(0.0, 0.0, 0.0, shadow_strength_final * shadow_opacity); + let shadow_color = vec4<f32>(0.0, 0.0, 0.0, shadow_strength_final * shadow_opacity * uniforms.opacity);If you want shadow independent of content opacity (e.g., camera-only overlays), keep as-is.
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
43-54: Good fix to clear stale hover state; also clear hoveredTime to avoid a ghost preview segment.When segments are deleted while hovering, resetting hoveringSegment prevents blocked creation. Consider also clearing hoveredTime so the “+” preview doesn’t linger until the next mouse move.
Apply:
createEffect(() => { const segments = project.timeline?.zoomSegments; if (!segments || segments.length === 0) { setHoveringSegment(false); + setHoveredTime(undefined); } });CLAUDE.md (2)
373-377: Specify a language for the fenced code block to satisfy markdownlint (MD040).Use a neutral language like
textfor the flow diagram.-``` +```text Desktop Recording → Local Files → Upload to S3 → Background Processing (tasks service) → Transcription/AI Enhancement → Database Storage--- `382-387`: **Replace bare URLs with proper links to satisfy markdownlint (MD034).** Wrap URLs with angle brackets or full markdown links. ```diff -- **TanStack Query**: https://tanstack.com/query/latest -- **React Patterns**: https://react.dev/learn/you-might-not-need-an-effect -- **Tauri v2**: https://github.com/tauri-apps/tauri -- **tauri_specta**: https://github.com/oscartbeaumont/tauri-specta -- **Drizzle ORM**: https://orm.drizzle.team/ -- **SolidJS**: https://solidjs.com/ +- **TanStack Query**: <https://tanstack.com/query/latest> +- **React Patterns**: <https://react.dev/learn/you-might-not-need-an-effect> +- **Tauri v2**: <https://github.com/tauri-apps/tauri> +- **tauri_specta**: <https://github.com/oscartbeaumont/tauri-specta> +- **Drizzle ORM**: <https://orm.drizzle.team/> +- **SolidJS**: <https://solidjs.com/> -- **Self-hosting**: https://cap.so/docs/self-hosting +- **Self-hosting**: <https://cap.so/docs/self-hosting>Also applies to: 389-389
apps/desktop/src/routes/editor/Timeline/index.tsx (1)
126-138: Also gate selection clear on layout drag state.The mousedown-up flow only checks zoomSegmentDragState. If a layout segment is being dragged, we can accidentally clear selection.
createEventListener(e.currentTarget, "mouseup", () => { handleUpdatePlayhead(e); - if (zoomSegmentDragState.type === "idle") { + if ( + zoomSegmentDragState.type === "idle" && + layoutSegmentDragState.type === "idle" + ) { setEditorState("timeline", "selection", null); } });Also applies to: 130-133
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (2)
32-40: Remove unused WaveformCanvas prop secsPerPixel; context value is used instead.WaveformCanvas reads secsPerPixel from timeline context and never uses the props.secsPerPixel. Dropping the prop simplifies the interface.
-function WaveformCanvas(props: { - systemWaveform?: number[]; - micWaveform?: number[]; - segment: { start: number; end: number }; - secsPerPixel: number; -}) { +function WaveformCanvas(props: { + systemWaveform?: number[]; + micWaveform?: number[]; + segment: { start: number; end: number }; +}) { const { project } = useEditorContext(); @@ return ( <canvas @@ - segment={segment} - secsPerPixel={secsPerPixel()} + segment={segment} />Also applies to: 130-140, 372-377
222-239: Optional: clarify volume gate semantics for drawing waveforms.The db thresholds short-circuit rendering (e.g., micVolumeDb < -30). If the intent is “don’t draw when muted/very low volume,” consider making the cutoff a named constant and explicitly handling undefined as “draw.”
- if (project.audio.micVolumeDb && project.audio.micVolumeDb < -30) return; + const MIN_DRAW_DB = -30; + if (typeof project.audio.micVolumeDb === "number" && project.audio.micVolumeDb < MIN_DRAW_DB) return;Also applies to: 230-239
apps/desktop/src/routes/editor/context.ts (1)
144-156: Add bounds guard to prevent out-of-range deletionsdeleteLayoutSegment should defensively verify the segment index exists before mutating to avoid silent no-ops or surprises if stale indices are passed (e.g., after a sort). Also, short-circuit if the timeline or layoutSegments is absent, mirroring the clip deletion guard.
Consider this tweak:
deleteLayoutSegment: (segmentIndex: number) => { batch(() => { - setProject( + if ( + !project.timeline?.layoutSegments || + segmentIndex < 0 || + segmentIndex >= project.timeline.layoutSegments.length + ) { + return; + } + setProject( "timeline", "layoutSegments", produce((s) => { if (!s) return; s.splice(segmentIndex, 1); }), ); setEditorState("timeline", "selection", null); }); },apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx (2)
317-323: Unify minimum segment duration with hover threshold (0.5s)Start/end resize constraints use ±1 second, but hover/creation logic uses 0.5 seconds as the minimum viable duration. Align these to avoid UX inconsistencies and “can’t match preview” issues.
Update the constraints:
- const maxValue = segment.end - 1; + const maxValue = segment.end - 0.5;- const minValue = segment.start + 1; + const minValue = segment.start + 0.5;Also applies to: 422-424
59-79: Tighten types for mode parameterBoth helpers accept mode: string | undefined. Narrowing to the actual mode type avoids typos and eases refactors.
If you can import the shared type:
+import type { LayoutSegment as TLayoutSegment } from "~/utils/tauri"; - const getLayoutIcon = (mode: string | undefined) => { + const getLayoutIcon = (mode: TLayoutSegment["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 getLayoutLabel = (mode: string | undefined) => { + const getLayoutLabel = (mode: TLayoutSegment["mode"]) => { switch (mode) { case "cameraOnly": return "Camera Only"; case "hideCamera": return "Hide Camera"; default: return "Default"; } };Alternatively, define a local union type: type LayoutMode = "default" | "cameraOnly" | "hideCamera" | undefined;
crates/rendering/src/layout.rs (2)
70-81: Unreachable pre-start transition branch (dead code)Inside the Some(segment) arm, cursor.time ∈ [segment.start, segment.end). The condition cursor.time < segment.start can never be true. This branch is redundant; the “entering next segment” case is already handled in the None/next_segment arm below.
Safe removal:
- if cursor.time < segment.start && cursor.time >= transition_start { - let prev_mode = cursor - .prev_segment - .map(|s| s.mode.clone()) - .unwrap_or(LayoutMode::Default); - let progress = (cursor.time - transition_start) / LAYOUT_TRANSITION_DURATION; - ( - prev_mode, - segment.mode.clone(), - ease_in_out(progress as f32) as f64, - ) - } else if cursor.time >= transition_end && cursor.time < segment.end { + if cursor.time >= transition_end && cursor.time < segment.end { if let Some(next_seg) = cursor.next_segment() {
83-96: Clamp transition progress to [0, 1] to avoid floating-point driftEven with range checks, fp error can push progress slightly outside [0, 1]; clamping before easing is cheap and robust.
- let progress = (cursor.time - transition_end) / LAYOUT_TRANSITION_DURATION; + let progress = ((cursor.time - transition_end) / LAYOUT_TRANSITION_DURATION).clamp(0.0, 1.0); ( segment.mode.clone(), next_seg.mode.clone(), ease_in_out(progress as f32) as f64, ) } else { - let progress = (cursor.time - transition_end) / LAYOUT_TRANSITION_DURATION; + let progress = ((cursor.time - transition_end) / LAYOUT_TRANSITION_DURATION).clamp(0.0, 1.0); ( segment.mode.clone(), LayoutMode::Default, ease_in_out(progress as f32) as f64, ) }- let transition_start = next_segment.start - LAYOUT_TRANSITION_DURATION; + let transition_start = next_segment.start - LAYOUT_TRANSITION_DURATION; if cursor.time >= transition_start { let prev_mode = cursor .prev_segment .map(|s| s.mode.clone()) .unwrap_or(LayoutMode::Default); - let progress = (cursor.time - transition_start) / LAYOUT_TRANSITION_DURATION; + let progress = ((cursor.time - transition_start) / LAYOUT_TRANSITION_DURATION).clamp(0.0, 1.0); ( prev_mode, next_segment.mode.clone(), ease_in_out(progress as f32) as f64, )Also applies to: 107-112
📜 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 (14)
CLAUDE.md(1 hunks)apps/desktop/src-tauri/src/recording.rs(1 hunks)apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(2 hunks)apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx(1 hunks)apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(4 hunks)apps/desktop/src/routes/editor/Timeline/index.tsx(5 hunks)apps/desktop/src/routes/editor/context.ts(3 hunks)apps/desktop/src/utils/tauri.ts(1 hunks)crates/project/src/configuration.rs(1 hunks)crates/rendering/src/composite_frame.rs(1 hunks)crates/rendering/src/layout.rs(1 hunks)crates/rendering/src/lib.rs(16 hunks)crates/rendering/src/shaders/composite-video-frame.wgsl(3 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
crates/project/src/configuration.rs (1)
apps/desktop/src/utils/tauri.ts (4)
LayoutSegment(724-728)TimelineConfiguration(730-734)TimelineSegment(735-740)ZoomSegment(780-785)
apps/desktop/src/routes/editor/Timeline/LayoutTrack.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/routes/editor/Timeline/index.tsx (3)
apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx (2)
LayoutSegmentDragState(28-31)LayoutTrack(33-486)apps/desktop/src/routes/editor/context.ts (1)
meta(303-305)crates/editor/src/editor_instance.rs (1)
meta(106-108)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
crates/editor/src/segments.rs (1)
segments(6-33)
crates/rendering/src/layout.rs (1)
apps/desktop/src/utils/tauri.ts (1)
LayoutSegment(724-728)
crates/rendering/src/lib.rs (2)
crates/rendering/src/layout.rs (2)
new(14-43)new(63-164)crates/rendering/src/layers/camera.rs (1)
new(18-42)
🪛 LanguageTool
CLAUDE.md
[grammar] ~9-~9: There might be a mistake here.
Context: ...macOS and Windows). ### Product Context - Core Purpose: Screen recording with in...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...ording with instant sharing capabilities - Target Users: Content creators, develo...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...elopers, product managers, support teams - Key Features: Instant recording, studi...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...nerated captions, collaborative comments - Business Model: Freemium SaaS with usa...
(QB_NEW_EN)
[grammar] ~82-~82: There might be a mistake here.
Context: ...chema managed through Drizzle migrations - Storage: S3-compatible (AWS, Cloudflar...
(QB_NEW_EN)
[grammar] ~85-~85: There might be a mistake here.
Context: ...s ### Auto-generated Bindings (Desktop) - NEVER EDIT: tauri.ts, queries.ts (...
(QB_NEW_EN)
[grammar] ~86-~86: There might be a mistake here.
Context: ...queries.ts(auto-generated on app load) - **NEVER EDIT**: Files underapps/desktop/...
(QB_NEW_EN)
[grammar] ~87-~87: There might be a mistake here.
Context: ...app load) - NEVER EDIT: Files under apps/desktop/src-tauri/gen/ - Icons: Auto-imported in desktop app; d...
(QB_NEW_EN)
[grammar] ~88-~88: There might be a mistake here.
Context: ...d in desktop app; do not import manually - Regeneration: These files update autom...
(QB_NEW_EN)
[grammar] ~91-~91: There might be a mistake here.
Context: ...ange ### Common Development Pain Points - Node Version: Must use Node 20 (specif...
(QB_NEW_EN)
[grammar] ~110-~110: There might be a mistake here.
Context: ...ding, camera, etc. ### Technology Stack - Package Manager: pnpm (pnpm@10.5.2) ...
(QB_NEW_EN)
[grammar] ~111-~111: There might be a mistake here.
Context: ...Package Manager*: pnpm (pnpm@10.5.2) - Build System: Turborepo - **Frontend (...
(QB_NEW_EN)
[grammar] ~112-~112: There might be a mistake here.
Context: ...m@10.5.2`) - Build System: Turborepo - Frontend (Web): React 19 + Next.js 14....
(QB_NEW_EN)
[grammar] ~113-~113: There might be a mistake here.
Context: ...: React 19 + Next.js 14.2.x (App Router) - Desktop: Tauri v2, Rust 2024, SolidSta...
(QB_NEW_EN)
[grammar] ~114-~114: There might be a mistake here.
Context: ...sktop**: Tauri v2, Rust 2024, SolidStart - Styling: Tailwind CSS (web consumes `@...
(QB_NEW_EN)
[grammar] ~115-~115: There might be a mistake here.
Context: ...nd CSS (web consumes @cap/ui/tailwind) - Server State: TanStack Query v5 on web...
(QB_NEW_EN)
[grammar] ~116-~116: There might be a mistake here.
Context: ... web; @tanstack/solid-query on desktop - Database: MySQL (PlanetScale) with Dri...
(QB_NEW_EN)
[grammar] ~117-~117: There might be a mistake here.
Context: ...**: MySQL (PlanetScale) with Drizzle ORM - AI Integration: Groq preferred, OpenAI...
(QB_NEW_EN)
[grammar] ~118-~118: There might be a mistake here.
Context: ...lback; invoked in Next.js Server Actions - Analytics: PostHog - Payments: Str...
(QB_NEW_EN)
[grammar] ~119-~119: There might be a mistake here.
Context: ... Server Actions - Analytics: PostHog - Payments: Stripe ### Critical Archite...
(QB_NEW_EN)
[grammar] ~125-~125: There might be a mistake here.
Context: ...t proper CORS, and revalidate precisely. 4. Desktop IPC: Use tauri_specta for st...
(QB_NEW_EN)
[grammar] ~128-~128: There might be a mistake here.
Context: ...ed bindings. #### Desktop event pattern Rust (emit): ```rust use specta::Type; u...
(QB_NEW_EN)
[grammar] ~155-~155: There might be a mistake here.
Context: ...ctices ### Code Organization Principles 1. Follow Local Patterns: Study neighbori...
(QB_NEW_EN)
[grammar] ~223-~223: There might be a mistake here.
Context: ...EB_URL-NEXT_PUBLIC_CAP_AWS_BUCKET, NEXT_PUBLIC_CAP_AWS_REGION-NEXT_PUBLIC_POSTHOG_KEY, NEXT_PUBLIC_POSTHOG_HOST-NEXT_PUB...
(QB_NEW_EN)
[grammar] ~227-~227: There might be a mistake here.
Context: ...tandalone output) ### Server (selected) - Core: DATABASE_URL, WEB_URL, `NEXTAU...
(QB_NEW_EN)
[grammar] ~228-~228: There might be a mistake here.
Context: ...ASE_URL, WEB_URL, NEXTAUTH_SECRET, NEXTAUTH_URL- S3:CAP_AWS_BUCKET, CAP_AWS_REGION`, ...
(QB_NEW_EN)
[grammar] ~229-~229: There might be a mistake here.
Context: ...CRET_KEY, optional CAP_AWS_ENDPOINT, CAP_AWS_BUCKET_URL- AI:GROQ_API_KEY, OPENAI_API_KEY` - E...
(QB_NEW_EN)
[grammar] ~230-~230: There might be a mistake here.
Context: ...P_AWS_BUCKET_URL- AI:GROQ_API_KEY, OPENAI_API_KEY- Email/Analytics:RESEND_API_KEY, RESE...
(QB_NEW_EN)
[grammar] ~231-~231: There might be a mistake here.
Context: ...STHOG_PERSONAL_API_KEY, DUB_API_KEY, DEEPGRAM_API_KEY- OAuth:GOOGLE_CLIENT_ID/SECRET, WORKO...
(QB_NEW_EN)
[grammar] ~232-~232: There might be a mistake here.
Context: ..._CLIENT_ID/SECRET, WORKOS_CLIENT_ID, WORKOS_API_KEY- Stripe:STRIPE_SECRET_KEY_TEST, STRIP...
(QB_NEW_EN)
[grammar] ~233-~233: There might be a mistake here.
Context: ...ET_KEY_TEST, STRIPE_SECRET_KEY_LIVE, STRIPE_WEBHOOK_SECRET- CDN signing:CLOUDFRONT_KEYPAIR_ID, C...
(QB_NEW_EN)
[grammar] ~234-~234: There might be a mistake here.
Context: ...- CDN signing: CLOUDFRONT_KEYPAIR_ID, CLOUDFRONT_KEYPAIR_PRIVATE_KEY - Optional S3 endpoints: `S3_PUBLIC_ENDPOI...
(QB_NEW_EN)
[grammar] ~239-~239: There might be a mistake here.
Context: ...Build Optimization ### Testing Strategy - Package-Specific: Check each `package....
(QB_NEW_EN)
[grammar] ~246-~246: There might be a mistake here.
Context: ...mework for crates ### Build Performance - Turborepo Caching: Aggressive caching ...
(QB_NEW_EN)
[grammar] ~247-~247: There might be a mistake here.
Context: ...: Aggressive caching across all packages - Cache Invalidation: Prefer targeted `-...
(QB_NEW_EN)
[grammar] ~248-~248: There might be a mistake here.
Context: ...targeted --filter over global rebuilds - Docker Builds: `NEXT_PUBLIC_DOCKER_BUI...
(QB_NEW_EN)
[grammar] ~249-~249: There might be a mistake here.
Context: ...ER_BUILD=true` enables standalone output - Development: Incremental builds via Ty...
(QB_NEW_EN)
[grammar] ~252-~252: There might be a mistake here.
Context: ...t references ### Performance Monitoring - Bundle Analysis: Check Next.js bundle ...
(QB_NEW_EN)
[grammar] ~260-~260: There might be a mistake here.
Context: ...ooting Common Issues ### Build Failures - "Cannot find module": Check workspace ...
(QB_NEW_EN)
[grammar] ~263-~263: There might be a mistake here.
Context: ...es - Turbo cache issues: Clear with rm -rf .turbo - Node version mismatch: Ensure Node 20 ...
(QB_NEW_EN)
[grammar] ~266-~266: There might be a mistake here.
Context: ...e Node 20 is active ### Database Issues - Migration failures: Check `packages/da...
(QB_NEW_EN)
[grammar] ~271-~271: There might be a mistake here.
Context: ...tabase db:check` ### Desktop App Issues - IPC binding errors: Restart dev server...
(QB_NEW_EN)
[grammar] ~277-~277: There might be a mistake here.
Context: ... capture permissions ### Web App Issues - Auth failures: Check NextAuth configur...
(QB_NEW_EN)
[grammar] ~287-~287: There might be a mistake here.
Context: ...omponents and let React Query take over. - Mutations should call Server Actions dir...
(QB_NEW_EN)
[grammar] ~329-~329: There might be a mistake here.
Context: ...riptions/timers. ### Next.js App Router - Prefer Server Components for SEO/initial...
(QB_NEW_EN)
[grammar] ~330-~330: There might be a mistake here.
Context: ...rate interactivity in client components. - Co-locate feature components, keep compo...
(QB_NEW_EN)
[grammar] ~345-~345: There might be a mistake here.
Context: ...y. - Directory naming: lower-case-dashed - Components: PascalCase; hooks: camelCase...
(QB_NEW_EN)
[grammar] ~346-~346: There might be a mistake here.
Context: ...calCase; hooks: camelCase starting with use - Strict TypeScript; avoid any; leverage...
(QB_NEW_EN)
[grammar] ~347-~347: There might be a mistake here.
Context: ...ript; avoid any; leverage shared types - Use Biome for linting/formatting; match ...
(QB_NEW_EN)
[grammar] ~352-~352: There might be a mistake here.
Context: ...rivacy Considerations ### Data Handling - Video Storage: S3-compatible storage w...
(QB_NEW_EN)
[grammar] ~353-~353: There might be a mistake here.
Context: ...: S3-compatible storage with signed URLs - Database: MySQL with connection poolin...
(QB_NEW_EN)
[grammar] ~354-~354: There might be a mistake here.
Context: ... with connection pooling via PlanetScale - Authentication: NextAuth with custom D...
(QB_NEW_EN)
[grammar] ~355-~355: There might be a mistake here.
Context: ...**: NextAuth with custom Drizzle adapter - API Security: CORS policies, rate limi...
(QB_NEW_EN)
[grammar] ~358-~358: There might be a mistake here.
Context: ...ia Hono middleware ### Privacy Controls - Recording Permissions: Platform-specif...
(QB_NEW_EN)
[grammar] ~359-~359: There might be a mistake here.
Context: ...ecific (macOS Screen Recording, Windows) - Data Retention: User-controlled deleti...
(QB_NEW_EN)
[grammar] ~360-~360: There might be a mistake here.
Context: ...: User-controlled deletion of recordings - Sharing Controls: Password protection,...
(QB_NEW_EN)
[grammar] ~361-~361: There might be a mistake here.
Context: ...protection, expiry dates on shared links - Analytics: PostHog with privacy-focuse...
(QB_NEW_EN)
[grammar] ~366-~366: There might be a mistake here.
Context: ...sing Pipeline ### AI Integration Points - Transcription: Deepgram API for captio...
(QB_NEW_EN)
[grammar] ~367-~367: There might be a mistake here.
Context: ...**: Deepgram API for captions generation - Metadata Generation: Groq (primary) + ...
(QB_NEW_EN)
[grammar] ~368-~368: There might be a mistake here.
Context: ...penAI (fallback) for titles/descriptions - Processing Location: All AI calls in N...
(QB_NEW_EN)
[grammar] ~369-~369: There might be a mistake here.
Context: ... AI calls in Next.js Server Actions only - Privacy: Transcripts stored in databas...
(QB_NEW_EN)
[grammar] ~381-~381: There might be a mistake here.
Context: ...s & Documentation ### Core Technologies - TanStack Query: https://tanstack.com/q...
(QB_NEW_EN)
[grammar] ~382-~382: There might be a mistake here.
Context: ...ery**: https://tanstack.com/query/latest - React Patterns: https://react.dev/lear...
(QB_NEW_EN)
[grammar] ~383-~383: There might be a mistake here.
Context: ...t.dev/learn/you-might-not-need-an-effect - Tauri v2: https://github.com/tauri-app...
(QB_NEW_EN)
[grammar] ~384-~384: There might be a mistake here.
Context: ...2**: https://github.com/tauri-apps/tauri - tauri_specta: https://github.com/oscar...
(QB_NEW_EN)
[grammar] ~385-~385: There might be a mistake here.
Context: ...//github.com/oscartbeaumont/tauri-specta - Drizzle ORM: https://orm.drizzle.team/...
(QB_NEW_EN)
[grammar] ~386-~386: There might be a mistake here.
Context: ...Drizzle ORM**: https://orm.drizzle.team/ - SolidJS: https://solidjs.com/ ### Cap...
(QB_NEW_EN)
[grammar] ~389-~389: There might be a mistake here.
Context: ...: https://solidjs.com/ ### Cap-Specific - Self-hosting: https://cap.so/docs/self...
(QB_NEW_EN)
[grammar] ~390-~390: There might be a mistake here.
Context: ...ting**: https://cap.so/docs/self-hosting - API Documentation: Generated from Type...
(QB_NEW_EN)
[grammar] ~391-~391: There might be a mistake here.
Context: ...n**: Generated from TypeScript contracts - Architecture Decisions: See individual...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
CLAUDE.md
373-373: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
382-382: Bare URL used
(MD034, no-bare-urls)
383-383: Bare URL used
(MD034, no-bare-urls)
384-384: Bare URL used
(MD034, no-bare-urls)
385-385: Bare URL used
(MD034, no-bare-urls)
386-386: Bare URL used
(MD034, no-bare-urls)
387-387: Bare URL used
(MD034, no-bare-urls)
390-390: Bare URL used
(MD034, no-bare-urls)
⏰ 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). (5)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (17)
crates/project/src/configuration.rs (1)
480-482: LGTM: timeline.layout_segments defaulting is correctUsing
#[serde(default)]ensures older projects load without errors and the field remains optional from the UI perspective.packages/ui-solid/src/auto-imports.d.ts (1)
7-94: Formatting-only changes; no API impactAmbient declarations and the export statement were reformatted. No functional changes detected.
apps/desktop/src-tauri/src/recording.rs (1)
988-989: LGTM: initialize layout_segments for new projectsAdding
layout_segments: Vec::new()aligns the default project config with the new timeline field and avoids missing-field issues when rendering or in the editor.apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (2)
172-175: Empty-state UX improvement LGTM.The neutral background, helper subtitle, and pointer-events-none reduce accidental interactions and clarify the CTA. No further changes needed.
367-395: Responsive content by width is sensible and readable.The tiered rendering (icon-only → icon+amount → full content) is a pragmatic way to keep segments readable at various scales. No functional issues noted.
apps/desktop/src/routes/editor/Timeline/index.tsx (2)
237-244: Gating LayoutTrack by hasCamera is a good call.This avoids exposing layout controls when there’s no camera feed, keeping the UI contextual.
103-105: Deletion path for layout selections is wired correctly.Routing Delete/Backspace to projectActions.deleteLayoutSegment on selection.type === "layout" aligns with the new selection variant.
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (1)
336-369: Single-clip selection behavior tweak is reasonable.Clearing selection when only one segment exists avoids opening an empty config panel. The mouseup-root/dispose pattern remains correct and prevents leaks.
Also applies to: 351-367
apps/desktop/src/routes/editor/context.ts (3)
125-128: Good fix: use in-place splice for deletionSwitching to in-place splice avoids accidentally replacing the array with the removed items. Clearing the selection afterward is consistent with other delete actions.
138-143: Consistent deletion flow and selection resetUsing splice for zoomSegments and clearing selection keeps behavior aligned with clip deletions. Looks good.
254-256: Layout selection handling verified
All downstream consumers ofeditorState.timeline.selectionhave been updated to handle{ type: "layout" }, including:
- Timeline delete key shortcuts in
apps/desktop/src/routes/editor/Timeline/index.tsx- Selection checks in
ZoomTrack.tsx,ClipTrack.tsx, andLayoutTrack.tsxNo missing handlers found.
crates/rendering/src/lib.rs (6)
568-576: Interpolate layout per-frame from timeline segments — LGTMComputing InterpolatedLayout via LayoutSegmentsCursor against project.timeline.layout_segments is clean and aligns with the zoom interpolation pattern.
638-639: Screen opacity driven by layout — LGTMFeeding layout.screen_opacity into CompositeVideoFrameUniforms.opacity cleanly gates the display layer.
645-662: Camera uniforms integrate layout state and shape-aware cropping — LGTM
- Properly gate camera with should_render_camera().
- Apply layout.camera_scale to zoomed_size.
- Handle Square vs Source crop_bounds correctly; avoids stretching.
- Opacity from regular_camera_transition_opacity covers transitions gracefully.
Also applies to: 707-716, 747-750
752-811: Camera-only path is well-isolated and fills output with safe croppingThe camera_only uniforms ignore shadow/rounding and use aspect-aware cropping to fill the output without distortion — appropriate for fullscreen transitions. Opacity driven by camera_only_transition_opacity is a good separation of responsibilities.
1009-1030: Render guards match layout semantics
- Screen and cursor are gated by should_render_screen (hidden in camera-only).
- camera_only renders during any CameraOnly involvement.
- Regular camera renders only when needed and with layout-driven opacity.
This staging prevents double-render and blends transitions predictably.
1169-1173: Confirm white fallback in parse_color_componentThe implementation in crates/rendering/src/lib.rs (fn parse_color_component, lines 1158–1173) now defaults R/G/B to 1.0 (white) whenever parsing fails. Previously this would have produced black (0.0) channels, so this change may lead to white text or backgrounds if an invalid or too-short hex string is passed.
Please confirm whether switching the fallback from black to white was intentional (and update any docs or add tests), or consider reverting to the original black fallback behavior.
• File: crates/rendering/src/lib.rs
fn parse_color_component(hex_color: &str, index: usize) -> f32
– match index { 0 => 1.0, 1 => 1.0, 2 => 1.0, _ => 1.0 }
| export type LayoutSegment = { | ||
| start: number; | ||
| end: number; | ||
| mode?: "default" | "cameraOnly" | "hideCamera"; | ||
| }; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Type bridging looks correct; double-check defaulting/serde behavior for layoutSegments/mode.
The TS types for LayoutSegment and TimelineConfiguration.layoutSegments align with the PR’s Rust-side intent. Since this file is generated, no action is needed here. Two checks to avoid runtime/type drift:
- Ensure Rust TimelineConfiguration has layout_segments with serde(default) so older projects without this field deserialize to an empty list, not None.
- Confirm the optional LayoutSegment.mode in TS corresponds to either Option in Rust or a LayoutMode with a default. If omitted in TS, the backend should treat it as Default.
Run this to confirm Rust-side defaults and field names:
Also applies to: 730-734
🏁 Script executed:
#!/bin/bash
# Verify layout_segments presence and serde(default)
rg -nP --type=rust -C3 '\blayout_segments\b' | sed -n '1,200p'
# Verify LayoutSegment struct and LayoutMode enum
rg -nP --type=rust -C3 'struct\s+LayoutSegment|enum\s+LayoutMode' | sed -n '1,200p'Length of output: 2843
Ensure defaulting for LayoutSegment.mode to avoid deserialization errors
We’ve verified:
layout_segments: Vec<LayoutSegment>is annotated with#[serde(default)](crates/project/src/configuration.rs:481), so missing the entire field falls back to an empty list.LayoutSegmentis defined without a default on itsmodefield (crates/project/src/configuration.rs:469–472):#[derive(Type, Serialize, Deserialize, Clone, Debug)] #[serde(rename_all = "camelCase")] pub struct LayoutSegment { pub start: f64, pub end: f64, pub mode: LayoutMode, }
LayoutModeis a non-optional enum (Default,CameraOnly,HideCamera) with noDefaultimpl.
Because the generated TS type makes mode optional (mode?: …), omitting it in JSON will cause Rust deserialization to fail. To align behaviors, please update the Rust side by either:
- Adding
#[serde(default)]to themodefield and deriving or implementingDefaultforLayoutMode(so missing modes map toLayoutMode::Default), or - Changing
modetoOption<LayoutMode>if you prefer explicit optionality.
No TS changes are needed—the generator can continue producing mode?: ….
🤖 Prompt for AI Agents
In apps/desktop/src/utils/tauri.ts around lines 724 to 729, the generated
TypeScript makes LayoutSegment.mode optional but the Rust LayoutSegment
currently requires a non-optional LayoutMode, causing deserialization failures
when mode is omitted; fix by updating the Rust type: either add
#[serde(default)] to the mode field and implement/derive Default for LayoutMode
so missing values map to LayoutMode::Default, or change the mode field to
Option<LayoutMode> to allow explicit omission—apply one of these two changes in
crates/project/src/configuration.rs where LayoutSegment and LayoutMode are
defined.
…t for deleting zoom segments
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
apps/desktop/src/routes/editor/Timeline/index.tsx (2)
79-90: Pause playhead updates while dragging layout segments too.This was already flagged in a prior review; please gate on both drag states to prevent jitter while moving layout segments.
Apply:
- if (zoomSegmentDragState.type !== "moving") { + if ( + zoomSegmentDragState.type !== "moving" && + layoutSegmentDragState.type !== "moving" + ) {
141-152: Don’t clear selection when either segment type is being dragged.Also previously noted: extend the guard to include layoutSegmentDragState to avoid clearing selection mid-drag.
Apply:
- if (zoomSegmentDragState.type === "idle") { + if ( + zoomSegmentDragState.type === "idle" && + layoutSegmentDragState.type === "idle" + ) { setEditorState("timeline", "selection", null); }
🧹 Nitpick comments (5)
apps/desktop/src/routes/editor/context.ts (2)
144-160: Bulk delete: dedupe and bounds-check indices to avoid accidental removals.Current approach sorts and splices descending, which handles shifting, but duplicates or out-of-range indices can still lead to surprises. Consider deduping and filtering to valid indices before splicing.
Apply this diff inside the produce block to harden the routine:
- // Sort indices in descending order to avoid index shifting issues - const sortedIndices = [...segmentIndices].sort((a, b) => b - a); - sortedIndices.forEach((index) => { - s.splice(index, 1); - }); + // Dedupe, bounds-check, and sort descending to avoid index shifting issues + const uniqueDesc = Array.from(new Set(segmentIndices)) + .filter((i) => Number.isInteger(i) && i >= 0 && i < s.length) + .sort((a, b) => b - a); + uniqueDesc.forEach((index) => { + s.splice(index, 1); + });
161-172: Add parity: consider a bulk delete for layout segments.You added deleteLayoutSegment; mirroring deleteZoomSegments with deleteLayoutSegments would help parity and simplify callers that operate on multi-select.
I can draft an implementation if you want to support multi-select layout deletions in this PR.
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (3)
43-54: Hover-reset effect addresses stuck state after deletions.This resolves the “hoveringSegment stays true after deletion” issue for the empty-list case.
If you also want to handle “delete hovered segment while others remain,” consider clearing hoveringSegment whenever hoveredTime becomes undefined, or when the specific DOM element is removed (e.g., track state or selection changes).
213-273: Multi-select toggle: ensure uniqueness and stable ordering.The toggle logic is correct but can accumulate duplicates and produce unsorted arrays depending on user actions. Normalize indices for stability.
Apply this diff within the non-moved click path:
- if (e.ctrlKey || e.metaKey) { + if (e.ctrlKey || e.metaKey) { if (currentSelection?.type === "zoom") { // If we already have zoom selections if ( "indices" in currentSelection && Array.isArray(currentSelection.indices) ) { - // Toggle this segment in the selection - const newIndices = currentSelection.indices.includes( - segmentIndex, - ) - ? currentSelection.indices.filter( - (idx) => idx !== segmentIndex, - ) - : [...currentSelection.indices, segmentIndex]; + // Toggle with uniqueness + sorted order + const set = new Set(currentSelection.indices); + if (set.has(segmentIndex)) set.delete(segmentIndex); + else set.add(segmentIndex); + const newIndices = Array.from(set).sort((a, b) => a - b); setEditorState( "timeline", "selection", newIndices.length > 0 ? { type: "zoom", indices: newIndices } : null, ); } else if ( "index" in currentSelection && typeof currentSelection.index === "number" ) { - // Convert single selection to multi-selection - const newIndices = - currentSelection.index === segmentIndex - ? [] // Deselect if clicking the same segment - : [currentSelection.index, segmentIndex]; + // Convert single selection to multi-selection (dedup + sorted) + const base = new Set<number>(); + if (currentSelection.index !== segmentIndex) { + base.add(currentSelection.index); + base.add(segmentIndex); + } + const newIndices = Array.from(base).sort((a, b) => a - b); setEditorState( "timeline", "selection", newIndices.length > 0 ? { type: "zoom", indices: newIndices } : null, ); }
310-333: Minor perf nit: reuse the reactive index rather than findIndex.For isSelected, you can use i() directly instead of findIndex over zoomSegments each time; keeps computations O(1).
Example:
- const segmentIndex = project.timeline?.zoomSegments?.findIndex( - (s) => s.start === segment.start && s.end === segment.end, - ); + const segmentIndex = i();
📜 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 (4)
apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx(1 hunks)apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(6 hunks)apps/desktop/src/routes/editor/Timeline/index.tsx(6 hunks)apps/desktop/src/routes/editor/context.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/desktop/src/routes/editor/Timeline/index.tsx (3)
apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx (2)
LayoutSegmentDragState(27-30)LayoutTrack(32-488)apps/desktop/src/routes/editor/context.ts (1)
meta(321-323)crates/editor/src/editor_instance.rs (1)
meta(106-108)
⏰ 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: Clippy
- 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 (9)
apps/desktop/src/routes/editor/context.ts (2)
123-129: Deletion fix is correct (splice without assigning).Good catch mutating the draft with splice instead of assigning the splice result. This avoids replacing the array with the removed items.
138-143: Consistent deletion + selection reset.Zoom segment deletion mirrors clip deletion semantics and correctly clears the selection. Looks good.
apps/desktop/src/routes/editor/Timeline/index.tsx (5)
99-111: Bulk vs single zoom deletion handling is solid.Correct branching for multi-select (indices) vs single selection (index), delegating to the new context actions.
113-115: Layout deletion wiring looks correct.Keyboard delete flows through to deleteLayoutSegment as expected.
120-123: Escape-to-deselect is a nice UX addition.Clear and consistent with the selection model.
251-258: Conditional render of LayoutTrack is appropriate.Gating the LayoutTrack behind meta().hasCamera and wiring drag state + playhead handler aligns with the overall design.
284-284: Marking dot class tweak is fine.No functional impact; style looks consistent with surrounding code.
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (2)
172-177: Empty-state UX improvements look good.Pointer-events: none keeps the track clickable; the message is clearer.
441-468: Ignore missing import suggestion for IconLucideSearch
IconLucideSearch is provided by our icon auto-import setup (see packages/ui-solid/src/auto-imports.d.ts) and does not require an explicit import here. No changes needed.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (4)
597-615: Add single-selection/index guard for Layout config (align with Zoom)Unlike the Zoom guard, the Layout guard doesn’t validate
indexpresence/type. Adding it avoids accidental rendering when multi-selected or when selection shape differs.Apply this diff:
<Show when={(() => { const layoutSelection = selection(); if (layoutSelection.type !== "layout") return; + // Only show config panel for single layout segment selection + if ( + !("index" in layoutSelection) || + typeof layoutSelection.index !== "number" + ) { + return; + } + const segment = project.timeline?.layoutSegments?.[layoutSelection.index]; if (!segment) return; return { selection: layoutSelection, segment }; })()} >
1864-1873: Reset preview loading state when switching video sourcesWhen
video.srcchanges,loaded()remains true until the nextonloadeddata, so the “Loading preview…” overlay won’t show for new sources. Reset it to false on source change.Apply this diff (uses a microtask to avoid ordering issues):
createEffect(() => { const path = convertFileSrc( // TODO: this shouldn't be so hardcoded `${editorInstance.path}/content/segments/segment-${segmentIndex()}/display.mp4`, ); - video.src = path; - // Force reload if video fails to load - video.load(); + // Reset loading state until the new source is ready + queueMicrotask(() => setLoaded(false)); + video.src = path; + // Force reload to ensure the new source is loaded + video.load(); });Alternatively (cleaner), move the
const [loaded, setLoaded] = createSignal(false)declaration above this effect and callsetLoaded(false)directly.
1884-1889: Avoid accumulating canplay listeners; use{ once: true }If
relativeTime()updates before the video becomes ready, multiplecanplaylisteners can accrue. Use a one-off listener.- video.addEventListener("canplay", handleCanPlay); + video.addEventListener("canplay", handleCanPlay, { once: true });
1937-1944: Consider capping video load retries to avoid tight error loopsOn persistent failures, the current logic will keep retrying every 100ms. Add a retry cap.
For example:
// Near the video element creation let loadRetries = 0; const MAX_RETRIES = 3; // ... video.onerror = (e) => { console.error("Failed to load video for zoom preview:", e); if (loadRetries < MAX_RETRIES) { loadRetries++; setTimeout(() => video.load(), 100); } else { console.error("Giving up after MAX_RETRIES"); } };
📜 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/ConfigSidebar.tsx(9 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
apps/desktop/src/utils/tauri.ts (1)
LayoutSegment(724-728)apps/desktop/src/routes/editor/ui.tsx (2)
EditorButton(352-412)Field(25-47)
⏰ 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: Clippy
- 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 (3)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (3)
576-582: Good: single-selection guard for Zoom segment configGuarding Zoom config behind a single-segment selection prevents ambiguous edits and matches the intended UX.
2027-2042: Nice UX: loading overlay + opacity transition for zoom previewThe loading gate, fade-in, and absolute overlay improve perceived responsiveness and prevent janky renders.
2156-2261: LayoutSegmentConfig: solid addition with clear UX
- Done/Delete actions follow existing patterns.
- Mode tabs map cleanly to
LayoutSegment["mode"], with sensible default fallback.- Indicator/description update based on mode is a nice touch.
Only follow-ups are the import fix (see earlier comment) and the optional selection guard (see layout gating comment).
| type LayoutSegment, | ||
| type StereoMode, | ||
| type TimelineSegment, | ||
| type ZoomSegment, | ||
| } from "~/utils/tauri"; | ||
| import IconLucideMonitor from "~icons/lucide/monitor"; | ||
| import IconLucideSparkles from "~icons/lucide/sparkles"; |
There was a problem hiding this comment.
Missing IconLucideLayout import will break rendering
<IconLucideLayout /> is used in LayoutSegmentConfig but not imported. This will cause a runtime/build error.
Apply this diff to add the missing import:
import {
type BackgroundSource,
type CameraShape,
commands,
type LayoutSegment,
type StereoMode,
type TimelineSegment,
type ZoomSegment,
} from "~/utils/tauri";
+import IconLucideLayout from "~icons/lucide/layout";
import IconLucideMonitor from "~icons/lucide/monitor";📝 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.
| type LayoutSegment, | |
| type StereoMode, | |
| type TimelineSegment, | |
| type ZoomSegment, | |
| } from "~/utils/tauri"; | |
| import IconLucideMonitor from "~icons/lucide/monitor"; | |
| import IconLucideSparkles from "~icons/lucide/sparkles"; | |
| import { | |
| type BackgroundSource, | |
| type CameraShape, | |
| commands, | |
| type LayoutSegment, | |
| type StereoMode, | |
| type TimelineSegment, | |
| type ZoomSegment, | |
| } from "~/utils/tauri"; | |
| import IconLucideLayout from "~icons/lucide/layout"; | |
| import IconLucideMonitor from "~icons/lucide/monitor"; | |
| import IconLucideSparkles from "~icons/lucide/sparkles"; |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/ConfigSidebar.tsx around lines 46 to 52, the
component uses <IconLucideLayout /> but the import is missing; add the missing
import statement for IconLucideLayout from the icon package (consistent with the
other icon imports, e.g., import IconLucideLayout from "~icons/lucide/layout";)
so the LayoutSegmentConfig can render without build/runtime errors.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src/routes/editor/Timeline/index.tsx (1)
81-84: Pause playhead during any non-idle drag state (not just "moving").Condition currently only blocks updates while
type === "moving". If other drag states exist (e.g., resizing), the playhead will still update and may cause jitter. Prefer checking explicitly foridle.Apply this diff:
- if ( - zoomSegmentDragState.type !== "moving" && - layoutSegmentDragState.type !== "moving" - ) { + if ( + zoomSegmentDragState.type === "idle" && + layoutSegmentDragState.type === "idle" + ) {
🧹 Nitpick comments (4)
crates/rendering/src/composite_frame.rs (1)
32-53: Default now sets opacity = 1.0 — nice; consider a concise impl using Zeroable::zeroed()Functionally perfect. Minor readability/maintenance tweak: leverage Zeroable to avoid listing every field, which helps when fields are added/removed later.
impl Default for CompositeVideoFrameUniforms { fn default() -> Self { - Self { - crop_bounds: Default::default(), - target_bounds: Default::default(), - output_size: Default::default(), - frame_size: Default::default(), - velocity_uv: Default::default(), - target_size: Default::default(), - rounding_px: Default::default(), - mirror_x: Default::default(), - motion_blur_amount: Default::default(), - camera_motion_blur_amount: Default::default(), - shadow: Default::default(), - shadow_size: Default::default(), - shadow_opacity: Default::default(), - shadow_blur: Default::default(), - opacity: 1.0, - _padding: Default::default(), - } + Self { opacity: 1.0, ..bytemuck::Zeroable::zeroed() } } }apps/desktop/src/routes/editor/Timeline/index.tsx (3)
102-114: Deletion flow: clear selection after zoom deletions and prevent default browser behavior.
- After deleting zoom segments, selection can point to stale indices. Clear it.
- Also prevent default Backspace/Delete behavior to avoid unintended navigation or OS-level actions (especially in Tauri/Web contexts).
Apply this diff to clear selection after deletions:
if ("indices" in selection && Array.isArray(selection.indices)) { // Use bulk deletion for better performance projectActions.deleteZoomSegments(selection.indices); + setEditorState("timeline", "selection", null); } else if ( "index" in selection && typeof selection.index === "number" ) { // Handle single zoom segment deletion projectActions.deleteZoomSegment(selection.index); + setEditorState("timeline", "selection", null); }Additionally, consider guarding against active text inputs and preventing default at the top of the keydown handler (outside the changed range), e.g.:
createEventListener(window, "keydown", (e) => { const hasNoModifiers = !e.shiftKey && !e.ctrlKey && !e.metaKey && !e.altKey; const target = e.target as Element | null; const isTypingContext = !!target && (target.closest("input, textarea, [contenteditable=''], [contenteditable='true']") !== null); if (isTypingContext) return; if ((e.code === "Backspace" || e.code === "Delete") && hasNoModifiers) { e.preventDefault(); e.stopPropagation(); // ... existing deletion logic } });
116-118: Also clear selection after layout-segment deletion.Resetting selection avoids dangling references and keeps UX consistent with zoom deletions.
- projectActions.deleteLayoutSegment(selection.index); + projectActions.deleteLayoutSegment(selection.index); + setEditorState("timeline", "selection", null);
144-151: Don’t clear selection on mouseup when a layout segment was just dragged.This block only checks the zoom drag state; if a layout drag occurred, selection will be cleared incorrectly.
Update the guard to consider both drag states:
- if (zoomSegmentDragState.type === "idle") { + if ( + zoomSegmentDragState.type === "idle" && + layoutSegmentDragState.type === "idle" + ) { setEditorState("timeline", "selection", null); }
📜 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 (7)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx(2 hunks)apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx(1 hunks)apps/desktop/src/routes/editor/Timeline/index.tsx(6 hunks)crates/project/src/configuration.rs(1 hunks)crates/rendering/src/composite_frame.rs(2 hunks)crates/rendering/src/shaders/composite-video-frame.wgsl(2 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/ui-solid/src/auto-imports.d.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/project/src/configuration.rs
- crates/rendering/src/shaders/composite-video-frame.wgsl
- apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx
- apps/desktop/src/routes/editor/Timeline/ClipTrack.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 (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
crates/rendering/src/composite_frame.rs (2)
11-11: No inadvertent zeroed() calls for CompositeVideoFrameUniforms foundI ran the search and didn’t find any uses of
CompositeVideoFrameUniforms::zeroed()ormem::zeroed()in the codebase. KeepingPod + Zeroablealongside an explicitDefaultis safe—no unintended zero‐initialization occurs.Approved.
28-30: Opacity doc added and WGSL alignment confirmed
- Added one-line doc to clarify the range and semantics of
opacity.- Verified that in
crates/rendering/src/shaders/composite-video-frame.wgsl, theUniformsstruct declaresopacity: f32immediately aftershadow_blur: f32(lines 15–16), matching the Rust layout.- Optional: consider setting
min_binding_sizeon the uniform’s#[repr(C)]layout to catch any future drift at runtime.Applied diff:
- pub opacity: f32, + /// Final alpha multiplier in [0.0, 1.0]; applied in the composite shader. + pub opacity: f32,apps/desktop/src/routes/editor/Timeline/index.tsx (6)
14-14: LayoutTrack import and public type usage look correct.Importing both
LayoutTrackand its publicLayoutSegmentDragStatetype is consistent with the new feature integration.
29-29: Good use of meta() from editor context for feature-gating.Pulling
metahere enables conditional rendering of the layout track later. Looks clean.
77-77: Non-reactive local drag state is fine here.Using a
letforlayoutSegmentDragStatemirrors the existing zoom approach and is adequate for gating event-driven logic without introducing reactivity.
123-125: Escape to clear selection: LGTM.Simple and expected UX to deselect all.
254-261: Conditional LayoutTrack rendering and wiring look correct.Gating on
meta().hasCameraand forwardingonDragStateChanged+handleUpdatePlayheadmatches the intended UX and keeps interactions consistent across tracks.
287-287: Marking dot style tweak: LGTM.Using
bg-currentkeeps the marker color aligned with the parent text color.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (2)
61-64: Good NaN hardening; consider centralizing min-dB and guarding gain (nit).You fixed the sample-NaN source. As a minor improvement, DRY the -60 sentinel and also guard gain to avoid accidental NaN if it ever becomes non-finite.
- const norm = (w: number) => { - const ww = Number.isFinite(w) ? w : -60; - return 1.0 - Math.max(ww + gain, -60) / -60; - }; + const MIN_DB = -60; + const norm = (w: number) => { + const ww = Number.isFinite(w) ? w : MIN_DB; + const gg = Number.isFinite(gain) ? gain : 0; + return 1.0 - Math.max(ww + gg, MIN_DB) / MIN_DB; + };If you prefer keeping MIN_DB outside this block, you can define it once near WaveformCanvas and reuse it:
const MIN_DB = -60;
358-373: Confirm UX intent: Click on single-segment clips now clears selection instead of opening config.This changes sidebar behavior; in single-clip timelines, users can’t open the clip config panel via click anymore. If that’s the intended UX (because there’s “nothing to configure”), all good. Otherwise, we might want a different rule (e.g., only avoid opening when we’re in a default/fallback single segment).
Optionally make the “single segment” check explicit against real timeline data (not the computed fallback) for clarity:
- const hasOnlyOneSegment = segments().length === 1; + const hasOnlyOneSegment = + (project.timeline?.segments?.length ?? 0) <= 1;Suggested QA:
- With exactly one real clip segment, click the clip: sidebar deselects and does not open clip config.
- With multiple segments, click a clip: sidebar selects { type: "clip", index } and opens config.
- Ensure clicking a clip also clears any existing layout/zoom selection as intended and that props.handleUpdatePlayhead(e) still advances the playhead correctly.
📜 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/ClipTrack.tsx(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (2)
crates/media/src/feeds/audio.rs (1)
gain(157-159)crates/editor/src/segments.rs (1)
segments(6-33)
⏰ 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 (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
apps/desktop/src/routes/editor/Timeline/ClipTrack.tsx (5)
74-77: LGTM: current sample fallback prevents NaN amplitudes.Using a finite guard with -60 dB sentinel here is solid and avoids NaNs in amplitude.
82-85: LGTM: previous-sample fallback is correct.This mirrors the current-sample guard and ensures cubic path control points remain finite at segment boundaries.
269-269: LGTM: class reordering only.No behavioral impact; safe UI tidy-up.
307-307: LGTM: wrapper class reorder only.Styling-only change, no functional differences.
560-577: LGTM: style-only changes.Wrapper class reorder around the right marker has no functional impact.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
crates/rendering/src/lib.rs (4)
345-350: ProjectUniforms extended with camera_only and layout: solid addition.The added fields make layout-driven rendering explicit. Ensure any consumers cloning
ProjectUniformsare aware of the increased payload (minor).
643-651: Avoid preparing the regular camera when it won’t render (perf).
layout.should_render_camera()can be true in CameraOnly segments depending on base values; the render pass later correctly gates withregular_camera_transition_opacity(). To avoid unnecessary .prepare() work, gate the camera Option on the same opacity heuristic used at render-time.Apply this diff:
- let camera = options - .camera_size - .filter(|_| !project.camera.hide && layout.should_render_camera()) + let camera = options + .camera_size + // Only prepare when the regular camera overlay will actually be visible + .filter(|_| !project.camera.hide && layout.regular_camera_transition_opacity() > 0.01) .map(|camera_size| { let output_size = [output_size.0 as f32, output_size.1 as f32];Also applies to: 658-663, 707-716, 747-750
752-813: Camera-only pass looks correct and robust.
- The output-aspect-driven crop prevents unwanted zoom when the user has “Square” camera shape – nice.
- Opacity, blur, and zoom are tied to layout progress and feel consistent.
Minor optional: you could similarly avoid preparing this layer when
camera_only_transition_opacity()is ~0 to shave some cycles during non-relevant frames.
1174-1179: Nit: default color component fallback.Falling back to
1.0yields white on invalid hex input. If that’s intentional, ignore. Otherwise consider0.0(black) or surfacing an error upstream to avoid silent whitening.crates/rendering/src/layout.rs (4)
69-116: Remove unnecessary clone() calls on Copy types (LayoutMode).Clippy is right here;
LayoutModeis Copy, so the.clone()calls are redundant and add noise.Apply this diff:
- let prev_mode = cursor - .prev_segment - .map(|s| s.mode.clone()) + let prev_mode = cursor + .prev_segment + .map(|s| s.mode) .unwrap_or(LayoutMode::Default); @@ - prev_mode, - segment.mode.clone(), + prev_mode, + segment.mode, ease_in_out(progress as f32) as f64, ) } else if cursor.time >= transition_end && cursor.time < segment.end { if let Some(next_seg) = cursor.next_segment() { let progress = (cursor.time - transition_end) / LAYOUT_TRANSITION_DURATION; ( - segment.mode.clone(), - next_seg.mode.clone(), + segment.mode, + next_seg.mode, ease_in_out(progress as f32) as f64, ) } else { let progress = (cursor.time - transition_end) / LAYOUT_TRANSITION_DURATION; ( - segment.mode.clone(), + segment.mode, LayoutMode::Default, ease_in_out(progress as f32) as f64, ) } } else { - (segment.mode.clone(), segment.mode.clone(), 1.0) + (segment.mode, segment.mode, 1.0) } } else if let Some(next_segment) = cursor.next_segment() { let transition_start = next_segment.start - LAYOUT_TRANSITION_DURATION; if cursor.time >= transition_start { - let prev_mode = cursor - .prev_segment - .map(|s| s.mode.clone()) + let prev_mode = cursor + .prev_segment + .map(|s| s.mode) .unwrap_or(LayoutMode::Default); let progress = (cursor.time - transition_start) / LAYOUT_TRANSITION_DURATION; ( - prev_mode, - next_segment.mode.clone(), + prev_mode, + next_segment.mode, ease_in_out(progress as f32) as f64, )
125-135: Collapse nested else-if and remove redundant clones.Simplify control flow per Clippy and drop
.clone()onLayoutMode.Apply this diff:
- } else { - if let Some(prev_segment) = cursor.prev_segment { - if cursor.time < prev_segment.end + 0.05 { - (prev_segment.mode.clone(), LayoutMode::Default, 1.0) - } else { - (LayoutMode::Default, LayoutMode::Default, 1.0) - } - } else { - (LayoutMode::Default, LayoutMode::Default, 1.0) - } - }; + } else if let Some(prev_segment) = cursor.prev_segment { + if cursor.time < prev_segment.end + 0.05 { + (prev_segment.mode, LayoutMode::Default, 1.0) + } else { + (LayoutMode::Default, LayoutMode::Default, 1.0) + } + } else { + (LayoutMode::Default, LayoutMode::Default, 1.0) + };
200-205: Drop clone() in layout_mode selection (Copy type).Small cleanliness improvement.
Apply this diff:
- layout_mode: if transition_progress > 0.5 { - next_mode.clone() - } else { - current_mode.clone() - }, + layout_mode: if transition_progress > 0.5 { next_mode } else { current_mode },
45-49: Potential future-proofing: next_segment assumes sorted, non-overlapping segments.If segments are ever unsorted,
find(|s| s.start > time)could pick an arbitrary “next”. If unsorted segments are possible, consider storing the current index in the cursor and returningsegments[idx + 1]when available, or sorting once upfront.
📜 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 (3)
crates/rendering/src/layout.rs(1 hunks)crates/rendering/src/lib.rs(17 hunks)packages/ui-solid/src/auto-imports.d.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/ui-solid/src/auto-imports.d.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
crates/rendering/src/layout.rs (2)
apps/desktop/src/utils/tauri.ts (1)
LayoutSegment(724-728)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/lib.rs (2)
crates/rendering/src/layout.rs (2)
new(14-43)new(66-212)crates/rendering/src/layers/camera.rs (1)
new(18-42)
🪛 GitHub Check: Clippy
crates/rendering/src/layout.rs
[warning] 113-113: using clone on type LayoutMode which implements the Copy trait
warning: using clone on type LayoutMode which implements the Copy trait
--> crates/rendering/src/layout.rs:113:21
|
113 | next_segment.mode.clone(),
| ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try removing the clone call: next_segment.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 108-108: using clone on type LayoutMode which implements the Copy trait
warning: using clone on type LayoutMode which implements the Copy trait
--> crates/rendering/src/layout.rs:108:30
|
108 | .map(|s| s.mode.clone())
| ^^^^^^^^^^^^^^ help: try removing the clone call: s.mode
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#clone_on_copy
[warning] 125-135: this else { if .. } block can be collapsed
warning: this else { if .. } block can be collapsed
--> crates/rendering/src/layout.rs:125:16
|
125 | } else {
| _______^
126 | | if let Some(prev_segment) = cursor.prev_segment {
127 | | if cursor.time < prev_segment.end + 0.05 {
128 | | (prev_segment.mode.clone(), LayoutMode::Default, 1.0)
... |
135 | | };
| |^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_else_if
= note: #[warn(clippy::collapsible_else_if)] on by default
help: collapse nested if block
|
125 ~ } else if let Some(prev_segment) = cursor.prev_segment {
126 + if cursor.time < prev_segment.end + 0.05 {
127 + (prev_segment.mode.clone(), LayoutMode::Default, 1.0)
128 + } else {
129 + (LayoutMode::Default, LayoutMode::Default, 1.0)
130 + }
131 + } else {
132 + (LayoutMode::Default, LayoutMode::Default, 1.0)
133 ~ };
|
[warning] 101-101: using clone on type LayoutMode which implements the Copy trait
warning: using clone on type LayoutMode which implements the Copy trait
--> crates/rendering/src/layout.rs:101:40
|
101 | (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] 101-101: using clone on type LayoutMode which implements the Copy trait
warning: using clone on type LayoutMode which implements the Copy trait
--> crates/rendering/src/layout.rs:101:18
|
101 | (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] 95-95: using clone on type LayoutMode which implements the Copy trait
warning: using clone on type LayoutMode which implements the Copy trait
--> crates/rendering/src/layout.rs:95:25
|
95 | 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] 89-89: using clone on type LayoutMode which implements the Copy trait
warning: using clone on type LayoutMode which implements the Copy trait
--> crates/rendering/src/layout.rs:89:25
|
89 | 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] 88-88: using clone on type LayoutMode which implements the Copy trait
warning: using clone on type LayoutMode which implements the Copy trait
--> crates/rendering/src/layout.rs:88:25
|
88 | 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] 81-81: using clone on type LayoutMode which implements the Copy trait
warning: using clone on type LayoutMode which implements the Copy trait
--> crates/rendering/src/layout.rs:81:21
|
81 | 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 LayoutMode which implements the Copy trait
warning: using clone on type LayoutMode which implements the Copy trait
--> crates/rendering/src/layout.rs:76:30
|
76 | .map(|s| s.mode.clone())
| ^^^^^^^^^^^^^^ help: try removing the clone call: s.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). (4)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
crates/rendering/src/lib.rs (5)
29-29: Good modularization: new layout module is cleanly integrated.Bringing
layoutin as a private module and importing its API locally keeps the public surface minimal. No concerns here.Also applies to: 39-39
568-575: Per-frame InterpolatedLayout construction: LGTM.Using the per-frame time and timeline layout_segments to drive layout interpolation is appropriate and matches the new feature’s intent.
620-621: Display uniforms driven by layout are correct.
- Incorporating
layout.screen_blurintomotion_blur_amountand settingopacityfromlayout.screen_opacityaligns the shader behavior with layout transitions.Also applies to: 638-639
907-966: Preparation path extended to camera_only: LGTM.The pattern mirroring the regular camera path using an Option triple keeps the code cohesive.
970-976: Verified: No externalRendererLayers::rendercalls remain
The only invocation is in crates/rendering/src/lib.rs at line 1151, which already passes&uniforms. No other call sites in the workspace rely on the old signature.
| fn get_layout_values(mode: &LayoutMode) -> (f64, f64, f64) { | ||
| match mode { | ||
| LayoutMode::Default => (1.0, 1.0, 1.0), | ||
| LayoutMode::CameraOnly => (1.0, 1.0, 1.0), | ||
| LayoutMode::HideCamera => (0.0, 1.0, 1.0), | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix CameraOnly base values: screen should be hidden.
Currently get_layout_values(LayoutMode::CameraOnly) returns a screen_opacity of 1.0, causing the screen to render behind the full-frame camera and breaking gating decisions. Set it to 0.0 so the screen is actually hidden in CameraOnly.
Apply this diff:
fn get_layout_values(mode: &LayoutMode) -> (f64, f64, f64) {
match mode {
LayoutMode::Default => (1.0, 1.0, 1.0),
- LayoutMode::CameraOnly => (1.0, 1.0, 1.0),
+ // Camera visible, screen hidden, neutral scale
+ LayoutMode::CameraOnly => (1.0, 0.0, 1.0),
LayoutMode::HideCamera => (0.0, 1.0, 1.0),
}
}📝 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.
| fn get_layout_values(mode: &LayoutMode) -> (f64, f64, f64) { | |
| match mode { | |
| LayoutMode::Default => (1.0, 1.0, 1.0), | |
| LayoutMode::CameraOnly => (1.0, 1.0, 1.0), | |
| LayoutMode::HideCamera => (0.0, 1.0, 1.0), | |
| } | |
| } | |
| fn get_layout_values(mode: &LayoutMode) -> (f64, f64, f64) { | |
| match mode { | |
| LayoutMode::Default => (1.0, 1.0, 1.0), | |
| // Camera visible, screen hidden, neutral scale | |
| LayoutMode::CameraOnly => (1.0, 0.0, 1.0), | |
| LayoutMode::HideCamera => (0.0, 1.0, 1.0), | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/rendering/src/layout.rs around lines 214 to 220, get_layout_values
currently returns (1.0, 1.0, 1.0) for LayoutMode::CameraOnly which leaves the
screen visible; change the returned tuple for CameraOnly to (1.0, 0.0, 1.0) so
the screen_opacity is 0.0 and the screen is actually hidden in CameraOnly mode.
| pub fn should_render_screen(&self) -> bool { | ||
| true | ||
| } |
There was a problem hiding this comment.
Implement effective gating: don’t always render the screen.
This always returns true, so the renderer never skips the display/cursor passes even when screen_opacity is ~0. Use a threshold on the computed opacity.
Apply this diff:
- pub fn should_render_screen(&self) -> bool {
- true
- }
+ pub fn should_render_screen(&self) -> bool {
+ self.screen_opacity > 0.01
+ }📝 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.
| pub fn should_render_screen(&self) -> bool { | |
| true | |
| } | |
| pub fn should_render_screen(&self) -> bool { | |
| self.screen_opacity > 0.01 | |
| } |
🤖 Prompt for AI Agents
In crates/rendering/src/layout.rs around lines 230 to 232, the method currently
unconditionally returns true which prevents skipping rendering when screen
opacity is effectively zero; replace the hardcoded true with a calculation of
the effective screen opacity (derive it from the existing fields used to
determine visual opacity, e.g., screen_opacity possibly multiplied by any
parent/background opacity), clamp the result to 0.0..1.0, guard against NaN, and
return opacity > a small threshold (e.g., 0.01) so the screen/cursor passes are
skipped when opacity is negligible.
| if uniforms.layout.should_render_screen() { | ||
| let mut pass = render_pass!(session.current_texture_view(), wgpu::LoadOp::Load); | ||
| self.display.render(&mut pass); | ||
| } | ||
|
|
||
| { | ||
| if uniforms.layout.should_render_screen() { | ||
| let mut pass = render_pass!(session.current_texture_view(), wgpu::LoadOp::Load); | ||
| self.cursor.render(&mut pass); | ||
| } | ||
|
|
There was a problem hiding this comment.
Screen and cursor gating depends on should_render_screen(), which currently always returns true.
As implemented, the screen and cursor always render, even during CameraOnly. That defeats the gating intent and can cause unnecessary overdraw and cursor visibility during camera-only fades. See my fix suggestion in layout.rs for should_render_screen() and get_layout_values().
🤖 Prompt for AI Agents
In crates/rendering/src/lib.rs around lines 1012 to 1021, the screen and cursor
rendering are gated by uniforms.layout.should_render_screen() but that function
currently always returns true; update this code to rely on the corrected
should_render_screen() implementation from layout.rs (and any related
get_layout_values() changes) so that in CameraOnly mode both the screen and
cursor blocks are skipped; ensure you import/use the updated layout logic and
leave the two render calls gated by the revised predicate so they no longer
render during CameraOnly.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/desktop/src/routes/editor/Timeline/index.tsx (1)
134-145: Don’t clear selection after layout dragsSelection clear on mouseup only checks zoom state. During a layout drag, zoom is idle, so selection may be inadvertently cleared post-drag. Check both drag states before clearing.
- if (zoomSegmentDragState.type === "idle") { + if ( + zoomSegmentDragState.type === "idle" && + layoutSegmentDragState.type === "idle" + ) { setEditorState("timeline", "selection", null); }apps/desktop/src/routes/editor/context.ts (2)
107-117: Bug: Misplaced negation breaks the guard in deleteClipSegmentThe condition
!segment.recordingSegment === undefinedis always false due to operator precedence, so the early-return doesn’t trigger when recordingSegment is undefined. This risks deleting the only clip representing a recording segment.Fix the check:
- !segment.recordingSegment === undefined || + segment.recordingSegment === undefined ||
148-160: Add bounds check for layout deletionSplicing with an invalid index (e.g., -1) will remove the wrong element. Validate the index first.
setProject( "timeline", "layoutSegments", produce((s) => { if (!s) return; - s.splice(segmentIndex, 1); + if ( + Number.isInteger(segmentIndex) && + segmentIndex >= 0 && + segmentIndex < s.length + ) { + s.splice(segmentIndex, 1); + } }), );
🧹 Nitpick comments (2)
apps/desktop/src/routes/editor/Timeline/index.tsx (2)
80-92: Minor: handleUpdatePlayhead should guard missing boundstimelineBounds.left can be undefined early in mount, producing NaN calculations. Early-return when left is undefined.
const { left } = timelineBounds; + if (left == null) return;
102-108: Zoom deletion needs a fallback for legacy single-index selectionsIt looks like
Timeline/ZoomTrack.tsxstill emits the legacy shape{ type: "zoom"; index: number }in its “regular single selection” branch. Since the delete handler inapps/desktop/src/routes/editor/Timeline/index.tsxunconditionally does:if (selection.type === "zoom") { projectActions.deleteZoomSegments(selection.indices); }a single-index selection will call
deleteZoomSegments(undefined)and silently no-op.You have two options:
Fully migrate all callers (e.g. in
ZoomTrack.tsx) to emit only{ type: "zoom"; indices: number[] }.Or add a lightweight fallback in the delete branch:
--- a/apps/desktop/src/routes/editor/Timeline/index.tsx +++ b/apps/desktop/src/routes/editor/Timeline/index.tsx @@ -102,7 +102,13 @@ function onKeyDown(e: KeyboardEvent) { } else if (selection.type === "layout") { projectActions.deleteLayoutSegment(selection.index); } - } else if (selection.type === "zoom") { - projectActions.deleteZoomSegments(selection.indices); + } else if (selection.type === "zoom") { + const indices = Array.isArray(selection.indices) + ? selection.indices + : typeof (selection as any).index === "number" + ? [(selection as any).index] + : []; + projectActions.deleteZoomSegments(indices); } } }This ensures the delete behavior works for both new and legacy selections.
📜 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 (2)
apps/desktop/src/routes/editor/Timeline/index.tsx(6 hunks)apps/desktop/src/routes/editor/context.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/desktop/src/routes/editor/Timeline/index.tsx (3)
apps/desktop/src/routes/editor/Timeline/LayoutTrack.tsx (2)
LayoutSegmentDragState(27-30)LayoutTrack(32-493)apps/desktop/src/routes/editor/context.ts (1)
meta(307-309)crates/editor/src/editor_instance.rs (1)
meta(106-108)
⏰ 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 (6)
apps/desktop/src/routes/editor/Timeline/index.tsx (4)
14-14: LayoutTrack import and typing look goodImporting LayoutTrack alongside its drag-state type is consistent with the new track’s contract. No issues spotted.
113-116: Escape-to-deselect is a nice UX touchClear and consistent with other editors.
29-29: Gating LayoutTrack on meta().hasCamera is correctConditionally rendering the layout track only when camera metadata is present is appropriate; drag-state wiring and time sync props are consistent with ZoomTrack.
Also applies to: 244-251
277-277: Styling change for timeline markings is fineClass tweak is harmless and doesn’t affect behavior. LGTM.
apps/desktop/src/routes/editor/context.ts (2)
125-126: LGTM on splice usageSwitching to in-place splice for clip deletion is correct given surrounding batching and selection reset.
255-260: Selection union updates look consistent with UI changesIncluding layout selection and switching zoom to indices[] aligns with the new Timeline behaviors.
| } | ||
|
|
||
| let zoomSegmentDragState = { type: "idle" } as ZoomSegmentDragState; | ||
| let layoutSegmentDragState = { type: "idle" } as LayoutSegmentDragState; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard should also treat "movePending" as dragging to avoid playhead jitter
Currently the playhead is only paused when drag state is "moving". While the drag is "movePending" (mouse down before movement), playhead updates can still occur, causing subtle jitter and selection oddities. Prefer updating playbackTime only when both zoom and layout drag states are idle.
Apply this diff:
- if (
- zoomSegmentDragState.type !== "moving" &&
- layoutSegmentDragState.type !== "moving"
- ) {
+ if (
+ zoomSegmentDragState.type === "idle" &&
+ layoutSegmentDragState.type === "idle"
+ ) {Also applies to: 81-84
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/Timeline/index.tsx around lines 77 and also
affecting lines 81-84, the drag-state guard only treats "moving" as an active
drag so when state is "movePending" playhead updates still occur causing jitter;
change the checks so both "movePending" and "moving" are considered active
dragging (i.e., treat any state !== "idle" as dragging or explicitly test for
both "movePending" and "moving") and update the conditional that pauses/blocks
playbackTime updates (and the zoom + layout combined idle check) to require both
zoomState === "idle" and layoutSegmentDragState === "idle" before allowing
playbackTime to update.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
1638-1642: Type mismatch in KSelect.Value generic (StereoMode vs CameraShape)The “Shape” selector is typed as CameraShape everywhere else, but the Value component uses StereoMode. This will cause a TS error.
- <KSelect.Value<{ - name: string; - value: StereoMode; - }> class="flex-1 text-sm text-left truncate text-[--gray-500] font-normal"> + <KSelect.Value<{ + name: string; + value: CameraShape; + }> class="flex-1 text-sm text-left truncate text-[--gray-500] font-normal">apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
94-121: findIndex(-1) not handled → incorrect “next segment” logic near track endWhen no “next” segment exists, findIndex returns -1, but the code treats any number as truthy and then accesses index -1 (last element). This can clamp hoveredTime incorrectly and block adding segments near the end.
Apply this diff to guard against -1 and compute prevSegmentIndex safely:
-const nextSegmentIndex = project.timeline?.zoomSegments?.findIndex( - (s) => time < s.start, -); - -if (nextSegmentIndex !== undefined) { - const prevSegmentIndex = nextSegmentIndex - 1; +const nextSegmentIndex = + project.timeline?.zoomSegments?.findIndex((s) => time < s.start); + +if (typeof nextSegmentIndex === "number" && nextSegmentIndex >= 0) { + const prevSegmentIndex = + nextSegmentIndex > 0 ? nextSegmentIndex - 1 : undefined; if (prevSegmentIndex === undefined) return; const nextSegment = project.timeline?.zoomSegments?.[nextSegmentIndex]; if (prevSegmentIndex !== undefined && nextSegment) { const prevSegment = project.timeline?.zoomSegments?.[prevSegmentIndex]; if (prevSegment) { const availableTime = nextSegment?.start - prevSegment?.end; if (availableTime < 1) return; } } if (nextSegment && nextSegment.start - time < 1) { time = nextSegment.start - 1; } }
♻️ Duplicate comments (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
46-52: Missing IconLucideLayout import (breaks build where used in LayoutSegmentConfig)
<IconLucideLayout />is referenced later but not imported here. This will fail at build/runtime.Apply this diff to add the missing import next to the other lucide icon imports:
import { type BackgroundSource, type CameraShape, commands, type LayoutSegment, type StereoMode, type TimelineSegment, type ZoomSegment, } from "~/utils/tauri"; +import IconLucideLayout from "~icons/lucide/layout"; import IconLucideMonitor from "~icons/lucide/monitor";
🧹 Nitpick comments (7)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (3)
1585-1592: Typo in class name: use shrink-0 (current: shink-0)This breaks the intended flex behavior (no shrinking) of the position control dots.
- "cursor-pointer size-6 shink-0 rounded-[0.375rem] bg-gray-5 absolute flex justify-center items-center ui-checked:bg-blue-9 focus-visible:outline peer-focus-visible:outline outline-2 outline-blue-9 outline-offset-2 outline-blue-9 transition-colors duration-100", + "cursor-pointer size-6 shrink-0 rounded-[0.375rem] bg-gray-5 absolute flex justify-center items-center ui-checked:bg-blue-9 focus-visible:outline peer-focus-visible:outline outline-2 outline-blue-9 outline-offset-2 outline-blue-9 transition-colors duration-100",
1959-1966: Bound the retry loop on video load errors to avoid infinite reloadsIf the video file is missing or inaccessible, this will continually retry. Cap retries.
- // Add error handling - video.onerror = (e) => { - console.error("Failed to load video for zoom preview:", e); - // Try to reload after a short delay - setTimeout(() => { - video.load(); - }, 100); - }; + // Add error handling with bounded retry to avoid infinite reload loops + let reloadAttempts = 0; + video.onerror = (e) => { + console.error("Failed to load video for zoom preview:", e); + if (reloadAttempts < 2) { + reloadAttempts += 1; + setTimeout(() => { + video.load(); + }, 150); + } + };
2118-2221: LayoutSegmentConfig UX and state updates look good
- Clear “Done/Delete” affordances consistent with other segment configs.
- Mode tabs map cleanly to TimelineConfiguration: "default", "cameraOnly", "hideCamera".
- The descriptive helper below the tabs is a nice touch.
Small type-safety tweak (optional): instead of casting the mode on update, you can lean on the declared type to avoid string drift.
- v as "default" | "cameraOnly" | "hideCamera", + v as LayoutSegment["mode"],Also: this component depends on IconLucideLayout; ensure the import is added (see earlier comment).
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (4)
43-54: Good fix for stuck hover; also clear hoveredTime when segments are removedThe effect correctly resets the hover gate when segments are deleted. To fully clear transient UI state, also reset hoveredTime so the ghost “+” preview doesn’t linger if it was showing.
Apply this diff:
createEffect(() => { const segments = project.timeline?.zoomSegments; if (!segments || segments.length === 0) { setHoveringSegment(false); + setHoveredTime(undefined); } });
291-306: isSelected supports both single and multi; consider stabilizing selection across reordersThe dual support for index and indices is good. However, storing selection by array positions will drift when segments are sorted/moved, causing highlights to jump. Consider tracking selection by a stable identifier (e.g., segment id) or by object identity and updating indices after sort.
I can sketch a minimal migration plan to add ids to zoomSegments and adapt selection/update flows if helpful.
88-93: Prefer currentTarget for bounding box mathUsing e.target can yield a child element’s rect and miscompute time offsets. currentTarget is the TrackRoot element you attached the handler to.
Apply this diff:
-const bounds = e.target.getBoundingClientRect()!; +const bounds = e.currentTarget.getBoundingClientRect()!;
413-441: IconLucideSearch is auto-imported; no manual import needed. Addaria-hiddenfor accessibility.• The project’s auto-import setup (see
packages/ui-solid/src/auto-imports.d.ts) already providesIconLucideSearch, so you don’t need to add animportstatement.
• To avoid unnecessary screen-reader announcements for decorative icons, addaria-hidden="true"to each usage inapps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx:--- ZoomTrack.tsx @@ (width < 40) block - <IconLucideSearch class="size-3.5 text-gray-1 dark:text-gray-12" /> + <IconLucideSearch aria-hidden="true" class="size-3.5 text-gray-1 dark:text-gray-12" /> @@ (width < 100) block - <IconLucideSearch class="size-3" /> + <IconLucideSearch aria-hidden="true" class="size-3" /> @@ (else) block - <IconLucideSearch class="size-3.5" /> + <IconLucideSearch aria-hidden="true" class="size-3.5" />
📜 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 (3)
apps/desktop/src/routes/editor/ConfigSidebar.tsx(14 hunks)apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(6 hunks)apps/desktop/src/routes/editor/context.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/routes/editor/context.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
apps/desktop/src/utils/tauri.ts (2)
ZoomSegment(780-785)LayoutSegment(724-728)apps/desktop/src/routes/editor/ui.tsx (2)
EditorButton(352-412)Field(25-47)
⏰ 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). (5)
- 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 (actions)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
593-641: Zoom segment selection panel looks solidGood single-selection gating, batch delete handling, and contextual “Done” workflow. This matches the UX of similar panels and keeps the state updates scoped.
643-652: Layout segment selection gating and handoff to LayoutSegmentConfig is cleanThe guard ensures only valid, single layout selections render the config UI. The handoff of both the segment and index keeps the editor actions straightforward.
Also applies to: 655-659
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (2)
9-15: SolidJS import for createEffect looks correctImporting createEffect is appropriate for the new hover-reset effect below.
172-177: Empty-state UX improvement LGTMThe two-line, gray-themed empty state with pointer-events-none is a solid improvement; it avoids intercepting clicks when adding the first segment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
213-250: Ctrl/Cmd multi-select toggle now normalizes legacy selection — fixes the crash pathYou correctly normalize single-index selections into an indices array before toggling. Also good call keeping handleUpdatePlayhead(e) invocation afterward.
🧹 Nitpick comments (4)
apps/desktop/src/styles/theme.css (2)
151-155: Use logical property with fallback to avoid undefined var issues; consider scroll-gutter instead of margins
--margin-top-scrollisn’t defined in this file. Without a fallback, the declaration is ignored when the variable is unset. Also,margin-topmay margin-collapse depending on context. Prefermargin-block-startwith a0fallback.- If the intent is to reserve space for the scrollbar/edges,
scrollbar-gutter: stable;(orstable both-edges) is a more robust, cross-OS approach than relying on margins.Apply within this block:
.custom-scroll { margin-right: 3px; /* Add margin to create space between scrollbar and window edge */ overflow-y: scroll; - margin-top: var(--margin-top-scroll); + margin-block-start: var(--margin-top-scroll, 0); + /* Optional: reserve layout space for scrollbars to avoid layout shift */ + /* scrollbar-gutter: stable; */ }Additionally, define a safe default (outside this hunk):
:root, :root.dark { --margin-top-scroll: 0; }If the goal is to offset content inside the scroll container (not the element itself), consider
padding-block-startinstead of margin.
157-166: Duplicate WebKit scrollbar rules with conflicting widths (7px vs 4px)You define
.custom-scroll::-webkit-scrollbartwice with differentwidthvalues. The latter (4px) wins in the cascade, making the earlier 7px config dead code and potentially confusing.Consider consolidating to a single definition to avoid ambiguity. For example, remove the earlier block:
-.custom-scroll::-webkit-scrollbar { - width: 7px; - background-color: transparent; -}And keep the later, consistent 4px version. If the intent was contextual (e.g., theme/OS), make that explicit with a parent selector or media query.
Also applies to: 174-188
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (2)
142-149: Insertion index can break sort order when multiple segments start aftertimeScanning from the end picks the last segment with start > time, which can insert after earlier segments that also satisfy start > time, leaving the array unsorted (e.g., existing starts [10,20], inserting at time=5 yields [10,5,20]). Prefer a forward findIndex and fallback to append.
- let index = zoomSegments.length; - - for (let i = zoomSegments.length - 1; i >= 0; i--) { - if (zoomSegments[i].start > time) { - index = i; - break; - } - } + const found = zoomSegments.findIndex((s) => s.start > time); + const index = found === -1 ? zoomSegments.length : found; zoomSegments.splice(index, 0, { start: time, end: time + 1, amount: 1.5, mode: { manual: { x: 0.5, y: 0.5, }, }, });Also applies to: 151-163
88-93: Use currentTarget for bounds to avoid child-element targeting quirksIf the event originates from a child, e.target may not be the TrackRoot, skewing the computed position. currentTarget is the element with the handler.
- const bounds = e.target.getBoundingClientRect()!; + const bounds = (e.currentTarget as HTMLElement).getBoundingClientRect();
📜 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 (4)
apps/desktop/src/routes/editor/ConfigSidebar.tsx(16 hunks)apps/desktop/src/routes/editor/ShadowSettings.tsx(1 hunks)apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(6 hunks)apps/desktop/src/styles/theme.css(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/desktop/src/routes/editor/ShadowSettings.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/routes/editor/ConfigSidebar.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 (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🔇 Additional comments (3)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (3)
43-53: Clearing hover-state leak after deletions — good preventive effectThe createEffect that resets hoveringSegment when zoomSegments becomes empty addresses the stuck-hover issue cleanly. Nice.
172-176: Empty-state UX improvements look solidThe two-line, subtle empty state with pointer-events-none reads well and lets clicks pass through to TrackRoot for segment creation.
296-311: Selection predicate supports both index and indicesThe isSelected logic handles both legacy and new selection shapes without branching elsewhere. Looks good.
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 (2)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (1)
143-152: Fix insertion index: descending scan inserts incorrectly for times before the first segmentThe descending loop chooses the last segment that satisfies start > time. For time earlier than all segments, this inserts near the end, leaving the list unsorted until some later sort. Use a forward scan with findIndex and fallback to append.
Apply this diff:
- let index = zoomSegments.length; - - for (let i = zoomSegments.length - 1; i >= 0; i--) { - if (zoomSegments[i].start > time) { - index = i; - break; - } - } + let index = zoomSegments.findIndex((s) => s.start > time); + if (index === -1) index = zoomSegments.length; zoomSegments.splice(index, 0, { start: time, end: time + 1, amount: 1.5, mode: { manual: {apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
1368-1401: BUG:projectHistory.pause()is never resumed on gradient angle dragYou pause history but do not call the
resumeHistory()you captured. This can leave history paused beyond the drag, breaking undo/redo aggregation.createRoot((dispose) => createEventListenerMap(window, { - mouseup: () => dispose(), + mouseup: () => { + resumeHistory(); + dispose(); + }, mousemove: (moveEvent) => { const rawNewAngle = Math.round(start + (downEvent.clientY - moveEvent.clientY)) % max; const newAngle = moveEvent.shiftKey ? rawNewAngle : Math.round(rawNewAngle / 45) * 45; ... }, }), );
♻️ Duplicate comments (4)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (2)
214-251: Multi-select toggle normalization is solid; resolves the legacy single-index crashThis addresses the earlier crash by normalizing to indices[] before toggling and preserves single-select behavior when modifiers aren’t held. Good fix.
419-447: Build break: IconLucideSearch is used but not importedJSX references IconLucideSearch, but there’s no import. This will fail at compile/runtime.
Add an import for your icon library (adjust path if your project uses a wrapper):
import { cx } from "cva"; +import { Search as IconLucideSearch } from "lucide-solid";apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
1807-1810: Icon usage requires import (covered above)
<IconLucideSearch />used here relies on the missing import noted in the import block comment. Once added, this section is fine.After adding the import, ensure the build passes.
46-52: Missing icon imports: IconLucideLayout (duplicate) and IconLucideSearch
<IconLucideLayout />and<IconLucideSearch />are used but not imported in this file. This will cause a compile/runtime error.Apply this diff to add the missing imports alongside the other lucide imports:
import { type BackgroundSource, type CameraShape, commands, type LayoutSegment, type StereoMode, type TimelineSegment, type ZoomSegment, } from "~/utils/tauri"; +import IconLucideLayout from "~icons/lucide/layout"; +import IconLucideSearch from "~icons/lucide/search"; import IconLucideMonitor from "~icons/lucide/monitor"; import IconLucideSparkles from "~icons/lucide/sparkles";
🧹 Nitpick comments (7)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (2)
43-55: Hover state reset only triggers when all segments are deleted; verify partial-delete behaviorThe effect fixes the stale-hover issue when the last segment is removed. However, if the hovered segment is deleted while other segments remain, hoveringSegment can still get stuck true (since onMouseLeave won’t fire). That keeps onMouseMove short-circuited and blocks new segment creation until a full reset occurs.
Consider also clearing hoveringSegment when the hovered element disappears mid-hover (e.g., detect if the mouse is no longer over any segment in onMouseMove before early-returning, or track hovered segment identity). If you want, I can sketch a lightweight approach that avoids DOM queries by tagging SegmentRoot with a data attribute and checking e.target.closest().
297-312: Harden selection check: guard against -1 from findIndexsegmentIndex is always a number; -1 indicates “not found”. Use a >= 0 check before includes to avoid accidental lookups with -1.
Apply this diff:
- if ( - "indices" in selection && - Array.isArray(selection.indices) && - segmentIndex !== undefined - ) { + if ( + "indices" in selection && + Array.isArray(selection.indices) && + segmentIndex !== undefined && + segmentIndex >= 0 + ) { return selection.indices.includes(segmentIndex); } else if ( "index" in selection && typeof selection.index === "number" ) { return segmentIndex === selection.index; }apps/desktop/src/routes/editor/ConfigSidebar.tsx (5)
303-306: Inline CSS var could be centralized (nit)The
--margin-top-scrollstyle is duplicated later; consider hoisting into a class to avoid repetition.- style={{ - "--margin-top-scroll": "5px", - }} + class={cx("mt-scroll-5px")}Outside this hunk, define
.mt-scroll-5pxin your styles to set the CSS var once.
971-1004: Avoid unnecessary type assertion when deriving selected wallpaper (nit)You’re already in the wallpaper branch; the extra cast is noisy. A null-safe access is sufficient.
- const selectedWallpaper = wallpapers()?.find((w) => - (project.background.source as { path?: string }).path?.includes(w.id), - ); + const selectedWallpaper = wallpapers()?.find((w) => + project.background.source.path?.includes(w.id), + );
1898-1907: Video reload and source path setup is soundUsing
convertFileSrcand callingvideo.load()covers stale resource issues. Consider settingpreload = "auto"for more eager readiness if needed.video.src = path; +video.preload = "auto"; // Force reload if video fails to load video.load();
1970-1977: Retry loop onvideo.onerrorneeds a cap/backoffAs written, persistent failures will retry every 100ms indefinitely. Add a max retry count and/or exponential backoff.
- video.onerror = (e) => { - console.error("Failed to load video for zoom preview:", e); - // Try to reload after a short delay - setTimeout(() => { - video.load(); - }, 100); - }; + { + let retryCount = 0; + const maxRetries = 5; + video.onerror = (e) => { + console.error("Failed to load video for zoom preview:", e); + if (retryCount < maxRetries) { + const delay = 100 * 2 ** retryCount++; + setTimeout(() => video.load(), delay); + } + }; + }
2129-2233: Layout segment config: small nits and missing import
- Missing
<IconLucideLayout />import (covered in import block comment).- Prefer nullish coalescing for default mode to avoid unintended fallbacks.
- You duplicate the indicator “left” computation twice; centralize to a helper or computed value to avoid inconsistencies.
- value={props.segment.mode || "default"} + value={props.segment.mode ?? "default"}Optionally, extract the left position:
// outside JSX, inside component const indicatorLeft = () => props.segment.mode === "cameraOnly" ? "50%" : props.segment.mode === "hideCamera" ? "83.33%" : "16.67%";Then in both style blocks:
- style={{ left: props.segment.mode === "cameraOnly" ? "50%" : props.segment.mode === "hideCamera" ? "83.33%" : "16.67%" }} + style={{ left: indicatorLeft() }}
📜 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 (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx(16 hunks)apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
apps/desktop/src/utils/tauri.ts (2)
ZoomSegment(780-785)LayoutSegment(724-728)apps/desktop/src/routes/editor/ui.tsx (2)
EditorButton(352-412)Field(25-47)
⏰ 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)
🔇 Additional comments (16)
apps/desktop/src/routes/editor/Timeline/ZoomTrack.tsx (3)
9-9: LGTM: createEffect import is appropriateThe import aligns with the new effect usage below.
89-94: LGTM: Use currentTarget for boundsSwitching to e.currentTarget.getBoundingClientRect() ensures calculations are relative to the track, not a child element. Correct change.
173-178: LGTM: Empty-state UX improvementsThe empty/fallback state looks good. The pointer-events-none ensures track-level interactions still work (click-to-add) through the overlay.
apps/desktop/src/routes/editor/ConfigSidebar.tsx (13)
33-33: Switch tocreateStorelooks goodState management for
selectedTabviacreateStoreis appropriate and consistent with the rest of the file’s Solid store usage.
46-50: Type import forLayoutSegmentis correctGood to see the explicit type wired from
~/utils/tauri. This aligns the layout segment editor with the shared timeline types.
217-225: Broader context fromuseEditorContextis wired correctlyPulling
setEditorState,projectActions, and others into scope is required for the new selection overlays and segment actions. Looks good.
579-649: Zoom selection overlay: solid gating and actions
- Correctly guards for selection type and filters undefined segments.
- Provides “Done” and bulk “Delete” with appropriate indices mapping.
This block is cohesive and aligns with existing Clip selection UX.
651-660: Layout selection gating is correctThe selection guard and segment lookup for layout segments mirror the zoom flow and should avoid null access issues.
663-666: Layout segment config routing looks rightPassing
segmentandsegmentIndexfrom the gated selection is consistent with other segment configs.
1593-1601: Camera position radio control – focus/selection styling and handlers look goodThe improved focus-visible outlines and absolute positioning logic are consistent and accessible.
1873-1895: Relative time calc is correctComputes per-clip seek offset robustly and guards missing segment. Clean addition.
1910-1924: Defensive seek only after readiness is solidThe
canplayfallback avoids invalidcurrentTimesets. Good guard against race conditions.
1933-1936: Render trigger is gated onloaded()Subscribes to the right dependencies and avoids redundant draws. LGTM.
1941-1960: Canvas render pipeline is correctReadiness checks, clearing, and draw parameters are sensible; disabling smoothing suits pixel-perfect previews.
2016-2047: Manual zoom drag updates are correctEvent scoping via
createRoot, bounds math, and constrained [0,1] normalization are clean. The path-basedsetProjectcall to updatemode.manualis correct within the manual tab.
2050-2068: Loading overlay UX is a nice touchData-attribute driven fade-in keeps it smooth; good conditional rendering while the video decodes.
There was a problem hiding this comment.
Actionable comments posted: 1
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/editor/ConfigSidebar.tsx (1)
2067-2080: Fix TDZ crash: calling croppedPosition/croppedSize before they’re defined
createEffect(on(() => { croppedPosition(); croppedSize(); }, ...))executes immediately; at this pointcroppedPosition/croppedSizearen’t initialized yet (const), causing a runtime ReferenceError.Move the effect below the declarations of
croppedPositionandcroppedSize.Remove the early effect:
- createEffect( - on( - () => { - croppedPosition(); - croppedSize(); - }, - () => { - if (loaded()) { - render(); - } - }, - ), - );Then add this effect after
croppedPositionandcroppedSizeare declared (just below line 2141):// Re-render when crop changes createEffect( on( () => [croppedPosition().x, croppedPosition().y, croppedSize().x, croppedSize().y], () => { if (loaded()) render(); }, ), );
♻️ Duplicate comments (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (1)
47-53: Missing IconLucideLayout import will break build
<IconLucideLayout />is used in LayoutSegmentConfig but not imported.Apply this diff to add the missing import next to the other lucide icons:
import { type BackgroundSource, type CameraShape, commands, type LayoutSegment, type StereoMode, type TimelineSegment, type ZoomSegment, } from "~/utils/tauri"; +import IconLucideLayout from "~icons/lucide/layout"; import IconLucideMonitor from "~icons/lucide/monitor";
🧹 Nitpick comments (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
1019-1021: Avoid substring collisions when resolving selected wallpaperUsing
includes(w.id)can match the wrong wallpaper id (e.g., "ventura" vs "ventura-dark"). Prefer an exact suffix match.Apply this diff:
- ).path?.includes(w.id), + ).path?.endsWith(`${w.id}.jpg`),
1889-1893: Release video resources on unmount to avoid leaksBoth previews attach event handlers and keep a video element alive without cleanup. Add
onCleanupto pause and clear the source when the component unmounts.Add
onCleanupimport:// add to existing solid-js imports import { ..., onCleanup } from "solid-js";Then in both components (ZoomSegmentPreview and ZoomSegmentConfig) after setting up
videoand its handlers:onCleanup(() => { try { video.pause(); } catch {} video.src = ""; // Force release of resources video.load(); });Also applies to: 2112-2118
📜 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/ConfigSidebar.tsx(17 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
apps/desktop/src/utils/tauri.ts (2)
ZoomSegment(780-785)LayoutSegment(724-728)apps/desktop/src/routes/editor/ui.tsx (2)
EditorButton(352-412)Field(25-47)
⏰ 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). (5)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
592-607: Nice: defensive selection handling for zoom segmentsGood use of guards to derive valid zoom segments from the selection and clear it when nothing resolves. This prevents dangling/invalid UI states.
2276-2379: LGTM: LayoutSegmentConfig UX and state updates are consistent
- Good use of controlled KTabs with a clear 3-state layout mode.
- Done/Delete actions align with the rest of the editor UI and use
projectActions.deleteLayoutSegmentcorrectly.- Fallback to "default" when mode is unset matches the type’s optional mode.
There was a problem hiding this comment.
Actionable comments posted: 1
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/editor/ConfigSidebar.tsx (1)
1667-1673: Type mismatch: KSelect.Value expects CameraShape, not StereoModeThis select is for camera “Shape” but the Value’s type param is StereoMode. This will cause TS errors and/or incorrect type inference.
Apply this diff:
<KSelect.Value<{ name: string; - value: StereoMode; + value: CameraShape; }> class="flex-1 text-sm text-left truncate text-[--gray-500] font-normal">
♻️ Duplicate comments (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
2192-2199: Reactive fix for zoom handle position looks goodSwitching from states.manual.* to mode().* makes the handle reactive to both drag updates and external changes. This resolves the earlier non-reactivity issue.
44-53: Missing IconLucideLayout import will break LayoutSegmentConfigLayoutSegmentConfig uses (Line 2303), but there’s no corresponding import. This will fail at build/runtime.
Apply this diff to add the missing import:
import { type BackgroundSource, type CameraShape, commands, type LayoutSegment, type StereoMode, type TimelineSegment, type ZoomSegment, } from "~/utils/tauri"; +import IconLucideLayout from "~icons/lucide/layout"; import IconLucideMonitor from "~icons/lucide/monitor";
🧹 Nitpick comments (3)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (3)
2294-2301: Clear selection after deleting a layout segmentAfter deleting the selected layout segment, the selection remains truthy, leaving an empty overlay. Clear the selection to match the zoom flow’s UX.
Apply this diff:
<EditorButton variant="danger" onClick={() => { projectActions.deleteLayoutSegment(props.segmentIndex); + setEditorState("timeline", "selection", null); }} leftIcon={<IconCapTrash />} >
2000-2000: Remove invalid/empty tabIndex proptabIndex expects a number; an empty string is unnecessary and can cause typing/DOM issues. Remove it or set a number explicitly (-1 or 0).
Apply this diff:
- <KTabs.Content value="manual" tabIndex=""> + <KTabs.Content value="manual">
2283-2379: LayoutSegmentConfig: clear, minimal controls; consider indicating camera availabilityThe “Camera Layout” tabs are straightforward. Small enhancement idea: disable or annotate “Camera Only” when no camera is present in the underlying recording segments, similar to how the Camera tab is disabled when no camera exists.
If desired, I can wire a disabled state based on editorInstance.recordings.segments.every((s) => s.camera === null).
📜 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/ConfigSidebar.tsx(17 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
apps/desktop/src/utils/tauri.ts (2)
ZoomSegment(780-785)LayoutSegment(724-728)apps/desktop/src/routes/editor/ui.tsx (2)
EditorButton(352-412)Field(25-47)
⏰ 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 (2)
apps/desktop/src/routes/editor/ConfigSidebar.tsx (2)
1806-1924: ZoomSegmentPreview: solid, resilient preview pipelineGood robustness improvements: relativeTime seeking, canplay gating, explicit load(), error retry, and render gating with a loading overlay. The canvas cropping and smoothing settings are sensible.
592-671: Zoom multi-select UX and guardrails are sound
- Properly maps indices to existing zoomSegments and filters out undefined.
- Clears selection if none remain, avoiding stale overlays.
- Nice fallback preview grid with for performance; after fixing index()/prop, the labels will be correct.
| <Index each={value().segments}> | ||
| {(item, index) => ( | ||
| <div class="p-2.5 rounded-lg border border-gray-4 bg-gray-3"> | ||
| <ZoomSegmentPreview | ||
| segment={item().segment} | ||
| segmentIndex={index} | ||
| /> |
There was a problem hiding this comment.
Fix Index accessor usage: pass index() rather than the accessor itself
Solid’s provides an accessor function for index. Passing the function causes label formatting bugs (e.g., “Zoom [object Function]1”). Use index() to pass a number.
Apply this diff:
- <ZoomSegmentPreview
- segment={item().segment}
- segmentIndex={index}
- />
+ <ZoomSegmentPreview
+ segment={item().segment}
+ segmentIndex={index()}
+ />📝 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.
| <Index each={value().segments}> | |
| {(item, index) => ( | |
| <div class="p-2.5 rounded-lg border border-gray-4 bg-gray-3"> | |
| <ZoomSegmentPreview | |
| segment={item().segment} | |
| segmentIndex={index} | |
| /> | |
| <Index each={value().segments}> | |
| {(item, index) => ( | |
| <div class="p-2.5 rounded-lg border border-gray-4 bg-gray-3"> | |
| <ZoomSegmentPreview | |
| segment={item().segment} | |
| segmentIndex={index()} | |
| /> |
🤖 Prompt for AI Agents
In apps/desktop/src/routes/editor/ConfigSidebar.tsx around lines 645 to 651, the
Index child is passing the index accessor function (index) into
ZoomSegmentPreview which leads to label formatting bugs; call the accessor
(index()) and pass the resulting number instead. Update the prop segmentIndex to
use index() rather than index so ZoomSegmentPreview receives a numeric index.
Ensure any other usages in this mapped block also call index() if they expect a
number.
2 participants
This PR implements
CLAUDE.mdfilehttps://Cap.link/cgt4sy2fptqtp2z
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation