Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. WalkthroughChangesSparkline rendering
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The sparkline fix preserves visible lone and null-separated observations without a confirmed remaining merge risk. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 47 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
AntTheLimey
left a comment
AntTheLimey
left a comment
There was a problem hiding this comment.
This fix does not close #458 as reproduced, and the root-cause measurement table does not hold on the echarts version this project ships.
→ Single/short-history data is still blank after the fix. Issue #458's own repro ("at least two cycles" of pg_stat_database) leaves exactly one time_series point. Feeding the PR-head build (cfc058bf) that exact shape through the real API path gives a Cache Hit Ratio canvas with 0 of 6630 non-transparent pixels — the reported symptom, unchanged. That is also the state a newly registered server is in, which is likely the most common real-world trigger.
→ The claimed containLabel collapse does not reproduce on echarts@6.1.0, which is what package-lock.json pins (the PR's table was measured against 6.0.0). Rebuilding the unmodified pre-fix Sparkline.tsx and feeding it a realistic 12-point series (matching the real API's shape) renders a sparkline with a non-transparent bounding box 27px tall of the 30px canvas — not the 4px collapse described for containLabel: true.
→ Recommend re-diagnosing before merging. The evidence points at a different, unaddressed cause: a line/area series with one point has nothing to draw a segment between, independent of grid or axis-label settings. The grid change itself looks safe to keep, but closing #458 on it leaves the single-point case (the most likely real trigger) exactly as broken as before.
Two smaller points worth a look, not blockers: grid.containLabel: false already skips ECharts' label-space reservation per its own docs, which suggests the two axisLabel: { show: false } overrides may be redundant for this fix rather than each independently necessary as the comment and quality-checklist.md entry assert; and the regression tests mock Chart entirely, so they assert on prop shape only and could not have caught either finding above — worth a rendered or screenshot-level check for this bug class.
Everything else checks out: ESLint clean on both touched files, Sparkline.tsx at 100% statements/branches/functions/lines matching the stated claim, the full client suite passing with coverage above every pinned threshold, and the quality-checklist.md change is purely additive. No blocking review threads exist (CodeRabbit hit its rate limit before reviewing, Codacy is excluded from this review per house rules). PR #510 merges cleanly with this branch per the coordinator's check.
The sparkline overrides the chart grid insets but Chart deep-merges
those over buildGrid()'s options key by key, so containLabel: true
survived. ECharts' contain-label pass reserves space for each axis's
estimated label rect and keys off axisLabel.show alone, which an
axis's own show: false does not imply, so the hidden labels still ate
the plot area: measured against echarts 6.0.0 on a 275px-wide canvas,
a 30px sparkline was left a 228x4 plot inset 45px from the left, and
the 40px default used by KpiTile a 228x14 one. That is why only the
Database Summaries call site, the sole one passing height={30},
looked completely empty.
The sparkline now sets containLabel: false and turns the axis labels
off explicitly, which restores the full 271x26 plot area.
Closes #458
A line series is drawn as segments between neighbouring values, so a series with no adjacent pair of values has no geometry at all: a lone point, or points separated by null gaps, paint nothing once markers are off. That is the state of a newly registered server, and of the "at least two cycles" repro on issue #458, so the grid fix alone left those rows blank. `Sparkline` now counts adjacent pairs and turns markers on when there are none, so each observation gets a dot. Drop the `axisLabel: { show: false }` overrides: measured on echarts 6.1.0, `containLabel: false` alone gives the identical 271x26 grid rect, so they were doing nothing. Correct the comment and the knowledge-base entry accordingly, and restate the measurements on 6.1.0, which is what package-lock.json pins. Add a rendered-level test that mocks only the echarts-for-react wrapper and runs the fully merged option object through ECharts' own SSR renderer, then measures the subpaths emitted. All four of its geometry assertions fail on the pre-fix component; the prop-shape tests it complements could not have failed at all.
cfc058b to
456be51
Compare
AntTheLimey
left a comment
AntTheLimey
left a comment
There was a problem hiding this comment.
Both blockers from the last review are fixed, verified by rebuilding the stack and reproducing issue #458's exact single-point shape against this head, and independently re-confirmed with a deterministic test that fails on the pre-fix component and passes on this one.
The single-point case now renders. Mocking the API to return exactly one cache_hit_ratio.time_series point and pixel-scanning the Cache Hit Ratio canvas gives a visible marker where the previous head gave zero non-transparent pixels on the identical repro. A gap-separated series (null, 95, null, 98) also now renders two markers. A realistic 12-point series still fills the canvas.
I confirmed this a second way, independent of any screenshot: the PR's new Sparkline.render.test.tsx renders — the exact real call site's configuration — through ECharts' own SSR renderer and checks what actually got drawn. On this head, 24/24 tests pass. Swapping in the pre-fix Sparkline.tsx with the same test file, the single-point and gap-separated tests fail immediately while the other four still pass. That's a clean, repeatable proof the fix works on the real component and the real configuration.
The root-cause table is now measured against the version this project ships. echarts@6.1.0 is confirmed installed and pinned in package-lock.json. The PR body states plainly that the earlier 6.0.0 table is superseded, with new figures for 6.1.0.
The axisLabel redundancy claim from the last review is corrected, not just asserted. Running the production option object through echarts@6.1.0's SSR renderer with and without the two axisLabel: { show: false } overrides, holding containLabel: false constant, gives an identical grid rect both times. The PR removes both overrides and corrects the code comment and the quality-checklist.md entry to match.
The test gap is closed. Sparkline.render.test.tsx renders the fully merged option object through ECharts' own SSR renderer instead of mocking Chart, and its assertions fail against the pre-fix component for exactly the two cases that mattered.
Checks: ESLint clean on the touched files (37 warnings, 0 errors across the whole client, none in this diff); tsc --noEmit clean; Sparkline.tsx at 100% statements/functions/branches; full suite at 4346 passed with all four coverage ratchets clear; branch is 0 behind origin/main and merges cleanly; CodeRabbit's full review and Codacy both report no issues.
Approving.
2 participants
Cause
Two independent faults, both of which have to be fixed before a
Database Summaries row draws anything.
The plot area collapses.
Sparklineoverrides the chart grid insets,but
Chartdeep-merges itsechartsOptionsoverbuildGrid()'s optionskey by key, so
containLabel: truesurvives. That layout pass reservesroom for each axis's estimated label rect whether or not the axis is
shown, which at sparkline heights leaves nothing to draw into.
A series with no adjacent pair of values has no geometry. ECharts
draws a line series as segments between neighbouring values, so a single
point, or points separated by null gaps, paint nothing at all once
markers are off: the area path degenerates to a zero-width
M x y L x y Zand the line path to a bare move. Issue #458's own repro leavesexactly one point in
time_series, and so does a newly registeredserver, which is the most common way to hit this.
Measured in Chrome against echarts 6.1.0 (the version
package-lock.jsonpins) on a 275x30 canvas, through the real optionpipeline, counting non-transparent canvas pixels:
containLabel[null, v, null, v][null, v, null, v]The same collapse hits
KpiTile's default 40px canvas, which came outas a 5px band rather than the full 37px, so it was squashed rather than
visibly empty. The
CHART_BOX_SXgeometry mismatch noted on the issueis real but is not the cause, so it is left alone.
An earlier revision of this PR measured the first row on echarts 6.0.0
and reported a 4px plot rect; on 6.1.0 the rect height is negative and
the drawn band is 3px. The table above replaces those figures.
Change
Sparklinesetsgrid.containLabel: false, and passesshowMarkersto
Chartwhen the series holds no adjacent pair of values, so a loneor fully gapped series gets a circle on each observation and turns the
markers off again as soon as the line can be joined up. A wholly empty
series, or one where every bucket is a null gap, still renders no chart
at all: there is nothing to show, and an empty canvas would be worse
than no canvas.
The
axisLabel: { show: false }overrides the first revision added aredropped. They were redundant:
containLabel: falseskips the labelreservation outright, and the grid rect is identical with and without
them. The code comment and the
.claude/react-expertentry thatclaimed all three flags were necessary are corrected.
Tests
The existing regression tests mock
Chartwholesale, so they assert onprop shape and could not have caught either fault.
Sparkline.render. test.tsxmocks only theecharts-for-reactwrapper and runs the fullymerged option object through ECharts' own SSR renderer, then measures
the subpaths actually emitted; all four of its geometry assertions fail
against the pre-fix component. The prop-shape tests stay as the cheap
guard on the options themselves.
Test plan
make lint: 0 errors, 37 pre-existing warnings, none in the touched files.npx tsc --noEmit: clean.make coverage: 4346 passed;Sparkline.tsx100% statements and branches; all four client ratchets clear (lines 91.68, statements 90.54, functions 88.14, branches 79.96).Closes #458
Summary by CodeRabbit
Bug Fixes
Tests