Fix stuck Concierge thinking indicator on batched Onyx updates#85620
Fix stuck Concierge thinking indicator on batched Onyx updates#85620marcochavezf wants to merge 1 commit intomainfrom
Conversation
…dates 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) <noreply@anthropic.com>
|
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
melvin-bot
bot
requested review from
trjExpensify
and removed request for
a team
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dffb249ec
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // 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; |
There was a problem hiding this comment.
kickoffWaitingIndicator() is invoked on every submit (src/pages/inbox/report/ReportActionCompose/ReportActionCompose.tsx:339-340), but this version check only tells us that some write hit reportNameValuePairs_${reportID} after the latest kickoff. If a user sends two Concierge messages in quick succession before the first one finishes, the first request’s later SET/CLEAR batch will make serverProcessedSinceKickoff true and the branch below clears optimisticStartTime, even though the second request is still pending. That regresses the multi-message flow called out in the QA steps by dropping the “Thinking…” indicator until the second request eventually emits its own label.
Useful? React with 👍 / 👎.
|
PR doesn’t have any new product considerations as a code clean-up PR. Unassigning and unsubscribing myself. |
2 participants
Explanation of Change
When a client misses real-time Pusher events (e.g., tab backgrounded, brief disconnect) and catches up via
GetMissingOnyxMessages, Onyx batches the SET and CLEAR merges for theagentZeroProcessingRequestIndicatorNVP into a single notification with the final (empty) value. TheuseAgentZeroStatusIndicatorhook never sees the intermediate non-empty server label, sooptimisticStartTimeis never cleared — the first branch that clears it only fires whenhasServerLabelis true, which never happens when the SET+CLEAR are coalesced.Fix: Track NVP write count via a direct
Onyx.connectsubscription. When the indicator NVP is written to (even if the final rendered value is empty), increment a version counter. On kickoff, snapshot the counter. The main effect compares versions to detect that the server processed the request, even when React/Onyx batching hides intermediate values.This is not a timeout hack — it's fixing the state machine to correctly detect server activity that Onyx's merge batching would otherwise hide.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/612534
Tests
Unit tests added:
should clear optimistic state when server SET and CLEAR arrive in the same Onyx batchshould clear optimistic state when server CLEAR arrives without a preceding SET being seenOffline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
N/A - no UI changes, logic-only fix in hook
Android: mWeb Chrome
N/A - no UI changes, logic-only fix in hook
iOS: Native
N/A - no UI changes, logic-only fix in hook
iOS: mWeb Safari
N/A - no UI changes, logic-only fix in hook
MacOS: Chrome / Safari
N/A - no UI changes, logic-only fix in hook