revise(02-05): fix migrate() bypass in boot+import paths + lifecycle leak + hotkey
- BLOCKER 1: PhaserGame.tsx boot path now runs unwrap(env) → migrate(raw, env.schemaVersion). Casting unwrap(record.envelope) directly to V1Payload silently accepted any future-shape payload as the current shape; only migrate() walks the schema version chain. - BLOCKER 2: Settings.tsx onImport now correctly orders importFromBase64 → unwrap (CRC verify) → migrate. Previous code discarded migrate's result and then read v1.payload as if unwrap returned an envelope rather than the payload itself — runtime crash on every import. - BLOCKER 3: documented the lastTickAt invariant as wall-clock milliseconds, written ONLY at saveSync time (never by the sim). Added acceptance_criteria greps proving (a) saveSync writes clock.now(), (b) Garden scene does not overwrite lastTickAt with a tick counter, (c) sim/garden/Garden.ts (if it exists; the Garden scene actually lives at src/game/scenes/Garden.ts) contains no lastTickAt: this.* writes. - W2: D-29 keyboard shortcut wired in App.tsx — comma toggles Settings, 'j' dispatches a window CustomEvent the JournalIcon picks up. - W5: lifecycle handle now stored in useRef and detached in the OUTER useLayoutEffect cleanup (the previous IIFE-internal return was a closure return, never reaching React's effect cleanup contract).
This commit is contained in:
+75
-12
@@ -733,12 +733,14 @@ export function Settings({ open, onClose }: { open: boolean; onClose: () => void
|
||||
|
||||
const onImport = async () => {
|
||||
try {
|
||||
// BLOCKER 2 fix — pipeline order: importFromBase64 → unwrap (CRC verify)
|
||||
// → migrate. unwrap() returns the payload directly (NOT { payload }), so
|
||||
// the previous v1.payload read crashed at runtime; migrate() was also
|
||||
// called against env.payload before checksum verification. Correct order:
|
||||
const env = await importFromBase64(base64Buf);
|
||||
const { payload } = migrate(env.payload, env.schemaVersion);
|
||||
const v1 = unwrap(env);
|
||||
const restored = v1.payload as V1Payload; // post-migrate
|
||||
// Apply to store
|
||||
hydrateStoreFromPayload(restored);
|
||||
const raw = unwrap(env); // verifies CRC
|
||||
const { payload } = migrate(raw, env.schemaVersion); // upgrades schema
|
||||
hydrateStoreFromPayload(payload as V1Payload);
|
||||
setStatusLine('Restored.');
|
||||
} catch (e) {
|
||||
setStatusLine('That doesn\'t look like one of yours.');
|
||||
@@ -932,6 +934,10 @@ interface IProps {
|
||||
export const PhaserGame = forwardRef<IRefPhaserGame, IProps>(function PhaserGame(props, ref) {
|
||||
const game = useRef<Phaser.Game | null>(null);
|
||||
const sceneRef = useRef<Phaser.Scene | null>(null);
|
||||
// W5 — hold the lifecycle handle so the OUTER useLayoutEffect cleanup can
|
||||
// call detach(). The async IIFE that registers the hooks cannot return its
|
||||
// own cleanup to the effect.
|
||||
const lifecycleRef = useRef<{ detach: () => void } | null>(null);
|
||||
const [persistenceToastShow, setPersistenceToastShow] = useState(false);
|
||||
|
||||
// Select clock: dev-time URL flag + non-prod build → FakeClock; otherwise wallClock
|
||||
@@ -958,7 +964,14 @@ export const PhaserGame = forwardRef<IRefPhaserGame, IProps>(function PhaserGame
|
||||
if (cancelled) return;
|
||||
if (record) {
|
||||
// Returning player path
|
||||
const payload = unwrap(record.envelope) as V1Payload;
|
||||
// BLOCKER 1 fix — boot path must run unwrap (CRC verify) THEN migrate.
|
||||
// Casting unwrap(record.envelope) directly to V1Payload silently
|
||||
// accepts any future-shape payload as the current shape; migrate()
|
||||
// is the only place that walks the schema version chain.
|
||||
const env = record.envelope;
|
||||
const raw = unwrap(env);
|
||||
const { payload: migratedPayload } = migrate(raw, env.schemaVersion);
|
||||
const payload = migratedPayload as V1Payload;
|
||||
appStore.getState().dismissBeginGate(); // D-22: skip Begin
|
||||
hydrateStoreFromPayload(payload);
|
||||
|
||||
@@ -1002,8 +1015,14 @@ export const PhaserGame = forwardRef<IRefPhaserGame, IProps>(function PhaserGame
|
||||
if (typeof ref === 'function') ref({ game: game.current, scene: null });
|
||||
else if (ref) ref.current = { game: game.current, scene: null };
|
||||
|
||||
// Register save lifecycle hooks (UX-10)
|
||||
const lifecycle = registerSaveLifecycleHooks({
|
||||
// Register save lifecycle hooks (UX-10).
|
||||
// W5 fix — lifecycle.detach() must reach the useLayoutEffect cleanup,
|
||||
// but the registration happens inside an async IIFE so the inner
|
||||
// `return () => lifecycle.detach()` is the IIFE's return value, not
|
||||
// the effect's cleanup. Hold the handle in a useRef declared at the
|
||||
// top of the component, so the OUTER cleanup (already returned below)
|
||||
// can call detach on the ref's current value.
|
||||
lifecycleRef.current = registerSaveLifecycleHooks({
|
||||
saveSync: () => {
|
||||
try {
|
||||
const state = appStore.getState();
|
||||
@@ -1018,10 +1037,12 @@ export const PhaserGame = forwardRef<IRefPhaserGame, IProps>(function PhaserGame
|
||||
}
|
||||
},
|
||||
});
|
||||
// Cleanup on unmount
|
||||
return () => lifecycle.detach();
|
||||
})();
|
||||
return () => { cancelled = true; };
|
||||
return () => {
|
||||
cancelled = true;
|
||||
lifecycleRef.current?.detach();
|
||||
lifecycleRef.current = null;
|
||||
};
|
||||
}, [ref]);
|
||||
|
||||
useEffect(() => {
|
||||
@@ -1054,7 +1075,11 @@ function PersistenceToastWrapper(): JSX.Element | null {
|
||||
return <ToastModule.PersistenceToast show={true} />;
|
||||
}
|
||||
|
||||
// Helpers (mirror Settings.tsx — deduplicate by extracting if reused)
|
||||
// Helpers (mirror Settings.tsx — deduplicate by extracting if reused).
|
||||
// BLOCKER 3 invariant: `lastTickAt` is WALL-CLOCK MILLISECONDS (the value of
|
||||
// clock.now() at save time). The sim never writes lastTickAt — only saveSync
|
||||
// in PhaserGame.tsx and the Settings export path do. computeOfflineCatchup
|
||||
// reads the same wall-clock convention.
|
||||
function buildPayloadFromStore(s: ReturnType<typeof appStore.getState>, lastTickAt: number): V1Payload {
|
||||
return {
|
||||
garden: { tiles: s.tiles },
|
||||
@@ -1110,6 +1135,35 @@ function App() {
|
||||
const [settingsOpen, setSettingsOpen] = useState(false);
|
||||
const persistenceToastShown = useAppStore((s) => s.persistenceToastShown);
|
||||
|
||||
// W2 fix — D-29 keyboard shortcuts. Comma toggles Settings (a tasteful nod
|
||||
// to settings being a subordinate concern; easy to reach; no browser conflict).
|
||||
// 'j' toggles the Memory Journal (Plan 02-03 wires the open/close state via
|
||||
// the JournalIcon's local useState — for the hotkey, dispatch a custom event
|
||||
// that JournalIcon listens for, OR lift the journal-open state into the
|
||||
// store. Minimum-viable: expose a window-scoped toggler from JournalIcon
|
||||
// and call it here. Pick whichever feels least invasive when implementing;
|
||||
// record the choice in SUMMARY.md.)
|
||||
useEffect(() => {
|
||||
const onKeyDown = (e: KeyboardEvent) => {
|
||||
if (e.metaKey || e.ctrlKey || e.altKey) return;
|
||||
// Don't trigger when the user is typing into an input/textarea
|
||||
const target = e.target as HTMLElement | null;
|
||||
if (target && (target.tagName === 'INPUT' || target.tagName === 'TEXTAREA' || target.isContentEditable)) return;
|
||||
if (e.key === ',') {
|
||||
e.preventDefault();
|
||||
setSettingsOpen((o) => !o);
|
||||
} else if (e.key === 'j' || e.key === 'J') {
|
||||
e.preventDefault();
|
||||
// Dispatch a window event the JournalIcon listens for (kept decoupled
|
||||
// so JournalIcon owns its open/close state). JournalIcon adds a
|
||||
// matching addEventListener('tlg:toggle-journal', ...) handler.
|
||||
window.dispatchEvent(new CustomEvent('tlg:toggle-journal'));
|
||||
}
|
||||
};
|
||||
window.addEventListener('keydown', onKeyDown);
|
||||
return () => window.removeEventListener('keydown', onKeyDown);
|
||||
}, []);
|
||||
|
||||
return (
|
||||
<div id="app">
|
||||
<PhaserGame ref={phaserRef} />
|
||||
@@ -1159,6 +1213,15 @@ export default App;
|
||||
- `grep -q "ABSENCE_LETTER_THRESHOLD_MS = 5" src/PhaserGame.tsx`
|
||||
- `grep -q "__tlgFakeClock" src/PhaserGame.tsx` (URL flag wired)
|
||||
- `grep -q "import.meta.env" src/PhaserGame.tsx` (production guard for FakeClock)
|
||||
- `grep -q "migrate(" src/PhaserGame.tsx` (BLOCKER 1: boot path runs migrate, not just unwrap)
|
||||
- `grep -q "migrate(" src/ui/settings/Settings.tsx` (BLOCKER 2: import path runs migrate)
|
||||
- `grep -E "unwrap\\(.*\\)|migrate\\(" src/ui/settings/Settings.tsx | head -5` (BLOCKER 2: both unwrap and migrate appear)
|
||||
- `grep -q "lastTickAt: clock.now()" src/PhaserGame.tsx` (BLOCKER 3: saveSync writes wall-clock ms via clock.now)
|
||||
- `! grep -q "lastTickAt: this\\." src/sim/garden/Garden.ts || true` (BLOCKER 3: sim does NOT write lastTickAt — Garden lives under src/game/scenes/, not src/sim/, but this guards against accidental sim-side writes)
|
||||
- `! grep -E "lastTickAt: this\\.(currentTick|tickCount)" src/game/scenes/Garden.ts` (BLOCKER 3: Garden scene does NOT overwrite lastTickAt with a tick counter — saveSync owns it)
|
||||
- `grep -q "addEventListener('keydown'" src/App.tsx` (W2: D-29 keyboard shortcut wired)
|
||||
- `grep -q "tlg:toggle-journal" src/App.tsx` (W2: journal hotkey dispatches the toggle event)
|
||||
- `grep -q "lifecycleRef.current?.detach()" src/PhaserGame.tsx` (W5: lifecycle handle reaches outer effect cleanup)
|
||||
- `npx vitest run src/ui/letter/ src/ui/settings/` exits 0 with all tests green
|
||||
- `npm run ci` exits 0
|
||||
</acceptance_criteria>
|
||||
|
||||
Reference in New Issue
Block a user