Remove todo!() in pause/resume and fix timing on macos #1153
Conversation
WalkthroughConsolidates recording control APIs to anyhow::Result, refactors MP4 timestamp handling with pause-aware offsets, adds error context in stop paths, adjusts web UI components (Navbar label weight, Caps empty-state/refresh, Folder link behavior), reworks ShareHeader props/layout removing org icon fields, updates video page types, removes org name max length, and simplifies CLI recording flow. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI (Desktop/Web)
participant Rec as InProgressRecording
participant Enc as MP4Encoder
participant Src as Sources
UI->>Rec: pause()
activate Rec
Rec->>Enc: set pause_timestamp
deactivate Rec
UI->>Rec: resume()
activate Rec
Rec->>Enc: clear pause state (offset maintained)
deactivate Rec
UI->>Rec: cancel()
activate Rec
Rec->>Src: stop() [with context]
Src-->>Rec: result
Rec-->>UI: anyhow::Result<()>
sequenceDiagram
autonumber
participant CLI as recording-cli
participant Handle as RecordingHandle
participant Enc as MP4Encoder
CLI->>Handle: start()
CLI-->>CLI: sleep 3s
CLI->>Handle: pause()
Handle->>Enc: set pause_timestamp
CLI-->>CLI: sleep 2s
CLI->>Handle: resume()
Handle->>Enc: continue with timestamp_offset
CLI-->>CLI: sleep 3s
CLI->>Handle: stop()
Handle->>Enc: finish at most_recent - offset
sequenceDiagram
autonumber
participant User as User
participant Header as ShareHeader
participant API as editTitle API
participant Router as Router
User->>Header: blur(title input)
Header->>API: editTitle(id, title)
API-->>Header: success/error
alt success
Header->>Router: refresh
else error
Header-->>User: toast error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/app/(org)/dashboard/caps/Caps.tsx (1)
245-248: Missing router.refresh() in bulk delete success handler.The bulk delete success handler no longer calls
router.refresh(), but the single delete handler at line 255 still does. This inconsistency means the UI won't update to reflect the deleted caps after bulk deletion, requiring a manual page refresh.Apply this diff to restore the refresh:
onSuccess: () => { setSelectedCaps([]); + router.refresh(); },apps/web/app/s/[videoId]/_components/ShareHeader.tsx (1)
71-85: Add validation and optimize title editing.The simplified title editing logic has several issues:
- Unnecessary API calls: No comparison with the original title means
editTitleis called even when the title hasn't changed.- Whitespace handling: No trimming allows saving titles with leading/trailing whitespace or empty strings (the
editTitleaction validates presence but doesn't trim).- Error recovery: On error, the local
titlestate remains changed while the server-side title is unchanged, creating UI/data inconsistency.Apply this diff to restore proper validation and error handling:
const handleBlur = async () => { setIsEditing(false); + const trimmedTitle = title.trim(); + + if (trimmedTitle === data.name) { + return; + } + + if (!trimmedTitle) { + setTitle(data.name); + return; + } try { - await editTitle(data.id, title); + await editTitle(data.id, trimmedTitle); toast.success("Video title updated"); refresh(); } catch (error) { + setTitle(data.name); if (error instanceof Error) { toast.error(error.message); } else { toast.error("Failed to update title - please try again."); } } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/desktop/src-tauri/src/recording.rs(3 hunks)apps/web/app/(org)/dashboard/_components/Navbar/Items.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/Caps.tsx(1 hunks)apps/web/app/(org)/dashboard/caps/components/Folder.tsx(1 hunks)apps/web/app/s/[videoId]/_components/ShareHeader.tsx(6 hunks)apps/web/app/s/[videoId]/page.tsx(0 hunks)apps/web/components/forms/NewOrganization.tsx(1 hunks)crates/enc-avfoundation/src/mp4.rs(7 hunks)crates/recording/examples/recording-cli.rs(2 hunks)crates/recording/src/output_pipeline/core.rs(1 hunks)crates/recording/src/sources/screen_capture/macos.rs(2 hunks)
💤 Files with no reviewable changes (1)
- apps/web/app/s/[videoId]/page.tsx
🧰 Additional context used
📓 Path-based instructions (9)
apps/web/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
apps/web/**/*.{ts,tsx}: Use TanStack Query v5 for all client-side server state and data fetching in the web app
Web mutations should call Server Actions directly and perform targeted cache updates with setQueryData/setQueriesData rather than broad invalidations
Client code should use useEffectQuery/useEffectMutation and useRpcClient from apps/web/lib/EffectRuntime.ts; do not create ManagedRuntime inside components
Files:
apps/web/components/forms/NewOrganization.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/components/Folder.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/_components/ShareHeader.tsx
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/web/components/forms/NewOrganization.tsxcrates/recording/src/output_pipeline/core.rsapps/web/app/(org)/dashboard/caps/Caps.tsxcrates/recording/src/sources/screen_capture/macos.rsapps/desktop/src-tauri/src/recording.rscrates/recording/examples/recording-cli.rsapps/web/app/(org)/dashboard/caps/components/Folder.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxcrates/enc-avfoundation/src/mp4.rsapps/web/app/s/[videoId]/_components/ShareHeader.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use strict TypeScript and avoid any; leverage shared types from packages
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/web/components/forms/NewOrganization.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/components/Folder.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/_components/ShareHeader.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/web/components/forms/NewOrganization.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/components/Folder.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/_components/ShareHeader.tsx
apps/web/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
On the client, always use
useEffectQueryoruseEffectMutationfrom@/lib/EffectRuntime; never callEffectRuntime.run*directly in components.
Files:
apps/web/components/forms/NewOrganization.tsxapps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/components/Folder.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/_components/ShareHeader.tsx
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/recording/src/output_pipeline/core.rscrates/recording/src/sources/screen_capture/macos.rscrates/enc-avfoundation/src/mp4.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/recording/src/output_pipeline/core.rscrates/recording/src/sources/screen_capture/macos.rsapps/desktop/src-tauri/src/recording.rscrates/recording/examples/recording-cli.rscrates/enc-avfoundation/src/mp4.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/output_pipeline/core.rscrates/recording/src/sources/screen_capture/macos.rscrates/enc-avfoundation/src/mp4.rs
apps/web/app/**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer Server Components for initial data in the Next.js App Router and pass initialData to client components
Files:
apps/web/app/(org)/dashboard/caps/Caps.tsxapps/web/app/(org)/dashboard/caps/components/Folder.tsxapps/web/app/(org)/dashboard/_components/Navbar/Items.tsxapps/web/app/s/[videoId]/_components/ShareHeader.tsx
🧬 Code graph analysis (5)
apps/web/app/(org)/dashboard/caps/Caps.tsx (1)
apps/web/app/(org)/dashboard/caps/components/EmptyCapState.tsx (1)
EmptyCapState(12-48)
apps/desktop/src-tauri/src/recording.rs (2)
crates/recording/src/instant_recording.rs (3)
pause(52-54)resume(56-58)cancel(60-62)crates/recording/src/studio_recording.rs (3)
pause(322-324)resume(326-328)cancel(330-332)
crates/recording/examples/recording-cli.rs (2)
crates/recording/src/instant_recording.rs (2)
handle(106-117)handle(125-141)crates/recording/src/studio_recording.rs (4)
handle(114-146)handle(154-175)handle(183-207)handle(215-223)
crates/enc-avfoundation/src/mp4.rs (4)
crates/recording/src/sources/screen_capture/windows.rs (1)
timestamp(81-83)crates/recording/src/output_pipeline/core.rs (5)
new(595-597)new(638-640)new(671-673)new(707-724)timestamp(750-750)crates/recording/src/sources/screen_capture/mod.rs (1)
info(416-418)crates/recording/src/sources/screen_capture/macos.rs (2)
new(247-256)timestamp(57-59)
apps/web/app/s/[videoId]/_components/ShareHeader.tsx (2)
packages/database/schema.ts (1)
videos(256-311)apps/web/actions/videos/edit-title.ts (1)
editTitle(10-51)
⏰ 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 (4)
apps/web/app/(org)/dashboard/_components/Navbar/Items.tsx (1)
495-497: LGTM!The addition of
font-mediumimproves the visual hierarchy of the navigation item counter.apps/web/app/(org)/dashboard/caps/components/Folder.tsx (1)
276-279: LGTM!Removing the conditional navigation block during delete operations improves UX. The delete confirmation is handled separately via the ConfirmationDialog, so there's no need to prevent navigation.
apps/web/app/s/[videoId]/_components/ShareHeader.tsx (1)
32-32: LGTM!The type signature simplification and layout restructure align with the broader refactoring to remove organization-specific fields from the video sharing interface. The changes improve code maintainability by relying on the base schema type.
Also applies to: 139-139, 201-230
apps/web/components/forms/NewOrganization.tsx (1)
28-28: No database constraint conflicts The organizations.name column is defined as varchar(255), so removing the 25-character limit from form validation is safe.
| let _ = recording.cancel().await; | ||
|
|
There was a problem hiding this comment.
Propagate cancel failure during restart
We now discard recording.cancel().await errors, so a failed cancel silently falls through to a new start_recording while the previous pipeline may still be alive. That can leak temp dirs and confuse state. Please propagate the error (e.g. return Err(err.to_string())) before continuing.
- let _ = recording.cancel().await;
+ if let Err(err) = recording.cancel().await {
+ return Err(err.to_string());
+ }📝 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.
| let _ = recording.cancel().await; | |
| if let Err(err) = recording.cancel().await { | |
| return Err(err.to_string()); | |
| } |
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/recording.rs around lines 673-674, the call `let _
= recording.cancel().await;` currently discards any error; change it to
propagate failures instead of ignoring them by checking the result and returning
an error (for example: `recording.cancel().await.map_err(|e| e.to_string())?` or
match on the Result and return `Err(e.to_string())`) so that if cancel fails the
function returns early with an Err and does not proceed to start a new
recording.
| ); | ||
|
|
||
| if (count === 0 && folders.length === 0) return <EmptyCapState />; | ||
| if (count === 0) return <EmptyCapState />; |
There was a problem hiding this comment.
Reconsider empty state condition.
The empty state now displays when count === 0 regardless of folders, showing "Record your first Cap" even when folders exist. This could confuse users who have organized their workspace with folders but haven't added videos yet. The previous condition count === 0 && folders.length === 0 was more accurate for a truly empty state.
Consider restoring the original condition:
-if (count === 0) return <EmptyCapState />;
+if (count === 0 && folders.length === 0) return <EmptyCapState />;📝 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.
| if (count === 0) return <EmptyCapState />; | |
| if (count === 0 && folders.length === 0) return <EmptyCapState />; |
🤖 Prompt for AI Agents
In apps/web/app/(org)/dashboard/caps/Caps.tsx around line 269, the empty-state
check uses `count === 0` which shows the "Record your first Cap" message even
when folders exist; restore the previous logic by changing the condition to
require both no caps and no folders (i.e., `count === 0 && folders.length ===
0`) so the empty state only appears when the workspace is truly empty.
| // before pausing at all, subtract 0. | ||
| // on pause, record last frame time. | ||
| // on resume, store last frame time and clear offset timestamp | ||
| // on next frame, set offset timestamp and subtract (offset timestamp - last frame time - previous offset) | ||
| // on next pause, store (offset timestamp - last frame time) into previous offset | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove inline comments per repo guidelines
Rust sources under this repo aren’t supposed to carry inline/block comments. Please drop these TODO notes and keep the code self-explanatory instead.
As per coding guidelines
-// before pausing at all, subtract 0.
-// on pause, record last frame time.
-// on resume, store last frame time and clear offset timestamp
-// on next frame, set offset timestamp and subtract (offset timestamp - last frame time - previous offset)
-// on next pause, store (offset timestamp - last frame time) into previous offset📝 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.
| // before pausing at all, subtract 0. | |
| // on pause, record last frame time. | |
| // on resume, store last frame time and clear offset timestamp | |
| // on next frame, set offset timestamp and subtract (offset timestamp - last frame time - previous offset) | |
| // on next pause, store (offset timestamp - last frame time) into previous offset | |
| // (lines 7–12 removed per repo guidelines) |
🤖 Prompt for AI Agents
In crates/enc-avfoundation/src/mp4.rs around lines 7 to 12, the file contains
inline comment TODO notes describing pause/resume timestamp logic; per repo
guidelines remove these inline/block comments. Delete the commented lines and,
if necessary, replace them with a short, self-explanatory function/variable name
or small helper function so the code expresses the behavior without inline
comments, ensuring no other code changes are introduced.
| self.timestamp_offset += timestamp - pause_timestamp; | ||
| self.pause_timestamp = None; | ||
| } |
There was a problem hiding this comment.
Fix pause offset for audio frames
timestamp_offset only gets updated/cleared when a video frame arrives. If audio resumes first (common on macOS), we subtract the stale offset here and enqueue audio with the full pause gap baked in. Result: audio jumps ahead and goes out of sync. Please update the offset/most_recent_timestamp on whichever stream resumes first (e.g. factor out a normalize_timestamp helper that handles pause_timestamp.take() and use it in both video & audio paths before computing pts).
Also applies to: 284-287
Summary by CodeRabbit