Skip to content

fix(console): keep the versions list one tab stop - #3023

Merged
freeqaz merged 6 commits into
mainfrom
fix-1020c-versions
Oct 20, 2026
Merged

freeqaz merged 6 commits into
mainfrom
fix-1020c-versions

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 20, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Finding 2115 (bughunt on #3006): the Versions list was not quite one Tab stop.

  • "Show older versions" sat inside the role="toolbar" list after the roving versions and kept tabIndex 0. A paged list had two Tab stops, and Up/Down/Home/End never reached the button.
  • A poll that landed the latest version's outcome rebuilt the list (renderVersions). A reader arrowing through the versions lost focus to <body>.

Change

apps/controller/src/console/agents/detail.mjs, console.css:

  • Show older versions now follows the toolbar instead of sitting in it. It loads versions; it is not a version. So the arrows stay on versions, as they do on the tab strip (design language: one Tab stop, arrow keys move within the set), and Tab reaches the button next. Putting it into the arrow order would have made the toolbar's "Versions" name and Home/End cover an action. The button also sits outside the scrolling list (max-height), so it stays visible on a long page of versions. CSS keeps the same spacing: the list's 8 px gap above the button and the list's 22 px bottom margin below it. The before and after screenshots match.
  • Show older versions keeps focus while it loads. "Loading older versions…" is now aria-disabled instead of disabled, so focus stays on the pressed button, and the click handler already ignores a press while loading. When the last page loads and the button goes away, focus moves to the first version that just loaded.
  • A rebuild keeps focus. Each control in the list has a key: a revision ID, the new-version button, Show older versions, or the version chooser. If focus was on one of them when renderVersions replaces the list, focus goes back to the matching new control. A version gets the roving Tab stop, so the next Tab goes back to the same version.
  • Follow-ups from internal review:
    • A rebuild keeps the list's scroll position, and the restored version is scrolled into view, as the arrow keys do.
    • When the focused control is gone, focus still has somewhere to go:
      • from Show older versions, it goes to the last version (for example, an empty last page)
      • from a version no longer listed, it goes to the list's Tab stop
    • After older versions load, focus goes to the first version below those shown before. A newer version that a poll added during the load does not take it.
    • While loading, the button looks disabled (opacity 0.6), and aria-busy gives it the busy cursor. Buttons with aria-disabled="true" no longer get the hover highlight. The other one in the console is Refresh deployment while it waits; it only loses the hover.
    • Show older versions' bottom margin is now var(--space-6) (24 px) instead of 22 px. That is 2 px, not visible.

Verification

Tests are in tests/browser/console-agent-overview.test.mjs. Both are existing tests, extended:

  • "Versions list marks the latest version's outcome and is one Tab stop": the page clock is paused, so polls run only when the test advances it. The record goes queued, then running, then failed while the reader has arrowed to v2. The test asserts:
    • the Queued badge (Queued/info) and its accessible name; this kills a surviving mutant that dropped Queued
    • focus is still on v2 after the poll that rebuilds the list
    • v2 holds the single Tab stop
  • "A failed version raises no recovery while the current version is on no loaded page" (paged history):
    • Show older versions is not inside the Versions toolbar
    • Tab from the version lands on it
    • Enter loads the older page, and focus lands on the newly loaded v1

Results:

  • Assertions added after internal review:

    • The rail test grabs v2 before the poll and asserts it is disconnected afterwards, so the focus check proves a rebuild happened. With the list's height shortened through the stylesheet so it scrolls, it also asserts that the scroll position carries over and that the focused v2 lies inside the list's visible box.
    • The paging test holds the older page unanswered. It asserts focus stays on "Loading older versions…" with aria-disabled="true", and that a second Enter sends no second request (counted after the page's fetches settle).
    • Mutants of those tests, all killed:
      • scroll restore removed
      • scroll restore and scroll-into-view removed
      • load guard re-entrant
      • disabled while loading
      • focus restore removed
      • restore without the roving Tab stop
      • no first-loaded focus
      • Show older back in the toolbar
  • ClawSweeper follow-ups (verdict on 015e66eb9):

    • [P2] The paging test shortens the list so v1 overflows it, then asserts the newly focused v1 lies inside the list's visible box after the final-page load. The rail test already asserts the same for a poll rebuild. The mutant that drops scrollIntoView fails it.
    • [P3] Storybook:
      • Pages / Agent detail / Long version history now loads each older page in 1.5 s. Its steps walk the keyboard through focus on the busy Show older versions, a repeated press, and the final-page handoff to v2.
      • New version in progress, then current adds the poll focus check: arrow to v6, and the poll that makes v7 current keeps focus on v6.
      • focus-and-disabled-workflow.md step 3 says the same.
      • A headless check of the built fixture frames passed at 1440 and 390 for both stories, with no page errors. It asserted: focus held on the loading button, then handed to v2 inside the overflowing list; the list rebuilt on the poll with focus on v6.
  • Final-page boundary (freeqaz-openai's report and ClawSweeper's verdict on 35bcf3a41): the version you are viewing, when it is on no loaded page, shows on its own below the list. It was counted as the end of the earlier pages, so after Show older versions loaded the page holding it, focus went back to it. The boundary now comes from the revisions the list actually showed. The new test "Show older versions hands focus to the first version it lists" opens v1 with only v3 listed and asserts that visible focus goes to v2 in an overflowing list. Its focused version is null on main, View version v1 on 35bcf3a41, and View version v2 with the fix.

  • Fails on main, passes with the fix: with origin/main's detail.mjs and console.css checked out under the new tests, both tests fail (actual: 1, expected: 0: the button is inside the toolbar; actual: null, expected: 'v2': focus dropped after the poll). Both pass with the fix.

  • Pin: the two tests passed 20/20 at 2 CPUs (taskset -c 6-7), 2 tests per run, on the first head with 1-min load under 40. On 0b7cf124f they passed 20/20 with load at 43 to 53. On 35bcf3a41, which merges main again (fix(console): finish an agent share before leaving its page #3020, fix(console): stop a member's create from repeating refused reads #3021, Fix deletion recovery after an Agent status read #3022), they passed 20/20 at 2 CPUs. On the current head e598a2813, the three Versions tests passed 20/20 at 2 CPUs, 3 tests per run. Both of those pins checked load under 40 before every run.

  • Full touched files green: console-agent-overview, console-focus-ring and console-button-hierarchy: 63/63 before the latest main merge, 65/65 on 35bcf3a41, 66/66 on e598a2813.

  • Mutation: 17 mutants of the Versions list code run against those three files; 16 are killed. Since bughunt-135 (10/12), these are newly killed:

    • queued_unmarked
    • focus restore removed
    • restore without the roving Tab stop
    • Show older back inside the toolbar
    • Show older disabled while loading
    • no focus on the first loaded version

    disabled_in_arrows survives. It is near-equivalent: versions are disabled only while a draft edit blocks navigation, and then all of them are.

  • prettier check (verified it can fail) and eslint --max-warnings 0 pass on the touched files.

Merge-tree, comparing conflicted files and conflict hunks against open PRs that touch these files (main vs this head):

PR main this PR
#3017 (ux-p8) clean clean
#2982, #2980, #2971 clean clean
#2596 1 hunk 1 hunk
#2524 11 11
#2518 15 15
#2445 13 13
#1619 20 20
#906 46 46

This PR adds no new conflict hunk.

Screenshots: Versions list with older pages (paged), Tab from the newest version, before and after.

Before After
1440 versions-before-1440 versions-after-1440
390 versions-before-390 versions-after-390

The layout is unchanged; the button is now outside role="toolbar", after the scrolling list.

Risks

Low. This is client-only console focus, scroll and DOM order. No API, permission or copy changes. The one global CSS change: a button with aria-disabled="true" gets no hover highlight. Today that affects only Refresh deployment while it waits, and the Agents back button after it is pressed. Focus is restored only when it was inside the Versions list and the rebuild dropped it, so focus elsewhere on the page is never taken.

Show older versions moves out of the Versions toolbar so the arrows stay on
versions and Tab reaches it next; it keeps focus while loading and hands it
to the first older version. A rail rebuild (a poll landing an outcome) keeps
focus on the same control. Tests assert both and a Queued latest outcome
(finding 2115, #3006).
@freeqaz
freeqaz requested a review from a team as a code owner October 20, 2026 08:22
@clawsweeper

clawsweeper Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 20, 2026
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 20, 2026
@clawsweeper

clawsweeper Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

This PR separates pagination from the Versions toolbar and preserves visible keyboard focus during loading and list rebuilds.

Example: Open v1 while the first history page lists only v3, then load the final page containing v2 and v1.

  • Before: Focus drops to the page on main; the earlier PR head incorrectly focused v1.
  • After: Focus moves to the newly listed v2 and scrolls it into view.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch addresses the known defects with meaningful browser coverage, while static screenshots provide only limited evidence of the changed interactions.
Proof confidence 🦪 silver shellfish (2/6) Real behavior proof is not required for maintainer- or bot-authored pull requests.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Bug fix · Worth it: Yes · Fix scope: Complete
User problem: Keyboard readers encounter an extra toolbar stop and lose their place when deployment polling rebuilds version controls.
Reason: The maintainer-authored repair restores the existing keyboard navigation contract without adding configuration or changing API behavior.

Merge readiness

✅ Ready for maintainer review

Keep this PR open: the focused accessibility repair is useful, and the previously reported findings are addressed without a new concrete correctness defect.

Priority: P2
Reviewed head: e598a2813f33c725bf4f63e6269dea08f9669ebd

Before merge

None.

Findings

None.

Tests

  • Missing end-to-end proof: Provide the required final-source Storybook walkthrough showing busy-button focus, final-page visible focus handoff, and focus retained across a poll rebuild.
Agent review details

How this fits together

The console Agent detail page combines revision summaries and deployment polling into version controls; keyboard input selects versions or requests older pages.

flowchart LR
  A[Revision API] --> C[Agent detail page]
  B[Deployment polling] --> C
  C --> D[Versions toolbar]
  C --> E[Older versions button]
  D --> F[Keyboard focus]
  E --> F
Loading

Technical review

Best possible solution:

Land the focused repair after the required visual walkthrough demonstrates the final keyboard behavior.

Do we have a high-confidence way to reproduce the issue?

Main places an independently tabbable pagination button inside the roving toolbar and replaces focused controls without restoration; source inspection and the supplied browser comparisons establish both paths, without a reviewer-run reproduction.

Is this the best way to solve the issue?

The repair uses the existing renderer and roving-focus owner, preserves the intended keyboard contract, and addresses the earlier scrolling, Storybook, and page-boundary findings.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 084d0f73f19f.

Provenance checked

  • Versions keyboard navigation keeps the original intent (52d3033: Make the Versions collection one Tab stop with arrow navigation while pagination keeps its separate stop.)
  • Revision pagination keeps the original intent (2a55afc: Load revision summaries newest first in pages, retaining loaded versions and allowing an empty final page.)
  • Button hover styling keeps the original intent (90c63bf: Provide consistent button hierarchy and distinguish pending controls; suppressing hover on aria-disabled buttons maintains that intent.)

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • Free Wortley: Raw commit 52d3033 adds apps/controller/src/console/agents/detail.mjs:505 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 52d3033d0d0f; files: apps/controller/src/console/agents/detail.mjs)
  • stevenlee-oai: Raw commit 7c3ecbf adds apps/controller/src/console/agents/detail.mjs:726 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: medium; commits: 7c3ecbfce4fa; files: apps/controller/src/console/agents/detail.mjs)

Labels

Label changes:

  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: Keyboard focus loss affects version-history navigation but leaves other console workflows available.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Attach the required final-source keyboard walkthrough and screenshots demonstrating the changed focus transitions.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

History

Review history (2 earlier review cycles)
  • reviewed 2026-10-20T08:38:17.528Z sha 015e66e :: needs changes before merge. :: [P2] [P2] Reveal the version receiving restored focus | [P3] [P3] Extend the existing Storybook keyboard scenarios
  • reviewed 2026-10-20T09:41:30.268Z sha 35bcf3a :: needs changes before merge. :: [P2] Calculate the page boundary from listed revisions only

Reviewed October 20, 2026, 5:51 AM ET / 09:51 UTC (Revision 3).

Review follow-ups for the Versions list (finding 2115):
- a rebuild keeps the list's scroll position and scrolls the restored
  version into view
- focus falls back to the last version (older pages) or the list's Tab stop
  (a version no longer listed) instead of dropping to the page
- after older versions load, focus goes to the first one below those shown
  before, not a newer one a poll added meanwhile
- the loading button looks disabled and busy; aria-disabled buttons get no
  hover highlight
- Show older versions' bottom margin uses the spacing scale
Tests assert the rebuild (old node disconnected), the kept scroll position,
and a held older page: focus stays on the busy button and a second press
sends no second request.
…ybook

Long version history now loads each older page in 1.5 s and walks the
keyboard through it: focus stays on the busy Show older versions, then moves
to the first loaded version, in view. New version in progress, then current
checks focus stays on v6 across the poll's rebuild. The focus workflow says
the same. The paging test asserts the newly focused v1 is visible inside an
overflowing list (finding 2115).
@freeqaz-openai

Copy link
Copy Markdown
Collaborator

One keyboard edge case to fix before merge, reproduced on 0b7cf124: open v1 directly while the first history page contains only v3. Tab to Show older versions and press Enter; the last page adds v2 and v1. Focus lands on the v1 you were already viewing, not the newly revealed v2.

The separately rendered current version is being counted as the end of the previous page. I tested a small fix that records only the versions actually listed before the fetch, uses that boundary to place focus, and keeps one helper for the roving tab stop. The same browser assertion got no version focus on the base, v1 on this head, and the expected v2 after the fix. All three focused controller-backed browser cases passed locally; I did not rerun hosted CI.

The earlier scrolling fix is worth keeping: on this head I also verified that a newly focused version actually scrolls into view in a short list viewport.

@freeqaz

freeqaz commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 20, 2026
…load

A viewed version on no loaded page shows on its own below the list, and it
counted as the end of the earlier pages, so loading the page that held it
focused it again instead of the first newly listed version. The boundary now
comes from the revisions the list actually showed. New test opens v1 with
only v3 listed and asserts Show older versions hands visible focus to v2
(finding 2115).
@freeqaz

freeqaz commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator Author

Free's clanker here. Thanks for the reproduction. Fixed in e598a28: the end of the earlier pages now comes from the revisions the list actually showed. The viewed version that shows on its own below the list no longer counts, so focus goes to the newly listed v2.

The new browser case, "Show older versions hands focus to the first version it lists", opens v1 while only v3 is listed, then presses Enter on Show older versions. It asserts that focus lands on v2 and that v2 is visible in an overflowing list. Results for the focused version: null on main, v1 on 35bcf3a, v2 with the fix. The three Versions cases passed 20/20 at 2 CPUs, and the three touched files pass 66/66. The scrolling fix stays.

@freeqaz

freeqaz commented Oct 20, 2026

Copy link
Copy Markdown
Collaborator Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Oct 20, 2026
@clawsweeper clawsweeper Bot removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 20, 2026
@freeqaz-openai

Copy link
Copy Markdown
Collaborator

Verified the fix in e598a281; my earlier keyboard-focus finding is resolved. Your new browser test fails on the prior code at exactly expected v2, focused v1, then passes on this head with v2 both focused and visible. All three focused controller-backed cases passed locally.

I also tried three pages: focus stays on Show older versions while another page remains, and Tab can leave it. No further change requested for this finding.

@freeqaz
freeqaz merged commit a77594a into main Oct 20, 2026
68 of 69 checks passed
@freeqaz
freeqaz deleted the fix-1020c-versions branch October 20, 2026 10:04
freeqaz added a commit that referenced this pull request Oct 20, 2026
Brings in the 9 commits landed on main since the last sync (#3023..#3042).
One textual conflict in docs/reference/controller.md: the worker.error entry
keeps both main's claim-lost causes (#3034) and this branch's repository
credential codes, followed by main's worker.claim-renewal-warning entry.
Main added no migrations, so 0060-0068 keep their numbers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants