Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds client-side Web Vitals reporting and logging, integrates the WebVitals component into the root layout (only in non-production), extends Tailwind content paths to include components, adds .lighthouse to .gitignore, and switches the Docker runner image and runtime invocation from Node.js to Bun. Changes
Sequence DiagramsequenceDiagram
participant Browser as Browser (Client)
participant Root as RootLayout
participant WebVitals as WebVitals Component
participant NextHook as useReportWebVitals (Next.js)
participant Logger as logWebVitals
participant Console as Browser Console
Browser->>Root: Render app/layout
Root->>WebVitals: Mount (conditional: NODE_ENV !== "production")
WebVitals->>NextHook: Register reporting callback
Note over Browser,NextHook: Page lifecycle events (load/paint/interaction)
NextHook->>Logger: Invoke with metric object
Logger->>Console: console.log(metric)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
Summary of ChangesHello @prdai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the application's performance monitoring capabilities by integrating Web Vitals reporting. It introduces a dedicated component and a logging utility, then strategically deploys this monitoring across various critical pages to gather essential user experience metrics. This foundational work will enable future analysis and optimization efforts to improve site performance. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Web Vitals reporting, which is a great addition for performance monitoring. However, the current implementation has a critical architectural issue: the WebVitals component is being called as a function (WebVitals()) in every page component. This violates the Rules of Hooks and will cause the application to crash. It also introduces a lot of code duplication. The correct approach is to render <WebVitals /> once in the root layout (app/layout.tsx) to apply it globally. I've left comments on app/accessories/page.tsx demonstrating the necessary changes, which should be applied to all other pages modified in this PR. Additionally, I've suggested improvements for type safety in the logWebVitals function and pointed out an unused dependency.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
crow-rogue-store-dev | 42dbbbe | Commit Preview URL Branch Preview URL |
Dec 24 2025, 02:21 PM |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
app/checkout/success/page.tsx (1)
12-25: WebVitals usage mirrors HomePage patternThis file follows the same
WebVitals();side-effect pattern asapp/page.tsx. The general suggestions there (treatingWebVitalsconsistently as either a hook or a component and possibly centralising its usage) apply here as well; no additional issues specific to this page.app/accessories/page.tsx (1)
18-25: Consistent with global WebVitals pattern; consider aligning import styleThis page’s
WebVitals();usage matches the pattern discussed inapp/page.tsx(see that comment for abstraction suggestions). Functionally it’s fine. You may also want to standardize on either alias-based (@/app/_components/web-vitals) or relative imports for this module across pages for consistency.app/checkout/page.tsx (1)
24-33: Checkout page WebVitals wiring matches other pagesThe Checkout page wires
WebVitals();the same way as the other client pages, with a stable hook order. No new concerns here; the abstraction/centralization suggestions fromapp/page.tsxapply equally to this file.
🧹 Nitpick comments (7)
tailwind.config.ts (1)
7-7: Confirm whether./_componentsis needed in Tailwind content
"./app/**/*.{js,ts,jsx,tsx,mdx}"already coversapp/_components/**, so this additional./_components/**/*entry is redundant unless you also have a top-level_componentsdirectory. Consider dropping it or clarifying the intended folder to keep the config minimal.lib/utils.ts (1)
58-62: TightenlogWebVitalstyping and integrateloglevelRight now
logWebVitalsusesanyplus a lint suppression and logs directly toconsole.log, whileloglevelhas been added as a dependency but isn’t used.Consider:
- Typing the argument with the Web Vitals metric type (e.g., from
next/web-vitalsorweb-vitals), and- Using
loglevel(e.g.,log.info(metric)) so you can drop the// eslint-disable-next-lineand have consistent, configurable logging.Example diff for this block:
-// eslint-disable-next-line -export const logWebVitals = (metric: any) => { - console.log(metric); -}; +export const logWebVitals = (metric: unknown) => { + // e.g., narrow `metric` to the concrete Web Vitals type here if desired + // and route through your logging library: + // log.info("web-vitals", metric); + console.log(metric); +};(You’d also add a
loglevelimport at the top if you choose to wire it in.)package.json (1)
34-34: Ensureloglevelis actually used or remove it
loglevelis added as a runtime dependency, but the current Web Vitals logger still usesconsole.log. To avoid unused dependencies, either:
- Hook
loglevelintologWebVitals(preferred for configurable logging), or- Remove
loglevelfor now and reintroduce it when you have a concrete usage.app/page.tsx (1)
8-11: ClarifyWebVitalsabstraction and usage patternHere
WebVitals()is invoked like a custom hook, but the implementation (inapp/_components/web-vitals.js) looks like a component-style wrapper arounduseReportWebVitals. To reduce confusion and make hook usage more idiomatic:
- Either treat it as a hook: rename to
useWebVitalsand keep calling it at the top of each client page, or- Treat it as a component: have it
return nulland render<WebVitals />in a central place (e.g., a client-only wrapper) so instrumentation is registered once instead of on every page.The current pattern works, but a clearer abstraction will make future changes to Web Vitals instrumentation safer and easier to follow, especially as more analytics logic is added.
app/men/page.tsx (1)
18-25: WebVitals wiring is correct; consider centralizing usageImporting
WebVitalsand callingWebVitals()at the top ofMenPagecorrectly ensures Web Vitals reporting is registered for this route. Longer term, you might consider mounting this once in a shared root layout/provider instead of per page to reduce duplication, but it's not required for correctness.app/products/[slug]/page.tsx (1)
29-45: Align WebVitals import path style with other pages (optional)
WebVitals()is correctly integrated here. This file uses the alias path"@/app/_components/web-vitals"while other pages use"../_components/web-vitals". For consistency and easier refactors, consider standardizing on one style (likely the alias form everywhere), but it's purely a style/maintainability tweak.app/_components/web-vitals.js (1)
1-6: WebVitals helper should explicitly return null to match Next.js documentationThe implementation correctly registers
logWebVitalswithuseReportWebVitals. However, the official Next.js 15 App Router documentation shows this exact pattern should returnnull, allowing safe rendering as<WebVitals />in your root layout (the recommended approach to keep the client boundary minimal).Add
return null;at the end of the function:"use client"; import { useReportWebVitals } from "next/web-vitals"; import { logWebVitals } from "@/lib/utils"; export function WebVitals() { useReportWebVitals(logWebVitals); + return null; }Optionally add a comment to clarify its purpose. The
logWebVitalsimport from@/lib/utilsis correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (16)
app/_components/web-vitals.js(1 hunks)app/accessories/page.tsx(1 hunks)app/cart/page.tsx(2 hunks)app/checkout/page.tsx(1 hunks)app/checkout/success/page.tsx(2 hunks)app/error.tsx(2 hunks)app/gifts/page.tsx(1 hunks)app/layout.tsx(1 hunks)app/men/page.tsx(1 hunks)app/page.tsx(1 hunks)app/products/[slug]/page.tsx(2 hunks)app/products/page.tsx(2 hunks)app/women/page.tsx(1 hunks)lib/utils.ts(1 hunks)package.json(1 hunks)tailwind.config.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
app/page.tsx (1)
app/_components/web-vitals.js (1)
WebVitals(4-6)
app/cart/page.tsx (1)
app/_components/web-vitals.js (1)
WebVitals(4-6)
app/accessories/page.tsx (1)
app/_components/web-vitals.js (1)
WebVitals(4-6)
app/gifts/page.tsx (1)
app/_components/web-vitals.js (1)
WebVitals(4-6)
app/women/page.tsx (1)
app/_components/web-vitals.js (1)
WebVitals(4-6)
app/checkout/page.tsx (1)
app/_components/web-vitals.js (1)
WebVitals(4-6)
app/checkout/success/page.tsx (1)
app/_components/web-vitals.js (1)
WebVitals(4-6)
app/men/page.tsx (1)
app/_components/web-vitals.js (1)
WebVitals(4-6)
app/_components/web-vitals.js (1)
lib/utils.ts (1)
logWebVitals(60-62)
app/products/[slug]/page.tsx (1)
app/_components/web-vitals.js (1)
WebVitals(4-6)
app/products/page.tsx (1)
app/_components/web-vitals.js (1)
WebVitals(4-6)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Workers Builds: rouge-store
🔇 Additional comments (6)
app/layout.tsx (1)
23-41: No-op layout changeOnly formatting changed around the closing brace; layout behavior and structure remain intact. Nothing to address here.
app/products/page.tsx (1)
27-38: WebVitals integration in ProductsContent looks goodCalling
WebVitals()at the start ofProductsContentcleanly hooks Web Vitals into this page without affecting existing state/logic; hook ordering is preserved.app/error.tsx (1)
8-27: Error boundary now participates in Web Vitals reportingImporting
WebVitalsand invokingWebVitals()at the top ofErrorPagecleanly ensures metrics are still collected when the error UI is shown, without changing the existinguseEffect-based logging or reset behavior.app/cart/page.tsx (1)
21-34: Cart page Web Vitals hook-in is straightforward and safeThe added
WebVitalsimport andWebVitals()call at the top ofCartPagecorrectly attach metrics reporting to this route without impacting cart state or conditional rendering for the empty/non-empty views.app/women/page.tsx (1)
18-25: Women’s page instrumentation matches the pattern elsewhereThe Web Vitals integration here (
WebVitalsimport andWebVitals()call) is consistent with other collection pages and does not alter the existing filtering or UI behavior.app/gifts/page.tsx (1)
18-25: Gifts page Web Vitals hook-in is correct and non-intrusiveThe
WebVitalsimport andWebVitals()call at the top ofGiftsPagecorrectly opt this route into metrics collection without affecting the existing gifts filtering or animations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
_components/web-vitals.js (1)
5-8: Returnnull(or nothing) instead of rendering an empty<div>.
This component exists for side effects only; an extra div inRootLayoutis avoidable.export const WebVitals = () => { useReportWebVitals(logWebVitals); - return <div></div>; + return null; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
_components/web-vitals.js(1 hunks)app/layout.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/layout.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
_components/web-vitals.js (1)
lib/utils.ts (1)
logWebVitals(60-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Workers Builds: rouge-store
- GitHub Check: build-and-push
…ntegration-for-rogue-store
…ntegration-for-rogue-store
3 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.