-
Notifications
You must be signed in to change notification settings - Fork 32
🤖 fix: allow ask_user_question back navigation #1192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
+235
−116
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 16, 2025
Fix `ask_user_question` back-navigation for single-select answers. - Removed effect-based auto-advance that re-triggered when navigating back to an answered question. - Auto-advance now happens only on the user’s selection event (single-select, selecting, non-`Other`). - Added Storybook play test covering: select → advances; click back → stays; change answer → advances. Also includes small test-suite stabilizations uncovered while running `make test-unit`: - Gate `web_fetch` internet tests behind `TEST_INTEGRATION=1`. - Reduce timing-based flakiness in background process output tests. - Guard telemetry client for window-less test environments. - Skip URL-hash sync in Storybook preview (avoids test-runner navigation retries). --- <details> <summary>📋 Implementation Plan</summary> # Fix: `ask_user_question` can’t navigate back after answering (single‑select) ## Diagnosis (why it happens) In `src/browser/components/tools/AskUserQuestionToolCall.tsx`, there’s a `useEffect` that **auto-advances** whenever the *current* question is single-select and has exactly one non-`Other` selection. Because this effect runs any time `activeIndex` / `currentQuestion` changes, when you click a previous “section” (the header buttons like `Approach`, `Platforms`) **you briefly land on that question and then the effect immediately pushes you forward again**. Result: you can’t stay on a previously-answered single-select question to edit it. ## Recommended fix (keep auto-advance, but only on the user action) **Net LoC (product code): ~-10 to -25** (remove effect + ref; add a small in-handler advance). 1. **Remove the `useEffect`-based auto-advance** and the `hasUserInteracted` ref. 2. Move auto-advance into the option `toggle()` handler for **single-select** questions: - Only advance when the user is **selecting** (not de-selecting) an option. - Only advance for **non-`Other`** selections. - Advance to `activeIndex + 1` (which will naturally hit “Summary” after the last question). This preserves the “fast path” UX for single-select while allowing users to click back to previous sections and change answers. ### Suggested implementation sketch - In `toggle()`: - detect `const isSelecting = !checked;` - after calling `setDraftAnswers(...)`, if `!currentQuestion.multiSelect && isSelecting && opt.label !== OTHER_VALUE`, call `setActiveIndex((i) => i + 1)`. - Delete the auto-advance `useEffect` entirely. ## Tests (prevent regression) ### Storybook interaction test (preferred for UI) 1. Add a `play` function to the existing `AskUserQuestionPending` story (or create a new focused story, e.g. `AskUserQuestionBackNavigation`). 2. Steps in `play`: - Click an option in the first single-select question → verify the UI advances to the next question. - Click the previous section button (`Approach`) → verify the first question remains visible (i.e., it does **not** auto-jump forward again). - Optionally: change the selection and confirm it advances again. This will run in CI via the existing **“Test / Storybook”** GitHub Actions job. ### (Optional) unit test if we want belt + suspenders If you’d like a unit test that runs in `bun test src` too, extract the “should auto-advance” logic into a small pure helper (e.g. `shouldAutoAdvanceSingleSelect(...)`) and test: - navigating back to an already-answered question does **not** auto-advance - selecting a new non-`Other` option does (But the Storybook interaction test should be sufficient and closer to the real bug.) ## Validation checklist - Repro the original issue locally (answer single-select → attempt to click back). - After fix: you can click back to `Approach` and stay there. - Run: - `make test-unit` - `make storybook-build && make test-storybook` <details> <summary>Alternatives considered</summary> 1. **Keep the `useEffect` but add guards** (track “last auto-advanced question+answer” in a ref). - Works, but is more stateful and easier to re-break. 2. **Disable auto-advance entirely**. - Simplest, but changes UX (users must always click Next). The in-handler auto-advance is the smallest change that preserves existing intent. </details> </details> --- _Generated with `mux` • Model: `openai:gpt-5.2` • Thinking: `xhigh`_
Fix ask_user_question auto-advance so users can jump back to answered sections. Add a Storybook interaction test and stabilize a few flaky unit/tool tests uncovered while running the suite. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `mux` • Model: `openai:gpt-5.2` • Thinking: `xhigh`_ Change-Id: Ib9ded7ca2ae10e749d93c769ecea2524d19042d1
- Use crypto.timingSafeEqual for safeEq (with padding) and document tradeoffs. - Gate timing microbench tests behind MUX_TEST_TIMING=1 to avoid flaky parallel runs. - Extract isStorybookIframe() helper in App.tsx. - Consolidate telemetry window/test/E2E guards. - Drain output once after process exit to avoid returning exited + empty output. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `mux` • Model: `openai:gpt-5.2` • Thinking: `xhigh`_ Change-Id: I4e7f9e01b257a7c19292ca3d023891076ad859a8
Make the uv installer step fail loudly and retry transient download errors, so `make -j static-check` doesn’t fail later with a missing uvx. Signed-off-by: Thomas Kosiewski <tk@coder.com> --- _Generated with `mux` • Model: `openai:gpt-5.2` • Thinking: `xhigh`_ Change-Id: I274f231807d73258741bb029f0f5107d9a3580f1
7322cc3 to
7e577e6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix
ask_user_questionback-navigation for single-select answers.Other).Also includes small test-suite stabilizations uncovered while running
make test-unit:web_fetchinternet tests behindTEST_INTEGRATION=1.📋 Implementation Plan
Fix:
ask_user_questioncan’t navigate back after answering (single‑select)Diagnosis (why it happens)
In
src/browser/components/tools/AskUserQuestionToolCall.tsx, there’s auseEffectthat auto-advances whenever the current question is single-select and has exactly one non-Otherselection.Because this effect runs any time
activeIndex/currentQuestionchanges, when you click a previous “section” (the header buttons likeApproach,Platforms) you briefly land on that question and then the effect immediately pushes you forward again. Result: you can’t stay on a previously-answered single-select question to edit it.Recommended fix (keep auto-advance, but only on the user action)
Net LoC (product code): ~-10 to -25 (remove effect + ref; add a small in-handler advance).
useEffect-based auto-advance and thehasUserInteractedref.toggle()handler for single-select questions:Otherselections.activeIndex + 1(which will naturally hit “Summary” after the last question).This preserves the “fast path” UX for single-select while allowing users to click back to previous sections and change answers.
Suggested implementation sketch
toggle():const isSelecting = !checked;setDraftAnswers(...), if!currentQuestion.multiSelect && isSelecting && opt.label !== OTHER_VALUE, callsetActiveIndex((i) => i + 1).useEffectentirely.Tests (prevent regression)
Storybook interaction test (preferred for UI)
playfunction to the existingAskUserQuestionPendingstory (or create a new focused story, e.g.AskUserQuestionBackNavigation).play:Approach) → verify the first question remains visible (i.e., it does not auto-jump forward again).This will run in CI via the existing “Test / Storybook” GitHub Actions job.
(Optional) unit test if we want belt + suspenders
If you’d like a unit test that runs in
bun test srctoo, extract the “should auto-advance” logic into a small pure helper (e.g.shouldAutoAdvanceSingleSelect(...)) and test:Otheroption does(But the Storybook interaction test should be sufficient and closer to the real bug.)
Validation checklist
Approachand stay there.make test-unitmake storybook-build && make test-storybookAlternatives considered
useEffectbut add guards (track “last auto-advanced question+answer” in a ref).The in-handler auto-advance is the smallest change that preserves existing intent.
Generated with
mux• Model:openai:gpt-5.2• Thinking:xhigh