feat(observatory): surface server rejections from the steer controls#1421
Conversation
Follow-up to the kilo review on #1418/#1420: pause, global cap, and per-lane cap all posted optimistically without inspecting res.ok, so a 4xx/5xx (forbidden, unknown lane, rejected value) left the optimistic value standing with no signal. Route all three through a shared postSteer that checks res.ok, shows a dismissible error banner on failure, clears it on the next success, and always reconciles against the server. Done uniformly so the three controls stay consistent rather than patching one. Two new tests (error shown on a rejected post; cleared after a later success); the existing 11 pass through the refactor.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesSteer POST Error Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify(body), | ||
| }); | ||
| setSteerError(res.ok ? null : failMsg); |
There was a problem hiding this comment.
WARNING: The server's rejection body is discarded, so the user sees only the generic failMsg even when the API returned a specific reason.
The PR description explicitly motivates this change on the cases "forbidden", "unknown lane", and "rejected value" — the whole point is that the server can tell the user why the write was refused, and currently that detail is thrown away by postSteer (no res.json() call, no body.detail extraction). The new test on line 239 even mocks body: { detail: "forbidden" }, which would be a useful string to surface — but the production code never reads it.
Consider parsing the response body on !res.ok and preferring body.detail (or body.message) over the hard-coded failMsg, with the generic message kept as a fallback when the body is missing or unparseable. This is the highest-value gap in this change.
| }); | ||
| setSteerError(res.ok ? null : failMsg); | ||
| } catch { | ||
| setSteerError(failMsg); |
There was a problem hiding this comment.
SUGGESTION: The catch branch reuses the same caller-supplied failMsg ("Could not update the pause state.") that the !res.ok branch uses, which is misleading for network/CORS/abort failures.
For a fetch that throws before any response is received, the user is told the server rejected the state when in reality the request never arrived. Consider a distinct message for the thrown branch (e.g. "Could not reach the server.") or, better, inline whatever detail err carries. Low priority but cheap to fix while you're already touching this path.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 5d95286)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5d95286)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Reviewed by minimax-m3 · Input: 44.4K · Output: 2.6K · Cached: 88.7K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@desktop/src/apps/ObservatoryApp.tsx`:
- Around line 132-149: The postSteer function does not guard against race
conditions where concurrent requests complete out of order, potentially clearing
newer errors with older successes. Introduce a request sequence counter that
increments each time postSteer is called, capture the current sequence number at
the start of each postSteer invocation, and gate the setSteerError calls to only
execute if the current request sequence matches the latest sequence number. This
ensures only the most recent in-flight request can update the steerError state,
preventing stale error states from being overwritten by older request
completions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f6d3210-0468-418b-bf4c-da21a0fdf3ee
📒 Files selected for processing (2)
desktop/src/apps/ObservatoryApp.test.tsxdesktop/src/apps/ObservatoryApp.tsx
|
Note Your trial team has used its Gitar budget, so automatic reviews are paused. Upgrade now to unlock full capacity. Comment "Gitar review" to trigger a review manually. Code Review ✅ ApprovedConsolidates steer control writes into a unified postSteer handler that validates server responses, ensuring optimistic updates are correctly reverted upon rejection. No issues found. OptionsDisplay: compact → Showing less information. Comment with these commands to change:
Important Your trial ends in 3 days — upgrade now to keep code review, CI analysis, auto-apply, custom automations, and more. Was this helpful? React with 👍 / 👎 | Gitar |
…rites Fold the CodeRabbit Minor: steer controls are not fully serialized (pause and a cap change use different busy scopes), so two postSteer calls can resolve out of order and a stale result could overwrite a newer one's banner. Stamp each call with a sequence number and only let the latest one set steerError. Deferred the two kilo nits (surfacing the server's specific reason, and distinguishing a network failure from a server rejection) as polish; the generic message is an intentional, accurate choice.
1 participant
What
Follow-up to the kilo review on #1418 / #1420. The pause, global-cap, and per-lane-cap controls all posted optimistically without inspecting
res.ok, so a server rejection (4xx/5xx: forbidden, unknown lane, rejected value) left the optimistic value standing with no user-visible signal until the 5s poll reconciled.Change
postSteer(url, body, failMsg)that checksres.ok, sets a dismissible error banner on failure, clears it on the next success, and always reconciles via the silent reload.setScope/setGlobalCap/setLaneCapso the controls stay consistent (rather than patching only the lane path), which is why this is a dedicated follow-up rather than a fold on feat(observatory): per-lane concurrency caps (steer v1 complete) #1420.Tests
ObservatoryApp.test.tsx: 2 new cases (error shown when a steer post is rejected; cleared after a later success) plus the existing 11. All 13 green;tsc -bclean.Verification note
Behaviourally verified via vitest; visual check of the banner deferred to a live session, as with the other desktop slices.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests