[WEB-4877] fix: webapp crash because of bar chart#7763
Conversation
|
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 centralized, safe color resolution for bars: introduces DEFAULT_BAR_FILL_COLOR, getBarColor and getAllBarColors callbacks, removes inline Bar fill assignment and stackDotColors derivation, and passes a per-key color map into Tooltip via itemDotColors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant BC as BarChart (root.tsx)
participant Bars as Bars[]
participant TT as Tooltip
rect rgb(245,249,255)
note right of BC: render -> derive stackKeys/stackLabels
end
U->>BC: render with data payload
BC->>BC: derive stackKeys & stackLabels (useMemo)
BC->>BC: getAllBarColors(payload)
loop for each barKey
BC->>BC: getBarColor(payload, barKey)
alt bar found and bar.fill is function
BC->>Bars: find bar by key
BC->>BC: find matching payload item
BC->>BC: color = safeCall(bar.fill, item.payload)
note right of BC: on error/missing → DEFAULT_BAR_FILL_COLOR
else bar.fill is value
BC->>BC: color = bar.fill
else bar not found
BC->>BC: color = DEFAULT_BAR_FILL_COLOR
end
end
BC->>TT: content({ itemDotColors: colorMap })
note over TT,BC: Tooltip uses provided per-key colors
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a runtime crash in the bar chart component by improving the bar color extraction logic. The fix addresses issues with function-based fill colors and ensures proper fallback handling.
- Replaced static color extraction with dynamic color resolution that handles function-based fills properly
- Added error handling and fallback colors to prevent crashes when color functions fail
- Removed direct function calls with empty objects that could cause runtime errors
Comments suppressed due to low confidence (1)
packages/propel/src/charts/bar-chart/root.tsx:1
- The
fillprop is being removed from the Bar component but no replacement is provided. This will likely cause the bars to render without any fill color since the dynamic color resolution ingetBarColoris only used for tooltips.
/* eslint-disable @typescript-eslint/no-explicit-any */
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![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
🧹 Nitpick comments (4)
packages/propel/src/charts/bar-chart/root.tsx (4)
24-25: Avoid hard-coded black; prefer theme token or configurable fallback.Using a design token (e.g., a CSS variable) or a prop-based override keeps charts consistent with theming and dark mode. Keeping "#000000" is fine as a fallback if a token isn’t available.
49-60: Tighten typing for keys/labels and simplify construction.Leverage generic K to keep types precise and reduce loop boilerplate.
- const { stackKeys, stackLabels } = useMemo(() => { - const keys: string[] = []; - const labels: Record<string, string> = {}; - - for (const bar of bars) { - keys.push(bar.key); - labels[bar.key] = bar.label; - } - - return { stackKeys: keys, stackLabels: labels }; - }, [bars]); + const { stackKeys, stackLabels } = useMemo(() => { + const keys = bars.map((b) => b.key) as K[]; + const labels = Object.fromEntries(bars.map((b) => [b.key, b.label])) as Record<K, string>; + return { stackKeys: keys, stackLabels: labels }; + }, [bars]);
61-85: Harden getBarColor: handle falsy strings, invalid returns, and quiet logs in prod.Prevents undefined/empty colors and avoids noisy console.error in production.
const getBarColor = useCallback( (payload: any[], barKey: string) => { const bar = bars.find((b) => b.key === barKey); if (!bar) return DEFAULT_BAR_FILL_COLOR; if (typeof bar.fill === "function") { - const payloadItem = payload?.find((item) => item.dataKey === barKey); - if (payloadItem?.payload) { - try { - return bar.fill(payloadItem.payload); - } catch (error) { - console.error(error); - return DEFAULT_BAR_FILL_COLOR; - } - } else { - return DEFAULT_BAR_FILL_COLOR; // fallback color when no payload data - } + const payloadItem = Array.isArray(payload) ? payload.find((item) => item.dataKey === barKey) : undefined; + let color: unknown; + if (payloadItem?.payload) { + try { + color = bar.fill(payloadItem.payload); + } catch (error) { + if (process.env.NODE_ENV !== "production") { + // eslint-disable-next-line no-console + console.warn(`[BarChart] bar.fill threw for key "${barKey}"`, error); + } + } + } + return typeof color === "string" && color ? color : DEFAULT_BAR_FILL_COLOR; } else { - return bar.fill; + const color = bar.fill as string | undefined; + return color && color.length > 0 ? color : DEFAULT_BAR_FILL_COLOR; } }, [bars] );
86-97: Minor perf: avoid O(nBars × nPayload) lookup.Today you scan payload for each bar via getBarColor. Not a big deal for small charts, but you can pre-index payload by dataKey once per tooltip render and reuse it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/propel/src/charts/bar-chart/root.tsx(4 hunks)
⏰ 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: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
packages/propel/src/charts/bar-chart/root.tsx (2)
4-4: LGTM: import changes are appropriate.useCallback addition matches new helpers.
196-197: CustomTooltip prop contract verified. itemDotColors is correctly implemented across all chart components and the CustomTooltip interface no longer references stackDotColors; no external tooltips require stackDotColors.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/propel/src/charts/bar-chart/root.tsx (1)
86-97: Replaceany[]with typed Recharts payload and tighten return typeKeeps tooltip interop sound and avoids accidental shape mistakes.
- const getAllBarColors = useCallback( - (payload: any[]) => { - const colors: Record<string, string> = {}; + const getAllBarColors = useCallback( + (payload: ReadonlyArray<TooltipPayload<number | string, string>> | undefined) => { + const colors: Record<string, string> = {}; for (const bar of bars) { colors[bar.key] = getBarColor(payload, bar.key); } return colors; }, [bars, getBarColor] );
🧹 Nitpick comments (4)
packages/propel/src/charts/bar-chart/root.tsx (4)
4-14: Type tooltip payloads to remove implicit anys and tighten signaturesImport Recharts’ Payload type for use in getBarColor/getAllBarColors.
import React, { useCallback, useMemo, useState } from "react"; import { BarChart as CoreBarChart, Bar, ResponsiveContainer, Tooltip, XAxis, YAxis, Legend, CartesianGrid, } from "recharts"; +import type { Payload as TooltipPayload } from "recharts/types/component/DefaultTooltipContent";
24-24: Prefer theme token over hardcoded blackUse a design-token fallback so dots match the app theme in light/dark modes.
-const DEFAULT_BAR_FILL_COLOR = "#000000"; +const DEFAULT_BAR_FILL_COLOR = "rgba(var(--color-text-100), 1)";
61-85: Harden color resolution + add types and safer fallbacks
- Strongly type payload.
- Guard when bar.fill is falsy or throws.
- Add contextual logging (dev-only).
- const getBarColor = useCallback( - (payload: Record<string, string>[], barKey: string) => { + const getBarColor = useCallback( + (payload: ReadonlyArray<TooltipPayload<number | string, string>> | undefined, barKey: string): string => { const bar = bars.find((b) => b.key === barKey); if (!bar) return DEFAULT_BAR_FILL_COLOR; if (typeof bar.fill === "function") { - const payloadItem = payload?.find((item) => item.dataKey === barKey); - if (payloadItem?.payload) { + const payloadItem = payload?.find((item) => item.dataKey === barKey); + if (payloadItem?.payload) { try { return bar.fill(payloadItem.payload); } catch (error) { - console.error(error); + if (process.env.NODE_ENV !== "production") { + // eslint-disable-next-line no-console + console.error(`[BarChart] bar.fill failed for key "${barKey}"`, error); + } return DEFAULT_BAR_FILL_COLOR; } } else { return DEFAULT_BAR_FILL_COLOR; // fallback color when no payload data } } else { - return bar.fill; + return bar.fill ?? DEFAULT_BAR_FILL_COLOR; } }, [bars] );
196-196: Avoid allocating an empty array; let helper handle undefinedgetAllBarColors already guards undefined payload.
- itemDotColors={getAllBarColors(payload || [])} + itemDotColors={getAllBarColors(payload)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/propel/src/charts/bar-chart/root.tsx(4 hunks)
⏰ 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: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
packages/propel/src/charts/bar-chart/root.tsx (1)
49-60: LGTM: derived keys/labels memoClean and efficient; dependencies are correct.
JayashTripathy
changed the title
* 🔧 fix: dynamic bar color handling and refactored color retrieval logic. * ♻️ refactor: updated any to Record in getBarColor
Description
Fixed app runtime crashing by updating the bar color extraction logic in propel
Type of Change
Summary by CodeRabbit
New Features
Bug Fixes