ux(frontend): list per-plan-type SP breakdown in plan summary (closes #248)

[cristim]

Summary

• Adds a regression-test that verifies a purchase plan covering three SP plan types (Compute, EC2 Instance, SageMaker) renders all three labels comma-joined in the plan card's Service detail row (closes ux(frontend): multi-SP purchase-plan summary should show all plan types #248).
• No production-code change needed: extractPlanInfo already joins all service labels; the existing issue #131 test covered 2 SP types, this extends coverage to 3.
• The new test asserts both individual label presence and the correct comma-joined ordering via a regex match.

Test plan

• cd frontend && npx jest src/__tests__/plans.test.ts --no-coverage passes (109 tests)
• cd frontend && npx tsc --noEmit clean
• Full suite cd frontend && npx jest --no-coverage passes (2143 tests)

[coderabbitai]

Warning

Review limit reached

@cristim, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 10 minutes and 27 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 28106275-cd76-410e-a1ef-964a43fa9d9c

📥 Commits

Reviewing files that changed from the base of the PR and between 4956d66 and 4662846.

📒 Files selected for processing (1)

• frontend/src/__tests__/plans.test.ts

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/248-wave8

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

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]

@coderabbitai full review

[coderabbitai]

✅ Actions performed

Full review triggered.

[cristim]

@coderabbitai review

[coderabbitai]

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 39 minutes and 16 seconds before sending another message.

[cristim]

Verification note: the multi-SP-plan summary fix for #248 (join all SP service labels comma-separated in extractPlanInfo) already shipped on base via #131; this PR's diff is test-only (+60/-0, no production change, as the PR body states) and its new test passes against pre-PR code, so it is coverage rather than a regression guard. Two small suggestions: (1) retitle/reframe so #248 is not auto-closed on the premise of a fix this PR did not make (the fix predates it); (2) the assertion regex /Compute SP.*EC2 Instance SP.*SageMaker SP/ depends on Object.values() insertion order and is brittle - prefer asserting each label is present (order-independent) or sort serviceValues deterministically. Leaving for a human.

[cristim]

@coderabbitai full review

---

Generated by Claude Code

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch PR `#824` details and diff
gh pr view 824 --repo LeanerCloud/CUDly --json title,body,files,state,labels,headRefName
echo "---FILES---"
gh pr diff 824 --repo LeanerCloud/CUDly

Length of output: 4759

---

🏁 Script executed:

#!/bin/bash
# Check the existing two-SP test and surrounding context
rg -n "issue `#131`\|two.*SP\|savings-plans\|extractPlanInfo" frontend/src/__tests__/plans.test.ts | head -60

echo "===extractPlanInfo implementation==="
rg -n "extractPlanInfo" frontend/src/ -A 20 | head -80

echo "===Object.values usage in plans==="
rg -n "Object\.values\|Object\.entries\|Object\.keys" frontend/src/plans.ts 2>/dev/null | head -20
fd "plans.ts" frontend/src --exclude "__tests__" --exec cat {}

Length of output: 72378

---

---

@cristim — full review complete. This is a clean, test-only diff; the three findings are below.

---

Review: frontend/src/__tests__/plans.test.ts

🟡 Finding 1 — PR metadata: closes #248`` will auto-close an issue the fix predates

The PR body states "No production-code change needed: extractPlanInfo already joins all service labels" and attributes the fix to PR #131. Merging with closes #248 in the title/body will auto-close the issue even though this PR only adds test coverage for pre-existing behaviour. Consider changing to `ref `#248 or tests #248`` so the issue can be closed with a note that links to both PRs.

---

🟡 Finding 2 — Ordering regex is redundant and couples the assertion to fixture insertion order

// Line ~317 in the new test block
expect(list?.innerHTML).toMatch(/Compute SP.*EC2 Instance SP.*SageMaker SP/);

extractPlanInfo calls Object.values(services) (plans.ts Line 322), which iterates in insertion order for non-integer string keys (ES2015+ / V8). The fixture defines keys in compute → ec2instance → sagemaker order, so the regex passes reliably for this test. However:

• The three toContain assertions immediately above already fully assert that all three labels are present in the rendered HTML.
• The regex adds ordering coverage, but the only ordering guarantee comes from the fixture's own key definition order — if a future test or refactor changes that order (or if JSON deserialization from a backend response reorders keys), the regex breaks without any functional regression.

Suggestion: Either drop the regex (the three toContain calls suffice for the issue #248 intent) or make it order-independent:

// order-independent: each label appears somewhere in the service detail value
expect(list?.innerHTML).toContain('Compute SP');
expect(list?.innerHTML).toContain('EC2 Instance SP');
expect(list?.innerHTML).toContain('SageMaker SP');
// all three must appear together in one detail span (not scattered across cards)
expect(list?.innerHTML).toMatch(/Compute SP[^<]*,\s*EC2 Instance SP[^<]*,\s*SageMaker SP|EC2 Instance SP[^<]*,\s*Compute SP/);

Or, since the rendered HTML is a comma-joined string inside a single <span>, asserting the comma-joined substring directly is both readable and order-safe with respect to the fixture:

expect(list?.innerHTML).toContain('Compute SP, EC2 Instance SP, SageMaker SP');

This is actually stricter (checks the comma separator too) and fails for the right reason if the join ever breaks.

---

🔵 Finding 3 — Pre-existing dead-code branch in extractPlanInfo (not introduced by this PR)

In frontend/src/plans.ts Lines 325–329:

if (firstService) {            // ← true only when serviceValues.length >= 1
  const service = serviceValues.length === 0   // ← always false here
    ? '—'
    : serviceValues.map(s => planServiceLabel(s.service || '—')).join(', ');

The serviceValues.length === 0 ? '—' branch is unreachable because the outer if (firstService) guard already guarantees serviceValues is non-empty. The simplification is:

if (firstService) {
  const service = serviceValues
    .map(s => planServiceLabel(s.service || '—'))
    .join(', ');

This is a pre-existing issue outside the scope of this PR — mentioning it in case the next pass on plans.ts wants to clean it up. Worth opening a follow-up issue if not already tracked.

---

Summary

#	Severity	Location	Issue
1	🟡 Metadata	PR title/body	closes #248`` auto-closes; fix predates this PR
2	🟡 Test quality	new test, line ~317	Ordering regex couples to fixture insertion order; three toContain calls suffice
3	🔵 Existing code	plans.ts Line 325	Unreachable serviceValues.length === 0 branch in extractPlanInfo

The test structure (fixture shape, mock setup, comment block) is solid. Findings 1 and 2 echo what you already noted; I'd lean toward replacing the regex with a toContain of the full comma-joined string as the cleanest fix.

---

🧠 Learnings used

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-05-05T15:44:54.463Z
Learning: In the LeanerCloud/CUDly repository, when a CodeRabbit review flags something as "outside-diff" that was already implemented in an earlier commit of the same PR, it is likely a stale-snapshot artifact where the review diff was generated against a pre-fix view. The inline comment convention `(CR pass on PR #<number> Finding N)` is used by the maintainer (cristim) to mark lines that were changed in direct response to a prior CodeRabbit review pass.

---

Learnt from: cristim
Repo: LeanerCloud/CUDly PR: 0
File: :0-0
Timestamp: 2026-06-06T06:43:53.792Z
Learning: In LeanerCloud/CUDly PR `#995` (issue `#950` ownership gate), the Dashboard "Upcoming purchases" widget (`dashboard.ts`) has two Cancel entry points (card-level and modal) that both call `api.deletePlannedPurchase`. The `UpcomingPurchase` response DTO must include `CreatedByUserID *string` (omitempty, mirroring `PlannedPurchase`) so the frontend can gate these buttons via a `canCancelUpcomingPurchase` helper that mirrors `canManageScheduledPurchase` on the Plans page. Without this, the backend correctly 403s non-owners but the UI has no gate and the UX is broken. Regressions: `frontend/src/__tests__/dashboard-ownership-950.test.ts` (6 tests) and `TestHandler_getUpcomingPurchases_PropagatesCreatedByUserID`. Fixed in commit 94326f6b9.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[cristim]

Closing as a no-op: the #248 multi-SP-plan-summary fix already shipped on base via #131 (extractPlanInfo joins all SP service labels). This PR is test-only, passes against pre-PR code, and includes a brittle Object.values()-order-dependent regex. 'closes #248' would be misleading. The added coverage can be re-proposed standalone with an order-independent assertion.