fix: memory leak and layout shift in finance page (#5114)#5118
fix: memory leak and layout shift in finance page (#5114)#5118Sahitya0805 wants to merge 2 commits intoasyncapi:masterfrom
Conversation
Sahitya0805
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. |
📝 WalkthroughWalkthroughDeferred chart rendering behind a mounted flag and removed runtime window resize handling across the Finance page and BarChart component; chart now renders with fixed dimensions and a loading fallback, and page layout no longer depends on client-side window width checks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 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-5118--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: 1
🤖 Fix all issues with AI agents
In `@components/FinancialSummary/BarChartComponent.tsx`:
- Around line 92-118: The BarChart is using a hardcoded width (width={800})
which causes horizontal overflow on small viewports; modify BarChartComponent to
import ResponsiveContainer from 'recharts' and wrap the <BarChart ...> inside
<ResponsiveContainer width="100%" height="100%"> (or height={400} on the
container) and remove the fixed width/height props from <BarChart>, preserving
existing props like data={chartData}, <CartesianGrid/>, <YAxis/>, <Tooltip
content={<CustomTooltip/>}/>, <Legend/> and the <Bar dataKey='Amount' ... />
click handler so the chart becomes fluid while still guarded by the isMounted
check.
🧹 Nitpick comments (1)
pages/finance.tsx (1)
22-36:renderComponentsis now unnecessary indirection.Since the conditional rendering based on
windowWidthhas been removed, this wrapper function serves no purpose — its contents can be inlined directly into thereturn.♻️ Inline the JSX
export default function FinancialSummary() { 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 /> - </> - ); - return ( <Container fluid className='min-[1701px]:mx-auto min-[1701px]:w-full min-[1701px]:max-w-screen-xl' padding='none' > - {renderComponents()} + <Head> + <title>{title}</title> + <meta name='description' content={description} /> + </Head> + <AsyncAPISummary /> + <SponsorshipTiers /> + <OtherFormsComponent /> + <ExpenseBreakdown /> + <BarChartComponent /> + <SuccessStories /> + <ContactUs /> </Container> ); }
| <div className='flex justify-center h-[400px]'> | ||
| {isMounted ? ( | ||
| <BarChart width={800} height={400} data={chartData}> | ||
| <CartesianGrid strokeDasharray='3 3' /> | ||
| <YAxis tickFormatter={(value) => `$${value}`} /> | ||
| <Tooltip content={<CustomTooltip />} /> | ||
| <Legend /> | ||
| <Bar | ||
| dataKey='Amount' | ||
| fill='#7B5DD3FF' | ||
| onClick={(data) => { | ||
| const category = data.payload.Category; | ||
| const matchedLinkObject: ExpensesLinkItem | undefined = ExpensesLinkData.find( | ||
| (obj) => obj.category === category | ||
| ); | ||
|
|
||
| if (matchedLinkObject) { | ||
| window.open(matchedLinkObject.link, '_blank'); | ||
| } | ||
| }} | ||
| /> | ||
| </BarChart> | ||
| if (matchedLinkObject) { | ||
| window.open(matchedLinkObject.link, '_blank'); | ||
| } | ||
| }} | ||
| /> | ||
| </BarChart> | ||
| ) : ( | ||
| <div className='h-[400px] w-full flex items-center justify-center bg-gray-50 rounded-lg'> | ||
| Loading chart... | ||
| </div> | ||
| )} |
There was a problem hiding this comment.
Fixed width={800} will cause horizontal overflow on viewports narrower than 800px.
The hardcoded width means the chart won't adapt to smaller screens, causing a horizontal scrollbar or clipping. This replaces one layout problem (CLS) with another (overflow). Use recharts' ResponsiveContainer to make the chart fluid.
🔧 Proposed fix using ResponsiveContainer
Add ResponsiveContainer to the recharts import:
-import { Bar, BarChart, CartesianGrid, Legend, Tooltip, YAxis } from 'recharts';
+import { Bar, BarChart, CartesianGrid, Legend, ResponsiveContainer, Tooltip, YAxis } from 'recharts';Then replace the fixed-dimension chart:
- <div className='flex justify-center h-[400px]'>
+ <div className='h-[400px] w-full'>
{isMounted ? (
- <BarChart width={800} height={400} data={chartData}>
- <CartesianGrid strokeDasharray='3 3' />
- <YAxis tickFormatter={(value) => `$${value}`} />
- <Tooltip content={<CustomTooltip />} />
- <Legend />
- <Bar
- dataKey='Amount'
- fill='#7B5DD3FF'
- onClick={(data) => {
- const category = data.payload.Category;
- const matchedLinkObject: ExpensesLinkItem | undefined = ExpensesLinkData.find(
- (obj) => obj.category === category
- );
-
- if (matchedLinkObject) {
- window.open(matchedLinkObject.link, '_blank');
- }
- }}
- />
- </BarChart>
+ <ResponsiveContainer width='100%' height={400}>
+ <BarChart data={chartData}>
+ <CartesianGrid strokeDasharray='3 3' />
+ <YAxis tickFormatter={(value) => `$${value}`} />
+ <Tooltip content={<CustomTooltip />} />
+ <Legend />
+ <Bar
+ dataKey='Amount'
+ fill='#7B5DD3FF'
+ onClick={(data) => {
+ const category = data.payload.Category;
+ const matchedLinkObject: ExpensesLinkItem | undefined = ExpensesLinkData.find(
+ (obj) => obj.category === category
+ );
+
+ if (matchedLinkObject) {
+ window.open(matchedLinkObject.link, '_blank');
+ }
+ }}
+ />
+ </BarChart>
+ </ResponsiveContainer>
) : (ResponsiveContainer handles resize internally via its own ResizeObserver, and since it's rendered only after mount (behind the isMounted guard), it won't cause hydration mismatches.
🤖 Prompt for AI Agents
In `@components/FinancialSummary/BarChartComponent.tsx` around lines 92 - 118, The
BarChart is using a hardcoded width (width={800}) which causes horizontal
overflow on small viewports; modify BarChartComponent to import
ResponsiveContainer from 'recharts' and wrap the <BarChart ...> inside
<ResponsiveContainer width="100%" height="100%"> (or height={400} on the
container) and remove the fixed width/height props from <BarChart>, preserving
existing props like data={chartData}, <CartesianGrid/>, <YAxis/>, <Tooltip
content={<CustomTooltip/>}/>, <Legend/> and the <Bar dataKey='Amount' ... />
click handler so the chart becomes fluid while still guarded by the isMounted
check.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5118 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 796 796
Branches 146 146
=========================================
Hits 796 796 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Just following up on the PR I raised. |
2 participants
Fixes #5114. Removed unstable resize listener in finance page and moved layout logic to CSS to prevent memory leaks and CLS. Also addressed a hydration mismatch in BarChartComponent.
Summary by CodeRabbit
Refactor
UI