From 53cdcbe9f1a5003e23c9e5d12b1919bd898aa0cc Mon Sep 17 00:00:00 2001 From: alpha Date: Tue, 15 Sep 2026 10:30:34 -0400 Subject: [PATCH 1/2] fix(mock): let the empty-session redirect past the navigation lock Ending a mock interview with nothing recorded resets the session to Idle, and `/mock-interview` renders `` for that. The redirect never fired: `useBlocker` registers its predicate from a `useEffect`, so the router still held the one closed over the previous render's `active` - true, the session having been `Stopping` a render earlier - while `` navigates from a child effect, which runs first. The navigation was blocked, `reset()` dropped it, and `` never retried, leaving the route rendering nothing at all. The predicate is now stable and reads its answer off a ref written in a layout effect, which runs during the commit ahead of every passive effect in it. `/main` only escaped this because `useEndLiveSession` awaits several IPC round trips between `beginInterviewExit()` and its navigate. Closes #131 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Kt5ucR5XPzzzxqPYzSpXaL --- CLAUDE.md | 15 +++++ src/renderer/hooks/use-interview-lock.ts | 37 ++++++++++-- test/interview-lock.test.mjs | 75 ++++++++++++++++++++++++ test/run.mjs | 3 + 4 files changed, 126 insertions(+), 4 deletions(-) create mode 100644 test/interview-lock.test.mjs diff --git a/CLAUDE.md b/CLAUDE.md index 83801db2..0b70ffe7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -466,6 +466,21 @@ 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. + **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..c7ff0367 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,38 @@ 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. + 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/test/interview-lock.test.mjs b/test/interview-lock.test.mjs new file mode 100644 index 00000000..279669f6 --- /dev/null +++ b/test/interview-lock.test.mjs @@ -0,0 +1,75 @@ +/** + * 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'; + +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(')); + + const predicate = source.slice(source.indexOf('useBlocker(')); + 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(predicate) + ); + + // 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); + check( + 'that layout effect runs on every render rather than on a dependency change', + /useLayoutEffect\(\(\) => \{\s*predicateRef\.current = \{ active, exiting, signedOut \};\s*\}\);/.test( + source + ) + ); + + // 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', From f89da5d32f9b32de3cf5126502ad3b86f5df3ea3 Mon Sep 17 00:00:00 2001 From: alpha Date: Tue, 15 Sep 2026 10:37:50 -0400 Subject: [PATCH 2/2] fix(mock): say why a session ended when it ends itself Review follow-up on the redirect this branch unblocks. `session.error` is how main explains an Idle it reached on its own, and nothing in the renderer read it. It did not show while the Idle branch was unreachable; with the redirect working it would have become a silent bounce to the launch cards. The case that matters is a dead or muted microphone - the silence backstop skips through a session whose questions are already billed, and `finishToScoring` resets with a message naming the microphone. Deduplicated by the message rather than reported from one place: `start()` writes the same string onto the session *and* throws, so the broadcast races the rejection, while a microphone the renderer cannot open fails before main hears about it and has only the rejection. Also scopes the new test's slices to the blocker call, so moving the layout effect below it fails nothing - it is the same shape, and an unbounded slice reported a defect that was not there. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Kt5ucR5XPzzzxqPYzSpXaL --- CLAUDE.md | 12 +++++++ src/renderer/hooks/use-interview-lock.ts | 7 ++++ src/renderer/pages/mock-interview/index.tsx | 35 ++++++++++++++++-- test/interview-lock.test.mjs | 39 ++++++++++++++++++--- 4 files changed, 86 insertions(+), 7 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0b70ffe7..4317111a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -481,6 +481,18 @@ round trips between `beginInterviewExit()` and its `navigate`, which is a timing 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 c7ff0367..77b1d591 100644 --- a/src/renderer/hooks/use-interview-lock.ts +++ b/src/renderer/hooks/use-interview-lock.ts @@ -111,6 +111,13 @@ export function useInterviewNavigationLock(active: boolean): void { // 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 }; 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 index 279669f6..38d00432 100644 --- a/test/interview-lock.test.mjs +++ b/test/interview-lock.test.mjs @@ -16,6 +16,19 @@ */ 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'); @@ -25,7 +38,16 @@ export async function run() { check('the lock still blocks through useBlocker', source.includes('useBlocker(')); - const predicate = source.slice(source.indexOf('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') @@ -45,7 +67,10 @@ export async function run() { // raced. check( 'the predicate is stable across renders', - /useBlocker\(\s*useCallback\b/.test(source) && /,\s*\[\]\s*\)\s*\);/.test(predicate) + /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 @@ -53,11 +78,15 @@ export async function run() { 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', - /useLayoutEffect\(\(\) => \{\s*predicateRef\.current = \{ active, exiting, signedOut \};\s*\}\);/.test( - source - ) + /^\(\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