GLOOK-58: Recharts 3 chart migration, with "Not measured" weeks - #75
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Filtered-series colour contract, count vs ratio week filling, hatch mechanics with per-instance pattern ids, signed-stack diverging bars, fixed ProgressRing angle domain, split palette validation, global ResizeObserver stub, third type-colour map in team/dev-table. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adds the Spend vs Impact scatter, UTC week keys at the source, separate commit-type badge treatment, unknown-type folding, the ProgressRing empty-state exception, react-is, the jsdom sizing correction, and the full data-visual inventory. Drops the 3:1 rule for decorative gridlines, which was an error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tch pattern Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…mmits Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dev pages Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d negative in-flight Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…page Adds StackedTypesChart, LinesChangedChart and CommitTypeDonut (Recharts), folds the org page's type breakdown through typeEntriesFrom, and deletes the old TYPE_COLORS/TYPE_HEX maps and hand-rolled SVG chart functions. The donut's slice-hover test (Step 7) passed in jsdom and was kept. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ble shape, hover guard Restores churn (added + removed as magnitudes) for the lines-changed tooltip's total, matching the "Lines Changed / Week" card title and the old chart's behavior; the brief's own step-4 formula and the previous net-change total were both wrong, and the previous test's '50 total' baked in the bug. Extracts a shared TypeSwatch into hatch.tsx to de-duplicate the hatched-vs-solid swatch rendering across the three chart components. Hoists StackedTypesChart's per-type bar shape to module scope so it's stable across renders. Guards CommitTypeDonut's onMouseEnter against an unresolved type. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ots' scale Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The brief's verbatim tooltip code called p.impact.toFixed(1) directly. Every value that reaches the tooltip via the chart is already toNum'd by SpendImpactScatter's `clean` mapping, so this never threw in practice — but the tooltip is exported on its own, and the spec says charts never throw on a string DECIMAL value. A string impact reached this way would throw TypeError: toFixed is not a function. Fixed and pinned with a test that renders the exported tooltip directly with a string-valued payload. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd badges Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…wording Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…0 domain Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Full suite: 187 suites / 1798 tests green (baseline 186/1791 + this guard's 7 tests).
Build green. Screenshot pass (Amber Glow, Daylight Blue, Fresh Mint; org incl. spend
tab, dev, team, home, reports, vulnerabilities, projects) checked against the chart
list and the inline-bar/badge table. Light-mode fixes: none needed.
Concern (not fixed, per brief step 8 item 4's own fallback): the org Commits/Week
chart's in-flight segment for the seeded 1-in-flight/11-commit week renders as an
invisible 2px sliver, indistinguishable from the solid shipped bar below it, in all
three themes. Tried the brief's suggested minPointSize={4} on the in-flight Bar in
timeline-chart.tsx: it fixed the sliver but forces every zero-in-flight week onto a
visible hatch height too (3 timeline-chart tests failed), so reverted. Filed as a
finding for the report; no chart-module edit was kept.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Fix round 1 (Decision 9: in-flight is never shown by colour alone). The org Commits/Week chart's in-flight Bar, StackedTypesChart's in_flight series, and LinesChangedChart's inFlightAdded/inFlightRemoved series all get a function minPointSize, floored to 4 only when the row's own in-flight value is non-zero. Deviated from the literal `v => (toNum(v) !== 0 ? 4 : 0)` snippet: Recharts' minPointSize callback receives `value[1]`, the cumulative top of this layer's slice in the stack (e.g. shipped + inFlight), not inFlight's own delta (confirmed by reading node_modules/recharts/es6/cartesian/Bar.js:601). On a shipped-only week that cumulative top is still non-zero, so that snippet forced a 4px hatch onto every week regardless of whether it actually had in-flight work — exactly the 3 test failures the first attempt hit. Each callback instead reads the row's own in-flight value by index from the chart's own `rows` array, which is correct regardless of what Recharts' internal stack layer math passes in. lines-changed-chart.tsx's removed-side segment renders with a negative `height` attribute (it extends downward from the baseline); minPointSize's sign-preserving math is already correct for it, so the added and removed callbacks are identical in shape (`!== 0`, not `> 0`, per the fix-round instructions). Tests: one behavioral test per chart (timeline-chart, stacked-types-chart, lines-changed-chart), each rendering a large-total/tiny-in-flight week next to a zero-in-flight week, asserting the hatch rect's height (or its absolute value, on lines-changed-chart's removed side) is >= 4, and that the zero-in-flight week draws no hatch rect at all. Verified RED against the un-fixed component for all three before restoring the fix, and RED against the literal-snippet version before switching to the row-indexed lookup, both by hand before committing. Full suite: 187 suites / 1801 tests green (previous 1798 + these 3). Build green. Re-screenshot of checklist item 4 (the short in-flight segment) in all three themes, at default and ~390px width, confirms a legible multi-stripe hatch on the Commits/Week, Lines Changed/Week, and Commit Types Over Time charts' Sep 21 column; items 3 (large in-flight segment) and 5 (diverging chart corners, zero line) re-checked with no regression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…t test; shared in-flight floor
1. Donut layout (commit-type-donut.tsx). The wrapper was `w-full max-w-[320px] shrink-0`, which
can't shrink below its content and clipped at the card's left edge at ~390px (confirmed in the
controller's own screenshot). Changed to `relative min-w-[160px] flex-1 max-w-[320px]`: a flex
item's default min-width is its content size, which blocks shrinking; an explicit min-width
(160px, not the literally-suggested `min-w-0`) replaces that default so it shrinks freely down
to a still-legible floor and never to 0 (aspect-square + Recharts needs a measurable width).
The legend column got `shrink-0`, and the row got `flex-wrap` so the legend drops below when
both can't fit side by side, instead of overflowing.
2. Donut direction regression vs. main. Recharts' `<Pie>` default draws counter-clockwise from
3 o'clock; main drew clockwise from 12. Added `startAngle={90} endAngle={-270}`, matching
ProgressRing. Wedges now read in COMMIT_TYPE_ORDER starting at 12, same as the legend.
3. Real week-domain alignment test. chart-format.test.ts's old guard compared
`recentWeekDomain(now)` with itself — a tautology that can't fail and never rendered a chart,
so a chart that quietly built its own domain from its own `data` would still pass it. Replaced
with `chart-domain-alignment.test.tsx`, which renders TimelineChart, StackedTypesChart and
LinesChangedChart from one shared `weeks` array with real data only in the first and last week,
and asserts each chart's first Bar renders exactly `weeks.length` category slots with the real
values landing on slot 0 and the last slot.
Deliberately does NOT read x-axis tick text/count, despite that being the more obvious
approach: a first attempt did, and found only 1 of 5 ticks rendered in jsdom (Recharts'
`interval="preserveEnd"` culls ticks using text-width math, and jsdom's
getComputedTextLength/getBoundingClientRect always return 0) — exactly what this repo's own
Testing conventions already warn about ("jsdom has no SVG text layout... never assert on tick
counts, tick positions, or label placement"). Reading Bar rectangle DOM-node counts instead
avoids that text-layout dependency entirely.
Mutation-checked: temporarily changed stacked-types-chart.tsx to build its rows from `data`
instead of the passed `weeks` prop; the new test failed (5 expected, 2 received). Reverted
(byte-identical diff against a backup) and reran green before committing.
chart-format.test.ts keeps only the one assertion that's actually specific to the pure
function (recentWeekDomain's default 90-day window matches an equivalent buildWeekDomain
call); the page-level "every chart gets the same domain" promise now has a test that can fail.
4. Shared in-flight floor. The index-based `minPointSize` callback
(`(_, i) => toNum(rows[i]?.[key]) !== 0 ? 4 : 0`) was duplicated four times across
timeline-chart.tsx, stacked-types-chart.tsx and lines-changed-chart.tsx. Extracted to
`inFlightFloor(rows, key)` in hatch.tsx (natural home: it exists only to keep the in-flight
hatch legible, the same reason hatch.tsx exists), with one shared comment, and used in all
four places with identical behavior. Existing minPointSize tests pass unchanged.
Full suite: 188 suites / 1804 tests green (previous 1801 + 3 new in chart-domain-alignment).
Build: green. Screenshotted the org Impact tab in Daylight Blue at 375/768/1024px: the donut is
fully visible at every width (no clipping), the legend wraps below at 375/768 and sits beside it
at 1024 without overflowing, and the slices run clockwise from 12 (feature→bug→refactor→docs→
test→in_flight), matching the legend order and main's pre-migration behavior.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1. chart-domain-alignment.test.tsx put its data at weeks[0]/weeks[4] — the shared domain's own edges. A realistic version of the bug this test exists to catch (a chart gap-filling its own domain from its own data's min/max week, e.g. buildWeekDomain(minWeek(data), maxWeek(data)), instead of the passed `weeks` prop) would, with boundary data, build a domain whose first/last week happen to equal weeks[0]/weeks[4] too — so the old version would have passed either way. Moved the data to interior weeks (index 1 and 3); the test now asserts weeks.length slots with values at slots 1 and 3, and slots 0/2/4 empty. Mutation-checked with that realistic bug: temporarily made stacked-types-chart.tsx build its domain via `buildWeekDomain` over its own data's min/max week instead of the `weeks` prop. The test failed (5 expected, 3 received: the 2-week-apart interior data produced a 3-week domain, not aligned to slots 1/3). Reverted, confirmed the restored file was byte-identical to a backup via `diff`, reran green. Also re-ran the existing naive mutation (rows built from `data` instead of `weeks`) against the new interior-week data to confirm it still fails (5 expected, 2 received) — same revert-and-diff process. 2. Added a jsdom-safe geometric test to commit-type-donut.test.tsx proving the first sector (feature) starts at 12 o'clock and sweeps clockwise: the outer edge's opening "M x,y" point has x equal to the pie's own cx and y above cy (SVG y-axis points down), and the first arc command's sweep-flag (the 5th of its six numeric args) is 1 — the actual bit `startAngle`/`endAngle` changes, independent of any one slice's size, unlike reading the arc's far endpoint. Mutation- checked: removed `startAngle`/`endAngle` from commit-type-donut.tsx, confirmed the new test failed (the M point landed at x=308.8 for a cx of 160, nowhere near 12 o'clock), reverted, confirmed byte-identical via diff, reran green. 3. hatch.tsx's inFlightFloor JSDoc said "each of which called this identically three times before," but there are four call sites (LinesChangedChart alone has two: inFlightAdded and inFlightRemoved). Fixed the count and reworded to name them. Test-only changes plus one comment; no build needed. Focused tests (chart-domain-alignment, commit-type-donut, timeline-chart, stacked-types-chart, lines-changed-chart, chart-format, chart-no-literal-colors): 7 suites / 66 tests green. Full suite: 188 suites / 1805 tests green (previous 1804 + 1 new donut test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Count charts no longer draw unmeasured weeks as zero: the server returns the weeks any report period covered, and the week window ends at the report's own completion week instead of today. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Completed reports only, windowed on completed_at; per-login coverage on the dev page; whole UTC days only; a server-computed anchorWeek; rows keep numeric values plus a measured flag that all three tooltips read. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d anchor Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nses Decision 15, server side. Only completed reports count, windowed on completed_at, and a week is listed only when all 7 of its UTC days are covered. The dev page counts only reports with a developer_stats row for that login. Windows are merged into their union first, so reports that meet mid-day leave no hole. anchorWeek is completed_at's week for a completed report and created_at's otherwise; an unparseable source falls back to the current UTC week. Both are read from the existing reports query, so the call order is unchanged. Full suite: 190 suites / 1836 tests green (expected about 190 / 1836). Both coverage files also green under TZ=UTC. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…member was searched Decision 15 review fix. report-runner.ts searches each member from runStart - period_days with NO upper bound, at some moment at or after runStart. A member searched early in a long run is not re-searched for commits made later in that same run, so a window ending at completed_at could over-claim the run's own tail. created_at <= runStart is the last instant every member's search, even the earliest, is guaranteed to have covered, so the window is now [completed_at - period_days, created_at]. A window whose edges land the wrong way round (a report resumed long after it was created) contributes nothing, which the existing start >= end guard already handles (confirmed by mutation). Both org.ts and dev.ts now SELECT created_at alongside the other reports columns in the existing widened query; call count and order are unchanged. completedReportWindows and coveredWeeksFromWindows in timeline.ts carry both edges explicitly (completedAt, createdAt) instead of a single end + period. Tests updated: pure tests now build windows from (completed_at, created_at, period), with new cases proving the right edge is created_at (not completed_at), a long run doesn't claim the week holding its own tail, and a report resumed far later yields an empty window. DB fixtures that put created_at exactly period_days before completed_at (rA, rOther, rAdj1, rAdj2) became zero-length windows under the fix, so they're changed to created_at == completed_at (a quick, non-resumed run), which reproduces the pre-fix window exactly; rB (resumed far later) is unchanged and now correctly contributes no coverage, updating three DB expectations accordingly. Full suite: 190 suites / 1839 tests green. Both coverage files also green under TZ=UTC. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Minor finding from the Task 12 re-review: no DB-level test went through the real getOrgReport SQL with a report whose created_at is a few hours before completed_at (a genuine narrowing that still leaves a non-empty window) - only the pure helper covered it. Adds rNarrow in its own org (acme-narrow), created Sunday 2026-03-15T20:00Z, completed Monday 2026-03-16T03:00Z, period 14 days. Sunday is deliberately the LAST day of its ISO week: the other 6 days (Mon-Sat) are already inside the window regardless of which right edge is used, so completing Sunday is what completes the week. With the correct right edge (created_at), coveredWeeks is []; mutation-tested by swapping the right edge back to completed_at, which makes Sunday whole and wrongly claims the week of Mar 9 - confirmed by running the test (7 failures total: this one plus the 6 from the prior round), then reverted by Edit. Note: a version of this fixture with the roles reversed (created Monday, completed Tuesday, as in the original finding's example) would NOT discriminate at the week level - Monday is the FIRST day of its week, and the other 6 days are never reached by either right edge, so that week can never be whole either way. Verified this by derivation and by running the Monday/Tuesday design's would-be mutation check, both independently of the advisor's arithmetic, before choosing Sunday/Monday instead. Full suite: 190 suites / 1840 tests green. Both coverage files also green under TZ=UTC (35/35). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ort's week Decision 15, client side. Rows of the three page-grid charts carry a measured flag (data, or a covered week), their tooltips say "Not measured" for an unmeasured week, and both pages build one domain with weekDomainEndingAt(anchorWeek) and pass coveredWeeks to every grid chart (org: 7, dev: 6). Empty states read "in the 90 days before this report". Full suite: 191 suites / 1861 tests green (expected about 191 / 1857). Build green. chart-format and the wiring test also green under TZ=UTC. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Seven chart call sites from 9d1f53b had coveredWeeks={coveredWeeks} joined to the next attribute with no space (org: 4, dev: 3). Whitespace only. Wiring test and tsc green. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Also fixes stale helper and window wording flagged in the final review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…dening Decision 15 final-review fixes. - The org page's in-flight overlay creates week buckets (unmerged_commits has no date filter), so "a row exists" proved nothing. A new shared helper, hasShippedData (commits - types.in_flight > 0), replaces it in all three row builders. An uncovered in-flight-only week is now unmeasured. - NotMeasuredTooltip takes an optional in-flight line, so a hatched week reads "Shipped: Not measured / In flight: N" rather than contradicting its bar. The TimelineChart header takes latest and change from measured weeks only. The dev page has no overlay and is unchanged. - anchorWeekFor: a completed report with a null or unparseable completed_at falls back to created_at, then to today. - A new child-process test under TZ=Asia/Tokyo feeds zone-less SQLite timestamps through completedReportWindows, coveredWeeksFromWindows and anchorWeekFor, and fails if they are parsed as UTC. Mutation-checked: reverting hasShippedData to "row exists" fails 9 tests across all three charts and the header. UTC parsing fails the Tokyo test on both the covered weeks and the anchor. Full suite: 192 suites / 1877 tests green. The coverage and chart suites are green on the host zone, under TZ=UTC and under TZ=Asia/Tokyo. tsc and build green. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The org page's AI % chart gated isDefined on merged commits, which include in-flight. A covered in-flight-only week then read a proven-looking 0%. It now uses hasShippedData, so that week is a null gap. The other ratio gates need no change: - Org Avg Lines/PR keys on prs, which the overlay never adds to. - Org Avg Impact is merged before the overlay creates buckets, so an overlay-only week has none. - The dev page has no overlay. The measured field comments now say "no SHIPPED data". The wiring test checks the org AI % chart's isDefined and its gap. Full suite: 192 suites / 1878 tests green. tsc green. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…exact guard-test steps Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…elines Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
Replaces the hand-rolled SVG charts with Recharts 3 and a ported shadcn/ui
chart.tsxwrapper (the Tailwind v3 variant). Every chart now follows the light/dark theme, shows its values on hover, and reads its colours from one set of tokens.--chart-*CSS variables for both modes inglobals.css, exposed throughtailwind.config.ts. The palette was checked for colour-vision-deficiency separation and 3:1 mark contrast on the light and dark chart surfaces. Guard tests stop a literal colour from creeping back into a chart module.TimelineChartshared by the org and dev pages, with synced hoverBehaviour change: "Not measured" weeks
Before this change, a week that no report covered drew as a real zero. The weekly charts now tell those two cases apart:
[completed_at − period_days, created_at], and the windows are unioned before counting.New setting: Chart colors
After the first dev deploy, the accent-coloured weekly timelines read as too loud on dark themes, especially Amber Glow. Five of them sit on the org page and six on the dev page. Settings → Appearance → Chart colors lets each user pick one of three options:
localStorage(glooker-chart-accent), like the theme. There is no DB or API change.Dependencies
recharts@^3.10.1,react-is@^19(a Recharts peer),clsx,tailwind-merge@^2.6(the last version for Tailwind v3).npm auditreports no new vulnerabilities.Rollout notes
created_atandcompleted_atas UTC.db/mysql.tsdoes not pin the session time zone. Run it in UTC, or some weeks may be marked "Not measured" or measured wrongly.Other
npm run dev:mockfills the 90-day chart window.ResizeObserverstub, which Recharts needs under jsdom.docs/superpowers/specs/2026-09-25-glook-58-recharts-migration-design.md(the chart colors setting is Decision 16). Plan:docs/superpowers/plans/2026-09-25-glook-58-recharts-migration.md.Test plan
npm test: 194 suites / 1898 tests, also underTZ=UTCand a UTC+9 zone for the coverage and week-key suitesnpx tsc --noEmit,npm run buildnpm run dev:mock, including the "Not measured" hover🤖 Generated with Claude Code