Take the user into the Concierge thread after asking a question - #97816
Take the user into the Concierge thread after asking a question#97816rushatgabhane wants to merge 1 commit into
Conversation
|
This PR adds a new |
|
@marcaaron 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] |
|
sorry for the ping, this is a WIP |
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.
|
![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: cdfa5751a8
ℹ️ 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".
| } | ||
| }, | ||
| }); | ||
| timeoutID = setTimeout(() => Onyx.disconnect(connectionID), CONCIERGE_THREAD_NAVIGATION_TIMEOUT_MS); |
There was a problem hiding this comment.
When a Concierge comment is sent offline or remains queued for more than 15 seconds, this timeout disconnects before the request reaches the server. If the user is still in the Concierge chat when connectivity returns, the eventual childReportID update is therefore missed and the user is not taken into the newly created thread. Tie cleanup to request completion/failure or network state rather than elapsed wall-clock time.
Useful? React with 👍 / 👎.
| // The user may have moved on while the thread was being opened; only follow the reply if they | ||
| // are still sitting in the chat they asked from. | ||
| if (Navigation.getTopmostReportId() !== reportID) { |
There was a problem hiding this comment.
When the user opens an RHP or another surface while the Concierge DM remains in the central pane, this guard still passes because getTopmostReportId() only reads the central-pane report, as documented in Navigation.ts. The delayed callback then changes the report behind the surface even though the user has moved on; compare the focused/active route captured when the question was sent instead.
Useful? React with 👍 / 👎.
1 participant
Explanation of Change
Front-end side of Concierge answering each question in its own thread, gated by the
conciergeRespondInThreadbeta.ConciergeDraftProviderandAgentZeroStatusProviderboth gate onreportID === conciergeReportID, which a thread fails, so streaming would have silently stopped working there; both now also match a report whoseparentReportIDis the Concierge DM.addActionsalso follows the user into the thread after they ask. Write commands resolve as soon as they are queued and never carry the server's response, so the thread ID arrives aschildReportIDon the user's own comment a moment later and the navigation waits for that, giving up after 15 seconds or if the user has navigated away.Requires https://github.com/Expensify/Web-Expensify/pull/55108; until that ships
childReportIDnever appears and the navigation is inert, so this is safe to merge first.Fixed Issues
$ #94503
PROPOSAL:
Tests
conciergeRespondInThreadbeta and ask Concierge a question in your Concierge DM.Offline tests
QA Steps
conciergeRespondInThreadbeta and ask Concierge a question in its DM.PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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.