Skip to content

fix: Fix iOS build issue on Xcode 16.3 #15045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

Cal-L
Copy link
Contributor

@Cal-L Cal-L commented Apr 30, 2025

Description

Fix iOS build issues on Xcode 16.3 by bumping Sentry and Expo deps. This also updates pods repo so a few other minor version bumps will exist in Podfile.lock as well

Related issues

Fixes:

Manual testing steps

How it was tested: Both Android and iOS apps should run after running yarn start:ios or yarn start:android

Screenshots/Recordings

Before

Individuals began experiencing iOS build issues with Xcode 16.3 as reported here - https://consensys.slack.com/archives/C02U025CVU4/p1746041871082239. The fix was to bump Expo SDK deps as well as Sentry.

Expo SDK deps - https://expo.dev/changelog/xcode-16-3-patches
Sentry issue - https://github.com/getsentry/sentry-cocoa/releases/tag/8.46.0 (we bumped to stable 8.48.0 with pod repo update)

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@Cal-L Cal-L requested a review from a team as a code owner April 30, 2025 23:26
@Cal-L Cal-L added the team-mobile-platform Mobile Platform team label Apr 30, 2025
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@Cal-L Cal-L added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 30, 2025
@Cal-L Cal-L added the No QA Needed Apply this label when your PR does not need any QA effort. label Apr 30, 2025
@github-project-automation github-project-automation bot moved this to Needs dev review in PR review queue Apr 30, 2025
@Cal-L Cal-L added no-changelog Indicates no external facing user changes, therefore no changelog documentation needed Run Smoke E2E Triggers smoke e2e on Bitrise labels Apr 30, 2025
Copy link
Contributor

https://bitrise.io/ Bitrise

🔄🔄🔄 pr_smoke_e2e_pipeline started on Bitrise...🔄🔄🔄

Commit hash: 4cbc4fb
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/93cb54fa-0088-4401-968a-48e57fd2dbdc

Note

  • This comment will auto-update when build completes
  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Copy link

socket-security bot commented Apr 30, 2025

All alerts resolved. Learn more about Socket for GitHub.

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring alerts on:

View full report

@Cal-L Cal-L changed the title Fix: Fix iOS build issue on Xcode 16.3 fix: Fix iOS build issue on Xcode 16.3 Apr 30, 2025
chrisleewilcox
chrisleewilcox previously approved these changes Apr 30, 2025
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-project-automation github-project-automation bot moved this from Needs dev review to Review finalised - Ready to be merged in PR review queue Apr 30, 2025
chrisleewilcox
chrisleewilcox previously approved these changes Apr 30, 2025
Copy link
Contributor

@chrisleewilcox chrisleewilcox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

tommasini
tommasini previously approved these changes May 1, 2025
@Cal-L Cal-L dismissed stale reviews from tommasini and chrisleewilcox via 71da8bb May 1, 2025 00:04
cortisiko
cortisiko previously approved these changes May 1, 2025
owencraston
owencraston previously approved these changes May 1, 2025
Copy link
Contributor

@owencraston owencraston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixed my iOS builds (using xcode 16.3)

@Cal-L Cal-L dismissed stale reviews from owencraston and cortisiko via d0e436e May 1, 2025 03:40
@Cal-L Cal-L requested a review from a team as a code owner May 1, 2025 03:40
@Cal-L Cal-L removed the Run Smoke E2E Triggers smoke e2e on Bitrise label May 1, 2025
@@ -285,7 +285,7 @@ function startSpan<T>(
op: op || OP_DEFAULT,
// This needs to be parentSpan once we have the withIsolatedScope implementation in place in the Sentry SDK for React Native
// Reference PR that updates @sentry/react-native: https://github.com/getsentry/sentry-react-native/pull/3895
parentSpanId: parentSpan?.spanId,
parentSpan,
Copy link
Contributor Author

@Cal-L Cal-L May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Takes an entire span now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know where this change is documented? I'm having a bit of trouble following.

I see something about Span mentioned here: https://docs.sentry.io/platforms/javascript/migration/v7-to-v8/#removal-of-span-class-export-from-sdk-packages

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see, the removed the Span class from exports, but they are still exporting the type.

I found a dedicated page about the trace API changes. There is a section about how to set a parent span: https://docs.sentry.io/platforms/javascript/migration/v7-to-v8/v8-new-performance-api/#creating-a-child-span-of-a-specific-span

They suggest a different method (Sentry.withActiveSpan). It's unclear how this differs from setting the parentSpan option 🤔.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this option wasn't referenced in the migration docs, but I found it elsewhere in their docs here: https://docs.sentry.io/platforms/javascript/tracing/instrumentation/#starting-spans-as-children-of-a-specific-span

This suggests what we're doing here is correct, no need to use withActiveSpan

@@ -373,11 +373,3 @@ function tryCatchMaybePromise<T>(

return undefined;
}

function sentrySetMeasurement(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need util. Can just call setMeasurement directly

+ * Patch to enforce better stack tracing using Error - https://docs.sentry.io/platforms/javascript/usage/#capturing-errors
*/
-export declare function captureException(exception: unknown, hint?: ExclusiveEventHintOrCaptureContext): string;
+export declare function captureException(exception: Error, hint?: ExclusiveEventHintOrCaptureContext): string;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure better stack trace with Error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that we should be passing errors to captureException, but why do we need this patch?

If we really want this type checked more strictly, we can define our own captureException that calls Sentry's. Typically we avoid patches except as a last resort, as they make updates significantly more difficult.

@Cal-L
Copy link
Contributor Author

Cal-L commented May 1, 2025

@SocketSecurity ignore-all

@Cal-L Cal-L added Run Smoke E2E Triggers smoke e2e on Bitrise and removed Run Smoke E2E Triggers smoke e2e on Bitrise labels May 1, 2025
Copy link
Contributor

github-actions bot commented May 1, 2025

https://bitrise.io/ Bitrise

❌❌❌ pr_smoke_e2e_pipeline failed on Bitrise! ❌❌❌

Commit hash: 4689ee5
Build link: https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/c10e1308-2eb9-4518-bf96-e63c3c1f4f1a

Note

  • You can kick off another pr_smoke_e2e_pipeline on Bitrise by removing and re-applying the Run Smoke E2E label on the pull request

Tip

  • Check the documentation if you have any doubts on how to understand the failure on bitrise

@Cal-L Cal-L enabled auto-merge May 1, 2025 04:51
OGPoyraz
OGPoyraz previously approved these changes May 1, 2025
Copy link
Member

@OGPoyraz OGPoyraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmation changes LGTM

tommasini
tommasini previously approved these changes May 1, 2025
package.json Outdated
@@ -254,7 +254,7 @@
"@reown/walletkit": "^1.2.3",
"@segment/sovran-react-native": "^1.0.4",
"@sentry/integrations": "6.3.1",
"@sentry/react-native": "^5.33.0",
"@sentry/react-native": "~6.10.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why limit the range just to this minor version? Is there a breaking change in version 6.11?

Copy link
Contributor Author

@Cal-L Cal-L May 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specific version was auto updated using Expo CLI via npx expo install --fix. It's part of Expo's workflow for version validation. My guess is that they have a manifest somewhere that keeps track of this. This was just their suggested version but I don't see why we need to limit to the minor version as well. I can update this in a bit

package.json Outdated
@@ -254,7 +254,7 @@
"@reown/walletkit": "^1.2.3",
"@segment/sovran-react-native": "^1.0.4",
"@sentry/integrations": "6.3.1",
"@sentry/react-native": "^5.33.0",
"@sentry/react-native": "~6.10.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaking change made in v6 seems to impact us: https://docs.sentry.io/platforms/react-native/migration/v5-to-v6/#updated-behavior-of-tracepropagationtargets-in-react-native

Effectively it means tracing headers are now added on all network requests made outside of the webview, whereas previously they were not. We may want to explicitly set tracePropagationTargets to the v5 default value to avoid any unintended changes here

package.json Outdated
@@ -427,7 +427,7 @@
"@open-rpc/test-coverage": "^2.2.2",
"@react-native/metro-config": "0.76.9",
"@rpii/wdio-html-reporter": "^7.7.1",
"@sentry/types": "^7.117.0",
"@sentry/core": "8.54.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The update to v8 includes some breaking changes that haven't been dealt with yet. I'm surprised it isn't throwing an error 🤔

See here: https://docs.sentry.io/platforms/javascript/migration/v7-to-v8/#removal-of-class-based-integrations
We're still using two class-based integrations

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the integrations aren't throwing an error because we're importing them from the integrations package, not from core. And the integrations package was not updated in this PR.

Still seems advisable to update it though, at least according to the migration doc. It's unclear whether there are any consequences to using outdated integrations.

Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like some breaking changes are not yet handled

@github-project-automation github-project-automation bot moved this from Review finalised - Ready to be merged to Needs more work from the author in PR review queue May 1, 2025
Comment on lines 286 to 287
// This needs to be parentSpan once we have the withIsolatedScope implementation in place in the Sentry SDK for React Native
// Reference PR that updates @sentry/react-native: https://github.com/getsentry/sentry-react-native/pull/3895
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is now obsolete

Suggested change
// This needs to be parentSpan once we have the withIsolatedScope implementation in place in the Sentry SDK for React Native
// Reference PR that updates @sentry/react-native: https://github.com/getsentry/sentry-react-native/pull/3895

@Cal-L Cal-L dismissed stale reviews from tommasini and OGPoyraz via eb5036c May 1, 2025 20:46
Copy link

sonarqubecloud bot commented May 1, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
50.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) No QA Needed Apply this label when your PR does not need any QA effort. no-changelog Indicates no external facing user changes, therefore no changelog documentation needed Run Smoke E2E Triggers smoke e2e on Bitrise team-mobile-platform Mobile Platform team
Projects
Status: Needs more work from the author
Development

Successfully merging this pull request may close these issues.

7 participants