Skip to content

Commit 4dfa1be

Browse files
pratikbuildspratikbuilds
committed
fix(conversation): hold the history window until the view is read
An item committed synchronously in the frame the reader scrolled up in, before the conversation's scroll pass, slid or trimmed the window: atBottom and the freeze were still a frame old, so the oldest item was removed while the reader was scrolled up. A new item now commits with nothing removed. A layout effect then trims or slides the window as before, unless the view has moved towards the top since the last scroll snapshot; the next pass then freezes it if the scroll was the reader's. The snapshot subscription moves to a layout effect so the one sent when the container mounts is not missed. A loader that throws now stalls like one that rejects, and its error is reported once instead of escaping the marker's observer callback. The scroll helpers both conversation specs copied now live in one module.
1 parent 76ccecc commit 4dfa1be

5 files changed

Lines changed: 205 additions & 173 deletions

File tree

‎e2e/conversation-helpers.ts‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
// Reads and drives the conversation scroll container the e2e scenarios render.
2+
import { expect, type Page } from "@playwright/test";
3+
4+
export const container = (page: Page) =>
5+
page.locator("[data-slot='conversation-content']");
6+
7+
export const view = (page: Page) =>
8+
container(page).evaluate((element) => ({
9+
scrollTop: element.scrollTop,
10+
top: element.getBoundingClientRect().top,
11+
distanceFromEnd:
12+
element.scrollHeight - element.scrollTop - element.clientHeight,
13+
}));
14+
15+
/** How far an item's top sits below the top of the view. */
16+
export const offsetOf = async (page: Page, id: string) => {
17+
const { top } = await view(page);
18+
const itemTop = await page
19+
.getByTestId(id)
20+
.evaluate((element) => element.getBoundingClientRect().top);
21+
return itemTop - top;
22+
};
23+
24+
/** Waits for the view to stop moving (a glide or a wheel scroll to land). */
25+
export const settled = async (page: Page) => {
26+
let previous = Number.NaN;
27+
await expect
28+
.poll(
29+
async () => {
30+
const { scrollTop } = await view(page);
31+
const still = scrollTop === previous;
32+
previous = scrollTop;
33+
return still;
34+
},
35+
{ intervals: [100] },
36+
)
37+
.toBe(true);
38+
};
39+
40+
/** The reader's own scroll: a wheel over the conversation. */
41+
export const wheel = async (page: Page, deltaY: number) => {
42+
const box = await container(page).boundingBox();
43+
if (box === null) throw new Error("expected the conversation on screen");
44+
await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2);
45+
await page.mouse.wheel(0, deltaY);
46+
await settled(page);
47+
};
48+
49+
export const atEnd = async (page: Page) => {
50+
await expect
51+
.poll(async () => (await view(page)).distanceFromEnd)
52+
.toBeLessThanOrEqual(1);
53+
};
54+
55+
/** The id of the topmost item in view. */
56+
export const firstVisible = (page: Page) =>
57+
container(page).evaluate((element) => {
58+
const top = element.getBoundingClientRect().top;
59+
const items = element.querySelectorAll<HTMLElement>("[data-testid]");
60+
for (const item of items) {
61+
if (item.getBoundingClientRect().bottom > top)
62+
return item.dataset.testid ?? "";
63+
}
64+
return "";
65+
});
66+
67+
/** Two frames: one for resize callbacks, one for any correction to land. */
68+
export const frames = (page: Page) =>
69+
page.evaluate(
70+
() =>
71+
new Promise((resolve) =>
72+
requestAnimationFrame(() => requestAnimationFrame(resolve)),
73+
),
74+
);

‎e2e/conversation.spec.ts‎

Lines changed: 11 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -1,67 +1,15 @@
11
import { expect, test, type Page } from "@playwright/test";
22

3-
const container = (page: Page) =>
4-
page.locator("[data-slot='conversation-content']");
5-
6-
const view = (page: Page) =>
7-
container(page).evaluate((element) => ({
8-
scrollTop: element.scrollTop,
9-
top: element.getBoundingClientRect().top,
10-
distanceFromEnd:
11-
element.scrollHeight - element.scrollTop - element.clientHeight,
12-
}));
13-
14-
/** How far an item's top sits below the top of the view. */
15-
const offsetOf = async (page: Page, id: string) => {
16-
const { top } = await view(page);
17-
const itemTop = await page
18-
.getByTestId(id)
19-
.evaluate((element) => element.getBoundingClientRect().top);
20-
return itemTop - top;
21-
};
22-
23-
/** Waits for the view to stop moving (a glide or a wheel scroll to land). */
24-
const settled = async (page: Page) => {
25-
let previous = Number.NaN;
26-
await expect
27-
.poll(
28-
async () => {
29-
const { scrollTop } = await view(page);
30-
const still = scrollTop === previous;
31-
previous = scrollTop;
32-
return still;
33-
},
34-
{ intervals: [100] },
35-
)
36-
.toBe(true);
37-
};
38-
39-
/** The reader's own scroll: a wheel over the conversation. */
40-
const wheel = async (page: Page, deltaY: number) => {
41-
const box = await container(page).boundingBox();
42-
if (box === null) throw new Error("expected the conversation on screen");
43-
await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2);
44-
await page.mouse.wheel(0, deltaY);
45-
await settled(page);
46-
};
47-
48-
const atEnd = async (page: Page) => {
49-
await expect
50-
.poll(async () => (await view(page)).distanceFromEnd)
51-
.toBeLessThanOrEqual(1);
52-
};
53-
54-
/** The id of the topmost item in view. */
55-
const firstVisible = (page: Page) =>
56-
container(page).evaluate((element) => {
57-
const top = element.getBoundingClientRect().top;
58-
const items = element.querySelectorAll<HTMLElement>("[data-testid]");
59-
for (const item of items) {
60-
if (item.getBoundingClientRect().bottom > top)
61-
return item.dataset.testid ?? "";
62-
}
63-
return "";
64-
});
3+
import {
4+
atEnd,
5+
container,
6+
firstVisible,
7+
frames,
8+
offsetOf,
9+
settled,
10+
view,
11+
wheel,
12+
} from "./conversation-helpers.js";
6513

6614
/** Opens a scenario once its history (which arrives after mount) is in. */
6715
const open = async (page: Page, scenario: string) => {
@@ -74,13 +22,7 @@ const write = async (page: Page, times: number) => {
7422
for (let count = 0; count < times; count += 1) {
7523
await page.getByRole("button", { name: "Write" }).click();
7624
}
77-
// One frame for the resize callback, one for any correction to land.
78-
await page.evaluate(
79-
() =>
80-
new Promise((resolve) =>
81-
requestAnimationFrame(() => requestAnimationFrame(resolve)),
82-
),
83-
);
25+
await frames(page);
8426
};
8527

8628
test.describe("settle", () => {

‎e2e/history-window.spec.ts‎

Lines changed: 10 additions & 71 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,15 @@
11
import { expect, test, type Page } from "@playwright/test";
22

3-
const container = (page: Page) =>
4-
page.locator("[data-slot='conversation-content']");
3+
import {
4+
atEnd,
5+
container,
6+
firstVisible,
7+
frames,
8+
offsetOf,
9+
settled,
10+
view,
11+
wheel,
12+
} from "./conversation-helpers.js";
513

614
const marker = (page: Page) =>
715
page.locator("[data-slot='conversation-earlier']");
@@ -10,75 +18,6 @@ const shown = (page: Page) => page.getByRole("status", { name: "Shown" });
1018

1119
const requests = (page: Page) => page.getByRole("status", { name: "Requests" });
1220

13-
const view = (page: Page) =>
14-
container(page).evaluate((element) => ({
15-
scrollTop: element.scrollTop,
16-
top: element.getBoundingClientRect().top,
17-
distanceFromEnd:
18-
element.scrollHeight - element.scrollTop - element.clientHeight,
19-
}));
20-
21-
/** How far an item's top sits below the top of the view. */
22-
const offsetOf = async (page: Page, id: string) => {
23-
const { top } = await view(page);
24-
const itemTop = await page
25-
.getByTestId(id)
26-
.evaluate((element) => element.getBoundingClientRect().top);
27-
return itemTop - top;
28-
};
29-
30-
/** Waits for the view to stop moving (a glide or a wheel scroll to land). */
31-
const settled = async (page: Page) => {
32-
let previous = Number.NaN;
33-
await expect
34-
.poll(
35-
async () => {
36-
const { scrollTop } = await view(page);
37-
const still = scrollTop === previous;
38-
previous = scrollTop;
39-
return still;
40-
},
41-
{ intervals: [100] },
42-
)
43-
.toBe(true);
44-
};
45-
46-
/** The reader's own scroll: a wheel over the conversation. */
47-
const wheel = async (page: Page, deltaY: number) => {
48-
const box = await container(page).boundingBox();
49-
if (box === null) throw new Error("expected the conversation on screen");
50-
await page.mouse.move(box.x + box.width / 2, box.y + box.height / 2);
51-
await page.mouse.wheel(0, deltaY);
52-
await settled(page);
53-
};
54-
55-
const atEnd = async (page: Page) => {
56-
await expect
57-
.poll(async () => (await view(page)).distanceFromEnd)
58-
.toBeLessThanOrEqual(1);
59-
};
60-
61-
/** Two frames: one for resize callbacks, one for any correction to land. */
62-
const frames = (page: Page) =>
63-
page.evaluate(
64-
() =>
65-
new Promise((resolve) =>
66-
requestAnimationFrame(() => requestAnimationFrame(resolve)),
67-
),
68-
);
69-
70-
/** The id of the topmost item in view. */
71-
const firstVisible = (page: Page) =>
72-
container(page).evaluate((element) => {
73-
const top = element.getBoundingClientRect().top;
74-
const items = element.querySelectorAll<HTMLElement>("[data-testid]");
75-
for (const item of items) {
76-
if (item.getBoundingClientRect().bottom > top)
77-
return item.dataset.testid ?? "";
78-
}
79-
return "";
80-
});
81-
8221
const open = async (page: Page, scenario: string, last: string) => {
8322
await page.goto(`/${scenario}`);
8423
await expect(page.getByTestId(last)).toBeVisible();

‎src/hooks/use-history-window.test.tsx‎

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ type Harness = {
2424
readonly commits: () => number;
2525
update(options: UseHistoryWindowOptions<Row>): void;
2626
leaveBottom(): Promise<void>;
27+
/** The reader scrolls away from the end, and the next frame has not run. */
28+
leaveBottomUnseen(): void;
2729
/** The view moves away from the end with no reader input (layout, a glide). */
2830
driftFromBottom(): Promise<void>;
2931
returnToBottom(): Promise<void>;
@@ -110,11 +112,14 @@ function mount(
110112
configurable: true,
111113
value: 500,
112114
});
113-
/** A scroll to `top`, landing on the next frame. */
114-
const scrollTo = async (top: number, byReader: boolean) => {
115+
const scroll = (top: number, byReader: boolean) => {
115116
if (byReader) scroller.dispatchEvent(new WheelEvent("wheel"));
116117
scroller.scrollTop = top;
117118
scroller.dispatchEvent(new Event("scroll"));
119+
};
120+
/** A scroll to `top`, landing on the next frame. */
121+
const scrollTo = async (top: number, byReader: boolean) => {
122+
scroll(top, byReader);
118123
await nextFrame();
119124
};
120125

@@ -123,6 +128,7 @@ function mount(
123128
commits: () => commits,
124129
update: (options) => act(() => render(options)),
125130
leaveBottom: () => scrollTo(1_000, true),
131+
leaveBottomUnseen: () => scroll(1_000, true),
126132
driftFromBottom: () => scrollTo(1_000, false),
127133
returnToBottom: () => scrollTo(99_500, true),
128134
setHidden: (next) => {
@@ -215,6 +221,24 @@ describe("useHistoryWindow with in-memory history", () => {
215221
expect(first(harness.result.current)).toBe("r181");
216222
});
217223

224+
test("an item that lands before the frame the reader left in removes nothing", async () => {
225+
const harness = mount({ items: rows(0, 300) });
226+
const { result } = harness;
227+
await harness.returnToBottom();
228+
// A host that commits synchronously within the frame (flushSync in a
229+
// frame-batched stream) lands its item before the frame's scroll pass.
230+
harness.leaveBottomUnseen();
231+
harness.update({ items: rows(0, 301) });
232+
expect(first(result.current)).toBe("r180");
233+
expect(result.current.items).toHaveLength(121);
234+
235+
// The frame then sees the reader's scroll and freezes the window.
236+
await nextFrame();
237+
harness.update({ items: rows(0, 302) });
238+
expect(first(result.current)).toBe("r180");
239+
expect(result.current.items).toHaveLength(122);
240+
});
241+
218242
test("a removed start item keeps the rest of the window", async () => {
219243
const harness = mount({ items: rows(0, 300) });
220244
await harness.leaveBottom();
@@ -371,6 +395,31 @@ describe("useHistoryWindow with server paging", () => {
371395
expect(result.current.stalled).toBe(true);
372396
});
373397

398+
test("a loader that throws stalls and reports the error once", async () => {
399+
const reported: unknown[] = [];
400+
const original = globalThis.reportError;
401+
globalThis.reportError = (error: unknown) => {
402+
reported.push(error);
403+
};
404+
try {
405+
const failure = new Error("offline");
406+
const { result } = mount({
407+
items: rows(250, 300),
408+
hasEarlier: true,
409+
onLoadEarlier: () => {
410+
throw failure;
411+
},
412+
});
413+
// Reached from the marker's observer callback: nothing may escape it.
414+
await act(async () => result.current.reachStart());
415+
expect(reported).toEqual([failure]);
416+
expect(result.current.loading).toBe(false);
417+
expect(result.current.stalled).toBe(true);
418+
} finally {
419+
globalThis.reportError = original;
420+
}
421+
});
422+
374423
test("a load that settles while hidden by Activity is not lost", async () => {
375424
const load = deferred();
376425
let options: UseHistoryWindowOptions<Row> = {

0 commit comments

Comments
 (0)