Fix thinking indicator flash when agent reply lands before NVP clear - #93648
Conversation
The server clears the agentZeroProcessingRequestIndicator NVP separately from the reply Pusher event, arriving ~250ms later on the client. During that window the thinking bubble remains visible even though the reply is already rendered. Suppress any agent from candidateAgentIDs as soon as their ADDCOMMENT is the newest action in the report, so the bubble hides the moment the reply lands rather than waiting for the NVP clear.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@codex review |
|
@neil-marcellini 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6fc1311f3b
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (latestReplyActorID !== undefined) { | ||
| candidateIDs.delete(latestReplyActorID); |
There was a problem hiding this comment.
Wait for the NVP clear before dropping agent bubbles
When an agent-authored ADDCOMMENT arrives while agentZeroProcessingRequestIndicator is still truthy, this removes that agent from candidateAgentIDs before ConciergeThinkingBubble can render. The per-agent hook explicitly treats this scenario as an intermediate/status action and keeps the indicator until the server clears the NVP; bypassing it here makes the thinking bubble disappear mid-processing even though serverAgentIDs still says the agent is active.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I'm not sure what fix is being proposed here... what we have now is pretty bad because a "thinking" indicator appears very briefly and then disappears. It seems like it would be better for some kind of "mid-processing thinking indicator" to not show up at all than to have a buggy flash happen.
|
Simple PR, no C+ needed. Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
🚧 @neil-marcellini has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/neil-marcellini in version: 9.4.17-0 🚀
Bundle Size Analysis (Sentry): |
|
🤖 Help site review — no changes required I reviewed the changes in this PR against Expensify's help site files under Conclusion: No help site updates are needed, so I did not create a draft PR. Why: This is a pure frontend timing fix in
The only related help articles are under @marcaaron, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.17-3 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.17-3 🚀
|
Explanation of Change
Reorders two operations in
ProcessAgentZeroRequest.phpso the agent's reply is sent before the thinking indicator is cleared.Previously: the backend cleared the thinking indicator NVP first, then sent the reply via
processAgentZeroAddedComments. Since Auth dispatches a separate Pusher event for each operation, the indicator could vanish on the client before the reply arrived — depending on Pusher delivery timing.After this change: the reply is sent first, then the indicator is cleared. The indicator now disappears at or after the reply arrives.
Fixes
$ https://github.com/Expensify/Expensify/issues/648410
Tests
_tests/integration/script/bwm/ProcessAgentZeroRequestTest.php— 58/59 pass (one pre-existing failure unrelated to this change).Web QA
Prerequisites: Use a workspace that has a custom agent configured.
Scenario 1: Indicator clears at or after the reply
Mobile QA
No mobile QA — the thinking indicator lives in NewDot (React Native) and the fix is backend-only. Mobile behavior is covered under Web QA above since it shares the same Pusher events.
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.