Skip to content

Commit dfb2d03

Browse files
npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7wpfleger96
andcommitted
fix(observer-feed): address Thufir review findings on eager hydration
Three issues found in review of 884ed9b: 1. CRITICAL: fetchOlderArchived captured hasOlderArchived from React state (closure), not the ref. On an exhausted channel A → switch to B, React state was still false while ps.hasOlderArchived had been reset to true by applyChannelReset. The hydration loop called fetchOlderArchived 10 times, each bailing at the !hasOlderArchived guard — leaving B with zero reads. Fix: read ps.hasOlderArchived from the ref throughout; remove hasOlderArchived from fetchOlderArchived deps so the callback is stable. 2. IMPORTANT: applyChannelReset clears isFetching even when A's Tauri read is in flight. When B then starts a read, A's late resolution wrote A's cursor into ps.cursor (now scoped to B). Fix: add activeChannelId token to ArchivePagingState, set by applyChannelReset. fetchOlderArchived captures requestChannelId at call start and discards cursor/exhaustion writes if requestChannelId !== ps.activeChannelId at resolution time. 3. IMPORTANT: new tests in ingestArchivedObserverEvents.test.mjs reimplemented the hydration loop inline (BUDGET hardcoded, loop logic copied), matching the tautological pattern removed in f13cfba. Fix: extract runHydrationLoop into archivePagingState.ts as a pure exported function; both the hook and the tests import and call it. Tests now fail if the production loop is deleted or misdirected. Co-authored-by: Will Pfleger <[email protected]> Signed-off-by: Will Pfleger <[email protected]>
1 parent 884ed9b commit dfb2d03

3 files changed

Lines changed: 201 additions & 108 deletions

File tree

‎desktop/src/features/agents/ingestArchivedObserverEvents.test.mjs‎

Lines changed: 109 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -394,6 +394,7 @@ describe("load-older cursor advance logic", () => {
394394
import {
395395
createArchivePagingState,
396396
applyChannelReset,
397+
runHydrationLoop,
397398
} from "@/features/agents/ui/archivePagingState.ts";
398399

399400
describe("archive paging state reset on channel change", () => {
@@ -415,6 +416,7 @@ describe("archive paging state reset on channel change", () => {
415416
false,
416417
"initialHydrationDone starts false",
417418
);
419+
assert.equal(ps.activeChannelId, null, "activeChannelId starts null");
418420
});
419421

420422
it("test_channel_switch_resets_cursor_exhaustion_and_fetch_lock", () => {
@@ -425,11 +427,12 @@ describe("archive paging state reset on channel change", () => {
425427
ps.hasOlderArchived = false; // channel A exhausted
426428
ps.isFetching = true; // mid-flight request (edge case)
427429
ps.initialHydrationDone = true; // hydration ran for channel A
430+
ps.activeChannelId = "chan-a";
428431
ps.backfillStatus = "done"; // backfill ran once already
429432
const originalPromise = ps.backfillPromise; // must survive reset
430433

431434
// Channel switch — this is what the useEffect([channelId]) calls.
432-
applyChannelReset(ps);
435+
applyChannelReset(ps, "chan-b");
433436

434437
assert.equal(ps.cursor, null, "cursor resets to null on channel switch");
435438
assert.equal(
@@ -447,6 +450,11 @@ describe("archive paging state reset on channel change", () => {
447450
false,
448451
"initialHydrationDone resets to false on channel switch so the new channel hydrates",
449452
);
453+
assert.equal(
454+
ps.activeChannelId,
455+
"chan-b",
456+
"activeChannelId updates to new channel on switch",
457+
);
450458

451459
// Backfill state must NOT be touched — it is identity-level and should
452460
// survive channel switches so the backfill only runs once per identity mount.
@@ -474,7 +482,7 @@ describe("archive paging state reset on channel change", () => {
474482
ps.cursor = { createdAt: 500, id: "a-oldest" };
475483
ps.hasOlderArchived = false;
476484
ps.initialHydrationDone = true;
477-
applyChannelReset(ps);
485+
applyChannelReset(ps, "chan-b");
478486

479487
assert.equal(ps.cursor, null, "switch A→B: cursor reset");
480488
assert.equal(
@@ -487,12 +495,17 @@ describe("archive paging state reset on channel change", () => {
487495
false,
488496
"switch A→B: initialHydrationDone reset",
489497
);
498+
assert.equal(
499+
ps.activeChannelId,
500+
"chan-b",
501+
"switch A→B: activeChannelId updated",
502+
);
490503

491504
// Simulate channel B also being paged and hydrated.
492505
ps.cursor = { createdAt: 200, id: "b-oldest" };
493506
ps.hasOlderArchived = false;
494507
ps.initialHydrationDone = true;
495-
applyChannelReset(ps);
508+
applyChannelReset(ps, "chan-c");
496509

497510
assert.equal(ps.cursor, null, "switch B→C: cursor reset again");
498511
assert.equal(
@@ -505,6 +518,11 @@ describe("archive paging state reset on channel change", () => {
505518
false,
506519
"switch B→C: initialHydrationDone reset again",
507520
);
521+
assert.equal(
522+
ps.activeChannelId,
523+
"chan-c",
524+
"switch B→C: activeChannelId updated",
525+
);
508526
});
509527
});
510528

@@ -514,127 +532,144 @@ describe("archive paging state reset on channel change", () => {
514532
// fetchOlderArchived up to INITIAL_HYDRATION_BUDGET_PAGES times. The loop must:
515533
// - Stop at budget (10 pages) even if more archive exists.
516534
// - Stop early when the archive is exhausted (ps.hasOlderArchived → false).
517-
// - Respect channel-switch cancellation (cancelled flag).
518-
// - Set ps.initialHydrationDone immediately to prevent duplicate loops.
535+
// - Respect channel-switch cancellation (signal.cancelled).
519536
//
520-
// We test the loop's control flow as pure logic using mock fetchOlderArchived
521-
// functions and ArchivePagingState directly — no React runtime needed.
537+
// These tests call the PRODUCTION runHydrationLoop from archivePagingState.ts
538+
// with mock fetchOnePage functions — so they fail if the production loop logic
539+
// is deleted or misrouted, not just if a reimplemented copy breaks.
522540

523-
describe("eager initial hydration loop control flow", () => {
541+
describe("eager initial hydration loop control flow (production runHydrationLoop)", () => {
524542
const BUDGET = 10; // mirrors INITIAL_HYDRATION_BUDGET_PAGES
525543

526-
// Simulate the hydration loop from the hook with injectable fetch + ps.
527-
async function runHydrationLoop(
528-
ps,
529-
fetchOlderArchived,
530-
cancelled = { value: false },
531-
) {
532-
ps.initialHydrationDone = true;
533-
for (let page = 0; page < BUDGET; page++) {
534-
if (cancelled.value || !ps.hasOlderArchived) {
535-
break;
536-
}
537-
await fetchOlderArchived();
538-
}
539-
}
540-
541544
it("test_hydration_stops_at_budget_when_archive_never_exhausted", async () => {
542545
const ps = createArchivePagingState();
546+
applyChannelReset(ps, "chan-1");
543547
let fetchCount = 0;
544-
// fetchOlderArchived that never exhausts the archive
545-
const fetchOlderArchived = async () => {
548+
const fetchOnePage = async () => {
546549
fetchCount++;
547-
// archive remains non-empty
550+
// archive remains non-empty — ps.hasOlderArchived stays true
548551
};
549-
await runHydrationLoop(ps, fetchOlderArchived);
552+
const signal = { cancelled: false };
553+
await runHydrationLoop(ps, fetchOnePage, BUDGET, signal);
550554
assert.equal(
551555
fetchCount,
552556
BUDGET,
553-
`hydration loop must stop after exactly ${BUDGET} pages (budget limit)`,
554-
);
555-
assert.equal(
556-
ps.initialHydrationDone,
557-
true,
558-
"initialHydrationDone must be true after budget reached",
557+
`production runHydrationLoop must stop after exactly ${BUDGET} pages (budget limit)`,
559558
);
560559
});
561560

562561
it("test_hydration_stops_early_when_archive_exhausted", async () => {
563562
const ps = createArchivePagingState();
563+
applyChannelReset(ps, "chan-1");
564564
let fetchCount = 0;
565-
// fetchOlderArchived that exhausts archive on page 3
566-
const fetchOlderArchived = async () => {
565+
const fetchOnePage = async () => {
567566
fetchCount++;
568567
if (fetchCount >= 3) {
569-
ps.hasOlderArchived = false;
568+
ps.hasOlderArchived = false; // mock: archive exhausted on page 3
570569
}
571570
};
572-
await runHydrationLoop(ps, fetchOlderArchived);
571+
const signal = { cancelled: false };
572+
await runHydrationLoop(ps, fetchOnePage, BUDGET, signal);
573573
assert.equal(
574574
fetchCount,
575575
3,
576-
"hydration loop must stop as soon as archive is exhausted (before budget)",
577-
);
578-
assert.equal(
579-
ps.initialHydrationDone,
580-
true,
581-
"initialHydrationDone must be true after exhaustion",
582-
);
583-
assert.equal(
584-
ps.hasOlderArchived,
585-
false,
586-
"hasOlderArchived must be false after exhaustion",
576+
"production runHydrationLoop must stop as soon as ps.hasOlderArchived is false (before budget)",
587577
);
588578
});
589579

590580
it("test_hydration_respects_cancellation_on_channel_switch", async () => {
591581
const ps = createArchivePagingState();
592-
const cancelled = { value: false };
582+
applyChannelReset(ps, "chan-1");
583+
const signal = { cancelled: false };
593584
let fetchCount = 0;
594-
// fetchOlderArchived that sets cancelled on page 2 (simulates channel switch mid-loop)
595-
const fetchOlderArchived = async () => {
585+
const fetchOnePage = async () => {
596586
fetchCount++;
597587
if (fetchCount >= 2) {
598-
cancelled.value = true;
588+
signal.cancelled = true; // mock: channel switched mid-loop
599589
}
600590
};
601-
await runHydrationLoop(ps, fetchOlderArchived, cancelled);
591+
await runHydrationLoop(ps, fetchOnePage, BUDGET, signal);
602592
assert.equal(
603593
fetchCount,
604594
2,
605-
"hydration loop must stop when cancelled (channel switch)",
595+
"production runHydrationLoop must stop when signal.cancelled is true (channel switch)",
606596
);
607-
// initialHydrationDone is true because it was set before the loop;
608-
// applyChannelReset will reset it for the new channel.
597+
});
598+
599+
it("test_hydration_zero_iterations_when_already_exhausted", async () => {
600+
// If ps.hasOlderArchived is already false before the loop starts (e.g.
601+
// channel A was exhausted and reset did not run yet), the loop must not
602+
// call fetchOnePage at all.
603+
const ps = createArchivePagingState();
604+
applyChannelReset(ps, "chan-1");
605+
ps.hasOlderArchived = false; // already exhausted
606+
let fetchCount = 0;
607+
const fetchOnePage = async () => {
608+
fetchCount++;
609+
};
610+
const signal = { cancelled: false };
611+
await runHydrationLoop(ps, fetchOnePage, BUDGET, signal);
609612
assert.equal(
610-
ps.initialHydrationDone,
611-
true,
612-
"initialHydrationDone stays true (applyChannelReset will reset it for new channel)",
613+
fetchCount,
614+
0,
615+
"must not fetch when archive is already exhausted",
613616
);
614617
});
615618

616-
it("test_initialHydrationDone_set_before_loop_prevents_duplicate_loops", () => {
619+
// Regression: stale React closure — the original fetchOlderArchived captured
620+
// hasOlderArchived from React state, which was false (exhausted) for channel A
621+
// while ps.hasOlderArchived had already been reset to true by applyChannelReset.
622+
// The production loop uses ps.hasOlderArchived (the ref) to guard iterations,
623+
// so the switch-then-hydrate path must call fetchOnePage for the new channel.
624+
it("test_exhausted_channel_A_then_switch_to_B_hydrates_B", async () => {
617625
const ps = createArchivePagingState();
618-
assert.equal(ps.initialHydrationDone, false, "starts false");
619-
// Simulate the synchronous guard at the top of the effect.
620-
ps.initialHydrationDone = true;
621-
// A second effect invocation would see initialHydrationDone=true and bail immediately.
622-
// Verify the guard value is set before any async work starts.
626+
627+
// Simulate exhausting channel A.
628+
applyChannelReset(ps, "chan-a");
629+
ps.hasOlderArchived = false; // channel A exhausted
630+
631+
// Switch to channel B — resets hasOlderArchived and activeChannelId.
632+
applyChannelReset(ps, "chan-b");
633+
634+
// ps.hasOlderArchived is now true; the loop should call fetchOnePage.
635+
let fetchCount = 0;
636+
const fetchOnePage = async () => {
637+
fetchCount++;
638+
ps.hasOlderArchived = false; // B also exhausted after 1 page
639+
};
640+
const signal = { cancelled: false };
641+
await runHydrationLoop(ps, fetchOnePage, BUDGET, signal);
642+
623643
assert.equal(
624-
ps.initialHydrationDone,
625-
true,
626-
"initialHydrationDone must be true immediately after loop starts (prevents re-entry)",
644+
fetchCount,
645+
1,
646+
"after switch from exhausted channel A to B, runHydrationLoop must call fetchOnePage for B (not skip due to stale exhaustion)",
627647
);
628648
});
629649

630-
it("test_channel_reset_clears_hydration_flag_for_new_channel", () => {
650+
// Regression: stale cursor write — an in-flight read from channel A should
651+
// not write A's cursor into ps.cursor after the switch to B. The activeChannelId
652+
// token (set by applyChannelReset) is what lets fetchOlderArchived detect and
653+
// discard the stale result. This test verifies that applyChannelReset correctly
654+
// advances the token so a pre-switch requestChannelId !== ps.activeChannelId.
655+
it("test_activeChannelId_token_detects_stale_read_from_prior_channel", () => {
631656
const ps = createArchivePagingState();
632-
ps.initialHydrationDone = true; // hydration ran for channel A
633-
applyChannelReset(ps);
657+
applyChannelReset(ps, "chan-a");
658+
const requestChannelId = ps.activeChannelId; // captured at request start = "chan-a"
659+
660+
// Simulate channel switch before the Tauri read resolves.
661+
applyChannelReset(ps, "chan-b");
662+
663+
// The in-flight A read checks requestChannelId !== ps.activeChannelId.
664+
assert.notEqual(
665+
requestChannelId,
666+
ps.activeChannelId,
667+
"requestChannelId from channel A must not match activeChannelId after switching to B — stale read must be discarded",
668+
);
634669
assert.equal(
635-
ps.initialHydrationDone,
636-
false,
637-
"applyChannelReset must clear initialHydrationDone so new channel gets fresh hydration pass",
670+
ps.activeChannelId,
671+
"chan-b",
672+
"activeChannelId must reflect the current channel after switch",
638673
);
639674
});
640675
});

‎desktop/src/features/agents/ui/archivePagingState.ts‎

Lines changed: 44 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,11 @@ export interface ArchivePagingState {
3232
* completed (budget reached or archive exhausted). Resets on channel change
3333
* so switching channels triggers a fresh hydration pass. */
3434
initialHydrationDone: boolean;
35+
/** The channelId that this paging state is currently scoped to.
36+
* Captured by fetchOlderArchived at request start; compared on resolution
37+
* to detect and discard cursor/exhaustion writes from in-flight reads that
38+
* completed after a channel switch. */
39+
activeChannelId: string | null;
3540
}
3641

3742
/**
@@ -48,6 +53,7 @@ export function createArchivePagingState(): ArchivePagingState {
4853
backfillResolve: null,
4954
cursor: null,
5055
initialHydrationDone: false,
56+
activeChannelId: null,
5157
};
5258
state.backfillPromise = new Promise<void>((resolve) => {
5359
state.backfillResolve = resolve;
@@ -58,16 +64,50 @@ export function createArchivePagingState(): ArchivePagingState {
5864
/**
5965
* Reset per-channel paging state when the viewed channel changes.
6066
*
61-
* Only cursor, exhaustion flag, and fetch lock are channel-scoped. Backfill
62-
* state is identity-level (the index covers ALL channels and needs to run only
63-
* once per identity mount), so it is intentionally NOT touched here.
67+
* Only cursor, exhaustion flag, fetch lock, and channel token are
68+
* channel-scoped. Backfill state is identity-level (the index covers ALL
69+
* channels and needs to run only once per identity mount), so it is
70+
* intentionally NOT touched here.
6471
*
6572
* Called by the useEffect([channelId]) in useLoadArchivedObserverEvents.
6673
* Exported so tests can verify the reset semantics without a React runtime.
6774
*/
68-
export function applyChannelReset(state: ArchivePagingState): void {
75+
export function applyChannelReset(
76+
state: ArchivePagingState,
77+
newChannelId: string | null,
78+
): void {
6979
state.cursor = null;
7080
state.isFetching = false;
7181
state.hasOlderArchived = true;
7282
state.initialHydrationDone = false;
83+
state.activeChannelId = newChannelId;
84+
}
85+
86+
/**
87+
* Run the eager initial-hydration paging loop.
88+
*
89+
* Calls `fetchOnePage()` up to `budget` times. Stops early when:
90+
* - `ps.hasOlderArchived` is false (archive exhausted for this channel), OR
91+
* - `signal.cancelled` is true (channel switched away mid-loop).
92+
*
93+
* `fetchOnePage` is the per-page read unit: it must respect `ps.isFetching`
94+
* (lock), await backfill, perform the Tauri read, ingest results, advance
95+
* the cursor, and set `ps.hasOlderArchived = false` when the page is short.
96+
* The hook wires the real implementation; tests supply a mock.
97+
*
98+
* Exported so tests can call the production loop logic directly — passing a
99+
* mock `fetchOnePage` — without reimplementing the control flow.
100+
*/
101+
export async function runHydrationLoop(
102+
ps: ArchivePagingState,
103+
fetchOnePage: () => Promise<void>,
104+
budget: number,
105+
signal: { cancelled: boolean },
106+
): Promise<void> {
107+
for (let page = 0; page < budget; page++) {
108+
if (signal.cancelled || !ps.hasOlderArchived) {
109+
break;
110+
}
111+
await fetchOnePage();
112+
}
73113
}

0 commit comments

Comments
 (0)