Repository navigation
fix(ui): persist AskUserQuestion wizard across session switches - #5854
garvit-arora wants to merge 10 commits into
Conversation
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head abb1624701531f2018ddb627ebec05dedc5fdbe2. The request-scoped store restores progress after an unmount/remount, but this revision is not merge-ready: one build blocker and two reachable state-loss paths (inline).
Node 24 clean npm ci passed. npm run build:test fails in the added UI test with TS2741; the emitted focused tests still pass 3/3, but do not exercise the failing paths. A mounted two-request probe showed returning to each request resets it to question 1. git diff --check and a static merge against current main b3933043 are clean. I did not run the full suite or native Electron. PR #5814 separately addresses the same issue; coordinate the two implementations rather than merging both independently.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| const drafts = createQuestionDrafts(questions); | ||
| drafts[0] = { kind: 'option', optionIndex: 1 }; | ||
| drafts[1] = { kind: 'other', value: 'maybe' }; | ||
| assert.deepEqual(buildUserQuestionResponse({ requestId: 'question-1', questions }, drafts), { |
There was a problem hiding this comment.
P1: This new fixture omits required UserQuestionRequest.toolUseId. On a clean Node 24 install, npm run build:test stops here with TS2741, so the UI build and downstream gates cannot pass. Include the required field in the test request.
| setResponseError(undefined); | ||
| try { | ||
| await props.onRespond(buildUserQuestionResponse(props.request, committed)); | ||
| clearUserQuestionWizardState(requestId); |
There was a problem hiding this comment.
P2: This clears the saved draft whenever onRespond resolves, but the production Desktop respondToInteraction adapter catches Host rejection and resolves normally (app-shell-chat-actions.ts:529-549). A failed answer therefore clears the cache while leaving the request active; switching sessions then remounts it at question 1 with the previous answers lost. Keep the state until the response actually succeeds, and cover the mounted failure/switch path.
| activeRequestIdRef.current = requestId; | ||
| if (previousRequestIdRef.current === requestId) return; | ||
| previousRequestIdRef.current = requestId; | ||
| const next = createUserQuestionWizardState(props.request.questions); |
There was a problem hiding this comment.
P2: When requestId changes without unmounting, the first effect writes the prior request's state under the new ID, then this effect unconditionally resets to question 1 instead of restoring the new request's saved state. ChatComposerRegion renders the prompt without a request key, so switching between two sessions with pending questions reuses the component. A mounted A→B→A→B probe returned to question 1 for both requests. Scope component state by request or load each request's remembered state on change, and test that switch sequence.
|
Thanks for the review — addressed all three items in P1 —
P2 — Clear on failed response
P2 — Request switch without unmount
Verification
Re #5814: happy to coordinate — let me know whether you’d prefer consolidating into that PR or keeping this one open. Ready for another look. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head 753b64d3489c8e154091c5d46d1c221421d3b925. The prior missing toolUseId build failure and normal A↔B request-switch regression are fixed. The new revision still has one reachable draft-loss race and two smaller regressions (inline), so I would not merge it yet.
Node 24 build:test and the five focused UI tests pass. A mounted probe showed that successful submission clears the map, but unmount immediately restores the completed draft; another probe showed that an adapter resolution after unmount removes a still-pending draft. The new head has no hosted check runs yet. The branch is one commit behind current main 1ffc6ca0; static merge-tree and diff-check are clean. I did not run native Electron or the full test suite. PR #5814 addresses the same issue, so coordinate which implementation should land.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| setResponseError(undefined); | ||
| try { | ||
| await props.onRespond(buildUserQuestionResponse(props.request, committed)); | ||
| clearUserQuestionWizardState(requestId); |
There was a problem hiding this comment.
P2: The Desktop adapter still returns normally from its catch when activeIdRef.current !== sessionId (app-shell-chat-actions.ts:538). If the user switches sessions while an answer is in flight and Host rejects it afterward, the prompt has already unmounted and saved its draft; this await then resolves and the clear deletes that still-pending request's only saved progress. A mounted deferred-response probe produced questionIndex=1 after switch and undefined after adapter resolution. Propagate failure independently of which session is now active, and cover the late rejection/switch sequence.
| @@ -546,6 +546,7 @@ export function createAppShellChatActions(deps: { | |||
| { sessionId }, | |||
| ); | |||
| } | |||
| throw error; | |||
There was a problem hiding this comment.
P3: Rethrowing here changes the shared respondToInteraction contract for sandbox-boundary and client-capability prompts too. Both call void respond(...) from their buttons, and their respond functions have try/finally but no catch (packages/ui/src/sandbox-boundary-prompt.tsx:53-67, client-capability-prompt.tsx:52-66). A rejected Host response now becomes an unhandled Promise rejection, even though this adapter already displays a toast. Add handling at those call sites or limit the new rejection contract to consumers that catch it.
| } | ||
| rememberUserQuestionWizardState(requestId, { questionIndex, drafts, answerText }); | ||
| return () => { | ||
| rememberUserQuestionWizardState(requestId, { questionIndex, drafts, answerText }); |
There was a problem hiding this comment.
P3: This cleanup unconditionally writes the last draft back to the module-wide map. submit() clears the entry after a successful response (:153), then normal removal of the completed prompt runs this cleanup and resurrects it. A mounted probe observed undefined immediately after success and the completed question-2 state again after unmount. Completed answer text is retained indefinitely in the unbounded map instead of being cleared; distinguish completed requests from pending cleanup and add a successful-submit/unmount regression.
|
Round 3 review feedback addressed on latest push ( e7d91f2b):
Overlap with #5814: happy to defer to whichever approach maintainers prefer for #5813. Ready for another look. |
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed exact head fe7d91f2bdb8a9394efa8a226c23a03a0ecc8bc9. The previous main-chat late-rejection, unhandled permission rejection, and post-success cleanup issues are addressed by rethrowing Host errors, catching permission response errors, and marking completed wizard requests. I found one remaining P2 and one P3:
- P2, Side Chat still loses wizard progress after a failed response followed by a session switch.
quote-companion-panel.tsx:274-279passescompanion.respondToUserQuestionto this component, butuse-quote-companion.ts:1694-1704catches a rejected Host response, sets a banner, and resolves the promise. The submit path atuser-question-prompt.tsx:151-153therefore marks that request completed and deletes its saved drafts even though the Host rejected it. The prompt may retain local state until it unmounts, but remounting starts at question one. The new late-rejection test injects a rejectingonResponddirectly and does not exercise the Side Chat adapter. - P3, every successful request leaves a permanent UUID in
completedWizardRequestIds. Production callscompleteUserQuestionWizardStatebut never callsclearUserQuestionWizardState; the Set has no lifecycle bound or eviction, so memory grows with every completed question in a long-running desktop process.
Node 24 build:test and the focused UI tests (7/7) pass. git diff --check and merge-tree against current main 6e21e611 pass. No checks are reported for this exact PR head. I did not run native Electron or an end-to-end Side Chat failure/switch sequence. This head is not ready to merge until the Side Chat failure path preserves drafts.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
github-actions
Bot
added
effort/L
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head. The Side Chat response path now propagates failures, so the prompt keeps progress when the Host rejects an answer. The prior unbounded completion tombstones are removed. One smaller lifetime issue remains: progress for stopped or externally settled requests is never deleted (inline finding).
Node 24 build:test and 7 focused UI tests pass; the branch merges cleanly with current main. No hosted checks are attached to this head. I did not run packaged Electron, native Windows/macOS, or a complete Side Chat end-to-end test.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| answerText: string; | ||
| } | ||
|
|
||
| const wizardStateByRequestId = new Map<string, UserQuestionWizardState>(); |
There was a problem hiding this comment.
[P3] Bound or retire entries for stopped/external resolutions. The completion Set is gone, but this requestId-keyed Map still retains every saved wizard that ends via Stop, abort, or an external/runtime settlement: clearUserQuestionWizardState() is only called after this component’s successful onRespond (user-question-prompt.tsx:157-158), never from onStop or queue retirement. Repeated interrupted questions therefore accumulate full drafts for the renderer lifetime. The new tests cover successful submit and rejection, not these terminal paths.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed the current head. The new bounded wizard cache and same-session settle cleanup address the previous accumulation issue, and the normal submit/switch paths are covered by focused tests. One stop-failure path still drops an unanswered draft (inline). Node 24 build:test and 9 focused UI tests pass; diff-check and a static merge against current main are clean. There are no hosted checks on this head. I did not run packaged Electron or a real Host failure journey. This is not ready to merge until the draft-loss path is fixed and current-head checks run.\n\n> Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| } | ||
|
|
||
| function stop() { | ||
| completedRef.current = true; |
There was a problem hiding this comment.
[P2] Keep the wizard draft until Stop is confirmed. This marks the request completed and deletes its saved state before onStop settles. Desktop Stop catches IPC failures and leaves the interaction active (app-shell-stop-action.ts:51-75); Side Chat also treats a failed stop as best-effort (use-quote-companion.ts:1460-1476). If the user has progressed through this wizard, clicks Stop, the Host refuses or times out, and then switches sessions, cleanup skips persistence because completedRef is true. Returning to the still-pending question restarts at question 1 with empty answers. Clear only after a confirmed stop/terminal settle, preserving state on failure.
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
The new commit no longer clears AskUserQuestion wizard drafts when Stop is clicked. The prompt keeps them if the Stop adapter rejects or the Host fails to stop; the composer-region cleanup still clears the remembered request when the Host removes that prompt in the active session. The previously reported immediate draft-loss path is addressed. I did not identify a separate new defect in this increment.
[P1] This head does not merge with current main@d7dffca9: git merge-tree reports a content conflict in apps/desktop/renderer-architecture.json. Main has since moved Conversation ownership and regenerated that ledger, while this branch updated the same entries. Rebase, regenerate the architecture ledger against the resulting source tree, and run the architecture check and affected UI tests on the merged result. A passing branch-only check does not validate that integration.
Node 24 build:test, 9 focused wizard tests, the renderer architecture check (121/121), and diff check pass on this branch. No hosted checks are attached to this head. I did not test a packaged Electron build, native Windows/macOS, or a real Host Stop failure followed by a session switch.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
215ef95 to
2779d0a
Compare
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 2779d0a2c2a14c3280e877d3dc532be3415c6707. The latest commit regenerates the renderer architecture ledger after rebasing; the preceding Stop change retains AskUserQuestion drafts until the Host actually drops the prompt (packages/ui/src/user-question-prompt.tsx:146-155; apps/desktop/src/renderer/chat-composer-region.tsx:197-204). The prior merge conflict is resolved: the tree merges cleanly with fetched main c838e1fa. In the inspected increment I found no new substantiated P0-P3 issue.
Node 24 full build:test, nine focused wizard tests, Desktop typecheck, renderer architecture check (121 fixtures and ledger), and git diff --check passed locally. No hosted checks are attached to this exact head, so current-head CI remains unestablished and this is not merge-ready. I did not run a packaged Electron session switch, a real Host Stop failure, native platforms, or the full local test suite.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Incremental re-review of 93a9560 (previous reviewed head 2779d0a, fast-forward with one commit: "satisfy renderer architecture strict-base debt gates").
What changed: the AskUserQuestion wizard-clear useEffect in chat-composer-region.tsx was folded into the existing useLayoutEffect through a single lifecycle ref. The clear still runs before the newTaskSendPending early return, so behaviour is preserved; it now runs at layout timing instead of passive timing, which is harmless. The interaction-response failure toast was extracted into notifyInteractionResponseFailure in session-workspace-errors.ts.
Renderer architecture check: it now passes. check:architecture --strict-base passes against merge-base c838e1f, and also on a synthetic merge with current main (1e80e3b) against main. The script's own tests pass (121/0). The branch merges cleanly into main.
Findings: this head does not compile, so I'm requesting changes. Hosted CI (run 36841691063, Build step) and a local tsc -p tsconfig.renderer.json report the same four errors:
app-shell-chat-actions.ts444-445: the send-failure path still referencesisSessionWorkspaceUnavailableErrorandshowSessionWorkspaceUnavailableToast, but this commit removed their import (TS2304).session-workspace-errors.ts79-80:responseFailedTitleandresponseFailedFallbackare read fromgetShellCopy(locale).errors, but those keys live under.chatActions(TS2339).
Ledger note for the fix: on main, app-shell-chat-actions.ts is at importSpecifiers 8 and nonTriviaTokens 2931. Re-importing both names next to the new helper would reach 9 specifiers and fail strict-base again. The send-failure branch probably needs to go through the same kind of helper.
Not exercised: full build:test, the desktop test suites and UI runtime. Workspace packages were not built locally, so typecheck errors were filtered to the touched files.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| isSessionWorkspaceUnavailableError, | ||
| showSessionWorkspaceUnavailableToast, | ||
| } from './session-workspace-errors.js'; | ||
| import { notifyInteractionResponseFailure } from './session-workspace-errors.js'; |
There was a problem hiding this comment.
P1: This import was narrowed to notifyInteractionResponseFailure, but the send-failure branch at lines 444-445 still calls isSessionWorkspaceUnavailableError(error) and showSessionWorkspaceUnavailableToast(...). The build fails with TS2304 (CI run 36841691063, Build). If it were emitted anyway, it would throw a ReferenceError inside the send catch. Keep in mind main's ledger caps this file at 8 import specifiers, so re-adding both names next to the helper (9) would fail strict-base. Routing the send-failure branch through a helper too would avoid that.
| }, | ||
| ): void { | ||
| if (!shouldNotify) return; | ||
| const copy = getShellCopy(input.locale).errors; |
There was a problem hiding this comment.
P1: responseFailedTitle and responseFailedFallback are not on getShellCopy(locale).errors; they live under .chatActions (see locales/shell-copy.ts ~179). This produces TS2339 at lines 79-80 and breaks the build. At runtime the toast would get an undefined title. Use getShellCopy(input.locale).chatActions for these two keys.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental re-review of exact head 2bc5d0f5. The previous reviewed head was 93a95604. This is a fast-forward with one commit, "repair interaction failure toast helpers after refactor", on the same merge-base c838e1fa.
Both P1 build errors from the last review are fixed:
app-shell-chat-actions.ts:447: the send-failure branch now calls a new helper,notifyWorkspaceUnavailableIfNeeded(session-workspace-errors.ts:56-72), so it no longer references the removed imports. The import count goes from 7 to 8, which is within main's ledger cap of 8 for this file.session-workspace-errors.ts:91:responseFailedTitleandresponseFailedFallbackare now read fromgetShellCopy(locale).chatActions, where they are defined in all three locales (locales/shell-copy.ts:179-180).
Earlier findings from the other review lineage:
- The P1 about the missing
toolUseIdfixture and the P2s about request switches, late rejection and Stop retention were all confirmed fixed in later reviews on this PR. The increment does not touch them. - The rethrow P3 (
:523) is handled: both the sandbox-boundary and client-capability prompts now catch the rejection (sandbox-boundary-prompt.tsx:58-61). - The wizard map is bounded at 32 entries (
user-question-prompt-state.ts:36,63-69).
No new P0-P2 in the increment. One P3 style note is inline.
Merge blocker: the PR is CONFLICTING with main (f7633c3d). Main's renderer reorganization moved both desktop files this PR edits: app-shell-chat-actions.ts is now features/conversation/controller/chat-actions.ts, and session-workspace-errors.ts is now application/contracts/session-workspace-errors.ts. A test merge conflicts in those two files and in renderer-architecture.json. Because of the conflict, GitHub has run no CI on this head, so the fix above has not been verified by hosted CI.
Overlap with #5814: #5814 (by another author, still open) adds the same packages/ui/src/user-question-prompt-state.ts and also edits user-question-prompt.tsx, to keep AskUserQuestion wizard progress across sessions. The two PRs use competing mechanisms. Whichever lands second will hit an add/add conflict and should be closed or rebased onto the other. Maintainers should pick one.
Not run locally: tests and typecheck (the shared checkout has no usable TypeScript install). This was a static read plus a git merge-tree against main.
| ); | ||
| } else if (isSessionWorkspaceUnavailableError(error)) { | ||
| showSessionWorkspaceUnavailableToast(toastApi, uiLocale, diagnosticTarget); | ||
| } else if (notifyWorkspaceUnavailableIfNeeded(error, toastApi, uiLocale, diagnosticTarget)) { |
There was a problem hiding this comment.
P3 (style): else if (notifyWorkspaceUnavailableIfNeeded(...)) {} hides the toast side effect inside a condition and leaves an empty block. It works, but on a quick read it looks like the workspace case is silently swallowed. Something clearer would be else if (!notifyWorkspaceUnavailableIfNeeded(...)) { toastApi.error(...) }, or a short comment in the empty block. Since main has moved this file to features/conversation/controller/chat-actions.ts, this can be folded into the rebase.
Keep multi-question wizard progress keyed by requestId so remounting after a session switch resumes the current question and prior answers. Fixes apache#5813 Generated-by: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
Restore each request's saved wizard on switch, keep drafts after failed responses, add toolUseId to the state test fixture, and key prompts by requestId in chat surfaces. Generated-by: Cursor Co-authored-by: Cursor <cursoragent@cursor.com>
Propagate interaction failures even after session switches so late Host rejections cannot clear saved wizard progress. Mark completed requests separately from test resets to prevent unmount cleanup from resurrecting submitted answers, and catch rejected responses in sandbox/capability prompts that use void respond(). Generated-by: Cursor
Rethrow Side Chat user-question Host failures so failed answers keep saved wizard progress. Use per-prompt completion ref instead of an unbounded global completed-request set. Generated-by: Cursor
- Clear wizard map entries on Stop and when the Host drops the prompt on the same session - Bound the requestId-keyed store to 32 entries with oldest-first eviction Generated-by: Cursor
Do not retire saved wizard state when Stop is clicked; wait for the Host to drop the prompt (or a rejecting adapter) so failed Stop attempts keep progress. Generated-by: Cursor
Fold wizard settle cleanup into the existing composer lifecycle layout effect and move interaction failure toasts into session-workspace-errors so legacy AppShell debt stays within main. Generated-by: Cursor
2bc5d0f to
8ddd410
Compare
|
Rebased the fix onto current main and force-pushed the fork branch (Apache main was not modified). The desktop refactor conflicts are resolved in the current feature paths. Focused wizard tests pass, including remount, A↔B switching, late rejection, failed response, and Stop retention. The broader UI build and architecture checks are currently blocked by unrelated baseline errors in this checkout; details are recorded in the PR. |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review: 2bc5d0f -> 8ddd410. The branch was rebased from c838e1f onto current main 74094c6. git range-diff shows commits 1, 4, 5 and 6 unchanged. Commits 2, 3 and 8 were reworked for main's move of app-shell-chat-actions.ts to features/conversation/controller/chat-actions.ts. The ledger-regen commit and the toast-helper repair commit were dropped. The PR is now 10 files, +643/-15, and no longer touches renderer-architecture.json or session-workspace-errors.ts.
Prior findings
- Earlier P1s (workspace-unavailable toast helper, copy source): no longer applicable. That code left the PR, and the catch block now uses main's own
showSessionWorkspaceUnavailableToastpath. - Earlier P3 (empty
else ifat app-shell-chat-actions.ts:447): gone along with that code.
New findings
- P2 - The rebase dropped the "propagate after session switch" fix. In
chat-actions.ts,respondToInteractionstill starts its catch block with main'sif (activeIdRef.current !== sessionId) return;. The newthrow error;(line 521) therefore runs only while the session is still active. Commit ed2cb4d says it should "propagate interaction failures even after session switches so late Host rejections cannot clear saved wizard progress", and before the rebase it did: the toast was guarded and the throw was unconditional.- Scenario: the user submits an AskUserQuestion answer in session A, then switches to B. The prompt unmounts, and its cleanup saves the drafts. The Host then rejects the answer.
respondToInteractionreturns normally, soUserQuestionPrompt.submitsetscompletedRefand callsclearUserQuestionWizardState(requestId). - Result: when the user goes back to A, the request is still pending but the wizard starts empty. This is the exact loss this PR is meant to fix.
- Fix: move the active-session check so it only guards the toast, and always rethrow. A desktop-side test for the switched-session rejection would help; the UI tests only cover
onRespondrejecting directly.
- Scenario: the user submits an AskUserQuestion answer in session A, then switches to B. The prompt unmounts, and its cleanup saves the drafts. The Host then rejects the answer.
- P2 - PR-caused CI failure. The
testjob fails inapps/desktop/src/main/__tests__/app-shell-form-interaction.test.tsat "keeps the prompt answerable when the platform rejects the answer" (Error: Host unavailable).- Cause: the new rethrow also applies to
respondToUserForm, so the test'sawait actions.respondToUserForm(...)now rejects. - Main CI is green at the same merge-base 74094c6, so this comes from the PR.
- Fix: change the test to
await assert.rejects(...)and keep the existing assertions.
- Cause: the new rethrow also applies to
- P3 - Form errors now show twice. At runtime,
FormInteractionPromptcatches the rejection. Because the error now propagates, the form shows the error inline as well as the shell toast it already showed. That may be intended, but it changes the form's behavior outside this PR's AskUserQuestion scope, so please confirm.
Other notes
- Overlap: #5814 (open, e892d84) still adds the same
packages/ui/src/user-question-prompt-state.tsand editsuser-question-prompt.tsx. These are competing implementations with an add/add conflict, so maintainers should pick one. - Protocol/epoch: no protocol files touched, no epoch impact.
- Mergeability and CI: GitHub reports MERGEABLE (merge state BLOCKED pending review/checks). CI
testis failing (see the second P2).
| { sessionId }, | ||
| ); | ||
| } | ||
| throw error; |
There was a problem hiding this comment.
P2: This rethrow is unreachable when the user has switched away. The catch block above still starts with main's if (activeIdRef.current !== sessionId) return;. In that case the call resolves, and UserQuestionPrompt.submit treats the late rejection as success: it sets completedRef and clears the saved wizard state. Before the rebase (ed2cb4d) only the toast was guarded and this throw was unconditional; please restore that. Separately, this rethrow also reaches respondToUserForm, which breaks app-shell-form-interaction.test.ts ('keeps the prompt answerable when the platform rejects the answer'); that test needs assert.rejects.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
Incremental review of 8ddd4101d35b0c5e89bce7e98262eefe68fb50b8 → a453d2b95f15fcdb84070b5b6ac446cdac1914a7: the seven existing commits are unchanged in git range-diff; both heads share merge-base 74094c6325868723c0b1446cd59c2ab7d209cba8. This adds one commit affecting the interaction adapter and its regression test, not another rebase.
Previous findings
- P2, late rejection after switching sessions: fixed. In
apps/desktop/src/renderer/features/conversation/controller/chat-actions.ts:511–525, the active-session check now only guards the toast. User-question responses explicitly propagate rejection at line 549. A probe connecting the compiled adapter to the actual React prompt submitted an answer, switched sessions/unmounted, rejected the Host promise, then remounted the original request: the answer survived, no interaction was settled, and no background-session toast appeared. - P2, PR-caused form-rejection test failure: fixed. Non-user-question responses retain the non-throwing behavior. The existing “keeps the prompt answerable when the platform rejects the answer” test and the added late-user-question test both pass locally and actually ran successfully in the current CI logs.
- P3, duplicate form error: fixed. A composed
FormInteractionPrompt/adapter probe confirmed a failed response leaves the prompt pending and produces one shell toast, without the additional inline form alert. User-question errors can still appear inline and in the toast while their session is active; that is unchanged from the previous head, not a new form regression.
I found no new P0–P2 issues in this increment.
Overlap: #5814 remains open at e892d84e078912bc6ce8e031321f3f9ec52a6bd5. Both PRs add packages/ui/src/__tests__/user-question-prompt.test.tsx; a trial merge reproduced an add/add conflict there and content conflicts in user-question-prompt-state.ts and user-question-prompt.tsx. The two branches are competing implementations and need reconciliation rather than being merged independently without checking their combined result.
Verification and compatibility: Nine dependency packages and the Desktop test build succeeded. Twenty targeted UI-state, prompt, Desktop adapter, and companion-interaction tests passed, with no skips; the composed probes also passed. Scoped Biome and diff checks passed. CI run 37972423343 is successful at the exact head, including renderer architecture, build, typecheck, affected tests, and Desktop end-to-end checks. Runtime Host and CLI packaging steps were skipped. There is no protocol, epoch, or schema change, and the branch merges without conflicts into the checked main. GitHub reports MERGEABLE/BLOCKED; that is not approval. I did not rerun the previous head or the full monorepo/Electron suite locally; the composed probes use a fixture Host promise and React/linkedom, not a live Host connection.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
I found one P2 at cc5612b4ad2777f74dd0e4d56beb4e4b580eebf5: five newly added wizard tests still use the pre-merge click behavior and fail. This is not ready to merge. The two previously reported P2 issues and the Form duplicate-error issue remain fixed; I did not find another production correctness regression in this update.
The eight existing authored commits are unchanged relative to a453d2b9. The new commit merges main 6b9457f5, including click-to-advance/submit behavior. I checked the manual composer conflict resolution: it retains the request-settlement cleanup and main's new-task draft handling, while removing the obsolete context-usage declaration.
The current CI test run 38162740074 fails the same five assert.ok(nextButton) checks I reproduced locally. Clicking an option now advances immediately; on the final question it submits immediately. Please update those tests' interaction steps, including their redundant final Submit clicks, while preserving the persistence and failed-response assertions.
I rebuilt the normal test targets with clean generated outputs. The complete UI suite finished with 738 passed and 5 failed out of 743. The original new wizard test file alone had 1 pass and 5 failures. A separate diagnostic changed only those obsolete interaction steps, left production code untouched, and passed all six original persistence scenarios; this does not make the submitted suite green.
The four Desktop typechecks, changed-file formatting checks, 167 architecture tests, strict renderer check against main, and nine focused adapter/state/click tests passed. A React fixture using the actual Desktop adapter confirmed that a rejection arriving after a session switch preserves the saved answer and does not display a background toast. The Form path still resolves without propagating the rejection, keeps the prompt pending, and displays only one error toast. These were controlled Host fixtures, not a live Host or native Electron test.
PR #5814 is still open at e892d84e and addresses the same persistence problem with a different state-lifetime strategy. A trial merge with this head conflicts in the prompt, state module, and added tests. The two changes need a deliberate choice or reconciliation, not independent merges.
This branch is mergeable against the current main and introduces no protocol/schema/epoch change. Its failing tests still need correction and a passing run on the resulting exact head.
|
|
||
| const nextButton = Array.from(container.querySelectorAll<HTMLButtonElement>('button')) | ||
| .find((button) => button.textContent === 'Next'); | ||
| assert.ok(nextButton); |
There was a problem hiding this comment.
[P2] Update these wizard tests for click-to-advance
After this merge, clicking Beta already advances to question 2, so there is no Next button and this assertion fails. The same obsolete step fails five new tests in CI run 38162740074 and in a clean local run. Final-question option clicks also submit automatically, so the later explicit Submit clicks are obsolete too. Update all five flows for the current click behavior while retaining their persistence/rejection assertions; changing only these interaction steps makes all six scenarios pass with production unchanged.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
The outstanding test-failure P2 is fixed at aeaef4046b71554a0589f06fc6131612e178ddea. The earlier two P2 issues and the Form duplicate-error issue also remain fixed. I found no new P0–P2 issue in this increment.
Compared with cc5612b4, this commit changes only packages/ui/src/__tests__/user-question-prompt.test.tsx (+4/-44). It removes the obsolete extra Next/Submit clicks after an option already advances or submits. The persistence, request-switch, successful-settlement and rejected-response assertions remain; the retry test additionally verifies that a successful retry clears the saved state. No production or dependency code changed.
After a fresh dependency install and normal clean test build, the complete UI suite passed 743/743, with no skips. All 15 focused wizard/state/click, Desktop adapter and companion-interaction tests passed. The four Desktop typechecks, scoped formatting and strict renderer check passed.
I also reran the actual React prompt connected to the Desktop adapter with a controlled Host promise. A rejection after switching sessions preserved the answer without settling the interaction or showing a background toast. The Form path still does not propagate its rejection and retains one toast without an additional inline alert. These confirm the earlier two P2 fixes and Form behavior, rather than treating the now-green wizard tests as proof of those separate adapter contracts.
CI run 38191771207 succeeded on this exact head. A trial merge with current main 105302c6 is conflict-free. There is no PR protocol/schema change; epoch 217 remains unchanged.
PR #5814 remains open at e892d84e. A fresh paired merge still conflicts in the prompt, state module and added test file. These are alternative implementations and need a choice or reconciliation before independent merges.
I did not run live Host/native Electron sessions or the complete repository test suite. The adapter probes use controlled Host fixtures; within that limit, the previously reported failures are closed.
This branch has not been deployed
3 participants
Summary
requestIdinuser-question-prompt-state.tsUserQuestionPromptremounts after a session switch instead of restarting at question 1Fixes
Fixes #5813
Verification
npm run buildinpackages/uinpm run test:dist -- --test-name-pattern user-questioninpackages/uibiome linton changed filesAI use
Checklist
Generated-by: Cursortrailer