feat: add retry for lock in frontend - #1121
Conversation
|
Review changes with Changed Files
|
sauraww
changed the title
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughFrontend HTTP utilities now detect locked-workspace conflict responses in browsers, wait with bounded exponential backoff, and retry requests. Error parsing, SSR cookies, skip-error handling, alerts, and terminal error propagation remain supported. ChangesWorkspace lock retry flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant request_with_skip_error
participant Backend
participant sleep
Browser->>request_with_skip_error: start request
request_with_skip_error->>Backend: send HTTP request
Backend-->>request_with_skip_error: 409 response with lock info
request_with_skip_error->>sleep: wait with exponential backoff
sleep-->>request_with_skip_error: delay complete
request_with_skip_error->>Backend: retry request
Backend-->>request_with_skip_error: success or terminal error
request_with_skip_error-->>Browser: return response or error
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds client-side retry/backoff behavior to frontend HTTP requests when the backend reports a workspace lock conflict (HTTP 409 with a lock payload), reducing transient failures during concurrent workspace write operations.
Changes:
- Introduces retry constants and an exponential backoff delay helper for workspace-lock conflicts.
- Adds a browser-only retry loop around
request_with_skip_error, retrying conflicted requests before surfacing an error. - Adds a small async
sleeputility (based onset_timeout) to delay between retries.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return Ok(response); | ||
| } | ||
|
|
||
| Ok(response) | ||
| Err(String::from("request attempts exhausted")) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/frontend/src/utils.rs (2)
203-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for
workspace_lock_retry_delay.This is a small pure function driving the retry backoff schedule; a quick table test (retry 0/1/2 → 250ms/500ms/1000ms) would lock in the exponential-backoff contract and catch regressions if
MAX_WORKSPACE_LOCK_RETRIESor the base delay changes later.✅ Suggested test
#[cfg(test)] mod tests { use super::*; #[test] fn workspace_lock_retry_delay_backs_off_exponentially() { assert_eq!(workspace_lock_retry_delay(0), Duration::from_millis(250)); assert_eq!(workspace_lock_retry_delay(1), Duration::from_millis(500)); assert_eq!(workspace_lock_retry_delay(2), Duration::from_millis(1000)); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/frontend/src/utils.rs` around lines 203 - 227, Add a unit-test module for workspace_lock_retry_delay with a table-style test covering retries 0, 1, and 2, asserting delays of 250ms, 500ms, and 1000ms respectively.
206-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider unifying
RequestErrorResponseandErrorResponse.
RequestErrorResponse(Lines 206-211) duplicates themessagefield already present onErrorResponse(used at Line 340) purely to add the optionallockpayload. Adding an optionallockfield to the existingErrorResponse(or havingRequestErrorResponsewrap/extend it) would avoid maintaining two near-identical error-response shapes going forward.Also applies to: 300-307, 339-342
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/frontend/src/utils.rs` around lines 206 - 211, Unify RequestErrorResponse with the existing ErrorResponse type by adding the optional lock payload to ErrorResponse and reusing it wherever request errors are deserialized. Remove the duplicate RequestErrorResponse definition and update the affected parsing and response handling near the existing ErrorResponse usage, preserving the current message and lock behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/frontend/src/utils.rs`:
- Around line 203-227: Add a unit-test module for workspace_lock_retry_delay
with a table-style test covering retries 0, 1, and 2, asserting delays of 250ms,
500ms, and 1000ms respectively.
- Around line 206-211: Unify RequestErrorResponse with the existing
ErrorResponse type by adding the optional lock payload to ErrorResponse and
reusing it wherever request errors are deserialized. Remove the duplicate
RequestErrorResponse definition and update the affected parsing and response
handling near the existing ErrorResponse usage, preserving the current message
and lock behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3229b98a-61f1-4b5d-9c1a-13258411a982
📒 Files selected for processing (1)
crates/frontend/src/utils.rs
c45f9a6 to
6171cbd
Compare
Datron
left a comment
Datron
left a comment
There was a problem hiding this comment.
Can you attach a video of a demo
| lock: Option<serde_json::Value>, | ||
| } | ||
|
|
||
| async fn sleep(duration: Duration) { |
There was a problem hiding this comment.
Can't a native JS or tokio module do this?
| @@ -301,7 +400,19 @@ pub async fn request<T>( | |||
| where | |||
| T: serde::Serialize, | |||
| { | |||
| request_with_skip_error(url, method, body, headers, &[]).await | |||
| request_with_policy(url, method, body, headers, &[], RetryPolicy::None).await | |||
| } | |||
|
|
|||
| pub async fn request_with_workspace_lock_retry<T>( | |||
| url: String, | |||
| method: reqwest::Method, | |||
| body: Option<T>, | |||
| headers: HeaderMap, | |||
| ) -> Result<reqwest::Response, String> | |||
| where | |||
| T: serde::Serialize, | |||
| { | |||
| request_with_policy(url, method, body, headers, &[], RetryPolicy::WorkspaceLock).await | |||
| } | |||
There was a problem hiding this comment.
Do we need these wrapper functions? Can they be made generic somehow - it is just different ways to call another function
There was a problem hiding this comment.
Making callers pass RetryPolicy directly would save a few wrapper lines but add policy arguments everywhere and expose internal behavior.
There was a problem hiding this comment.
request() → normal one shot request.
request_with_skip_error() → one shot with accepted error statuses.
request_with_workspace_lock_retry() → explicit lock retry.
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| struct RequestErrorResponse { |
There was a problem hiding this comment.
Would WorkspaceLockedErrorResponse be better for this?
6171cbd to
cbf41ea
Compare
Problem
Describe the problem you are trying to solve here
Solution
Provide a brief summary of your solution so that reviewers can understand your code
Environment variable changes
What ENVs need to be added or changed
Pre-deployment activity
Things needed to be done before deploying this change (if any)
Post-deployment activity
Things needed to be done after deploying this change (if any)
API changes
Possible Issues in the future
Describe any possible issues that could occur because of this change
Summary by CodeRabbit