rt/wrolpi
1
0
Fork 0
mirror of https://github.com/lrnselfreliance/wrolpi.git synced 2026-10-26 19:01:39 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

review: the spread was overwriting the stack, and the guard was blind

Browse source 
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 `<Modal\b[^>]*?\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.
This commit is contained in:
Roland 2026-08-02 15:39:43 -06:00
parent afa127a992
commit ad78cdee7c
No known key found for this signature in database
GPG key ID: 1BA620CE6C9A4680
3 changed files with 141 additions and 25 deletions
Download patch file Download diff file Expand all files Collapse all files

56
app/src/components/ui/Overlays.tsx
View file

@ -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<string, React.ReactNode[]> = {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 <MModal
opened={isOpen}
@ -136,16 +162,18 @@ function ModalBase({open, opened, size, closeIcon, title, children, ...props}: M
title={slots.header.length ? slots.header : title}
// Flat surfaces: the overlay separates the modal from the page, not a shadow.
overlayProps={{backgroundOpacity: 0.55, blur: 0}}
/*
* Only the modal on top answers the keyboard. Without this every open modal has its
* own window-level Escape listener and one press closes the lot. Focus goes with it:
* two traps fighting over the same Tab is its own bug, and the user is working in the
* top one.
*/
closeOnEscape={isTop}
trapFocus={isTop}
zIndex={zIndex}
{...props}
/*
* AFTER the spread, deliberately -- see the destructuring above. Only the modal on
* top answers the keyboard: without this every open modal has its own window-level
* Escape listener and one press closes the lot. Focus goes with it, because two
* traps fighting over the same Tab is its own bug and the user is working in the top
* one. Mantine's own Modal.Stack applies its stack props after the caller's for the
* same reason.
*/
closeOnEscape={isTop && closeOnEscape}
trapFocus={isTop && trapFocus}
zIndex={zIndex ?? stackZIndex}
>
{slots.body}
{slots.actions.length > 0 && <div className='wrolpi-modal-actions'>{slots.actions}</div>}
@ -185,7 +213,9 @@ export function Confirm({
onConfirm,
onCancel,
}: ConfirmProps) {
return <Modal opened={open} onClose={() => 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 <Modal opened={open} onClose={() => onCancel?.()} title={title} size='tiny'>
{children}
<div style={{display: 'flex', justifyContent: 'flex-end', gap: 10, marginTop: 18}}>
<Button role='cancel' onClick={onCancel}>{cancelLabel}</Button>

62
app/src/components/ui/modal-stack.cy.js
View file

@ -50,8 +50,6 @@ const Nested = ({onParentClose, onChildClose}) => {
</>
};
const titles = () => [...Cypress.$('.mantine-Modal-title')].map(el => el.textContent);
const openBoth = () => {
cy.mountUI(<Nested/>);
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 <>
<Modal open={parent} size='large' closeOnEscape={!busy}
onClose={() => setParent(false)}>
<Modal.Header>Reorganize</Modal.Header>
<Modal.Content><p>working</p></Modal.Content>
</Modal>
<Modal open={child} size='small' onClose={() => setChild(false)}>
<Modal.Header>Conflicts</Modal.Header>
<Modal.Content><p>resolve these</p></Modal.Content>
</Modal>
</>
};
it('still closes only the child when the parent has re-enabled Escape', () => {
cy.mountUI(<Busy busy={false}/>);
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(<Busy busy/>);
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');
});
});

48
app/src/components/ui/ui.test.js
View file

@ -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 `<Modal ... size='x'>`; a computed `size={...}` is the caller's business.
for (const match of source.matchAll(/<Modal\b[^>]*?\bsize='([^']+)'/g)) {
if (!known.has(match[1])) {
offenders.push(`${path.basename(file)}: size='${match[1]}'`);
for (const match of source.matchAll(/<Modal\b/g)) {
const tag = openingTag(source, match.index);
// Quote-agnostic; a computed `size={...}` is the caller's business.
const declared = tag.match(/\bsize=["']([^"']+)["']/);
if (!declared) continue;
scanned += 1;
if (!known.has(declared[1])) {
offenders.push(`${path.basename(file)}: size='${declared[1]}'`);
}
}
}
// The premise. A scanner that silently matched nothing would report no offenders
// and read exactly like a clean codebase, which is how the first version passed.
expect(scanned).toBeGreaterThan(50);
expect(offenders).toEqual([]);
});
});