Plot React Component #612
Plot React Component #612
Conversation
WalkthroughA new Plot component system is introduced for rendering interactive line charts using Recharts. It includes core components (Plot and Series), comprehensive type definitions, configuration objects, data handling utilities, styling, and documentation. Changes
Sequence DiagramsequenceDiagram
participant User as User Code
participant Plot as Plot Component
participant Series as Series Children
participant DataLoad as useSeriesData Hook
participant Tools as Data Tools
participant Recharts as Recharts
User->>Plot: Render Plot with fn/lookup ± Series children
Plot->>Plot: Compute effectiveChildren<br/>(provided or implicit Series)
Plot->>DataLoad: useSeriesData(effectiveChildren, bounds)
DataLoad->>Series: Identify Series elements
loop For each Series
DataLoad->>Tools: loadLookup(url) or<br/>sampleFunction(fn, bounds)
Tools->>Tools: Parse/normalize data
Tools-->>DataLoad: DataPoint[]
DataLoad->>DataLoad: Merge with Series props<br/>(id, label, color, etc.)
end
DataLoad-->>Plot: SeriesWithData[] (+ loading/error state)
alt Loading State
Plot->>Recharts: Render loading message
else Error State
Plot->>Recharts: Render error message
else Data Ready
Plot->>Plot: calculateDomain(series, bounds)
Plot->>Plot: renderSeriesLines(series)<br/>→ Line components
Plot->>Recharts: Render LineChart<br/>with axes, grid, lines, legend
Recharts-->>User: Interactive chart
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
|
Preview URL: https://pr612.betaflight-com.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/components/Plot/Tools.tsx (2)
53-67: Array input is cast without validation.Line 54 casts
rawdirectly toDataPoint[]without verifying that array elements actually havexandynumeric properties. Malformed JSON data would pass through and potentially cause downstream rendering issues.🔎 Proposed fix with validation
export function normalizeData(raw: unknown): DataPoint[] { - if (Array.isArray(raw)) return raw as DataPoint[]; + if (Array.isArray(raw)) { + return raw.filter( + (p): p is DataPoint => + p != null && + typeof p === 'object' && + typeof (p as DataPoint).x === 'number' && + typeof (p as DataPoint).y === 'number' + ); + } if (raw && typeof raw === 'object') {
93-98: Spread operator inMath.min/maxmay fail on very large datasets.Using
Math.min(...xs)andMath.max(...xs)can exceed the call stack limit whenxscontains many elements (typically >100k). For large CSV files loaded vialookup, this could throw aRangeError.🔎 Proposed fix using reduce
export function calculateDomain(series: SeriesWithData[], xmin: number, xmax: number): AxisDomain { const xs = series.flatMap((s) => s.data.map((p) => p.x)); - const domainMin = xs.length ? Math.min(...xs) : xmin; - const domainMax = xs.length ? Math.max(...xs) : xmax; + const domainMin = xs.length ? xs.reduce((a, b) => Math.min(a, b), xs[0]) : xmin; + const domainMax = xs.length ? xs.reduce((a, b) => Math.max(a, b), xs[0]) : xmax; return [domainMin, domainMax]; }src/components/Plot/Plot.module.css (2)
36-50: Loading state background doesn't adapt to dark theme.The
.loadingclass usesbackground: rgba(0, 0, 0, 0.04)which works for light theme but will be nearly invisible on dark backgrounds. Consider using a theme-aware variable similar to--plot-grid-fill.🔎 Proposed fix
.loading { color: inherit; display: flex; align-items: center; justify-content: center; min-height: 350px; - background: rgba(0, 0, 0, 0.04); + background: var(--plot-grid-fill); border: 1px dashed var(--ifm-color-emphasis-300);
21-34: Error state styling may have poor contrast in dark theme.The
.errorclass uses hardcodedcolor: #c62828andbackground: rgba(198, 40, 40, 0.08)which may not provide sufficient contrast on dark backgrounds. Consider using Infima's danger color variables for consistency.🔎 Proposed fix using theme variables
.error { display: flex; align-items: center; justify-content: center; min-height: 350px; - color: #c62828; - background: rgba(198, 40, 40, 0.08); - border: 1px solid rgba(198, 40, 40, 0.35); + color: var(--ifm-color-danger-dark); + background: var(--ifm-color-danger-contrast-background); + border: 1px solid var(--ifm-color-danger-dark);src/components/Plot/Series.tsx (1)
27-49: Consider validating data source presence.If neither
fnnorlookupis provided, the function returns a series with an emptydataarray. This could lead to confusing empty plots without any indication of misconfiguration.🔎 Suggested validation
async function buildSeriesData(props: SeriesProps, idx: number, n: number, xmin: number, xmax: number, samples: number): Promise<SeriesWithData> { const id = props.id || (n === 1 ? 'y' : `y-${idx}`); const min = props.xmin ?? xmin; const max = props.xmax ?? xmax; const resolvedSamples = props.samples ?? samples; let data: DataPoint[] = []; if (props.lookup) { data = await loadLookup(props.lookup); } else if (props.fn) { data = sampleFunction(props.fn, min, max, resolvedSamples); + } else { + throw new Error(`Series "${id}" requires either 'fn' or 'lookup' prop`); } return { id, label: props.label || id, color: props.color, strokeWidth: props.strokeWidth, strokeDasharray: props.strokeDasharray, dot: props.dot, data, }; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonstatic/data/example_noise.csvis excluded by!**/*.csv
📒 Files selected for processing (11)
docs/development/Plot-Component.mdx(1 hunks)docs/development/mdx-reference.mdx(2 hunks)package.json(1 hunks)src/components/Plot/Config.tsx(1 hunks)src/components/Plot/Plot.module.css(1 hunks)src/components/Plot/Plot.tsx(1 hunks)src/components/Plot/Series.tsx(1 hunks)src/components/Plot/Tools.tsx(1 hunks)src/components/Plot/Types.tsx(1 hunks)src/components/Plot/index.tsx(1 hunks)src/css/custom.css(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
src/components/Plot/Config.tsx (2)
src/components/Plot/Tools.tsx (1)
formatSignificant(11-16)src/components/Plot/index.tsx (1)
SERIES_COLORS(32-32)
src/components/Plot/Plot.tsx (4)
src/components/Plot/Types.tsx (1)
PlotProps(53-53)src/components/Plot/index.tsx (2)
PlotProps(25-25)Series(53-53)src/components/Plot/Series.tsx (3)
Series(13-15)useSeriesData(56-99)renderSeriesLines(104-117)src/components/Plot/Tools.tsx (1)
calculateDomain(93-98)
src/components/Plot/Tools.tsx (1)
src/components/Plot/Types.tsx (2)
DataPoint(7-10)SeriesWithData(55-58)
⏰ 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). (1)
- GitHub Check: deploy
🔇 Additional comments (17)
src/components/Plot/Tools.tsx (2)
1-35: LGTM!The
formatSignificantandloadLookuputilities are well-implemented with proper edge case handling (non-finite numbers, HTTP errors, JSON/CSV fallback logic).
69-87: LGTM!The CSV parser handles multiple separator formats and gracefully skips invalid lines, which is appropriate for flexible data loading.
src/css/custom.css (2)
40-42: LGTM!The new
--plot-grid-fillvariable follows the existing theming pattern and provides appropriate subtle values for light/dark modes.
55-57: LGTM!Dark theme override is correctly scoped and uses an appropriate contrasting value.
src/components/Plot/Plot.module.css (1)
1-19: LGTM!The color palette and container styling are well-structured. The container uses theme-aware variables appropriately.
docs/development/mdx-reference.mdx (2)
7-7: LGTM!The import follows the existing pattern for client-side components in this file.
208-218: LGTM!The new Plots section provides a clean introduction with a working example and appropriate link to the full documentation.
src/components/Plot/Config.tsx (1)
1-144: LGTM!The configuration is well-organized with clear separation of concerns:
- Uses CSS variables for theme-aware styling
- Properly delegates to
formatSignificantfor consistent number formatting- The aggregated
PLOT_PROPSexport provides a clean API for consumerssrc/components/Plot/index.tsx (1)
1-53: LGTM!The entry point is well-designed:
- The
Object.assignpattern for compound components is idiomatic React- Dual export strategy (
Plot.Seriesand standaloneSeries) provides flexibility- Type and constant re-exports maintain a clean public API surface
docs/development/Plot-Component.mdx (1)
1-255: LGTM!Excellent documentation with:
- Progressive examples from basic to advanced usage
- Live, runnable demonstrations paired with code snippets
- Complete property reference table
- Practical tips for production use
The exported helper functions (
f,df,g) provide reusable examples for the grid layout demonstrations.package.json (1)
51-51: Add recharts for charting components.The addition of
"recharts": "^3.6.0"is appropriate. Version 3.6.0 is the latest version, published 4 days ago, and the latest non-vulnerable version is 3.6.0. The caret notation allows for compatible patch and minor updates within the v3 range.src/components/Plot/Types.tsx (1)
1-58: LGTM! Well-structured type definitions.The type system is clean and well-documented. The composition pattern using intersection types (SeriesProps, PlotProps) provides a flexible API while maintaining type safety.
src/components/Plot/Plot.tsx (2)
43-64: LGTM! Implicit Series wrapping is well-implemented.The logic correctly handles the dual-mode API (direct Plot props vs. explicit Series children) with proper memoization.
73-96: LGTM! Chart rendering is well-structured.The rendering logic correctly handles single and multi-series scenarios with appropriate conditional display of Legend and Lines.
src/components/Plot/Series.tsx (3)
13-22: LGTM! Marker component pattern is correctly implemented.The Series marker component and type guard follow React best practices for detecting and filtering specific child types.
56-99: LGTM! Robust async data loading with proper cleanup.The hook correctly handles parallel data loading, manages loading/error states, and prevents memory leaks with cancellation on unmount.
104-117: LGTM! Series rendering logic is clean.The function correctly maps series data to Recharts Line components with proper configuration merging and color cycling.
| if (loading) return <div className={css.loading}>Loading plot…</div>; | ||
| if (error) return <div className={css.error}>Error…</div>; |
There was a problem hiding this comment.
Include error details in error message.
The error state displays a generic "Error…" message without showing the actual error details, making debugging difficult.
🔎 Proposed fix to display error details
- if (error) return <div className={css.error}>Error…</div>;
+ if (error) return <div className={css.error}>Error loading plot: {error.message}</div>;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Plot/Plot.tsx around lines 70-71 the UI returns a generic
error message; update the error branch to render the actual error details by
displaying a safe stringified error (e.g. use error?.message || String(error) or
JSON.stringify for objects) instead of the fixed "Error…"; ensure null/undefined
are handled and consider showing stack only in development (process.env.NODE_ENV
=== 'development') to avoid leaking internals in production.
| export function sampleFunction(fn: (x: number) => number, xmin: number, xmax: number, n: number): DataPoint[] { | ||
| const data: DataPoint[] = []; | ||
| const step = (xmax - xmin) / (n - 1); | ||
| for (let i = 0; i < n; i++) { | ||
| const x = xmin + i * step; | ||
| data.push({ x, y: fn(x) }); | ||
| } | ||
| return data; | ||
| } |
There was a problem hiding this comment.
Division by zero when n is 1.
When n === 1, the step calculation (xmax - xmin) / (n - 1) results in division by zero, producing Infinity. This causes the loop to produce a single point with x = xmin but subsequent usages expecting valid step behavior may fail.
🔎 Proposed fix
export function sampleFunction(fn: (x: number) => number, xmin: number, xmax: number, n: number): DataPoint[] {
+ if (n <= 0) return [];
+ if (n === 1) return [{ x: xmin, y: fn(xmin) }];
const data: DataPoint[] = [];
const step = (xmax - xmin) / (n - 1);
for (let i = 0; i < n; i++) {
const x = xmin + i * step;
data.push({ x, y: fn(x) });
}
return data;
}🤖 Prompt for AI Agents
In src/components/Plot/Tools.tsx around lines 39 to 47, the step calculation
divides by (n - 1) which yields Infinity when n === 1; add an early guard for
invalid or singleton sample counts: if n <= 1 return a single DataPoint [{ x:
xmin, y: fn(xmin) }]; otherwise compute step = (xmax - xmin) / (n - 1) and
proceed with the loop. Optionally normalize n to an integer (e.g., n =
Math.max(1, Math.floor(n))) before the guard.
haslinghuis
left a comment
haslinghuis
left a comment
There was a problem hiding this comment.
Nice, personal prefer smaller strokeWidth
2 participants
This pull request introduces a new, reusable
<Plot>React component for data visualizations in the documentation, powered by the Recharts library.It supports plotting analytic functions and data loading from CSV/JSON files.
More details are found in the documentation