Skip to content

feat: chart-type-aware row cap — chartRowCap(type) replaces flat CHART_ROW_CAP=500#116

Merged
BorisTyshkevich merged 1 commit into
mainfrom
feat/chart-row-caps-109
Jul 1, 2026
Merged

feat: chart-type-aware row cap — chartRowCap(type) replaces flat CHART_ROW_CAP=500#116
BorisTyshkevich merged 1 commit into
mainfrom
feat/chart-row-caps-109

Conversation

@BorisTyshkevich

Copy link
Copy Markdown
Collaborator

What & why

Closes #109

CHART_ROW_CAP was one flat constant (500) applied to every chart type. Each shape has a different readable ceiling:

  • Pie legibility tops out around 20-30 slices regardless of monitor width.
  • Bar/column are bound by minimum bar+gap width for legible category ticks.
  • Line/area are point-density bound — a wide canvas can plot thousands of points before individual ones blur together (the report that prompted Chart-type-aware row cap: chartRowCap(type) replaces flat CHART_ROW_CAP=500 #109: a 1.4K-row result only plotted its first 500 points on a line chart).

CHART_ROW_CAPS / chartRowCap(type) in src/core/chart-data.js replace the constant: { pie: 30, hbar: 500, bar: 1000, line: 5000, area: 5000 }, falling back to 500 for an unmapped type. buildChartData() and the results.js truncation note both key off cfg.type; switching chart type re-slices and updates the note via the existing rerender() path (no new plumbing needed there).

Out of scope (tracked separately in #111, referenced in #109's body): buildChartData() slices raw rows before aggregating, which is a correctness issue for un-pre-aggregated results, not a readability one — different fix shape, kept out of this same-day pure-lookup change.

Verification

  • npm test — full 100/100/100/100 gate on chart-data.js, updated tests in results.js.
  • npm run test:e2e — 26 passed (Chromium + WebKit clean); 13 Firefox failures are all pre-existing unshare(CLONE_NEWPID): EPERM sandbox errors in beforeEach (an environment limitation of this container), none touching chart code.
  • Manual verify against a live ClickHouse (26.3.10, antalya demo cluster) via npm run local + Playwright: ran a 2000-row query, cycled every chart type, confirmed the note and plotted-point count for each: Bar first 500 of 2.0K rows, Pie first 30 of 2.0K rows (visually ~30 legible slices), Column first 1000 of 2.0K rows, Line/Area — no truncation (2000 < 5000 cap).
  • /code-review (5 parallel finder angles + verify): one confirmed finding — a stale docstring on chartRowCap claiming the fallback was "the bar/column default" when it's actually 500 (hbar's value, not bar's 1000) — fixed in this PR.

Checklist

🤖 Generated with Claude Code

https://claude.ai/code/session_01784dKCpk5W7rdpUAwcQnix

…T_ROW_CAP=500

Each chart shape has a different readable ceiling (pie legibility tops
out ~30 slices; bar/column are bar-width bound; line/area are point-
density bound and can plot thousands before blurring), so a single
flat cap either truncates too aggressively (pie) or too conservatively
(line/area — a 1.4K-row result only plotted its first 500 points).

CHART_ROW_CAPS / chartRowCap(type) in src/core/chart-data.js replace
the constant; buildChartData() and the results.js truncation note both
key off cfg.type, and switching chart type re-slices + updates the
note via the existing rerender() path.

Closes #109

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01784dKCpk5W7rdpUAwcQnix
@BorisTyshkevich BorisTyshkevich merged commit 7017ed1 into main Jul 1, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chart-type-aware row cap: chartRowCap(type) replaces flat CHART_ROW_CAP=500

1 participant