Skip to content

fix(appStart): Handle undefined AppRegistryIntegration.onRunApplication gracefully #4743

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Apr 14, 2025
Merged

fix(appStart): Handle undefined AppRegistryIntegration.onRunApplication gracefully #4743

merged 7 commits into from
Apr 14, 2025

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Apr 10, 2025 •
edited
Loading

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Handle undefined AppRegistryIntegration.onRunApplication edge case gracefully.

💡 Motivation and Context

While working on #4604, I've noticed that after merging #4641 from main the e2e-tests/RnDiffApp crashed on startup when the root app component is wrapped with Sentry.wrap.

💚 How did you test it?

Manual, CI

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

#skip-changelog

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 10, 2025 •
edited
Loading

iOS (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1228.90 ms 1235.16 ms 6.26 ms
Size 3.19 MiB 4.34 MiB 1.16 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8bda0cc+dirty 1217.90 ms 1223.02 ms 5.12 ms
8fe7c9d+dirty 1227.63 ms 1245.28 ms 17.65 ms
e5bc97b+dirty 1229.17 ms 1227.64 ms -1.54 ms
80e955a+dirty 1243.63 ms 1245.58 ms 1.95 ms
d0bf494+dirty 1266.20 ms 1267.52 ms 1.32 ms
4161236+dirty 1245.33 ms 1245.63 ms 0.30 ms
31fcca2+dirty 1222.04 ms 1226.51 ms 4.47 ms
690220d+dirty 1227.45 ms 1221.67 ms -5.78 ms
cdc3945+dirty 1216.51 ms 1223.55 ms 7.04 ms
7fd512a+dirty 1239.41 ms 1241.50 ms 2.09 ms

App size

Revision Plain With Sentry Diff
8bda0cc+dirty 3.19 MiB 4.26 MiB 1.08 MiB
8fe7c9d+dirty 3.19 MiB 4.24 MiB 1.06 MiB
e5bc97b+dirty 2.92 MiB 3.66 MiB 758.40 KiB
80e955a+dirty 3.19 MiB 4.26 MiB 1.08 MiB
d0bf494+dirty 2.92 MiB 3.40 MiB 488.08 KiB
4161236+dirty 3.19 MiB 4.25 MiB 1.06 MiB
31fcca2+dirty 2.92 MiB 3.46 MiB 557.31 KiB
690220d+dirty 2.92 MiB 3.66 MiB 758.77 KiB
cdc3945+dirty 3.19 MiB 4.32 MiB 1.13 MiB
7fd512a+dirty 2.92 MiB 3.66 MiB 758.62 KiB

Previous results on branch: antonis/undefinedOnRunApplication

Startup times

Revision Plain With Sentry Diff
c05e6b4+dirty 1215.33 ms 1220.13 ms 4.80 ms
f36e020+dirty 1214.92 ms 1235.06 ms 20.14 ms

App size

Revision Plain With Sentry Diff
c05e6b4+dirty 3.19 MiB 4.34 MiB 1.16 MiB
f36e020+dirty 3.19 MiB 4.34 MiB 1.16 MiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 10, 2025 •
edited
Loading

iOS (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1233.84 ms 1229.30 ms -4.54 ms
Size 2.63 MiB 3.78 MiB 1.14 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
8bda0cc+dirty 1221.90 ms 1208.11 ms -13.79 ms
8fe7c9d+dirty 1241.83 ms 1244.35 ms 2.51 ms
e5bc97b+dirty 1230.63 ms 1234.83 ms 4.20 ms
80e955a+dirty 1227.94 ms 1230.26 ms 2.32 ms
d0bf494+dirty 1289.40 ms 1298.40 ms 9.00 ms
4161236+dirty 1213.47 ms 1215.28 ms 1.81 ms
31fcca2+dirty 1209.17 ms 1216.21 ms 7.04 ms
690220d+dirty 1228.27 ms 1233.55 ms 5.29 ms
cdc3945+dirty 1202.43 ms 1223.65 ms 21.23 ms
7fd512a+dirty 1218.78 ms 1217.25 ms -1.53 ms

App size

Revision Plain With Sentry Diff
8bda0cc+dirty 2.63 MiB 3.70 MiB 1.06 MiB
8fe7c9d+dirty 2.63 MiB 3.68 MiB 1.04 MiB
e5bc97b+dirty 2.36 MiB 3.10 MiB 753.14 KiB
80e955a+dirty 2.63 MiB 3.70 MiB 1.06 MiB
d0bf494+dirty 2.36 MiB 2.83 MiB 481.15 KiB
4161236+dirty 2.63 MiB 3.69 MiB 1.05 MiB
31fcca2+dirty 2.36 MiB 2.90 MiB 552.95 KiB
690220d+dirty 2.36 MiB 3.10 MiB 753.57 KiB
cdc3945+dirty 2.63 MiB 3.75 MiB 1.12 MiB
7fd512a+dirty 2.36 MiB 3.10 MiB 753.35 KiB

Previous results on branch: antonis/undefinedOnRunApplication

Startup times

Revision Plain With Sentry Diff
c05e6b4+dirty 1215.96 ms 1215.67 ms -0.29 ms
f36e020+dirty 1233.07 ms 1226.91 ms -6.15 ms

App size

Revision Plain With Sentry Diff
c05e6b4+dirty 2.63 MiB 3.78 MiB 1.14 MiB
f36e020+dirty 2.63 MiB 3.78 MiB 1.14 MiB

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 10, 2025 •
edited
Loading

Android (new) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 393.16 ms 387.50 ms -5.66 ms
Size 7.15 MiB 8.40 MiB 1.25 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
f54118b+dirty 250.87 ms 312.22 ms 61.35 ms
8d251c2+dirty 376.67 ms 417.91 ms 41.24 ms
70caa60+dirty 308.83 ms 393.06 ms 84.23 ms
686b3bc+dirty 363.48 ms 356.17 ms -7.31 ms
b8ff156+dirty 386.72 ms 398.18 ms 11.46 ms
52a8031+dirty 330.72 ms 358.76 ms 28.03 ms
ed3d77e+dirty 366.04 ms 411.33 ms 45.28 ms
24cb2a4+dirty 366.28 ms 410.96 ms 44.68 ms
0eacc98+dirty 393.31 ms 445.21 ms 51.90 ms
c81e67f+dirty 355.71 ms 364.96 ms 9.24 ms

App size

Revision Plain With Sentry Diff
f54118b+dirty 7.15 MiB 8.37 MiB 1.22 MiB
8d251c2+dirty 7.15 MiB 8.38 MiB 1.23 MiB
70caa60+dirty 7.15 MiB 8.03 MiB 901.79 KiB
686b3bc+dirty 7.15 MiB 8.38 MiB 1.23 MiB
b8ff156+dirty 7.15 MiB 8.37 MiB 1.22 MiB
52a8031+dirty 7.15 MiB 8.09 MiB 965.95 KiB
ed3d77e+dirty 7.15 MiB 8.35 MiB 1.21 MiB
24cb2a4+dirty 7.15 MiB 8.38 MiB 1.23 MiB
0eacc98+dirty 7.15 MiB 8.38 MiB 1.23 MiB
c81e67f+dirty 7.15 MiB 8.39 MiB 1.23 MiB

Previous results on branch: antonis/undefinedOnRunApplication

Startup times

Revision Plain With Sentry Diff
c05e6b4+dirty 427.85 ms 448.33 ms 20.48 ms

App size

Revision Plain With Sentry Diff
c05e6b4+dirty 7.15 MiB 8.40 MiB 1.25 MiB

@antonis antonis marked this pull request as ready for review April 10, 2025 15:07
antonis
antonis commented Apr 10, 2025
View reviewed changes
packages/core/src/js/tracing/reactnativeprofiler.tsx Outdated

if (!appRegistryIntegration || typeof appRegistryIntegration.onRunApplication !== 'function') {
// eslint-disable-next-line no-console
__DEV__ && console.warn('AppRegistryIntegration.onRunApplication not found or invalid.');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Covering this edge case with a test proved difficult since the mocking affected other test case. I'll be happy to revisit if this is a scenario we should cover.

example 

test('handles graceful missing onRunApplication', async () => {
  const invalidMockedGetAppRegistryIntegration = jest.spyOn(AppRegistry, 'getAppRegistryIntegration').mockReturnValue({
    name: 'appRegistryIntegrationMocked',
    onRunApplication: undefined,
  });

  const testClient = new TestClient(getDefaultTestClientOptions());

  reactNavigationIntegration().afterAllSetup(testClient);

  expect(invalidMockedGetAppRegistryIntegration).not.toHaveBeenCalled();
});

Copy link
Member

Choose a reason for hiding this comment

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

You could use mockReturnValueOnce so the mock is reset after use or reset mock after each.

I think the test would make sense to add to

sentry-react-native/packages/core/test/wrap.test.tsx

Line 5 in dce131b

describe('Sentry.wrap', () => {

Something like this:

  afterEach(() => {
    getCurrentScope().clear();
    getCurrentScope().setClient(undefined);
  });
  
  it('should register on application hook on mount', () => {
    const client = new TestClient(
      getDefaultTestClientOptions(),
    );
    setCurrentClient(client);
    client.init();

    const getAppRegistryIntegration = jest.spyOn(appRegistryIntegration, 'getAppRegistryIntegration').mockReturnValueOnce(undefined);

    const App = wrap(function App() {
      return <></>;
    });
    render(<App />);

    expect(getAppRegistryIntegration).toHaveBeenCalled();
  });

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you for the hint Krystof 🙇
Added a test case in wrap.test.tsx

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Apr 10, 2025 •
edited
Loading

Android (legacy) Performance metrics 🚀

  Plain With Sentry Diff
Startup time 406.47 ms 421.93 ms 15.46 ms
Size 17.75 MiB 20.13 MiB 2.38 MiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
ae7b03d 428.82 ms 412.33 ms -16.49 ms
a989877 425.71 ms 411.04 ms -14.67 ms
6707be9 445.04 ms 464.24 ms 19.20 ms
148f924 492.65 ms 500.28 ms 7.63 ms
8f0282e 451.19 ms 451.69 ms 0.51 ms
c6f01ea 486.20 ms 486.98 ms 0.77 ms
4ed9c54 446.27 ms 433.56 ms -12.71 ms
b7e707e 422.32 ms 412.10 ms -10.22 ms
416f465 446.96 ms 454.22 ms 7.26 ms
43e66e0 373.32 ms 366.57 ms -6.75 ms

App size

Revision Plain With Sentry Diff
ae7b03d 17.75 MiB 20.11 MiB 2.37 MiB
a989877 17.74 MiB 20.07 MiB 2.34 MiB
6707be9 17.75 MiB 20.11 MiB 2.37 MiB
148f924 17.73 MiB 19.94 MiB 2.21 MiB
8f0282e 17.75 MiB 20.13 MiB 2.38 MiB
c6f01ea 17.74 MiB 20.10 MiB 2.36 MiB
4ed9c54 17.75 MiB 20.12 MiB 2.37 MiB
b7e707e 17.75 MiB 20.13 MiB 2.38 MiB
416f465 17.74 MiB 20.09 MiB 2.35 MiB
43e66e0 17.74 MiB 20.09 MiB 2.35 MiB

Previous results on branch: antonis/undefinedOnRunApplication

Startup times

Revision Plain With Sentry Diff
f36e020 410.50 ms 422.12 ms 11.63 ms
c05e6b4 437.84 ms 445.95 ms 8.11 ms

App size

Revision Plain With Sentry Diff
f36e020 17.75 MiB 20.13 MiB 2.38 MiB
c05e6b4 17.75 MiB 20.13 MiB 2.38 MiB

@antonis antonis mentioned this pull request Apr 10, 2025
Draft
10 tasks
@antonis antonis requested a review from krystofwoldrich April 14, 2025 12:24
krystofwoldrich
krystofwoldrich approved these changes Apr 14, 2025
View reviewed changes
Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Thank you for the catch!

@krystofwoldrich krystofwoldrich merged commit b8f1da9 into main Apr 14, 2025
64 of 66 checks passed
@krystofwoldrich krystofwoldrich deleted the antonis/undefinedOnRunApplication branch April 14, 2025 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krystofwoldrich krystofwoldrich krystofwoldrich approved these changes

@lucas-zimerman lucas-zimerman Awaiting requested review from lucas-zimerman lucas-zimerman is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@antonis @krystofwoldrich