fix(ui): persist AskUserQuestion wizard across session switches by garvit-arora · Pull Request #5854 · apache/maka

garvit-arora · 2026-09-29T18:04:07Z

Summary

Persist AskUserQuestion wizard progress (question index, per-question drafts, free-form text) keyed by requestId in user-question-prompt-state.ts
Restore that state when UserQuestionPrompt remounts after a session switch instead of restarting at question 1
Clear the remembered wizard state after a successful submit

Fixes

Fixes #5813

Verification

npm run build in packages/ui
npm run test:dist -- --test-name-pattern user-question in packages/ui
biome lint on changed files

AI use

This PR was prepared with AI assistance (Cursor)

Checklist

Tests added for the regression
Commit includes Generated-by: Cursor trailer

hqhq1025

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.

hqhq1025 · 2026-09-29T19:14:40Z

+  const drafts = createQuestionDrafts(questions);
+  drafts[0] = { kind: 'option', optionIndex: 1 };
+  drafts[1] = { kind: 'other', value: 'maybe' };
+  assert.deepEqual(buildUserQuestionResponse({ requestId: 'question-1', questions }, drafts), {


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.

hqhq1025 · 2026-09-29T19:14:40Z

    setResponseError(undefined);
    try {
      await props.onRespond(buildUserQuestionResponse(props.request, committed));
+      clearUserQuestionWizardState(requestId);


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.

hqhq1025 · 2026-09-29T19:14:40Z

+    activeRequestIdRef.current = requestId;
+    if (previousRequestIdRef.current === requestId) return;
+    previousRequestIdRef.current = requestId;
+    const next = createUserQuestionWizardState(props.request.questions);


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.

garvit-arora · 2026-09-29T19:25:22Z

Thanks for the review — addressed all three items in 753b64d34:

P1 — build:test fixture

Added the required toolUseId field to the buildUserQuestionResponse test request.

P2 — Clear on failed response

respondToInteraction now rethrows after showing the error toast, so UserQuestionPrompt only clears wizard state when the response actually succeeds.

P2 — Request switch without unmount

On requestId change, the prompt restores from readUserQuestionWizardState instead of resetting to question 1.
Persist writes skip the stale post-switch render; cleanup saves state on unmount.
Added key={requestId} on UserQuestionPrompt in main chat and quote-companion surfaces.
Added tests for A→B→A→B switching and failed-submit retry retention.

Verification

packages/ui user-question tests (3/3)
apps/desktop typecheck

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

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.

hqhq1025 · 2026-09-29T21:14:08Z

    setResponseError(undefined);
    try {
      await props.onRespond(buildUserQuestionResponse(props.request, committed));
+      clearUserQuestionWizardState(requestId);


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.

hqhq1025 · 2026-09-29T21:14:09Z

          { sessionId },
        );
      }
+      throw error;


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.

hqhq1025 · 2026-09-29T21:14:09Z

+    }
+    rememberUserQuestionWizardState(requestId, { questionIndex, drafts, answerText });
+    return () => {
+      rememberUserQuestionWizardState(requestId, { questionIndex, drafts, answerText });


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.

garvit-arora · 2026-09-29T21:21:38Z

Round 3 review feedback addressed on latest push (e7d91f2b):

P2 late rejection / session switch: respondToInteraction now rethrows Host failures even when the active session changed, so a deferred rejection cannot clear another session saved wizard progress.
P3 cleanup resurrection: successful submits call completeUserQuestionWizardState(); unmount cleanup ignores completed request IDs.
P3 rethrow contract: SandboxBoundaryPrompt and ClientCapabilityPrompt now catch rejected responses locally (toast still shown when the originating session is active).

Overlap with #5814: happy to defer to whichever approach maintainers prefer for #5813. Ready for another look.

hqhq1025

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-279 passes companion.respondToUserQuestion to this component, but use-quote-companion.ts:1694-1704 catches a rejected Host response, sets a banner, and resolves the promise. The submit path at user-question-prompt.tsx:151-153 therefore 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 rejecting onRespond directly and does not exercise the Side Chat adapter.
P3, every successful request leaves a permanent UUID in completedWizardRequestIds. Production calls completeUserQuestionWizardState but never calls clearUserQuestionWizardState; 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.

hqhq1025

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.

hqhq1025 · 2026-09-30T09:18:52Z

+  answerText: string;
+}
+
+const wizardStateByRequestId = new Map<string, UserQuestionWizardState>();


[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

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.

hqhq1025 · 2026-09-30T11:13:45Z

  }

+  function stop() {
+    completedRef.current = true;


[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

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.

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

Generated-by: Cursor

hqhq1025

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.

garvit-arora mentioned this pull request Sep 29, 2026

AskUserQuestion multi-question wizard resets to question 1 after switching sessions #5813

Open

github-actions Bot added the effort/M Under 500 readable lines label Sep 29, 2026

hqhq1025 reviewed Sep 29, 2026

View reviewed changes

github-actions Bot added effort/L Under 1000 readable lines and removed effort/M Under 500 readable lines labels Sep 30, 2026

hqhq1025 reviewed Sep 30, 2026

View reviewed changes

garvit-arora and others added 7 commits October 1, 2026 02:46

fix(ui): address round-4 AskUserQuestion wizard review feedback

d68d6b2

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

fix(ui): retire AskUserQuestion wizard state on stop and external settle

3f09ca1

- 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

fix(ui): keep AskUserQuestion wizard drafts until Stop settles

a1a2ed7

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

chore(desktop): regenerate renderer architecture ledger after rebase

2779d0a

Generated-by: Cursor

garvit-arora force-pushed the fix/5813-user-question-wizard-persist branch from 215ef95 to 2779d0a Compare September 30, 2026 21:18

hqhq1025 reviewed Sep 30, 2026

View reviewed changes

Conversation

garvit-arora commented Sep 29, 2026

Summary

Fixes

Verification

AI use

Checklist

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garvit-arora commented Sep 29, 2026

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garvit-arora commented Sep 29, 2026

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

hqhq1025 left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants