Skip to content

Commit 0713f3a

Browse files
ehsan6shaclaude
andcommitted
apps/box: fix Diagnostics listActivePlugins infinite loop
The Diagnostics screen kept the device + go-fula busy in a recursive listActivePlugins → store-update → useEffect-refire → listActivePlugins loop, visible as 2x "listActivePlugins in react-native started" per tick on the app console and eventual crash. Root cause is a two-layer trap: 1. Diagnostics.screen.tsx had a single useEffect that both CALLED listActivePlugins() AND depended on activePlugins. Each call updates the store, which re-triggers the effect. The lint suppression and "stable enough" comment masked the bug. 2. usePluginsStore.listActivePlugins did set({ activePlugins: [] }) for both the empty-list case AND the null-msg case (the latter happens whenever go-fula's active-plugins.txt is empty: its readActivePlugins returns [""], the listActivePluginsImpl filters out empty strings leaving nil, which JSON-marshals to null). Every call created a fresh [] reference, and zustand's shallow equality flagged that as a change. So even consumers without the latent effect bug would re-render unnecessarily. Why this surfaced now: the prior commit (cbca913, Loyal Agent tab swap) promoted DiagnosticsScreen from a Settings stack child to a top-level tab. The bug existed before but only fired when a user manually navigated to Settings then Diagnostics. As a top-level tab the screen mounts on app open, and the loop kicks off immediately. Fix: Diagnostics.screen.tsx — split into two effects so the fetcher does not depend on what it sets: - Effect A: listActivePlugins() once on mount, flips hasFetched true in finally(). - Effect B: derives pluginPresence from activePlugins whenever it changes, gated on hasFetched so the initial empty-store state does not flash a one-frame "Blox AI not installed" before the response lands. usePluginsStore.ts — defense-in-depth reference-stable update. Only set if the new contents differ from the old. Protects any future consumer that depends on activePlugins by reference, not just Diagnostics. Audited other activePlugins consumers (Plugin.screen.tsx, GlobalBottomSheet.tsx) — neither has a useEffect that both calls listActivePlugins AND depends on activePlugins, so neither had this specific bug. Plugin.screen.tsx's effects depend on listActivePlugins the setter (referentially stable from zustand), not on activePlugins the state. Store dedup still helps them avoid spurious re-renders. Tests: 23/23 Diagnostics jest tests pass. Pre-existing ApprovalModal.test.tsx module-resolve failure is unrelated (jest moduleNameMapper infra; ApprovalModal not touched). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent cbca913 commit 0713f3a

2 files changed

Lines changed: 64 additions & 25 deletions

File tree

apps/box/src/screens/Diagnostics/Diagnostics.screen.tsx

Lines changed: 39 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -94,31 +94,47 @@ export const DiagnosticsScreen: React.FC = () => {
9494

9595
// Plugin presence — tolerant of pre-plugin firmware per Codex review:
9696
// missing data ≠ "not installed", so the UI copy avoids overclaiming.
97+
//
98+
// Split into two effects to avoid the infinite-loop trap:
99+
// (1) fetch once on mount — listActivePlugins() updates the store,
100+
// which would re-fire any effect that depends on activePlugins.
101+
// (2) react to store changes — derives pluginPresence from whatever
102+
// activePlugins currently is. No fetching here, so updating
103+
// activePlugins (whether via the mount fetch, an install action
104+
// elsewhere, or a future polling refresh) flips presence without
105+
// re-triggering the fetch.
106+
// The previous single-effect form (which called listActivePlugins
107+
// inside an effect that depended on activePlugins) looped forever
108+
// because the store creates a fresh `[]` reference on every empty
109+
// result and zustand's shallow equality flags that as a change.
110+
const [hasFetched, setHasFetched] = React.useState(false);
111+
97112
React.useEffect(() => {
98-
let cancelled = false;
99-
(async () => {
100-
try {
101-
await listActivePlugins();
102-
} catch {
113+
listActivePlugins()
114+
.catch(() => {
103115
// listActivePlugins captures its own errors into the store
104-
}
105-
if (cancelled) return;
106-
const list = (activePlugins || []) as string[];
107-
if (!Array.isArray(list)) {
108-
setPluginPresence('notInstalledOrUnavailable');
109-
return;
110-
}
111-
setPluginPresence(
112-
list.includes(BLOX_AI_PLUGIN_NAME)
113-
? 'installed'
114-
: 'notInstalledOrUnavailable'
115-
);
116-
})();
117-
return () => { cancelled = true; };
118-
// listActivePlugins/activePlugins from zustand are stable enough; we want
119-
// this to run on every mount AND when activePlugins flips
120-
// eslint-disable-next-line react-hooks/exhaustive-deps
121-
}, [activePlugins]);
116+
})
117+
.finally(() => setHasFetched(true));
118+
// listActivePlugins is the zustand setter — referentially stable.
119+
// eslint-disable-next-line react-hooks/exhaustive-deps
120+
}, []);
121+
122+
React.useEffect(() => {
123+
// Wait for the mount fetch before deriving presence — without this,
124+
// the initial activePlugins=[] from the store would flash a
125+
// one-frame "Blox AI not installed" before the real response lands.
126+
if (!hasFetched) return;
127+
const list = (activePlugins || []) as string[];
128+
if (!Array.isArray(list)) {
129+
setPluginPresence('notInstalledOrUnavailable');
130+
return;
131+
}
132+
setPluginPresence(
133+
list.includes(BLOX_AI_PLUGIN_NAME)
134+
? 'installed'
135+
: 'notInstalledOrUnavailable'
136+
);
137+
}, [activePlugins, hasFetched]);
122138

123139
return (
124140
<FxKeyboardAwareScrollView>

apps/box/src/stores/usePluginsStore.ts

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -81,13 +81,36 @@ const createPluginsModelSlice: StateCreator<
8181
const result = await fxblox.listActivePlugins();
8282
if (result.status) {
8383
if (result.msg && Array.isArray(result.msg)) {
84-
set({ activePlugins: result.msg });
84+
// Reference-stable update: only `set` if the contents actually
85+
// changed. zustand uses shallow equality; without this guard
86+
// every call produces a new array reference even when the
87+
// contents are identical, which re-renders every subscriber
88+
// and is what caused the Diagnostics-screen listActivePlugins
89+
// infinite loop (effect dep on activePlugins → fetch → new
90+
// [] → state change → effect fires → fetch …).
91+
const next = result.msg as string[];
92+
const prev = get().activePlugins;
93+
const same =
94+
Array.isArray(prev) &&
95+
prev.length === next.length &&
96+
prev.every((p, i) => p === next[i]);
97+
if (!same) {
98+
set({ activePlugins: next });
99+
}
85100
return {
86101
success: true,
87102
message: 'Active plugins listed successfully',
88103
};
89104
} else {
90-
set({ activePlugins: [] });
105+
// Same guard for the empty case: msg=null from go-fula (which
106+
// happens when active-plugins.txt is empty — its filter strips
107+
// empty strings, leaving nil that JSON-marshals to null) used
108+
// to fire `set({ activePlugins: [] })` on every call, producing
109+
// a fresh [] reference and the same re-render storm.
110+
const prev = get().activePlugins;
111+
if (!Array.isArray(prev) || prev.length !== 0) {
112+
set({ activePlugins: [] });
113+
}
91114
return { success: true, message: 'No active plugins found' };
92115
}
93116
} else {

0 commit comments

Comments
 (0)