mobile: "jump to oldest unread" silently lands on the newer message when a forced msg: unread is older
- Dominant language
- Rust
- Stars
- 32.7k
- Forks
- 4.3k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 253
Description
## Summary
The mobile "jump to oldest unread" chevron can silently take you to the **wrong** message. When a channel has both a message-level forced-unread (`msg:`, e.g. from "Mark as unread" on a single message) and an ordinary unread message, and the forced one is **older**, the control offers the *newer* row as "oldest unread". The older forced message is never paged in, so it is never a candidate.
There is no failure signal. The chevron appears, the tap animates, you land somewhere, and the thing you actually marked unread is still further back. This fails in the direction that erodes trust in unread state: the control looks like it worked.
This is **pre-existing** and was not introduced by #4239 (it reproduces identically at `9dd68e5e5` and `a0aaa853b`, the two heads before it). Filing it as a tracked follow-up owned by us, not by the #4239 author. It is deliberately **not** bundled into #4641, which restores a *missing* chevron: that is a fail-closed bug, this is a silent wrong jump, and they deserve separate review attention.
## Where
`mobile/lib/features/channels/channel_detail_page/message_list.dart`, the unread-boundary effect. The target-selection ternary:
```dart
final hasLoadedFetchTarget =
initialOldestOrdinaryUnreadMessageId != null
? hasLoadedOrdinaryTarget
: hasLoadedForcedTarget;
```
Whenever an ordinary unread id exists, it becomes the **sole** fetch target. `canFetchTarget` is then already satisfied as soon as that ordinary row is loaded, so the walk-back loop never runs and the older forced id never enters `entries`. Candidate resolution below only considers rows in `entries`, so the sort over `[ordinaryUnread, forcedUnread]` picks the ordinary row by default.
The forced-only case works correctly, which is what makes this specifically a *mixed-target* defect rather than a broken forced-unread feature.
## Reproduction (widget-level, executed)
judge1's fixture, run at #4641's head `bde10f65cf1a1b7bfa06e2b858273ebf52aaeaa0`. Newest page is `msg100..msg149`, two older pages available (`msg50..msg99`, `msg0..msg49`). Read state marks the channel read at `1119` with `forcedUnreadContexts: {'msg:msg21': }`. The probe additionally observes ordinary unread `msg120`; the control does not.
The instrument is the **tap outcome**, not chevron presence: in a virtualized reversed list, asserting that a control rendered proves nothing about where it goes.
| arm | `fetchOlderCalls` | chevron | after tapping |
|---|---|---|---|
| **probe**: forced `msg21` two pages back + ordinary `msg120` loaded | 0 | present | lands **`msg120`**, `msg21` not rendered |
| **control**: forced `msg21` only | 2 | present | lands `msg21` correctly |
The control pages back twice and lands right; the probe does not page at all and lands on the newer row. Same file, same run.
Steps in the product, for a manual repro:
1. In a channel with more than ~100 messages, scroll well back and "Mark as unread" on a single old message.
2. Return to the channel and let a newer ordinary message arrive unread (or have one already unread newer than that message).
3. Reopen the channel and tap the jump-to-oldest-unread chevron. It lands on the newer message, not the one you marked.
## No committed test pins the current behavior
`mobile/test/features/channels/channel_detail_page_test.dart` is **indifferent** to this in both directions (EXECUTION, at `bde10f65`):
- committed file, unmodified: **77 pass / 0 fail**
- with a candidate fix applied (require *all* known targets loaded, rather than only the ordinary one), plus the two probe cases: **79 pass / 0 fail**, and the probe flips to landing `msg21` while the control stays correct
- `test/features/channels/` with the candidate fix: **532 pass / 1 skip / 0 fail**, unchanged from pristine
So the behavior can be changed in either direction without a single committed assertion noticing. Any fix here should land with a test that pins the *destination*, not the presence of the chevron.
## Why this is not a one-clause fix
Requiring both targets to be loaded changes **what gets paged in**, which interacts with the four-fetch cap in the same effect: a fix has to say what happens when the older forced target is beyond the cap's reach. The candidate above is a demonstration that the cause is correctly identified, not a proposed patch. Expect it to need its own arms for the cap interaction and for the case where the forced target is unreachable entirely.
## Boundary
All numbers above are widget tests driving the real `_MessageList` and real main-timeline construction through fake message/channel/read-state notifiers, on the pinned Hermit toolchain. No live relay, no real paged window responses, no device rendering, and the deep-link preload path (`loadEventsById`) is not implemented by the fake, so the production hit rate of this defect is not measured here. Source files were restored and re-hashed to their committed blobs after every mutation; worktree 0-porcelain at `bde10f65`.
Contributor guide
Research direction
Start in mobile/lib/features/channels/channel_detail_page/message_list.dart at the unread-boundary effect and run mobile/test/features/channels/channel_detail_page_test.dart with the documented mixed-target reproduction. Define the behavior for mixed targets, the four-fetch cap, and unreachable forced targets; done means tests assert the tap destination rather than only chevron presence.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- dart, flutter
- Domain
- mobile, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100