Add otel to desktop app #1180
Add otel to desktop app #1180
Conversation
WalkthroughAdds OpenTelemetry dependencies and wires an optional OTLP layer (debug builds); applies widespread #[instrument] tracing annotations and Debug derives across the Tauri desktop app; changes upload API to include file/meta/progress; adds an npm script Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Tauri App (main.rs)
participant Tracing as Tracing Subscriber
participant OTLP as OTLP Exporter (debug only)
Note over App,Tracing: Startup initialization
App->>Tracing: build registry (filter cap_*)
App->>Tracing: attach reload_layer
alt debug build
App->>OTLP: init OTLP tracer/provider
App->>Tracing: attach otel_layer
else release build
Note over Tracing: OTLP layer omitted
end
App->>Tracing: attach fmt layer + file appender
Note over App: Continue normal app startup
sequenceDiagram
autonumber
participant UI as UI
participant Cmd as Tauri Cmd: upload_video(...)
participant Up as upload.rs
participant API as Backend API
participant S3 as S3 (multipart)
Note over Cmd: #[instrument] with progress channel
UI->>Cmd: invoke upload_video(video_id, file_path, screenshot_path, meta, channel)
Cmd->>Up: prepare metadata/screenshots
Up->>API: create_or_get_video
Up->>S3: multipart upload (init, presign parts, upload parts)
S3-->>Up: etags per part
Up->>API: complete multipart with parts/etags
Up-->>Cmd: UploadedItem
Cmd-->>UI: result + progress updates via channel
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
I like the idea of this! Having as much info as possible for the user to look at, if they want to, is really helpful |
oscartbeaumont
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: 0
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-tauri/src/captions.rs (2)
518-533: UB/double-free risk: ptr::read on WhisperContext creates a second ownerYou construct a WhisperContext, store it in the mutex, then clone it out using std::ptr::read. This moves the value out of a shared reference, leaving the original to be dropped later, causing double-free/UB.
Fix by storing Arc in the cache and cloning the Arc safely.
Apply these diffs:
Within this function:
async fn get_whisper_context(model_path: &str) -> Result<Arc<WhisperContext>, String> { - let mut context_guard = WHISPER_CONTEXT.lock().await; - - // Always create a new context to avoid issues with multiple uses - log::info!("Initializing Whisper context with model: {model_path}"); - let ctx = WhisperContext::new_with_params(model_path, WhisperContextParameters::default()) - .map_err(|e| format!("Failed to load Whisper model: {e}"))?; - - *context_guard = Some(ctx); - - // Get a reference to the context and wrap it in an Arc - let context_ref = context_guard.as_ref().unwrap(); - let context_arc = unsafe { Arc::new(std::ptr::read(context_ref)) }; - Ok(context_arc) + let mut context_guard = WHISPER_CONTEXT.lock().await; + log::info!("Initializing Whisper context with model: {model_path}"); + let ctx = Arc::new( + WhisperContext::new_with_params(model_path, WhisperContextParameters::default()) + .map_err(|e| format!("Failed to load Whisper model: {e}"))?, + ); + *context_guard = Some(ctx.clone()); + Ok(ctx) }Additionally, update the cache type:
- static ref WHISPER_CONTEXT: Arc<Mutex<Option<WhisperContext>>> = Arc::new(Mutex::new(None)); + static ref WHISPER_CONTEXT: Arc<Mutex<Option<Arc<WhisperContext>>>> = + Arc::new(Mutex::new(None));
1096-1131: Downloading entire model into memory; stream to disk to avoid OOMresponse.bytes().await buffers the full model (hundreds of MB/GB) before writing. This can exhaust memory. Stream the body via bytes_stream and write incrementally while emitting progress.
Apply this diff to stream safely:
- // Download and write in chunks - let mut downloaded = 0; - let mut bytes = response - .bytes() - .await - .map_err(|e| format!("Failed to get response bytes: {e}"))?; - - // Write the bytes in chunks to show progress - const CHUNK_SIZE: usize = 1024 * 1024; // 1MB chunks - while !bytes.is_empty() { - let chunk_size = std::cmp::min(CHUNK_SIZE, bytes.len()); - let chunk = bytes.split_to(chunk_size); - - file.write_all(&chunk) - .await - .map_err(|e| format!("Error while writing to file: {e}"))?; - - downloaded += chunk_size as u64; + // Stream and write in chunks + let mut downloaded: u64 = 0; + let mut stream = response.bytes_stream(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|e| format!("Failed to read response chunk: {e}"))?; + file.write_all(&chunk) + .await + .map_err(|e| format!("Error while writing to file: {e}"))?; + downloaded += chunk.len() as u64; // Calculate and emit progress let progress = if total_size > 0 { (downloaded as f64 / total_size as f64) * 100.0 } else { 0.0 }; window .emit( DownloadProgress::EVENT_NAME, DownloadProgress { message: format!("Downloading model: {progress:.1}%"), progress, }, ) .map_err(|e| format!("Failed to emit progress: {e}"))?; }And add the needed import:
use futures_util::StreamExt;apps/desktop/src-tauri/src/lib.rs (1)
1938-1945: Private type alias leaked into public API (breaks compilation)FilteredRegistry is now private, but it’s used by the public DynLoggingLayer alias and in the public run signature via LoggingHandle. This is a “private type in public interface” error.
Make the aliases public:
-type FilteredRegistry = tracing_subscriber::layer::Layered< +pub type FilteredRegistry = tracing_subscriber::layer::Layered< tracing_subscriber::filter::FilterFn<fn(m: &tracing::Metadata) -> bool>, tracing_subscriber::Registry, >; -pub type DynLoggingLayer = Box<dyn tracing_subscriber::Layer<FilteredRegistry> + Send + Sync>; -type LoggingHandle = tracing_subscriber::reload::Handle<Option<DynLoggingLayer>, FilteredRegistry>; +pub type DynLoggingLayer = Box<dyn tracing_subscriber::Layer<FilteredRegistry> + Send + Sync>; +pub type LoggingHandle = + tracing_subscriber::reload::Handle<Option<DynLoggingLayer>, FilteredRegistry>;
🧹 Nitpick comments (8)
apps/desktop/src-tauri/src/hotkeys.rs (1)
184-185: Consider skipping large parameters in traces.The
AppHandleparameter is a large structure. For cleaner trace output, consider using#[instrument(skip(app))]to avoid serializing it.Apply this diff:
-#[instrument] +#[instrument(skip(app))] pub fn set_hotkey(app: AppHandle, action: HotkeyAction, hotkey: Option<Hotkey>) -> Result<(), ()> {apps/desktop/src-tauri/src/recording.rs (1)
668-668: Consider skipping large parameters in traces.These recording control functions take
AppHandleandMutableStateparameters, which are large structures. For cleaner trace output, consider usingskip()to avoid serializing them:Example for one function:
-#[instrument] +#[instrument(skip(app, state))] pub async fn stop_recording(app: AppHandle, state: MutableState<'_, App>) -> Result<(), String> {Apply similar changes to
pause_recording,resume_recording,restart_recording, anddelete_recording.Also applies to: 681-681, 694-695, 711-714, 733-734
apps/desktop/src-tauri/src/windows.rs (2)
35-51: Consider adding Debug derive for consistency.The
CapWindowIdenum doesn't deriveDebug, while the relatedShowCapWindowenum (line 181) does. For consistency and improved observability, consider addingDebugtoCapWindowId.-#[derive(Clone, Deserialize, Type)] +#[derive(Debug, Clone, Deserialize, Type)] pub enum CapWindowId {
819-820: Consider skipping large parameters in traces.These window-related functions take
tauri::WindoworAppHandleparameters, which are large structures. For cleaner trace output, consider usingskip():Examples:
-#[instrument] +#[instrument(skip(window))] pub fn set_theme(window: tauri::Window, theme: AppTheme) {-#[instrument] +#[instrument(skip(_window))] pub fn position_traffic_lights(_window: tauri::Window, _controls_inset: Option<(f64, f64)>) {-#[instrument] +#[instrument(skip(app))] pub fn refresh_window_content_protection(app: AppHandle<Wry>) -> Result<(), String> {Also applies to: 836-837, 886-887
apps/desktop/src-tauri/src/captions.rs (1)
60-63: Avoid instrumenting heavy/binary/opaque argsSeveral #[instrument] sites capture large or non-Debug args (Window/AppHandle, Vec, CaptionData). This can bloat logs or fail to compile if Debug isn’t implemented. Skip them.
Apply these diffs:
save_model_file:
-#[instrument] +#[instrument(skip(data))] pub async fn save_model_file(path: String, data: Vec<u8>) -> Result<(), String> {download_whisper_model:
-#[instrument] +#[instrument(skip(window))] pub async fn download_whisper_model( window: Window,save_captions:
-#[instrument] +#[instrument(skip(app, captions))] pub async fn save_captions(load_captions:
-#[instrument] +#[instrument(skip(app))] pub async fn load_captions(export_captions_srt:
-#[instrument] +#[instrument(skip(app))] pub async fn export_captions_srt(Also applies to: 1052-1057, 719-725, 974-979, 1197-1202
apps/desktop/src-tauri/src/lib.rs (1)
824-831: Skip Window in instrumentation to avoid heavy/opaque arg captureWindow is large/opaque; capturing it can be noisy and may require Debug. Skip it.
-#[instrument] +#[instrument(skip(window))] async fn create_editor_instance(window: Window) -> Result<SerializedEditorInstance, String> {apps/desktop/src-tauri/src/main.rs (1)
106-112: Flush OTel spans on shutdownWithout a shutdown, buffered spans may be lost. Call opentelemetry::global::shutdown_tracer_provider() when the app exits (debug builds).
Add at shutdown (e.g., after run returns or in a RunEvent::Exit handler):
#[cfg(debug_assertions)] opentelemetry::global::shutdown_tracer_provider();apps/desktop/src-tauri/src/api.rs (1)
113-121: Fix instrumentation arguments
- upload_multipart_complete currently logs parts; that’s noisy and may leak ETags. Skip parts (and meta if large).
- upload_signed uses #[instrument(skip())]; empty skip is likely unintended. Skip app/body.
-#[instrument] +#[instrument(skip(parts, meta))] pub async fn upload_multipart_complete(-#[instrument(skip())] +#[instrument(skip(app, body))] pub async fn upload_signed(Ensure #[instrument(skip())] compiles as-is; if not, the change above is required.
Also applies to: 183-189
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
apps/desktop/src-tauri/Cargo.toml(2 hunks)apps/desktop/src-tauri/src/api.rs(7 hunks)apps/desktop/src-tauri/src/captions.rs(8 hunks)apps/desktop/src-tauri/src/export.rs(3 hunks)apps/desktop/src-tauri/src/fake_window.rs(2 hunks)apps/desktop/src-tauri/src/general_settings.rs(2 hunks)apps/desktop/src-tauri/src/hotkeys.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(42 hunks)apps/desktop/src-tauri/src/main.rs(2 hunks)apps/desktop/src-tauri/src/permissions.rs(3 hunks)apps/desktop/src-tauri/src/platform/mod.rs(2 hunks)apps/desktop/src-tauri/src/recording.rs(7 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(6 hunks)apps/desktop/src-tauri/src/upload.rs(13 hunks)apps/desktop/src-tauri/src/windows.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/desktop/src-tauri/src/general_settings.rsapps/desktop/src-tauri/src/export.rsapps/desktop/src-tauri/src/fake_window.rsapps/desktop/src-tauri/src/hotkeys.rsapps/desktop/src-tauri/src/platform/mod.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/permissions.rsapps/desktop/src-tauri/src/target_select_overlay.rsapps/desktop/src-tauri/src/api.rsapps/desktop/src-tauri/src/captions.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/main.rsapps/desktop/src-tauri/src/upload.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:
apps/desktop/src-tauri/src/general_settings.rsapps/desktop/src-tauri/src/export.rsapps/desktop/src-tauri/src/fake_window.rsapps/desktop/src-tauri/src/hotkeys.rsapps/desktop/src-tauri/src/platform/mod.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/permissions.rsapps/desktop/src-tauri/src/target_select_overlay.rsapps/desktop/src-tauri/src/api.rsapps/desktop/src-tauri/src/captions.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/main.rsapps/desktop/src-tauri/src/upload.rs
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/fake_window.rs (1)
apps/desktop/src/utils/tauri.ts (1)
LogicalBounds(415-415)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src-tauri/src/windows.rs (6)
fmt(93-113)app(214-214)app(366-366)app(456-456)app(771-771)app(991-991)
⏰ 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 (13)
apps/desktop/src-tauri/src/general_settings.rs (1)
7-7: LGTM! Instrumentation added correctly.The import and
#[instrument]attribute are properly added to enable tracing for this Tauri command, consistent with the PR's goal of adding observability.Also applies to: 259-259
apps/desktop/src-tauri/src/export.rs (3)
7-7: LGTM!The addition of
instrumentto the tracing imports is necessary for the instrumentation attributes applied to the functions below.
27-27: LGTM!The instrumentation attribute is correctly applied with
skip(progress)for the channel parameter. PathBuf and ExportSettings both implement Debug, so they will be properly captured in traces.
92-92: Confirm cap_project::XY implements Debug. The instrumentation macro will capture theresolutionparameter of typeXY<u32>; ifXYlacks aDebugimpl, the build will fail.apps/desktop/src-tauri/src/target_select_overlay.rs (1)
21-21: LGTM! Instrumentation pattern is appropriate.The
#[instrument]attributes enable tracing for these Tauri commands, and the use ofskip(state)on line 45 correctly avoids serializing the large state parameter.Also applies to: 45-45, 106-106, 119-119, 133-133, 149-149
apps/desktop/src-tauri/Cargo.toml (1)
137-142: Clarify whether OpenTelemetry dependencies should be debug-only.The commented-out
[target.'cfg(debug_assertions)'.dependencies]header on line 138 suggests these dependencies were intended to be conditionally included only in debug builds. However, the dependencies are currently active unconditionally. Is this intentional (always-on tracing) or should the condition be uncommented?apps/desktop/src-tauri/src/platform/mod.rs (1)
11-11: LGTM! Instrumentation is appropriate.The
#[instrument]attribute onperform_haptic_feedbackenables observability for this platform-specific function.Also applies to: 33-33
apps/desktop/src-tauri/src/hotkeys.rs (1)
16-16: LGTM! Debug derive improves observability.Adding
Debugto theHotkeystruct enables better trace output and debugging.apps/desktop/src-tauri/src/fake_window.rs (1)
5-5: LGTM! Correct use of skip for state parameters.The instrumentation correctly uses
skip(state)to avoid serializing the large state structure in traces.Also applies to: 11-11, 28-28
apps/desktop/src-tauri/src/permissions.rs (1)
5-5: LGTM! Instrumentation and Debug derive are appropriate.The
Debugderive onOSPermissionand the#[instrument]attribute onrequest_permissionimprove observability without adding unnecessary overhead.Also applies to: 15-15, 65-65
apps/desktop/src-tauri/src/recording.rs (1)
216-216: LGTM! Instrumentation enhances observability for display/window listing.The
#[instrument]attributes on these functions enable tracing for capture target enumeration.Also applies to: 227-227
apps/desktop/src-tauri/src/windows.rs (1)
181-181: LGTM! Debug derive improves observability.Adding
DebugtoShowCapWindowenables better trace output for window operations.apps/desktop/src-tauri/src/upload.rs (1)
620-683: Multipart uploader structure and instrumentation look solidChunked presign + PUT with MD5, retryable client, part de-dupe, and span timing are well-structured. No blocking issues found here.
Please confirm S3 expects base64(MD5) in Content-MD5 for your configuration (some proxies require hex). Also verify timeouts are acceptable for large parts (currently 120s).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
27-27: Pin the grafana/otel-lgtm image tag for reproducibility.Running
docker.io/grafana/otel-lgtmwithout an explicit tag pulls whatever “latest” points to at runtime, so the script can suddenly break (or change behavior) when the image is updated upstream. Please pin this to a known-good version (e.g.,docker.io/grafana/otel-lgtm:<tag>) so the team gets consistent results.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json(1 hunks)
⏰ 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)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
apps/desktop/src-tauri/src/api.rs (5)
11-15: Instrument without breaking Debug constraints; include useful fieldsAppHandle likely doesn’t implement Debug. #[instrument] will try to format all args by default. Skip it and add key fields for correlation.
Apply:
-#[instrument] +#[instrument(name = "upload_multipart_initiate", skip(app), fields(%video_id))]
49-56: Avoid logging non-Debug and sensitive/large args in tracing spanSkip AppHandle and md5_sum (sensitive). Include identifiers.
-#[instrument] +#[instrument(name = "upload_multipart_presign_part", skip(app, md5_sum), fields(%video_id, %upload_id, part_number))]
113-120: Skip large collections in spans; record summary fieldsDon’t log the entire parts slice. Skip AppHandle and meta, include counts.
-#[instrument] +#[instrument(name = "upload_multipart_complete", skip(app, parts, meta), fields(%video_id, %upload_id, parts = parts.len()))]
183-187: Invalid #[instrument(skip())] syntax; skip specific argsskip() with no args is invalid. Also skip AppHandle and body to avoid logging request content.
Apply:
-#[instrument(skip())] +#[instrument(name = "upload_signed", skip(app, body))]
221-227: Skip AppHandle in spans; add identifiersAvoid Debug requirement for AppHandle; include key fields.
-#[instrument] +#[instrument(name = "desktop_video_progress", skip(app), fields(%video_id, uploaded, total))]apps/desktop/src-tauri/src/upload.rs (3)
60-68: Instrument upload_video with skip and key fieldsSkip non-Debug/heavy args; record the identifier.
-#[instrument(skip(channel))] +#[instrument(name = "upload_video", skip(app, channel, file_path, screenshot_path, meta), fields(%video_id))]
173-178: Skip AppHandle in upload_image spanAvoid Debug requirement; include filename via later logs if needed.
-#[instrument] +#[instrument(skip(app, file_path))]
693-701: Skip AppHandle and stream; include minimal fieldsReduce noise and avoid Debug on AppHandle.
-#[instrument(skip(stream))] +#[instrument(skip(app, stream), fields(total_size))]
🧹 Nitpick comments (14)
apps/desktop/src-tauri/src/fake_window.rs (1)
11-17: Avoid Debug-formatting non-Debug/heavy args in #[instrument]Skip window and state to prevent potential compile errors and noisy logs; record the window label instead if needed.
-#[instrument(skip(state))] +#[instrument(skip(window, state))] pub async fn set_fake_window_bounds(-#[instrument(skip(state))] +#[instrument(skip(window, state))] pub async fn remove_fake_window(Also applies to: 28-33
apps/desktop/src-tauri/src/target_select_overlay.rs (2)
45-49: Instrument: skip AppHandle/State and capture errorsTo avoid Debug requirements on AppHandle and reduce noise, skip them and record errors automatically.
-#[instrument(skip(state))] +#[instrument(skip(app, state), err)] pub async fn open_target_select_overlays(-#[instrument] +#[instrument(skip(app), err)] pub async fn close_target_select_overlays(app: AppHandle) -> Result<(), String> {Also applies to: 106-108
119-121: Optional: add err to Result-returning commandsHelpful for tracing failures without extra logs.
-#[instrument] +#[instrument(err)] pub async fn get_window_icon(window_id: &str) -> Result<Option<String>, String> {-#[instrument] +#[instrument(err)] pub async fn display_information(display_id: &str) -> Result<DisplayInformation, String> {-#[instrument] +#[instrument(err)] pub async fn focus_window(window_id: WindowId) -> Result<(), String> {Also applies to: 133-135, 149-151
apps/desktop/src-tauri/src/hotkeys.rs (1)
184-186: Instrument: skip AppHandle and capture errorsAppHandle may not implement Debug; skip it and record errors.
-#[instrument] +#[instrument(skip(app), err)] pub fn set_hotkey(app: AppHandle, action: HotkeyAction, hotkey: Option<Hotkey>) -> Result<(), ()> {apps/desktop/src-tauri/src/main.rs (2)
76-105: OTel exporter lifecycle: ensure graceful shutdown; avoid unnecessary unsafe
- Add a shutdown call to flush spans before exit; otherwise traces can be lost.
- set_var does not require unsafe; remove the unsafe block.
Add at program end (after block_on(...)):
opentelemetry::global::shutdown_tracer_provider();And remove unsafe around RUST_LOG setup:
std::env::set_var("RUST_LOG", "trace");Optional:
- Consider Protocol::HttpProtobuf for lower overhead if your collector supports it.
- Ensure the batch runtime matches your environment; if you see panics about tokio runtime, initialize the provider after building the tokio runtime or configure the batch processor with an explicit tokio runtime.
107-112: Filter scope: confirm intent to drop non-cap_ logs*The filter restricts all events (including OTLP export) to targets starting with "cap_". If you want third‑party crate spans/logs (tauri, tokio, wry) in files but not in OTLP, split filters per layer; otherwise this is fine.
apps/desktop/src-tauri/src/windows.rs (1)
836-840: Skip heavy args in position_traffic_lights spanSkip window (and optionally inset) to avoid Debug and noisy logs.
-#[instrument] +#[instrument(skip(_window, _controls_inset))]apps/desktop/src-tauri/src/upload.rs (7)
69-76: Use tracing instead of println! for consistencyPrefer structured logs.
- println!("Uploading video {video_id}..."); + info!("Uploading video {video_id}...");
222-229: Skip AppHandle in create_or_get_video span; add concise fieldsRecord only minimal identifiers.
-#[instrument] +#[instrument(skip(app, meta, name, video_id), fields(is_screenshot))]
464-485: Attach generated stream to current span; also skip heavy inputsfrom_file_to_chunks logs are fine; consider skipping path to avoid exposing paths in spans if privacy is a concern.
-#[instrument] +#[instrument(skip(path))]
491-495: Skip inputs in from_pending_file_to_chunks spanAvoid logging paths and channels; attach to current span is already done.
-#[instrument(skip(realtime_upload_done))] +#[instrument(skip(path, realtime_upload_done))]
651-655: Header value should be a string; avoid passing usize directlyBe explicit to prevent type issues across reqwest versions.
- .header("Content-Length", chunk.len()) + .header("Content-Length", size.to_string())
686-694: Also stringify Content-Length in singlepart_uploaderMatch header type expectations.
- .header("Content-Length", total_size) + .header("Content-Length", total_size.to_string())Also applies to: 701-704
125-131: Consider using provided meta when availableYou compute metadata via build_video_meta even though meta is provided to upload_video; use provided meta if valid to avoid duplicate work.
No diff provided as this spans multiple scopes; suggest:
- Prefer meta from function arg when present/valid.
- Fallback to build_video_meta(&file_path) if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
apps/desktop/src-tauri/Cargo.toml(2 hunks)apps/desktop/src-tauri/src/api.rs(7 hunks)apps/desktop/src-tauri/src/captions.rs(8 hunks)apps/desktop/src-tauri/src/export.rs(3 hunks)apps/desktop/src-tauri/src/fake_window.rs(2 hunks)apps/desktop/src-tauri/src/general_settings.rs(2 hunks)apps/desktop/src-tauri/src/hotkeys.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(42 hunks)apps/desktop/src-tauri/src/main.rs(2 hunks)apps/desktop/src-tauri/src/permissions.rs(3 hunks)apps/desktop/src-tauri/src/platform/mod.rs(2 hunks)apps/desktop/src-tauri/src/recording.rs(7 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(6 hunks)apps/desktop/src-tauri/src/upload.rs(13 hunks)apps/desktop/src-tauri/src/windows.rs(5 hunks)package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/desktop/src-tauri/src/platform/mod.rsapps/desktop/src-tauri/src/general_settings.rsapps/desktop/src-tauri/src/fake_window.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/main.rsapps/desktop/src-tauri/src/captions.rsapps/desktop/src-tauri/src/api.rsapps/desktop/src-tauri/src/export.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/hotkeys.rsapps/desktop/src-tauri/src/target_select_overlay.rsapps/desktop/src-tauri/src/permissions.rsapps/desktop/src-tauri/src/upload.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:
apps/desktop/src-tauri/src/platform/mod.rsapps/desktop/src-tauri/src/general_settings.rsapps/desktop/src-tauri/src/fake_window.rsapps/desktop/src-tauri/src/lib.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/main.rsapps/desktop/src-tauri/src/captions.rsapps/desktop/src-tauri/src/api.rsapps/desktop/src-tauri/src/export.rsapps/desktop/src-tauri/src/windows.rsapps/desktop/src-tauri/src/hotkeys.rsapps/desktop/src-tauri/src/target_select_overlay.rsapps/desktop/src-tauri/src/permissions.rsapps/desktop/src-tauri/src/upload.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). (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 (11)
apps/desktop/src-tauri/Cargo.toml (1)
138-142: Verify the intended dependency target.The commented-out text on line 138 suggests these OpenTelemetry dependencies should be for
debug_assertions, but they're actually configured forunix. This means production builds on Unix platforms will include these heavyweight telemetry dependencies.If telemetry should only be enabled in debug builds, apply this diff:
-[target.'cfg(unix)'.dependencies] - -# [target.'cfg(debug_assertions)'.dependencies] +[target.'cfg(debug_assertions)'.dependencies] tracing-opentelemetry = "0.32.0" opentelemetry = "0.31.0" -opentelemetry-otlp = "0.31.0" #{ version = , features = ["http-proto", "reqwest-client"] } +opentelemetry-otlp = "0.31.0" opentelemetry_sdk = { version = "0.31.0", features = ["rt-tokio", "trace"] }Alternatively, if Unix-specific builds genuinely require OTLP in production, consider updating the comment to reflect this or adding a feature flag for optional inclusion.
apps/desktop/src-tauri/src/lib.rs (1)
1938-1938: LGTM: Good visibility reduction.Changing
FilteredRegistryfrompubto module-private is appropriate, as this internal type alias doesn't need to be part of the public API.apps/desktop/src-tauri/src/hotkeys.rs (1)
16-24: Deriving Debug on Hotkey is a good additionEnables cleaner tracing and diagnostics with #[instrument].
apps/desktop/src-tauri/src/permissions.rs (1)
15-22: LGTM: Debug derive and instrumentationAdding Debug to OSPermission and #[instrument] on request_permission improves observability with minimal overhead.
Also applies to: 65-67
apps/desktop/src-tauri/src/api.rs (2)
92-101: LGTM on Debug derive for UploadedPartHelps spans and logs without exposing total_size due to serde skip.
165-181: LGTM on Debug derives for public typesAppropriate for structured logs/spans.
apps/desktop/src-tauri/src/windows.rs (2)
21-21: LGTM on tracing importsPrepares file for instrumentation and structured logs.
181-209: LGTM on Debug derive for ShowCapWindowImproves diagnostics when included in spans/logs.
apps/desktop/src-tauri/src/upload.rs (3)
282-306: LGTM: build_video_meta instrumentationSafe inputs; helpful for diagnosing metadata issues.
308-337: LGTM: compress_image instrumentationBounded blocking inside spawn_blocking; good error paths.
680-683: LGTM: overall multipart timing in spanUseful end-to-end timing.
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-tauri/src/upload.rs (2)
238-249: URL-encode query parameters (name) to avoid broken/ambiguous requests
nameis embedded raw into the query string. Spaces or special characters will break the URL or be misinterpreted. Percent‑encode it.- if let Some(name) = name { - s3_config_url.push_str(&format!("&name={name}")); - } + if let Some(name) = name { + s3_config_url.push_str("&name="); + s3_config_url.push_str(&urlencoding::encode(&name)); + }Add import:
+use urlencoding::encode;
309-321: Clamp resized dimensions to avoid zero-size on tiny imagesHalving very small images can produce 0 width/height. Clamp to at least 1.
- let resized_img = img.resize( - img.width() / 2, - img.height() / 2, + let resized_img = img.resize( + std::cmp::max(1, img.width() / 2), + std::cmp::max(1, img.height() / 2), image::imageops::FilterType::Nearest, );
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/upload.rs (1)
619-626: Add identifiers to span and make Content-Length header explicitInclude
video_idandupload_idin the span for correlation. Also, passContent-Lengthas a string to avoid header-value conversion issues.-#[instrument(skip(app, stream))] +#[instrument(skip(app, stream), fields(%video_id, %upload_id))] fn multipart_uploader( @@ - .header("Content-Length", chunk.len()) + .header("Content-Length", chunk.len().to_string())Also applies to: 652-653
🧹 Nitpick comments (4)
apps/desktop/src-tauri/src/hotkeys.rs (1)
184-184: LGTM!The instrumentation attribute is correctly configured with
skip(app)sinceAppHandledoesn't implementDebug. The remaining parameters (actionandhotkey) will be automatically logged and contain no sensitive data.Consider instrumenting the
handle_hotkeyfunction (line 133) as well to provide end-to-end visibility into hotkey actions:+#[instrument] async fn handle_hotkey(app: AppHandle, action: HotkeyAction) -> Result<(), String> {You'll need to skip
apphere too:+#[instrument(skip(app))] async fn handle_hotkey(app: AppHandle, action: HotkeyAction) -> Result<(), String> {apps/desktop/src-tauri/src/upload.rs (3)
60-70: Add span fields and avoid println; prefer tracing macrosInclude
video_idin the span and replaceprintln!withinfo!for consistent observability.-#[instrument(skip(app, channel))] +#[instrument(skip(app, channel), fields(%video_id))] pub async fn upload_video( @@ - println!("Uploading video {video_id}..."); + info!("Uploading video {video_id}...");
282-306: Generalize path argument to be idiomatic and flexibleAccept
impl AsRef<Path>instead of&PathBuf. This improves ergonomics and aligns with Rust conventions.-#[instrument] -pub fn build_video_meta(path: &PathBuf) -> Result<S3VideoMeta, String> { - let input = - ffmpeg::format::input(path).map_err(|e| format!("Failed to read input file: {e}"))?; +#[instrument] +pub fn build_video_meta(path: impl AsRef<Path>) -> Result<S3VideoMeta, String> { + let input = ffmpeg::format::input(path.as_ref()) + .map_err(|e| format!("Failed to read input file: {e}"))?;
693-699: Be explicit with Content-Length header typeFor consistency and to avoid implicit conversions, send
Content-Lengthas a string (or use the typed constant).- .header("Content-Length", total_size) + .header("Content-Length", total_size.to_string())Alternatively:
+use reqwest::header::CONTENT_LENGTH; @@ - .header("Content-Length", total_size.to_string()) + .header(CONTENT_LENGTH, total_size.to_string())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/desktop/src-tauri/Cargo.toml(1 hunks)apps/desktop/src-tauri/src/captions.rs(8 hunks)apps/desktop/src-tauri/src/fake_window.rs(2 hunks)apps/desktop/src-tauri/src/hotkeys.rs(2 hunks)apps/desktop/src-tauri/src/lib.rs(41 hunks)apps/desktop/src-tauri/src/recording.rs(7 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(6 hunks)apps/desktop/src-tauri/src/upload.rs(13 hunks)apps/desktop/src-tauri/src/windows.rs(6 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/desktop/src-tauri/src/fake_window.rs
- apps/desktop/src-tauri/Cargo.toml
- apps/desktop/src-tauri/src/target_select_overlay.rs
- apps/desktop/src-tauri/src/lib.rs
- apps/desktop/src-tauri/src/recording.rs
- apps/desktop/src-tauri/src/windows.rs
- package.json
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/desktop/src-tauri/src/hotkeys.rsapps/desktop/src-tauri/src/upload.rsapps/desktop/src-tauri/src/captions.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:
apps/desktop/src-tauri/src/hotkeys.rsapps/desktop/src-tauri/src/upload.rsapps/desktop/src-tauri/src/captions.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). (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 (13)
apps/desktop/src-tauri/src/hotkeys.rs (2)
14-14: LGTM!The tracing import is correctly added to support the instrumentation attribute on line 184.
16-16: LGTM!Adding the
Debugderive is necessary for the#[instrument]macro to automatically log thehotkeyparameter inset_hotkey. This change enables OpenTelemetry to capture the hotkey configuration details.apps/desktop/src-tauri/src/captions.rs (11)
19-19: LGTM!The tracing import is required for the instrumentation attributes added throughout the file.
52-52: LGTM!The instrumentation attribute is correctly applied to capture function parameters for tracing.
60-60: LGTM!The instrumentation attribute is correctly applied to capture function parameters for tracing.
656-656: LGTM!The instrumentation attribute is correctly applied to capture function parameters for tracing.
1052-1052: LGTM!The instrumentation correctly uses
skip(window)since theWindowtype doesn't implementDebug.
1144-1144: LGTM!The instrumentation attribute is correctly applied to capture function parameters for tracing.
1152-1152: LGTM!The instrumentation attribute is correctly applied to capture function parameters for tracing.
1197-1213: Run the following script to locate all call sites ofexportCaptionsSrtin the frontend code:#!/bin/bash rg -n --type=ts --type=tsx 'exportCaptionsSrt\s*\('
1-1246: Run rustfmt and clippy on captions.rs
Sandbox couldn’t verify; manually run:cargo fmt -- apps/desktop/src-tauri/src/captions.rs cargo clippy --workspace -- -D warnings
719-723: No impact to JS callers from reorderingappparameter
Tauri injects theAppHandleat runtime—JS callers still pass onlyvideoIdandcaptions. No updates to existing TypeScript call sites are needed.Likely an incorrect or invalid review comment.
974-977: Ignore parameter order change; no breaking change The Tauri command uses named arguments forvideo_idand auto-injectsAppHandle, so reordering parameters doesn’t impact any frontend calls.Likely an incorrect or invalid review comment.
3 participants
This allows us to see what's going on under the hood which will help detect and fix performance issues.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores