feat(frontend/recs): row-click toggles selection on Opportunities (closes #344)#352
Conversation
…oses #344) Replace the row-click → detail-drawer behaviour (issue #44) with row-click → toggle selection. Clicking anywhere on a recommendation row's body now toggles the row's checkbox + dispatches the existing change handler (which enforces the one-variant-per-cell radio behaviour from #224). Clicks on interactive descendants (input / button / a / label / select / [data-action]) still flow through to their own handlers, untouched. This closes the last task in the issue-344 phase-2 plan. The original T4 (right-side Plan-builder drawer) was dropped in plan review: every payload the drawer could carry (per-rec payment-option comparison, target/baseline tracking) is explicitly backend-deferred, so the drawer would have shipped as a prettier copy of the existing bottom action-box with no new information. The detail drawer that already opened on row-click had the same character — confidence + provenance duplicated the table, and usage history is empty by design until the collector wiring lands (known_issues/28). Net changes: - recommendations.ts: -227 lines (openDetailDrawer + fetchRecommend- ationDetail + detailFetchCache + clearRecommendationDetailCache + renderUsageSparkline removed); +13 lines (row-click toggle handler). - styles/components.css: -84 lines (.detail-drawer*, .confidence-*). - recommendations.test.ts: -136 lines (5 drawer tests removed) + +84 lines (4 row-click selection tests covering: toggle on non-interactive cell, second click unselects, interactive descendant click does NOT toggle, native checkbox click doesn't double-toggle). - api/recommendations.ts: untouched. getRecommendationDetail stays exported so the backend endpoint contract is preserved; resurrecting the drawer if needed is a git revert of this commit. Verification: 1629/1632 tests pass / 1 skipped (2 unrelated failures in users.test.ts pre-date this branch — they came in with PR #348). Coverage 80.70% stmts / 82.46% lines (slight dip from 80.93/82.70 as expected when removing tested code paths; well within parity gates). Build 517 KiB (down from 522 KiB after T3; the deleted drawer assets shaved ~5 KiB).
|
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:
📝 WalkthroughWalkthroughThe PR removes the recommendation detail-drawer feature and changes row-click behavior from opening a drawer to toggling the Include checkbox selection. This includes removing detail-fetch logic, caching, drawer rendering, exported cache helpers, and all associated styling, while updating tests to validate the new selection-toggle interaction. ChangesDetail Drawer Removal and Selection Interaction Replacement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
cristim
added
triaged
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
CI run 25823020676 (pre-commit) passed on c372468. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 3287-3289: The click handler on tr (tr.addEventListener) unsafely
casts e.target to HTMLElement and calls closest(), which can throw for text
nodes; update the handler to first check that e.target is an Element (e.g.,
using "if (!(e.target instanceof Element)) return;") before assigning to target
or calling target.closest(...) so only Element nodes invoke closest and avoid
TypeError when clicking plain text.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 637ad69f-db54-422b-bebe-b0525116dfed
📒 Files selected for processing (3)
frontend/src/__tests__/recommendations.test.tsfrontend/src/recommendations.tsfrontend/src/styles/components.css
💤 Files with no reviewable changes (1)
- frontend/src/styles/components.css
| tr.addEventListener('click', (e) => { | ||
| const target = e.target as HTMLElement; | ||
| if (target.closest('input[type="checkbox"], button, a')) return; | ||
| const id = tr.dataset['recId'] || ''; | ||
| const rec = recommendations.find((r) => r.id === id); | ||
| if (rec) openDetailDrawer(rec); | ||
| if (target.closest('input, button, a, label, select, [data-action]')) return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the recommendations.ts file
fd -t f "recommendations.ts" frontend/Repository: LeanerCloud/CUDly
Length of output: 129
🏁 Script executed:
# Check file size first
wc -l frontend/src/recommendations.tsRepository: LeanerCloud/CUDly
Length of output: 98
🏁 Script executed:
# Read the relevant section of the file
sed -n '3280,3300p' frontend/src/recommendations.tsRepository: LeanerCloud/CUDly
Length of output: 1126
🌐 Web query:
Element.closest() method behavior on text nodes DOM specification
💡 Result:
Element.closest is defined on the Element interface, so when you have a Text node (Node.TEXT_NODE, nodeType 3) there is no Element.closest method to call; you must use the Text node’s parent/ancestors (which are Elements) instead. [1][2] Behavior when a “text node” is involved: 1) If you call closest on an actual Element, closest traverses that element and its ancestor Elements toward the document root until it finds one that matches the selector, returning that Element (or itself) or null if no match exists. [3] 2) If your code’s “current node” is a Text node (for example, event.target might be a Text node in some cases, or you obtained a node from a traversal), you can’t directly run closest because Text nodes are not Elements and don’t implement Element.closest. [1][2] 3) There is no requirement for closest to consider non-Element nodes (Text, Comment, etc.) in its traversal; the traversal is through Elements/Element ancestors only. [3] Practical patterns (safe for text nodes): - Start from the text node’s parentElement: let el = textNode.parentElement?.closest(selector); - Or use parentNode (if you want to reach an Element even if parentElement is null): let el = textNode.parentNode?.closest?.(selector); If you want “strictest spec answer”: closest is on Element and returns an Element or null; a text node is a Node of type TEXT_NODE (3), not an Element. [1][2][3]
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/Node
- 2: https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType
- 3: https://developer.mozilla.org/en-US/docs/Web/API/Element/closest
🏁 Script executed:
# Let's verify the behavior with a simple test
node <<'EOF'
// Test 1: Verify that Text nodes don't have closest() method
const textNode = document.createTextNode("hello");
console.log("Text node has closest?", typeof textNode.closest);
console.log("Text node nodeType:", textNode.nodeType); // Should be 3 (TEXT_NODE)
// Test 2: Verify that Element nodes have closest() method
const el = document.createElement("div");
console.log("Element has closest?", typeof el.closest);
// Test 3: Verify instanceof check
console.log("Text node instanceof Element?", textNode instanceof Element);
console.log("Element instanceof Element?", el instanceof Element);
EOFRepository: LeanerCloud/CUDly
Length of output: 688
🏁 Script executed:
# Check if there are other similar patterns in the file that might have the same issue
rg -n "e\.target as HTML" frontend/src/recommendations.tsRepository: LeanerCloud/CUDly
Length of output: 112
🏁 Script executed:
# Check if there are other similar force-cast patterns with closest() calls in the entire codebase
rg -n "\.target as HTML" frontend/src/ | head -20Repository: LeanerCloud/CUDly
Length of output: 922
🏁 Script executed:
# Let's check the context around line 332 in userList.ts to see if it has the same issue
sed -n '328,340p' frontend/src/users/userList.tsRepository: LeanerCloud/CUDly
Length of output: 506
Guard e.target before calling closest().
Line 3288 force-casts MouseEvent.target to HTMLElement without validation, but clicks can originate from text nodes. The closest() method exists only on Element nodes—calling it on a text node throws a TypeError, breaking the row toggle on plain text clicks.
Suggested fix
- const target = e.target as HTMLElement;
- if (target.closest('input, button, a, label, select, [data-action]')) return;
+ const target = e.target;
+ if (target instanceof Element && target.closest('input, button, a, label, select, [data-action]')) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tr.addEventListener('click', (e) => { | |
| const target = e.target as HTMLElement; | |
| if (target.closest('input[type="checkbox"], button, a')) return; | |
| const id = tr.dataset['recId'] || ''; | |
| const rec = recommendations.find((r) => r.id === id); | |
| if (rec) openDetailDrawer(rec); | |
| if (target.closest('input, button, a, label, select, [data-action]')) return; | |
| tr.addEventListener('click', (e) => { | |
| const target = e.target; | |
| if (target instanceof Element && target.closest('input, button, a, label, select, [data-action]')) return; |
🤖 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 3287 - 3289, The click handler
on tr (tr.addEventListener) unsafely casts e.target to HTMLElement and calls
closest(), which can throw for text nodes; update the handler to first check
that e.target is an Element (e.g., using "if (!(e.target instanceof Element))
return;") before assigning to target or calling target.closest(...) so only
Element nodes invoke closest and avoid TypeError when clicking plain text.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
…358) Four users.test.ts cases were stale on `feat/multicloud-web-frontend` and are failing on every PR-merge ref against it (including #336, #352, others). The tests assert the pre-#348 / pre-#347 behaviour: 1. `loadUsers › should load users and groups` 2. `loadUsers › should render groups after loading` `mockGroups` is missing the `allowed_accounts: []` field that the backend now ships on every group (multi-cloud account-scoping work). The actual groups from `api.listGroups` carry the field, so the deep-equal assertion against the fixture diverges. 3. `openCreateUserModal › should make password required` 4. `saveUser › should validate empty password for new user` Issue #348 ("invite users without a password and let them set it on first login") made password optional on create. userModals.ts now explicitly sets `passwordInput.required = false` (line 36) so the form-level HTML validation doesn't block the invite flow, and saveUser intentionally calls `api.createUser` with an empty password to trigger the backend invite-email path. Both old tests asserted the opposite behaviour and now fail. Fix: - Add `allowed_accounts: []` to the two `mockGroups` entries. - Invert "should make password required" → "should not mark password as required (blank invites the user)" asserting `required === false`. - Replace "should validate empty password" → "should allow empty password for new user (invite flow, issue #348)" asserting createUser was called with an empty password and no error toast was rendered. Unblocks the frontend-build-sentinel workflow on PRs against `feat/multicloud-web-frontend`. 1633 frontend tests pass.
…to feat/issue-344-row-click-selection
1 participant
Summary
Closes the last task in the issue-344 phase-2 plan — row-click toggles selection on Opportunities, replacing the previous row-click → detail-drawer behaviour.
The original T4 (right-side Plan-builder drawer) was dropped in plan review on 2026-05-13: every payload the drawer could carry (per-rec payment-option comparison, target/baseline tracking) is explicitly backend-deferred, so the drawer would have shipped as a prettier copy of the existing bottom action-box with no new information. The detail drawer that already opened on row-click had the same character: confidence + provenance duplicated the table, and
usage_historyis empty by design until the collector wiring follow-up lands (known_issues/28).What changes
Behaviour:
input,button,a,label,select,[data-action]) still flow through to their own handlers, untouched. Native checkbox clicks don't double-toggle.Code:
frontend/src/recommendations.ts:-227lines (openDetailDrawer+fetchRecommendationDetail+detailFetchCache+clearRecommendationDetailCache+renderUsageSparklineremoved);+13lines for the row-click toggle handler.frontend/src/styles/components.css:-84lines (.detail-drawer*+.confidence-*rules).frontend/src/__tests__/recommendations.test.ts:-136lines (5 drawer tests removed) ++84lines (4 row-click selection tests).frontend/src/api/recommendations.ts: untouched —getRecommendationDetailstays exported so the backend endpoint contract is preserved. Resurrecting the drawer if we ever want it back is a git revert of this commit.New tests (4)
Verification
users.test.ts(openCreateUserModal › should make password required,saveUser › should validate empty password for new user) pre-date this branch — they came in with PR feat(auth): invite users without a password and let them set it on first login #348 (invite users without password). Verified by running the failing tests on a clean stash of this branch's base.Closes #344.
Summary by CodeRabbit
Release Notes
Changes