Refresh mobile utility surfaces and theme picker - #6944
Conversation
Signed-off-by: kenny lopez <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c9d63125b
ℹ️ 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".
Signed-off-by: Kenny Lopez <klopez4212@gmail.com> Signed-off-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz> Co-authored-by: Princess Donut <b238ea756dee4d98afa5883fc7f1de61eeabe65bf700e3a5a5a80db5e42e2c2b@buzz.block.builderlab.xyz>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Reviewed exact head bcb8f511064db20843eeb80e54d8acb86452d277 against base c363ce13c07cb454d6fa92b96d65cb52322c13e9.
Blocking finding
P2 — Commit the theme picker’s Set action as one preference snapshot (mobile/lib/features/settings/theme_picker_page/theme_preview_sheet.dart:84-90)
applySelection() presents Set as one commit, but implements it as up to three independent notifier mutations (setMode, setTheme, setAccent). Every setter calls _save, and _save immediately enqueues a separate cache/outbox operation (mobile/lib/shared/theme/community_theme_provider.dart:112-188). The queue serializes those operations; it does not coalesce them. The sync manager’s pending guard can prevent an obsolete relay publish, but it cannot undo the earlier cache/outbox writes.
Consequently, termination or a failed later write can durably retain/replay a mode-only or mode+theme intermediate state from a single Set action. This behavior is introduced by the new picker commit path; the existing test only checks that nothing is written before Set and that the eventual legacy theme changes, not that Set is atomic.
Author action: add a notifier operation that accepts the complete CommunityThemePreference (or the mode/theme/accent tuple), have Set invoke _save once, and add an instrumented regression proving one cache/outbox snapshot is committed for one Set action.
Validation and remaining verification
- Systems trace found no other author-actionable ownership, identity-scope, relay, platform-channel lifecycle, argument-validation, migration, or release defect.
- Clean exact-head reviewer run:
git diff --checkpassed;just mobile-checkpassed;just mobile-testpassed 1,872/1,872; unsigned release iOS build passed and producedBuzz.app(65.9 MB). - GitHub’s Mobile, Security, DCO, and normal CI lanes are green.
Mark Previous Review Staleis bot/tooling noise, not a product-code failure. - Native iOS liquid-glass rendering, gestures, and VoiceOver composition remain a confidence gap for iOS device/release validation. In particular, Flutter and the native pagination control both declare adjustable semantics; device validation should confirm this does not create duplicate focus or announcements. No author action on that point unless native observation reproduces it.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
REQUEST CHANGES at exact head bcb8f511064db20843eeb80e54d8acb86452d277 against base c363ce13c07cb454d6fa92b96d65cb52322c13e9.
Blocking finding
[P2] One Set action persists three independently recoverable theme snapshots — mobile/lib/features/settings/theme_picker_page/theme_preview_sheet.dart:84-90, mobile/lib/shared/theme/community_theme_provider.dart:112-190.
applySelection() calls setMode, setTheme, and setAccent sequentially. Each setter calls _save; _save immediately updates provider state, stages relay state, and enqueues its own _persistAndPublish operation. _enqueuePersistence serializes all three operations rather than coalescing them, and each operation writes both the scoped cache and outbox before attempting relay publication.
The sync manager's _pending != preference guard does prevent the first two snapshots from reaching relay publication once the final value has been staged. It does not prevent the first two queued operations from durably overwriting local cache/outbox. A process termination or later write failure can therefore recover or replay a mode-only or mode+theme preference even though the user pressed one atomic-looking Set action. The new widget test proves that drafts do not write before Set and that the eventual value changes, but it does not assert one durable commit.
Author action: add one notifier operation accepting the complete CommunityThemePreference (or equivalent mode/theme/accent tuple), make Set invoke _save once, and add an instrumented persistence regression proving one cache/outbox transition for one Set action.
Verification owner: author for the atomic save and regression; reviewer/CI for replacement-head mobile and native freshness.
Integrated exact-head evidence
- Systems tracing found community/identity scoping, serialized persistence/outbox/retry behavior, per-view iOS channel isolation, bounded native inputs, and no relay schema/auth/migration/release change.
just mobile-checkpassed; full mobile suite passed 1,872/1,872; unsigned iOS release build passed and producedBuzz.appat this head.- Exact-head Mobile, Security, DCO, and normal CI lanes are green.
Mark Previous Review Staleis administrative review plumbing, not this product defect. - The possible duplicate VoiceOver ownership between Flutter
Semanticsand the native adjustable control was not reproduced on a device. That remains a native-validation confidence gap, not separate author rework. Verification owner: iOS release/device validation.
Any new head invalidates this verdict.
Duplicate orchestration race: an exact-head team verdict had already been submitted seconds earlier. Dismissing this duplicate; review 5044630779 is authoritative.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed exact head bcb8f511064db20843eeb80e54d8acb86452d277 against base c363ce13c07cb454d6fa92b96d65cb52322c13e9. The intended direction is a consistent utility-surface hierarchy and a single draft-then-commit theme-picker action across Android and iOS. Three exact-head defects still break that contract.
Blocking findings
-
[P2] Commit Set as one durable theme preference —
mobile/lib/features/settings/theme_picker_page/theme_preview_sheet.dart:84-90,mobile/lib/shared/theme/community_theme_provider.dart:112-190applySelection()makes one user-visible commit but callssetMode,setTheme, andsetAccentindependently. Each changed field calls_save, immediately mutates/stages state, and enqueues its own cache/outbox persistence. Serialization does not make the group atomic: termination after an early write, or failure of a later write, can leave an intermediate mode-only or mode+theme snapshot durable from one Set action. Add one notifier operation for the complete preference, invoke_saveonce, and add an instrumented regression proving one cache/outbox transition. -
[P2] Give untitled Android routes the transformed sheet surface —
mobile/lib/shared/widgets/modal_presentation.dart:56-86Standard untitled calls with no explicit
backgroundColorcompute the transformedsurfaceColor, but passnulltoshowModalBottomSheeton non-iOS. The route'sBottomSheettherefore resolves its material color from the caller's original theme; the transformedThemeexists only below that material and cannot recolor it. Untitled forum/profile/invite action sheets can consequently paint the old surface while their controls resolve transformed container roles, collapsing the intended contrast. PasssurfaceColoron this path and cover an untitled Android sheet, not only titled Android and iOS cases. -
[P2] Collapse decorative device previews into their image labels —
mobile/lib/features/settings/theme_picker_page/theme_home_preview.dart:31-104The overall preview and each mock device are labeled as images, but none excludes descendant semantics. The Home mock includes real text such as the active community name, so assistive technology can traverse/merge decorative internals in addition to “Theme preview,” “Home preview,” and “Chat preview.” Set
excludeSemantics: trueat the intended image boundary (or wrap the mock screens inExcludeSemantics) and add a semantics regression asserting the exposed labels and absence of mock content.
Verification
- Source/API-only review; no PR code was checked out or executed.
- Exact-head Mobile, Security, DCO, and standard CI lanes are green.
Mark Previous Review Staleis administrative tooling noise and does not address these defects. - Stored-accent preservation is fixed and causally covered at this head. Native registration, per-view channel cleanup, input bounds, and reduced-motion fallback showed no additional source defect. Physical-device VoiceOver composition remains a release-validation gap.
- The utility transform is marker-idempotent, but a composed utility-page → sheet widget test would strengthen the keystone contract; this is not a separate blocker.
Signed-off-by: kenny lopez <klopez4212@gmail.com>
🔐 Codex Security Review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef9b58e445
ℹ️ 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".
Signed-off-by: kenny lopez <klopez4212@gmail.com>
Signed-off-by: kenny lopez <klopez4212@gmail.com>
|
🤖 Addressed Wes and Jude’s review blockers on the latest head
Validation: |
Summary
Related issue
None found.
Testing
just mobile-checkjust mobile-test(1,871 tests)