diff --git a/CLAUDE.md b/CLAUDE.md index 83801db2..4317111a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -466,6 +466,33 @@ exists to allow, stranding the candidate on a console whose Stop has already bee for. The lock clears it when the guarded route unmounts, and again whenever a session becomes active, so an exit that never navigated cannot leave the next interview unguarded. +**And the predicate has to be current, which is not the same as being correct.** `useBlocker` hands +its function to the router from a `useEffect`, so the router holds whatever the *previous* +committed render gave it - while `` navigates from an effect of its own, and a child's +passive effect runs before its parent's. A route that renders a redirect in the same commit as the +state change permitting it was therefore asked the question with last render's answer, and refused; +`reset()` then dropped that navigation and `` never asked again, its dep array being +stable. Ending a mock interview with nothing recorded resets the session to Idle, and +`/mock-interview` then rendered `null` for the rest of the session - a blank screen where the home +page should have been. The predicate is a stable `useCallback` reading `active` / `exiting` / +`signedOut` off a ref written in a **layout** effect, which runs during the commit ahead of every +passive effect in it. `/main` only ever escaped this because `useEndLiveSession` awaits several IPC +round trips between `beginInterviewExit()` and its `navigate`, which is a timing accident rather +than a guard. `test/interview-lock.test.mjs` pins it, source-level, for the same reason the device +tests are. + +**A redirect that works has to carry the reason with it.** `session.error` is how main explains an +Idle it arrived at on its own, and nothing in the renderer read it - which did not show while that +branch was unreachable, and would have turned a blank screen into a silent bounce to the launch +cards the moment it was. The case it exists for is a dead or muted microphone: the silence backstop +skips its way through a session whose questions have already been billed, `finishToScoring` resets +with a message naming the microphone, and the candidate is owed it. `/mock-interview` reports it +deduplicated by the message rather than from one place, because both places see the same string and +neither can be dropped - `start()` writes it onto the session *and* throws, so the broadcast races +the rejection, while a microphone the renderer cannot open fails before main hears about it at all. +The ref is seeded from whatever is on the session at mount, which is an earlier session's ending: +`start()` clears it, but this route mounts before it runs. + **Stealth mode lives on the live control bar and nowhere else.** It was in the titlebar menu and the command palette, both reachable from the login screen and the payment page, where hiding the window from a screen capture answers a question nobody is asking - the screen share it exists for diff --git a/src/renderer/hooks/use-interview-lock.ts b/src/renderer/hooks/use-interview-lock.ts index decc9175..77b1d591 100644 --- a/src/renderer/hooks/use-interview-lock.ts +++ b/src/renderer/hooks/use-interview-lock.ts @@ -1,5 +1,5 @@ -import { useEffect } from 'react'; -import { useBlocker } from 'react-router-dom'; +import { useCallback, useEffect, useLayoutEffect, useRef } from 'react'; +import { type BlockerFunction, useBlocker } from 'react-router-dom'; import { create } from 'zustand'; import { RunningState } from '@/types/app-state'; @@ -94,9 +94,45 @@ export function useInterviewNavigationLock(active: boolean): void { return () => useInterviewExit.getState().clear(); }, [active]); + // The predicate is stable and reads the answer off a ref, rather than closing over this + // render's values. + // + // `useBlocker` hands the function to the router from a `useEffect`, so the router holds + // whatever the *previous* committed render gave it. `` navigates from an effect of + // its own, and a child's passive effect runs before its parent's - so a route that renders a + // redirect in the same commit as the state change that permits it was asked the question with + // last render's answer, and refused. The blocked navigation is then dropped by `reset()` below + // and `` never retries (its dep array is stable), leaving the route rendering + // nothing at all: ending a mock interview with no answers recorded resets the session to Idle + // and landed on a blank screen instead of the home page. + // + // A layout effect is what closes that window. Layout effects for the whole tree run during the + // commit, before any passive effect in it, so the ref is current by the time any child asks to + // navigate. `/main` only ever escaped this because `useEndLiveSession` awaits several IPC + // round trips between `beginInterviewExit()` and its own `navigate`, which is a timing + // accident rather than a guard. + // + // Writing the ref in the render body would cover one case further - layout effects themselves + // run child-first, so a child that navigated from one of its own would still read this render's + // predecessor - but no caller does that, and a render React discards would leave the router + // holding values that were never committed. A commit is the earliest point at which the answer + // is true, and it is the same point react-router's own `useNavigateStable` writes its `activeRef` + // from. + const predicateRef = useRef({ active, exiting, signedOut }); + useLayoutEffect(() => { + predicateRef.current = { active, exiting, signedOut }; + }); + const blocker = useBlocker( - ({ currentLocation, nextLocation }) => - active && !exiting && !signedOut && currentLocation.pathname !== nextLocation.pathname + useCallback(({ currentLocation, nextLocation }) => { + const current = predicateRef.current; + return ( + current.active && + !current.exiting && + !current.signedOut && + currentLocation.pathname !== nextLocation.pathname + ); + }, []) ); // Reset in an effect rather than from the predicate: the predicate runs during the router's own diff --git a/src/renderer/pages/mock-interview/index.tsx b/src/renderer/pages/mock-interview/index.tsx index ab888da9..cd01ff8a 100644 --- a/src/renderer/pages/mock-interview/index.tsx +++ b/src/renderer/pages/mock-interview/index.tsx @@ -1,4 +1,4 @@ -import { useEffect, useRef, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; import { Navigate, useLocation, useNavigate } from 'react-router-dom'; import { toast } from 'sonner'; @@ -72,6 +72,37 @@ export default function MockInterviewPage() { // waiting keeps that render on the loading screen instead. const [autoStarting, setAutoStarting] = useState(() => Boolean(pendingSetup)); + /** + * Say why the session ended, exactly once, whichever side says it first. + * + * `session.error` is how main explains an Idle it arrived at on its own, and until now nothing + * in the renderer read it. It did not show while the Idle branch below was unreachable - the + * navigation lock was refusing that redirect and the route rendered nothing at all - and with + * the redirect working it would have become a silent bounce to the home screen instead. The + * case that matters is a dead or muted microphone: the silence backstop skips its way through + * a session whose questions have already been billed, `finishToScoring` resets to Idle with a + * message naming the microphone, and being returned to the launch cards with no explanation + * leaves the candidate nothing to act on. + * + * Deduplicated by the message rather than reported from one place, because both places see the + * same string and neither can be dropped: `start()` writes it onto the session *and* throws, + * so the broadcast and the rejection race, while a microphone the renderer cannot open fails + * before main hears about it at all and has only the rejection. + * + * Seeded from whatever is already on the session at mount, which is a message some earlier + * session's ending left behind - `start()` clears it, but this route mounts before it runs. + */ + const reportedError = useRef(sessionRef.current?.error ?? null); + const reportError = useCallback((message: string) => { + if (reportedError.current === message) return; + reportedError.current = message; + toast.error(message); + }, []); + + useEffect(() => { + if (session?.error) reportError(session.error); + }, [session?.error, reportError]); + useEffect(() => { if (!pendingSetup || autoStartRequested.current) return; if (sessionRef.current && sessionRef.current.state !== MockInterviewState.Idle) return; @@ -79,7 +110,7 @@ export default function MockInterviewPage() { setAutoStarting(true); startSession(pendingSetup).catch((error) => { console.error('Failed to auto-start mock interview:', error); - toast.error(error instanceof Error ? error.message : 'Failed to start the mock interview'); + reportError(error instanceof Error ? error.message : 'Failed to start the mock interview'); setAutoStarting(false); }); // eslint-disable-next-line react-hooks/exhaustive-deps diff --git a/test/interview-lock.test.mjs b/test/interview-lock.test.mjs new file mode 100644 index 00000000..38d00432 --- /dev/null +++ b/test/interview-lock.test.mjs @@ -0,0 +1,104 @@ +/** + * The interview navigation lock is renderer code, so these are source-level checks in the same + * shape as `audio-device-switch.test.mjs` - and for the same reason. What they pin is an + * ordering between two React effects, which no type checker or linter can see and which reads + * like an over-complication to whoever next tidies it up. + * + * The failure being guarded against: `useBlocker` hands its predicate to the router from a + * `useEffect`, so the router holds whatever the *previous* committed render gave it. A route + * that renders `` in the same commit as the state change that permits the navigation + * is therefore asked the question with last render's answer, and refused - and `` has + * a stable dep array, so it never asks again. Ending a mock interview with nothing recorded + * resets the session to Idle and used to land on a blank screen for exactly this reason. + * + * Reading the answer off a ref written in a *layout* effect is what closes that window: layout + * effects for the whole tree run during the commit, before any passive effect in it. + */ +import { codeOnly, createChecker, readSource } from './helpers.mjs'; + +/** The slice from the first `open` at or after `from` to its matching `close`, or ''. */ +function balanced(source, from, open, close) { + if (from < 0) return ''; + const start = source.indexOf(open, from); + if (start === -1) return ''; + let depth = 0; + for (let i = start; i < source.length; i++) { + if (source[i] === open) depth++; + else if (source[i] === close && --depth === 0) return source.slice(start, i + 1); + } + return ''; +} + +export async function run() { + const { check, failures } = createChecker('interview-lock'); + + const source = codeOnly( + readSource(new URL('../src/renderer/hooks/use-interview-lock.ts', import.meta.url)) + ); + + check('the lock still blocks through useBlocker', source.includes('useBlocker(')); + + // Both slices are bounded rather than run to the end of the file. The closure check below + // forbids naming `active` at all, and the ref is *written* from `{ active, exiting, signedOut }` + // a few lines above - so an unbounded slice would fail the moment someone moved the layout + // effect under the blocker call, with a message describing a defect that is not there. + const blockerCall = balanced(source, source.indexOf('useBlocker('), '(', ')'); + // Falling back to the whole call rather than to nothing: an arrow with a concise body - which + // is the shape the bug had - has no braces to bound, and an empty slice would pass every check + // below by containing none of what they forbid. + const predicate = balanced(blockerCall, blockerCall.indexOf('=>'), '{', '}') || blockerCall; + + check( + 'the predicate reads the current answer off a ref', + predicate.includes('predicateRef.current') + ); + + // A reference to the bare variable rather than to the field on the ref is a reference to one + // render's answer, which is the whole of the defect. + check( + 'and does not close over the render values it is deciding on', + !/[^.\w]active\b/.test(predicate) && + !/[^.\w]exiting\b/.test(predicate) && + !/[^.\w]signedOut\b/.test(predicate) + ); + + // Stability is not cosmetic here. A predicate whose identity changes every render is one the + // router re-registers every render, from the same passive effect, which is the thing being + // raced. + check( + 'the predicate is stable across renders', + /useBlocker\(\s*useCallback\b/.test(source) && + /,\s*\[\s*\]\s*\)$/.test( + balanced(blockerCall, blockerCall.indexOf('useCallback'), '(', ')').trim() + ) + ); + + // The ordering itself. A plain `useEffect` here runs *after* a child's, which is precisely the + // window the blank page appeared in. + const refWrite = source.indexOf('predicateRef.current = '); + const layout = source.lastIndexOf('useLayoutEffect(', refWrite); + check('the ref is written from a layout effect', refWrite !== -1 && layout !== -1); + + // Prettier is not enforced in this repo, so this asks for the shape rather than the spacing: + // an argument list that is only the effect body is one that runs on every render. + const layoutCall = balanced(source, layout, '(', ')'); + check( + 'that layout effect runs on every render rather than on a dependency change', + /^\(\s*\(\s*\)\s*=>/.test(layoutCall) && + /predicateRef\.current\s*=\s*\{\s*active,\s*exiting,\s*signedOut\s*\}/.test(layoutCall) && + !/,\s*\[/.test(layoutCall.slice(layoutCall.lastIndexOf('}'))) + ); + + // The redirect this exists to let through: ending a mock interview with nothing recorded + // resets the session to Idle in main, and this is the only thing that takes the candidate off + // a route that then has nothing left to show. + const page = codeOnly( + readSource(new URL('../src/renderer/pages/mock-interview/index.tsx', import.meta.url)) + ); + check( + 'an Idle mock session with nothing pending redirects home', + //.test(page) + ); + + return failures; +} diff --git a/test/run.mjs b/test/run.mjs index e03050ec..2c68549a 100644 --- a/test/run.mjs +++ b/test/run.mjs @@ -76,6 +76,9 @@ for (const module of [ './suggestion-emphasis.test.mjs', './suggestion-truncate.test.mjs', './navigation-guard.test.mjs', + // Source-level and independent of everything above: it reads the renderer's navigation lock + // and the mock route off disk, so it sits with the other source-level checks. + './interview-lock.test.mjs', './mac-update-util.test.mjs', './change-password.test.mjs', './password-reset.test.mjs',