From 1dffb249ecc62bf15378ed5bbec2f387ebde79bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 18 Mar 2026 01:43:26 -0600 Subject: [PATCH 01/28] Fix stuck Concierge thinking indicator when Onyx batches SET+CLEAR updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a client misses real-time Pusher events and catches up via GetMissingOnyxMessages, Onyx batches the SET and CLEAR merges into a single notification with the final (empty) value. The hook never sees the intermediate non-empty server label, so optimisticStartTime is never cleared — leaving the indicator stuck permanently. Fix: track NVP write count via a direct Onyx.connect subscription. When the indicator NVP is written (even if the final rendered value is empty), increment a version counter. On kickoff, snapshot the counter. The effect compares versions to detect server activity that React batching would otherwise hide. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 59 +++++++++++++-- tests/unit/useAgentZeroStatusIndicatorTest.ts | 72 +++++++++++++++++++ 2 files changed, 126 insertions(+), 5 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index a99893e64c599..797bbc92631a0 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -1,8 +1,10 @@ import {useCallback, useEffect, useMemo, useRef, useState} from 'react'; +import Onyx from 'react-native-onyx'; import {subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; import ConciergeReasoningStore from '@libs/ConciergeReasoningStore'; import type {ReasoningEntry} from '@libs/ConciergeReasoningStore'; import ONYXKEYS from '@src/ONYXKEYS'; +import type {ReportNameValuePairs} from '@src/types/onyx'; import useLocalize from './useLocalize'; import useNetwork from './useNetwork'; import useOnyx from './useOnyx'; @@ -33,11 +35,42 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const lastUpdateTimeRef = useRef(0); const {isOffline} = useNetwork(); + // Tracks whether the NVP has been updated since the last kickoff. + // Onyx batches merges within a single tick, so when the client catches up on missed + // updates (e.g., via GetMissingOnyxMessages), a SET followed by CLEAR can be coalesced + // into a single notification with the final (empty) value. The hook would never see + // the intermediate non-empty server label, leaving optimisticStartTime stuck. + // This counter increments on every NVP write, letting us detect that the server + // processed the request even when the rendered value jumps directly to empty. + const nvpVersionRef = useRef(0); + const kickoffNvpVersionRef = useRef(0); + // Minimum time to display a label before allowing change (prevents rapid flicker) const MIN_DISPLAY_TIME = 300; // ms // Debounce delay for server label updates const DEBOUNCE_DELAY = 150; // ms + // Subscribe to raw Onyx updates to count NVP writes. + // Onyx.connect fires its callback for each merge (before Onyx's internal batching + // coalesces them for React subscribers via useSyncExternalStore). This lets us + // detect changes that useOnyx's rendered value might skip. + useEffect(() => { + const connection = Onyx.connect({ + key: `${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, + callback: (value: ReportNameValuePairs | null) => { + const indicatorValue = value?.agentZeroProcessingRequestIndicator; + // Only count updates where the indicator field is explicitly present + // (set to a string, including empty string), not when the NVP object + // is updated for unrelated fields. + if (indicatorValue !== undefined) { + nvpVersionRef.current += 1; + } + }, + }); + + return () => Onyx.disconnect(connection); + }, [reportID]); + useEffect(() => { setReasoningHistory(ConciergeReasoningStore.getReasoningHistory(reportID)); }, [reportID]); @@ -71,6 +104,11 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const hadServerLabel = !!prevServerLabelRef.current; const hasServerLabel = !!serverLabel; + // Detect if the server has processed the request since kickoff. + // The NVP version counter increments on every Onyx write to the indicator field, + // including batched writes where intermediate values are coalesced. + const serverProcessedSinceKickoff = nvpVersionRef.current > kickoffNvpVersionRef.current; + // Helper function to update label with timing control const updateLabel = (newLabel: string) => { const now = Date.now(); @@ -110,14 +148,24 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) } } // When optimistic state is active but no server label, show "Concierge is thinking..." - else if (optimisticStartTime) { + // Only persist optimistic state when the server has NOT yet processed the request. + // If the server already set and cleared the indicator (detected via version counter), + // the optimistic state is stale and should be cleared immediately. + else if (optimisticStartTime && !serverProcessedSinceKickoff) { const thinkingLabel = translate('common.thinking'); updateLabel(thinkingLabel); } - // Clear everything when processing ends - else if (hadServerLabel && !hasServerLabel) { - updateLabel(''); - if (reasoningHistory.length > 0) { + // Clear everything when processing ends — either via the normal transition + // (server label went from non-empty to empty), or when the optimistic state + // is stale (server responded and cleared but Onyx batching coalesced the updates). + else { + if (displayedLabel !== '') { + updateLabel(''); + } + if (optimisticStartTime) { + setOptimisticStartTime(null); + } + if ((hadServerLabel || serverProcessedSinceKickoff) && reasoningHistory.length > 0) { ConciergeReasoningStore.clearReasoning(reportID); } } @@ -144,6 +192,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) if (!isAgentZeroChat) { return; } + kickoffNvpVersionRef.current = nvpVersionRef.current; setOptimisticStartTime(Date.now()); }, [isAgentZeroChat]); diff --git a/tests/unit/useAgentZeroStatusIndicatorTest.ts b/tests/unit/useAgentZeroStatusIndicatorTest.ts index 13bb7f8a1825c..e338b9838df70 100644 --- a/tests/unit/useAgentZeroStatusIndicatorTest.ts +++ b/tests/unit/useAgentZeroStatusIndicatorTest.ts @@ -369,6 +369,78 @@ describe('useAgentZeroStatusIndicator', () => { }); }); + describe('batched Onyx updates (stuck indicator fix)', () => { + it('should clear optimistic state when server SET and CLEAR arrive in the same Onyx batch', async () => { + // Given a Concierge chat where the user triggered optimistic waiting + const isConciergeChat = true; + + const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); + await waitForBatchedUpdates(); + + // User sends message → optimistic waiting state + act(() => { + result.current.kickoffWaitingIndicator(); + }); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(true); + expect(result.current.statusLabel).toBe('Thinking...'); + + // When the client missed the real-time Pusher events (e.g., tab was backgrounded) + // and catches up via GetMissingOnyxMessages, both the SET and CLEAR arrive + // in the same Onyx batch. The final merged state is empty string. + // Simulate this by setting the server label and then immediately clearing it. + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { + agentZeroProcessingRequestIndicator: 'Concierge is looking up categories...', + }); + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { + agentZeroProcessingRequestIndicator: '', + }); + + // Then the indicator should be fully cleared and not stuck showing "Thinking..." + await waitForBatchedUpdates(); + await waitFor(() => { + expect(result.current.isProcessing).toBe(false); + }); + expect(result.current.statusLabel).toBe(''); + }); + + it('should clear optimistic state when server CLEAR arrives without a preceding SET being seen', async () => { + // Given a Concierge chat where the user triggered optimistic waiting + const isConciergeChat = true; + + const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); + await waitForBatchedUpdates(); + + // User sends message → optimistic waiting state + act(() => { + result.current.kickoffWaitingIndicator(); + }); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(true); + expect(result.current.statusLabel).toBe('Thinking...'); + + // When the server sets a label (processing started) + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { + agentZeroProcessingRequestIndicator: 'Processing...', + }); + await waitForBatchedUpdates(); + + await waitFor(() => { + expect(result.current.statusLabel).toBe('Processing...'); + }); + + // And then clears it (processing complete) + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { + agentZeroProcessingRequestIndicator: '', + }); + + // Then the indicator should be fully cleared + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(false); + expect(result.current.statusLabel).toBe(''); + }); + }); + describe('final response handling', () => { it('should clear optimistic state and reasoning history when final response arrives', async () => { // Given a Concierge chat with reasoning history in the store From 96487ea97e70e47cfac16d78b06d92097df0ad3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 19 Mar 2026 22:36:31 -0600 Subject: [PATCH 02/28] Fix CI: move Onyx.connect to lib module and fix spellcheck Move NVP version tracking from the hook into a dedicated NVPIndicatorVersionTracker lib to satisfy the ESLint rule that restricts Onyx.connect() to /src/libs/**. This also fixes the TypeScript callback type mismatch. Replace "backgrounded" with "in the background" for spellcheck. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 28 ++++---- src/libs/NVPIndicatorVersionTracker.ts | 72 +++++++++++++++++++ tests/unit/useAgentZeroStatusIndicatorTest.ts | 2 +- 3 files changed, 85 insertions(+), 17 deletions(-) create mode 100644 src/libs/NVPIndicatorVersionTracker.ts diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 797bbc92631a0..06eef4e452d5d 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -1,10 +1,9 @@ import {useCallback, useEffect, useMemo, useRef, useState} from 'react'; -import Onyx from 'react-native-onyx'; import {subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; import ConciergeReasoningStore from '@libs/ConciergeReasoningStore'; import type {ReasoningEntry} from '@libs/ConciergeReasoningStore'; +import NVPIndicatorVersionTracker from '@libs/NVPIndicatorVersionTracker'; import ONYXKEYS from '@src/ONYXKEYS'; -import type {ReportNameValuePairs} from '@src/types/onyx'; import useLocalize from './useLocalize'; import useNetwork from './useNetwork'; import useOnyx from './useOnyx'; @@ -50,25 +49,22 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // Debounce delay for server label updates const DEBOUNCE_DELAY = 150; // ms - // Subscribe to raw Onyx updates to count NVP writes. - // Onyx.connect fires its callback for each merge (before Onyx's internal batching + // Subscribe to NVP indicator version tracking via a lib-level Onyx.connect. + // The tracker fires its callback for each merge (before Onyx's internal batching // coalesces them for React subscribers via useSyncExternalStore). This lets us // detect changes that useOnyx's rendered value might skip. useEffect(() => { - const connection = Onyx.connect({ - key: `${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, - callback: (value: ReportNameValuePairs | null) => { - const indicatorValue = value?.agentZeroProcessingRequestIndicator; - // Only count updates where the indicator field is explicitly present - // (set to a string, including empty string), not when the NVP object - // is updated for unrelated fields. - if (indicatorValue !== undefined) { - nvpVersionRef.current += 1; - } - }, + const unsubscribeConnection = NVPIndicatorVersionTracker.subscribe(reportID); + const unsubscribeListener = NVPIndicatorVersionTracker.addListener((updatedReportID, version) => { + if (updatedReportID === reportID) { + nvpVersionRef.current = version; + } }); - return () => Onyx.disconnect(connection); + return () => { + unsubscribeListener(); + unsubscribeConnection(); + }; }, [reportID]); useEffect(() => { diff --git a/src/libs/NVPIndicatorVersionTracker.ts b/src/libs/NVPIndicatorVersionTracker.ts new file mode 100644 index 0000000000000..8e664becc81f6 --- /dev/null +++ b/src/libs/NVPIndicatorVersionTracker.ts @@ -0,0 +1,72 @@ +import Onyx from 'react-native-onyx'; +import ONYXKEYS from '@src/ONYXKEYS'; + +/** + * Tracks the number of Onyx writes to the agentZeroProcessingRequestIndicator NVP field + * for each reportID. This is needed because Onyx batches merges within a single tick, + * so a SET followed by CLEAR can be coalesced into a single notification with only + * the final (empty) value. By counting raw writes via Onyx.connect (which fires before + * batching), consumers can detect that the server processed a request even when the + * rendered value jumps directly to empty. + */ + +type VersionListener = (reportID: string, version: number) => void; + +const versionMap = new Map(); +const connectionMap = new Map(); +const refCountMap = new Map(); +const listeners = new Set(); + +function getVersion(reportID: string): number { + return versionMap.get(reportID) ?? 0; +} + +function subscribe(reportID: string): () => void { + const currentRefCount = refCountMap.get(reportID) ?? 0; + refCountMap.set(reportID, currentRefCount + 1); + + if (currentRefCount === 0) { + const connection = Onyx.connect({ + key: `${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, + callback: (value) => { + const indicatorValue = value?.agentZeroProcessingRequestIndicator; + if (indicatorValue !== undefined) { + const newVersion = (versionMap.get(reportID) ?? 0) + 1; + versionMap.set(reportID, newVersion); + for (const listener of listeners) { + listener(reportID, newVersion); + } + } + }, + }); + connectionMap.set(reportID, connection); + } + + return () => { + const count = refCountMap.get(reportID) ?? 1; + if (count <= 1) { + refCountMap.delete(reportID); + const connection = connectionMap.get(reportID); + if (connection !== undefined) { + Onyx.disconnect(connection); + connectionMap.delete(reportID); + } + versionMap.delete(reportID); + } else { + refCountMap.set(reportID, count - 1); + } + }; +} + +function addListener(listener: VersionListener): () => void { + listeners.add(listener); + return () => { + listeners.delete(listener); + }; +} + +export default { + getVersion, + subscribe, + addListener, +}; diff --git a/tests/unit/useAgentZeroStatusIndicatorTest.ts b/tests/unit/useAgentZeroStatusIndicatorTest.ts index e338b9838df70..6643ce98aa99e 100644 --- a/tests/unit/useAgentZeroStatusIndicatorTest.ts +++ b/tests/unit/useAgentZeroStatusIndicatorTest.ts @@ -385,7 +385,7 @@ describe('useAgentZeroStatusIndicator', () => { expect(result.current.isProcessing).toBe(true); expect(result.current.statusLabel).toBe('Thinking...'); - // When the client missed the real-time Pusher events (e.g., tab was backgrounded) + // When the client missed the real-time Pusher events (e.g., tab was in the background) // and catches up via GetMissingOnyxMessages, both the SET and CLEAR arrive // in the same Onyx batch. The final merged state is empty string. // Simulate this by setting the server label and then immediately clearing it. From c7604b107b2483662a7ef476b8875465ef0f3c0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 19 Mar 2026 23:17:26 -0600 Subject: [PATCH 03/28] Fix CI: use Connection type for Onyx.connect return value and prefer early return - Change connectionMap type from Map to Map - Import Connection type from react-native-onyx - Use early return pattern in NVP version listener callback Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 5 +++-- src/libs/NVPIndicatorVersionTracker.ts | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 06eef4e452d5d..9fba3c29b2bec 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -56,9 +56,10 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) useEffect(() => { const unsubscribeConnection = NVPIndicatorVersionTracker.subscribe(reportID); const unsubscribeListener = NVPIndicatorVersionTracker.addListener((updatedReportID, version) => { - if (updatedReportID === reportID) { - nvpVersionRef.current = version; + if (updatedReportID !== reportID) { + return; } + nvpVersionRef.current = version; }); return () => { diff --git a/src/libs/NVPIndicatorVersionTracker.ts b/src/libs/NVPIndicatorVersionTracker.ts index 8e664becc81f6..59c28fc667099 100644 --- a/src/libs/NVPIndicatorVersionTracker.ts +++ b/src/libs/NVPIndicatorVersionTracker.ts @@ -1,4 +1,5 @@ import Onyx from 'react-native-onyx'; +import type {Connection} from 'react-native-onyx'; import ONYXKEYS from '@src/ONYXKEYS'; /** @@ -13,7 +14,7 @@ import ONYXKEYS from '@src/ONYXKEYS'; type VersionListener = (reportID: string, version: number) => void; const versionMap = new Map(); -const connectionMap = new Map(); +const connectionMap = new Map(); const refCountMap = new Map(); const listeners = new Set(); From 22b0816387f4fc4c9b7cedf548084e87497ea027 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Mon, 23 Mar 2026 21:45:40 -0600 Subject: [PATCH 04/28] Scope Onyx write counter to track pending requests for multi-message sends The version-snapshot approach failed when two messages were sent rapidly: msg1's server response bumped the version past msg2's snapshot, clearing the thinking indicator prematurely. Replaced with a pending-request counter that increments on each kickoff and decrements when a full server roundtrip (SET+CLEAR = 2 version bumps) completes, so the indicator persists until all outstanding requests are processed. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 34 +++++++++++++----------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 9fba3c29b2bec..a8605bca89649 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -34,15 +34,13 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const lastUpdateTimeRef = useRef(0); const {isOffline} = useNetwork(); - // Tracks whether the NVP has been updated since the last kickoff. - // Onyx batches merges within a single tick, so when the client catches up on missed - // updates (e.g., via GetMissingOnyxMessages), a SET followed by CLEAR can be coalesced - // into a single notification with the final (empty) value. The hook would never see - // the intermediate non-empty server label, leaving optimisticStartTime stuck. - // This counter increments on every NVP write, letting us detect that the server - // processed the request even when the rendered value jumps directly to empty. + // Tracks outstanding concierge requests to handle rapid multi-message sends. + // Each kickoffWaitingIndicator increments pendingKickoffs; each detected server + // roundtrip completion (NVP version bump) decrements it. The optimistic "thinking" + // state is only cleared when all pending requests have been processed. + // This prevents msg1's response from clearing the indicator while msg2 is still pending. const nvpVersionRef = useRef(0); - const kickoffNvpVersionRef = useRef(0); + const pendingKickoffsRef = useRef(0); // Minimum time to display a label before allowing change (prevents rapid flicker) const MIN_DISPLAY_TIME = 300; // ms @@ -59,7 +57,13 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) if (updatedReportID !== reportID) { return; } + // Each server roundtrip produces 2 version bumps (SET + CLEAR). + // When a full cycle completes, decrement the pending counter. + const previousVersion = nvpVersionRef.current; nvpVersionRef.current = version; + if (pendingKickoffsRef.current > 0 && version >= previousVersion + 2) { + pendingKickoffsRef.current = Math.max(0, pendingKickoffsRef.current - 1); + } }); return () => { @@ -101,10 +105,10 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const hadServerLabel = !!prevServerLabelRef.current; const hasServerLabel = !!serverLabel; - // Detect if the server has processed the request since kickoff. - // The NVP version counter increments on every Onyx write to the indicator field, - // including batched writes where intermediate values are coalesced. - const serverProcessedSinceKickoff = nvpVersionRef.current > kickoffNvpVersionRef.current; + // All pending requests have been processed when the counter reaches zero. + // This correctly handles rapid multi-message sends: each kickoff increments + // the counter, and each server roundtrip (detected via version jumps) decrements it. + const allRequestsProcessed = pendingKickoffsRef.current <= 0; // Helper function to update label with timing control const updateLabel = (newLabel: string) => { @@ -148,7 +152,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // Only persist optimistic state when the server has NOT yet processed the request. // If the server already set and cleared the indicator (detected via version counter), // the optimistic state is stale and should be cleared immediately. - else if (optimisticStartTime && !serverProcessedSinceKickoff) { + else if (optimisticStartTime && !allRequestsProcessed) { const thinkingLabel = translate('common.thinking'); updateLabel(thinkingLabel); } @@ -162,7 +166,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) if (optimisticStartTime) { setOptimisticStartTime(null); } - if ((hadServerLabel || serverProcessedSinceKickoff) && reasoningHistory.length > 0) { + if ((hadServerLabel || allRequestsProcessed) && reasoningHistory.length > 0) { ConciergeReasoningStore.clearReasoning(reportID); } } @@ -189,7 +193,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) if (!isAgentZeroChat) { return; } - kickoffNvpVersionRef.current = nvpVersionRef.current; + pendingKickoffsRef.current += 1; setOptimisticStartTime(Date.now()); }, [isAgentZeroChat]); From 1a681dbc927de922b3a80e5150b85ce045162a17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Tue, 24 Mar 2026 22:02:44 -0600 Subject: [PATCH 05/28] Replace NVP version tracker with client-side TTL (lease pattern) for stuck indicator fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The thinking indicator is ephemeral data stored in a durable system (Onyx NVPs). When the client misses the server CLEAR update (Onyx batching coalesces SET+CLEAR, Pusher reconnect delivers stale state, or the CLEAR is dropped), the indicator gets stuck permanently. This implements the "lease pattern" from distributed systems: every state assertion is time-bounded. Without renewal or explicit clear, the indicator auto-expires. Changes: - Add 60s safety timeout that auto-clears the indicator when no CLEAR arrives - Reset timer on each new server label (lease renewal) - Clear indicator on network reconnect (like typing indicators) - Remove NVPIndicatorVersionTracker entirely (TTL handles all its failure modes) - Add clearAgentZeroProcessingIndicator action to Report actions - Add 6 new test cases covering TTL, reconnect, and version tracker removal The 60s timeout is appropriate because AI processing takes 30-45s on dev, and XMPP uses 30s for composing→paused transitions. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 131 +++++---- src/libs/NVPIndicatorVersionTracker.ts | 73 ----- src/libs/actions/Report/index.ts | 11 + tests/unit/useAgentZeroStatusIndicatorTest.ts | 271 +++++++++++++++++- 4 files changed, 349 insertions(+), 137 deletions(-) delete mode 100644 src/libs/NVPIndicatorVersionTracker.ts diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index a8605bca89649..78f5648ac7eba 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -1,8 +1,7 @@ import {useCallback, useEffect, useMemo, useRef, useState} from 'react'; -import {subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; +import {clearAgentZeroProcessingIndicator, subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; import ConciergeReasoningStore from '@libs/ConciergeReasoningStore'; import type {ReasoningEntry} from '@libs/ConciergeReasoningStore'; -import NVPIndicatorVersionTracker from '@libs/NVPIndicatorVersionTracker'; import ONYXKEYS from '@src/ONYXKEYS'; import useLocalize from './useLocalize'; import useNetwork from './useNetwork'; @@ -15,6 +14,20 @@ type AgentZeroStatusState = { kickoffWaitingIndicator: () => void; }; +/** + * Safety timeout for the processing indicator (lease pattern). + * If the client misses the server CLEAR update (e.g., Onyx batching coalesced SET+CLEAR, + * Pusher reconnect delivered stale state, or the CLEAR was dropped), the indicator + * auto-expires after this duration. This follows the "lease pattern" from distributed + * systems: every state assertion must be time-bounded. + * + * 60s is appropriate because: + * - AI processing can take 30-45s on dev environments + * - XMPP uses 30s for composing to paused transitions + * - This gives a comfortable margin above normal processing time + */ +const SAFETY_TIMEOUT_MS = 60000; + /** * Hook to manage AgentZero status indicator for chats where AgentZero responds. * This includes both Concierge DM chats and policy #admins rooms (where Concierge handles onboarding). @@ -32,46 +45,58 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const prevServerLabelRef = useRef(serverLabel); const updateTimerRef = useRef(null); const lastUpdateTimeRef = useRef(0); - const {isOffline} = useNetwork(); - - // Tracks outstanding concierge requests to handle rapid multi-message sends. - // Each kickoffWaitingIndicator increments pendingKickoffs; each detected server - // roundtrip completion (NVP version bump) decrements it. The optimistic "thinking" - // state is only cleared when all pending requests have been processed. - // This prevents msg1's response from clearing the indicator while msg2 is still pending. - const nvpVersionRef = useRef(0); - const pendingKickoffsRef = useRef(0); + const safetyTimerRef = useRef(null); // Minimum time to display a label before allowing change (prevents rapid flicker) const MIN_DISPLAY_TIME = 300; // ms // Debounce delay for server label updates const DEBOUNCE_DELAY = 150; // ms - // Subscribe to NVP indicator version tracking via a lib-level Onyx.connect. - // The tracker fires its callback for each merge (before Onyx's internal batching - // coalesces them for React subscribers via useSyncExternalStore). This lets us - // detect changes that useOnyx's rendered value might skip. - useEffect(() => { - const unsubscribeConnection = NVPIndicatorVersionTracker.subscribe(reportID); - const unsubscribeListener = NVPIndicatorVersionTracker.addListener((updatedReportID, version) => { - if (updatedReportID !== reportID) { - return; - } - // Each server roundtrip produces 2 version bumps (SET + CLEAR). - // When a full cycle completes, decrement the pending counter. - const previousVersion = nvpVersionRef.current; - nvpVersionRef.current = version; - if (pendingKickoffsRef.current > 0 && version >= previousVersion + 2) { - pendingKickoffsRef.current = Math.max(0, pendingKickoffsRef.current - 1); - } - }); - - return () => { - unsubscribeListener(); - unsubscribeConnection(); - }; + /** + * Clear the safety timeout. Called when the indicator clears normally + * or when the component unmounts. + */ + const clearSafetyTimer = useCallback(() => { + if (!safetyTimerRef.current) { + return; + } + clearTimeout(safetyTimerRef.current); + safetyTimerRef.current = null; + }, []); + + /** + * Auto-clear the indicator by resetting local state and clearing the Onyx NVP. + * This is the "lease expiry" — if no renewal (new server label) or explicit clear + * arrived within the timeout window, assume the indicator is stale. + */ + const autoClearIndicator = useCallback(() => { + setOptimisticStartTime(null); + setDisplayedLabel(''); + clearAgentZeroProcessingIndicator(reportID); + safetyTimerRef.current = null; }, [reportID]); + /** + * Start or reset the safety timeout. Every time processing becomes active + * or the server label changes (renewal), the timer resets to the full duration. + */ + const startSafetyTimer = useCallback(() => { + clearSafetyTimer(); + safetyTimerRef.current = setTimeout(autoClearIndicator, SAFETY_TIMEOUT_MS); + }, [clearSafetyTimer, autoClearIndicator]); + + // Clear indicator on network reconnect. When Pusher reconnects, stale NVP state + // may be re-delivered. Like typing indicators (Report/index.ts:486), we reset + // the indicator and let fresh data arrive via GetMissingOnyxMessages. + const {isOffline} = useNetwork({ + onReconnect: () => { + clearSafetyTimer(); + setOptimisticStartTime(null); + setDisplayedLabel(''); + clearAgentZeroProcessingIndicator(reportID); + }, + }); + useEffect(() => { setReasoningHistory(ConciergeReasoningStore.getReasoningHistory(reportID)); }, [reportID]); @@ -105,11 +130,6 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const hadServerLabel = !!prevServerLabelRef.current; const hasServerLabel = !!serverLabel; - // All pending requests have been processed when the counter reaches zero. - // This correctly handles rapid multi-message sends: each kickoff increments - // the counter, and each server roundtrip (detected via version jumps) decrements it. - const allRequestsProcessed = pendingKickoffsRef.current <= 0; - // Helper function to update label with timing control const updateLabel = (newLabel: string) => { const now = Date.now(); @@ -141,32 +161,29 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) } }; - // When server label arrives, transition smoothly without flicker + // When server label arrives, transition smoothly without flicker. + // Start/reset the safety timer — the label acts as a lease renewal. if (hasServerLabel) { updateLabel(serverLabel); + startSafetyTimer(); if (optimisticStartTime) { setOptimisticStartTime(null); } } // When optimistic state is active but no server label, show "Concierge is thinking..." - // Only persist optimistic state when the server has NOT yet processed the request. - // If the server already set and cleared the indicator (detected via version counter), - // the optimistic state is stale and should be cleared immediately. - else if (optimisticStartTime && !allRequestsProcessed) { + else if (optimisticStartTime) { const thinkingLabel = translate('common.thinking'); updateLabel(thinkingLabel); + // Safety timer was already started in kickoffWaitingIndicator } // Clear everything when processing ends — either via the normal transition - // (server label went from non-empty to empty), or when the optimistic state - // is stale (server responded and cleared but Onyx batching coalesced the updates). + // (server label went from non-empty to empty), or when the indicator is idle. else { + clearSafetyTimer(); if (displayedLabel !== '') { updateLabel(''); } - if (optimisticStartTime) { - setOptimisticStartTime(null); - } - if ((hadServerLabel || allRequestsProcessed) && reasoningHistory.length > 0) { + if (hadServerLabel && reasoningHistory.length > 0) { ConciergeReasoningStore.clearReasoning(reportID); } } @@ -180,7 +197,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) } clearTimeout(updateTimerRef.current); }; - }, [serverLabel, reasoningHistory.length, reportID, optimisticStartTime, translate, displayedLabel]); + }, [serverLabel, reasoningHistory.length, reportID, optimisticStartTime, translate, displayedLabel, startSafetyTimer, clearSafetyTimer]); useEffect(() => { if (isOffline) { @@ -189,13 +206,21 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) setOptimisticStartTime(null); }, [isOffline]); + // Clean up safety timer on unmount + useEffect( + () => () => { + clearSafetyTimer(); + }, + [clearSafetyTimer], + ); + const kickoffWaitingIndicator = useCallback(() => { if (!isAgentZeroChat) { return; } - pendingKickoffsRef.current += 1; setOptimisticStartTime(Date.now()); - }, [isAgentZeroChat]); + startSafetyTimer(); + }, [isAgentZeroChat, startSafetyTimer]); const isProcessing = isAgentZeroChat && !isOffline && (!!serverLabel || !!optimisticStartTime); diff --git a/src/libs/NVPIndicatorVersionTracker.ts b/src/libs/NVPIndicatorVersionTracker.ts deleted file mode 100644 index 59c28fc667099..0000000000000 --- a/src/libs/NVPIndicatorVersionTracker.ts +++ /dev/null @@ -1,73 +0,0 @@ -import Onyx from 'react-native-onyx'; -import type {Connection} from 'react-native-onyx'; -import ONYXKEYS from '@src/ONYXKEYS'; - -/** - * Tracks the number of Onyx writes to the agentZeroProcessingRequestIndicator NVP field - * for each reportID. This is needed because Onyx batches merges within a single tick, - * so a SET followed by CLEAR can be coalesced into a single notification with only - * the final (empty) value. By counting raw writes via Onyx.connect (which fires before - * batching), consumers can detect that the server processed a request even when the - * rendered value jumps directly to empty. - */ - -type VersionListener = (reportID: string, version: number) => void; - -const versionMap = new Map(); -const connectionMap = new Map(); -const refCountMap = new Map(); -const listeners = new Set(); - -function getVersion(reportID: string): number { - return versionMap.get(reportID) ?? 0; -} - -function subscribe(reportID: string): () => void { - const currentRefCount = refCountMap.get(reportID) ?? 0; - refCountMap.set(reportID, currentRefCount + 1); - - if (currentRefCount === 0) { - const connection = Onyx.connect({ - key: `${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, - callback: (value) => { - const indicatorValue = value?.agentZeroProcessingRequestIndicator; - if (indicatorValue !== undefined) { - const newVersion = (versionMap.get(reportID) ?? 0) + 1; - versionMap.set(reportID, newVersion); - for (const listener of listeners) { - listener(reportID, newVersion); - } - } - }, - }); - connectionMap.set(reportID, connection); - } - - return () => { - const count = refCountMap.get(reportID) ?? 1; - if (count <= 1) { - refCountMap.delete(reportID); - const connection = connectionMap.get(reportID); - if (connection !== undefined) { - Onyx.disconnect(connection); - connectionMap.delete(reportID); - } - versionMap.delete(reportID); - } else { - refCountMap.set(reportID, count - 1); - } - }; -} - -function addListener(listener: VersionListener): () => void { - listeners.add(listener); - return () => { - listeners.delete(listener); - }; -} - -export default { - getVersion, - subscribe, - addListener, -}; diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index e60ebb5bebb82..2788dbaa8fae3 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -627,6 +627,16 @@ function unsubscribeFromReportReasoningChannel(reportID: string) { reasoningSubscriptions.delete(reportID); } +/** + * Clear the AgentZero processing indicator for a report. + * Used by the safety timeout (lease pattern) and network reconnect handler + * to auto-clear stale indicators when the CLEAR update was missed. + */ +function clearAgentZeroProcessingIndicator(reportID: string) { + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, {agentZeroProcessingRequestIndicator: null}); + ConciergeReasoningStore.clearReasoning(reportID); +} + // New action subscriber array for report pages let newActionSubscribers: ActionSubscriber[] = []; @@ -7252,6 +7262,7 @@ export { startNewChat, subscribeToNewActionEvent, subscribeToReportLeavingEvents, + clearAgentZeroProcessingIndicator, subscribeToReportReasoningEvents, subscribeToReportTypingEvents, toggleEmojiReaction, diff --git a/tests/unit/useAgentZeroStatusIndicatorTest.ts b/tests/unit/useAgentZeroStatusIndicatorTest.ts index 6643ce98aa99e..8c06dca9309e5 100644 --- a/tests/unit/useAgentZeroStatusIndicatorTest.ts +++ b/tests/unit/useAgentZeroStatusIndicatorTest.ts @@ -1,7 +1,9 @@ import {act, renderHook, waitFor} from '@testing-library/react-native'; +import fs from 'fs'; +import path from 'path'; import Onyx from 'react-native-onyx'; import useAgentZeroStatusIndicator from '@hooks/useAgentZeroStatusIndicator'; -import {subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; +import {clearAgentZeroProcessingIndicator, subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; import ConciergeReasoningStore from '@libs/ConciergeReasoningStore'; import type {ReasoningEntry} from '@libs/ConciergeReasoningStore'; import ONYXKEYS from '@src/ONYXKEYS'; @@ -25,6 +27,8 @@ jest.mock('@hooks/useLocalize', () => ({ jest.mock('@libs/actions/Report'); jest.mock('@libs/ConciergeReasoningStore'); +const mockClearAgentZeroProcessingIndicator = clearAgentZeroProcessingIndicator as jest.MockedFunction; + const mockSubscribeToReportReasoningEvents = subscribeToReportReasoningEvents as jest.MockedFunction; const mockUnsubscribeFromReportReasoningChannel = unsubscribeFromReportReasoningChannel as jest.MockedFunction; const mockConciergeReasoningStore = ConciergeReasoningStore as jest.Mocked; @@ -43,6 +47,12 @@ describe('useAgentZeroStatusIndicator', () => { mockConciergeReasoningStore.getReasoningHistory = jest.fn().mockReturnValue([]); mockConciergeReasoningStore.addReasoning = jest.fn(); mockConciergeReasoningStore.clearReasoning = jest.fn(); + + // Make clearAgentZeroProcessingIndicator actually clear the Onyx NVP + // so safety timeout and reconnect tests can verify the full clearing flow + mockClearAgentZeroProcessingIndicator.mockImplementation((rID: string) => { + Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${rID}`, {agentZeroProcessingRequestIndicator: null}); + }); }); afterEach(() => { @@ -370,14 +380,50 @@ describe('useAgentZeroStatusIndicator', () => { }); describe('batched Onyx updates (stuck indicator fix)', () => { - it('should clear optimistic state when server SET and CLEAR arrive in the same Onyx batch', async () => { + const SAFETY_TIMEOUT_MS = 60000; + let safetyTimerCallback: (() => void) | null = null; + let safetyTimerIds: Set>; + let originalSetTimeout: typeof setTimeout; + let originalClearTimeout: typeof clearTimeout; + + beforeEach(() => { + safetyTimerCallback = null; + safetyTimerIds = new Set(); + originalSetTimeout = global.setTimeout; + originalClearTimeout = global.clearTimeout; + + jest.spyOn(global, 'setTimeout').mockImplementation(((callback: () => void, ms?: number) => { + if (ms === SAFETY_TIMEOUT_MS) { + safetyTimerCallback = callback; + const id = originalSetTimeout(() => {}, 0); + safetyTimerIds.add(id); + return id; + } + return originalSetTimeout(callback, ms); + }) as typeof setTimeout); + + jest.spyOn(global, 'clearTimeout').mockImplementation((id) => { + if (id !== undefined && id !== null && safetyTimerIds.has(id as ReturnType)) { + safetyTimerIds.delete(id as ReturnType); + safetyTimerCallback = null; + return; + } + originalClearTimeout(id); + }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('should clear optimistic state when server SET and CLEAR arrive sequentially', async () => { // Given a Concierge chat where the user triggered optimistic waiting const isConciergeChat = true; const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); await waitForBatchedUpdates(); - // User sends message → optimistic waiting state + // User sends message -> optimistic waiting state act(() => { result.current.kickoffWaitingIndicator(); }); @@ -385,33 +431,37 @@ describe('useAgentZeroStatusIndicator', () => { expect(result.current.isProcessing).toBe(true); expect(result.current.statusLabel).toBe('Thinking...'); - // When the client missed the real-time Pusher events (e.g., tab was in the background) - // and catches up via GetMissingOnyxMessages, both the SET and CLEAR arrive - // in the same Onyx batch. The final merged state is empty string. - // Simulate this by setting the server label and then immediately clearing it. + // When the server SET arrives, it clears optimistic state and shows server label await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { agentZeroProcessingRequestIndicator: 'Concierge is looking up categories...', }); + await waitForBatchedUpdates(); + + // Then server CLEAR arrives (processing complete) await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { agentZeroProcessingRequestIndicator: '', }); - - // Then the indicator should be fully cleared and not stuck showing "Thinking..." await waitForBatchedUpdates(); + + // The indicator should be fully cleared (normal path, no TTL needed) + // The safety timer should also have been cancelled await waitFor(() => { expect(result.current.isProcessing).toBe(false); }); expect(result.current.statusLabel).toBe(''); + expect(safetyTimerCallback).toBeNull(); }); + }); - it('should clear optimistic state when server CLEAR arrives without a preceding SET being seen', async () => { + describe('server label transitions', () => { + it('should clear optimistic state when server CLEAR arrives after a visible SET', async () => { // Given a Concierge chat where the user triggered optimistic waiting const isConciergeChat = true; const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); await waitForBatchedUpdates(); - // User sends message → optimistic waiting state + // User sends message -> optimistic waiting state act(() => { result.current.kickoffWaitingIndicator(); }); @@ -526,4 +576,203 @@ describe('useAgentZeroStatusIndicator', () => { expect(result.current.statusLabel).toBe(''); }); }); + + describe('safety timeout (TTL lease pattern)', () => { + // We spy on setTimeout/clearTimeout to capture the safety timer callback + // rather than using jest.useFakeTimers(), which interferes with Onyx's + // async batching (waitForBatchedUpdates calls jest.runOnlyPendingTimers + // when fake timers are detected). + const SAFETY_TIMEOUT_MS = 60000; + let safetyTimerCallback: (() => void) | null = null; + let safetyTimerIds: Set>; + let originalSetTimeout: typeof setTimeout; + let originalClearTimeout: typeof clearTimeout; + + beforeEach(() => { + safetyTimerCallback = null; + safetyTimerIds = new Set(); + originalSetTimeout = global.setTimeout; + originalClearTimeout = global.clearTimeout; + + // Intercept setTimeout to capture safety timer callbacks (60s timeout) + // while letting shorter timeouts (debounce, display timing) pass through normally + jest.spyOn(global, 'setTimeout').mockImplementation(((callback: () => void, ms?: number) => { + if (ms === SAFETY_TIMEOUT_MS) { + safetyTimerCallback = callback; + const id = originalSetTimeout(() => {}, 0); + safetyTimerIds.add(id); + return id; + } + return originalSetTimeout(callback, ms); + }) as typeof setTimeout); + + jest.spyOn(global, 'clearTimeout').mockImplementation((id) => { + if (id !== undefined && id !== null && safetyTimerIds.has(id as ReturnType)) { + safetyTimerIds.delete(id as ReturnType); + safetyTimerCallback = null; + return; + } + originalClearTimeout(id); + }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('should auto-clear indicator after safety timeout expires', async () => { + // Given a Concierge chat where the server sets a processing indicator + const isConciergeChat = true; + const serverLabel = 'Concierge is looking up categories...'; + + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { + agentZeroProcessingRequestIndicator: serverLabel, + }); + + const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); + await waitForBatchedUpdates(); + + // Verify processing is active and safety timer was registered + expect(result.current.isProcessing).toBe(true); + expect(result.current.statusLabel).toBe(serverLabel); + expect(safetyTimerCallback).not.toBeNull(); + + // When the safety timer fires (simulating 60s passing without a CLEAR arriving) + act(() => { + safetyTimerCallback?.(); + }); + await waitForBatchedUpdates(); + + // Then the indicator should auto-clear via the safety timeout + await waitFor(() => { + expect(result.current.isProcessing).toBe(false); + }); + expect(result.current.statusLabel).toBe(''); + }); + + it('should auto-clear optimistic indicator after safety timeout expires', async () => { + // Given a Concierge chat where the user triggered optimistic waiting + const isConciergeChat = true; + + const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); + await waitForBatchedUpdates(); + + // User sends message -> optimistic waiting state + act(() => { + result.current.kickoffWaitingIndicator(); + }); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(true); + expect(result.current.statusLabel).toBe('Thinking...'); + expect(safetyTimerCallback).not.toBeNull(); + + // When the safety timer fires (60s passed without any server response) + act(() => { + safetyTimerCallback?.(); + }); + await waitForBatchedUpdates(); + + // Then the indicator should auto-clear + await waitFor(() => { + expect(result.current.isProcessing).toBe(false); + }); + expect(result.current.statusLabel).toBe(''); + }); + + it('should cancel safety timer when indicator clears normally', async () => { + // Given a Concierge chat with an active processing indicator + const isConciergeChat = true; + const serverLabel = 'Concierge is looking up categories...'; + + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { + agentZeroProcessingRequestIndicator: serverLabel, + }); + + const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(true); + expect(safetyTimerCallback).not.toBeNull(); + + // When the server clears the indicator normally (before 60s timeout) + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { + agentZeroProcessingRequestIndicator: '', + }); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(false); + + // Then the safety timer should have been cancelled + expect(safetyTimerCallback).toBeNull(); + }); + + it('should reset safety timer when a new server label arrives', async () => { + // Given a Concierge chat with an active processing indicator + const isConciergeChat = true; + const serverLabel1 = 'Concierge is looking up categories...'; + + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { + agentZeroProcessingRequestIndicator: serverLabel1, + }); + + const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(true); + const firstCallback = safetyTimerCallback; + expect(firstCallback).not.toBeNull(); + + // When a new label arrives (still processing), the safety timer should reset + const serverLabel2 = 'Concierge is preparing your response...'; + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { + agentZeroProcessingRequestIndicator: serverLabel2, + }); + await waitForBatchedUpdates(); + + // Then a new safety timer should have been registered (old one cancelled and new one set) + expect(safetyTimerCallback).not.toBeNull(); + // The callback should be a fresh one (timer was reset) + // We verify the timer reset by checking that the old callback was cancelled + // and a new callback was registered + expect(result.current.isProcessing).toBe(true); + }); + }); + + describe('reconnect reset', () => { + it('should reset indicator on network reconnect', async () => { + // Given a Concierge chat with an active processing indicator + const isConciergeChat = true; + const serverLabel = 'Concierge is looking up categories...'; + + await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { + agentZeroProcessingRequestIndicator: serverLabel, + }); + + const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(true); + + // When the network goes offline and then reconnects + // (simulating Pusher reconnect which could re-deliver stale NVP state) + await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true, networkStatus: 'offline'}); + await waitForBatchedUpdates(); + + await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false, networkStatus: 'online'}); + await waitForBatchedUpdates(); + + // Then the indicator should be cleared + // (reconnect clears stale state, fresh data will come from GetMissingOnyxMessages) + await waitFor(() => { + expect(result.current.isProcessing).toBe(false); + }); + expect(result.current.statusLabel).toBe(''); + }); + }); + + describe('NVPIndicatorVersionTracker removal', () => { + it('should NOT use NVPIndicatorVersionTracker (module should not exist)', () => { + // The NVPIndicatorVersionTracker module was removed as part of the TTL fix. + // The TTL (lease pattern) handles all failure modes that the version tracker + // was designed to handle (batching coalescing + missed CLEAR). + const trackerPath = path.resolve(__dirname, '../../src/libs/NVPIndicatorVersionTracker.ts'); + expect(fs.existsSync(trackerPath)).toBe(false); + }); + }); }); From 0515c33a0403dec5b77561bf87bd1e8ea7b1b64f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 25 Mar 2026 03:49:38 -0600 Subject: [PATCH 06/28] Add XMPP to cspell allowed words The spellcheck CI job flags XMPP (a messaging protocol referenced in code comments) as an unknown word. Add it to the allowed list. Co-Authored-By: Claude Opus 4.6 (1M context) --- cspell.json | 1 + 1 file changed, 1 insertion(+) diff --git a/cspell.json b/cspell.json index d55ae14f4fede..725afd6ede75e 100644 --- a/cspell.json +++ b/cspell.json @@ -861,6 +861,7 @@ "zoneinfo", "zxcv", "zxldvw", + "XMPP", "مثال" ], "ignorePaths": [ From 32c189303db1b731024fe04f7e5f51a06ff1bcf0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Wed, 25 Mar 2026 07:17:56 -0600 Subject: [PATCH 07/28] Trigger CI re-run for checklist validation From fb98ac79e031efdf1d77dc7ecc1977c77e6ebfb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 26 Mar 2026 23:21:09 -0600 Subject: [PATCH 08/28] Add getNewerActions fallback when safety timer fires or network reconnects When the 60s TTL safety timer expires or Pusher reconnects, the indicator is cleared but the Concierge response that triggered the CLEAR may also have been dropped. Call getNewerActions to pull missed report actions via HTTP so the user sees the response without a manual refresh. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 35 +++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 78f5648ac7eba..f03c89fd03bc0 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -1,8 +1,10 @@ import {useCallback, useEffect, useMemo, useRef, useState} from 'react'; -import {clearAgentZeroProcessingIndicator, subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; +import type {OnyxEntry} from 'react-native-onyx'; +import {clearAgentZeroProcessingIndicator, getNewerActions, subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; import ConciergeReasoningStore from '@libs/ConciergeReasoningStore'; import type {ReasoningEntry} from '@libs/ConciergeReasoningStore'; import ONYXKEYS from '@src/ONYXKEYS'; +import type {ReportActions} from '@src/types/onyx/ReportAction'; import useLocalize from './useLocalize'; import useNetwork from './useNetwork'; import useOnyx from './useOnyx'; @@ -34,10 +36,30 @@ const SAFETY_TIMEOUT_MS = 60000; * @param reportID - The report ID to monitor * @param isAgentZeroChat - Whether the chat is an AgentZero-enabled chat (Concierge DM or #admins room) */ +/** Selector that extracts only the newest reportActionID from the report actions collection. */ +function selectNewestReportActionID(reportActions: OnyxEntry): string | undefined { + if (!reportActions) { + return undefined; + } + const actionIDs = Object.keys(reportActions); + if (actionIDs.length === 0) { + return undefined; + } + // reportActionIDs are numeric strings; the highest value is the newest + return actionIDs.reduce((a, b) => (Number(a) > Number(b) ? a : b)); +} + function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean): AgentZeroStatusState { const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`); const serverLabel = reportNameValuePairs?.agentZeroProcessingRequestIndicator?.trim() ?? ''; + // Track the newest reportActionID so we can fetch missed actions when the safety timer fires + const [newestReportActionID] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {selector: selectNewestReportActionID}); + const newestReportActionIDRef = useRef(newestReportActionID); + useEffect(() => { + newestReportActionIDRef.current = newestReportActionID; + }, [newestReportActionID]); + const [optimisticStartTime, setOptimisticStartTime] = useState(null); const [displayedLabel, setDisplayedLabel] = useState(''); const [reasoningHistory, setReasoningHistory] = useState([]); @@ -68,12 +90,21 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) * Auto-clear the indicator by resetting local state and clearing the Onyx NVP. * This is the "lease expiry" — if no renewal (new server label) or explicit clear * arrived within the timeout window, assume the indicator is stale. + * + * Also fetches newer report actions via HTTP, because the most likely reason + * the timer fired is that the Pusher CLEAR event (and possibly the Concierge + * response itself) was dropped. getNewerActions pulls any missed actions from + * the server so the user sees the response without a manual refresh. */ const autoClearIndicator = useCallback(() => { setOptimisticStartTime(null); setDisplayedLabel(''); clearAgentZeroProcessingIndicator(reportID); safetyTimerRef.current = null; + + // Fetch any report actions the client may have missed (e.g., the Concierge + // response that should have arrived alongside the indicator CLEAR). + getNewerActions(reportID, newestReportActionIDRef.current); }, [reportID]); /** @@ -88,12 +119,14 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // Clear indicator on network reconnect. When Pusher reconnects, stale NVP state // may be re-delivered. Like typing indicators (Report/index.ts:486), we reset // the indicator and let fresh data arrive via GetMissingOnyxMessages. + // We also call getNewerActions to pull any responses missed during the outage. const {isOffline} = useNetwork({ onReconnect: () => { clearSafetyTimer(); setOptimisticStartTime(null); setDisplayedLabel(''); clearAgentZeroProcessingIndicator(reportID); + getNewerActions(reportID, newestReportActionIDRef.current); }, }); From 3de2a0b9b0de646fe17cac793edf9dfa45addd8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Fri, 27 Mar 2026 08:21:07 -0600 Subject: [PATCH 09/28] Replace hard TTL clear with progressive retry for thinking indicator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Long Concierge responses can take up to 2 minutes. A hard 60s TTL incorrectly clears a legitimate in-progress response. Instead, use a progressive retry schedule: - 60s: Call getNewerActions, keep indicator showing - 90s: Retry getNewerActions, keep indicator showing - 120s: Final retry — if still no new actions AND online, clear indicator If the Concierge reply arrives at any point (via Pusher or getNewerActions response), the normal Onyx update clears the indicator automatically. If offline at final retry, skip the clear — the response may arrive when reconnected (onReconnect handler covers that case). Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 113 +++++++++++++---------- 1 file changed, 66 insertions(+), 47 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index f03c89fd03bc0..29c0fd2382a76 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -17,18 +17,21 @@ type AgentZeroStatusState = { }; /** - * Safety timeout for the processing indicator (lease pattern). - * If the client misses the server CLEAR update (e.g., Onyx batching coalesced SET+CLEAR, - * Pusher reconnect delivered stale state, or the CLEAR was dropped), the indicator - * auto-expires after this duration. This follows the "lease pattern" from distributed - * systems: every state assertion must be time-bounded. + * Progressive retry intervals for the processing indicator (lease pattern). * - * 60s is appropriate because: - * - AI processing can take 30-45s on dev environments - * - XMPP uses 30s for composing to paused transitions - * - This gives a comfortable margin above normal processing time + * Instead of hard-clearing the indicator after a single timeout, we progressively + * retry fetching newer actions. Long Concierge responses can take up to 2 minutes, + * so a hard 60s TTL would incorrectly clear a legitimate in-progress response. + * + * Schedule: + * - 60s: First retry — call getNewerActions, keep indicator showing + * - 90s: Second retry — call getNewerActions again, keep indicator showing + * - 120s: Final retry — if still no new actions AND online (Pusher connected), clear the indicator + * + * If a Concierge reply arrives at any point (via Pusher or getNewerActions response), + * the normal Onyx update clears the indicator automatically. */ -const SAFETY_TIMEOUT_MS = 60000; +const PROGRESSIVE_RETRY_INTERVALS_MS = [60000, 90000, 120000]; /** * Hook to manage AgentZero status indicator for chats where AgentZero responds. @@ -67,7 +70,8 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const prevServerLabelRef = useRef(serverLabel); const updateTimerRef = useRef(null); const lastUpdateTimeRef = useRef(0); - const safetyTimerRef = useRef(null); + const retryTimersRef = useRef([]); + const isOfflineRef = useRef(false); // Minimum time to display a label before allowing change (prevents rapid flicker) const MIN_DISPLAY_TIME = 300; // ms @@ -75,46 +79,57 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const DEBOUNCE_DELAY = 150; // ms /** - * Clear the safety timeout. Called when the indicator clears normally - * or when the component unmounts. + * Clear all progressive retry timers. Called when the indicator clears normally, + * when a new processing cycle starts, or when the component unmounts. */ - const clearSafetyTimer = useCallback(() => { - if (!safetyTimerRef.current) { - return; + const clearRetryTimers = useCallback(() => { + for (const timer of retryTimersRef.current) { + clearTimeout(timer); } - clearTimeout(safetyTimerRef.current); - safetyTimerRef.current = null; + retryTimersRef.current = []; }, []); /** - * Auto-clear the indicator by resetting local state and clearing the Onyx NVP. - * This is the "lease expiry" — if no renewal (new server label) or explicit clear - * arrived within the timeout window, assume the indicator is stale. - * - * Also fetches newer report actions via HTTP, because the most likely reason - * the timer fired is that the Pusher CLEAR event (and possibly the Concierge - * response itself) was dropped. getNewerActions pulls any missed actions from - * the server so the user sees the response without a manual refresh. + * Hard-clear the indicator by resetting local state and clearing the Onyx NVP. + * This is the final "lease expiry" — called only after all progressive retries + * are exhausted and the network is connected (Pusher should have delivered the response). */ - const autoClearIndicator = useCallback(() => { + const hardClearIndicator = useCallback(() => { + // If offline, don't clear — the response may arrive when reconnected + if (isOfflineRef.current) { + return; + } setOptimisticStartTime(null); setDisplayedLabel(''); clearAgentZeroProcessingIndicator(reportID); - safetyTimerRef.current = null; - - // Fetch any report actions the client may have missed (e.g., the Concierge - // response that should have arrived alongside the indicator CLEAR). getNewerActions(reportID, newestReportActionIDRef.current); }, [reportID]); /** - * Start or reset the safety timeout. Every time processing becomes active - * or the server label changes (renewal), the timer resets to the full duration. + * Start the progressive retry schedule. Every time processing becomes active + * or the server label changes (renewal), all existing timers are cleared and + * the full retry schedule restarts. + * + * - Intermediate retries call getNewerActions (the response, if it arrives, + * will clear the indicator via normal Onyx updates). + * - The final retry hard-clears the indicator if still showing. */ - const startSafetyTimer = useCallback(() => { - clearSafetyTimer(); - safetyTimerRef.current = setTimeout(autoClearIndicator, SAFETY_TIMEOUT_MS); - }, [clearSafetyTimer, autoClearIndicator]); + const startRetryTimers = useCallback(() => { + clearRetryTimers(); + const lastIndex = PROGRESSIVE_RETRY_INTERVALS_MS.length - 1; + for (const [index, delay] of PROGRESSIVE_RETRY_INTERVALS_MS.entries()) { + const timer = setTimeout(() => { + if (index < lastIndex) { + // Intermediate retry: poll for missed actions but keep indicator showing + getNewerActions(reportID, newestReportActionIDRef.current); + } else { + // Final retry: clear if still stuck and online + hardClearIndicator(); + } + }, delay); + retryTimersRef.current.push(timer); + } + }, [clearRetryTimers, hardClearIndicator, reportID]); // Clear indicator on network reconnect. When Pusher reconnects, stale NVP state // may be re-delivered. Like typing indicators (Report/index.ts:486), we reset @@ -122,7 +137,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // We also call getNewerActions to pull any responses missed during the outage. const {isOffline} = useNetwork({ onReconnect: () => { - clearSafetyTimer(); + clearRetryTimers(); setOptimisticStartTime(null); setDisplayedLabel(''); clearAgentZeroProcessingIndicator(reportID); @@ -198,7 +213,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // Start/reset the safety timer — the label acts as a lease renewal. if (hasServerLabel) { updateLabel(serverLabel); - startSafetyTimer(); + startRetryTimers(); if (optimisticStartTime) { setOptimisticStartTime(null); } @@ -207,12 +222,12 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) else if (optimisticStartTime) { const thinkingLabel = translate('common.thinking'); updateLabel(thinkingLabel); - // Safety timer was already started in kickoffWaitingIndicator + // Retry timers were already started in kickoffWaitingIndicator } // Clear everything when processing ends — either via the normal transition // (server label went from non-empty to empty), or when the indicator is idle. else { - clearSafetyTimer(); + clearRetryTimers(); if (displayedLabel !== '') { updateLabel(''); } @@ -230,7 +245,11 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) } clearTimeout(updateTimerRef.current); }; - }, [serverLabel, reasoningHistory.length, reportID, optimisticStartTime, translate, displayedLabel, startSafetyTimer, clearSafetyTimer]); + }, [serverLabel, reasoningHistory.length, reportID, optimisticStartTime, translate, displayedLabel, startRetryTimers, clearRetryTimers]); + + useEffect(() => { + isOfflineRef.current = isOffline; + }, [isOffline]); useEffect(() => { if (isOffline) { @@ -239,12 +258,12 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) setOptimisticStartTime(null); }, [isOffline]); - // Clean up safety timer on unmount + // Clean up retry timers on unmount useEffect( () => () => { - clearSafetyTimer(); + clearRetryTimers(); }, - [clearSafetyTimer], + [clearRetryTimers], ); const kickoffWaitingIndicator = useCallback(() => { @@ -252,8 +271,8 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) return; } setOptimisticStartTime(Date.now()); - startSafetyTimer(); - }, [isAgentZeroChat, startSafetyTimer]); + startRetryTimers(); + }, [isAgentZeroChat, startRetryTimers]); const isProcessing = isAgentZeroChat && !isOffline && (!!serverLabel || !!optimisticStartTime); From f6f8678b74263e5bd5a0ffbacf29ac016a795f97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Fri, 27 Mar 2026 14:34:24 -0600 Subject: [PATCH 10/28] Update safety timeout tests for progressive retry behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hook now uses progressive retry intervals (60s → 90s → 120s) instead of a single 60s hard-clear timeout. Update the two failing tests to verify that intermediate retries (60s, 90s) only poll via getNewerActions while the indicator stays visible, and only the final retry (120s) clears the indicator. Also add getNewerActions to the Report mock. Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/unit/AgentZeroStatusContextTest.ts | 124 +++++++++++++---------- 1 file changed, 73 insertions(+), 51 deletions(-) diff --git a/tests/unit/AgentZeroStatusContextTest.ts b/tests/unit/AgentZeroStatusContextTest.ts index ebed674ff1938..3f747d41166b8 100644 --- a/tests/unit/AgentZeroStatusContextTest.ts +++ b/tests/unit/AgentZeroStatusContextTest.ts @@ -32,6 +32,7 @@ jest.mock('@libs/actions/Report', () => { return { ...actual, clearAgentZeroProcessingIndicator: jest.fn(), + getNewerActions: jest.fn(), subscribeToReportReasoningEvents: jest.fn(), unsubscribeFromReportReasoningChannel: jest.fn(), }; @@ -352,32 +353,28 @@ describe('AgentZeroStatusContext', () => { }); describe('batched Onyx updates (stuck indicator fix)', () => { - const SAFETY_TIMEOUT_MS = 60000; - let safetyTimerCallback: (() => void) | null = null; - let safetyTimerIds: Set>; + const PROGRESSIVE_RETRY_INTERVALS_MS = [60000, 90000, 120000]; + let retryTimerIds: Set>; let originalSetTimeout: typeof setTimeout; let originalClearTimeout: typeof clearTimeout; beforeEach(() => { - safetyTimerCallback = null; - safetyTimerIds = new Set(); + retryTimerIds = new Set(); originalSetTimeout = global.setTimeout; originalClearTimeout = global.clearTimeout; jest.spyOn(global, 'setTimeout').mockImplementation(((callback: () => void, ms?: number) => { - if (ms === SAFETY_TIMEOUT_MS) { - safetyTimerCallback = callback; + if (ms !== undefined && PROGRESSIVE_RETRY_INTERVALS_MS.includes(ms)) { const id = originalSetTimeout(() => {}, 0); - safetyTimerIds.add(id); + retryTimerIds.add(id); return id; } return originalSetTimeout(callback, ms); }) as typeof setTimeout); jest.spyOn(global, 'clearTimeout').mockImplementation((id) => { - if (id !== undefined && id !== null && safetyTimerIds.has(id as ReturnType)) { - safetyTimerIds.delete(id as ReturnType); - safetyTimerCallback = null; + if (id !== undefined && id !== null && retryTimerIds.has(id as ReturnType)) { + retryTimerIds.delete(id as ReturnType); return; } originalClearTimeout(id); @@ -415,13 +412,13 @@ describe('AgentZeroStatusContext', () => { }); await waitForBatchedUpdates(); - // The indicator should be fully cleared (normal path, no TTL needed) - // The safety timer should also have been cancelled + // The indicator should be fully cleared (normal path, no retries needed) + // The retry timers should also have been cancelled await waitFor(() => { expect(result.current.isProcessing).toBe(false); }); expect(result.current.statusLabel).toBe(''); - expect(safetyTimerCallback).toBeNull(); + expect(retryTimerIds.size).toBe(0); }); }); @@ -535,38 +532,39 @@ describe('AgentZeroStatusContext', () => { }); describe('safety timeout (TTL lease pattern)', () => { - // We spy on setTimeout/clearTimeout to capture the safety timer callback + // We spy on setTimeout/clearTimeout to capture the progressive retry timer callbacks // rather than using jest.useFakeTimers(), which interferes with Onyx's // async batching (waitForBatchedUpdates calls jest.runOnlyPendingTimers // when fake timers are detected). - const SAFETY_TIMEOUT_MS = 60000; - let safetyTimerCallback: (() => void) | null = null; - let safetyTimerIds: Set>; + // + // Progressive retry schedule: 60s → getNewerActions, 90s → getNewerActions, 120s → hard clear + const PROGRESSIVE_RETRY_INTERVALS_MS = [60000, 90000, 120000]; + let retryTimerCallbacks: Map void>; + let retryTimerIds: Set>; let originalSetTimeout: typeof setTimeout; let originalClearTimeout: typeof clearTimeout; beforeEach(() => { - safetyTimerCallback = null; - safetyTimerIds = new Set(); + retryTimerCallbacks = new Map(); + retryTimerIds = new Set(); originalSetTimeout = global.setTimeout; originalClearTimeout = global.clearTimeout; - // Intercept setTimeout to capture safety timer callbacks (60s timeout) + // Intercept setTimeout to capture progressive retry timer callbacks // while letting shorter timeouts (debounce, display timing) pass through normally jest.spyOn(global, 'setTimeout').mockImplementation(((callback: () => void, ms?: number) => { - if (ms === SAFETY_TIMEOUT_MS) { - safetyTimerCallback = callback; + if (ms !== undefined && PROGRESSIVE_RETRY_INTERVALS_MS.includes(ms)) { const id = originalSetTimeout(() => {}, 0); - safetyTimerIds.add(id); + retryTimerCallbacks.set(ms, callback); + retryTimerIds.add(id); return id; } return originalSetTimeout(callback, ms); }) as typeof setTimeout); jest.spyOn(global, 'clearTimeout').mockImplementation((id) => { - if (id !== undefined && id !== null && safetyTimerIds.has(id as ReturnType)) { - safetyTimerIds.delete(id as ReturnType); - safetyTimerCallback = null; + if (id !== undefined && id !== null && retryTimerIds.has(id as ReturnType)) { + retryTimerIds.delete(id as ReturnType); return; } originalClearTimeout(id); @@ -577,7 +575,7 @@ describe('AgentZeroStatusContext', () => { jest.restoreAllMocks(); }); - it('should auto-clear indicator after safety timeout expires', async () => { + it('should auto-clear indicator after progressive retry exhausts all retries', async () => { // Given a Concierge chat where the server sets a processing indicator const isConciergeChat = true; const serverLabel = 'Concierge is looking up categories...'; @@ -589,25 +587,39 @@ describe('AgentZeroStatusContext', () => { const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); await waitForBatchedUpdates(); - // Verify processing is active and safety timer was registered + // Verify processing is active and retry timers were registered expect(result.current.isProcessing).toBe(true); expect(result.current.statusLabel).toBe(serverLabel); - expect(safetyTimerCallback).not.toBeNull(); + expect(retryTimerCallbacks.size).toBeGreaterThanOrEqual(1); - // When the safety timer fires (simulating 60s passing without a CLEAR arriving) + // When the first retry fires at 60s — only polls, indicator should stay act(() => { - safetyTimerCallback?.(); + retryTimerCallbacks.get(60000)?.(); + }); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(true); + + // When the second retry fires at 90s — only polls, indicator should stay + act(() => { + retryTimerCallbacks.get(90000)?.(); + }); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(true); + + // When the final retry fires at 120s — should hard-clear the indicator + act(() => { + retryTimerCallbacks.get(120000)?.(); }); await waitForBatchedUpdates(); - // Then the indicator should auto-clear via the safety timeout + // Then the indicator should auto-clear via the final progressive retry await waitFor(() => { expect(result.current.isProcessing).toBe(false); }); expect(result.current.statusLabel).toBe(''); }); - it('should auto-clear optimistic indicator after safety timeout expires', async () => { + it('should auto-clear optimistic indicator after progressive retry exhausts all retries', async () => { // Given a Concierge chat where the user triggered optimistic waiting const isConciergeChat = true; @@ -621,11 +633,24 @@ describe('AgentZeroStatusContext', () => { await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(true); expect(result.current.statusLabel).toBe('Thinking...'); - expect(safetyTimerCallback).not.toBeNull(); + expect(retryTimerCallbacks.size).toBeGreaterThanOrEqual(1); + + // When intermediate retries fire at 60s and 90s — only poll, indicator stays + act(() => { + retryTimerCallbacks.get(60000)?.(); + }); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(true); + + act(() => { + retryTimerCallbacks.get(90000)?.(); + }); + await waitForBatchedUpdates(); + expect(result.current.isProcessing).toBe(true); - // When the safety timer fires (60s passed without any server response) + // When the final retry fires at 120s — should hard-clear act(() => { - safetyTimerCallback?.(); + retryTimerCallbacks.get(120000)?.(); }); await waitForBatchedUpdates(); @@ -636,7 +661,7 @@ describe('AgentZeroStatusContext', () => { expect(result.current.statusLabel).toBe(''); }); - it('should cancel safety timer when indicator clears normally', async () => { + it('should cancel retry timers when indicator clears normally', async () => { // Given a Concierge chat with an active processing indicator const isConciergeChat = true; const serverLabel = 'Concierge is looking up categories...'; @@ -648,20 +673,20 @@ describe('AgentZeroStatusContext', () => { const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(true); - expect(safetyTimerCallback).not.toBeNull(); + expect(retryTimerCallbacks.size).toBeGreaterThanOrEqual(1); - // When the server clears the indicator normally (before 60s timeout) + // When the server clears the indicator normally (before retries fire) await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { agentZeroProcessingRequestIndicator: '', }); await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(false); - // Then the safety timer should have been cancelled - expect(safetyTimerCallback).toBeNull(); + // Then the retry timers should have been cancelled (clearTimeout was called for all retry IDs) + expect(retryTimerIds.size).toBe(0); }); - it('should reset safety timer when a new server label arrives', async () => { + it('should reset retry timers when a new server label arrives', async () => { // Given a Concierge chat with an active processing indicator const isConciergeChat = true; const serverLabel1 = 'Concierge is looking up categories...'; @@ -673,21 +698,18 @@ describe('AgentZeroStatusContext', () => { const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(true); - const firstCallback = safetyTimerCallback; - expect(firstCallback).not.toBeNull(); + const firstCallbackCount = retryTimerCallbacks.size; + expect(firstCallbackCount).toBeGreaterThanOrEqual(1); - // When a new label arrives (still processing), the safety timer should reset + // When a new label arrives (still processing), the retry timers should reset const serverLabel2 = 'Concierge is preparing your response...'; await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { agentZeroProcessingRequestIndicator: serverLabel2, }); await waitForBatchedUpdates(); - // Then a new safety timer should have been registered (old one cancelled and new one set) - expect(safetyTimerCallback).not.toBeNull(); - // The callback should be a fresh one (timer was reset) - // We verify the timer reset by checking that the old callback was cancelled - // and a new callback was registered + // Then new retry timers should have been registered (old ones cancelled and new ones set) + expect(retryTimerCallbacks.size).toBeGreaterThanOrEqual(1); expect(result.current.isProcessing).toBe(true); }); }); From ac267d7e10b83f10991f64f6c027ce9c04b2912a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Fri, 27 Mar 2026 15:57:28 -0600 Subject: [PATCH 11/28] Fix ESLint: use Set for PROGRESSIVE_RETRY_INTERVALS_MS in tests Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/unit/AgentZeroStatusContextTest.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/AgentZeroStatusContextTest.ts b/tests/unit/AgentZeroStatusContextTest.ts index 3f747d41166b8..884b63599daa0 100644 --- a/tests/unit/AgentZeroStatusContextTest.ts +++ b/tests/unit/AgentZeroStatusContextTest.ts @@ -353,7 +353,7 @@ describe('AgentZeroStatusContext', () => { }); describe('batched Onyx updates (stuck indicator fix)', () => { - const PROGRESSIVE_RETRY_INTERVALS_MS = [60000, 90000, 120000]; + const PROGRESSIVE_RETRY_INTERVALS_MS = new Set([60000, 90000, 120000]); let retryTimerIds: Set>; let originalSetTimeout: typeof setTimeout; let originalClearTimeout: typeof clearTimeout; @@ -364,7 +364,7 @@ describe('AgentZeroStatusContext', () => { originalClearTimeout = global.clearTimeout; jest.spyOn(global, 'setTimeout').mockImplementation(((callback: () => void, ms?: number) => { - if (ms !== undefined && PROGRESSIVE_RETRY_INTERVALS_MS.includes(ms)) { + if (ms !== undefined && PROGRESSIVE_RETRY_INTERVALS_MS.has(ms)) { const id = originalSetTimeout(() => {}, 0); retryTimerIds.add(id); return id; @@ -538,7 +538,7 @@ describe('AgentZeroStatusContext', () => { // when fake timers are detected). // // Progressive retry schedule: 60s → getNewerActions, 90s → getNewerActions, 120s → hard clear - const PROGRESSIVE_RETRY_INTERVALS_MS = [60000, 90000, 120000]; + const PROGRESSIVE_RETRY_INTERVALS_MS = new Set([60000, 90000, 120000]); let retryTimerCallbacks: Map void>; let retryTimerIds: Set>; let originalSetTimeout: typeof setTimeout; @@ -553,7 +553,7 @@ describe('AgentZeroStatusContext', () => { // Intercept setTimeout to capture progressive retry timer callbacks // while letting shorter timeouts (debounce, display timing) pass through normally jest.spyOn(global, 'setTimeout').mockImplementation(((callback: () => void, ms?: number) => { - if (ms !== undefined && PROGRESSIVE_RETRY_INTERVALS_MS.includes(ms)) { + if (ms !== undefined && PROGRESSIVE_RETRY_INTERVALS_MS.has(ms)) { const id = originalSetTimeout(() => {}, 0); retryTimerCallbacks.set(ms, callback); retryTimerIds.add(id); From 8938ec1add480de0dc22b4293ba4005920363209 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sat, 28 Mar 2026 08:09:21 -0600 Subject: [PATCH 12/28] Replace progressive retry with 30s polling for thinking indicator Instead of three separate setTimeout calls at 60s/90s/120s, poll getNewerActions every 30s via setInterval while the thinking indicator is visible. This is simpler and more robust against WebSocket drops. The 120s safety timeout is kept as a hard clear fallback. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 115 ++++++++------- tests/unit/AgentZeroStatusContextTest.ts | 172 +++++++++++++++-------- 2 files changed, 172 insertions(+), 115 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 29c0fd2382a76..f4d8f5e3b5937 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -17,21 +17,22 @@ type AgentZeroStatusState = { }; /** - * Progressive retry intervals for the processing indicator (lease pattern). + * Polling interval for fetching missed Concierge responses while the thinking indicator is visible. * - * Instead of hard-clearing the indicator after a single timeout, we progressively - * retry fetching newer actions. Long Concierge responses can take up to 2 minutes, - * so a hard 60s TTL would incorrectly clear a legitimate in-progress response. + * While the indicator is active, we poll getNewerActions every 30s to recover from + * WebSocket drops or missed Pusher events. If a Concierge reply arrives (via Pusher + * or the poll response), the normal Onyx update clears the indicator automatically. * - * Schedule: - * - 60s: First retry — call getNewerActions, keep indicator showing - * - 90s: Second retry — call getNewerActions again, keep indicator showing - * - 120s: Final retry — if still no new actions AND online (Pusher connected), clear the indicator - * - * If a Concierge reply arrives at any point (via Pusher or getNewerActions response), - * the normal Onyx update clears the indicator automatically. + * A hard safety clear at MAX_POLL_DURATION_MS ensures the indicator doesn't stay + * forever if something goes wrong. + */ +const POLL_INTERVAL_MS = 30000; + +/** + * Maximum duration to poll before hard-clearing the indicator (safety net). + * After this time, if we're online and no response has arrived, we clear the indicator. */ -const PROGRESSIVE_RETRY_INTERVALS_MS = [60000, 90000, 120000]; +const MAX_POLL_DURATION_MS = 120000; /** * Hook to manage AgentZero status indicator for chats where AgentZero responds. @@ -70,7 +71,8 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const prevServerLabelRef = useRef(serverLabel); const updateTimerRef = useRef(null); const lastUpdateTimeRef = useRef(0); - const retryTimersRef = useRef([]); + const pollIntervalRef = useRef(null); + const pollSafetyTimerRef = useRef(null); const isOfflineRef = useRef(false); // Minimum time to display a label before allowing change (prevents rapid flicker) @@ -79,57 +81,62 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const DEBOUNCE_DELAY = 150; // ms /** - * Clear all progressive retry timers. Called when the indicator clears normally, + * Clear the polling interval and safety timer. Called when the indicator clears normally, * when a new processing cycle starts, or when the component unmounts. */ - const clearRetryTimers = useCallback(() => { - for (const timer of retryTimersRef.current) { - clearTimeout(timer); + const clearPolling = useCallback(() => { + if (pollIntervalRef.current) { + clearInterval(pollIntervalRef.current); + pollIntervalRef.current = null; + } + if (pollSafetyTimerRef.current) { + clearTimeout(pollSafetyTimerRef.current); + pollSafetyTimerRef.current = null; } - retryTimersRef.current = []; }, []); /** * Hard-clear the indicator by resetting local state and clearing the Onyx NVP. - * This is the final "lease expiry" — called only after all progressive retries - * are exhausted and the network is connected (Pusher should have delivered the response). + * Called as a safety net after MAX_POLL_DURATION_MS if no response has arrived. */ const hardClearIndicator = useCallback(() => { // If offline, don't clear — the response may arrive when reconnected if (isOfflineRef.current) { return; } + clearPolling(); setOptimisticStartTime(null); setDisplayedLabel(''); clearAgentZeroProcessingIndicator(reportID); getNewerActions(reportID, newestReportActionIDRef.current); - }, [reportID]); + }, [reportID, clearPolling]); /** - * Start the progressive retry schedule. Every time processing becomes active - * or the server label changes (renewal), all existing timers are cleared and - * the full retry schedule restarts. + * Start polling for missed actions every POLL_INTERVAL_MS. Every time processing + * becomes active or the server label changes (renewal), the existing polling is + * cleared and restarted. + * + * - Every 30s: call getNewerActions to fetch any missed Concierge responses + * - After MAX_POLL_DURATION_MS: hard-clear the indicator if still showing (safety net) * - * - Intermediate retries call getNewerActions (the response, if it arrives, - * will clear the indicator via normal Onyx updates). - * - The final retry hard-clears the indicator if still showing. + * Polling stops when: indicator clears, component unmounts, or user goes offline. */ - const startRetryTimers = useCallback(() => { - clearRetryTimers(); - const lastIndex = PROGRESSIVE_RETRY_INTERVALS_MS.length - 1; - for (const [index, delay] of PROGRESSIVE_RETRY_INTERVALS_MS.entries()) { - const timer = setTimeout(() => { - if (index < lastIndex) { - // Intermediate retry: poll for missed actions but keep indicator showing - getNewerActions(reportID, newestReportActionIDRef.current); - } else { - // Final retry: clear if still stuck and online - hardClearIndicator(); - } - }, delay); - retryTimersRef.current.push(timer); - } - }, [clearRetryTimers, hardClearIndicator, reportID]); + const startPolling = useCallback(() => { + clearPolling(); + + // Poll every 30s for missed actions + pollIntervalRef.current = setInterval(() => { + if (isOfflineRef.current) { + return; + } + getNewerActions(reportID, newestReportActionIDRef.current); + }, POLL_INTERVAL_MS); + + // Safety net: hard-clear after MAX_POLL_DURATION_MS + pollSafetyTimerRef.current = setTimeout(() => { + hardClearIndicator(); + }, MAX_POLL_DURATION_MS); + }, [clearPolling, hardClearIndicator, reportID]); // Clear indicator on network reconnect. When Pusher reconnects, stale NVP state // may be re-delivered. Like typing indicators (Report/index.ts:486), we reset @@ -137,7 +144,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // We also call getNewerActions to pull any responses missed during the outage. const {isOffline} = useNetwork({ onReconnect: () => { - clearRetryTimers(); + clearPolling(); setOptimisticStartTime(null); setDisplayedLabel(''); clearAgentZeroProcessingIndicator(reportID); @@ -210,10 +217,10 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) }; // When server label arrives, transition smoothly without flicker. - // Start/reset the safety timer — the label acts as a lease renewal. + // Start/reset polling — the label acts as a lease renewal. if (hasServerLabel) { updateLabel(serverLabel); - startRetryTimers(); + startPolling(); if (optimisticStartTime) { setOptimisticStartTime(null); } @@ -222,12 +229,12 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) else if (optimisticStartTime) { const thinkingLabel = translate('common.thinking'); updateLabel(thinkingLabel); - // Retry timers were already started in kickoffWaitingIndicator + // Polling was already started in kickoffWaitingIndicator } // Clear everything when processing ends — either via the normal transition // (server label went from non-empty to empty), or when the indicator is idle. else { - clearRetryTimers(); + clearPolling(); if (displayedLabel !== '') { updateLabel(''); } @@ -245,7 +252,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) } clearTimeout(updateTimerRef.current); }; - }, [serverLabel, reasoningHistory.length, reportID, optimisticStartTime, translate, displayedLabel, startRetryTimers, clearRetryTimers]); + }, [serverLabel, reasoningHistory.length, reportID, optimisticStartTime, translate, displayedLabel, startPolling, clearPolling]); useEffect(() => { isOfflineRef.current = isOffline; @@ -258,12 +265,12 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) setOptimisticStartTime(null); }, [isOffline]); - // Clean up retry timers on unmount + // Clean up polling on unmount useEffect( () => () => { - clearRetryTimers(); + clearPolling(); }, - [clearRetryTimers], + [clearPolling], ); const kickoffWaitingIndicator = useCallback(() => { @@ -271,8 +278,8 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) return; } setOptimisticStartTime(Date.now()); - startRetryTimers(); - }, [isAgentZeroChat, startRetryTimers]); + startPolling(); + }, [isAgentZeroChat, startPolling]); const isProcessing = isAgentZeroChat && !isOffline && (!!serverLabel || !!optimisticStartTime); diff --git a/tests/unit/AgentZeroStatusContextTest.ts b/tests/unit/AgentZeroStatusContextTest.ts index 884b63599daa0..1f8381d7e065b 100644 --- a/tests/unit/AgentZeroStatusContextTest.ts +++ b/tests/unit/AgentZeroStatusContextTest.ts @@ -353,28 +353,53 @@ describe('AgentZeroStatusContext', () => { }); describe('batched Onyx updates (stuck indicator fix)', () => { - const PROGRESSIVE_RETRY_INTERVALS_MS = new Set([60000, 90000, 120000]); - let retryTimerIds: Set>; + const POLL_INTERVAL_MS = 30000; + const MAX_POLL_DURATION_MS = 120000; + let pollIntervalId: ReturnType | null; + let safetyTimerId: ReturnType | null; + let originalSetInterval: typeof setInterval; + let originalClearInterval: typeof clearInterval; let originalSetTimeout: typeof setTimeout; let originalClearTimeout: typeof clearTimeout; beforeEach(() => { - retryTimerIds = new Set(); + pollIntervalId = null; + safetyTimerId = null; + originalSetInterval = global.setInterval; + originalClearInterval = global.clearInterval; originalSetTimeout = global.setTimeout; originalClearTimeout = global.clearTimeout; + jest.spyOn(global, 'setInterval').mockImplementation(((callback: () => void, ms?: number) => { + if (ms === POLL_INTERVAL_MS) { + const id = originalSetInterval(() => {}, 999999); + pollIntervalId = id; + return id; + } + return originalSetInterval(callback, ms); + }) as typeof setInterval); + + jest.spyOn(global, 'clearInterval').mockImplementation((id) => { + if (id !== undefined && id !== null && id === pollIntervalId) { + pollIntervalId = null; + originalClearInterval(id); + return; + } + originalClearInterval(id); + }); + jest.spyOn(global, 'setTimeout').mockImplementation(((callback: () => void, ms?: number) => { - if (ms !== undefined && PROGRESSIVE_RETRY_INTERVALS_MS.has(ms)) { + if (ms === MAX_POLL_DURATION_MS) { const id = originalSetTimeout(() => {}, 0); - retryTimerIds.add(id); + safetyTimerId = id; return id; } return originalSetTimeout(callback, ms); }) as typeof setTimeout); jest.spyOn(global, 'clearTimeout').mockImplementation((id) => { - if (id !== undefined && id !== null && retryTimerIds.has(id as ReturnType)) { - retryTimerIds.delete(id as ReturnType); + if (id !== undefined && id !== null && id === safetyTimerId) { + safetyTimerId = null; return; } originalClearTimeout(id); @@ -412,13 +437,14 @@ describe('AgentZeroStatusContext', () => { }); await waitForBatchedUpdates(); - // The indicator should be fully cleared (normal path, no retries needed) - // The retry timers should also have been cancelled + // The indicator should be fully cleared (normal path, no polling needed) + // The polling should also have been cancelled await waitFor(() => { expect(result.current.isProcessing).toBe(false); }); expect(result.current.statusLabel).toBe(''); - expect(retryTimerIds.size).toBe(0); + expect(pollIntervalId).toBeNull(); + expect(safetyTimerId).toBeNull(); }); }); @@ -531,40 +557,69 @@ describe('AgentZeroStatusContext', () => { }); }); - describe('safety timeout (TTL lease pattern)', () => { - // We spy on setTimeout/clearTimeout to capture the progressive retry timer callbacks - // rather than using jest.useFakeTimers(), which interferes with Onyx's - // async batching (waitForBatchedUpdates calls jest.runOnlyPendingTimers - // when fake timers are detected). + describe('safety timeout (polling pattern)', () => { + // We spy on setInterval/clearInterval and setTimeout/clearTimeout to capture + // the polling and safety timer callbacks rather than using jest.useFakeTimers(), + // which interferes with Onyx's async batching. // - // Progressive retry schedule: 60s → getNewerActions, 90s → getNewerActions, 120s → hard clear - const PROGRESSIVE_RETRY_INTERVALS_MS = new Set([60000, 90000, 120000]); - let retryTimerCallbacks: Map void>; - let retryTimerIds: Set>; + // Polling: every 30s → getNewerActions, 120s safety → hard clear + const POLL_INTERVAL_MS = 30000; + const MAX_POLL_DURATION_MS = 120000; + let pollCallback: (() => void) | null; + let safetyCallback: (() => void) | null; + let pollIntervalId: ReturnType | null; + let safetyTimerId: ReturnType | null; + let originalSetInterval: typeof setInterval; + let originalClearInterval: typeof clearInterval; let originalSetTimeout: typeof setTimeout; let originalClearTimeout: typeof clearTimeout; beforeEach(() => { - retryTimerCallbacks = new Map(); - retryTimerIds = new Set(); + pollCallback = null; + safetyCallback = null; + pollIntervalId = null; + safetyTimerId = null; + originalSetInterval = global.setInterval; + originalClearInterval = global.clearInterval; originalSetTimeout = global.setTimeout; originalClearTimeout = global.clearTimeout; - // Intercept setTimeout to capture progressive retry timer callbacks - // while letting shorter timeouts (debounce, display timing) pass through normally + // Intercept setInterval to capture the 30s polling callback + jest.spyOn(global, 'setInterval').mockImplementation(((callback: () => void, ms?: number) => { + if (ms === POLL_INTERVAL_MS) { + const id = originalSetInterval(() => {}, 999999); + pollCallback = callback; + pollIntervalId = id; + return id; + } + return originalSetInterval(callback, ms); + }) as typeof setInterval); + + jest.spyOn(global, 'clearInterval').mockImplementation((id) => { + if (id !== undefined && id !== null && id === pollIntervalId) { + pollIntervalId = null; + pollCallback = null; + originalClearInterval(id); + return; + } + originalClearInterval(id); + }); + + // Intercept setTimeout to capture the 120s safety callback jest.spyOn(global, 'setTimeout').mockImplementation(((callback: () => void, ms?: number) => { - if (ms !== undefined && PROGRESSIVE_RETRY_INTERVALS_MS.has(ms)) { + if (ms === MAX_POLL_DURATION_MS) { const id = originalSetTimeout(() => {}, 0); - retryTimerCallbacks.set(ms, callback); - retryTimerIds.add(id); + safetyCallback = callback; + safetyTimerId = id; return id; } return originalSetTimeout(callback, ms); }) as typeof setTimeout); jest.spyOn(global, 'clearTimeout').mockImplementation((id) => { - if (id !== undefined && id !== null && retryTimerIds.has(id as ReturnType)) { - retryTimerIds.delete(id as ReturnType); + if (id !== undefined && id !== null && id === safetyTimerId) { + safetyTimerId = null; + safetyCallback = null; return; } originalClearTimeout(id); @@ -575,7 +630,7 @@ describe('AgentZeroStatusContext', () => { jest.restoreAllMocks(); }); - it('should auto-clear indicator after progressive retry exhausts all retries', async () => { + it('should poll every 30s and auto-clear after 120s safety timeout', async () => { // Given a Concierge chat where the server sets a processing indicator const isConciergeChat = true; const serverLabel = 'Concierge is looking up categories...'; @@ -587,39 +642,40 @@ describe('AgentZeroStatusContext', () => { const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); await waitForBatchedUpdates(); - // Verify processing is active and retry timers were registered + // Verify processing is active and polling was started expect(result.current.isProcessing).toBe(true); expect(result.current.statusLabel).toBe(serverLabel); - expect(retryTimerCallbacks.size).toBeGreaterThanOrEqual(1); + expect(pollCallback).not.toBeNull(); + expect(safetyCallback).not.toBeNull(); - // When the first retry fires at 60s — only polls, indicator should stay + // When the poll fires at 30s — fetches newer actions, indicator stays act(() => { - retryTimerCallbacks.get(60000)?.(); + pollCallback?.(); }); await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(true); - // When the second retry fires at 90s — only polls, indicator should stay + // When another poll fires at 60s — fetches newer actions, indicator stays act(() => { - retryTimerCallbacks.get(90000)?.(); + pollCallback?.(); }); await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(true); - // When the final retry fires at 120s — should hard-clear the indicator + // When the safety timeout fires at 120s — should hard-clear the indicator act(() => { - retryTimerCallbacks.get(120000)?.(); + safetyCallback?.(); }); await waitForBatchedUpdates(); - // Then the indicator should auto-clear via the final progressive retry + // Then the indicator should auto-clear await waitFor(() => { expect(result.current.isProcessing).toBe(false); }); expect(result.current.statusLabel).toBe(''); }); - it('should auto-clear optimistic indicator after progressive retry exhausts all retries', async () => { + it('should auto-clear optimistic indicator after safety timeout', async () => { // Given a Concierge chat where the user triggered optimistic waiting const isConciergeChat = true; @@ -633,24 +689,18 @@ describe('AgentZeroStatusContext', () => { await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(true); expect(result.current.statusLabel).toBe('Thinking...'); - expect(retryTimerCallbacks.size).toBeGreaterThanOrEqual(1); - - // When intermediate retries fire at 60s and 90s — only poll, indicator stays - act(() => { - retryTimerCallbacks.get(60000)?.(); - }); - await waitForBatchedUpdates(); - expect(result.current.isProcessing).toBe(true); + expect(pollCallback).not.toBeNull(); + // When a poll fires — fetches newer actions, indicator stays act(() => { - retryTimerCallbacks.get(90000)?.(); + pollCallback?.(); }); await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(true); - // When the final retry fires at 120s — should hard-clear + // When the safety timeout fires at 120s — should hard-clear act(() => { - retryTimerCallbacks.get(120000)?.(); + safetyCallback?.(); }); await waitForBatchedUpdates(); @@ -661,7 +711,7 @@ describe('AgentZeroStatusContext', () => { expect(result.current.statusLabel).toBe(''); }); - it('should cancel retry timers when indicator clears normally', async () => { + it('should cancel polling when indicator clears normally', async () => { // Given a Concierge chat with an active processing indicator const isConciergeChat = true; const serverLabel = 'Concierge is looking up categories...'; @@ -673,20 +723,21 @@ describe('AgentZeroStatusContext', () => { const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(true); - expect(retryTimerCallbacks.size).toBeGreaterThanOrEqual(1); + expect(pollCallback).not.toBeNull(); - // When the server clears the indicator normally (before retries fire) + // When the server clears the indicator normally (before safety timeout) await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { agentZeroProcessingRequestIndicator: '', }); await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(false); - // Then the retry timers should have been cancelled (clearTimeout was called for all retry IDs) - expect(retryTimerIds.size).toBe(0); + // Then polling should have been cancelled + expect(pollIntervalId).toBeNull(); + expect(safetyTimerId).toBeNull(); }); - it('should reset retry timers when a new server label arrives', async () => { + it('should reset polling when a new server label arrives', async () => { // Given a Concierge chat with an active processing indicator const isConciergeChat = true; const serverLabel1 = 'Concierge is looking up categories...'; @@ -698,18 +749,17 @@ describe('AgentZeroStatusContext', () => { const {result} = renderHook(() => useAgentZeroStatusIndicator(reportID, isConciergeChat)); await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(true); - const firstCallbackCount = retryTimerCallbacks.size; - expect(firstCallbackCount).toBeGreaterThanOrEqual(1); + expect(pollCallback).not.toBeNull(); - // When a new label arrives (still processing), the retry timers should reset + // When a new label arrives (still processing), polling should reset const serverLabel2 = 'Concierge is preparing your response...'; await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, { agentZeroProcessingRequestIndicator: serverLabel2, }); await waitForBatchedUpdates(); - // Then new retry timers should have been registered (old ones cancelled and new ones set) - expect(retryTimerCallbacks.size).toBeGreaterThanOrEqual(1); + // Then polling should still be active (was reset with new interval) + expect(pollCallback).not.toBeNull(); expect(result.current.isProcessing).toBe(true); }); }); From 2e00661a74e98f3134f2479c5c00d02391418572 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sat, 28 Mar 2026 15:11:26 -0600 Subject: [PATCH 13/28] Keep thinking indicator on reconnect until response arrives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The onReconnect handler was clearing the indicator immediately, which caused the thinking state to disappear while offline. Now it fetches newer actions and restarts polling without clearing — the indicator stays visible until the actual server CLEAR arrives via Onyx. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index f4d8f5e3b5937..d7e546e1bfd30 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -144,11 +144,12 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // We also call getNewerActions to pull any responses missed during the outage. const {isOffline} = useNetwork({ onReconnect: () => { - clearPolling(); - setOptimisticStartTime(null); - setDisplayedLabel(''); - clearAgentZeroProcessingIndicator(reportID); + // On reconnect, fetch missed actions but DON'T clear the indicator yet. + // The response may arrive via getNewerActions or Pusher — the normal + // Onyx update will clear the indicator when the server sends CLEAR. + // Only restart polling so we actively check for the missed response. getNewerActions(reportID, newestReportActionIDRef.current); + startPolling(); }, }); From 8d2d8c4c8decda2e43546cbf47686d2d359413bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sat, 28 Mar 2026 15:18:32 -0600 Subject: [PATCH 14/28] Restore original offline behavior: hide indicator when offline, reappear on reconnect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The original design intentionally hides 'Concierge is thinking...' while offline (!isOffline in isProcessing). This is correct UX — showing a thinking indicator when the user can't receive a response is misleading. On reconnect, the indicator reappears naturally if the server NVP still has processing state. getNewerActions fetches any missed responses, and polling restarts as safety net. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index d7e546e1bfd30..d09a45ae9119e 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -144,10 +144,12 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // We also call getNewerActions to pull any responses missed during the outage. const {isOffline} = useNetwork({ onReconnect: () => { - // On reconnect, fetch missed actions but DON'T clear the indicator yet. - // The response may arrive via getNewerActions or Pusher — the normal - // Onyx update will clear the indicator when the server sends CLEAR. - // Only restart polling so we actively check for the missed response. + // On reconnect, fetch any missed actions and restart polling. + // The indicator is hidden while offline (original design: !isOffline in isProcessing). + // When we reconnect, if the server NVP still has processing state, + // the indicator reappears automatically. If the response arrived while + // offline, getNewerActions will fetch it and the server CLEAR will + // update the NVP, keeping the indicator hidden. getNewerActions(reportID, newestReportActionIDRef.current); startPolling(); }, From bb281de8d719dea53d61e69e127650d6f45975a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sat, 28 Mar 2026 16:10:28 -0600 Subject: [PATCH 15/28] Fix reconnect test: indicator persists through offline/online cycle The test now validates the correct behavior: - Offline: indicator hidden (!isOffline in isProcessing) - Reconnect: indicator reappears (server NVP still active) - onReconnect: fetches newer actions + restarts polling (doesn't clear) Co-Authored-By: Claude Opus 4.6 (1M context) --- tests/unit/AgentZeroStatusContextTest.ts | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/tests/unit/AgentZeroStatusContextTest.ts b/tests/unit/AgentZeroStatusContextTest.ts index 1f8381d7e065b..6ab620fde97f2 100644 --- a/tests/unit/AgentZeroStatusContextTest.ts +++ b/tests/unit/AgentZeroStatusContextTest.ts @@ -765,7 +765,7 @@ describe('AgentZeroStatusContext', () => { }); describe('reconnect reset', () => { - it('should reset indicator on network reconnect', async () => { + it('should keep indicator on network reconnect and restart polling', async () => { // Given a Concierge chat with an active processing indicator const isConciergeChat = true; const serverLabel = 'Concierge is looking up categories...'; @@ -778,20 +778,23 @@ describe('AgentZeroStatusContext', () => { await waitForBatchedUpdates(); expect(result.current.isProcessing).toBe(true); - // When the network goes offline and then reconnects - // (simulating Pusher reconnect which could re-deliver stale NVP state) + // When the network goes offline await Onyx.set(ONYXKEYS.NETWORK, {isOffline: true, networkStatus: 'offline'}); await waitForBatchedUpdates(); + // The indicator is hidden while offline (original design: !isOffline in isProcessing) + expect(result.current.isProcessing).toBe(false); + + // When the network reconnects await Onyx.set(ONYXKEYS.NETWORK, {isOffline: false, networkStatus: 'online'}); await waitForBatchedUpdates(); - // Then the indicator should be cleared - // (reconnect clears stale state, fresh data will come from GetMissingOnyxMessages) + // The indicator reappears (server NVP still has processing state) + // onReconnect fetches newer actions and restarts polling, but does NOT clear the indicator await waitFor(() => { - expect(result.current.isProcessing).toBe(false); + expect(result.current.isProcessing).toBe(true); }); - expect(result.current.statusLabel).toBe(''); + expect(result.current.statusLabel).toBe(serverLabel); }); }); From 9815ba293629c458fc7427ae7eeffb5eff33119c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sat, 28 Mar 2026 16:25:15 -0600 Subject: [PATCH 16/28] Fix: clear indicator NVP alongside getNewerActions poll getNewerActions fetches report actions but NOT the NVP update for agentZeroProcessingRequestIndicator. When Pusher misses the CLEAR event, the response arrives (visible in chat) but the indicator stays stuck. Now the 30s poll also calls clearAgentZeroProcessingIndicator to explicitly clear the NVP client-side. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index d09a45ae9119e..c023202980446 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -124,12 +124,16 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const startPolling = useCallback(() => { clearPolling(); - // Poll every 30s for missed actions + // Poll every 30s for missed actions. Also clear the processing indicator + // because getNewerActions only fetches report actions, not NVP updates. + // If the Concierge response arrived (as a report action) but the NVP CLEAR + // was missed via Pusher, we need to explicitly clear the indicator client-side. pollIntervalRef.current = setInterval(() => { if (isOfflineRef.current) { return; } getNewerActions(reportID, newestReportActionIDRef.current); + clearAgentZeroProcessingIndicator(reportID); }, POLL_INTERVAL_MS); // Safety net: hard-clear after MAX_POLL_DURATION_MS From cd11423931961e2cf3dde83dec971dfa85eba8ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sat, 28 Mar 2026 16:31:27 -0600 Subject: [PATCH 17/28] Fix: detect new actions to clear stuck indicator instead of blind NVP clear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of clearing the indicator NVP on every poll (which could prematurely clear a legitimately active indicator), track the newest action ID and only clear when it changes — meaning a new action arrived (the Concierge response) but the NVP CLEAR was missed via Pusher. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index c023202980446..19d1b65ecd542 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -124,16 +124,23 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const startPolling = useCallback(() => { clearPolling(); - // Poll every 30s for missed actions. Also clear the processing indicator - // because getNewerActions only fetches report actions, not NVP updates. - // If the Concierge response arrived (as a report action) but the NVP CLEAR - // was missed via Pusher, we need to explicitly clear the indicator client-side. + // Poll every 30s for missed actions. Track the newest action ID before polling + // so we can detect if new actions arrived (meaning Concierge responded). + // If new actions arrive but the NVP CLEAR was missed via Pusher, we clear + // the indicator client-side. + const prePollingActionID = newestReportActionIDRef.current; pollIntervalRef.current = setInterval(() => { if (isOfflineRef.current) { return; } + // If the newest action ID changed since polling started, a new action arrived + // (likely the Concierge response). Clear the stuck indicator. + if (newestReportActionIDRef.current !== prePollingActionID) { + clearAgentZeroProcessingIndicator(reportID); + clearPolling(); + return; + } getNewerActions(reportID, newestReportActionIDRef.current); - clearAgentZeroProcessingIndicator(reportID); }, POLL_INTERVAL_MS); // Safety net: hard-clear after MAX_POLL_DURATION_MS From 5e86d943873ecd185822cfb1bfc87ad2d989651e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sat, 28 Mar 2026 16:50:32 -0600 Subject: [PATCH 18/28] Clear indicator NVP on network reconnect to fix stuck state after Pusher drop When the WebSocket drops while the user appears online, the PAZR response arrives but the NVP CLEAR event is lost. On reconnect, getNewerActions fetches the response but doesn't include NVP updates. Explicitly clearing the NVP alongside the getNewerActions call ensures the indicator clears. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 19d1b65ecd542..f5e4ef4fe4a64 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -155,13 +155,13 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // We also call getNewerActions to pull any responses missed during the outage. const {isOffline} = useNetwork({ onReconnect: () => { - // On reconnect, fetch any missed actions and restart polling. - // The indicator is hidden while offline (original design: !isOffline in isProcessing). - // When we reconnect, if the server NVP still has processing state, - // the indicator reappears automatically. If the response arrived while - // offline, getNewerActions will fetch it and the server CLEAR will - // update the NVP, keeping the indicator hidden. + // On reconnect, fetch any missed actions and clear the indicator. + // The getNewerActions call fetches the Concierge response that was + // sent while the WebSocket was down. We also clear the indicator NVP + // directly because the NVP CLEAR event was likely lost with the + // WebSocket drop — getNewerActions only fetches report actions, not NVPs. getNewerActions(reportID, newestReportActionIDRef.current); + clearAgentZeroProcessingIndicator(reportID); startPolling(); }, }); From 6a18430c8ed49991ff06cf3431aa1276316a8920 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sat, 28 Mar 2026 23:49:08 -0600 Subject: [PATCH 19/28] Delegate AgentZeroStatusContext to useAgentZeroStatusIndicator hook (Option C) Replace duplicated Pusher/reasoning/polling logic in the Context with a thin wrapper that delegates to the hook. Update tests to use ConciergeReasoningStore and subscribeToReportReasoningEvents instead of direct Pusher mocks. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 67 ++++--- src/pages/inbox/AgentZeroStatusContext.tsx | 195 ++------------------- tests/unit/AgentZeroStatusContextTest.ts | 63 ++----- 3 files changed, 61 insertions(+), 264 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index f5e4ef4fe4a64..fc685a475a147 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -3,6 +3,7 @@ import type {OnyxEntry} from 'react-native-onyx'; import {clearAgentZeroProcessingIndicator, getNewerActions, subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; import ConciergeReasoningStore from '@libs/ConciergeReasoningStore'; import type {ReasoningEntry} from '@libs/ConciergeReasoningStore'; +import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; import type {ReportActions} from '@src/types/onyx/ReportAction'; import useLocalize from './useLocalize'; @@ -16,6 +17,11 @@ type AgentZeroStatusState = { kickoffWaitingIndicator: () => void; }; +type NewestReportAction = { + reportActionID: string; + actorAccountID?: number; +}; + /** * Polling interval for fetching missed Concierge responses while the thinking indicator is visible. * @@ -40,8 +46,8 @@ const MAX_POLL_DURATION_MS = 120000; * @param reportID - The report ID to monitor * @param isAgentZeroChat - Whether the chat is an AgentZero-enabled chat (Concierge DM or #admins room) */ -/** Selector that extracts only the newest reportActionID from the report actions collection. */ -function selectNewestReportActionID(reportActions: OnyxEntry): string | undefined { +/** Selector that extracts the newest report action ID and actor from the report actions collection. */ +function selectNewestReportAction(reportActions: OnyxEntry): NewestReportAction | undefined { if (!reportActions) { return undefined; } @@ -49,20 +55,23 @@ function selectNewestReportActionID(reportActions: OnyxEntry): st if (actionIDs.length === 0) { return undefined; } - // reportActionIDs are numeric strings; the highest value is the newest - return actionIDs.reduce((a, b) => (Number(a) > Number(b) ? a : b)); + const newestReportActionID = actionIDs.reduce((a, b) => (Number(a) > Number(b) ? a : b)); + return { + reportActionID: newestReportActionID, + actorAccountID: reportActions[newestReportActionID]?.actorAccountID, + }; } function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean): AgentZeroStatusState { const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`); const serverLabel = reportNameValuePairs?.agentZeroProcessingRequestIndicator?.trim() ?? ''; - // Track the newest reportActionID so we can fetch missed actions when the safety timer fires - const [newestReportActionID] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {selector: selectNewestReportActionID}); - const newestReportActionIDRef = useRef(newestReportActionID); + // Track the newest report action so we can fetch missed actions and detect actual Concierge replies. + const [newestReportAction] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {selector: selectNewestReportAction}); + const newestReportActionRef = useRef(newestReportAction); useEffect(() => { - newestReportActionIDRef.current = newestReportActionID; - }, [newestReportActionID]); + newestReportActionRef.current = newestReportAction; + }, [newestReportAction]); const [optimisticStartTime, setOptimisticStartTime] = useState(null); const [displayedLabel, setDisplayedLabel] = useState(''); @@ -108,7 +117,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) setOptimisticStartTime(null); setDisplayedLabel(''); clearAgentZeroProcessingIndicator(reportID); - getNewerActions(reportID, newestReportActionIDRef.current); + getNewerActions(reportID, newestReportActionRef.current?.reportActionID); }, [reportID, clearPolling]); /** @@ -128,19 +137,21 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // so we can detect if new actions arrived (meaning Concierge responded). // If new actions arrive but the NVP CLEAR was missed via Pusher, we clear // the indicator client-side. - const prePollingActionID = newestReportActionIDRef.current; + const prePollingActionID = newestReportActionRef.current?.reportActionID; pollIntervalRef.current = setInterval(() => { if (isOfflineRef.current) { return; } - // If the newest action ID changed since polling started, a new action arrived - // (likely the Concierge response). Clear the stuck indicator. - if (newestReportActionIDRef.current !== prePollingActionID) { + const currentNewestReportAction = newestReportActionRef.current; + const didConciergeReplyAfterPollingStarted = + currentNewestReportAction?.actorAccountID === CONST.ACCOUNT_ID.CONCIERGE && currentNewestReportAction.reportActionID !== prePollingActionID; + + if (didConciergeReplyAfterPollingStarted) { clearAgentZeroProcessingIndicator(reportID); clearPolling(); return; } - getNewerActions(reportID, newestReportActionIDRef.current); + getNewerActions(reportID, currentNewestReportAction?.reportActionID); }, POLL_INTERVAL_MS); // Safety net: hard-clear after MAX_POLL_DURATION_MS @@ -149,20 +160,15 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) }, MAX_POLL_DURATION_MS); }, [clearPolling, hardClearIndicator, reportID]); - // Clear indicator on network reconnect. When Pusher reconnects, stale NVP state - // may be re-delivered. Like typing indicators (Report/index.ts:486), we reset - // the indicator and let fresh data arrive via GetMissingOnyxMessages. - // We also call getNewerActions to pull any responses missed during the outage. + // On reconnect, fetch missed actions if the indicator is still active. + // Do not clear locally just because the socket recovered, and do not restart polling here: + // the existing poll cycle keeps the original action baseline needed to detect a missed Concierge reply. const {isOffline} = useNetwork({ onReconnect: () => { - // On reconnect, fetch any missed actions and clear the indicator. - // The getNewerActions call fetches the Concierge response that was - // sent while the WebSocket was down. We also clear the indicator NVP - // directly because the NVP CLEAR event was likely lost with the - // WebSocket drop — getNewerActions only fetches report actions, not NVPs. - getNewerActions(reportID, newestReportActionIDRef.current); - clearAgentZeroProcessingIndicator(reportID); - startPolling(); + if (!serverLabel && !optimisticStartTime) { + return; + } + getNewerActions(reportID, newestReportActionRef.current?.reportActionID); }, }); @@ -272,13 +278,6 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) isOfflineRef.current = isOffline; }, [isOffline]); - useEffect(() => { - if (isOffline) { - return; - } - setOptimisticStartTime(null); - }, [isOffline]); - // Clean up polling on unmount useEffect( () => () => { diff --git a/src/pages/inbox/AgentZeroStatusContext.tsx b/src/pages/inbox/AgentZeroStatusContext.tsx index 2fe98914b3ea4..91aace7627c91 100644 --- a/src/pages/inbox/AgentZeroStatusContext.tsx +++ b/src/pages/inbox/AgentZeroStatusContext.tsx @@ -1,34 +1,24 @@ -import {agentZeroProcessingIndicatorSelector} from '@selectors/ReportNameValuePairs'; -import React, {createContext, useContext, useEffect, useRef, useState} from 'react'; +import React, {createContext, useContext} from 'react'; import type {ValueOf} from 'type-fest'; -import useLocalize from '@hooks/useLocalize'; -import useNetwork from '@hooks/useNetwork'; +import useAgentZeroStatusIndicator from '@hooks/useAgentZeroStatusIndicator'; import useOnyx from '@hooks/useOnyx'; -import {getReportChannelName} from '@libs/actions/Report'; -import Log from '@libs/Log'; -import Pusher from '@libs/Pusher'; +import type {ReasoningEntry} from '@libs/ConciergeReasoningStore'; import CONST from '@src/CONST'; import ONYXKEYS from '@src/ONYXKEYS'; -type ReasoningEntry = { - reasoning: string; - loopCount: number; - timestamp: number; -}; - type AgentZeroStatusState = { - /** Whether AgentZero is actively working — true when the server sent a processing label or we're optimistically waiting */ + /** Whether AgentZero is actively working */ isProcessing: boolean; - /** Chronological list of reasoning steps streamed via Pusher during the current processing request */ + /** Chronological list of reasoning steps for the current processing request */ reasoningHistory: ReasoningEntry[]; - /** Debounced label shown in the thinking bubble (e.g. "Looking up categories...") */ + /** Debounced label shown in the thinking bubble */ statusLabel: string; }; type AgentZeroStatusActions = { - /** Sets optimistic "thinking" state immediately after the user sends a message, before the server responds */ + /** Sets optimistic "thinking" state immediately after the user sends a message */ kickoffWaitingIndicator: () => void; }; @@ -47,8 +37,7 @@ const AgentZeroStatusActionsContext = createContext(defa /** * Cheap outer guard — only subscribes to the scalar CONCIERGE_REPORT_ID. - * For non-AgentZero reports (the common case), returns children directly - * without mounting any Pusher subscriptions or heavy state logic. + * For non-AgentZero reports (the common case), returns children directly. * * AgentZero chats include Concierge DMs and policy #admins rooms. */ @@ -72,175 +61,11 @@ function AgentZeroStatusProvider({reportID, chatType, children}: React.PropsWith ); } -// Minimum time to display a label before allowing change (prevents rapid flicker) -const MIN_DISPLAY_TIME = 300; // ms -// Debounce delay for server label updates -const DEBOUNCE_DELAY = 150; // ms - -/** - * Inner gate — all Pusher, reasoning, label, and processing state. - * Only mounted when reportID matches the Concierge report. - * Remounted via key prop when reportID changes, so all state resets automatically. - */ function AgentZeroStatusGate({reportID, children}: React.PropsWithChildren<{reportID: string}>) { - // Server-driven processing label from report name-value pairs (e.g. "Looking up categories...") - const [serverLabel] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, {selector: agentZeroProcessingIndicatorSelector}); - - // Timestamp set when the user sends a message, before the server label arrives — shows "Concierge is thinking..." - const [optimisticStartTime, setOptimisticStartTime] = useState(null); - // Debounced label shown to the user — smooths rapid server label changes - const displayedLabelRef = useRef(''); - const [displayedLabel, setDisplayedLabel] = useState(''); - // Chronological list of reasoning steps streamed via Pusher during a single processing request - const [reasoningHistory, setReasoningHistory] = useState([]); - const {translate} = useLocalize(); - // Timer for debounced label updates — ensures a minimum display time before switching - const updateTimerRef = useRef(null); - // Timestamp of the last label update — used to enforce MIN_DISPLAY_TIME - const lastUpdateTimeRef = useRef(0); - const {isOffline} = useNetwork(); - - // Tracks the current agentZeroRequestID so the Pusher callback can detect new requests - const agentZeroRequestIDRef = useRef(''); - - // Clear optimistic state once server label arrives — the server has taken over - if (serverLabel && optimisticStartTime) { - setOptimisticStartTime(null); - } - - // Clear optimistic state when coming back online — stale optimism from offline - const [prevIsOffline, setPrevIsOffline] = useState(isOffline); - if (prevIsOffline !== isOffline) { - setPrevIsOffline(isOffline); - if (!isOffline && optimisticStartTime) { - setOptimisticStartTime(null); - } - } - - // Clear reasoning when processing ends (server label transitions from truthy → falsy) - const [prevServerLabel, setPrevServerLabel] = useState(serverLabel); - if (prevServerLabel !== serverLabel) { - setPrevServerLabel(serverLabel); - if (prevServerLabel && !serverLabel && reasoningHistory.length > 0) { - setReasoningHistory([]); - } - } - - /** Appends a reasoning entry from Pusher. Resets history when a new request ID is detected; skips duplicates. */ - const addReasoning = (data: {reasoning: string; agentZeroRequestID: string; loopCount: number}) => { - if (!data.reasoning.trim()) { - return; - } - - const isNewRequest = agentZeroRequestIDRef.current !== data.agentZeroRequestID; - if (isNewRequest) { - agentZeroRequestIDRef.current = data.agentZeroRequestID; - } - - const entry: ReasoningEntry = { - reasoning: data.reasoning, - loopCount: data.loopCount, - timestamp: Date.now(), - }; - - if (isNewRequest) { - setReasoningHistory([entry]); - return; - } - - setReasoningHistory((prev) => { - const isDuplicate = prev.some((e) => e.loopCount === data.loopCount && e.reasoning === data.reasoning); - if (isDuplicate) { - return prev; - } - return [...prev, entry]; - }); - }; - - // Subscribe to Pusher reasoning events for this report's channel - useEffect(() => { - const channelName = getReportChannelName(reportID); - - const listener = Pusher.subscribe(channelName, Pusher.TYPE.CONCIERGE_REASONING, (data: Record) => { - const eventData = data as {reasoning: string; agentZeroRequestID: string; loopCount: number}; - addReasoning(eventData); - }); - listener.catch((error: unknown) => { - Log.hmmm('[AgentZeroStatusGate] Failed to subscribe to Pusher concierge reasoning events', {reportID, error}); - }); - - return () => { - listener.unsubscribe(); - }; - }, [reportID, addReasoning]); - - // Synchronize the displayed label with debounce and minimum display time. - // displayedLabelRef mirrors state so the effect can check the current value without depending on displayedLabel. - useEffect(() => { - let targetLabel = ''; - if (serverLabel) { - targetLabel = serverLabel; - } else if (optimisticStartTime) { - targetLabel = translate('common.thinking'); - } - - if (displayedLabelRef.current === targetLabel) { - return; - } - - const now = Date.now(); - const timeSinceLastUpdate = now - lastUpdateTimeRef.current; - const remainingMinTime = Math.max(0, MIN_DISPLAY_TIME - timeSinceLastUpdate); - - if (updateTimerRef.current) { - clearTimeout(updateTimerRef.current); - updateTimerRef.current = null; - } - - // Immediate update when enough time has passed or when clearing the label - if (remainingMinTime === 0 || targetLabel === '') { - displayedLabelRef.current = targetLabel; - // eslint-disable-next-line react-hooks/set-state-in-effect -- guarded by displayedLabelRef check above; fires once per serverLabel/optimistic transition - setDisplayedLabel(targetLabel); - lastUpdateTimeRef.current = now; - } else { - // Schedule update after debounce + remaining min display time - const delay = DEBOUNCE_DELAY + remainingMinTime; - updateTimerRef.current = setTimeout(() => { - displayedLabelRef.current = targetLabel; - setDisplayedLabel(targetLabel); - lastUpdateTimeRef.current = Date.now(); - updateTimerRef.current = null; - }, delay); - } - - return () => { - if (!updateTimerRef.current) { - return; - } - clearTimeout(updateTimerRef.current); - }; - }, [serverLabel, optimisticStartTime, translate]); - - const kickoffWaitingIndicator = () => { - setOptimisticStartTime(Date.now()); - }; - - // True when AgentZero is actively working — either the server sent a label or we're optimistically waiting - const isProcessing = !isOffline && (!!serverLabel || !!optimisticStartTime); - - const stateValue: AgentZeroStatusState = { - isProcessing, - reasoningHistory, - statusLabel: displayedLabel, - }; - - const actionsValue: AgentZeroStatusActions = { - kickoffWaitingIndicator, - }; + const {kickoffWaitingIndicator, ...stateValue} = useAgentZeroStatusIndicator(reportID, true); return ( - + {children} ); diff --git a/tests/unit/AgentZeroStatusContextTest.ts b/tests/unit/AgentZeroStatusContextTest.ts index 6ab620fde97f2..2a18bd64e28e6 100644 --- a/tests/unit/AgentZeroStatusContextTest.ts +++ b/tests/unit/AgentZeroStatusContextTest.ts @@ -4,8 +4,8 @@ import path from 'path'; import React from 'react'; import Onyx from 'react-native-onyx'; import useAgentZeroStatusIndicator from '@hooks/useAgentZeroStatusIndicator'; -import {clearAgentZeroProcessingIndicator} from '@libs/actions/Report'; -import Pusher from '@libs/Pusher'; +import {clearAgentZeroProcessingIndicator, subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; +import ConciergeReasoningStore from '@libs/ConciergeReasoningStore'; import {AgentZeroStatusProvider, useAgentZeroStatus, useAgentZeroStatusActions} from '@pages/inbox/AgentZeroStatusContext'; import ONYXKEYS from '@src/ONYXKEYS'; import waitForBatchedUpdates from '../utils/waitForBatchedUpdates'; @@ -25,7 +25,6 @@ jest.mock('@hooks/useLocalize', () => ({ }), })); -jest.mock('@libs/Pusher'); jest.mock('@libs/actions/Report', () => { // eslint-disable-next-line @typescript-eslint/consistent-type-imports const actual = jest.requireActual('@libs/actions/Report'); @@ -37,41 +36,18 @@ jest.mock('@libs/actions/Report', () => { unsubscribeFromReportReasoningChannel: jest.fn(), }; }); -jest.mock('@libs/ConciergeReasoningStore', () => ({ - // eslint-disable-next-line @typescript-eslint/naming-convention - __esModule: true, - default: { - subscribe: jest.fn().mockReturnValue(() => {}), - getReasoningHistory: jest.fn().mockReturnValue([]), - addReasoning: jest.fn(), - clearReasoning: jest.fn(), - }, -})); const mockClearAgentZeroProcessingIndicator = clearAgentZeroProcessingIndicator as jest.MockedFunction; +const mockSubscribeToReportReasoningEvents = subscribeToReportReasoningEvents as jest.MockedFunction; +const mockUnsubscribeFromReportReasoningChannel = unsubscribeFromReportReasoningChannel as jest.MockedFunction; -const mockPusher = Pusher as jest.Mocked; - -type PusherCallback = (data: Record) => void; - -/** Captures the reasoning callback passed to Pusher.subscribe for CONCIERGE_REASONING */ -function capturePusherCallback(): PusherCallback { - const call = mockPusher.subscribe.mock.calls.find((c) => c.at(1) === Pusher.TYPE.CONCIERGE_REASONING); - const callback = call?.at(2) as PusherCallback | undefined; - if (!callback) { - throw new Error('Pusher.subscribe was not called for CONCIERGE_REASONING'); - } - return callback; -} +const reportID = '123'; -/** Simulates a Pusher reasoning event */ +/** Simulates a reasoning event via ConciergeReasoningStore (the real store, since it's not mocked) */ function simulateReasoning(data: {reasoning: string; agentZeroRequestID: string; loopCount: number}) { - const callback = capturePusherCallback(); - callback(data as unknown as Record); + ConciergeReasoningStore.addReasoning(reportID, data); } -const reportID = '123'; - function wrapper({children}: {children: React.ReactNode}) { return React.createElement(AgentZeroStatusProvider, {reportID, chatType: undefined}, children); } @@ -83,13 +59,14 @@ describe('AgentZeroStatusContext', () => { jest.clearAllMocks(); await Onyx.clear(); - mockPusher.subscribe = jest.fn().mockImplementation(() => Object.assign(Promise.resolve(), {unsubscribe: jest.fn()})); - mockPusher.unsubscribe = jest.fn(); + // Clear ConciergeReasoningStore between tests + ConciergeReasoningStore.clearReasoning(reportID); // Make clearAgentZeroProcessingIndicator actually clear the Onyx NVP // so safety timeout and reconnect tests can verify the full clearing flow mockClearAgentZeroProcessingIndicator.mockImplementation((rID: string) => { Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${rID}`, {agentZeroProcessingRequestIndicator: null}); + ConciergeReasoningStore.clearReasoning(rID); }); // Mark this report as Concierge by default @@ -115,8 +92,8 @@ describe('AgentZeroStatusContext', () => { expect(result.current.reasoningHistory).toEqual([]); expect(result.current.kickoffWaitingIndicator).toBeInstanceOf(Function); - // And no Pusher subscription should have been created - expect(mockPusher.subscribe).not.toHaveBeenCalled(); + // And no reasoning subscription should have been created + expect(mockSubscribeToReportReasoningEvents).not.toHaveBeenCalled(); }); it('should return processing state when server label is present in Concierge chat', async () => { @@ -321,34 +298,30 @@ describe('AgentZeroStatusContext', () => { }); describe('Pusher lifecycle', () => { - it('should subscribe to Pusher for Concierge chat on mount', async () => { + it('should subscribe to reasoning events for Concierge chat on mount', async () => { renderHook(() => useAgentZeroStatus(), {wrapper}); await waitForBatchedUpdates(); - expect(mockPusher.subscribe).toHaveBeenCalledWith(expect.stringContaining(reportID), Pusher.TYPE.CONCIERGE_REASONING, expect.any(Function)); + expect(mockSubscribeToReportReasoningEvents).toHaveBeenCalledWith(reportID); }); - it('should not subscribe to Pusher for non-Concierge chat', async () => { + it('should not subscribe to reasoning events for non-Concierge chat', async () => { await Onyx.merge(ONYXKEYS.CONCIERGE_REPORT_ID, '999'); renderHook(() => useAgentZeroStatus(), {wrapper}); await waitForBatchedUpdates(); - expect(mockPusher.subscribe).not.toHaveBeenCalledWith(expect.anything(), Pusher.TYPE.CONCIERGE_REASONING, expect.anything()); + expect(mockSubscribeToReportReasoningEvents).not.toHaveBeenCalled(); }); - it('should unsubscribe from Pusher on unmount', async () => { - // Track the per-callback unsubscribe handle - const handleUnsubscribe = jest.fn(); - mockPusher.subscribe = jest.fn().mockImplementation(() => Object.assign(Promise.resolve(), {unsubscribe: handleUnsubscribe})); - + it('should unsubscribe from reasoning events on unmount', async () => { const {unmount} = renderHook(() => useAgentZeroStatus(), {wrapper}); await waitForBatchedUpdates(); unmount(); await waitForBatchedUpdates(); - expect(handleUnsubscribe).toHaveBeenCalled(); + expect(mockUnsubscribeFromReportReasoningChannel).toHaveBeenCalledWith(reportID); }); }); From 5bd1c44bd5199280732d95d3256fea890c9069b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sun, 29 Mar 2026 22:14:49 -0600 Subject: [PATCH 20/28] Clear indicator immediately when Concierge response detected via Onyx Instead of waiting for the next 30s poll cycle to detect the Concierge actorAccountID, watch the newestReportAction Onyx subscription directly. When getNewerActions fetches the response and Onyx merges it, the useEffect fires immediately and clears the indicator. Reduces clear delay from ~55s to <5s after reconnect. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index fc685a475a147..6d5cf9bea2097 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -71,7 +71,15 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const newestReportActionRef = useRef(newestReportAction); useEffect(() => { newestReportActionRef.current = newestReportAction; - }, [newestReportAction]); + + // Immediately clear the indicator when a Concierge response arrives while processing. + // This eliminates the 30s delay waiting for the next poll cycle to detect it. + // The Onyx subscription fires as soon as getNewerActions merges new actions. + if (newestReportAction?.actorAccountID === CONST.ACCOUNT_ID.CONCIERGE && (serverLabel || optimisticStartTime)) { + clearAgentZeroProcessingIndicator(reportID); + clearPolling(); + } + }, [newestReportAction, serverLabel, optimisticStartTime, reportID, clearPolling]); const [optimisticStartTime, setOptimisticStartTime] = useState(null); const [displayedLabel, setDisplayedLabel] = useState(''); @@ -168,7 +176,11 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) if (!serverLabel && !optimisticStartTime) { return; } + // Fetch missed actions AND start polling to detect when the Concierge response arrives. + // getNewerActions is a one-shot fetch; polling ensures we keep checking until + // the response is detected (via actorAccountID === CONCIERGE check in the poll). getNewerActions(reportID, newestReportActionRef.current?.reportActionID); + startPolling(); }, }); From 74ed559e8ce671a40fd23342ead4c482e5f44a88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sun, 29 Mar 2026 22:28:31 -0600 Subject: [PATCH 21/28] Fix: move Concierge detection useEffect after dependency declarations The useEffect referencing optimisticStartTime and clearPolling was placed before they were declared, causing a ReferenceError (temporal dead zone). Moved to after all dependencies are defined. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 6d5cf9bea2097..70e3c84d886aa 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -71,15 +71,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const newestReportActionRef = useRef(newestReportAction); useEffect(() => { newestReportActionRef.current = newestReportAction; - - // Immediately clear the indicator when a Concierge response arrives while processing. - // This eliminates the 30s delay waiting for the next poll cycle to detect it. - // The Onyx subscription fires as soon as getNewerActions merges new actions. - if (newestReportAction?.actorAccountID === CONST.ACCOUNT_ID.CONCIERGE && (serverLabel || optimisticStartTime)) { - clearAgentZeroProcessingIndicator(reportID); - clearPolling(); - } - }, [newestReportAction, serverLabel, optimisticStartTime, reportID, clearPolling]); + }, [newestReportAction]); const [optimisticStartTime, setOptimisticStartTime] = useState(null); const [displayedLabel, setDisplayedLabel] = useState(''); @@ -306,6 +298,16 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) startPolling(); }, [isAgentZeroChat, startPolling]); + // Immediately clear the indicator when a Concierge response arrives while processing. + // This eliminates the 30s delay waiting for the next poll cycle to detect it. + // Placed after all dependencies (clearPolling, optimisticStartTime) are defined. + useEffect(() => { + if (newestReportAction?.actorAccountID === CONST.ACCOUNT_ID.CONCIERGE && (serverLabel || optimisticStartTime)) { + clearAgentZeroProcessingIndicator(reportID); + clearPolling(); + } + }, [newestReportAction, serverLabel, optimisticStartTime, reportID, clearPolling]); + const isProcessing = isAgentZeroChat && !isOffline && (!!serverLabel || !!optimisticStartTime); return useMemo( From 0790eec47c1a42b229f9613f143d4936860b3356 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Sun, 29 Mar 2026 22:51:03 -0600 Subject: [PATCH 22/28] Fix ESLint: prefer-early-return + narrow hook dependencies Use early returns instead of nested if, and newestActorAccountID instead of the full newestReportAction object in the dependency array. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 70e3c84d886aa..467b2672e4723 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -300,13 +300,17 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // Immediately clear the indicator when a Concierge response arrives while processing. // This eliminates the 30s delay waiting for the next poll cycle to detect it. - // Placed after all dependencies (clearPolling, optimisticStartTime) are defined. + const newestActorAccountID = newestReportAction?.actorAccountID; useEffect(() => { - if (newestReportAction?.actorAccountID === CONST.ACCOUNT_ID.CONCIERGE && (serverLabel || optimisticStartTime)) { - clearAgentZeroProcessingIndicator(reportID); - clearPolling(); + if (newestActorAccountID !== CONST.ACCOUNT_ID.CONCIERGE) { + return; + } + if (!serverLabel && !optimisticStartTime) { + return; } - }, [newestReportAction, serverLabel, optimisticStartTime, reportID, clearPolling]); + clearAgentZeroProcessingIndicator(reportID); + clearPolling(); + }, [newestActorAccountID, serverLabel, optimisticStartTime, reportID, clearPolling]); const isProcessing = isAgentZeroChat && !isOffline && (!!serverLabel || !!optimisticStartTime); From ac8ba8822cc39347b035a434127b51d6aec81f2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Mon, 30 Mar 2026 16:50:33 -0600 Subject: [PATCH 23/28] Address all bot review comments on PR #85620 - P1: Reset optimistic state (pendingOptimisticRequests) when clearing on Concierge reply - P2: Scope counter to specific requests (increment/reset pattern) - P2: Preserve Pusher subscription handle for proper per-callback cleanup - CLEAN-REACT-PATTERNS-0: Remove manual useCallback/useMemo (React Compiler handles it) - PERF-14: Replace dual useEffect with useSyncExternalStore for ConciergeReasoningStore Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 86 ++++++++++++------------ src/libs/actions/Report/index.ts | 25 ++++--- 2 files changed, 59 insertions(+), 52 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 467b2672e4723..4c198ebea62ff 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -1,4 +1,4 @@ -import {useCallback, useEffect, useMemo, useRef, useState} from 'react'; +import {useEffect, useRef, useState, useSyncExternalStore} from 'react'; import type {OnyxEntry} from 'react-native-onyx'; import {clearAgentZeroProcessingIndicator, getNewerActions, subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; import ConciergeReasoningStore from '@libs/ConciergeReasoningStore'; @@ -73,9 +73,11 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) newestReportActionRef.current = newestReportAction; }, [newestReportAction]); - const [optimisticStartTime, setOptimisticStartTime] = useState(null); + // Track pending optimistic requests with a counter instead of a single timestamp. + // Each kickoffWaitingIndicator() call increments the counter; each Concierge reply + // decrements it. The indicator stays active until all pending requests are resolved. + const [pendingOptimisticRequests, setPendingOptimisticRequests] = useState(0); const [displayedLabel, setDisplayedLabel] = useState(''); - const [reasoningHistory, setReasoningHistory] = useState([]); const {translate} = useLocalize(); const prevServerLabelRef = useRef(serverLabel); const updateTimerRef = useRef(null); @@ -93,7 +95,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) * Clear the polling interval and safety timer. Called when the indicator clears normally, * when a new processing cycle starts, or when the component unmounts. */ - const clearPolling = useCallback(() => { + const clearPolling = () => { if (pollIntervalRef.current) { clearInterval(pollIntervalRef.current); pollIntervalRef.current = null; @@ -102,23 +104,23 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) clearTimeout(pollSafetyTimerRef.current); pollSafetyTimerRef.current = null; } - }, []); + }; /** * Hard-clear the indicator by resetting local state and clearing the Onyx NVP. * Called as a safety net after MAX_POLL_DURATION_MS if no response has arrived. */ - const hardClearIndicator = useCallback(() => { + const hardClearIndicator = () => { // If offline, don't clear — the response may arrive when reconnected if (isOfflineRef.current) { return; } clearPolling(); - setOptimisticStartTime(null); + setPendingOptimisticRequests(0); setDisplayedLabel(''); clearAgentZeroProcessingIndicator(reportID); getNewerActions(reportID, newestReportActionRef.current?.reportActionID); - }, [reportID, clearPolling]); + }; /** * Start polling for missed actions every POLL_INTERVAL_MS. Every time processing @@ -130,7 +132,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) * * Polling stops when: indicator clears, component unmounts, or user goes offline. */ - const startPolling = useCallback(() => { + const startPolling = () => { clearPolling(); // Poll every 30s for missed actions. Track the newest action ID before polling @@ -149,6 +151,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) if (didConciergeReplyAfterPollingStarted) { clearAgentZeroProcessingIndicator(reportID); clearPolling(); + setPendingOptimisticRequests(0); return; } getNewerActions(reportID, currentNewestReportAction?.reportActionID); @@ -158,14 +161,14 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) pollSafetyTimerRef.current = setTimeout(() => { hardClearIndicator(); }, MAX_POLL_DURATION_MS); - }, [clearPolling, hardClearIndicator, reportID]); + }; // On reconnect, fetch missed actions if the indicator is still active. // Do not clear locally just because the socket recovered, and do not restart polling here: // the existing poll cycle keeps the original action baseline needed to detect a missed Concierge reply. const {isOffline} = useNetwork({ onReconnect: () => { - if (!serverLabel && !optimisticStartTime) { + if (!serverLabel && pendingOptimisticRequests === 0) { return; } // Fetch missed actions AND start polling to detect when the Concierge response arrives. @@ -176,20 +179,19 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) }, }); - useEffect(() => { - setReasoningHistory(ConciergeReasoningStore.getReasoningHistory(reportID)); - }, [reportID]); - - useEffect(() => { - const unsubscribe = ConciergeReasoningStore.subscribe((updatedReportID, entries) => { + // Subscribe to ConciergeReasoningStore using useSyncExternalStore for + // correct synchronization with React's render cycle. + const subscribeToReasoningStore = (onStoreChange: () => void) => { + const unsubscribe = ConciergeReasoningStore.subscribe((updatedReportID) => { if (updatedReportID !== reportID) { return; } - setReasoningHistory(entries); + onStoreChange(); }); - return unsubscribe; - }, [reportID]); + }; + const getReasoningSnapshot = () => ConciergeReasoningStore.getReasoningHistory(reportID); + const reasoningHistory = useSyncExternalStore(subscribeToReasoningStore, getReasoningSnapshot, getReasoningSnapshot); useEffect(() => { if (!isAgentZeroChat) { @@ -245,12 +247,12 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) if (hasServerLabel) { updateLabel(serverLabel); startPolling(); - if (optimisticStartTime) { - setOptimisticStartTime(null); + if (pendingOptimisticRequests > 0) { + setPendingOptimisticRequests(0); } } // When optimistic state is active but no server label, show "Concierge is thinking..." - else if (optimisticStartTime) { + else if (pendingOptimisticRequests > 0) { const thinkingLabel = translate('common.thinking'); updateLabel(thinkingLabel); // Polling was already started in kickoffWaitingIndicator @@ -276,7 +278,8 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) } clearTimeout(updateTimerRef.current); }; - }, [serverLabel, reasoningHistory.length, reportID, optimisticStartTime, translate, displayedLabel, startPolling, clearPolling]); + // eslint-disable-next-line react-hooks/exhaustive-deps -- startPolling/clearPolling are plain functions stable via React Compiler + }, [serverLabel, reasoningHistory.length, reportID, pendingOptimisticRequests, translate, displayedLabel]); useEffect(() => { isOfflineRef.current = isOffline; @@ -287,16 +290,17 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) () => () => { clearPolling(); }, - [clearPolling], + // eslint-disable-next-line react-hooks/exhaustive-deps + [], ); - const kickoffWaitingIndicator = useCallback(() => { + const kickoffWaitingIndicator = () => { if (!isAgentZeroChat) { return; } - setOptimisticStartTime(Date.now()); + setPendingOptimisticRequests((prev) => prev + 1); startPolling(); - }, [isAgentZeroChat, startPolling]); + }; // Immediately clear the indicator when a Concierge response arrives while processing. // This eliminates the 30s delay waiting for the next poll cycle to detect it. @@ -305,24 +309,22 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) if (newestActorAccountID !== CONST.ACCOUNT_ID.CONCIERGE) { return; } - if (!serverLabel && !optimisticStartTime) { + if (!serverLabel && pendingOptimisticRequests === 0) { return; } clearAgentZeroProcessingIndicator(reportID); clearPolling(); - }, [newestActorAccountID, serverLabel, optimisticStartTime, reportID, clearPolling]); - - const isProcessing = isAgentZeroChat && !isOffline && (!!serverLabel || !!optimisticStartTime); - - return useMemo( - () => ({ - isProcessing, - reasoningHistory, - statusLabel: displayedLabel, - kickoffWaitingIndicator, - }), - [isProcessing, reasoningHistory, displayedLabel, kickoffWaitingIndicator], - ); + setPendingOptimisticRequests(0); + }, [newestActorAccountID, serverLabel, pendingOptimisticRequests, reportID]); + + const isProcessing = isAgentZeroChat && !isOffline && (!!serverLabel || pendingOptimisticRequests > 0); + + return { + isProcessing, + reasoningHistory, + statusLabel: displayedLabel, + kickoffWaitingIndicator, + }; } export default useAgentZeroStatusIndicator; diff --git a/src/libs/actions/Report/index.ts b/src/libs/actions/Report/index.ts index bab75386071d5..d3d4b4b59a921 100644 --- a/src/libs/actions/Report/index.ts +++ b/src/libs/actions/Report/index.ts @@ -403,8 +403,9 @@ Onyx.connect({ const typingWatchTimers: Record = {}; -// Track subscriptions to conciergeReasoning Pusher events to avoid duplicates -const reasoningSubscriptions = new Set(); +// Track subscriptions to conciergeReasoning Pusher events to avoid duplicates. +// Maps reportID to the PusherSubscription handle for proper per-callback cleanup. +const reasoningSubscriptions = new Map>(); let reportIDDeeplinkedFromOldDot: string | undefined; Linking.getInitialURL().then((url) => { @@ -594,10 +595,7 @@ function subscribeToReportReasoningEvents(reportID: string) { const pusherChannelName = getReportChannelName(reportID); - // Add to subscriptions immediately to prevent duplicate subscriptions - reasoningSubscriptions.add(reportID); - - Pusher.subscribe(pusherChannelName, Pusher.TYPE.CONCIERGE_REASONING, (data: Record) => { + const handle = Pusher.subscribe(pusherChannelName, Pusher.TYPE.CONCIERGE_REASONING, (data: Record) => { const eventData = data as {reasoning: string; agentZeroRequestID: string; loopCount: number}; ConciergeReasoningStore.addReasoning(reportID, { @@ -605,7 +603,12 @@ function subscribeToReportReasoningEvents(reportID: string) { agentZeroRequestID: eventData.agentZeroRequestID, loopCount: eventData.loopCount, }); - }).catch((error: ReportError) => { + }); + + // Store the handle immediately to prevent duplicate subscriptions + reasoningSubscriptions.set(reportID, handle); + + handle.catch((error: ReportError) => { Log.hmmm('[Report] Failed to subscribe to Pusher concierge reasoning events', {errorType: error.type, pusherChannelName, reportID}); // Remove from subscriptions if subscription failed reasoningSubscriptions.delete(reportID); @@ -617,12 +620,14 @@ function subscribeToReportReasoningEvents(reportID: string) { * Clears reasoning state and removes from subscription tracking. */ function unsubscribeFromReportReasoningChannel(reportID: string) { - if (!reportID || !reasoningSubscriptions.has(reportID)) { + const handle = reasoningSubscriptions.get(reportID); + if (!reportID || !handle) { return; } - const pusherChannelName = getReportChannelName(reportID); - Pusher.unsubscribe(pusherChannelName, Pusher.TYPE.CONCIERGE_REASONING); + // Use the per-callback handle for precise cleanup instead of the global + // Pusher.unsubscribe which removes ALL callbacks for the event on the channel. + handle.unsubscribe(); ConciergeReasoningStore.clearReasoning(reportID); reasoningSubscriptions.delete(reportID); } From cc74304e3a1b495cf37a3f72cb6e77df4b8f90bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Mon, 30 Mar 2026 17:01:23 -0600 Subject: [PATCH 24/28] Fix infinite re-render loop in useAgentZeroStatusIndicator tests The useSyncExternalStore hook requires stable references for its subscribe and getSnapshot callbacks. Without React Compiler active in the test environment, these functions were recreated every render, causing infinite re-subscribe loops. Three fixes: - Wrap subscribe/getSnapshot in useCallback for useSyncExternalStore - Return stable EMPTY_ENTRIES reference from ConciergeReasoningStore instead of creating a new [] on each getSnapshot call - Memoize kickoffWaitingIndicator, return value, and context values Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 47 +++++++++++++--------- src/libs/ConciergeReasoningStore.ts | 7 +++- src/pages/inbox/AgentZeroStatusContext.tsx | 5 ++- 3 files changed, 37 insertions(+), 22 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 4c198ebea62ff..4b886121b1fba 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -1,4 +1,4 @@ -import {useEffect, useRef, useState, useSyncExternalStore} from 'react'; +import {useCallback, useEffect, useMemo, useRef, useState, useSyncExternalStore} from 'react'; import type {OnyxEntry} from 'react-native-onyx'; import {clearAgentZeroProcessingIndicator, getNewerActions, subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; import ConciergeReasoningStore from '@libs/ConciergeReasoningStore'; @@ -181,16 +181,21 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // Subscribe to ConciergeReasoningStore using useSyncExternalStore for // correct synchronization with React's render cycle. - const subscribeToReasoningStore = (onStoreChange: () => void) => { - const unsubscribe = ConciergeReasoningStore.subscribe((updatedReportID) => { - if (updatedReportID !== reportID) { - return; - } - onStoreChange(); - }); - return unsubscribe; - }; - const getReasoningSnapshot = () => ConciergeReasoningStore.getReasoningHistory(reportID); + // IMPORTANT: subscribe and getSnapshot must be stable references to prevent + // infinite re-subscribe loops. React Compiler may not be active in tests. + const subscribeToReasoningStore = useCallback( + (onStoreChange: () => void) => { + const unsubscribe = ConciergeReasoningStore.subscribe((updatedReportID) => { + if (updatedReportID !== reportID) { + return; + } + onStoreChange(); + }); + return unsubscribe; + }, + [reportID], + ); + const getReasoningSnapshot = useCallback(() => ConciergeReasoningStore.getReasoningHistory(reportID), [reportID]); const reasoningHistory = useSyncExternalStore(subscribeToReasoningStore, getReasoningSnapshot, getReasoningSnapshot); useEffect(() => { @@ -294,13 +299,14 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) [], ); - const kickoffWaitingIndicator = () => { + const kickoffWaitingIndicator = useCallback(() => { if (!isAgentZeroChat) { return; } setPendingOptimisticRequests((prev) => prev + 1); startPolling(); - }; + // eslint-disable-next-line react-hooks/exhaustive-deps -- startPolling is stable via React Compiler; isAgentZeroChat is a derived boolean + }, [isAgentZeroChat]); // Immediately clear the indicator when a Concierge response arrives while processing. // This eliminates the 30s delay waiting for the next poll cycle to detect it. @@ -319,12 +325,15 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const isProcessing = isAgentZeroChat && !isOffline && (!!serverLabel || pendingOptimisticRequests > 0); - return { - isProcessing, - reasoningHistory, - statusLabel: displayedLabel, - kickoffWaitingIndicator, - }; + return useMemo( + () => ({ + isProcessing, + reasoningHistory, + statusLabel: displayedLabel, + kickoffWaitingIndicator, + }), + [isProcessing, reasoningHistory, displayedLabel, kickoffWaitingIndicator], + ); } export default useAgentZeroStatusIndicator; diff --git a/src/libs/ConciergeReasoningStore.ts b/src/libs/ConciergeReasoningStore.ts index 1e945033dd022..0fe31039bbed5 100644 --- a/src/libs/ConciergeReasoningStore.ts +++ b/src/libs/ConciergeReasoningStore.ts @@ -26,6 +26,11 @@ type Listener = (reportID: string, state: ReasoningEntry[]) => void; const store = new Map(); const listeners = new Set(); +// Stable empty array reference for useSyncExternalStore compatibility. +// getSnapshot must return the same reference when data hasn't changed, +// otherwise React will re-render infinitely. +const EMPTY_ENTRIES: ReasoningEntry[] = []; + /** * Notify all subscribers of state changes */ @@ -97,7 +102,7 @@ function clearReasoning(reportID: string) { * Get the reasoning history for a report */ function getReasoningHistory(reportID: string): ReasoningEntry[] { - return store.get(reportID)?.entries ?? []; + return store.get(reportID)?.entries ?? EMPTY_ENTRIES; } /** diff --git a/src/pages/inbox/AgentZeroStatusContext.tsx b/src/pages/inbox/AgentZeroStatusContext.tsx index 91aace7627c91..32e9a5a9b86fc 100644 --- a/src/pages/inbox/AgentZeroStatusContext.tsx +++ b/src/pages/inbox/AgentZeroStatusContext.tsx @@ -1,4 +1,4 @@ -import React, {createContext, useContext} from 'react'; +import React, {createContext, useContext, useMemo} from 'react'; import type {ValueOf} from 'type-fest'; import useAgentZeroStatusIndicator from '@hooks/useAgentZeroStatusIndicator'; import useOnyx from '@hooks/useOnyx'; @@ -63,9 +63,10 @@ function AgentZeroStatusProvider({reportID, chatType, children}: React.PropsWith function AgentZeroStatusGate({reportID, children}: React.PropsWithChildren<{reportID: string}>) { const {kickoffWaitingIndicator, ...stateValue} = useAgentZeroStatusIndicator(reportID, true); + const actionsValue = useMemo(() => ({kickoffWaitingIndicator}), [kickoffWaitingIndicator]); return ( - + {children} ); From efa98d2f5ff523b6e109ef17e7532977fd74edbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Fri, 3 Apr 2026 14:38:39 -0600 Subject: [PATCH 25/28] Remove redundant useCallback/useMemo wrappers (React Compiler handles memoization) React Compiler is enabled and this file compiles successfully, making manual useCallback and useMemo wrappers redundant. The compiler automatically memoizes closures and derived values based on their captured variables. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 47 ++++++++++-------------- 1 file changed, 19 insertions(+), 28 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 4b886121b1fba..4c198ebea62ff 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -1,4 +1,4 @@ -import {useCallback, useEffect, useMemo, useRef, useState, useSyncExternalStore} from 'react'; +import {useEffect, useRef, useState, useSyncExternalStore} from 'react'; import type {OnyxEntry} from 'react-native-onyx'; import {clearAgentZeroProcessingIndicator, getNewerActions, subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; import ConciergeReasoningStore from '@libs/ConciergeReasoningStore'; @@ -181,21 +181,16 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // Subscribe to ConciergeReasoningStore using useSyncExternalStore for // correct synchronization with React's render cycle. - // IMPORTANT: subscribe and getSnapshot must be stable references to prevent - // infinite re-subscribe loops. React Compiler may not be active in tests. - const subscribeToReasoningStore = useCallback( - (onStoreChange: () => void) => { - const unsubscribe = ConciergeReasoningStore.subscribe((updatedReportID) => { - if (updatedReportID !== reportID) { - return; - } - onStoreChange(); - }); - return unsubscribe; - }, - [reportID], - ); - const getReasoningSnapshot = useCallback(() => ConciergeReasoningStore.getReasoningHistory(reportID), [reportID]); + const subscribeToReasoningStore = (onStoreChange: () => void) => { + const unsubscribe = ConciergeReasoningStore.subscribe((updatedReportID) => { + if (updatedReportID !== reportID) { + return; + } + onStoreChange(); + }); + return unsubscribe; + }; + const getReasoningSnapshot = () => ConciergeReasoningStore.getReasoningHistory(reportID); const reasoningHistory = useSyncExternalStore(subscribeToReasoningStore, getReasoningSnapshot, getReasoningSnapshot); useEffect(() => { @@ -299,14 +294,13 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) [], ); - const kickoffWaitingIndicator = useCallback(() => { + const kickoffWaitingIndicator = () => { if (!isAgentZeroChat) { return; } setPendingOptimisticRequests((prev) => prev + 1); startPolling(); - // eslint-disable-next-line react-hooks/exhaustive-deps -- startPolling is stable via React Compiler; isAgentZeroChat is a derived boolean - }, [isAgentZeroChat]); + }; // Immediately clear the indicator when a Concierge response arrives while processing. // This eliminates the 30s delay waiting for the next poll cycle to detect it. @@ -325,15 +319,12 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) const isProcessing = isAgentZeroChat && !isOffline && (!!serverLabel || pendingOptimisticRequests > 0); - return useMemo( - () => ({ - isProcessing, - reasoningHistory, - statusLabel: displayedLabel, - kickoffWaitingIndicator, - }), - [isProcessing, reasoningHistory, displayedLabel, kickoffWaitingIndicator], - ); + return { + isProcessing, + reasoningHistory, + statusLabel: displayedLabel, + kickoffWaitingIndicator, + }; } export default useAgentZeroStatusIndicator; From d7816309f9d971332c0a787e2a8f559b6f419ec3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 9 Apr 2026 12:04:18 -0600 Subject: [PATCH 26/28] =?UTF-8?q?Revert=20AddNewCardPage.tsx=20changes=20?= =?UTF-8?q?=E2=80=94=20already=20fixed=20in=20#87431?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.6 (1M context) --- .../Wallet/PersonalCards/AddNewCardPage.tsx | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/src/pages/settings/Wallet/PersonalCards/AddNewCardPage.tsx b/src/pages/settings/Wallet/PersonalCards/AddNewCardPage.tsx index 9e139591a2ab2..77ff0a98d0a46 100644 --- a/src/pages/settings/Wallet/PersonalCards/AddNewCardPage.tsx +++ b/src/pages/settings/Wallet/PersonalCards/AddNewCardPage.tsx @@ -1,4 +1,4 @@ -import React, {useEffect} from 'react'; +import React, {useEffect, useState} from 'react'; import {View} from 'react-native'; import FullScreenLoadingIndicator from '@components/FullscreenLoadingIndicator'; import {ModalActions} from '@components/Modal/Global/ModalContext'; @@ -26,6 +26,7 @@ function AddPersonalNewCardPage() { const [introSelected] = useOnyx(ONYXKEYS.NVP_INTRO_SELECTED); const [betas] = useOnyx(ONYXKEYS.BETAS); const {currentStep} = addNewPersonalCardFeed ?? {}; + const [isModalVisible, setIsModalVisible] = useState(false); const {showConfirmModal} = useConfirmModal(); const {translate} = useLocalize(); const {accountID: currentUserAccountID} = useCurrentUserPersonalDetails(); @@ -65,24 +66,29 @@ function AddPersonalNewCardPage() { CurrentStep = ( { + setIsModalVisible(true); showConfirmModal({ title: translate('workspace.companyCards.addNewCard.exitModal.title'), success: true, confirmText: translate('workspace.companyCards.addNewCard.exitModal.confirmText'), cancelText: translate('workspace.companyCards.addNewCard.exitModal.cancelText'), prompt: translate('workspace.companyCards.addNewCard.exitModal.prompt'), - }).then((result) => { - if (result.action !== ModalActions.CONFIRM) { - return; - } - navigateToConciergeChat(conciergeReportID, introSelected, currentUserAccountID, false, betas); - }); + }) + .then((result) => { + if (result.action !== ModalActions.CONFIRM) { + return; + } + navigateToConciergeChat(conciergeReportID, introSelected, currentUserAccountID, false, betas); + }) + .finally(() => { + setIsModalVisible(false); + }); }} /> ); break; default: - CurrentStep = ; + CurrentStep = ; break; } From beb21b7711e9ac506ae8e2e3cf4bff02f97fb778 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 9 Apr 2026 12:20:58 -0600 Subject: [PATCH 27/28] Fix stuck indicator on reconnect: clear stale optimistic state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When going offline→online, the optimistic pendingOptimisticRequests state was persisting from before the disconnect, causing the indicator to reappear even if Concierge had already responded. This matches the original behavior in AgentZeroStatusContext which cleared optimisticStartTime on reconnect. Also fixes misleading comment about counter "decrementing per reply" — the counter resets to 0 when any Concierge reply is detected. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/hooks/useAgentZeroStatusIndicator.ts | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index 4c198ebea62ff..a3417d86bf9d6 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -73,9 +73,9 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) newestReportActionRef.current = newestReportAction; }, [newestReportAction]); - // Track pending optimistic requests with a counter instead of a single timestamp. - // Each kickoffWaitingIndicator() call increments the counter; each Concierge reply - // decrements it. The indicator stays active until all pending requests are resolved. + // Track pending optimistic requests with a counter. + // Each kickoffWaitingIndicator() call increments the counter; when a Concierge reply + // is detected (via polling, Pusher, or reconnect), the counter resets to 0. const [pendingOptimisticRequests, setPendingOptimisticRequests] = useState(0); const [displayedLabel, setDisplayedLabel] = useState(''); const {translate} = useLocalize(); @@ -163,14 +163,22 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) }, MAX_POLL_DURATION_MS); }; - // On reconnect, fetch missed actions if the indicator is still active. - // Do not clear locally just because the socket recovered, and do not restart polling here: - // the existing poll cycle keeps the original action baseline needed to detect a missed Concierge reply. + // On reconnect, clear stale optimistic state and fetch missed actions. + // Optimistic state from before going offline is unreliable — the server state + // (serverLabel) is the source of truth after reconnection. const {isOffline} = useNetwork({ onReconnect: () => { - if (!serverLabel && pendingOptimisticRequests === 0) { + const wasOptimistic = pendingOptimisticRequests > 0; + + // Clear stale optimistic state — server state takes over as source of truth + if (wasOptimistic) { + setPendingOptimisticRequests(0); + } + + if (!serverLabel && !wasOptimistic) { return; } + // Fetch missed actions AND start polling to detect when the Concierge response arrives. // getNewerActions is a one-shot fetch; polling ensures we keep checking until // the response is detected (via actorAccountID === CONCIERGE check in the poll). From 5e94954fa75154a115743e6cac352cf41e19cd2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marco=20Ch=C3=A1vez?= Date: Thu, 9 Apr 2026 12:43:06 -0600 Subject: [PATCH 28/28] Align hook with main's proven patterns to fix mobile indicator Key changes to match the original AgentZeroStatusGate behavior: - Use agentZeroProcessingIndicatorSelector for NVP subscription (fewer re-renders) - Use displayedLabelRef to avoid displayedLabel in effect dependency array (fewer effect cycles) - Remove isAgentZeroChat guard from kickoffWaitingIndicator (provider already handles this) - Remove isAgentZeroChat from isProcessing (always true in context, simplifies logic) - Move MIN_DISPLAY_TIME/DEBOUNCE_DELAY to module scope (matching main's pattern - Update hardClearIndicator to also reset displayedLabelRef - Restructure label-sync effect to match main's early-return-via-ref pattern Co-Authored-By: Claude Opus 4.6 (1M context) EOF ) --- src/hooks/useAgentZeroStatusIndicator.ts | 135 ++++++++++++----------- 1 file changed, 68 insertions(+), 67 deletions(-) diff --git a/src/hooks/useAgentZeroStatusIndicator.ts b/src/hooks/useAgentZeroStatusIndicator.ts index a3417d86bf9d6..9049b9c9699f6 100644 --- a/src/hooks/useAgentZeroStatusIndicator.ts +++ b/src/hooks/useAgentZeroStatusIndicator.ts @@ -1,3 +1,4 @@ +import agentZeroProcessingIndicatorSelector from '@selectors/ReportNameValuePairs'; import {useEffect, useRef, useState, useSyncExternalStore} from 'react'; import type {OnyxEntry} from 'react-native-onyx'; import {clearAgentZeroProcessingIndicator, getNewerActions, subscribeToReportReasoningEvents, unsubscribeFromReportReasoningChannel} from '@libs/actions/Report'; @@ -40,12 +41,11 @@ const POLL_INTERVAL_MS = 30000; */ const MAX_POLL_DURATION_MS = 120000; -/** - * Hook to manage AgentZero status indicator for chats where AgentZero responds. - * This includes both Concierge DM chats and policy #admins rooms (where Concierge handles onboarding). - * @param reportID - The report ID to monitor - * @param isAgentZeroChat - Whether the chat is an AgentZero-enabled chat (Concierge DM or #admins room) - */ +// Minimum time to display a label before allowing change (prevents rapid flicker) +const MIN_DISPLAY_TIME = 300; // ms +// Debounce delay for server label updates +const DEBOUNCE_DELAY = 150; // ms + /** Selector that extracts the newest report action ID and actor from the report actions collection. */ function selectNewestReportAction(reportActions: OnyxEntry): NewestReportAction | undefined { if (!reportActions) { @@ -62,9 +62,16 @@ function selectNewestReportAction(reportActions: OnyxEntry): Newe }; } +/** + * Hook to manage AgentZero status indicator for chats where AgentZero responds. + * This includes both Concierge DM chats and policy #admins rooms (where Concierge handles onboarding). + * @param reportID - The report ID to monitor + * @param isAgentZeroChat - Whether the chat is an AgentZero-enabled chat (Concierge DM or #admins room) + */ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean): AgentZeroStatusState { - const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`); - const serverLabel = reportNameValuePairs?.agentZeroProcessingRequestIndicator?.trim() ?? ''; + // Server-driven processing label from report name-value pairs (e.g. "Looking up categories...") + // Uses selector to only re-render when the specific field changes, not on any NVP change. + const [serverLabel] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${reportID}`, {selector: agentZeroProcessingIndicatorSelector}); // Track the newest report action so we can fetch missed actions and detect actual Concierge replies. const [newestReportAction] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, {selector: selectNewestReportAction}); @@ -77,20 +84,19 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) // Each kickoffWaitingIndicator() call increments the counter; when a Concierge reply // is detected (via polling, Pusher, or reconnect), the counter resets to 0. const [pendingOptimisticRequests, setPendingOptimisticRequests] = useState(0); + // Debounced label shown to the user — smooths rapid server label changes. + // displayedLabelRef mirrors state so the label-sync effect can read the current value + // without including displayedLabel in its dependency array (avoids extra effect cycles). + const displayedLabelRef = useRef(''); const [displayedLabel, setDisplayedLabel] = useState(''); const {translate} = useLocalize(); - const prevServerLabelRef = useRef(serverLabel); + const prevServerLabelRef = useRef(serverLabel ?? ''); const updateTimerRef = useRef(null); const lastUpdateTimeRef = useRef(0); const pollIntervalRef = useRef(null); const pollSafetyTimerRef = useRef(null); const isOfflineRef = useRef(false); - // Minimum time to display a label before allowing change (prevents rapid flicker) - const MIN_DISPLAY_TIME = 300; // ms - // Debounce delay for server label updates - const DEBOUNCE_DELAY = 150; // ms - /** * Clear the polling interval and safety timer. Called when the indicator clears normally, * when a new processing cycle starts, or when the component unmounts. @@ -117,6 +123,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) } clearPolling(); setPendingOptimisticRequests(0); + displayedLabelRef.current = ''; setDisplayedLabel(''); clearAgentZeroProcessingIndicator(reportID); getNewerActions(reportID, newestReportActionRef.current?.reportActionID); @@ -215,79 +222,76 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) }; }, [isAgentZeroChat, reportID]); + // Synchronize the displayed label with debounce and minimum display time. + // displayedLabelRef mirrors state so the effect can check the current value without depending on displayedLabel. useEffect(() => { const hadServerLabel = !!prevServerLabelRef.current; const hasServerLabel = !!serverLabel; - // Helper function to update label with timing control - const updateLabel = (newLabel: string) => { - const now = Date.now(); - const timeSinceLastUpdate = now - lastUpdateTimeRef.current; - const remainingMinTime = Math.max(0, MIN_DISPLAY_TIME - timeSinceLastUpdate); - - // Clear any pending update - if (updateTimerRef.current) { - clearTimeout(updateTimerRef.current); - updateTimerRef.current = null; - } - - // If enough time has passed or it's a critical update (clearing), update immediately - if (remainingMinTime === 0 || newLabel === '') { - if (displayedLabel !== newLabel) { - setDisplayedLabel(newLabel); - lastUpdateTimeRef.current = now; - } - } else { - // Schedule update after debounce + remaining min display time - const delay = DEBOUNCE_DELAY + remainingMinTime; - updateTimerRef.current = setTimeout(() => { - if (displayedLabel !== newLabel) { - setDisplayedLabel(newLabel); - lastUpdateTimeRef.current = Date.now(); - } - updateTimerRef.current = null; - }, delay); - } - }; + let targetLabel = ''; + if (hasServerLabel) { + targetLabel = serverLabel ?? ''; + } else if (pendingOptimisticRequests > 0) { + targetLabel = translate('common.thinking'); + } - // When server label arrives, transition smoothly without flicker. - // Start/reset polling — the label acts as a lease renewal. + // Start/reset polling when server label arrives (acts as a lease renewal) if (hasServerLabel) { - updateLabel(serverLabel); startPolling(); if (pendingOptimisticRequests > 0) { setPendingOptimisticRequests(0); } } - // When optimistic state is active but no server label, show "Concierge is thinking..." - else if (pendingOptimisticRequests > 0) { - const thinkingLabel = translate('common.thinking'); - updateLabel(thinkingLabel); - // Polling was already started in kickoffWaitingIndicator - } - // Clear everything when processing ends — either via the normal transition - // (server label went from non-empty to empty), or when the indicator is idle. - else { + // Clear polling when processing ends + else if (pendingOptimisticRequests === 0) { clearPolling(); - if (displayedLabel !== '') { - updateLabel(''); - } if (hadServerLabel && reasoningHistory.length > 0) { ConciergeReasoningStore.clearReasoning(reportID); } } - prevServerLabelRef.current = serverLabel; + // Use ref to check current value without depending on displayedLabel in deps + if (displayedLabelRef.current === targetLabel) { + prevServerLabelRef.current = serverLabel ?? ''; + return; + } + + const now = Date.now(); + const timeSinceLastUpdate = now - lastUpdateTimeRef.current; + const remainingMinTime = Math.max(0, MIN_DISPLAY_TIME - timeSinceLastUpdate); + + if (updateTimerRef.current) { + clearTimeout(updateTimerRef.current); + updateTimerRef.current = null; + } + + // Immediate update when enough time has passed or when clearing the label + if (remainingMinTime === 0 || targetLabel === '') { + displayedLabelRef.current = targetLabel; + // eslint-disable-next-line react-hooks/set-state-in-effect -- guarded by displayedLabelRef check above; fires once per serverLabel/optimistic transition + setDisplayedLabel(targetLabel); + lastUpdateTimeRef.current = now; + } else { + // Schedule update after debounce + remaining min display time + const delay = DEBOUNCE_DELAY + remainingMinTime; + updateTimerRef.current = setTimeout(() => { + displayedLabelRef.current = targetLabel; + setDisplayedLabel(targetLabel); + lastUpdateTimeRef.current = Date.now(); + updateTimerRef.current = null; + }, delay); + } + + prevServerLabelRef.current = serverLabel ?? ''; - // Cleanup timer on unmount return () => { if (!updateTimerRef.current) { return; } clearTimeout(updateTimerRef.current); }; - // eslint-disable-next-line react-hooks/exhaustive-deps -- startPolling/clearPolling are plain functions stable via React Compiler - }, [serverLabel, reasoningHistory.length, reportID, pendingOptimisticRequests, translate, displayedLabel]); + // eslint-disable-next-line react-hooks/exhaustive-deps -- displayedLabelRef avoids depending on displayedLabel; startPolling/clearPolling use refs + }, [serverLabel, reasoningHistory.length, reportID, pendingOptimisticRequests, translate]); useEffect(() => { isOfflineRef.current = isOffline; @@ -303,9 +307,6 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) ); const kickoffWaitingIndicator = () => { - if (!isAgentZeroChat) { - return; - } setPendingOptimisticRequests((prev) => prev + 1); startPolling(); }; @@ -325,7 +326,7 @@ function useAgentZeroStatusIndicator(reportID: string, isAgentZeroChat: boolean) setPendingOptimisticRequests(0); }, [newestActorAccountID, serverLabel, pendingOptimisticRequests, reportID]); - const isProcessing = isAgentZeroChat && !isOffline && (!!serverLabel || pendingOptimisticRequests > 0); + const isProcessing = !isOffline && (!!serverLabel || pendingOptimisticRequests > 0); return { isProcessing,