provide md5 hash for self hosted instances #1250
Conversation
WalkthroughAdds optional MD5 support to multipart presign requests and integrates MD5-aware upload flow; expands upload_video to accept file and screenshot paths; adds ManagerExt::is_server_url_custom to toggle MD5 behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant upload_video
participant ManagerExt as ManagerExt::is_server_url_custom
participant multipart as MultipartUploader
participant api as upload_multipart_presign_part
User->>upload_video: upload_video(video_id, file_path, screenshot_path, meta, ...)
activate upload_video
upload_video->>ManagerExt: is_server_url_custom()
ManagerExt-->>upload_video: use_md5_hashes (bool)
alt use_md5_hashes == true
upload_video->>multipart: start (MD5-enabled)
multipart->>multipart: read chunk, compute MD5
multipart->>api: presign_part(videoId, uploadId, partNumber, md5_sum=Some)
api-->>multipart: presigned_url
multipart->>multipart: upload chunk with Content-MD5 header
else use_md5_hashes == false
upload_video->>multipart: start (MD5-disabled)
multipart->>multipart: read chunk
multipart->>api: presign_part(..., md5_sum=None)
api-->>multipart: presigned_url
multipart->>multipart: upload chunk without Content-MD5
end
multipart-->>upload_video: progress / UploadedItem
deactivate upload_video
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
⏰ 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)
🔇 Additional comments (2)
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: 2
🧹 Nitpick comments (4)
apps/desktop/src-tauri/src/api.rs (2)
55-56: API surface OK; consider owned String for ergonomic calls.Option<&str> works, but callers often hold owned Strings (as in upload.rs). Using Option avoids borrow lifetimes across awaits. Not required, just a nicety.
63-71: Prefer a typed request struct with skip_serializing_if over ad‑hoc Map.Cleaner, clippy‑friendly, and avoids json!(body) double-conversion. Also makes the contract explicit.
@@ - let mut body = serde_json::Map::from_iter([ - ("videoId".to_string(), json!(video_id)), - ("uploadId".to_string(), json!(upload_id)), - ("partNumber".to_string(), json!(part_number)), - ]); - - if let Some(md5_sum) = md5_sum { - body.insert("md5Sum".to_string(), json!(md5_sum)); - } + #[derive(Serialize)] + #[serde(rename_all = "camelCase")] + struct PresignPartRequest<'a> { + video_id: &'a str, + upload_id: &'a str, + part_number: u32, + #[serde(skip_serializing_if = "Option::is_none")] + md5_sum: Option<&'a str>, + } + let body = PresignPartRequest { + video_id, + upload_id, + part_number, + md5_sum, + }; @@ - .json(&serde_json::json!(body)) + .json(&body)Also applies to: 77-78
apps/desktop/src-tauri/src/web_api.rs (1)
88-89: Method name vs behavior is confusing.is_server_url_custom suggests true for custom URLs, but current impl returns true for the default (env) URL. Either rename to is_default_server or invert the logic to match the name.
apps/desktop/src-tauri/src/upload.rs (1)
637-639: Header typing and small clippy nits.
- Use typed header constants; pass Content-Length as u64/string.
- Keep timeout/body chaining but avoid intermediate mut unless needed.
- .put(&presigned_url) - .header("Content-Length", chunk.len()) - .timeout(Duration::from_secs(120)).body(chunk); + .put(&presigned_url) + .header(reqwest::header::CONTENT_LENGTH, size as u64) + .timeout(Duration::from_secs(120)) + .body(chunk); @@ - if let Some(md5_sum) = &md5_sum { - req = req.header("Content-MD5", md5_sum); - } + if let Some(md5_sum) = &md5_sum { + req = req.header(reqwest::header::CONTENT_MD5, md5_sum); + }Optionally hoist
use reqwest::header::{CONTENT_LENGTH, CONTENT_MD5};at file top and reference those.Also applies to: 645-650, 651-653, 655-659
📜 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 (3)
apps/desktop/src-tauri/src/api.rs(1 hunks)apps/desktop/src-tauri/src/upload.rs(2 hunks)apps/desktop/src-tauri/src/web_api.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/web_api.rsapps/desktop/src-tauri/src/api.rsapps/desktop/src-tauri/src/upload.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/upload.rs (2)
apps/desktop/src-tauri/src/web_api.rs (2)
is_server_url_custom(88-88)is_server_url_custom(131-140)apps/desktop/src-tauri/src/api.rs (5)
upload_multipart_presign_part(50-95)resp(43-43)resp(91-91)resp(166-166)resp(222-222)
⏰ 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 (3)
apps/desktop/src-tauri/src/api.rs (1)
69-71: Confirm backend field name.Please verify the server expects "md5Sum" (camelCase) in the JSON (not "contentMd5" or similar).
apps/desktop/src-tauri/src/upload.rs (2)
622-625: MD5 computation approach is fine.Chunk-local MD5 via md5 crate + base64 matches Content-MD5 expectations for S3-compatible stores.
Confirm your presign logic includes Content-MD5 in the signature conditions for self-hosted backends; otherwise uploads will fail server-side.
59-67: All callsites correctly updated to match the new signature.Verification confirms:
- Recording.rs line 980: passes 6 args correctly (app, video_id, file_path, screenshot_path, meta, None)
- Lib.rs line 1149: passes 6 args correctly (app, video_id, file_path, screenshot_path, metadata, Some(channel))
- Lib.rs line 2622: passes 6 args correctly (app, video_id, file_path, screenshot_path, meta, None)
All arguments match the new signature. The
#[instrument]attribute correctly skips sensitive data (app, file_path, screenshot_path, channel).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/desktop/src-tauri/src/upload.rs (1)
596-599: Rename the flag for intent clarity; it denotes “custom server,” not “use MD5.”The logic is correct, but the name reads as behavior, not condition. Prefer is_custom_server and branch on that.
- let use_md5_hashes = app.is_server_url_custom().await; + let is_custom_server = app.is_server_url_custom().await; @@ - let (item, mut presigned_url, md5_sum) = if use_md5_hashes { + let (item, mut presigned_url, md5_sum) = if is_custom_server {To ensure MD5 actually engages for self‑hosted builds, please verify that VITE_SERVER_URL is set in your packaging/build and that custom detection won’t default to false:
#!/bin/bash set -euo pipefail echo "Searching for VITE_SERVER_URL definitions/usages…" rg -nS "VITE_SERVER_URL" -C2 echo "Where is server_url set/loaded?" rg -nS "server_url" apps/desktop -C2
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/upload.rs (1)
644-656: Use typed header names and explicit string for Content-Length.Avoid implicit numeric-to-header conversions and prefer constants; keeps clippy quiet and improves readability.
- let mut req = retryable_client(url.host().unwrap_or("<unknown>").to_string()) + let mut req = retryable_client(url.host().unwrap_or("<unknown>").to_string()) .build() .map_err(|err| format!("uploader/part/{part_number}/client: {err:?}"))? .put(&presigned_url) - .header("Content-Length", chunk.len()) + .header(reqwest::header::CONTENT_LENGTH, size.to_string()) .timeout(Duration::from_secs(120)).body(chunk); if let Some(md5_sum) = &md5_sum { - req = req.header("Content-MD5", md5_sum); + req = req.header(reqwest::header::CONTENT_MD5, md5_sum); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/upload.rs(2 hunks)apps/desktop/src-tauri/src/web_api.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/web_api.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/upload.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/upload.rs (2)
apps/desktop/src-tauri/src/web_api.rs (2)
is_server_url_custom(88-88)is_server_url_custom(131-140)apps/desktop/src-tauri/src/api.rs (5)
upload_multipart_presign_part(50-95)resp(43-43)resp(91-91)resp(166-166)resp(222-222)
⏰ 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-tauri/src/upload.rs (3)
59-59: Good call: don’t instrument file paths.Skipping app, channel, and the new file_path/screenshot_path avoids leaking local paths in spans.
603-631: MD5 path behavior looks correct.Reads the actual chunk, computes base64(MD5), presigns with md5Sum, and returns Some(md5_sum); non‑MD5 path keeps the prefetch optimization.
637-639: Nice: re-presign on part mismatch preserves MD5.Passing md5_sum.as_deref() ensures the server pre-signs with the expected digest for the actual part.
1 participant
Summary by CodeRabbit
New Features
Refactor