feat(quill-charts): monotone curve and axis-flush line strokes - #67989
Conversation
Opt-in, default-off line rendering options: - curve: 'monotone' — monotone-cubic (Fritsch-Carlson) line/area smoothing that passes through every point without overshooting; a shortened control arm keeps the bend gentle. The linear default keeps the zero-allocation streaming path. - yFloor draw-time clamp and a left-edge clip so charts that draw axis lines can rest baseline-hugging strokes on the x-axis and end the first point's stroke at the y-axis instead of straddling either. Scales, ticks, and hit-testing stay at true positions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
sampennington
left a comment
sampennington
left a comment
There was a problem hiding this comment.
🤖 sp-ship · 🚫 Blockers
The monotone-cubic smoothing and axis-flush stroke math are correct and single-pass (division-by-zero guarded, NaN gaps split into subpaths, curveReverse geometrically retraces curveForward, yFloor is a pure draw-time clamp that leaves scales/ticks untouched). Three things block: two test bodies use for…of loops with assertions inside (branching logic in tests), and the new clipLeftEdge/withVerticalClip left-clip branch ships with no test at all — a regression there would pass silently. Non-blocking themes worth a look: hot-path allocation churn in the smooth draw path at the 1k×100k ceiling, a couple of untested/edge geometry cases (endpoint overshoot, area bottom edge not clamped to yFloor), and housekeeping — the new curve option isn't documented in the charts guide and has no story, the hand-rolled monotone math duplicates d3-shape's curveMonotoneX (justified by the shortened arm, worth a tighter comment), and withVerticalClip/fillAreaPath growing positional params (with an undefined placeholder at the call site) would read better as options objects.
Replace nested for-of loops with expect inside the bezier overshoot and yFloor clamping tests with aggregate min/max assertions. Add a drawLineSeriesLayer test asserting ctx.rect args for clipLeftEdge true/false. Generated-By: PostHog Code Task-Id: 7d472e91-eb72-439a-be92-ab33652875d8
Addresses two Greptile review findings on the monotone-curve change: - drawFractionalTailDash now splits the final monotone bezier at the fraction via de Casteljau, so a smooth line with a stroke.partial tail keeps the tail on the curve instead of reverting to a straight chord. - The monotone-curve helpers (monotoneTangents/Segments, curveForward, curveReverse) take a Point[] so fillAreaPath passes top/bottom directly, dropping the four per-call throwaway arrays in the smooth area draw loop. traceSmoothPath shares the new collectSmoothRuns. Generated-By: PostHog Code Task-Id: 66eb1ef6-d0d9-4bb5-ad24-cdb2914e0c0f
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
All three blocking review comments (loop-with-assertion test smell x2, missing clipLeftEdge test coverage) are visibly fixed in the current diff with matching test code, remaining feedback is acknowledged follow-up/perf suggestions (not blockers), the new curve/clipLeftEdge behavior is opt-in and defaults preserve existing rendering, and an independent bot reviewer (greptile) thumbed-up the PR with no outstanding unresolved concerns.
There was a problem hiding this comment.
Core rendering math (monotone-cubic curves, axis-flush clamping/clipping) is carefully guarded (NaN gaps, div-by-zero, unreachable edge cases documented) and now has solid test coverage, including the previously-flagged missing clipLeftEdge test and the loop-with-assertion test smells — both were fixed in a later commit per the resolved review threads, which the current diff confirms. An independent bot reviewer (greptile-apps) commented and reacted 👍 on the PR with its one flagged concern acknowledged as an out-of-scope follow-up; a security bot also 👍'd. Remaining review threads are non-blocking documentation/perf follow-ups, explicitly deferred.
🔺 Bundle sizeUncompressed size of every built Total: 81.55 MiB · 🔺 +3.3 KiB (+0.0%)
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report |
Frontend typechecking failed: the clipLeftEdge test annotated its resolveYScale helper as ReturnType<typeof scaleLinear>, which resolves against d3's overloaded signature to a broader type than the drawLineSeriesLayer option expects (ScaleLinear<number, number> | ScaleLogarithmic<number, number>). Annotate it with the concrete ScaleLinear<number, number> so the call site typechecks. Generated-By: PostHog Code Task-Id: 66eb1ef6-d0d9-4bb5-ad24-cdb2914e0c0f
|
Retaining stamphog approval — delta since last review classified as |
1 participant
Problem
Part 1 of the quill charts style refresh (split from #67982 for reviewability). Line charts can only draw straight segments, and a stroke whose first point sits at the plot edge (or whose value hugs zero) straddles any axis line drawn there — half the stroke bleeds past the axis into the gutter.
Changes
Library-only, opt-in, default-off — nothing renders differently unless a host passes the new options.
curve: 'monotone'— monotone-cubic (Fritsch–Carlson) line/area smoothing that passes through every point and never overshoots a peak; a shortened control arm (1/4 vs the standard 1/3) keeps the bend gentle. Hand-rolled rather than d3-shape because the arm length isn't expressible withcurveMonotoneX. The default linear path keeps the existing zero-allocation streamingmoveTo/lineTo.yFloordraw-time clamp + a left-edge clip on the shared line/area layer, so charts that draw axis lines (part 2) can rest baseline-hugging strokes on the x-axis and end the first point's stroke at the y-axis. Scales, ticks, tooltips, and hit-testing stay at true positions — only the drawn stroke is adjusted.How did you test this code?
Automatic notifications
Docs update
The charts AGENTS.md gains the option docs in part 2 (#67982) where the axis-line style that uses them lands.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Built with Claude Code with the human directing the visuals against the live app. This split is one of four stacked PRs replacing the original monolithic #67982 after review. Incorporates sp-ship review findings: the linear path keeps streaming (no per-draw allocation), and a scale-inset approach to the axis-flush problem was tried and rejected because it moved ticks/crosshair off the axis.
🤖 Generated with Claude Code