Skip to content

Commit 18deaaa

Browse files
azhukaudevazhukaudev
committed
docs: record the advisor audit plans and execution log
1 parent ddfeaf3 commit 18deaaa

7 files changed

Lines changed: 1099 additions & 0 deletions

‎plans/001-fix-paginated-ssr-seed-clobber.md‎

Lines changed: 242 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,242 @@
1+
# Plan 001: Stop the paginated SSR/hydration seed being wiped by the client's initial `LoadingFirstPage` emission
2+
3+
> **Executor instructions**: Follow this plan step by step. Run every
4+
> verification command and confirm the expected result before moving to the
5+
> next step. If anything in the "STOP conditions" section occurs, stop and
6+
> report — do not improvise. When done, update the status row for this plan
7+
> in `plans/README.md`.
8+
>
9+
> **Drift check (run first)**: `git diff --stat d2ccbed..HEAD -- packages/convex-angular/src/lib/providers/inject-paginated-query.ts packages/convex-angular/src/lib/providers/inject-paginated-query.ssr.spec.ts`
10+
> If any in-scope file changed since this plan was written, compare the
11+
> "Current state" excerpts against the live code before proceeding; on a
12+
> mismatch, treat it as a STOP condition.
13+
14+
## Status
15+
16+
- **Priority**: P1
17+
- **Effort**: M
18+
- **Risk**: MED (touches the live-subscription callback all paginated consumers run through)
19+
- **Depends on**: none
20+
- **Category**: bug
21+
- **Planned at**: commit `d2ccbed`, 2026-07-10
22+
23+
## Why this matters
24+
25+
`injectPaginatedQuery` supports SSR: the server fetches page one over HTTP, transfers it via Angular `TransferState`, and the browser seeds its signals from the transferred page so the hydrated UI matches the server HTML with no loading flash. That seeding is currently defeated: the real Convex client (`convex` 1.42.1) always fires an **initial** callback for a fresh paginated subscription — with `{ results: [], status: 'LoadingFirstPage' }` — one macrotask after subscribing. The library's subscription callback applies that emission unconditionally, so the seeded first page is overwritten with `[]` and `status()` flips from `'success'` back to `'pending'`. Users see server-rendered list content appear, then vanish into a loading skeleton until the WebSocket delivers the real first page. The existing SSR spec never drives this initial emission, so the suite passes despite the bug.
26+
27+
## Current state
28+
29+
Files:
30+
31+
- `packages/convex-angular/src/lib/providers/inject-paginated-query.ts` — the helper; seed + clobber both live here.
32+
- `packages/convex-angular/src/lib/providers/inject-paginated-query.ssr.spec.ts` — SSR/hydration specs; the gap to close.
33+
- `packages/convex-angular/src/lib/providers/inject-paginated-query.spec.ts` — main spec file (regression checks only; you should not need to edit it unless a test asserts the old clobber behavior).
34+
35+
The seed is applied inside `subscribe()` (`inject-paginated-query.ts:349-363`):
36+
37+
```ts
38+
const subscribe = (
39+
args: PaginatedQueryArgs<Query>,
40+
initialNumItems: number,
41+
generation: number,
42+
argsKey: string,
43+
) => {
44+
cleanupSubscription();
45+
resetState();
46+
47+
// Seed from a server-rendered first page so the hydrated UI matches
48+
// the server HTML; the live subscription below replaces it on sync.
49+
const transferred = hydration?.consume(getFunctionName(query), argsKey);
50+
if (transferred?.value !== undefined) {
51+
applyFirstPage(transferred.value as unknown as PaginationResult<PaginatedQueryItem<Query>>);
52+
}
53+
```
54+
55+
Immediately below, the live-subscription success callback applies every emission unconditionally (`inject-paginated-query.ts:370-391`, abbreviated):
56+
57+
```ts
58+
(rawResult) => {
59+
if (generation !== activeGeneration) {
60+
return;
61+
}
62+
63+
const result = rawResult;
64+
65+
// Store the loadMore function
66+
currentLoadMore = result.loadMore;
67+
68+
// Update results
69+
results.set(result.results);
70+
error.set(undefined);
71+
72+
// Update status signals based on status
73+
switch (result.status) {
74+
case 'LoadingFirstPage':
75+
isLoadingFirstPage.set(true);
76+
isLoadingMore.set(false);
77+
canLoadMore.set(false);
78+
isExhausted.set(false);
79+
break;
80+
```
81+
82+
Why the initial emission always happens (verified in `node_modules/convex/dist/browser.bundle.js` at commit `d2ccbed`): `onPaginatedUpdate_experimental` checks `!!this.paginatedClient.localQueryResultByToken(paginatedQueryToken)` right after registering the listener and, when truthy, schedules `setTimeout(() => this.callNewListenersWithCurrentValues(), 0)`. For a paginated query with **zero active pages** (i.e. every fresh subscription), `localQueryResultByToken` returns a truthy sentinel `{ results: [], status: 'LoadingFirstPage', loadMore }` — so the initial `LoadingFirstPage` callback fires within one macrotask of subscribing, every time. (The non-paginated `onUpdate` guards on `queryResultReady`, which is false for a fresh query — that is why `injectQuery`'s transferred seed does NOT have this bug. Do not "fix" `injectQuery`.)
83+
84+
The existing hydration spec drives the live callback only with `status: 'CanLoadMore'` (`inject-paginated-query.ssr.spec.ts`, in the hydration `describe` — the test titled around "live subscription … replaces the seed"):
85+
86+
```ts
87+
// The live subscription is still established and replaces the seed.
88+
tick();
89+
onUpdateCallback({
90+
results: [{ _id: '1', name: 'Live todo' }],
91+
status: 'CanLoadMore',
92+
loadMore: jest.fn().mockReturnValue(true),
93+
});
94+
```
95+
96+
It never simulates the real client's initial `LoadingFirstPage` emission — that is the coverage gap.
97+
98+
Repo conventions that apply:
99+
100+
- A monotonic `activeGeneration` guard protects against stale callbacks; the fix must not weaken it.
101+
- The design doc `docs/superpowers/specs/2026-06-10-parity-gaps-design.md` (Gap 3) states for the seeded state: "`loadMore()` no-ops (returns false) until live" — i.e. while the seed is showing, `loadMore` stays inert; it becomes functional when the first real live emission arrives. Preserve that.
102+
- Comments state constraints only; match the file's existing comment style.
103+
104+
## Commands you will need
105+
106+
| Purpose | Command (repo root) | Expected on success |
107+
| -------------------- | --------------------------------------------------------------------------- | --------------------------------------- |
108+
| Node version | `nvm use` (Node 22 per `.nvmrc`; Node 24.14.0 segfaults Jest) | node v22.x active |
109+
| Targeted tests | `pnpm nx test convex-angular --testFile=inject-paginated-query.ssr.spec.ts` | all pass |
110+
| Full paginated suite | `pnpm nx test convex-angular --testFile=inject-paginated-query.spec.ts` | all pass |
111+
| Spec typecheck | `pnpm typecheck:spec` | exit 0 (suite is currently fully green) |
112+
| Quick gate | `pnpm verify:quick` | exit 0 |
113+
114+
## Scope
115+
116+
**In scope** (the only files you should modify):
117+
118+
- `packages/convex-angular/src/lib/providers/inject-paginated-query.ts`
119+
- `packages/convex-angular/src/lib/providers/inject-paginated-query.ssr.spec.ts`
120+
121+
**Out of scope** (do NOT touch, even though they look related):
122+
123+
- `packages/convex-angular/src/lib/providers/inject-query.ts` and `inject-queries.ts` — their seeding is not affected (see "Why the initial emission always happens" above).
124+
- `packages/convex-angular/src/lib/ssr/*` — the transfer machinery is correct; the bug is purely in the browser-side callback.
125+
- Any change to the public `injectPaginatedQuery` API surface or its option types.
126+
127+
## Git workflow
128+
129+
- Branch: `advisor/001-fix-paginated-ssr-seed-clobber`
130+
- Conventional commits, matching `git log` style (e.g. `fix: keep paginated SSR seed until a real live emission arrives`).
131+
- Do NOT push or open a PR unless the operator instructed it.
132+
133+
## Steps
134+
135+
### Step 1: Write the failing test first
136+
137+
In `inject-paginated-query.ssr.spec.ts`, inside the hydration `describe` block, add a test modeled on the existing "replaces the seed" test (same TestBed/TransferState scaffolding). After asserting the seed is applied (`status() === 'success'`, seeded results present), drive the live callback with the real client's initial emission:
138+
139+
```ts
140+
tick();
141+
onUpdateCallback({
142+
results: [],
143+
status: 'LoadingFirstPage',
144+
loadMore: jest.fn().mockReturnValue(false),
145+
});
146+
147+
// The seed must survive the client's initial empty emission.
148+
expect(fixture.componentInstance.todos.status()).toBe('success');
149+
expect(fixture.componentInstance.todos.results()).toEqual([{ _id: '1', name: 'Transferred todo' }]);
150+
expect(fixture.componentInstance.todos.canLoadMore()).toBe(true);
151+
expect(fixture.componentInstance.todos.loadMore(10)).toBe(false); // still inert
152+
153+
// A real emission then replaces the seed and re-arms loadMore.
154+
onUpdateCallback({
155+
results: [{ _id: '1', name: 'Live todo' }],
156+
status: 'CanLoadMore',
157+
loadMore: jest.fn().mockReturnValue(true),
158+
});
159+
expect(fixture.componentInstance.todos.results()).toEqual([{ _id: '1', name: 'Live todo' }]);
160+
expect(fixture.componentInstance.todos.loadMore(10)).toBe(true);
161+
```
162+
163+
**Verify**: `pnpm nx test convex-angular --testFile=inject-paginated-query.ssr.spec.ts` → the new test FAILS (status flips to `'pending'`, results `[]`), all previously existing tests still pass.
164+
165+
### Step 2: Skip the empty initial emission while the seed is showing
166+
167+
In `inject-paginated-query.ts`, inside `subscribe()`:
168+
169+
1. Track whether a transferred seed is currently displayed:
170+
171+
```ts
172+
const transferred = hydration?.consume(getFunctionName(query), argsKey);
173+
let seedActive = false;
174+
if (transferred?.value !== undefined) {
175+
applyFirstPage(transferred.value as unknown as PaginationResult<PaginatedQueryItem<Query>>);
176+
seedActive = true;
177+
}
178+
```
179+
180+
2. At the top of the success callback, immediately after the generation guard, ignore the client's initial empty emission while the seed is active, and mark the seed replaced on any other emission:
181+
182+
```ts
183+
if (generation !== activeGeneration) {
184+
return;
185+
}
186+
187+
// The client always fires one LoadingFirstPage emission for a fresh
188+
// subscription; while a transferred seed is displayed it must not
189+
// clobber the server-rendered page. loadMore stays inert until a
190+
// real emission arrives (see parity-gaps design, Gap 3).
191+
if (seedActive && rawResult.status === 'LoadingFirstPage') {
192+
return;
193+
}
194+
seedActive = false;
195+
```
196+
197+
Note the early return happens BEFORE `currentLoadMore = result.loadMore` — the sentinel's `loadMore` operates on an empty page set and must not be stored. The error callback needs no change (errors already preserve existing results).
198+
199+
**Verify**: `pnpm nx test convex-angular --testFile=inject-paginated-query.ssr.spec.ts` → all pass, including the new test.
200+
201+
### Step 3: Confirm no regression in the fresh-load (unseeded) path
202+
203+
The unseeded path must be byte-identical in behavior: `seedActive` is `false`, so the `LoadingFirstPage` emission still applies (`isLoadingFirstPage` true, `results` `[]`).
204+
205+
**Verify**: `pnpm nx test convex-angular --testFile=inject-paginated-query.spec.ts` → all pass. Then `pnpm typecheck:spec` → exit 0.
206+
207+
### Step 4: Run the gate
208+
209+
**Verify**: `pnpm verify:quick` → exit 0. Then `pnpm test:library` → all suites pass.
210+
211+
## Test plan
212+
213+
- New test (Step 1) in `inject-paginated-query.ssr.spec.ts`: seed survives the initial `LoadingFirstPage` emission; `loadMore` stays inert until a real emission; a subsequent `CanLoadMore` emission replaces the seed and re-arms `loadMore`.
214+
- Optional second case if cheap: seed followed directly by an `Exhausted` emission with an empty page (legitimately empty query) — the empty live result must replace the seed (this works because only `LoadingFirstPage` is skipped).
215+
- Pattern: the existing hydration tests in the same file (TransferState-seeded TestBed + captured `onUpdateCallback`).
216+
- Verification: `pnpm nx test convex-angular --testFile=inject-paginated-query.ssr.spec.ts` → all pass, ≥1 new test.
217+
218+
## Done criteria
219+
220+
Machine-checkable. ALL must hold:
221+
222+
- [ ] `pnpm nx test convex-angular --testFile=inject-paginated-query.ssr.spec.ts` exits 0 with the new test(s) present
223+
- [ ] `pnpm nx test convex-angular --testFile=inject-paginated-query.spec.ts` exits 0
224+
- [ ] `pnpm typecheck` and `pnpm typecheck:spec` exit 0
225+
- [ ] `pnpm verify:quick` exits 0
226+
- [ ] `git status` shows no modified files outside the in-scope list
227+
- [ ] `plans/README.md` status row updated
228+
229+
## STOP conditions
230+
231+
Stop and report back (do not improvise) if:
232+
233+
- The excerpts in "Current state" don't match the live code (drift since `d2ccbed`).
234+
- The convex dependency has been upgraded past 1.42.x AND the initial-emission behavior of `onPaginatedUpdate_experimental` differs (re-check `node_modules/convex/dist/browser.bundle.js`: does a fresh paginated subscription still schedule an initial callback from a truthy `LoadingFirstPage` sentinel?). If the upstream behavior changed, the fix shape may be wrong — report instead of adapting.
235+
- An existing test asserts that a `LoadingFirstPage` emission resets seeded state (that would mean the clobber is somewhere relied upon) — report the test name.
236+
- The fix appears to require changes in `inject-query.ts`, `inject-queries.ts`, or `src/lib/ssr/`.
237+
238+
## Maintenance notes
239+
240+
- If the library later adopts a shared subscription-orchestration primitive across the three query helpers (a known refactor candidate), the `seedActive` skip must move into the paginated strategy of that primitive — it is paginated-specific, because only `onPaginatedUpdate_experimental` fires the initial sentinel emission.
241+
- Reviewer focus: the early return must sit after the generation guard and before `currentLoadMore` is stored; verify with the test that `loadMore` returns `false` while the seed is active.
242+
- If Convex ever exposes a non-experimental paginated subscription API, re-verify its initial-emission contract before migrating.

0 commit comments

Comments
 (0)