Skip to content

fix(frontend): share the 730-hour month for hourly cost conversions - #1446

Merged
IAmJSD merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-337-rest
Oct 26, 2026
Merged

IAmJSD merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-337-rest

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Closes #337
Closes #394

Why

The Opportunities cost-period selector turned monthly costs into hourly ones with 1 / 720 (a 30-day month). Savings history, the RI exchange confirm and the backend (ricost.HoursPerMonth, AWS's 365 * 24 / 12 = 730) all use 730. So hourly figures on Opportunities came out about 1.4% higher than the monthly figures the API derives. Both issues ask for one shared constant.

Everything else on these issues was already on main:

What

  • New frontend/src/lib/hours-per-month.ts exports HOURS_PER_MONTH = 730. Its comment points to ricost.HoursPerMonth so the two stay in sync.
  • recommendations.ts: the hourly factor is now 1 / HOURS_PER_MONTH. Daily stays 1 / 30 (left unchanged on purpose, see below).
  • modules/savings-history.ts and riexchange-confirm.ts import the shared constant instead of defining their own. The savings-history comment no longer says 365.25 * 24 / 12.
  • Tests: the hourly expectations now use 730. For example, scaleCost(730, 'hourly') is 1, the 720/month row shows $0.9863, and 2,000,000/mo shows $2,739.7260/hr.

For the owner to decide: daily still divides by 30, so hourly × 24 is about 1.4% below the daily figure. Deriving daily from 730 (24 / 730) would make the two agree, but it changes every daily figure and several daily tests. It is left out to keep this PR to the conversion the issues name.

Verification

  • Fail-before: with main's recommendations.ts and the new tests, npx jest src/__tests__/recommendations.test.ts -t scaleCost gives 2 failed, 5 passed. With this change: 7 passed.
  • npm run lint: 0 errors (141 warnings, all already on main). npm run typecheck: clean. npm run build: ok.
  • npx jest --no-coverage: 171 suites, 4502 passed, 1 skipped.
  • Full Playwright suite (local, private port): 267 passed.
  • Secrets scan: clean, planted key flagged. scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
  • No Go changes.

🤖 Generated with Claude Code

The Opportunities cost-period selector converted monthly to hourly with
1/720 while savings history, the RI exchange confirm and the backend
(ricost.HoursPerMonth) all use 730, so hourly figures were about 1.4%
higher than the backend-derived monthly ones. One exported
HOURS_PER_MONTH in lib/hours-per-month.ts now feeds all three frontend
call sites, and the savings-history comment no longer claims 365.25 days.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@IAmJSD IAmJSD added priority/p3 Polish / idea / may never ship severity/low Minor harm type/chore Maintenance / non-user-visible triaged Item has been triaged effort/s Hours urgency/eventually No deadline urgency/this-quarter Within the quarter impact/internal Team-internal only impact/few Limited audience labels Oct 26, 2026
@coderabbitai

coderabbitai Bot commented Oct 26, 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.

Or wait 59 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 2a11540 and 8c67439.

Files selected for processing: 6
  • frontend/src/__tests__/money-format.test.ts
  • frontend/src/__tests__/recommendations.test.ts
  • frontend/src/lib/hours-per-month.ts
  • frontend/src/modules/savings-history.ts
  • frontend/src/recommendations.ts
  • frontend/src/riexchange-confirm.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

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

Blocking:

  • none

Non-blocking:

  • Owner decision, which the body raises honestly: until now hourly × 24 equalled daily exactly (1/720 = 1/(24·30)). After this PR, hourly follows the backend's 730 h month but daily stays at 1/30, so on Opportunities hourly × 24 is about 1.4% below the daily figure. Hourly-vs-monthly consistency with the API matters more, so I wouldn't block on this. If the owner wants all three periods to agree, deriving daily as 24 / HOURS_PER_MONTH is a small follow-up.

Attacked:

  • Checked the "already on main" claims that the Closes lines depend on. internal/ricost.HoursPerMonth = 730.0 exists, and both monthlyCostFromConvertibleRI copies (internal/api, internal/server) now delegate to ricost, so ARCH-10: monthlyCostFromConvertibleRI money formula duplicated verbatim, with bare 730 despite a named constant [platform part] #337's Go criteria are met. For chore: minor follow-ups from the #373/#376/#380/#383 reviews #394 on main: invalidateQuote() on "+ Add target" (riexchange.ts ~1371), the marketplace note built from a percent constant (history.ts:1779), and the non-200 persistent already_running (internal/server/task_lock.go). This PR covers the remaining item 2, so Closes on both is right.
  • Grepped frontend/src (non-test) for any other hours/month conversion (720, 730, 24 * 30, 8760). The only users left are the three that now import HOURS_PER_MONTH. monthly_730h_* are API field names, and the 30.4375 in history.ts is elapsed-months, not cost. So nothing still uses 720.
  • Fail-before: I reverted only recommendations.ts. 4 tests failed: the money-format thousands test and three chore(frontend): loop-invariant work repeated per row in table renders #319 hourly tests. Restored, recommendations, money-format, savings-history and riexchange-confirm pass (4 suites, 540 tests). tsc --noEmit is clean, and eslint reports 0 errors on the changed files.

Body-stale:

  • none

Reviewer: rD1. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at 8c67439b52885018d1ef5390fe4af0861ff04ea2 merged onto main (529b6e38), together with #1444, #1446, #1449, #1450, #1451, #1457, #1458 in that order (combined tree 7fb727bb, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 26, 2026 10:51
@IAmJSD
IAmJSD requested a review from cristim as a code owner October 26, 2026 10:51
@IAmJSD
IAmJSD merged commit 30386b3 into LeanerCloud:main Oct 26, 2026
42 checks passed
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on 8c67439b, green CI, and the combined-tree verification above. Squash-merged at exactly 8c67439b52885018d1ef5390fe4af0861ff04ea2.

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

Labels

effort/s Hours impact/few Limited audience impact/internal Team-internal only priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/chore Maintenance / non-user-visible urgency/eventually No deadline urgency/this-quarter Within the quarter

Projects

None yet

1 participant