From 62fbd57ce69989fe0e4f86c3be92c9d4fbeff05c Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 2 Aug 2026 15:39:43 -0600 Subject: [PATCH] review: the spread was overwriting the stack, and the guard was blind Acting on the review of #622. Two bugs, both real, and the second one is a test of mine that could not fail for its stated reason. The stack props were set BEFORE `{...props}`, so any call site passing `closeOnEscape` overwrote the stack's decision and re-armed Mantine's window-level Escape listener on a modal that is not on top -- reinstating the bug the change exists to fix. Three call sites pass it: CollectionReorganizeModal and BatchReorganizeModal disable Escape while a reorganize runs, Flasher while it writes an image. The concrete breakage is the one place the app really does stack two modals: ConflictResolutionModal opens as a sibling above the reorganize modal, so the moment the reorganize finished and `closeOnEscape` returned to true, one Escape closed both. They are now destructured out and composed -- `isTop && closeOnEscape` -- because "only the top modal answers Escape" and "do not dismiss me while I am busy" are both true. Mantine's own Modal.Stack applies its stack props after the caller's for the same reason. Two Cypress cases, one per direction; planting the old order reddens the first. `Confirm` was still passing Mantine's raw `sm`, and the audit guard I added in the same PR was supposed to catch exactly that. It did not, and the reason is worse than the miss: the scanner matched `]*?\bsize='...'`, and `onClose={() => ...}` contains a `>`, so it stopped at the arrow and never saw the rest of the tag. It read 44 of the 56 modals that declare a size; the twelve it skipped included the single offender in the codebase, in this very file. A guard that scans a fifth of nothing looks identical to a clean codebase. It now walks the tag with a paren counter, accepts either quote style, and asserts it scanned more than fifty tags first -- so a scanner that silently matches nothing fails instead of passing. Confirm says `tiny`. Also from the review: a bare `expect(titles()).to.not.include('Parent')` in the Cypress spec ran during the synchronous phase, before any cy command executed and while nothing was open, so it always passed and guarded nothing. Deleted rather than repaired -- the retried assertion above it already makes the claim. An unregistered-but-open modal reported `isTop` even with a parent registered, which gave both the same z-index for the frame before the effect ran -- a visible flash of the wrong modal in front. It is only top now if the stack is empty. Nit: the comment claimed Mantine ignores unknown size names. It ignores unknown SEMANTIC names and falls back to 440px; it recognises and honours its own. Two different hazards, both silent, and the comment now says which is which. 1124 jest / 62 suites, 10 in modal-stack.cy.js, 196 in ui-layout.cy.js, clean npm run build. --- app/src/components/ui/Overlays.tsx | 56 ++++++++++++++++------ app/src/components/ui/modal-stack.cy.js | 62 +++++++++++++++++++++++-- app/src/components/ui/ui.test.js | 48 +++++++++++++++---- 3 files changed, 141 insertions(+), 25 deletions(-) diff --git a/app/src/components/ui/Overlays.tsx b/app/src/components/ui/Overlays.tsx index b72776ea..a7c4ee45 100644 --- a/app/src/components/ui/Overlays.tsx +++ b/app/src/components/ui/Overlays.tsx @@ -107,7 +107,13 @@ function useModalStackPosition(opened: boolean): {isTop: boolean; zIndex: number const depth = openModalIds.indexOf(id); return { - isTop: depth === -1 || depth === openModalIds.length - 1, + /* + * Not yet registered -- the first paint of a modal that just opened, before the + * effect runs. It is only on top if nothing else is open; claiming top while a + * parent is registered gives both modals the same z-index for a frame, which is a + * visible flash of the wrong one in front when a child opens. + */ + isTop: depth === -1 ? openModalIds.length === 0 : depth === openModalIds.length - 1, /* * Two per level, so a child's overlay clears its parent's content. Kept small on * purpose: Mantine puts popovers and select dropdowns at 300, and a modal stack that @@ -117,7 +123,27 @@ function useModalStackPosition(opened: boolean): {isTop: boolean; zIndex: number }; } -function ModalBase({open, opened, size, closeIcon, title, children, ...props}: ModalProps) { +function ModalBase({ + open, opened, size, closeIcon, title, children, + /* + * Pulled out of `props` so the spread below cannot overwrite the stack's decision. + * + * These were set BEFORE `{...props}` at first, which meant a call site passing + * `closeOnEscape` won and re-armed Mantine's window-level Escape listener on a modal + * that is not on top -- reinstating the exact bug this exists to fix. Three call sites + * pass it: CollectionReorganizeModal and BatchReorganizeModal disable Escape while a + * reorganize is running, and Flasher while it is writing an image. Conflict resolution + * opens as a sibling ON TOP of the reorganize modal, so once the reorganize finished and + * `closeOnEscape` went back to true, one Escape closed both again. + * + * They are composed rather than ignored: "only the top modal answers Escape" and "this + * modal must not be dismissed while it is busy" are both true, and the answer is AND. + */ + closeOnEscape = true, + trapFocus = true, + zIndex, + ...props +}: ModalProps) { const slots: Record = {header: [], actions: [], body: []}; React.Children.forEach(children, child => { const type = React.isValidElement(child) ? child.type : undefined; @@ -127,7 +153,7 @@ function ModalBase({open, opened, size, closeIcon, title, children, ...props}: M }); const isOpen = opened ?? open ?? false; - const {isTop, zIndex} = useModalStackPosition(isOpen); + const {isTop, zIndex: stackZIndex} = useModalStackPosition(isOpen); return {slots.body} {slots.actions.length > 0 &&
{slots.actions}
} @@ -185,7 +213,9 @@ export function Confirm({ onConfirm, onCancel, }: ConfirmProps) { - return onCancel?.()} title={title} size='sm'> + // `tiny`, not Mantine's raw `sm`. Same 380px, but the audited vocabulary -- and the + // guard in ui.test.js exists to keep raw Mantine names out of call sites. + return onCancel?.()} title={title} size='tiny'> {children}
diff --git a/app/src/components/ui/modal-stack.cy.js b/app/src/components/ui/modal-stack.cy.js index 911bb899..46592ca0 100644 --- a/app/src/components/ui/modal-stack.cy.js +++ b/app/src/components/ui/modal-stack.cy.js @@ -50,8 +50,6 @@ const Nested = ({onParentClose, onChildClose}) => { }; -const titles = () => [...Cypress.$('.mantine-Modal-title')].map(el => el.textContent); - const openBoth = () => { cy.mountUI(); cy.get('[data-testid="open-parent"]').click(); @@ -194,6 +192,64 @@ describe('a single modal', () => { pressEscape(); cy.contains('.mantine-Modal-title', 'Parent').should('not.exist'); - expect(titles()).to.not.include('Parent'); + }); +}); + +describe('a parent that disables Escape while it is busy', () => { + /* + * Three call sites pass `closeOnEscape` of their own: CollectionReorganizeModal and + * BatchReorganizeModal disable it while a reorganize runs, Flasher while it writes an + * image. The stack props were set BEFORE the `{...props}` spread, so those call sites + * overwrote the stack's decision and re-armed Mantine's window-level listener on a modal + * that is not on top. + * + * The concrete breakage: ConflictResolutionModal opens as a sibling above the reorganize + * modal, so as soon as the reorganize finished and `closeOnEscape` went back to true, one + * Escape closed both again -- the original bug, in the one place the app really does + * stack two modals. + * + * "Only the top modal answers Escape" and "do not dismiss me while I am busy" are both + * true; they compose with AND. + */ + const Busy = ({busy}) => { + const [parent, setParent] = React.useState(true); + const [child, setChild] = React.useState(true); + return <> + setParent(false)}> + Reorganize +

working

 + + setChild(false)}> + Conflicts +

resolve these

 + + + }; + + it('still closes only the child when the parent has re-enabled Escape', () => { + cy.mountUI(); + cy.contains('.mantine-Modal-title', 'Conflicts').should('exist'); + + pressEscape(); + + cy.contains('.mantine-Modal-title', 'Conflicts').should('not.exist'); + cy.contains('.mantine-Modal-title', 'Reorganize').should('exist'); + }); + + it('honours the parent\'s own refusal once it is on top again', () => { + /* + * The other direction, and the reason the two are composed rather than the stack + * simply winning: a busy parent must stay put even when nothing is above it. + */ + cy.mountUI(); + cy.contains('.mantine-Modal-title', 'Conflicts').should('exist'); + + pressEscape(); + cy.contains('.mantine-Modal-title', 'Conflicts').should('not.exist'); + pressEscape(); + + cy.contains('.mantine-Modal-title', 'Reorganize') + .should('exist'); }); }); diff --git a/app/src/components/ui/ui.test.js b/app/src/components/ui/ui.test.js index 192fb2e1..4b5e2961 100644 --- a/app/src/components/ui/ui.test.js +++ b/app/src/components/ui/ui.test.js @@ -1530,13 +1530,34 @@ describe('modal sizes', () => { it('never lets a call site name a size the table does not know', () => { /* - * A name the table has never heard of is passed straight to Mantine, which ignores - * anything it does not recognise and renders the default -- so a typo is a modal that - * silently reverts to 440px with no warning anywhere. Flasher was passing Mantine's - * own `sm` directly, which worked but bypassed the naming the audit used. + * Two different hazards, and neither announces itself. + * + * A misspelled SEMANTIC name is not in the table, so it passes through untranslated, + * Mantine does not recognise it either, and the modal silently renders at the default + * 440px. A raw MANTINE name -- `sm`, `md` -- is recognised and honoured: it works, + * so nothing ever complains, and the call site quietly sits outside the vocabulary + * the widths were audited in and outside the table that can retune them. Flasher and + * Confirm were both doing the second. + * + * The tag is scanned with a paren counter rather than matched with `[^>]*?`, which is + * what the first version of this did. `onClose={() => ...}` contains a `>`, so that + * pattern stopped at the arrow and skipped the rest of the tag: it saw 44 of the 56 + * modals that declare a size, and the twelve it missed included Confirm's `size='sm'` + * in this very library -- the one offender it was written to catch. */ const known = new Set(['mini', 'tiny', 'small', 'large', 'fullscreen']); - const offenders = []; + + /** The opening tag starting at `start`, honouring braces in prop expressions. */ + const openingTag = (source, start) => { + let depth = 0; + for (let i = start; i < source.length; i++) { + const character = source[i]; + if (character === '{' || character === '(') depth += 1; + else if (character === '}' || character === ')') depth -= 1; + else if (character === '>' && depth === 0) return source.slice(start, i + 1); + } + return ''; + }; const walk = (directory) => fs.readdirSync(directory, {withFileTypes: true}) .flatMap(entry => { @@ -1547,15 +1568,24 @@ describe('modal sizes', () => { return [[full, fs.readFileSync(full, 'utf8')]]; }); + const offenders = []; + let scanned = 0; for (const [file, source] of walk(path.join(__dirname, '..', '..'))) { - // Only ``; a computed `size={...}` is the caller's business. - for (const match of source.matchAll(/]*?\bsize='([^']+)'/g)) { - if (!known.has(match[1])) { - offenders.push(`${path.basename(file)}: size='${match[1]}'`); + for (const match of source.matchAll(/