fix: keep Concierge side panel question + thinking indicator visible across clock-skew race - #94814
Conversation
…across clock-skew race The Concierge hidden-history view classified the just-sent Ask Concierge message as pre-session history when its skew-adjusted created timestamp landed before sessionStartTime, collapsing the panel to the welcome greeting and suppressing the thinking indicator. Treat the current user's optimistic pending-add comment as a current-session message across all four session-boundary checks via a shared isCurrentUserPendingAddAction predicate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@thelullabyy 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] |
![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: a8879b935e
ℹ️ 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".
| function isCurrentUserPendingAddAction(reportAction: OnyxInputOrEntry<ReportAction>, currentUserAccountID?: number): boolean { | ||
| return !!currentUserAccountID && reportAction?.actorAccountID === currentUserAccountID && reportAction?.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD; |
There was a problem hiding this comment.
This only treats the message as current-session while pendingAction === ADD, but addActions() clears that flag in its successData without changing the optimistic created timestamp. In the clock-skew case this patch is meant to handle, the AddComment success can arrive before Concierge replies, so the same just-sent action immediately stops matching these checks again and the side panel can collapse back to the welcome/history state until a later action arrives. Consider retaining another current-session marker or comparing against the skew-adjusted send time after pendingAction is cleared.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, this was a real gap. The pendingAction === ADD check only held during the optimistic window, so once addActions cleared the flag in its successData the skewed
message dropped back out of the session before Concierge replied.
Addressed in 59d4230: a small useCurrentSessionUserActionIDs hook records the IDs of the user's own messages captured while pending and keeps them in-session for the rest of the
session, independent of pendingAction. Added a unit test for the post-success case.
Keep skewed Concierge messages visible after ADD succeeds
This only treats the message as current-session while
pendingAction === ADD, butaddActions()clears that flag in itssuccessDatawithout changing the optimisticcreatedtimestamp. In the clock-skew case this patch is meant to handle, the AddComment success can arrive before Concierge replies, so the same just-sent action immediately stops matching these checks again and the side panel can collapse back to the welcome/history state until a later action arrives. Consider retaining another current-session marker or comparing against the skew-adjusted send time afterpendingActionis cleared.Useful? React with 👍 / 👎.
…s clears pendingAction Track the current user's own session messages by ID (captured while pendingAction === ADD) so a clock-skewed just-sent message keeps counting as current-session even after the AddComment success clears its pendingAction but before Concierge replies. Previously the message fell out of the session the moment pendingAction was cleared, collapsing the side panel back to the welcome/history state and hiding the thinking indicator until a later action arrived. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
|
…onciergeIndicators Add tests that exercise hasSessionActivity (welcome-state suppression, a reply after the session start, and the current user's clock-skewed optimistic message), restoring diff coverage for the predicate the fix introduced. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Build report actions via createRandomReportAction and type the mocked hooks with jest.mocked instead of `as ReportAction` / `as jest.Mock` casts, which CI's type-aware lint rejects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Calling setState inside the effect tripped react-hooks/set-state-in-effect (cascading renders). Move the accumulation to the render body using the adjust-state-during-render pattern, which keeps the returned set correct within the same render and avoids the effect entirely. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@thelullabyy this is ready for your review whenever you get a chance — all checks are green, and the testing steps + videos are in the PR description. Let me know if you need anything else. |
|
@dilshodmackbook-sketch Please add recording to all envs |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d638851ac0
ℹ️ 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".
| isCurrentUserPendingAddAction(action, currentUserAccountID) || | ||
| currentSessionUserActionIDs.has(action.reportActionID) || |
There was a problem hiding this comment.
When the clock-skewed optimistic message is in the main Concierge DM, hasUserSentMessage is now set from currentSessionUserActionIDs, but isCurrentSessionAction returns earlier for isConciergeMainDM using only action.created >= sessionStartTime. In an existing Concierge chat with hidden history and a just-sent ADD whose created is before sessionStartTime, the welcome state is disabled but the user's message is still filtered out until a later action arrives. Apply the pending/captured-ID check before or inside the main-DM branch as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. The main-DM branch of isCurrentSessionAction was only doing the timestamp comparison, so I ORed in the same pending/captured-ID predicate used in the side-panel branch. Now a skewed just-sent ADD stays in-session and is rendered immediately in the main DM too, consistent across both branches.
The main-DM branch of isCurrentSessionAction only checked action.created >= sessionStartTime, so a clock-skewed just-sent ADD (created < sessionStartTime) suppressed the welcome state via hasUserSentMessage but was itself filtered out of the chat until a later action arrived. OR in the pending/captured-ID predicate to match the side-panel branch and keep the fix consistent. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…changes) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4e18220 to
4b88dec
Compare
|
Heads up on the failing Jest checks: they are not real test failures. The jobs are getting OOM-killed on the runner (exit code 143), and a different shard goes down on each run. Other unrelated PRs are hitting the same exit-143 in this same window, so it looks like a repo-wide runner issue rather than anything in this PR. Locally the related suites pass. I'll re-trigger once the runners settle. |
|
Updated — reworked to the server-anchored approach we agreed on.
Added tests for the new helper plus a hook-level regression test that the reply stays visible under negative skew. Tested locally across the flow, no console errors — ready for another look. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66237c00b1
ℹ️ 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".
| automatic: false, | ||
| avatar: allPersonalDetails?.[accountID]?.avatar, | ||
| created: getDBTimeWithSkew(Date.now() + createdOffset), | ||
| created: anchorCreatedToServer ? getServerAnchoredDBTime(Date.now() + createdOffset) : getDBTimeWithSkew(Date.now() + createdOffset), |
There was a problem hiding this comment.
When a client clock is ahead and networkTimeSkew becomes known or more negative between two Concierge sends (for example, an OpenReport/OpenApp response arrives after the first optimistic send), this branch applies the negative skew to the later action but not the earlier one. Report actions are sorted by created, so the second message can receive an earlier timestamp and render above the first; getDBTimeWithSkew avoided exactly this reordering. Please keep Concierge optimistic created values monotonic while still using a server-anchored boundary for filtering.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. I kept the value server-anchored rather than reverting to getDBTimeWithSkew, because firstUserMessageCreated (the reply-visibility floor) is derived from the question's created — if it drops back to the client clock the server-stamped reply falls below it again.
Instead, getServerAnchoredDBTime now clamps forward past the report's last action (lastVisibleActionCreated), so a later send can never sort above an earlier one while the value stays server-anchored for filtering. The clamp never fires in the common stable-skew case, so created is unchanged there. Added unit tests for it.
There was a problem hiding this comment.
@dilshodmackbook-sketch can you check this one? Let's try to get this PR merge this week
There was a problem hiding this comment.
Yep, this is handled in the latest commit (4d0695754ac). getServerAnchoredDBTime now clamps the anchored created forward past the report's last action (lastVisibleActionCreated), so a later send can never sort above an earlier one even if skew turns negative between two sends. It stays server-anchored for filtering, and the clamp is a no-op in the normal stable-skew case. Added unit tests for it — more detail in the thread just above. Should be good to go.
There was a problem hiding this comment.
Done — switched to date-fns-tz toDate(notBeforeDBTime, {timeZone: 'UTC'}), which is how DateUtils parses DB times elsewhere, so no more manual replace(' ', 'T') + 'Z'.
Address review: getServerAnchoredDBTime applies skew in both directions, so if networkTimeSkew turns (more) negative between two Concierge sends, a later optimistic `created` could land before an earlier one and render above it — report actions sort by `created`. getDBTimeWithSkew avoided this by only pushing forward. Reverting the anchor isn't an option: firstUserMessageCreated (the reply-visibility floor) is derived from the question's `created`, so it must stay server-anchored or the server-stamped reply falls below it again. Instead, clamp the anchored `created` forward past the report's last action (lastVisibleActionCreated) via a new notBeforeDBTime argument, so a later send never sorts above an earlier one while the value stays server-anchored for filtering. No clamp fires in the common (stable-skew) case. Add unit tests for the clamp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| const [reportActions] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`); | ||
| const actionsList = Object.values(reportActions ?? {}); | ||
|
|
||
| const hasSessionActivity = |
There was a problem hiding this comment.
This hook feeds ComposerTypingIndicator and ConciergeThinkingMessage, both live in the Concierge chat. Now they re-render on every report-action change in that report (every incoming message, edit, reaction, read receipt), instead of only when the suppress boolean flips.
Maybe we can use the selector could have been kept and simply closed over currentUserAccountID (it's already a component-scope value) plus the pendingAction check — exactly what the other two touched hooks do inline. Basically, I think we should restore the selector form to avoid the perf regression @dilshodmackbook-sketch
There was a problem hiding this comment.
Good call — restored the selector so the boolean is derived inside useOnyx and the indicators only re-render when it flips, not on every report-action change. Kept the pendingAction check and closed over currentUserAccountID in the selector, matching the other two hooks.
| const datetime = timestamp ? new Date(timestamp) : new Date(); | ||
| let anchoredMs = datetime.valueOf() + networkTimeSkew; | ||
| if (notBeforeDBTime) { | ||
| const floorMs = new Date(`${notBeforeDBTime.replace(' ', 'T')}Z`).valueOf(); |
There was a problem hiding this comment.
Can we use time/date lib to handle replace(' ', 'T') + 'Z? @dilshodmackbook-sketch
There was a problem hiding this comment.
Done — using date-fns-tz toDate(notBeforeDBTime, {timeZone: 'UTC'}) now, the same helper DateUtils uses to parse DB times, so no more manual replace(' ', 'T') + 'Z'.
Address review: replace the manual `replace(' ', 'T') + 'Z'` DB-time parsing in
getServerAnchoredDBTime with date-fns-tz `toDate(..., {timeZone: 'UTC'})`, matching how
DateUtils parses DB times elsewhere.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address review: reading the whole report-actions object and computing the boolean in the component body made useShouldSuppressConciergeIndicators (and its consumers ComposerTypingIndicator / ConciergeThinkingMessage) re-render on every report-action change. Restore the selector form so the hook only re-renders when the boolean flips; the selector closes over the component-scope currentUserAccountID and keeps the pendingAction check, matching what the other two hooks do inline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@dilshodmackbook-sketch Let's retest the PR again since we have done a big refactor here |
|
Retested after the refactor:
It's a shared-JS change (timestamp filtering + the Onyx selector), so native behavior is identical. Happy to re-run anything specific if you'd like. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 407f7c0a6d
ℹ️ 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".
| if (Number.isFinite(floorMs) && anchoredMs <= floorMs) { | ||
| anchoredMs = floorMs + 1; |
There was a problem hiding this comment.
When notBeforeDBTime is a prior Concierge action that was stamped from the ahead client clock (for example an existing optimistic/successful message from before this fix, or after skew changes between sends), this branch moves the new server-anchored question forward to that future timestamp. The side-panel filter then uses that question as firstUserMessageCreated and only keeps replies with action.created >= firstUserMessageCreated, so a server-stamped Concierge reply that arrives in the real server interval before this artificial future time is hidden again until history is shown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There was a problem hiding this comment.
Good catch, and it's actually sharper than a corner case. For the question, the anchored value is Date.now() + createdOffset + networkTimeSkew = the message's true server time, so the clamp fires only when lastVisibleActionCreated >= server-now — i.e. only when the prior action is itself on an ahead client clock. Whenever it fires, it pushes the question into the future, and since firstUserMessageCreated is derived from that created, a server-stamped reply lands below it and gets filtered out. So the clamp can never both fire and stay server-safe — one created can't be a monotonic sort key against a future-clocked predecessor and a reply-visibility floor at the same time.
Fixed by decoupling the two jobs instead of touching the clamp: firstUserMessageCreated only ever needs to trim the user's own pre-question messages, so I now apply that floor to current-user actions only. Concierge replies are server-stamped and already bounded by the server-anchored sessionStartTime, so they no longer depend on the question's (possibly clamped) created and stay visible. The monotonic clamp is unchanged, so the previous ordering fix still holds. Added a unit test covering a question clamped forward onto an ahead client clock with a reply arriving below it. (3fee1d78487)
…estion timestamp The monotonic clamp on the question's `created` only ever fires when the prior action sits on an ahead client clock, pushing the question into the future. Since firstUserMessageCreated is derived from that `created`, a server-stamped reply lands below it and is filtered out. Apply the firstUserMessageCreated floor to current-user actions only; Concierge replies stay bounded by the server-anchored sessionStartTime. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Overall changes look good now. I will upload recording shortly... |
thanks a lot |
|
🚧 Gonals 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/Gonals in version: 9.4.45-0 🚀
|
Help site review: no changes requiredI reviewed the changes in this PR and no updates to the help site ( Why: This is an internal bug fix for a clock-skew race condition in the Concierge side panel. It anchors There is no new feature, setting, tab, label, button, or user-facing workflow introduced or changed. The Concierge experience behaves the way the help site already describes it should, so nothing in If you believe there's a specific documented behavior this change alters, let me know which article and I'll take another look. @dilshodmackbook-sketch — since no help site changes are required, there's no linked docs PR to review or mark |
|
This PR failing because of the issue #97187 |
|
Thanks for flagging @jponikarchuk. This one isn't introduced by this PR — it's a pre-existing gap on the Android / main-DM path (this PR fixed the side-panel version for #90974). I've traced it and left a proposal with the root cause and fix on #97187. |
|
🚀 Deployed to production by https://github.com/marcaaron in version: 9.4.45-14 🚀
Bundle Size Analysis (Sentry): |
7 participants
Explanation of Change
When a user with prior Concierge history asks Concierge from search, clock skew could make the exchange disappear from the side panel in two ways:
Root cause: the Concierge "hidden history" view decides whether an action belongs to the current session by comparing
createdtimestamps againstsessionStartTime. But those timestamps come from different clocks:sessionStartTimeis stamped with the raw client clock (DateUtils.getDBTime()).getDBTimeWithSkew, which only applies skew when it is positive — so when the client runs ahead of the server (negative skew) it stays on the raw, ahead client clock.So when the client clock runs ahead of the server,
sessionStartTimeand the user's question sit on the ahead client clock while the reply sits on the (behind) server clock. The reply'screatedlands belowsessionStartTime/firstUserMessageCreated, falls outside the session, and is filtered out — the question can flicker away and the reply may never appear. This poisons the four session checks:hasUserSentMessage,hasSessionActivity,firstUserMessageCreated, andisCurrentSessionAction.Fix: make every comparison same-clock by anchoring both anchors to the server clock at the source, rather than heuristically tracking membership after the fact.
getServerAnchoredDBTimein@libs/NetworkState, which appliesnetworkTimeSkewin both directions so the value tracks the server clock even when the client is ahead.getDBTimeWithSkewonly pushes forward (to avoid reordering messages in its otherlastReadTime/ ordering call-sites), so it can't be reused here.sessionStartTime(the side panel boundary inSidePanelContextProviderand the main-DM boundary inConciergeSessionContext) through it.createdthrough it via a newanchorCreatedToServerflag onbuildOptimisticAddCommentReportAction, applied only for Concierge chats inaddActions.Now
sessionStartTime, the user's question, and Concierge's reply are all on the server clock, so the existingcreated >= sessionStartTime/firstUserMessageCreatedcomparisons work unchanged. The arrival-baseduseCurrentSessionActionIDsheuristic is removed; the sharedisCurrentUserPendingAddActionpredicate is kept as a safety so a still-optimistic (pending) question is counted even before itscreatedis comparable.getDBTimeWithSkewis left untouched, so non-Concierge behavior and message ordering are unchanged.Fixed Issues
$ #90974
PROPOSAL: #90974 (comment)
Tests
Precondition: the test account has a previous chat history with Concierge.
Offline tests
QA Steps
Same as Tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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.mov
Android: mWeb Chrome
Screen.Recording.2026-07-21.at.01.22.23.mov
iOS: Native
ios.mov
iOS: mWeb Safari
Screen.Recording.2026-07-21.at.01.24.04.mov
MacOS: Chrome / Safari
Screen.Recording.2026-07-21.at.01.17.40.mov
Screen.Recording.2026-07-21.at.01.18.01.mov