Show New message marker when a user marks their own message as unread - #95217
Show New message marker when a user marks their own message as unread#95217MelvinBot wants to merge 18 commits into
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:
|
![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: 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.
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. |
Marking an optimistic self-message unread while offline dropped the New marker on reconnect: the optimistic->confirmed merge shifts the action's created, which re-runs handleReportChangeMarkAsRead, and on native (Visibility.hasFocus() is always true) that fires readNewestAction, nulling manuallyMarkedUnreadReportActionID. Instead of bailing whenever the field is set (which would suppress auto-read for genuinely newer messages too), latch whether the marked action was ever seen optimistic - the optimism flag is cleared on the same key at confirm time, so it must be captured beforehand - and skip the reconnect auto-read only in that case. Applied to both the useMarkAsRead hook and the money-request report list's inline copy of the logic. Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
Done — pushed Instead of bailing whenever What changed (2 files):
Both are keyed by Verification
|
|
@MelvinBot revert that. |
…ad action" This reverts commit 1b0eb1e.
|
@MelvinBot revert f76655a |
…r sends a message" This reverts commit f76655a.
|
Reverted |
|
@MelvinBot in useMarkAsRead.ts, add a check for manuallyMarkedUnreadReportActionID == null in this condition |
Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
Done — pushed Added the if (report?.manuallyMarkedUnreadReportActionID == null && !isUnread(report, transactionThreadReport, isReportArchived) && !isLastActionUnread) {
return;
}Now when a manual-unread marker is set (where Checks: oxfmt, ESLint, and React Compiler compliance clean on the file; One thing worth confirming on your native repro: with this, once you're actively viewing (visible + focused + scrolled to end) the read path fires on the next report/visibility change — so the marker clears as soon as you're looking at it. If you intended it to persist until you leave and only clear on the next open, let me know and I'll gate it differently. |
|
@MelvinBot |
|
Agreed — good catch. When a manual mark is active, the Why it moves down. After you mark M unread, the marker anchors on M and Fix — const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic || !!manuallyMarkedUnreadReportActionID;Now while a mark is active, every self-message except the marked one yields, the scan falls through to M, and M keeps the marker via the Why it's safe:
Want me to push this? (Static trace only — worth confirming with your send-after-mark repro that the marker now stays put on the marked message.) |
|
@MelvinBot ok implement it |
… sent Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
Done — pushed Gated const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic || !!manuallyMarkedUnreadReportActionID;So while a manual mark is active, every self-message except the marked one yields — the scan falls through to the marked action, which keeps the marker via the Checks: oxfmt, ESLint, and React Compiler compliance clean on the file; Worth a quick confirm on your repro: mark an older message unread → send a new message → the marker stays on the marked one and doesn't jump to the just-sent message. |
|
@MelvinBot instead of in the shouldIgnoreUnreadForCurrentUserMessage condition, !manuallyMarkedUnreadReportActionID should be below like |
|
Agreed — the two are behaviorally identical, and your placement reads better. I'd make the swap. Right now the manual-mark term is folded into const shouldIgnoreUnreadForCurrentUserMessage = isNewMessage || isPreviouslyOptimistic;
if (isFromCurrentUser) {
if (prevUnreadMarkerReportActionID && !manuallyMarkedUnreadReportActionID) {
return !shouldIgnoreUnreadForCurrentUserMessage;
}
return false;
}Both collapse to the same output — with
And yours is the better expression of intent: Want me to push this refactor? |
|
@MelvinBot we should remove !isNextMessageUnread from |
…nt unread state Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
Done — dropped One thing worth flagging so you know exactly what this did and didn't change: in both real callers this was already a no-op — the marker already anchors on the marked action when a newer message arrives. The removal just makes that intent explicit (and drops a comment that was actively misleading). Why it was already a no-op — the direction of
|
|
@MelvinBot remove |
…geMarkAsRead early return Co-authored-by: Olly <Ollyws@users.noreply.github.com>
|
Done — pushed The guard is now back to: if (!isUnread(report, transactionThreadReport, isReportArchived) && !isLastActionUnread) {
return;
}
Net effect: when a self-authored action is manually marked unread (so
|
|
@MelvinBot how can we prevent readNewestAction in handleReportChangeMarkAsRead from immediately resetting the manuallyMarkedUnreadReportActionID when a new message is received? |
|
Prevent it at the call site, not inside // A report-data change (new message arriving, or the user's offline message confirming on reconnect)
// re-runs this effect via the lastVisibleActionCreated dep. Don't auto-read it away — readNewestAction
// would null manuallyMarkedUnreadReportActionID and drop the marker the user explicitly set.
if (report?.manuallyMarkedUnreadReportActionID) {
return;
}It has to be a full Why this path is the right layer. The one product call to make — how broadly to scope itThe presence guard above keeps the report unread through any incoming message until the user reads it themselves. That's stronger than just fixing your native reconnect repro — with it, a genuinely-new message from someone else arriving while you're viewing the report also won't auto-read. I think that's the correct read of "the user deliberately marked this unread," but it is a behavior change worth a conscious decision. If you want to protect only the optimistic→confirmed false trigger (your offline-mark → reconnect repro) and let genuinely-new incoming messages auto-read as they do today, scope the guard to that transition instead — latch whether the marked action was optimistic before it confirms (you can't read Either way, the Which behavior do you want — honor the manual-unread through all incoming messages (simple presence guard), or only survive the optimistic-confirm (scoped latch)? I can push whichever. (Static analysis only — I couldn't drive a native build this run, so please confirm against your reconnect repro.) |
|
@MelvinBot The problem with preventing readNewestAction is that it also prevents new messages from being read as they come in, so when you re-open the report they will be marked as unread. |
|
You're right — bailing whenever But the trigger we actually need to neutralize is narrow: on reconnect the effect re-fires only because the marked message's own optimistic→confirmed That distinction is already expressed in this same file — // The user deliberately marked an action unread. On reconnect its optimistic→confirmed created shift
// re-fires this effect, but nothing new arrived — don't let readNewestAction wipe the mark. Still allow
// the read when a genuinely-new message from someone else exists, so incoming messages are read as normal.
if (report?.manuallyMarkedUnreadReportActionID) {
const hasUnreadFromOthers = sortedVisibleReportActions.some(
(action) =>
report.lastReadTime &&
report.lastReadTime < action.created &&
(isReportPreviewAction(action) ? action.childLastActorAccountID : action.actorAccountID) !== currentUserAccountID,
);
if (!hasUnreadFromOthers) {
return;
}
}This resolves all three cases you've raised:
It also drops the optimistic-latch idea entirely — "no unread from others" subsumes it and is simpler. One product call to confirm when you test: reading that genuinely-new other-user message still goes through Want me to push this version? (Static analysis only — I couldn't drive a native build this run, so please confirm against your offline-mark → reconnect and new-message-arrives repros.) |
|
@MelvinBot NO the point is that if we mark as unread, then recieve new messages from another user, if readNewestAction is not executed after recieving new messages they will show up as unread when we re-open the report, if we DO execute it, the optimistic unread marker will be removed and it will immediately dissapear. NOTE: THIS HAS NOTHING TO DO WITH GOING OFFLINE. |
|
You're right — I anchored on the offline/optimistic path and that was wrong. The "was previously optimistic" check doesn't help here: in your scenario the marked action was never optimistic, and the effect re-fires because another user's message bumps The real problem is that So there's no timing trick that resolves it — it's a product call, and I think honoring the manual-unread is the right one: don't auto-read while The one case worth an explicit decision: a newer message arriving while you're actively watching the report scrolled to the bottom. Today that auto-reads; with the guard it stays unread with the marker in place. Keeping it unread is consistent ("manual unread wins until I read it"), but it's a real behavior change from today, so it's your call to confirm. Why decoupling (advance lastReadTime but keep the id) isn't a clean third optionThe marker anchors purely on If you're aligned on honoring manual-unread, the change is |
4 participants
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