Polish Huddle participant interactions - #6312
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Kenny Lopez <klopez4212@gmail.com> Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
Co-authored-by: Kenny Lopez <klopez4212@gmail.com> Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
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>
Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz> Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz> Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
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: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
![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: ebe747b902
ℹ️ 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".
klopez4212
left a comment
klopez4212
left a comment
There was a problem hiding this comment.
Carl reviewing on Kenny Lopez’s behalf. I found two user-visible correctness blockers. Codex’s live-state finding is valid and reproduced with a focused widget mutation: after opening Miles’ spotlight and emitting remote audio, only the underlying avatar changed to Miles, speaking; the dialog remained frozen. Separately, this branch removes the post-admission authoritative-roster guard added in 8b950daef3, so stale backing-channel members can again appear as connected participants after a socket loss. Please preserve transport authority after admission while implementing the new cluster motion, and make the spotlight subscribe to live participant/profile state.
Validation at exact head ebe747b90274e04d0c9dd3603be61c5ec9100e5c: git diff --check passed; the existing focused full-screen Huddle widget test passed; the live-overlay mutation failed as described and was removed, leaving the review worktree clean.
GitHub does not permit a changes-requested review from the PR author account, so this is submitted as a blocking comment review rather than an approval.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Review pinned to base 7969ebde01ab4b95f5d001dc94264d50ca0440b9 and head ebe747b90274e04d0c9dd3603be61c5ec9100e5c.
Changes requested
1. Admitted calls treat backing-channel membership as live audio presence
mobile/lib/features/channels/channel_detail_page/huddle_sheet.dart:23-43,543-554 now always unions channelMembersProvider(ephemeralChannelId) into the transport participant list. That conflicts with the live presence authority: HuddleSessionState.participantPubkeys is populated from admitted transport peers in mobile/lib/shared/huddle/huddle_session.dart:287-298,404-422,509-513.
After admission, stale or delayed channel membership can therefore appear as a person currently in the call, consume one of the ten visible slots, inflate +N, and trigger profile subscriptions despite never being an audio peer. The previous regression behavior—backing membership only before admission—was inverted by the test at mobile/test/features/channels/channel_detail_page_test.dart:4425-4539, so the green test currently codifies the wrong authority boundary.
Please restore transport peers as authoritative after admission, retaining backing membership only as the pre-admission/fallback source. Add a regression that admits audio, injects a membership-only pubkey, and proves it creates no avatar, overflow entry, or profile subscription; separately cover transport peer join/leave.
2. Participant overlays freeze live call and accessibility state when opened
mobile/lib/features/channels/channel_detail_page/huddle_sheet.dart:639-658 passes snapshots of profiles, hidden pubkeys, and active speakers into showGeneralDialog. The route children retain and render those ordinary values at mobile/lib/features/channels/channel_detail_page/huddle_participant_overlay.dart:147-190,233-249,301-344; they do not watch the session, profile cache, or current roster.
Consequently, an open spotlight can keep stale speaking styling and falsely announce “speaking” after state changes, while an open overflow roster can retain departed participants, miss joins/profile updates, and show the wrong count until dismissed. Existing coverage at mobile/test/features/channels/channel_detail_page_test.dart:3716-3757,3788-3793 closes the spotlight before remote audio changes and does not exercise this lifecycle.
Please derive overlay content from live provider/listenable state, including dismissing or otherwise resolving a spotlight whose participant leaves. Add widget coverage that keeps each overlay open while speaker, peer, and profile state changes and verifies visual and semantic updates.
Validation
At this exact clean head:
git diff --check 7969ebde...ebe747b: passedjust mobile-check: passed (format, analyze, file-size gate)cd mobile && flutter test: passed, 1,538 tests- GitHub currently exposes only a passing DCO check; no substantive mobile/platform CI check is attached
Scope otherwise matches the Huddle participant-interaction intent and VISION.md. No native iOS/Android exact-artifact journey was run, so haptics, animation/blur behavior, TalkBack/VoiceOver focus, narrow/landscape layout, and large-text behavior remain unproven. Green widget tests are useful; they are not a séance capable of proving device behavior.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Requesting changes at exact head ebe747b90274e04d0c9dd3603be61c5ec9100e5c. The integrated systems and product/UI review found two material live-state defects:
-
Backing-channel membership is treated as admitted audio presence.
mobile/lib/features/channels/channel_detail_page/huddle_sheet.dart:23-43,543-554now always unionschannelMembersProvider(ephemeralChannelId)into transport peers, although admitted presence comes fromHuddleSessionState.participantPubkeys(mobile/lib/shared/huddle/huddle_session.dart:287-298,404-422,509-513). Stale membership can therefore render non-peers as current callers, consume visible slots, inflate+N, and expand profile subscriptions. The new test atmobile/test/features/channels/channel_detail_page_test.dart:4425-4539encodes the inverted authority. Once admitted, transport peers should remain authoritative; backing membership should be pre-admission/fallback only. Add a regression proving a membership-only pubkey does not create an avatar, overflow entry, or profile subscription after admission. -
Spotlight and overflow dialogs freeze participant state at open time. The call page passes snapshots into
showGeneralDialog(huddle_sheet.dart:639-658), and the route children render those ordinary values without watching session/profile state (huddle_participant_overlay.dart:147-190,233-249,301-344). Speaking status and screen-reader semantics can remain false or stale, departed participants remain shown, and roster count/names do not update until dismissal. Make the overlays derive current state by pubkey (or use provider/listenable-backed builders), dismiss/refresh a departed selection, and add tests that mutate speaking, join/leave, and profile state while each overlay remains open.
Exact-head evidence: git diff --check passed; just mobile-check passed; full flutter test passed 1,538/1,538 on clean pinned trees. Those tests do not invalidate the findings—the roster test currently asserts the wrong authority, and overlay tests mutate state only after dismissal. GitHub exposed only DCO at review time. Native iOS/Android rendering, haptics, assistive technology, blur performance, landscape/narrow, and large-text journeys remain unverified.
Cleanup: reviewer trees were reported clean; no author-branch writes or pushes; REPOS/*-carl is empty.
Duplicate exact-head automation review caused by a concurrent sweep race; superseded by the earlier integrated A Team review on the same head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c4019363c
ℹ️ 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".
Resolves six conflicts, preserving both sides' intent: - mobile/android/app/build.gradle.kts: main added AppOverrides.properties (developer overrides) and this branch added worktree.properties appName. Both survive with AppOverrides taking precedence over the worktree appName, which in turn beats the branch label. - mobile/ios/Runner.xcodeproj/project.pbxproj: both sides minted the SAME four object IDs for different Swift files. Kept main's IDs and re-minted this branch's HuddleMediaPlugin/HuddleAudioEngine entries to unused IDs. - mobile/ios/Runner/AppDelegate.swift: kept all three new stored properties. - mobile/lib/features/channels/channel_detail_page.dart: unioned the dart:math shows and kept both imports; main renamed the tail-follow widget to JumpToLatestButton and the huddle controller import is unchanged. - mobile/test/features/channels/channel_detail_page_test.dart: kept both test doubles. - scripts/test-mobile-worktree-overrides.sh: kept both contract assertions. Verified on the merge result: mobile/ios/Runner.xcodeproj parses with plutil and its file-reference, sources-membership and Runner-group sets each equal the union of base, main and this branch; the worktree identity contract script passes all checks from both sides; flutter analyze is clean; the mobile suite passes 1596 tests. No relay file is touched by this merge. Co-authored-by: Tom Brow <tomb@block.xyz> Signed-off-by: Tom Brow <tomb@block.xyz>
Resolves four mobile Dart conflicts, preserving both sides' intent: - channel_management_provider.dart: keep main's channel_metadata_updates export and channel_management_actions part alongside the branch's channel_huddle_actions part. - channel_detail_page.dart: take main's _ChannelAppBarTitle refactor (richer than the branch's inline title: icon + label + ephemeral badge + tap-to-details) and its isDm-split actions, then layer the branch's _HuddleButton (gated on showsComposer) into both the DM and non-DM action lists. - channel_management_provider_test.dart / channel_detail_page_test.dart: keep both sides' additive test groups and test doubles; union the _FakeChannelActions fields, constructor params, and method overrides (onLeaveChannel/onArchiveChannel + onAddMembers/onUpdateChannel), keeping the branch's nsec sourced from relay config. flutter analyze clean on all four resolved files. Co-authored-by: Mongo <9cfd347903944d5b85aa6c93d2ab67381b978a92a31914bca69998968752a1d7@buzz.block.builderlab.xyz> Signed-off-by: Kenny Lopez <klopez4212@gmail.com>
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Requesting changes at exact head 7c4019363ce3622cc631bba4e978e563f16ae516 (base 7969ebde01ab4b95f5d001dc94264d50ca0440b9).
Major — dismiss spotlight when the selected remote participant leaves
_HuddleParticipantSpotlight watches the selected participant's profile, directory label, and active-speaker membership, but not the authoritative participant roster (mobile/lib/features/channels/channel_detail_page/huddle_participant_overlay.dart:124-153). When the remote peer leaves, HuddleSessionState.participantPubkeys drops that pubkey and the avatar behind the dialog disappears, yet the spotlight route remains open and continues presenting the departed person as a current caller.
That is a false live-presence state and is especially misleading to assistive-technology users because the modal participant semantics remain after the underlying participant has vanished. The new spotlight regression changes profile/speaking state while open (mobile/test/features/channels/channel_detail_page_test.dart:3924-3960) but does not exercise peer departure. The overflow route already demonstrates the safe lifecycle pattern at huddle_participant_overlay.dart:224-240.
Please observe whether huddleSessionProvider.participantPubkeys still contains the selected remote pubkey and schedule dismissal after frame when it does not (without applying this rule to self). Add a widget regression that opens a remote spotlight, emits emitPeerLeave, and proves the route closes without user action.
What the new delta fixed
- After admission, transport peers are authoritative again; backing membership no longer drives rendering/profile subscriptions (
huddle_sheet.dart:24-54,545-557). - Overflow participants, profiles, count, and speaker semantics update while open, and the roster closes when overflow empties (
huddle_participant_overlay.dart:224-245;channel_detail_page_test.dart:4473-4537). Spotlight profile/speaking semantics now update live as well.
Validation
git diff --check 7969ebd..7c401936: pass.just mobile-check: pass (440 files formatted; analyzer clean) at pinned, clean head.- Two independent full
cd mobile && flutter testruns did not produce a green package gate: 1,593 passed / 3 failed and 1,594 passed / 2 failed. Each failure was an unrelated timing-sensitive row and passed immediately in isolation; affected Huddle rows passed individually. This is flaky evidence, not a claimed green local suite. - Exact-head GitHub checks are green, including Mobile, Unit Tests, Security, and platform builds.
- No exact-artifact native iOS/Android journey was run; haptics, rendering/blur/animation, VoiceOver/TalkBack lifecycle, large text, and narrow/landscape layouts remain unproven.
- GitHub currently reports
mergeable=CONFLICTING/mergeStateStatus=DIRTY. Any conflict-resolution head invalidates this verdict and requires delta review.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Co-authored-by: Carl <3c4caeafb646d23867f1c4832e68211d77e2561946171625f75c3ce1a3f2670f@buzz.block.builderlab.xyz> Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
7c40193 to
4dfbd4c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4dfbd4c0bc
ℹ️ 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".
| left: 0, | ||
| top: 0, | ||
| width: frameSize, | ||
| height: frameSize + _huddleParticipantLabelSpace, |
There was a problem hiding this comment.
With five or more remote participants, the deliberately overlapping cluster slots cause later avatars in the Stack to sit above earlier ones for hit testing. This height also retains 28 pixels for labels that _HuddleCallAvatar no longer renders, and its opaque rectangular GestureDetector captures taps in that transparent area; for example, the fifth participant's unused lower strip overlaps visible pixels of the center avatar, so tapping the center can spotlight the fifth participant instead. Restrict the hit box to the visible avatar frame or use circular hit testing.
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: ede2107b8b4be599707609b5b4c5e301b15250ee..4dfbd4c0bc16f5acb5c930d164e6072e40010a4e (exact head 4dfbd4c0bc16f5acb5c930d164e6072e40010a4e)
Risk: medium — user-visible mobile call interaction, live participant state, semantics, animation, and haptics; no relay/schema/identity/release contract changes.
No material unresolved finding remains in the integrated systems/integration and product/UI review.
Behavior and contracts traced
- After admission, transport peers remain authoritative; backing-channel membership is fallback only and cannot fabricate a live caller (
mobile/lib/features/channels/channel_detail_page/huddle_sheet.dart:24-45,539-560; regression atmobile/test/features/channels/channel_detail_page_test.dart:5253-5383). - Spotlight and overflow content remain provider-backed while open. A selected remote peer leaving schedules spotlight dismissal, while self is deliberately exempt (
huddle_participant_overlay.dart:124-168,240-256; regression atchannel_detail_page_test.dart:4507-4574). - Self remains non-actionable; remote avatars and overflow expose button labels/hints; reduced-motion branches cover cluster, halo, and overlay transitions (
huddle_call_participants.dart:73-87,119-131;huddle_call_avatar.dart:26-51,68-82;huddle_participant_cluster.dart:277-287;huddle_participant_overlay.dart:35-44,181-186). - Scope is confined to nine mobile Huddle files (
+1159/-213) and matches the stated interaction polish stacked on #6056.
Validation at matching exact head
git diff --check— pass.just mobile-check— pass; 444 Dart files unchanged by format, analyzer clean.- Full
cd mobile && flutter test— pass, 1,617 tests. - Causal mutation: removing only the new participant-presence watch/dismiss block makes the focused peer-departure regression fail because the spotlight remains; restoring the exact bytes passes.
- Adversarial responsive probes: the 25-person portrait journey renders ten avatars plus
+14and exercises overflow; a 375×667 probe retained ≥44 px remote/overflow targets. 320×568 and 844×390 expose pre-existingRenderFlexoverflow reproduced with the PR production files reverted to base, so it is not attributed to this PR. - GitHub Mobile, Desktop Build (macOS), DCO, path detection, and dead-token checks are successful at review time. Unrelated Desktop matrix jobs are still running; this approval does not claim all CI complete.
- Live PR head was rechecked immediately before submission:
4dfbd4c0bc16f5acb5c930d164e6072e40010a4e,mergeable=MERGEABLE.
Residual risk
No exact-artifact iOS/Android journey was available: the checked-in native-review harness documents iOS Simulator support as unimplemented, and this PR adds no mobile integration journey. Physical haptic feel, compositor blur/animation performance, VoiceOver/TalkBack focus restoration, actual-device rotation, and large-text rendering therefore remain unproven. Static screenshots and widget tests do not establish those device behaviors. Any new head invalidates this approval.
9a6abf4 to
cff6e82
Compare
2 participants
Summary
+NoverflowValidation
just mobile-checkDependency
Built on #6056 and contains only the follow-up interaction work. Merge after #6056 lands.