diff --git a/src/features/store-admin/StoreOpenControl.tsx b/src/features/store-admin/StoreOpenControl.tsx index 2d919ea..42a6d07 100644 --- a/src/features/store-admin/StoreOpenControl.tsx +++ b/src/features/store-admin/StoreOpenControl.tsx @@ -1,4 +1,4 @@ -import { useEffect, useState } from 'react'; +import { useEffect, useRef, useState } from 'react'; import { useMutation, useQueryClient } from '@tanstack/react-query'; import { Switch } from '@astryxdesign/core/Switch'; import { Button } from '@astryxdesign/core/Button'; @@ -56,17 +56,35 @@ export function StoreOpenControl({ const [until, setUntil] = useState(''); const [error, setError] = useState(null); - // Re-seed if the page refetches with a different answer, but never while the - // confirmation is up — that would move the switch under the shopkeeper's hand. + /* + Re-seed only when the PROP ITSELF changes. + + This used to run whenever `confirming` flipped, and that undid its own + save: closing the branch set open to false and confirming to false, the + change of `confirming` re-ran the effect, and it wrote the prop — still + true, because the page behind had not refetched yet — straight back over + the answer. The switch snapped open the instant the shop was closed. + + Comparing against the last value seen means a genuinely new answer from the + server still lands, and nothing else can reach in and overwrite what the + shopkeeper just did. + */ + const lastSeen = useRef(isOpen); useEffect(() => { - if (!confirming) setOpen(isOpen); - }, [isOpen, confirming]); + if (lastSeen.current !== isOpen) { + lastSeen.current = isOpen; + setOpen(isOpen); + } + }, [isOpen]); const save = useMutation({ mutationFn: (next: { isopen: boolean; closeduntil?: string }) => tenantsApi.setStoreOpen({ tenantid, locationid, ...next }), onMutate: () => setError(null), onSuccess: (_data, next) => { + // Move the reference too. Otherwise the next render carrying the old + // prop reads as a change and reverts what was just saved. + lastSeen.current = next.isopen; setOpen(next.isopen); setConfirming(false); setUntil(''); @@ -75,6 +93,7 @@ export function StoreOpenControl({ // A close that fails silently is the worst outcome here: the shopkeeper // walks away believing the shop is shut while it keeps taking orders. onError: (cause) => { + lastSeen.current = isOpen; setOpen(isOpen); setError(errorMessage(cause)); }, diff --git a/src/features/store-admin/storeOpenControl.test.ts b/src/features/store-admin/storeOpenControl.test.ts new file mode 100644 index 0000000..6b7de8f --- /dev/null +++ b/src/features/store-admin/storeOpenControl.test.ts @@ -0,0 +1,82 @@ +/** + * The switch must not undo its own save. + * + * It shipped doing exactly that. Closing a branch set the local flag false and + * dismissed the confirmation, the change of `confirming` re-ran the seeding + * effect, and the effect wrote the prop — still `true`, because the page behind + * had not refetched — back over the answer. The shop closed and the switch + * snapped open, with nothing on screen to say the save had worked. + * + * A source-text guard rather than a render test: the defect was entirely in + * which values the effect watched, which is readable without a DOM, and this + * fails the moment somebody puts `confirming` back in the list. + */ + +import { strict as assert } from 'node:assert'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import { describe, it } from 'node:test'; + +const SOURCE = readFileSync( + join(process.cwd(), 'src/features/store-admin/StoreOpenControl.tsx'), + 'utf8', +); + +/** The dependency list of each useEffect in the file. */ +function effectDeps(source: string): string[] { + return [...source.matchAll(/useEffect\([\s\S]*?\}, \[([^\]]*)\]\)/g)].map((m) => + (m[1] ?? '').trim(), + ); +} + +describe('StoreOpenControl seeding effect', () => { + it('does not re-run when the confirmation dialog opens or closes', () => { + for (const deps of effectDeps(SOURCE)) { + assert.ok( + !/\bconfirming\b/.test(deps), + `an effect watches "confirming" (deps: [${deps}]). Dismissing the ` + + 'confirmation then re-seeds from the prop, which still holds the old ' + + 'value, and the save is undone on screen.', + ); + } + }); + + it('seeds from the prop only when the prop itself has changed', () => { + // A bare `setOpen(isOpen)` in an effect overwrites local state on every + // render that touches the dependencies. The guard is the comparison. + assert.match( + SOURCE, + /lastSeen\.current !== isOpen/, + 'the seeding effect no longer compares against the last value seen, so ' + + 'it can overwrite a save that has not reached the prop yet', + ); + }); + + it('moves the reference when a save succeeds', () => { + // Without this the next render carrying the old prop reads as a change, + // and reverts what was just written. + assert.match( + SOURCE, + /onSuccess:[\s\S]{0,400}lastSeen\.current = next\.isopen/, + 'onSuccess does not update lastSeen, so a stale prop will look like a ' + + 'new answer and revert the switch', + ); + }); + + it('surfaces a failed save', () => { + // A close that fails silently is the worst outcome: the shopkeeper walks + // away believing the shop is shut while it keeps taking orders. + assert.match(SOURCE, /onError:/, 'the mutation has no onError'); + assert.match(SOURCE, /setError\(errorMessage\(cause\)\)/, 'the error is never shown'); + }); + + it('asks before closing, and does not ask before reopening', () => { + // Closing stops the shop taking money; reopening is harmless. + assert.match( + SOURCE, + /save\.mutate\(\{ isopen: true \}\)/, + 'reopening should happen on the tap, not behind a confirmation', + ); + assert.match(SOURCE, /setConfirming\(true\)/, 'closing should ask first'); + }); +});