Fix Concierge hidden-history hiding unread notification message on cold open#93711
Conversation
On cold open, startSession runs before oldestUnreadReportAction resolves, locking sessionStartTime to `now` and hiding the notification message behind "Show full history". Re-invoke startSession when the unread anchor resolves and allow ConciergeSessionContext to pull sessionStartTime back to an earlier boundary within an active session. Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
I created this PR from an upstream branch since I don't have push access to your fork. To take ownership of this branch and be able to push updates, run: Then you can close this PR and open a new one from your fork. |
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 extend the fix to cover:
@MelvinBot please investigate these scenarios and apply if valid. |
… return Use useInitial(report?.lastReadTime) so the unread anchor resolves when the report loads after first render, and add oldestUnreadReportAction?.created to the native focus-return effect deps so the boundary pulls back when the anchor arrives after returning to the chat. Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
Investigated all four scenarios. Applied 2 of them; the other 2 were either redundant or reintroduce a hazard this PR deliberately avoids. Pushed in 7c6fb2c. ✅ #2 — This was a real cold-login bug. The old ✅ #3 — add Done at ❌ #1 — fallback to This reintroduces the exact hazard the PR was built to avoid. ❌ #4 — capture open-time unread state before This is already how the architecture works: Verification: |
|
@MelvinBot merge main |
…onPullBackUnreadBoundary Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
🤖 Merged |
PR Author Checklist
|
|
@MelvinBot add this for testing steps Tests
|
| // On a cold open the report can be undefined on first render; useInitial waits for the | ||
| // first defined (pre-read) value instead of latching undefined like a first-render ref, | ||
| // so the unread anchor can still resolve after the report loads. | ||
| const initialReportLastReadTime = useInitial(report?.lastReadTime); |
There was a problem hiding this comment.
@MelvinBot Scope the usePaginatedReportActions change to Concierge only. ReportActionsView and ConciergeSessionContext are already Concierge-scoped; the shared-hook change is the only part affecting regular inbox chats.
Suggested approach:
- Add an option like
shouldSnapshotInitialLastReadTime(orisConciergeChat) tousePaginatedReportActions. - In
useReportActionsPagination, pass it only whenisConciergeChatReport(report, conciergeReportID)is true (moveconciergeReportID/isConciergeChatabove the hook call). - When the flag is false, keep production behavior:
useRef(report?.lastReadTime)on first render. - When true, keep
useInitial(report?.lastReadTime)so the unread anchor resolves on Concierge cold open.
This keeps #93196 fixed while leaving regular DM/group pagination unchanged.
Co-authored-by: Krishna <Krishna2323@users.noreply.github.com>
|
🤖 Scoped the What changed:
Regular inbox/DM/group pagination is now byte-for-byte the production first-render-ref behavior; only Concierge gets the first-defined snapshot. #93196 stays fixed. Checks run locally
|
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot mark this ready for review |
|
@Krishna2323 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
joekaufmanexpensify
and removed request for
a team
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid_hybrid.mp4Android: mWeb Chromeandroid_mWeb.mp4iOS: HybridAppios_hybrid.mp4iOS: mWeb Safariios_mWeb.mp4MacOS: Chrome / Safariweb_chrome.mp4 |
![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: d73477b217
ℹ️ 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".
| // Concierge only: snapshot the first non-undefined lastReadTime. On a cold open the report can be | ||
| // undefined on first render; useInitial waits for the first defined (pre-read) value instead of | ||
| // latching undefined like a first-render ref, so the unread anchor can still resolve after the report loads. | ||
| const firstDefinedLastReadTime = useInitial(report?.lastReadTime); |
There was a problem hiding this comment.
When the same report screen instance is reused for a different report, this useInitial(report?.lastReadTime) value is never reset and is captured even while shouldSnapshotInitialLastReadTime is false. If a user opens any non-Concierge report first and then switches to Concierge, the Concierge unread-anchor logic can compare Concierge actions against the previous report's lastReadTime, so an already-read Concierge chat may get a bogus oldestUnreadReportAction or the real unread anchor may be missed. Scope this snapshot by reportID or only seed it once the current report is known to be Concierge.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Investigated — this is a real code-smell but not a reachable bug. No change required, though scoping the snapshot by reportID would be cheap defense-in-depth.
The bot is correct that useInitial(report?.lastReadTime) runs unconditionally (hooks can't be conditional) and never resets within a component instance. The concern only becomes a bug if one usePaginatedReportActions instance serves two different reportIDs over its lifetime. That doesn't happen here:
- Navigating to a different report dispatches a PUSH, not NAVIGATE (
linkTo/index.ts:181-189—isNavigatingToReportWithSameReportIDis false for a different report). PUSH creates a new route key → a freshReportScreenmount → freshuseRef/useInitial. The reports split navigator is explicitly designed to hold multiple report screens with different IDs (ReportsSplitNavigator.tsx:22-25). - Consequently
reportIDis constant for the lifetime of anyusePaginatedReportActionsinstance, soreport?.lastReadTimealways belongs to the current report — never the previous one.
The "captured even while shouldSnapshotInitialLastReadTime is false" wording is technically true but harmless: the value is only read when shouldSnapshotInitialLastReadTime (= isConciergeChat) is true, and by the point above that's always the current Concierge report's own pre-read lastReadTime.
One edge case I checked (also fine)
isConciergeChat can flip false → true within the same instance if conciergeReportID (Onyx) resolves after first render. But during that window useInitial is still latching the same report's lastReadTime, so there's no cross-report contamination. Note the keyed remount at ReportActionsView.tsx:92 only remounts the inner InvertedFlashList, not this hook — so it isn't what protects against staleness; the PUSH-per-report navigation is.
Recommendation: Safe to resolve as-is. If maintainers want to harden against a future refactor that reuses a ReportActionsView instance across reportIDs, scoping useInitial by reportID (reset the snapshot when reportID changes) is a low-risk improvement — but it's optional, not a bug fix.
|
🤖 Investigated the failing Root cause: The check reads the checklist straight from the PR description body, not from comments. Every box in the body's Fix: Check all the boxes in the I didn't auto-check them since those boxes are your attestations — but you've already attested via your checklist comment, so if you'd like, reply Evidence
Note: this is a human-attestation check (it's intentionally excluded from MelvinBot's automatic CI-fix loop), which is why it needs the author to complete it rather than a code fix. |
|
@MelvinBot investigate this issue #93711 (comment) |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid_hybrid.mp4Android: mWeb Chromeandroid_mWeb.mp4iOS: HybridAppios_hybrid.mp4iOS: mWeb Safariios_mWeb.mp4MacOS: Chrome / Safariweb_chrome.mp4 |
joekaufmanexpensify
left a comment
joekaufmanexpensify
left a comment
There was a problem hiding this comment.
Makes sense for product 👍
|
🚧 cristipaval has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.4.19-1 🚀
Bundle Size Analysis (Sentry): |
|
🤖 I reviewed the changes in this PR against the help site articles under This is an internal bug fix to the Concierge main DM's history-collapse logic: it corrects how the unread/session boundary ( It introduces no new user-facing feature, setting, label, tab, or workflow — and the existing help articles don't document the Concierge Show full history session-boundary mechanics — so there's nothing in Since no docs update is needed, I did not create a draft PR. |
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.4.19-5 🚀
|
5 participants
Explanation of Change
The Concierge main DM collapses everything created before
sessionStartTimebehind Show full history. On a cold open (e.g. opening Concierge from an approval-notification email),startSessionruns as soon as cached actions exist — beforeoldestUnreadReportActionresolves. At that instant the unread anchor isundefined, so the boundary locks tonowand the unread notification message, created moments earlier, ends up on the hidden side of the boundary. The user only sees "Hi, how can I help?" until they click Show full history.The C+ reviewer verified that re-invoking
startSessionalone is insufficient:ConciergeSessionContextkeepsprevfor the duration of an active session, so a secondstartSessioncall is a no-op. This PR fixes both halves:ReportActionsView.tsx— the session-start effect now re-runs whenoldestUnreadReportActionresolves (addedoldestUnreadReportAction?.createdto its deps), and passes that action'screatedtime as the boundary.oldestUnreadReportAction.createdis derived from the pre-readlastReadTimesnapshot inusePaginatedReportActions, so it is immune to thereadNewestActionoptimistic bump of the livereport.lastReadTimetonow. It yields the same visible set as the intended pre-read boundary (nothing exists betweenlastReadTimeand the oldest unread action by definition).ConciergeSessionContext.tsx— within an active (non-expired) session,startSessionnow pullssessionStartTimeback to an earlierunreadBoundarywhen one arrives, instead of unconditionally returningprev. The session age (sessionCreatedAtRef) is left unchanged — only the display boundary is refined. When the boundary moves back, downstreamuseConciergeSidePanelReportActionsrecomputeshadMessagesAtSessionStart(it keys offsessionStartTimechanging), so the unread message renders and the greeting-only collapse is suppressed.When there is no unread action, the boundary still defaults to
now, preserving the intended fresh-session collapse for already-read chats.Fixed Issues
$ #93196
PROPOSAL: #93196 (comment)
Tests
Offline tests
N/A
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
// Please describe what QA needs to do to validate your changes and what areas do they need to test for regressions.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari