Repository navigation
Prepare iOS App Store review readiness - #7862
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR adds App Store review setup and release automation, blocks App Store billing flows, introduces opt-out mobile analytics with consent-generation gating, reworks crash reporting revocation, expands privacy disclosures, and deletes PostHog account records during account deletion. ChangesApp Store review readiness
Mobile analytics consent
Crash reporting lifecycle
Privacy disclosures and account deletion
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Settings
participant ConsentObserver
participant AnalyticsEmitter
participant AnalyticsUploader
Settings->>ConsentObserver: Publish telemetry consent change
ConsentObserver->>AnalyticsEmitter: Synchronize consent generation
AnalyticsEmitter->>AnalyticsUploader: Enable or cancel uploads
sequenceDiagram
participant ConsentProvider
participant MobileCrashRevocationWatcher
participant MobileCrashReporter
participant TransportSession
ConsentProvider->>MobileCrashRevocationWatcher: Report consent transition
MobileCrashRevocationWatcher->>MobileCrashReporter: Invoke lifecycle action
MobileCrashReporter->>TransportSession: Start or invalidate session
sequenceDiagram
participant AccountRoute
participant PostHog
participant Stack
AccountRoute->>PostHog: Delete account person
PostHog-->>AccountRoute: Return deletion response
AccountRoute->>Stack: Delete Stack user
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR prepares the iOS App Store review flow and updates related privacy and billing behavior.
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (40): Last reviewed commit: "Accept already-deleted analytics identit..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
azooz2003-bit
force-pushed
the
task-ios-appstore-review-readiness
branch
from
July 10, 2026 22:37
74c4477 to
0a8fa0f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/app/`[locale]/(legal)/privacy-policy/page.tsx:
- Around line 24-30: Clarify the Sparkle update statement so it applies only to
the cmux macOS application, not the iPhone and iPad application. Update the
paragraph containing the Sparkle reference to explicitly say that macOS
Application updates are checked via Sparkle, while leaving iOS updates excluded
from that claim.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a4cd4490-16fb-41af-bb41-07497e9d98b4
📒 Files selected for processing (9)
ios/AppStoreReview/README.mdios/AppStoreReview/metadata-screenshots-checklist.mdios/AppStoreReview/review-notes.mdios/AppStoreReview/reviewer-setup.mdweb/app/[locale]/(legal)/privacy-policy/page.tsxweb/app/api/billing/checkout/route.tsweb/app/api/billing/portal/route.tsweb/app/lib/billing.tsweb/tests/billing-portal-route.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/app/[locale]/(legal)/privacy-policy/page.tsx (1)
67-70: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSparkle references still say "The Application" after expanding the definition to include iOS.
The past review flagged line 68 and was marked addressed, but the current code still reads "The Application checks for updates via Sparkle." Since lines 25–27 now define "Application" to include the iPhone and iPad app, this statement is inaccurate — iOS apps update through the App Store, not Sparkle. The same ambiguity applies to the Sparkle entry in the third-party services list at line 148.
🔧 Proposed fix
<p> - The Application checks for updates via Sparkle, which may transmit your + The macOS Application checks for updates via Sparkle, which may transmit your operating system version and application version to our update server. </p>And for the third-party services list:
<li> - <strong>Sparkle</strong> — auto-update framework. Transmits - application and OS version to check for updates. + <strong>Sparkle</strong> — macOS auto-update framework. Transmits + application and OS version to check for updates on macOS. </li>Also applies to: 148-150
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/`[locale]/(legal)/privacy-policy/page.tsx around lines 67 - 70, Clarify both Sparkle references in the privacy policy by limiting them to the macOS application rather than the broadly defined “Application.” Update the update-check paragraph and the Sparkle third-party-services entry so they explicitly state that Sparkle is used by the macOS app, while iOS updates are handled separately through the App Store.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/app/`[locale]/(legal)/privacy-policy/page.tsx:
- Around line 67-70: Clarify both Sparkle references in the privacy policy by
limiting them to the macOS application rather than the broadly defined
“Application.” Update the update-check paragraph and the Sparkle
third-party-services entry so they explicitly state that Sparkle is used by the
macOS app, while iOS updates are handled separately through the App Store.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0c270db8-b648-4dc2-8ab8-cb6687dd1d21
📒 Files selected for processing (10)
ios/AppStoreReview/README.mdios/AppStoreReview/metadata-screenshots-checklist.mdios/AppStoreReview/review-notes.mdios/AppStoreReview/reviewer-setup.mdweb/app/[locale]/(legal)/privacy-policy/page.tsxweb/app/api/billing/checkout/route.tsweb/app/api/billing/portal/route.tsweb/app/lib/billing.tsweb/tests/billing-checkout-route.test.tsweb/tests/billing-portal-route.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/app/[locale]/(legal)/privacy-policy/page.tsx (1)
14-14: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftLocalize the updated privacy-policy copy.
This
[locale]page adds substantial user-facing English literals without anext-intl/catalog lookup. Non-English routes will therefore show English, and the new disclosures are missing from every supported locale catalog. Move the changed strings to locale-specific messages and add matching entries for all locales.As per coding guidelines and path instructions, user-facing web text must use a locale-specific runtime source and be represented in every supported locale.
Also applies to: 24-30, 45-47, 57-102, 115-131, 146-162, 177-181, 259-260
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/`[locale]/(legal)/privacy-policy/page.tsx at line 14, Localize all newly added user-facing text in the privacy-policy page, including the “Last updated” text and disclosures at the referenced sections, using the project’s next-intl/runtime message lookup pattern. Add matching message keys and translations to every supported locale catalog, then replace the hardcoded English literals in the page with those localized keys.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/app/`[locale]/(legal)/privacy-policy/page.tsx:
- Line 14: Localize all newly added user-facing text in the privacy-policy page,
including the “Last updated” text and disclosures at the referenced sections,
using the project’s next-intl/runtime message lookup pattern. Add matching
message keys and translations to every supported locale catalog, then replace
the hardcoded English literals in the page with those localized keys.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f6d92d4-efee-4094-b1f7-866b910f2ee2
📒 Files selected for processing (4)
ios/AppStoreReview/review-notes.mdios/AppStoreReview/reviewer-setup.mdweb/app/[locale]/(legal)/privacy-policy/page.tsxweb/app/api/billing/portal/route.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/app/[locale]/(legal)/privacy-policy/page.tsx (2)
14-14: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSynchronize the sitemap modification date.
The page now displays July 10, 2026, but
web/app/sitemap.ts:86-93still reports/privacy-policyas last modified on March 18, 2026. Update the sitemap metadata with the policy change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/`[locale]/(legal)/privacy-policy/page.tsx at line 14, Update the `/privacy-policy` entry in the `sitemap` function to use July 10, 2026 as its `lastModified` date, matching the date displayed on the privacy policy page.
24-30: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftLocalize the newly changed privacy-policy copy.
This
[locale]page adds substantial hard-coded English text instead of reading fromnext-intlor another locale-specific source. Add catalog keys and translations for every locale inweb/i18n/routing.ts; the same applies to the changed retention, analytics, permission, and third-party-service text.As per coding guidelines, user-facing web text must use a locale-specific runtime source and be represented for every supported locale. As per path instructions,
web/**/*.{ts,tsx,md,mdx,json}must apply full internationalization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/app/`[locale]/(legal)/privacy-policy/page.tsx around lines 24 - 30, The privacy-policy page’s newly added English copy is hard-coded and not localized. Update the privacy-policy page component and its changed retention, analytics, permission, and third-party-service sections to read all user-facing text through next-intl (or the project’s established locale source), add catalog keys for every string, and provide translations for every locale defined in the routing configuration.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@web/app/`[locale]/(legal)/privacy-policy/page.tsx:
- Line 14: Update the `/privacy-policy` entry in the `sitemap` function to use
July 10, 2026 as its `lastModified` date, matching the date displayed on the
privacy policy page.
- Around line 24-30: The privacy-policy page’s newly added English copy is
hard-coded and not localized. Update the privacy-policy page component and its
changed retention, analytics, permission, and third-party-service sections to
read all user-facing text through next-intl (or the project’s established locale
source), add catalog keys for every string, and provide translations for every
locale defined in the routing configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47755d4d-3f1c-4cd2-86d0-529048db2ebd
📒 Files selected for processing (3)
ios/AppStoreReview/review-notes.mdios/AppStoreReview/reviewer-setup.mdweb/app/[locale]/(legal)/privacy-policy/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ios/AppStoreReview/metadata-screenshots-checklist.md`:
- Line 58: Update the checklist entry to say that in-app Delete Account deletes
account-linked PostHog analytics before deleting the Stack user, matching the
awaited deletePostHogPersonForAccountDeletion(...) behavior.
In `@ios/AppStoreReview/reviewer-setup.md`:
- Around line 37-40: Replace the generic `100.64.0.0/10` CIDR guidance in the
`App Review Mac` setup entry with the prepared Mac’s specific Tailscale
`100.64.x.x` address or its concrete MagicDNS hostname, so reviewers have an
actual host to connect to.
In `@ios/cmux/Resources/PrivacyInfo.xcprivacy`:
- Around line 1-29: Add an NSPrivacyCollectedDataTypes array to the privacy
manifest, declaring the analytics and crash telemetry data collected by the app
with their applicable collection, tracking, and purpose details. Ensure the
entries use Apple’s required privacy manifest keys and accurately reflect the
telemetry payloads, or provide equivalent declarations through the telemetry
packages.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift`:
- Around line 8-13: Replace the new static-only MobileTelemetryDefaults
namespace with an immutable key defined on the owning settings type or existing
shared settings contract. Update all references to use that owner, preserving
the exact sendAnonymousTelemetry key value and avoiding new ambient production
globals.
In `@web/app/`[locale]/(legal)/privacy-policy/page.tsx:
- Around line 98-105: Update the mobile analytics disclosure to accurately state
that telemetry is disabled by default and only collected after the user
explicitly enables the Send anonymous telemetry setting. Revise the paragraph in
the privacy policy page so it no longer claims analytics are sent unless
disabled, while retaining the opt-out and deletion-request details.
In `@web/app/api/account/route.ts`:
- Around line 639-659: Add a bounded timeout to the PostHog fetch in the
account-deletion flow, using an AbortController or equivalent AbortSignal and
ensuring the timer is cleaned up after completion. Preserve the existing non-OK
response handling and allow timeout errors to propagate through the existing
retryable-error path; update the fetch call associated with the account deletion
mutation before afterExternalMutation is invoked.
- Around line 661-690: postHogPersonDeletionConfig currently falls back to
POSTHOG_DEFAULT_ENVIRONMENT_ID for destructive deletions. Require a non-empty
explicit POSTHOG_ENVIRONMENT_ID or POSTHOG_PROJECT_ID, remove the default-ID
fallback, and preserve the existing error when neither is configured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 933f7b62-61c4-4547-b14f-fe495552a471
📒 Files selected for processing (12)
Packages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsConsentProviding.swiftPackages/iOS/CmuxMobileAnalytics/Tests/CmuxMobileAnalyticsTests/AnalyticsEmitterTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swiftios/AppStoreReview/metadata-screenshots-checklist.mdios/AppStoreReview/review-notes.mdios/AppStoreReview/reviewer-setup.mdios/cmux-ios.xcodeproj/project.pbxprojios/cmux/Resources/Localizable.xcstringsios/cmux/Resources/PrivacyInfo.xcprivacyweb/app/[locale]/(legal)/privacy-policy/page.tsxweb/app/api/account/route.tsweb/tests/account-route.test.ts
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…iew-readiness # Conflicts: # web/app/[locale]/(legal)/privacy-policy/page.tsx # web/proxy.ts
| publish: @Sendable (AnalyticsConsentSnapshot) -> Void | ||
| ) -> AnalyticsConsentSnapshot { | ||
| state.withCriticalRegion { state in | ||
| guard state.generation == base.generation else { return base } |
There was a problem hiding this comment.
Stale consent admits post-revoke events
Medium Severity
When synchronize sees a generation mismatch, it returns the caller’s original base snapshot unchanged. If another thread already revoked consent, that stale snapshot can still have isEnabled true even though the fresh UserDefaults read was false. capture then only checks consent.isEnabled and enqueues the event into the AsyncStream after opt-out, which breaks the fail-closed “do not buffer while disabled” gate until the consumer later drops it via allows.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 9f3d8d5. Configure here.
added 6 commits
July 12, 2026 21:06
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c99ed1a. Configure here.
| default: | ||
| true | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicated crash reporting flag logic
Medium Severity
The CMUXCrashReportingEnabled Info.plist parser is copied in both AppCompositionRoot and MobileSettingsView. One copy decides whether Sentry is armed; the other decides whether Settings promises crash reports. If those parsers diverge later, the toggle label can advertise crash sharing while the composition root never starts reporting, or the reverse.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit c99ed1a. Configure here.
azooz2003-bit
enabled auto-merge (squash)
July 13, 2026 04:54
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@Packages/iOS/CmuxMobileAnalytics/Tests/CmuxMobileAnalyticsTests/AnalyticsEmitterTests.swift`:
- Around line 44-62: Update the makeEmitter test helper to require an explicitly
injected NotificationCenter instead of defaulting to NotificationCenter.default.
Preserve existing call sites by passing their isolated local centers, and ensure
every invocation supplies a per-test NotificationCenter so observers cannot use
shared global state.
In
`@Packages/iOS/CmuxMobileCrashReporting/Sources/CmuxMobileCrashReporting/MobileCrashRevocationWatcher.swift`:
- Around line 46-57: Update the observer closure in MobileCrashRevocationWatcher
to capture self weakly and guard that it still exists before dispatching to
lifecycleQueue. Preserve the existing consent-change handling and state updates,
while ensuring the NotificationCenter observer cannot retain the watcher and
prevent deinit from removing it.
In
`@Packages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/CrashTestCounter.swift`:
- Around line 17-19: Update CrashTestCounter.waitForValue to use a bounded
timeout while waiting for value to reach target, matching the test suite’s
existing DispatchSemaphore timeout pattern. Ensure the method exits or fails
clearly when the timeout expires instead of looping indefinitely.
In
`@Packages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/CrashTestSequenceRecorder.swift`:
- Around line 21-23: Update CrashTestSequenceRecorder.waitForCount to use the
same bounded timeout behavior as CrashTestCounter.waitForValue. Ensure it fails
when the sequence does not reach the requested count within the timeout instead
of waiting indefinitely, while preserving the existing wait behavior when
progress is expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3ad51ba8-baf0-47c5-acfd-1a1e7186c313
📒 Files selected for processing (31)
.github/workflows/ios-app-store.ymlPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsConsentGenerationGate.swiftPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsConsentProviding.swiftPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsConsentRevocationObserver.swiftPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsConsentSnapshot.swiftPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsCriticalState.swiftPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsEmitter.swiftPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsPendingEvent.swiftPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsUploadStartGate.swiftPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsUploadTaskRegistry.swiftPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/AnalyticsUploading.swiftPackages/iOS/CmuxMobileAnalytics/Sources/CmuxMobileAnalytics/HTTPAnalyticsUploader.swiftPackages/iOS/CmuxMobileAnalytics/Tests/CmuxMobileAnalyticsTests/AnalyticsEmitterTests.swiftPackages/iOS/CmuxMobileAnalytics/Tests/CmuxMobileAnalyticsTests/AnalyticsUploadTaskRegistryTests.swiftPackages/iOS/CmuxMobileAnalytics/Tests/CmuxMobileAnalyticsTests/BlockingAnalyticsTokenProvider.swiftPackages/iOS/CmuxMobileAnalytics/Tests/CmuxMobileAnalyticsTests/BlockingAnalyticsUploader.swiftPackages/iOS/CmuxMobileAnalytics/Tests/CmuxMobileAnalyticsTests/ConsentAwareRecordingUploader.swiftPackages/iOS/CmuxMobileAnalytics/Tests/CmuxMobileAnalyticsTests/HTTPAnalyticsUploaderTests.swiftPackages/iOS/CmuxMobileAnalytics/Tests/CmuxMobileAnalyticsTests/TestGate.swiftPackages/iOS/CmuxMobileCrashReporting/Sources/CmuxMobileCrashReporting/CrashLifecycleAction.swiftPackages/iOS/CmuxMobileCrashReporting/Sources/CmuxMobileCrashReporting/MobileCrashReporter.swiftPackages/iOS/CmuxMobileCrashReporting/Sources/CmuxMobileCrashReporting/MobileCrashRevocationWatcher.swiftPackages/iOS/CmuxMobileCrashReporting/Sources/CmuxMobileCrashReporting/MobileCrashTransportSessionController.swiftPackages/iOS/CmuxMobileCrashReporting/Sources/CmuxMobileCrashReporting/MobileCrashTransportSessionControlling.swiftPackages/iOS/CmuxMobileCrashReporting/Sources/CmuxMobileCrashReporting/SentryCachePurger.swiftPackages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/CrashTestCounter.swiftPackages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/CrashTestSequenceRecorder.swiftPackages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/CrashTestToggleConsent.swiftPackages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/CrashTestTransportController.swiftPackages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/MobileCrashReporterTests.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/MobileSettingsView.swift
Comment on lines
44
to
62
| private func makeEmitter( | ||
| uploader: RecordingAnalyticsUploader, | ||
| uploader: any AnalyticsUploading, | ||
| consent: (any AnalyticsConsentProviding)? = nil, | ||
| consentEnabled: Bool = true, | ||
| anonymousID: String = "anon-1", | ||
| flushBatchSize: Int = 50, | ||
| maxPendingEvents: Int = 1000 | ||
| maxPendingEvents: Int = 1000, | ||
| notificationCenter: NotificationCenter = .default | ||
| ) -> AnalyticsEmitter { | ||
| AnalyticsEmitter( | ||
| uploader: uploader, | ||
| consent: consent ?? FixedConsent(isTelemetryEnabled: consentEnabled), | ||
| anonymousID: anonymousID, | ||
| now: { Date(timeIntervalSince1970: 1_000_000) }, | ||
| flushBatchSize: flushBatchSize, | ||
| maxPendingEvents: maxPendingEvents | ||
| maxPendingEvents: maxPendingEvents, | ||
| notificationCenter: notificationCenter | ||
| ) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Avoid defaulting notificationCenter to the shared global instance in a test helper.
makeEmitter's new notificationCenter: NotificationCenter = .default parameter defaults to the process-wide shared center. Current call sites that need notification delivery already inject a local NotificationCenter(), so this isn't causing a bug today, but any future test that omits the parameter while still posting UserDefaults.didChangeNotification on .default — or any other concurrently-running suite doing the same — could leak into this emitter's observer for the life of the test.
As per path instructions, test code must "isolate shared static, global, UserDefaults, file, and related state per test."
♻️ Proposed fix
- notificationCenter: NotificationCenter = .default
+ notificationCenter: NotificationCenter = NotificationCenter()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func makeEmitter( | |
| uploader: RecordingAnalyticsUploader, | |
| uploader: any AnalyticsUploading, | |
| consent: (any AnalyticsConsentProviding)? = nil, | |
| consentEnabled: Bool = true, | |
| anonymousID: String = "anon-1", | |
| flushBatchSize: Int = 50, | |
| maxPendingEvents: Int = 1000 | |
| maxPendingEvents: Int = 1000, | |
| notificationCenter: NotificationCenter = .default | |
| ) -> AnalyticsEmitter { | |
| AnalyticsEmitter( | |
| uploader: uploader, | |
| consent: consent ?? FixedConsent(isTelemetryEnabled: consentEnabled), | |
| anonymousID: anonymousID, | |
| now: { Date(timeIntervalSince1970: 1_000_000) }, | |
| flushBatchSize: flushBatchSize, | |
| maxPendingEvents: maxPendingEvents | |
| maxPendingEvents: maxPendingEvents, | |
| notificationCenter: notificationCenter | |
| ) | |
| } | |
| private func makeEmitter( | |
| uploader: any AnalyticsUploading, | |
| consent: (any AnalyticsConsentProviding)? = nil, | |
| consentEnabled: Bool = true, | |
| anonymousID: String = "anon-1", | |
| flushBatchSize: Int = 50, | |
| maxPendingEvents: Int = 1000, | |
| notificationCenter: NotificationCenter = NotificationCenter() | |
| ) -> AnalyticsEmitter { | |
| AnalyticsEmitter( | |
| uploader: uploader, | |
| consent: consent ?? FixedConsent(isTelemetryEnabled: consentEnabled), | |
| anonymousID: anonymousID, | |
| now: { Date(timeIntervalSince1970: 1_000_000) }, | |
| flushBatchSize: flushBatchSize, | |
| maxPendingEvents: maxPendingEvents, | |
| notificationCenter: notificationCenter | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileAnalytics/Tests/CmuxMobileAnalyticsTests/AnalyticsEmitterTests.swift`
around lines 44 - 62, Update the makeEmitter test helper to require an
explicitly injected NotificationCenter instead of defaulting to
NotificationCenter.default. Preserve existing call sites by passing their
isolated local centers, and ensure every invocation supplies a per-test
NotificationCenter so observers cannot use shared global state.
Source: Path instructions
Comment on lines
+46
to
+57
| token = notificationCenter.addObserver( | ||
| forName: UserDefaults.didChangeNotification, | ||
| object: nil, | ||
| queue: nil | ||
| ) { _ in | ||
| self.lifecycleQueue.async { [self] in | ||
| let nextIsEnabled = consent.isTelemetryEnabled | ||
| guard nextIsEnabled != isEnabled else { return } | ||
| isEnabled = nextIsEnabled | ||
| (nextIsEnabled ? self.onEnable : self.onRevoke)?.body() | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Observer closure retains self strongly, creating a cycle that makes deinit unreachable.
self.center = notificationCenter plus the strongly-captured self in the addObserver closure (line 50) and the [self] capture (line 51, still a strong capture, not weak) means center keeps the closure alive, which keeps self alive, which keeps center alive. deinit's removeObserver (lines 22-24) can therefore never run while the observer is installed. Harmless for the production .default singleton, but every test-owned NotificationCenter() + watcher pair (see MobileCrashReporterTests.swift) leaks for the rest of the process instead of being torn down as deinit implies it should be.
🔧 Proposed fix: capture weakly and guard
token = notificationCenter.addObserver(
forName: UserDefaults.didChangeNotification,
object: nil,
queue: nil
- ) { _ in
- self.lifecycleQueue.async { [self] in
- let nextIsEnabled = consent.isTelemetryEnabled
- guard nextIsEnabled != isEnabled else { return }
- isEnabled = nextIsEnabled
- (nextIsEnabled ? self.onEnable : self.onRevoke)?.body()
+ ) { [weak self] _ in
+ guard let self else { return }
+ self.lifecycleQueue.async { [weak self] in
+ guard let self else { return }
+ let nextIsEnabled = consent.isTelemetryEnabled
+ guard nextIsEnabled != self.isEnabled else { return }
+ self.isEnabled = nextIsEnabled
+ (nextIsEnabled ? self.onEnable : self.onRevoke)?.body()
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| token = notificationCenter.addObserver( | |
| forName: UserDefaults.didChangeNotification, | |
| object: nil, | |
| queue: nil | |
| ) { _ in | |
| self.lifecycleQueue.async { [self] in | |
| let nextIsEnabled = consent.isTelemetryEnabled | |
| guard nextIsEnabled != isEnabled else { return } | |
| isEnabled = nextIsEnabled | |
| (nextIsEnabled ? self.onEnable : self.onRevoke)?.body() | |
| } | |
| } | |
| token = notificationCenter.addObserver( | |
| forName: UserDefaults.didChangeNotification, | |
| object: nil, | |
| queue: nil | |
| ) { [weak self] _ in | |
| guard let self else { return } | |
| self.lifecycleQueue.async { [weak self] in | |
| guard let self else { return } | |
| let nextIsEnabled = consent.isTelemetryEnabled | |
| guard nextIsEnabled != self.isEnabled else { return } | |
| self.isEnabled = nextIsEnabled | |
| (nextIsEnabled ? self.onEnable : self.onRevoke)?.body() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileCrashReporting/Sources/CmuxMobileCrashReporting/MobileCrashRevocationWatcher.swift`
around lines 46 - 57, Update the observer closure in
MobileCrashRevocationWatcher to capture self weakly and guard that it still
exists before dispatching to lifecycleQueue. Preserve the existing
consent-change handling and state updates, while ensuring the NotificationCenter
observer cannot retain the watcher and prevent deinit from removing it.
Comment on lines
+17
to
+19
| func waitForValue(_ target: Int) { | ||
| while value < target { changed.wait() } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Add a bounded timeout to waitForValue to avoid indefinite test hangs.
Unlike the other DispatchSemaphore.wait(timeout:) calls elsewhere in this test suite, this loop blocks forever if the target count is never reached (e.g. on a real regression), instead of failing fast with a clear timeout.
♻️ Suggested bounded wait
- func waitForValue(_ target: Int) {
- while value < target { changed.wait() }
- }
+ `@discardableResult`
+ func waitForValue(_ target: Int, timeout: TimeInterval = 5) -> Bool {
+ let deadline = DispatchTime.now() + timeout
+ while value < target {
+ if changed.wait(timeout: deadline) == .timedOut { return value >= target }
+ }
+ return true
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func waitForValue(_ target: Int) { | |
| while value < target { changed.wait() } | |
| } | |
| `@discardableResult` | |
| func waitForValue(_ target: Int, timeout: TimeInterval = 5) -> Bool { | |
| let deadline = DispatchTime.now() + timeout | |
| while value < target { | |
| if changed.wait(timeout: deadline) == .timedOut { return value >= target } | |
| } | |
| return true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/CrashTestCounter.swift`
around lines 17 - 19, Update CrashTestCounter.waitForValue to use a bounded
timeout while waiting for value to reach target, matching the test suite’s
existing DispatchSemaphore timeout pattern. Ensure the method exits or fails
clearly when the timeout expires instead of looping indefinitely.
Comment on lines
+21
to
+23
| func waitForCount(_ count: Int) { | ||
| while sequence.count < count { changed.wait() } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Same unbounded-wait concern as CrashTestCounter.waitForValue.
waitForCount blocks forever if the expected sequence never arrives, instead of failing with a timeout. Same fix applies here.
♻️ Suggested bounded wait
- func waitForCount(_ count: Int) {
- while sequence.count < count { changed.wait() }
- }
+ `@discardableResult`
+ func waitForCount(_ count: Int, timeout: TimeInterval = 5) -> Bool {
+ let deadline = DispatchTime.now() + timeout
+ while sequence.count < count {
+ if changed.wait(timeout: deadline) == .timedOut { return sequence.count >= count }
+ }
+ return true
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func waitForCount(_ count: Int) { | |
| while sequence.count < count { changed.wait() } | |
| } | |
| `@discardableResult` | |
| func waitForCount(_ count: Int, timeout: TimeInterval = 5) -> Bool { | |
| let deadline = DispatchTime.now() + timeout | |
| while sequence.count < count { | |
| if changed.wait(timeout: deadline) == .timedOut { return sequence.count >= count } | |
| } | |
| return true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@Packages/iOS/CmuxMobileCrashReporting/Tests/CmuxMobileCrashReportingTests/CrashTestSequenceRecorder.swift`
around lines 21 - 23, Update CrashTestSequenceRecorder.waitForCount to use the
same bounded timeout behavior as CrashTestCounter.waitForValue. Ensure it fails
when the sequence does not reach the requested count within the timeout instead
of waiting indefinitely, while preserving the existing wait behavior when
progress is expected.
azooz2003-bit
pushed a commit
that referenced
this pull request
Jul 15, 2026
Lands main's dev-instance pairing isolation so the phone can tell tagged dev builds apart from production and each other (instance tags carried through pairing, routes, authority checks, and computer labels) — the root cause behind the sim silently rendering another instance's workspaces, including production's. This branch's v1 agent-chat deletion stays authoritative: main's interim chat fixes and the artifact-viewing UI built on the deleted chat package (#7862-era artifact chips, gallery, sheets, and their tests) are removed rather than restored; artifact viewing is a recorded parity item to reintegrate as transcript activity-rail content in the new GUI. ProcessSnapshotCentralizationTests keeps its compatibility test with the chat-registry-dependent test and helper actors stripped. AgentProcessObservationSource adapts to main's actor-backed process snapshot store, preserving exact-basename detection semantics. Composer hosting keeps this branch's judged implementation (nothing to port from main's in-file variant). Full localization-catalog union; budget, package-group, resolved-policy, and test-wiring gates green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
Notes
Note
High Risk
Touches consent-gated telemetry, Sentry lifecycle, and App Store release automation; regressions could leak analytics after opt-out or block/shape submission. App Store crash reporting is deliberately off until consent is proven.
Overview
Prepares the official App Store lane (Apple ID
6783338052) with CI that creates/updates ASC versions, applies local metadata, validates and uploads iPhone 6.9" and iPad screenshots before archive/upload, and tightens release validation to those device types.Privacy & compliance: iOS telemetry now defaults off until the user enables Share Analytics (and crash reports when enabled) in Settings via
sendAnonymousTelemetry. AddsPrivacyInfo.xcprivacy, localized privacy policy content for mobile data/permissions/retention, and App Store review notes plusreviewer-setup.mdfor a prepared review Mac and Tailscale manual pairing.Analytics (
CmuxMobileAnalytics): Introduces a consent generation gate,UserDefaultsrevocation observer, and per-event consent snapshots so opt-out drops buffered/in-flight work and quick re-enable cannot resurrect pre-revocation events.HTTPAnalyticsUploaderregisters upload tasks and cancels them when uploads are disabled.Crash reporting (
CmuxMobileCrashReporting): Replaces revoke-only watching with bidirectional consent lifecycle (start Sentry on opt-in, stop on opt-out), dedicatedURLSessioninvalidation beforeclose(), and cache purging. App Store IPAs setCMUXCrashReportingEnabled=NO(beta/TestFlight stayYES); composition root and Settings copy respect that flag.Build scripts: Upload/archive paths verify and stamp
CMUX_CRASH_REPORTING_ENABLEDper lane so App Store artifacts cannot ship with crash reporting enabled by mistake.Reviewed by Cursor Bugbot for commit 2b65cd6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit