Fix stability, calculation and UI weak spots (#1) by YurMil · Pull Request #2 · YurMil/Tube-Sheet-Generator-

YurMil · 2026-07-17T15:54:13Z

Closes #1.

Addresses all 12 findings from the code review, grouped into four verified commits. Each change was exercised in the running app (typecheck + production build are green).

🔴 Stability (`f7b1cab`)

Worker recreated after a crash. cad-worker-client now terminates and drops the dead worker so a crash no longer leaves the STEP button stuck disabled forever. Added a watchdog timeout (STEP_TIMEOUT_MS) and an explicit cancel path (CadWorkerCancelledError), wired to a Cancel generation button.
Hard cap on layout size. The layout runs against a useDeferredValue copy of params and refuses when the estimated point count exceeds MAX_TUBE_POINTS, so a huge diameter + tiny pitch can no longer freeze the main thread. Verified: pitch 0.5 (~1.2M points) now shows a warning and stays responsive instead of hanging the tab.
Error boundary replaces the white-screen-on-render-error failure mode.

🟠 Calculations (`75f8859`)

Heat-transfer area sums each active tube's real outer diameter instead of the nominal one, so mixed-diameter sheets report correctly.
New warnings: tubes overlapping a pass-partition lane (they'd collide with the plate) and holes pushed past the sheet edge by a custom diameter.
Shared getPartitionOffsets / isWithinPartitionBand helpers now back the preview, DXF exporter and the conflict check, removing triplicated partition geometry.

🟡 UI (`4647192`)

Number inputs use a local text buffer, so a field can be cleared / hold intermediate values (0., empty) instead of snapping back every keystroke; it reconciles on blur.
Wheel zoom bound natively with {passive:false} so preventDefault works and no longer warns.
DXF emits a HEADER with $INSUNITS=4 so strict CAD readers import it as millimetres.

⚪ CI (`b083b1d`)

Workflow concurrency group serialises deploys; the host-repo push retries with git pull --rebase so a racing non-fast-forward retries instead of failing.

Verification

pnpm typecheck ✅
pnpm build ✅
Manual smoke test in-browser: layout guard, partition-conflict warning, clearable number field, wheel zoom (no console warning), DXF $INSUNITS header.

🤖 Generated with Claude Code

Fixes the top stability issues from the code review (#1-#4): - cad-worker-client: terminate and recreate the worker after a crash so a dead worker no longer leaves the STEP button stuck forever; add a watchdog timeout and an explicit cancel path (CadWorkerCancelledError / TimeoutError). - useGeneratorState: run the layout against a deferred copy of params and refuse layouts whose estimated point count exceeds MAX_TUBE_POINTS, so a huge diameter + tiny pitch can no longer freeze the main thread. - App: wire a Cancel button and STEP_TIMEOUT_MS, disable exports and surface a clear message when the layout is too large. - Add an ErrorBoundary so a render error shows a recoverable screen instead of a blank page. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Addresses calculation findings (#5-#7): - Heat-transfer area sums each active tube's real outer diameter instead of always using the nominal one, so mixed-diameter sheets report correctly. - Warn when tubes overlap a pass-partition lane (they would collide with the partition plate) and when a custom diameter pushes a hole past the sheet edge. - Add shared getPartitionOffsets / isWithinPartitionBand helpers and route the preview, DXF exporter and the new conflict check through them so partition geometry can no longer drift between views. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Addresses UI findings (#9-#11): - GeneratorForm number inputs use a local text buffer so a field can be cleared and accept intermediate values (empty, "0.") without snapping back on every keystroke; it reconciles to the numeric value on blur. - PreviewCanvas binds the wheel handler natively with {passive:false} so preventDefault actually works and no longer warns in the console. - DXF export now emits a HEADER with $INSUNITS=4 so strict CAD readers import it as millimetres. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Addresses CI finding (#12): add a workflow concurrency group so two pushes to main no longer deploy in parallel, and retry the host-repo push with a rebase so a racing non-fast-forward retries instead of failing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b083b1da2d

ℹ️ 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".

chatgpt-codex-connector · 2026-07-17T15:57:49Z

+    return getLayoutStrategy(deferredParams.tubeLayout)
+      .calculatePoints(deferredParams)
+      .filter((point) => !isWithinCutoffZone(point, deferredParams));


Prevent exports while deferred layout is stale

When a parameter edit is pending in useDeferredValue, params has the new values but tubeCoords and layoutTooLarge still correspond to the previous deferredParams. The DXF handler and STEP generation then combine current dimensions with stale coordinates if the user clicks export immediately after changing pitch/layout, and the too-large-layout disable can also be bypassed until the deferred render catches up. Track a stale/deferred state and disable exports, or export using the same params that produced the coordinates.

Useful? React with 👍 / 👎.

chatgpt-codex-connector · 2026-07-17T15:57:49Z

+  const safeRadius = getSafeRadius(params);
+  if (safeRadius <= 0) {
+    return 0;
+  }
+  const minSpacing = params.tubePitch / Math.SQRT2;
+  // Circle area / cell area, with a small safety factor for boundary rows.
+  return Math.ceil((Math.PI * safeRadius * safeRadius) / (minSpacing * minSpacing)) + 1;


Cap layout iterations, not just accepted points

This estimate uses safeRadius, but the layout strategies still iterate out to boardRadius; when the tube diameter is almost the board diameter, safeRadius is tiny and the estimate can stay below MAX_TUBE_POINTS even though the nested loops are enormous. For example, boardDiameter=1000000, tubeDiameter=999998, and tubePitch=1 estimates only about 8 points, yet square layout would iterate roughly 250 billion cells before rejecting most of them, freezing the tab instead of showing the warning. The guard should bound loop iterations (or the layout loop extents), not only accepted point count.

Useful? React with 👍 / 👎.

gemini-code-assist

Code Review

This pull request introduces performance and stability improvements to the tube sheet generator, including a hard ceiling on generated tube points, a watchdog timeout for STEP generation, worker cancellation support, an ErrorBoundary component, and a buffered NumberField input to prevent UI freezing. The review feedback highlights a potential bug in the CAD worker client where an error on an old worker instance could incorrectly reject active requests on a new instance. Additionally, the reviewer recommends consistently using the newly introduced deferredParams instead of params across several components and hooks (such as generateStep, tubeStats, and filename generation) to ensure visual and data synchronization with the deferred layout coordinates.

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.

gemini-code-assist · 2026-07-17T15:58:38Z

    async (options?: {
      onProgress?: (message: CadWorkerProgressMessage) => void;
      modifiedHoles?: Map<string, ModifiedHole>;
+      timeoutMs?: number;
    }) => {
      if (workerStatus !== 'ready') {
        await warmupWorker();
      }
-      return generateStepInWorker(params, tubeCoords, options?.modifiedHoles, {onProgress: options?.onProgress});
+      return generateStepInWorker(params, tubeCoords, options?.modifiedHoles, {
+        onProgress: options?.onProgress,
+        timeoutMs: options?.timeoutMs,
+      });
    },
    [params, tubeCoords, warmupWorker, workerStatus],
  );


Update generateStep to use deferredParams instead of params to ensure that the generated 3D model is perfectly synchronized with the deferred layout coordinates (tubeCoords).

Suggested change

async (options?: {

onProgress?: (message: CadWorkerProgressMessage) => void;

modifiedHoles?: Map<string, ModifiedHole>;

timeoutMs?: number;

}) => {

if (workerStatus !== 'ready') {

await warmupWorker();

}

return generateStepInWorker(params, tubeCoords, options?.modifiedHoles, {onProgress: options?.onProgress});

return generateStepInWorker(params, tubeCoords, options?.modifiedHoles, {

onProgress: options?.onProgress,

timeoutMs: options?.timeoutMs,

});

},

[params, tubeCoords, warmupWorker, workerStatus],

);

async (options?: {

onProgress?: (message: CadWorkerProgressMessage) => void;

modifiedHoles?: Map<string, ModifiedHole>;

timeoutMs?: number;

}) => {

if (workerStatus !== 'ready') {

await warmupWorker();

}

return generateStepInWorker(deferredParams, tubeCoords, options?.modifiedHoles, {

onProgress: options?.onProgress,

timeoutMs: options?.timeoutMs,

});

},

[deferredParams, tubeCoords, warmupWorker, workerStatus],

gemini-code-assist · 2026-07-17T15:58:38Z

    params,
    setParams,
    tubeCoords,
+    layoutTooLarge,
+    estimatedPointCount,
    handleChange,
    workerStatus,
    workerError,


Return deferredParams from the hook so it can be consumed by the parent component.

Suggested change

params,

setParams,

tubeCoords,

layoutTooLarge,

estimatedPointCount,

handleChange,

workerStatus,

workerError,

params,

deferredParams,

setParams,

tubeCoords,

layoutTooLarge,

estimatedPointCount,

handleChange,

workerStatus,

workerError,

gemini-code-assist · 2026-07-17T15:58:38Z

+  const {
+    params,
+    setParams,
+    tubeCoords,
+    layoutTooLarge,
+    estimatedPointCount,
+    handleChange,
+    generateStep,
+    workerStatus,
+    workerError,
+  } = useGeneratorState();


Destructure deferredParams from useGeneratorState to use it for all layout-dependent calculations, previews, and exports.

Suggested change

const {

params,

setParams,

tubeCoords,

layoutTooLarge,

estimatedPointCount,

handleChange,

generateStep,

workerStatus,

workerError,

} = useGeneratorState();

const {

params,

deferredParams,

setParams,

tubeCoords,

layoutTooLarge,

estimatedPointCount,

handleChange,

generateStep,

workerStatus,

workerError,

} = useGeneratorState();

gemini-code-assist · 2026-07-17T15:58:38Z

  const tubeStats = useMemo(() => {
    let hidden = 0;
    let tieRods = 0;
+    let heatTransferArea = 0;
+    let partitionConflicts = 0;
+    let edgeOverflow = 0;
+    const boardRadius = params.boardDiameter / 2;

    tubeCoords.forEach((point) => {
      const modified = modifiedHoles.get(createPointKey(point));
      if (modified?.hidden) {
        hidden += 1;
        return;
      }
+
+      const diameter = modified?.diameter ?? params.tubeDiameter;
+      const radius = diameter / 2;
+
+      if (Math.hypot(point.x, point.y) + radius > boardRadius + 1e-6) {
+        edgeOverflow += 1;
+      }
+      if (isWithinPartitionBand(point, radius, params)) {
+        partitionConflicts += 1;
+      }
+
      if (modified?.type === 'tieRod') {
        tieRods += 1;
+        return;
      }
+
+      // Heat-transfer surface uses each active tube's real outer diameter, not
+      // the nominal one, so mixed-diameter sheets report a correct area.
+      heatTransferArea += Math.PI * diameter * params.tubeLength;
    });

    const cutHoles = Math.max(0, tubeCoords.length - hidden);
    const activeTubes = Math.max(0, cutHoles - tieRods);

-    return {hidden, tieRods, cutHoles, activeTubes};
-  }, [modifiedHoles, tubeCoords]);
+    return {hidden, tieRods, cutHoles, activeTubes, heatTransferArea, partitionConflicts, edgeOverflow};
+  }, [modifiedHoles, params, tubeCoords]);


Use deferredParams instead of params in tubeStats to ensure that statistics, conflicts, and heat transfer area calculations are perfectly synchronized with the deferred tubeCoords. This prevents temporary visual glitches or incorrect warning counts during typing.

const tubeStats = useMemo(() => { let hidden = 0; let tieRods = 0; let heatTransferArea = 0; let partitionConflicts = 0; let edgeOverflow = 0; const boardRadius = deferredParams.boardDiameter / 2; tubeCoords.forEach((point) => { const modified = modifiedHoles.get(createPointKey(point)); if (modified?.hidden) { hidden += 1; return; } const diameter = modified?.diameter ?? deferredParams.tubeDiameter; const radius = diameter / 2; if (Math.hypot(point.x, point.y) + radius > boardRadius + 1e-6) { edgeOverflow += 1; } if (isWithinPartitionBand(point, radius, deferredParams)) { partitionConflicts += 1; } if (modified?.type === 'tieRod') { tieRods += 1; return; } // Heat-transfer surface uses each active tube's real outer diameter, not // the nominal one, so mixed-diameter sheets report a correct area. heatTransferArea += Math.PI * diameter * deferredParams.tubeLength; }); const cutHoles = Math.max(0, tubeCoords.length - hidden); const activeTubes = Math.max(0, cutHoles - tieRods); return {hidden, tieRods, cutHoles, activeTubes, heatTransferArea, partitionConflicts, edgeOverflow}; }, [modifiedHoles, deferredParams, tubeCoords]);

gemini-code-assist · 2026-07-17T15:58:38Z

+  instance.addEventListener('error', (event) => {
+    const error = event.error instanceof Error ? event.error : new Error(event.message || 'CAD worker crashed.');
+    // A worker `error` event means the instance is unusable. Reject everything
+    // and drop the reference so the next call recreates a healthy worker.
+    if (worker === instance) {
+      destroyWorker(error);
+    } else {
+      const handlers = Array.from(pending.values());
+      pending.clear();
+      handlers.forEach((handler) => handler.reject(error));
+      instance.terminate();
+    }
  });


When worker !== instance, clearing the global pending map and rejecting all handlers is a bug. It will reject active, healthy requests belonging to the new active worker instance, even though the error occurred on an old, discarded worker instance.

To fix this, we should associate each pending request with the specific worker instance that sent it, and only reject/cleanup requests belonging to the errored worker instance.

Suggested change

instance.addEventListener('error', (event) => {

const error = event.error instanceof Error ? event.error : new Error(event.message || 'CAD worker crashed.');

// A worker `error` event means the instance is unusable. Reject everything

// and drop the reference so the next call recreates a healthy worker.

if (worker === instance) {

destroyWorker(error);

} else {

const handlers = Array.from(pending.values());

pending.clear();

handlers.forEach((handler) => handler.reject(error));

instance.terminate();

}

});

instance.addEventListener('error', (event) => {

const error = event.error instanceof Error ? event.error : new Error(event.message || 'CAD worker crashed.');

// A worker `error` event means the instance is unusable. Reject everything

// and drop the reference so the next call recreates a healthy worker.

if (worker === instance) {

destroyWorker(error);

} else {

for (const [requestId, handler] of pending.entries()) {

if (handler.worker === instance) {

handler.reject(error);

pending.delete(requestId);

}

}

instance.terminate();

}

});

gemini-code-assist · 2026-07-17T15:58:39Z

+    pending.set(requestId, {
+      resolve: (value) => {
+        cleanup();
+        resolve(value);
+      },
+      reject: (error) => {
+        cleanup();
+        reject(error);
+      },
+      onProgress: options?.onProgress,
+    });


Store the worker instance w in the pending request entry so we can safely clean up only the requests associated with this specific worker if it crashes or times out.

Suggested change

pending.set(requestId, {

resolve: (value) => {

cleanup();

resolve(value);

},

reject: (error) => {

cleanup();

reject(error);

},

onProgress: options?.onProgress,

});

pending.set(requestId, {

resolve: (value) => {

cleanup();

resolve(value);

},

reject: (error) => {

cleanup();

reject(error);

},

onProgress: options?.onProgress,

worker: w,

});

gemini-code-assist · 2026-07-17T15:58:39Z


-  const heatTransferArea = tubeStats.activeTubes * Math.PI * params.tubeDiameter * params.tubeLength;
+  const heatTransferArea = tubeStats.heatTransferArea;
  const pitchRatioWarning = params.tubePitch < params.tubeDiameter * 1.25;


Use deferredParams for pitchRatioWarning to ensure the warning state matches the current deferred layout.

Suggested change

const pitchRatioWarning = params.tubePitch < params.tubeDiameter * 1.25;

const pitchRatioWarning = deferredParams.tubePitch < deferredParams.tubeDiameter * 1.25;

gemini-code-assist · 2026-07-17T15:58:39Z

      const blob = new Blob([stepArrayBuffer], {type: 'application/step'});
      downloadBlob(blob, `tubesheet_${params.boardDiameter}mm.step`);


Use deferredParams for the exported STEP filename to ensure consistency.

Suggested change

const blob = new Blob([stepArrayBuffer], {type: 'application/step'});

downloadBlob(blob, `tubesheet_${params.boardDiameter}mm.step`);

const blob = new Blob([stepArrayBuffer], {type: 'application/step'});

downloadBlob(blob, `tubesheet_${deferredParams.boardDiameter}mm.step`);

YurMil and others added 4 commits July 17, 2026 18:43

YurMil merged commit 69516da into main Jul 17, 2026
2 checks passed

YurMil deleted the fix/stability-calc-ui branch July 17, 2026 15:57

YurMil mentioned this pull request Jul 17, 2026

Code review: stability, calculation and UI weak spots #1

Closed

chatgpt-codex-connector Bot reviewed Jul 17, 2026

View reviewed changes

gemini-code-assist Bot reviewed Jul 17, 2026

View reviewed changes

YurMil mentioned this pull request Jul 17, 2026

CI: bump GitHub Actions off deprecated Node.js 20 runtime #3

Closed

Navigation Menu

Fix stability, calculation and UI weak spots (#1) - #2

Fix stability, calculation and UI weak spots (#1)#2
YurMil merged 4 commits into
mainfrom
fix/stability-calc-ui

YurMil commented Jul 17, 2026

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jul 17, 2026

Uh oh!

chatgpt-codex-connector Bot Jul 17, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	const pitchRatioWarning = params.tubePitch < params.tubeDiameter * 1.25;
	const pitchRatioWarning = deferredParams.tubePitch < deferredParams.tubeDiameter * 1.25;

		const blob = new Blob([stepArrayBuffer], {type: 'application/step'});
		downloadBlob(blob, `tubesheet_${params.boardDiameter}mm.step`);

Conversation

YurMil commented Jul 17, 2026

🔴 Stability (f7b1cab)

🟠 Calculations (75f8859)

🟡 UI (4647192)

⚪ CI (b083b1d)

Verification

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

chatgpt-codex-connector Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

🔴 Stability (`f7b1cab`)

🟠 Calculations (`75f8859`)

🟡 UI (`4647192`)

⚪ CI (`b083b1d`)