fix: memory leak and layout shift issue#5119
fix: memory leak and layout shift issue#5119krishhna24 wants to merge 1 commit intoasyncapi:masterfrom
Conversation
krishhna24
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
📝 WalkthroughWalkthroughRefactors the window resize handling in pages/finance.tsx from a useRef-based pattern to a useEffect-based approach with null-initialized state. Delays rendering until windowWidth is known to prevent hydration mismatches and layout shifts. Updates layout wrapping logic based on the resolved width and adds proper event listener cleanup. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ 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 |
|
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5119--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pages/finance.tsx (2)
11-11:⚠️ Potential issue | 🟡 MinorRemove unused
Containerimport — pipeline is failing on this.The
Containerimport is no longer used since the layout now uses plain<div>elements with Tailwind classes. The linter flags this as an error.Proposed fix
-import Container from '../components/layout/Container';
18-73: 🛠️ Refactor suggestion | 🟠 MajorIf the layout is now CSS-driven, the entire
windowWidthmachinery is dead code.The PR description says: "Removes JavaScript-driven layout switching and replaces it with CSS breakpoints." The JSX on lines 68-72 already uses Tailwind
2xl:utility classes to handle the layout breakpoint purely in CSS. That means:
windowWidthstate (line 20)handleResize(lines 21-24)- The
useEffect(lines 27-38)- The null-guard (lines 60-63)
shouldUseContainer(line 65)…are all unnecessary. Removing them fully delivers on the PR objective, eliminates the SSR/SEO blank-page issue, and removes the resize listener entirely (which was the original memory-leak concern).
Simplified component
export default function FinancialSummary() { - const [windowWidth, setWindowWidth] = useState<number | null>(null); - const handleResize = () => { - console.log("resized") - setWindowWidth(window.innerWidth); - }; - - useEffect(() => { - - - handleResize(); - - window.addEventListener('resize', handleResize); - - return () => { - window.removeEventListener('resize', handleResize); - }; - }, []); - const title = 'AsyncAPI Finance Summary'; const description = 'Financial Summary of AsyncAPI'; - const renderComponents = () => ( - <> - <Head> - <title>{title}</title> - <meta name="description" content={description} /> - </Head> - - <AsyncAPISummary /> - <SponsorshipTiers /> - <OtherFormsComponent /> - <ExpenseBreakdown /> - <BarChartComponent /> - <SuccessStories /> - <ContactUs /> - </> - ); - - if (windowWidth === null) { - return null; - } - - const shouldUseContainer = windowWidth > 1700; - return ( - <div className="w-full"> - <div className="2xl:container 2xl:mx-auto 2xl:px-8"> - {renderComponents()} - </div> -</div> + <div className='w-full'> + <Head> + <title>{title}</title> + <meta name='description' content={description} /> + </Head> + <div className='2xl:container 2xl:mx-auto 2xl:px-8'> + <AsyncAPISummary /> + <SponsorshipTiers /> + <OtherFormsComponent /> + <ExpenseBreakdown /> + <BarChartComponent /> + <SuccessStories /> + <ContactUs /> + </div> + </div> ); }This also lets you drop the
useEffectanduseStateimports, and the unusedContainerimport.
🤖 Fix all issues with AI agents
In `@pages/finance.tsx`:
- Around line 67-73: The JSX return block in pages/finance.tsx has inconsistent
indentation and uses double quotes in JSX attributes violating jsx-quotes; fix
the indentation so the inner <div> and its child {renderComponents()} are
indented one level inside the outer <div>, and change JSX attribute quotes from
double to single quotes (e.g., className attributes) to satisfy the linter;
locate the return block containing renderComponents() and adjust spacing and
quote style accordingly.
- Line 65: Remove the unused JS-driven layout sizing: delete the
shouldUseContainer constant and the windowWidth state plus its useEffect (and
any window resize handler or cleanup) in pages/finance.tsx, ensuring you also
remove unused imports (useState/useEffect) if they become unused; verify no
other code references windowWidth or shouldUseContainer and rely on the CSS
breakpoints (e.g., 2xl:) instead.
- Around line 60-63: The current guard returning null when windowWidth === null
causes blank SSR output; remove the early return and always render the component
(e.g., keep the exported Finance component JSX) and rely on CSS-driven
breakpoints (the existing 2xl: classes) or a sensible default layout until the
client effect sets windowWidth; update any conditional branches that depended on
windowWidth to fall back to a default (wide or narrow) when windowWidth is null,
and remove the null-check return to prevent empty HTML being sent to crawlers.
- Around line 19-24: The component currently defines handleResize in the render
body and includes a debug console.log and double quotes plus bad indentation;
move the handleResize function inside the useEffect that subscribes to resizing,
remove the console.log, use single quotes for any strings (e.g., 'resized' if
needed, though remove entirely here), and fix indentation around the useState
declaration and effect; update references to windowWidth and setWindowWidth
accordingly and ensure the effect adds/removes the resize listener and lists no
unnecessary dependencies (no eslint-disable needed because the handler is
declared inside useEffect).
| // Initialize as null to avoid hydration/layout shift | ||
| const [windowWidth, setWindowWidth] = useState<number | null>(null); | ||
| const handleResize = () => { | ||
| console.log("resized") | ||
| setWindowWidth(window.innerWidth); | ||
| }; |
There was a problem hiding this comment.
Remove console.log, fix formatting, and move handleResize inside useEffect.
Multiple pipeline-failing issues here:
console.log("resized")is a debug artifact — the linter rejects it (no-console).- Double quotes must be single quotes (
quotesrule). - Indentation is inconsistent (extra leading space on line 21, extra indent on lines 23-24).
- Defining
handleResizein the component body means a new reference is created each render, yet it's omitted from theuseEffectdeps. Move it inside the effect to keep the closure tight and avoid the stale-reference concern.
Proposed fix (move handler into useEffect)
- // Initialize as null to avoid hydration/layout shift
- const [windowWidth, setWindowWidth] = useState<number | null>(null);
- const handleResize = () => {
- console.log("resized")
- setWindowWidth(window.innerWidth);
- };
-
- // Properly scoped resize handler with correct cleanup
- useEffect(() => {
-
-
- // Set initial width on mount
- handleResize();
-
- window.addEventListener('resize', handleResize);
-
- return () => {
- window.removeEventListener('resize', handleResize);
- };
- }, []);
+ const [windowWidth, setWindowWidth] = useState<number | null>(null);
+
+ useEffect(() => {
+ const handleResize = () => {
+ setWindowWidth(window.innerWidth);
+ };
+
+ handleResize();
+ window.addEventListener('resize', handleResize);
+
+ return () => {
+ window.removeEventListener('resize', handleResize);
+ };
+ }, []);Based on learnings: when intentionally omitting dependencies in React hooks, add an eslint-disable comment with an explanatory note. Here, moving the handler inside the effect eliminates the need for that.
🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[error] 21-21: Delete · prettier/prettier
[error] 22-22: Unexpected console statement. no-console
[error] 22-22: Replace "resized" ) with 'resized'); prettier/prettier
[error] 22-22: Strings must use singlequote. quotes
[error] 23-23: Delete ·· prettier/prettier
[error] 24-24: Delete ·· prettier/prettier
🤖 Prompt for AI Agents
In `@pages/finance.tsx` around lines 19 - 24, The component currently defines
handleResize in the render body and includes a debug console.log and double
quotes plus bad indentation; move the handleResize function inside the useEffect
that subscribes to resizing, remove the console.log, use single quotes for any
strings (e.g., 'resized' if needed, though remove entirely here), and fix
indentation around the useState declaration and effect; update references to
windowWidth and setWindowWidth accordingly and ensure the effect adds/removes
the resize listener and lists no unnecessary dependencies (no eslint-disable
needed because the handler is declared inside useEffect).
| // Avoid rendering until window size is known (prevents CLS) | ||
| if (windowWidth === null) { | ||
| return null; // or a skeleton/loading placeholder | ||
| } |
There was a problem hiding this comment.
Returning null until client-side width is resolved hurts SSR/SEO and causes a flash of empty content.
Because windowWidth is null on the server (and on the first client render before the effect fires), this page will:
- Send an empty HTML response to search engines / social-media crawlers.
- Flash a blank screen before hydration completes.
This trades the original layout-shift problem for a blank-page problem. A better approach is to always render the content using a sensible default layout (e.g., the wide or narrow variant) and only switch once the width is known, or use CSS-only breakpoints for the layout difference (which the new JSX on lines 68-72 already does via 2xl: classes). If the layout is purely CSS-driven, you don't need the windowWidth guard at all.
Proposed fix — remove the null guard
- // Avoid rendering until window size is known (prevents CLS)
- if (windowWidth === null) {
- return null; // or a skeleton/loading placeholder
- }📝 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.
| // Avoid rendering until window size is known (prevents CLS) | |
| if (windowWidth === null) { | |
| return null; // or a skeleton/loading placeholder | |
| } |
🤖 Prompt for AI Agents
In `@pages/finance.tsx` around lines 60 - 63, The current guard returning null
when windowWidth === null causes blank SSR output; remove the early return and
always render the component (e.g., keep the exported Finance component JSX) and
rely on CSS-driven breakpoints (the existing 2xl: classes) or a sensible default
layout until the client effect sets windowWidth; update any conditional branches
that depended on windowWidth to fall back to a default (wide or narrow) when
windowWidth is null, and remove the null-check return to prevent empty HTML
being sent to crawlers.
| return null; // or a skeleton/loading placeholder | ||
| } | ||
|
|
||
| const shouldUseContainer = windowWidth > 1700; |
There was a problem hiding this comment.
Remove unused shouldUseContainer — pipeline is failing on this.
This variable is computed but never referenced. If the layout is now CSS-driven (2xl: breakpoints), the JS-based width check is dead code. Remove it along with the windowWidth state and effect if they serve no other purpose.
Proposed fix
- const shouldUseContainer = windowWidth > 1700;📝 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 shouldUseContainer = windowWidth > 1700; |
🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[error] 65-65: ShouldUseContainer is assigned a value but never used. no-unused-vars
[error] 65-65: ShouldUseContainer is assigned a value but never used. @typescript-eslint/no-unused-vars
[error] 65-65: ShouldUseContainer is assigned a value but never used. unused-imports/no-unused-vars
🤖 Prompt for AI Agents
In `@pages/finance.tsx` at line 65, Remove the unused JS-driven layout sizing:
delete the shouldUseContainer constant and the windowWidth state plus its
useEffect (and any window resize handler or cleanup) in pages/finance.tsx,
ensuring you also remove unused imports (useState/useEffect) if they become
unused; verify no other code references windowWidth or shouldUseContainer and
rely on the CSS breakpoints (e.g., 2xl:) instead.
| return ( | ||
| <div className="w-full"> | ||
| <div className="2xl:container 2xl:mx-auto 2xl:px-8"> | ||
| {renderComponents()} | ||
| </div> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Fix indentation and quote style — multiple prettier/lint failures.
The JSX block uses double quotes (violating jsx-quotes) and inconsistent indentation (the inner <div> and children are not properly indented relative to the outer <div>).
Proposed fix
- return (
- <div className="w-full">
- <div className="2xl:container 2xl:mx-auto 2xl:px-8">
- {renderComponents()}
- </div>
-</div>
- );
+ return (
+ <div className='w-full'>
+ <div className='2xl:container 2xl:mx-auto 2xl:px-8'>
+ {renderComponents()}
+ </div>
+ </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.
| return ( | |
| <div className="w-full"> | |
| <div className="2xl:container 2xl:mx-auto 2xl:px-8"> | |
| {renderComponents()} | |
| </div> | |
| </div> | |
| ); | |
| return ( | |
| <div className='w-full'> | |
| <div className='2xl:container 2xl:mx-auto 2xl:px-8'> | |
| {renderComponents()} | |
| </div> | |
| </div> | |
| ); |
🧰 Tools
🪛 GitHub Actions: PR testing - if Node project
[error] 68-68: Replace "w-full" with 'w-full' prettier/prettier
[error] 68-68: Unexpected usage of doublequote. jsx-quotes
[error] 69-69: Replace <div className="2xl:container 2xl:mx-auto 2xl:px-8">\n····{renderComponents()}\n·· with <div className='2xl:container 2xl:mx-auto 2xl:px-8'>{renderComponents()} prettier/prettier
[error] 69-69: Unexpected usage of doublequote. jsx-quotes
[error] 72-72: Insert prettier/prettier
🤖 Prompt for AI Agents
In `@pages/finance.tsx` around lines 67 - 73, The JSX return block in
pages/finance.tsx has inconsistent indentation and uses double quotes in JSX
attributes violating jsx-quotes; fix the indentation so the inner <div> and its
child {renderComponents()} are indented one level inside the outer <div>, and
change JSX attribute quotes from double to single quotes (e.g., className
attributes) to satisfy the linter; locate the return block containing
renderComponents() and adjust spacing and quote style accordingly.
|
fix the title please ,I believe its layout also add the issue number |
1 similar comment
|
fix the title please ,I believe its layout also add the issue number |
|
@krishhna24 thanks for your efforts. I think this PR is linked to #5114, but the current author of that issue is already working on it and is waiting for maintainer approval. If you want to work on that issue, you need to ask the author first. |
krishhna24
changed the title
|
4 participants
Summary by CodeRabbit
Release Notes