Shared reqwest::Client
#1303
Shared reqwest::Client
#1303
Conversation
|
Warning Rate limit exceeded@Brendonovich has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughIntroduces a shared HTTP client layer: adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Tauri App
participant State as App State
participant RC as RetryableClient
participant C as reqwest::Client
participant Upload as Uploader
rect rgb(220,235,255)
Note over App,State: Startup / Initialization
App->>State: manage(Client::default())
App->>State: manage(RetryableClient::default())
State-->>RC: stored
RC->>C: construct with retry policy (max 5, classify_fn)
end
rect rgb(235,255,220)
Note over Upload,RC: Upload flow using managed client
Upload->>State: get RetryableClient
State-->>Upload: RetryableClient (Deref -> reqwest::Result<Client>)
Upload->>C: build PUT request via &Client
C->>C: execute request with retry/backoff
C-->>Upload: response / error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 0
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/upload.rs (1)
598-634: Add documentation and clarify retry configuration.The RetryableClient implementation looks solid, but could benefit from improvements:
Missing documentation: Add doc comments explaining the purpose, retry behavior, and why it wraps a Result.
Unclear parameter: What does
max_extra_load(5.0)represent? The unit and meaning aren't clear from context.Consider Default vs new(): Since construction can fail (returns Result), consider whether exposing a fallible constructor alongside Default would be clearer.
Apply this diff to add documentation:
+/// A shared HTTP client with automatic retry logic for resilient uploads. +/// +/// This client is configured to retry on: +/// - Server errors (5xx status codes) +/// - Rate limiting (429 Too Many Requests) +/// - Network errors (connection failures, timeouts) +/// +/// The client is wrapped in a Result to handle potential construction failures, +/// which are propagated to usage sites for appropriate error handling. pub struct RetryableClient(reqwest::Result<reqwest::Client>);Also, clarify or document what
max_extra_load(5.0)represents in the retry configuration.
📜 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 (5)
Cargo.toml(1 hunks)apps/desktop/src-tauri/Cargo.toml(1 hunks)apps/desktop/src-tauri/src/lib.rs(1 hunks)apps/desktop/src-tauri/src/upload.rs(4 hunks)crates/api/Cargo.toml(1 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/upload.rsapps/desktop/src-tauri/src/lib.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/lib.rs (1)
apps/desktop/src-tauri/src/upload.rs (3)
app(747-748)app(829-830)default(601-625)
⏰ 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: Analyze (rust)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
🔇 Additional comments (7)
crates/api/Cargo.toml (1)
8-8: LGTM: Version bump aligns with workspace patch.The reqwest version bump to 0.12.24 is consistent with the workspace-level patch and the broader dependency update across the project.
apps/desktop/src-tauri/src/lib.rs (1)
2196-2196: LGTM: Proper initialization of shared HTTP client.The RetryableClient is correctly registered as managed state during the app setup phase, enabling connection pooling across all upload operations.
apps/desktop/src-tauri/src/upload.rs (3)
24-24: LGTM: Import additions support new RetryableClient.The added imports (
Deref,Manager) are necessary for the RetryableClient implementation and its integration with Tauri state management.Also applies to: 30-30
747-756: LGTM: Proper error handling with shared client.The refactor correctly:
- Obtains the client from app state instead of constructing it ad-hoc
- Uses
.as_ref().map_err()to handle the potential client construction failure- Maintains the existing timeout and header configuration
- Enables connection pooling through the shared client
829-838: LGTM: Consistent client usage pattern.The singlepart uploader follows the same pattern as multipart_uploader, correctly using the shared RetryableClient from app state with proper error handling.
apps/desktop/src-tauri/Cargo.toml (1)
68-68: No breaking changes detected — the reqwest update is safe.The changelog from v0.12.7 to v0.12.24 shows only additive features (TCP keepalive, automatic retries API, unix socket support, DNS resolver improvements), bug fixes, and internal refactoring (cookie handling middleware), with no breaking changes. The json, stream, and multipart features remain stable across this version range.
Cargo.toml (1)
89-91: I need to verify if the upstream reqwest already has retry support, which would help determine if this custom patch is necessary.Now let me search the codebase to understand how this custom patch is actually being used.
Documentation required for custom reqwest patch; alternative crate suggestion reconsidered.
The custom reqwest patch from CapSoftware has been verified to exist (commit 9b5ecbd5210a9510fde766015cabb724c1e70d2e) and adds a
retry::alwaysfunction to reqwest's native retry capabilities.However, the original review comment's suggestion to use
reqwest-retryinstead requires clarification:
- As of version 0.12.23, reqwest includes automatic retry functionality that retries only specific protocol-level errors like HTTP/2 REFUSED_STREAM by default. The custom patch adds an "always retry" convenience function for cases where you need broader retry logic.
- reqwest-retry is middleware built on reqwest_middleware, requiring a different client wrapper architecture. The custom patch integrates directly into reqwest's native retry system.
Add a comment in Cargo.toml explaining why the custom fork is necessary. For example: "Requires retry::always() for file upload retry handling" or similar. This addresses the transparency concern without changing the dependency itself.
The security and maintenance considerations from the original review remain valid—document the rationale so future maintainers understand the architectural decision.
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 (2)
apps/desktop/src-tauri/src/captions.rs (1)
1055-1060: Don’t buffer the entire model in memory; stream it to disk and emit progress per chunk.
response.bytes().awaitloads the full file into memory before writing. Whisper models can be very large, which risks OOM and stalls progress updates. Stream withbytes_stream()instead, writing each chunk as it arrives. Optionally, add a per-request timeout.Apply this diff to switch to streaming:
@@ -pub async fn download_whisper_model( - app: AppHandle, +pub async fn download_whisper_model( + app: AppHandle, window: Window, model_name: String, output_path: String, ) -> Result<(), String> { @@ - // Create the client and download the model - let response = app - .state::<client::Client>() - .get(model_url) - .send() - .await - .map_err(|e| format!("Failed to download model: {e}"))?; + // Create the client and start the download (consider a timeout on slow networks) + let response = app + .state::<client::Client>() + .get(model_url) + // .timeout(Duration::from_secs(10 * 60)) // optional: uncomment if you want a hard cap + .send() + .await + .map_err(|e| format!("Failed to download model: {e}"))?; @@ - // 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; + // Download and write in streamed chunks + let mut downloaded: u64 = 0; + let mut stream = response.bytes_stream(); + use futures::StreamExt; + 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; @@ - window - .emit( - DownloadProgress::EVENT_NAME, - DownloadProgress { - message: format!("Downloading model: {progress:.1}%"), - progress, - }, - ) - .map_err(|e| format!("Failed to emit progress: {e}"))?; + window + .emit( + DownloadProgress::EVENT_NAME, + DownloadProgress { + message: format!("Downloading model: {progress:.1}%"), + progress, + }, + ) + .map_err(|e| format!("Failed to emit progress: {e}"))?; }Add the needed import near the top of this file:
+use futures::StreamExt;Also applies to: 1072-1078, 1098-1139
apps/desktop/src-tauri/src/upload.rs (1)
791-799: Add a per-request timeout to singlepart uploads.Multipart has a 5‑minute timeout; singlepart should match to prevent indefinite hangs.
- let resp = app + let resp = app .state::<RetryableClient>() .as_ref() .map_err(|err| format!("singlepart_uploader/client: {err:?}"))? .put(&presigned_url) .header("Content-Length", total_size) .body(reqwest::Body::wrap_stream(stream)) + .timeout(Duration::from_secs(5 * 60)) .send() .await .map_err(|err| format!("singlepart_uploader/error: {err:?}"))?;
🧹 Nitpick comments (1)
apps/desktop/src-tauri/src/web_api.rs (1)
130-133: LGTM: non-authed requests correctly reuse the shared client.This aligns with the pooling goal; keep timeouts centralized in the client builder for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/desktop/src-tauri/src/captions.rs(3 hunks)apps/desktop/src-tauri/src/client.rs(1 hunks)apps/desktop/src-tauri/src/lib.rs(2 hunks)apps/desktop/src-tauri/src/upload.rs(4 hunks)apps/desktop/src-tauri/src/web_api.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src-tauri/src/lib.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/client.rsapps/desktop/src-tauri/src/captions.rsapps/desktop/src-tauri/src/web_api.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 (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
apps/desktop/src-tauri/src/captions.rs (1)
24-24: LGTM on switching to the shared HTTP client.Import is correct and follows the new module boundary.
apps/desktop/src-tauri/src/web_api.rs (1)
99-114: LGTM: shared client wiring for authed requests looks correct.Deref coercions from
State<client::Client>→&reqwest::Clientare idiomatic, and the auth header application remains centralized.If CI warns about lifetime or deref coercion on
State<T>, ping me; we can adjustdo_authed_requestto acceptimpl Deref<Target = reqwest::Client>instead.
This allows for connection pooling and also means the retry policy will be more sane with prevent DOS attacks against our backend.
Summary by CodeRabbit
Chores
Refactor
Bug Fixes / Reliability
New Features