refactor(console): merge Sheet into Dialog, adopt Web Animations API for exit, sentinel focus guards#311
Open
refactor(console): merge Sheet into Dialog, adopt Web Animations API for exit, sentinel focus guards#311
Conversation
…for exit, sentinel focus guards - Replace useExitAnimation (CSS event listeners) with useAnimationsFinished (Web Animations API getAnimations() / Promise.all(a.finished)) in Dialog and Toast; cancelled animations still trigger unmount via the rejection path. - Remove the focusout / requestAnimationFrame pull-back from useFocusTrap; add FocusGuard sentinel components rendered at the boundaries of DialogContent so Tab/Shift+Tab at the edge redirects focus back into the panel. - Merge Sheet into Dialog: Dialog gains isDrawer? + side? props; DialogContent renders a centered modal or a side-sliding <section> drawer depending on context. Add Drawer / DrawerContent / DrawerHeader / DrawerTitle / DrawerDescription / DrawerClose re-export aliases. - Update Sidebar to use Dialog (isDrawer) instead of Sheet; delete the entire sheet/ component directory.
…etAnimations polyfill
- New: useAnimationsFinished.test.ts — covers exiting=false, no-anim, multi-anim, rejection, and unmount-before-finish cases
- New: FocusGuard.test.tsx — verifies aria-hidden, tabIndex=0, and onFocus callback
- dialog.test.tsx — polyfill HTMLElement.getAnimations for jsdom; add describe('Dialog — drawer mode') covering section/role, data-state, data-side, Escape, overlay click, aria-labelledby, and no-unmount-on-close
- useFocusTrap.test.tsx — replace stale focusout-isConnected test with a FocusGuard sentinel focus-redirect test
- sidebar.test.tsx, toast.test.tsx, gateway.test.tsx — add getAnimations polyfill so pre-existing tests pass after Web Animations API was introduced
Remove unused `cleanup` and `afterEach` imports (TS6133) and the orphaned `afterEach(cleanup)` call they powered.
…dialog utilities - Add console/src/test-setup.ts with a single getAnimations stub and register it via setupFiles in vitest.config.ts; remove the copy-pasted polyfill block from dialog, sidebar, toast, gateway and useAnimationsFinished test files. - Export FOCUSABLE_SELECTORS from hooks/useFocusTrap.ts and import it in components/dialog/index.tsx, removing the duplicate local definition. - Guard the open->closed exiting effect in DialogContent with !isDrawer so drawers never enter the JS-unmount path; remove the now-redundant cleanup effect that immediately cleared exiting for drawers. - Lift focusFirst/focusLast in DialogContent into stable useCallback hooks so FocusGuard onFocus handlers are not recreated on every render.
Member
Author
Code reviewFound 2 issues:
hybridclaw/console/src/components/sidebar/index.tsx Lines 164 to 171 in d5661cd
hybridclaw/console/src/components/dialog/index.tsx Lines 378 to 394 in d5661cd 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Sidebar's DialogHeader was rendering "Navigation" / "Sidebar navigation panel." visibly after the Sheet→Dialog migration, regressing the old SheetHeader's automatic srOnly behavior. Add a visuallyHidden prop to DialogHeader and use it in the mobile sidebar. Also drop the Drawer/DrawerClose/DrawerContent/DrawerDescription/ DrawerHeader/DrawerTitle re-exports, which had no callers (AGENTS.md §3.2 YAGNI). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # console/src/components/dialog/index.tsx # console/src/components/sheet/index.tsx
…b stale Sheet refs Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Resolve sidebar conflict: keep the Dialog-based mobile drawer (this PR) while adopting the PanelLeft icon and collapsible='none' variant from main. Drop SheetSide since the sheet module is removed by this refactor.
5a8c44e to
582c999
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
useAnimationsFinishedreplacesuseExitAnimation: usesel.getAnimations()+Promise.all(a.finished)so multiple concurrent animations are all awaited; cancelled animations (which reject) still trigger unmount. Falls back to immediate call when no animations are running (jsdom,prefers-reduced-motion).FocusGuardsentinels replace thefocusout/requestAnimationFramepull-back inuseFocusTrap: two invisible<span tabIndex={0}>elements are rendered before and afterDialogContent's children; when Tab/Shift+Tab exits the container the sentinel firesonFocusand redirects focus back in. The hook retains initial focus setup and return-focus-on-close.Sheetmerged intoDialog:DialoggainsisDrawer?: boolean;DialogContentgainsside?: 'left' | 'right' | 'top' | 'bottom'. WhenisDraweris true,DialogContentrenders a<section>with slide-in/out via CSS transform class toggle (matching the old Sheet behaviour). Drawer CSS rules added todialog/index.module.css. Re-export aliases (Drawer,DrawerContent, etc.) added at the bottom ofdialog/index.tsx.Sidebarupdated to useDialog+DialogContentwithisDrawer/sideinstead ofSheet/SheetContent.sheet/directory deleted.Toastupdated to useuseAnimationsFinished(it also depended on the deleteduseExitAnimation).Test plan
npx tsc --noEmitpasses (verified — zero errors)