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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a client-side ShortcutModal and a Shortcuts trigger in the Header (with a mobile Sheet menu); simplifies the Home page and JSON‑LD; requires server.AUTH_SECRET in env schema; adds pnpm allowBuilds entries; and pins ChangesShortcuts Modal Integration & Related Updates
Sequence Diagram(s)sequenceDiagram
participant Header
participant ShortcutModal
participant Sheet
Header->>ShortcutModal: open={shortcutOpen} / onOpenChange={setShortcutOpen}
Header->>Sheet: open={menuOpen} / onOpenChange={setMenuOpen}
Sheet->>Header: render auth-dependent links
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
app/(root)/page.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. modules/home/header.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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.
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.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/(root)/page.tsx (2)
83-83: ⚡ Quick winRemove development comments.
The "✅ FIXED BUTTON" comments appear to be development artifacts and should be removed before merging.
🧹 Proposed cleanup
- {/* ✅ FIXED BUTTON */} <Button asChild size="lg"> <Link href="/dashboard"> Start Coding for Free <ArrowRight className="ml-2 h-4 w-4" /> </Link> </Button> - {/* ✅ FIXED BUTTON */} <Button asChild variant="outline" size="lg"> <Link href="`#features`"> Explore Features </Link> </Button>Also applies to: 91-91
🤖 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 `@app/`(root)/page.tsx at line 83, Remove the stray development JSX comments that say {/* ✅ FIXED BUTTON */}; locate the two occurrences of that JSX comment (they sit adjacent to the button elements in the page component) and delete those comment nodes so the production JSX contains no development artifacts.
66-66: 💤 Low valueConsider using a semantic z-index value.
The
z-[9999]is an arbitrary, extremely high value. If your design system defines semantic z-index values (e.g.,z-modal,z-overlay), prefer those for maintainability and to avoid future stacking context conflicts.🤖 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 `@app/`(root)/page.tsx at line 66, Replace the hardcoded z-[9999] utility in the modal/root element class string with a semantic z-index token from your design system (e.g., use z-modal or z-overlay) and, if that token doesn't exist yet, add it to your Tailwind config as a named z-index (e.g., theme.extend.z.modal = 9999) so the component in app/(root)/page.tsx uses the semantic class instead of the numeric literal.
🤖 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 `@app/`(root)/page.tsx:
- Around line 64-71: The loading overlay div currently lacks accessibility
attributes; update the div that wraps CommitsGrid (the element using isLoading)
to include ARIA attributes: add role="status", aria-live="polite", and set
aria-busy={isLoading} so assistive tech knows the app is loading, and toggle
aria-hidden={!isLoading} (or remove live announcements) when not loading; ensure
the CommitsGrid component itself does not duplicate announcements so only the
overlay controls the loading announcement.
- Around line 43-44: The schema currently uses hardcoded URLs for the url and
logo fields ("https://editron.vercel.app"); replace those with an
environment-aware base URL (e.g. use NEXT_PUBLIC_BASE_URL) or derive the origin
at runtime and concatenate paths so the `url` and `logo` values are correct in
dev/staging/preview; update the code that builds the structured data (the object
containing `url` and `logo`) to read from process.env.NEXT_PUBLIC_BASE_URL with
a sensible fallback to the current request origin, and ensure the env var is
documented/added to CI so all environments produce correct absolute URLs.
---
Nitpick comments:
In `@app/`(root)/page.tsx:
- Line 83: Remove the stray development JSX comments that say {/* ✅ FIXED BUTTON
*/}; locate the two occurrences of that JSX comment (they sit adjacent to the
button elements in the page component) and delete those comment nodes so the
production JSX contains no development artifacts.
- Line 66: Replace the hardcoded z-[9999] utility in the modal/root element
class string with a semantic z-index token from your design system (e.g., use
z-modal or z-overlay) and, if that token doesn't exist yet, add it to your
Tailwind config as a named z-index (e.g., theme.extend.z.modal = 9999) so the
component in app/(root)/page.tsx uses the semantic class instead of the numeric
literal.
🪄 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: e5cc75ed-05ab-4cee-8651-8ff8249ccd22
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
app/(root)/page.tsxcomponents/ShortcutModal.tsxmodules/home/header.tsx
| url: "https://editron.vercel.app", | ||
| logo: "https://editron.vercel.app/logo.svg", |
There was a problem hiding this comment.
Replace hardcoded production URLs with environment-aware base URL.
The schema markup contains hardcoded editron.vercel.app URLs that will be incorrect in development, staging, or preview environments. This can confuse search engines when they encounter mismatched URLs between the page location and the structured data.
🌐 Proposed fix using environment variable or Next.js URL
+ const baseUrl = process.env.NEXT_PUBLIC_APP_URL || "https://editron.vercel.app";
+
const schemaMarkup = {
"`@context`": "https://schema.org",
"`@graph`": [
// ...
{
"`@type`": "Organization",
name: "Editron",
- url: "https://editron.vercel.app",
- logo: "https://editron.vercel.app/logo.svg",
+ url: baseUrl,
+ logo: `${baseUrl}/logo.svg`,
},
],
};🤖 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 `@app/`(root)/page.tsx around lines 43 - 44, The schema currently uses
hardcoded URLs for the url and logo fields ("https://editron.vercel.app");
replace those with an environment-aware base URL (e.g. use NEXT_PUBLIC_BASE_URL)
or derive the origin at runtime and concatenate paths so the `url` and `logo`
values are correct in dev/staging/preview; update the code that builds the
structured data (the object containing `url` and `logo`) to read from
process.env.NEXT_PUBLIC_BASE_URL with a sensible fallback to the current request
origin, and ensure the env var is documented/added to CI so all environments
produce correct absolute URLs.
| <div | ||
| className={cn( | ||
| "fixed inset-0 z-[9999] flex items-center justify-center bg-background transition-opacity duration-700 ease-in-out", | ||
| isLoading ? "opacity-100" : "opacity-0 pointer-events-none", | ||
| "fixed inset-0 z-[9999] flex items-center justify-center bg-background transition-opacity", | ||
| isLoading ? "opacity-100" : "opacity-0 pointer-events-none" | ||
| )} | ||
| > | ||
| <div className="w-full flex justify-center"> | ||
| <CommitsGrid text="EDITRON" /> | ||
| </div> | ||
| <CommitsGrid text="EDITRON" /> | ||
| </div> |
There was a problem hiding this comment.
Add ARIA attributes for loading state accessibility.
The loading overlay lacks ARIA attributes to announce the loading state to screen readers. Users relying on assistive technology won't know the page is loading.
♿ Proposed fix adding accessibility attributes
<div
className={cn(
"fixed inset-0 z-[9999] flex items-center justify-center bg-background transition-opacity",
isLoading ? "opacity-100" : "opacity-0 pointer-events-none"
)}
+ role="status"
+ aria-live="polite"
+ aria-label={isLoading ? "Loading application" : ""}
>
<CommitsGrid text="EDITRON" />
</div>📝 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.
| <div | |
| className={cn( | |
| "fixed inset-0 z-[9999] flex items-center justify-center bg-background transition-opacity duration-700 ease-in-out", | |
| isLoading ? "opacity-100" : "opacity-0 pointer-events-none", | |
| "fixed inset-0 z-[9999] flex items-center justify-center bg-background transition-opacity", | |
| isLoading ? "opacity-100" : "opacity-0 pointer-events-none" | |
| )} | |
| > | |
| <div className="w-full flex justify-center"> | |
| <CommitsGrid text="EDITRON" /> | |
| </div> | |
| <CommitsGrid text="EDITRON" /> | |
| </div> | |
| <div | |
| className={cn( | |
| "fixed inset-0 z-[9999] flex items-center justify-center bg-background transition-opacity", | |
| isLoading ? "opacity-100" : "opacity-0 pointer-events-none" | |
| )} | |
| role="status" | |
| aria-live="polite" | |
| aria-label={isLoading ? "Loading application" : ""} | |
| > | |
| <CommitsGrid text="EDITRON" /> | |
| </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 `@app/`(root)/page.tsx around lines 64 - 71, The loading overlay div currently
lacks accessibility attributes; update the div that wraps CommitsGrid (the
element using isLoading) to include ARIA attributes: add role="status",
aria-live="polite", and set aria-busy={isLoading} so assistive tech knows the
app is loading, and toggle aria-hidden={!isLoading} (or remove live
announcements) when not loading; ensure the CommitsGrid component itself does
not duplicate announcements so only the overlay controls the loading
announcement.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/home/header.tsx (1)
96-189:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix the mobile Sheet block: it is currently JSX-invalid and build-blocking.
This section appears to contain an unresolved merge of two different
SheetContentimplementations (duplicated containers, broken conditional branches/fragments), and it also callssetOpen(...)even though onlysetMenuOpen(...)exists.Suggested direction (minimal cleanup)
-<SheetContent side="right"> - ... - <SheetContent side="right" className="..."> - ... - onClick={() => setOpen(false)} +<SheetContent side="right" className="w-[85%] max-w-[320px] border-l border-border/50 bg-background/95 backdrop-blur-xl p-0 transition-all duration-300"> + ... + onClick={() => setMenuOpen(false)}Keep only one
SheetContenttree and one auth conditional branch.🤖 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 96 - 189, The mobile Sheet block contains duplicated SheetContent trees, broken JSX fragments and calls setOpen(...) which doesn't exist; collapse to a single SheetContent tree (remove the extra nested SheetContent), restore a single coherent auth conditional around {!user ? (...) : (...)} using the existing user variable, replace all setOpen(...) calls with setMenuOpen(...) and ensure fragments/ternaries are properly opened and closed so the JSX is valid (check the block that renders Links/Buttons and the bottom auth buttons to match the single SheetContent structure).
🤖 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 `@app/`(root)/page.tsx:
- Around line 18-23: The file has two conflicting Home exports and mixed client
hooks (useState) with server-side async logic (getPopularTemplateSummaries) plus
stray JSX after the function close; fix by keeping a single default export: make
the top-level Home an async server component that awaits
getPopularTemplateSummaries(4) and renders markup, then extract any client-only
behavior (useState, useEffect, interactive handlers) into a separate
"ClientHome" React component annotated with "use client" and import/render that
from the server Home, remove the duplicate export and the stray JSX outside any
function, and ensure all references to Home, getPopularTemplateSummaries,
useState, and the stray JSX block are relocated or deleted so the file compiles.
In `@modules/home/header.tsx`:
- Around line 205-243: NavLink currently contains two consecutive opening <Link>
tags which breaks JSX; open only a single <Link> in the NavLink function (the
one that includes the full className and props) and remove the stray first <Link
href={href} className="px-3 py-2 text-sm"> so there is one <Link href={href}
className="..."> ... {children} </Link>; ensure the final element is properly
closed and that any needed props (href, className) from the removed tag are
preserved in the remaining <Link>.
---
Outside diff comments:
In `@modules/home/header.tsx`:
- Around line 96-189: The mobile Sheet block contains duplicated SheetContent
trees, broken JSX fragments and calls setOpen(...) which doesn't exist; collapse
to a single SheetContent tree (remove the extra nested SheetContent), restore a
single coherent auth conditional around {!user ? (...) : (...)} using the
existing user variable, replace all setOpen(...) calls with setMenuOpen(...) and
ensure fragments/ternaries are properly opened and closed so the JSX is valid
(check the block that renders Links/Buttons and the bottom auth buttons to match
the single SheetContent structure).
🪄 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: 9fe40b30-8175-4093-906c-04942c27c873
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
app/(root)/page.tsxmodules/home/header.tsxpackage.json
| export default function Home() { | ||
| const [isLoading, setIsLoading] = useState(true); | ||
| export default async function Home() { | ||
| const popularTemplates = await getPopularTemplateSummaries(4); | ||
|
|
||
| // Schema Markup for AI SEO (Organization & SoftwareApplication) | ||
| const schemaMarkup = { |
There was a problem hiding this comment.
Resolve the mixed client/server page merge; this file is currently non-compilable.
There are duplicate default exports, client hooks in the same merged body, and stray JSX after the function close. This needs to be reduced to a single implementation path.
Minimal fix direction
-import { useState, useEffect } from "react";
-import { Button } from "`@/components/ui/button`";
-import { ArrowRight } from "lucide-react";
-import Link from "next/link";
-import { Features } from "`@/modules/home/features`";
-import { HeroCodeDemo } from "`@/modules/home/hero-code`";
-import dynamic from "next/dynamic";
-import { CommitsGrid } from "`@/components/ui/commits-grid`";
-import { cn } from "`@/lib/utils`";
-import { templates } from "`@/lib/constants/templates`";
-import { TemplateCard } from "`@/components/marketing/template-card`";
-
-export default function Home() {
- const [isLoading, setIsLoading] = useState(true);
export default async function Home() {
const popularTemplates = await getPopularTemplateSummaries(4);
- // ...merged legacy client JSX...
+ return <HomePageClient popularTemplates={popularTemplates} />;
}
-
- <HomePageClient popularTemplates={popularTemplates} />
- </div>
- );
-}Also applies to: 48-53, 119-123
🧰 Tools
🪛 Biome (2.4.15)
[error] 18-123: Illegal duplicate default export declarations
(parse)
🤖 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 `@app/`(root)/page.tsx around lines 18 - 23, The file has two conflicting Home
exports and mixed client hooks (useState) with server-side async logic
(getPopularTemplateSummaries) plus stray JSX after the function close; fix by
keeping a single default export: make the top-level Home an async server
component that awaits getPopularTemplateSummaries(4) and renders markup, then
extract any client-only behavior (useState, useEffect, interactive handlers)
into a separate "ClientHome" React component annotated with "use client" and
import/render that from the server Home, remove the duplicate export and the
stray JSX outside any function, and ensure all references to Home,
getPopularTemplateSummaries, useState, and the stray JSX block are relocated or
deleted so the file compiles.
| @@ -222,4 +242,4 @@ function NavLink({ href, children }: { href: string; children: React.ReactNode } | |||
| {children} | |||
| </Link> | |||
There was a problem hiding this comment.
NavLink has duplicate opening <Link> tags.
NavLink renders one <Link ...> and immediately starts another <Link ...> before closing, which is invalid JSX and will fail parsing.
Minimal fix
function NavLink({ href, children }: any) {
return (
- <Link href={href} className="px-3 py-2 text-sm">
- <Link
+ <Link
href={href}
className="
relative
px-3
py-2
text-sm
font-medium
text-muted-foreground
hover:text-red-500
rounded-md
transition-all
duration-300
@@
hover:after:scale-x-100
"
>
{children}
</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.
| function NavLink({ href, children }: any) { | |
| return ( | |
| <Link | |
| href={href} | |
| className=" | |
| relative | |
| px-3 | |
| py-2 | |
| text-sm | |
| font-medium | |
| text-muted-foreground | |
| hover:text-red-500 | |
| rounded-md | |
| transition-all | |
| duration-300 | |
| after:content-[''] | |
| after:absolute | |
| after:left-1/2 | |
| after:bottom-1 | |
| after:h-[2px] | |
| after:w-[70%] | |
| after:-translate-x-1/2 | |
| after:scale-x-0 | |
| after:origin-center | |
| after:rounded-full | |
| after:bg-gradient-to-r | |
| after:from-red-900 | |
| after:via-red-600 | |
| after:to-red-400 | |
| after:transition-transform | |
| after:duration-300 | |
| hover:after:scale-x-100 | |
| " | |
| > | |
| {children} | |
| </Link> | |
| ); | |
| } |
🧰 Tools
🪛 Biome (2.4.15)
[error] 206-206: expected } but instead found return
(parse)
🤖 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 205 - 243, NavLink currently contains
two consecutive opening <Link> tags which breaks JSX; open only a single <Link>
in the NavLink function (the one that includes the full className and props) and
remove the stray first <Link href={href} className="px-3 py-2 text-sm"> so there
is one <Link href={href} className="..."> ... {children} </Link>; ensure the
final element is properly closed and that any needed props (href, className)
from the removed tag are preserved in the remaining <Link>.
piyushdotcomm
left a comment
piyushdotcomm
left a comment
There was a problem hiding this comment.
check the code rabbit review and fix those
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