From 98c0d5a94d91f2ebe6c54e2aa9af9a8fe652cf88 Mon Sep 17 00:00:00 2001 From: matthewtrahan Date: Wed, 14 Oct 2026 21:25:09 -0500 Subject: [PATCH] suggest: review fixes -- a ranking in flight is dropped (worker terminated) by whatever clears the list (new part, undo, reset), a message error falls back inline, and the test ranks a torus whose answer the gap really changes --- tests/README.md | 6 +++--- tests/suggest_worker.test.js | 18 ++++++++++++------ web/ui/suggest.js | 30 +++++++++++++++++++++--------- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/tests/README.md b/tests/README.md index 75d5b27..974194b 100644 --- a/tests/README.md +++ b/tests/README.md @@ -104,9 +104,9 @@ first version added a squat wall 1.6 mm from the one main builds (review). which crossed a cube's edge). **`suggest_worker.test.js`** -- Suggest orientation's Worker (`web/suggestworker.js`) ranks -**exactly** as `suggestOrientations` does on this thread, with the default settings and -with a changed Support gap sent as tunables, and the gap doesn't leak back into this -thread's modules. +**exactly** as `suggestOrientations` does on this thread, on a torus with the default +settings and with a changed Support gap sent as tunables -- and the two rankings +differ, so the gap really reached the Worker. **`orient.test.js`** -- the orientation/strength logic behind the left rail (pure, no DOM), so a change to a verdict or a solver can't silently drift: diff --git a/tests/suggest_worker.test.js b/tests/suggest_worker.test.js index cded786..1e1b3da 100644 --- a/tests/suggest_worker.test.js +++ b/tests/suggest_worker.test.js @@ -1,8 +1,8 @@ // Suggest orientation runs in a Worker (web/suggestworker.js), with the page's // tunables applied first. Its ranking has to be the one the page used to compute -// itself -- with the default settings and with a changed Support gap, which moves -// the finalists' fin counts -- and the Worker's module copy must not leak the -// gap into this thread's. +// itself, with the default settings and with a changed Support gap -- on a torus, +// whose finalists all take walls, so the gap really changes the answer (checked +// below, or the test would prove nothing about the tunables reaching the Worker). import { WEB, assert, loadModel } from './_util.js'; const { suggestOrientations } = await import(`${WEB}orient.js`); @@ -18,12 +18,13 @@ function inWorker(topo, threshold, tunables) { }); } +const ranked = {}; for (const gap of [undefined, 0.35]) { - Deno.test(`suggest worker ranks exactly as the page did (lbracket, gap ${gap ?? 'default'})`, async () => { - const topo = loadModel('lbracket'); + Deno.test(`suggest worker ranks exactly as the page did (torus, gap ${gap ?? 'default'})`, async () => { + const topo = loadModel('torus'); const before = PROP.gap; const got = await inWorker(topo, 45, gap === undefined ? undefined : { propGap: gap }); - assert(PROP.gap === before, 'the Worker\'s gap leaked into this thread'); + ranked[gap ?? 'default'] = JSON.stringify(got); try { if (gap !== undefined) applyTunables({ propGap: gap }); const want = suggestOrientations(topo, { top: 3, threshold: 45 }); @@ -34,3 +35,8 @@ for (const gap of [undefined, 0.35]) { } }); } + +Deno.test('the changed gap changes the torus ranking (so the tests above test the tunables)', () => { + assert(ranked.default && ranked['0.35'], 'run with the two tests above'); + assert(ranked.default !== ranked['0.35'], 'gap 0.35 ranked the torus exactly like 0.2'); +}); diff --git a/web/ui/suggest.js b/web/ui/suggest.js index 88aab31..3c8292c 100644 --- a/web/ui/suggest.js +++ b/web/ui/suggest.js @@ -112,6 +112,9 @@ function renderSuggestions() { * invalidates the ranking). The disclosure chevron does NOT come through here -- * it only collapses/expands what's already there. */ export function hideSuggestions() { + // A ranking still running belongs to what is being cleared (a new part, an undo, + // a reset): drop it, so its list doesn't pop up after the action that cleared it. + cancelRanking?.(); el('suggest-list').hidden = true; el('suggest-list').replaceChildren(); const note = el('suggest-note'); @@ -137,29 +140,36 @@ export function clearSuggestionMark() { * can run (file://, a clone or worker failure) it ranks here, as it always did. */ let rankId = 0; +let cancelRanking = null; // drops the ranking in flight; it then resolves to null function rankPoses(topo, thr) { + let cancelled = false; const inline = () => new Promise((resolve, reject) => { // let the button repaint before the solve blocks the thread requestAnimationFrame(() => requestAnimationFrame(() => { + if (cancelled) { resolve(null); return; } try { resolve(suggestOrientations(topo, { top: 3, threshold: thr })); } catch (err) { reject(err); } })); }); - let w; + let w = null; try { w = new Worker(new URL('../suggestworker.js', import.meta.url), { type: 'module' }); - } catch { - return inline(); - } + } catch { /* no Worker here: rank inline */ } const id = ++rankId; return new Promise((resolve) => { - const fallBack = () => { w.terminate(); resolve(inline()); }; + const cancel = () => { cancelled = true; finish(null); }; + const done = () => { if (cancelRanking === cancel) cancelRanking = null; }; + const finish = (v) => { done(); w?.terminate(); resolve(v); }; + // inline stays cancellable until it runs: it checks `cancelled` first + const fallBack = () => { w?.terminate(); w = null; resolve(inline().finally(done)); }; + cancelRanking = cancel; + if (!w) { fallBack(); return; } w.onmessage = (e) => { if (e.data.id !== id) return; if (e.data.error) { console.warn('suggest worker failed; ranking inline', e.data.error); fallBack(); return; } - w.terminate(); - resolve(e.data.ranked); + finish(e.data.ranked); }; w.onerror = () => fallBack(); + w.onmessageerror = () => fallBack(); // the inside grid holds a closure structured clone can't copy (finbuild.js) const topoMsg = { ...topo }; delete topoMsg._insideGrid; @@ -180,8 +190,10 @@ el('suggest-orient').addEventListener('click', () => { // Walls the current pose stands inside a bore -- the baseline the "points the // bores up" verdict measures its win against. const curBore = (lastBuilt?.props ?? []).filter((q) => q.inBore).length; - rankPoses(topo, threshold).then(({ candidates, confidence }) => { - if (topo !== topology) return; // a different part was loaded while it ranked + rankPoses(topo, threshold).then((ranked) => { + // cancelled (hideSuggestions), or a different part was loaded while it ranked + if (!ranked || topo !== topology) return; + const { candidates, confidence } = ranked; suggestions = candidates; suggestCurBore = curBore; // Fresh results always land expanded, with the collapse chevron available.