feat(consent): in-app marketing email consent prompt + preferences toggle - #401
Conversation
…ggle Add the shared consent contract: a nullable marketingEmailConsent profile field, the marketing-consent endpoint, a setMarketingConsent mutation type, an engagement-prompt-store 'consent' kind at top priority with a cooldown-bypassing arming path, and i18n in both locales. Add a one-time consent prompt on web and mobile that shows when marketingEmailConsent is null and onboarding is complete (single-slot arbitration, records markEngagementPrompted on show), and a "Product updates by email" preferences toggle with an optimistic cache update and rollback. Shared changes are additive; the API deploys first. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review — PR #401 (feat(consent): in-app marketing email consent prompt + preferences toggle)
Reviewed via /pr-review against .claude/skills/pr-review/rubric.md. Cross-checked all 23 changed files (shared types/store/endpoints, web + mobile prompt/section components, layout arming logic, offline-mutation registration, i18n, tests).
Findings
[High] FEATURES.md not updated for the new marketing-consent preference + prompt
· dimension: 14 — FEATURES.md parity
· location: FEATURES.md (Settings & Account section, ~line 232)
· issue: This PR ships a new user-facing feature — the one-time marketing-email consent prompt and the "Product updates by email" preferences toggle (apps/web/app/(app)/preferences/_components/marketing-consent-section.tsx, apps/mobile/components/marketing-consent/marketing-consent-section.tsx) — but FEATURES.md has no row for it. The existing precedent (e.g. Proactive check-ins at FEATURES.md:62, AI daily summary toggle in the Settings & Account table) shows toggles of this kind are expected to be tracked there.
· risk: FEATURES.md is the source doc for the Play listing, landing page, and pre-launch QA matrix (per its own header). A shipped, user-visible preference silently missing from that inventory means it goes undersold/untested downstream.
· fix: Add a row to the Settings & Account table, e.g. | Marketing email consent | One-time opt-in prompt + "Product updates by email" toggle | Free | Both | en + pt-BR |.
· reference: CLAUDE.md rule "FEATURES.md parity" / rubric dimension 14
Verified clean
- Parity (web ↔ mobile): every changed file has its mirror updated with behaviorally-identical logic (arming condition, optimistic mutate/rollback, settle delay, celebration gate). Only platform adapters differ (Server Action vs
performQueuedApiMutation,AppOverlayvsBottomSheetModal). - i18n: all 8 new keys present in both
en.jsonandpt-BR.json, no orphaned callsites, brand words untranslated. - Contract (this-repo half):
packages/sharedadditions are additive-only —marketingEmailConsentis.nullable().optional()on the response schema,'setMarketingConsent'is appended (not inserted/reordered) to the mutation-type enum. Web Server Action and mobileperformQueuedApiMutationcall the same endpoint/verb/payload shape. orbit-api DTO/controller side is not verifiable in this job (sibling repo not checked out) — confirm againstorbit-api#287that the response field name/casing andPUT /api/profile/marketing-consentroute match before merge. - Backward compatibility: both the new schema field and the new enum value are additive; no rename/removal of anything an old mobile client reads. No
⚠️ breaks old mobile clientsmarker warranted. - Correctness: the arm/settle/priority logic (consent outranks review/milestone-share/referral; cooldown deliberately bypassed but
markEngagementPromptedstill recordslastPromptedAtIso; strict=== nullgate prevents re-showing after a decision) is implemented identically on both platforms and covered by new store-level tests (arbitration priority, cooldown-bypass,markEngagementPromptedside effects) plus component tests (armed/not-armed, celebration-in-flight gate, accept/decline optimistic patch, error rollback in the section tests). - Type safety / comments / console.log: no
any/as any, no narration comments (all new JSDoc is on exported components per the allowed pattern), noconsole.login the diff. - DESIGN.md: new UI uses semantic tokens only (
--primary-rgb,tintFromPrimary), no raw slate/hardcoded violet, no AI-slop tells, no em dashes in copy. - Dead code: no orphaned exports or unused imports introduced.
Recommendation
Request changes — one High finding (missing FEATURES.md row). Everything else is clean; this is a same-PR documentation fix, not a design or logic issue.
Addresses PR #401 review — track the new consent prompt + preferences toggle in the Settings & Account section. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed in 7a806a5: added the marketing-email-consent row to the Settings & Account table in FEATURES.md. |
There was a problem hiding this comment.
Code Review: PR #401 — feat(consent): in-app marketing email consent prompt + preferences toggle
Recommendation: APPROVE
Summary
Reviewed the client-side marketing-email-consent feature: shared Zod contract additions, engagement-prompt-store arbitration (new consent kind, top priority, cooldown bypass), a one-time opt-in prompt (web AppOverlay / mobile TrueSheet bottom sheet), and a non-Pro-gated preferences toggle, on both platforms, with tests. The implementation is a faithful, well-tested mirror of the existing referral/milestone-share/review-moment prompt pattern, correctly reuses the shared armed-slot arbiter, and respects DESIGN.md tokens.
Findings
Critical
None.
High
None.
Medium
Prompt activation depends on the API returning explicit null (not an omitted field) for existing users
- Location:
packages/shared/src/types/profile.ts:65(marketingEmailConsent: z.boolean().nullable().optional());apps/web/app/(app)/layout.tsx:257-258;apps/mobile/app/_layout.tsx:335-336 - Issue: Both platforms arm the consent prompt with a strict
=== nullcheck. Because the field is.nullable().optional(), an API response that omits the key (undefined) will never satisfy the gate — only an explicitnullwill. Whether the paired orbit-api PR (#287, not present in this checkout) serializes the field as explicitnullfor pre-existing accounts, versus omitting an unset nullable column, is what decides whether the prompt ever fires for the current user base. - Risk: If the API omits the key instead of emitting
null, the feature silently never triggers for any existing user — no error, no signal, just permanent silence. - Fix: Before this ships to users, confirm in orbit-api#287 that the DTO always serializes the property (no conditional/omit-on-null behavior) so unset accounts get
null. If that can't be guaranteed structurally, do a manual QA check against a pre-existing test account once both PRs are deployed.
Low / Info
None — the i18n key placement for communication/marketingEmails isn't strictly alphabetized within its hierarchy, but the surrounding file is already broadly out of alphabetical order pre-existing this PR (e.g. aboutHelp sits last in the same sections block despite starting with "a"), so this isn't a regression introduced here and isn't flagged as actionable.
Subagent verdicts
| Check | Result |
|---|---|
| Parity (web ↔ mobile) | PAIRED — all consent files, gating logic, mutation payloads, dedupeKeys, and optimistic-update/rollback match; differences are sanctioned platform adapters only (AppOverlay vs TrueSheet bottom sheet, Server Action vs direct apiClient) |
| i18n (en.json / pt-BR.json) | IN SYNC — all 8 new keys present with real translations in both locales, no orphaned callsites, brand words untranslated |
| Contract (shared types/endpoints) | MATCH on the frontend-only contract; orbit-api DTO/Controller side NOT VERIFIABLE (sibling repo not checked out in this CI sandbox) — same coordination risk as the Medium finding above |
| Security | N/A — diff contains no orbit-api code |
What's good
- Faithful, low-risk reuse of the existing engagement-prompt-store armed-slot/priority pattern instead of inventing a parallel mechanism.
- Correct, matching optimistic-update + rollback (
onMutate/onError) on both the preferences toggle and the prompt, on both platforms —previous ?? nullcorrectly preserves an explicitfalse. - Full mobile/web parity across gating logic, mutation payload, dismiss-then-reask-later semantics, and settle-delay/celebration-in-flight gating.
- i18n keys present and real in both locales.
- Not Pro-gated, matching the PR's stated intent and FEATURES.md's "Free" row.
- Solid test coverage: arbitration priority + cooldown bypass in the shared store, and prompt/section visibility, accept/decline, and rollback behavior on both platforms.
- No dead code, no banned comment patterns, no raw slate/violet hardcoding, DESIGN.md-token usage throughout.
Deferred — not verifiable in this CI sandbox
- orbit-api DTO/Controller side (thomasluizon/orbit-api#287): sibling repo not checked out here. Must confirm the
marketingEmailConsentfield is always serialized (even as explicitnull) rather than omitted for pre-existing accounts. - Phase 6 (lint/typecheck/tests): skipped per this PR's CI wrapper — Build/Unit Tests/SonarCloud run as separate required checks.
Recommendation
Safe to merge from the frontend side. Before relying on the prompt in production, confirm with the paired API PR (#287) that the marketingEmailConsent field is returned as explicit null (not an omitted key) for existing accounts predating this field, so the one-time prompt actually reaches the current user base as intended.
There was a problem hiding this comment.
Code Review: PR #401 — feat(consent): in-app marketing email consent prompt + preferences toggle
Scope: PR #401 in thomasluizon/orbit-ui-mobile (branch issue-397 → main)
Recommendation: APPROVE
Summary
Adds the client half of marketing-email consent: a one-time opt-in bottom-sheet/overlay prompt shown once onboarding completes and marketingEmailConsent === null, plus a "Product updates by email" preferences toggle, on both platforms. The new engagement-prompt-store consent kind (priority 4, highest) deliberately bypasses the shared 14-day cooldown while still recording markEngagementPrompted so sibling prompts treat it as recent. The new packages/shared schema/endpoint additions are purely additive (nullable/optional response field, brand-new request schema for a brand-new route), so old mobile clients are unaffected. Web/mobile parity, i18n, and the arbitration logic all check out; two Medium-severity cleanup items are worth addressing.
Findings
Critical
None.
High
None.
Medium
[MEDIUM] New mutation type not registered for offline-queue dedup compaction
· dimension: 1 Correctness / 3 SOLID (pattern inconsistency)
· location: orbit-ui-mobile/apps/mobile/lib/offline-queue.ts:165-181 (LAST_WRITE_WINS_TYPES)
· issue: This PR adds `setMarketingConsent` to `mutationTypeSchema` (packages/shared/src/types/sync.ts) and wires it into `inferScope` (apps/mobile/lib/offline-mutations.ts:368), following the exact same dedupeKey convention as `setAiMemory`/`setAiSummary`/`setProactiveAstra`/`setColorScheme`/`setThemePreference` (both new call sites use a fixed `dedupeKey: 'profile-marketing-consent'`, same as `profile-ai-memory`/`profile-proactive-astra` etc.). But unlike every one of those sibling types, `setMarketingConsent` was never added to `LAST_WRITE_WINS_TYPES` in offline-queue.ts, which is the only place `compactQueuedMutations` collapses same-`dedupeKey` queue entries (offline-queue.ts:207-209).
· risk: If a user is offline and toggles the setting (or answers the prompt) more than once before reconnecting, both mutations stay queued instead of collapsing to the latest one. `runQueueFlush` does process them strictly in FIFO/enqueue order (offline-mutations.ts:602-620) so the final persisted state still converges correctly — this is not a data-integrity bug — but it is an avoidable extra network round-trip, and it silently diverges from the established pattern for every other single-value profile setting, which is exactly the kind of drift the `LAST_WRITE_WINS_TYPES` set exists to prevent.
· fix: Add `'setMarketingConsent'` to the `LAST_WRITE_WINS_TYPES` set in apps/mobile/lib/offline-queue.ts (alongside `setProactiveAstra` etc.), and add a queue-compaction unit test alongside the existing offline-queue.test.ts coverage for the sibling types.
· reference: CLAUDE.md rule 10 (DRY at the right level / pattern consistency)
[MEDIUM] Fourth near-identical copy of the settle-timer + celebration-gate state machine
· dimension: 3 SOLID / clean architecture (no premature abstraction / DRY)
· location: orbit-ui-mobile/apps/mobile/components/marketing-consent/marketing-consent-prompt.tsx:535-549 and apps/web/components/marketing-consent/marketing-consent-prompt.tsx:1241-1255
· issue: `MarketingConsentPrompt` duplicates, per platform, the exact `SETTLE_DELAY_MS = 500` timer + `celebrationInFlight` gate + `markEngagementPrompted`-then-`setVisible(true)` useEffect already present in `apps/mobile/components/referral/referral-prompt.tsx:58-86`, `apps/mobile/components/milestone-share/milestone-share-prompt.tsx:64-92`, and `apps/mobile/components/review-moment/review-moment-sheet.tsx:49-77` (and their web mirrors). This is now the 4th copy of the identical state machine per platform (8 copies total across web+mobile), well past the "extract on the third real use" threshold in CLAUDE.md rule 6.
· risk: No behavioral bug today, but any future fix to the settle/celebration-gate timing (e.g. a race-condition fix, a different settle delay, or reduced-motion handling) now has to be applied identically across 4 files per platform; a future edit that only patches 3 of the 4 copies is a latent inconsistency waiting to happen.
· fix: Extract the shared piece (timer + celebration gate + mark-prompted, returning `{ visible, dismiss }`) into a small hook per platform (e.g. `apps/mobile/hooks/use-settled-engagement-prompt.ts`, mirrored in `apps/web/hooks/`) — it can't live in `packages/shared` since it needs React, but a single per-platform hook consumed by all 4 prompt components removes the quadruplicated logic. Not blocking; reasonable as a fast-follow.
· reference: CLAUDE.md rule 6 (no premature abstraction — extract on the third real use) and rule 10 (DRY at the right level)
Low / Info
None posted (signal gate: Low/Info are not surfaced on a PR review).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — every changed apps/web/** file has its apps/mobile/** mirror changed with equivalent behavior; the mobile-only touch to offline-mutations.ts is the expected offline-queue platform adapter (mobile has an offline mutation queue, web calls the Server Action directly — an already-established split, see setProactiveAstra/setAiMemory). |
| i18n-syncer | IN SYNC — all 8 new keys (profile.sections.communication, profile.marketingEmails.title/.description, marketingConsent.prompt.eyebrow/.title/.body/.accept/.decline) exist in both en.json and pt-BR.json at identical paths; every callsite resolves; no em dashes, brand words untranslated. |
| contract-aligner | MATCH (additive-only, judged from the diff) — marketingEmailConsent is added as z.boolean().nullable().optional() to the response schema and setMarketingEmailConsentRequestSchema is a brand-new schema for a brand-new endpoint; no existing field renamed/removed/newly-required, so this cannot break an already-shipped mobile client. The paired orbit-api repo (thomasluizon/orbit-api#287) is not checked out in this review sandbox, so the actual DTO/controller shape could not be directly diffed — see Deferred. |
| security-reviewer | N/A — no orbit-api files changed in this diff. |
Validation
| Check | Result |
|---|---|
| Lint | N/A — npm invocations require interactive approval that this session could not obtain; see Deferred. |
| Type check | N/A — same reason. |
| Tests | N/A — same reason. |
| Build (api) | N/A — no orbit-api changes in this diff. |
Deferred — N/A dimensions & files not verdicted
- Validation (lint/type-check/tests): could not execute
npm run lint/npm run type-check/npm testin this sandbox — everynpminvocation was blocked pending interactive approval that never arrived. The PR's own CI is the authoritative signal for these three checks; treat this table as "not run here," not "clean." - Contract drift (dimension 11) vs actual orbit-api DTOs: the sibling
orbit-apirepo is not checked out in this environment, so the field-by-field comparison against the real C# DTOs/controller for PR thomasluizon/orbit-api#287 could not be performed directly. The MATCH verdict above is based on reading this diff's additive-only shape, not a live cross-repo diff. - Security (dimension 12) / Backend hard rules (dimension 13): N/A — this diff touches only
orbit-ui-mobile(frontend + shared); noorbit-apicode changed. - DESIGN.md / AI-slop (dimension 8): checked — the prompt reuses the house bottom-sheet/overlay pattern (eyebrow, icon disc via
tintFromPrimary/rgba(var(--primary-rgb),…), body, pill CTA + text "decline") already used byReferralPrompt/MilestoneSharePrompt; no raw slate refs, no hardcoded violet, no gradient decoration, no cards-in-cards. No findings. - Every other changed file (FEATURES.md, the 4 new component files, the 4 new test files,
_layout.tsx/layout.tsxwiring,preferences-sections.tsx/preference-settings-list.tsx,profile.tsaction,offline-mutations.ts,lucide-react-native.tsmock,endpoints.ts,en.json/pt-BR.json,engagement-prompt-store.ts/.test.ts,stores/index.ts,types/profile.ts,types/sync.ts) was read and given a verdict above; nothing was silently skipped.
What's good
- The priority-4 consent arbitration and cooldown-bypass-but-still-records-prompted-timestamp design is subtle and well tested (
packages/shared/src/__tests__/engagement-prompt-store.test.tscovers both the outranking behavior and the "keeps armed consent when a lower-priority prompt arms after" ordering-independence case). - The strict
=== nullre-arming gate combined with backdrop-dismiss-leaves-undecided is a clean way to guarantee the prompt never re-shows after an explicit answer while still re-asking after an accidental dismiss. - Both platforms' toggle sections implement the identical optimistic-update + rollback-on-error mutation pattern, matching the existing
setProactiveAstra/setAiMemorysections exactly. FEATURES.mdwas updated in the same PR with accurate Gating/Platform/Locale columns.
Recommendation
Safe to merge once the paired orbit-api PR (#287) is live, per the PR's own stated deploy order. The two Medium findings (offline-queue dedup registration, settle-timer duplication) are non-blocking cleanup — fine to land as fast-follows rather than blocking this PR.
# Conflicts: # apps/mobile/app/_layout.tsx # packages/shared/src/types/sync.ts
There was a problem hiding this comment.
Code Review: PR #401 — orbit-ui-mobile
Scope: PR #401 in thomasluizon/orbit-ui-mobile — feat(consent): in-app marketing email consent prompt + preferences toggle
Recommendation: NEEDS WORK
Summary
This PR adds the client half of marketing-email consent: a one-time, cooldown-bypassing, top-priority in-app prompt (web AppOverlay / mobile TrueSheet-backed BottomSheetModal) shown once when marketingEmailConsent === null and onboarding is complete, plus a non-Pro-gated "Product updates by email" preferences toggle, both wired through the shared engagement-prompt-store arbiter with full web/mobile parity, i18n, and tests. The implementation itself is correct and well-tested; the one blocking issue is a factual error in the FEATURES.md row this PR adds, which describes an architecture that was explicitly not built (and, per the paired backend PR, was actively removed).
Findings
Critical
None.
High
[HIGH] FEATURES.md row describes Resend segment sync that does not exist
· dimension: FEATURES.md parity (#14)
· location: orbit-ui-mobile/FEATURES.md:240
· issue: The new row reads: "One-time opt-in prompt + \"Product updates by email\" toggle; syncs to a Resend segment and honors unsubscribes both directions." This PR's own body states the opposite: "Consent is stored in the app database (the single source of truth the backend code-sender reads); there is no Resend-side contact/segment sync." Independently verified against the paired backend PR (thomasluizon/orbit-api#287), which explicitly *removes* "the now-dead Resend segment sync, contact.updated webhook, ResendWebhookVerifier, and the Svix dependency" in favor of a DB-only, code-based sender. All three sources (this PR's diff, this PR's body, and the paired API PR's body) agree: there is no Resend segment sync in the shipped design.
· risk: FEATURES.md is documented as the code-derived, authoritative feature inventory that other engineers, future PRs, and other Orbit review/audit skills read as ground truth. A misleading claim here about where consent data flows (a third-party marketing platform vs. DB-only) is exactly the kind of statement that matters for privacy/LGPD documentation accuracy and will misdirect anyone building on or auditing this feature later.
· fix: Correct the row's description to match the shipped design, e.g.: "One-time opt-in prompt + \"Product updates by email\" toggle; consent stored in-app (DB is the source of truth for the code-based sender), self-hosted unsubscribe flips it back." Keep Gating/Platform/Locale columns as-is — those are accurate.
· reference: rubric.md dimension 14 (FEATURES.md parity)
Medium
None concretely actionable beyond the above.
Low / Info
[INFO] Contract changes are additive and forward-compatible
· dimension: Contract drift + backward-compat guard (#11)
· location: packages/shared/src/types/profile.ts:66 (marketingEmailConsent: z.boolean().nullable().optional()), :95-99 (new setMarketingEmailConsentRequestSchema), packages/shared/src/types/sync.ts:10 (new 'setMarketingConsent' enum value), packages/shared/src/api/endpoints.ts:31 (new marketingConsent endpoint)
· issue: None — noted for completeness.
· risk: No risk. New optional/nullable response field, a brand-new request schema, and a new enum value are all additive; nothing is renamed or removed, so already-shipped Android clients running a frozen @orbit/shared snapshot are unaffected.
· fix: N/A.
· reference: CLAUDE.md "Backward compatibility (append-only + deploy-order)"
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — mobile and web marketing-consent-prompt.tsx / marketing-consent-section.tsx are behaviorally identical (arming condition, 500ms settle delay, celebration-in-flight gate, cooldown-bypass, optimistic mutation + rollback, dedupeKey); only platform adapters differ as expected. |
| i18n-syncer | IN SYNC — all 8 new keys (profile.sections.communication, profile.marketingEmails.title/description, marketingConsent.prompt.eyebrow/title/body/accept/decline) present in both en.json and pt-BR.json with genuine (non-copy-pasted) Portuguese translations; no orphaned callsites; brand words untranslated. |
| contract-aligner | NOT VERIFIABLE IN CI — packages/shared/src/types/profile.ts and endpoints.ts changed, which would normally gate this in, but the sibling orbit-api repo is not checked out in this job. Cross-checked manually against the paired backend PR's stated body instead (see the High finding above); the DTO/route-level field-by-field comparison itself was not performed. |
| security-reviewer | N/A — no orbit-api code in this diff. |
Validation
| Check | Result |
|---|---|
| Lint | N/A — skipped per CI adaptation (separate required check runs Build/Unit Tests/SonarCloud) |
| Type check | N/A — skipped per CI adaptation |
| Tests | N/A — skipped per CI adaptation |
| Build (api) | N/A — orbit-api not touched, not checked out |
Deferred — N/A dimensions & files not verdicted
- Backend hard rules (#13) — N/A, no
orbit-apifiles in this diff. - Security (#12), API side — N/A, no
orbit-apifiles in this diff; frontend-security categories (XSS, auth-state leakage) were checked directly and found clean (mutations route through the existingserverAuthFetch/performQueuedApiMutationauth paths, no new client-side trust boundary introduced). - contract-aligner's DTO-level field comparison — not verifiable in this CI job (sibling
orbit-apirepo not checked out); substituted with a manual cross-check against the paired PR's stated body/description, which is how the FEATURES.md finding above was surfaced. - Phase 7 (Validate: lint/typecheck/tests/build) — skipped per this workflow's CI adaptation; a separate required Build/Unit Tests/SonarCloud check covers this.
- No changed file was left without a verdict — all 24 files touched by this PR (components, tests, layouts, shared store/types/i18n/endpoints, FEATURES.md) were reviewed.
What's good
- Full, verified web/mobile parity on both new components, down to the exact 500ms settle delay and the intentional cooldown-bypass (the one deliberate behavioral difference from the sibling referral/milestone-share/review prompts, and it's applied identically on both platforms).
- Thoughtful engagement-prompt-store extension: consent given top arbiter priority (4) without disturbing the existing referral/milestone-share/review ordering, and
markEngagementPromptedstill clears the armed slot on show so it doesn't permanently block lower-priority prompts. - i18n added to both locales in the same commit, real Portuguese translations (not copy-pasted English).
- Solid test coverage: engagement-prompt-store arbitration + cooldown-bypass, and per-platform prompt visibility/answer + section reflect/optimistic-update/rollback — behavior-focused, not implementation-detail tests.
- Correct backward-compat hygiene: every shared-type change in this diff is additive (nullable/optional field, new schema, new enum value); nothing renamed or removed.
- Clean on comments/type-safety/console.log/DESIGN.md tokens — no narration comments, no
any, no hardcoded colors or banned utility classes in the new UI.
Recommendation
Fix the FEATURES.md:240 description (drop the "syncs to a Resend segment" claim, replace with the actual DB-only / code-sender architecture) before merge. Everything else in this PR is solid and ready to ship once that one line is corrected.
The row claimed "syncs to a Resend segment"; the shipped design stores consent in-app (DB is the source of truth for the code-based sender) with a self-hosted unsubscribe. Matches the paired orbit-api#287 rework. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Code Review: PR #401 — feat(consent): in-app marketing email consent prompt + preferences toggle
Recommendation: APPROVE
Summary
This is a re-review focused on the diff since my last CHANGES_REQUESTED pass (2026-07-06T18:13:21Z), which found one blocking High finding: FEATURES.md:240 incorrectly claimed the feature "syncs to a Resend segment," contradicting both this PR's own description and the paired backend PR's removal of Resend segment sync.
That finding is fixed. Commit 7f9740226ecf38016cb5768b7814f0321aa4defe ("docs: correct marketing-consent FEATURES row to DB-only sender") rewrites the row to: "consent stored in-app (DB is the source of truth for the code-based sender), self-hosted unsubscribe flips it back" — accurate, matches the PR body and orbit-api#287, no duplicate/leftover row.
The remaining commits since then are merges from main (e64fc312 e2e fix, 06451309 onboarding feature, 89c07cda achievements fix) plus one real conflict resolution in apps/mobile/app/_layout.tsx and packages/shared/src/types/sync.ts. Verified the resolution keeps both branches' additions intact — armConsentPrompt/marketingEmailConsent wiring from this PR and pendingOnboardingAnswers/dismissImportPrompt from main are both present, nothing dropped.
Findings
Critical / High
None surviving.
Medium / Low / Info
None new. The two Medium findings from an earlier review pass (offline-queue LAST_WRITE_WINS_TYPES registration for setMarketingConsent; settle-timer/celebration-gate duplication across 4 prompt components) were already surfaced, explicitly judged non-blocking fast-follows in a prior APPROVED review, and are unchanged by the diff since then — not re-flagged per the signal gate.
Subagents (this pass)
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — all web/mobile file pairs behaviorally identical (arming condition, settle delay, optimistic mutation + rollback); only sanctioned platform adapters differ. |
| i18n-syncer | IN SYNC — all 8 keys present in both en.json/pt-BR.json (3,300 keys each), no orphans, brand words untranslated. |
| contract-aligner | MATCH (frontend-side) — marketingEmailConsent additive/nullable/optional, setMarketingConsent appended (not inserted) to the enum, single endpoint constant, no hardcoded paths. orbit-api DTO/controller side not verifiable in this CI job (sibling repo not checked out) — same deferred item as prior reviews. |
| security-reviewer | N/A — no orbit-api files in this diff. |
Deferred — not verifiable in CI
- orbit-api DTO/controller shape (orbit-api#287) — sibling repo not checked out.
- Phase 6 (lint/typecheck/tests) — skipped per this PR's CI wrapper; Build/Unit Tests/SonarCloud run as separate required checks.
Recommendation
Approve. The sole blocking finding from the prior review is correctly fixed, and nothing new was introduced by the subsequent merge/conflict-resolution commits.
There was a problem hiding this comment.
PR Review — #401 (feat(consent): in-app marketing email consent prompt + preferences toggle)
Recommendation: Request changes
| Severity | Count |
|---|---|
| Critical | 0 |
| High | 1 |
| Medium | 0 |
| Low / Info | 3 |
High — Marketing-consent prompt omits the canPromptEngagement re-show guard every sibling prompt uses
apps/mobile/components/marketing-consent/marketing-consent-prompt.tsx:46-76andapps/web/components/marketing-consent/marketing-consent-prompt.tsx:38-76gate display only onisArmed(armedPrompt?.kind === 'consent') andcelebrationInFlight.- The three sibling engagement prompts —
ReferralPrompt(.../referral/referral-prompt.tsx, viacanPromptReferral, an alias ofcanPromptEngagement),MilestoneSharePrompt(.../milestone-share/milestone-share-prompt.tsx:70-76), andReviewMomentSheet(.../review-moment/review-moment-sheet.tsx:57) — all additionally checkcanPromptEngagement(store, key, now)before starting the settle timer, and callclearArmedMilestone()if the check fails. Verified this pattern is consistent on both mobile and web. canPromptEngagement(packages/shared/src/stores/engagement-prompt-store.ts:144-156) returnsfalseoncepromptedMilestoneKeysalready contains the milestone key. The marketing-consent prompt does callmarkEngagementPrompted(MARKETING_CONSENT_MILESTONE_KEY, ...)on first display (adding it to that list), but never re-checks the list before allowing a second display.- The arming effects (
apps/mobile/app/_layout.tsx:302-306, mirrored on web) re-arm purely offmarketingEmailConsent === null, with nopromptedMilestoneKeyscheck either. So ifmarketingEmailConsentreverts tonullfor any reason after a user already answered — e.g., the offline-queuedsetMarketingConsentmutation (performQueuedApiMutation) never lands and a later profile refetch (staleTime-based,apps/mobile/hooks/use-profile.ts:30) overwrites the optimistic patch back tonull— the prompt re-arms and, missing the guard its siblings have, reappears to a user who already answered. This contradicts the component's own docstring claim that "the strict=== nullarming gate prevents it from ever re-showing." - Fix: add the same
canPromptEngagement(...)check (already exported from@orbit/shared/stores) to the arm/display path on both platforms, matchingMilestoneSharePrompt/ReviewMomentSheet/ReferralPrompt.
Low / Info
- orbit-api DTO/controller side of the contract (paired PR
orbit-api#287) — not verifiable, sibling repo unavailable in this CI environment. - Dimensions 12/13 (backend security/hard rules) — N/A, no backend files touched in this diff.
- Dimension 14 (headline-feature-guide cross-check) — N/A, marketing consent isn't a headline-set feature.
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — web/mobile mirrors are behaviorally identical; only platform adapters differ |
| i18n-syncer | IN SYNC — all 8 new keys present + translated in en/pt-BR, no orphans |
| contract-aligner | MATCH (in-repo self-consistency); orbit-api DTO side not verifiable — sibling repo not checked out |
| security-reviewer | N/A — no orbit-api changes in this diff |
What's good
Shared-package changes are cleanly additive (nullable+optional field, new request schema, additive enum value — no backward-compat break). Web/mobile parity is genuinely strong (identical timing, i18n keys, mutation shape). DESIGN.md compliance is clean (semantic tokens only, no raw slate/violet, matches the existing overlay/bottom-sheet pattern). Good arbitration/cooldown-bypass test coverage in engagement-prompt-store.test.ts. FEATURES.md updated accurately, and the cross-repo deploy-order dependency is explicitly called out in the PR body.
Note: /pr-review's Phase 6 (/validate) skipped per CI wrapper instructions — Build/Unit Tests/SonarCloud run as separate required checks on this PR.
| const previous = profile?.marketingEmailConsent ?? null | ||
| patchProfile({ marketingEmailConsent: enabled }) | ||
| return { previous } | ||
| }, | ||
| onError: ( | ||
| _err: unknown, | ||
| _enabled: boolean, | ||
| context: { previous?: boolean | null } | undefined, | ||
| ) => { | ||
| patchProfile({ marketingEmailConsent: context?.previous ?? null }) | ||
| }, | ||
| }) | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
High: This effect only gates on isArmed/celebrationInFlight. Every sibling engagement prompt (ReferralPrompt via canPromptReferral, MilestoneSharePrompt, ReviewMomentSheet) additionally checks canPromptEngagement(store, key, now) before starting the settle timer, and bails via clearArmedMilestone() if it fails. Here, if marketingEmailConsent reverts to null after the user already answered (e.g. the offline-queued setMarketingConsent mutation never lands and a later profile refetch overwrites the optimistic patch), armConsentPrompt re-arms this prompt and it will re-display — even though MARKETING_CONSENT_MILESTONE_KEY is already in promptedMilestoneKeys. Add the same canPromptEngagement(...) guard used by the sibling prompts.
| if (visible || !isArmed || celebrationInFlight) return | ||
|
|
||
| settleTimerRef.current = setTimeout(() => { | ||
| markEngagementPrompted( | ||
| MARKETING_CONSENT_MILESTONE_KEY, | ||
| new Date().toISOString(), | ||
| ) | ||
| setVisible(true) | ||
| }, SETTLE_DELAY_MS) | ||
|
|
||
| return () => { | ||
| if (settleTimerRef.current) clearTimeout(settleTimerRef.current) | ||
| } | ||
| }, [isArmed, celebrationInFlight, visible, markEngagementPrompted]) |
There was a problem hiding this comment.
High: Same gap as the mobile mirror — this effect only gates on isArmed/celebrationInFlight, missing the canPromptEngagement(...) re-show guard every sibling prompt (ReferralPrompt, MilestoneSharePrompt, ReviewMomentSheet) uses. If marketingEmailConsent reverts to null after the user already answered, this will re-arm and re-display despite MARKETING_CONSENT_MILESTONE_KEY already being in promptedMilestoneKeys.
Mobile tsc was never run in CI, which let test-only type errors reach main (from #401's marketing-consent tests and a template-packs mock). Add a Type Check job running `npx turbo run type-check` (web + mobile + shared). Fix the two pre-existing mobile test type errors so the gate is green: - template-packs: type the createHabitsBulk mock signature so mock.calls[0] is a populated tuple (drops an unneeded cast). - marketing-consent: narrow getSwitch to throw when the stub is absent instead of returning RenderedNode | undefined. turbo run type-check: 3/3 workspaces green. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
Adds the client half of marketing-email consent (both platforms). The backend that consumes this consent is the paired API PR thomasluizon/orbit-api#287 (a code-based marketing sender + admin concept + self-hosted unsubscribe) — merge/deploy that first.
marketingEmailConsentprofile field,/api/profile/marketing-consentendpoint,setMarketingConsentsync type, engagement-prompt-storeconsentkind (top priority, cooldown-bypassing arming path), i18n in en + pt-BR.marketingEmailConsent === nulland onboarding is complete; single-slot arbitration, recordsmarkEngagementPromptedon show, bypasses the 14-day cooldown. Yes → opt-in, No → opt-out; strict=== nullgate means it never re-shows after a decision.Consent is stored in the app database (the single source of truth the backend code-sender reads); there is no Resend-side contact/segment sync. All shared changes are additive; old mobile clients are unaffected.
Tests: engagement-prompt-store consent arbitration + cooldown bypass; web + mobile prompt visibility/answer and section reflect/optimistic/rollback.
Closes #397
API PR: thomasluizon/orbit-api#287
🤖 Generated with Claude Code