Repository navigation
Conversation
fetchCommitmentPurchases logged GetActivePurchaseHistory errors and returned ok=false, so the dashboard shipped a 200 with 0 active commitments, $0 committed and $0 YTD savings, indistinguishable from an empty portfolio. The error now propagates through calculateCommitmentMetrics to getDashboardSummary and becomes a 500, the same contract the handler already uses for a ListRecommendations error. The explicit zero-account scope (issue LeanerCloud#956) still short-circuits to genuine zeros without querying. calculateCurrentCoverage also returned a hard-coded 100% whenever potential savings were 0, even with nothing committed, so a zeroed commitment half rendered as "100% coverage, $0 committed". It now computes the ratio in every case and returns nil (JSON null) for 0/0; committed savings with nothing left to buy still compute to 100%. Closes LeanerCloud#72 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
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
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.
Root cause
fetchCommitmentPurchases(internal/api/handler_dashboard.go) logged aGetActivePurchaseHistoryerror ("KPIs will be zeroed") and returnedok=false.calculateCommitmentMetricsturned that into0, 0, 0, nil, andgetDashboardSummarysent a 200 withactive_commitments: 0,committed_monthly: 0andytd_savings: 0. A store outage looked exactly like an empty portfolio.cristim's comment on the issue points out a second effect.
calculateCurrentCoveragereturned a hard-coded100.0wheneverpotentialSavings == 0, even when nothing was committed. So the zeroed commitment half plus an empty recommendations half rendered as "100% coverage, $0 committed". That figure looks consistent, is made up, and reassures instead of alarming. The existing unit test even had a subtest namedboth zero returns 0%that asserted100.0.Fix
Error contract: propagate, and the request returns a 500. I picked this by matching how
getDashboardSummaryalready treats its dependencies:ListRecommendationserror (failed to get recommendations), an account-scope error and agetAccountScopeerror all return an error, which becomes a 500 (handleRequestErrorsends a genericInternal server errorbody, so store details don't leak). Purchase history is the only source for the commitment KPIs, the same way recommendations are the only source for the savings KPIs. It now follows the same rule.resolveCoverageByAccountKey) falls back to un-scaled totals, and the target (resolveTargetCoverage) falls back to the 80% default. Neither invents a $0. The summary response has no partial/unavailable flag anywhere, and adding one would put a new contract on the frontend.loadDashboardtreats a rejected summary fetch as an error and rendersFailed to load dashboard: …, never zeros.Changes:
fetchCommitmentPurchasesnow returns([]PurchaseHistoryRecord, error). A store error comes back wrapped asfailed to get active purchase history: %w. The explicit zero-account scope sentinel (issue sec(auth): an admin-scoped API key ignores its account and amount constraints on non-strict checks #956) still returns(nil, nil)without querying, because for a scoped user with no accessible accounts the zeros are real.calculateCommitmentMetricsgained anerrorreturn.getDashboardSummaryreturns that error.calculateCurrentCoverageno longer special-casespotentialSavings == 0. It computescommitted / (committed + potential)in every case and returnsnilfor 0/0.DashboardSummaryResponse.CurrentCoverageis now*float64, socurrent_coverageis serialized as an explicitnull. With commitments and nothing left to buy, coverage is still 100%, now from the ratio itself. Only the case with nothing to measure changed, from an invented 100% to null.openapi.yamlmarks the fieldnullable: true.Closes #72
Verification
Unit/mock evidence only (testify mocks of the config store and scheduler). The bug is in the handler's error handling, not in SQL, so there's no DB change and no integration test. No concurrent-caller concerns: this adds no writes and no shared state.
New/changed tests in
internal/api/handler_dashboard_test.go:TestHandleRequest_DashboardSummary_PurchaseHistoryErrorIs500: goes through the router (HandleRequest) using the issue's compounding scenario (no recommendations plus a store error). It asserts a 500, no KPI payload, and that the store error text doesn't leak.TestGetDashboardSummary_NothingToMeasure_CoverageIsNull: 0/0 serializes as"current_coverage":null.TestHandler_getDashboardSummary_Errors/purchase_history_error:errors.Isthe store error, with a nil result.TestHandler_calculateCommitmentMetrics/purchase_history_error_is_returned,_not_zeroed: replaces the oldpurchase history error returns zerossubtest, which pinned the bug.TestHandler_calculateCurrentCoverage: pointer results.both zero returns nil, not 100%replaces the mislabeledboth zero returns 0%, which asserted 100.calculateCommitmentMetricscall sites now also takeerrandrequire.NoError.Fail-before: I copied the two new top-level tests unchanged into a detached
origin/mainworktree (a14dc8f) and ran:That body is the "100% coverage, $0 committed" response from the issue, produced on main.
Pass-after (this branch, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true):Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.API contract change
GET /api/dashboard/summary:{"error":"Internal server error"}) when the purchase-history read fails, where it used to return a 200 with zeroed commitment KPIs;current_coverageisnullwhen committed and potential monthly savings are both 0, where it used to be100.A Go client that decodes into a
float64readsnullas 0 without error.Out of scope
handler_inventory.go(~:224-230), from cristim's comment. A failedListRecommendationsleavesonDemandByKeyempty, socoveragePct(covered, 0)gives 100% on every Coverage-tab row. cristim recorded it in the groupedinternal/apimedium/low issue, and it has a different endpoint and response shape, so I didn't fix it here.committed / (committed + 0)). Telling "nothing left to buy" apart from "never collected" needs a collection-freshness signal in the summary, which is a separate design question.resolveCoverageByAccountKey(un-scaled totals) andresolveTargetCoverage(80% default) are documented trade-offs that don't produce a fabricated $0, so I left them unchanged.Merge note
This touches the
calculateCommitmentMetricscall line ingetDashboardSummary, which our not-yet-opened branchfix/issue-312also edits (params["provider"]becomesprovider). The conflict is that one line. Resolve it by keeping this branch's side, including the newif err != nilblock, and usingprovideras the argument. I checked that resolution: it builds andgo test ./internal/api/passes. It merges cleanly with #1200.Frontend follow-up for the UI owner
None of this needs a UI change to be correct:
dashboard.tsalready renders a rejected summary asFailed to load dashboard: …, and a nullcurrent_coverageas--(data.current_coverage ?? null). Suggested follow-ups:frontend/src/api/types.ts: changecurrent_coverage: numbertocurrent_coverage: number | nullso the type matches the API.frontend/src/types.tsalready has it optional.current_coverage: nullwithtotal_recommendations > 0, expecting--. TodaynothingTrackedcovers only the case where both counts are 0.Could a maintainer please mirror the issue's triage labels onto this PR:
severity/medium,priority/p3,urgency/this-quarter,impact/many,effort/s,type/bug,triaged.🤖 Generated with Claude Code