Skip to content

fix(planning): keep focus on a choice across a state push - #11932

Merged
kyle-sexton merged 2 commits into
mainfrom
fix/surface-journey-flake
Nov 1, 2026
Merged

kyle-sexton merged 2 commits into
mainfrom
fix/surface-journey-flake

Conversation

@kyle-sexton

Copy link
Copy Markdown
Member

No related issue: main-red flake in test-bash (4)

Summary

plugins/planning/surface/surface-journey.test.sh failed intermittently in test-bash (4) on one check, ui_journey.2: Answer again moves focus to the first choice:

The cause is a product bug, not a slow wait. Answer again focuses the first Decide radio synchronously. When a state push lands before the check reads focus, apply rebuilds #choices with innerHTML and calls restoreFocus(focusKey()). focusKey() builds its selector from the focused control's id or data-* attributes, and the radios have neither, so it returns null and focus drops to <body>. The check's 5 s until cannot recover focus that is already gone.

Fix

  • plugins/planning/surface/index.html focusKey(): a focused radio is keyed by its name and value (#choices input[name="choice"][value="N"]), so a state push puts focus back on the same choice.
  • plugins/planning/surface/tests/ui_journey.js: after Answer again, the journey posts a note, waits for the resulting state frame, and checks that focus is still on the same radio. Before the fix this check failed on every run (PASS=256 FAIL=1, focus on BODY), so the race is now covered deterministically rather than by luck of timing.
  • Changelog fragment in .changes/planning/.

Verification

  • Before the fix, with only the new check: in 1 local run the new check failed with focus on BODY. The original "Answer again" check passed in that run. The CI flake rate was not reproduced locally; what was reproduced, every time, is the mechanism.
  • After the fix: surface-journey.test.sh passed 10 of 10 runs locally, each PASS=257 FAIL=0. 6 runs were sequential and 4 ran two at a time to load the host the way the shard does; each server binds a free port. The first sequential run used settle(300) on the new check, the other runs settle(2000).
  • surface.test.sh, which holds ui_e.js's radio-focus checks, passed 1 run: PASS=164 FAIL=0 SKIP=1.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 2 commits November 1, 2026 08:52
focusKey() rebuilt the focused control's selector from its id or data-*
attributes. The Decide radios carry neither, so a state push that
rebuilt #choices dropped focus to the body. The journey's "Answer again
moves focus to the first choice" check failed whenever a push landed
between the click and the check (main run 41311427851, PR #11912 run
41290672576 attempt 2). Radios now key on name and value, and the
journey checks that a state push keeps focus on the same radio.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 1, 2026 13:56
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-01T13:58:00.851971Z f0ab46a Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

claude-security-review reviewed f0ab46ab45ec821ebb7c0569d18c54fe72e6af9f: read 3 of 3 changed file(s); 0 finding(s), 0 as inline comments.

Security review: PR #11932

I found no security issues.

I read every hunk of all 3 changed files. None of them match a trusted path, none is a plugin manifest, and none changes a trust control, a merge hold or a standing instruction.

  • plugins/planning/surface/index.html: focusKey() now also builds a focus selector from a focused radio's name and value attributes. Both values pass through CSS.escape and sit inside a quoted attribute selector, the same way the existing data-* attributes are handled. Nothing reaches HTML or script execution, so the change adds no selector or markup injection path. The selector is only used to put focus back on an element that already exists.
  • plugins/planning/surface/tests/ui_journey.js: this adds a test check after a state push. It is test-only code.
  • .changes/planning/fix-surface-journey-radio-focus-7d2e4b91.md: a changelog fragment, prose only.

The PR description contains no embedded instructions aimed at the review lane. There was no settled-threads file.

critical-findings: 0

@github-actions

github-actions Bot commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

claude-review reviewed f0ab46ab45ec821ebb7c0569d18c54fe72e6af9f: read 3 of 3 changed file(s); 0 finding(s), 0 as inline comments.

Code review: no findings

I read every hunk of all 3 changed files. None of them match a trusted-path glob.

plugins/planning/surface/index.html, focusKey()

  • What changed. A focused radio is now keyed by its name and value attributes, alongside any data-* attributes. Radios with an id still take the earlier #id branch.
  • Why it works. The Decide radios are rendered at index.html:1498 as <input type="radio" name="choice" value="N">, with no id and no data-*. Before this change focusKey() returned null for them, so the restoreFocus(f) call in apply at index.html:2427 couldn't put focus back after the #choices rebuild. The new selector #choices input[name="choice"][value="N"] matches the rebuilt radio. restoreFocus is unchanged and still does nothing when no element matches.
  • Escaping. CSS.escape is used on the value inside a quoted attribute selector, the same way the existing data-* keys already do it.
  • Other radios. A radio that has data-* attributes now gets extra name/value constraints. Those still match the same rebuilt element, so this widens nothing.

plugins/planning/surface/tests/ui_journey.js

  • The new check records the focused radio's value, posts a note so the server sends a state push, then waits with settle(2000) until that state frame is applied. It then asserts focus is still on the same #choices radio.
  • focused() is defined at ui_journey.js:16 and gives the diagnostic text when the check fails.
  • Because the check runs after a push it triggered itself, it tests the restore path directly instead of depending on a timing race.
  • Author-claimed, unverified: the PR says this check failed before the fix with focus on BODY, and that the suite passed 10 of 10 runs after it. This lane has no shell, so I ran none of it.

.changes/planning/fix-surface-journey-radio-focus-7d2e4b91.md

  • bump: patch fits a user-visible bug fix.
  • The ### Fixed layout matches existing planning fragments, for example work-resolve-issue-issue-11843-5b1e9c3d.md.

Other checks

  • Closing issue: none. The PR says it fixes a flake on main and has no linked issue, so there are no acceptance criteria to check.
  • Plugin manifests: none changed.
  • Embedded instructions: the PR context contains no text telling the lane to approve, skip or post anything.

@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 1, 2026
Merged via the queue into main with commit 39a1f14 Nov 1, 2026
72 checks passed
@kyle-sexton
kyle-sexton deleted the fix/surface-journey-flake branch November 1, 2026 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant