feat(#178): Responsiveness & interactivity for PI Dashboard#180
feat(#178): Responsiveness & interactivity for PI Dashboard#180jimmysway wants to merge 3 commits intonerc-project:mainfrom
Conversation
jimmysway
force-pushed
the
issue-178-responsive-interactive
branch
2 times, most recently
from
9398e38 to
9126988
Compare
Builds on nerc-project#176 to add interactive and responsive features: - Add custom external tooltip handler with missing data indicators - Add responsive tick intervals and font sizes based on viewport - Add low-data state handling with centered display - Add missing data interpolation and grayscale visualization - Add graph/table toggle view - Add cumulative/daily data mode toggle - Add CSV export functionality - Add DataTable integration for table view - Add chart_tooltip.css and chart_controls.css styling Files: - patches/04_add_usage_chart.patch - Template modifications - src/static/chartjs/ - Chart.js library - src/static/css/ - Chart styling (controls, tooltips, variables) - src/static/js/ - Chart logic (utils, config, tooltip, allocation_detail) Closes nerc-project#178
jimmysway
force-pushed
the
issue-178-responsive-interactive
branch
from
9126988 to
8f89ba0
Compare
Snapshots are needed as a baseline comparison for tests. Tests cover UI states for different viewport sizes along with changes in data states Unit tests cover handling of chart logic and calculations
jimmysway
force-pushed
the
issue-178-responsive-interactive
branch
from
3223035 to
c6787c2
Compare
|
Added tests for the frontend. I do apologize for the absolutely monumental size of changes |
|
I'll have Copilot do a first pass at this and I will do a review after it and review the comments provided by Copilot as well. |
There was a problem hiding this comment.
Pull request overview
Adds an interactive, responsive SU usage visualization to the allocation detail “PI Dashboard” UI (Chart.js chart + table view), including missing-data handling and a new frontend test harness (unit + Playwright visual regression) to keep the behavior stable across breakpoints.
Changes:
- Introduces Chart.js-based usage chart utilities/config/tooltip logic with responsive ticks/fonts, low-/no-data layouts, missing-data interpolation, and grayscale visualization.
- Adds chart controls (graph/table toggle, cumulative/daily toggle, CSV export) and DataTables-backed table rendering on the allocation detail page.
- Adds a frontend test setup: Vitest unit tests for chart utils and Playwright visual tests with committed snapshot baselines + CI workflow.
Reviewed changes
Copilot reviewed 17 out of 35 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/static/vitest.config.js | Vitest config for running unit tests in jsdom. |
| src/static/package.json | Frontend test scripts and devDependencies (Vitest/JSDOM/Playwright). |
| src/static/playwright.config.js | Playwright config for visual regression tests and snapshot locations. |
| src/static/README.md | Instructions for running unit + visual frontend tests. |
| src/static/js/chart_utils.js | Core chart utilities: interpolation, responsive ticks/fonts, centering, gradient fill, missing-data grayscale plugin. |
| src/static/js/chart_tooltip.js | External tooltip handler with missing-data indicators. |
| src/static/js/chart_config.js | Centralized dataset palette + chart styling constants. |
| src/static/js/allocation_detail.js | Allocation detail chart/table rendering, toggles, CSV export, DataTables integration. |
| src/static/css/chart_controls.css | Styling for chart controls and DataTables pagination + responsive adjustments. |
| src/static/css/chart_tooltip.css | Styling for the external tooltip, including responsive breakpoints. |
| src/static/css/chart_variables.css | CSS custom properties for chart colors/spacing/typography. |
| src/static/tests/unit/chart-utils.test.js | Unit tests covering interpolation, daily deltas, centering helpers, responsive sizing. |
| src/static/tests/visual/chart.spec.js | Breakpoint-based visual tests + a functional daily-delta check. |
| src/static/tests/fixtures/chart_test.html | HTML fixture used by Playwright to render the chart for screenshots. |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/regular-data-desktop-chromium-linux.png | Visual baseline snapshot (regular data, desktop). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/regular-data-tablet-chromium-linux.png | Visual baseline snapshot (regular data, tablet). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/regular-data-mobile-chromium-linux.png | Visual baseline snapshot (regular data, mobile). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/regular-data-narrow-chromium-linux.png | Visual baseline snapshot (regular data, narrow). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/no-data-desktop-chromium-linux.png | Visual baseline snapshot (no data, desktop). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/no-data-tablet-chromium-linux.png | Visual baseline snapshot (no data, tablet). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/no-data-mobile-chromium-linux.png | Visual baseline snapshot (no data, mobile). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/no-data-narrow-chromium-linux.png | Visual baseline snapshot (no data, narrow). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/low-data-desktop-chromium-linux.png | Visual baseline snapshot (low data, desktop). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/low-data-tablet-chromium-linux.png | Visual baseline snapshot (low data, tablet). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/low-data-mobile-chromium-linux.png | Visual baseline snapshot (low data, mobile). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/low-data-narrow-chromium-linux.png | Visual baseline snapshot (low data, narrow). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/missing-data-desktop-chromium-linux.png | Visual baseline snapshot (missing data, desktop). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/missing-data-tablet-chromium-linux.png | Visual baseline snapshot (missing data, tablet). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/missing-data-mobile-chromium-linux.png | Visual baseline snapshot (missing data, mobile). |
| src/static/tests/visual/snapshots/chart.spec.js-snapshots/missing-data-narrow-chromium-linux.png | Visual baseline snapshot (missing data, narrow). |
| .github/workflows/frontend-tests.yaml | CI workflow to run unit + visual tests and upload artifacts. |
| patches/04_add_usage_chart.patch | Patch adding the SU costs card + loading chart JS/CSS into the Coldfront template. |
| Dockerfile | Applies the new patch during build and copies new static assets into Coldfront’s static tree. |
Files not reviewed (1)
- src/static/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/static/js/allocation_detail.js
Outdated
| options: { | ||
| responsive: true, | ||
| maintainAspectRatio: false, | ||
| devicePixelRatio: Math.max(window.devicePixelRatio || 1, 3), |
There was a problem hiding this comment.
devicePixelRatio is forced to be at least 3. This can significantly increase canvas resolution (and therefore CPU/GPU + memory cost) on many devices, especially when combined with expensive per-frame work like getImageData/putImageData in the grayscale plugin.
Suggestion: avoid increasing DPR above the browser default (or cap it more conservatively), and consider enabling higher DPR only when needed (e.g., for screenshot tests).
| devicePixelRatio: Math.max(window.devicePixelRatio || 1, 3), | |
| devicePixelRatio: window.devicePixelRatio || 1, |
patches/04_add_usage_chart.patch
Outdated
| </div> | ||
|
|
||
| +<!-- NERC Usage Chart --> | ||
| +<div class="card mb-3"> |
There was a problem hiding this comment.
The production template markup added in this patch does not give the SU costs card an id, but the JS (allocation_detail.js) expects an element with id="su-costs-card" (used for hide/show logic and tests). As-is, the card will not be hidden in the real page when there is no data, and the production DOM will diverge from the test fixture.
Suggestion: add id="su-costs-card" to the outer card element (or update the JS to target the actual element you render).
| +<div class="card mb-3"> | |
| +<div id="su-costs-card" class="card mb-3"> |
src/static/js/chart_tooltip.js
Outdated
| const toGrayscale = ChartUtils.toGrayscale; | ||
|
|
||
| const getOrCreateTooltip = (chart) => { | ||
| let tooltipEl = chart.canvas.parentNode.querySelector('div'); |
There was a problem hiding this comment.
getOrCreateTooltip() uses chart.canvas.parentNode.querySelector('div'), which will return the first div under the parent and may not be the tooltip element (especially since the chart is inside a div container). This can cause the handler to overwrite unrelated DOM or fail to find the tooltip reliably.
Suggestion: query specifically for the tooltip element (e.g., by #chartjs-tooltip) and only create it if it doesn't exist.
| let tooltipEl = chart.canvas.parentNode.querySelector('div'); | |
| let tooltipEl = chart.canvas.parentNode.querySelector('#chartjs-tooltip'); |
| bodyLines.forEach((body, i) => { | ||
| const dataset = chart.data.datasets[i]; | ||
| const dataPoint = tooltip.dataPoints[i]; | ||
|
|
||
| if (!dataPoint) return; | ||
|
|
||
| // Check if this data point is missing/interpolated | ||
| const dataIndex = dataPoint.dataIndex; | ||
| const missingIndices = dataset.missingIndices || new Set(); | ||
| const isMissing = missingIndices.has(dataIndex); |
There was a problem hiding this comment.
The tooltip rendering loop assumes tooltip.body/tooltip.dataPoints align 1:1 with chart.data.datasets by index (dataset = chart.data.datasets[i]). In Chart.js, tooltip.dataPoints can be filtered/reordered (e.g., hidden datasets, sorting), so using i can associate missingIndices/colors with the wrong dataset.
Suggestion: use dataPoint.datasetIndex (or dataPoint.dataset) to pick the correct dataset and its missingIndices/borderColor.
| for (let i = 0; i < data.length; i++) { | ||
| if (data[i] === null || data[i] === undefined || isNaN(data[i])) { | ||
| missingIndices.add(i); | ||
|
|
||
| // Find previous valid value | ||
| let prevValue = null; | ||
| let prevIndex = i - 1; | ||
| while (prevIndex >= 0 && (data[prevIndex] === null || data[prevIndex] === undefined || isNaN(data[prevIndex]))) { | ||
| prevIndex--; | ||
| } | ||
| if (prevIndex >= 0) { | ||
| prevValue = data[prevIndex]; | ||
| } | ||
|
|
||
| // Find next valid value | ||
| let nextValue = null; | ||
| let nextIndex = i + 1; | ||
| while (nextIndex < data.length && (data[nextIndex] === null || data[nextIndex] === undefined || isNaN(data[nextIndex]))) { | ||
| nextIndex++; | ||
| } | ||
| if (nextIndex < data.length) { | ||
| nextValue = data[nextIndex]; | ||
| } | ||
|
|
||
| // Interpolate midpoint | ||
| if (prevValue !== null && nextValue !== null) { | ||
| interpolated[i] = (prevValue + nextValue) / 2; | ||
| } else if (prevValue !== null) { | ||
| interpolated[i] = prevValue; | ||
| } else if (nextValue !== null) { | ||
| interpolated[i] = nextValue; | ||
| } else { | ||
| interpolated[i] = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
interpolateMissingData() fills each missing point using a single midpoint between the nearest prev/next valid values. For a run of consecutive nulls (e.g., [10, null, null, 40]) this will fill both nulls with the same value (25), producing a flat step instead of a linear interpolation across the gap.
Suggestion: detect contiguous missing spans and interpolate each point proportionally across the span (or document that the intended behavior is to use a constant midpoint for all missing points).
| for (let i = 0; i < data.length; i++) { | |
| if (data[i] === null || data[i] === undefined || isNaN(data[i])) { | |
| missingIndices.add(i); | |
| // Find previous valid value | |
| let prevValue = null; | |
| let prevIndex = i - 1; | |
| while (prevIndex >= 0 && (data[prevIndex] === null || data[prevIndex] === undefined || isNaN(data[prevIndex]))) { | |
| prevIndex--; | |
| } | |
| if (prevIndex >= 0) { | |
| prevValue = data[prevIndex]; | |
| } | |
| // Find next valid value | |
| let nextValue = null; | |
| let nextIndex = i + 1; | |
| while (nextIndex < data.length && (data[nextIndex] === null || data[nextIndex] === undefined || isNaN(data[nextIndex]))) { | |
| nextIndex++; | |
| } | |
| if (nextIndex < data.length) { | |
| nextValue = data[nextIndex]; | |
| } | |
| // Interpolate midpoint | |
| if (prevValue !== null && nextValue !== null) { | |
| interpolated[i] = (prevValue + nextValue) / 2; | |
| } else if (prevValue !== null) { | |
| interpolated[i] = prevValue; | |
| } else if (nextValue !== null) { | |
| interpolated[i] = nextValue; | |
| } else { | |
| interpolated[i] = 0; | |
| } | |
| } | |
| const isMissing = (value) => | |
| value === null || value === undefined || isNaN(value); | |
| let i = 0; | |
| while (i < data.length) { | |
| if (!isMissing(data[i])) { | |
| i++; | |
| continue; | |
| } | |
| // We've hit the start of a contiguous span of missing values. | |
| const spanStart = i; | |
| // Find previous valid value/index. | |
| let prevIndex = spanStart - 1; | |
| while (prevIndex >= 0 && isMissing(data[prevIndex])) { | |
| prevIndex--; | |
| } | |
| const hasPrev = prevIndex >= 0; | |
| const prevValue = hasPrev ? data[prevIndex] : null; | |
| // Find end of missing span and next valid value/index. | |
| let j = spanStart; | |
| while (j < data.length && isMissing(data[j])) { | |
| j++; | |
| } | |
| const spanEnd = j - 1; // last missing index in this span | |
| const nextIndex = j; | |
| const hasNext = nextIndex < data.length; | |
| const nextValue = hasNext ? data[nextIndex] : null; | |
| // Fill the span [spanStart, spanEnd]. | |
| for (let k = spanStart; k <= spanEnd; k++) { | |
| missingIndices.add(k); | |
| if (hasPrev && hasNext && nextIndex !== prevIndex) { | |
| // Linear interpolation between prevValue and nextValue. | |
| const t = (k - prevIndex) / (nextIndex - prevIndex); | |
| interpolated[k] = prevValue + (nextValue - prevValue) * t; | |
| } else if (hasPrev) { | |
| interpolated[k] = prevValue; | |
| } else if (hasNext) { | |
| interpolated[k] = nextValue; | |
| } else { | |
| interpolated[k] = 0; | |
| } | |
| } | |
| // Continue from the first index after this span. | |
| i = spanEnd + 1; |
There was a problem hiding this comment.
Honestly, for this specific chart it is a pretty minor difference in practice. The missing data is already highlighted in the UI via a grayscale overlay and actually a flat plateau may make it more obvious that there is actually no data. If we were to linearly interpolate consistently throughout the gap, the user might might be confused as to where we got the data points from
| chartView.style.display = 'none'; | ||
| tableView.style.display = 'block'; | ||
| graphBtn.classList.remove('active'); | ||
| tableBtn.classList.add('active'); |
There was a problem hiding this comment.
The DataTable is initialized immediately on page load (populateUsageTable) even though the table container (#table-view) starts as display:none. DataTables often mis-measures column widths when initialized in a hidden container, and the current graph/table toggle doesn't adjust/redraw when showing the table.
Suggestion: either delay DataTable initialization until the table view is first shown, or call columns.adjust().draw(false) (or re-init) inside the tableBtn click handler after making #table-view visible.
| tableBtn.classList.add('active'); | |
| tableBtn.classList.add('active'); | |
| // Ensure DataTable columns are correctly sized when the table view becomes visible | |
| if (typeof window !== 'undefined' && | |
| window.jQuery && | |
| window.jQuery.fn && | |
| typeof window.jQuery.fn.DataTable === 'function') { | |
| var $usageTable = window.jQuery('#usage-table'); | |
| if ($usageTable.length && typeof $usageTable.DataTable === 'function') { | |
| var dt = $usageTable.DataTable(); | |
| if (dt && typeof dt.columns === 'function' && typeof dt.draw === 'function') { | |
| dt.columns.adjust().draw(false); | |
| } | |
| } | |
| } |
src/static/js/allocation_detail.js
Outdated
| const chartSubtitle = document.getElementById('chart-subtitle'); | ||
|
|
||
| const modeConfig = { | ||
| cumulative: { title: 'SU Cost - Cumulative', subtitle: 'Showing the cumulative cost for thismonth' }, |
There was a problem hiding this comment.
Spelling: the cumulative subtitle text uses "thismonth" (missing space). Also, the initial template subtitle says "last month" while the modeConfig subtitle says "this month"; these should be consistent to avoid confusing users.
| cumulative: { title: 'SU Cost - Cumulative', subtitle: 'Showing the cumulative cost for thismonth' }, | |
| cumulative: { title: 'SU Cost - Cumulative', subtitle: 'Showing the cumulative cost for this month' }, |
| <script src="https://cdn.jsdelivr.net/npm/chart.js@4/dist/chart.umd.min.js"></script> | ||
| <script src="../../js/chart_config.js"></script> | ||
| <script src="../../js/chart_utils.js"></script> | ||
| <script src="../../js/chart_tooltip.js"></script> | ||
| <script src="../../js/allocation_detail.js"></script> |
There was a problem hiding this comment.
The Playwright fixture loads Chart.js from a public CDN. That makes visual tests dependent on external network availability and the CDN's contents, which can cause flaky CI runs.
Suggestion: load Chart.js from the vendored copy in this repo (e.g., the same file shipped under src/static/chartjs/) or add a local test dependency so the fixture is fully offline/self-contained.
| missingRegions.forEach(({ start, end }) => { | ||
| const startX = scales.x.getPixelForValue(start); | ||
| const width = scales.x.getPixelForValue(end) - startX; | ||
| const height = chartArea.bottom - chartArea.top; | ||
| if (width <= 0) return; | ||
|
|
||
| const imageData = ctx.getImageData(startX, chartArea.top, width, height); | ||
| const pixels = imageData.data; | ||
| for (let i = 0; i < pixels.length; i += 4) { | ||
| if (pixels[i + 3] > 0) { | ||
| const gray = (0.299 * pixels[i] + 0.587 * pixels[i + 1] + 0.114 * pixels[i + 2]) * 0.85; | ||
| pixels[i] = pixels[i + 1] = pixels[i + 2] = gray; | ||
| } | ||
| } | ||
| ctx.putImageData(imageData, startX, chartArea.top); | ||
| }); |
There was a problem hiding this comment.
missingDataGrayscalePlugin calls ctx.getImageData(startX, chartArea.top, width, height) and putImageData() with startX/width computed from scales.x.getPixelForValue(). These values can be fractional and/or outside the canvas bounds, which can throw an IndexSizeError and break chart rendering.
Suggestion: clamp to the chartArea/canvas bounds and round coordinates to integers (e.g., floor/ceil), ensuring width/height are positive integers before calling getImageData/putImageData.
src/static/js/allocation_detail.js
Outdated
| console.log('[NERC Chart] Using REAL data from backend'); | ||
| } else { | ||
| rawData = null; | ||
| } | ||
| } catch (e) { | ||
| console.warn('[NERC Chart] Failed to parse backend data:', e); | ||
| } | ||
| } | ||
|
|
||
| // Fall back to dev mock data if enabled and no real data | ||
| if (!rawData && DEV_USE_MOCK_RAW_DATA) { | ||
| rawData = generateMockRawData(); | ||
| console.log('[NERC Chart] Using DEV MOCK data (set DEV_USE_MOCK_RAW_DATA=false to disable)'); | ||
| } | ||
|
|
||
| if (!rawData) { | ||
| console.log('[NERC Chart] No data available'); | ||
| return null; | ||
| } | ||
|
|
||
| console.log('[NERC Chart] Raw data:', rawData); | ||
| const transformed = transformCumulativeCharges(rawData); | ||
| console.log('[NERC Chart] Transformed data:', transformed); | ||
| return transformed; |
There was a problem hiding this comment.
loadUsageDataFromDOM() logs raw and transformed usage data unconditionally. This can leak potentially sensitive usage/cost details into browser consoles and adds noise in production.
Suggestion: gate these console.log calls behind a dev flag (or remove them), keeping only warnings/errors where needed.
jimmysway
force-pushed
the
issue-178-responsive-interactive
branch
2 times, most recently
from
633bbf4 to
3314857
Compare
- Added NERC template tags and overrides to the Dockerfile. - Updated Playwright configuration. - Modified chart controls CSS for better rendering and responsiveness. - Enhanced JavaScript files to support verbose logging and improved data handling. - Added tests for consecutive missing data handling in charts. - Updated HTML fixture to use local Chart.js for testing. These changes improve the integration and visualization of NERC data in the application.
jimmysway
force-pushed
the
issue-178-responsive-interactive
branch
from
3314857 to
c7f57fc
Compare
Builds on #176 to add interactive and responsive features:
Files:
Closes #178