Potentially fix "Recording in progress"

[oscartbeaumont]

Summary by CodeRabbit

• Bug Fixes
  • Improved error handling when starting recording with user-facing error messages and proper resource cleanup on failure.

[coderabbitai]

Walkthrough

The start_recording function in the recording module now implements explicit error handling instead of immediate error propagation. When the actor spawning fails, the code now emits a UI failure event, displays a dialog, and performs cleanup operations before returning, ensuring proper user feedback and resource management.

Changes

Cohort / File(s)	Summary
Error handling in recording startup apps/desktop/src-tauri/src/recording.rs	Modified start_recording to replace immediate error propagation with explicit error handling; added .flatten() on spawn result, emits RecordingEvent::Failed, shows error dialog, and calls handle_recording_end cleanup on spawn failures

Sequence Diagram

sequenceDiagram
    participant Caller
    participant start_recording
    participant actor_spawn
    participant UI
    participant cleanup

    Caller->>start_recording: initiate recording start
    start_recording->>actor_spawn: spawn actor
    
    alt Spawn Success
        actor_spawn-->>start_recording: Ok(rx)
        start_recording->>start_recording: proceed with Rx future handling
    else Spawn Failure
        actor_spawn-->>start_recording: Err(error)
        rect rgb(255, 200, 200)
            Note over start_recording: Error handling path
            start_recording->>UI: emit RecordingEvent::Failed
            start_recording->>UI: show error dialog
            start_recording->>cleanup: call handle_recording_end
        end
        start_recording-->>Caller: return with error
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

• Error handling correctness: Verify that the new error branch properly handles all spawn failure cases and that the flatten operation correctly converts nested Result types
• Cleanup completeness: Ensure handle_recording_end performs all necessary cleanup steps equivalent to or better than previous error handling
• User feedback timing: Confirm that dialog display and event emission occur in the correct sequence without race conditions
• State consistency: Validate that the UI and internal state remain consistent after error recovery

Poem

🐰 When actors stumble and spawns may fail,
We catch the error with grace and hail!
No silent crashes to lament,
But dialogs showing what went wrong—
Then cleanup dances us along! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title Check	❓ Inconclusive	The title "Potentially fix 'Recording in progress'" is related to the changeset since it addresses the recording functionality being modified in recording.rs. However, the title uses uncertain language with "Potentially" and does not clearly describe the actual code change, which is an improvement to error handling in the start_recording function that replaces immediate error propagation with explicit user-visible error handling and cleanup. The title appears to reference an issue or problem statement by name rather than summarizing the technical implementation details of the change itself.	Consider clarifying the title to better reflect the technical change. For example, "Improve error handling in recording with explicit cleanup" would more clearly convey that this PR improves how recording errors are handled. Alternatively, if "Recording in progress" is a specific known issue, the title could be more definitive (removing "Potentially") and optionally mention the nature of the fix, such as "Fix 'Recording in progress' by improving error handling and cleanup."

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch recording-in-progress

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (4)

apps/desktop/src-tauri/src/recording.rs (4)

599-622: Early failure handling is user-friendly; avoid blocking in async and verify pending-state reset

• Nice: emit Failed, show dialog, cleanup via handle_recording_end; this likely fixes the lingering “Recording already in progress.”
• Avoid blocking the async thread: wrap blocking_show in spawn_blocking or use a non-blocking show if available.

Suggested change (keeps behavior, avoids blocking the runtime thread):

-            dialog.blocking_show();
+            tauri::async_runtime::spawn_blocking(move || dialog.blocking_show());

Also, please verify:

• handle_recording_end resets any “pending” state (not just current recording), so RecordingState returns to None on this path.
• The “InProgressRecording” window is closed by handle_recording_end (it closes RecordingControls; ensure that maps to the same window ID in this flow).

---

646-658: Deduplicate error-dialog code into a helper

The dialog creation/emission logic is duplicated here and in the spawn failure branch. Consider extracting a small helper like show_error_dialog(app: &AppHandle, msg: &str) that handles optional parent window and display mode consistently.

---

277-281: Avoid unwrap on app_data_dir() to prevent hard crash

app_data_dir() can return None. Convert to a user-visible error instead of panic:

-        .app_data_dir()
-        .unwrap()
+        .app_data_dir()
+        .ok_or_else(|| "Failed to resolve app data directory".to_string())?

---

703-706: Minor: remove redundant ? after Err

Use a straightforward early return:

-        return Err("Recording not in progress".to_string())?;
+        return Err("Recording not in progress".to_string());

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f701ef and 591d92a.

📒 Files selected for processing (1)

• apps/desktop/src-tauri/src/recording.rs (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

**/*.rs

📄 CodeRabbit inference engine (AGENTS.md)

**/*.rs: Format Rust code using rustfmt and 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:

• apps/desktop/src-tauri/src/recording.rs

⏰ 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). (2)

• GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
• GitHub Check: Analyze (rust)

🔇 Additional comments (1)

apps/desktop/src-tauri/src/recording.rs (1)

594-598: Good normalization of join errors for flatten()

Mapping JoinError to String before awaiting enables Result::flatten with a consistent error type. Looks correct.