Skip to content

feat(frontend/recs): surface VCPU + MemoryGB in table (closes #219)#840

Open
cristim wants to merge 2 commits into
feat/multicloud-web-frontendfrom
fix/219-wave12
Open

feat(frontend/recs): surface VCPU + MemoryGB in table (closes #219)#840
cristim wants to merge 2 commits into
feat/multicloud-web-frontendfrom
fix/219-wave12

Conversation

@cristim
Copy link
Copy Markdown
Member

@cristim cristim commented May 28, 2026

Summary

Changes

  • frontend/src/types.ts - add vcpu?: number | null and memory_gb?: number | null to LocalRecommendation
  • frontend/src/state.ts - add 'capacity' to RecommendationsColumnId
  • frontend/src/recommendations.ts - COLUMN_DEFS entry, formatCapacity() helper (exported, mirrors Go GetDetailDescription), cases in categoricalCellValue, numericCellValue, displayPrecision, renderColumnCell; all values go through escapeHtml()
  • frontend/src/__tests__/recommendations.test.ts - +11 new tests; update 5 hardcoded td-index and column-list assertions that shifted with the new column

Test plan

  • npx tsc --noEmit - clean
  • npx jest src/__tests__/recommendations.test.ts - 330 pass, 0 fail (+11 vs baseline 319)
  • Visually verify "Capacity" column renders 8 vCPU / 32 GB for a compute rec with populated fields
  • Verify non-compute recs (RDS, savings plans) show in the Capacity cell

@cristim cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/s Hours type/feat New capability labels May 28, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

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 71 minutes and 29 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: fb5de5bd-1f2c-4433-82db-8f8a1b81716b

📥 Commits

Reviewing files that changed from the base of the PR and between c00445c and 8835c40.

📒 Files selected for processing (8)
  • frontend/src/__tests__/recommendations.test.ts
  • frontend/src/api/types.ts
  • frontend/src/recommendations.ts
  • frontend/src/state.ts
  • frontend/src/types.ts
  • internal/api/handler_recommendations.go
  • internal/api/handler_recommendations_test.go
  • internal/config/types.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/219-wave12

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

@cristim
Copy link
Copy Markdown
Member Author

cristim commented May 28, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

✅ 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
Copy link
Copy Markdown
Member Author

cristim commented May 30, 2026

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 30, 2026

✅ Actions performed

Full review triggered.

@cristim
Copy link
Copy Markdown
Member Author

cristim commented Jun 3, 2026

Rebased onto feat/multicloud-web-frontend. Resolved additive conflicts in:

  • frontend/src/types.ts -> kept both usage_history (base) and vcpu/memory_gb (this PR).
  • frontend/src/__tests__/recommendations.test.ts -> merged both sides' COLUMN_DEFS import additions (renderUsageSparkline + formatCapacity) and reconciled the column-count assertions: the table now has 15 columns (13 base + capacity + usage_history) and 11 toggleable columns, with both the usage_history and capacity-after-resource_type assertions retained.

recommendations.ts and state.ts auto-merged cleanly. Verified post-rebase: tsc --noEmit clean, recommendations.test.ts green (343 tests), npm run build succeeds.

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 3, 2026

@cristim Thanks for the detailed rebase notes! The conflict resolution looks clean — keeping both usage_history (base) and vcpu/memory_gb (this PR) as additive fields, and reconciling the column-count assertions to 15 total / 11 toggleable, is exactly the right approach. Triggering the review now.


🧠 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: 277
File: frontend/src/__tests__/recommendations.test.ts:2472-2476
Timestamp: 2026-05-05T07:46:01.902Z
Learning: In the CUDly frontend (`frontend/src/recommendations.ts`), `effectiveSavingsPct` intentionally has NO plausibility guard (no null return or warning for reconstructed percentages above per-term ceilings). The maintainer explicitly rejected that approach in PR `#277`. The fix for inflated percentages (issue `#274`) is purely a data-path fix: plumbing the provider's canonical `on_demand_cost` through to the frontend so the denominator is not reconstructed from potentially misleading `monthly_cost + savings + amortized`. Tests that pin the reconstructed >80% behavior are intentional documentation of the pre-fix path, not bugs.
✅ Action performed

Review finished.

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
Copy link
Copy Markdown
Member Author

cristim commented Jun 3, 2026

Rebased on feat/multicloud-web-frontend to clear the DIRTY/CONFLICTING state caused by sibling merges (#887, #845, #826, #819, #791, #789).

Conflict resolution: only frontend/src/__tests__/recommendations.test.ts had a content conflict, and it was a purely additive import-list collision. The base advanced with #819's column-filter persistence (loadColumnFilters, saveColumnFilters, resetColumnFiltersState) while this PR adds formatCapacity. Resolved by taking the union of both import sets. recommendations.ts and state.ts auto-merged cleanly; COLUMN_DEFS merged with the capacity column intact and in position, no cell-builder or column-layout divergence.

Verification (frontend): npx tsc --noEmit clean, recommendations.test.ts 360 passing / 0 failing.

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 3, 2026

@cristim, the rebase looks clean — taking the union of the import sets for the additive collision between #819's column-filter persistence helpers and this PR's formatCapacity is the right call, and COLUMN_DEFS staying intact with capacity in position is good to confirm. Triggering the incremental review now.


🧠 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: 277
File: frontend/src/__tests__/recommendations.test.ts:2472-2476
Timestamp: 2026-05-05T07:46:01.902Z
Learning: In the CUDly frontend (`frontend/src/recommendations.ts`), `effectiveSavingsPct` intentionally has NO plausibility guard (no null return or warning for reconstructed percentages above per-term ceilings). The maintainer explicitly rejected that approach in PR `#277`. The fix for inflated percentages (issue `#274`) is purely a data-path fix: plumbing the provider's canonical `on_demand_cost` through to the frontend so the denominator is not reconstructed from potentially misleading `monthly_cost + savings + amortized`. Tests that pin the reconstructed >80% behavior are intentional documentation of the pre-fix path, not bugs.
✅ Action performed

Review finished.

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.

#219)

Add a new "Capacity" column to the recommendations table immediately
after "Resource Type". The column renders "N vCPU / M GB" when the
backend populates ComputeDetails.VCPU and ComputeDetails.MemoryGB
(populated by PRs #810/#816/#833 for Azure/AWS/GCP). Recs without those
fields show a dash, matching the feedback_nullable_not_zero rule.

Changes:
- types.ts: add vcpu/memory_gb optional fields to LocalRecommendation
- state.ts: add 'capacity' to RecommendationsColumnId
- recommendations.ts: add COLUMN_DEFS entry, formatCapacity() helper,
  and cases in categoricalCellValue/numericCellValue/displayPrecision/
  renderColumnCell; all innerHTML values go through escapeHtml()
- tests: +11 tests (formatCapacity unit, capacity filter, column count
  and position assertions); update 5 hardcoded td-index/column-list
  assertions shifted by the new column
@cristim
Copy link
Copy Markdown
Member Author

cristim commented Jun 3, 2026

Rebased on feat/multicloud-web-frontend (was DIRTY/CONFLICTING).

Conflict resolution:

  • frontend/src/__tests__/recommendations.test.ts (import line) - additive collision between base's spGroupKey/groupSpCellKeys exports (PR feat(recommendations): usage_history + per-row sparkline (closes #239) #801 et al.) and this PR's formatCapacity. Took the union of all four; no logic change.
  • frontend/src/recommendations.ts - auto-merged cleanly; verified all three exports (formatCapacity, spGroupKey, groupSpCellKeys) are present in the rebased file at their expected sites.

Verification post-rebase:

  • npx tsc --noEmit clean.
  • npx jest src/__tests__/recommendations.test.ts 378 pass, 0 fail.
  • npx jest (full frontend suite) 2327 pass, 0 fail, 1 skipped.

No CR pass yet on this branch - prior @coderabbitai review pings (2026-05-30 and 2026-06-03) returned "incremental review system does not re-review already reviewed commits" because the only commit had already been seen. This force-push lands a new tip SHA on the rebased base, so the next review run should now produce a substantive pass.

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 3, 2026

@cristim, the rebase looks clean — taking the union of all four imports (formatCapacity, spGroupKey, groupSpCellKeys, plus the existing set) for the additive collision is the right resolution, and having recommendations.ts auto-merge cleanly with all exports in place is a good sign. The new tip SHA after the force-push should unblock the incremental review system. Triggering the review now.


🧠 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: 277
File: frontend/src/__tests__/recommendations.test.ts:2472-2476
Timestamp: 2026-05-05T07:46:01.902Z
Learning: In the CUDly frontend (`frontend/src/recommendations.ts`), `effectiveSavingsPct` intentionally has NO plausibility guard (no null return or warning for reconstructed percentages above per-term ceilings). The maintainer explicitly rejected that approach in PR `#277`. The fix for inflated percentages (issue `#274`) is purely a data-path fix: plumbing the provider's canonical `on_demand_cost` through to the frontend so the denominator is not reconstructed from potentially misleading `monthly_cost + savings + amortized`. Tests that pin the reconstructed >80% behavior are intentional documentation of the pre-fix path, not bugs.
✅ Action performed

Review finished.

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
Copy link
Copy Markdown
Member Author

cristim commented Jun 4, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 4, 2026

Rate Limit Exceeded

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

…s (refs #219)

The Capacity column rendered "—" for every recommendation in production.
The frontend reads top-level rec.vcpu / rec.memory_gb, but the API never
emitted them: VCPU/MemoryGB live nested inside the opaque Details JSON
(common.ComputeDetails). The existing tests stayed green because they
injected top-level vcpu/memory_gb the real API never produces.

Backend (Option A from the issue): buildRecommendationsResponse now decodes
each rec's Details via common.DecodeServiceDetailsFor and, when it resolves
to a ComputeDetails with a known size (VCPU>0 && MemoryGB>0), stamps
top-level VCPU/MemoryGB pointers on the record. The api layer may import
pkg/common; config may not, so the fields are derived-on-read
(dynamodbav:"-") and use pointers + omitempty so non-compute, legacy, and
unknown-size recs omit them rather than emit a misleading 0.

Frontend: add vcpu/memory_gb to the Recommendation type. The existing
column code (formatCapacity, renders "—" on absent/0, escapes the value)
needed no change once the backend actually populates the fields.

Tests:
- Backend regression on buildRecommendationsResponse feeds the REAL shape
  (Details = marshalled ComputeDetails VCPU=8/MemoryGB=32) and asserts the
  response JSON carries top-level vcpu=8/memory_gb=32; non-compute,
  unknown-size, and empty-Details recs omit them. Fails against the pre-fix
  code (no top-level fields emitted).
- Frontend render-path test loads a rec with top-level vcpu/memory_gb (the
  post-fix response shape) and asserts the Capacity cell renders
  "8 vCPU / 32 GB", while a rec without them renders "—".
@cristim
Copy link
Copy Markdown
Member Author

cristim commented Jun 5, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 5, 2026

✅ Action performed

Review finished.

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.

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 priority/p3 Polish / idea / may never ship severity/low Minor harm triaged Item has been triaged type/feat New capability urgency/eventually No deadline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant