[PLAT-11664] Refactor navigation components (#418)

* Refactor navigation component to ensure spans are ended with the appropriate time and condition
bugsnag · Mar 18, 2024 · f7881d6 · f7881d6
1 parent 6a898a6
commit f7881d6
Show file tree

Hide file tree

Showing 15 changed files with 302 additions and 114 deletions.
diff --git a/.buildkite/react-native-navigation-pipeline.full.yml b/.buildkite/react-native-navigation-pipeline.full.yml
@@ -7,7 +7,7 @@ steps:
 
       # Android Build
       # ------------------------------
-      - label: ":building_construction: :android: Build RN {{matrix.rn_version}} (Old Arch) test fixture"
+      - label: ":building_construction: :android: Build react-native-navigation test fixture - RN {{matrix.rn_version}} (Old Arch)"
         key: "build-react-native-navigation-android-fixture-old-arch"
         timeout_in_minutes: 30
         agents:
@@ -24,12 +24,13 @@ steps:
           setup:
             rn_version:
               - "0.71"
+              - "0.72"
         retry:
           automatic:
             - exit_status: "*"
               limit: 1
 
-      - label: ":building_construction: :android: Build RN {{matrix.rn_version}} (New Arch) test fixture"
+      - label: ":building_construction: :android: Build react-native-navigation test fixture - RN {{matrix.rn_version}} (New Arch)"
         key: "build-react-native-navigation-android-fixture-new-arch"
         timeout_in_minutes: 30
         agents:
@@ -47,14 +48,15 @@ steps:
           setup:
             rn_version:
               - "0.71"
+              - "0.72"
         retry:
           automatic:
             - exit_status: "*"
               limit: 1
 
       # iOS Build
       # ------------------------------
-      - label: ':building_construction: :mac: Build RN {{matrix.rn_version}} (Old Arch) test fixture'
+      - label: ':building_construction: :mac: Build react-native-navigation test fixture - RN {{matrix.rn_version}} (Old Arch)'
         key: "build-react-native-navigation-ios-fixture-old-arch"
         timeout_in_minutes: 30
         agents:
@@ -73,12 +75,13 @@ steps:
           setup:
             rn_version:
               - "0.71"
+              - "0.72"
         retry:
           automatic:
             - exit_status: "*"
               limit: 1
 
-      - label: ':building_construction: :mac: Build RN {{matrix.rn_version}} (New Arch) test fixture'
+      - label: ':building_construction: :mac: Build react-native-navigation test fixture - RN {{matrix.rn_version}} (New Arch)'
         key: "build-react-native-navigation-ios-fixture-new-arch"
         timeout_in_minutes: 30
         agents:
@@ -98,14 +101,15 @@ steps:
           setup:
             rn_version:
               - "0.71"
+              - "0.72"
         retry:
           automatic:
             - exit_status: "*"
               limit: 1
 
       # Android Test
       # ------------------------------
-      - label: ":bitbar: :android: RN {{matrix.rn_version}} (Old Arch) Android {{matrix.android_version}} react-native-navigation end-to-end tests"
+      - label: ":bitbar: :android: react-native-navigation end-to-end tests - RN {{matrix.rn_version}} (Old Arch) Android {{matrix.android_version}}"
         depends_on: "build-react-native-navigation-android-fixture-old-arch"
         timeout_in_minutes: 60
         env: 
@@ -139,8 +143,9 @@ steps:
               - "12"
             rn_version:
               - "0.71"
+              - "0.72"
 
-      - label: ":bitbar: :android: RN {{matrix.rn_version}} (New Arch) Android {{matrix.android_version}} react-native-navigation end-to-end tests"
+      - label: ":bitbar: :android: react-native-navigation end-to-end tests - RN {{matrix.rn_version}} (New Arch) Android {{matrix.android_version}}"
         depends_on: "build-react-native-navigation-android-fixture-new-arch"
         timeout_in_minutes: 60
         env: 
@@ -175,10 +180,11 @@ steps:
               - "12"
             rn_version:
               - "0.71"
+              - "0.72"
 
       # iOS Test
       # ------------------------------
-      - label: ":bitbar: :mac: RN {{matrix.rn_version}} (Old Arch) iOS {{matrix.ios_version}} react-native-navigation end-to-end tests"
+      - label: ":bitbar: :mac: react-native-navigation end-to-end tests - RN {{matrix.rn_version}} (Old Arch) iOS {{matrix.ios_version}}"
         depends_on: "build-react-native-navigation-ios-fixture-old-arch"
         timeout_in_minutes: 60
         env: 
@@ -212,8 +218,9 @@ steps:
               - "14"
             rn_version:
               - "0.71"
+              - "0.72"
 
-      - label: ":bitbar: :mac: RN {{matrix.rn_version}} (New Arch) iOS {{matrix.ios_version}} react-native-navigation end-to-end tests"
+      - label: ":bitbar: :mac: react-native-navigation end-to-end tests - RN {{matrix.rn_version}} (New Arch) iOS {{matrix.ios_version}}"
         depends_on: "build-react-native-navigation-ios-fixture-new-arch"
         timeout_in_minutes: 60
         env: 
@@ -248,3 +255,4 @@ steps:
               - "14"
             rn_version:
               - "0.71"
+              - "0.72"
diff --git a/.buildkite/react-native-navigation-pipeline.yml b/.buildkite/react-native-navigation-pipeline.yml
@@ -6,7 +6,7 @@ steps:
     steps:
       # Android Build
       # ------------------------------
-      - label: ":building_construction: :android: Build RN {{matrix.rn_version}} (Old Arch) test fixture"
+      - label: ":building_construction: :android: Build react-native-navigation test fixture - RN {{matrix.rn_version}} (Old Arch)"
         key: "build-react-native-navigation-android-fixture-old-arch"
         timeout_in_minutes: 30
         agents:
@@ -28,7 +28,7 @@ steps:
             - exit_status: "*"
               limit: 1
 
-      - label: ":building_construction: :android: Build RN {{matrix.rn_version}} (New Arch) test fixture"
+      - label: ":building_construction: :android: Build react-native-navigation test fixture - RN {{matrix.rn_version}} (New Arch)"
         key: "build-react-native-navigation-android-fixture-new-arch"
         timeout_in_minutes: 30
         agents:
@@ -53,7 +53,7 @@ steps:
 
       # iOS Build
       # ------------------------------
-      - label: ":building_construction: :mac: Build RN {{matrix.rn_version}} (Old Arch) test fixture"
+      - label: ":building_construction: :mac: Build react-native-navigation test fixture - RN {{matrix.rn_version}} (Old Arch)"
         key: "build-react-native-navigation-ios-fixture-old-arch"
         timeout_in_minutes: 30
         agents:
@@ -77,7 +77,7 @@ steps:
             - exit_status: "*"
               limit: 1
 
-      - label: ":building_construction: :mac: Build RN {{matrix.rn_version}} (New Arch) test fixture"
+      - label: ":building_construction: :mac: Build react-native-navigation test fixture - RN {{matrix.rn_version}} (New Arch)"
         key: "build-react-native-navigation-ios-fixture-new-arch"
         timeout_in_minutes: 30
         agents:
@@ -104,7 +104,7 @@ steps:
 
       # Android Test
       # ------------------------------
-      - label: ":bitbar: :android: RN {{matrix.rn_version}} (Old Arch) Android {{matrix.android_version}} react-native-navigation end-to-end tests"
+      - label: ":bitbar: :android: react-native-navigation end-to-end tests - RN {{matrix.rn_version}} (Old Arch) / Android {{matrix.android_version}} "
         depends_on: "build-react-native-navigation-android-fixture-old-arch"
         timeout_in_minutes: 60
         env:
@@ -139,7 +139,7 @@ steps:
             rn_version:
               - "0.72"
 
-      - label: ":bitbar: :android: RN {{matrix.rn_version}} (New Arch) Android {{matrix.android_version}} react-native-navigation end-to-end tests"
+      - label: ":bitbar: :android: react-native-navigation end-to-end tests - RN {{matrix.rn_version}} (New Arch) / Android {{matrix.android_version}}"
         depends_on: "build-react-native-navigation-android-fixture-new-arch"
         timeout_in_minutes: 60
         env:
@@ -177,7 +177,7 @@ steps:
 
       # iOS Test
       # ------------------------------
-      - label: ":bitbar: :mac: RN {{matrix.rn_version}} (Old Arch) iOS {{matrix.ios_version}} react-native-navigation end-to-end tests"
+      - label: ":bitbar: :mac: react-native-navigation end-to-end tests - RN {{matrix.rn_version}} (Old Arch) / iOS {{matrix.ios_version}}"
         depends_on: "build-react-native-navigation-ios-fixture-old-arch"
         timeout_in_minutes: 60
         env:
@@ -212,7 +212,7 @@ steps:
             rn_version:
               - "0.72"
 
-      - label: ":bitbar: :mac: RN {{matrix.rn_version}} (New Arch) iOS {{matrix.ios_version}} react-native-navigation end-to-end tests"
+      - label: ":bitbar: :mac: react-native-navigation end-to-end tests - RN {{matrix.rn_version}} (New Arch) / iOS {{matrix.ios_version}} react-native-navigation end-to-end tests"
         depends_on: "build-react-native-navigation-ios-fixture-new-arch"
         timeout_in_minutes: 60
         env:

diff --git a/.buildkite/react-native-pipeline.full.yml b/.buildkite/react-native-pipeline.full.yml
@@ -18,6 +18,7 @@ steps:
           - ./bin/generate-react-native-fixture
         matrix:
           - "0.71"
+          - "0.72"
         retry:
           automatic:
             - exit_status: "*"
@@ -38,6 +39,7 @@ steps:
           - ./bin/generate-react-native-fixture
         matrix:
           - "0.71"
+          - "0.72"
         retry:
           automatic:
             - exit_status: "*"
@@ -59,6 +61,7 @@ steps:
           - ./bin/generate-react-native-fixture
         matrix:
           - "0.71"
+          - "0.72"
         retry:
           automatic:
             - exit_status: "*"
@@ -81,6 +84,7 @@ steps:
           - ./bin/generate-react-native-fixture
         matrix:
           - "0.71"
+          - "0.72"
         retry:
           automatic:
             - exit_status: "*"
@@ -116,6 +120,7 @@ steps:
         concurrency_method: eager
         matrix:
           - "0.71"
+          - "0.72"
 
       - label: ":bitbar: :android: RN {{matrix}} Android 12 (New Arch) end-to-end tests"
         depends_on: "build-react-native-android-fixture-new-arch"
@@ -146,6 +151,7 @@ steps:
         concurrency_method: eager
         matrix:
           - "0.71"
+          - "0.72"
 
       - label: ":bitbar: :mac: RN {{matrix}} iOS 14 (Old Arch) end-to-end tests"
         depends_on: "build-react-native-ios-fixture-old-arch"
@@ -174,6 +180,7 @@ steps:
         concurrency_method: eager
         matrix:
           - "0.71"
+          - "0.72"
 
       - label: ":bitbar: :mac: RN {{matrix}} iOS 14 (New Arch) end-to-end tests"
         depends_on: "build-react-native-ios-fixture-new-arch"
@@ -204,3 +211,4 @@ steps:
         concurrency_method: eager
         matrix:
           - "0.71"
+          - "0.72"
diff --git a/packages/platforms/react-native/lib/auto-instrumentation/app-start-plugin.tsx b/packages/platforms/react-native/lib/auto-instrumentation/app-start-plugin.tsx
@@ -42,7 +42,9 @@ export class AppStartPlugin implements Plugin<ReactNativeConfiguration> {
 
     const AppStartWrapper = ({ children }: WrapperProps) => {
       useEffect(() => {
-        this.spanFactory.endSpan(appStartSpan, this.clock.now())
+        if (appStartSpan.isValid()) {
+          this.spanFactory.endSpan(appStartSpan, this.clock.now())
+        }
       }, [])
 
       return children

diff --git a/packages/react-native-navigation/__mocks__/react-native-navigation.ts b/packages/react-native-navigation/__mocks__/react-native-navigation.ts
@@ -1,6 +1,6 @@
 export const Navigation = {
   events: jest.fn().mockReturnValue({
     registerCommandListener: jest.fn(),
-    registerComponentDidAppearListener: jest.fn()
+    registerComponentWillAppearListener: jest.fn()
   })
 }
diff --git a/packages/react-native-navigation/__tests__/react-native-navigation-plugin.test.ts b/packages/react-native-navigation/__tests__/react-native-navigation-plugin.test.ts
@@ -26,7 +26,7 @@ describe('ReactNativeNavigationPlugin', () => {
       componentId: '123'
     })
 
-    jest.mocked(Navigation.events().registerComponentDidAppearListener).mock.calls[0][0]({
+    jest.mocked(Navigation.events().registerComponentWillAppearListener).mock.calls[0][0]({
       componentId: '123',
       componentName: 'TestScreen',
       componentType: 'Component'
@@ -58,7 +58,7 @@ describe('ReactNativeNavigationPlugin', () => {
       componentId: '123'
     })
 
-    jest.mocked(Navigation.events().registerComponentDidAppearListener).mock.calls[1][0]({
+    jest.mocked(Navigation.events().registerComponentWillAppearListener).mock.calls[1][0]({
       componentId: '123',
       componentName: 'TestScreen',
       componentType: 'Component'
@@ -107,7 +107,7 @@ describe('ReactNativeNavigationPlugin', () => {
       componentId: '1'
     })
 
-    jest.mocked(Navigation.events().registerComponentDidAppearListener).mock.calls[2][0]({
+    jest.mocked(Navigation.events().registerComponentWillAppearListener).mock.calls[2][0]({
       componentId: '1',
       componentName: 'FirstScreen',
       componentType: 'Component'
@@ -121,7 +121,7 @@ describe('ReactNativeNavigationPlugin', () => {
       componentId: '2'
     })
 
-    jest.mocked(Navigation.events().registerComponentDidAppearListener).mock.calls[2][0]({
+    jest.mocked(Navigation.events().registerComponentWillAppearListener).mock.calls[2][0]({
       componentId: '2',
       componentName: 'SecondScreen',
       componentType: 'Component'

diff --git a/packages/react-native-navigation/lib/CompleteNavigation.tsx b/packages/react-native-navigation/lib/CompleteNavigation.tsx
@@ -25,19 +25,17 @@ export const CompleteNavigation: React.FunctionComponent<Props> = ({ children, o
 
     if (plugin) {
       plugin.blockNavigationEnd()
-    }
 
-    if (on === 'mount') {
-      setTimeout(() => {
-        if (plugin) {
+      if (on === 'mount') {
+        setTimeout(() => {
           plugin.unblockNavigationEnd('mount')
-        }
-      })
-    }
+        }, 0)
+      }
 
-    return () => {
-      if (plugin && on === 'unmount') {
-        plugin.unblockNavigationEnd('unmount')
+      if (on === 'unmount') {
+        return () => {
+          plugin.unblockNavigationEnd('unmount')
+        }
       }
     }
   }, [])

diff --git a/packages/react-native-navigation/lib/react-native-navigation-plugin.ts b/packages/react-native-navigation/lib/react-native-navigation-plugin.ts
@@ -13,7 +13,6 @@ class ReactNativeNavigationPlugin implements Plugin<ReactNativeConfiguration> {
   private startTime?: number
   private startTimeout?: NodeJS.Timeout
   private endTimeout?: NodeJS.Timeout
-  private endedBy: Reason = 'immediate'
   private componentsWaiting = 0
   private spanFactory?: SpanFactory<ReactNativeConfiguration>
   private previousRoute?: string
@@ -29,19 +28,19 @@ class ReactNativeNavigationPlugin implements Plugin<ReactNativeConfiguration> {
     this.currentNavigationSpan = undefined
   }
 
-  private endActiveSpan (endTime: number) {
+  private endActiveSpan (endTime: number, endedBy: Reason) {
     if (this.componentsWaiting === 0 && this.currentNavigationSpan && this.spanFactory) {
-      this.currentNavigationSpan.setAttribute('bugsnag.navigation.ended_by', this.endedBy)
+      this.currentNavigationSpan.setAttribute('bugsnag.navigation.ended_by', endedBy)
       this.spanFactory.endSpan(this.currentNavigationSpan, endTime)
       this.clearActiveSpan()
     }
   }
 
   /** Trigger the end of the current navigation span after 100ms */
-  private triggerNavigationEnd = (endTime: number) => {
+  private triggerNavigationEnd = (endTime: number, endedBy: Reason) => {
     clearTimeout(this.endTimeout)
     this.endTimeout = setTimeout(() => {
-      this.endActiveSpan(endTime)
+      this.endActiveSpan(endTime, endedBy)
     }, NAVIGATION_COMPLETE_TIMEOUT)
   }
 
@@ -58,11 +57,10 @@ class ReactNativeNavigationPlugin implements Plugin<ReactNativeConfiguration> {
   */
   unblockNavigationEnd = (endedBy: Reason) => {
     const renderTime = performance.now()
-    this.endedBy = endedBy
     this.componentsWaiting = Math.max(this.componentsWaiting - 1, 0)
 
     if (this.componentsWaiting === 0 && this.spanFactory) {
-      this.triggerNavigationEnd(renderTime)
+      this.triggerNavigationEnd(renderTime, endedBy)
     }
   }
 
@@ -79,7 +77,7 @@ class ReactNativeNavigationPlugin implements Plugin<ReactNativeConfiguration> {
     })
 
     // Navigation has occurred
-    this.Navigation.events().registerComponentDidAppearListener(event => {
+    this.Navigation.events().registerComponentWillAppearListener(event => {
       if (typeof this.startTime === 'number') {
         clearTimeout(this.startTimeout)
 
@@ -99,9 +97,7 @@ class ReactNativeNavigationPlugin implements Plugin<ReactNativeConfiguration> {
 
         this.previousRoute = routeName
 
-        this.endedBy = 'immediate'
-
-        this.triggerNavigationEnd(performance.now())
+        this.triggerNavigationEnd(performance.now(), 'immediate')
       }
     })
   }