Skip to content

fix(mobile): keep latest messages above composer - #4981

Open
tellaho wants to merge 3 commits into
mainfrom
tho/mobile-latest-composer-offset
Open

fix(mobile): keep latest messages above composer#4981
tellaho wants to merge 3 commits into
mainfrom
tho/mobile-latest-composer-offset

Conversation

@tellaho

@tellaho tellaho commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Category: fix
User Impact: Mobile users who jump to Latest now see the newest message fully above the composer instead of partially hidden behind it.

Problem: The channel message list treated the raw viewport bottom as the latest boundary even though the composer occupies part of that viewport. Latest jumps and follow-mode corrections could therefore place the newest message underneath the composer.

Solution: Derive the latest alignment from the measured composer inset and use that same boundary for scrolling, follow detection, and layout correction.

Screen Recording 2026-08-05 at 5 18 19 PM
File changes

mobile/lib/features/channels/channel_detail_page/message_list.dart
Aligns Latest navigation and follow-mode correction with the visible bottom edge above the composer, and evaluates boundary state against the same geometry.

mobile/test/features/channels/channel_detail_page_test.dart
Adds a regression assertion that the newest live message clears the composer and that the Latest control disappears after navigation.

Reproduction steps

  1. Open a mobile channel with enough messages to scroll away from the newest message.
  2. Tap Latest.
  3. Confirm the newest message is fully visible immediately above the composer and the Latest control disappears.
  4. Resize the composer or keyboard while following latest and confirm the newest message remains above the composer.

Tested fix

The newest message remains fully visible above the composer after jumping to Latest.

Tested fix: latest message remains above the composer

Validation

  • flutter analyze — no issues
  • flutter test — 1,243 passed

Co-authored-by: Taylor Ho <taylorkmho@gmail.com>

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
@tellaho
tellaho marked this pull request as ready for review August 6, 2026 00:29
@tellaho
tellaho requested a review from a team as a code owner August 6, 2026 00:29
brow
brow previously approved these changes Aug 6, 2026
@brow

brow commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

🤖 Reviewed at 68e718f77a7dc854f242eb7205b7a6ecd63a8e38. The fix is correct and I measured it two ways. Non-blocking notes only.

The number that proves it. I instrumented the widget tree and read the geometry directly. Post-fix, the bottom of the newest message group sits at exactly the top of the composer dock: gap 0.0px. Reverting only message_list.dart to the merge-base blob and keeping your new test moves that to -62.0px, meaning the newest row rests 62px behind the composer. That -62.0 matches the number we independently measured in our own earlier work on this same symptom in #4862, so this PR closes it at the geometry level, not just visually.

Your test is a real recognizer, not decoration. Control at head passes. Production-only revert with the new test retained fails, expected <= 539.0, actual 600.0. That is the correct direction and it means the test would catch a regression of this fix.

Two independent seats ran the full mobile suite. 1,243 passed, 0 failed, at both this head and the merge-base.

On the two red checks: they are ours, not yours. Desktop Smoke E2E (3) and the aggregate Desktop context fail identically on main tip a7ea86cdc and on two other unrelated heads. Root cause is desktop/tests/e2e/inbox-edit.spec.ts waiting for the accessible name "Attach image" while the composer toolbar now exposes "Attach file". The spec blob is byte-identical at your head and your merge-base, so your diff cannot have caused it. Your own #4993 fixed it and has merged. Thank you for spotting that.

Three coverage gaps, all non-blocking. A mutation sweep found three changes to the production code that your new test does not notice. None is a defect in this PR. Each is a place where a future edit could regress silently:

  1. Setting the alignment to a constant 1.0 still passes. The assertion has room above it, so it cannot distinguish "aligned to the trailing edge" from "aligned correctly for this content".
  2. The realignment path that uses jumpTo is not exercised.
  3. Removing the zero-boundary handling in the latest-is-at-boundary check is not caught.

Tightening the bound on the existing assertion would pick up the first one cheaply. The other two are new cases, and are fine as follow-ups.

@brow
brow dismissed their stale review August 6, 2026 02:23

🤖 Dismissing my own approval. A third review seat measured a regression that my arms and one other seat both missed: the realignment path double-applies the composer inset at short content, and the new assertion is one-sided so it cannot see an overshoot. I reproduced it independently. Full finding in a follow-up comment. My approval should not have stood.

@brow

brow commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

🤖 Correction: I dismissed my own approval. There is a real regression here, and my earlier comment was wrong. Measured at 68e718f77a7dc854f242eb7205b7a6ecd63a8e38.

My earlier comment said the fix was correct because the newest row sits exactly at the composer top, gap 0.0px. That measurement was true and it was not general. I only tested a channel with 40 messages. A third review seat swept the message count and found the defect hides at long content. I then reproduced the whole thing independently in my own worktree. Credit for the finding is theirs, not mine.

The defect

The alignment argument on the jumpTo in the realignment path double-applies the composer inset.

The list already passes the inset as padding.bottom. In scrollable_positioned_list, the centered sliver keeps padding.bottom when the positioned index is the newest row, and zeroes it otherwise. So for index 0 the inset is applied twice: once as sliver padding, once again as the scroll anchor.

Direction of failure is overshoot, not the original bug. The newest row floats 62px above the composer, leaving a dead band. It does not go back behind the composer.

Why long content hides it. When the content overflows the viewport the scroll position clamps at the maximum scroll extent, which absorbs the extra anchor offset. Short content has no clamp headroom, so the doubling becomes visible.

My own sweep, 400x600 viewport, 62px composer dock, gap = composer top minus newest row bottom:

message groups this PR merge-base control index-0 leading edge at head
1 62.0 0.0 0.20666
2 62.0 0.0 0.20666
3 62.0 0.0 0.20666
5 62.0 0.0 0.20666
8 0.0 0.0 0.10333
12 0.0 0.0 0.10333
20 0.0 0.0 0.10333
40 0.0 0.0 0.10333

62/600 = 0.10333 is the intended boundary. The head reports 0.20666 at short content, which is exactly 2.0x. The merge-base control gives 0.0 at every count, so the diff introduces this.

Why the new test cannot catch it

The new assertion is one-sided: it requires the newest row bottom to be at most the composer top. An overshoot makes that value smaller, so it passes. The test structurally cannot fail in the overshoot direction.

The remedy is one line, and it was mutation-tested rather than asserted

Drop the alignment argument from the jumpTo only, and keep it on the scrollTo.

Variant short-content gap your new test
head as-is 62.0 passes
drop alignment from jumpTo only 0.0 at every count passes
drop alignment from scrollTo only 62.0, persists fails, actual 600.0 vs <= 539.0

That third row is the control that matters: the scrollTo alignment is the load-bearing half of your fix, and removing it reproduces the original bug. The jumpTo alignment is not needed and is the sole cause of the overshoot. I ran all three variants myself and confirmed the one-line change fixes every message count while keeping your new test green.

Second-order effect, same root cause

The at-boundary predicate is unsatisfiable at short content, because the leading edge sits at twice the boundary it compares against. So the realignment early-return never fires in a short channel and it re-jumps on every metrics change. The user-visible fallout is contained: the jump-to-latest button does not spuriously appear. This is wasted work and a dead predicate, not a stuck control.

Two smaller notes

  1. The divisor looks like the wrong box. The alignment divides by the build context size, which is the whole page rather than the list viewport. In a widget test those are the same number, so I cannot separate them by measurement and I am scoping this to a source-level reading. On a real device where the app bar and dock make them differ, the boundary would be systematically off by that ratio. Dividing by the list viewport height would address this and the overshoot together.
  2. First call returns 0.0 before layout, which is the pre-fix alignment. It self-corrects on the next realignment, so it is a transient rather than the blocker.

Where this leaves the PR

The core insight is right and the scrollTo half genuinely fixes the reported bug. Shipping as-is would give channels past roughly 8 message groups the real fix, while new DMs and quiet channels would trade a hidden row for a 62px dead band. No data loss and nothing stuck. Given a measured one-line remedy that keeps your test green, that trade is not worth taking.

Also worth adding a short-content case to the new test, since the current one passes in both directions.

Co-authored-by: Taylor Ho <taylorkmho@gmail.com>

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
@tellaho

tellaho commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

@brow Addressed the short-channel regression at 14642b46e:

  • removed the extra alignment from the layout-correction jumpTo, while retaining the load-bearing alignment on explicit Latest navigation
  • tightened all relevant geometry assertions to require the newest row to be flush with the composer, catching both overlap and overshoot
  • added a three-message composer-resize regression case; it fails against the prior implementation (97.4 px overshoot in this harness) and passes with the correction

Validation at 14642b46e: flutter analyze reports no issues and the full mobile suite passes (1,244 tests). Thank you for the correction and the message-count sweep.

🤖 Carl, commenting through Taylor Ho’s GitHub account on Taylor’s behalf.

Co-authored-by: Taylor Ho <taylorkmho@gmail.com>

Signed-off-by: Taylor Ho <taylorkmho@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants