fix(app): handle question response races clearly#839
Conversation
There was a problem hiding this comment.
Suggested priority: P2 (includes user-path files (packages/app/src/pages/session/composer/session-question-dock.test.ts, packages/app/src/pages/session/composer/session-question-dock.tsx)).
P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR adds request-scoped response guards and error normalization to the question dock to prevent duplicate submissions and eliminate misleading "[object Object]" error messages. A new ChangesQuestion Dock Response Guard and Error Handling
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request improves error handling in the SessionQuestionDock component by introducing a normalization layer for API errors and a local submission guard to handle idempotent 'already_resolved' responses. It also includes comprehensive unit tests for these new utilities. Feedback was provided regarding the fallback error message in the fail handler, suggesting that using a generic 'invalid payload' message may be misleading and that specific error reasons from the API should be surfaced instead.
Perf delta summaryComparator: pass
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@packages/app/e2e/session/session-composer-dock.spec.ts`:
- Around line 793-798: The current check uses expect.poll(() =>
responseCalls).toBe(1) which returns as soon as responseCalls hits 1 and does
not guarantee no duplicates afterward; change the assertion to first wait for
the single submission (expect.poll(() => responseCalls).toBe(1)) and then
immediately poll for a short stability window to ensure no further increments
(e.g., expect.poll(() => responseCalls, { timeout: 1000 }).toSatisfy(v => v <=
1)); update the test around the forced clicks that use dock.evaluate, submit,
and firstOption so it first asserts responseCalls === 1 and then asserts
responseCalls stays <= 1 for the window to catch any duplicate submission from
raw DOM clicks.
In `@packages/app/src/pages/session/composer/session-question-dock.tsx`:
- Around line 383-386: The toast fallback currently uses
language.t("session.question.error.invalidPayload") when normalized.detail is
empty, which mislabels unknown errors; update the fallback in
session-question-dock.tsx where showToast(...) is called so it uses a generic
unknown error translation key (e.g.,
language.t("session.question.error.unknown") or
language.t("common.unknownError")) instead of the invalidPayload key, and ensure
the matching translation entry is added to the language catalogs.
🪄 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: 8bfc461c-8c28-4643-9473-ff7c095ea129
📒 Files selected for processing (3)
packages/app/e2e/session/session-composer-dock.spec.tspackages/app/src/pages/session/composer/session-question-dock.test.tspackages/app/src/pages/session/composer/session-question-dock.tsx
Summary
/session/:id/tool/respondattempts so users can correct and retry without losing drafts.[object Object], including plain bodyno_pending_tool_calland decoder-detail errors.already_resolvedas idempotent completion only when it matches a request that this dock already submitted, and keep the stale local dock reactively non-interactive after pending/confirmed completion until sync removes it.Why
Issue #837 reported that answering a question dock could show unreadable
[object Object]errors and close the dock before users could fix decoder/payload failures. The fix keeps retryable failures interactive while preserving idempotent completion for the same locally submitted request.Related Issue
Fixes #837
Human Review Status
Pending
Review Focus
already_resolvedonly completes after the same local question request was submitted.no_pending_tool_callmaps to stale-session copy, and structured decoder errors keep readable details without[object Object].Risk Notes
Skipped checklist items:
How To Verify
Screenshots or Recordings
Not provided. No screenshot/recording was captured for this copy/error-state change.
Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
Release Notes
Bug Fixes
Tests