refactor(common-adapters): give each popup mode its own module - #29609
Draft
chrisnojima wants to merge 1 commit into
Draft
refactor(common-adapters): give each popup mode its own module#29609chrisnojima wants to merge 1 commit into
chrisnojima wants to merge 1 commit into
Conversation
Popup was one 255-line dispatcher behind a 16-field interface that
branched on runtime state into four unrelated presentations, each
reading a different subset of the props and silently ignoring the rest.
The platform rule was encoded independently in three places.
Split the presentations into three modules with narrow interfaces -
AnchoredPopup, Sheet, ModalCover - and keep Popup as a policy module
that picks one from an explicit intent ('menu' or 'dialog'). The
platform default now lives only there, so usePopup2 no longer gates the
anchor ref and min-writer-role no longer gates its ref either. The type
system rejects placement props on a mode that ignores them, and
hideKeyboard now exists only on the mode that implements it.
Popup no longer takes visible: every caller passed a literal true.
FloatingMenu was the exception - it let mode='modal' past its own
visibility guard and leaned on Popup to drop the hidden menu on the way
past, so that guard is now unconditional.
Sheet and ModalCover get file suffixes, so @gorhom/bottom-sheet and
react-native-screens leave the desktop bundle and the raw div leaves the
native one. ModalCover, previously untestable inside the dispatcher,
gets tests for its press and escape handling.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rccpV5nLxxC5opF5xzrz7
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
1 participant
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.
Problem
PopupPropshad 16 fields, andpopup/index.tsxdispatched on runtime state into four presentation modes. Most props were dead in any given mode:PopupCenteredPopupSheetDesktopPopupPositionedThe mode was selected by which combination you passed, so a caller had to already know the platform to fill the interface in correctly. 81 files render
Popup/FloatingMenu; 26 branch on platform within ±15 lines of it.reaction-tooltip.tsxrenders twoKb.Popupelements in one component sharing only two props.The platform rule was encoded three times independently. Changing one default inside
Popuppreviously required editing 13 files to opt out.Change
Three real modules with narrow interfaces —
AnchoredPopup,Sheet,ModalCover— plusPopupkept as a small policy module picking a mode from intent, so the platform default lives in exactly one place and the 40+ menu one-liners are unchanged.The intent set is
'menu' | 'dialog', derived by classifying all 22 real call sites, not invented. Sites whose behaviour never depended on platform now take a mode directly.popup/index.tsx: 255 → 45 lines. The file-suffix seam is restored, so@gorhom/bottom-sheetandreact-native-screenslive only insheet.native.tsx, and the raw<div>only inmodal-cover.desktop.tsx— which also resolves a no-DOM-in-plain-.tsxviolation.Removed as provably dead
visible(see below),mobileAnchored(2 sites →AnchoredPopup), andhideKeyboard's latent trap — it now exists only on the one mode that implements it.usePopup2'sisMobile ? undefined : popupAnchorandmin-writer-role'sref={isMobile ? null : popupAnchor}are both gone.One regression caught in review
Removing
visiblewas justified as "every caller passestrue, andFloatingMenureturns null first". The second half was wrong: the guard isif (!visible && mode !== 'modal'). Formode="modal"withvisible={false},FloatingMenufell through andPopupcaught it. Deleting that catch while leaving the guard meant that combination would render.Not reachable today — the only
mode="modal"site passes a literaltrue— but fixed by collapsing the guard toif (!visible) return null, verified equivalent (the modal branch has no exit animation to wait for). Pinned by atest.eachover all three modes that fails on exactly the modal case.Validation
lint:allclean —0 bailed out, 0 whole-props deps, tsc clean both projects.jest --runInBand— 232 suites / 2247 tests (baseline 230 / 2236).ModalCovergets 8 tests andFloatingMenu3 — modes that could not be tested at all while they were branches inside a dispatcher importingFullWindowOverlayat module scope.