Skip to content

refactor(api): delete the never-constructed HistorySummaryAnalytics type - #1458

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

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

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Closes #276

Why

HistorySummaryAnalytics (internal/api/types.go) declared total_period_savings, average_savings_per_period and peak_savings, but nothing ever built it. GET /api/history/analytics returns Summary *HistorySummary. The frontend half of #276 already moved SavingsAnalyticsSummary to HistorySummary's real keys and stopped reading the phantom ones. The dead Go type was the last piece, and it still suggested a wire contract that never existed.

What

  • internal/api/types.go: deletes HistorySummaryAnalytics. Nothing referenced it.
  • OpenAPI: no change needed. I checked internal/api/openapi.yaml. AnalyticsResponse.summary already $refs #/components/schemas/HistorySummary, whose properties match the Go HistorySummary JSON tags. No schema mentions total_period_savings, average_savings_per_period or peak_savings.

Verification

  • No fail-before test: this deletes an unused type, so there's no behaviour to test. go build and go vet across all build tags show nothing referenced it. grep -rn HistorySummaryAnalytics now finds only the audit report in docs/audits/.
  • go build ./... && go vet ./... && go vet -tags integration ./... && (cd tests/e2e && go vet -tags e2e ./...): pass.
  • golangci-lint v2.10.1 on ./internal/api/..., with and without --build-tags integration: 0 issues. gocyclo -over 10 internal/api/types.go: nothing reported.
  • go test -race -short -count=1 ./internal/api/: ok.
  • go test -race -tags integration -count=1 ./internal/api/: ok (the full package passed in one run, 898s).
  • Secrets scan: clean (planted key flagged). scripts/test-git-secrets-allowlist.sh: 183 PASS, 0 FAIL.
  • Note: frontend/src/__tests__/savings-history.test.ts:178 (the fix(frontend): dashboard charts have no text alternative and layers differ only by shade #744 chart-summary test) still mocks the response with the old summary keys. That test only checks the chart table, so it is harmless, and I left it alone because this PR is backend only.

🤖 Generated with Claude Code

HistorySummaryAnalytics declared total_period_savings,
average_savings_per_period and peak_savings, but nothing ever built it:
GET /history/analytics returns *HistorySummary. The frontend stopped
reading those keys in an earlier LeanerCloud#276 change, so the type is dead and
only suggested a wire contract that never existed. The OpenAPI
AnalyticsResponse.summary already references HistorySummary, so the
spec needs no change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 1 billable file and costs up to $0.25.

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 b1d361e.

Files selected for processing: 1
  • internal/api/types.go
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@IAmJSD IAmJSD added priority/p2 Backlog-worthy severity/medium Moderate harm impact/internal Team-internal only urgency/this-quarter Within the quarter effort/xs Trivial / one-liner type/bug Defect triaged Item has been triaged labels Oct 26, 2026
@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 b1d361e31b4762a215d9f544abf664e38faabfb1

Blocking:

  • none

Non-blocking:

  • frontend/src/types.ts:328-331 and frontend/src/api/types.ts:679-682 still say the legacy keys "belonged to an unused Go struct". That stays true historically, but a reader can no longer find the struct. A follow-up could reword it to "a since-deleted Go struct (fix(frontend): savings-analytics summary keys read by the client never exist on the wire #276)". This is optional.
  • frontend/src/__tests__/savings-history.test.ts:178 still mocks the phantom keys, as the body says. It is harmless because that test only checks the chart table.

Attacked:

  • Ran grep -rn HistorySummaryAnalytics over all .go, .yaml and .json files in the tree: no hits. internal/ can't be imported from other modules, so nothing outside this repo can depend on the type.
  • Checked the issue's acceptance criteria. The frontend half is already on main: SavingsAnalyticsSummary mirrors HistorySummary, savings-history.ts:313 reads total_monthly_savings, and a test pins that the legacy keys are ignored. With this deletion both halves are done, so Closes #276 is correct.
  • Ran go build ./..., go vet on ./internal/api with and without -tags integration, the e2e vet, and go test -race -short -count=1 ./internal/api/ (ok). I skipped the 15-minute integration run. A pure type deletion can't change runtime behaviour, and the integration-tagged vet already shows it compiles.
  • No fail-before is needed because there is no behaviour change.

Body-stale: none

Reviewer: rD3. 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 b1d361e31b4762a215d9f544abf664e38faabfb1 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:54
@IAmJSD
IAmJSD requested a review from cristim as a code owner October 26, 2026 10:54
@IAmJSD
IAmJSD merged commit 2baf23d into LeanerCloud:main Oct 26, 2026
41 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 b1d361e3, green CI, and the combined-tree verification above. Squash-merged at exactly b1d361e31b4762a215d9f544abf664e38faabfb1.

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

Labels

effort/xs Trivial / one-liner impact/internal Team-internal only priority/p2 Backlog-worthy severity/medium Moderate harm triaged Item has been triaged type/bug Defect urgency/this-quarter Within the quarter

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(frontend): savings-analytics summary keys read by the client never exist on the wire

1 participant