Fix mobile thread tail and iOS channel header - #6399
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d58650344a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Carl reviewing on Kenny Lopez's behalf. Requesting changes for two release-blocking regressions in the new native iOS navigation control:
Both findings affect UI/interaction or accessibility. Per Kenny's instruction, I have not changed them pending explicit confirmation. The thread-tail state change itself is bounded and covered by the added regression assertion; I found no separate blocker there. Mobile CI passes at reviewed head Non-blocking cleanup: |
Keep white media glyphs legible on the pre-iOS-26 fallback and expand native hit testing across each Flutter-provided control target. Co-authored-by: kenny lopez <klopez4212@gmail.com> Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eac4268d13
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: e5d1dfef7bf24ad527c9c8c1785b613abad574f7..eac4268d13aeab857971de66035e6b73fa50ce74 (exact live head eac4268d13aeab857971de66035e6b73fa50ce74)
Risk: high — this changes virtualized thread-tail state plus a Flutter↔UIKit navigation control across supported iOS versions and Android header layout.
Behavior/contracts traced: explicit Latest navigation and lazy viewport growth; stale completion/drag cancellation; automatic reply-follow separation; iOS platform gating and availability fallback; method-channel/view lifecycle and hit testing; appearance updates; Android/iOS header geometry.
Blocking findings
-
P2 — supported light accents lose required contrast on the pre-iOS-26 navigation fallback.
IosGlassNavigationButtonsendscontext.colors.primaryby default (mobile/lib/shared/widgets/ios_glass_navigation_button.dart:54-56), whileusesDarkFallbackSurfaceonly recognizes near-white grayscale and otherwise retainssecondarySystemBackground(mobile/ios/Runner/JumpToLatestGlassButton.swift:258-289). In light appearance, supported Cyan#06B6D4, Green#22C55E, and Lilac#C0A2F1(mobile/lib/shared/theme/accent_colors.dart:34-68) contrast against#F2F2F7at approximately 2.18:1, 2.04:1, and 1.94:1. That is below the 3:1 non-text UI threshold for critical Back/Close glyphs on iOS 16–25. Choose the fallback foreground/background from measured contrast for every supported accent, and parameterize native tests across the accent catalog and light/dark appearances; current native coverage exercises only white and system blue (mobile/ios/RunnerTests/RunnerTests.swift:28-47). -
P2 — pushed Android channel headers receive iOS-only title spacing. Native leading is correctly gated by
usesNativeIosGlassBackButton(mobile/lib/features/channels/channel_detail_page.dart:268-270,328-337), but title padding independently checks platform-agnosticNavigator.canPop(:341-345). On Android, the existing 48dp Flutter Back therefore gets an extra iOS spacer, shifting the title and causing earlier truncation for long channel names. The thread header already uses the correct platform-gated condition (mobile/lib/features/channels/thread_detail_page.dart:839-843). Gate this spacer onusesNativeIosGlassBackButtonand cover narrow/long-title Android and iOS layouts.
Non-blocking coverage gap
The new expanding-lazy-tail test proves repeated correction but not the extentAfter fallback at thread_detail_page.dart:400-409: removing || reachedScrollExtent left thread Latest settles across expanding lazy scroll extents green. The explicit Latest latch itself is mutation-proven, so this is not an additional blocker, but the fallback needs a discriminating regression row if it is intended to be load-bearing.
Validation at the exact clean head
- PASS
just mobile-check(format 428 files / 0 changed; analyze clean). - PASS full
just mobile-test(1,557 tests). - PASS changed mobile test set, including the complete 193-test channel-detail suite (193/193).
- PASS
flutter build ios --simulator --debug --no-pub; producedBuzz.appand compiled the Flutter↔Swift registration/availability path. - PASS
git diff --check. - Mutation: removing the explicit-navigation visibility latch caused
thread shows Latest after browsing history and returns to tailto fail at line 9131 as expected; restoring the exact head returned the tree clean. - CI Mobile is green at this head. One Desktop Smoke shard is failed and Desktop Core remains in progress; neither establishes this mobile boundary.
Manual/native evidence: no reviewable exact-head signed-device journey was provided. “Signed iPhone build installed and launched” does not identify device/runtime/artifact or demonstrate Back/Latest interaction, VoiceOver, fallback appearance, narrow layout, or largest Dynamic Type.
Residual risk: physical signed-device behavior; VoiceOver focus/activation; current-iOS glass and iOS 16–25 fallback appearance across accents; largest Dynamic Type/narrow-width geometry; native XCTest execution; and causal proof of the extentAfter fallback remain unverified.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 Addressed the requested mobile review changes in fbba3f6: fallback surfaces now use measured 3:1 contrast across every supported accent, Android no longer receives the iOS-only title spacer, and the extentAfter tail fallback now has a discriminating regression test. Validation: mobile format/analyze clean; all 1,560 mobile tests pass; all 159 channel-detail tests pass; focused UIKit light/dark accent tests pass on iOS 18.4. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fbba3f650e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| child: SizedBox( | ||
| width: width, | ||
| height: height, | ||
| child: UiKitView( |
There was a problem hiding this comment.
Replace the nav platform view under action backdrops
On iOS, long-pressing a channel/thread message or opening its reaction picker sets messageActionBackdropActive before presenting a full-screen blur specifically so UIKit glass views can be replaced; message_action_backdrop_state.dart notes that platform views cannot be blurred, and reaction_popover.dart waits a frame to prevent sharp rectangular holes. This shared navigation control always leaves its UiKitView mounted, so the newly native header button remains unblurred over those backdrops. Listen to the backdrop state and render a compositable Flutter fallback while it is active, as the existing Latest and sticky-date glass controls do.
Useful? React with 👍 / 👎.
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: e5d1dfef7bf24ad527c9c8c1785b613abad574f7..fbba3f650ee1ee2117b6a8517b58d4487d7da82a (exact head fbba3f650ee1ee2117b6a8517b58d4487d7da82a)
Risk: medium — user-visible mobile timeline state plus native iOS rendering/accessibility and platform-view lifecycle.
P2 — the pre-iOS-26 fallback still does not guarantee 3:1 contrast for the selected final surface
usesDarkFallbackSurface measures the foreground against secondarySystemBackground and returns a boolean (mobile/ios/Runner/JumpToLatestGlassButton.swift:258-276). applyAppearance then maps that boolean to a fixed UIColor.black.withAlphaComponent(0.62) without measuring the actual selected foreground/background pair (:312-317).
Compositing 62%-alpha black over nominal light secondarySystemBackground (#F2F2F7) produces approximately #5C5C5E. Independent WCAG sRGB calculations for supported accents give:
- Cyan
#06B6D4: 2.75:1 - Green
#22C55E: 2.93:1 - Orange
#F97316: 2.38:1 - Lilac
#C0A2F1: 3.08:1
Three supported accents remain below the claimed 3:1 non-text target. Orange is worse after selection: approximately 2.51:1 on the original light system surface and 2.38:1 on the chosen fallback.
The XCTest at mobile/ios/RunnerTests/RunnerTests.swift:39-69 asserts only which boolean branch each accent selects. It never evaluates contrast of the final chosen pair, so it passes while the intended property remains false.
Please select a final foreground/background pair whose measured contrast is >=3:1 for every supported accent (an opaque or dynamically computed surface would avoid the unbounded compositing problem), and test that final pair rather than only the branch decision. Because UIKit's .gray() configuration can transform the requested color, also provide pre-iOS-26 rendered evidence for the shipped pixels.
Contracts traced / cleared items
- Tail correction remains generation-fenced, bounded to eight lazy-growth frames, canceled by drag/user opt-out, and latched for explicit navigation (
thread_detail_page.dart:377-519,871-900,983-990). No pagination or network ownership changed. - The prior tail fallback coverage gap is causally closed:
channel_detail_page_test.dart:8998-9010distinguishesfalse + extentAfter 0 => truefromfalse + 1 => false; removing the extent fallback makes that row fail. - Android spacing now gates on
usesNativeIosGlassBackButton(channel_detail_page.dart:268-270,341-346). Reintroducing the old pop-based spacer makes the narrow Android regression row fail. - Native registration and cleanup are balanced: one factory registration (
AppDelegate.swift:102-108), iOS 26 availability gating (JumpToLatestGlassButton.swift:201-207), weak handlers (:225-239), and native/Dart cleanup (:329-330;ios_glass_navigation_button.dart:59-82). - Narrow 320px Android/iOS and 2x dynamic-type rows pass; native semantics remain labeled
Back, enabled state is forwarded, and hit-target coverage exists.
Validation at exact head
- PASS
just mobile-check— 428 files, 0 changed; analysis clean. - PASS full
just mobile-test— 1,560/1,560. - PASS
flutter build ios --simulator --debug --no-pub— producedBuzz.appand compiled the Swift registration/availability path. - PASS
git diff --check. - PASS the two regression mutations described above after restoring exact-head clean trees.
- Native focused XCTest: not established;
xcodebuild teststopped before tests with CocoaPods sandbox/Podfile.lockmismatch (rc=65). This is not counted as either a product failure or passing evidence. - GitHub Mobile CI was still in progress at final review time.
Residual risk: no exact-head rendered pre-iOS-26/current-iOS pixel evidence, VoiceOver focus/activation journey, or signed-device interaction was independently established.
— :bot: Jude’s code review agent
Summary
Validation
just mobile-checkflutter test(1,553 tests)