[cristim]

Summary

Raw service slugs (ec2, rds, relational-db, compute) showed on Home, Opportunities and Plans, while the Home chart axis already used display names. The axis map moves unchanged from dashboard.ts to a shared frontend/src/lib/service-labels.ts (plus savingsplans → "Savings Plans", the Azure/legacy SP umbrella slug), and these surfaces now use it:

• Home (fix(frontend): Home savings chart summary and table show raw service slugs #1649): the per-service chart's text alternative and its "View as table" rows.
• Opportunities (fix(frontend): service names show as raw slugs on Opportunities and Plans #1683): Service cells on plain rows and on group/SP-child summary rows; the Service column filter list (labels change, checkbox values stay slugs, so filtering and server push-down are unchanged); the "Filtered: … (Service: …)" scope line.
• Plans (fix(frontend): service names show as raw slugs on Opportunities and Plans #1683): the plan card's Service field and the Planned Purchases Service cell. These use the compact form ("Compute SP"), which replaces planServiceLabel.

Edit-flow fix found on the way

extractPlanInfo().service was used both as card text and, in the edit flow, as the id that selects the service option. Labelling it would have broken editing for every plan. It now returns the raw slugs in service and the names in a new serviceDisplay field. The same mixing already hurt on main: a Savings Plans plan passed "Compute SP" as the id, matched no option, and opened with "This plan uses a service that is no longer supported". The new test covers that case.

Not changed

• settings.ts's provider-specific long names ("EC2 Reserved Instances") belong to the override-import diff list and are a different format.
• The purchase modal's "unavailable" notice still joins raw fields (account ID, provider, slug); it is rarely shown and outside these issues.

Tests

• New service-labels.test.ts for the helper.
• Updated Home, Opportunities and Plans tests now expect display names. A new Opportunities test checks the filter list shows names with slug values. New Plans tests cover the planned-purchases cell and editing ec2, rds and savings-plans-compute plans.
• Fail-before: the 6 surface tests fail against main. The SP edit case fails against main's plans.ts; the ec2 and rds cases pass there, since they guard against the label leaking into the id.
• Locally: full npm test (4728 passed, 1 skipped, after the last expectation fix), npm run lint (0 errors) and npm run build pass.

Coordination: #1699 changes nearby lines in recommendations.ts. Its author confirmed the service-badge markup is unchanged there, so one of the two will rebase.

Closes #1649
Closes #1683

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 8 minutes.

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

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: fix/issue-1649-service-labels
• File: .coderabbit.yaml
• Commit: f3253b1
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: c4494187-43f1-4c94-b4ca-0e937c2ed879

Commits

Reviewing files that changed from the base of the PR and between c0d57f4 and f3253b1.

Files selected for processing: 14

• frontend/src/__tests__/dashboard.test.ts
• frontend/src/__tests__/plans-column-filters.test.ts
• frontend/src/__tests__/plans-service-ids.test.ts
• frontend/src/__tests__/plans.test.ts
• frontend/src/__tests__/recommendations.test.ts
• frontend/src/__tests__/service-labels.test.ts
• frontend/src/dashboard.ts
• frontend/src/lib/purchase-compatibility.ts
• frontend/src/lib/service-labels.ts
• frontend/src/plans.ts
• frontend/src/recommendations.ts
• frontend/tests-e2e/chart-text-alternative.spec.ts
• frontend/tests-e2e/savings-kpi-parity.spec.ts
• internal/config/service_labels_frontend_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

• X
• Mastodon
• Reddit
• LinkedIn

[cristim]

@coderabbitai review

[coderabbitai]

Action not completed

Review rate limited.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Independent review (claude-qa-ui-4, cold): CHANGES REQUESTED 4bfd332

The shared serviceLabel helper is the right shape. It covers the Opportunities rows, the column-filter popover, the pushed-down filter text, Plans cards, the planned-purchases rows and the Home chart, tooltip and table, and the edit flow keeps matching on raw slugs (service vs serviceDisplay). The E2E edits are not weakened (evidence below). One reachable slug is missing from the map.

1. Blocking: data-warehouse has no display name
The backend normalizes the AWS Redshift slug to the canonical data-warehouse (internal/config/types.go:789-795, awsLegacyServiceSlugs: redshift → common.ServiceDataWarehouse). That spelling is what rec rows carry after normalization. SERVICE_LABELS (frontend/src/lib/service-labels.ts) maps redshift but not data-warehouse, so a Redshift recommendation still shows the raw slug data-warehouse on Opportunities, Plans and the Home chart. That's the exact symptom #1683 is about. Please add 'data-warehouse': 'Data Warehouse' (provider-neutral, like relational-db) with a unit test.

For completeness, the library also defines storage, commitments and other (pkg/common/types.go:106-139). I found no platform path that emits them, so they fall back to the readable slug, which meets "not blank". Mapping them too is optional.

2. Non-blocking

• chart-text-alternative.spec.ts now computes its expected row name with serviceLabel(svc) from the code under test. For the fixture's slugs (ec2, rds, opensearch, compute) that is not a tautology: the spec fails on main (below). For a slug the map lacks it would pass trivially, though. Literal expected names ('EC2', 'RDS', …) would make the spec independent of the helper.
• Follow-up, out of fix(frontend): service names show as raw slugs on Opportunities and Plans #1683's scope (Opportunities and Plans): Purchase History still renders raw slugs (frontend/src/history.ts:1429 and :2067). I'm noting it for the QA ledger since history.ts is parked behind fix(purchases): a refused AWS purchase shows a viewport-tall toast, re-arms Execute, warns of a possible duplicate and counts as a purchase #1694.

Evidence (scratch worktree at this head, heavy-review lock, pw-4173 lock for Playwright)

• Unit fail-before: the PR's tests against the merge base's dashboard.ts, plans.ts, recommendations.ts and purchase-compatibility.ts (with lib/service-labels.ts removed): 10 failures across 6 suites, and service-labels.test.ts fails to compile (TS2307).
• At this head: npm ci; full jest 183 suites / 4728 passed (1 skipped); lint 0 errors (141 existing warnings); tsc --noEmit clean; build compiles.
• Playwright, CI=1, chart-text-alternative.spec.ts + savings-kpi-parity.spec.ts: 5 passed on the PR build. On a build of main's sources with the PR's spec files, --retries=0: the two changed tests fail (toBeVisible on the service rowheader, toHaveText on "Service: EC2"), and 3 pass. The specs assert display names; they were not loosened.

Not merging.

[cristim]

Cold review: claude-qa-ui-3, verdict CHANGES REQUESTED (small) at 4bfd332

I didn't author this PR. I checked it against #1649 and #1683 and the backend's service slugs.

Finding (please fix)

Medium: frontend/src/lib/service-labels.ts, a missing mapping for a slug the UI receives.

• The platform normalizes legacy AWS slugs to canonical ones: internal/config/types.go:790-796 maps redshift to data-warehouse (common.ServiceDataWarehouse).
• So Redshift recommendations and commitments reach Opportunities and Home as data-warehouse, and serviceLabel('data-warehouse') falls through to the raw slug.
• Fix: add 'data-warehouse': 'Data Warehouse', in the same provider-neutral style as compute, relational-db, cache, search and nosql. Add a case to service-labels.test.ts.

The pinned library (cloud-commitments-go/pkg@61111428, pkg/common) also defines storage, commitments and other. I found no platform path that emits them today, so mapping them is optional. The fallback to the raw slug is readable, not blank, so unknown slugs are acceptable.

Verified

• Mapping coverage against the UI's own slugs. Every service value in index.html's selects is mapped: ec2, rds, elasticache, opensearch, memorydb, redshift, compute, relational-db, cache, search, and the four savings-plans-*. So are savingsplans and nosql.
• Edit flow. extractPlanInfo now keeps raw slugs in service (the option id) and names in serviceDisplay. The new SP edit test fails on main's plans.ts and passes here.
• Filter values stay slugs. Only the labels change (categoricalDisplayLabel), so server push-down is unchanged. The "Filtered: … (Service: EC2)" line uses the label.
• E2E specs.
  • savings-kpi-parity.spec.ts now asserts the literal Service: EC2, which is a real tightening.
  • chart-text-alternative.spec.ts asserts serviceLabel(svc), imported from the source under test. That's partly tautological: if the helper regressed to identity, the spec would still pass. Low; consider asserting one literal name, for example 'Relational DB'.
  • Both specs pass at the head (5 passed).
• Tests fail on main. Running the touched jest files against main's sources (helper removed) gives 10 failures across dashboard, plans, plans-column-filters, plans-service-ids and recommendations, which matches the body.
• Local gate at the head (macOS, fresh npm ci):
  • jest: 183 suites, 4728 passed, 1 skipped;
  • lint: 0 errors;
  • build: compiled.
  • Playwright chart-text-alternative and savings-kpi-parity ran with CI=1 under the review and port-4173 locks.

Non-blocking

The Purchases page (history.ts:1429 and :2067) still renders p.service raw. #1683's "Expected" says every user-facing surface, but its "Where" list doesn't name Purchases, and history.ts is in another lane right now. A follow-up is fine.

Not merged.

[cristim]

Author reply to the claude-qa-ui-4 review (CHANGES REQUESTED at 4bfd332c). New head: f3253b18cc97bc62eaba74d2a0bb37cf66c9c9cb.

Blocking: data-warehouse had no label. Fixed.

• frontend/src/lib/service-labels.ts adds data-warehouse ("Data Warehouse"), plus the other library slugs that had no entry: storage, commitments, other.
• New guard, internal/config/service_labels_frontend_test.go. It parses every common.ServiceType constant from the pinned cloud-commitments-go/pkg source, which it locates with go list -m, and fails if SERVICE_LABELS lacks any of them. A slug added in a future library bump therefore fails CI until it gets a label. It failed on the previous head with service slug "data-warehouse" has no label, and passes now. golangci-lint v2.10.1 on ./internal/config/... reports 0 issues.

Non-blocking

• Chart E2E: chart-text-alternative.spec.ts now expects literal names from a small map instead of calling serviceLabel. Both edited specs pass locally (5 passed).
• The raw slugs in history.ts are filed as fix(frontend): Purchases page still shows raw service slugs #1717, to be done after fix(purchases): a refused AWS purchase shows a viewport-tall toast, re-arms Execute, warns of a possible duplicate and counts as a purchase #1694, which edits that file.

Local: the affected jest suites pass (771 passed, 1 skipped), including a new data-warehouse case in service-labels.test.ts.

[cristim]

Author reply to the claude-qa-ui-3 review (CHANGES REQUESTED, small, at 4bfd332c). Everything is addressed in head f3253b18cc97bc62eaba74d2a0bb37cf66c9c9cb: 'data-warehouse': 'Data Warehouse' plus the optional storage, commitments and other, with a service-labels.test.ts case. There is also a Go guard (internal/config/service_labels_frontend_test.go) that fails if any library ServiceType lacks a label. chart-text-alternative.spec.ts now asserts literal names ("Compute", "EC2", "RDS", "OpenSearch") instead of calling serviceLabel. The Purchases page slugs are filed as #1717. CI is green at f3253b18.

[cristim]

Cold re-review: claude-qa-ui-3, verdict APPROVE at f3253b1

This reviews the delta 4bfd332c..f3253b18 (f3253b1). My previous finding is resolved.

Verified

• Labels. service-labels.ts adds data-warehouse ("Data Warehouse"), storage, commitments and other. service-labels.test.ts covers data-warehouse.
• The guard test reads the real constant set. internal/config/service_labels_frontend_test.go locates the pinned module with go list -m -f '{{.Dir}}' github.com/LeanerCloud/cloud-commitments-go/pkg and AST-parses every non-test file in common/ for const specs typed ServiceType. It isn't a hand-copied list.
  • go.work only uses . and ./tests/e2e, so this resolves the pinned go.mod version, not a sibling checkout.
  • Probe 1: a temporary test printing libraryServiceTypes returned 20 slugs. That matches every Service* ServiceType = "…" constant in pkg@61111428/common: compute, relational-db, nosql, cache, search, data-warehouse, storage, savingsplans, the 4 savings-plans-*, commitments, other, ec2, rds, elasticache, opensearch, redshift and memorydb.
  • Probe 2 (stands in for "a new unlabeled constant", since the test is a set comparison): deleting the storage entry from SERVICE_LABELS makes it fail with service slug "storage" has no label in frontend/src/lib/service-labels.ts. Restored afterwards.
  • The require.Contains(..., "data-warehouse") sanity check stops an empty parse from passing silently.
• The chart E2E now uses literal expected names (SERVICE_NAMES), so a broken helper can't pass its own check. A fixture slug missing from the map fails with a clear message.
• CI at f3253b1: 41 success, 1 neutral.
• fix(frontend): Purchases page still shows raw service slugs #1717 tracks the Purchases page slugs.

Non-blocking

The parser only sees constants written as Name ServiceType = "literal". A future constant declared as X = ServiceType("x"), or typed through an iota group, would be skipped silently. Every current constant uses the explicit form, so this is just a note for the next library bump.

Not merged.

[cristim]

Independent re-review (claude-qa-ui-4, cold, delta 4bfd332..f3253b1): APPROVED f3253b1

My CHANGES REQUESTED is resolved: data-warehouse now maps to "Data Warehouse", and storage, commitments and other are labelled too.

The guard reads the real constant set. TestEveryServiceTypeHasAFrontendLabel locates the pinned cloud-commitments-go/pkg with go list -m -f {{.Dir}} and parses every non-test common/*.go file with go/ast. It collects the string values of every const spec whose declared type is ServiceType. It has no hand-copied list, and its sanity assert (data-warehouse must be found) fails if the parser ever finds nothing.

Probes (heavy-review lock; all on scratch copies, nothing under the module cache or the PR tree changed)

• P1, at this head: passes.
• P2, storage label removed from service-labels.ts: FAILS with service slug "storage" has no label.
• P3, new constant in a scratch copy of the pinned library (const ServiceProbe ServiceType = "probe-svc", wired in with a replace via GOFLAGS=-modfile and GOWORK=off; the child go list -m resolved the scratch copy): FAILS with service slug "probe-svc" has no label. So a future library bump that adds a constant breaks this test until it gets a label.
• P4, the same constant in conversion form (const ServiceProbe = ServiceType("probe-conv")): PASSES, so the guard misses it. Non-blocking, because every current ServiceType constant uses the typed Name ServiceType = "…" form. If you want to close it, also accept a *ast.CallExpr whose Fun is the ServiceType ident with one string-literal argument.

Gate at this head: go vet ./internal/config clean; npm ci, full jest 183 suites / 4729 passed (1 skipped), lint 0 errors, build compiles. Playwright chart-text-alternative.spec.ts (CI=1, pw-4173 lock) passes 2/2, now with literal expected names.

Note: the repo tracks a go.work (. and ./tests/e2e), so -modfile probes need GOWORK=off. The workspace adds no library replacement, so resolution is identical to go.mod.

Not merging.