Added a Keyboard Shortcut Modal component#166
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
👋 Thanks for opening a PR, @aparna24bce11388!Your PR has entered the 🚦 PR Review Pipeline.
What happens next
A pipeline status comment will appear below and update automatically as your PR progresses. While you wait
This comment is posted only once. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a client-side ShortcutModal and a Shortcuts trigger in the Header (with a mobile Sheet menu); makes server.AUTH_SECRET required in env schema; adds pnpm workspace allowBuilds entries; and pins ChangesShortcuts Modal Integration
Sequence Diagram(s)sequenceDiagram
participant Header
participant ShortcutModal
participant Sheet
Header->>ShortcutModal: open / close (isOpen,onClose)
Header->>Sheet: open mobile menu (menuOpen)
Sheet->>Header: render auth-dependent links
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hi @piyushdotcomm, I have implemented the keyboard shortcut modal feature with navbar integration. Kindly review and let me know if any changes are required. Thank you! |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/home/header.tsx`:
- Around line 64-73: The code currently nests <Link> around a <Button> which
creates nested interactive elements; change to render the Button as the parent
and place the Link inside using the Button's asChild prop so the anchor becomes
the button's rendered element. Locate the sign-in and sign-up button blocks (the
<Link href="/auth/sign-in"> / <Link href="/auth/sign-up"> usages and the mobile
variants) and replace the pattern Link->Button with Button asChild containing
the Link, preserving the Button props (variant, size, className) and moving any
classes/text into the inner Link so semantics and keyboard/screen-reader
behavior are correct. Ensure the same change is applied to all three occurrences
mentioned.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 64be634d-bbc9-48bf-aeac-e922fe26d8c8
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
components/ShortcutModal.tsxmodules/home/header.tsxpackage.jsonpnpm-workspace.yaml
| <Link href="/auth/sign-in"> | ||
| <Button | ||
| variant="ghost" | ||
| size="sm" | ||
| className="hidden sm:flex text-muted-foreground hover:text-foreground hover:bg-red-500/5" | ||
| > | ||
| Sign In | ||
| </Button> | ||
| </Link> | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Button supports `asChild` and find nested Link/Button patterns.
fd -i 'button.tsx' | xargs -r sed -n '1,220p'
rg -nP --type=tsx '<Link[^>]*>\s*<Button' -C2Repository: piyushdotcomm/Editron
Length of output: 7670
🏁 Script executed:
cat -n modules/home/header.tsx | sed -n '60,95p'Repository: piyushdotcomm/Editron
Length of output: 2303
Avoid nested interactive elements (<Link> wrapping <Button>).
Nested anchors and buttons break HTML semantics and can cause keyboard navigation and screen-reader issues. Refactor using Button asChild with Link inside instead.
Suggested fix
-<Link href="/auth/sign-in">
- <Button
- variant="ghost"
- size="sm"
- className="hidden sm:flex text-muted-foreground hover:text-foreground hover:bg-red-500/5"
- >
- Sign In
- </Button>
-</Link>
+<Button
+ asChild
+ variant="ghost"
+ size="sm"
+ className="hidden sm:flex text-muted-foreground hover:text-foreground hover:bg-red-500/5"
+>
+ <Link href="/auth/sign-in">Sign In</Link>
+</Button>-<Link href="/dashboard">
- <Button
- size="sm"
- className="bg-red-600 hover:bg-red-700 text-white shadow-md shadow-red-500/20 text-xs font-semibold px-4 rounded-full"
- >
- Get Started
- </Button>
-</Link>
+<Button
+ asChild
+ size="sm"
+ className="bg-red-600 hover:bg-red-700 text-white shadow-md shadow-red-500/20 text-xs font-semibold px-4 rounded-full"
+>
+ <Link href="/dashboard">Get Started</Link>
+</Button>-<Link href="/dashboard">
- <Button
- size="sm"
- variant="outline"
- className="hidden sm:flex text-xs font-semibold px-3 h-8 border-red-500/20 text-red-600 hover:bg-red-500/5 mr-1"
- >
- Dashboard
- </Button>
-</Link>
+<Button
+ asChild
+ size="sm"
+ variant="outline"
+ className="hidden sm:flex text-xs font-semibold px-3 h-8 border-red-500/20 text-red-600 hover:bg-red-500/5 mr-1"
+>
+ <Link href="/dashboard">Dashboard</Link>
+</Button>Applies to lines 64-72, 74-81, 85-93.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Link href="/auth/sign-in"> | |
| <Button | |
| variant="ghost" | |
| size="sm" | |
| className="hidden sm:flex text-muted-foreground hover:text-foreground hover:bg-red-500/5" | |
| > | |
| Sign In | |
| </Button> | |
| </Link> | |
| <Button | |
| asChild | |
| variant="ghost" | |
| size="sm" | |
| className="hidden sm:flex text-muted-foreground hover:text-foreground hover:bg-red-500/5" | |
| > | |
| <Link href="/auth/sign-in">Sign In</Link> | |
| </Button> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/home/header.tsx` around lines 64 - 73, The code currently nests
<Link> around a <Button> which creates nested interactive elements; change to
render the Button as the parent and place the Link inside using the Button's
asChild prop so the anchor becomes the button's rendered element. Locate the
sign-in and sign-up button blocks (the <Link href="/auth/sign-in"> / <Link
href="/auth/sign-up"> usages and the mobile variants) and replace the pattern
Link->Button with Button asChild containing the Link, preserving the Button
props (variant, size, className) and moving any classes/text into the inner Link
so semantics and keyboard/screen-reader behavior are correct. Ensure the same
change is applied to all three occurrences mentioned.
piyushdotcomm
left a comment
piyushdotcomm
left a comment
There was a problem hiding this comment.
Revert all pnpm and package.json changes. We use npm. Also, fix the invalid HTML nesting.
| </Button> | ||
| </Link> | ||
| <UserButton /> | ||
| </div> |
There was a problem hiding this comment.
Invalid HTML. You can't nest a <Button> inside a <Link>. Use <Button asChild> and put the link inside it.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/ShortcutModal.tsx`:
- Around line 71-76: The inline modal style object (modal) uses a fixed width
"400px" which can overflow on small screens; update the modal style in
ShortcutModal.tsx (the modal constant) to use viewport-aware sizing such as
width: "90vw" with a maxWidth: "400px", add maxHeight (e.g., "90vh") and
overflowY: "auto" (and boxSizing: "border-box" if needed) so the dialog
constrains to the viewport and becomes scrollable on small devices.
- Around line 32-57: ShortcutModal currently lacks required accessibility: add
dialog semantics, Esc-to-close, and initial focus management. Update the
ShortcutModal component to render the modal container with role="dialog",
aria-modal="true", and an accessible label (aria-labelledby pointing to the
<h2>), add a ref for the Close button and on mount (when isOpen true) call
focus() on that ref, and set up a keydown listener (useEffect) that closes the
modal when Escape is pressed by calling onClose; also trap focus inside the
modal while open (simple tab-wrap using a keydown handler or integrate a
focus-trap util) and on unmount/close return focus to the previously focused
element. Ensure the Close button has a descriptive label and the modal container
is keyboard focusable (tabIndex={-1}) to support focus management.
In `@modules/home/header.tsx`:
- Around line 89-119: The mobile Sheet (components Sheet, SheetContent) remains
open after tapping navigation links because link clicks don't update menuOpen;
update every Link rendered inside the Sheet (including "/docs", "/#features",
"/templates", "/auth/sign-in", "/dashboard") to close the sheet by calling
setMenuOpen(false) on click (e.g., add an onClick handler or pass a prop that
triggers setMenuOpen(false)); ensure both the signed-in and signed-out link
branches call this so the sheet always closes for in-page/hash navigation and
persistent layouts.
- Around line 91-93: The icon-only Button rendering the Menu icon lacks an
accessible label; update the Button (the element rendering <Button size="icon">
with the <Menu /> child) to provide a programmatic name—e.g., add an aria-label
(aria-label="Open menu" or similar) or include visually hidden text inside the
button—so screen readers can identify the control; ensure the label describes
the action (open/close/menu) and matches any toggle behavior if applicable.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fb911f8-0053-4b03-bd12-81fb258db04d
📒 Files selected for processing (3)
components/ShortcutModal.tsxenv.tsmodules/home/header.tsx
| export default function ShortcutModal({ isOpen, onClose }: Props) { | ||
| if (!isOpen) return null; | ||
|
|
||
| return ( | ||
| <div style={overlay}> | ||
| <div style={modal}> | ||
| <h2>Keyboard Shortcuts</h2> | ||
|
|
||
| {shortcuts.map((section) => ( | ||
| <div key={section.category}> | ||
| <h3>{section.category}</h3> | ||
|
|
||
| {section.items.map((item, i) => ( | ||
| <div key={i} style={row}> | ||
| <span>{item.action}</span> | ||
| <kbd>{item.key}</kbd> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| ))} | ||
|
|
||
| <button onClick={onClose}>Close</button> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Modal misses required accessibility behavior (Esc + dialog semantics).
This implementation does not currently implement Esc-to-close, dialog semantics, or initial focus management, so it falls short of the stated accessibility behavior for this feature.
Suggested fix
+"use client";
+import { useEffect, useRef } from "react";
type Props = {
isOpen: boolean;
onClose: () => void;
};
@@
export default function ShortcutModal({ isOpen, onClose }: Props) {
+ const closeButtonRef = useRef<HTMLButtonElement>(null);
+
+ useEffect(() => {
+ if (!isOpen) return;
+
+ const onKeyDown = (event: KeyboardEvent) => {
+ if (event.key === "Escape") onClose();
+ };
+
+ document.addEventListener("keydown", onKeyDown);
+ closeButtonRef.current?.focus();
+ return () => document.removeEventListener("keydown", onKeyDown);
+ }, [isOpen, onClose]);
+
if (!isOpen) return null;
return (
- <div style={overlay}>
+ <div role="dialog" aria-modal="true" aria-labelledby="keyboard-shortcuts-title" style={overlay}>
<div style={modal}>
- <h2>Keyboard Shortcuts</h2>
+ <h2 id="keyboard-shortcuts-title">Keyboard Shortcuts</h2>
@@
- <button onClick={onClose}>Close</button>
+ <button ref={closeButtonRef} onClick={onClose}>Close</button>
</div>
</div>
);
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ShortcutModal.tsx` around lines 32 - 57, ShortcutModal currently
lacks required accessibility: add dialog semantics, Esc-to-close, and initial
focus management. Update the ShortcutModal component to render the modal
container with role="dialog", aria-modal="true", and an accessible label
(aria-labelledby pointing to the <h2>), add a ref for the Close button and on
mount (when isOpen true) call focus() on that ref, and set up a keydown listener
(useEffect) that closes the modal when Escape is pressed by calling onClose;
also trap focus inside the modal while open (simple tab-wrap using a keydown
handler or integrate a focus-trap util) and on unmount/close return focus to the
previously focused element. Ensure the Close button has a descriptive label and
the modal container is keyboard focusable (tabIndex={-1}) to support focus
management.
| const modal = { | ||
| background: "white", | ||
| padding: "20px", | ||
| borderRadius: "10px", | ||
| width: "400px", | ||
| }; |
There was a problem hiding this comment.
Modal sizing is not mobile-safe.
A fixed width: "400px" can overflow on small devices. Add viewport-aware width/height constraints and scrolling.
Suggested fix
const modal = {
background: "white",
padding: "20px",
borderRadius: "10px",
- width: "400px",
+ width: "min(400px, 92vw)",
+ maxHeight: "85vh",
+ overflowY: "auto" as const,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const modal = { | |
| background: "white", | |
| padding: "20px", | |
| borderRadius: "10px", | |
| width: "400px", | |
| }; | |
| const modal = { | |
| background: "white", | |
| padding: "20px", | |
| borderRadius: "10px", | |
| width: "min(400px, 92vw)", | |
| maxHeight: "85vh", | |
| overflowY: "auto" as const, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ShortcutModal.tsx` around lines 71 - 76, The inline modal style
object (modal) uses a fixed width "400px" which can overflow on small screens;
update the modal style in ShortcutModal.tsx (the modal constant) to use
viewport-aware sizing such as width: "90vw" with a maxWidth: "400px", add
maxHeight (e.g., "90vh") and overflowY: "auto" (and boxSizing: "border-box" if
needed) so the dialog constrains to the viewport and becomes scrollable on small
devices.
| <Sheet open={menuOpen} onOpenChange={setMenuOpen}> | ||
| <SheetTrigger asChild> | ||
| <Button size="icon"> | ||
| <Menu /> | ||
| </Button> | ||
| </SheetTrigger> | ||
|
|
||
| <SheetContent side="right"> | ||
| <div className="flex flex-col gap-4 mt-4"> | ||
|
|
||
| <Link href="/docs">Docs</Link> | ||
| <Link href="/#features">Features</Link> | ||
| <Link href="/templates">Templates</Link> | ||
|
|
||
| {!user ? ( | ||
| <> | ||
| <Link href="/auth/sign-in"> | ||
| Sign In | ||
| </Link> | ||
| <Link href="/dashboard"> | ||
| Get Started | ||
| </Link> | ||
| </> | ||
| ) : ( | ||
| <Link href="/dashboard"> | ||
| Dashboard | ||
| </Link> | ||
| )} | ||
| </div> | ||
| </SheetContent> | ||
| </Sheet> |
There was a problem hiding this comment.
Mobile sheet state is not closed when a navigation link is selected.
For in-page/hash navigation and persistent layouts, the sheet can remain open after a link click. Close it on link selection.
Suggested fix
-<Link href="/docs">Docs</Link>
-<Link href="/#features">Features</Link>
-<Link href="/templates">Templates</Link>
+<Link href="/docs" onClick={() => setMenuOpen(false)}>Docs</Link>
+<Link href="/#features" onClick={() => setMenuOpen(false)}>Features</Link>
+<Link href="/templates" onClick={() => setMenuOpen(false)}>Templates</Link>
@@
-<Link href="/auth/sign-in">
+<Link href="/auth/sign-in" onClick={() => setMenuOpen(false)}>
Sign In
</Link>
-<Link href="/dashboard">
+<Link href="/dashboard" onClick={() => setMenuOpen(false)}>
Get Started
</Link>
@@
-<Link href="/dashboard">
+<Link href="/dashboard" onClick={() => setMenuOpen(false)}>
Dashboard
</Link>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Sheet open={menuOpen} onOpenChange={setMenuOpen}> | |
| <SheetTrigger asChild> | |
| <Button size="icon"> | |
| <Menu /> | |
| </Button> | |
| </SheetTrigger> | |
| <SheetContent side="right"> | |
| <div className="flex flex-col gap-4 mt-4"> | |
| <Link href="/docs">Docs</Link> | |
| <Link href="/#features">Features</Link> | |
| <Link href="/templates">Templates</Link> | |
| {!user ? ( | |
| <> | |
| <Link href="/auth/sign-in"> | |
| Sign In | |
| </Link> | |
| <Link href="/dashboard"> | |
| Get Started | |
| </Link> | |
| </> | |
| ) : ( | |
| <Link href="/dashboard"> | |
| Dashboard | |
| </Link> | |
| )} | |
| </div> | |
| </SheetContent> | |
| </Sheet> | |
| <Sheet open={menuOpen} onOpenChange={setMenuOpen}> | |
| <SheetTrigger asChild> | |
| <Button size="icon"> | |
| <Menu /> | |
| </Button> | |
| </SheetTrigger> | |
| <SheetContent side="right"> | |
| <div className="flex flex-col gap-4 mt-4"> | |
| <Link href="/docs" onClick={() => setMenuOpen(false)}>Docs</Link> | |
| <Link href="/#features" onClick={() => setMenuOpen(false)}>Features</Link> | |
| <Link href="/templates" onClick={() => setMenuOpen(false)}>Templates</Link> | |
| {!user ? ( | |
| <> | |
| <Link href="/auth/sign-in" onClick={() => setMenuOpen(false)}> | |
| Sign In | |
| </Link> | |
| <Link href="/dashboard" onClick={() => setMenuOpen(false)}> | |
| Get Started | |
| </Link> | |
| </> | |
| ) : ( | |
| <Link href="/dashboard" onClick={() => setMenuOpen(false)}> | |
| Dashboard | |
| </Link> | |
| )} | |
| </div> | |
| </SheetContent> | |
| </Sheet> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/home/header.tsx` around lines 89 - 119, The mobile Sheet (components
Sheet, SheetContent) remains open after tapping navigation links because link
clicks don't update menuOpen; update every Link rendered inside the Sheet
(including "/docs", "/#features", "/templates", "/auth/sign-in", "/dashboard")
to close the sheet by calling setMenuOpen(false) on click (e.g., add an onClick
handler or pass a prop that triggers setMenuOpen(false)); ensure both the
signed-in and signed-out link branches call this so the sheet always closes for
in-page/hash navigation and persistent layouts.
There was a problem hiding this comment.
We use Tailwind CSS. Inline styles are strictly forbidden.
Issues in this PR
-
Dark Mode Broken
Hardcodingbackground: "white"completely breaks dark mode support. -
Reinventing the Wheel
Don’t build custom modals from scratch. We already use Shadcn UI — use the<Dialog>component instead. -
Missing Features
The PR description claims the modal closes onEsc, but there are no keyboard event listeners or related handling in the code. -
UI Consistency
Use the project’s<Button>component instead of a raw<button>element.
Please remove components/ShortcutModal.tsx and rewrite it using the Shadcn <Dialog> component with Tailwind classes.
I’m blocking this merge until these issues are fixed.
2 participants
Summary
Type of change
Related issue
Closes #98
Validation
List any additional manual verification you performed:
Screenshots or recordings
(Add screenshot of modal UI if possible)
Checklist
Summary by CodeRabbit
New Features
Bug Fixes / UX
Chores / Configuration