Show New message marker when a user marks their own message as unread - #95217
Conversation
Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
🤖 I looked into the failing checks. There's no code issue to fix — the only failing check is
This is a human-completed checklist, so I can't/shouldn't auto-tick it — Every automated code check is green: typecheck, all 8 test jobs, ESLint, Prettier, Bun tests, Storybook, knip, unused styles, React Compiler, and the completed builds all pass. The only checks still running are the long-lived standalone builds and perf tests, which don't indicate any problem with the diff. No commit was pushed since nothing in the code needs changing. |
|
🤖 The newly-failing check — Analysis: This PR only touches two TypeScript files — A JS/TS-only change cannot break native iOS Swift compilation — and indeed every other build passed, including the Android build Recommendation: Re-run the failing iOS build job — it should pass on retry. The other failing check, |
|
@MelvinBot mark this as ready to review. |
|
🤖 Done — this PR is now marked Ready for review. Two heads-ups before reviewers dig in:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c52f485375
ℹ️ 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 (isFromCurrentUser) { | ||
| // When an existing marker is being relocated (e.g. after the original unread message is deleted), | ||
| // allow the marker to land on a self-authored action. | ||
| // Otherwise, never anchor the "New" marker above a self-authored action on first open/re-entry. | ||
| if (prevUnreadMarkerReportActionID) { | ||
| return !shouldIgnoreUnreadForCurrentUserMessage; | ||
| } | ||
| return false; | ||
| // Only suppress the "New" marker for a self-authored message that was just sent (newly added or still | ||
| // transitioning from an optimistic action). An existing self-authored action that the user explicitly | ||
| // marked as unread should anchor the marker even when no marker exists yet (e.g. on first open/re-entry). | ||
| return !shouldIgnoreUnreadForCurrentUserMessage; |
There was a problem hiding this comment.
Preserve the self-message suppression for cold opens
This now treats any persisted self-authored action as a valid unread-marker anchor, not only actions explicitly marked unread. On a cold open/re-entry after the user's just-sent message has been confirmed with a server created time later than the optimistic lastReadTime, usePrevious initializes the previous-actions map with the current actions, so isNewMessage and isPreviouslyOptimistic are both false and this return shows the green marker above the user's own message, reintroducing the self-message marker case this guard previously prevented.
Useful? React with 👍 / 👎.
The failing build is a flaky iOS infra failure — not caused by this PRNo code fix is needed. This PR only changes two pure JS/TS files ( I attempted to re-run the failed job automatically but couldn't in this environment. Please click Re-run failed jobs on the failed build run, or push any trivial change to re-trigger it. Evidence
The two Note: I could not verify anything in a browser in this run, but that isn't needed here — the conclusion is based on the CI logs and the PR diff. |
Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
I pushed an empty commit ( As detailed in my previous comment: this PR only changes JS/TS (a display-logic function + a unit test), the iOS native The re-run is in progress now. The two I could not verify anything in a browser this run, but that isn't needed here — the conclusion rests on the CI logs and the PR diff. |
The flaky iOS build is resolved — only the checklist remainsThe previously-failing The only remaining red check is So there's no code fix required. Once the checklist is completed and the in-progress test jobs finish, the PR should be clean. Note: I could not verify in a browser in this run, but that isn't needed here — this is based on the CI status and the PR diff. |
The flaky iOS build is resolved — nothing left to fix in codeThe re-trigger commit (
So there's no code fix required — the only real failure was the flaky iOS build, which is now cleared. Just complete the checklist/testing sections when the PR is ready for review. Note: I could not verify anything in a browser in this run, but that isn't needed here — this is based on the CI check results and the PR diff. |
|
@Ollyws can you add a video test? The test steps are still the placeholders. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp01_Android_Native.mp4Android: mWeb Chrome02_Android_Chrome.mp4iOS: HybridApp03_iOS_Native.mp4iOS: mWeb Safari05_MacOS_Chrome.mp4MacOS: Chrome / SafariMacOS_Safari.mp4 |
|
@MelvinBot if you mark an optimistic message as unread while offline, the unread marker will disappear when you come back online. |
|
Fixed — it was branch staleness, not a defect in this PR. Merged Why it failed: CI tests the PR merged with Evidence and verificationIt was never this PR's code. The Verified after merging current
The only remaining red check is One local-only note: view run · no recording available |
|
All updated. |
luacmartins
left a comment
There was a problem hiding this comment.
Thanks! Can we simplify the comments in this PR please? They're quite long
|
@MelvinBot reduce the comments in this PR to be more concise. |
Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
Done — pushed The biggest cuts were the block comments in Two judgement calls worth flagging:
VerificationConfirmed comment-only by filtering the diff for non-comment lines — empty result. Tests (all pass, 735 total):
ESLint passes on the changed files. One pre-existing Not run: view run · no recording available |
|
@MelvinBot resolve conflicts |
…readMarker Co-authored-by: Olly <Ollyws@users.noreply.github.com> # Conflicts: # src/components/MoneyRequestReportView/MoneyRequestReportActionsList.tsx
|
Conflicts resolved — merged There was one conflict, in I took main's side, dropping this PR's added line. That's not a loss of behavior: the merged Everything else merged cleanly, and all of this PR's logic survived intact.
VerificationMerge shape: normal two-parent merge commit ( Tests — 772 passed, 0 failed:
Typecheck: the only errors are the two pre-existing Lint: the resolved file reports one I also confirmed no stale references were left behind: view run · no recording available |
|
@MelvinBot merge main |
…readMarker Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
Merged One thing to be aware of: main's #100947 deleted VerificationMerge was clean (
744 tests total across the suites this PR touches. view run · no recording available |
|
@luacmartins updated. |
luacmartins
left a comment
There was a problem hiding this comment.
LGTM. @lakchote all yours
|
🚧 luacmartins 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/luacmartins in version: 9.4.82-0 🚀
|
Help site review: one change required (a gap, not a correction)No existing article in What I did find is a gap: the Mark as unread / Mark as read actions are not documented anywhere on the help site, even though Chat Features and Moderation Tools covers every other message action (edit, delete, reply in thread, flag). Since this PR is what makes the marker behave correctly for a member's own message, it's the right moment to document the workflow. Draft PR: Docs updates for E/A#95217 document marking a chat or message as unread — one new article, What the article covers and how labels were verifiedCovered:
Labels came from source rather than paraphrase: Written against One caveat to check while you review. I could not verify the flow in the live UI — the per-message hover toolbar never mounted in the headless web session, so @Ollyws, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.4.86-0 🚀
|
|
🤖 Yes — help site changes are required. Draft PR: Docs updates for E/A#95217 document marking a chat or message as unread ( Mark as unread, Mark as read, and the green New marker have no coverage anywhere under This PR changed user-visible behavior worth documenting: marking your own message as unread now anchors the New marker above it, the marker stays put while you remain in the chat, and a just-sent message still never gets the marker. What the docs PR does
A new article rather than a new section in Label verification and the one thing I could not checkI could not verify against the running app. A web tester tried the flow but could not open the message context menu in the headless session — right-click returned
Behavior claims come from the same place: Mark as unread shows on a message and on a read chat ( Worth your attention: this PR's own test steps say the marker is still shown after navigating away and back, but the merged code clears
@Ollyws, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR |
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 9.4.89-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.4.89-0 🚀
Bundle Size Analysis (Sentry): |
|
⏱️ Ran out of time before I could finish. This usually means the task was too complex or I got stuck exploring. Please try again with a more specific request or post in #melvinbot-ai. |
Explanation of Change
When a user marks their own message as unread, the green "New" message marker was not shown above that message, even though the conversation was correctly bolded as unread in the LHN. Marking another user's message worked fine, and the marker only appeared inconsistently (e.g. after a marker already existed) and disappeared again after navigating away and back.
The write path is correct —
markCommentAsUnreadsetslastReadTimeto just before the marked action, so the action is genuinely unread regardless of author. The bug was purely in the display decision inshouldDisplayNewMarkerOnReportAction. For a self-authored action it hard-blocked the marker unless a marker already existed:prevUnreadMarkerReportActionIDstarts asnullon first open and resets tonullon re-entry, so thereturn falsesuppressed the marker in exactly the reported scenarios. This guard was introduced by #91940 (issue #91443) to stop the marker from anchoring above a message the user just sent, but that case is already fully captured byshouldIgnoreUnreadForCurrentUserMessage(isNewMessage || isPreviouslyOptimistic).This change collapses the branch to
return !shouldIgnoreUnreadForCurrentUserMessage, so:shouldIgnoreUnreadForCurrentUserMessageistrue), preserving the fix from [Due for payment 2026-07-03] [$250] Investigate: Self-authored posts should never be marked as unread #91443.The now-unused
prevUnreadMarkerReportActionIDparam is dropped from this function's destructuring. A unit test for the explicit self-mark-unread case was updated to assert the marker now shows.Fixed Issues
$ #94794
PROPOSAL: #94794 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
Offline tests
Same as Tests.
QA Steps
// TODO: The human co-author must fill out the QA tests you ran before marking this PR as "ready for review".
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)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/Videosundefined