Repository navigation
Conversation
Anchor each pregenerated follow-up turn to its question reportActionID and use it to detect a newer user message. The follow-up reconciliation no longer wipes the server indicator for that message, the composer skips its optimistic clear while a turn is still reconciling, and the pending follow-up list stops suppressing indicators once the user has sent again.
…r its followup list
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
The code standards check asks for plain comment sentences, so the dashes and the semicolon-joined clause become separate sentences.
![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: 97389e5147
ℹ️ 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".
| const isPendingFollowupReply = !!pendingFollowupReplyID && pendingFollowupReplyID === newestReportAction.reportActionID; | ||
| if (!isLabelOn || (!isNewestFromBeforeLabel && !isPendingFollowupReply)) { |
There was a problem hiding this comment.
When the canonical pregenerated action reaches REPORT_ACTIONS before the four-second local reveal finishes, usePendingConciergeResponse takes its persistedAction path and discards the pending response without ever creating CONCIERGE_PENDING_FOLLOWUP_LIST. If the user sent another message in that window, the old reply is future-stamped newest and the processing label was already on, so both exceptions here are false and line 201 removes Concierge; the indicator is again hidden until the newer answer arrives. Track the pending response's reply ID (or otherwise preserve turn identity), not only the post-apply skeleton marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
hmm I couldnt hit this live. in 8 runs (prod + staging) the reveal ended at 7-14s and the server copy landed 50-76s after the click, so it would need a ~5000 char reply. keeping it out of this PR
A label that is already on when the gate mounts can belong to a reply that has landed and is waiting for the server clear, so recording it as the baseline kept the bubble on top of that reply. Also add the missing Given/When/Then comments to the new suppression tests.
|
@situchan this is ready to review. My only concern is this #97049 (comment) .
|
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.
|
|
Please fix conflict |
Code reviewThe approach looks right to me: anchoring each pregenerated turn to its question with 1. The indicator can still disappear when the canned reply moves below the typed message (medium)
Here is how it fails, using the "canned reply jumps below the typed message" behavior you reported:
The fix only works if the gate sees the label go off and back on after the reply becomes newest. That does not happen if the clear and set arrive in one batch, or if B is processed while the old turn is still running. Suggestion: keep the reply → question link after the pending marker clears. For example, store the last pregenerated reply ID with its 2. Any change to the agent list resets the baseline (minor)
Everything else looks fine:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc4c242313
ℹ️ 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".
| const {unmount} = renderHook(() => usePendingConciergeResponse(REPORT_ID)); | ||
| await waitForBatchedUpdates(); | ||
|
|
||
| await Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${REPORT_ID}`, {[REPORT_ACTION_ID]: canonicalFollowupAction('2026-07-27 10:00:04.000')}); |
There was a problem hiding this comment.
This newly added case has a // Given section and a // Then section, but the canonical-reply action here is not introduced by a // When comment; add that section before the merge so the test satisfies the repository's required Given/When/Then structure.
AGENTS.md reference: AGENTS.md:L90-L90
Useful? React with 👍 / 👎.
| const hasNewerRequest = hasUserMessageSinceQuestion(getAllReportActions(reportID), pendingFollowupList.questionReportActionID, CONST.ACCOUNT_ID.CONCIERGE); | ||
| if (!hasNewerRequest) { | ||
| clearAgentZeroProcessingIndicator(reportID, CONST.ACCOUNT_ID.CONCIERGE); | ||
| } |
There was a problem hiding this comment.
When a newer message is still in flight as the canonical reply for the earlier followup gains a real <followup-list>, this path deliberately leaves Concierge's NVP set but immediately clears CONCIERGE_PENDING_FOLLOWUP_LIST. AgentZeroStatusGate uses that marker as its only isPendingFollowupReply identity, so once it disappears, the future-stamped old reply is still newest and the continuously-set server-agent ID never captures it as a new baseline; the gate removes Concierge for the remainder of the newer turn. Fresh evidence beyond the earlier pre-reveal race is that this occurs on the normal post-reveal reconciliation (and TTL) path, so preserve the old-reply identity independently of the skeleton marker.
Useful? React with 👍 / 👎.
| action.actorAccountID !== agentAccountID && | ||
| isActionOfType(action, CONST.REPORT.ACTIONS.TYPE.ADD_COMMENT) && | ||
| action.created > question.created, |
There was a problem hiding this comment.
In a supported multi-agent report, a later ADDCOMMENT from any co-resident agent satisfies this predicate because only Concierge's account ID is excluded. When the old followup reconciles, hardClearIndicator therefore mistakes that agent reply for a new human request, skips clearing Concierge's processing slot, and can leave Concierge's thinking bubble visible until the safety timeout even though no user sent another message; distinguish human-authored comments or exclude every agent identity rather than only the passed agent.
Useful? React with 👍 / 👎.
|
🤖 Web test steps verification results: ❌ fail Typed messages and followup clicks worked without a stuck indicator. When a message was sent right after a followup click, Concierge never replied and no indicator appeared, even after about 3 minutes.
Code check for regressionsI found no regressions in the existing flows. Steps 1 and 2 above confirm the normal typed and followup paths still behave. The step 3 failure is probably not caused by this PR. The PR changes only local Onyx state, and Behavior changes I checked
|
Explanation of Change
Sending a message while a pregenerated followup reply was still loading hid the Concierge thinking indicator for the rest of the turn, because followup suppression covered the whole report and the status gate took the canned reply, which sorts after the new message, as the answer. This ties each pregenerated turn to its question so suppression lifts once a newer user message exists, and keeps the indicator while the newest action is a reply to an earlier turn.
Fixed Issues
$ #97049
PROPOSAL: #97049 (comment)
Tests
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged theExpensify/designteam so the design team can review the changes.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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
682752085-2e93a3e8-b410-45ef-b524-9b3762a7dd2a.mp4