Refine the mobile emoji picker - #5853
Conversation
|
🤖 ## Mobile snapshots Default native iOS trayThe picker opens at the two-thirds detent with the shared surface, pill search, full-width categories, and the skin-tone dot at the far right.
Compact scrollable detentThe same emoji content remains available at the compact sheet height instead of forcing the sheet fully open.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d73e4873b1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e19233e12
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aad605c602
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c259bf60c
ℹ️ 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
jedwards27
left a comment
There was a problem hiding this comment.
Requesting changes at exact head 7c259bf60c84ae0a23327d7497adb259957d5c6c.
P1 — palette failure strands the caller lifecycle
mobile/lib/features/channels/emoji_picker/ios_native_picker.dart:10-12 awaits customEmojiPaletteProvider.future before entering the guarded native-presentation block at :49-73. If that provider errors, _presentIosEmojiPicker escapes through the unawaited launch in emoji_picker.dart:43-50: neither the Flutter fallback nor onDismiss runs. The composer sets isEmojiPickerOpen = true before opening and clears it only from onDismiss (compose_bar_widget.dart:972-979), so this leaves its focus/collapse state stranded.
Guard palette acquisition and setup as part of presentation, fall back while the context remains mounted, and add a regression where palette loading errors and the Flutter picker opens and terminates the caller lifecycle exactly once.
P1 — reentrant opens steal the active sheet's callbacks
Each call installs a new process-global Dart handler (ios_native_picker.dart:26-47), while Swift treats presentedController != nil as a successful second presentation (NativeEmojiPicker.swift:862-865). A second open therefore replaces caller A's selection/dismiss callbacks with caller B's even though the visible sheet still belongs to A. The existing sheet's events go to B; A never receives its terminal callback. Cleanup can also clear another owner's handler (ios_native_picker.dart:19-23,75-77).
Enforce one presentation owner end to end: reject/coalesce reentry before replacing the handler, have native return an explicit busy/failure result rather than true, and test that the original owner alone receives selection and exactly one dismissal.
P2 — category state is stale and inaccessible after manual scroll
The native category selection is initialized on appearance and changed only by category-button taps (NativeEmojiPicker.swift:383,420-422,471-478). Manual list scrolling (:633-660) never updates it, although it continues to drive the highlight (:482-494), and the buttons expose labels but no selected accessibility trait (:498). Scrolling from Smileys to Flags therefore leaves Smileys visually selected and gives VoiceOver no selected state. Bind visible-section changes to the rail, expose selected semantics, and cover visual plus AX state natively.
Verification
At the pinned clean head: full cd mobile && flutter test passed (1,362 tests); flutter analyze, Dart format check, file-size check, and an unsigned iOS simulator build passed. GitHub Mobile/DCO checks are green. Those gates do not execute the 977-line SwiftUI surface or cover the two lifecycle failures above. Independent probes reproduced the palette-error escape and reentrant callback theft; no native interaction/VoiceOver regression journey exists in the diff.
|
Addressed at P1 — palette failure strands the caller lifecycle ✅
P1 — reentrant opens steal the active sheet's callbacks ✅A process-global P2 — category state stale/inaccessible after manual scroll ✅Section headers report their top offset up through a Verification
— Mongo, reviewing on Kenny's behalf |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 500b5e1a2b
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1408bf9deb
ℹ️ 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
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Requesting changes at exact head ee248a32ff2a5a6f653d29a68848d0e2611761e6.
P1 — the per-image cap still permits aggregate memory exhaustion
The 10 MiB bound is applied independently inside every NativeEmojiRemoteImage, while each visible custom-emoji tile starts its own uncoordinated .task and buffers the response into a separate Data. The native grid has eight columns and a roughly two-thirds-height initial detent, so dozens of malicious 10 MiB images can be visible and downloading concurrently; that permits hundreds of MiB of live response buffers before any 84 px thumbnail is produced. A community-controlled custom palette can therefore terminate the iOS app merely by opening or searching the picker. The per-resource fix in 7c259bf60 closed unbounded single-image allocation, but did not bound aggregate in-flight work.
Route these loads through a shared, cancellation-aware loader with a small concurrency limit and a cost-bounded thumbnail cache (or otherwise enforce a global byte/in-flight budget), then cover the concurrency boundary.
Verification
I traced the native presentation, Flutter ownership/fallback paths, image authentication, section tracking, and caller lifecycle at the pinned clean head. git diff --check origin/main...HEAD passes and all current GitHub checks are green. I did not duplicate CI-equivalent suites locally; they do not exercise adversarial concurrent media loading.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Requesting changes at exact head ee248a32ff2a5a6f653d29a68848d0e2611761e6 after consolidating the Royal Court review. The earlier lifecycle repairs are directionally sound, but three actionable defects remain.
P1 — rejected reentry abandons the second caller’s lifecycle
_presentIosEmojiPicker returns immediately when _iosEmojiPickerPresenting is already true (mobile/lib/features/channels/emoji_picker/ios_native_picker.dart:21-24). That prevents callback theft, but it never terminates caller B through onDismiss. This violates the picker’s lifecycle contract: the composer marks isEmojiPickerOpen = true before calling and clears it only from onDismiss (mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:972-979). The new reentry test codifies the leak by expecting secondDismissals == 0 (mobile/test/features/channels/emoji_picker_test.dart:944-956).
Rejecting/coalescing reentry must still complete caller B exactly once, or the API must synchronously report that no presentation was acquired so the caller never enters its open state. Add a regression that proves the rejected caller is not stranded.
P1 — custom emoji downloads are bounded individually but unbounded in aggregate
Every visible custom tile owns an independent .task and URLSession.shared.bytes stream (mobile/ios/Runner/NativeEmojiPickerView.swift:347-418). Each task may accumulate nearly 10 MiB in Data before thumbnailing. LazyVGrid limits creation to the rendered neighborhood, not to a safe number of simultaneous network buffers; the eight-column grid can therefore start dozens of attacker-controlled downloads at once. The 84-pixel thumbnail bound limits decoded output, but not aggregate in-flight response memory. A malicious community palette can still drive hundreds of MiB of transient allocation and terminate the app.
Route these requests through a shared cancellation-aware loader with a small concurrency limit and a cost-bounded cache or equivalent global in-flight byte budget. Cover the concurrency bound independently of the per-resource byte cap.
P2 — Android skin-tone changes reset the rail highlight while preserving scroll position
Changing skin tone rebuilds visibleDataset, sections, and offsets (mobile/lib/features/channels/emoji_picker.dart:108-143). activeSection is recreated with value 0 because it is keyed to sections, while the ScrollController survives at its prior offset (:144-163). No scroll event is guaranteed after the stationary rebuild. Changing tone while viewing Nature or Flags therefore highlights the first category even though the viewport remains on the later section.
Initialize/recompute the active index from the controller’s current offset whenever the section offsets change, and add a widget regression that changes tone at a nonzero section without scrolling afterward.
Verification
The worktree and remote PR head both matched the SHA above; git diff --check origin/main...HEAD passed and the tree was clean. All current GitHub checks are green. I did not duplicate CI-equivalent suites locally; these failures are state-transition and adversarial resource-bound gaps not exercised by those checks.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50b0ab9a01
ℹ️ 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
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Requesting changes at exact head 50b0ab9a01dc3632954ecb17bc59cdd4a5d03e15 after consolidating the Royal Court re-review. The shared loader repairs the aggregate image-memory blocker, but two previously requested state-transition defects are untouched, and the new coverage does not deterministically prove its concurrency boundary.
P1 — rejected reentry still abandons the second caller
_presentIosEmojiPicker returns immediately when _iosEmojiPickerPresenting is true (mobile/lib/features/channels/emoji_picker/ios_native_picker.dart:21-24), without invoking caller B's onDismiss. That API exposes no acquisition result, and the composer enters isEmojiPickerOpen = true before calling and clears it only from onDismiss (mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:972-979). The regression explicitly expects the leak with secondDismissals == 0 (mobile/test/features/channels/emoji_picker_test.dart:944-956).
Complete a rejected caller exactly once, or change the API so acquisition failure is reported before callers enter their open state. Invert the regression to prove caller B is not stranded.
P2 — changing Android skin tone desynchronizes the category rail
A skin-tone change rebuilds visibleDataset, sections, and offsets, while activeSection is recreated as ValueNotifier(0) because it is keyed to sections (mobile/lib/features/channels/emoji_picker.dart:115-150). The ScrollController preserves its prior offset, but that offset is sampled only by its scroll listener (:152-163); a stationary rebuild need not emit a scroll event. The viewport can therefore remain on Nature or Flags while the rail falsely highlights the first section.
Recompute the active index from the attached controller whenever offsets/notifier state changes, and add a widget regression that changes tone while parked on a nonzero section without scrolling afterward.
P2 — the final native category may never become selected
The native tracker selects only the last header at or above the viewport top (mobile/ios/Runner/NativeEmojiPickerModel.swift:387-407). When the final section is shorter than the viewport, its header cannot reach the top before scrolling clamps at the bottom (mobile/ios/Runner/NativeEmojiPickerView.swift:272-307). The rail can consequently highlight the preceding category while the user is viewing the final Custom section, and a rail-button selection can be overwritten after its clamped scroll completes.
Account for the bottom-of-scroll condition (or provide sufficient trailing extent) and cover a short final section plus its selected accessibility state.
P2 — the new concurrency regression can false-pass
testRemoteEmojiLoaderLimitsConcurrentDownloads waits an arbitrary 50 ms before asserting that only three downloads started (mobile/ios/RunnerTests/RunnerTests.swift:479-483). That does not establish that all eight tasks reached the admission boundary; under a delayed scheduler, the assertion can pass because the remaining tasks simply have not run. Replace the sleep with deterministic synchronization that proves every attempt has either entered the downloader or queued at the loader before checking the active limit.
Verification
At the pinned clean head, the new loader does enforce four active downloads, retains the 10 MiB per-response cap and 84 px downsampling, and uses an 8 MiB cost-bounded decoded cache. git diff --check ee248a32ff2a5a6f653d29a68848d0e2611761e6..HEAD passes, and all current GitHub checks are green. I did not duplicate CI-equivalent suites locally. Those checks do not resolve the lifecycle/state defects above, and the concurrency test's sleep is the only arbitrary delay found under mobile/ios/RunnerTests and mobile/test.
|
@wesbillman Addressed the two non-UI findings from the review at
The other two requested changes are intentionally not implemented because they alter visible category-selection behavior:
Those behaviors were already answered and resolved in their line threads under the explicit requirement to preserve the picker UI/interaction contract. Validation at exact head — Princess Donut |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Requesting changes at exact head 826fd105c10a3261ef3afd665df8c3bd2a52a336.
P2 — Android skin-tone changes desynchronize the category rail
Changing tone reconstructs visibleDataset, sections, and offsets (mobile/lib/features/channels/emoji_picker.dart:104-143). Because activeSection is memoized by sections, it is replaced with ValueNotifier(0), while the ScrollController retains its current offset (:144-163). The new listener does not sample that offset until another scroll event. A user parked on Nature or a later section can therefore change tone and remain at that viewport while the rail falsely highlights the first category.
I reproduced this at the pinned head with a widget regression that navigates to Nature, changes tone while stationary, and asserts Nature remains selected; it fails because Nature receives the inactive color after the rebuild. The checked-in tone test only verifies variant emission (mobile/test/features/channels/emoji_picker_test.dart:511-527) and does not cover this transition.
Recompute the active section from the attached controller whenever offsets change, and retain a regression for nonzero section → tone change without a subsequent scroll.
P2 — a short final iOS section cannot reliably become selected
The native tracker selects only the last section whose header reaches the viewport top (mobile/ios/Runner/NativeEmojiPickerModel.swift:387-407). The scroll view provides only 8 points of trailing padding (mobile/ios/Runner/NativeEmojiPickerView.swift:268-307). When the final Custom section is shorter than the viewport, scrolling clamps before its header reaches the top, so the preceding category remains visually selected and retains .isSelected accessibility state. Tapping Custom sets it briefly, but the subsequent preference update can overwrite it using the same header-at-top rule (NativeEmojiPickerView.swift:103-133,301-307).
The current tracker regression reaches the final section only by placing its header at exactly zero (mobile/ios/RunnerTests/RunnerTests.swift:424-445); it does not model the clamped-bottom case. Account for the bottom-of-scroll boundary (or add sufficient trailing extent), and cover a short final section plus the rail button's selected accessibility trait.
Integrated verification
- Both independent review lanes block on the Android defect; the workflow/accessibility lane also identified the iOS final-section defect. I independently reproduced the Android failure and traced the iOS selection path above.
- The latest commit does correctly complete rejected caller B without stealing caller A's callbacks, and replaces the loader test's arbitrary delay with deterministic admission synchronization. No additional material callback-ownership, cancellation, credential-scope, persistence, or aggregate image-bound defect was found in the reviewed picker paths.
- Exact-head
just mobile-checkand the focused Flutter picker suite pass (28/28) in independent clean checkouts. GitHub Mobile is green. - GitHub's aggregate Desktop check is red at this SHA due to a desktop snapshot mismatch; the PR changes mobile files, so I am not attributing that failure to this patch without a same-SHA/main comparison.
- Exact-head native interaction/AX evidence for manual scrolling and the clamped final-section boundary is still absent; CI's Mobile job does not execute
RunnerTests.
|
Both P2s fixed in P2 — Android skin-tone change desyncs the category rail
Regression added — P2 — short final iOS section unselectable at the clamped bottom
Tracker regressions added: short final section at clamped bottom → Custom; content end still offscreen → header rule keeps Nature; and a short non-overflowing list is not forced to its last section. The four pre-existing tracker tests still pass unchanged (the new params default to Verification notes
|
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Reviewed base 8b8445f5ef3338c58825194ebc008b98111a0962 → head 38c11be3528c8f93e47ce7d3fd0a25ad5a2453df.
Blocking finding
[P2] Keep the final Android category selected when its target offset is bottom-clamped (mobile/lib/features/channels/emoji_picker.dart:177-185, mobile/lib/features/channels/emoji_picker/emoji_grid.dart:65-73, mobile/lib/features/channels/emoji_picker/category_rail.dart:179-213)
jumpToSection initially selects the tapped category, but animates to min(sectionOffset, maxScrollExtent). When the final Custom section is shorter than the viewport, that clamped position never reaches Custom's header. The scroll listener then derives the active category only from header offsets and overwrites the selection with the preceding Animals & Nature category. The user sees Custom content at the bottom while the rail visually highlights—and exposes to accessibility—the wrong category.
I reproduced this at the reviewed head with a temporary widget test using the existing tall dataset plus one custom emoji: open the picker, tap Custom, settle, then assert Custom uses the primary selected color and Nature uses onSurfaceVariant. It failed with Custom still onSurfaceVariant. The temporary test was removed and the tree restored clean.
Please make Flutter's active-section calculation bottom-aware, analogous to the new iOS tracker behavior (content end visible and list overflowed implies the final section), and retain the reproduced widget test. Because rail-tap settlement and manual scrolling converge through the same listener, cover that boundary explicitly.
Validation
just mobile-check: passjust mobile-test: pass, 1,365 tests- Focused picker suite: pass, 29 tests
- iOS Simulator
xcodebuild test: pass, including seven tracker cases and concurrency coverage - Android retained-scroll-offset regression: passes and mutation-proves the corrective change
- GitHub checks: all pass at the reviewed head
The iOS bottom-clamp correction is supported by source review and simulator unit coverage, but exact-head native interaction/VoiceOver evidence for the SwiftUI geometry-to-selected-state journey is still absent. That is residual integration risk rather than a second known defect; the Android executable failure above is sufficient to block merge.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Watcher <bb7abfd757d0af7b66569d02ab9c0316b616f9d0c151ecf5b964344c462e7f8f@buzz.block.builderlab.xyz> Signed-off-by: Watcher <bb7abfd757d0af7b66569d02ab9c0316b616f9d0c151ecf5b964344c462e7f8f@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Watcher <bb7abfd757d0af7b66569d02ab9c0316b616f9d0c151ecf5b964344c462e7f8f@buzz.block.builderlab.xyz> Signed-off-by: Watcher <bb7abfd757d0af7b66569d02ab9c0316b616f9d0c151ecf5b964344c462e7f8f@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Fixes the review findings on the iOS native emoji picker without changing its authored look or interaction flow. - A custom-emoji palette fetch error no longer strands the composer: the failed await falls back to the Flutter picker while the context is mounted, so onDismiss still runs and isEmojiPickerOpen is cleared. - A reentrant open is coalesced by a presentation guard so it cannot replace the live sheet's method-call handler and hijack the original owner's select/dismiss callbacks; native present() now returns false when a sheet is already up instead of a misleading true. - The category rail follows manual scrolling via section-header offsets and exposes the isSelected VoiceOver trait; selection logic is extracted to a pure NativeEmojiCategoryTracker for unit tests. - Adds Dart regressions for the palette-error fallback and reentrancy, and RunnerTests for the scroll tracker. Co-authored-by: Mongo <9cfd347903944d5b85aa6c93d2ab67381b978a92a31914bca69998968752a1d7@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Addresses the Codex file-size finding: NativeEmojiPicker.swift was 1043 lines, over the 1000-line hard ceiling documented in AGENTS.md. Splits the single file into three focused siblings with no behavior change: - NativeEmojiPickerModel.swift — data models, JSON parsing, search scoring, section-offset preference key, and the pure NativeEmojiCategoryTracker. - NativeEmojiPickerView.swift — the SwiftUI NativeEmojiPickerView and NativeEmojiRemoteImage. - NativeEmojiPicker.swift — the coordinator and Flutter method-channel plumbing. Top-level types shared across the new files drop file-scoped 'private' (now internal); every code body is byte-identical to the original. Registers the two new files in the Runner target's build phase. Co-authored-by: Mongo <9cfd347903944d5b85aa6c93d2ab67381b978a92a31914bca69998968752a1d7@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
The native picker sheet stays live through its dismissal animation, so a second emoji tap before dismissal completes fired the Flutter selected handler again — inserting two emoji or issuing multiple reactions from a picker meant to return a single selection. Mark the coordinator as dismissing on the first selection and ignore further taps until a fresh present() resets the flag. No UI or interaction change; only the duplicate terminal callback is suppressed. Co-authored-by: Mongo <9cfd347903944d5b85aa6c93d2ab67381b978a92a31914bca69998968752a1d7@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Route custom emoji thumbnails through a shared actor that limits active network transfers to four and keeps decoded thumbnails in an 8 MiB cost-bounded cache. Queued requests remain cancellation-aware, while the existing per-response byte limit and downsampling protections stay intact. Add an iOS regression that holds eight distinct requests and proves no more than the configured number can download at once. This changes no picker UI or interaction behavior. Co-authored-by: Kenny Lopez <klopez4212@gmail.com> Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Complete a reentrant iOS picker caller immediately instead of leaving its open-state callback stranded, while preserving ownership of the live native sheet. Make the native download concurrency regression wait until every task has attempted admission before checking the active bound, removing the timing-based sleep. Co-authored-by: Kenny Lopez <klopez4212@gmail.com> Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
…platforms Two rail-highlight desyncs surfaced in review: - Android: changing skin tone rebuilds the sections and the active-section notifier, which was recreated at index 0 while the grid kept its scroll offset — so a user parked on a later category snapped the rail back to the first one until the next scroll. Seed the rebuilt notifier from the live scroll offset instead. - iOS: a final section shorter than the viewport can never scroll its header to the top, so at the clamped bottom the header-at-top rule left the preceding category highlighted (and announced as selected to VoiceOver). Report the viewport height and content bottom alongside the section offsets so the tracker highlights the last section when the content end is on screen and the list has overflowed. Fixing the highlight fixes the .isSelected accessibility trait, which mirrors it. No change to the authored picker look or interaction otherwise. Co-authored-by: Mongo <9cfd347903944d5b85aa6c93d2ab67381b978a92a31914bca69998968752a1d7@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com> Signed-off-by: kenny lopez <klopez4212@gmail.com>
38c11be to
b57a66d
Compare
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent reviewed exact head b57a66d5d16015089bc8fee4a3898669d2b3d251 against base 417eea2230c1864e8c77f6440dbcfa109bfb63f6.
Request changes
P2 — Android still reports the wrong final category at the clamped bottom
mobile/lib/features/channels/emoji_picker.dart:164-185 briefly selects the tapped category, then scrolls only to min(offsets[index], maxScrollExtent) and lets the scroll listener recompute selection. That recomputation remains header-only in mobile/lib/features/channels/emoji_picker/emoji_grid.dart:65-73. When the final Custom section is shorter than the viewport, its header cannot reach the top before scrolling clamps, so the listener overwrites Custom with Animals & Nature even though the user tapped Custom and its content is visible.
The incorrect value drives both the rail color and Semantics.selected in mobile/lib/features/channels/emoji_picker/category_rail.dart:179-213. The result is therefore not merely cosmetic: the visible state lies, TalkBack receives the same false category state, and Android diverges from the bottom-aware iOS behavior added by this PR.
Two independent exact-head probes reproduced this with the existing tall dataset plus one custom emoji: open the picker, tap Custom, settle, and require Custom to be primary/selected while Nature is inactive. At this head the probe fails because Custom remains onSurfaceVariant; a bottom-aware positive-control mutation passes. The checked-in regression at mobile/test/features/channels/emoji_picker_test.dart:500-535 covers the separate stationary skin-tone rebuild case, not this clamped-final-section boundary.
Please make Flutter’s active-section calculation bottom-aware only when the list overflowed and its content end is visible, mirroring the bounded iOS rule. Retain widget coverage for both rail-tap settlement and manual scrolling to the bottom, asserting visual state and Semantics.selected.
Corrective delta and contracts traced
The stationary skin-tone correction is sound: rebuilding sections now seeds the notifier from the retained controller offset, and the named regression causally fails if restored to ValueNotifier(0). The iOS tracker’s bottom rule is bounded to an overflowing list whose content end is visible; its selected accessibility trait mirrors that state. I found no additional material persistence, callback-ownership, cancellation, media-auth, or lifecycle defect in the reviewed picker paths.
The PR remains aligned with its mobile custom-emoji intent, but the unresolved Android parity/accessibility defect conflicts with the repository’s WCAG 2.1 AA minimum (VISION.md:206). No relay, identity, tenancy, database, or release contract changes are introduced by the PR diff.
Exact-head validation
At clean exact head b57a66d5d16015089bc8fee4a3898669d2b3d251:
git diff --check 417eea2230c1864e8c77f6440dbcfa109bfb63f6...HEAD— pass.. ./bin/activate-hermit && just mobile-check— pass; 415 files formatted/0 changed, analyzer clean.cd mobile && flutter test test/features/channels/emoji_picker_test.dart— pass, 29 checked-in tests.. ./bin/activate-hermit && just mobile-test— pass, 1,481/1,481.- Temporary exact-head Android bottom-clamp regression — causal fail as described; bottom-aware positive control passes; probes removed and trees restored clean.
- Both assigned independent review lanes reproduced the same blocker.
GitHub checks were still running during the final review pass. Their eventual result cannot clear the reproduced defect.
Manual/native evidence and residual risk
The iOS correction is source-sound and has focused tracker coverage, but no exact-head native interaction/VoiceOver journey demonstrates the SwiftUI geometry preference wiring at the short-final-section boundary. The available PR artifacts are static and predate this head. That is residual evidence risk, not a second reproduced blocker.
Any new head invalidates this verdict until the corrective delta and affected evidence are rechecked.
Treat the clamped bottom of an overflowing emoji grid as belonging to its final section, so a short Custom section retains the rail highlight and selected accessibility state after scrolling settles. Add a widget regression that exercises the rail tap, verifies the scroll target is bottom-clamped, and checks both Custom and the preceding category colors. 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>
|
Addressed the remaining Android bottom-clamp finding in
Verification at exact pushed head
— Princess Donut, on Kenny's behalf |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dc61ab93f
ℹ️ 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".
| final container = ProviderScope.containerOf(context, listen: false); | ||
| final List<CustomEmoji> customEmoji; | ||
| try { | ||
| customEmoji = await container.read(customEmojiPaletteProvider.future); |
There was a problem hiding this comment.
When the iOS picker is opened while the palette request is pending, this await prevents any sheet or loading indicator from appearing. If a connected relay fails to send EOSE, CustomEmojiPaletteNotifier._fetch() waits for fetchHistory's default eight-second timeout before falling back, so the emoji button appears unresponsive for the entire interval. Preserve the resolved Custom section without blocking presentation invisibly—for example, present a loading state immediately while awaiting the palette.
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Reviewed: 417eea2230c1864e8c77f6440dbcfa109bfb63f6..2dc61ab93f2265c28819bf044395caf42e3da289 (exact live head 2dc61ab93f2265c28819bf044395caf42e3da289)
Risk: Medium — user-visible, platform-specific picker geometry, persisted preference state, async native lifecycle, and accessibility-selected state.
The Android bottom-clamp blocker is resolved. _activeSectionIndex now treats the final section as active only when the list actually overflows and the controller is at its clamped end (mobile/lib/features/channels/emoji_picker/emoji_grid.dart:65-85). Both notifier reconstruction and scroll updates pass the live maxScrollExtent (mobile/lib/features/channels/emoji_picker.dart:154-180), so rail taps, manual scrolling, and the skin-tone rebuild converge on one rule. The same active value drives visual color and Semantics.selected (category_rail.dart:179-209). A non-scrollable list is not forced to its last category.
The retained widget regression exercises the original boundary: tapping the short final Custom category settles at maxScrollExtent, keeps Custom primary, and clears Nature (mobile/test/features/channels/emoji_picker_test.dart:384-413). Two independent exact-head probes also drove manual bottom scrolling and inspected the selected semantics state. Removing only the new bottom-aware branch made the retained regression and the expanded semantics probe fail; restoring the branch made both pass. The earlier skin-tone retention regression was independently mutation-proved as well.
Validation at matching clean HEAD:
just mobile-check— pass; 415 files format-clean, analyzer clean.just mobile-test— pass; 1,482/1,482 Flutter tests.- Focused picker suites — pass in independent lanes (30/30 and 31/31 reporting differed because one lane included its temporary probe; both scopes were stated).
- Corrective
git diff --check b57a66d..2dc61ab— pass. - GitHub Mobile, macOS build, integration, release-candidate, and DCO checks — pass at the reviewed head.
- GitHub Desktop Smoke E2E shard 4 failed in unrelated Desktop tests (
video-attachment, plus two retry-flaky Desktop cases); this PR changes only mobile picker files. I attempted a failed-job rerun but lack repository admin permission. This is CI noise to resolve separately, not evidence against the mobile correction.
Native/manual evidence: Android behavior was exercised in the Flutter widget boundary with rail-tap, manual-scroll, visual, and semantics assertions. The iOS bottom-aware tracker and .isSelected mapping remain source- and unit-test-supported, but no exact-head native iOS interaction/VoiceOver artifact was available.
Residual risk: Real-device iOS detent/VoiceOver integration and Android TalkBack rendering were not exercised at this head. No material unresolved source or executable finding remains in the reviewed change.
— :bot: Jude’s code review agent
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Princess Donut, automated reviewer acting via Wes's GitHub account.
Requesting changes at exact head 2dc61ab93f2265c28819bf044395caf42e3da289.
P2 — iOS can make the emoji button appear dead for the relay-history timeout
_presentIosEmojiPicker takes the global presentation guard and then awaits customEmojiPaletteProvider.future before presenting any sheet or loading state (mobile/lib/features/channels/emoji_picker/ios_native_picker.dart:24-46). The palette fetch waits on fetchHistory, whose default timeout is eight seconds (mobile/lib/shared/custom_emoji/custom_emoji_provider.dart:24-36; mobile/lib/shared/relay/relay_session.dart:232-252). During an initial load, reconnect, or refresh against a relay that delays/omits EOSE, tapping Emoji therefore produces no visible response for up to eight seconds. The checked-in test explicitly proves presentation waits for completion; it does not protect responsiveness.
This is a reachable interaction trap in a primary composer control, not merely slow custom content. Present the native sheet immediately with standard/recent emoji and a loading/late custom section, or show an immediate cancellable loading surface while the palette resolves; add a delayed-palette regression asserting immediate user-visible presentation/feedback.
The prior lifecycle, authenticated-media, bounded-download, category-tracking, and Android bottom-clamp blockers are resolved at this head. I found no additional material security defect.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Requesting changes at exact head 2dc61ab93f2265c28819bf044395caf42e3da289 after consolidating the Royal Court review. Three lanes found the prior lifecycle, resource-bound, category-tracking, and Android bottom-clamp defects resolved. I independently verified one remaining user-visible blocker.
P2 — opening the iOS picker can show no response for the relay-history timeout
_presentIosEmojiPicker sets the process-global presentation guard and then awaits customEmojiPaletteProvider.future before presenting either the native sheet or any loading UI (mobile/lib/features/channels/emoji_picker/ios_native_picker.dart:24-46). The palette's connected-session path awaits fetchHistory (mobile/lib/shared/custom_emoji/custom_emoji_provider.dart:20-40), whose default timeout is eight seconds (mobile/lib/shared/relay/relay_session.dart:230-257). If the palette is loading during startup, reconnect, or refresh and the relay delays or omits EOSE, tapping the primary Emoji control therefore produces no visible response for up to eight seconds. Reentry is also rejected during that invisible wait because the global guard is already held.
The test at mobile/test/features/channels/emoji_picker_test.dart:790-838 confirms this ordering: native presentation remains absent until the palette future completes. It protects data completeness but codifies the interaction stall rather than responsiveness.
Present standard/recent emoji immediately and attach Custom when available, or provide an immediate cancellable loading surface while the palette resolves. Add a delayed-palette regression proving the first tap produces immediate visible feedback and that dismissal/reentry remain single-shot.
Consolidated verification
At the clean live head above, git diff --check 417eea2230c1864e8c77f6440dbcfa109bfb63f6...HEAD passes. GitHub Mobile and DCO pass. Desktop Smoke E2E shard 4 is red in desktop-only tests while the PR changes only mobile files; that unrelated gate still needs a rerun or separate resolution, but it is not the reason for this review state. I did not duplicate CI-equivalent suites locally.
The previous palette-error fallback, callback ownership, aggregate image limits, deterministic concurrency test, skin-tone retention, and bottom-clamped category selection are resolved. No additional material security or lifecycle defect remains in the reviewed picker paths.
Present a cancellable loading sheet immediately when the custom emoji palette is unresolved, then hand off to the native picker once the palette arrives. Cover visible feedback, completion, and cancellation ownership. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 417eea2230c1864e8c77f6440dbcfa109bfb63f6..8378c2bf2a434e4038509e256b506de0714fd1d2 (exact head 8378c2bf2a434e4038509e256b506de0714fd1d2)
Risk: medium — user-visible iOS loading, cancellation, callback ownership, and accessibility behavior.
Findings: No unresolved material findings. The new loading route gives immediate, cancellable feedback before the relay-backed palette resolves (mobile/lib/features/channels/emoji_picker/ios_native_picker.dart:31-73). Success and failure handoffs re-check cancellation before route replacement (ios_native_picker.dart:75-108), while native callback ownership starts only after that transition (ios_native_picker.dart:120-170). The composer lifecycle still clears its open/focus state through onDismiss (mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:969-976). The two-file follow-up remains aligned with the picker intent and does not introduce unrelated scope.
Validation at matching clean HEAD:
git diff --check 417eea2...HEAD— pass.just mobile-check— pass; 415 files format-clean and analyzer clean.just mobile-test— pass; 1,483/1,483 tests.- Focused picker suite — pass; 31/31.
- Causal mutation replacing visible loading content with an empty widget made the new responsiveness regression fail at
mobile/test/features/channels/emoji_picker_test.dart:830; restoring candidate bytes returned 31/31 green. - Adversarial dismissal race completed palette resolution after dismissal began; observed one dismissal and zero native presentations.
- All required GitHub checks completed successfully at this exact head, including Mobile, macOS Desktop Build, Windows Rust, Security, Unit Tests, Relay E2E, and integration gates.
Manual/native evidence: No exact-head iOS simulator/device video or VoiceOver run was performed on the shared machine. The real Flutter-loading-to-native-detent visual/assistive handoff remains residual risk; source tracing, causal widget coverage, lifecycle race coverage, full mobile validation, and exact-head CI make it non-blocking for this narrow correction.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 417eea2230c1864e8c77f6440dbcfa109bfb63f6..8378c2bf2a434e4038509e256b506de0714fd1d2 (exact head 8378c2bf2a434e4038509e256b506de0714fd1d2)
Risk: medium — user-visible iOS loading, cancellation, callback ownership, and accessibility behavior.
Findings: No unresolved material findings. The new loading route gives immediate, cancellable feedback before the relay-backed palette resolves (mobile/lib/features/channels/emoji_picker/ios_native_picker.dart:31-73). Success and failure handoffs re-check cancellation before route replacement (ios_native_picker.dart:75-108), while native callback ownership starts only after that transition (ios_native_picker.dart:120-170). The composer lifecycle still clears its open/focus state through onDismiss (mobile/lib/features/channels/compose_bar/compose_bar_widget.dart:969-976). The two-file follow-up remains aligned with the picker intent and does not introduce unrelated scope.
Validation at matching clean HEAD:
git diff --check 417eea2...HEAD— pass.just mobile-check— pass; 415 files format-clean and analyzer clean.just mobile-test— pass; 1,483/1,483 tests.- Focused picker suite — pass; 31/31.
- Causal mutation replacing visible loading content with an empty widget made the new responsiveness regression fail at
mobile/test/features/channels/emoji_picker_test.dart:830; restoring candidate bytes returned 31/31 green. - Adversarial dismissal race completed palette resolution after dismissal began; observed one dismissal and zero native presentations.
- All required GitHub checks completed successfully at this exact head, including Mobile, macOS Desktop Build, Windows Rust, Security, Unit Tests, Relay E2E, and integration gates.
Manual/native evidence: No exact-head iOS simulator/device video or VoiceOver run was performed on the shared machine. The real Flutter-loading-to-native-detent visual/assistive handoff remains residual risk; source tracing, causal widget coverage, lifecycle race coverage, full mobile validation, and exact-head CI make it non-blocking for this narrow correction.
Resolve overlapping iOS native-surface registrations by retaining both the emoji picker and message action surface, assigning unique Xcode object IDs, and preserving both native test suites. Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com>
* 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
…-in-thread * origin/main: (32 commits) Revert "fix(acp): gate relay-signed workflow messages on their attributed author" (#6311) fix(desktop): morph the drawer panel icon instead of sliding it (#6306) feat(desktop): refine repository-aware project workspaces (#6003) Fix mobile Activity thread navigation (#5850) perf(desktop): parallelize relay agent directory rebuild (#6258) Refine the mobile emoji picker (#5853) fix(desktop): exclude archived agents from nest, order regeneration (#5905) Add font size and conversation density preferences (#5644) fix(desktop): emit camelCase config-write payload fields (#6062) fix(desktop): downscale large avatars for agent-share PNG body (#6260) fix(desktop): preserve early relay auth challenges (#3320) Polish mobile message actions (#5873) Refine mobile pairing confirmation (#6018) chore(scripts): add buzz-adopt-prod-agents.sh (#6250) feat(managed-agents): close five Claude Code agent-config gaps (#4557) chore(hooks): keep mobile analysis out of pre-commit (#6236) fix(shared-ui): delay hover disclosures by default (#5821) fix(desktop-chrome): preserve balanced layout when sidebar collapses (#6000) Polish mobile timeline navigation (#5874) chore(release): release Buzz Desktop version 0.5.17 (#6234) ... Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…c-agent-commit-identity * origin/main: Revert "fix(acp): gate relay-signed workflow messages on their attributed author" (#6311) fix(desktop): morph the drawer panel icon instead of sliding it (#6306) feat(desktop): refine repository-aware project workspaces (#6003) Fix mobile Activity thread navigation (#5850) perf(desktop): parallelize relay agent directory rebuild (#6258) Refine the mobile emoji picker (#5853) fix(desktop): exclude archived agents from nest, order regeneration (#5905) Add font size and conversation density preferences (#5644) fix(desktop): emit camelCase config-write payload fields (#6062) fix(desktop): downscale large avatars for agent-share PNG body (#6260) fix(desktop): preserve early relay auth challenges (#3320) Polish mobile message actions (#5873) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
3 participants
Summary
Testing
just cigates completed, with the disk-heavy stages resumed individually after generated artifacts filled the worktree volumeSnapshots are attached in a PR comment.