feat(pickers): cycle assignee, label, and reviewer rows with Tab - #386
Conversation
- list-keyboard-nav: add opt-in `tabAdvances`. Tab/Shift+Tab move the active row and wrap at both ends, while the arrows still clamp. Modified Tab and an empty list stay native, and Shift+Tab never extends a range. - list-keyboard-nav: add `useRovingRows`, which extracts the roving-tabindex state and row props that five pickers each duplicated. - Assignees, Labels, Reviewers popovers: adopt the hook with Tab cycling, since nothing else in those popups can take focus. Each popover shows "Tab cycles the rows; Esc closes." so the keyboard exit stays visible. - Projects popover, project multi-select fields: adopt the hook without Tab cycling. Tab still has to reach Reconnect/Retry and the neighbouring field editors. - Projects popover, project fields editor, cleanup-branches dialog: add py-2 to the scroll containers. The Checkbox touch target's vertical bleed no longer forces a Windows scrollbar on a single row. - check-banned-patterns: allowlist list-keyboard-nav.ts, whose ctrl/meta read rejects every modified Tab and does not derive a platform modifier. - changelog: extend the projects-picker parity fragment with Tab cycling.
Deploying gitdesktop with
|
| Latest commit: |
83a6680
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://02b37dc8.gitdesktop.pages.dev |
| Branch Preview URL: | https://feat-picker-tab-row-cycling.gitdesktop.pages.dev |
|
Context for reviewers — deliberate calls and their evidence, from the session that built this. Items are numbered; each is a single claim.
Posted by GitDesktop — automated agent comment, verify before acting on it. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
🔇 Additional comments (3)
📝 WalkthroughWalkthroughThe shared keyboard navigation utility now supports optional Tab cycling and roving row focus. Conversation, issue, and pull request pickers use the shared hook. Tests cover Tab behavior, and supporting changes contain checkbox touch-target overflow. ChangesRoving row navigation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Picker
participant Checkbox
participant useRovingRows
participant listKeyboardNav
Picker->>Checkbox: render row properties
Checkbox->>useRovingRows: send arrow or Tab keydown
useRovingRows->>listKeyboardNav: calculate next row
listKeyboardNav-->>useRovingRows: return wrapped or clamped row
useRovingRows-->>Checkbox: update active row and tab stop
Merge Risk: ⚪ Minimal · up to The PR adds wrapped Tab navigation and shared picker row focus behavior while preserving native navigation where needed; current evidence supports merging with no remaining merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 9 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🤖 GitDesktop AI review · The refactor is sound. The five copied roving-tab-stop blocks now come from one Recorded decisions I'm not re-flagging (all noted in the description):
Accessibility
Tests
Layout
Copy
Posted by GitDesktop — AI output, verify before acting on it. |
|
🤖 GitDesktop AI security audit · I found no security issues in this PR. It changes client-side keyboard focus handling and CSS classes only, and none of the changes pass untrusted input into anything that could run it. Posted by GitDesktop — AI output, verify before acting on it. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The keyboard navigation behavior and shared hook refactor are consistent with the PR description, and the scrollbar fix is scoped and well-justified without introducing observable regressions in the reviewed call sites.
Review effort: Lite
Findings: None
What changed in this PR
Adds Tab/Shift+Tab cycling for the assignees, labels, and reviewers picker rows (wrapping at both ends) to enable one-handed Tab+Space multi-select, and consolidates the previously duplicated roving-tabindex logic into a shared hook in src/lib/list-keyboard-nav.ts. Also addresses a Windows-only scrollbar artifact caused by the Checkbox touch target bleeding outside scroll containers.
Changes:
- Extend
listKeyboardNavwith an opt-intabAdvancesmode for Tab/Shift+Tab row navigation (wrapping) while preserving modified-Tab native behavior. - Introduce
useRovingRowshook and migrate relevant pickers to it (Tab-cycling enabled where Tab has no other job). - Fix stray scrollbars by adding
py-2padding to affected scroll containers; update banned-patterns allowlist and changelog entry accordingly.
| File | Description |
|---|---|
| src/lib/list-keyboard-nav.ts | Adds tabAdvances to listKeyboardNav and introduces useRovingRows to share roving-tabindex + key handling. |
| src/features/pulls/ReviewersPopover.tsx | Switches to useRovingRows with Tab cycling and adds an on-screen keyboard hint. |
| src/features/issues/IssueMetaPickers.tsx | Updates AssigneesPopover to use useRovingRows with Tab cycling and adds the keyboard hint. |
| src/features/conversations/LabelsPopover.tsx | Updates to useRovingRows with Tab cycling and appends the keyboard hint to the footer. |
| src/features/conversations/ProjectsPopover.tsx | Uses useRovingRows (without Tab cycling) and adds py-2 to contain Checkbox bleed; adjusts ProjectRow to accept rowKey. |
| src/features/conversations/ProjectFieldsEditor.tsx | Uses useRovingRows for multi-select rows (without Tab cycling) and adds py-2 to contain Checkbox bleed. |
| src/features/repository/CleanupBranchesDialog.tsx | Adds py-2 to the scroll container to prevent overflow-induced scrollbars. |
| scripts/check-banned-patterns.mjs | Allowlists src/lib/list-keyboard-nav.ts for the ctrl/meta Tab-modifier check with rationale. |
| changelog.d/changed-projects-picker-parity.md | Documents Tab cycling in the assignees/labels/reviewers pickers. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Round 1 dispositions — all four AI-review findings accepted; batch applied, lands in the next push.
Security audit: clean bill, acknowledged — nothing to triage. Round-close gates on the batch: tsc clean, biome CI-form clean (one formatting red on the new test file caught and fixed by the gate itself), changelog check 44 valid, guards + self-tests 447/447, second-provider shadow dry (two consecutive clean passes on the applied batch). Note for the push: Posted by GitDesktop — automated agent comment, verify before acting on it. |
- Assignees, labels, reviewers: the "Tab cycles the rows; Esc closes."
footer now renders as `Popover.Description`, which wires it as the
popup's `aria-describedby`. Tab is trapped in both directions while
rows exist, so screen readers need to hear the Esc exit, not only see it.
- Projects picker: the scroll rail renders only when there are rows. Its
`py-2` padding (which contains the checkbox touch-target bleed) no
longer paints a blank band under the loading and empty notices.
- `scripts/list-keyboard-nav.test.mjs`: new tests for the `tabAdvances`
branch:
- Tab and Shift+Tab wrap at both ends.
- With no active row, Tab enters at the first row and Shift+Tab at
the last.
- Shift+Tab reports `shift: false`, since Shift only sets direction.
- Ctrl/Alt/Meta+Tab and an empty list stay native.
- The arrow keys still clamp at the ends.
The module is imported dynamically. The suite skips only on
`ERR_MODULE_NOT_FOUND`, because the `guards` job runs without
installing react.
- `list-keyboard-nav.ts`: the `tabAdvances` doc now names the
`Popover.Description` contract.
- Changelog fragment reworded into two sentences.
|
🤖 GitDesktop AI review · The earlier findings are fixed, apart from one gap in the new test. The Tab-branch suite this round adds never runs in CI, and it can skip silently anywhere. Nothing blocks the merge. Recorded decisions I'm still not re-flagging: Tab wraps while the arrows clamp; Projects and project-field rows keep native Tab; CleanupBranchesDialog stays inline; and README, site and in-app guide stay unchanged. Tests
Resolved since last review
Verdict: no blocking issues — remaining items are non-blocking; merge when ready Posted by GitDesktop — AI output, verify before acting on it. |
|
Round 2 disposition — the one finding accepted as prescribed, fixed; lands in the next push. Test CI-enforcement + skip bound — fixed. The suite's dynamic import now resolves a hoisted Both guard arms were negative-controlled from scratch copies before accepting: module-moved with react present → the runner fails (1 fail, exit 1; the pre-hardening guard would have skipped it silently), and module-present with a dependency unresolvable → clean skip. Gates on the batch: node --test 8/8, checks 447/447 self-tests, tsc clean, biome CI-form clean (it caught one import-order defect on the test copy; biome's own sort applied), second-provider shadow dry again. The two workflow edits are a pure 6-line step addition and a comment-only sync — careful-diffed against the adjacent working step, since no local YAML runner exists; the added step's first real execution will be this push's CI. Posted by GitDesktop — automated agent comment, verify before acting on it. |
The keyboard-nav suite skips when react can't be resolved, and the installless guards job never resolves it. Until now nothing ever ran the suite for real. - frontend.yml: run scripts/list-keyboard-nav.test.mjs in the installed job, the suite's only enforced run - list-keyboard-nav.test.mjs: skip only when ERR_MODULE_NOT_FOUND occurs and the module under test still exists on disk. A moved or renamed module now fails loudly instead of skipping forever as "react missing". - quality.yml: name the keyboard-nav suite alongside the marked render oracle in the guard self-tests comment
|
🤖 GitDesktop AI review · The previous round's test fix is in place: the Tab-branch suite now has an installed CI run, and a moved module no longer turns into a silent skip. The fix is correct. Two small comment nits are left, and nothing blocks the merge. Readability
Resolved since last review
Verdict: no blocking issues — remaining items are non-blocking; merge when ready Posted by GitDesktop — AI output, verify before acting on it. |
|
Round 3 dispositions — both nits fixed; this is the closing batch and it rides the final push.
Gates on the batch: suite 8/8 (count read from the runner output), biome CI-form clean on the test file, NUL/newline sweep clean; the workflow delta is comment-only. Second-provider shadow deliberately skipped for this batch — comment-only deltas keep the recorded waiver. With this push the board is: three consecutive merge-when-ready verdicts, Copilot approval with no findings, CodeRabbit no actionable comments, security audit clean, and CI green with the new suite step's first enforced run landing on this head. Recommending merge once that run is green. Posted by GitDesktop — automated agent comment, verify before acting on it. |
The frontend workflow's `scripts/**` path-filter comment named only the reference-detector step. It now also names the keyboard-nav suite step, which reads the same directory. The list-keyboard-nav test header is cut down to its constraints: - The suite is stdlib-only, so the installless `guards` job can run it. - The import is dynamic because the module under test value-imports react. - The skip fires only when the specifier is unresolved AND the module is still on disk, so a moved module fails loudly. - The frontend job's installed step is the enforced run. Comment-only change with no behavior change.
|
🤖 GitDesktop AI review · This round only changes comments. Both nits from the last review are fixed, and neither fix introduces new problems. Nothing blocks the merge. Resolved since last review
I also checked the hook's call sites against its "call it before any early return" rule. Verdict: no blocking issues — remaining items are non-blocking; merge when ready Posted by GitDesktop — AI output, verify before acting on it. |
|
🤖 GitDesktop AI review · This round rebases the PR onto master and moves the keyboard-nav suite into master's combined "Node suites needing node_modules (enforced)" step. The picker and hook code hasn't changed and is still sound. One problem came in with the rebase: the new suite doesn't honor that step's CI enforcement
Verdict: no blocking issues — remaining items are non-blocking; merge when ready Posted by GitDesktop — AI output, verify before acting on it. |
|
Round 5 dispositions — both accepted, fixed; ready for the push.
Gates: biome CI-form clean on the test file, NUL/newline sweep clean, scratch controls deleted. Two-file batch ( Posted by GitDesktop — automated agent comment, verify before acting on it. |
The keyboard-nav suite imports its module dynamically and skips when react is unresolved, so the installless guards job can still run it. In the installed frontend run, that same skip would let a missing dependency pass silently. With GD_EXPECT_DEPS set, any import failure now throws, so the enforced run cannot fail open. The quality.yml comment no longer keeps its own list of optional imports. It points to frontend.yml's Node-suites step as the roster instead, so the two lists cannot drift apart.
|
🤖 GitDesktop AI review · This round addresses both findings from the last review. It changes only the keyboard-nav test's import guard and header, and one Resolved since last review
Neither fix caused collateral damage:
There's nothing further to raise. Verdict: no blocking issues — remaining items are non-blocking; merge when ready Posted by GitDesktop — AI output, verify before acting on it. |
In the assignees, labels, and reviewers pickers, Tab and Shift+Tab now cycle through the rows, wrapping at both ends, so you can multi-select the whole list one-handed with Tab and Space. Nothing else in those popups is focusable, so Tab has no other job there, and Esc still closes them. The roving-tabindex wiring that five pickers each copied is now one shared hook. The change also removes a stray scrollbar caused by the Checkbox touch target.
Keyboard navigation helper
tabAdvancesoption onlistKeyboardNavinsrc/lib/list-keyboard-nav.ts:preventDefault, so Tab behaves natively.onActivatereceivesshift: falsefor it, so it never extends a range.useRovingRowshook in the same file:activeIdstate and keeps a single tab stop on the active row, falling back to the first row.onRowKeyDown,rowProps(item)(data-row,tabIndex,onFocus) andisActive(item).activeId,navIndexByIdandfocusIndexcode.scripts/check-banned-patterns.mjsallowlistssrc/lib/list-keyboard-nav.tsfor the hand-rolled ctrl/meta check. The rationale:tabAdvancesexcludes every modified Tab, so it doesn't read a platform modifier.Pickers
AssigneesPopover(src/features/issues/IssueMetaPickers.tsx),LabelsPopover(src/features/conversations/LabelsPopover.tsx) andReviewersPopover(src/features/pulls/ReviewersPopover.tsx) useuseRovingRowswithtabAdvances: true.LabelsPopoverappends the hint to its existing "Changes apply when this closes." footer.ProjectsPopover(src/features/conversations/ProjectsPopover.tsx) usesuseRovingRowswithouttabAdvances, because Tab still has to reach its Reconnect and Retry buttons.ProjectRowgains arowKeyprop so itsdata-rowmatches the key the hook queries by.MultiSelectRowsinsrc/features/conversations/ProjectFieldsEditor.tsxalso skipstabAdvances, because its rows sit among the popup's other field editors.Scrollbar fix
py-2on three scroll containers:ProjectsPopover.tsx,ProjectFieldsEditor.tsxandsrc/features/repository/CleanupBranchesDialog.tsx.after:-inset-y-2), which created scrollable overflow.Documentation
changelog.d/changed-projects-picker-parity.mdgains a sentence about Tab cycling in the assignees, labels, and reviewers pickers.README.md,site/, orsrc/features/help/content.ts.Summary by CodeRabbit
New Features
Bug Fixes