Skip to content

fix(frontend): don't offer Exchange for reshape rows with no target - #1468

Merged
IAmJSD merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-1410
Oct 26, 2026
Merged

IAmJSD merged 1 commit into
LeanerCloud:mainfrom
IAmJSD:fix/issue-1410

Conversation

@IAmJSD

@IAmJSD IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

Closes #1410

Root cause

The reshape analyzer in cloud-commitments-go returns target_instance_type: "" and target_count: 0 when it has no exchange target for an RI. That happens for a completely unused RI, and, since the go bump in #1246, for a target capped at the units the account owns. renderRecommendations in frontend/src/riexchange.ts printed ${target_count}x ${target_instance_type} unconditionally, which gave "0x ", and always rendered a live Exchange button. That button opened the exchange modal with a target count of 0.

Fix

  • The Suggested cell for a no-target row now reads "No exchange target" (text-muted). The Reason column already carries the analyzer's explanation.
  • The Exchange button for such a row is disabled, with no data-action, so no quote handler is attached. A title/aria-label gives the reason: "No exchange target available for this RI".
  • One judgement call: a no-target row that still carries cross-family alternative_targets keeps a live Exchange button, because the modal's picker can preselect from those alternatives. The modal now opens with a count of 1 instead of 0. Manual exchange of any RI is still available from the Active Convertible RIs table.

Verification

  • New frontend/src/__tests__/riexchange-no-target.test.ts (4 tests). Against main's riexchange.ts, 3 fail (Expected: "No exchange target" Received: "0x", and the button not disabled). With the fix, all 4 pass.
  • npm run -s lint (0 errors, existing warnings only, none in the changed files), npm run -s typecheck, npm run -s build, and npx jest --no-coverage --silent (172 suites, 4507 passed, 1 skipped).
  • Playwright via the local e2e runner: 266 passed, 1 failed (a11y-focus-return.spec.ts › Escape in the Active RI filter popover). That spec doesn't touch the reshape table. It passed when rerun alone, and 15/15 with --repeat-each=5, so the failure looks like flakiness under full-suite load.
  • Go build/vet (plain, integration, e2e), the secrets scan (clean), and scripts/test-git-secrets-allowlist.sh (183 PASS).
  • Not driven in a real browser against live data, and there are no screenshots.

🤖 Generated with Claude Code

The reshape analyzer returns target_instance_type "" and target_count 0
when it has no exchange target for an RI (a completely unused RI, or,
since the go bump in LeanerCloud#1246, a target capped at the units the account
owns). The table rendered those rows as "0x " under Suggested and kept a
live Exchange button that opened the modal with a target count of 0.

Such rows now say "No exchange target" (the Reason column already
explains why) and get a disabled Exchange button with no quote action.
A no-target row that still carries cross-family alternatives keeps a
live button, since the modal can preselect from them, and opens it with
a count of 1 instead of 0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

This review includes 2 billable files and costs up to $0.50.

Or wait 58 minutes for your next included review.

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
Commits

Reviewing files that changed from the base of the PR and between 01fb35a and d951229.

Files selected for processing: 2
  • frontend/src/__tests__/riexchange-no-target.test.ts
  • frontend/src/riexchange.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Opened as a draft at d951229. Reshape rows with no target now say "No exchange target" and get a disabled Exchange button. Rows that have alternatives keep a live button, and the modal opens with a count of 1. Label note: #1410 has no triage labels (priority/severity/type/...), so there was nothing to mirror onto this PR. The issue needs triage first.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE d951229ea2b711f4c7af59c9cf3a241e17807383

Blocking:

  • none
    Non-blocking:
  • frontend/src/riexchange.ts:1103-1106 and the body's "the modal's picker can preselect from those alternatives": with target_instance_type "" nothing is preselected. suggestedOfferingId (riexchange.ts:1387) only matches on a non-empty type, so the modal opens on the "Select a target offering" placeholder with count 1, and the user picks from the "CE Recommendations" optgroup. That's the right behaviour, but the comment and the body overstate it. A quote and an explicit execute are still required, so a default count of 1 carries no money risk.
  • Some browsers don't show a title tooltip on a disabled button, so the "discoverable on hover" reason may not appear there. The aria-label and the "No exchange target" text in the Suggested cell cover it. No screenshot or real-browser check was done (the body says so).
    Attacked: I traced fillQuoteFromRecommendation into openExchangeModal. The count seeds the editable count input (min 1). The suggested type only preselects through an exact alternative_targets match, and a disabled button without data-action gets no fill-quote handler. I checked escaping: the new static strings carry no user data, and target_instance_type still goes through escapeHtml. Ran (frontend, npm ci --ignore-scripts): npx jest src/__tests__/riexchange-no-target.test.ts src/__tests__/riexchange* (12 suites, 188 tests pass), npm run -s typecheck (clean), and npx eslint on the two changed files (clean). Fail-before: with src/riexchange.ts reset to the merge-base, 3 of the 4 new tests fail. The one that passes is the "row with a target stays live" control. GitHub reports MERGEABLE.
    Body-stale: "the modal's picker can preselect from those alternatives" is inaccurate. The alternatives are offered in the picker but not preselected when there is no primary target.

Reviewer: rE3. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.

@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Local verification at d951229ea2b711f4c7af59c9cf3a241e17807383 merged onto main (990f4936), together with #1462, #1467, #1468 in that order (combined tree 7a76ef40, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 1 approving independent review(s) on it. Nothing was run against any cloud.

@IAmJSD
IAmJSD marked this pull request as ready for review October 26, 2026 12:26
@IAmJSD
IAmJSD requested a review from cristim as a code owner October 26, 2026 12:26
@IAmJSD
IAmJSD merged commit 08105c8 into LeanerCloud:main Oct 26, 2026
42 checks passed
@IAmJSD

IAmJSD commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator Author

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 1 approving independent review(s) on d951229e, green CI, and the combined-tree verification above. Squash-merged at exactly d951229ea2b711f4c7af59c9cf3a241e17807383.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(frontend): don't offer Exchange for reshape rows with no target

1 participant