useAgentState: re-arm the poll loop from refresh() instead of killing it
argus's review on the original fix: refresh() did a parallel one-off fetch instead of calling the mount effect's self-rescheduling poll(), so it never re-armed timerRef after firing. Wiring refresh() to run on every visibilitychange-to-visible meant the very first tab-switch back into focus would kill periodic polling for the rest of the session -- reproducing the exact staleness bug this branch set out to fix, just delayed by one tab switch instead of immediate. Fix: hoist poll into a ref set by the mount effect so refresh() invokes the same self-rescheduling function rather than a parallel fetch that drops the loop. Also gave the visibilitychange effect an empty dependency array per the review's second note -- it only closes over stable refs/setters, and now that Root's 1s ticker re-renders the calling component every second, a deps-less effect would tear down and reattach the listener that often for no reason.
This commit is contained in:
parent
9c72a4ae3e
commit
d49a9df479
1 changed files with 18 additions and 16 deletions
|
|
@ -23,6 +23,15 @@ export function useAgentState(): UseAgentStateResult {
|
||||||
const [error, setError] = useState<string | null>(null);
|
const [error, setError] = useState<string | null>(null);
|
||||||
const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
|
const timerRef = useRef<ReturnType<typeof setTimeout> | null>(null);
|
||||||
const stoppedRef = useRef(false);
|
const stoppedRef = useRef(false);
|
||||||
|
// Holds the mount effect's self-rescheduling `poll` so `refresh()` can
|
||||||
|
// invoke the *same* function instead of a parallel one-off fetch — a
|
||||||
|
// fresh call still runs through `poll`'s own `setTimeout(poll, …)` tail,
|
||||||
|
// so the loop stays alive afterwards instead of dying silently the
|
||||||
|
// first time something calls `refresh()` (see the review discussion on
|
||||||
|
// this file's PR: an earlier version of `refresh()` did its own
|
||||||
|
// one-off fetch with no reschedule, which permanently killed periodic
|
||||||
|
// polling the first time a caller invoked it).
|
||||||
|
const pollRef = useRef<() => void>(() => {});
|
||||||
|
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
stoppedRef.current = false;
|
stoppedRef.current = false;
|
||||||
|
|
@ -42,6 +51,7 @@ export function useAgentState(): UseAgentStateResult {
|
||||||
timerRef.current = setTimeout(poll, RETRY_MS);
|
timerRef.current = setTimeout(poll, RETRY_MS);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
pollRef.current = poll;
|
||||||
poll();
|
poll();
|
||||||
|
|
||||||
return () => {
|
return () => {
|
||||||
|
|
@ -52,20 +62,8 @@ export function useAgentState(): UseAgentStateResult {
|
||||||
|
|
||||||
function refresh() {
|
function refresh() {
|
||||||
if (timerRef.current) clearTimeout(timerRef.current);
|
if (timerRef.current) clearTimeout(timerRef.current);
|
||||||
// Re-trigger the same poll loop immediately; the effect's closure
|
|
||||||
// owns `poll`, so the simplest safe re-trigger from outside it is a
|
|
||||||
// fresh fetch here rather than reaching back into the effect.
|
|
||||||
stoppedRef.current = false;
|
stoppedRef.current = false;
|
||||||
fetch('api/state')
|
pollRef.current();
|
||||||
.then((resp) => {
|
|
||||||
if (!resp.ok) throw new Error(`http ${resp.status}`);
|
|
||||||
return resp.json();
|
|
||||||
})
|
|
||||||
.then((s: AgentState) => {
|
|
||||||
setState(s);
|
|
||||||
setError(null);
|
|
||||||
})
|
|
||||||
.catch((err) => setError(err instanceof Error ? err.message : String(err)));
|
|
||||||
}
|
}
|
||||||
|
|
||||||
// A backgrounded/hidden tab has its `setTimeout` chain throttled by the
|
// A backgrounded/hidden tab has its `setTimeout` chain throttled by the
|
||||||
|
|
@ -74,15 +72,19 @@ export function useAgentState(): UseAgentStateResult {
|
||||||
// stalls, and everything derived from `state` (not just the fetch
|
// stalls, and everything derived from `state` (not just the fetch
|
||||||
// itself) goes stale until it happens to fire again. Force a resync the
|
// itself) goes stale until it happens to fire again. Force a resync the
|
||||||
// moment the tab becomes visible so switching back doesn't leave a
|
// moment the tab becomes visible so switching back doesn't leave a
|
||||||
// reading that's minutes old. `refresh` closes over stable refs/setters
|
// reading that's minutes old. Empty deps: `refresh` only closes over
|
||||||
// only, so a fresh instance each render is safe to use here.
|
// refs and setState setters, both stable across renders, so the first
|
||||||
|
// render's closure stays correct for the component's whole lifetime —
|
||||||
|
// and, unlike re-subscribing on every render, doesn't re-attach this
|
||||||
|
// listener every second now that `Root`'s own 1s ticker re-renders the
|
||||||
|
// component that calls this hook.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
function onVisible() {
|
function onVisible() {
|
||||||
if (document.visibilityState === 'visible') refresh();
|
if (document.visibilityState === 'visible') refresh();
|
||||||
}
|
}
|
||||||
document.addEventListener('visibilitychange', onVisible);
|
document.addEventListener('visibilitychange', onVisible);
|
||||||
return () => document.removeEventListener('visibilitychange', onVisible);
|
return () => document.removeEventListener('visibilitychange', onVisible);
|
||||||
});
|
}, []);
|
||||||
|
|
||||||
return { state, error, refresh };
|
return { state, error, refresh };
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue