fix: the dashboard total said zero when it could not price anything - #121
Conversation
A user with real earnings saw a total of ZERO. Proven, not theorised:
DisplayCurrency = "JPY"
Total = 0 <-- real balance is 250.00 USD
breakdown honeygain balance=250 currency=USD convertible=true
Every balance is priced by routing through USD, so the DISPLAY leg is a
single point of failure: one missing rate makes every platform
unpriceable at once. Each contribution is then dropped and Total keeps
Go's zero value. The trigger is not an exotic currency -- it is one
failed fiat fetch, which hits every user not on USD.
Two things made it a rule violation rather than a cosmetic bug. The
dashboard stated 0 as a measured fact, and it contradicted the page it
sat on: render/earnings.ts already falls back to the native figure when
a conversion is missing, so the per-service chip showed $250.00 directly
beneath a headline of zero.
A total of 0 means two different things and the number alone cannot tell
them apart, so the summary now carries TotalKnown and the UI branches on
that. Known when something was actually priced, or when there was
nothing to price at all -- a new install really is at zero, and blanking
that would tell every new user their total is unavailable. A partial sum
stays known and flagged stale, because an understated real figure is
still a measurement.
totalText() requires TotalKnown to be explicitly true; false, absent, or
a summary that never loaded all render an em dash. Absent is not zero,
and it is not true either.
Also removes totalBalance(), which was `summary?.total ?? 0` -- the same
fabricated zero, feeding the topbar on every page. topbar() now reads
the summary itself, so no caller can pass a figure that disagrees with
the one rendered.
Tests: four cases separate unpriceable from genuine zero, and each of
three mutations of the TotalKnown expression breaks a different one.
The render harness covers the same rule browser-free, including that a
genuine zero still renders as a number.
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe backend now distinguishes known earnings totals from totals affected by unavailable pricing. The frontend renders unknown totals as an em dash, preserves genuine zero values, labels stale rates, and validates these states. ChangesEarnings total state
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
earnings_total_known_test.go (1)
119-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a priceable zero-balance case.
This case has no balances, so
unpriced == 0makesTotalKnowntrue without exercising conversion. Add a seeded zero-value USD balance and assertTotal == 0,TotalKnown == true, andRatesStale == false.Proposed test case
+ { + name: "priceable zero balance is a real zero", + display: "USD", + fiatRates: `{"EUR":0.9}`, + seed: []store.EarningsRecord{{Platform: "honeygain", Balance: 0, Currency: "USD", CreatedAt: daysAgoTS(0, 10)}}, + wantKnown: true, + wantStale: false, + wantTotalGT: -1, + },🤖 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 `@earnings_total_known_test.go` around lines 119 - 129, Add a seeded zero-value USD balance to the “no earnings at all is a real zero” case so conversion is exercised, then assert an exact Total of 0 alongside TotalKnown true and RatesStale false; update the existing total assertion rather than relying only on wantTotalGT.
🤖 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.go`:
- Around line 258-260: Update the contract comment for TotalKnown to state that
the UI renders an em dash (—) instead of the quoted double-hyphen placeholder,
keeping the surrounding Total and Absent semantics unchanged.
In `@frontend/scripts/total_render_check.mjs`:
- Line 20: Extend the render checks in total_render_check.mjs beyond helper
values by importing and invoking the real dashboard and topbar render functions
with constructed AppState fixtures. Assert their returned HTML contains an em
dash for unknown totals and the formatted zero for known zero totals, without
asserting source text.
In `@frontend/src/main.ts`:
- Line 375: Update the topbar total rendering in the summary display to pass
current.summary.displayCurrency to totalText, matching the dashboard currency
source and avoiding the config currency fallback.
In `@frontend/src/render/total.ts`:
- Around line 32-45: Update totalIsKnown to return true only when
summary.totalKnown is true and summary.total is a finite number, rejecting
missing totals and NaN. Keep totalText using totalIsKnown so malformed known
totals return UNKNOWN_TOTAL instead of passing zero to formatBalance.
---
Nitpick comments:
In `@earnings_total_known_test.go`:
- Around line 119-129: Add a seeded zero-value USD balance to the “no earnings
at all is a real zero” case so conversion is exercised, then assert an exact
Total of 0 alongside TotalKnown true and RatesStale false; update the existing
total assertion rather than relying only on wantTotalGT.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 86482690-befd-47ca-9c15-9ed163989cce
📒 Files selected for processing (7)
app.goearnings_total_known_test.gofrontend/package.jsonfrontend/scripts/total_render_check.mjsfrontend/src/main.tsfrontend/src/render/total.tsfrontend/src/wails.d.ts
| // | ||
| // node scripts/total_render_check.mjs # against ./.harness-build | ||
|
|
||
| import { totalText, totalCaption, totalIsKnown, UNKNOWN_TOTAL } from "../.harness-build/render/total.js"; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Test the dashboard and topbar render path.
This script tests helper return values only. It does not test the dashboard or topbar output with constructed AppState. Add render checks that assert the emitted HTML contains the em dash for unknown totals and a formatted zero for known zero totals.
As per coding guidelines, “Frontend render tests must exercise the real render functions against constructed state and assert on returned HTML; never assert on source text.”
🤖 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 `@frontend/scripts/total_render_check.mjs` at line 20, Extend the render checks
in total_render_check.mjs beyond helper values by importing and invoking the
real dashboard and topbar render functions with constructed AppState fixtures.
Assert their returned HTML contains an em dash for unknown totals and the
formatted zero for known zero totals, without asserting source text.
Source: Coding guidelines
Review findings, three of four applied. totalIsKnown() trusted the flag alone. formatBalance coerces a non-finite value to 0, so a summary claiming totalKnown:true while carrying an absent, NaN, Infinity or string total rendered a confident "$0.00" -- reintroducing through the back door exactly the bug this branch removes. Confirmed by reverting the guard: all four cases render "$0.00". It now requires a finite number as well as the flag. The topbar labelled the total with config.displayCurrency while the figure was computed in summary.displayCurrency. When the two differ the number carried the wrong currency -- the same family of defect as the one being fixed, so it is fixed here rather than filed. Adds a priced-zero test case. "No earnings at all" satisfies TotalKnown through `unpriced == 0` and never converts anything, so the `priced > 0` half of the rule was only ever exercised by non-zero totals. Also corrects a comment that said the UI renders "--" where it renders an em dash. NOT applied: the suggestion to assert on the dashboard and topbar output directly. main.ts grabs #app at module scope and so cannot be imported by a harness -- that is the whole reason render/ exists and is what CashPilot-Desktop-806 tracks. Asserting on it today would mean matching source text, which this repo has already been bitten by.
|
All four review findings addressed — three applied, one skipped with a reason. The non-finite one was real and worth the catch. Reverting the guard shows all four inputs rendering That is the exact fabricated zero this PR removes, reachable through the back door. The topbar currency one was also real — the figure is computed in The priced-zero test case was a genuine gap. "No earnings at all" satisfies Not applied: asserting directly on dashboard/topbar output. 18/18 render checks, 5 Go cases, |
1 participant
A user with real earnings saw a total of zero
Proven with a test before any code changed:
exchange.ToDisplayroutes every balance through USD, so the display leg is asingle point of failure: one missing rate makes every platform unpriceable at
once, each contribution is dropped, and
Totalkeeps Go's zero value.The trigger is not an exotic currency. It is one failed fiat fetch, which
takes out every user who is not on USD.
Why it was a rule violation, not a cosmetic bug
subtitle hedged the wording, but the number is what a user reads.
render/earnings.tsalready falls back tothe native figure when a conversion is missing, so the per-service chip
showed
$250.00directly beneath a headline total of0.Worth noting
Convertibleis misleading here too: it is computed from thesource currency alone, so when the display leg is the broken one every
service reports
convertible: trueand none converts.The fix
A total of
0means two different things and the number alone cannot separatethem, so the summary now carries
TotalKnown:price at all — a new install genuinely is at zero, and blanking that would
tell every new user their total is unavailable.
is still a measurement, and blanking it would discard the priced services in
order to describe the unpriced one.
totalText()requirestotalKnown === true. False, absent, or a summary thatnever loaded all render an em dash. Absent is not zero, and it is not true
either.
Also removed a second fabricated zero
totalBalance()wassummary?.total ?? 0, feeding the topbar on every page.It is gone;
topbar()reads the summary itself, so no caller can pass a figurethat disagrees with the one rendered. The typechecker found the resulting dead
locals rather than my leaving them behind.
Verification
Everything passed first time, so each check was mutated to confirm it can fail.
Go — four cases, and three mutations of the
TotalKnownexpression eachbreak a different one:
= true= priced > 0= unpriced == 0Frontend — 13 browser-free checks; mutating
totalIsKnownto always-true, totruthy, and the caption to reuse "stale" each break a distinct subset. The
sharpest check is that the unpriceable and genuine-zero summaries carry an
identical
total: 0yet must render differently, which cannot hold if therenderer ever branches on the number.
go test -race ./...green across all 12 packages ·gofmt/go vetclean ·tsc --noEmitclean ·npm test93 checks ·vite buildOK.Closes CashPilot-Desktop-0mb.
Summary by CodeRabbit
New Features
Bug Fixes