Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
TortoiseWolfe merged 2 commits into from
Aug 15, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ddd9395
fix(#739): one honest settle helper instead of five copies of a lie
TurtleWolfe Aug 14, 2026
69316cb
fix(#300): T009 had a SECOND failure hiding behind the vacuous guard
TurtleWolfe Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 99 additions & 0 deletions scripts/__tests__/settle-helper.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/**
* `settleFrames` must have exactly one implementation, and must not regain a name that
* promises more than it does (#739).
*
* It was `waitForUIStability`, copy-pasted into five messaging specs, and the name caused a
* real hard failure: T009 used it to wait out a `behavior: 'smooth'` scroll and measured a
* scroll that had barely started, failing with 2393px remaining against a 100px threshold —
* on chromium in one run and firefox in the next. It observes nothing; the frames elapse
* whether the UI settled or not.
*
* Five copies is also five places to fix when the lesson is next learned, which is why this
* asserts there is exactly one.
*/

'use strict';

const { describe, it } = require('node:test');
const assert = require('node:assert');
const fs = require('node:fs');
const path = require('node:path');

const REPO_ROOT = path.resolve(__dirname, '..', '..');
const E2E = path.join(REPO_ROOT, 'tests', 'e2e');
const UTIL = path.join(E2E, 'utils', 'settle.ts');

function walk(dir, acc = []) {
for (const e of fs.readdirSync(dir, { withFileTypes: true })) {
const full = path.join(dir, e.name);
if (e.isDirectory()) walk(full, acc);
else if (e.name.endsWith('.ts')) acc.push(full);
}
return acc;
}

describe('the settle helper has one honest implementation (#739)', () => {
const files = walk(E2E);

it('the scan found the spec tree', () => {
assert.ok(
files.length > 20,
`only ${files.length} .ts files under tests/e2e — stale path?`
);
});

it('exactly one file defines it', () => {
const definers = files
.filter((f) =>
/export async function settleFrames|async function settleFrames/.test(
fs.readFileSync(f, 'utf8')
)
)
.map((f) => f.replace(REPO_ROOT + '/', ''));
assert.deepStrictEqual(
definers,
['tests/e2e/utils/settle.ts'],
'settleFrames is defined somewhere other than the shared util, or in more than one ' +
'place. Five copies of its predecessor is what made #739 five bugs instead of one.'
);
});

it('nothing reintroduces the name that lied', () => {
const offenders = files
.filter((f) => f !== UTIL)
.filter((f) =>
/function waitForUIStability/.test(fs.readFileSync(f, 'utf8'))
)
.map((f) => f.replace(REPO_ROOT + '/', ''));
assert.deepStrictEqual(
offenders,
[],
'a helper called waitForUIStability is back. It cannot wait for stability — it ' +
'advances N animation frames and observes nothing. Use settleFrames, or better, ' +
'assert the outcome with expect.poll.'
);
});

it('the util documents the rule that caused #739', () => {
const src = fs.readFileSync(UTIL, 'utf8');
assert.ok(
/does not retry|expect\.poll/.test(src),
'the util no longer warns against putting it before a non-retrying measurement — ' +
'which is the single mistake that produced #739'
);
});

it('the control can fail — the definition detector works', () => {
const fake = 'export async function settleFrames(page) {}';
assert.ok(
/export async function settleFrames|async function settleFrames/.test(
fake
),
'the detector cannot see a definition it should see'
);
assert.ok(
!/function waitForUIStability/.test(fake),
'the detector false-positives'
);
});
});
32 changes: 6 additions & 26 deletions tests/e2e/messaging/gdpr-compliance.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/

import { test, expect, type Page, type Browser } from '@playwright/test';
import { settleFrames } from '../utils/settle';
import {
seedIsolatedConversation,
deleteIsolatedConversation,
Expand All @@ -29,27 +30,6 @@ import {
// and means a real account deletion only ever destroys this test's throwaway user.
test.describe.configure({ mode: 'parallel' });

/**
* Wait for UI to stabilize after navigation or interaction.
*/
async function waitForUIStability(page: Page) {
await page.waitForLoadState('domcontentloaded');
await page.waitForFunction(
() => {
return new Promise((resolve) => {
let stableFrames = 0;
const checkStability = () => {
stableFrames++;
if (stableFrames >= 3) resolve(true);
else requestAnimationFrame(checkStability);
};
requestAnimationFrame(checkStability);
});
},
{ timeout: 15000 }
);
}

/** A browser context opened on /account authenticated as the isolated viewer. */
interface OpenedAccount {
page: Page;
Expand Down Expand Up @@ -115,7 +95,7 @@ async function openAccountAsViewer(
state: 'visible',
timeout: 30000,
});
await waitForUIStability(page);
await settleFrames(page);
await dismissCookieBanner(page);

return { page, close: () => context.close() };
Expand Down Expand Up @@ -370,7 +350,7 @@ test.describe('GDPR Account Deletion', () => {

const modal = page.getByRole('dialog');
await expect(modal).toBeVisible();
await waitForUIStability(page);
await settleFrames(page);

// Use ID selector for the confirmation input (more reliable than label)
const confirmInput = page.locator('#confirmation-input');
Expand Down Expand Up @@ -439,7 +419,7 @@ test.describe('GDPR Account Deletion', () => {

const modal = page.getByRole('dialog');
await expect(modal).toBeVisible();
await waitForUIStability(page);
await settleFrames(page);

const confirmInput = page.locator('#confirmation-input');
const confirmButton = modal.getByRole('button', {
Expand Down Expand Up @@ -478,7 +458,7 @@ test.describe('GDPR Account Deletion', () => {

const modal = page.getByRole('dialog');
await expect(modal).toBeVisible();
await waitForUIStability(page);
await settleFrames(page);

const confirmInput = page.locator('#confirmation-input');
const confirmButton = modal.getByRole('button', {
Expand Down Expand Up @@ -527,7 +507,7 @@ test.describe('GDPR Account Deletion', () => {

const modal = page.getByRole('dialog');
await expect(modal).toBeVisible();
await waitForUIStability(page);
await settleFrames(page);

// Modal should have an accessible name (via aria-labelledby OR aria-label OR title)
const hasAriaLabelledBy =
Expand Down
32 changes: 6 additions & 26 deletions tests/e2e/messaging/message-editing.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/

import { test, expect, type Page } from '@playwright/test';
import { settleFrames } from '../utils/settle';
import {
fillMessageInput,
scrollThreadToBottom,
Expand Down Expand Up @@ -49,27 +50,6 @@ function forwardConsole(page: Page, label = 'browser') {
});
}

/**
* Wait for the UI to stabilize after navigation or interaction (3 stable frames).
*/
async function waitForUIStability(page: Page) {
await page.waitForLoadState('domcontentloaded');
await page.waitForFunction(
() => {
return new Promise((resolve) => {
let stableFrames = 0;
const checkStability = () => {
stableFrames++;
if (stableFrames >= 3) resolve(true);
else requestAnimationFrame(checkStability);
};
requestAnimationFrame(checkStability);
});
},
{ timeout: 15000 }
);
}

/**
* Send a message in the current conversation via the real encrypted UI path.
*
Expand Down Expand Up @@ -123,7 +103,7 @@ async function sendMessage(page: Page, message: string) {
await messageElement.scrollIntoViewIfNeeded();

// Wait for UI to stabilize after sending
await waitForUIStability(page);
await settleFrames(page);

// Small additional wait for React to fully render Edit/Delete buttons
// (buttons depend on isOwn and timestamp checks)
Expand Down Expand Up @@ -213,7 +193,7 @@ test.describe('Message Editing', () => {
await expect(editTextarea).not.toBeVisible({ timeout: 15000 });

// Wait for UI to stabilize after save (state update + re-render)
await waitForUIStability(viewer.page);
await settleFrames(viewer.page);

// Find the message bubble with edited content (updates in place)
const editedBubble = getMessageBubble(viewer.page, editedMessage);
Expand Down Expand Up @@ -372,7 +352,7 @@ test.describe('Message Deletion', () => {
await expect(modal).not.toBeVisible({ timeout: 10000 });

// Wait for UI to stabilize after deletion
await waitForUIStability(viewer.page);
await settleFrames(viewer.page);

// Either the message is removed OR replaced with "[Message deleted]"
const messageGone = viewer.page.getByText(messageToDelete);
Expand Down Expand Up @@ -464,7 +444,7 @@ test.describe('Message Deletion', () => {
await expect(modal).not.toBeVisible({ timeout: 10000 });

// Wait for UI to stabilize after deletion
await waitForUIStability(viewer.page);
await settleFrames(viewer.page);

// Either the message is removed OR replaced with "[Message deleted]"
const messageGone = viewer.page.getByText(messageToDelete);
Expand Down Expand Up @@ -762,7 +742,7 @@ test.describe('Accessibility', () => {
name: /Delete Message/i,
});
await expect(modal).toBeVisible();
await waitForUIStability(viewer.page);
await settleFrames(viewer.page);

// Button accessible names are "Cancel deletion" and "Confirm deletion"
const cancelButton = modal.getByRole('button', {
Expand Down
74 changes: 45 additions & 29 deletions tests/e2e/messaging/messaging-scroll.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { test, expect, Page } from '@playwright/test';
import { settleFrames } from '../utils/settle';
import {
dismissCookieBanner,
handleReAuthModal,
Expand All @@ -20,27 +21,6 @@ const PRIMARY_EMAIL = process.env.TEST_USER_PRIMARY_EMAIL;
const SCROLL_FIXTURE_MESSAGE_COUNT = 30;
let scrollFixture: ScrollFixture | null = null;

/**
* Wait for UI to stabilize after navigation or interaction
*/
async function waitForUIStability(page: Page) {
await page.waitForLoadState('domcontentloaded');
await page.waitForFunction(
() => {
return new Promise((resolve) => {
let stableFrames = 0;
const checkStability = () => {
stableFrames++;
if (stableFrames >= 3) resolve(true);
else requestAnimationFrame(checkStability);
};
requestAnimationFrame(checkStability);
});
},
{ timeout: 15000 }
);
}

/**
* Messaging Scroll E2E Tests
* Feature: 005-fix-messaging-scroll
Expand Down Expand Up @@ -117,7 +97,7 @@ async function clickFirstConversation(page: Page): Promise<void> {

// Wait for chat window to load after clicking
await page.waitForSelector('[data-testid="chat-window"]', { timeout: 10000 });
await waitForUIStability(page);
await settleFrames(page);
}

// Helper to check if element is in viewport
Expand Down Expand Up @@ -269,7 +249,7 @@ test.describe('Messaging Scroll - User Story 2: Scroll Through Messages', () =>
});

// Wait for scroll to complete
await waitForUIStability(page);
await settleFrames(page);

// Get input position after scroll
const afterScrollInputBox = await messageInput.boundingBox();
Expand Down Expand Up @@ -312,15 +292,15 @@ test.describe('Messaging Scroll - User Story 3: Jump to Bottom Button', () => {

const messageThread = page.locator('[data-testid="message-thread"]');
await expect(messageThread).toBeVisible({ timeout: 30000 });
await waitForUIStability(page);
await settleFrames(page);

// Scroll up more than 500px to trigger button
await messageThread.evaluate((el) => {
el.scrollTop = Math.max(0, el.scrollHeight - el.clientHeight - 600);
el.dispatchEvent(new Event('scroll', { bubbles: true }));
});

await waitForUIStability(page);
await settleFrames(page);

const jumpButton = page.locator('[data-testid="jump-to-bottom"]');

Expand Down Expand Up @@ -388,15 +368,51 @@ test.describe('Messaging Scroll - User Story 3: Jump to Bottom Button', () => {
el.dispatchEvent(new Event('scroll', { bubbles: true }));
});

await waitForUIStability(page);
await settleFrames(page);

const jumpButton = page.locator('[data-testid="jump-to-bottom"]');

// WAIT THE WAY T007/T008 DOES, WHICH IS WHY T007/T008 DOES NOT FLAKE.
//
// Removing the old `if (await jumpButton.isVisible())` wrapper — which made the whole
// test vacuous whenever the button was absent — exposed a SECOND failure hiding behind
// it: on firefox the button was simply not there yet, and `expect(...).toBeVisible()`
// reported `<element(s) not found>` after 5s.
//
// `settleFrames` advances three animation frames, roughly 50 ms. That is not long
// enough for the scroll to propagate through React state to a rendered button on every
// engine. T007/T008 solves this by waiting on the component's OWN signal instead of on
// time, and this now does the same.
//
// First: prove the thread really did scroll. A thread that is too short to pass the
// 500px threshold SHOULD have no button, and asserting the button in that case would be
// blaming the component for a fixture problem.
const scrollInfo = await messageThread.evaluate((el) => ({
distanceFromBottom: el.scrollHeight - (el.scrollTop + el.clientHeight),
}));
expect(
scrollInfo.distanceFromBottom,
'fixture thread is not tall enough to scroll 500px+ from the bottom, so the jump ' +
'button is correctly absent — this is a fixture failure, not a UI regression'
).toBeGreaterThan(500);

// Then: the attribute MessageThread writes synchronously when it decides to show the
// button, which sidesteps the React-state-flush vs event-loop race entirely.
const wrapper = page.locator('[data-show-scroll-button]').first();
await expect
.poll(async () => await wrapper.getAttribute('data-show-scroll-button'), {
message:
'MessageThread never set data-show-scroll-button="true" after scrolling to the ' +
'top — the component did not register the scroll',
timeout: 5000,
intervals: [50, 100, 200, 500],
})
.toBe('true');

// NOT `if (await jumpButton.isVisible())`. The whole body used to sit inside that
// condition, so a thread where the button never appeared passed having asserted
// nothing — and "the jump button stopped rendering" is precisely what this test is
// named for. T007/T008 above already establish it appears when scrolled up, so here
// it is a requirement, not a precondition.
// named for. It is a requirement here, not a precondition.
await expect(jumpButton).toBeVisible();

await jumpButton.click();
Expand All @@ -405,7 +421,7 @@ test.describe('Messaging Scroll - User Story 3: Jump to Bottom Button', () => {
//
// This is what made T009 flaky, and it is not a browser quirk: the button calls
// `scrollToBottom(true)`, i.e. `behavior: 'smooth'` (MessageThread.tsx:239-243), while
// `waitForUIStability` waits three animation frames — about 50 ms. A smooth scroll from
// `settleFrames` (then named `waitForUIStability`) waits three animation frames — about 50 ms. A smooth scroll from
// the top of a 30-message thread takes several hundred. So the assertion measured a
// scroll that had barely started, and failed with **2393px** remaining rather than
// marginally over the 100px threshold.
Expand Down
Loading
Loading