feat(cost): add settings cost dashboard with global tracker, dashboard RPCs, and charts#2762
Conversation
… RPC handlers - Introduced a global to manage cost data across the application. - Added RPC handlers for fetching cost dashboard data, including daily history and summary. - Updated to enable cost tracking by default and added configuration for the dashboard. - Implemented data structures for daily cost entries and budget status. - Enhanced the to record usage and provide historical data for the dashboard. - Ensured that the dashboard can function even if the global tracker is uninitialized, using a fallback mechanism.
…ings - Introduced , , , , and components for visualizing cost data. - Implemented to aggregate and display cost-related metrics. - Added for loading states in the cost dashboard. - Updated page to include a route for the new cost dashboard. - Enhanced user experience with responsive design and dark mode support.
…ost dashboard panel, format currency, model cost table, and cost dashboard hook - Created comprehensive unit tests for the BudgetSummary component to validate rendering and status display. - Added tests for the colorForCost function to ensure correct color coding based on spending thresholds. - Implemented tests for the CostDashboardPanel to verify loading states and error handling. - Developed tests for currency formatting functions to ensure accurate currency and token representation. - Added tests for the ModelCostTable to confirm correct rendering of model data. - Created tests for the useCostDashboard hook to validate data fetching and error handling.
- Added translations for the cost dashboard in English, Arabic, Bengali, German, Spanish, French, Hindi, Indonesian, Italian, Korean, Portuguese, Russian, and Chinese (Simplified). - Included key phrases related to cost tracking, budget limits, and utilization metrics to enhance user experience across different locales.
…ieval - Implemented a new test to verify that usage recording bypasses the disabled gate. - Added tests for retrieving daily history, ensuring it returns the correct number of days and excludes out-of-window records. - Included tests for dashboard calculations, confirming accurate period totals and budget status based on recorded costs. - Enhanced the test suite for the CostTracker to improve coverage and reliability of cost tracking functionalities.
… recharts and d3 type definitions - Added recharts version 2.15.4 to package.json for enhanced charting capabilities. - Updated pnpm-lock.yaml to include new type definitions for d3 packages, ensuring better TypeScript support. - Included additional dependencies such as decimal.js-light and eventemitter3 for improved functionality.
📝 WalkthroughWalkthroughAdds a 7-day cost and token dashboard: Rust telemetry/tracker, JSON-RPC endpoints, config defaults, i18n strings, a polling React hook, formatting utilities, recharts-based charts and tooltip/table/summary components, Settings route wiring, and tests. ChangesCost Dashboard Telemetry & Backend
Frontend Data Fetching & Formatting
React Components & Visualization
Integration & Internationalization
Sequence Diagram(s) sequenceDiagram
participant Browser
participant Hook as useCostDashboard
participant RPC as openhuman.cost_get_dashboard
participant Tracker as CostTracker
Browser->>Hook: mount or refetch()
Hook->>RPC: call openhuman.cost_get_dashboard
RPC->>Tracker: get_dashboard()
Tracker-->>RPC: CostDashboard DTO
RPC-->>Hook: RpcOutcome{result}
Hook->>Browser: provide data to CostDashboardPanel
Browser->>CostBarChart: render days, thresholds
Browser->>TokenUsageChart: render token bars
Browser->>ModelCostTable: render rows
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
coderabbitai
Bot
added
feature
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@YellowSnnowmann hey — CI is still pending (Build & smoke-test core image, E2E on Windows/macOS, Rust Core Coverage) so I'll hold the formal approval until those green up. That said, I read through the whole diff carefully and have a few things worth sorting out before this lands.
What this does
Adds a full Settings → Cost Dashboard page: 7-day cost/token bar charts, budget summary, per-model breakdown table, polling hook, and three new JSON-RPC controllers (cost_get_dashboard, cost_get_daily_history, cost_get_summary). Activates a process-global CostTracker singleton at bootstrap and wires it into the agent turn loop so records accumulate automatically. Extends config with cost.dashboard.* settings and flips cost.enabled default to true.
Findings
[major] cost.enabled = false no longer prevents data collection (src/openhuman/cost/global.rs)
The record_provider_usage function calls record_usage_unconditional, which bypasses the cost.enabled gate. The comment explains the intent — observability stays on even when budget enforcement is off — but this is a silent semantic breaking change. Anyone who set cost.enabled = false expecting zero cost telemetry to be collected now has their JSONL populated without knowing it. The flag was previously documented as "Enable cost tracking"; it now means "enable budget enforcement" only. At minimum, emit a log::warn! at init_global when cost.enabled = false so operators can see the semantics changed, and update the config field doc comment to reflect the new meaning.
[major] cost.enabled default flip is a silent opt-in for existing deployments (src/openhuman/config/schema/identity_cost.rs)
Changing the default from false to true means every existing deployment that omitted cost.enabled will silently start writing cost JSONL on next restart after upgrading. No migration hint, no startup notice. A log line at init_global noting cost tracking is now on by default would at least make it visible in logs on first boot with the new version.
[minor] Issue AC mismatch — SQLite vs JSONL
Issue #1850 explicitly states "SQLite persistence — daily totals survive dashboard restarts." This PR uses JSONL. The functional requirement (persistence across restarts) is met, but the PR closes the issue without acknowledging the deviation. Add a note to the PR description explaining why JSONL was chosen so the issue can be closed cleanly.
[minor] recharts bundle weight (app/package.json)
recharts pulls in d3-* transitively (~130KB gzipped). Components import from 'recharts' as a barrel. Confirm your Vite config is actually tree-shaking this (run a bundle analysis) — if not, named chunk-level imports or a lighter alternative (e.g. uplot) may be worth considering.
[minor] resolve_tracker fallback on every RPC call (src/openhuman/cost/rpc.rs)
If the global init failed at boot (bad workspace path), every subsequent dashboard RPC poll will attempt and fail to build a fresh tracker, hitting the filesystem on each call. Consider caching the failure state so repeated polls don't spam the error path.
[minor] Initial fetch fires even when paused = true (app/src/hooks/useCostDashboard.ts)
void fetchOnce() fires unconditionally on mount before the if (paused) return guard kicks in on the interval. If the intent is "no fetch when paused", gate the initial call too. If the intent is "one fetch on mount then pause", add a comment — it's not obvious.
What's solid
The Rust side is clean — OnceCell singleton, idempotent init, zero panics, no .unwrap() in production paths, sensible fallback tracker construction in RPC handlers, errors logged and never propagated to turn execution. Test coverage on the tracker is comprehensive: gap filling, clamp behaviour, budget status thresholds, record_usage_unconditional bypass, model sort order. Frontend polling hook handles cancellation on unmount correctly. i18n coverage across 10 locales is good (English fallback strings consistent with existing pattern in the file).
Fix the two behavioural issues above (unconditional tracking semantics + silent upgrade opt-in) and address the SQLite AC note in the PR description. Once CI is green and those are sorted I'll approve.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
app/src/components/dashboard/CostDashboardPanel.tsx (1)
24-30: ⚡ Quick winMove the 1s tick to a small child component to avoid whole-panel rerenders.
The interval state currently lives in the panel, so charts/table rerender every second. Extract the “updated ago” pill into a child that owns the tick state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard/CostDashboardPanel.tsx` around lines 24 - 30, The interval and setTick in CostDashboardPanel cause full-panel rerenders; extract that logic into a small child (e.g., UpdatedAgoPill or UpdatedAgoTicker) which owns its own useState/useEffect interval (replacing the current useEffect and setTick in CostDashboardPanel), accept only the timestamp/lastUpdated prop from the parent, and render the "Updated Ns ago" UI there; then replace the pill markup in CostDashboardPanel with this child component (optionally wrap the child in React.memo) so only the pill re-renders each second instead of the whole panel.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/components/dashboard/CostBarChart.tsx`:
- Line 193: The code in CostBarChart uses
t('settings.costDashboard.today').toUpperCase(), which applies locale-unsafe
uppercasing; remove the .toUpperCase() call and render
t('settings.costDashboard.today') directly (or update the translation string to
an already-cased value if uppercase is desired) so locale-specific casing rules
are preserved; locate the usage in the CostBarChart component where
t('settings.costDashboard.today') is invoked and delete the .toUpperCase()
transformation.
In `@app/src/components/dashboard/formatCurrency.ts`:
- Around line 77-86: relativeTime currently returns hard-coded English strings;
replace those literals by using the app's i18n hook (useT) and translation keys
so UI text is localized. Update the relativeTime implementation to accept or
obtain a translation function (useT) and map each branch ("Just now", seconds,
minutes, hours, days) to calls like t('relative.justNow') or
t('relative.secondsAgo', {count: deltaSec}) etc., returning the localized string
and ensuring numeric values are passed as interpolation parameters; keep the
same branches and return semantics but remove hard-coded English literals from
relativeTime.
In `@app/src/components/dashboard/ModelCostTable.tsx`:
- Around line 59-60: The displayed percentage text can diverge from the clamped
bar width because the code clamps the bar using sharePct but still renders the
raw row.percent_of_total; update the render to use the clamped sharePct wherever
the percentage is shown so the text matches the bar (change references to
display row.percent_of_total to use sharePct in ModelCostTable, including the
other occurrences around lines referencing the same logic e.g., the block using
sharePct and the code between the other mentioned ranges).
- Around line 73-76: The fallback provider label is hard-coded as '—'; update
the component to use the i18n hook instead: import and call useT() from
app/src/lib/i18n/I18nContext inside the component that renders ModelCostTable,
replace the literal {row.provider ?? '—'} with the localized string using
t('modelCostTable.unknownProvider') (or a similarly scoped key), and add that
key/value to the app i18n files for all supported locales. Ensure
providerChipClass(row.provider) logic remains unchanged and that the new t call
handles null/undefined row.provider.
In `@app/src/hooks/useCostDashboard.ts`:
- Around line 111-125: The effect currently returns early when paused, so the
cleanup that sets cancelledRef.current to true and clears the interval never
runs; change the effect to always register the cleanup: set cancelledRef.current
= false and call fetchOnce(), then only create the interval when paused is false
(e.g., let interval: number | undefined; if (!paused) interval =
window.setInterval(() => void fetchOnce(), Math.max(1000, refreshMs));) and at
the end always return a cleanup that sets cancelledRef.current = true and clears
the interval if it was created (window.clearInterval(interval!)); keep
references to fetchOnce, refreshMs, paused in the dependency array.
In `@src/openhuman/agent/harness/session/turn.rs`:
- Around line 736-738: The inline comment above the call to
crate::openhuman::cost::record_provider_usage(&effective_model, usage) is
incorrect: record_provider_usage performs unconditional telemetry capture (it is
not a no-op when cost.enabled = false). Update the comment to state that this
call always records provider usage/telemetry and note any gating happens inside
record_provider_usage rather than implying the call itself is skipped; reference
the effective_model and usage parameters so the comment explains what is being
recorded.
In `@src/openhuman/cost/global.rs`:
- Around line 29-38: The code currently calls
GLOBAL_TRACKER.set(Arc::new(tracker)) and then unconditionally logs success,
which can misreport when set() fails under concurrent init; change the logic in
the init path that uses CostTracker::new(config, workspace_dir) to inspect the
Result from GLOBAL_TRACKER.set(...) and only emit the log::info about
initialization (including workspace_dir.display()) when set() returns Ok(()); if
set() returns Err(_), avoid the success log and either ignore or log a different
debug/trace message indicating another initializer won the race.
In `@src/openhuman/cost/rpc.rs`:
- Around line 158-200: Add structured entry/exit and branch diagnostics around
tracker resolution and each RPC path: in resolve_tracker(log on entry), emit a
stable grep-friendly prefix (e.g., "COST:RESOLVE:ENTRY") and a correlation_id
field if available; log whether try_global() returned a global tracker
("COST:RESOLVE:BRANCH global") or you constructed a fallback
("COST:RESOLVE:BRANCH fallback") and on any error produce a "COST:RESOLVE:ERROR"
log with the error context (redacting any sensitive workspace path if
necessary). In dashboard, daily_history, and summary add entry ("COST:RPC:ENTRY
<method>"), before calling CostTracker methods log an external-call line
("COST:RPC:CALL <method> -> get_dashboard|get_daily_history|get_summary"), on
errors log "COST:RPC:ERROR <method>" with the error context and correlation_id,
and after successful dto construction/serialization log exit/success
("COST:RPC:EXIT <method>") including stable fields like method name, count of
entries (no PII), and duration if feasible; ensure all logs use the same
prefixes and redact sensitive data.
In `@src/openhuman/cost/schemas.rs`:
- Around line 100-126: Add structured, request-scoped entry/exit/error logs
inside each controller handler (handle_cost_get_dashboard,
handle_cost_get_daily_history, handle_cost_get_summary): at handler start emit
an "ENTRY" log with a stable controller name and a generated correlation id
(e.g., request_id/uuid) but do not log raw params; before/after each external
call (config_rpc::load_config_with_timeout and
cost_rpc::{dashboard,daily_history,summary}) emit "CALL" and "RETURN" logs
including the correlation id and short outcome (success/failure, duration,
retry/timeout markers); on errors (map_err branches and serde_json::from_value)
emit an "ERROR" log with the correlation id and sanitized error message; finally
emit an "EXIT" log with the controller name, correlation id and final outcome.
Use the same correlation id across the async block so logs are traceable per
request.
---
Nitpick comments:
In `@app/src/components/dashboard/CostDashboardPanel.tsx`:
- Around line 24-30: The interval and setTick in CostDashboardPanel cause
full-panel rerenders; extract that logic into a small child (e.g.,
UpdatedAgoPill or UpdatedAgoTicker) which owns its own useState/useEffect
interval (replacing the current useEffect and setTick in CostDashboardPanel),
accept only the timestamp/lastUpdated prop from the parent, and render the
"Updated Ns ago" UI there; then replace the pill markup in CostDashboardPanel
with this child component (optionally wrap the child in React.memo) so only the
pill re-renders each second instead of the whole panel.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4f894168-2539-4f3c-b4af-2ca766c91fbb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (41)
app/package.jsonapp/src/components/dashboard/BudgetSummary.test.tsxapp/src/components/dashboard/BudgetSummary.tsxapp/src/components/dashboard/ChartTooltip.tsxapp/src/components/dashboard/CostBarChart.test.tsxapp/src/components/dashboard/CostBarChart.tsxapp/src/components/dashboard/CostDashboardPanel.test.tsxapp/src/components/dashboard/CostDashboardPanel.tsxapp/src/components/dashboard/DashboardSkeleton.tsxapp/src/components/dashboard/ModelCostTable.test.tsxapp/src/components/dashboard/ModelCostTable.tsxapp/src/components/dashboard/TokenUsageChart.tsxapp/src/components/dashboard/formatCurrency.test.tsapp/src/components/dashboard/formatCurrency.tsapp/src/hooks/useCostDashboard.test.tsapp/src/hooks/useCostDashboard.tsapp/src/lib/i18n/chunks/ar-1.tsapp/src/lib/i18n/chunks/bn-1.tsapp/src/lib/i18n/chunks/de-1.tsapp/src/lib/i18n/chunks/en-1.tsapp/src/lib/i18n/chunks/es-1.tsapp/src/lib/i18n/chunks/fr-1.tsapp/src/lib/i18n/chunks/hi-1.tsapp/src/lib/i18n/chunks/id-1.tsapp/src/lib/i18n/chunks/it-1.tsapp/src/lib/i18n/chunks/ko-1.tsapp/src/lib/i18n/chunks/pt-1.tsapp/src/lib/i18n/chunks/ru-1.tsapp/src/lib/i18n/chunks/zh-CN-1.tsapp/src/lib/i18n/en.tsapp/src/pages/Settings.tsxsrc/core/jsonrpc.rssrc/openhuman/agent/harness/session/turn.rssrc/openhuman/config/schema/identity_cost.rssrc/openhuman/cost/global.rssrc/openhuman/cost/mod.rssrc/openhuman/cost/rpc.rssrc/openhuman/cost/schemas.rssrc/openhuman/cost/tracker.rssrc/openhuman/cost/tracker_tests.rssrc/openhuman/cost/types.rs
…ime formatting - Updated the CostBarChart to use a localized key for the today label. - Modified the CostDashboardPanel to include a translation function for the last updated time. - Refactored the relativeTime function to accept a translation function, ensuring all time strings are localized. - Adjusted the ModelCostTable to display a localized message for unknown providers and updated percentage display logic. - Improved the useCostDashboard hook to ensure data is fetched on mount, even when polling is paused.
- Improved the cost tracking system to ensure telemetry is recorded even when budget enforcement is disabled. - Updated documentation to clarify the behavior of the cost tracking features, including the implications of the flag. - Added a fallback mechanism for the cost tracker to handle cases where the global tracker is unavailable, improving reliability. - Introduced new correlation IDs for logging in async handlers, enhancing traceability across dashboard requests.
…TokenUsageChart, and relative time formatting - Introduced unit tests for the ChartTooltip component to verify rendering of title, rows, and optional footer. - Added tests for the DashboardSkeleton component to ensure it renders with the correct accessibility attributes. - Implemented tests for the TokenUsageChart component to check rendering with both populated and empty data. - Enhanced the formatCurrency test suite by adding tests for the new relativeTime function, covering various time formats and localization.
coderabbitai
Bot
added
rust-core
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/components/dashboard/formatCurrency.ts`:
- Around line 99-112: The current code in formatCurrency.ts uses t(...) which
doesn't accept interpolation objects, so it manually calls .replace('{value}',
...) which only replaces the first occurrence; update those replacements to
replace all occurrences of "{value}" (e.g., use .replaceAll('{value}',
String(...)) or create/use a small helper like
replaceAllPlaceholders(keyedString, placeholder, value) and call that for
secondsAgo, minutesAgo, hoursAgo, daysAgo—change each .replace call in the block
to the new replaceAll/helper to ensure every "{value}" is substituted while
keeping the useT().t(...) calls unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d56aeb6d-2186-46d6-9d14-514253ba8b69
📒 Files selected for processing (28)
app/src/components/dashboard/ChartTooltip.test.tsxapp/src/components/dashboard/CostBarChart.tsxapp/src/components/dashboard/CostDashboardPanel.tsxapp/src/components/dashboard/DashboardSkeleton.test.tsxapp/src/components/dashboard/ModelCostTable.tsxapp/src/components/dashboard/TokenUsageChart.test.tsxapp/src/components/dashboard/formatCurrency.test.tsapp/src/components/dashboard/formatCurrency.tsapp/src/hooks/useCostDashboard.tsapp/src/lib/i18n/chunks/ar-1.tsapp/src/lib/i18n/chunks/bn-1.tsapp/src/lib/i18n/chunks/de-1.tsapp/src/lib/i18n/chunks/en-1.tsapp/src/lib/i18n/chunks/es-1.tsapp/src/lib/i18n/chunks/fr-1.tsapp/src/lib/i18n/chunks/hi-1.tsapp/src/lib/i18n/chunks/id-1.tsapp/src/lib/i18n/chunks/it-1.tsapp/src/lib/i18n/chunks/ko-1.tsapp/src/lib/i18n/chunks/pt-1.tsapp/src/lib/i18n/chunks/ru-1.tsapp/src/lib/i18n/chunks/zh-CN-1.tsapp/src/lib/i18n/en.tssrc/openhuman/agent/harness/session/turn.rssrc/openhuman/config/schema/identity_cost.rssrc/openhuman/cost/global.rssrc/openhuman/cost/rpc.rssrc/openhuman/cost/schemas.rs
✅ Files skipped from review due to trivial changes (3)
- app/src/components/dashboard/ChartTooltip.test.tsx
- app/src/lib/i18n/chunks/hi-1.ts
- app/src/lib/i18n/chunks/ru-1.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- app/src/lib/i18n/chunks/id-1.ts
- src/openhuman/agent/harness/session/turn.rs
- app/src/lib/i18n/chunks/ar-1.ts
- app/src/lib/i18n/chunks/zh-CN-1.ts
- app/src/hooks/useCostDashboard.ts
- app/src/lib/i18n/chunks/fr-1.ts
- app/src/components/dashboard/ModelCostTable.tsx
- app/src/components/dashboard/CostDashboardPanel.tsx
- app/src/lib/i18n/chunks/bn-1.ts
- src/openhuman/cost/global.rs
- app/src/lib/i18n/en.ts
- src/openhuman/cost/schemas.rs
- src/openhuman/config/schema/identity_cost.rs
- app/src/lib/i18n/chunks/ko-1.ts
- src/openhuman/cost/rpc.rs
- app/src/lib/i18n/chunks/es-1.ts
- app/src/lib/i18n/chunks/en-1.ts
- Modified the relativeTime function to use replaceAll for substituting every occurrence of the {value} placeholder in translation strings, ensuring accurate localization for repeated time formats.
- Added a new test case to verify that all instances of the {value} placeholder are replaced correctly in the relative time output.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/src/components/dashboard/formatCurrency.test.ts (3)
50-53:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerify the actual fallback content, not just the type.
The assertion
expect(typeof label).toBe('string')only confirms the return type. A bug returning"","undefined", or any other string would pass.Check the actual fallback value or at least verify it's a non-empty string to ensure the fallback logic works correctly.
🔍 Suggested improvement
it('falls back to the suffix for malformed input', () => { const label = shortDayLabel('not-a-date'); - expect(typeof label).toBe('string'); + expect(label).toBe('not-a-date'); // or check .toContain('not-a-date') if it transforms + // Alternatively, if the fallback is unknown: + // expect(label.length).toBeGreaterThan(0); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard/formatCurrency.test.ts` around lines 50 - 53, The test currently only checks the return type for shortDayLabel('not-a-date'); update it to verify the actual fallback content: either assert the exact expected suffix (e.g., expect(label).toBe('<expected-suffix>')) if the known fallback value exists, or at minimum assert it's non-empty with expect(label.length).toBeGreaterThan(0); replace the typeof assertion in the it block for shortDayLabel to one of these stronger checks so empty or placeholder strings fail the test.
45-48:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen the weekday label assertion.
The comment claims "3-letter weekday" but the assertion checks
>= 2, and only validates length rather than content. A bug returning"??"or"xy"would pass.Consider asserting a minimum length of 3 or matching a weekday pattern to verify the actual behavior described in the comment.
🔍 Suggested improvement
it('returns a 3-letter weekday for a valid ISO date', () => { const label = shortDayLabel('2026-05-27'); - expect(label.length).toBeGreaterThanOrEqual(2); + expect(label.length).toBeGreaterThanOrEqual(3); + expect(label).toMatch(/^[A-Za-z]{3}$/); // or just check >= 3 if locale varies });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard/formatCurrency.test.ts` around lines 45 - 48, The test for shortDayLabel currently only asserts length >=2 which contradicts the "3-letter weekday" intent; update the assertion in formatCurrency.test.ts to enforce a 3-letter weekday by checking either label.length === 3 and matching a weekday pattern (e.g., /^[A-Za-z]{3}$/) or asserting the label is one of the expected three-letter weekdays (Mon, Tue, Wed, Thu, Fri, Sat, Sun) so shortDayLabel('2026-05-27') cannot pass with "??" or other invalid values.
18-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTighten the assertion to avoid false positives.
The regex
/0/matches any string containing "0", including"$10.00"or"$100". IfformatCurrencyincorrectly returned"$10.00"forNaN, this test would still pass.Use a more specific pattern or exact string match to verify that non-finite inputs truly format as zero.
✅ Proposed fix
it('treats non-finite input as zero', () => { - expect(formatCurrency(Number.NaN, 'USD')).toMatch(/0/); - expect(formatCurrency(Number.POSITIVE_INFINITY, 'USD')).toMatch(/0/); + expect(formatCurrency(Number.NaN, 'USD')).toMatch(/^\$0\.00$/); + expect(formatCurrency(Number.POSITIVE_INFINITY, 'USD')).toMatch(/^\$0\.00$/); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/components/dashboard/formatCurrency.test.ts` around lines 18 - 21, The test for formatCurrency uses a loose regex that only checks for the character "0" and can yield false positives; update the two assertions in formatCurrency.test to assert the exact formatted zero output for the 'USD' locale (i.e., the canonical USD zero representation with two decimal places) or replace the loose regex with a tightly anchored regex that matches only that exact USD-zero format so non-finite inputs truly validate as zero; locate the assertions around the expect(formatCurrency(Number.NaN, 'USD')) and expect(formatCurrency(Number.POSITIVE_INFINITY, 'USD')) calls and change them to strict equality or an anchored pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/src/components/dashboard/formatCurrency.test.ts`:
- Around line 50-53: The test currently only checks the return type for
shortDayLabel('not-a-date'); update it to verify the actual fallback content:
either assert the exact expected suffix (e.g.,
expect(label).toBe('<expected-suffix>')) if the known fallback value exists, or
at minimum assert it's non-empty with expect(label.length).toBeGreaterThan(0);
replace the typeof assertion in the it block for shortDayLabel to one of these
stronger checks so empty or placeholder strings fail the test.
- Around line 45-48: The test for shortDayLabel currently only asserts length
>=2 which contradicts the "3-letter weekday" intent; update the assertion in
formatCurrency.test.ts to enforce a 3-letter weekday by checking either
label.length === 3 and matching a weekday pattern (e.g., /^[A-Za-z]{3}$/) or
asserting the label is one of the expected three-letter weekdays (Mon, Tue, Wed,
Thu, Fri, Sat, Sun) so shortDayLabel('2026-05-27') cannot pass with "??" or
other invalid values.
- Around line 18-21: The test for formatCurrency uses a loose regex that only
checks for the character "0" and can yield false positives; update the two
assertions in formatCurrency.test to assert the exact formatted zero output for
the 'USD' locale (i.e., the canonical USD zero representation with two decimal
places) or replace the loose regex with a tightly anchored regex that matches
only that exact USD-zero format so non-finite inputs truly validate as zero;
locate the assertions around the expect(formatCurrency(Number.NaN, 'USD')) and
expect(formatCurrency(Number.POSITIVE_INFINITY, 'USD')) calls and change them to
strict equality or an anchored pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 77cbf372-dbce-4409-ac39-7d279212140e
📒 Files selected for processing (2)
app/src/components/dashboard/formatCurrency.test.tsapp/src/components/dashboard/formatCurrency.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/components/dashboard/formatCurrency.ts
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Code Review
Overview
This is a solid, well-structured PR that adds the cost dashboard end-to-end. The RPC wiring, DTO layer, config defaults, test coverage, i18n completeness, and UI polish are all high quality. A few items need attention before merge.
Required
Missing route/menu registration — CostDashboardPanel.tsx is implemented but I can't find the corresponding /settings/cost-dashboard route added to AppRoutes.tsx or a settings menu entry for it. The test uses MemoryRouter initialEntries={['/settings/cost-dashboard']} and renders the component directly, so it doesn't validate that navigation works. If this route isn't wired the panel is unreachable in the app. Please confirm (or add) the route + menu entry.
Bugs / Logic Issues
init_global_is_idempotent has a vacuous assertion (src/openhuman/cost/global.rs):
assert!(try_global().is_some() || try_global().is_none());This is always true — it proves nothing. The test should assert that (a) a second init_global call doesn't panic, and (b) try_global() is Some when the first call succeeded. Suggested fix:
init_global(cfg.clone(), tmp.path());
init_global(cfg, tmp.path()); // second call must be a no-op, not panic
// If this test process ran init_global first, the global must be set.
// (If another test pre-empted it, global is still Some — either way.)
assert!(try_global().is_some());Minor Issues
Daily target uses a fixed /30 divisor (app/src/components/dashboard/CostBarChart.tsx, line 504):
const dailyTarget = budgetLimitMonthlyUsd > 0 ? budgetLimitMonthlyUsd / 30 : 0;Months range from 28–31 days, so in February this overstates the daily target by ~10% and affects bar colour thresholds. This is a known simplification but warrants a comment:
// Simplified: divides by 30 regardless of actual month length.
const dailyTarget = budgetLimitMonthlyUsd > 0 ? budgetLimitMonthlyUsd / 30 : 0;Background polling continues when panel is not visible (app/src/components/dashboard/CostDashboardPanel.tsx):
useCostDashboard() is called without paused, so the 10s polling interval runs continuously for as long as the panel component is mounted — including when it's behind another settings route. The hook already supports paused; consider passing it:
const { data, ... } = useCostDashboard({ paused: !document.visibilityState || document.hidden });Or at minimum, a usePageVisibility / visibilitychange integration. Low actual cost on desktop, but worth the cleanup.
1-second tick runs unconditionally (app/src/components/dashboard/CostDashboardPanel.tsx, line 810–813):
The setInterval(..., 1000) that drives the "Updated N ago" pill fires even when lastUpdated === null (before the first fetch), causing 1 re-render/s with no visible effect. Gating it on lastUpdated !== null avoids the churn:
useEffect(() => {
if (lastUpdated === null) return;
const id = window.setInterval(() => setTick(n => n + 1), 1000);
return () => window.clearInterval(id);
}, [lastUpdated]);I/O inside FALLBACK_TRACKER mutex (src/openhuman/cost/rpc.rs, resolve_tracker):
CostTracker::new opens a file while the parking_lot::Mutex guard is held. Under concurrent RPC calls (unlikely at 10s intervals but possible) this blocks the lock for the duration of the file open. Worth a comment:
// NOTE: CostTracker::new does I/O (opens costs.jsonl) while holding the mutex.
// Concurrent calls are rare at the 10s polling interval, so this is acceptable,
// but a future optimisation could build the tracker outside the lock.Nit
The cost.enabled = false warning in init_global (src/openhuman/cost/global.rs) says "budget enforcement is OFF" but doesn't explicitly say that JSONL recording now ignores cost.enabled. Users upgrading from a build where cost.enabled = false also blocked recording may not realise their costs.jsonl is now being written. Consider adding:
"dashboard telemetry will still append to costs.jsonl — set cost.dashboard.enabled=false to hide the panel. The JSONL file is local and never leaves the workspace."
This is already partially in the log message; just make the "recording happens regardless" part the headline rather than a subordinate clause.
|
@YellowSnnowmann unresolved review feedback — please address before we review. |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
@YellowSnnowmann the two major issues from the first pass are both properly addressed now — approving.
The cost.enabled semantics change lands cleanly: the config doc comment, the startup info log showing (cost.enabled=..., dashboard telemetry always-on), and the warn when cost.enabled=false all make the behavior legible to anyone upgrading. That was the main concern.
The fallback tracker path in resolve_tracker is also good now — FallbackState + the 30s error TTL means we're not hammering JSONL opens on every RPC call if the tracker failed to construct.
One nit worth fixing in a follow-up: the init_global_is_idempotent test ends with assert!(try_global().is_some() || try_global().is_none()) which is a tautology — it proves nothing. The real thing to assert is that double-calling init_global doesn't panic (that's the contract), and if this test runs first, try_global() should be Some afterward. Something like:
init_global(cfg.clone(), tmp.path());
init_global(cfg, tmp.path()); // no-op, must not panic
assert!(try_global().is_some());Also noticed M3gA-Mind flagged a missing route — that's already there in Settings.tsx (the Route and menu entry are both in this diff), so nothing to do there.
Good work on this feature.
|
Unresolved review feedback from coderabbitai[bot] — please address before we review. |
4 participants
Summary
Added a new Settings → Cost Dashboard page with 7-day cost and token visualizations, budget summary, model breakdown table, loading/error/empty states, and manual + polling refresh.
Introduced new core JSON-RPC controllers: openhuman.cost_get_dashboard, openhuman.cost_get_daily_history, and openhuman.cost_get_summary.
Activated a process-global CostTracker at core bootstrap and wired provider usage recording from the agent turn loop so dashboard data is populated automatically.
Extended cost domain types/tracker logic to support daily buckets (gap-filled), monthly pace projection, budget utilization/status thresholds, and per-model aggregation/sorting.
Expanded configuration with dashboard settings (cost.dashboard.*) and set cost.enabled default to true so telemetry is captured by default.
Added comprehensive frontend + Rust tests for dashboard rendering, formatting, hook polling, tracker aggregation, history windowing, and budget-state edge cases; added i18n strings across supported locales.
Problem
Cost data was tracked in isolation and not surfaced in a dedicated dashboard experience for users.
Reviewers and users lacked a single place to inspect short-term spend trends, token usage shape, and per-model contribution.
Dashboard RPC reads could race bootstrap and fail if no tracker instance was ready.
The prior cost.enabled behavior made observability weaker when enforcement was off, reducing visibility into usage trends.
Solution
Implemented a full dashboard UI in the Settings surface using reusable components (BudgetSummary, CostBarChart, TokenUsageChart, ModelCostTable) and a polling hook (useCostDashboard) backed by core RPC.
Added a global singleton CostTracker initialized during bootstrap_core_runtime, with fallback tracker resolution in dashboard RPC handlers for resilience.
Wired agent usage events into cost tracking (record_provider_usage) so records are persisted continuously from real provider responses.
Added new cost domain DTOs and aggregation logic for:
daily history (1..366 day clamp, zero-fill gaps),
budget utilization/status (normal/warning/exceeded),
projected monthly pace and per-model totals.
Kept tracking non-blocking and fault-tolerant (cost tracking errors are logged and never break turn execution).
Submission Checklist
If a section does not apply to this change, mark the item as N/A with a one-line reason. Do not delete items.
Tests added or updated (happy path + at least one failure / edge case) per Testing Strategy
Diff coverage ≥ 80% — changed lines (Vitest + cargo-llvm-cov merged via diff-cover) meet the gate enforced by
.github/workflows/coverage.yml. Run pnpm test:coverage and pnpm test:rust locally; PRs below 80% on changed lines will not merge.Coverage matrix updated — added/removed/renamed feature rows in
docs/TEST-COVERAGE-MATRIX.mdreflect this change (or N/A: behaviour-only change)All affected feature IDs from the matrix are listed in the PR description under ## Related
No new external network dependencies introduced (mock backend used per Testing Strategy)
Manual smoke checklist updated if this touches release-cut surfaces (
docs/RELEASE-MANUAL-SMOKE.md)Linked issue closed via Closes #NNN in the ## Related section
Impact
Platform/runtime: desktop app + in-process Rust core (JSON-RPC + Settings UI); no mobile/web-specific changes.
Performance: lightweight 10s polling in UI; backend cost aggregation reads from JSONL with bounded windows.
Security/compatibility: no new external runtime network dependency; RPC surface expanded; config schema extended with dashboard settings and new defaults.
Migration: existing config remains compatible via serde defaults; dashboard fields auto-default when absent.
Related
Summary by CodeRabbit
New Features
Chores
Tests