fix(status): bound the stacked-area forward-fill to each series' cadence - #1589
Merged
dawsontoth merged 1 commit intoJul 30, 2026
Merged
Conversation
#1576's fix bounded last-observation-carry-forward for cross-node gauge sums in the pipeline. StackedAreaChart has a second, independent forward-fill at the render layer, and it was unbounded: it merges every series onto the union of x positions and carries each series' last-seen value across the positions it did not report, with no staleness limit. That second fill is why "Stack by: Node" never showed the reported dives — and, by the same unboundedness, why an idle node keeps contributing to the stack. The pipeline's bound cannot reach this case: the renderer remaps the spec's dimension to 'node', so each series holds exactly one node and is present in every bucket it has, making pipeline carry-forward a structural no-op. Nothing but this merge bridges those bands, so the bound has to exist here too. The fill stays — it is load-bearing. Harper's gauge rows carry `period: 0` and snap onto the spec's 60 s fallback lattice, which a 90 s emission cadence beats against, so per-node coverage of the lattice is ragged and a strict merge draws a shredded stack. What changes is that a band now expires: `STALENESS_INTERVALS × max(median reporting gap, lattice step)`, plus one lattice step of slack the pipeline does not need — a primitive only sees times that have already been snapped, and snapping compresses alternate gaps, so without the slack a series could expire on the beat pattern the snap induced. Cadence is derived from the series' own point spacing rather than threaded through SeriesData: derived metrics and hand-built SeriesData reach this primitive too, so a threaded field would be absent often enough that this derivation would remain the live path anyway. Past the horizon a band fills 0, not null. That mirrors the pipeline (dropping an expired node from a `crossNode: 'sum'` *is* contributing 0), and it is what a stacked chart means — bands add, so 0 is the neutral element while a null punches a hole through the stack and drops every band above it. A cluster-wide gap still reads as a gap: x positions come only from series that reported, so when everything stops the rows stop rather than filling with zeros. Checked every stacked-area panel for regressions. The gauges (connections, database-size — crossNode 'sum' over 'max'/'last') are the motivating fix. The additive counters (mqtt-traffic-*, bytes-sent/received — sum/sum) improve too, for the reason the pipeline excluded them: a missing bucket for a rate series is no traffic, so expiring to 0 is the honest value where carrying the last throughput forward invented traffic indefinitely. Observed values are never rewritten, including a genuine drop to 0, and nothing is ever back-filled before a series' first point. Extracted the merge to mergeStackedRows so it is unit-testable — the existing stacked-area suite shows why: every assertion about rendered geometry there is `it.skip`, because neither jsdom nor happy-dom lays Recharts out. Also memoized it in the component, since the chart re-renders on every hover tick. csvExport keeps exporting raw points with empty cells for gaps. It never shared this fill, deliberately — the export is the data, not the drawing. Refs #1576 Co-Authored-By: Claude Opus 5 <[email protected]>
Contributor
There was a problem hiding this comment.
Code Review
This pull request extracts and refactors the merging and forward-filling logic of StackedAreaChart into a separate utility module mergeStackedRows.ts. It introduces a bounded carry-forward mechanism based on a calculated staleness horizon to prevent idle nodes from contributing to the stacked total indefinitely (fixing issue #1576). Comprehensive regression tests are also added in stacked-row-staleness.test.ts. There are no review comments, so I have no additional feedback to provide.
Member
|
Reviewed — |
dawsontoth
merged commit Jul 30, 2026
f20aa94
into
claude/mqtt-status-chart-gaps-ace583
1 check passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #1587 — review that one first. Base auto-retargets to
stagewhen it merges.#1587 bounded last-observation-carry-forward for cross-node gauge sums in the pipeline.
StackedAreaCharthas a second, independent forward-fill at the render layer, and it was unbounded: it merges every series onto the union of x positions and carries each series' last-seen value across the positions it did not report, with no staleness limit.Why #1587's bound can't reach this
In Stack by: Node,
TrafficByTypeRendererremaps the spec's dimension to'node', so each series holds exactly one node and that node is present in every bucket the series has. Since pipeline carry-forward never invents bucket times, it is a structural no-op in that mode — nothing but this merge bridges those bands.That is both why node mode never showed the #1576 dives, and why an idle node's band was carried to the right-hand edge of the window. Core emits
mqtt-connectionsonly while the count is nonzero (if (numberOfConnections > 0)inserver/mqtt.ts), so "no row" is exactly what an idle node looks like — the same overstatement #1587 fixed, one layer up.What changed
The fill stays; it is load-bearing.
period: 0gauge rows snap onto the spec's 60 s fallback lattice, which a 90 s emission cadence beats against, so per-node lattice coverage is ragged and a strict merge draws a shredded stack. What changes is that a band now expires:That trailing lattice step is slack the pipeline does not need. The pipeline estimates cadence from raw emission instants; a primitive only ever sees times already snapped onto the lattice, and snapping compresses alternate gaps — a 90 s cadence on a 60 s lattice reports at 0/120/180/300…, so its gaps alternate 120/60 and their median reads 60 or 90 depending on how many samples the window caught, while the widest gap is 120. Without the slack a series can expire on the very beat pattern the snap induced (I hit a zero-margin case before adding it). One step is the most the snap can stretch a gap by, which keeps this horizon at least as generous as the pipeline's.
Cadence is derived from the series' own point spacing rather than threaded through
SeriesData: derived metrics (mqtt-traffic-*) and hand-builtSeriesDatareach this primitive too, so a threaded field would be absent often enough that this derivation would remain the live path anyway.Past the horizon a band fills 0, not null
crossNode: 'sum'is contributing 0 to that sum.Regression check across every stacked-area panel
connectionsdatabase-sizebytes-sent/bytes-receivedmqtt-traffic-sent/-receivedThe additive counters improve for the same reason #1587 excluded them from pipeline carry-forward: a missing bucket for a rate series means no traffic, so expiring to 0 is the honest value where carrying the last throughput forward invented traffic indefinitely.
Observed values are never rewritten — including a genuine drop to 0 — and nothing is back-filled before a series' first point. The ceiling series stays unbounded deliberately: it is a reference line drawn over the stack, not a band in it, with no zero to expire to (a ceiling at 0 reads as a limit of nothing).
csvExportis untouched. It exports raw points with empty cells for gaps and never shared this fill — the export is the data, not the drawing.Testing
Extracted the merge to
mergeStackedRowsso it is unit-testable at all: every assertion about rendered geometry in the existingstacked-areasuite isit.skip, because neither jsdom nor happy-dom lays Recharts out. Also memoized it in the component, which re-renders on every hover tick.16 cases in
__tests__/primitives/stacked-row-staleness.test.ts, shaped aftercross-node-gauge-gaps.test.tsand driven through the realconnections/bytes-sentspecs and the real pipeline: the staggered-cadence shape actually producing ragged coverage, the pipeline no-op in node mode, horizon expiry, still-bridged-inside-the-horizon, no flicker back, the exact horizon boundary on hand-checkable numbers, observed-value passthrough, no back-fill,y: nullhandling, no invented rows, and the ceiling exception.Verified the coverage is not vacuous by reverting the horizon to
Infinity— 4 cases fail, then restored.Gate: 265 files / 1898 tests pass,
tsc -b, oxlint, and dprint all clean.Not browser-verified. Reproducing the artifact needs a genuinely idle node in a live cluster, which can't be summoned on demand, and port 5173 was held by another checkout. Verification is mechanism-level, through the real specs and pipeline.
Refs #1576
🤖 Generated with Claude Code