feat(mobile,desktop): open a thread at the last-read reply - #5983
feat(mobile,desktop): open a thread at the last-read reply#5983mfethe1 wants to merge 4 commits into
Conversation
|
Rebuilt on current |
Screenshots — desktop thread resumeA thread with one already-read reply followed by 40 unread ones, reopened. Before — the reader lands past every unread replyToday the panel bottom-pins on open. The reader arrives at reply 40, with After — the reader lands on the first unread replyThe same thread with this PR. The A fully read thread is untouchedNothing unread means no divider and no resume — today's bottom pin stands, so These come from Capturing these found a real bug, now fixed in The pre-existing unit tests cover One note on that spec, since it is easy to get wrong: it asserts with Screenshots are hosted on 🤖 Generated with Claude Code |
|
@klopez4212 — flagging this to you since #5645 touched the same thread surface. Some context on how these two relate, because there was overlap and it's now This branch originally carried both a mobile jump-to-latest pill and a Worth a look regardless of who lands it: capturing the before/after Mobile is No reviewer was ever auto-assigned here — CODEOWNERS is |
Screenshots — mobile thread resumeCompanion to the desktop screenshots above. A nested thread with 30 replies, These are rendered from the widget tree at a 390×844 phone surface with the Resuming — the thread opens at the first unread replyThe channel was last read through Nothing unread — today's behaviour is untouchedThe read marker is past every reply, so there is no divider and no resume: The never-opened guard renders identically — deliberatelyA reader with no marker over any reply has nowhere to return to, so an Worth stating because it is the case most likely to feel wrong if it were Verified on this commit with the pinned toolchain (Flutter 3.41.7 / Dart The states above come from the widget tests already in this PR 🤖 Generated with Claude Code |
themiguelamador
left a comment
There was a problem hiding this comment.
I found and fixed two blocking issues plus one gate failure.
- Desktop resumed never-opened threads at their first unread reply, while mobile intentionally keeps first-open threads pinned to the tail. That contradicted the PR's cross-client contract. The fix carries a frozen
hasReadHistorysignal into the desktop selector and adds a direct never-read regression test. - Mobile coverage asserted only that the unread divider existed; it did not prove the thread actually landed there. The new 60-reply widget test verifies the first unread reply is inside the list viewport and that
Latestremains available. - The PR grew
ChannelScreen.tsxbeyond the 1,000-line file-size ratchet. The fix moves its load-bearing resume commentary to the dedicated hook and leaves the screen at 994 lines.
Verified: desktop Biome, TypeScript, file-size ratchet, full desktop unit suite, E2E build, thread-resume Playwright spec (2/2), Flutter analyze, Dart format, and relevant Flutter tests (144/144). Rendered desktop behavior was also inspected with the mock bridge.
Fix: https://github.com/Complear/buzz/commit/816270b2a
Branch: https://github.com/Complear/buzz/tree/review/pr-5983-fix
…n tests Addresses review feedback on block#5983. Desktop resumed a never-opened thread at its first unread reply while mobile kept it pinned to the tail, contradicting the cross-client contract this PR describes. selectThreadResumeTargetId now takes hasReadHistory and returns null when it is false, so a thread the reader has never been in keeps today's tail pin. The signal comes from useChannelUnreadState's openFrontierSeconds — the per-visit channel frontier captured during render before the mark-read effect — and rides the existing settled-query latch rather than one of its own, so a partially loaded reply list cannot latch it false. It is deliberately not derived from getMessageReadAt: desktop markers are hierarchical, so msg:<id> resolves to max(own, active channel marker) and every reply reports read history the moment the channel is open. An earlier attempt did exactly that and still resumed a never-opened thread to the top. Coverage was the second finding, and the more important one. Both clients asserted the unread divider existed without ever proving the thread landed on it — a bottom-pinned thread with the divider off-screen above passed every existing test. Desktop adds 03-never-read-thread-opens-at-newest, asserting with toBeInViewport rather than a bounding box against the scrollable replies container, whose box is the full content and reports every child as inside. Mobile adds a 60-reply widget test on a 390x844 surface checking the first unread reply and the divider are within the visible rect, the newest and oldest are not, and Latest still returns to the newest reply. Mobile's own hasThreadReadHistory guard turned out to have no coverage at all: deleting it left all 134 tests in the file green. Since mobile is the reference implementation of the contract desktop was being fixed to match, it now has a never-read test of its own. ChannelScreen.tsx exceeded the 1000-line ratchet at 1013. The load-bearing latch and fetchStatus commentary moved to useThreadOpenResumeTarget.ts, where the latch lives, leaving the screen at 999. No limit raised, no override added. Known deviation, documented in code: desktop uses the channel marker as its read-history proxy while mobile folds in msg: and thread: markers, so a reader who read a reply from the Home inbox in a channel they never opened gets the tail pin instead of a resume. Conservative — it never drags the reader wrongly. Exposing getOwnTimestamp through AppShellContext would close it and is out of scope here. Verified: tsc clean, Biome clean, file-size and px-text guards pass, desktop unit 4964/4964, thread-resume e2e 3/3, flutter analyze clean, dart format 408 files unchanged, mobile 1432/1432. Each new test was confirmed to fail with its guard removed and pass with it restored. Signed-off-by: Michael Feth <michael@jira-flow.com>
|
@themiguelamador — thanks, this was a good catch. All three are fixed. Taking them in turn, Your fix branch was unreachable. 1. Desktop resumed never-opened threads. Confirmed and fixed.
Worth recording why it is not derived from 2. Mobile proved the divider existed, not that the thread landed on it. Confirmed and fixed. New 60-reply widget test on a 390x844 surface asserting the first unread reply and the divider Both new tests are proven to discriminate rather than merely pass, which mattered here — an earlier 3. File-size ratchet. One thing you did not flag, which your review led me to. Mobile's Three caveats, all pre-existing, none introduced here:
All of the above is in Verification (Windows host, pinned toolchain, Flutter 3.41.7 / Dart 3.11.5): 🤖 Generated with Claude Code |
Threads settle on the newest reply, which is right when you are caught up and wrong when you are not: returning to a thread you were part-way through drops you past everything you had not read yet. Resume at the oldest unread reply instead, expressed as the existing initial-settle target rather than a competing scroll, so it inherits the generation guard and the hydration race handling already there. An unread divider marks the boundary. Resuming only applies to a thread the reader has actually seen before. A reader with no marker over any reply has nowhere to return to, so an ordinary first open still settles on the tail; the divider still marks the replies as new. The mobile jump-to-latest pill this work originally carried landed separately in block#5645, so only the resume behaviour remains here. Signed-off-by: Michael Feth <michael@jira-flow.com>
The resume never fired. Reopening a thread renders one reply off the channel timeline before the reply query returns, and the latch — which keeps a captured null as a deliberate decision — fired on that render and captured null, because the unread marker had not been recomputed yet. The real target arrived with the remaining replies and was then ignored for the life of the open, leaving the reader bottom-pinned at the newest reply: exactly what the resume exists to replace. Gate the latch on `fetchStatus === "idle"`. `isPending` is the wrong signal — a refetch over cached data reports `isPending: false` while still in flight, which is precisely the render that captured null. An idle fetch status means nothing is on the wire, which is true once the replies have landed and also true for a forum thread whose query is disabled and whose replies resolve from the channel timeline. Add an e2e spec that catches this. The existing unit tests cover `selectThreadResumeTargetId`'s decision and never exercise the scroll, so they stayed green throughout. The spec asserts with `toBeInViewport`, which honours clipping by scrolling ancestors — measuring against the replies container's own box instead reports every child as inside, since that box is the full scrollable content, and passes while the panel is visibly bottom-pinned. Signed-off-by: Michael Feth <michael@jira-flow.com>
…n tests Addresses review feedback on block#5983. Desktop resumed a never-opened thread at its first unread reply while mobile kept it pinned to the tail, contradicting the cross-client contract this PR describes. selectThreadResumeTargetId now takes hasReadHistory and returns null when it is false, so a thread the reader has never been in keeps today's tail pin. The signal comes from useChannelUnreadState's openFrontierSeconds — the per-visit channel frontier captured during render before the mark-read effect — and rides the existing settled-query latch rather than one of its own, so a partially loaded reply list cannot latch it false. It is deliberately not derived from getMessageReadAt: desktop markers are hierarchical, so msg:<id> resolves to max(own, active channel marker) and every reply reports read history the moment the channel is open. An earlier attempt did exactly that and still resumed a never-opened thread to the top. Coverage was the second finding, and the more important one. Both clients asserted the unread divider existed without ever proving the thread landed on it — a bottom-pinned thread with the divider off-screen above passed every existing test. Desktop adds 03-never-read-thread-opens-at-newest, asserting with toBeInViewport rather than a bounding box against the scrollable replies container, whose box is the full content and reports every child as inside. Mobile adds a 60-reply widget test on a 390x844 surface checking the first unread reply and the divider are within the visible rect, the newest and oldest are not, and Latest still returns to the newest reply. Mobile's own hasThreadReadHistory guard turned out to have no coverage at all: deleting it left all 134 tests in the file green. Since mobile is the reference implementation of the contract desktop was being fixed to match, it now has a never-read test of its own. ChannelScreen.tsx exceeded the 1000-line ratchet at 1013. The load-bearing latch and fetchStatus commentary moved to useThreadOpenResumeTarget.ts, where the latch lives, leaving the screen at 999. No limit raised, no override added. Known deviation, documented in code: desktop uses the channel marker as its read-history proxy while mobile folds in msg: and thread: markers, so a reader who read a reply from the Home inbox in a channel they never opened gets the tail pin instead of a resume. Conservative — it never drags the reader wrongly. Exposing getOwnTimestamp through AppShellContext would close it and is out of scope here. Verified: tsc clean, Biome clean, file-size and px-text guards pass, desktop unit 4964/4964, thread-resume e2e 3/3, flutter analyze clean, dart format 408 files unchanged, mobile 1432/1432. Each new test was confirmed to fail with its guard removed and pass with it restored. Signed-off-by: Michael Feth <michael@jira-flow.com>
Co-authored-by: Michael Feth <michael@jira-flow.com> Signed-off-by: Michael Feth <michael@jira-flow.com>
bf4c42c to
f4802a6
Compare
|
@themiguelamador — separate from the review itself, a process ask, because I think it is CI has never run on this PR.
They do not surface in This is not specific to this PR — 14 of my 16 open PRs are in the same state, none of them Would you be able to approve the runs here, or point me at whoever owns that? I would rather you Happy to keep waiting on the code review itself; this is only about getting the pipeline to run. |





Summary
Threads settle on the newest reply. That is right when you are caught up and wrong when
you are not: returning to a thread you were part-way through drops you past everything you
had not read yet, with no sign of what you skipped.
This opens a thread at the oldest unread reply instead, on mobile and desktop, with an
unread divider marking the boundary.
The contract, as the code now implements it: resuming applies only to a thread the
reader has read something in before. A reader with no read marker anywhere has nowhere to
return to, so a first open still settles on the tail exactly as it does today. The scroll
is what the guard suppresses, not the marker — the divider still renders.
Desktop did not honour that second paragraph when this PR was first reviewed. It
resumed never-opened threads to their first unread reply while mobile pinned them to the
tail, so the two clients disagreed and the description above was true of only one of them.
@themiguelamadorcaught it. It is fixed inf11ca54, along with the reason the testscould not have caught it themselves (below).
The four commits, on
d8281b9bd1fd08ba0dbddfetchStatus === "idle"), plus the e2e spec that catches the failure.f11ca54hasReadHistory); landing coverage on both clients; relocate latch commentary out ofChannelScreen.tsxto stay under the 1000-line ratchet.f4802a6Mobile
The resume is expressed as the existing
InitialThreadTailSettletarget rather than asa competing scroll, so it inherits that machinery's generation guard and hydration-race
handling instead of racing it.
Read markers are snapshotted during build, before the post-frame pass marks the thread
read — markers are monotonic, so reading them afterwards reports the whole thread as
already seen and leaves nothing to resume to. The snapshot folds the channel and thread
markers in via
effectiveMessageReadAtrather than reading a baremsg:marker; withoutthat, a thread whose replies were only ever covered by the channel marker reads as entirely
unread. A captured
nullis a real answer ("never read"), so the map is built withputIfAbsentand tested withcontainsKey, never collapsed with??.The divider itself is new on this client:
unread_divider.dartis a "New" separator builtlike
DayDividerbut tinted with the error colour, rendered on the matching reply.Desktop
selectThreadResumeTargetIdcomputes the target and returnsnullwhenhasReadHistoryis false, mirroring mobile's guard.
useThreadOpenResumeTargetlatches the result once perthread open and yields to any deep link, which names its own target and wins.
Two things about that latch are load-bearing:
off the channel timeline before the query returns, and latching there captures a
nullthe hook then keeps as a deliberate decision, ignoring the real target when it arrives.
fetchStatusis the signal rather thanisPending, since a refetch over cached datareports
isPending: falsewhile still in flight.hasReadHistoryrides that same gate and arrives already frozen fromuseChannelUnreadState. It cannot be re-derived fromgetMessageReadAt, which ishierarchical:
msg:<id>resolves tomax(own marker, active channel marker), so once thechannel is open every reply reports a read time and a snapshot taken here claims read
history for a thread the reader has never opened. That was measured, not theorised — the
first attempt at the fix did exactly this and still resumed to the top.
The resume rides the panel's existing external-scroll-target slot, so a deep link simply
outranks it (
ChannelPane.tsx:466-471), and it never pulses the row the way a citationdoes (
scrollTargetHighlights,ChannelPane.tsx:838-846).Thread panel loading state (
f4802a6)Reopening a thread refetches its replies over a list that is already committed, and two
places treated that refetch as "no content yet":
useAnchoredScrollwas handedisLoading: threadRepliesPending || repliesRenderState === "pending", so it refused to position against a list it could already measure. It nowkeys on
repliesRenderStatealone (MessageThreadPanel.tsx:512).threadRepliesPendingwas true, replacing acommitted list mid-read. It is now also gated on
repliesRenderState !== "list"(
MessageThreadPanel.tsx:649-651).Both are inside
MessageThreadPanel; no otheruseAnchoredScrollconsumer changes.Shared scroll hook (
useAnchoredScroll.ts:438-447)scrollToMessageImperativenow cancels a pendingpinToBottomOnMountframe beforecentring. That mount pin is otherwise cancelled only by the channelId reset effect, and the
thread panel is keyed by a constant so it never remounts per thread — left armed, the queued
frame fires after the resume scroll and yanks the reader to the bottom for the rest of the
open. The mount path re-arms the pin itself when centring fails
(
useAnchoredScroll.ts:648-657). This hook is shared withMessageTimeline,InboxDetailPane,AgentSessionThreadPanelandAgentSessionTranscriptList; the changeonly bites when a deliberate scroll-to-message is issued while a mount pin is still queued.
Known limits
Three, all pre-existing rather than introduced here, stated because a reviewer will find
them:
msg:andthread:markers. A reader who read a reply from the Home inbox in a channel they neveropened gets the tail pin instead of a resume. Conservative — it never drags the reader
wrongly. Exposing
getOwnTimestampthroughAppShellContextwould close it and is outof scope here.
index > 0(
MessageThreadPanel.tsx:694, pre-existing), so a thread whose very first reply isunread shows no divider; mobile renders it on the matching reply unconditionally
(
thread_detail_page.dart:763). Every resume case has a read reply above the boundary,so both clients show a divider there — only the all-unread thread differs. Worth a
decision, not folded in here.
ReadStateManager.initialize()resolves, thefrozen frontier latches null for that visit and a legitimate resume is skipped. It
self-heals on re-entry, and the divider already misreports in that same window.
Related issue
Supersedes #5657 (closed), which carried this same resume work plus a mobile
jump-to-latest pill.
Searched for duplicates before opening: #5645 ("Polish mobile message threads and
composer", merged) landed the jump-to-latest pill independently, generalized into the shared
LatestMessageButton. That overlap is not duplicated here — this branch carries onlythe resume behaviour, which #5645 does not implement. No other open or merged PR covers
thread resume at the last-read reply.
Filed separately while working on this: #6046 — a future-dated
created_atpushes thechannel read marker into the future and silently marks every later message read. It reaches
the same comparison this feature keys off, but it predates this work and affects the unread
badge and divider identically, so it is not folded in here.
Testing
Repo CI has not run on this PR. Every workflow is
action_requiredpending maintainerapproval; DCO is the only check that has executed. Everything below is a local run at
f4802a6on a Windows host with the repo-pinned toolchain (Flutter 3.41.7 / Dart 3.11.5).Desktop
pnpm exec tsc --noEmit— cleanpnpm exec biome check .— clean, 2091 filesratchet:
ChannelScreen.tsx999,MessageThreadPanel.tsx998,ChannelPane.tsx966.ChannelScreen.tsxgot there by relocating commentary into the hook that owns the latch.No limit raised, no override added — but review comments that add lines to those three
files will need the same treatment.
pnpm test— 4963/4964. The one failure issrc/shared/lib/useDocumentVisible.test.mjs:254, a timing-sensitive polling test. Neitherit nor its subject is touched by this PR (
git log <base>..HEAD -- …/useDocumentVisible.*is empty), so it runs unmodified upstream code.
playwright test --project=smokeacrossthread-resume(3),thread-unread(13),thread-focus-mode(2) — 18/18 passedMobile
flutter analyze— No issues founddart format --output=none --set-exit-if-changed .— 412 files, 0 changedflutter test— 1480/1480 passedBefore/after screenshots for both clients are in comments below — desktop captured from the
e2e spec, mobile rendered from the widget tree at phone size. Producing the desktop set is
what caught the original defect on that side: the resume never fired at all, and the unit
tests could not have seen it because they cover the target decision and never exercise
the scroll.
Coverage added by this PR
thread_unread_marker_test.dart— 10 unit tests over the marker predicate: strictnewer-than at the marker, forced-unread override, own-pubkey skip and its
case-insensitivity, a captured
nullmeaning never-read, empty thread.channel_detail_page_test.dart— 5 widget tests in two groups. Three open a nested(submessage) thread the way tapping a summary row does: divider on a never-read reply, no
divider once fully read, and a guard that the channel marker alone can mark a nested reply
read. Two more prove the landing on a 60-reply thread at a 390x844 surface — that the
first unread reply and the divider are inside the visible rect while the newest and oldest
are not, that
Latestis present and returns to the newest reply, and that a never-readthread still opens at the tail.
threadResumeTarget.test.mjs— 10 tests, including deep-link precedence, nested threadheads, the never-read guard, and its counterpart: a thread that has been seen but whose
replies are all unread does resume, to the first reply. Those two cases are why
firstUnreadReplyIdalone cannot decide this.thread-resume.spec.ts— 3 desktop e2e tests: resume to the first unread, a fully readthread bottom-pinning, and a never-read thread opening at the newest reply. Registered in
playwright.config.tsunder thesmokeproject. Test 01 reopens the thread after achannel switch, which is the refetch path
f4802a6fixes.On the tests being meaningful. The original coverage on both clients asserted the
divider existed and never that the thread landed on it — a bottom-pinned thread with the
divider off-screen above passed every one of them. That is what let the desktop divergence
survive. Each new test here was run with its guard removed to confirm it fails, and
restored to confirm it passes. The desktop e2e assert with
toBeInViewport(); an earlierrevision of this PR used a bounding-box comparison against the scrollable replies container,
whose box is the full content, so every child measured as "inside" and the assertion passed
green while the panel was visibly wrong.
Mobile's own
hasThreadReadHistory— the reference implementation desktop was fixed tomatch — turned out to have no coverage at all: deleting the guard left all 134 tests in the
file green. It has a test now.
Not in scope
getOwnTimestampthroughAppShellContext).index > 0divider gate, and Future-dated created_at suppresses unread state for every later message in a channel #6046.