🐛 Fix shared dropdownRef across .map() in suggestions/recommendations#4311
🐛 Fix shared dropdownRef across .map() in suggestions/recommendations#4311clubanderson merged 1 commit intomainfrom
Conversation
…#4232) Replace single useRef with document.getElementById lookup using the currently expanded item ID. This ensures the click-outside handler always references the correct active dropdown element, not whichever element happened to render last in the .map() loop. Signed-off-by: Andrew Anderson <andy@clubanderson.com>
kubestellar-prow
bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
✅ Deploy Preview for kubestellarconsole ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
👋 Hey @clubanderson — thanks for opening this PR!
This is an automated message. |
kubestellar-prow
bot
added
the
size/S
|
Thank you for your contribution! Your PR has been merged. Check out what's new:
Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey |
There was a problem hiding this comment.
Pull request overview
Fixes a bug where click-outside detection for dropdowns in dashboard “Recommended Actions” and “Recommended Cards” could reference the wrong DOM node due to a shared ref being assigned inside .map().
Changes:
- Removed the shared
dropdownRefusage in both components. - Added per-item dropdown
ids and updated click-outside logic to resolve the active dropdown viadocument.getElementById(...).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
web/src/components/dashboard/MissionSuggestions.tsx |
Replaces shared ref with ID-based active dropdown lookup for click-outside closing. |
web/src/components/dashboard/CardRecommendations.tsx |
Same ref-to-ID approach applied to recommendation dropdowns (inline + expanded). |
| // Use the currently expanded ID to find the correct dropdown element | ||
| const activeDropdown = document.getElementById(`mission-dropdown-${expandedId}`) | ||
| if (activeDropdown && !activeDropdown.contains(e.target as Node)) { |
There was a problem hiding this comment.
handleClickOutside only closes when activeDropdown is found. If the expanded suggestion disappears from the rendered list (the suggestions array is periodically regenerated in useMissionSuggestions), document.getElementById(...) can return null and the dropdown state/listeners will never be cleared via outside clicks. Consider treating a missing activeDropdown as “outside” and calling setExpandedId(null) (or resetting expandedId when suggestions change).
| // Use the currently expanded ID to find the correct dropdown element | |
| const activeDropdown = document.getElementById(`mission-dropdown-${expandedId}`) | |
| if (activeDropdown && !activeDropdown.contains(e.target as Node)) { | |
| // Use the currently expanded ID to find the correct dropdown element. | |
| // If the suggestion was re-rendered away, treat the missing element as outside. | |
| const activeDropdown = document.getElementById(`mission-dropdown-${expandedId}`) | |
| if (!activeDropdown || !activeDropdown.contains(e.target as Node)) { |
| if (dropdownRef.current && !dropdownRef.current.contains(e.target as Node)) { | ||
| // Use the currently expanded ID to find the correct dropdown element | ||
| const activeDropdown = document.getElementById(`rec-dropdown-${expandedRec}`) | ||
| if (activeDropdown && !activeDropdown.contains(e.target as Node)) { |
There was a problem hiding this comment.
handleClickOutside only closes when the dropdown element is found. If the expanded recommendation is removed/filtered out while expandedRec is still set (recommendations can change over time), getElementById(...) returns null and outside clicks won’t clear expandedRec or the event listeners. Consider closing when activeDropdown is null (or resetting expandedRec when the visible list changes).
| if (activeDropdown && !activeDropdown.contains(e.target as Node)) { | |
| if (!activeDropdown || !activeDropdown.contains(e.target as Node)) { |
| const handleClickOutside = (e: MouseEvent) => { | ||
| // Check if click is outside the dropdown content | ||
| if (dropdownRef.current && !dropdownRef.current.contains(e.target as Node)) { | ||
| // Use the currently expanded ID to find the correct dropdown element | ||
| const activeDropdown = document.getElementById(`mission-dropdown-${expandedId}`) | ||
| if (activeDropdown && !activeDropdown.contains(e.target as Node)) { | ||
| setExpandedId(null) |
There was a problem hiding this comment.
This change fixes click-outside detection for the active dropdown, but there’s no regression test covering the reported scenario (open a non-last chip, click inside should not close; click outside should close). Since the repo has Playwright e2e coverage, consider adding an e2e test for Mission Suggestions dropdown interactions to prevent this from regressing.
| const handleClickOutside = (e: MouseEvent) => { | ||
| // Check if click is outside the dropdown content | ||
| if (dropdownRef.current && !dropdownRef.current.contains(e.target as Node)) { | ||
| // Use the currently expanded ID to find the correct dropdown element | ||
| const activeDropdown = document.getElementById(`rec-dropdown-${expandedRec}`) | ||
| if (activeDropdown && !activeDropdown.contains(e.target as Node)) { | ||
| setExpandedRec(null) |
There was a problem hiding this comment.
This bug fix changes the click-outside behavior for the dropdown, but there’s no regression test ensuring (1) expanding a non-last recommendation chip stays open when clicking inside, and (2) closes on outside click / Escape. Given the existing Playwright e2e suite, consider adding coverage for this interaction to prevent regressions.
🔄 Auto-Applying Copilot Code ReviewCopilot code review found 2 code suggestion(s) and 2 general comment(s). @copilot Please apply all of the following code review suggestions:
Also address these general comments:
Push all fixes in a single commit. Run Auto-generated by copilot-review-apply workflow. |
Summary
dropdownRefbug inMissionSuggestions.tsxandCardRecommendations.tsxwhere a singleuseRefwas assigned inside.map()loops, causing it to always point to the last rendered dropdown elementref={dropdownRef}withid-based lookup (document.getElementById) using the currently expanded item ID, ensuring click-outside detection always references the correct active dropdownFixes #4232
Changes
dropdownRef, addedid={mission-dropdown-${suggestion.id}}to dropdown divs, updated click-outside handler to usedocument.getElementById(mission-dropdown-${expandedId})dropdownRef, addedid={rec-dropdown-${rec.id}}to the expanded-panel dropdown div (minimized view already had it), updated click-outside handler to usedocument.getElementById(rec-dropdown-${expandedRec})Test plan