[cristim]

Closes #1680
Closes #1708

Plans page polish (#1680) plus the follow-ups from the #1706 review (#1708). Both touch frontend/src/plans.ts, so they are one PR.

Changes

#1708: empty-step run-now follow-ups

• No internal issue number in user-facing errors. ErrPlanStepNoRecommendations (internal/purchase/execution.go) and the retry refusal (internal/api/handler_purchases.go, retryPurchase) no longer end with "(platform#631: …)". The feat(plans): plan steps resolve real recommendations and show real counts and prices (tier B of #609) #631 reference moved to a code comment.
  • Rows already in the database carry the old text, so the matcher in internal/purchase/errnotes.go accepts both the old and the new message, bare and after the execution <id>: prefix. Tests cover both.
• The Run now reason is reachable. An empty step's Run now is aria-disabled="true" with an is-disabled style instead of the disabled attribute. It stays focusable, so keyboard and screen-reader users can read its label: "Run now: this step has no recommendations, so there is nothing to run".
  • Activating it (click, Enter or Space) shows the existing info toast and returns. It never calls the run API and doesn't re-fetch the list, so focus stays on the button.
• Item 3 (refresh the list on focus once steps can carry recommendations) waits for feat(plans): plan steps resolve real recommendations and show real counts and prices (tier B of #609) #631. It's noted on fix(plans): empty-step run-now follow-ups: drop the internal issue reference from the 409, make the disabled reason reachable #1708.

#1680: Planned Purchases and plan form

• Plan name width. It uses .plan-name-cell (max-width: 24rem, ellipsis, full name in title) instead of the narrow cell-truncate--sm. A 100-character name (the maximum) no longer widens the table past its wrapper. New Playwright spec plan-name-width.spec.ts.
• Placeholder dashes. Empty Count, Upfront and Est. Savings cells render one muted dash, right-aligned like the numbers. That includes the all-upfront $0 case.
• Filters on Plan and Scheduled Date, through the existing column-filter mechanism.
  • Scheduled Date is keyed by local calendar day, so two steps on the same day give one option, and options sort by date.
  • Plans filter state is in memory only, so there's no persisted state to migrate.
• Ramp wording. The custom field reads "Coverage per step (% of target)". The Weekly and Monthly presets say "25% / 10% of target each week / month". The computation is unchanged.
• Not changed: the icon-only row actions already have aria-label and title, as verified during QA.

Tests

• Go: message tests (no #) and errnotes tests (old text and new text).
• Jest: run-now aria-disabled, no API call, no refetch and focus kept; placeholders including the $0 all-upfront branch; Plan and Scheduled Date filters including same-day collapse and chronological order; ramp labels.
• Playwright: plan-name-width.spec.ts.
• Fail-before.
  • The Go and jest tests fail on the base sources. The width spec gives 2120 > 1641 without the CSS fix.
  • The date-order test fails on the previous commit's plans.ts.
  • Mutation probes, run by the reviewer: removing the has_recommendations guard, restoring the reload after the empty-step click, and removing the NO_VALUE placeholder branch each fail a test.

Local verification (macOS, rebased on main ecb118e)

• go build ./...: ok.
• go test ./internal/api ./internal/purchase: ok.
• golangci-lint v2.10.1: 0 issues.
• Frontend: jest 187 suites, 4832 passed, 1 skipped; lint 0 errors; tsc --noEmit clean; build compiled.
• Playwright plan-name-width, plans-column-filter-popover, narrow-width-layout and money-columns: 17 passed (CI=1).
• A non-author adversarial pre-commit review (2 passes plus a closing pass) found a width regression and three small issues. All are fixed in this branch.

Labels

The union of #1680 and #1708: priority/p3, severity/low, urgency/eventually, effort/s, impact/many, impact/few, type/ux, type/bug, triaged.

🤖 Generated with Claude Code

[cristim]

@coderabbitai review

[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 33 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/plans-polish-1680-1708
• File: .coderabbit.yaml
• Commit: e214fc8
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: eef36607-dadb-498b-b856-89b867ef0d5c

Commits

Reviewing files that changed from the base of the PR and between ecb118e and e214fc8.

Files selected for processing: 13

• frontend/src/__tests__/plans-column-filters.test.ts
• frontend/src/__tests__/plans-ramp-label.test.ts
• frontend/src/__tests__/plans.test.ts
• frontend/src/index.html
• frontend/src/plans.ts
• frontend/src/state.ts
• frontend/src/styles/tables.css
• frontend/tests-e2e/plan-name-width.spec.ts
• internal/api/handler_purchases.go
• internal/api/handler_purchases_retry_dedupe_test.go
• internal/purchase/errnotes.go
• internal/purchase/errnotes_test.go
• internal/purchase/execution.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

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

Cold review: claude-ui-fix-1, verdict APPROVE at e214fc8c1a18b9d8052a7b4c11fc290c3431ef25

I checked this against #1680, #1708 and the code paths they touch, treating the PR body as a claim.

#1708

• No internal issue reference in user-facing text. ErrPlanStepNoRecommendations (internal/purchase/execution.go) and the retryPurchase 409 (internal/api/handler_purchases.go) no longer name platform#631. The sentinel's identity is unchanged, so errors.Is callers are unaffected.
• Stored rows still pass the allowlist. The serverNotePatterns entry (internal/purchase/errnotes.go) makes the (platform#631: ...) suffix optional, so it matches the new text and the legacy stored text, bare and after execution <id>: . The new errnotes_test cases cover both.
• Frontend. Nothing in frontend/src matches on the old text: the only "cannot buy anything" strings are separate UI copy in plans.ts and lib/purchase-row-guards.ts.
• Run now. An empty step's button is aria-disabled="true" with .is-disabled, not disabled, so it stays focusable, and its title/aria-label give the reason. A click reaches runPlannedPurchaseNow, which toasts and returns false, so handlePlannedPurchaseAction returns before the reload. No API call, no confirm, focus kept. Tests pin each of these.

#1680

• Plan and Scheduled Date filters go through the existing column-filter path. Scheduled Date is keyed by local YYYY-MM-DD. The option label uses formatDate, whose parseDateInput reads a date-only string as local midnight (frontend/src/utils.ts:96-110), so labels don't shift a day west of UTC. The key and the table cell's formatDate(scheduled_date) read the same instant, so they agree.
• Ramp wording. "Coverage per step (% of target)", and the Weekly and Monthly preset descriptions say "of target each week/month".
• Plan name width and placeholders. .plan-name-cell with title replaces cell-truncate--sm. Placeholder dashes share .cell-placeholder, including the all-upfront $0 case.

Verification (local, macOS, at this SHA, scratch worktree)

• Go: go test ./internal/purchase/ ./internal/api/ ok. Fail-before, with base ecb118e8's execution.go, errnotes.go and handler_purchases.go: TestErrPlanStepNoRecommendations_NamesNoInternalIssue and TestHandler_retryPurchase_EmptyPlanStepMessageHasNoInternalRef fail.
• jest fail-before: with base's plans.ts, state.ts, index.html and tables.css, 12 new or updated tests fail across plans.test.ts, plans-column-filters.test.ts and plans-ramp-label.test.ts. The tree was restored and checked afterwards.
• At head: full npm test (187 suites, 4832 passed, 1 skipped), lint 0 errors, tsc --noEmit clean, build OK.
• Playwright (CI=1, review and port-4173 locks): plan-name-width, plans-column-filter-popover, plan-card-actions and plan-schedule-preview, 11 passed.
• CI at this SHA: 41 green.

No findings. Not merging.

[cristim]

APPROVE @ e214fc8 by claude-nibblets

Cold review of ecb118e...e214fc8 (4 commits), done in my own scratch clone; ecb118e is the current main tip. 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 409 text and the stored-text matcher (#1708 item 1).

• ErrPlanStepNoRecommendations (internal/purchase/execution.go) and the retryPurchase 409 (handler_purchases.go) no longer end with "(platform#631: …)". The reference moves to a code comment.
• No other user-facing error string in internal/ names a platform# issue.
• The errnotes.go allowlist entry now makes the old suffix optional. serverNoteRE is anchored (^…(?:; |$)), so text stored before this change passes only when the suffix matches in full, and the new text passes without it. Both are accepted bare and after the execution <id>: prefix.
• TestSanitizeStoredError_AllowlistedNotesPassThroughVerbatim pins all four forms.

Run now on an empty step (#1708 item 2).

• The button is now aria-disabled="true" with an is-disabled style instead of disabled, so it stays focusable and its aria-label and title ("Run now: this step has no recommendations, so there is nothing to run") are reachable by keyboard and screen readers.
• Activating it, by click, Enter or Space, which a button gets natively, runs the existing has_recommendations === false guard: an info toast, no confirm and no API call.
• runPlannedPurchaseNow now returns false for that case, so handlePlannedPurchaseAction skips the reload and focus stays on the button.

The Plan and Scheduled Date filters (#1680).

• Both use the existing column-filter machinery: new PlansColumnId members, the categorical value, the label and the numeric and precision cases.
• Scheduled Date is keyed by localDayKey: a bare YYYY-MM-DD key is kept, and a timestamp becomes its local calendar day. Same-day steps therefore share one option, and the keys sort chronologically.
• Option labels go through formatDate. Its parseDateInput reads a bare YYYY-MM-DD as local midnight (utils.ts), so an option shows the same calendar day as the table cell in every time zone, with no off-by-one west of UTC.

The ramp "of target" label (#1680). The wording is accurate. RampSchedule.GetCurrentCoverage is baseCoverage * CumulativePercent / 100, and CumulativePercent is step * PercentPerStep capped at 100 (internal/config/types.go:303-321). Each step's percentage is therefore a share of the plan's target coverage, as the custom field ("Coverage per step (% of target)") and the preset descriptions now say. The computation is unchanged.

Placeholders and the name width (#1680).

• Empty Count, Upfront and Est. Savings cells render one muted NO_VALUE in the right-aligned td.num, including the all-upfront $0 case (upfrontCell === NO_VALUE).
• The plan name moves from the narrow cell-truncate--sm to .plan-name-cell: max-width: 24rem with an ellipsis and the full name in title.

The new tests fail on main.

• Go: with the three changed Go sources at the base, TestHandler_retryPurchase_EmptyPlanStepMessageHasNoInternalRef and TestErrPlanStepNoRecommendations_NamesNoInternalIssue fail. The errnotes legacy-text cases pass at the base, as expected, since the base sentinel still carries the suffix.
• jest: with plans.ts, state.ts, index.html and tables.css at the base, all 12 new tests fail: the filter triggers, narrowing, same-day collapse, chronological order, both ramp labels, aria-disabled, the placeholder style, the name not clamped, focus kept with no refetch, the $0 placeholder, and no API call.
• Playwright: plan-name-width.spec.ts passes against main's bundle, because main clamps the name narrowly and never overflows. It fails with only the new CSS reverted (Expected: <= 1641, Received: 2120), which is the intermediate regression the PR body names. The original "name cut off" bug is pinned by the jest test "the plan name is not clamped to the small truncate width".

Mutation probes (each run in my clone and reverted): 13 of 13 were killed.

Mutation	Result
Stored old text no longer allowlisted	killed
New text no longer allowlisted	killed
Retry 409 names the issue again	killed
disabled instead of aria-disabled	killed (3)
An empty-step click reloads the list	killed
Date filter keyed by the raw timestamp	killed (same-day collapse)
Plan filter has a single empty value	killed
All-upfront $0 keeps the unstyled dash	killed
Ramp label reverted	killed

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

• go build ./... and go vet succeed, and go test -count=1 ./internal/api ./internal/purchase passes;
• npm ci succeeds;
• jest gives 187 suites, 4832 passed and 1 skipped;
• lint reports 0 errors (141 existing warnings), and typecheck and build succeed;
• Playwright plan-name-width and plans-column-filter-popover with CI=1: 3 passed.

CI at this SHA: 41 checks pass, CodeRabbit is NEUTRAL, and the merge state is CLEAN.

Not merging.

[cristim]

Cold review verdict: APPROVE at e214fc8

Reviewer: claude-as-qa-lead. I didn't write this change.

The 409 empty-step text

• No internal reference in user-facing text. ErrPlanStepNoRecommendations (internal/purchase/execution.go) and the retryPurchase 409 (internal/api/handler_purchases.go:2464) no longer mention platform#631. The reference moved to a code comment. No other user-facing string carries it: git grep 'platform#631|no recommendations attached' outside tests finds only code comments, migrations, and the frontend's own copy, which never had the reference.
• errnotes still matches stored rows. The pattern now makes the (platform#631: …) suffix optional. serverNoteRE is anchored as ^(?:prefix: )?(?:…)(?:; |$) (errnotes.go:63), so the bare old text, the old text after execution <id>: , and the new text all match. The new errnotes tests cover the old and new forms.

Run now on an empty step

• Focusable, not disabled. The button renders aria-disabled="true" with is-disabled instead of disabled, so it stays in the tab order and its aria-label/title reason can be read.
• The style keeps the focus ring. is-disabled only sets opacity: 0.55 and cursor: not-allowed (tables.css), so the default focus ring is unchanged.
• Activation does nothing harmful. Click, Enter or Space reaches runPlannedPurchaseNow, which shows the info toast and returns false. The caller then skips the reload, so no run API call happens and focus stays put. The jest tests cover each part.

Plan and Scheduled Date filters

• Both go through the existing column-filter mechanism (ColumnId in state.ts).
• Scheduled Date is keyed by local YYYY-MM-DD (localDayKey), so same-day steps collapse and keys sort chronologically.
• The option label uses formatDate, which parses a bare YYYY-MM-DD as local midnight (utils.ts:96-104), so the label shows the same day as the key in negative-offset timezones too.

Ramp wording

"25% of target each week, 4 weeks" and "10% … each month, 10 months" both add up to 100%, and the computation is unchanged.

Tests fail on main (current main 1a95098)

• jest. With this head's three test files: 12 failed, 203 passed.
• Go. TestHandler_retryPurchase_EmptyPlanStepMessageHasNoInternalRef and TestErrPlanStepNoRecommendations_NamesNoInternalIssue fail. The errnotes old-text case passes, as it should, since it pins the stored text.
• Playwright. plan-name-width.spec.ts passes on main too (1 passed). See note 1.

Gate at this head (scratch worktree)

• full jest passes: 187 suites, 4832 passed, 1 skipped;
• lint shows 0 errors;
• tsc --noEmit ok;
• build ok;
• Playwright plan-name-width and plans-column-filter-popover: 3 passed (run under the 4173 port lock);
• go vet and go test ./internal/api ./internal/purchase ok.

The worktree's node_modules was removed afterwards.

Note (not a blocker)

1. The width spec guards against regression but doesn't prove the fix. It checks that a 100-character name doesn't widen the table past its wrapper. Main's narrow cell-truncate--sm already passes that, so the spec only catches the overflow the first commit introduced (the "2120 > 1641" in the body refers to the branch without its CSS fix, not main). Nothing asserts that the name now shows more than before. One assertion that .plan-name-cell's rendered width is above the old --sm limit (or that a 40-character name isn't truncated) would pin the ux(plans): Plans page polish: truncated plan name, icon-only row actions, inconsistent placeholders and filters #1680 improvement.

[cristim]

Merge gate at e214fc8:

• Cold reviews: claude-ui-fix-1 APPROVE and claude-as-qa-lead APPROVE, both at this SHA.
• CI: 41 success, 1 neutral.
• Review threads: 0 unresolved.
• Merge state: CLEAN.
• Combo re-test (local, macOS): this head merged onto current main 1a95098. go test ./internal/api ./internal/purchase ok; frontend npm ci, jest 4838 passed (1 skipped), lint 0 errors, build compiled; Playwright plan-name-width and plans-column-filter-popover: 3 passed (CI=1).
• as-qa-lead's note (plan-name-width.spec only pins the no-overflow case) is filed as test(plans): pin that a normal-length plan name is not truncated in Planned Purchases #1749.
• Not verified yet on the deployed UI; that's after the deploy.