Skip to content

Commit 239cd56

Browse files
committed
fix(webapp): stop the URI resolver working after the panel closes
A request still in flight at unmount rejected afterwards, and the catch scheduled a retry that fetched again and set state for a component that was gone. The hook tracks whether it is still mounted and neither records nor reschedules once it is not.
1 parent e315926 commit 239cd56

3 files changed

Lines changed: 74 additions & 8 deletions

File tree

apps/webapp/app/components/dashboard-agent/resolve-uris.test.ts

Lines changed: 38 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
1+
import { readFileSync } from "node:fs";
12
import { describe, expect, it } from "vitest";
2-
import { MAX_URIS_PER_RESOLVE_REQUEST, planUriBatches } from "./resolve-uris";
3+
import { MAX_URIS_PER_RESOLVE_REQUEST, planUriBatches, shouldScheduleRetry } from "./resolve-uris";
34

45
const uri = (index: number) => `trigger://runs/run_${index}`;
56

@@ -30,3 +31,39 @@ describe("planUriBatches", () => {
3031
expect(planUriBatches([])).toEqual([]);
3132
});
3233
});
34+
35+
describe("shouldScheduleRetry", () => {
36+
it("retries a transient failure while the cards are still on screen", () => {
37+
expect(shouldScheduleRetry({ mounted: true, timerPending: false })).toBe(true);
38+
});
39+
40+
it("schedules nothing once the panel is gone", () => {
41+
// A request in flight at unmount rejects afterwards; its retry would fetch
42+
// again and set state for a component that no longer exists.
43+
expect(shouldScheduleRetry({ mounted: false, timerPending: false })).toBe(false);
44+
expect(shouldScheduleRetry({ mounted: false, timerPending: true })).toBe(false);
45+
});
46+
47+
it("lets one timer serve every batch", () => {
48+
expect(shouldScheduleRetry({ mounted: true, timerPending: true })).toBe(false);
49+
});
50+
});
51+
52+
/**
53+
* Structural guard, not behavioural proof: the webapp has no DOM test environment, so nothing
54+
* here mounts the hook or unmounts it mid-flight. It asserts the policy above is the one the
55+
* hook asks, and that the unmount path is wired.
56+
*/
57+
describe("useTriggerUriResolver's unmount wiring", () => {
58+
const source = readFileSync(new URL("./useTriggerUriResolver.ts", import.meta.url), "utf8");
59+
60+
it("asks `shouldScheduleRetry` rather than testing the timer itself", () => {
61+
expect(source).toContain("shouldScheduleRetry({");
62+
expect(source).not.toMatch(/if \(retryTimer\.current === undefined\)/);
63+
});
64+
65+
it("marks itself unmounted on cleanup and drops state updates after that", () => {
66+
expect(source).toContain("mounted.current = false");
67+
expect(source).toContain("if (!mounted.current) return;");
68+
});
69+
});

apps/webapp/app/components/dashboard-agent/resolve-uris.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,21 @@ export const MAX_RESOLVE_ATTEMPTS = 3;
1111

1212
export const RESOLVE_RETRY_DELAY_MS = 1_000;
1313

14+
/**
15+
* A request in flight at unmount rejects afterwards. Its retry must not be scheduled: the
16+
* callback would fetch again for a component that is gone, and record the answer into state.
17+
* One timer serves every batch, so a pending one is not replaced either.
18+
*/
19+
export function shouldScheduleRetry({
20+
mounted,
21+
timerPending,
22+
}: {
23+
mounted: boolean;
24+
timerPending: boolean;
25+
}): boolean {
26+
return mounted && !timerPending;
27+
}
28+
1429
/** Deduplicates, then splits into requests no bigger than the cap. */
1530
export function planUriBatches(
1631
uris: readonly string[],

apps/webapp/app/components/dashboard-agent/useTriggerUriResolver.ts

Lines changed: 21 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,12 @@
11
import { isTriggerUri } from "@internal/dashboard-agent-contracts";
22
import { useCallback, useEffect, useRef, useState } from "react";
33
import type { ResolvedUri } from "./ReportView";
4-
import { MAX_RESOLVE_ATTEMPTS, planUriBatches, RESOLVE_RETRY_DELAY_MS } from "./resolve-uris";
4+
import {
5+
MAX_RESOLVE_ATTEMPTS,
6+
planUriBatches,
7+
RESOLVE_RETRY_DELAY_MS,
8+
shouldScheduleRetry,
9+
} from "./resolve-uris";
510

611
/**
712
* Synchronous facade over the panel's async `resolve-many` action: the first render of a URI
@@ -16,6 +21,7 @@ export function useTriggerUriResolver(actionPath: string): (uri: string) => Reso
1621
const inFlight = useRef(new Set<string>());
1722
const attempts = useRef(new Map<string, number>());
1823
const retryTimer = useRef<number | undefined>(undefined);
24+
const mounted = useRef(true);
1925

2026
const resolveUri = useCallback(
2127
(uri: string): ResolvedUri | null => {
@@ -27,6 +33,7 @@ export function useTriggerUriResolver(actionPath: string): (uri: string) => Reso
2733
);
2834

2935
const record = useCallback((entries: Record<string, ResolvedUri | null>) => {
36+
if (!mounted.current) return;
3037
answered.current = { ...answered.current, ...entries };
3138
setResolved((previous) => ({ ...previous, ...entries }));
3239
}, []);
@@ -72,7 +79,12 @@ export function useTriggerUriResolver(actionPath: string): (uri: string) => Reso
7279
}
7380
if (Object.keys(exhausted).length > 0) record(exhausted);
7481
// One timer for all batches; a render may never follow, so it can't be the trigger.
75-
if (retryTimer.current === undefined) {
82+
if (
83+
shouldScheduleRetry({
84+
mounted: mounted.current,
85+
timerPending: retryTimer.current !== undefined,
86+
})
87+
) {
7688
retryTimer.current = window.setTimeout(() => {
7789
retryTimer.current = undefined;
7890
flushRef.current();
@@ -92,12 +104,14 @@ export function useTriggerUriResolver(actionPath: string): (uri: string) => Reso
92104
flush();
93105
});
94106

95-
useEffect(
96-
() => () => {
107+
useEffect(() => {
108+
mounted.current = true;
109+
return () => {
110+
mounted.current = false;
97111
if (retryTimer.current !== undefined) window.clearTimeout(retryTimer.current);
98-
},
99-
[]
100-
);
112+
retryTimer.current = undefined;
113+
};
114+
}, []);
101115

102116
return resolveUri;
103117
}

0 commit comments

Comments
 (0)