[cristim]

Summary

Fixes the Opportunities usability findings in #1684 and the Azure savings plan nit in its comment.

Finding	Cause	Fix
1. Collapsed group rows leave Region..Upfront Cost blank	.rec-cell-summary-content set display: flex on the <td>, so it stopped being a table cell and ignored its colspan	Flex moves to an inner .rec-cell-summary-inner wrapper; the cell spans to the last column again
2. Variant rows repeat the group columns and scroll sideways	Every variant re-rendered provider/account/service/resource type, and padding-left: 2rem applied to every data cell	Variant rows leave the columns every variant shares (cellKey: provider, account, service, resource type, region) blank and indent only the first data cell. Checkbox labels keep the full identity
3. Payback shows "0.0 – X months"	A no-upfront best end gives a 0 lower bound	A 0 lower bound reads "Up to X months"; zero upfront alone still shows "—"
4. Columns popover scrolls in ~330px	max-height: 360px	max-height: calc(100vh - 2rem)
5. Disabled Purchase looks enabled	.btn-primary keeps the blue fill at 0.55 opacity	.btn-primary:disabled uses the muted surface, strong border and subtle text; hover no longer applies to disabled buttons
6. Wording	"(Select cells to act on)", "$594/ mo"	"rows" in the action bar summary and hint; row summaries print "/mo"
Comment: Azure SP group reads "— — 4 variants"	Blank resource type and region still joined with dashes	Blank identity parts are skipped

The disabled-button style applies to every .btn-primary in the app, not just Purchase.

Tests

• jest (recommendations.test.ts, new Issue #1684 block): 5 of 6 fail on main (variant identity cells, /mo, blank Azure identity, payback lower bound, row wording). The 6th guards the existing "—" for no upfront. Existing wording asserts updated.
• Playwright tests-e2e/opportunities-group-layout.spec.ts (1920x1080, multi-variant fixture): the summary cell is a table cell reaching the last header; expanding keeps header positions (main: 19px shift) with no horizontal overflow; the Columns popover does not scroll inside (main: 453 > 358); disabled Purchase differs from the enabled fill. All 4 fail on main's source and pass with the fix.

Local verification (macOS, commit c3ef8d1)

• npm ci && npm test: 182 suites, 4720 passed, 1 skipped
• npm run lint: 0 errors (141 warnings, all already on main)
• npm run build: OK
• Playwright on the production bundle (CI=1): the new spec plus recommendations.spec.ts and page-overflow.spec.ts, 38 passed
• Screenshots of the collapsed, expanded, KPI and action-box states checked by eye on the mocked fixture
• Pre-commit hooks pass on the changed files; cfn-lint was skipped (SKIP=cfn-lint) because its env does not install on the local Python, and no CloudFormation file changed

Not verified against production data.

Closes #1684

🤖 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 16 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: ux/opportunities-table-layout
• File: .coderabbit.yaml
• Commit: 24ae940
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: fde2bbc7-0197-4e6e-be9d-40fb6ac060cc

Commits

Reviewing files that changed from the base of the PR and between 08e8d41 and 24ae940.

Files selected for processing: 6

• frontend/src/__tests__/recommendations.test.ts
• frontend/src/recommendations.ts
• frontend/src/styles/components.css
• frontend/tests-e2e/opportunities-group-layout.spec.ts
• frontend/tests-e2e/recommendations.spec.ts
• frontend/tests-e2e/table-scroll-affordance.spec.ts

• 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]

Pushed 24ae940: table-scroll-affordance.spec.ts scrolled a fixed 120px, but the 1280px table now overflows by only ~41px because variant rows no longer pad every cell. The spec now scrolls to half of the real overflow. Full local Playwright suite at this head: 303 passed. @coderabbitai review

[coderabbitai]

Analysis chain

Script executed:

#!/bin/bash
set -euo pipefail
fd -t f '^table-scroll-affordance\.spec\.ts$' --exec bash -c '
  for file do
    ast-grep outline "$file"
    cat -n "$file"
  done
' bash {}

Length of output: 7059

---

@cristim I checked table-scroll-affordance.spec.ts. The test now scrolls to half the measured overflow and still checks synchronization and both edge fades. I’ll trigger the review for the latest changes.

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-darling, cold): APPROVE 24ae940

All six #1684 findings plus the Azure SP comment are fixed as described. The spec fix keeps what the affordance test asserts. The notes below are non-blocking.

Spec change (24ae940, table-scroll-affordance)

• The old step scrolled to a fixed 120px. That implicitly required at least 122px of overflow, which this PR's narrower variant rows no longer give at 1280.
• Against this head's build, the old spec fails 1 of 4, and the new one passes 4/4. On main's build the new spec also passes 4/4.
• The new step still asserts real overflow (mid > 0), that the strip drives the table (scrollLeft === mid) and that both fades show. The fade rule (src/lib/table-scroll.ts:57-58: scrollLeft > 1 and < max - 1) means the midpoint can't satisfy the test without real overflow. So it isn't weaker; it only drops the overflow-amount assumption.

Fails on main

• Swapping in main's recommendations.ts and components.css (merge base 08e8d41): 5 of the 6 new jest tests fail. The 6th is the existing "—" guard.
• opportunities-group-layout.spec.ts fails 4/4 on main's build and passes 4/4 at this head.

Payback maths

• pageLevelRange pairs each cell's best and worst upfront-to-savings variant. The lower bound is 0 only when every cell's best choice is no-upfront, so "Up to X months" fits.
• The fixture's 120 / 20 = 6.0 matches the jest assertion. A selected no-upfront variant still shows "—" (min = max = 0).
• Nit: the check is min > 0, so a lower bound in (0, 0.05) still prints "0.0 – X months". Testing the rounded lo === '0.0' would cover that.
• Pre-existing, not this PR: a variant with savings ≤ 0 has ratio Infinity, so it becomes a cell's worst pick, and its non-positive savings are summed into the denominator. That can inflate the upper bound, or zero it out.

Grouping and wording

• CELL_IDENTITY_KEYS (provider, account, service, resource type, region) are all in cellKey, along with engine, AZ, platform and tenancy. So the blanked cells really are shared by every variant of the cell. Engine shows inside the resource-type cell, and it is also in the key.
• Checkbox labels keep the full identity.
• filter(Boolean) fixes the Azure SP "— — 2 variants".
• The "/mo" and "rows" wording is correct. The action-bar count is cellCount, and each selectable row is one variant, one per cell (radio rule), so "rows" is accurate.

Probes

• I ran one change per run: TS changes against recommendations.test.ts, CSS changes rebuilt and run against the new spec. Killed:
  • payback revert;
  • region not blanked;
  • flat rows blanked;
  • non-SP expansion showing the identity;
  • no blank-part filter;
  • spaced unit;
  • "cells" wording;
  • flex back on the td;
  • padding on every cell;
  • disabled style removed;
  • popover back to 360px.
• Survived, both in the Savings Plans child paths:
  • blanking the identity on single SP children ('sp-child');
  • showing the identity on expanded multi-variant SP children.
• The new nesting distinction exists for exactly that path. Blanking 'sp-child' would hide which plan a row is, because the SP group row only names the bucket. I suggest a jest case with an SP group holding one single-variant child and one multi-variant child.

Evidence

• In a scratch copy: npm ci, npm test (4720 passed, 1 skipped), lint (0 errors), typecheck and build pass.
• Playwright: opportunities-group-layout, table-scroll-affordance and recommendations.spec give 12/12.
• CI is all green at 24ae940 (CodeRabbit skipped), and there are 0 open threads.
• The .btn-primary:disabled change is app-wide, as the body says.

Not merging.

[cristim]

APPROVE @ 24ae940 by claude-nibblets

Cold review of 08e8d41...24ae940 (2 commits), done in my own scratch clone; 08e8d41 is the current main tip. This reviews the new head, not c3ef8d15. The PR body was treated as a claim. This is a comment review because GitHub does not let the PR author's account approve its own PR.

The spec fix (24ae940) doesn't weaken the test.

• At c3ef8d15, Frontend E2E failed with Expected: 120, Received: 41. At 1280px the table's maximum horizontal scroll is now 41px, which is this PR's intended effect: variant rows no longer repeat the group columns.
• The new step scrolls to floor((scrollWidth - clientWidth) / 2), which is 20px here. It still requires real overflow (mid > 0) and still checks that the strip and table stay in step.
• The edge hints switch on at more than 1px from each edge (lib/table-scroll.ts:57-58), so asserting both scroll-hint-left and scroll-hint-right at the midpoint remains a real check.
• The 390px case, the far-end check and the other tests are unchanged. Frontend E2E passes at this head.

Grouping: blanking never hides a value that differs between variants.

• Variant rows blank provider, account, service, resource type and region.
• All five are parts of cellKey (recommendations.ts:1066-1076: provider, account, service, region, resource type and engine, plus AZ, platform and tenancy when set), so every variant of a cell shares them, and the summary row shows the identity.
• One row: a single-variant cell renders flat, with nothing blanked.
• Savings Plans: a single-variant child of an SP group renders sp-child, which keeps its identity cells, because the SP bucket row is an aggregate.
• Mixed services: different services never share a cell key, so their rows are never grouped together.
• Blank identity parts: they are filtered before the join, which fixes the Azure SP "— — 4 variants" case.

Payback maths against the data.

• pageLevelRange (recommendations.ts:1332-1385) gives each cell its lowest-ratio variant and highest-ratio variant. A variant with zero savings counts as an infinite ratio.
• The minimum is 0 exactly when every cell's best variant has zero upfront, that is no-upfront. A 0 from the clamp, where total best savings is 0 or less, implies the maximum is 0 too, and the card already shows "—" whenever the maximum is 0 (:965-967).
• So "Up to X months" appears only for a real no-upfront lower bound, and a positive minimum still prints "lo – hi months".

Wording.

• "rows" in the action bar is accurate. The count is targetRange.cellCount, and radio selection allows one variant per cell (#224), so selected rows equal selected cells.
• periodUnit turns periodSuffix's "/ mo" into "/mo" for values only. Labels keep the spaced form.

Other checks.

• The <td> keeps its colspan, now that flex moved to the inner wrapper.
• .btn-primary:disabled overrides the fill app-wide, and hover is limited to :not(:disabled).
• The popover height is now calc(100vh - 2rem).

The new tests fail on main. With recommendations.ts and components.css reverted to main and the PR's tests kept:

• jest: 5 of the 6 Issue #1684 tests fail. The sixth ("no-value dash when nothing is paid upfront") passes there, as the body says.
• Playwright: after rebuilding main's bundle, all 4 opportunities-group-layout.spec.ts tests fail.

Mutation probes (each run in a scratch copy and reverted, jest JSON results):

Mutation in recommendations.ts	Result
A 0 lower bound printed as "0.0 – X"	killed
Region not blanked on variant rows	killed
Flat single-variant rows blanked	killed (7)
Blank identity parts joined	killed
Spaced period unit	killed
SP single-variant child rows blanked	survives (see 1)

Local verification at this SHA (macOS, under the review lock):

• npm ci succeeds;
• npm test gives 182 suites, 4720 passed and 1 skipped;
• npm run lint reports 0 errors and 141 warnings;
• npm run typecheck and npm run build succeed;
• Playwright on the production bundle with CI=1 (opportunities-group-layout, recommendations, table-scroll-affordance, page-overflow): 42 passed.

Not blocking

1. SP child rows. Nothing pins that an sp-child row keeps its account, service and plan-type cells. If nesting === 'variant' widened to nesting !== 'flat', every SP child would lose its identity under the aggregate bucket row, and no test would fail. One assertion in the existing Azure SP group test (#658), that an expanded single-variant SP child still shows its service or account, would pin it.
2. Rounding. A positive minimum under 0.05 months still prints "0.0 – X months". That needs an upfront tiny relative to the savings, so it's cosmetic.

CI at this SHA: CI - Build & Test, Frontend E2E, Frontend build (PR) and pre-commit all succeed: 41 checks SUCCESS, with CodeRabbit NEUTRAL. The merge state is CLEAN.

Not merging.

[cristim]

Independent review (claude-qa-ui-4, cold): APPROVED 24ae940

Grouping edge cases. Variant rows blank exactly the columns in CELL_IDENTITY_KEYS (provider, account, service, resource type, region). Every one of them is part of cellKey (recommendations.ts:1066-1076: provider, cloud account, service, region, resource type, engine, plus AZ, platform and tenancy when set), so all variants of a cell share those values and blanking hides nothing that differs. AZ, platform and tenancy stay visible on each variant row. Savings Plans singletons under an SP group ('sp-child') keep their identity cells, which is right because they are not under their own summary row. A blank resource type and region (the Azure SP case) are now dropped from the joined identity instead of rendering "— —".

Payback maths against the data. pageLevelRange picks each cell's best and worst upfront_cost / savings variant (savings ≤ 0 counts as infinite) and clamps a non-positive savings sum to 0 (:1340-1378). So the lower bound is 0 exactly when every cell's best variant has no upfront. "Up to X months" is then correct: that range starts at "pays back immediately". A page where every variant has non-positive savings also clamps the upper bound to 0, so the card keeps "—". It never shows "Up to 0".

Wording. "Select rows to act on", "Select at least one row to enable", "… across N rows" and "/mo" with no space all read correctly against what the action box counts (selected cells, one row each when collapsed).

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

• Jest fail-before: the PR's recommendations.test.ts against the merge base's recommendations.ts: 10 failures (the 5 new ux(frontend): Opportunities table layout, payback display and action bar wording #1684 tests plus the updated wording asserts).
• At this head: npm ci; full jest 182 suites / 4720 passed (1 skipped); lint 0 errors (141 existing warnings); tsc --noEmit clean; build compiles.
• Playwright (CI=1) on the PR build: opportunities-group-layout, recommendations and table-scroll-affordance, 12 passed.
• Playwright on a build of main's recommendations.ts + components.css with the PR's new spec, --retries=0: all 4 tests fail (summary span, header shift/overflow, Columns popover inner scroll, disabled Purchase fill). The spec catches each fix.

Non-blocking

• formatPaybackRange tests min > 0 rather than the rounded value. A lower bound in (0, 0.05) months, such as a tiny upfront against large savings, still prints "0.0 – X months". Comparing on lo !== '0.0' would close it.
• The new .btn-primary:disabled style applies app-wide, as the body says. That's fine, but worth a glance at the other disabled primary buttons (plan modal, settings save) after deploy.

Not merging.