Fleet UI: Command palette followup - #46346
Conversation
Follow-ups to the merged Fleet Spotlight (#43756): - Fleet switcher: extract buildFleetSwitchUrl helper; mirror useTeamIdParam.handleTeamChange's strip rules so page/team_id and Hosts-only fleet-scoped params (script_batch_execution_*, software_status on All) don't leak across fleets. - Add hosts / manage enroll secrets: new withTeamRequired helper so the "Unassigned" chip on All-fleets matches the URL the modal opens against. Free-tier suppresses the chip and the fleet_id=0 redirect-strip dance. - Policy automations: use withTeamId for the four sub-items instead of manual fleet_id interpolation. - Manage report automations: gate the palette item on canManageReportAutomations (admin) instead of canWrite. - No-access users: gate global key handlers on !isNoAccess so Cmd+K and the Escape capture listener don't intercept shortcuts for a palette they can't open. - Cmd+K: normalize key case so Caps Lock doesn't break the toggle. - valueToParentId: normalize the cmdk value before lookup so arrow-key auto-expand actually fires. - Back button: tabIndex=-1 so Radix's open-autofocus lands on the input (Backspace/Escape still navigate back). Deep-link permission gates (param alone must not bypass in-page authorization): - ManageQueriesPage: gate ?manage_automations=1 on canManageAutomations && isManageAutomationsEnabled. - ManageFleetsPage: gate ?create_fleet=1 on shared isCreateFleetDisabled (Primo + GitOps). - DashboardPage: gate ?manage_automations=1 on canEditActivityFeedAutomations. Routing convention: - ScriptLibrary and ManageFleetsPage: replace window.history.replaceState with router.replace so react-router's location stays in sync. - .claude/rules/fleet-frontend.md: add Routing & URL state section codifying the pattern.
Round 2 of the deep-link audit — pages that opened modals from URL params without re-checking the same predicate the in-page action uses. Same shape as the prior commit: gate the modal open on the page's existing permission predicate; strip the param either way so refreshes don't keep trying. - ManageHostsPage: gate ?add_hosts=1 and ?manage_enroll_secrets=1 on canEnrollHosts (hoisted above the deep-link effects). - ManagePoliciesPage: wrap the manage_automations type switch in canManageAutomations; fix the stale single-key dep array. - SoftwarePage: add isGlobalAdmin to the manage_automations gate (mirrors canManageSoftwareAutomations); keep the wait-for-config semantics so the param survives until ready. - Variables: rewrite the ?add_variable=1 handler from window.location.search + window.history.replaceState to location.query + router.replace, and gate on canEdit. Added router/location props via the usual InjectedRouter pattern. Updated 11 existing tests to pass the new props via baseProps. After this commit, every palette-emitted deep-link (add_hosts, manage_enroll_secrets, add_script, add_variable, create_fleet, and the four manage_automations destinations) is gated on the same predicate as its in-page entry point, and no page handler uses window.* APIs to read or mutate the URL.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #46346 +/- ##
==========================================
+ Coverage 66.89% 66.91% +0.02%
==========================================
Files 2782 2793 +11
Lines 221734 221876 +142
Branches 11356 11444 +88
==========================================
+ Hits 148334 148474 +140
+ Misses 59999 59998 -1
- Partials 13401 13404 +3
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:
|
Revert the withTeamRequired/teamRequiredDestination experiment. The earlier change made add-hosts and manage-enroll-secrets route to Unassigned (fleet_id=0) when invoked from All fleets so the "Unassigned" chip matched the URL. The better behavior is to not switch teams at all: on All fleets these commands operate against the global enroll secrets, which is what users expect. - Switch both items back to withTeamId — drops fleet_id on All fleets, preserving the user's current team context. - Remove the chip entirely for these commands (no destination switch, no chip needed). - Delete the now-unused withTeamRequired callback, teamRequiredDestination derivation, and their interface/test scaffolding. Also tighten canManageReportAutomations to (isGlobalAdmin || isTeamAdmin) so it matches ManageQueriesPage exactly — the palette URL targets the current team via withTeamId, so an isAnyTeamAdmin user who isn't admin of the current team would land on Reports and find no Manage automations button.
![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
This PR follows up on the Fleet command palette by tightening deep-link handling, aligning command visibility with destination-page permissions, and moving fleet-switch URL logic into a tested helper.
Changes:
- Adds permission-aware query-param handling for command-palette deep links across pages.
- Refactors and tests command palette fleet-switch URL generation and item gating.
- Documents frontend routing guidance to use react-router instead of direct window history/location mutation.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/pages/SoftwarePage/SoftwarePage.tsx | Gates software automation deep-link opening on config/team/admin state. |
| frontend/pages/queries/ManageQueriesPage/ManageQueriesPage.tsx | Gates report automation deep-link opening on admin access and available queries. |
| frontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsx | Gates policy automation modal deep-links on automation permissions. |
| frontend/pages/ManageControlsPage/Variables/Variables.tsx | Moves add-variable deep-link handling to router/location props with edit gating. |
| frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx | Updates Variables tests to pass router/location props. |
| frontend/pages/ManageControlsPage/Scripts/Scripts.tsx | Adds add_script to script page query typing. |
| frontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tsx | Replaces direct window history mutation with router-based add-script deep-link handling. |
| frontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tests.tsx | Adds add-script deep-link tests for admins and technicians. |
| frontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx | Gates add-hosts and enroll-secret deep-links on enrollment permissions. |
| frontend/pages/DashboardPage/DashboardPage.tsx | Gates activity automation deep-link handling on global-admin/all-fleets access. |
| frontend/pages/admin/ManageFleetsPage/ManageFleetsPage.tsx | Adds router-based create-fleet deep-link handling and shared create-disabled logic. |
| frontend/components/CommandPalette/helpers.ts | Adds report automation permission context and buildFleetSwitchUrl. |
| frontend/components/CommandPalette/helpers.tests.ts | Adds tests for report automation gating and fleet-switch URL behavior. |
| frontend/components/CommandPalette/groups/derivations.ts | Removes unused team-required chip derivation. |
| frontend/components/CommandPalette/groups/commands.ts | Removes add-host/enroll-secret destination chip usage. |
| frontend/components/CommandPalette/groups/automations.ts | Narrows report automation gating and normalizes policy automation paths. |
| frontend/components/CommandPalette/CommandPalette.tsx | Uses fleet-switch helper, improves shortcut handling, and adds no-access protections. |
| frontend/components/CommandPalette/CommandPalette.tests.tsx | Adds a no-access shortcut interception test. |
| .claude/rules/fleet-frontend.md | Documents react-router URL state conventions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (location.query.create_fleet !== "1") return; | ||
| if (!isCreateFleetDisabled) { | ||
| setShowCreateFleetModal(true); |
| if (location.query.manage_automations !== "1") return; | ||
| if (canEditActivityFeedAutomations) { | ||
| setShowActivityFeedAutomationsModal(true); | ||
| // Clean up the query param from the URL, preserving other params | ||
| const { manage_automations, ...rest } = location.query; | ||
| router.replace({ pathname, query: rest }); | ||
| } | ||
| }, [location.query.manage_automations, pathname, router]); | ||
| const { manage_automations, ...rest } = location.query; | ||
| router.replace({ pathname, query: rest }); |
| if (queryParams?.add_hosts !== "1") return; | ||
| if (canEnrollHosts) { | ||
| setShowAddHostsModal(true); | ||
| const { add_hosts, ...rest } = queryParams; | ||
| router.replace({ pathname: location.pathname, query: rest }); | ||
| } | ||
| }, [queryParams?.add_hosts, location.pathname, router]); | ||
| const { add_hosts, ...rest } = queryParams; | ||
| router.replace({ pathname: location.pathname, query: rest }); |
| if (queryParams?.manage_enroll_secrets !== "1") return; | ||
| if (canEnrollHosts) { | ||
| setShowEnrollSecretModal(true); | ||
| const { manage_enroll_secrets, ...rest } = queryParams; | ||
| router.replace({ pathname: location.pathname, query: rest }); | ||
| } | ||
| }, [queryParams?.manage_enroll_secrets, location.pathname, router]); | ||
| const { manage_enroll_secrets, ...rest } = queryParams; | ||
| router.replace({ pathname: location.pathname, query: rest }); |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds buildFleetSwitchUrl and tests, updates CommandPalette to use it, introduces canManageReportAutomations, removes teamRequiredDestination, prevents keyboard registration and Escape-to-root handling for no-access users, normalizes highlight lookup, and sets Back button tabIndex. It also standardizes deep-link handling across Variables, ManageHosts, ScriptLibrary, Dashboard, Software, ManagePolicies, ManageQueries, and ManageFleets to gate modal opening on permissions/data readiness and to strip triggering query params via router.replace. Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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
🧹 Nitpick comments (1)
frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx (1)
16-19: ⚡ Quick winAdd explicit deep-link tests for
add_variablegating and param cleanup.This suite wires the new props, but it doesn’t verify the new behavior (
add_variable=1opens only for editors and always gets stripped). Adding those two cases would lock in the security/UX contract.🤖 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/pages/ManageControlsPage/Variables/Variables.tests.tsx` around lines 16 - 19, Add two unit tests that verify deep-link behavior for the add_variable query param: (1) when rendering the Variables page (the test subject using baseProps and createMockRouter) as an editor, include location.query = { add_variable: "1" } and assert the add-variable modal/dialog is opened and that the router.replace (or router.push used for cleanup) is called to strip the add_variable param; (2) when rendering as a non-editor with the same location.query, assert the modal does NOT open and that router.replace is still called to remove the add_variable param. Mock or stub the permission/role check you use in the component (so tests can simulate editor vs non-editor), and verify the query cleanup call and modal-open state in each test.
🤖 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/pages/admin/ManageFleetsPage/ManageFleetsPage.tsx`:
- Around line 68-85: The deep-link useEffect opens/removes the ?create_fleet
param before config is loaded because isCreateFleetDisabled is computed when
config is undefined; change the effect (useEffect) to first guard for config
availability (e.g., return if config is undefined/null), then evaluate
isCreateFleetDisabled and only after that decide to call
setShowCreateFleetModal(true) and call router.replace to strip create_fleet from
location.query; reference isCreateFleetDisabled, useEffect, location.query,
setShowCreateFleetModal, and router.replace so the check and param removal
happen only after config is present.
In `@frontend/pages/SoftwarePage/SoftwarePage.tsx`:
- Around line 246-252: The code returns early when !isAllTeamsSelected &&
!isPrimoMode, which prevents removing the manage_automations query param; always
strip manage_automations from queryParams regardless of the gate. Update the
effect so that extraction and router.replace({ pathname: location.pathname,
query: rest }) (using queryParams and manage_automations) always runs, and only
conditionally call setShowManageAutomationsModal when isGlobalAdmin (keeping
checks on isAllTeamsSelected and isPrimoMode for showing the modal), ensuring
manage_automations is removed from the URL even if the modal is not shown.
---
Nitpick comments:
In `@frontend/pages/ManageControlsPage/Variables/Variables.tests.tsx`:
- Around line 16-19: Add two unit tests that verify deep-link behavior for the
add_variable query param: (1) when rendering the Variables page (the test
subject using baseProps and createMockRouter) as an editor, include
location.query = { add_variable: "1" } and assert the add-variable modal/dialog
is opened and that the router.replace (or router.push used for cleanup) is
called to strip the add_variable param; (2) when rendering as a non-editor with
the same location.query, assert the modal does NOT open and that router.replace
is still called to remove the add_variable param. Mock or stub the
permission/role check you use in the component (so tests can simulate editor vs
non-editor), and verify the query cleanup call and modal-open state in each
test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff556984-8e84-4d39-8288-98cb0700e292
⛔ Files ignored due to path filters (1)
.claude/rules/fleet-frontend.mdis excluded by!**/*.md
📒 Files selected for processing (18)
frontend/components/CommandPalette/CommandPalette.tests.tsxfrontend/components/CommandPalette/CommandPalette.tsxfrontend/components/CommandPalette/groups/automations.tsfrontend/components/CommandPalette/groups/commands.tsfrontend/components/CommandPalette/groups/derivations.tsfrontend/components/CommandPalette/helpers.tests.tsfrontend/components/CommandPalette/helpers.tsfrontend/pages/DashboardPage/DashboardPage.tsxfrontend/pages/ManageControlsPage/Scripts/Scripts.tsxfrontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tests.tsxfrontend/pages/ManageControlsPage/Scripts/cards/ScriptLibrary/ScriptLibrary.tsxfrontend/pages/ManageControlsPage/Variables/Variables.tests.tsxfrontend/pages/ManageControlsPage/Variables/Variables.tsxfrontend/pages/SoftwarePage/SoftwarePage.tsxfrontend/pages/admin/ManageFleetsPage/ManageFleetsPage.tsxfrontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsxfrontend/pages/policies/ManagePoliciesPage/ManagePoliciesPage.tsxfrontend/pages/queries/ManageQueriesPage/ManageQueriesPage.tsx
💤 Files with no reviewable changes (2)
- frontend/components/CommandPalette/groups/commands.ts
- frontend/components/CommandPalette/groups/derivations.ts
On a fresh page load, role flags and config can still be unset when deep-link effects first run. The previous code treated those falsy gates as authoritative and stripped the query param, so an authorized user landing directly on ?add_hosts=1, ?create_fleet=1, ?manage_automations=1, etc. could be silently denied the modal. Each effect now waits for the relevant slice of AppContext (config, isGlobalAdmin, isRouteOk) before evaluating the gate or stripping the param. Software page additionally always strips after gate evaluation so a team-scoped load doesn't leave manage_automations stuck in the URL.
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/pages/SoftwarePage/SoftwarePage.tsx (1)
243-259:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd hydration gate for
isGlobalAdminto prevent race condition.The effect waits for
isSoftwareConfigLoadedbut not forisGlobalAdmin !== undefined. If config loads before the role flag is set, the modal won't open for an authorized admin and the query param will be stripped prematurely. WhenisGlobalAdminlater becomestrue, the param is already gone and the effect early-returns at line 244.ManageHostsPage (lines 250, 269 in this PR) follows the standardized pattern by gating on both config readiness and
isGlobalAdmin !== undefined. SoftwarePage should match that approach.🔐 Proposed fix to add the hydration gate
useEffect(() => { if (queryParams?.manage_automations !== "1") return; if (!isSoftwareConfigLoaded) return; + if (isGlobalAdmin === undefined) return; if (isGlobalAdmin && (isAllTeamsSelected || isPrimoMode)) { setShowManageAutomationsModal(true); }🤖 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/pages/SoftwarePage/SoftwarePage.tsx` around lines 243 - 259, The effect in SoftwarePage's useEffect that handles queryParams/manage_automations needs a hydration gate for isGlobalAdmin: currently it only waits for isSoftwareConfigLoaded and may strip the query before isGlobalAdmin is known. Update the early-return logic in the useEffect (the block using queryParams, isSoftwareConfigLoaded, setShowManageAutomationsModal, and router.replace) to also return early when isGlobalAdmin === undefined (i.e., wait for isGlobalAdmin to be hydrated), mirroring the pattern used in ManageHostsPage, and keep isGlobalAdmin in the dependency list so the modal opens correctly once the flag becomes defined.
🤖 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.
Outside diff comments:
In `@frontend/pages/SoftwarePage/SoftwarePage.tsx`:
- Around line 243-259: The effect in SoftwarePage's useEffect that handles
queryParams/manage_automations needs a hydration gate for isGlobalAdmin:
currently it only waits for isSoftwareConfigLoaded and may strip the query
before isGlobalAdmin is known. Update the early-return logic in the useEffect
(the block using queryParams, isSoftwareConfigLoaded,
setShowManageAutomationsModal, and router.replace) to also return early when
isGlobalAdmin === undefined (i.e., wait for isGlobalAdmin to be hydrated),
mirroring the pattern used in ManageHostsPage, and keep isGlobalAdmin in the
dependency list so the modal opens correctly once the flag becomes defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fd877f6b-25e7-41f6-9da3-e15e04e4f853
📒 Files selected for processing (4)
frontend/pages/DashboardPage/DashboardPage.tsxfrontend/pages/SoftwarePage/SoftwarePage.tsxfrontend/pages/admin/ManageFleetsPage/ManageFleetsPage.tsxfrontend/pages/hosts/ManageHostsPage/ManageHostsPage.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/pages/admin/ManageFleetsPage/ManageFleetsPage.tsx
- frontend/pages/DashboardPage/DashboardPage.tsx
Swap fireEvent.click → user.click for the "View host" navigation test in the command palette suite. Keep fireEvent for the chevron sub-item toggle (and add a note explaining why) — cmdk's userEvent-aware selection handlers fire the parent item's onSelect on user.click, navigating instead of toggling. A bare fireEvent.click respects the chevron's stopPropagation. Also add a ScriptLibrary regression test covering the second-deep-link flow: arrive via ?add_script=1 → modal opens, simulate the param strip, user closes the modal, palette pushes ?add_script=1 again → modal reopens. At the component level the effect handles this correctly; the test guards against regressions there.
|
@claude review |
| useEffect(() => { | ||
| const param = queryParams?.manage_automations; | ||
| if (!param) return; | ||
|
|
||
| switch (param) { | ||
| case "webhooks": | ||
| setShowOtherWorkflowsModal(true); | ||
| break; | ||
| case "install_software": | ||
| setShowInstallSoftwareModal(true); | ||
| break; | ||
| case "run_script": | ||
| setShowPolicyRunScriptModal(true); | ||
| break; | ||
| case "calendar": | ||
| setShowCalendarEventsModal(true); | ||
| break; | ||
| case "conditional_access": | ||
| setShowConditionalAccessModal(true); | ||
| break; | ||
| default: | ||
| if (canManageAutomations) { | ||
| switch (param) { | ||
| case "webhooks": | ||
| setShowOtherWorkflowsModal(true); | ||
| break; | ||
| case "install_software": | ||
| setShowInstallSoftwareModal(true); | ||
| break; | ||
| case "run_script": | ||
| setShowPolicyRunScriptModal(true); | ||
| break; | ||
| case "calendar": | ||
| setShowCalendarEventsModal(true); | ||
| break; | ||
| case "conditional_access": | ||
| setShowConditionalAccessModal(true); | ||
| break; | ||
| default: | ||
| } | ||
| } | ||
|
|
||
| const { manage_automations, ...rest } = queryParams; | ||
| router.replace({ pathname: location.pathname, query: rest }); | ||
| }, [queryParams?.manage_automations]); | ||
| }, [queryParams, location.pathname, router, canManageAutomations]); |
There was a problem hiding this comment.
🔴 ManagePoliciesPage's new ?manage_automations=… effect (lines 574-601) has the same hydration race that Copilot flagged and you fixed in DashboardPage / ManageHostsPage: canManageAutomations is falsy while AppContext role flags are still undefined, so on a direct/refreshed/shared load of the URL the palette emits, the switch block is skipped, router.replace strips the param, and the authorized admin never sees the modal. Adding the same if (isGlobalAdmin === undefined || !isRouteOk) return; gate you already applied elsewhere fixes it — isRouteOk is already in scope from useTeamIdParam. Variables.tsx and ScriptLibrary.tsx have the same shape but are protected by the AuthAnyMaintainerAdminTechnicianRoutes wrapper on /controls, so the race can't manifest there in practice; still worth a defensive guard for consistency.
Extended reasoning...
The race
canManageAutomations (line 246) resolves to isGlobalAdmin || isGlobalMaintainer || isTeamMaintainer || isTeamAdmin. The first two come from AppContext and start as undefined (frontend/context/app.tsx:235). setPermissions only populates them once both currentUser and config have hydrated (frontend/context/app.tsx:295-297 returns {} when either is null). The team-scoped flags from useTeamIdParam also short-circuit to false until currentTeam?.id is set (frontend/hooks/useTeamIdParam.ts:512-516). So canManageAutomations is falsy on the first render.
The new effect at ManagePoliciesPage.tsx:574-601 has only one guard — if (!param) return;. There is no hydration gate. The effect runs to completion in the falsy-role state, skips the switch block, and unconditionally calls router.replace to strip the param.
Why the previously-merged fixes in the same PR don't cover this one
This PR's commit 7eb56ec5 ("Wait for AppContext hydration before evaluating deep-links") added if (isGlobalAdmin === undefined || !isRouteOk) return; to DashboardPage, ManageHostsPage (×2), ManageFleetsPage, and SoftwarePage after Copilot flagged the identical race. ManagePoliciesPage exposes isRouteOk from the same useTeamIdParam destructure (line 234) but never got the gate — likely an oversight, because the permission check (if (canManageAutomations)) was added in this PR's commit 953899dc and the matching hydration gate was not.
Step-by-step proof for /policies/manage?manage_automations=webhooks
Assume a global admin uses the new palette item this PR ships (manage-policy-automations-webhooks in automations.ts:90-101) on a cold tab.
- Initial render:
currentUserisundefined, so AuthenticatedRoutes blocks the children — no harm yet. SET_CURRENT_USERreducer fires after/meresolves.setPermissionsruns withstate.config == nulland returns{}(app.tsx:295-297). SocurrentUseris now truthy butisGlobalAdmin,isGlobalMaintainer, etc. are stillundefined. ManagePoliciesPage mounts.- Effect runs with
queryParams.manage_automations === "webhooks",canManageAutomations === undefined. Theif (canManageAutomations)branch is skipped.router.replace({ pathname, query: rest })stripsmanage_automations. SET_CONFIGfires when/configresolves.setPermissionsnow produces real values.canManageAutomationsbecomestrue. The effect re-runs (it depends oncanManageAutomations), butqueryParams.manage_automationsis gone, so theif (!param) return;early-return fires. The modal never opens.
The URL is now clean and the user has no signal that anything went wrong — they simply land on /policies/manage and have to discover Manage automations through the page UI instead.
Impact and reachability
The palette this PR introduces emits exactly these URLs (groups/automations.ts:90-178 builds manage-policy-automations-webhooks, …-install_software, …-run_script, …-calendar, …-conditional_access all pointing at ${paths.MANAGE_POLICIES}?manage_automations=…). Anyone using those palette items, or anyone who bookmarks / shares one, hits the race on cold-tab loads. React 18 batching mitigates most cases where setCurrentUser and setConfig commit together, but Fleet's fetchCurrentUser dispatches setCurrentUser then awaits a separate fetchConfig() network roundtrip — those commits are not batched together, so step 2 above is observable rather than hypothetical.
Fix
Same pattern you applied four times in this PR:
useEffect(() => {
const param = queryParams?.manage_automations;
if (!param) return;
if (isGlobalAdmin === undefined || !isRouteOk) return; // <-- add this
if (canManageAutomations) { ... switch ... }
const { manage_automations, ...rest } = queryParams;
router.replace({ pathname: location.pathname, query: rest });
}, [queryParams, location.pathname, router, canManageAutomations, isGlobalAdmin, isRouteOk]);Addressing the route-guard refutation for Variables.tsx and ScriptLibrary.tsx
The synthesis bundled in a claim that Variables.tsx (lines 67-79) and ScriptLibrary.tsx (lines 66-75) have the same race. The structure is the same, but routing/index.tsx wraps /controls in AuthAnyMaintainerAdminTechnicianRoutes (line 309), which returns null and schedules a 403 when all six role flags are simultaneously falsy/undefined — i.e., during the exact window the race would otherwise manifest. So the inner effect never runs in the bad state, and the modal-leak / param-strip bug cannot actually be observed on those two pages.
That's why I'm flagging this strictly as a ManagePoliciesPage bug (which is on /policies/manage and has no such wrapper at router/index.tsx:422). The matching guard on Variables.tsx and ScriptLibrary.tsx would be a nice defensive consistency tweak — the rest of the PR adopted the same pattern — but it isn't correctness-blocking on its own, so I wouldn't hold the PR for it.
🔬 also observed by copilot-pull-request-reviewer
There was a problem hiding this comment.
will followup Monday
Issue
Closes #43757
Description
Testing
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests