Migrate recharts from v2 to v3#585
Conversation
📝 WalkthroughWalkthroughUpdated Recharts to v3 and migrated chart components: tooltip/hover wiring in nitrogen chart, color token syntax in charts, chart utilities adjustments, minor Card style change, and package dependency bump to Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ic to a single bar
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
fdm-app/app/components/blocks/balance/nitrogen-chart.tsx (2)
54-55: Type-laundering cast to satisfyRecord<string, number | undefined>.
name: "Balans" as unknown as undefinedintentionally inserts a string into a record typed as numeric, relying on the double cast to silence TypeScript. This leaks through tochartData[name]on line 574, where the tooltip renders"Balans"(string) followed by the unit — harmless today only because the "name" bar never receives hover focus, but it is a latent bug waiting for future tooltip changes.Consider widening the type instead of laundering the value:
♻️ Suggested refactor
- const chartData: Record<string, number | undefined> = { - name: "Balans" as unknown as undefined, // Needed for chart to render at y-axis points called "Balans" - see the JSX below + const chartData: Record<string, number | string | undefined> = { + name: "Balans", // YAxis category label; see JSX below…and narrow back to
numberat the tooltip render site (e.g.typeof chartData[name] === "number" ? chartData[name].toLocaleString() : null).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/balance/nitrogen-chart.tsx` around lines 54 - 55, The chartData object is currently type‑laundered via name: "Balans" as unknown as undefined which injects a string into a numeric record; change the chartData type to allow string labels (e.g. Record<string, number | string | undefined> or introduce a separate label key) and remove the cast on name so the literal "Balans" is stored with the correct type, then at the tooltip render site (where chartData[name] is read, referenced around the tooltip logic near line 574) narrow the value with a runtime check like typeof chartData[name] === "number" ? formatNumber(...) : null (or skip rendering the numeric unit) so strings never get formatted as numbers in the tooltip.
490-512: Consider pairingonMouseOverwithonMouseOutper-bar for reliable outline clearing.Outline focus is set on
<Bar onMouseOver>and cleared only on the outerChartContainer'sonMouseLeave. When the cursor moves between bars quickly, this works (each bar sets a new focus), but moving off a bar onto the chart's padding — while still insideChartContainer— leaves the previous bar outlined until the cursor fully exits. AddingonMouseOut={() => setBarOutlineFocus("")}on each<Bar>(or usingonMouseLeave) eliminates the stale outline.Also: the clear-state is set to
""(empty string) on line 518 while the state type isstring | number | undefined— preferundefinedfor consistency with the initial value sobarOutlineFocus && ...guards read uniformly.♻️ Suggested refactor
- onMouseOver={() => setBarOutlineFocus(dataKey)} + onMouseOver={() => setBarOutlineFocus(dataKey)} + onMouseOut={() => setBarOutlineFocus(undefined)} @@ - onMouseLeave={() => { - setBarOutlineFocus("") - }} + onMouseLeave={() => setBarOutlineFocus(undefined)}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/balance/nitrogen-chart.tsx` around lines 490 - 512, The bar outline focus is only set via onMouseOver on each Bar (rendered in the map) and cleared on the outer ChartContainer, which leaves a stale outline when moving into chart padding; update each Bar to also call onMouseOut (or onMouseLeave) with setBarOutlineFocus(undefined) to clear focus immediately, and change the ChartContainer clear call (currently setting "") to setBarOutlineFocus(undefined) so the state type (string | number | undefined) is consistent and the barOutlineFocus checks behave uniformly; target the Bar elements and the setBarOutlineFocus usage to implement these changes.fdm-app/app/components/ui/chart.tsx (1)
176-189:nestLabelis computed against the unfiltered payload.
const nestLabel = payload.length === 1 && indicator !== "dot"is evaluated before thefilter((item) => item.type !== "none")on line 189. If the original payload has more than one entry but only one remains after filtering (a common case since Recharts v3 now emits extratype: "none"entries for hidden series/cursors), the tooltip will render in "flat" mode instead of the nested single-item mode, and vice-versa.Consider filtering once and deriving
nestLabelfrom the filtered result:♻️ Suggested refactor
- if (!active || !payload?.length) { + const visiblePayload = payload?.filter((item) => item.type !== "none") + if (!active || !visiblePayload?.length) { return null } - const nestLabel = payload.length === 1 && indicator !== "dot" + const nestLabel = visiblePayload.length === 1 && indicator !== "dot" @@ - {payload - .filter((item) => item.type !== "none") - .map((item, index) => { + {visiblePayload.map((item, index) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/ui/chart.tsx` around lines 176 - 189, The computed nestLabel uses the raw payload before filtering which causes incorrect layout when Recharts emits type: "none" entries; change the component to first compute a filteredPayload = payload.filter(item => item.type !== "none') (or similar) and then set const nestLabel = filteredPayload.length === 1 && indicator !== "dot', and use filteredPayload in the subsequent map/filter logic and rendering instead of the original payload so the single-item nested tooltip mode reflects the post-filtered data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fdm-app/app/components/blocks/balance/nitrogen-chart.tsx`:
- Around line 540-545: The custom tooltip content passed to the recharts
component currently assumes payload is always defined and uses payload.length
and payload[0], which can throw when payload is undefined; update the content
render prop (the content={({ payload }) => { ... }} block) to first guard for
payload being undefined or null and return null, then check payload.length ===
0, and when reading payload[0] ensure it exists before accessing its name; also
account for upstream filtering (e.g., ChartTooltipContent may remove entries
with item.type === "none") so rely on safe existence checks and not on
payload[0] always corresponding to the expected item before looking up
chartConfig[name].
In `@fdm-app/app/components/ui/chart.tsx`:
- Around line 271-275: The tooltip currently hides legitimate zero values
because it uses a truthy check on item.value; update the conditional rendering
around {item.value && (...)} in the chart component so it only excludes
null/undefined (e.g., use item.value !== undefined && item.value !== null or a
helper isNil check) ensuring 0 (and other valid falsy numbers) still render
inside the <span> showing item.value.toLocaleString().
- Around line 224-250: The Tailwind classes in the div's className (e.g.,
"border-[--color-border] bg-[--color-bg]") are invalid for Tailwind v4 and
prevent CSS variables from applying; update those classes to use a valid v4
arbitrary CSS-variable form—either replace with the var() bracket form
(border-[var(--color-border)] bg-[var(--color-bg)]) or the v4 parentheses
shorthand (border-(--color-border) bg-(--color-bg)) so the inline style's
--color-bg / --color-border set by indicatorColor take effect; keep the rest of
the conditional class logic (indicator/dashed/line/dot, nestLabel) unchanged.
---
Nitpick comments:
In `@fdm-app/app/components/blocks/balance/nitrogen-chart.tsx`:
- Around line 54-55: The chartData object is currently type‑laundered via name:
"Balans" as unknown as undefined which injects a string into a numeric record;
change the chartData type to allow string labels (e.g. Record<string, number |
string | undefined> or introduce a separate label key) and remove the cast on
name so the literal "Balans" is stored with the correct type, then at the
tooltip render site (where chartData[name] is read, referenced around the
tooltip logic near line 574) narrow the value with a runtime check like typeof
chartData[name] === "number" ? formatNumber(...) : null (or skip rendering the
numeric unit) so strings never get formatted as numbers in the tooltip.
- Around line 490-512: The bar outline focus is only set via onMouseOver on each
Bar (rendered in the map) and cleared on the outer ChartContainer, which leaves
a stale outline when moving into chart padding; update each Bar to also call
onMouseOut (or onMouseLeave) with setBarOutlineFocus(undefined) to clear focus
immediately, and change the ChartContainer clear call (currently setting "") to
setBarOutlineFocus(undefined) so the state type (string | number | undefined) is
consistent and the barOutlineFocus checks behave uniformly; target the Bar
elements and the setBarOutlineFocus usage to implement these changes.
In `@fdm-app/app/components/ui/chart.tsx`:
- Around line 176-189: The computed nestLabel uses the raw payload before
filtering which causes incorrect layout when Recharts emits type: "none"
entries; change the component to first compute a filteredPayload =
payload.filter(item => item.type !== "none') (or similar) and then set const
nestLabel = filteredPayload.length === 1 && indicator !== "dot', and use
filteredPayload in the subsequent map/filter logic and rendering instead of the
original payload so the single-item nested tooltip mode reflects the
post-filtered data.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 467ba567-f444-4045-8e86-c2707803588f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
fdm-app/app/components/blocks/balance/nitrogen-chart.tsxfdm-app/app/components/blocks/balance/organic-matter-chart.tsxfdm-app/app/components/ui/card.tsxfdm-app/app/components/ui/chart.tsxfdm-app/package.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
fdm-app/app/components/blocks/balance/nitrogen-chart.tsx (1)
550-556:⚠️ Potential issue | 🔴 CriticalGuard against
undefinedpayload before calling array methods.In Recharts v3, the
payloadparameter can beundefined(not just an empty array) during initial render and while the chart is inactive. Callingpayload.find(...)onundefinedwill throwTypeError: Cannot read properties of undefined (reading 'find').🐛 Proposed fix
shared={false} content={({ payload }) => { + if (!payload) return null const validPayloadItem = payload.find( (item) => item && item.type !== "none", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fdm-app/app/components/blocks/balance/nitrogen-chart.tsx` around lines 550 - 556, The tooltip renderer in nitrogen-chart.tsx assumes payload is always an array; guard against payload being undefined before calling array methods: in the content={({ payload }) => { ... }} handler, check that payload is an array (e.g., if (!payload || !Array.isArray(payload)) return null) before using payload.find and accessing its items (references: the content prop callback, the payload variable, and validPayloadItem/name extraction). Ensure the early return prevents any further payload processing when payload is undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fdm-app/app/components/blocks/balance/nitrogen-chart.tsx`:
- Around line 550-556: The tooltip renderer in nitrogen-chart.tsx assumes
payload is always an array; guard against payload being undefined before calling
array methods: in the content={({ payload }) => { ... }} handler, check that
payload is an array (e.g., if (!payload || !Array.isArray(payload)) return null)
before using payload.find and accessing its items (references: the content prop
callback, the payload variable, and validPayloadItem/name extraction). Ensure
the early return prevents any further payload processing when payload is
undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3f5818dc-d863-40da-8db1-3ff5ff5af8c6
📒 Files selected for processing (2)
fdm-app/app/components/blocks/balance/nitrogen-chart.tsxfdm-app/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- fdm-app/package.json
2 participants
Chores
Closes #581
Summary by CodeRabbit
Style
Chores