Skip to content

Polish mobile message actions - #5873

Merged
wesbillman merged 16 commits into
mainfrom
kennylopez-message-actions-popover
Aug 18, 2026
Merged

Polish mobile message actions#5873
wesbillman merged 16 commits into
mainfrom
kennylopez-message-actions-popover

Conversation

@klopez4212

@klopez4212 klopez4212 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add a lifted-message long-press popover with the reaction tray and grouped actions.
  • Use a native action surface
  • Preserve taps and scrolling, exclude attached reactions from the lifted preview, and align the composition to the bottom safe area.
Screenshot_20260814-160732

Testing

  • bin/just mobile-check
  • flutter test test/features/channels/message_actions_test.dart
  • flutter test (1,364 tests)
  • Manual interaction review on iPhone and Pixel 10

@klopez4212
klopez4212 marked this pull request as ready for review August 14, 2026 15:19
@klopez4212
klopez4212 requested a review from a team as a code owner August 14, 2026 15:19

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f8658b30b

ℹ️ 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".

Comment thread mobile/ios/Runner/NativeMessageActionSurface.swift

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new full message-action popover ignores the software-keyboard inset. _MessageActionsPopover computes safeBottom from only MediaQuery.padding.bottom (mobile/lib/features/channels/message_actions/message_action_popover.dart:495-503), unlike the adjacent reaction popover, which subtracts MediaQuery.viewInsets.bottom (reaction_popover.dart:63-71). With the composer keyboard open, long-pressing a visible message therefore lays the preview/menu out behind the keyboard; the bottom actions can be obscured and untappable.

I reproduced this at exact head 1ae80934d616d1754320a68ec4ec20fbd8169957 with a temporary widget probe setting viewInsets.bottom = 300: the action surface bottom remained at y=592 on a 600-high viewport (expected <=300). Please include viewInsets in the safe bounds (and add the regression test), or explicitly dismiss the keyboard before presentation and test that contract.

@klopez4212
klopez4212 force-pushed the kennylopez-message-actions-popover branch from 1ae8093 to b99b297 Compare August 15, 2026 08:25
@klopez4212

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b99b297db4

ℹ️ 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".

Comment thread mobile/lib/features/channels/message_actions/message_action_popover.dart Outdated
@klopez4212

Copy link
Copy Markdown
Contributor Author

@jedwards27 Addressed the keyboard-inset issue in c25895d. The action surface stays above the keyboard; while open, the composer keyboard closes and is restored only after outside dismissal if it had been open. Action selection and initially closed state remain closed. Added regression coverage and validated the interaction on a physical iPhone. — Princess Donut

@klopez4212

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c25895d1fd

ℹ️ 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".

Comment thread mobile/lib/features/channels/message_actions/message_action_popover.dart Outdated
@klopez4212

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 661f08ae28

ℹ️ 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".

Comment thread mobile/lib/features/channels/message_long_press_region.dart Outdated
Comment thread mobile/ios/Runner/NativeMessageActionSurface.swift
Comment thread mobile/lib/features/channels/message_actions/message_action_popover.dart Outdated
@klopez4212

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: aab75fb9a9

ℹ️ 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".

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e87ee8a7ba

ℹ️ 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".

Comment thread mobile/ios/Runner/NativeMessageActionSurface.swift

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Team re-review — CHANGES REQUESTED

Reviewed exact source head e87ee8a7bafcabd0893b88c096c29946afd338d3 over base 69107dc3bfecbb80cc5f5b8bb6a7647ad054ce57. Native evidence used conflict-free test composite 11b3c97e5a3cdd03fc878f5023bf392a95f42e52, whose first two parents trace through harness ed30bef7c73d9c3964e0989f58c284ca19621a12 and this PR head.

[P1] Keep the preview image alive through the dialog's reverse transition

_showMessageActionsPopover disposes snapshot in the finally immediately after showGeneralDialog completes (mobile/lib/features/channels/message_actions/message_action_popover.dart:159-186), but the dialog subtree can continue rebuilding during route teardown/reverse transition. _LiftedMessagePreview then passes that disposed handle into RawImage at :726-760.

The real iPhone 17 Pro / iOS 26.5 simulator journey reproduced this on dismissal at accessibility XXXL: Bad state: Cannot clone a disposed image from _LiftedMessagePreview at the production call site, repeated across subsequent frames. After removing the test page's unrelated 0.143 px harness overflow, the unchanged production code still failed with four disposed-image exceptions. Cleanup passed.

Evidence: test-results/native-review/11b3c97e5a3c/ios_pairing/ios-20260815T113427-b635d3/{receipt.json,flutter.log,video.mp4,final.png} in the review worktree. The receipt is dirty only for the test-only journey page; the production tree matches the PR head.

Please transfer/dispose image ownership only after the dialog route and its transition can no longer build the preview, and add a causal lifecycle regression that opens and dismisses with nonzero transition duration on the real/native path.

[P2] Native Dynamic Type rows still use a fixed 48-point height

The current head makes the Flutter fallback Dynamic-Type-aware, but the selected iOS 16+ UIKit surface still computes preferred height as actions.count × 48, uses a one-line auto-shrinking label, and constrains only height >= 48 plus center-Y (mobile/ios/Runner/NativeMessageActionSurface.swift:36-71,122-165). Flutter independently budgets its platform-view box from scaled bodyLarge metrics (message_action_popover.dart:924-950). At accessibility categories, those two layout contracts diverge and can shrink, overlap, or clip action titles. The current native tests assert modal/theme constants but do not construct and lay out a row at an accessibility content-size category.

Please derive native row/preferred height from scaled typography (with vertical constraints) and add an XCTest that lays out the actual row/list at an accessibility category and proves labels remain contained and readable.

What cleared

The prior keyboard-inset blocker is fixed. A 300 px causal mutation made the focused regression fail at bottom 592 vs expected 292; restored code passed. Full Mobile package suite passed 1,437/1,437, focused message-actions suite passed 36/36, and all GitHub checks are green. Snapshot raster size is bounded and source cleanup paths otherwise look sound.

Verdict expires if HEAD moves.

@klopez4212

Copy link
Copy Markdown
Contributor Author

🤖 @jedwards27 Addressed both review items in f976562. The snapshot now stays alive until RawDialogRoute.completed, after the reverse transition and overlay removal, with a causal widget regression. Native rows now receive Flutter’s scaled minimum, derive height from UIKit Dynamic Type, wrap rather than shrink, and have an accessibility XXXL layout test against the actual row control. Validation: mobile checks, 36 focused message-action tests, all 1,438 Flutter tests, and all 30 RunnerTests.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f976562b01

ℹ️ 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".

Comment thread mobile/lib/features/channels/message_actions/message_action_popover.dart Outdated
Comment thread mobile/lib/features/channels/message_actions/message_action_popover.dart Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f7b30c2ed

ℹ️ 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".

Comment thread mobile/lib/features/channels/message_actions/message_action_popover.dart Outdated

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Team re-review — prior blockers resolved

Reviewed current head 8f7b30c2edad575967c825f56924b5d8afed9aec, including the delta from the last A Team reviewed head e87ee8a7bafcabd0893b88c096c29946afd338d3.

The delta addresses the blocking lifecycle/accessibility cases:

  • Short keyboard-constrained viewports now shed the reaction tray and preview before compressing the action surface, with a focused viewport regression.
  • Native rows use Dynamic Type metrics, multiline labels, vertical expansion, and an iOS layout test rather than shrinking text.
  • Repeated selection is idempotent, reverse-transition snapshot ownership lasts until RawDialogRoute.completed, and nested-route dismissal is covered.

Current CI is green, including Mobile. I could not rerun Flutter locally because flutter is not on this worktree shell's PATH; that is an explicit evidence limit, not a hidden pass. Source/delta review and CI found no new material defect. Native keyboard/Dynamic Type acceptance on a real iOS artifact remains residual risk.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

I independently reproduced one blocking navigation/state defect at exact head 8f7b30c2edad575967c825f56924b5d8afed9aec.

P1: make every popover dismissal path idempotent. _MessageActionsPopover gates menu-row selection with selectionStarted, but its full-screen backdrop calls Navigator.pop() directly (mobile/lib/features/channels/message_actions/message_action_popover.dart:657-661). Invoking that backdrop callback twice before the 220/320 ms reverse transition completes pops both the dialog and the underlying nested route. I added a temporary nested-route widget regression against this head and it failed because message-actions-underlying-page was gone after the two callbacks.

The quick-reaction and “more” callbacks have the same structural problem (mobile/lib/features/channels/message_actions/quick_reaction_row.dart:78-96): they bypass the menu's one-shot gate, pop directly, and quick reactions immediately perform their side effect. A repeated callback can therefore over-pop and submit the reaction twice.

Please route backdrop dismissal, native/fallback menu rows, quick reactions, and “more” through one dialog-owned idempotent selection/dismissal gate. Add nested-route regressions that invoke the backdrop and a quick reaction twice before reverse completion, asserting the underlying route remains and the reaction is submitted once.

The existing repeat regression only invokes a menu row twice (mobile/test/features/channels/message_actions_test.dart:861-890), so green CI does not exercise either failing path. The behavior-changing tail-detachment test also raises its drag from 24 to 48 pixels; that is consistent with the new gesture-arena touch-slop behavior, but it makes direct regressions for backdrop/reaction dismissal more important.

All applicable checks, including Mobile, are green, and git diff --check origin/main...HEAD passes. This causal path still fails independently.

@wesbillman
wesbillman dismissed their stale review August 17, 2026 17:02

Fixed in 2ac288f; focused regressions now cover repeated backdrop and quick-reaction callbacks.

wesbillman
wesbillman previously approved these changes Aug 17, 2026

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Team systems/integration re-review — APPROVE

Reviewed exact head 44ae66a8f0dd56d16379e390c3323261dfc17b6c over base 69107dc3bfecbb80cc5f5b8bb6a7647ad054ce57, including the full delta from previously reviewed 8f7b30c2edad575967c825f56924b5d8afed9aec.

The dialog-owned one-shot gate now covers backdrop dismissal, native/fallback action rows, quick reactions, and the “more” picker. The nested-route regressions directly invoke repeated callbacks before teardown and prove the underlying route survives and the reaction side effect occurs once (message_action_popover.dart:523-532,660-665,725-733; quick_reaction_row.dart:83-116; message_actions_test.dart:916-976). I mutation-tested the guard: removing it made both new regressions fail by popping the underlying route; restored exact head passes. The final file split is behavior-neutral.

Exact-head verification:

  • Focused Flutter message-actions suite: 40/40 passed.
  • Full Mobile package suite (just mobile-test): 1,442/1,442 passed.
  • Focused iOS XCTest on iPhone 17 Pro / iOS 26.5: 4/4 passed, including native Dynamic Type expansion, row selection, grouping/height, and accessibility sibling visibility.
  • git diff --check passed; worktree restored clean.
  • Live PR head matched the tested SHA immediately before this review. Mobile and every completed CI lane were green; unrelated Desktop Smoke E2E (3) remained in progress at the last query.

No material systems/integration blocker found. Verdict expires if HEAD moves.

klopez4212 and others added 5 commits August 18, 2026 19:03
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Kenny Lopez <klopez4212@gmail.com>
Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
Co-authored-by: Kenny Lopez <klopez4212@gmail.com>
Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
klopez4212 and others added 5 commits August 18, 2026 19:03
Co-authored-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>
Route popover backdrops, action rows, and quick reactions through a
single one-shot gate so repeated callbacks cannot pop an underlying
route or submit duplicate reactions.

Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
Keep the message action popover below the mobile file-size ratchet by moving
the reaction tray wrapper into its own part file without changing behavior.

Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d857b24e4b

ℹ️ 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".

Comment thread mobile/lib/features/channels/message_long_press_region.dart
Comment thread mobile/lib/features/channels/compose_bar/compose_bar_widget.dart Outdated
Signed-off-by: kenny lopez <klopez4212@gmail.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d09744f629

ℹ️ 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".

Comment thread mobile/lib/features/channels/compose_bar/compose_bar_widget.dart
Comment thread mobile/lib/features/channels/message_long_press_region.dart
Comment thread mobile/lib/features/channels/compose_bar/helpers.dart Outdated
klopez4212 and others added 2 commits August 18, 2026 20:05
Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Co-authored-by: Kenny Lopez <klopez4212@gmail.com>
Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
@klopez4212

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 99160be2e6

ℹ️ 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".

Comment thread mobile/lib/features/channels/message_actions.dart
Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Co-authored-by: Kenny Lopez <klopez4212@gmail.com>
Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
@klopez4212

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 019bff8414

ℹ️ 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".

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Princess Donut adversarial correctness/user-trust review — CLEAR at exact head 019bff841470b6a11138808322389648fccb2fec (merge base 417eea2230c1864e8c77f6440dbcfa109bfb63f6).

I reviewed the complete 21-file Mobile diff, with extra scrutiny on gesture arbitration, lifted-preview ownership, keyboard/focus restoration, action authorization/presentation, destructive confirmation, repeated dismissal/side effects, native UIKit accessibility sizing, and source visibility cleanup.

Material protections are present and causally covered:

  • Snapshot capture is bounded and retried safely; ownership survives RawDialogRoute.completed before disposal (message_long_press_region.dart:13-17,135-169; message_action_popover.dart:137-150,181-203).
  • The original source is hidden only after capture and restored after teardown, while failed capture falls back without hiding it (message_bubble.dart:80-97; message_action_popover.dart:137-155,189-203).
  • Backdrop, action rows, quick reactions, and the “more” picker all pass through one one-shot gate, preventing underlying-route pops and duplicate reaction side effects (message_action_popover.dart:523-532,660-665,725-733; quick_reaction_row.dart:83-116; regressions at message_actions_test.dart:885-976).
  • Composer focus is restored only for outside dismissal, and stale restorers become inert after replacement/unmount (message_action_popover.dart:200-204; compose_bar/helpers.dart:3-25; compose_bar_test.dart:629-743).
  • Delete remains confirmation-gated, and action availability is still derived from the existing canManageMessage decision (message_action_popover.dart:357-393).
  • Native rows derive height from Dynamic Type, wrap rather than shrink, remain scrollable under constrained height, and preserve VoiceOver access to sibling controls (NativeMessageActionSurface.swift:66-92,95-100,125-211,378-438).

Verification: local HEAD matched the live PR SHA immediately before review; worktree was clean; git diff --check 417eea223..HEAD passed. Current GitHub checks are green, including Mobile. Per review policy I did not duplicate CI-equivalent suites locally. No material security, correctness, accessibility, or user-trust defect found. Verdict expires if HEAD moves.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction to my immediately preceding review: CONDITIONAL — one P2 visual-state defect. Mordecai found, and Carl independently confirmed, a constrained-layout state I missed.

onPopoverPresented always hides the source before route push (message_action_popover.dart:153-186; callers at channel_detail_page/message_bubble.dart:80-96 and thread_detail_page/thread_message.dart:75-91), but layout can deliberately omit the lifted preview when height is constrained (message_action_popover.dart:555-607). The short-viewport regression proves that state is supported (message_actions_test.dart:698-735). Therefore the source and preview can both be absent behind the dimmed action surface, stripping the user of which message they are acting on. This is reachable in short landscape/accessibility layouts and while keyboard geometry is constrained.

Smallest safe remedy: keep the source visible whenever showPreview == false (or otherwise couple source hiding to actual preview visibility), with a regression asserting source and lifted preview are never simultaneously absent.

My earlier CLEAR verdict is withdrawn. No other material finding. Exact head remains 019bff841470b6a11138808322389648fccb2fec; verdict expires if HEAD moves.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Reviewed exact head 019bff841470b6a11138808322389648fccb2fec.

P2: Preserve the selected-message context when the constrained layout omits the lifted preview.

showMessageActions invokes onPopoverPresented before pushing the route (message_action_popover.dart:153-186), and the ordinary channel/thread callers use that callback to hide the source message (message_bubble.dart:80-96, thread_message.dart:75-91). But the constrained layout intentionally sets showPreview = false when the keyboard-safe height is too small (message_action_popover.dart:555-607). The existing 220-point viewport regression confirms that both the reaction tray and preview are absent in this state (message_actions_test.dart:698-735), while the harness's presentation callback has already hidden the source (message_actions_test.dart:243-250). The user therefore sees the action menu with neither the original message nor its lifted representation, losing the context for potentially destructive actions.

Please couple source visibility to whether the lifted preview is actually rendered, and add a regression asserting that the source and preview are never both absent.

Everything else reviewed cleanly, including the recent snapshot-lifetime, one-shot dismissal, Dynamic Type, bounded-raster, and composer-focus-restoration changes. git diff --check origin/main...origin/pr-5873 passed, the focused Flutter message-actions/composer run passed all 134 tests at this exact head, and current applicable GitHub checks are green.

Keep the source message visible whenever the action popover cannot render its lifted preview, including short keyboard-safe viewports.

Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>
@wesbillman
wesbillman dismissed their stale review August 18, 2026 20:52

Resolved by 084ebfb: source visibility now follows actual lifted-preview visibility, with constrained-viewport regression coverage.

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Follow-up at exact head 084ebfb489253a58f38ab08ec4e2f1e5bea9da8e: the constrained-layout blocker is resolved. The source message now hides only when the layout actually renders the lifted preview; the 220-point keyboard-safe viewport regression proves the source remains visible when that preview is omitted. Normal-layout coverage still proves the source hides behind the rendered preview.

Validation on this exact head:

  • Focused message-action and composer suites: 134 passed
  • Focused nested-rich-content long-press regression: passed
  • Pre-push hook: all gates passed, including the full mobile suite, file-size ratchet, desktop checks/tests/typecheck, Rust tests, and Tauri checks
  • git diff --check HEAD^..HEAD: passed
  • DCO: green

No remaining material blocker found. Submitted as COMMENTED rather than APPROVED because Wes did not explicitly request approval.

Format the popover implementation with the repository-pinned Dart formatter
so the Mobile CI formatting gate accepts the branch.

Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
Signed-off-by: Wes <wesbillman@users.noreply.github.com>

@jedwards27 jedwards27 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — APPROVE

Reviewed base 417eea2230c1864e8c77f6440dbcfa109bfb63f6 through exact head e4fbe5cf54be89f3d7064d51fc183684b1cbc5bb against the PR intent, VISION.md, and TESTING.md.

The latest delta fixes constrained layouts without losing message context: the source message is hidden only when the lifted preview actually renders, while short keyboard-safe viewports keep the source visible. I also traced the complete Mobile diff across gesture arbitration, bounded snapshot capture and disposal, keyboard/focus restoration, action authorization, one-shot dismissal and side effects, UIKit method-channel lifecycle, Dynamic Type, and accessibility sibling visibility. No material unresolved finding remains. The change stays within the mobile message-action surface; it does not alter relay, persistence, identity, or community contracts.

Exact-head validation:

  • just mobile-check: passed; 418 files format-clean and analyzer clean.
  • Focused Flutter message-action suite: 40/40 passed. Independent exact-head coverage also passed action + composer suites 134/134 and the full Mobile package 1,498/1,498.
  • iPhone 17 Pro / iOS 26.5 Simulator XCTest: all 30 RunnerTests passed, including all six native message-action tests for grouping/height, selection, Dynamic Type expansion, material/theme behavior, and accessibility sibling visibility.
  • git diff --check 417eea2230c1864e8c77f6440dbcfa109bfb63f6..e4fbe5cf54be89f3d7064d51fc183684b1cbc5bb: passed.
  • All applicable GitHub checks are complete and green; Web is expectedly skipped.

One local full-suite run hit the unrelated activity-provider test refreshes the inbox projection when addressed activity arrives; that exact test then passed 3/3 isolated reruns, while two independent clean exact-head full-package runs passed 1,498/1,498. I therefore treat the single failure as suite-level flake evidence, not a regression from this mobile-only diff.

Residual evidence limit: I did not independently perform a fresh end-to-end long-press journey or physical-device/VoiceOver traversal. Simulator widget/UIKit coverage is strong, but native animation feel and real VoiceOver traversal remain unverified here.

Verdict expires if HEAD moves.

@wesbillman
wesbillman merged commit 78267b0 into main Aug 18, 2026
25 checks passed
@wesbillman
wesbillman deleted the kennylopez-message-actions-popover branch August 18, 2026 22:00
jedwards27 pushed a commit to jedwards27/buzz that referenced this pull request Aug 18, 2026
* origin/main: (43 commits)
  perf(desktop): parallelize relay agent directory rebuild (block#6258)
  Refine the mobile emoji picker (block#5853)
  fix(desktop): exclude archived agents from nest, order regeneration (block#5905)
  Add font size and conversation density preferences (block#5644)
  fix(desktop): emit camelCase config-write payload fields (block#6062)
  fix(desktop): downscale large avatars for agent-share PNG body (block#6260)
  fix(desktop): preserve early relay auth challenges (block#3320)
  Polish mobile message actions (block#5873)
  Refine mobile pairing confirmation (block#6018)
  chore(scripts): add buzz-adopt-prod-agents.sh (block#6250)
  feat(managed-agents): close five Claude Code agent-config gaps (block#4557)
  chore(hooks): keep mobile analysis out of pre-commit (block#6236)
  fix(shared-ui): delay hover disclosures by default (block#5821)
  fix(desktop-chrome): preserve balanced layout when sidebar collapses (block#6000)
  Polish mobile timeline navigation (block#5874)
  chore(release): release Buzz Desktop version 0.5.17 (block#6234)
  fix(prompt): simplify pickup follow-through (block#6186)
  fix(mcp): scope todo usage (block#6216)
  fix(desktop): bound remote agent mention authorization (block#6224)
  fix: bump h2 for RUSTSEC-2026-0258 (block#6222)
  ...

Signed-off-by: Princess Donut <3cb959c7eb65d61f634e61df318e450f18f82fa0e01849e7010b82666ead0587@buzz.block.builderlab.xyz>

# Conflicts:
#	desktop/src/main.tsx
#	mobile/ios/Podfile.lock
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.

3 participants