fix(mobile): hide notification settings without relay push capability - #8025
Conversation
Signed-off-by: Tom Brow <tomb@block.xyz>
|
@codex review |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25916bbe8d
ℹ️ 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: 25916bbe8d
ℹ️ 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
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 036215636f530a93ef5e0cddf985e579ed52e393..25916bbe8dd15c483a7e8ff1f315adfd0035dee4 (exact head 25916bbe8dd15c483a7e8ff1f315adfd0035dee4)
Risk: high — this changes whether an iOS user can reach notification opt-out and recovery controls while a persisted relay lease may remain active.
Behavior/contracts traced: active-community selection → relay capability discovery → Settings visibility → persisted opt-in → native snapshot removal and relay tombstone; loading/error/absent discovery, community switching, and configured/unconfigured builds.
Blocking findings
-
[P1] Keep the opt-out/recovery surface available for an already-enabled community.
mobile/lib/features/settings/settings_page/notifications_section.dart:11-18now hides the entire section whenever capability discovery is loading, fails, or returns no descriptor—even whencommunity.pushNotificationsEnabledis already true. Disabling this setting is the path that persistsfalse, removes the community from the native snapshot, and journals the relay tombstone (mobile/lib/shared/community/community_provider.dart:461-497). With the row hidden, an existing lease can continue delivering while the user has no in-app way to turn it off; the same guard also removes Open iOS Notification Settings for denied/unreadable permission. The new enabled-community test codifies the inaccessible state (mobile/test/features/settings/settings_page_test.dart:83-138). This violates the repository’s recovery-affordance rule (AGENTS.md:257-262).Author action: hide first-time opt-in only when the persisted setting is off and capability is unavailable. When it is already on, retain a truthful loading/unavailable state with a working local off action and retain iOS Settings recovery where applicable. Add regression coverage proving enabled + loading/absent/error can disable and drive the durable state/snapshot/tombstone seam.
-
[P2] Add a production retry trigger for transient capability discovery failure.
mobile/lib/shared/push/push_relay_capability_provider.dart:22-54collapses timeout/network/parse failures tonull, and its dependencies do not change while an otherwise healthy websocket remains connected. The new UI then stays hidden indefinitely after one failed NIP-11 request. The rediscovery test manually invalidates the provider (mobile/test/features/settings/settings_page_test.dart:183-196), but production code undermobile/libcontains no corresponding invalidation/refresh path.Author action: preserve absent capability versus transient failure as needed and add bounded/backed-off rediscovery or a real lifecycle trigger. Bind the regression to that production trigger—not direct test-only invalidation—and prove failure followed by valid capability reveals first-time opt-in.
Author action: the two changes above.
Verification owner: author for code and production-seam regressions; reviewer/tooling for native observation; release gate for signed App Store/dogfood enrollment and delivery.
Validation at matching HEAD:
- PASS —
git diff --check 0362156...25916bb. - PASS —
just mobile-check(Dart formatting and Flutter analysis; 608 files, no issues). - PARTIAL/BLOCKED BY REVIEWER HOST —
just mobile-test: gateway recipe checks passed, but Flutter tests stopped before execution becauseobjective_ccould not obtain the macOS SDK while the local Xcode license is unaccepted. This is not author rework. - PASS — exact-head GitHub
Clients / Mobile,Mobile,Mobile Swift Domain / Mobile Swift, both results gates, DCO, Semgrep, and zizmor.
Manual/native evidence: not obtained; iOS simulator/device and signed release artifact remain reviewer/release confidence gaps, not additional author defects.
Residual risk: companion release wiring does not itself prove App Store gateway identity/APNs enrollment or physical-device delivery; keep that rollout behind its release-owned verification.
— :bot: Jude’s code review agent
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed 036215636f530a93ef5e0cddf985e579ed52e393..25916bbe8dd15c483a7e8ff1f315adfd0035dee4 at the exact live head. Risk is high: this changes persisted opt-in controls, relay capability discovery/failure recovery, and an iOS native push boundary.
Blocking findings
-
[P1] Keep the opt-out and permission-recovery controls available for already-enabled communities.
The new early return hides the entire notification section while capability discovery is loading, errored, or returns no descriptor (
mobile/lib/features/settings/settings_page/notifications_section.dart:13-18). The regression rows explicitly seedpushNotificationsEnabled: trueand require the switch to disappear in those states (mobile/test/features/settings/settings_page_test.dart:83-138,:183-201).That strands users who already have an accepted lease: the hidden switch is the only UI path that persists
pushNotificationsEnabled=false, removes the community from the native snapshot, and journals/retries the higher-generation relay tombstone (mobile/lib/shared/community/community_provider.dart:461-497;mobile/lib/shared/push/push_bridge.dart:295-308). It also hides Open iOS Notification Settings from an enabled user whose permission is denied or unreadable. This violates the repository rule that a visibility guard must not hide the only recovery affordance (AGENTS.md:257-262).Author action: hide first-time opt-in only when the community is disabled and capability is unavailable. For a persisted enabled community, retain a truthful loading/unavailable state with a working local off action and retain iOS Settings recovery when permission is denied/unreadable. Add regression coverage for enabled + loading/absent/error that exercises disabling and proves durable state, native snapshot removal, and the tombstone path. Keep disabled + incapable hidden.
-
[P2] Add a production rediscovery path after transient NIP-11 failure.
Discovery collapses timeout/network/parse failures into a cached
null(mobile/lib/shared/push/push_relay_capability_provider.dart:44-54). The provider only rebuilds when its watched session/config/community/pubkey dependencies change (:22-42); a search ofmobile/libfound no explicit invalidation or refresh. Thus one transient NIP-11 failure can hide first-time opt-in indefinitely while the websocket remains healthy. The new rediscovery test manually callscontainer.invalidate(...)(mobile/test/features/settings/settings_page_test.dart:186-196), but production has no equivalent trigger.Author action: distinguish absence from transient discovery failure and add bounded/backed-off rediscovery or a real lifecycle-triggered refresh. The regression must drive that production trigger—not manually invalidate the provider—and prove failure → valid descriptor reveals opt-in.
Validation
- Exact head and clean tree rechecked immediately before this review; live PR head matched.
git diff --check 036215636f530a93ef5e0cddf985e579ed52e393..25916bbe8dd15c483a7e8ff1f315adfd0035dee4: PASS.- Exact-head CI:
Clients / Mobile,Mobile,Mobile Swift Domain / Mobile Swift, results aggregators, DCO, Semgrep, and zizmor: PASS. Codex security review was still running when this review was filed; it does not affect the two reproduced source findings. just mobile-check: PASS in the product/UI lane (Flutter analyze clean).- Focused Flutter suites: blocked before execution on reviewer tooling because the host has not accepted the Xcode license and
objective_ccould not obtain the macOS SDK. This is a confidence gap, not requested author rework. - Signed IPA/device enrollment, denied-permission → Settings → return, VoiceOver, and community-switch observation: not run; reviewer/release-owned confidence gaps.
Companion release seam
squareup/buzz-releases#104 head 21192c6b7bcaf618dfae50b9eb7819985352aac3 supplies one URL per iOS variant and its checks are green. The mobile enrollment/native code still hard-codes the dogfood profile, while docs/push-gateway-deployment.md:210-220 says the App Store profile is not part of this MVP. Treat the App Store URL as compile-time wiring only; keep App Store relay advertisement/rollout off until the gateway App Attest identity, APNs topic/profile, and physical-device enrollment/delivery are release-proven. That is a release-owned confidence gap, not additional #8025 author rework.
Any new head invalidates this verdict.
Signed-off-by: Tom Brow <tomb@block.xyz>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0f169aeef
ℹ️ 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: Tom Brow <tomb@block.xyz>
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE
Reviewed 036215636f530a93ef5e0cddf985e579ed52e393..a1dd1f7a3fb038ada3abe26f0e5b5b8c8614adbc at the exact live head. The changed head resolves the prior blockers; no author-actionable defect remains.
Integrated findings
- Persisted-enabled communities retain opt-out and iOS Settings recovery while capability is loading, absent, or failed. Disabled communities without capability remain hidden.
- Disable now distinguishes local truth from relay settlement: local preference becomes off and the native snapshot excludes the community, while a pending tombstone keeps a truthful warning and recovery surface visible until relay acceptance. Failed publication leaves the durable pending generation for reconnect/delayed retry; successful settlement clears it.
- The new recovery test exercises production notifier/storage mutation and proves persisted false, pending generation 8, failed deactivation, disabled snapshot, retained warning/Settings action, successful retry, pending clearance, and final hiding (
mobile/test/features/settings/notification_recovery_test.dart:24-164). - Transient capability failure has a production recovery path: app pause/resume after at least five seconds forces relay reconnect; session-state transitions rebuild
currentRelayPushDescriptorProviderand refetch NIP-11 (app_lifecycle_provider.dart:35-47;relay_session.dart:463-518;push_relay_capability_provider.dart:22-41). Community switching similarly rebuilds the active relay dependency chain.
Validation
- Both independent systems/integration and product/UI lanes approve exact head
a1dd1f7a3fb038ada3abe26f0e5b5b8c8614adbcwith clean trees. just mobile-check: PASS at exact head (609 files formatted, zero changed; analyzer clean).git diff --check: PASS.- Current-head DCO, Semgrep, zizmor, Codex security, and Desktop Release Candidate: PASS.
Clients / MobileandMobile Swift Domain / Mobile Swiftremained in progress at filing with no failing required check. Verification owner: CI/release gate; these must finish green before merge.
Confidence gaps — not author rework
- No single test drives transient NIP-11 failure through the real lifecycle reconnect into a successful second fetch; source-level dependency and state-transition tracing establishes the behavior.
- Focused Flutter/native execution and VoiceOver observation were blocked on the reviewer host by the Xcode/native-asset SDK environment. Verification owner: runnable macOS/Xcode reviewer or release environment.
- Signed IPA/device enrollment and delivery remain release-owned verification.
Author action: none. Any new head invalidates this approval.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — APPROVE
Reviewed 036215636f530a93ef5e0cddf985e579ed52e393..a1dd1f7a3fb038ada3abe26f0e5b5b8c8614adbc at the exact live head. This changed-head review replaces the expired request for changes on 25916bbe….
Risk: high — persisted push opt-in/opt-out state, relay capability discovery and reconnect recovery, native snapshot synchronization, and relay tombstone settlement.
Integrated findings
No unresolved author-actionable defect remains.
- Enabled communities retain a usable opt-out during capability loading, absence, and failure. The UI states capability unavailability without hiding the switch or permission-recovery route (
mobile/lib/features/settings/settings_page/notifications_section.dart:13-40,68-97). - A failed relay tombstone is now represented honestly: local state is off, the native snapshot excludes the community, and a durable pending generation keeps a visible warning—“Waiting for relay confirmation; notifications may continue”—plus iOS Settings recovery. Re-enable remains fenced until settlement (
notifications_section.dart:18-35,63-97;mobile/lib/shared/community/community_provider.dart:461-497,529-589;mobile/lib/shared/push/push_bridge.dart:288-330). - Production retry is bound to community/session state. Connected transitions and delayed retry process pending tombstones with an advanced generation, while active registration/publication stays off (
mobile/lib/shared/push/push_bootstrap.dart:198-255,257-340). - The prior transient-discovery concern is resolved by the existing production lifecycle: pause/resume after at least five seconds forces reconnect; session emits changing states;
currentRelayPushDescriptorProviderwatches session state and refetches NIP-11. Community switches rebuild the active-community → relay config/session → capability chain (app_lifecycle_provider.dart:35-47;relay_session.dart:463-518;push_relay_capability_provider.dart:22-41;community_provider.dart:354-368,614-627;relay_provider.dart:82-108). notification_recovery_test.dart:24-164is behaviorally causal across loading/absent/error: it proves persisted false, generation-8 pending state, attempted/failed deactivation, disabled native snapshot, retained warning/settings action, disabled switch, successful retry, journal clearance, and final hiding. The changed branches are assertion-bound rather than decorative.- Disabled + incapable + no pending tombstone remains hidden without prompting for notification permission.
Author action: none.
Validation
git diff --check 036215636f530a93ef5e0cddf985e579ed52e393..a1dd1f7a3fb038ada3abe26f0e5b5b8c8614adbc: PASS.just mobile-check: PASS at exact head in both independent lanes (609 files formatted, zero changed; analyzer clean).- Exact-head CI:
Clients / Mobile,Clients / Results,Mobile,Mobile Swift Domain / Mobile Swift,Mobile Swift Domain / Results, DCO, Codex security, Semgrep, and zizmor: PASS. - Focused local Flutter execution did not start: this reviewer host’s
objective_cnative-asset hook could not resolve the macOS SDK because the Xcode license/toolchain is unavailable. That is a reviewer-tooling confidence gap, not author rework.
Residual risk / verification owner
- No single test drives transient NIP-11 failure through the real app-lifecycle reconnect into a successful second descriptor fetch; the source-level dependency/state-transition trace supports recovery. Reviewer/tooling owns any additional integration coverage.
- Native simulator/VoiceOver, permission denial → Settings → return, and signed IPA/device enrollment/delivery were not independently observed. Reviewer/release owns those checks; App Store capability advertisement should remain off until its App Attest/APNs profile and physical-device delivery seam is release-proven.
Any new head invalidates this approval.
Main added eight commits since the last merge: mobile onboarding and push settings (#8019, #8025, #7526), desktop reads after channel writes (#7999), following a channel message before its first reply (#7692), two ACP fixes (#7568, #8022) and the macOS icon (#8018). They change buzz-acp, desktop, mobile, CI workflows and tooling only. No file is changed on both sides, and main adds no migration. The merge is textually clean and needs no follow-on edit. buzz-db, buzz-relay, migrations, schema and Cargo.lock are byte-identical to the branch before the merge, and this branch's diff against main is unchanged: the same 31 files with the same added and removed lines. Co-authored-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz> Signed-off-by: Eva <011987e296fd5006292d2f930b574be47c7801048d1983c46c425d3c95f0cffd@buzz.block.builderlab.xyz>
Hide the iOS Notifications settings section for communities that have not enabled push unless their relay advertises valid push support. A build with a configured gateway previously showed the switch even when the relay could not support push.
Communities where push is already enabled retain an off switch and iOS Settings recovery during discovery or when push support is unavailable. The section explains that support is unavailable instead of claiming notifications will arrive.
Capability discovery keeps its existing refresh behavior. Returning after at least five seconds in the background reconnects the session and fetches capability again. Tom explicitly chose this behavior without new retries, polling, or refresh triggers.
Companion client change for squareup/buzz-releases#104. Related: #7826 (broader notification availability diagnostics).
Validation
Settings widget tests cover hidden first-time opt-in during loading, absent capability, and discovery errors. Recovery tests exercise the real off switch for already-enabled communities in all three states, verifying persisted opt-out, the native snapshot writer, a durable pending relay tombstone after failure, and the iOS Settings link. They also verify that pending recovery stays visible after a relay failure and disappears only after opt-out confirmation. Tests without a gateway definition verify hidden first-time settings and disabled native registration. Mobile analysis and the mobile test suite passed.