Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded roving-tabindex keyboard navigation to the sidebar: Sidebar now tracks focused nav item, manages ArrowUp/ArrowDown/Home/End traversal and DOM focus; NavItem accepts Changes
Sequence DiagramsequenceDiagram
participant User
participant NavItem as NavItem Button
participant Sidebar as Sidebar (Keyboard Handler)
participant Store as Dashboard Store
User->>NavItem: KeyDown (ArrowDown)
NavItem->>Sidebar: onKeyDown(ArrowDown)
activate Sidebar
Sidebar->>Sidebar: compute next index
Sidebar->>Store: set focusedView(next)
Sidebar->>NavItem: refOf(next).focus()
deactivate Sidebar
NavItem->>Sidebar: onFocus(next)
activate Sidebar
Sidebar->>Store: sync focusedView
Sidebar->>NavItem: set tabIndex=0 for focused, -1 for others
deactivate Sidebar
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
mpiton
changed the title
Greptile SummaryThis PR implements roving tabindex navigation for the sidebar by adding Confidence Score: 5/5Safe to merge — all previous P1 concerns have been resolved and no new issues were found. Both prior findings (role=toolbar and the redundant setFocusedView call) are addressed. The roving tabindex logic, keyboard handler, and useEffect guard are all implemented correctly. No P0 or P1 issues remain. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User presses key on NavItem] --> B{handleNavKeyDown}
B -->|ArrowDown| C["nextIndex = index + 1 mod count"]
B -->|ArrowUp| D["nextIndex = index - 1 + count mod count"]
B -->|Home| E["nextIndex = 0"]
B -->|End| F["nextIndex = count - 1"]
B -->|Other key| G[return — no preventDefault]
C & D & E & F --> H[event.preventDefault]
H --> I[focusNavItem]
I --> J["navItemRefs.current at index .focus()"]
J --> K["onFocus fires: setFocusedView item.view"]
K --> L["tabIndex: focusedView === item.view ? 0 : -1"]
M[currentView changes externally] --> N{nav group owns activeElement?}
N -->|Yes - user navigating| O[keep focusedView unchanged]
N -->|No - focus elsewhere| P["setFocusedView currentView"]
Reviews (3): Last reviewed commit: "fix: address PR review comments" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/Sidebar/Sidebar.tsx`:
- Around line 85-87: The current useEffect always calls
setFocusedView(currentView) which snaps the roving tabindex back even when a nav
button still has DOM focus; change the effect (useEffect) to only resync
focusedView to currentView when focus is outside the sidebar/nav group by
checking the container ref (e.g., navRef or sidebarRef) and using
navRef.current.contains(document.activeElement) (guarding for null/SSR) — call
setFocusedView(currentView) only if the container does NOT contain
document.activeElement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ad5fba44-68f2-4444-9cd3-f71bf50a473d
📒 Files selected for processing (1)
src/components/Sidebar/Sidebar.tsx
1 participant
Summary
Why
Arrow key navigation in the sidebar did not move focus between items. Only Tab worked, which broke expected keyboard navigation for this control and left issue #178 unresolved.
Root Cause
Sidebar navigation items were plain buttons without shared focus state, per-item
tabIndexmanagement, or arrow-key handling.Impact
Users can now move focus through the sidebar using arrow keys and jump to the first/last item with Home and End, matching the roving tabindex pattern expected for this kind of navigation.
Validation
npm test -- src/components/Sidebar/Sidebar.test.tsx src/components/Sidebar/NavItem.test.tsxnpm run buildnpm run lint -- src/components/Sidebar/Sidebar.tsx src/components/Sidebar/NavItem.tsx src/components/Sidebar/Sidebar.test.tsx src/components/Sidebar/NavItem.test.tsxFixes #178
Summary by cubic
Adds roving tabindex and ArrowUp/ArrowDown/Home/End navigation to the sidebar so users can move focus between items without changing the active view. Groups nav items as "Primary views" for better screen reader context. Fixes #178.
focusedViewand refs; preserves focus when the view changes and resets to the active view if focus leaves the group.preventDefaultto avoid triggering navigation.tabIndex,onFocus,onKeyDown, andbuttonRefonNavItem; tests cover prop pass-through, roving behavior, and focus movement.Written for commit c9c779a. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests