Fleet UI: View, add, edit, delete custom categories - #46443
Conversation
Adds the Software > Library > Self-service categories page (Premium) with add / edit / delete flows, wires a Categories button into the existing Library page as the entry point, and adds tests covering the empty, populated, conflict, cancel, and observer states. BE is not implemented yet — service client wraps a dev-only mock store behind NODE_ENV === "development" so the UI is usable in `make serve`.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feat/39018-self-service-categories #46443 +/- ##
======================================================================
+ Coverage 66.88% 66.95% +0.07%
======================================================================
Files 2785 2800 +15
Lines 222093 222404 +311
Branches 11416 11527 +111
======================================================================
+ Hits 148538 148910 +372
+ Misses 60123 60061 -62
- Partials 13432 13433 +1
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:
|
BE for #39018 is wiring up the real routes, so drop the dev-only in-memory mock store and call the actual endpoints directly.
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.
|
@claude review |
- Reveal row actions on :focus-within for keyboard users - Derive isTeamAdmin/isTeamMaintainer from useTeamIdParam so the page reflects the route's fleet immediately, and key the React Query cache on teamIdForApi - Disable Save in Edit modal when the name is unchanged - Disable Cancel and lock modal content while submit/delete is in flight - Use straight apostrophes in error copy + matching test assertions - Add generic-error MSW handlers and tests for add/edit/delete
| <Modal | ||
| title="Add category" | ||
| onExit={onExit} | ||
| className={baseClass} | ||
| isContentDisabled={isSubmitting} | ||
| > |
There was a problem hiding this comment.
🟡 Follow-up to the prior review: the isContentDisabled={isSubmitting}/{isDeleting} fix applied to all three modals doesn't actually gate the X icon, Escape key, or backdrop click — those paths are only gated by disableClosingModal in Modal.tsx (lines 80, 137, 185), and Modal.tsx:32-37 JSDoc explicitly says "The top right will not be disabled and will still be clickable." Worse, Modal.tsx:191 sets autofocus={isContentDisabled}, so during an in-flight request the X button actively receives focus. If the user hits Escape / clicks X / clicks the backdrop mid-submit, the modal unmounts but the closure-captured onSuccess still fires when the request resolves — calling renderFlash("success", ...) on the parent (most jarring on Delete: "Successfully deleted self-service category." after a cancel, and the row really is gone). One-line fix per modal: use disableClosingModal={isSubmitting} (or disableClosingModal={isDeleting}) in addition to (or in place of) isContentDisabled on each of AddCategoryModal, EditCategoryModal, and DeleteCategoryModal.
Extended reasoning...
What the bug is
This is a follow-up to the previous review comment about the cancel-mid-submit race. The author applied the suggested fix on all three modals — isContentDisabled={isSubmitting} on AddCategoryModal (AddCategoryModal.tsx:66) and EditCategoryModal (EditCategoryModal.tsx:67), isContentDisabled={isDeleting} on DeleteCategoryModal (DeleteCategoryModal.tsx:43), plus disabled={isSubmitting}/disabled={isDeleting} on the Cancel buttons. That gates Cancel correctly. But isContentDisabled is the wrong prop for the X icon, Escape key, and backdrop click — those are governed exclusively by disableClosingModal. The original review suggested isContentDisabled as a complete fix, and that suggestion was incomplete.
The code path
Verified in frontend/components/Modal/Modal.tsx:
Modal.tsx:32-37JSDoc onisContentDisabled: "At the moment this will place an overlay over the modal content and make it unclickable. The top right will not be disabled and will still be clickable."Modal.tsx:80-89: the Escape-keyuseEffectonly attaches/detaches based on!disableClosingModal.isContentDisabledis not consulted.Modal.tsx:135-144:handleBackgroundMouseUp(backdrop click) gates on!disableClosingModal(plusisFormDirtyRef). For DeleteCategoryModal there is no form input soisFormDirtyRefstays false and the backdrop closes freely during the DELETE request.Modal.tsx:185-196: the X-icon<Button>is rendered only when!disableClosingModal—isContentDisableddoesn't hide or disable it.Modal.tsx:191:autofocus={isContentDisabled}literally moves focus to the close X whenisContentDisabledistrue. So during the request, the active element is the very button that's supposed to be blocked.Modal.tsx:198-202: the__disabled-overlaydiv sits inside the content-wrapper, beneath the header — it covers the form area, not the X icon.
The race
For AddCategoryModal (AddCategoryModal.tsx:38-48), EditCategoryModal (analogous), and DeleteCategoryModal (DeleteCategoryModal.tsx:25-35):
- User clicks Add/Save/Delete →
setIsSubmitting(true)/setIsDeleting(true),awaitAPI call begins. - While in flight, user hits Escape / clicks X / clicks backdrop. Per Modal.tsx the close paths are not gated.
onExitfires → parentsetShowAddModal(false)/setCategoryToEdit(null)/setCategoryToDelete(null)→ modal unmounts.- The pending Promise still holds the closure including the
onSuccessprop. React unmounting the child does NOT abort the fetch (no AbortController). - API resolves successfully →
onSuccess()fires → parent'sonAddSuccess/onEditSuccess/onDeleteSuccessinSelfServiceCategoriesPage.tsx:97-113runsinvalidateList()andrenderFlash("success", "Successfully added/updated/deleted self-service category.").
Step-by-step proof (Delete flow — the most user-confusing case)
- User opens DeleteCategoryModal for category id 1 named "🛟 Support" and clicks Delete.
onDelete(DeleteCategoryModal.tsx:25-35):setIsDeleting(true),await selfServiceCategoriesAPI.deleteCategory(1)starts.- Per
Modal.tsx:191focus moves to the X button. User reflexively hits Enter →handleCloseruns (Modal.tsx:64-71) →onExitfires after the 100ms close animation. - Parent
setCategoryToDelete(null)→ modal unmounts. - DELETE resolves with 204. Async resumes inside the unmounted component's closure, calls
onSuccess()→ still pointing at the parent'sonDeleteSuccess(SelfServiceCategoriesPage.tsx:109-113). - Parent runs
invalidateList(),setCategoryToDelete(null)(no-op), andrenderFlash("success", "Successfully deleted self-service category."). - User just "canceled" a destructive action, sees a success toast saying it was deleted, and the row disappears from the list.
The Escape and backdrop variants reproduce identically because Modal.tsx:80 and Modal.tsx:137 both only consult disableClosingModal. The backdrop variant is especially easy to hit on DeleteCategoryModal because there is no form input so isFormDirtyRef.current stays false, removing the only soft guard.
Impact
Nit — the data state on the server is consistent (the operation really did succeed), the window is one network round-trip, no data loss, no security issue. But the user-visible outcome contradicts the user's intent and contradicts the explicit fix that was just applied. The destructive Delete case is the most jarring.
Fix
One line per modal — switch the prop from isContentDisabled to disableClosingModal (or set both):
// AddCategoryModal.tsx:62-67, EditCategoryModal.tsx:63-68
<Modal
title="Add category" // or "Edit category"
onExit={onExit}
className={baseClass}
disableClosingModal={isSubmitting}
>
// DeleteCategoryModal.tsx:39-44
<Modal
title="Delete category"
onExit={onExit}
className={baseClass}
disableClosingModal={isDeleting}
>disableClosingModal is the prop that actually gates the X icon, Escape key, and backdrop click (Modal.tsx:80, 137, 185). If you want the dimmed-content visual treatment AND the gated close paths, pass both. (An AbortController on the API call would also work as a no-op-on-resolve mitigation, but a one-character prop change is the lighter fix and matches the same convention already in use elsewhere in this app.)
There was a problem hiding this comment.
This is not a pattern in our API calling modals so it will not be addressed here.
9680221
into
feat/39018-self-service-categories
|
@eashaw - as long as I get your approval before the feat is ready to merge 🙏 thanks! |
Issue
Part of #39018
Closes #46370
Description
Screenrecording
Screen.Recording.2026-06-01.at.1.49.07.PM.mov
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing