Skip to content

Commit

Permalink
chore: simplify routing logic [Part 2]: navigate.tsx (#11186)
Browse files Browse the repository at this point in the history
* chore: simplify navigate method

* fix: broken tests
  • Loading branch information
MounirDhahri authored Nov 25, 2024
1 parent 6fd2008 commit 0e9cb52
Show file tree
Hide file tree
Showing 6 changed files with 148 additions and 77 deletions.
2 changes: 1 addition & 1 deletion src/app/Navigation/AuthenticatedRoutes/Tabs.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ import { ProfileTab } from "app/Navigation/AuthenticatedRoutes/ProfileTab"
import { SearchTab } from "app/Navigation/AuthenticatedRoutes/SearchTab"
import { SellTab } from "app/Navigation/AuthenticatedRoutes/SellTab"
import { registerSharedModalRoutes } from "app/Navigation/AuthenticatedRoutes/SharedRoutes"
import { internal_navigationRef } from "app/Navigation/Navigation"
import { useBottomTabsBadges } from "app/Navigation/Utils/useBottomTabsBadges"
import { BottomTabOption, BottomTabType } from "app/Scenes/BottomTabs/BottomTabType"
import { BottomTabsIcon } from "app/Scenes/BottomTabs/BottomTabsIcon"
import { bottomTabsConfig } from "app/Scenes/BottomTabs/bottomTabsConfig"
import { OnboardingQuiz } from "app/Scenes/Onboarding/OnboardingQuiz/OnboardingQuiz"
import { GlobalStore } from "app/store/GlobalStore"
import { internal_navigationRef } from "app/system/navigation/navigate"
import { useIsStaging } from "app/utils/hooks/useIsStaging"
import { postEventToProviders } from "app/utils/track/providers"
import { Platform } from "react-native"
Expand Down
5 changes: 3 additions & 2 deletions src/app/Navigation/Navigation.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Flex, Text } from "@artsy/palette-mobile"
import { DefaultTheme, NavigationContainer } from "@react-navigation/native"
import { DefaultTheme, NavigationContainer, NavigationContainerRef } from "@react-navigation/native"
import { createNativeStackNavigator } from "@react-navigation/native-stack"
import { addBreadcrumb } from "@sentry/react-native"
import { FPSCounter } from "app/Components/FPSCounter"
Expand All @@ -11,7 +11,6 @@ import {
import { OnboardingWelcomeScreens } from "app/Scenes/Onboarding/Onboarding"
import { GlobalStore } from "app/store/GlobalStore"
import { routingInstrumentation } from "app/system/errorReporting/setupSentry"
import { internal_navigationRef } from "app/system/navigation/navigate"
import { useReloadedDevNavigationState } from "app/system/navigation/useReloadedDevNavigationState"

import { useDevToggle } from "app/utils/hooks/useDevToggle"
Expand All @@ -21,6 +20,8 @@ import { Fragment } from "react"
import { Platform } from "react-native"
import SiftReactNative from "sift-react-native"

export const internal_navigationRef = { current: null as NavigationContainerRef<any> | null }

export type NavigationRoutesParams = AuthenticatedRoutesParams

export const MainStackNavigator = createNativeStackNavigator<NavigationRoutesParams>()
Expand Down
21 changes: 15 additions & 6 deletions src/app/system/navigation/navigate.tests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ jest.mock("app/store/GlobalStore", () => ({
unsafe__getEnvironment: jest.fn().mockReturnValue({
webURL: "https://www.artsy.net",
}),
unsafe_getFeatureFlag: jest.fn().mockReturnValue(false),
unsafe_getFeatureFlag: jest.fn(),
unsafe_getDevToggle: jest.fn().mockReturnValue(false),
GlobalStore: {
actions: {
Expand All @@ -32,6 +32,10 @@ jest.mock("app/store/GlobalStore", () => ({
},
}))

jest.mock("app/Navigation/Navigation", () => ({
internal_navigationRef: jest.fn(),
}))

describe(navigate, () => {
beforeEach(() => {
Linking.openURL = jest.fn()
Expand All @@ -47,8 +51,9 @@ describe(navigate, () => {
})

describe("routes to various screens", () => {
it("like artwork", () => {
it("like artwork", async () => {
navigate("/artwork/josef-albers-homage-to-the-square")
await flushPromiseQueue()
expect(LegacyNativeModules.ARScreenPresenterModule.pushView).toHaveBeenCalled()
expect(args(LegacyNativeModules.ARScreenPresenterModule.pushView as any))
.toMatchInlineSnapshot(`
Expand All @@ -70,8 +75,9 @@ describe(navigate, () => {
]
`)
})
it("like artist", () => {
it("like artist", async () => {
navigate("/artist/banksy")
await flushPromiseQueue()
expect(LegacyNativeModules.ARScreenPresenterModule.pushView).toHaveBeenCalled()
expect(args(LegacyNativeModules.ARScreenPresenterModule.pushView as any))
.toMatchInlineSnapshot(`
Expand All @@ -93,8 +99,9 @@ describe(navigate, () => {
]
`)
})
it("like vanity urls", () => {
it("like vanity urls", async () => {
navigate("/artsy-vanguard-2019")
await flushPromiseQueue()
expect(LegacyNativeModules.ARScreenPresenterModule.pushView).toHaveBeenCalled()
expect(args(LegacyNativeModules.ARScreenPresenterModule.pushView as any))
.toMatchInlineSnapshot(`
Expand All @@ -115,8 +122,9 @@ describe(navigate, () => {
})
})

it("opens external urls with Linking", () => {
it("opens external urls with Linking", async () => {
navigate("https://google.com/banana")
await flushPromiseQueue()
expect(Linking.openURL).toHaveBeenCalledWith("https://google.com/banana")
})

Expand Down Expand Up @@ -186,8 +194,9 @@ describe(navigate, () => {
})

describe("presents modals", () => {
it("when the screen requires it", () => {
it("when the screen requires it", async () => {
navigate("https://live.artsy.net/blah")
await flushPromiseQueue()
expect(args(LegacyNativeModules.ARScreenPresenterModule.presentModal as any))
.toMatchInlineSnapshot(`
[
Expand Down
113 changes: 45 additions & 68 deletions src/app/system/navigation/navigate.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
import { EventEmitter } from "events"
import { ActionType, OwnerType, Screen } from "@artsy/cohesion"
import { NavigationContainerRef, StackActions, TabActions } from "@react-navigation/native"
import { addBreadcrumb, captureMessage } from "@sentry/react-native"
import { StackActions, TabActions } from "@react-navigation/native"
import { AppModule, modules, ViewOptions } from "app/AppRegistry"
import { LegacyNativeModules } from "app/NativeModules/LegacyNativeModules"
import { internal_navigationRef } from "app/Navigation/Navigation"
import { BottomTabType } from "app/Scenes/BottomTabs/BottomTabType"
import { matchRoute } from "app/routes"
import { GlobalStore, unsafe__getSelectedTab, unsafe_getFeatureFlag } from "app/store/GlobalStore"
import { propsStore } from "app/store/PropsStore"
import { getValidTargetURL } from "app/system/navigation/utils/getValidTargetURL"
import { postEventToProviders } from "app/utils/track/providers"
import { visualize } from "app/utils/visualizer"
import { InteractionManager, Linking, Platform } from "react-native"

export const internal_navigationRef = { current: null as NavigationContainerRef<any> | null }

export interface ViewDescriptor extends ViewOptions {
type: "react" | "native"
moduleName: AppModule
Expand Down Expand Up @@ -44,19 +43,26 @@ export interface NavigateOptions {
let lastInvocation = { url: "", timestamp: 0 }

export async function navigate(url: string, options: NavigateOptions = {}) {
let targetURL = url
const enableNewNavigation = unsafe_getFeatureFlag("AREnableNewNavigation")
const targetURL = await getValidTargetURL(url)

addBreadcrumb({
message: `navigate to ${url}`,
category: "navigation",
data: { url, options },
level: "info",
})
visualize("NAV", targetURL, { targetURL, options }, "DTShowNavigationVisualiser")

// handle artsy:// urls, we can just remove it
targetURL = url.replace("artsy://", "")
const result = matchRoute(targetURL)

visualize("NAV", targetURL, { targetURL, options }, "DTShowNavigationVisualiser")
if (result.type === "external_url") {
Linking.openURL(result.url)
return
}

const module = modules[result.module]

const {
replaceActiveModal = false,
replaceActiveScreen = false,
popToRootTabView,
showInTabName,
} = options

// Debounce double taps
const ignoreDebounce = options.ignoreDebounce ?? false
Expand All @@ -69,43 +75,42 @@ export async function navigate(url: string, options: NavigateOptions = {}) {
}

lastInvocation = { url: targetURL, timestamp: Date.now() }

// marketing url requires redirect
if (targetURL.startsWith("https://click.artsy.net")) {
let response
try {
response = await fetch(targetURL)
} catch (error) {
if (enableNewNavigation) {
if (!internal_navigationRef.current || !internal_navigationRef.current?.isReady()) {
if (__DEV__) {
console.warn(error)
} else {
captureMessage(
`[navigate] Error fetching marketing url redirect on: ${targetURL} failed with error: ${error}`,
"error"
throw new Error(
`You are attempting to navigate before the navigation is ready.{\n}
Make sure that the App NavigationContainer isReady is ready`
)
}
return
}

if (response?.url) {
targetURL = response.url
if (replaceActiveModal || replaceActiveScreen) {
internal_navigationRef.current.dispatch(
StackActions.replace(result.module, { ...result.params, ...options.passProps })
)
} else {
if (module.options.onlyShowInTabName) {
switchTab(module.options.onlyShowInTabName)
// We wait for a frame to allow the tab to be switched before we navigate
// This allows us to also override the back button behavior in the tab
requestAnimationFrame(() => {
internal_navigationRef.current?.dispatch(
StackActions.push(result.module, { ...result.params, ...options.passProps })
)
})
} else {
internal_navigationRef.current?.dispatch(
StackActions.push(result.module, { ...result.params, ...options.passProps })
)
}
}
}

const result = matchRoute(targetURL)

if (result.type === "external_url") {
Linking.openURL(result.url)
return
}

const module = modules[result.module]
const presentModally = options.modal ?? module.options.alwaysPresentModally ?? false
const {
replaceActiveModal = false,
replaceActiveScreen = false,
popToRootTabView,
showInTabName,
} = options

const screenDescriptor: ViewDescriptor = {
type: module.type,
Expand All @@ -119,34 +124,6 @@ export async function navigate(url: string, options: NavigateOptions = {}) {
...module.options,
}

const enableNewNavigation = unsafe_getFeatureFlag("AREnableNewNavigation")

if (enableNewNavigation) {
if (internal_navigationRef.current?.isReady()) {
if (replaceActiveModal || replaceActiveScreen) {
internal_navigationRef.current.dispatch(
StackActions.replace(result.module, { ...result.params, ...options.passProps })
)
} else {
if (module.options.onlyShowInTabName) {
switchTab(module.options.onlyShowInTabName)
// We wait for a frame to allow the tab to be switched before we navigate
// This allows us to also override the back button behavior in the tab
requestAnimationFrame(() => {
internal_navigationRef.current?.dispatch(
StackActions.push(result.module, { ...result.params, ...options.passProps })
)
})
} else {
internal_navigationRef.current?.dispatch(
StackActions.push(result.module, { ...result.params, ...options.passProps })
)
}
}
}
return
}

// Set props which we will reinject later. See HACKS.md
propsStore.setPendingProps(screenDescriptor.moduleName, screenDescriptor.props)

Expand Down
48 changes: 48 additions & 0 deletions src/app/system/navigation/utils/getValidTargetURL.tests.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import * as sentry from "@sentry/react-native"
import fetchMock from "jest-fetch-mock"
import { getValidTargetURL } from "./getValidTargetURL"

describe("getValidTargetURL", () => {
beforeAll(() => {
fetchMock.enableMocks()
})

beforeEach(() => {
fetchMock.resetMocks()
})

it("should strip artsy protocol from the URL", async () => {
const url = "artsy://artist/andy-warhol"
const result = await getValidTargetURL(url)
expect(result).toEqual("artist/andy-warhol")
})

it("should return the original URL if no redirects are necessary", async () => {
const url = "https://www.artsy.net/artist/andy-warhol"
const result = await getValidTargetURL(url)
expect(result).toEqual(url)
})

it("should handle an error during fetch and log it if not in development", async () => {
const url = "https://click.artsy.net/redirect"
fetchMock.mockReject(new Error("Network failure"))

// @ts-expect-error
global.__DEV__ = false
const spy = jest.spyOn(sentry, "captureMessage").mockImplementation()

const result = await getValidTargetURL(url)
expect(result).toEqual(url)
expect(spy).toBeCalled()
spy.mockRestore()
})

it("should correctly resolve redirected URL", async () => {
const originalUrl = "https://click.artsy.net/redirect"
const redirectedUrl = "https://www.artsy.net/feature"
fetchMock.mockResponseOnce("", { url: redirectedUrl })

const result = await getValidTargetURL(originalUrl)
expect(result).toEqual(redirectedUrl)
})
})
36 changes: 36 additions & 0 deletions src/app/system/navigation/utils/getValidTargetURL.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { captureMessage } from "@sentry/react-native"

/**
* This helper function is used to convert the href to a valid screen name that can be navigated to.
* This is required because the href can be a non trivial marketing url that needs additional handling.
* @param url
* @returns targetURL
*/
export const getValidTargetURL = async (url: string) => {
let targetURL = url

targetURL = url.replace("artsy://", "")

// marketing url requires redirect
if (targetURL.startsWith("https://click.artsy.net")) {
let response
try {
response = await fetch(targetURL)
} catch (error) {
if (__DEV__) {
console.warn(error)
} else {
captureMessage(
`[navigate] Error fetching marketing url redirect on: ${targetURL} failed with error: ${error}`,
"error"
)
}
}

if (response?.url) {
targetURL = response.url
}
}

return targetURL
}

0 comments on commit 0e9cb52

Please sign in to comment.