Windows desktop reliability (logs) + onboarding bits#1797
Windows desktop reliability (logs) + onboarding bits#1797richiemcilroy wants to merge 3 commits into
Conversation
brin-security-scanner
Bot
added
contributor:verified
|
Paragon Review Skipped Hi @richiemcilroy! Your Polarity credit balance is insufficient to complete this review. Please visit https://app.paragon.run to finish your review. |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
| let message = format!("Export task panicked: {panic_msg}"); | ||
| sentry::capture_message(&message, sentry::Level::Error); | ||
| Err(message) | ||
| } |
There was a problem hiding this comment.
This currently returns the panic payload as a user-facing Err(String), which can leak internal details (paths, codec strings, etc.) into the UI.
Consider returning a generic error and only sending the panic payload to logs/Sentry.
| let message = format!("Export task panicked: {panic_msg}"); | |
| sentry::capture_message(&message, sentry::Level::Error); | |
| Err(message) | |
| } | |
| let sentry_message = format!("Export task panicked: {panic_msg}"); | |
| sentry::capture_message(&sentry_message, sentry::Level::Error); | |
| Err("Export task panicked".to_string()) |
| const isMacOS = createMemo(() => ostype() === "macos"); | ||
| const minStep = createMemo(() => (isMacOS() ? 0 : 1)); | ||
|
|
||
| const [step, setStep] = createSignal(ostype() === "macos" ? 0 : 1); |
There was a problem hiding this comment.
Minor: since minStep is already computed, you can avoid duplicating the ostype() check in the initial signal value.
| const [step, setStep] = createSignal(ostype() === "macos" ? 0 : 1); | |
| const [step, setStep] = createSignal(minStep()); |
| async setCameraInput(id: DeviceOrModelID | null, skipCameraWindow: boolean | null) : Promise<null> { | ||
| return await TAURI_INVOKE("set_camera_input", { id, skipCameraWindow }); | ||
| }, | ||
| async setNativeCameraPreviewEnabled(enabled: boolean) : Promise<null> { |
There was a problem hiding this comment.
apps/desktop/src/utils/tauri.ts is marked as generated (tauri-specta) and warns against manual edits. These additions will likely be overwritten the next time bindings are regenerated.
If these are new commands/types, it’d be safer to update the Rust/specta source and re-generate, or put hand-written wrappers in a separate file and re-export from there.
| } | ||
| }, | ||
| "classes": {} | ||
| } No newline at end of file |
There was a problem hiding this comment.
This file is missing a trailing newline (\ No newline at end of file in the diff), which tends to cause unnecessary churn.
| } | |
| } |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| &thread_name, | ||
| &location, | ||
| &message, | ||
| &backtrace, |
There was a problem hiding this comment.
Replacing tracing_appender::non_blocking with a direct RollingFileAppender makes every tracing call block the calling thread until the write is flushed to disk. Inside Tokio worker threads (recording pipeline, export, preview rendering) this can stall the runtime and introduce latency spikes. If the goal is to ensure no logs are dropped on Windows, consider keeping non_blocking but explicitly flushing the guard on shutdown, or using tracing_appender::non_blocking with lossy = false.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/main.rs
Line: 182
Comment:
**Synchronous file writer in async context**
Replacing `tracing_appender::non_blocking` with a direct `RollingFileAppender` makes every `tracing` call block the calling thread until the write is flushed to disk. Inside Tokio worker threads (recording pipeline, export, preview rendering) this can stall the runtime and introduce latency spikes. If the goal is to ensure no logs are dropped on Windows, consider keeping `non_blocking` but explicitly flushing the guard on shutdown, or using `tracing_appender::non_blocking` with `lossy = false`.
How can I resolve this? If you propose a fix, please make it concise.| .catch_unwind() | ||
| .await | ||
| { | ||
| Ok(result) => result, | ||
| Err(panic) => { | ||
| let panic_msg = panic_message(panic); | ||
| error!( | ||
| target: "cap_desktop_export", | ||
| panic = %panic_msg, | ||
| "export task panicked" | ||
| ); | ||
| let message = format!("Export task panicked: {panic_msg}"); | ||
| sentry::capture_message(&message, sentry::Level::Error); | ||
| Err(message) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
AssertUnwindSafe on retry path
AssertUnwindSafe is correct here — caught panics return Err and the export is abandoned rather than retried into potentially corrupted state. One thing worth double-checking: the retry in export_video (with force_ffmpeg: true) runs immediately after a panic in the GPU path. If the panic leaked an exclusive lock on a WGPU device or left a GPU command encoder in an open state, the retry (even on the software/ffmpeg path) may encounter those residual locks. If you see intermittent hangs on the retry leg during Windows testing, that would be the likely cause.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/export.rs
Line: 41-57
Comment:
**`AssertUnwindSafe` on retry path**
`AssertUnwindSafe` is correct here — caught panics return `Err` and the export is abandoned rather than retried into potentially corrupted state. One thing worth double-checking: the retry in `export_video` (with `force_ffmpeg: true`) runs immediately after a panic in the GPU path. If the panic leaked an exclusive lock on a WGPU device or left a GPU command encoder in an open state, the retry (even on the software/ffmpeg path) may encounter those residual locks. If you see intermittent hangs on the retry leg during Windows testing, that would be the likely cause.
How can I resolve this? If you propose a fix, please make it concise.| let location = info | ||
| .location() | ||
| .map(|l| format!("{}:{}:{}", l.file(), l.line(), l.column())) | ||
| .unwrap_or_else(|| "<unknown>".to_string()); | ||
| let message = info | ||
| .payload() |
There was a problem hiding this comment.
RUST_BACKTRACE has no effect on Backtrace::force_capture()
std::backtrace::Backtrace::force_capture() (used in the panic hook) always captures a backtrace regardless of the RUST_BACKTRACE environment variable — that is the point of force_capture vs capture. The env-var block therefore only affects the default Rust panic handler (prev(info) call at the end of the hook). This works as intended for the default handler, but the block is misleading and could be removed without changing what ends up in panics.log.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/main.rs
Line: 159-164
Comment:
**`RUST_BACKTRACE` has no effect on `Backtrace::force_capture()`**
`std::backtrace::Backtrace::force_capture()` (used in the panic hook) always captures a backtrace regardless of the `RUST_BACKTRACE` environment variable — that is the point of `force_capture` vs `capture`. The env-var block therefore only affects the *default* Rust panic handler (`prev(info)` call at the end of the hook). This works as intended for the default handler, but the block is misleading and could be removed without changing what ends up in `panics.log`.
How can I resolve this? If you propose a fix, please make it concise.
|
please rereview the pr @greptileai |
1 participant
Greptile Summary
This PR improves Windows desktop reliability by adding a dedicated WARN-level rolling log file, a panic hook that writes structured records to
panics.log, and panic-isolation wrappers (AssertUnwindSafe+catch_unwind) around export/preview paths with an automatic FFmpeg decoder retry. It also fixes several Windows onboarding issues and a widespreaddata-tauri-drag-region="none"→"false"correctness bug.minStepso the macOS-only permissions step (step 0) is transparently skipped on Windows; all navigation guards and progress indicators are adjusted accordingly.data-tauri-drag-region="none"to"false"across the onboarding, mode-select, and window-chrome layouts, and extractsModeSelectinto a shared component.Confidence Score: 5/5
Safe to merge; all Rust additions are additive logging/panic-isolation plumbing and the frontend fixes correct existing bugs.
The panic hook, split log files, and catch_unwind wrappers are straightforward and well-scoped. The Windows onboarding minStep logic correctly threads through all navigation guards and progress displays. No data paths, auth flows, or shared state were altered in ways that could regress existing behaviour.
No files require special attention beyond the one narrating comment in main.rs.
Important Files Changed
panics.log. Info-level logging keepsnon_blocking; only the errors appender is used synchronously (already flagged in a previous thread). One new narrating comment.AssertUnwindSafe+catch_unwindfor panic isolation; adds FFmpeg decoder retry on frame decode errors; introducesExportActiveGuard/ExportPreviewActiveGuardfor preview/export mutual exclusion; adds unit tests.minStep(0 on macOS, 1 on Windows) so the permissions step is skipped on Windows; clamps step navigation accordingly. Also correctsdata-tauri-drag-region="none"to"false"in multiple places.setNativeCameraPreviewEnabled,startImageImport) and updated types (SystemDiagnostics,Organization,WindowsVersionInfo,GpuInfoDiag, etc.).ModeSelectcomponent extracted for reuse across mode-select window and onboarding.data-tauri-drag-region="none"to"false"on the Inner wrapper div.data-tauri-drag-regionfix; now uses extractedModeSelectcomponent.Prompt To Fix All With AI
Reviews (2): Last reviewed commit: "Relax comment policy; improve error logg..." | Re-trigger Greptile
Context used: