Add keyboard navigation - #10
Conversation
|
@jadefw is attempting to deploy a commit to the Dara's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@codex tldr of the pr? |
TL;DRThis PR adds keyboard shortcuts to the gallery’s variant switcher:
It also avoids triggering shortcuts while the user is typing in an input, textarea, or editable element, and ignores shortcuts combined with Ctrl, Cmd, or Alt. The keyboard listener is removed when the component unmounts. src/components/gallery/variant-header.tsxL40-L52 src/components/gallery/variant-header.tsxL78-L80 Checks
No code changes were needed for this summary. |
|
@jadefw fix the merge conflicts n i'll get to merge |
|
@greptile-ai |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (e.key === "h") { | ||
| e.preventDefault(); | ||
| router.push("/"); | ||
| return; | ||
| } | ||
|
|
||
| if (e.key === "c") { |
There was a problem hiding this comment.
The handler only matches lowercase "h" and "c". On the rendered gallery route, lowercase shortcuts navigate correctly, but Shift+H and Shift+C leave the visitor on the current variant even when focus is outside an editable element. Normalize the letter key before comparing it so navigation works regardless of Shift or Caps Lock state.
Artifacts
Executed Playwright shortcut probe source
- The authored Chromium Playwright probe opens the live gallery variant, sends lowercase and Shift-uppercase H/C keys outside editable controls, and records the resulting URLs; the executable test source establishes the reproduction method and takeaway.
Observed gallery shortcut navigation output
- Output captured from `node trex-artifacts/variant-shortcuts-probe.mjs` in `/home/user/repo` records each key, non-editable BODY focus, expected URL, and actual URL; lowercase navigation works while both Shift-uppercase routes fail, the takeaway.
▶ Lowercase h shortcut navigating from the gallery variant to home
- Chromium recording of the real gallery variant before the Shift-uppercase comparison, showing lowercase `h` navigating to the home URL; the baseline shortcut works, the takeaway.
Poster frame after lowercase h navigation to home
- Poster frame from the lowercase `h` recording after the gallery route navigated to home; it visually supports the working lowercase baseline, the takeaway.
▶ Shift+H shortcut remaining on the gallery variant
- Chromium recording of `Shift+H` on the same live gallery variant, showing the route remains unchanged rather than navigating home; the Shift-uppercase shortcut failure is confirmed, the takeaway.
Poster frame after Shift+H leaves the gallery variant unchanged
- Poster frame from the Shift+H recording showing the same gallery variant still rendered after the key press; uppercase navigation did not occur, the takeaway.
|
@greptileai Resolved the merge conflicts against current master and addressed the Shift/Caps Lock shortcut finding in ec3b110. Preserved the current switcher, added guards for select controls, composing and already-handled events, and restricted number shortcuts to available iterations. Validation: all 9 Chromium shortcut regression tests passed; TypeScript passed. Focused ESLint passes with the existing set-state-in-effect rule excluded; both errors also reproduce on master. The production build was blocked by Google Fonts download failures in existing variants. Please review the current head. |
|
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 (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe gallery now supports global keyboard shortcuts for home navigation, comparison navigation, and iteration selection. Playwright coverage verifies shortcut behavior, hydration, command-palette typing, and suppression rules. ChangesGallery keyboard shortcuts
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant User
participant VariantSwitcher
participant buildCompareHrefForSelection
participant Router
User->>VariantSwitcher: Press eligible shortcut
VariantSwitcher->>buildCompareHrefForSelection: Build comparison URL for C
buildCompareHrefForSelection-->>VariantSwitcher: Return comparison URL
VariantSwitcher->>Router: Navigate to home, comparison, or iteration
Router-->>User: Display destination route
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Keyboard shortcuts and their suppression in editable and modified contexts are covered without an identified merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| return; | ||
| } | ||
|
|
||
| const key = e.key.toLowerCase(); |
There was a problem hiding this comment.
- Bug
-
Shift-modified gallery shortcuts fail to navigate
Shift+H and Shift+C must navigate from a hydrated gallery page just as their lowercase counterparts do. This remains actionable because the attempted rendered validation could not complete the hydration checkpoint needed to exercise the reported failure path; no executed check contradicts the prior report. The current implementation is at the key-normalization branch, but source inspection alone is not runtime disproof.
-
- Cause
- The browser validation timed out while waiting for the hydrated model-picker textbox before it could send the Shift-modified key events.
- Fix
- Restore a rendered regression test that reaches hydration and verifies
Shift+Hnavigates home andShift+Copens the expected comparison route, alongside lowercase and uppercase key values.
- Restore a rendered regression test that reaches hydration and verifies
There was a problem hiding this comment.
The requested rendered regression coverage is already in tests/gallery-shortcuts.spec.ts on ec3b110. Its beforeEach opens the model picker and waits for its textbox to be focused before sending shortcuts. All 9 Chromium tests passed, including h, Shift+H, H, c, Shift+C and C. I also verified Shift+H and Shift+C on the deployed Vercel preview. A timeout in the review environment does not reproduce a shortcut failure. The final Greptile summary for this same commit reports 5/5, no blocking failure, and successful rendered Shift+C navigation. No additional code change is needed for this finding.
There was a problem hiding this comment.
Thanks for the additional rendered evidence. With all 9 Chromium tests passing—including h, Shift+H, H, c, Shift+C, and C—plus successful verification on the Vercel preview, the timeout was environment-specific and does not indicate a shortcut regression. I agree that no code change or additional test coverage is needed for this finding.
Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.
| return; | ||
| } | ||
|
|
||
| if (key === "c") { |
There was a problem hiding this comment.
- Bug
- On the hydrated current page, Shift+C leaves
/with-design-skill/composer-1.5/1open rather than navigating to the expected comparison URL.
- On the hydrated current page, Shift+C leaves
- Cause
- The rendered shortcut handling path does not complete comparison navigation for the Shift+C keypress.
- Fix
- Ensure the Shift+C browser key event reaches the
ccomparison-routing branch and navigates to the href produced bybuildCompareHrefForSelection.
- Ensure the Shift+C browser key event reaches the
There was a problem hiding this comment.
This finding is contradicted by the completed Greptile summary for ec3b110, which explicitly confirms that Shift+C produced uppercase C and navigated to comparison. Our hydrated Chromium regression test also passed, and I independently verified the deployed preview navigates to /compare?leftGroup=with-design-skill&leftModel=composer-1.5&leftIteration=1&rightGroup=without-design-skill&rightModel=composer-1.5&rightIteration=1 after Shift+C. The handler already normalizes e.key with toLowerCase(). No further fix is needed.
There was a problem hiding this comment.
You're right — this finding is incorrect. e.key.toLowerCase() normalizes Shift+C to "c", so it reaches the comparison-routing branch. The regression test and deployed-preview verification confirm the shortcut navigates correctly. No code change is needed; please disregard this comment.
2 participants
This PR adds keyboard navigation to the VariantSwitcher to make switching between views and variants way smoother:
Shortcuts are disabled when typing in input fields or using modifier keys (Ctrl/Cmd) to prevent accidental triggers.
Summary by CodeRabbit
New Features
Tests