Skip to content
This repository was archived by the owner on Nov 4, 2026. It is now read-only.

Commit a1ddb5b

Browse files
os-justinclaude
andauthored
test(plugin-map,plugin-timeline): pin what these two waits were standing on (#8713)
Both `contractEnvelope-6839` pins were green for reasons their wait expressions did not state. `ObjectMap` carries the family's only absence-shaped wait — `queryByText('Loading map...')` is null. An absence nothing entered is satisfied by a mount that never started: with the component mutated to `return null` unconditionally, an implementation strictly worse than the bug, the refusal case still passed. And the panel is a settle signal, not a rows signal — it stands in for "the rows are on screen" only because `setData` and `setLoading(false)` land in one React commit, which nothing asserted. With `setData` deferred by 50ms and the `records` arm restored to `extractRecords`, the refusal case read zero markers and passed while the settled map plotted two. `ObjectTimeline` waited for `getByTestId('timeline-renderer')`'s `data-item-count` to be not-null. That clause is inert: the renderer double writes the attribute unconditionally, so once `getByTestId` stops throwing the attribute is always a string, and the throw was the entire gate. It named a row count and gated on a mount, so `"0"` satisfied it — under the same deferral the wait was satisfied at `data-item-count="0"` while the settled timeline drew two rows. `markersThrough` now asserts the loading panel is on screen before waiting for it to go, and that the marker count read at the transition survives a 50ms settle window. `itemsThrough` takes the count its arm claims, gates the wait on that value, requires it to survive the same window, and observes the loading skeleton first so a renderer mounted empty on the first paint is not read as a settled zero. 50ms rather than 0ms because RTL's `asyncWrapper` drains one macrotask, so a 0ms deferral hides inside the window meant to detect it. Test-only; no component was changed. Refs objectui#8709 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S Co-authored-by: Claude <noreply@anthropic.com>
1 parent 5ece49b commit a1ddb5b

3 files changed

Lines changed: 159 additions & 10 deletions

File tree

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
---
3+
4+
Test-only (no release). `ObjectMap.contractEnvelope-6839` and
5+
`ObjectTimeline.contractEnvelope-6839` each waited on something that could not
6+
fail: the map's absence-shaped wait was satisfied by a mount that never showed
7+
the loading panel and by a `setData` that had not committed yet, and the
8+
timeline's `data-item-count` not-null clause was inert — `getByTestId` throwing
9+
was the whole gate, so `"0"` satisfied it. Both waits now observe the
10+
transition and gate on a settled row count. No package source changed.

‎packages/plugin-map/src/ObjectMap.contractEnvelope-6839.test.tsx‎

Lines changed: 66 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,30 @@
3434
* ⚠️ The refusal case is ALSO satisfied by an `extractRecords` that returns
3535
* `[]` for everything — an implementation strictly worse than the bug. The
3636
* `data` and bare-array cases refuse it: same rows, same mount.
37+
*
38+
* ## The two things this file's wait used to STAND ON without saying so (objectui#8709)
39+
*
40+
* This is the family's only ABSENCE-shaped wait, and an absence carries two
41+
* unstated preconditions. Both are now assertions in `markersThrough`, and both
42+
* were measured failing first:
43+
*
44+
* 1. **The panel was ever on screen.** An absence nothing entered is
45+
* satisfied by a mount that never started. With the component mutated to
46+
* `return null` unconditionally the refusal case PASSED — "render nothing,
47+
* ever" is strictly worse than the bug and cleared the old bar.
48+
* 2. **`setData` and `setLoading(false)` commit TOGETHER.** The panel is a
49+
* settle signal, not a rows signal; it is only a usable proxy for "the
50+
* rows are on screen" because the two `setState` calls land in one commit.
51+
* With `setData` deferred by 50ms and the `records` arm restored to
52+
* `extractRecords`, the refusal case read zero markers and PASSED while
53+
* the settled map plotted two.
54+
*
55+
* ⭐ Neither gate subsumes the other: (1) alone still lets a split commit
56+
* through, and (2) alone still passes a component that renders nothing.
3757
*/
3858

3959
import React from 'react';
40-
import { render, screen, waitFor, cleanup } from '@testing-library/react';
60+
import { act, render, screen, waitFor, cleanup } from '@testing-library/react';
4161
import { describe, it, expect, vi, afterEach } from 'vitest';
4262

4363
// The MapLibre canvas, replaced by DOM the test can count. Copied from
@@ -72,6 +92,17 @@ const schema: any = {
7292
data: { provider: 'object', object: 'store' },
7393
};
7494

95+
/**
96+
* The window held open AFTER the loading panel clears, to prove no markers
97+
* arrive behind it.
98+
*
99+
* ⚠️ 50ms, not 0ms. RTL's `asyncWrapper` drains one macrotask before it
100+
* returns, so a commit scheduled with `setTimeout(…, 0)` lands INSIDE that
101+
* drain window — a 0ms window cannot tell a deferred commit from a
102+
* same-commit one, and reports "stable" for both (measured on objectui#8664).
103+
*/
104+
const POST_SETTLE_MS = 50;
105+
75106
/** How one case wraps its rows on the way back out of `find()`. */
76107
type Envelope = (rows: unknown[]) => unknown;
77108

@@ -105,6 +136,17 @@ async function markersThrough(envelope: Envelope): Promise<number> {
105136
})),
106137
};
107138
render(<ObjectMap schema={schema} dataSource={ds} enableClustering={false} />);
139+
// ① The transition's START, observed — the half an ABSENCE-shaped wait cannot
140+
// supply for itself. "The panel has cleared" and "nothing was ever rendered"
141+
// are the SAME DOM, so without this line the wait below is satisfied by a
142+
// mount that never started. MEASURED, not argued: with `ObjectMap` mutated to
143+
// `return null` unconditionally — an implementation strictly worse than the
144+
// bug — the refusal case below still PASSED (objectui#8709 leg B). It does
145+
// not survive this line.
146+
expect(
147+
screen.getByText('Loading map...'),
148+
'the loading panel must be on screen BEFORE the absence wait — an absence nothing entered is satisfied by never having started',
149+
).toBeInTheDocument();
108150
await waitFor(() => expect(find).toHaveBeenCalled());
109151
// `find`'s OWN answer, settled — a pure read of the mock's call record that
110152
// touches no DOM. Without it "no markers" is satisfied by the mount's
@@ -114,7 +156,29 @@ async function markersThrough(envelope: Envelope): Promise<number> {
114156
// rather than a rows signal, which is exactly what makes it usable as the
115157
// one wait shared by the live cases and the refusal case.
116158
await waitFor(() => expect(screen.queryByText('Loading map...')).toBeNull());
117-
return screen.queryAllByTestId('map-marker').length;
159+
const atPanelClear = screen.queryAllByTestId('map-marker').length;
160+
// ② The unstated precondition the line above STANDS ON, now pinned:
161+
// `setData(capped.rows)` and `setLoading(false)` reach the DOM in ONE commit.
162+
// React 18 batches them today because they sit in one `await` continuation —
163+
// true, load-bearing, and asserted by nothing else in this repo. The day they
164+
// split (a transition, an async boundary, a `startTransition`) the panel
165+
// clears over an EMPTY `data` and the count read above is the INTERMEDIATE
166+
// state. MEASURED: with `setData` deferred by 50ms and the `records` arm
167+
// restored to `extractRecords`, the refusal case read zero markers and PASSED
168+
// while the settled map plotted two — the bug went undetected (leg A).
169+
await act(async () => {
170+
await new Promise((resolve) => setTimeout(resolve, POST_SETTLE_MS));
171+
});
172+
// A dependency landing mid-window (`objectSchema`, `perms`) re-runs the fetch
173+
// effect and re-raises the panel, so re-settle before re-reading: the claim is
174+
// about the SETTLED count, not about a moment inside a refetch.
175+
await waitFor(() => expect(screen.queryByText('Loading map...')).toBeNull());
176+
const settled = screen.queryAllByTestId('map-marker').length;
177+
expect(
178+
settled,
179+
'markers must not arrive AFTER the loading panel clears — the count read at the transition must already be the settled one',
180+
).toBe(atPanelClear);
181+
return settled;
118182
}
119183

120184
afterEach(() => {

‎packages/plugin-timeline/src/ObjectTimeline.contractEnvelope-6839.test.tsx‎

Lines changed: 83 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,10 +32,27 @@
3232
* The `data` and bare-array cases are the ones that refuse it: they push the
3333
* SAME rows through the SAME mount, so an arm that delivers nothing delivered
3434
* nothing because the envelope was refused.
35+
*
36+
* ## What this file's wait used to STAND ON without saying so (objectui#8709)
37+
*
38+
* The wait named `data-item-count` and did not gate on it. `getByTestId`
39+
* THROWING inside `waitFor` was the entire gate; by the time the attribute was
40+
* read the node existed and the renderer double writes the attribute
41+
* unconditionally, so `.not.toBeNull()` could never be the clause that failed.
42+
* ⭐ That is worse than no clause: it reads like a row-count gate and is a
43+
* mount signal, satisfied the instant the renderer appears — `"0"` included.
44+
*
45+
* MEASURED, not argued: with `setFetchedData` deferred by 50ms and the
46+
* `records` arm restored to `extractRecords`, the wait was satisfied at
47+
* `data-item-count="0"` and the refusal case PASSED while the settled timeline
48+
* drew two rows. `itemsThrough` now gates on the count the wait names AND on
49+
* that count surviving a settle window, with the loading skeleton observed
50+
* first so a renderer mounted empty on the first paint is not read as a
51+
* settled zero.
3552
*/
3653

3754
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
38-
import { render, waitFor, cleanup, screen } from '@testing-library/react';
55+
import { act, render, waitFor, cleanup, screen } from '@testing-library/react';
3956
import React from 'react';
4057

4158
// The timeline's own visualisation is orthogonal to what this file observes
@@ -55,6 +72,17 @@ const ROWS = [
5572
{ id: 't2', subject: 'Ship it again', starts_at: '2026-01-02T09:00:00Z' },
5673
];
5774

75+
/**
76+
* The window held open AFTER the renderer reports the expected count, to prove
77+
* no further rows arrive behind it.
78+
*
79+
* ⚠️ 50ms, not 0ms. RTL's `asyncWrapper` drains one macrotask before it
80+
* returns, so a commit scheduled with `setTimeout(…, 0)` lands INSIDE that
81+
* drain window — a 0ms window cannot tell a deferred commit from a same-commit
82+
* one, and reports "stable" for both (measured on objectui#8664).
83+
*/
84+
const POST_SETTLE_MS = 50;
85+
5886
/** How one case wraps its rows on the way back out of `find()`. */
5987
type Envelope = (rows: unknown[]) => unknown;
6088

@@ -69,8 +97,14 @@ const schema: any = {
6997
startDateField: 'starts_at',
7098
};
7199

72-
/** Mount the timeline over a `find()` answering `envelope`, return rows drawn. */
73-
async function itemsThrough(envelope: Envelope): Promise<number> {
100+
/**
101+
* Mount the timeline over a `find()` answering `envelope`, return rows drawn.
102+
*
103+
* `expectedItems` is the count this arm claims the envelope produces, and it is
104+
* what the wait GATES on — see the comments in the body for why the count has
105+
* to be named here rather than merely read at the end.
106+
*/
107+
async function itemsThrough(envelope: Envelope, expectedItems: number): Promise<number> {
74108
const ds: Record<string, any> = {
75109
find: vi.fn(async () => envelope(ROWS)),
76110
findOne: vi.fn(),
@@ -87,15 +121,56 @@ async function itemsThrough(envelope: Envelope): Promise<number> {
87121
})),
88122
};
89123
render(<ObjectTimeline schema={schema} dataSource={ds as any} />);
124+
// ① The transition's START, observed. Without it a component that mounts the
125+
// renderer EMPTY on its first paint and fetches afterwards clears every bar
126+
// below on the refusal arm — `"0"`, stable, forever. The skeleton is the
127+
// proof that this mount actually went through a loading phase.
128+
//
129+
// ⚠️ `toBeTruthy`, not `toBeInTheDocument`: this package's
130+
// `tsconfig.test.json` does not name `@testing-library/jest-dom` in `types`
131+
// (the sibling `plugin-map` one does), so that matcher is green under vitest
132+
// and TS2339 under `tsc -p tsconfig.test.json`. `getByTestId` THROWING when
133+
// the skeleton is absent is the assertion either way; the matcher only
134+
// attaches the message.
135+
expect(
136+
screen.getByTestId('timeline-loading'),
137+
'the loading skeleton must be on screen first — a renderer mounted empty from the first paint is not a settled zero',
138+
).toBeTruthy();
90139
await waitFor(() => expect(ds.find).toHaveBeenCalled());
91140
// `find`'s OWN answer, settled — a pure read of the mock's call record that
92141
// touches no DOM. Without it the assertion can be satisfied by the mount's
93142
// initial empty state, which every arm renders identically.
94143
await ds.find.mock.results[0].value;
144+
// ② Gate on the count this wait NAMES, at the value this arm claims.
145+
//
146+
// The clause here used to be `.not.toBeNull()`, and it was INERT: the
147+
// attribute is written unconditionally by the renderer double, so once
148+
// `getByTestId` stops throwing the attribute is always a string. The THROW
149+
// was the whole gate, which made this a bare mount signal wearing the
150+
// vocabulary of a row count — `"0"` satisfied it, so an arm expecting rows
151+
// and an arm expecting none waited on exactly the same event.
95152
await waitFor(() =>
96-
expect(screen.getByTestId('timeline-renderer').getAttribute('data-item-count')).not.toBeNull(),
153+
expect(
154+
screen.getByTestId('timeline-renderer').getAttribute('data-item-count'),
155+
'the renderer must report the row count this envelope produces, not merely exist',
156+
).toBe(String(expectedItems)),
97157
);
98-
return Number(screen.getByTestId('timeline-renderer').getAttribute('data-item-count'));
158+
// ③ … and it must STILL be that count after the settle window. This is the
159+
// half ② cannot supply on the refusal arm, where the expected value is `"0"`
160+
// and `"0"` is also what an unpopulated renderer reports. MEASURED: with
161+
// `setFetchedData` deferred by 50ms and the `records` arm restored to
162+
// `extractRecords`, the old wait was satisfied at `data-item-count="0"` and
163+
// the refusal case PASSED while the settled timeline drew two rows
164+
// (objectui#8709 leg A).
165+
await act(async () => {
166+
await new Promise((resolve) => setTimeout(resolve, POST_SETTLE_MS));
167+
});
168+
const settled = screen.getByTestId('timeline-renderer').getAttribute('data-item-count');
169+
expect(
170+
settled,
171+
'rows must not arrive AFTER the count was read — a count that moves in the settle window was an intermediate state',
172+
).toBe(String(expectedItems));
173+
return Number(settled);
99174
}
100175

101176
beforeEach(() => {
@@ -113,18 +188,18 @@ describe('ObjectTimeline — the find() envelope it reads (objectui#6839)', () =
113188
// (objectui#7802): it renders, and `waitFor` re-runs its callback on DOM
114189
// mutations, so the predicate feeds itself and leaks a container div per
115190
// run. The render happens once, out here, and the case reads its answer.
116-
expect(await itemsThrough(asData), 'the declared rows member must still draw').toBe(2);
191+
expect(await itemsThrough(asData, 2), 'the declared rows member must still draw').toBe(2);
117192
});
118193

119194
it('still reads a bare array — the live non-envelope shape fakes answer with', async () => {
120-
expect(await itemsThrough(asBareArray), 'the bare-array arm must still draw').toBe(2);
195+
expect(await itemsThrough(asBareArray, 2), 'the bare-array arm must still draw').toBe(2);
121196
});
122197

123198
it('does NOT read `records` — not a QueryResult member', async () => {
124199
// Before the fix these two rows drew off a key `QueryResult` does not
125200
// declare, and did so AHEAD of `data`.
126201
expect(
127-
await itemsThrough(asRecords),
202+
await itemsThrough(asRecords, 0),
128203
'a `records` envelope must reach the rail as zero rows, not as the rows it names',
129204
).toBe(0);
130205
});

0 commit comments

Comments
 (0)