Fleet UI: Manage local accounts host details page followup - #44013
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Pull request overview
Follow-up UI tweaks for managed local accounts on the Host details page, improving keyboard behavior, activity feed messaging, and host action tooltips.
Changes:
- Removes
onEnterbehavior from the managed account modal to avoid Enter-key conflicts. - Refetches host past activities when closing the managed account modal so “viewed managed account” appears in the feed.
- Splits the “pending” vs “failed” managed account tooltip messaging and refines activity feed copy for (un)assigned hosts.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| frontend/pages/hosts/details/HostDetailsPage/modals/ManagedAccountModal/ManagedAccountModal.tsx | Stops binding Enter key to modal cancel. |
| frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx | Refetches past activities on managed account modal close. |
| frontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/helpers.tsx | Adds distinct tooltip copy for managed account “failed” state. |
| frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx | Updates wording for managed local accounts enable/disable activities. |
| frontend/components/Modal/Modal.tsx | Documents onEnter usage caveats for modals. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RachelElysia
changed the title
WalkthroughThe pull request introduces an Possibly related PRs
🚥 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 docstrings
🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44013 +/- ##
==========================================
- Coverage 65.10% 65.09% -0.01%
==========================================
Files 2608 2608
Lines 254025 254032 +7
Branches 9401 9291 -110
==========================================
+ Hits 165371 165373 +2
- Misses 75852 75857 +5
Partials 12802 12802
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/components/Modal/Modal.tsx (1)
76-91:⚠️ Potential issue | 🟠 MajorDocument-level Enter listener has real impact and needs hardening — avoid global reach.
The
onEnterprop is actively used by 50+ modal components across the codebase. However, the listener implementation has a genuine problem:When a modal passes
onEnter, pressing Enter anywhere—including inside focused inputs, buttons, or nested modals—triggers the callback andpreventDefault(). This silently breaks native form submission, button activation, and other Enter-dependent flows.Evidence:
ActivityItem.tsxincludesstopPropagation()in its click handler specifically to work around unwanted bubbling up to the Modal'sonEnterhandler (lines 97-100). This is a symptom of the listener's overly broad reach.Required hardening:
- Only fire when the event target is inside this modal (scope via a ref on the modal container), or at minimum ignore events when
event.defaultPreventedis true or the target is a<button>,<textarea>, or contenteditable element.- Skip IME composition:
if (event.isComposing || event.keyCode === 229) return;.- Consider
keyupor checkevent.repeatto avoid repeated fires on held keys.This affects many components and should be fixed before more modals depend on this behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/components/Modal/Modal.tsx` around lines 76 - 91, The document-level Enter handler (closeOrSaveWithEnterKey used in the useEffect for onEnter) is too broad — restrict it by tying it to this modal's container ref (check that event.target is contained within the modal element) and/or ignore events when event.defaultPrevented is true or the target is a control that should keep native behavior (e.g., tagName "BUTTON", "TEXTAREA", contentEditable elements or input types that submit forms); also skip IME composition by returning early when event.isComposing or event.keyCode === 229, and avoid duplicate firings by checking event.type/key repeat (use keyup or test event.repeat) before calling onEnter; keep the same add/remove listener logic in the useEffect but apply these guards inside closeOrSaveWithEnterKey.
🧹 Nitpick comments (1)
frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx (1)
1625-1635: Refetch on close looks correct; minor: only past activities are refreshed.The backend (
ee/server/service/hosts.goGetHostManagedAccountPassword) writes theActivityTypeViewedManagedLocalAccountsynchronously before returning the password, so by the time the modal is closed the activity is already persisted — the refetch here will pick it up. Two small nits:
- If the user is currently viewing the "Upcoming" tab, the new past activity won't appear until they switch tabs and
pastActivitiesis re-read. That's probably fine since this activity is always "past", but consider whether arefetchUpcomingActivities()is also warranted for consistency with the other close handlers (onCloseScriptModalGroup,onCancelRunScriptDetailsModal) that refetch both.- The inline arrow allocates a new handler every render. Not a functional issue, just stylistically inconsistent with the surrounding
useCallback-based handlers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx` around lines 1625 - 1635, The current inline onCancel handler for ManagedAccountModal always allocates a new function and only calls refetchPastActivities(), so refetching is inconsistent with other close handlers; change the inline arrow to a stable useCallback handler (e.g., handleManagedAccountModalCancel) and have it call setShowManagedAccountModal(false) and both refetchPastActivities() and refetchUpcomingActivities() for consistency with onCloseScriptModalGroup/onCancelRunScriptDetailsModal so the new activity is visible in all tabs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/components/Modal/Modal.tsx`:
- Around line 76-91: The document-level Enter handler (closeOrSaveWithEnterKey
used in the useEffect for onEnter) is too broad — restrict it by tying it to
this modal's container ref (check that event.target is contained within the
modal element) and/or ignore events when event.defaultPrevented is true or the
target is a control that should keep native behavior (e.g., tagName "BUTTON",
"TEXTAREA", contentEditable elements or input types that submit forms); also
skip IME composition by returning early when event.isComposing or event.keyCode
=== 229, and avoid duplicate firings by checking event.type/key repeat (use
keyup or test event.repeat) before calling onEnter; keep the same add/remove
listener logic in the useEffect but apply these guards inside
closeOrSaveWithEnterKey.
---
Nitpick comments:
In `@frontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsx`:
- Around line 1625-1635: The current inline onCancel handler for
ManagedAccountModal always allocates a new function and only calls
refetchPastActivities(), so refetching is inconsistent with other close
handlers; change the inline arrow to a stable useCallback handler (e.g.,
handleManagedAccountModalCancel) and have it call
setShowManagedAccountModal(false) and both refetchPastActivities() and
refetchUpcomingActivities() for consistency with
onCloseScriptModalGroup/onCancelRunScriptDetailsModal so the new activity is
visible in all tabs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: deb25bb9-f60f-4e1d-80ea-8e4318b6acde
📒 Files selected for processing (6)
frontend/components/Modal/Modal.tsxfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsxfrontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/HostActionsDropdown.tests.tsxfrontend/pages/hosts/details/HostDetailsPage/HostActionsDropdown/helpers.tsxfrontend/pages/hosts/details/HostDetailsPage/HostDetailsPage.tsxfrontend/pages/hosts/details/HostDetailsPage/modals/ManagedAccountModal/ManagedAccountModal.tsx
Issue
Closes #31741
Description
Follow-up post-review
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Tests