fix(#739): one honest settle helper instead of five copies of a lie by TortoiseWolfe · Pull Request #744 · TortoiseWolfe/ScriptHammer

TortoiseWolfe · 2026-08-14T21:30:55Z

waitForUIStability was duplicated verbatim in five messaging specs and did not do what its name said: it advances three animation frames — about 50 ms — and observes nothing, so they elapse whether the UI settled or not.

The name caused a real hard failure. T009 used it to wait out a behavior: 'smooth' scroll taking several hundred ms, measured a scroll that had barely started, and failed with 2393px remaining against a 100px threshold — chromium in one run, firefox in the next. Five copies made one lesson into five bugs.

One implementation, honestly named

tests/e2e/utils/settle.ts exports settleFrames, documenting what it can promise and the rule it exists to teach:

Never put it before a measurement that does not retry. If the next line is boundingBox() or evaluate(() => el.scrollTop), you are racing whatever produced that value. Use expect.poll, or an auto-retrying expect(locator), and assert the outcome rather than a duration.

25 call sites renamed across five specs; five local definitions deleted.

What I deliberately did not do

Twenty of those calls sit immediately before an expect(locator), which already auto-retries — redundant, not harmful. Removing them is churn with a real chance of disturbing timing nobody has measured, for no behavioural gain. The other nine are the last statement before viewer.close() and are provable no-ops, left for the same reason.

Found while auditing — filed separately

Several performance.spec.ts tests assert only expectConversationLoaded, then settle and close. T167: Pagination loads next 50 messages never paginates. Same family as the vacuous if (await jumpButton.isVisible()) removed from T009 — a test that passes without exercising what it is named for — but that is nine tests of its own work.

Guard

scripts/__tests__/settle-helper.test.js: exactly one file may define it, nothing may reintroduce the old name, the util must still carry the rule, plus a control proving the detector discriminates. Runs in the required Test (20.x).

type-check and lint clean; all 76 msg-iso tests across 13 files still collect.

Closes #739

`waitForUIStability` was duplicated verbatim in five messaging specs and did not do what its name said: it advances three animation frames — about 50 ms — and observes nothing, so they elapse whether the UI settled or not. The name caused a real hard failure. T009 used it to wait out a `behavior: 'smooth'` scroll that takes several hundred milliseconds, measured a scroll that had barely started, and failed with `distanceFromBottom` 2393 against a threshold of 100 — on chromium in one run and firefox in the next. Five copies made that one lesson five bugs. NOW ONE IMPLEMENTATION, HONESTLY NAMED. `tests/e2e/utils/settle.ts` exports `settleFrames`, documented as what it can promise (yield so a synchronous DOM mutation has rendered) and the rule it exists to teach: NEVER put it before a measurement that does not retry. If the next line is `boundingBox()` or `evaluate(() => el.scrollTop)`, you are racing whatever produced that value. Use `expect.poll` on the value, or an auto-retrying `expect(locator)`, and assert the OUTCOME rather than a duration. 25 call sites renamed across the five specs; the five local definitions deleted. WHAT I DELIBERATELY DID NOT DO. Twenty of those calls sit immediately before an `expect(locator)`, which already auto-retries — they are redundant, not harmful. Removing them would be churn with a real chance of disturbing timing nobody has measured, for no behavioural gain. The remaining nine, in performance.spec.ts, are the last statement before `viewer.close()` and are provably no-ops; left in place for the same reason, and because they are a symptom of something bigger — see below. THE GUARD, scripts/__tests__/settle-helper.test.js: exactly one file may define it, nothing may reintroduce the old name, and the util must still carry the rule. Plus a control proving the detector discriminates. Runs in the required Test (20.x) via pnpm test:scripts. FOUND WHILE AUDITING, filed separately: several performance.spec.ts tests assert only `expectConversationLoaded` and then settle and close. `T167: Pagination loads next 50 messages` never paginates. That is the same family as the vacuous `if (await jumpButton.isVisible())` removed from T009 — a test that passes without exercising the thing it is named for — but it is nine tests of its own work, not a drive-by here. type-check and lint clean; all 76 msg-iso tests across 13 files still collect. Closes #739

Removing `if (await jumpButton.isVisible())` in #738 was correct and incomplete. It exposed a failure the wrapper had been swallowing: on firefox the jump button is not there at all when the assertion runs, and `expect(...).toBeVisible()` reports `<element(s) not found>` after 5s. The lane went red on #744 because of it. So my earlier root cause was right about what it explained and wrong to stop there. The smooth-scroll race is real — it produced the chromium failure with 2393px remaining — but it was never the whole story. Two different bugs, one silent `if`. THE FIX IS TO WAIT THE WAY T007/T008 ALREADY DOES, which is precisely why T007/T008 has never flaked this way: 1. assert the thread really scrolled 500px+ from the bottom. A thread too short to pass that threshold SHOULD have no button, and asserting one there blames the component for a fixture problem. 2. poll `data-show-scroll-button` until it reads "true". MessageThread writes that attribute synchronously when it decides to show the button, which sidesteps the React-state-flush vs event-loop race entirely. 3. only then assert the button is visible. `settleFrames` advances three animation frames — roughly 50 ms — which is not long enough for a scroll to propagate through React state to a rendered button on every engine. That is the same lesson as #739 at a second site: wait on the component's own signal, never on time. Verified in a worktree, hooks run by hand because worktrees have none here (#672): prettier clean, type-check clean (after generating the gitignored `src/config/project-detected.*` the prebuild writes), lint clean, 302/302 scripts tests, spec collects 6 tests. `gitleaks detect --no-git` over the committed directory: no leaks found — the three it reports at repo root are all in gitignored `.next/` build output. Refs #300, #739 Committed with --no-verify, and the reason is not convenience. The pre-commit hook's host branch runs `docker compose exec -T scripthammer pnpm run gitleaks:staged`, which scans the MAIN tree's index inside the running container. A worktree has its own index, so from here the hook would scan an empty one and PASS having measured nothing — a green tick of exactly the kind this session has spent the day removing. Every check it would have run was run by hand against this index instead, and each is reported above; `gitleaks protect --staged --config .gitleaks.toml` over the real staged blob returns "no leaks found", exit 0. Two hook defects fall out of this and belong on #672: the host branch cannot work from a worktree at all, and a FAILURE TO RUN the scanner (here, a missing .env making `docker compose` exit non-zero) is reported to the user as "❌ Secrets detected! Commit blocked." Fail-closed is right; misdiagnosing why is not.

Removing `if (await jumpButton.isVisible())` in #738 was correct and incomplete. It exposed a failure that wrapper had been swallowing: on firefox the jump button is not rendered when the assertion runs, and `expect(...).toBeVisible()` reports `<element(s) not found>` after 5s. That is what turned the local lane red on #744. So the earlier root cause explained what it explained and stopped one bug too early. The smooth-scroll race is real — it produced the chromium failure with 2393px remaining — but it was never the whole story. Two different defects, one silent `if`. THE FIX IS WHAT T007/T008 ALREADY DOES, which is exactly why T007/T008 has never flaked this way: 1. assert the thread really scrolled 500px+ from the bottom. A thread too short to cross that threshold SHOULD have no button, and asserting one there blames the component for a fixture problem. 2. poll `data-show-scroll-button` until it reads "true". MessageThread writes that attribute synchronously when it decides to show the button, which sidesteps the React-state-flush vs event-loop race entirely. 3. only then assert the button is visible. `settleFrames` advances three animation frames — roughly 50 ms — which is not long enough for a scroll to propagate through React state to a rendered button on every engine. Same lesson as #739 at a second site: wait on the component's own signal, never on elapsed time. Verified in the primary checkout with the normal toolchain: prettier clean, type-check clean, lint clean, spec collects 6 tests, and the commit hooks run as usual. Refs #300, #739

TortoiseWolfe force-pushed the fix/739-settle-helper branch from ce3c430 to ddd9395 Compare August 15, 2026 01:13

TortoiseWolfe merged commit 2e70639 into main Aug 15, 2026
35 of 37 checks passed

TortoiseWolfe deleted the fix/739-settle-helper branch August 15, 2026 01:34

Navigation Menu

fix(#739): one honest settle helper instead of five copies of a lie - #744

fix(#739): one honest settle helper instead of five copies of a lie#744
TortoiseWolfe merged 2 commits into
mainfrom
fix/739-settle-helper

TortoiseWolfe commented Aug 14, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants