mirror of
https://github.com/Significant-Gravitas/AutoGPT.git
synced 2026-04-30 03:00:41 -04:00
fix(frontend/builder): re-add navigation race guard for parsedActions
When the user navigates between graphs, the flowID-reset effect resets
`lastParsedMessageIndexRef` and the parsed-actions cache, then queues
`setMessages([])`. The parse-actions effect runs in the same effect
cycle — *before* the queued state updates are committed — so its
`messages` closure still belongs to the previous graph. With the index
reset to -1 and the cache empty, it would re-scan those stale messages
from index 0 and briefly flash the previous graph's actions in the new
panel.
A previous guard (`277c19642`) was lost when commit `1935137c1` (the
DefaultChatTransport memoization fix) accidentally dropped the
`if (currentFlowIDRef.current !== flowID) return;` line. That guard
was actually a no-op because `currentFlowIDRef` is updated by an earlier
effect in the same cycle, so the check never fired — the bug was masked
in practice but came back into view when sentry re-flagged it.
Replace the removed line with a one-shot `skipNextParseRef` flag that
the cleanup effect sets only on *actual* navigation (not initial mount,
detected via `prevFlowIDRef`). The parse-actions effect skips one pass
when the flag is set, then clears it. This correctly handles:
- Initial mount: no skip (flag stays false), first run parses normally.
- Navigation: skip one pass; next render arrives with fresh messages
from useChat's re-key and parses them correctly.
- Same-flowID re-render: cleanup doesn't fire, no skip, normal parse.
New regression test reproduces the navigation race in the parsed-actions
integration suite.
Sentry bug prediction: PRRT_kwDOJKSTjM56RVeU (severity HIGH).
This commit is contained in:
@@ -843,6 +843,46 @@ describe("useBuilderChatPanel – parsedActions integration", () => {
|
||||
|
||||
expect(result.current.parsedActions).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("does NOT re-parse stale messages from the previous graph after navigation (sentry race PRRT_kwDOJKSTjM56RVeU)", () => {
|
||||
// Reproduces the navigation race: when flowID changes, the cleanup
|
||||
// effect resets the parsed-actions cache and queues setMessages([]),
|
||||
// but the parse-actions effect runs in the same effect cycle while
|
||||
// the messages closure still holds the previous graph's messages.
|
||||
// Without the navigation guard, the parser would re-scan those stale
|
||||
// messages from index 0 (because the cache was reset) and populate
|
||||
// parsedActions with the previous graph's actions.
|
||||
const flow1Action =
|
||||
'```json\n{"action":"update_node_input","node_id":"flow-1-node","key":"query","value":"flow-1 value"}\n```';
|
||||
mockChatMessages = [
|
||||
{
|
||||
id: "msg-1",
|
||||
role: "assistant",
|
||||
parts: [{ type: "text", text: flow1Action }],
|
||||
},
|
||||
];
|
||||
mockChatStatus = "ready";
|
||||
mockFlowID = "flow-1";
|
||||
|
||||
const { result, rerender } = renderHook(() => useBuilderChatPanel());
|
||||
|
||||
// Initial mount on flow-1: actions parsed normally.
|
||||
expect(result.current.parsedActions).toHaveLength(1);
|
||||
expect(result.current.parsedActions[0]).toMatchObject({
|
||||
nodeId: "flow-1-node",
|
||||
});
|
||||
|
||||
// Simulate navigation to flow-2. The test mock keeps `mockChatMessages`
|
||||
// pointing at the flow-1 messages (mirroring the real race window where
|
||||
// useChat hasn't yet picked up `setMessages([])`).
|
||||
mockFlowID = "flow-2";
|
||||
rerender();
|
||||
|
||||
// The navigation guard must prevent the parse-actions effect from
|
||||
// re-populating parsedActions with the stale flow-1 message it can
|
||||
// still see in its closure.
|
||||
expect(result.current.parsedActions).toHaveLength(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe("useBuilderChatPanel – Escape key handler", () => {
|
||||
|
||||
@@ -144,6 +144,18 @@ export function useBuilderChatPanel({
|
||||
actions: GraphAction[];
|
||||
seen: Set<string>;
|
||||
}>({ actions: [], seen: new Set() });
|
||||
// Navigation race guard: set by the flowID-reset effect when an *actual*
|
||||
// graph navigation occurs (not initial mount). The parse-actions effect
|
||||
// checks this flag and skips one pass, because the cleanup effect's
|
||||
// `setMessages([])` is queued and not yet committed when parse-actions
|
||||
// runs in the same effect cycle — without the skip, we'd re-scan the
|
||||
// previous graph's messages from index 0 (refs were just reset) and
|
||||
// briefly flash old action buttons in the new graph's panel.
|
||||
const skipNextParseRef = useRef(false);
|
||||
// Tracks the previous flowID so the reset effect can distinguish initial
|
||||
// mount (no skip needed — fresh hook, no stale messages) from real
|
||||
// navigation (skip needed — closure has prior-graph messages).
|
||||
const prevFlowIDRef = useRef<string | null>(null);
|
||||
|
||||
const [{ flowID }, setQueryStates] = useQueryStates({
|
||||
flowID: parseAsString,
|
||||
@@ -169,6 +181,16 @@ export function useBuilderChatPanel({
|
||||
// so restoring messages while resetting action state would show previously applied
|
||||
// actions as unapplied, allowing them to be re-applied and creating duplicate undo entries.
|
||||
useEffect(() => {
|
||||
// Detect actual navigation (not initial mount) so the parse-actions
|
||||
// effect can skip its next pass — see ``skipNextParseRef`` declaration
|
||||
// for the race-condition rationale.
|
||||
const isNavigation =
|
||||
prevFlowIDRef.current !== null && prevFlowIDRef.current !== flowID;
|
||||
prevFlowIDRef.current = flowID;
|
||||
if (isNavigation) {
|
||||
skipNextParseRef.current = true;
|
||||
}
|
||||
|
||||
const cachedSessionId = flowID ? (cacheGetSession(flowID) ?? null) : null;
|
||||
setSessionId(cachedSessionId);
|
||||
setSessionError(false);
|
||||
@@ -326,6 +348,17 @@ export function useBuilderChatPanel({
|
||||
// new messages, losing actions.
|
||||
useEffect(() => {
|
||||
if (status !== "ready") return;
|
||||
// Navigation race guard: the flowID-reset effect above signals a graph
|
||||
// navigation by setting `skipNextParseRef`. The cleanup runs first in the
|
||||
// effect cycle and resets `lastParsedMessageIndexRef` + the cache, but the
|
||||
// `setMessages([])` it queues is not committed until the next render —
|
||||
// so this effect's `messages` closure still belongs to the previous graph.
|
||||
// Skipping one pass prevents a re-scan of stale messages from index 0
|
||||
// (which would flash the prior graph's actions in the new panel).
|
||||
if (skipNextParseRef.current) {
|
||||
skipNextParseRef.current = false;
|
||||
return;
|
||||
}
|
||||
const cache = parsedActionsCacheRef.current;
|
||||
const startIndex = lastParsedMessageIndexRef.current + 1;
|
||||
let appendedAny = false;
|
||||
|
||||
Reference in New Issue
Block a user