Skip to content

fix(ladder): show cumulative commitment as a stepped stacked chart - #823

Merged
cristim merged 4 commits into
mainfrom
fix/ladder-timeline-697-698
Oct 12, 2026
Merged

cristim merged 4 commits into
mainfrom
fix/ladder-timeline-697-698

Conversation

@cristim

@cristim cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member

Chart changes for #698: stepped stacked line of cumulative on-demand-equivalent $/hr per layer, base = selected run's existing commitment (0 labelled planned additions only without a run), dashed target line when target_usd_hr is non-null. Created once and updated in place, not built for empty data, colors from --cudly-* tokens (missing token throws), aria-label summary and a cumulative table column with caption. Also: sub-0.0001 amounts read <$0.0001/hr; edit reasons use readable run/tranche status (follow-ups from #820).

Not covered: chart disposal on navigate-away (#667 residue) and the non-ladder parts of #747.

Tests: jest (6 fail on main), full jest, tsc, lint, build, Playwright ladder-timeline specs. ladder-config.spec untouched (#802).

Closes #698

Generated with Claude Code

cristim and others added 3 commits October 12, 2026 22:39
Table, filters, summary, snapshot, tooltip and editor showed raw 6-decimal
USD strings, ISO timestamps and enum codes. Reuse formatCurrency, formatDate,
formatTerm and getPaymentLabel; add a label map for layer and status enums
that shows unknown codes raw. Dates render in UTC, amounts under $1 keep four
digits, and absent snapshots read 'unavailable' rather than zero.

Closes #697

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
The timeline chart was a scatter of single tranches, built before the empty
check, with hardcoded colors. Render a stepped stacked line of cumulative
on-demand-equivalent $/hr per layer, starting from the selected run's existing
commitment (0, labelled planned additions only, without a run), with a dashed
target line when the run has one. The chart is created once and updated in
place, skipped for empty data, colored from --cudly-* tokens (missing token
fails loudly), and described by an aria-label plus a cumulative table column.
Also show sub-0.0001 amounts as <$0.0001/hr and readable run status in edit
reasons.

Closes #698

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim cristim added priority/p2 Backlog-worthy severity/high Significant harm urgency/this-quarter Within the quarter impact/many Affects most users effort/m Days type/bug Defect triaged Item has been triaged labels Oct 12, 2026
@coderabbitai

coderabbitai Bot commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 6 billable files and costs up to $1.50.

  • Ask an admin to make reviews automatic

Open in CodeRabbit

Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing.

Or wait 20 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Repository: LeanerCloud/cloud-commitments-platform/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 3f000bf5-f1ff-49d9-80fa-f7d3e62f079d

📥 Commits

Reviewing files that changed from the base of the PR and between 1372057 and a315e27.


📒 Files selected for processing (6)
  • frontend/src/__tests__/ladder-timeline-filters.test.ts
  • frontend/src/__tests__/ladder-timeline.test.ts
  • frontend/src/ladder-timeline-chart.ts
  • frontend/src/ladder-timeline-filters.ts
  • frontend/src/ladder-timeline.ts
  • frontend/tests-e2e/ladder-timeline.spec.ts

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Gate review: CHANGES REQUESTED at c40f80e

Reviewer: independent merge-gate agent (did not write this PR). Head confirmed c40f80e5, up to date with main.

Blocking finding

Cancelled and failed tranches are summed into the cumulative "planned commitment". frontend/src/ladder-timeline-filters.ts:302-316 (ladderCumulative) adds every event it is given, and frontend/src/ladder-timeline.ts passes visible (all statuses unless the user picks a status filter). The chart, the aria-label summary and the new table Cumulative column therefore present never-bought tranches as committed.

This is the normal case, not an edge: cancelOverdueLadderTranchesTx (internal/config/store_postgres_ladder.go) flips every overdue scheduled tranche to cancelled on each run save, and the next run re-plans that gap. With the default "All runs" view the replanned tranche and the cancelled original are both summed, so the total is double-counted. The backend in-flight definition (GetInFlightLadderCommitUSDHr, plan ladder-118-tranche-status.md) counts scheduled (+ fired once the executor lands) and excludes cancelled/failed because they were never bought.

Reproduced on the PR head (reviewer-written jest, not committed):

const evs = [ev('a','compute-sp','1.000000','2030-01-01T00:00:00Z'),
             ev('x','compute-sp','5.000000','2030-02-01T00:00:00Z','cancelled'),
             ev('y','compute-sp','7.000000','2030-03-01T00:00:00Z','failed')];
ladderChartModel(evs, undefined).cumulative.totals
// => [1000000n, 6000000n, 13000000n]   expected the last value 1000000n
// summary: "...planned additions only: $1.00/hr on Jan 1, 2030 rising to $13.00/hr on Mar 1, 2030."

Suggested fix: count only scheduled, fired and completed tranches in ladderCumulative (state the rule in its doc comment, cite the backend netting). Keep cancelled/failed rows in the table, with the Cumulative cell showing that they are not counted (e.g. "not counted (cancelled)") instead of a running total, and add a regression test like the one above plus a table-level assertion.

Checked and fine

  • Math: bigint units end to end, Number only at the chart edge; sorted by scheduled_date; ties and several tranches on the same day/layer collapse onto one step; 14-digit amounts exact; zero-amount tranches kept (reviewer tests pass).
  • Base = run existing_usd_hr, 0 labelled "planned additions only" without a run; target dataset only when target_usd_hr is non-null; empty selection destroys/hides and never constructs; one Chart updated in place on filter change (chart.update('none')).
  • Unit label on y axis, legend title, table Budget/Cumulative headers, caption and aria-label.
  • Local (from git archive c40f80e5, under the heavy-run lock): npm ci ok; jest --maxWorkers=2 102 suites, 3229 passed / 1 skipped; tsc --noEmit 0; lint 0 errors (125 pre-existing warnings); npm run build 0.

Non-blocking notes

  • Tooltip reads "Convertible RI: $0.1235/hr cumulative" without the on-demand-equivalent unit; consider appending it.
  • Layers differ by color only in the plot (legend labels, tooltip and table carry the names, target is dashed); patterns were in the plan, not required.
  • Navigate-away disposal residue (fix(ladder): timeline editor 409 recovery, focus and chart cleanup #667): at most one Chart instance stays alive until the next initLadderTimeline call disposes it; bounded, not a growing leak. Not blocking.

CI was still queued at review time; git-secrets scan and Playwright not run because the PR is blocked on the finding above. Re-gate at the new head once fixed.

…mulative

Cancelled, failed and unrecognised-status tranches were added to the chart,
aria-label summary and Cumulative column, double counting plans the backend
cancels and re-plans. They stay visible in the table marked 'not counted'.
The tooltip now states the unit and layers differ by point shape as well as
color. A selection with no counted tranche draws no chart.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Addressed the gate (c40f80e): the cumulative now counts only scheduled, fired and completed tranches; cancelled, failed and unknown statuses stay visible in the table as 'Not counted' / 'Not counted (unknown status)'. Repro scheduled 1.0 + cancelled 5.0 + failed 7.0 now gives $1.00/hr (regression test). aria-label and table caption state the counted statuses. Tooltip includes the on-demand-equivalent unit; layers also differ by point shape. A selection with no counted tranche draws no chart. Verification at a315e27: full jest, tsc, lint, build, Playwright ladder-timeline specs; git secrets scan exit 0 under BSD grep and GNU grep (ggrep).

@cristim

cristim commented Oct 12, 2026

Copy link
Copy Markdown
Member Author

Gate re-review: APPROVED at a315e27

Independent merge-gate agent (did not write this PR). Delta reviewed against c40f80e; head up to date with main.

Previous blocker resolved. ladderCumulative now counts only scheduled, fired and completed tranches. Cancelled, failed and unknown statuses get a null row total and appear in the table as "Not counted" / "Not counted (unknown status)". When nothing is counted, ladderChartModel returns undefined, so the chart is not built. The aria-label and the caption both name the counted statuses. The tooltip now shows the on-demand-equivalent unit, and layers differ by point shape (usePointStyle legend).

Reviewer-written jest at this head (not committed), 3/3 pass:

  • my earlier repro (1.0 + cancelled 5.0 + failed 7.0): totals [1000000n], aria "$1.00/hr ... rising to $1.00/hr", cancelled row total null
  • mixed fired 0.1 + completed 0.2 + cancelled 0.3 (same day) + scheduled 0.000001 + unknown-status 9.0 with base 2.0: exact steps [2300000n, 2300001n], per-layer units and row totals correct, uncounted rows null
  • all rows cancelled/failed: model undefined (no chart)

Local evidence (fresh git archive a315e27e, under the heavy-run lock):

  • npm ci ok
  • jest --maxWorkers=2: 103 suites, 3235 passed, 1 skipped
  • tsc --noEmit 0
  • lint 0 errors (125 pre-existing warnings)
  • build 0
  • Playwright tests-e2e/ladder-timeline.spec.ts: 2 passed (real Chart.js against built dist)

git-secrets: fresh clone at this SHA, --register-aws, git ls-files -z | xargs -0 git secrets --scan: exit 0 under BSD grep 2.6.0 and exit 0 under GNU grep 3.12.

CI: all checks green on this SHA, including CI Success, Build frontend, Playwright Chromium, E2E Tests and pre-commit. mergeStateStatus CLEAN. Labels mirror #698 and the body has Closes #698. No UX reviewer comment had been posted at merge time.

Non-blocking: navigate-away disposal residue (#667) leaves at most one Chart instance alive until the next init.

@cristim
cristim merged commit 6c4be95 into main Oct 12, 2026
27 checks passed
@cristim

cristim commented Oct 14, 2026

Copy link
Copy Markdown
Member Author

Post-merge visual review: #820 / #823

Reviewer: cx-rv-ux-visual, successor to rv-ux-visual. Verdict: one medium follow-up remains; the principal H4/H5 improvements are verified.

Reviewed main 8f330014ba20563428d7e64f5f2a0653af8e4ffb, containing #820 (ad0a97e566c3092406573e72f3d037a49e5f6ace, merge 137205778f98e931d18eec5bf86806967c4d9fdd) and #823 (a315e27eac4ca9ef667ef09b3c98367667f08346, merge 6c4be959aade00a236e429666e9e5c8a0015069f). Compared production bundles against pre-#820 20f6f4fb860de1c1f604f89f30bc72ab8837eee3, built from isolated exports with identical dependency lockfiles. No branch, source checkout, purchase or deployment changed.

Medium: tooltip date title renders epoch milliseconds

frontend/src/ladder-timeline-chart.ts:88-91 supplies a tooltip label callback but leaves its title at Chart.js's default. The linear x scale supplies the timestamp as a number. Open Purchasing policies, clear timeline filters, select a run and hover a plotted point: the tooltip title displays e.g. 1,791,102,970,515 instead of the scheduled UTC date. This was reproduced by intercepting the canvas's actual fillText calls and independently reading the captured screenshot. The body correctly reads Compute Savings Plan: $0.2500/hr cumulative, on-demand-equivalent $/hr.

Suggested follow-up: provide a tooltip title callback using the point's x timestamp and the shared UTC date/time formatter. Add an assertion on the rendered tooltip date, since table formatting tests do not exercise the default Chart.js title. #697 is closed; this is residual H4 date formatting, not a reason to revert the chart. No duplicate issue was filed.

Verified

  • Playwright Chromium at 1280 / 768 / 390: 3/3 review scenarios passed. Page widths equal viewport widths; zero page errors; axe WCAG A/AA checks on the ladder section report zero violations at each width. CSP bypass was used only to inject axe instrumentation.
  • Before screenshots show raw enum codes, ISO dates, six decimal amounts and a scatter plot. After screenshots show readable labels, UTC dates, formatted amounts, stepped cumulative layers, existing commitment and dashed target.
  • Tiny nonzero amount 0.000001 renders <$0.0001/hr rather than zero. Editor labels are formatted; editor opens and Escape closes at each width.
  • Cancelled/failed rows remain visible as Not counted. Fixture base 7.1 plus counted additions 3.000001 gives cumulative 10.100001, displayed $10.10/hr. Cancelled 0.625 and failed 0.75 are excluded. Cancelled-only selection hides the chart.
  • Unit appears on axis, legend title, tooltip body and table headers. Canvas accessible name states the base/target and status-counting rule. Layer legend names are mapped, point shapes distinguish the layers, and target is dashed. This does not establish complete color-vision accessibility; axe cannot assess canvas contrast.
  • Existing ladder spacing/date-input polish remains tracked in open fix(frontend): ladder planned-purchases block has no layout styling and locale date inputs #753. Wide table scrolls within its container on mobile. No additional finding filed.

Evidence is fixture-based, using a production webpack bundle with mocked API responses, not a live backend or cloud purchase. Local captures: screens/ladder-{before,main,detail,editor}-{1280,768,390}.png, screens/tooltip-main-1280.png, and screens/axe-{1280,768,390}.json under the isolated review evidence directory. Before/detail naming also includes ladder-before-detail-<width>.png. Sticky header duplication within full-page crops is a capture artifact.

Recovery note: saved coordinator GO messages dated 2026-10-12 authorized this combined post-merge review. The original session had missed those files and incorrectly reported no GO. Original histories remain preserved. Original frontend PR watcher PID 93993 was found alive; no duplicate watcher started. No PRs, branches or worktrees owned by this reviewer.

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

Labels

effort/m Days impact/many Affects most users 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.

fix(ladder): timeline chart is a scatter and never shows cumulative commitment

1 participant