fix(frontend/recs): require explicit selection for Purchase/Plan actions (closes #273)#275
Merged
cristim merged 3 commits intoMay 5, 2026
Merged
Conversation
…ons (closes #273) Before this commit, the bottom action box let the user click "Purchase N visible" / "Plan from N visible" with no checkbox ticked — and a single misclick on either button triggered an irreversible action across every visible cell. The visible set silently changes when Refresh runs or filters mutate, so the no-selection fallback was structurally unsafe (a Refresh between intent and click could turn a "buy these 3 recs" into a "buy these 12 recs"). Changes in updateBottomActionBox (recommendations.ts:1842): - Drop the no-selection-fallback path entirely. When 0 rows are selected, both buttons are `disabled`, the label is the static form ("Purchase" / "Create Plan"), and the tooltip prompts the user ("Select at least one cell to enable"). - When ≥1 rows are selected, label shows the selection count ("Purchase N selected" / "Plan from N selected") — same as before. - Summary text replaces the chatty "(All N cells visible — no selection)" with the prompt "(Select cells to act on)" so the intended state is unambiguous. Defence-in-depth in resolvePurchaseTarget (recommendations.ts:1826): return only the explicit selection ∩ visible. Removes the prior fallback to pickBestVariantPerCell(visible) so a programmatic caller that bypasses the disabled UI cannot trigger the unsafe path either. Test updates: - New "button labels reflect the selection (#273)" pins both branches (disabled state + tooltip when no selection; enabled with count when selected). - Existing "All N visible" test renamed to "prompts for selection when no row is selected" with the new summary copy and tests that the new prompt fires. - 5 fan-out / SP-flow tests that previously relied on the no-selection default-target fallback now seed the selection mock explicitly so they assert the post-click flow, not the gating UI.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR removes the auto-selection fallback from Purchase/Plan buttons. When no cells are explicitly selected, the buttons are now disabled with an explanatory tooltip; selection summary text prompts users to "Select cells to act on" instead of advertising a visible-cells fallback. ChangesSelection-Gated Purchase & Action Box
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cristim
added
priority/p2
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/recommendations.ts`:
- Around line 1816-1831: resolvePurchaseTarget() produces the correct snapshot
but the plan flow ignores it and later re-derives the selection in savePlan(),
allowing mutations between click and save; fix by threading the resolved target
into the plan modal: change openCreatePlanFromBottomBox(...) to accept the
resolved LocalRecommendation[] (or ids) returned by resolvePurchaseTarget(),
update the call site that currently does the emptiness check to pass that
snapshot through, and modify savePlan(...) (and its modal/context plumbing) to
use the passed-in snapshot instead of calling
state.getVisibleRecommendations()/getSelectedRecommendationIDs() so the plan
uses the exact resolved target the user clicked on.
- Around line 1869-1892: The title/tooltip on disabled buttons (purchaseBtn,
planBtn) is unreliable and inaccessible; instead wrap each button in a focusable
container (or add a sibling hint element) and move the explanatory text from
button.title/disabledTooltip to that wrapper/sibling, then link it with
aria-describedby when the button is disabled and ensure the wrapper is
keyboard-focusable (e.g., tabindex="0") so the message is available to mouse and
keyboard users; update the logic around hasSelection to set/remove the
aria-describedby or toggle the hint text visibility rather than setting title on
the disabled control.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f57a5f6-ffa7-4547-856e-f5fbdaff60e2
📒 Files selected for processing (2)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.ts
Comment on lines
+1816
to
+1831
| // Resolve the action target: selected ∩ visible. Returns an empty slice when | ||
| // no rows are selected — the action buttons are disabled in that state by | ||
| // updateBottomActionBox (closes #273), so callers should never reach this | ||
| // helper without a selection. The empty-return is defence-in-depth: if a | ||
| // caller bypasses the disabled UI (programmatic invocation, future code path, | ||
| // regression on the gating), no purchase happens. | ||
| // | ||
| // CR #253: when no rows are manually selected, fall back to one variant per | ||
| // cell (pickBestVariantPerCell) rather than all visible variants. With the | ||
| // grouped table, a collapsed 2-variant cell counts as "1 visible cell" to | ||
| // the user but "2 visible recs" to the flat visible list — submitting both | ||
| // would create 2 conflicting reservations for the same resource, violating | ||
| // the one-variant-per-cell contract from #224. | ||
| // Historical context: prior to #273 this fell back to | ||
| // pickBestVariantPerCell(visible) when no rows were selected, so misclicking | ||
| // a "Purchase visible" button could trigger an irreversible bulk purchase. | ||
| // The fallback was removed because Refresh and filter changes silently | ||
| // mutate the visible set, making the no-selection path structurally unsafe. | ||
| function resolvePurchaseTarget(): LocalRecommendation[] { | ||
| const visible = state.getVisibleRecommendations() as unknown as LocalRecommendation[]; | ||
| const selected = state.getSelectedRecommendationIDs(); | ||
| const intersection = visible.filter((r) => selected.has(r.id)); | ||
| // Selection path: use the explicit user selection (radio enforcement from | ||
| // #224 already caps at one per cell). | ||
| if (intersection.length > 0) return intersection; | ||
| // Default (no selection): pick the best variant per cell so the purchase | ||
| // target is exactly one rec per physical resource, matching what the user | ||
| // sees in the grouped collapsed table. | ||
| return pickBestVariantPerCell(visible); | ||
| return visible.filter((r) => selected.has(r.id)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -i 'plans.ts' frontend
rg -n -C3 'openCreatePlanModal|savePlan|getVisibleRecommendations|getSelectedRecommendationIDs|all visible|selected' \
frontend/src/recommendations.ts frontend/src/plans.tsRepository: LeanerCloud/CUDly
Length of output: 33860
Plan creation still does not snapshot the resolved target.
resolvePurchaseTarget() correctly computes the safe selected ∩ visible set, but the Plan path only uses that result as an emptiness check (line 1805) and then calls openCreatePlanFromBottomBox() without passing the snapshot through. The plan flow re-derives the target from global state in savePlan() (plans.ts:559–565), including the fallback to all visible recommendations when no selection exists (line 563–564). This allows state mutations between button-click and save (Refresh, filter changes, deselections) to silently change which recommendations are planned. Thread the resolved target into the plan modal the same way the purchase flow already does.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/recommendations.ts` around lines 1816 - 1831,
resolvePurchaseTarget() produces the correct snapshot but the plan flow ignores
it and later re-derives the selection in savePlan(), allowing mutations between
click and save; fix by threading the resolved target into the plan modal: change
openCreatePlanFromBottomBox(...) to accept the resolved LocalRecommendation[]
(or ids) returned by resolvePurchaseTarget(), update the call site that
currently does the emptiness check to pass that snapshot through, and modify
savePlan(...) (and its modal/context plumbing) to use the passed-in snapshot
instead of calling
state.getVisibleRecommendations()/getSelectedRecommendationIDs() so the plan
uses the exact resolved target the user clicked on.
Comment on lines
+1869
to
+1892
| const hasSelection = selectedVisibleCount > 0; | ||
| const disabledTooltip = loadedCount === 0 | ||
| ? 'No recommendations loaded' | ||
| : visibleCount === 0 | ||
| ? 'No rows visible — adjust filters' | ||
| : 'Select at least one cell to enable'; | ||
|
|
||
| if (purchaseBtn) { | ||
| purchaseBtn.disabled = empty; | ||
| purchaseBtn.textContent = empty | ||
| ? 'Purchase' | ||
| : targetIsSelection | ||
| ? `Purchase ${targetCount} selected` | ||
| : `Purchase ${targetCount} visible`; | ||
| purchaseBtn.title = empty | ||
| ? (loadedCount === 0 | ||
| ? 'No recommendations loaded' | ||
| : 'No rows visible — adjust filters') | ||
| : 'Buy these reservations now (one-off, processed immediately)'; | ||
| purchaseBtn.disabled = !hasSelection; | ||
| purchaseBtn.textContent = hasSelection | ||
| ? `Purchase ${selectedVisibleCount} selected` | ||
| : 'Purchase'; | ||
| purchaseBtn.title = hasSelection | ||
| ? 'Buy these reservations now (one-off, processed immediately)' | ||
| : disabledTooltip; | ||
| } | ||
| if (planBtn) { | ||
| planBtn.disabled = empty; | ||
| planBtn.textContent = empty | ||
| ? 'Create Plan' | ||
| : targetIsSelection | ||
| ? `Plan from ${targetCount} selected` | ||
| : `Plan from ${targetCount} visible`; | ||
| planBtn.title = empty | ||
| ? (loadedCount === 0 | ||
| ? 'No recommendations loaded' | ||
| : 'No rows visible — adjust filters') | ||
| : 'Schedule a recurring plan that will purchase these recommendations on a defined cadence'; | ||
| planBtn.disabled = !hasSelection; | ||
| planBtn.textContent = hasSelection | ||
| ? `Plan from ${selectedVisibleCount} selected` | ||
| : 'Create Plan'; | ||
| planBtn.title = hasSelection | ||
| ? 'Schedule a recurring plan that will purchase these recommendations on a defined cadence' | ||
| : disabledTooltip; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Do disabled HTML <button> elements reliably show native title tooltips or receive keyboard focus across major browsers?
💡 Result:
No. Disabled native elements are not reliable for native title tooltips, and they are also not reliably focusable via keyboard because disabled form controls are specified to be non-focusable/non-interactive. 1) Native title tooltips on disabled - Disabled elements are effectively non-interactive, and browsers have historically differed on whether the element’s title attribute is shown as a native tooltip. Mozilla tracked this directly: “Disabled form controls…don’t show title tooltip attribute,” noting that “disabled HTML items and tooltips” worked in IE/Chrome/Safari/Opera but did not in older Firefox, and that the issue was fixed so that Firefox matched other browsers for mousemove-driven tooltip behavior [1]. - However, because this has been a browser bug/regression class (at least in Firefox), you should not treat native title tooltips as reliably consistent across all versions and edge cases (e.g., differing event suppression and focus/hover triggers). In practice, teams often use wrappers (e.g., focusable /
) or custom tooltip implementations to ensure consistent behavior for disabled controls rather than relying on the disabled element itself [2][3]. 2) Keyboard focus on disabled
- The HTML disabled attribute “makes the element not mutable, focusable” (i.e., a disabled control is not in the focus/interaction model) [4]. - The HTML Standard similarly treats disabled elements as excluded from focusability: the focusing steps require that “the element is not actually disabled” [5]. - Therefore, across major browsers, a disabled should not receive keyboard focus (Tab focus) and should not be treated as a focusable interactive control. Bottom line - Native title tooltips: not something to rely on as “reliable across major browsers and versions,” because disabled controls are non-interactive and browser implementations have differed/bugged in the past, including Firefox [1]. - Native keyboard focus: disabled is specified/implemented to be non-focusable [4][5]. If you need tooltip + keyboard discoverability, common robust patterns are: - Keep the disabled for semantics, but render the tooltip trigger on a separate wrapper that remains focusable (tabindex="0"), and override event/pointer handling so the wrapper—not the disabled button—drives the tooltip [3][6].
Citations:
- 1: https://bugzilla.mozilla.org/show_bug.cgi?id=274626
- 2: https://stackoverflow.com/questions/2151575/how-do-you-render-tooltip-on-disabled-html-button
- 3: https://stackoverflow.com/questions/61394642/better-solution-to-display-title-attributes-in-disabled-a
- 4: http://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Attributes/disabled
- 5: https://html.spec.whatwg.org/multipage/interaction.html
- 6: https://stackoverflow.com/questions/77701318/add-tooltip-to-a-disabled-bootstrap-button
🏁 Script executed:
# Check if the file exists and read the specific lines
if [ -f "frontend/src/recommendations.ts" ]; then
echo "File found. Reading lines 1869-1892:"
sed -n '1869,1892p' "frontend/src/recommendations.ts"
else
echo "File not found at frontend/src/recommendations.ts"
# Try to locate it
find . -name "recommendations.ts" -type f 2>/dev/null
fiRepository: LeanerCloud/CUDly
Length of output: 980
Disabled buttons don't reliably show title tooltips across browsers.
Setting the title attribute on a disabled <button> is unreliable for users:
- Mouse users: Disabled buttons don't consistently display native title tooltips across browsers (Firefox historically excluded them; behavior is not guaranteed even now)
- Keyboard users: Disabled buttons are non-focusable per HTML spec, so they can't be accessed via keyboard at all, making the tooltip completely unavailable
Move the help text to a focusable wrapper element, use aria-describedby, or expose it as adjacent hint text instead of relying on the disabled control itself.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/recommendations.ts` around lines 1869 - 1892, The title/tooltip
on disabled buttons (purchaseBtn, planBtn) is unreliable and inaccessible;
instead wrap each button in a focusable container (or add a sibling hint
element) and move the explanatory text from button.title/disabledTooltip to that
wrapper/sibling, then link it with aria-describedby when the button is disabled
and ensure the wrapper is keyboard-focusable (e.g., tabindex="0") so the message
is available to mouse and keyboard users; update the logic around hasSelection
to set/remove the aria-describedby or toggle the hint text visibility rather
than setting title on the disabled control.
Two CodeRabbit findings on PR #275 (closes #273): 1. **Plan flow re-derives target from global state** — `resolvePurchaseTarget()` correctly returns `selected ∩ visible` after #273 dropped the unsafe visible-fallback for the Purchase path, but the Plan path threads only the *emptiness* of the snapshot through `openCreatePlanFromBottomBox()` and `savePlan()` re-derives the target from `state.getVisibleRecommendations()` + `getSelectedRecommendationIDs()` at modal-save time. That left the same race window the Purchase path had before #273: Refresh / filter changes / deselections between button-click and Save can silently change which recs are planned. Fix: drop the `else if (visibleRecs.length > 0)` fallback in `plans.ts::savePlan` that mirrored the old `pickBestVariantPerCell(visible)` path. Plans now require an explicit selection; with no selection the `recommendations` field is omitted entirely, so an empty plan fails visibly at Save rather than silently planning everything visible. The bottom action box already disables the button in that state; this is defence-in-depth for any caller that bypasses the disabled UI (programmatic invocation, future code paths, regressions on the gating). 2. **Disabled `<button>` `title` tooltips are unreliable** — the HTML spec excludes disabled form controls from the focus model, and browsers historically differ on whether `title` tooltips render on them. The previous fix put the disabled-state explanation directly on `purchaseBtn.title` / `planBtn.title`, which is invisible to keyboard users (can't focus disabled buttons) and inconsistent for mouse users. Fix: add a sibling `<span#recommendations-action-disabled-hint>` with `role="status"` + `aria-live="polite"` and link the buttons via `aria-describedby` when they're disabled. Hidden when a selection is active. Both keyboard and mouse paths get the message. `updateBottomActionBox` toggles the hint visibility / aria-describedby linkage on every render. Tests: - `plans.test.ts::does NOT include any recs when no selection (#273 CR follow-up)` replaces the prior `falls back to all visible` test — same setup, asserts `recommendations` is empty / undefined. - `recommendations.test.ts::button labels reflect the selection (#273)` expanded to assert the new a11y wiring: hint span visibility + textContent, `aria-describedby` set on disabled buttons and cleared when enabled, `title` attribute removed when disabled (so the unreliable channel isn't competing with the reliable one) and restored when enabled.
Member
Author
|
Addressed CR feedback. See follow-up commit. @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
…273 CR pass 2) CR's first finding on PR #275 surfaced a residual race that the initial follow-up only partly closed: dropping `savePlan`'s visible-fallback prevented the "no selection → plan everything visible" misclick path, but the Plan flow still re-derived its target from `state.getVisibleRecommendations()` + `getSelectedRecommendationIDs()` at Save time. State mutations between modal-open and modal-Save (Refresh, filter changes, deselections) could still silently shrink, widen, or replace the planned set — the same race the Purchase flow already avoided by capturing `resolvePurchaseTarget()` at button-click time. Mirror the Purchase pattern in the Plan flow: 1. `recommendations.ts::openCreatePlanFromBottomBox(snapshot)` accepts the resolved target. 2. The Plan-button click handler captures `resolvePurchaseTarget()` and passes it through. 3. `plans.ts::openCreatePlanModal(snapshot?)` stamps the snapshot on a new module-level cache `pendingPlanRecommendations`. The "Create Purchase Plan" vs "New Purchase Plan" title now keys off the snapshot, not the live selection state, so it's race-free too. 4. `openNewPlanModal()` clears the cache (the New-Plan-from-scratch path explicitly has no pre-resolved target). 5. `closePlanModal()` invalidates the cache on every close path (Save, Cancel, Esc, anything else). Once the modal closes, the snapshot's tied-to-button-click moment is over. 6. `savePlan` reads from `pendingPlanRecommendations` instead of re-deriving from state. The `state` import in plans.ts becomes unused after this and was removed. Tests updated: - Replaced `includes selected ∩ visible recommendations (Bundle B)` with `includes the snapshot stamped by openCreatePlanModal (#273 CR)` — same setup intent, but exercises the new snapshot path. - New `snapshot is immune to state mutations between modal-open and Save (#273 CR)` is the load-bearing race-closure assertion: stamps the snapshot, then mutates getSelectedRecommendationIDs + getVisibleRecommendations, then calls savePlan, asserts the plan carries the snapshot's recs (not the post-mutation state). - `does NOT include any recs when no selection (#273 CR follow-up)` now starts with `openNewPlanModal()` to clear any cache stamped by the previous test (module-level state persists across jest tests in the same file; jest.clearAllMocks doesn't touch our cache). - New `empty snapshot from openCreatePlanModal also produces no recs` pins the defensive empty-array path. - `sets "Create Purchase Plan" title when a selection exists` → `… when a snapshot is passed` — title now keys off the snapshot, not state. CR labeled this finding "🏗️ Heavy lift" — agreed in retrospect: the right fix needed plumbing through three layers (button click → openCreatePlanFromBottomBox → openCreatePlanModal → savePlan). The result is structurally simpler than the prior "re-derive from global state at Save time" model and matches the Purchase flow.
Member
Author
|
Threaded the resolved Plan target through to savePlan via a module-level snapshot, mirroring the Purchase flow. Closes the residual race window between modal-open and modal-Save. New |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
5 tasks
This was referenced May 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #273. Drops the unsafe "Purchase / Plan from N visible" fallback
on the Recommendations page bottom action box.
Before: with no checkboxes ticked, both buttons defaulted to "all
visible cells" and remained enabled. A single misclick triggered an
irreversible bulk Purchase / Plan across every visible row, and the
visible set silently changes when Refresh runs or filters mutate.
After: with 0 selected → buttons disabled, label is the static form
("Purchase" / "Create Plan"), tooltip prompts "Select at least one cell
to enable", and
resolvePurchaseTarget()returns an empty slice asdefence-in-depth (so a programmatic caller bypassing the disabled UI
also can't fire). With ≥1 selected → buttons enabled, label shows the
selection count.
Changes
frontend/src/recommendations.ts:1842::updateBottomActionBox—drops
pickBestVariantPerCell(visible)fallback; gates labels +tooltips + disabled state on
selectedVisibleCount > 0.frontend/src/recommendations.ts:1826::resolvePurchaseTarget—returns
selected ∩ visibleonly.(All N cells visible — no selection)with(Select cells to act on)so the prompt is unambiguous.Test plan
button labels reflect the selection (#273)pins bothbranches (disabled + tooltip when no selection; enabled + count
when selected).
bottom action box shows "All N visible"test renamedto
prompts for selection when no row is selected (#273)with thenew copy.
no-selection default-target fallback now seed the selection mock
explicitly so they assert the post-click flow, not the gating UI.
npm test -- --testPathPattern=recommendations— 140 / 140pass.
npm run typecheck— clean.🤖 Generated with claude-flow
Summary by CodeRabbit
New Features
Tests