Skip to content

refactor(frontend): read chart colors from design tokens - #1360

Merged
IAmJSD merged 7 commits into
LeanerCloud:mainfrom
IAmJSD:ui/issue-699
Oct 25, 2026
Merged

IAmJSD merged 7 commits into
LeanerCloud:mainfrom
IAmJSD:ui/issue-699

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Charts hard-code their hex colors in TypeScript and the stylesheets hold 121 raw hex/rgb colors (on main at b42245c) that bypass the --cudly-* design tokens, so no theme (dark mode, a contrast fix) could reach the charts and nothing stopped new literals. This PR adds the theming path for charts (--cudly-chart-* tokens read through a new lib/chart-theme.ts), swaps CSS literals that already equal a token, and adds guard tests. Nothing renders differently by default.

Refs #699
Partial: the remaining 113 one-off CSS literals and a prefers-color-scheme dark palette are left for follow-ups, because both need palette decisions.

What changed

  • frontend/src/styles/base.css: --cudly-chart-* tokens in :root (10-color series palette, cumulative line, 3 per-service layer tokens --cudly-chart-layer-current/-lowest/-upside, axis text, 2 gridline tokens), each with the exact value the charts used before.
  • New frontend/src/lib/chart-theme.ts: reads the tokens at render time through chartColor(), chartPalette(), withAlpha(), applyChartDefaults() (sets Chart.js's global text and grid colors) and surfaceColor() (reads --cudly-surface). requireToken() moves here from ladder-timeline-chart.ts, and parseHexColor lives here too. Fallbacks repeat the base.css values for jsdom or a failed CSS load.
  • frontend/src/dashboard.ts: the per-service savings chart reads its three layer colors through a new savingsLayerColors() (replacing the SAVINGS_LAYER_COLORS constant, with main's exact values as the token defaults), and the 2px gap between its stacked segments reads --cudly-surface (#ffffff) instead of '#ffffff'. The savings trend line reads --cudly-chart-1. dashboard.test.ts calls savingsLayerColors() where it used the constant.
  • frontend/src/modules/savings-history.ts: the single-series chart reads its colors from the tokens: the cumulative line from --cudly-chart-cumulative (#4285f4, fill at 0.1 alpha), the per-period bars from --cudly-chart-2 (#34a853 border, fill at 0.7 alpha), the gridlines from --cudly-chart-grid-faint.
  • frontend/src/trends.ts (Inventory > Trends bar chart) reads its series and gridline colors from the tokens.
  • None of dashboard.ts, trends.ts, modules/savings-history.ts or ladder-timeline-chart.ts contains a color literal any more.
  • CSS literals already equal to a token now use it: modals.css #fff -> --cudly-surface or --cudly-primary-fg, components.css #f8f9fa -> --cudly-surface-muted, charts.css #2563eb -> --cudly-primary.
  • New frontend/src/__tests__/chart-theme.test.ts.
  • Commits: 3ffc3b7b (chart tokens and theme helpers), the merge with main b8a54fdc, f4e84397 (ceiling lowered to 136, since main removed 3 literals), 21d231b6 (Trends chart tokenized), the merge with main 540b0169, 5ab3ad41 (segment gap reads --cudly-surface) and 4677d7cb (ceiling set to 113, the count on this branch after merging main at b42245c).

Why this approach

  • Only refactors that render pixel-identical. The remaining 113 CSS literals are one-off shades (for example #1a73e8 next to the #2563eb primary, many grays). Mapping each one to a token, or choosing a dark palette, changes how the app looks, so it is left as a design decision.
  • Tokens are read at render time, not import time, so a later theme switch only needs a re-render.
  • The fallbacks keep jsdom tests and a failed stylesheet load working; a test keeps them in sync with base.css.
  • A ratchet test (ceiling 113: main's 121 minus this PR's 8 token swaps, i.e. main's count once this lands) makes migration progress stick without blocking unrelated work.

Screenshots

Screenshots were taken before this branch was merged with main; before = main at a14dc8f. After is this branch at that point. Both are headless Chromium at 1440x900 against the repo's mocked e2e API, with the same --cudly-chart-* override (purple palette, red axis text) applied to both builds to show which colors the tokens control.

Before After
trend-chart before trend-chart after
  • trend-chart: with the override, main's savings-over-time chart keeps the hard-coded blue line and gray axis; on this branch line, fill and axis labels follow the tokens.

Verification

Run on this branch's head 4677d7cb:

  • Lint: 0 errors. Typecheck clean, build OK.
  • Jest: 4415 passed, 1 skipped.
  • New chart-theme.test.ts (13 tests): fallbacks equal base.css and cover every --cudly-chart-* token; series and layer tokens are #rrggbb; a live token wins over the fallback; surfaceColor() follows --cudly-surface and its fallback equals base.css; requireToken throws; withAlpha and applyChartDefaults work; savingsLayerColors() follows an overridden layer token; dashboard.ts, modules/savings-history.ts, ladder-timeline-chart.ts and trends.ts have no color literals; the CSS literal count is at most 113.
  • New dashboard.test.ts case: the stacked-segment gap follows a themed --cudly-surface. It fails with the '#ffffff' literal (expected #1f2937, received #ffffff) and passes with the token.
  • Playwright (e2e-local.sh): 264 passed.
  • Secrets scan clean (planted key flagged); scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
  • Merged with main at b42245c; conflicts in frontend/src/dashboard.ts, frontend/src/modules/savings-history.ts and frontend/src/styles/modals.css resolved, see the merge commit 540b0169.

Notes for review

  • Done here: the chart theming path (the issue's lib/chart-theme.ts step), exact-match CSS literal swaps and guard tests.
  • Left out:
    • The remaining 113 one-off CSS literals (components 43, modals 29, settings 14, responsive 13, tables 8, layout 4, forms 2). Each needs a decision to snap it to an existing token (a visible shift) or add a new token.
    • A @media (prefers-color-scheme: dark) token override. It needs a dark palette and only makes sense after the literal migration, or the UI comes out half dark.
    • docs.css (API docs page).
  • Chart palette tokens are named by slot (--cudly-chart-1..10), not by service. Since main's fix(frontend): color the per-service savings chart by layer #1335 dropped the per-service palette, only slots 1 and 2 are drawn; slots 3-10 and chartPalette() are unused outside the tests and can be dropped if a spare series palette is not wanted.
  • The CSS literal ceiling is a ratchet: any branch that adds a hex/rgb()/hsl() literal fails it and has to use or add a token (named colors such as white are not counted).
  • This PR is meant to land after the other UI PRs, so CSS_LITERAL_CEILING in chart-theme.test.ts is set to 113, the count after this PR's 8 token swaps (main at b42245c has 121), i.e. main's count once this lands.

Overlaps

Note for the UI owner

This changes UI code that @cristim owns. IAmJSD reviewed the before/after screenshots and asked for it to be merged; any design point you disagree with can be a follow-up.

🤖 Generated with Claude Code

Chart.js paints on canvas, so the dashboard and savings-history charts
hard-coded their hex colors and no stylesheet could restyle them: a
theme or contrast fix could reach every surface except the charts.

Add --cudly-chart-* tokens (series palette, cumulative line, legend
swatches, axis text and gridlines) to the :root block in base.css, with
the same values the charts used, and lib/chart-theme.ts to read them at
render time (falling back to the same values when the stylesheet is
absent, e.g. under jsdom). The dashboard per-service bars, legend and
savings trend, and the savings-history chart now take every color from
it, and Chart.js's global text/grid defaults follow the tokens too. The
ladder chart's existing token reader moves into the same module.

Also swap the stylesheet literals that already equal a token's value
(#fff surfaces and button text, #f8f9fa, #2563eb) for the token. With no
token values changed, every chart and surface renders pixel-identical.

New tests keep the TypeScript fallbacks in sync with base.css, keep the
chart modules free of color literals, and cap the remaining stylesheet
color literals at today's 139 so the count can only go down.

This is the theming groundwork from the issue. Migrating the remaining
one-off literals and adding a prefers-color-scheme dark palette are left
for follow-ups that need palette decisions.

Refs LeanerCloud#699

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added priority/p2 Backlog-worthy effort/l Weeks triaged Item has been triaged severity/high Significant harm urgency/this-quarter Within the quarter impact/all-users Affects every user type/bug Defect labels Oct 24, 2026
@coderabbitai

coderabbitai Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Caution

Review failed

The pull request is closed.

Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

IAmJSD and others added 3 commits October 24, 2026 16:48
Conflicts:
- frontend/src/dashboard.ts: main (LeanerCloud#1335, Refs LeanerCloud#189) recoloured the
  per-service savings chart by layer (one hex per stacked layer in
  SAVINGS_LAYER_COLORS, Chart.js default legend) and removed the per-service
  palette, darkenHexColor and the custom legend; this branch had moved those
  per-service colours onto --cudly-chart-* tokens. Kept main's layer design
  and this branch's tokens: the three layer colours are now
  --cudly-chart-layer-current/-lowest/-upside tokens (main's exact hex
  values) read at render time by a new savingsLayerColors(), which replaces
  the SAVINGS_LAYER_COLORS constant. Dropped serviceBarColor,
  KNOWN_SERVICE_COLOR_INDEX, legendSwatchColors, darkenHexColor and the
  parseHexColor re-export, as main did. Import block: main's
  formatTrendTooltipTitle plus the chart-theme helpers still used
  (applyChartDefaults, chartColor, withAlpha).
- frontend/src/styles/base.css: :root token block. Kept both main's motion
  tokens (LeanerCloud#725) and this branch's chart tokens; replaced the now-unused
  --cudly-chart-legend-dark/-mid/-border tokens with the three layer tokens.
- frontend/src/lib/chart-theme.ts (follows from the above): fallbacks swap
  the legend tokens for the layer tokens; comments updated.
- frontend/src/__tests__/chart-theme.test.ts, dashboard.test.ts (follow
  from the above): the themed-palette test now overrides a layer token via
  savingsLayerColors(); main's layer-colour tests call savingsLayerColors()
  instead of SAVINGS_LAYER_COLORS, assertions unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ing main

Main's CSS tidy-ups since a14dc8f removed three more literals, so the
ratchet drops from 139 to the current count.

Refs LeanerCloud#699

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The Trends sub-tab chart that landed on main (LeanerCloud#1317) hard-coded its bar and
gridline colors. They already equal --cudly-chart-2 and
--cudly-chart-grid-faint, so it now reads those tokens (and the shared
Chart.js text/grid defaults) like the other charts, rendering identically,
and the no-color-literals test covers trends.ts.

Refs LeanerCloud#699

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): APPROVE 21d231b6ac7904bbc1d69062fefbf217766b173e

Blocking:

  • none
    Non-blocking:
  • frontend/src/lib/chart-theme.ts:71 and base.css --cudly-chart-3..--cudly-chart-10: main fix(frontend): color the per-service savings chart by layer #1335 removed the per-service palette, so chartPalette() and 8 of the 10 series tokens are now used by nothing except chart-theme.test.ts. Only chart-1 and chart-2 are used. This is dead API and dead tokens left by the merge. Either drop them or document them as a reserved palette.
  • frontend/src/tests/chart-theme.test.ts:105: CSS_LITERAL_CEILING is set to exactly the current count (136, re-counted on HEAD and on HEAD merged with current main 51aa5b1). Any in-flight branch that adds one raw color will fail after this lands. That is the ratchet's intent, but it makes merge order matter.
  • frontend/src/styles/modals.css:351/:362/:465/:476: .archera-signup-btn and .archera-offer-signup keep literal #1a73e8/#1557b0 backgrounds, while their text color now comes from --cudly-primary-fg. A theme that changes primary-fg could give poor contrast on the fixed blue. This is pixel-identical today.
  • frontend/src/lib/chart-theme.ts:80: withAlpha() silently falls back to chart-1 blue for a non-#rrggbb token (for example #abc or rgb()). A themed line would then get a mismatched blue fill. The constraint is documented, and a test checks the defaults only.
  • frontend/src/tests/dashboard.test.ts:1767-1781: main's layer-colour assertions now compare against savingsLayerColors() itself. That is as tautological as the old constant comparison and would not catch a swapped token.
    Attacked: Merge: dashboard.ts (I confirmed that main's fix(frontend): color the per-service savings chart by layer #1335 layer design, its Chart.js default legend and the removal of serviceBarColor/darkenHexColor all survive; only the three hex values moved into --cudly-chart-layer-* tokens, with main's exact values) and base.css (main's chore(frontend): low-severity table and CSS hygiene fixes #725 motion tokens kept). Follow-ups: f4e8439 lowers the ratchet 139->136; I re-counted 136 with the test's own regex on HEAD and on HEAD+current main. 21d231b tokenizes main's new trends.ts chart and adds it to the no-literals test. Full jest plus tsc on HEAD merged with current main pass (3927/3928).

IAmJSD and others added 3 commits October 24, 2026 23:53
Brings in main at b42245c (LeanerCloud#1369 and the UI PRs before it) so the
chart-theme guards run against the current chart code.

Conflicts and resolutions:
- frontend/src/dashboard.ts imports: kept main's enabled-providers,
  kpi-tile, upcoming-scope, chart-utils (sparse trend, upsertChart) and
  chart-a11y imports plus this branch's lib/chart-theme import.
- frontend/src/dashboard.ts renderSavingsByService: kept main's LeanerCloud#744
  summary and "View as table" before the chart and LeanerCloud#747's upsertChart,
  with this branch's savingsLayerColors() and applyChartDefaults() read
  right before it. Main's LeanerCloud#744 SEGMENT_GAP spread stays on all three
  layers.
- frontend/src/dashboard.ts loadSavingsTrendChart: main's LeanerCloud#747 upsertChart
  with this branch's --cudly-chart-1 trend color and applyChartDefaults().
- frontend/src/modules/savings-history.ts imports: kept both (main's
  upsertChart, shared formatters and chart-a11y; this branch's
  chart-theme).
- frontend/src/modules/savings-history.ts renderSavingsChart: took main's
  LeanerCloud#749 single-series dataset (line for cumulative, bar per period), LeanerCloud#744
  summary/table and LeanerCloud#747 upsertChart; the colors main added are read from
  the existing tokens with the same values: cumulative line
  --cudly-chart-cumulative (#4285f4, fill at 0.1 alpha), period bars
  --cudly-chart-2 (#34a853 border, fill at 0.7 alpha).
  applyChartDefaults() runs before upsertChart.
- frontend/src/styles/modals.css .archera-offer-panel: took main's
  var(--cudly-r-lg) and var(--cudly-shadow-dialog); the background keeps
  this branch's var(--cudly-surface).

Main's '#ffffff' segment gap in dashboard.ts still trips the "no color
literals" guard here; the next commit moves it to a token.

Refs LeanerCloud#699

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main's LeanerCloud#744 gave the per-service savings chart a 2px '#ffffff' gap between
stacked layers, a color literal in dashboard.ts that the chart-theme guard
rejects and that a theme could not reach. The gap is meant to read as the
card background, so it now uses the --cudly-surface token (#ffffff, so the
chart renders the same) through a new surfaceColor() in lib/chart-theme.ts,
read at render time like the other chart colors. The fallback for jsdom or
a failed stylesheet load equals base.css, and a test keeps the two in sync.

Refs LeanerCloud#699

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This PR lands after the other UI PRs, so the ratchet's baseline is the
count on main at that point. Merged with main at b42245c, styles/*.css
hold 113 color literals outside token definitions (components 43,
modals 29, settings 14, responsive 13, tables 8, layout 4, forms 2), down
from 136. The comment now says the baseline is the count when the
ratchet landed.

Refs LeanerCloud#699

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD

IAmJSD commented Oct 24, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): APPROVE 4677d7cb6dc76dae982db84a3bb869c161bbcf2b

Blocking:

  • none
    Non-blocking:
  • frontend/src/tests/chart-theme.test.ts:115 and commit 4677d7c subject ("main's count"): the ceiling is 113. That is the count at HEAD, after this PR's 8 swaps (modals 35->29, components 44->43, charts 1->0). It is not the count on main at b42245c, which is 121 by the test's own counter. 113 is the right ceiling, because it equals main after the squash. Only the wording is off, and the squash drops the commit subject, so only the PR body needs fixing (see Body-stale).
  • frontend/src/tests/chart-theme.test.ts:24-33: the CSS ratchet counts only hex/rgb()/hsl() literals. Named colors pass it. Main already has at least 10 of them (forms.css:9 background: white, settings.css:340, components.css:146/157/168 color: white, ...), so a new color: white or black still slips past the "no new raw color" guard. The TS guard (line 104) matches only single-quoted hex and rgb(, so it misses "#..." in double quotes or template strings and hsl().
  • frontend/src/lib/chart-theme.ts:107: applyChartDefaults mutates the global Chart.defaults, but ladder-timeline.ts never calls it. The ladder chart's axis and grid colors therefore follow a theme only if a dashboard, savings-history or Trends chart rendered first. Today's values are identical to Chart.js 4.5.1's own defaults ('test(server): tighten RI exchange reservation tests; decide audit row for refused reservations #666', 'rgba(0,0,0,0.1)'), so nothing changes now.
  • Carried over from the earlier review, still true: withAlpha() quietly falls back to chart-1 blue for a non-#rrggbb token. dashboard.test.ts:2171-2186 compares the layer colors with savingsLayerColors() itself (circular), and only current is pinned to a literal (chart-theme.test.ts:92). lowest and upside rely on the fallback-equals-base.css test. modals.css .archera--signup still uses literal #1a73e8/#1557b0 backgrounds under the --cudly-primary-fg text. Chart tokens 3-10 and chartPalette() are unused (the body now says so).
    Attacked: The merge 540b016 (parents 21d231b and b42245c) differs from main only in the 11 PR files. I read every hunk of git diff origin/main...HEAD; merge-base == origin/main. All of main's newer chart code survives: fix(frontend): dashboard charts have no text alternative and layers differ only by shade #744 summary, table and 2px gap on all three layers, fix(frontend): charts rebuilt on every change; dashboard trend has no stale guard #747 upsertChart, fix(frontend): purchases savings chart uses two y-axes and shadows formatCurrency #749 single-series line/bar, fix(frontend): color the per-service savings chart by layer #1335 layer colors, and feat(frontend): add a Trends sub-tab for the analytics trends endpoint #1317 Trends. No other references to the removed SAVINGS_LAYER_COLORS or SEGMENT_GAP remain. Rendered colors are unchanged: every token value in base.css equals the literal it replaced (#2a78d6/#eb6834/#1baf7a, #1a73e8 and its 0.1 alpha, #4285f4 at 0.1, #34a853 at 0.7, rgba(0,0,0,0.05), surface #ffffff). The CSS swaps map onto tokens with equal values (--cudly-surface and --cudly-primary-fg #ffffff, --cudly-surface-muted #f8f9fa, --cudly-primary #2563eb), and no token is overridden anywhere (no dark mode or data-theme). Chart.defaults values match Chart.js 4.5.1.
    I re-counted CSS literals with the test's regex: HEAD 113, main 121. No ID selectors produced false positives. No color literal is left in any TS file outside lib/chart-theme.ts. fix(frontend): partial bulk purchase failure closes the modal, no retry of failed buckets #757 (1390, e2858b9) and feat(frontend): wire Azure RI exchange compatible-offerings + execute UI (refs #596) #81 (1325, 8ffadd6) are still open, merge-tree clean with HEAD, and add no color literals. Jest in a scratch worktree (chart-theme, dashboard, trends, savings-history, ladder-timeline
    ): 296 passed, 1 skipped.

@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 4677d7cb merged onto current main (23b0cb05; merge-tree 02c60d19; Linux, Node, headless Chromium):

  • This branch's own checks passed at this head: lint, typecheck, build, the full jest suite, the full Playwright e2e suite, and the git-secrets checks (see Verification in the PR body).
  • The combined tree of every UI PR in this merge wave (tree 02c60d19, main plus 1 PRs) passes npm ci, lint, typecheck, build, jest (1 skipped, 4426 passed, 4427 total), and the full Playwright suite (265 passed).
  • CI is green on 4677d7cb (CI Success, Run pre-commit hooks).
  • The independent review above approves this exact head.
  • Nothing was applied to any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 25, 2026 00:13
@IAmJSD
IAmJSD merged commit 5584234 into LeanerCloud:main Oct 25, 2026
29 checks passed
@IAmJSD

IAmJSD commented Oct 25, 2026

Copy link
Copy Markdown
Collaborator Author

Marked ready and merged by IAmJSD's agent, at IAmJSD's request (IAmJSD reviewed the before/after screenshots; cristim is traveling), after the independent review and local verification above, all at 4677d7cb.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/l Weeks impact/all-users Affects every user priority/p2 Backlog-worthy severity/high Significant harm triaged Item has been triaged type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant