fix: move themeColor and viewport out of metadata into separate viewport export#311
fix: move themeColor and viewport out of metadata into separate viewport export#311HamidKhan1001 wants to merge 3 commits intoAOSSIE-Org:devfrom
Conversation
📝 WalkthroughWalkthroughThe pull request moves Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/layout.js (1)
23-27: LGTM — correct migration to the dedicatedviewportexport.
themeColorin themetadataexport is deprecated as of Next.js 14 andviewportinmetadatais similarly deprecated; both must be in theviewportexport instead, exactly as done here.Two minor, optional improvements worth considering:
Redundant fields: Next.js automatically sets the viewport meta tag, so manual configuration is usually unnecessary as the default is already
width=device-width, initial-scale=1. ThewidthandinitialScaleentries therefore duplicate the framework default and can be omitted, leaving onlythemeColorwhich is the non-default value.JSDoc type annotation: Since this is a
.jsfile, adding a JSDoc comment provides editor type-checking and autocomplete for theViewportshape. For JavaScript projects you can use JSDoc to add type safety:/**@type{import("next").Viewport} */.♻️ Optional simplification
+/** `@type` {import("next").Viewport} */ export const viewport = { themeColor: "#000000", - width: "device-width", - initialScale: 1, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/layout.js` around lines 23 - 27, Remove redundant viewport fields by keeping only themeColor in the exported viewport object (symbol: viewport) since Next.js defaults to width=device-width and initial-scale=1; additionally, add a JSDoc type annotation above the viewport export (use import("next").Viewport) to provide editor type-checking and autocomplete in this .js file.app/page.jsx (1)
213-224: Consider collecting and killing the anonymous feature-item timelines in cleanup.The four named timelines are now correctly killed. However, the anonymous
tlinstances created inside thegsap.utils.toArray(".feature-item").forEach(...)block (lines 84–106) are not tracked or killed.ScrollTrigger.getAll().forEach((t) => t.kill())only kills the ScrollTrigger instances — it does not automatically kill the associated GSAP timeline objects. Those timelines remain in GSAP's internal ticker post-unmount, holding stale DOM references and risking duplicate animations on re-mount (particularly in React StrictMode).♻️ Proposed fix: collect feature timelines for cleanup
+ const featureTls = []; gsap.utils.toArray(".feature-item").forEach((feature, i) => { const image = feature.querySelector(".feature-image"); const text = feature.querySelector(".feature-content h3"); const tl = gsap.timeline({ scrollTrigger: { trigger: feature, start: "top 80%", toggleActions: "play none none reverse", }, }); tl.fromTo(/* ... */); + featureTls.push(tl); });Then in the cleanup:
heroTl.kill(); techStackTl.kill(); aboutTl.kill(); downloadTl.kill(); + featureTls.forEach((tl) => tl.kill()); ScrollTrigger.getAll().forEach((t) => t.kill());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/page.jsx` around lines 213 - 224, The anonymous timelines created inside gsap.utils.toArray(".feature-item").forEach(...) (the local tl instances) are not being destroyed; modify the code to collect those tl objects into a module/local array (e.g., featureTimelines) when you create them and then, in the cleanup return block where you currently kill heroTl, techStackTl, aboutTl, downloadTl and call ScrollTrigger.getAll().forEach(...), also iterate over featureTimelines and call .kill() on each (and optionally clear the array) so the feature-item timelines are removed from GSAP's ticker and no stale DOM refs remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/layout.js`:
- Around line 23-27: Remove redundant viewport fields by keeping only themeColor
in the exported viewport object (symbol: viewport) since Next.js defaults to
width=device-width and initial-scale=1; additionally, add a JSDoc type
annotation above the viewport export (use import("next").Viewport) to provide
editor type-checking and autocomplete in this .js file.
In `@app/page.jsx`:
- Around line 213-224: The anonymous timelines created inside
gsap.utils.toArray(".feature-item").forEach(...) (the local tl instances) are
not being destroyed; modify the code to collect those tl objects into a
module/local array (e.g., featureTimelines) when you create them and then, in
the cleanup return block where you currently kill heroTl, techStackTl, aboutTl,
downloadTl and call ScrollTrigger.getAll().forEach(...), also iterate over
featureTimelines and call .kill() on each (and optionally clear the array) so
the feature-item timelines are removed from GSAP's ticker and no stale DOM refs
remain.
|
Hi @HamidKhan1001, this is an incorrect PR template. You will need to fix your PR by properly using our standard tempelate and then clearly mentioning the related issue which is #310 |
3 participants
PR description: