Fix mobile jump-to-latest flicker - #6807
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: COMMENT
Reviewed 113a33b7e49b7173ee1767c49ef2f49c63803034..7df8cf9f238d95731dc40065bea03f7367680fb8 at exact head 7df8cf9f238d95731dc40065bea03f7367680fb8 as :bot: Jude’s code review agent.
Risk: medium — user-visible timeline state plus an iOS native platform-view lifecycle. No relay, persistence, identity, schema, or release contract changes are in the seven-file PR diff.
Findings: no author-actionable code defect found.
The changed contracts trace coherently:
- Channel visibility preserves follow-tail intent through stale layout frames, while an actual user detach still exposes Latest (
mobile/lib/features/channels/channel_detail_page/message_list.dart:440-449; detach/resume paths at742-756). - Thread effective-end detection trusts
extentAfter <= 0.5only when the lazy tail has laid out (mobile/lib/features/channels/thread_detail_helpers.dart:8-17; consumer atmobile/lib/features/channels/thread_detail_page.dart:314-341). Tail corrections remain generation/drag/opt-out guarded (thread_detail_page.dart:383-418), and composer, explicit-Latest, local-reply, and user-scroll paths maintain the intended ownership transitions (435-515,647-688,868-899,982-992). - iOS skips
AnimatedSwitcher, synchronously removing a hidden platform view (mobile/lib/features/channels/jump_to_latest_switcher.dart:29-44), and an inactive route removes theUiKitViewsubtree (mobile/lib/features/channels/jump_to_latest_button.dart:36,102-125). The method handler is detached when its channel changes/unmounts (jump_to_latest_button.dart:38-45). Accessibility remains a named button (94-99), and non-iOS reduced-motion behavior is unchanged.
Validation at matching clean HEAD:
. ./bin/activate-hermit && just mobile-install && just mobile-check && just mobile-test— PASS; format/analyze clean and 1,864 tests passed. Pre/post HEAD was7df8cf9f...; finalgit status --shortempty.- CI
Mobilejob97921114982— SUCCESS at this head. - Mutation: reducing
shouldHideChannelJumpToLatesttoisAtLatestmadeflutter test test/features/channels/jump_to_latest_button_test.dart --plain-name 'keeps channel Latest hidden while followed-tail geometry catches up'fail as expected; restored clean. - Mutation: allowing exact extent before tail layout made
flutter test test/features/channels/channel_detail_page_test.dart --plain-name 'thread tail ignores oscillating item positions at exact extent'fail as expected; restored clean. - Independently reproduced lifecycle mutation evidence shows deleting inactive-route suppression fails the targeted route/
UiKitViewtest; restoring iOSAnimatedSwitcherfails the synchronous-unmount test. Trees were restored clean.
Confidence gaps (non-blocking): the higher-level channel composer/IME journey remains green when the production visibility fence is replaced with false, and the expanding-lazy-extent journey remains green when effective-end fallback is removed. The direct helper/lifecycle tests are causal, but those two widget journeys do not deterministically recreate the reported stale geometry. I also did not independently inspect an exact-head native iOS recording/receipt, keyboard frames, AX output, or Liquid Glass compositing; the PR’s physical-device build/install claim is therefore secondhand rather than reviewer-observed.
Author action: none required. Recommended follow-up: add deterministic stale-geometry seams so the channel and thread journeys fail when their production fences are removed, and retain an exact-head native iOS recording/receipt for this platform-view regression.
Verification owner: reviewer/tooling for native iOS observation and causal integration-level flicker reproduction.
Residual risk: real iOS keyboard timing and platform-view compositing can diverge from Flutter widget tests. Any head movement expires this review.
Chessing234
left a comment
There was a problem hiding this comment.
the routeIsCurrent guard is the right fix for the bleed-through — unmounting the native platform view when the route isn't current is exactly what stops the channel's glass arrow drawing over an opened thread.
three things.
the new predicate can strand a user away from the tail.
bool shouldHideChannelJumpToLatest({isAtLatest, followsLatest, userHasDetached})
=> isAtLatest || (followsLatest && !userHasDetached);the old condition was !latestIsAtBoundary(). the new one also hides the button whenever followsLatest is set and userHasDetached isn't — which is correct for the stale-measurement frames it targets, but it means any path that leaves followsLatest == true while the user is genuinely scrolled up hides the only affordance for getting back. a flicker is a cosmetic bug; a missing Latest button in a long backlog is a functional one, and it fails silently. what clears followsLatest, and is hasUserScrolled guaranteed to be set by every detach path including a fling, a programmatic jumpTo from a deep link, and a keyboard dismissal? if the answer is "not all of them", a time bound on the intent (ignore followsLatest if the last tail-follow was more than a frame or two ago) would keep the fix without the failure mode.
iOS loses the transition entirely, not just in the flicker case. the new branch returns a bare KeyedSubtree before AnimatedSwitcher on iOS, so every appearance and disappearance of Latest on that platform is now instant, in threads as well as channels. that's a much broader change than the bug described, and reduceMotion becomes dead code on the platform where it was most likely to matter. if AnimatedSwitcher is what was double-mounting the native view, routeIsCurrent already prevents that — worth checking whether the switcher can stay now that the button unmounts itself.
is ModalRoute.isCurrentOf reactive here? routeIsCurrent is read once at the top of build and captured by the ValueListenableBuilder closure, so it only re-evaluates when the outer widget rebuilds. that's fine if isCurrentOf registers a dependency on the route's status (so pushing a thread rebuilds this widget), and broken if it doesn't — the button would keep its last-known value and the bleed-through would come back intermittently. worth a comment stating which, since the whole fix rests on it, and a widget test that pushes a route and asserts the native surface is gone would pin it.
the just mobile-check + 1,864 flutter tests + signed device install is a good verification story; the gap is that none of those cover the three-state predicate. shouldHideChannelJumpToLatest is now a pure function in its own file — a table test over the eight input combinations is nearly free and would document the intent better than the doc comment does.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: the thread’s composer-follow visibility fence can permanently suppress the recovery control after a bounded tail correction fails.
followThreadTailFromComposer() sets hidesLatestForComposerTailFollow to true, but this state is cleared only when a user scroll starts. If the lazy tail never lays out during the eight-frame correction window, finishThreadTailCorrection() leaves isAtThreadTail false while the visibility predicate still hides Latest. Dismissing the keyboard does not clear the fence, so the user has no Latest affordance until they first drag the list.
Please scope this suppression to the active composer/correction window and clear it when correction completes or focus is lost. Add a regression test where the composer-triggered correction exhausts while the tail is not laid out, then focus is dismissed, and verify Latest becomes available.
Mobile CI is green at this exact head. The remaining iOS platform-view and channel follow-tail changes were source-consistent; real-device keyboard timing and Liquid Glass compositing remain non-blocking residual risk.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 Addressed Wes’s composer-tail recovery blocker in d1dd80c: the composer correction fence is now separate from ordinary landing state and clears when bounded correction completes or focus leaves. Added a deterministic regression that exhausts the correction without laying out the tail, dismisses focus, and verifies Latest returns. Local mobile checks and all 1,865 mobile tests pass. Wes, please re-review. |
🔐 Codex Security Review
|
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear at exact head d1dd80c8d8ef8fea0447cf80321de01dc90ecd85.
The prior blocker is resolved. Composer-tail suppression is now separate from initial-tail settling and is cleared when correction completes or aborts, focus leaves, or the user starts scrolling. Tail detection also no longer trusts an exact extentAfter while the actual tail item remains unlaid. The deterministic regression reproduces bounded correction exhaustion with the lazy tail absent, dismisses focus, and verifies Latest returns.
I traced the visibility contract across the reversed channel list and top-anchored thread list, ordinary and deep-linked entry, hydration/error fallback, composer and IME geometry updates, user detachment, local and remote arrivals, correction generations, route transitions, and iOS native-view unmounting. No actionable defects found. Mobile CI is green at this exact head. Real-device IME and Liquid Glass timing remain non-blocking residual risk beyond this read-only source review.
…agent-edit * origin/main: Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) refactor(relay): NIP-98 admin auth with Operator/Moderator roles and NIP-11 discovery (#3777) refactor(db): split channel membership store (#6782) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…rding-v3 * origin/main: Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) refactor(relay): NIP-98 admin auth with Operator/Moderator roles and NIP-11 discovery (#3777) refactor(db): split channel membership store (#6782) feat(auth): add NIP-FI canonical assertion verifier and contracts (#6776) Signed-off-by: Clay Delk <clay.delk@gmail.com>
…enericize * origin/main: feat(desktop): add team sharing to community catalog (#3995) Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) refactor(relay): NIP-98 admin auth with Operator/Moderator roles and NIP-11 discovery (#3777) refactor(db): split channel membership store (#6782) feat(auth): add NIP-FI canonical assertion verifier and contracts (#6776) fix(desktop): resolve exact typed mentions on space (#6862) perf(desktop): restore project context during startup (#6939) fix(desktop): lift right auxiliary pane above shared header backdrop (#6966) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…-history * origin/main: Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…c-agent-commit-identity * origin/main: feat(desktop): add team sharing to community catalog (#3995) Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) refactor(relay): NIP-98 admin auth with Operator/Moderator roles and NIP-11 discovery (#3777) refactor(db): split channel membership store (#6782) feat(auth): add NIP-FI canonical assertion verifier and contracts (#6776) fix(desktop): resolve exact typed mentions on space (#6862) perf(desktop): restore project context during startup (#6939) fix(desktop): lift right auxiliary pane above shared header backdrop (#6966) fix(ci): bump Codex CLI to 0.150.1 to unhang security review jobs (#6962) feat(desktop): implement 30178 team catalog backend (#5112) feat(model-capabilities): humanize Databricks UC model families (#6955) feat(agent): discover Databricks Unity Catalog models (#6918) test(db): use canonical channel roster fixtures (#6819) preserve channel description paragraph breaks (#6946) fix(cli): enrich template cardinality error with per-candidate presence and profile hints (#4825) Fix Codex security review authorization (#6913) fix(db): disable heartbeat vacuum truncation (#6898) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…ssage-builder * origin/main: refactor(db): extract domain stores from database runtime (#6987) feat(desktop): add team sharing to community catalog (#3995) Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) refactor(relay): NIP-98 admin auth with Operator/Moderator roles and NIP-11 discovery (#3777) refactor(db): split channel membership store (#6782) feat(auth): add NIP-FI canonical assertion verifier and contracts (#6776) Signed-off-by: Clay Delk <clay.delk@gmail.com>
* origin/main: refactor(db): extract domain stores from database runtime (#6987) feat(desktop): add team sharing to community catalog (#3995) Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…age-rw * origin/main: fix(desktop): resolve bundled sidecar on cheap path and bound login-shell spawns (#6904) perf(mobile): reduce cold startup and channel rendering delays (#6996) feat(mobile): push notifications MVP (#6269) refactor(db): extract domain stores from database runtime (#6987) feat(desktop): add team sharing to community catalog (#3995) Refresh mobile utility surfaces and theme picker (#6944) fix(desktop): complete project empty and context states (#6980) Fix mobile jump-to-latest flicker (#6807) refactor(relay): NIP-98 admin auth with Operator/Moderator roles and NIP-11 discovery (#3777) refactor(db): split channel membership store (#6782) feat(auth): add NIP-FI canonical assertion verifier and contracts (#6776) Signed-off-by: Joel Robotham <jrobotham@squareup.com>
Summary
Testing