Spend Center: exercise the charts for the first time, fix unreadable bars (#533) - #534
Merged
Conversation
…bars (#533) This child was filed to BUILD spend visuals. Recon found the budget burn meter and per-stage bars already shipped — but no canonical fixture ever wrote metrics.json, so their drawn path had never rendered in a test or a demo. So the work became: exercise them, pin them, and fix what that exposed. fixtureSpendRun adds real cost telemetry (cumulative + per-stage cost and tokens) under a project cap. Seven source tests now pin the drawn path AND the honest-null path (no meter without telemetry, no bars without stage cost, existing copy verbatim) plus a browser journey rendering both side by side: a run with spend beside one without. The first real render exposed a genuine defect: .stage-bar-row fixed its label/value columns at 180px/110px, so inside the Spend Center's narrow panel the track collapsed to NINE pixels — a 3x cost difference drew as 4px vs 3px. The percentages were correct and the chart was unreadable, which markup assertions cannot see (the #526 lesson). Flexible columns keep the track dominant wherever the component sits; the label ellipses instead of the data vanishing. Pinned by geometry: track >= 80px and a visible delta between the largest and smallest stage. DROPPED, deliberately: the issue also proposed an amber "approaching cap" band at 80%. dashboard-money-pages already forbids exactly that — "uses the exact loop boundary without an invented near threshold" — and it is right: run_budget_usd is enforced in code and the loop stops AT the cap, so 80% is a judgment the harness never makes and the UI must not assert. I implemented it, hit that test, and reverted; the prohibition is now pinned in this child's own suite so the idea is not revived by a future reader of #533. Closes #533. Part of epic #500. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Strix is installed on this repository, but we couldn't run this PR security review because this workspace's trial has ended. Add a card to resume code reviews here. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesSpend Center visual validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoSpend Center: add spend telemetry fixture and fix stage bar readability
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
300 rules✅ Skills:
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+33
to
+70
| await page.waitForFunction(() => { | ||
| const el = document.getElementById('page-cost-budget'); | ||
| return el && el.offsetParent !== null; | ||
| }, null, { timeout: 5000 }); | ||
| } | ||
|
|
||
| suite.browserTest('the spend page draws real telemetry and stays honest about the run without it (#533)', { seed: seedSpend, tmpPrefix: 'rstack-browser-533-' }, async ({ page, server }) => { | ||
| await openSpendCenter(page, server); | ||
| const spend = await page.evaluate(() => ({ | ||
| meters: [...document.querySelectorAll('#page-cost-budget .budget-fill')].length, | ||
| rows: [...document.querySelectorAll('#page-cost-budget .budget-row')].map((row) => row.textContent.replace(/\s+/g, ' ').trim()), | ||
| bars: [...document.querySelectorAll('#cost-budget-stages .stage-bar-row')].map((row) => row.textContent.replace(/\s+/g, ' ').trim()), | ||
| })); | ||
| assert.equal(spend.meters, 1, 'exactly one burn meter — the run WITH telemetry'); | ||
| assert.equal(spend.rows.length, 2, 'both runs are listed under the cap'); | ||
| assert.ok(spend.rows.some((row) => /no spend recorded yet against this cap/.test(row)), | ||
| 'the run without telemetry says so instead of showing a 0% meter'); | ||
| assert.equal(spend.bars.length, 3, 'three stages carry persisted cost'); | ||
| assert.ok(spend.bars.some((bar) => bar.includes('$3.55')), 'with exact persisted values'); | ||
| }); | ||
|
|
||
| suite.browserTest('per-stage bars are actually readable, not 9px slivers (#533)', { seed: seedSpend, tmpPrefix: 'rstack-browser-533-' }, async ({ page, server }) => { | ||
| await openSpendCenter(page, server); | ||
| const bars = await page.evaluate(() => [...document.querySelectorAll('#cost-budget-stages .stage-bar-row')].map((row) => ({ | ||
| label: row.querySelector('.stage-bar-label').textContent.trim(), | ||
| trackPx: Math.round(row.querySelector('.stage-bar-track').getBoundingClientRect().width), | ||
| fillPx: Math.round(row.querySelector('.stage-bar-fill').getBoundingClientRect().width), | ||
| }))); | ||
| assert.equal(bars.length, 3); | ||
| for (const bar of bars) { | ||
| assert.ok(bar.trackPx >= 80, | ||
| `the bar track needs room to mean anything — ${bar.label} got ${bar.trackPx}px`); | ||
| } | ||
| // $3.55 vs $1.25 must be visibly different, not 4px vs 3px. | ||
| const biggest = bars.find((bar) => bar.label.includes('07-code')); | ||
| const smallest = bars.find((bar) => bar.label.includes('08-testing')); | ||
| assert.ok(biggest.fillPx - smallest.fillPx >= 20, | ||
| `a ~3x cost difference must be visible — ${biggest.fillPx}px vs ${smallest.fillPx}px`); |
There was a problem hiding this comment.
3. Magic numbers in spend tests 📜 Skill insight ⚙ Maintainability
dashboard-533-spend.test.js introduces multiple hard-coded numeric thresholds (e.g., timeouts and pixel thresholds) instead of named constants, reducing clarity and making future tuning harder. This violates the requirement to replace magic numbers with named constants.
Agent Prompt
## Issue description
`tests/browser/dashboard-533-spend.test.js` uses hard-coded numeric literals (e.g., `5000`, `80`, `20`) in test logic.
## Issue Context
Compliance requires replacing magic numbers with named constants to make intent explicit and avoid scattered tuning.
## Fix Focus Areas
- tests/browser/dashboard-533-spend.test.js[33-36]
- tests/browser/dashboard-533-spend.test.js[62-70]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo, all three real: 1. Seed order silently changed the chart. Every fixture's projectShell rewrites .rstack/budget.json, so composing spend-then-blocked let the default $25 cap overwrite the spend fixture's explicit $10. I SAW this during live verification — the demo rendered $25 — and waved it off as a harmless artifact; it is a real order dependency. The seed now runs fixtureSpendRun last and the journey asserts the expected cap, so a future reordering fails loudly instead of quietly redrawing the meter. 2. The browser test dereferenced querySelector/find results unguarded, so a regression would surface as a TypeError instead of a diagnosis. 3. .stage-bar-value sits in a bounded column but had no overflow policy — Run Analytics prints cost AND tokens there, which would wrap and re-inflate the row. It ellipses now. Refs #533 (PR #534 review follow-up). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
All three Qodo findings applied — all three were real. The second one is worth calling out: every fixture's Also: guarded the browser test's derefs so regressions report a diagnosis rather than a TypeError, and gave Core 1921/1921, browser 32/32, lint clean. — Claude Main |
5 tasks
This was referenced Aug 1, 2026
Merged
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
2 participants
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.
Closes #533 · third implementation child of epic #500.
What this child turned out to be
It was filed to build spend visuals. Recon found the budget burn meter and per-stage bars already shipped — but no canonical fixture ever wrote
metrics.json, so their drawn path had never rendered in a test or a demo. The work became: exercise them, pin them, and fix what that exposed.fixtureSpendRun— real cost telemetry (cumulative + per-stage cost and tokens) under a project cap.The defect the first real render exposed
.stage-bar-rowfixed its label/value columns at180px/110px, so inside the Spend Center's narrow panel the track collapsed to nine pixels — a 3× cost difference drew as 4px vs 3px:Percentages correct, chart unreadable — invisible to markup assertions (the #526 lesson). Flexible columns now keep the track dominant wherever the component sits; the label ellipses instead of the data vanishing. Pinned by geometry: track ≥ 80px and a visible delta between largest and smallest stage.
Dropped, deliberately
The issue also proposed an amber "approaching cap" band at 80%.
dashboard-money-pagesalready forbids exactly that — "uses the exact loop boundary without an invented near threshold" — and it is right:run_budget_usdis enforced in code and the loop stops at the cap, so 80% is a judgment the harness never makes and the UI must not assert. I implemented it, hit that test, and reverted. The prohibition is now pinned in this child's own suite so the idea isn't revived by the next reader of #533.Verification
Core 1921/1921 · browser 32/32 (2 new) · lint 0 · typecheck 0 · validate 196 · security green · live-verified (screenshot in thread: $6.20 measured against the cap, three stage bars with exact values, and the no-telemetry run correctly showing "— of $25.00").
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests