Move layout generation into a Web Worker (#7) - #8
Conversation
Move the layout computation off the main thread into a dedicated worker: - shared computeLayoutPoints() runs the strategy + cutoff filter - layout-worker + client transfer points as a Float64Array buffer - useGeneratorState requests layouts asynchronously, ignoring stale responses (latest wins), seeds the first layout synchronously to avoid a paint flash, and falls back to a synchronous compute if the worker is unavailable/errors MAX_TUBE_POINTS stays as a render/interaction guard (canvas + RBush remain on the main thread). Codec is unit-tested. Closes #7. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request offloads the layout point computation to a Web Worker to keep the UI thread responsive, introducing a worker client with synchronous fallback and binary serialization for points. Feedback highlights a critical race condition in the React state hook when handling large layouts, a potential crash during worker instantiation under strict CSPs, and a potential memory leak if message posting throws synchronously.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const layoutRequestSeq = useRef(0); | ||
|
|
||
| useEffect(() => { | ||
| if (layoutTooLarge) { | ||
| return []; | ||
| setTubeCoords([]); | ||
| return; | ||
| } | ||
| return getLayoutStrategy(deferredParams.tubeLayout) | ||
| .calculatePoints(deferredParams) | ||
| .filter((point) => !isWithinCutoffZone(point, deferredParams)); | ||
| const seq = ++layoutRequestSeq.current; | ||
| void requestLayout(deferredParams).then((points) => { | ||
| if (seq === layoutRequestSeq.current) { | ||
| setTubeCoords(points); | ||
| } | ||
| }); | ||
| }, [deferredParams, layoutTooLarge]); |
There was a problem hiding this comment.
There is a critical race condition in the useEffect hook when layoutTooLarge becomes true.\n\nIf a layout request is currently pending and the user quickly changes parameters such that layoutTooLarge becomes true:\n1. The effect runs again, enters the if (layoutTooLarge) block, calls setTubeCoords([]), and returns early.\n2. Crucially, layoutRequestSeq.current is NOT incremented because the early return happens before the increment.\n3. When the pending request eventually resolves, the condition seq === layoutRequestSeq.current will evaluate to true (since layoutRequestSeq.current was never incremented).\n4. The stale points will then be set via setTubeCoords(points), completely overwriting the empty array and bypassing the layoutTooLarge guard. This can freeze the main thread/tab when rendering the massive layout.\n\nUsing the standard React active flag pattern in the effect cleanup solves this race condition elegantly and eliminates the need for the layoutRequestSeq ref entirely.
useEffect(() => {\n let active = true;\n if (layoutTooLarge) {\n setTubeCoords([]);\n return;\n }\n void requestLayout(deferredParams).then((points) => {\n if (active) {\n setTubeCoords(points);\n }\n });\n return () => {\n active = false;\n };\n }, [deferredParams, layoutTooLarge]);| const getWorker = (): Worker | null => { | ||
| if (worker) return worker; | ||
| if (typeof Worker === 'undefined') return null; | ||
|
|
||
| const instance = new Worker(new URL('./layout-worker.ts', import.meta.url), {type: 'module'}); | ||
| instance.addEventListener('message', (event: MessageEvent<LayoutWorkerResult>) => { | ||
| const message = event.data; | ||
| if (!message || typeof message !== 'object') return; | ||
| const handler = pending.get(message.requestId); | ||
| if (!handler) return; | ||
| pending.delete(message.requestId); | ||
| if (message.ok) { | ||
| handler.resolve(decodePoints(message.buffer)); | ||
| } else { | ||
| handler.reject(new Error(message.message)); | ||
| } | ||
| }); | ||
| instance.addEventListener('error', (event) => { | ||
| const error = event.error instanceof Error ? event.error : new Error(event.message || 'Layout worker crashed.'); | ||
| if (worker === instance) { | ||
| destroyWorker(error); | ||
| } | ||
| }); | ||
|
|
||
| worker = instance; | ||
| return instance; | ||
| }; |
There was a problem hiding this comment.
Instantiating a new Worker can throw a synchronous exception in environments with strict Content Security Policies (CSP) (e.g., SecurityError: Failed to construct 'Worker') or if the browser doesn't support module workers.\n\nSince getWorker() is called synchronously inside requestLayout before the promise is created, any synchronous throw will propagate up and crash the React render/effect cycle (triggering an Error Boundary).\n\nWrapping the worker instantiation in a try...catch block and returning null on failure ensures that the application safely falls back to the synchronous layout computation without crashing.
const getWorker = (): Worker | null => {\n if (worker) return worker;\n if (typeof Worker === 'undefined') return null;\n\n try {\n const instance = new Worker(new URL('./layout-worker.ts', import.meta.url), {type: 'module'});\n instance.addEventListener('message', (event: MessageEvent<LayoutWorkerResult>) => {\n const message = event.data;\n if (!message || typeof message !== 'object') return;\n const handler = pending.get(message.requestId);\n if (!handler) return;\n pending.delete(message.requestId);\n if (message.ok) {\n handler.resolve(decodePoints(message.buffer));\n } else {\n handler.reject(new Error(message.message));\n }\n });\n instance.addEventListener('error', (event) => {\n const error = event.error instanceof Error ? event.error : new Error(event.message || 'Layout worker crashed.');\n if (worker === instance) {\n destroyWorker(error);\n }\n });\n\n worker = instance;\n return instance;\n } catch (error) {\n console.error('Failed to initialize layout worker:', error);\n return null;\n }\n};| return new Promise<Point[]>((resolve, reject) => { | ||
| pending.set(requestId, {resolve, reject}); | ||
| const request: LayoutWorkerRequest = {requestId, params}; | ||
| w.postMessage(request); | ||
| }).catch(() => computeLayoutPoints(params)); |
There was a problem hiding this comment.
If w.postMessage(request) throws an error synchronously (for example, a DataCloneError if non-serializable parameters are introduced in the future, or if the worker is in an invalid state), the promise is rejected, but the requestId is never removed from the pending map. This can lead to a memory leak.\n\nWrapping w.postMessage in a try...catch block inside the promise executor and deleting the pending request on failure prevents this leak.
return new Promise<Point[]>((resolve, reject) => {\n pending.set(requestId, {resolve, reject});\n const request: LayoutWorkerRequest = {requestId, params};\n try {\n w.postMessage(request);\n } catch (error) {\n pending.delete(requestId);\n reject(error);\n }\n }).catch(() => computeLayoutPoints(params));![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 750404f08a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (layoutTooLarge) { | ||
| return []; | ||
| setTubeCoords([]); | ||
| return; |
There was a problem hiding this comment.
If a valid layout request is still in flight and the user changes to parameters whose estimate exceeds MAX_TUBE_POINTS, this branch clears the coordinates but leaves layoutRequestSeq unchanged. The old promise still has the current sequence value and can later call setTubeCoords(points), so the preview and hole counts can repopulate with an out-of-date layout while layoutTooLarge is true. Increment the sequence or otherwise cancel/ignore pending requests before returning here.
Useful? React with 👍 / 👎.
| if (worker) return worker; | ||
| if (typeof Worker === 'undefined') return null; | ||
|
|
||
| const instance = new Worker(new URL('./layout-worker.ts', import.meta.url), {type: 'module'}); |
There was a problem hiding this comment.
When Worker exists but this constructor throws, for example because a deployed CSP blocks worker-src or module workers are unavailable, the exception escapes before requestLayout reaches its Promise .catch(). In that environment the documented synchronous fallback is skipped and the React effect fails instead of computing the layout on the main thread. Wrap worker creation in try/catch and return null (or fall back directly) on construction failure.
Useful? React with 👍 / 👎.
1 participant
Closes #7.
Moves tube-point layout generation off the main thread into a dedicated Web Worker, so a layout is computed without competing with rendering.
Changes
core/layout-strategies: extracted sharedcomputeLayoutPoints(params)(strategy points + cutoff filter), used by both the worker and the fallback.layout-worker+layout-worker-client: compute points in a worker and transfer them back as an interleavedFloat64Arraybuffer (cheap for large layouts). The client recreates the worker after a crash and falls back to a synchronous compute if a worker can't be created or errors — the app never loses layout.useGeneratorState:tubeCoordsis now async state, requested on each deferred params change with latest-wins stale-response handling. The initial layout is seeded synchronously to avoid a first-paint flash.MAX_TUBE_POINTSstays as a render/interaction guard (canvas + RBush are still main-thread); comment updated.Verification
pnpm typecheck✅ ·pnpm test(58, incl. new codec round-trip tests) ✅ ·pnpm build✅layout-worker.ts?worker_filerequest), initial 169 holes seeded, pitch→40 recomputes to 109 asynchronously, click-select/hit-test works against worker-provided points, no error boundary.🤖 Generated with Claude Code