fix(mobile): unify sheet sections and bottom spacing - #8081
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
🤖 Members sheet comparison, rendered from the base and updated widgets with sample data. BeforeMembers appear as ungrouped rows.
AfterPeople and agents use rounded sections with inset dividers.
|
![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: fd36749a7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| child: SafeArea( | ||
| top: false, | ||
| left: false, | ||
| right: false, | ||
| child: Padding( | ||
| padding: const EdgeInsets.only(bottom: Grid.half), | ||
| child: Builder(builder: builder), |
There was a problem hiding this comment.
When a device has a nonzero gesture-navigation inset, this new SafeArea moves the whole sheet viewport above that inset, but it only consumes MediaQuery.padding; MediaQuery.viewPadding remains unchanged. Several sheet bodies still add that physical inset themselves—such as MembersSheet at members_sheet.dart:42,99, the reaction list at reaction_row.dart:475, the status sheet at set_status_sheet.dart:172, and both emoji lists—so their final rows now sit one additional home-indicator height above the already-safe viewport instead of the intended extra 4 dp. Remove/migrate those child-owned viewPadding clearances (or explicitly normalize the media query); the new test's one-sided lessThanOrEqualTo assertion currently passes even with this excess spacing.
AGENTS.md reference: AGENTS.md:L234-L238
Useful? React with 👍 / 👎.
github-actions
Bot
added
the
codex-security-review-current
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 03f24a69bf34adf25bc7f85aa00ab74c9e80c759..fd36749a7a3da54361b6d32a53b9e03f8526d08e (exact live head; systems/integration and product/UI/adversarial lanes reconciled).
Risk: medium — this centralizes system-inset ownership and migrates many user-visible sheets and grouped action surfaces.
Author-actionable defect
P2 — several scrolling sheets double-reserve the system gesture inset after inset ownership moved to the shared modal wrapper. showBuzzModalBottomSheet moves the full child viewport above a bottom SafeArea and adds 4dp (mobile/lib/shared/widgets/modal_presentation.dart:79-90), but modal descendants still add MediaQuery.viewPadding.bottom, which SafeArea does not zero: Members (mobile/lib/features/channels/members_sheet.dart:42,94,99), reaction details (mobile/lib/features/channels/reaction_row.dart:471-476), and activity transcript/empty states reached from Members (mobile/lib/features/agents/agent_activity_sheet.dart:75-76,117-133). On a 34dp home indicator this yields roughly 76dp after the final row instead of the intended 38dp, with analogous Android gesture-navigation behavior; Members also inflates its maximum viewport.
- Author action: once the wrapper owns system clearance, remove descendant
viewPadding.bottomreservations from these modal-only paths while retaining intended design spacing. Audit remaining modal callers readingviewPadding(includingset_status_sheet.dart:161-172) under one explicit owner. Add widget rows opening each affected sheet with nonzeropaddingandviewPaddingon iOS/Android and assert exactly one system inset plus intended spacing for final and empty content. - Verification owner: author for fix/regressions; reviewer/CI for exact-head gates and native visual confirmation.
Reconciled evidence
The systems lane found the shared modal and grouped-row primitives coherent in isolation: one outer bottom SafeArea, constrained body, preserved lazy lists, presentation-only grouping, and intact callback/semantic ownership. The product lane found the material integration problem above in migrated descendants that use viewPadding rather than consumed padding; that exact distinction reconciles the apparent disagreement. No separate tap-target, keyboard obstruction, or row-semantics defect was established.
Validation and residual risk
- Complete 24-file exact-head diff/call-site audit and
git diff --checkpassed; one lane’sjust mobile-checkpassed (632 files unchanged; analyzer clean). - Exact-head Mobile, Mobile Swift, security review, Semgrep, zizmor, and DCO gates are green.
- Local Flutter widget execution was blocked before tests by the reviewer host’s unaccepted Xcode license/native-assets SDK lookup. Native iOS/Android short, empty, long, keyboard-open, and gesture-area visual/AX journeys were not independently observed. These are confidence gaps, not extra author defects. Verification owner: reviewer/tooling/native validation.
Cleanup: no PR-specific *-carl checkout or retained build/scratch artifact remains.
:bot: Jude’s code review agent — retracting this review because its blocking finding was based on an incorrect statement about Flutter SafeArea behavior. Flutter 3.41.7 MediaQueryData.removePadding(removeBottom: true) also reduces descendant viewPadding.bottom by the consumed padding.bottom, so the claimed no-keyboard double reservation does not occur. A corrected exact-head review follows.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 03f24a69bf34adf25bc7f85aa00ab74c9e80c759..fd36749a7a3da54361b6d32a53b9e03f8526d08e (exact live head; systems/integration and product/UI/adversarial lanes reconciled).
The prior REQUEST CHANGES review is retracted and dismissed. Its blocker incorrectly stated that Flutter SafeArea consumes descendant padding but not viewPadding. In pinned Flutter 3.41.7, SafeArea.build delegates to MediaQuery.removePadding(removeBottom: true), whose MediaQueryData.removePadding sets padding.bottom to zero and reduces viewPadding.bottom by the consumed padding. In ordinary no-keyboard gesture-area state, the affected Members, Reaction Details, and Agent Activity descendants therefore see zero system viewPadding; their remaining Grid.md / Grid.half / Grid.sm is design spacing, not a duplicate system inset. Both review lanes rechecked the framework source and agree there is no author-actionable defect.
Integrated review
showBuzzModalBottomSheetestablishes one outer bottomSafeAreaplus 4dp and keeps the full scrolling viewport above the gesture area (mobile/lib/shared/widgets/modal_presentation.dart:66-90)._SheetContentretains bounded body behavior throughFlexible(:121-159).- The Android/iOS × titled/untitled × child-
SafeAreamatrix verifies viewport clearance, consumed descendant padding, scrolling to the final row, and stable viewport bounds (mobile/test/shared/widgets/modal_presentation_test.dart:10-91). An assertion that descendantviewPadding.bottom == 0would document the framework contract more explicitly, but is optional coverage—not author rework. SheetActionSectionandAppListCardItemare presentation-only grouping primitives; first/last clipping and separators preserve childListTile/Semanticsinteraction ownership (mobile/lib/shared/widgets/sheet_action_section.dart:7-37,mobile/lib/shared/widgets/app_list_card_item.dart:5-54). Callback/capability behavior remained attached across migrated sheets.- Lazy and long-list behavior remains bounded: Add Members and Browse Channels retain builders, while Members retains its constrained scrolling list and separate People/Agents sections.
Evidence and residual risk
git diff --check: PASS; clean exact-head worktree.just mobile-check: PASS at exact head (632 files unchanged; analyzer clean).- Exact-head
Clients / Mobile,Mobile,Mobile Swift Domain / Mobile Swift, Codex Security Review, Semgrep, zizmor, and DCO: PASS. - Local Flutter widget execution was blocked before tests by the reviewer host’s unaccepted Xcode license/native-assets SDK lookup. Independent native iOS/Android visual/AX journeys were not observed; the author reports Pixel 10 install/launch. These are reviewer/tooling confidence gaps, not author action.
Author action: none.
Verification owner: CI for automated gates; reviewer/tooling for optional native visual/AX follow-up and optional descendant-viewPadding contract assertion.
Cleanup: no PR-specific *-carl checkout or retained build/scratch artifact remains.
This branch was successfully deployed
2 participants
Summary
Use rounded, grouped rows consistently across mobile sheets and menus, including Members and list pickers. Keep sheet content above the system gesture area with 4dp of extra bottom padding on Android and iOS.
Related issue
No duplicate found; related member-picker work: #4051.
Testing
just cistopped during Rust compilation because local disk space ran out.