Repository navigation
fix(mobile): align Android conversation headers and reactions - #8082
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — verdict: REQUEST CHANGES
Reviewed: 03f24a69bf34adf25bc7f85aa00ab74c9e80c759..9f31a8c9dc88d3fa280be14f732047f20625eae2 (exact live head, clean worktree)
Risk: medium — user-visible Android navigation and reaction attribution behavior
Blocking finding
Android discards the reaction the user long-pressed. The pill passes its emoji as initialEmoji (mobile/lib/features/channels/reaction_row.dart:143-148), and both native iOS and the non-Android fallback consume that selection. The new Android branch constructs _AndroidReactionDetailSheet without it (mobile/lib/features/channels/reaction_row.dart:359-372), while that sheet creates DefaultTabController without initialIndex (mobile/lib/features/channels/reaction_row.dart:421-423). Long-pressing 👀 therefore always opens All, not the 👀 attribution tab. The new test currently codifies this regression by expecting both reaction rows immediately after holding Eyes (mobile/test/features/channels/reaction_row_test.dart:354-383).
Consequence: a user asking “who reacted with this emoji?” is shown an unfiltered list; repeated names or larger sets make the result materially misleading. It also diverges from the unchanged iOS path and the prior fallback contract.
Author action: pass initialEmoji into _AndroidReactionDetailSheet; derive a safe initial tab index (indexWhere + 1, falling back to 0); initialize DefaultTabController with it. Rewrite the regression to first prove a held non-first emoji opens only that emoji’s page/selected tab, then explicitly verify All, tab selection, and swipe synchronization.
Verification owner: author for the causal widget regression; reviewer/tooling for exact-head Android interaction evidence after the fix.
Contracts traced
Header identity/member ownership and live presence/member updates; symmetric center-title constraints and large-text truncation; frosted header behavior across timeline/composer notifications; typing shimmer and reduced motion; Android tab/page synchronization; native iOS-first reaction dispatch. No second author-actionable defect found in those surfaces.
Validation
git diff --check 03f24a69bf34adf25bc7f85aa00ab74c9e80c759...HEAD— PASS at exact head.dart analyze lib/features/channels/channel_detail_page.dart lib/features/channels/reaction_row.dart lib/shared/widgets/frosted_app_bar.dart lib/shared/widgets/frosted_scaffold.dart— PASS, no issues.- GitHub exact-head
Clients / Mobile,Mobile, Mobile Swift domain, DCO, Semgrep, and security-review checks — PASS. - Focused local Flutter tests — not executed:
objective_c-9.3.0native-asset setup failed while resolving the Apple SDK (Bad state: No element/ local Xcode tooling). This is a reviewer-host confidence gap, not a PR-caused gate failure.
Manual/native evidence: author reports Android debug install/launch on Pixel 10; no independent exact-head Android recording or accessibility-tree inspection was available.
Residual risk: Android focus/selected-tab announcements, swipe selection, and large reactor-set behavior remain independently unwitnessed. Those are reviewer/tooling-owned confidence gaps; the selected-reaction regression above is established directly from the production path and must be fixed.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 03f24a69bf34adf25bc7f85aa00ab74c9e80c759..9f31a8c9dc88d3fa280be14f732047f20625eae2 (exact live head; systems/integration and product/UI/adversarial lanes reconciled).
Risk: medium — this changes Android conversation-header behavior and the reaction-attribution navigation contract.
Author-actionable defect
P2 — Android discards the specific reaction the user long-pressed and always opens the All tab. ReactionRow passes the held emoji as initialEmoji (mobile/lib/features/channels/reaction_row.dart:143-148,343-354). Native iOS and the non-Android fallback consume it, but the Android branch constructs _AndroidReactionDetailSheet without it (:359-372), and DefaultTabController has no initialIndex (:390-423). Holding 👀 therefore opens an unfiltered attribution list rather than “who reacted with 👀.” The new test currently locks in the regression by expecting both 👀 and 🔥 rows after holding 👀 (mobile/test/features/channels/reaction_row_test.dart:354-383).
- Author action: pass
initialEmojiinto_AndroidReactionDetailSheet, derive a safe emoji-tab index (indexWhere + 1, fallback All), initialize the controller there, and rewrite the regression to first prove a held non-first emoji opens only its selected tab/page before checking All, tap, and swipe synchronization. - Verification owner: author for the causal widget regression; reviewer for exact-head Mobile CI and Android interaction spot-check.
Traced behavior
Both lanes independently confirmed this defect. Header member/presence state remains reactive; narrow/long titles are centered within symmetric reservations, constrained, tappable, and covered at narrow width/2× text; group and direct-message subtitles remain truthful; typing shimmer/reduced-motion handling is intact; frosted behavior is scoped after the native-iOS branch; tab/page synchronization otherwise uses one controller; native iOS reaction presentation remains first and unchanged.
Validation and residual risk
- Exact-head source audits and
git diff --checkpassed; one lane’sflutter analyzepassed with a clean tree. - Exact-head Mobile, Mobile Swift, security review, Semgrep, zizmor, and DCO gates are green. The author’s local repository-wide
just cidisk exhaustion is not an author defect for this mobile-only diff. - Focused local Flutter tests did not start because the reviewer host’s native-assets SDK lookup hits the unaccepted Xcode license. Android screenshot/recording and AX evidence for selected-tab announcement, swipe selection, empty attribution, and large reactor sets remain confidence gaps. Author action: none solely for those gaps. Verification owner: reviewer/tooling/native validation.
Cleanup: no PR-specific *-carl checkout or retained build/scratch artifact remains.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
…nversation-headers Signed-off-by: kenny lopez <klopez4212@gmail.com> # Conflicts: # mobile/lib/features/channels/reaction_row.dart
|
🤖 Addressed Jude’s selected-reaction finding in a99e7c3 (current head 5c90b44 after merging main). Android now receives All 19 focused reaction tests passed, followed by mobile analysis, formatting, file-size checks, and the full mobile test suite on the merged head. The push succeeded. Ready for re-review. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 75a4efd0cabed81c1583cb3868b232b4bde50c81..5c90b44bb072e7e2f48d8813e24faf718d528177 (exact live head; systems/integration and product/UI/adversarial lanes reconciled).
Risk: medium — Android reaction-attribution state plus conversation header/platform behavior.
Findings: no unresolved author-actionable defect. The prior blocker is fixed: Android now passes initialEmoji into _AndroidReactionDetailSheet; indexWhere + 1 safely maps matches to emoji tabs and a miss to All (mobile/lib/features/channels/reaction_row.dart:360-426). The causal regression holds a non-first reaction, proves its tab/index and exclusive rows first, then verifies All, tab taps, and swipe synchronization (mobile/test/features/channels/reaction_row_test.dart:353-420). Both independent lanes confirmed the fix and found no regression in the previously traced header, frost, typing, or iOS-first dispatch paths.
Author action: none.
Verification owner: CI owns the still-running exact-head Mobile and Mobile Swift gates; reviewer/tooling owns optional Android device/AX observation.
Validation: exact clean head; git diff --check passed; independent exact-head just mobile-check passed (632 files unchanged; analyzer clean); DCO, Semgrep, zizmor, and Codex security review passed. Focused/full local Flutter execution did not reach tests because this reviewer host’s unaccepted Xcode license breaks the objective_c native-assets SDK lookup. That is a confidence gap, not author rework.
Residual risk: exact-head Clients / Mobile and Mobile Swift Domain / Mobile Swift were still running at final inspection. No independent Android recording/AX inspection was performed. Those named gates and reviewer/tooling own the remaining verification.
Cleanup: no PR-specific *-carl checkout or retained build/scratch artifact remains.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 75a4efd0cabed81c1583cb3868b232b4bde50c81..5c90b44bb072e7e2f48d8813e24faf718d528177 (exact live head; systems/integration and product/UI/adversarial lanes reconciled).
The prior Android reaction-selection blocker is fixed. Android now carries initialEmoji into _AndroidReactionDetailSheet and initializes the tab controller from indexWhere + 1, mapping a held emoji to its specific tab and a nonmatch safely to All (mobile/lib/features/channels/reaction_row.dart:344-426). The causal regression holds the non-first Eyes reaction, proves only Eyes is initially visible at controller index 2, then verifies All selection, Fire tap, and swipe synchronization through both controller state and visible rows (mobile/test/features/channels/reaction_row_test.dart:353-420). Both independent lanes found no remaining author-actionable defect, and the fix-only commit does not alter the previously cleared header, frost, typing, or native-iOS-first paths.
Validation at exact head: full mobile-check passed in both lanes (632 files unchanged by formatting; Flutter analysis clean); git diff --check passed; DCO, Semgrep, zizmor, and Codex security review passed. The live PR head remained pinned throughout review.
Confidence gaps — no author action: local mobile-test could not start because this review host's unaccepted Xcode license prevents SDK discovery in the objective_c native-asset hook. Independent Android device/AX observation was unavailable. At final review submission, exact-head Clients / Mobile and Mobile Swift Domain / Mobile Swift remained in progress without a reported failure; CI owns those terminal gates.
Author action: none. Verification owner: CI for the two running exact-head jobs; reviewer/tooling for any later native Android/AX spot-check.
Cleanup: both product/UI and Carl-owned review artifacts are removed; no *-carl checkout is retained. The systems lane checkout is being removed after this verdict.
Summary
Align Android channel and DM headers with iOS: centered, tappable titles with presence or member counts and no avatars. Keep conversation navigation frosted through keyboard and scroll changes, and run the typing shimmer left to right.
Add an Android reaction details sheet with All/emoji tabs, swipe navigation, and compact rows aligned to the tab gutters. Native iOS reaction presentation is unchanged.
Related issue
None found.
Testing
just cicould not complete: disk space exhausted while fetching an unrelated Rust dependency.