Repository navigation
Fix unread marker disappears after revealing Concierge history in side panel - #100038
Conversation
|
@codex review |
collectioneur
changed the title
![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: 90afad24e9
ℹ️ 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".
|
@codex review |
…om side panel context
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90afad24e9
ℹ️ 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".
|
@mananjadhav 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] |
| const isInSidePanel = useIsInSidePanel(); | ||
| const {sessionStartTime: mainDMSessionStartTime} = useConciergeSessionState(); | ||
| const {sessionStartTime: sidePanelSessionStartTime} = useSidePanelState(); | ||
| return isInSidePanel ? sidePanelSessionStartTime : mainDMSessionStartTime; |
There was a problem hiding this comment.
question: For visibility we use isConciergeSidePanel (useReportActionsVisibility.ts) and this hook uses isInSidePanel. Do you think this can be an issue?
There was a problem hiding this comment.
I kept the hook simple to avoid extra lookup. In practice, they never diverge because the hook's output is gated behind isConciergeHiddenHistory in ReportActionsList.tsx.
Since that gate guarantees it's a Concierge report, isInSidePanel effectively means the exact same thing as isConciergeSidePanel. Also in our one edge case (the #admins side panel), the gate is false, so the hook's value is just safely ignored
| @@ -123,6 +124,117 @@ describe('useUnreadMarker', () => { | |||
| expect(result.current.unreadMarkerReportActionIndex).toBe(-1); | |||
| }); | |||
|
|
|||
| it('keeps the unread marker when Concierge hidden history is revealed at once (side panel "Show history")', () => { | |||
There was a problem hiding this comment.
question: This test is named after the side-panel "Show history" bug, but it never passes newMessageBoundaryTime, bumps the greeting to after the unread message, and needs a second rerender(fullHistory) before asserting.
There was a problem hiding this comment.
Good catch, but these details are actually intentional! This test tests the fallback scenario when no session boundary is available.
- The greeting tracks "now", so bumping it after the unread message simulates production. It forces the created time to go backwards, which is what our guard needs to test.
- The second
rerender(fullHistory)is required because, without a boundary, the marker legitimately doesn't show on the first render. The fix stops that first render from incorrectly pushing the read watermark, allowing the marker to appear on the next render.
I’ll rename the test to make this setup more obvious
| // report.lastReadTime and would drag the watermark to "now". | ||
| const newestVisibleReportActionCreated = sortedVisibleReportActions.at(0)?.created ?? ''; | ||
| const prevNewestVisibleReportActionCreated = usePrevious(newestVisibleReportActionCreated); | ||
| const mostRecentReportActionCreated = sortedVisibleReportActions.find((action) => action.reportActionID !== CONST.CONCIERGE_GREETING_ACTION_ID)?.created ?? ''; |
There was a problem hiding this comment.
sortedVisibleReportActions.find(...) runs on every render before the guards that usually skip the watermark push (isAnonymousUser, existing marker, newest created actually advanced). usePrevious must stay unconditional; the scan does not. The greeting is at index 0 when present, so this does not need a full walk.
There was a problem hiding this comment.
Since the list is newest-first, find always short-circuits at index 0 or 1. The cost is basically constant anyway, so moving it behind guards won't change performance
mananjadhav
left a comment
mananjadhav
left a comment
There was a problem hiding this comment.
Left a few small comments.
|
@mananjadhav comments addressed 🙂 |
|
Code changes are all fine. Testing this out. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid-unread-marker.movAndroid: mWeb Chromemweb-chrome-unread-marker.moviOS: HybridAppios-unread-marker.moviOS: mWeb Safarimweb-safari-unread-marker.movMacOS: Chrome / Safariweb-unread-maker.mov |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks, looks good to me
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 mountiny 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.75-0 🚀
|
No help site changes requiredThis PR is a bug fix that restores already-intended behavior. It adds no feature, setting, button, or user-facing copy, so nothing under Why I reached that conclusionWhat the PR changes: it fixes the green New unread divider disappearing when Concierge hidden history is revealed in the side panel. The change is entirely internal read-watermark logic:
The linked issue, #100006, confirms this: the expected result was already "the green New divider is visible in the side-panel chat." The PR makes the app match documented-as-intended behavior rather than changing it. What the help site currently says: no article documents the unread New divider, marking a message as unread, or the Concierge Show history reveal. The only Concierge side-panel coverage is the generic entry point in Because I wrote no article text, there were no feature names, tab names, or button labels to verify against the live UI. One optional gap, unrelated to this PR: the message context menu actions Mark as unread / Mark as read and the New divider aren't documented anywhere on the help site, even though @collectioneur, no help site PR was created because no changes are required. If you disagree — for example if Concierge Anywhere and Show history should be documented as part of this feature's rollout — reply with |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.75-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Fixes the "New" unread marker disappearing when Concierge hidden history is revealed via "Show history".
useUnreadMarkernow only advances the read watermark when the newest visible action actually advances (a bulk history reveal no longer pushes it past unread messages), ignores the synthetic Concierge greeting as a push target, and a newnewMessageBoundaryTimeparam prevents revealed history actions from being treated as read-on-arrival.Fixed Issues
$ #100006
Tests
Offline tests
QA Steps
Same as tests.
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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-09-02.at.12.09.15.mov