Perf/measurement graph - #1075
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe measurement graph now uses normalized, typed chart data with Chart.js ChangesMeasurement chart flow
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The chart rewrite can currently produce incorrect user-visible results: aggregated CSV downloads may report the wrong values, hover interactions can align points from different timestamps, invalid-only data can render as an empty chart, and range shading can misrepresent the plotted band. These localized UI and data-correctness issues should be fixed or explicitly accepted before merging. Sequence Diagram(s)sequenceDiagram
participant ExploreRoute
participant Graph
participant MeasurementChart
participant Line
participant ChartJS
participant HoveredPointContext
ExploreRoute->>Graph: pass date bounds and sensor inputs
Graph->>MeasurementChart: createMeasurementChartData(sensor inputs)
MeasurementChart-->>Graph: return normalized chart data
Graph->>Line: render chart data and options
Line->>ChartJS: register zoom plugin and render chart
ChartJS->>Graph: report zoom state and tooltip point
Graph->>HoveredPointContext: update hovered point when not zooming
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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 |
Coverage Report
File CoverageNo changed files found. |
![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 (1)
app/components/device-detail/graph.tsx (1)
421-443: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe aggregated CSV export writes max values instead of the measured values.
createMeasurementChartDatainapp/lib/measurement-chart.tsline 126 returns[maxDataset, baseDataset, minDataset]for an aggregated single sensor. The CSV export is offered only for a single sensor (line 532), so this is the exact case that runs here.Two consequences:
- Line 422 reads timestamps from
chartData.datasets[0], which is the(Max)dataset.- Line 429-431 matches with
ds.label.includes(sensor.title). The(Max)label also containssensor.title, sofindreturns the max dataset first.The
valuecolumn then holds max values for every aggregated export.🐛 Proposed fix to select the mean dataset explicitly
function handleCsvDownloadClick() { - const labels = chartData.datasets[0].data.map((point: any) => point.x) + // Datasets can include "(Min)"/"(Max)" bands, so match the exact label. + const findValueDataset = (sensor: any) => + chartData.datasets.find( + (ds) => + ds.label === sensor.title || + ds.label === `${sensor.title} (${sensor.device_name})`, + ) + const labels = (findValueDataset(sensors[0])?.data ?? []).map( + (point) => point.x, + ) let csvContent = 'timestamp,deviceId,sensorId,value,unit,phenomena\n' // Loop through each timestamp and sensor data labels.forEach((timestamp: number, index: number) => { sensors.forEach((sensor: any) => { - const dataset = chartData.datasets.find( - (ds: { label: string | any[] }) => ds.label.includes(sensor.title), - ) + const dataset = findValueDataset(sensor) if (dataset) { - const value = (dataset.data as any)[index]?.y ?? '' + const value = dataset.data[index]?.y ?? '' csvContent += `${new Date(timestamp).toISOString()},`
🧹 Nitpick comments (2)
app/components/device-detail/graph.tsx (2)
188-202: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the redundant ref guard in the sync effect.
The effect dependency array is
[sensors, isAggregated]. React runs the effect only when one of those identities changes, which is the same condition thepreviousChartInputRefcomparison checks. The ref adds state to keep in sync without changing behavior.♻️ Proposed simplification
- useEffect(() => { - if ( - previousChartInputRef.current.sensors === sensors && - previousChartInputRef.current.isAggregated === isAggregated - ) { - return - } - - previousChartInputRef.current = { sensors, isAggregated } - setChartData(createMeasurementChartData(sensors, isAggregated)) - }, [sensors, isAggregated]) + useEffect(() => { + setChartData(createMeasurementChartData(sensors, isAggregated)) + }, [sensors, isAggregated])Also remove the
previousChartInputRefdeclaration at line 159.
303-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the narrowed
pointfor the label value.Line 315 reads
context.raw.ythrough theany-typedcontextafter line 309 already narrowed the same object toMeasurementChartPoint.♻️ Proposed cleanup
- return `${context.dataset.label}: ${context.raw.y}` + return `${context.dataset.label}: ${point.y}`
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 48c83a2b-093e-4e2e-9342-5941a9796236
📒 Files selected for processing (4)
app/components/device-detail/graph.tsxapp/lib/measurement-chart.tsapp/routes/explore.$deviceId.$sensorId.$.tsxapp/routes/explore.$deviceId.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/components/device-detail/graph.tsx (3)
199-200: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse normalized data for the empty-state decision.
createMeasurementChartDataremoves invalid points, but the render guard at Line [566-568] still checks rawsensors[*].data.length. If all input points are invalid, the component renders an empty graph instead ofno_data_in_range. DerivehasChartDatafrom the filtered datasets.Proposed fix
+ const hasChartData = chartData.datasets.some( + (dataset) => dataset.data.length > 0, + )
438-446: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSelect the base dataset for CSV export.
For one aggregated sensor,
createMeasurementChartDatareturns datasets inmax,base,minorder. CSV export remains enabled for this case at Line [543-546]. The label substring search therefore selects theMaxdataset and writesmax_valueunder thevaluecolumn. Select the base dataset by exact label or a stable dataset key.Proposed fix
const dataset = chartData.datasets.find( - (ds: { label: string | any[] }) => ds.label.includes(sensor.title), + (ds) => ds.label === sensor.title, )
222-225: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid index-based tooltip matching.
mode: 'index'can combine different timestamps when sensor datasets have missing points. Set both interaction modes tox. If exact timestamp matching is required, align datasets by timestamp or implement a custom interaction mode. Add a regression case with two sensors that have different timestamps.Source: MCP tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b85908ca-060a-41cc-96d2-311228b5102b
📒 Files selected for processing (1)
app/components/device-detail/graph.tsx
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
scheidtdav
left a comment
scheidtdav
left a comment
There was a problem hiding this comment.
Looking good to me.
One of code rabbits out of diff comments may be relevant though. We can also generate new issues for that:
In graph.tsxs handleCsvDownload the dataset with index 0 is used. Thats the dataset for "max" and not for base resulting in the download only containing the max values. This should probably be index 1 for the base?
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/lib/measurement-chart.ts (1)
150-173: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMin/max range bands still overlap instead of forming one continuous band.
Datasets are returned in order
[maxDataset, baseDataset, minDataset](indices 0, 1, 2). BothminDataset.fillandmaxDataset.fillare set to1, so both fill towardbaseDataset. This produces two separate shaded regions (max-to-base and min-to-base) instead of one continuous min-to-max band, and the regions can overlap. This was already flagged in a prior review round and remains unresolved in the current code.Set
maxDataset.fillto2(targetminDataset) andminDataset.filltofalse.🎨 Proposed fix for a single continuous band
const minDataset: MeasurementChartDataset = { ...baseDataset, label: `${label} (Min)`, data: createPoints( aggregateMeasurements, ({ minValue }) => minValue, false, ), borderColor: sensor.color + '33', backgroundColor: sensor.color + '33', - fill: 1, + fill: false, } const maxDataset: MeasurementChartDataset = { ...baseDataset, label: `${label} (Max)`, data: createPoints( aggregateMeasurements, ({ maxValue }) => maxValue, false, ), borderColor: sensor.color + '33', backgroundColor: sensor.color + '33', - fill: 1, + fill: 2, }app/components/device-detail/graph.tsx (1)
294-297: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDisable decimation for aggregated range datasets. Chart.js 4.5.1 decimates each dataset independently. Different
yvalues can produce different retained indices for[maxDataset, baseDataset, minDataset].interaction.mode: 'index'then combines points at the same array index, even when their timestamps differ. Disable decimation whenisAggregatedis true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58aa525e-4a80-478a-b10b-f5a18fa5cac5
📒 Files selected for processing (2)
app/components/device-detail/graph.tsxapp/lib/measurement-chart.ts
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Ah yes, index 0 is only correct if the graph only shows raw measurements as then we only have the base dataset. Will fix it! |
2 participants
Type of Change
Implementation
Checklist
devbranchAdditional Information