From 033e7efb19a5e3d3af3323309e1edb1e3fde11e8 Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Thu, 17 Oct 2024 16:50:56 -0400 Subject: [PATCH 1/5] remove modal navigation from router transactions --- app/packages/app/src/routing/RouterContext.ts | 203 +++++++++++------- .../core/src/components/Grid/useRefreshers.ts | 9 +- .../state/src/recoil/sidebarExpanded.ts | 14 -- 3 files changed, 123 insertions(+), 103 deletions(-) diff --git a/app/packages/app/src/routing/RouterContext.ts b/app/packages/app/src/routing/RouterContext.ts index e684acaf8d..d33b0623b4 100644 --- a/app/packages/app/src/routing/RouterContext.ts +++ b/app/packages/app/src/routing/RouterContext.ts @@ -69,6 +69,8 @@ export const createRouter = ( ): Router => { const history = isNotebook() ? createMemoryHistory() : createBrowserHistory(); + const getEntryResource = makeGetEntryResource(); + let currentEntryResource: Resource>; let nextCurrentEntryResource: Resource>; @@ -79,17 +81,22 @@ export const createRouter = ( >(); const update = (location: FiftyOneLocation, action?: Action) => { - requestAnimationFrame(() => { - for (const [_, [__, onPending]] of subscribers) onPending?.(); - }); currentEntryResource.load().then(({ cleanup }) => { - nextCurrentEntryResource = getEntryResource( - environment, - routes, - location as FiftyOneLocation, - false, - handleError - ); + try { + nextCurrentEntryResource = getEntryResource({ + environment, + routes, + location: location as FiftyOneLocation, + hard: false, + handleError, + }); + } catch { + return; + } + + requestAnimationFrame(() => { + for (const [_, [__, onPending]] of subscribers) onPending?.(); + }); const loadingResource = nextCurrentEntryResource; loadingResource.load().then((entry) => { @@ -135,13 +142,13 @@ export const createRouter = ( load(hard = false) { const runUpdate = !currentEntryResource || hard; if (!currentEntryResource || hard) { - currentEntryResource = getEntryResource( + currentEntryResource = getEntryResource({ environment, - routes, - history.location as FiftyOneLocation, hard, - handleError - ); + handleError, + location: history.location as FiftyOneLocation, + routes, + }); } runUpdate && update(history.location as FiftyOneLocation); return currentEntryResource.load(); @@ -171,79 +178,111 @@ export const createRouter = ( }; }; -const getEntryResource = ( - environment: Environment, - routes: RouteDefinition[], - location: FiftyOneLocation, - hard = false, - handleError?: (error: unknown) => void -): Resource> => { - let route: RouteDefinition; - let matchResult: MatchPathResult | undefined = undefined; - for (let index = 0; index < routes.length; index++) { - route = routes[index]; - const match = matchPath( - location.pathname, - route, - location.search, - location.state - ); - - if (match) { - matchResult = match; - break; +const makeGetEntryResource = () => { + let currentLocation: FiftyOneLocation; + let currentResource: Resource>; + + const isReusable = (location: FiftyOneLocation) => { + if (currentLocation) { + return ( + location.state.event === "modal" || + currentLocation?.state.event === "modal" + ); } - } - if (matchResult == null) { - throw new NotFoundError({ path: location.pathname }); - } + return false; + }; - const fetchPolicy = hard ? "network-only" : "store-or-network"; + const getEntryResource = ({ + environment, + handleError, + hard = false, + location, + routes, + }: { + current?: FiftyOneLocation; + environment: Environment; + routes: RouteDefinition[]; + location: FiftyOneLocation; + hard: boolean; + handleError?: (error: unknown) => void; + }): Resource> => { + if (isReusable(location)) { + throw currentResource; + } - return new Resource(() => { - return Promise.all([route.component.load(), route.query.load()]).then( - ([component, concreteRequest]) => { - const preloadedQuery = loadQuery( - environment, - concreteRequest, - matchResult.variables || {}, - { - fetchPolicy, - } - ); - - let resolveEntry: (entry: Entry) => void; - const promise = new Promise>((resolve) => { - resolveEntry = resolve; - }); - const subscription = fetchQuery( - environment, - concreteRequest, - matchResult.variables || {}, - { fetchPolicy } - ).subscribe({ - next: (data) => { - const { state: _, ...rest } = location; - resolveEntry({ - state: matchResult.variables as LocationState, - ...rest, - component, - data, - concreteRequest, - preloadedQuery, - cleanup: () => { - subscription?.unsubscribe(); - }, - }); - }, - error: (error) => handleError?.(error), - }); + let route: RouteDefinition; + let matchResult: MatchPathResult | undefined = undefined; + for (let index = 0; index < routes.length; index++) { + route = routes[index]; + const match = matchPath( + location.pathname, + route, + location.search, + location.state + ); - return promise; + if (match) { + matchResult = match; + break; } - ); - }); + } + + if (matchResult == null) { + throw new NotFoundError({ path: location.pathname }); + } + + const fetchPolicy = hard ? "network-only" : "store-or-network"; + + currentLocation = location; + currentResource = new Resource(() => { + return Promise.all([route.component.load(), route.query.load()]).then( + ([component, concreteRequest]) => { + const preloadedQuery = loadQuery( + environment, + concreteRequest, + matchResult.variables || {}, + { + fetchPolicy, + } + ); + + let resolveEntry: (entry: Entry) => void; + const promise = new Promise>((resolve) => { + resolveEntry = resolve; + }); + const subscription = fetchQuery( + environment, + concreteRequest, + matchResult.variables || {}, + { fetchPolicy } + ).subscribe({ + next: (data) => { + const { state: _, ...rest } = location; + resolveEntry({ + state: matchResult.variables as LocationState, + ...rest, + component, + data, + concreteRequest, + preloadedQuery, + cleanup: () => { + subscription?.unsubscribe(); + }, + }); + }, + error: (error) => handleError?.(error), + }); + + return promise; + } + ); + }); + + return currentResource; + }; + + return getEntryResource; }; export const RouterContext = React.createContext< diff --git a/app/packages/core/src/components/Grid/useRefreshers.ts b/app/packages/core/src/components/Grid/useRefreshers.ts index f811d36b1a..10922ab09f 100644 --- a/app/packages/core/src/components/Grid/useRefreshers.ts +++ b/app/packages/core/src/components/Grid/useRefreshers.ts @@ -63,13 +63,8 @@ export default function useRefreshers() { useEffect( () => - subscribe(({ event }, { reset }, previous) => { - if ( - event === "fieldVisibility" || - event === "modal" || - previous?.event === "modal" - ) - return; + subscribe(({ event }, { reset }) => { + if (event === "fieldVisibility") return; // if not a modal page change, reset the grid location reset(gridAt); diff --git a/app/packages/state/src/recoil/sidebarExpanded.ts b/app/packages/state/src/recoil/sidebarExpanded.ts index 2c83c5f8cd..2e3044e9d3 100644 --- a/app/packages/state/src/recoil/sidebarExpanded.ts +++ b/app/packages/state/src/recoil/sidebarExpanded.ts @@ -1,4 +1,3 @@ -import { subscribe } from "@fiftyone/relay"; import { DefaultValue, atom, atomFamily, selectorFamily } from "recoil"; export const sidebarExpandedStore = atomFamily< @@ -7,12 +6,6 @@ export const sidebarExpandedStore = atomFamily< >({ key: "sidebarExpandedStore", default: {}, - effects: [ - ({ node }) => - subscribe(({ event }, { reset }, previous) => { - event !== "modal" && previous?.event !== "modal" && reset(node); - }), - ], }); export const sidebarExpanded = selectorFamily< @@ -36,13 +29,6 @@ export const sidebarExpanded = selectorFamily< export const granularSidebarExpandedStore = atom<{ [key: string]: boolean }>({ key: "granularSidebarExpandedStore", default: {}, - effects: [ - ({ node }) => - subscribe( - ({ event }, { set }, previous) => - event !== "modal" && previous?.event !== "modal" && set(node, {}) - ), - ], }); export const granularSidebarExpanded = selectorFamily({ From 263b696abb290d82bb7ff99b1fc84a2a32c6d43a Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Thu, 17 Oct 2024 16:51:15 -0400 Subject: [PATCH 2/5] cache gathered paths --- .../state/src/recoil/pathData/counts.ts | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/app/packages/state/src/recoil/pathData/counts.ts b/app/packages/state/src/recoil/pathData/counts.ts index 6059375fcd..1de0e8d8eb 100644 --- a/app/packages/state/src/recoil/pathData/counts.ts +++ b/app/packages/state/src/recoil/pathData/counts.ts @@ -146,6 +146,21 @@ export const counts = selectorFamily({ }, }); +export const gatheredPaths = selectorFamily({ + key: "gatheredPaths", + get: + ({ + embeddedDocType, + ftype, + }: { + embeddedDocType?: string | string[]; + ftype: string | string[]; + }) => + ({ get }) => { + return [...new Set(gatherPaths(get, ftype, embeddedDocType))]; + }, +}); + export const cumulativeCounts = selectorFamily< { [key: string]: number }, { @@ -160,7 +175,7 @@ export const cumulativeCounts = selectorFamily< get: ({ extended, path: key, modal, ftype, embeddedDocType }) => ({ get }) => { - return [...new Set(gatherPaths(get, ftype, embeddedDocType))].reduce( + return get(gatheredPaths({ ftype, embeddedDocType })).reduce( (result, path) => { const data = get(counts({ extended, modal, path: `${path}.${key}` })); for (const value in data) { From fb56e2338fd4a257b07fa0dd82363d06d301f379 Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Mon, 21 Oct 2024 09:44:57 -0400 Subject: [PATCH 3/5] e2e updates --- app/packages/app/src/routing/RouterContext.ts | 6 ++++-- app/packages/app/src/routing/matchPath.ts | 2 +- app/packages/app/src/useEvents/useSetGroupSlice.ts | 6 +++++- app/packages/app/src/useWriters/onSetGroupSlice.ts | 6 +++++- e2e-pw/src/oss/poms/grid/index.ts | 8 +++----- e2e-pw/src/oss/poms/modal/index.ts | 14 +++++--------- 6 files changed, 23 insertions(+), 19 deletions(-) diff --git a/app/packages/app/src/routing/RouterContext.ts b/app/packages/app/src/routing/RouterContext.ts index d33b0623b4..6f25d6d46f 100644 --- a/app/packages/app/src/routing/RouterContext.ts +++ b/app/packages/app/src/routing/RouterContext.ts @@ -178,6 +178,8 @@ export const createRouter = ( }; }; +const SKIP_EVENTS = new Set(["modal", "slice"]); + const makeGetEntryResource = () => { let currentLocation: FiftyOneLocation; let currentResource: Resource>; @@ -185,8 +187,8 @@ const makeGetEntryResource = () => { const isReusable = (location: FiftyOneLocation) => { if (currentLocation) { return ( - location.state.event === "modal" || - currentLocation?.state.event === "modal" + SKIP_EVENTS.has(location.state.event || "") || + SKIP_EVENTS.has(currentLocation?.state.event || "") ); } diff --git a/app/packages/app/src/routing/matchPath.ts b/app/packages/app/src/routing/matchPath.ts index b8c7533359..1223647081 100644 --- a/app/packages/app/src/routing/matchPath.ts +++ b/app/packages/app/src/routing/matchPath.ts @@ -10,7 +10,7 @@ const compilePath = (path: string) => }); export type LocationState = { - event?: "modal"; + event?: "modal" | "slice"; fieldVisibility?: State.FieldVisibilityStage; groupSlice?: string; modalSelector?: ModalSelector; diff --git a/app/packages/app/src/useEvents/useSetGroupSlice.ts b/app/packages/app/src/useEvents/useSetGroupSlice.ts index 9ef152cea5..a58ba62754 100644 --- a/app/packages/app/src/useEvents/useSetGroupSlice.ts +++ b/app/packages/app/src/useEvents/useSetGroupSlice.ts @@ -15,7 +15,11 @@ const useSetGroupSlice: EventHandlerHook = ({ router, session }) => { session.current.sessionGroupSlice = slice; }); - router.push(pathname, { ...router.location.state, groupSlice: slice }); + router.push(pathname, { + ...router.location.state, + event: "slice", + groupSlice: slice, + }); }, [router, session] ); diff --git a/app/packages/app/src/useWriters/onSetGroupSlice.ts b/app/packages/app/src/useWriters/onSetGroupSlice.ts index f2a58d37ba..4045cac469 100644 --- a/app/packages/app/src/useWriters/onSetGroupSlice.ts +++ b/app/packages/app/src/useWriters/onSetGroupSlice.ts @@ -13,7 +13,11 @@ const onSetGroupSlice: RegisteredWriter<"sessionGroupSlice"> = const pathname = router.history.location.pathname + string; - router.push(pathname, { ...router.location.state, groupSlice: slice }); + router.push(pathname, { + ...router.location.state, + event: "slice", + groupSlice: slice, + }); if (env().VITE_NO_STATE) return; commitMutation(environment, { diff --git a/e2e-pw/src/oss/poms/grid/index.ts b/e2e-pw/src/oss/poms/grid/index.ts index daa42cd6f0..b05ab5e870 100644 --- a/e2e-pw/src/oss/poms/grid/index.ts +++ b/e2e-pw/src/oss/poms/grid/index.ts @@ -1,4 +1,4 @@ -import { expect, Locator, Page } from "src/oss/fixtures"; +import { Locator, Page, expect } from "src/oss/fixtures"; import { EventUtils } from "src/shared/event-utils"; import { GridActionsRowPom } from "../action-row/grid-actions-row"; import { GridSliceSelectorPom } from "../action-row/grid-slice-selector"; @@ -52,9 +52,7 @@ export class GridPom { } async openNthSample(n: number) { - await this.url.pageChange(() => - this.getNthLooker(n).click({ position: { x: 10, y: 80 } }) - ); + await this.getNthLooker(n).click({ position: { x: 10, y: 80 } }); } async openFirstSample() { @@ -80,7 +78,7 @@ export class GridPom { } async selectSlice(slice: string) { - await this.url.pageChange(() => this.sliceSelector.selectSlice(slice)); + await this.sliceSelector.selectSlice(slice); } /** diff --git a/e2e-pw/src/oss/poms/modal/index.ts b/e2e-pw/src/oss/poms/modal/index.ts index 176856107f..33a8e58cdc 100644 --- a/e2e-pw/src/oss/poms/modal/index.ts +++ b/e2e-pw/src/oss/poms/modal/index.ts @@ -1,4 +1,4 @@ -import { expect, Locator, Page } from "src/oss/fixtures"; +import { Locator, Page, expect } from "src/oss/fixtures"; import { EventUtils } from "src/shared/event-utils"; import { Duration } from "../../utils"; import { ModalTaggerPom } from "../action-row/tagger/modal-tagger"; @@ -118,11 +118,9 @@ export class ModalPom { ) { const currentSampleId = await this.sidebar.getSampleId(); - await this.url.pageChange(() => - this.locator - .getByTestId(`nav-${direction === "forward" ? "right" : "left"}-button`) - .click() - ); + await this.locator + .getByTestId(`nav-${direction === "forward" ? "right" : "left"}-button`) + .click(); // wait for sample id to change await this.page.waitForFunction((currentSampleId) => { @@ -219,9 +217,7 @@ export class ModalPom { async close() { // close by clicking outside of modal - await this.url.pageChange(() => - this.page.click("body", { position: { x: 0, y: 0 } }) - ); + await this.page.click("body", { position: { x: 0, y: 0 } }); } async navigateNextSample(allowErrorInfo = false) { From 3c5109d378f340a32b85dc9673fb4821628f3014 Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Mon, 21 Oct 2024 11:17:47 -0400 Subject: [PATCH 4/5] update selection test --- e2e-pw/src/oss/specs/selection.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e-pw/src/oss/specs/selection.spec.ts b/e2e-pw/src/oss/specs/selection.spec.ts index 604b0c015b..858ab29d3f 100644 --- a/e2e-pw/src/oss/specs/selection.spec.ts +++ b/e2e-pw/src/oss/specs/selection.spec.ts @@ -95,7 +95,7 @@ extensionDatasetNamePairs.forEach(([extension, datasetName]) => { await grid.openNthSample(1); await modal.assert.verifySelectionCount(1); - await grid.url.back(); + await modal.close(); await modal.assert.isClosed(); await grid.assert.isSelectionCountEqualTo(1); }); From 21ef687788df299fa58f649283643e9fceb4855d Mon Sep 17 00:00:00 2001 From: Benjamin Kane Date: Tue, 22 Oct 2024 11:06:44 -0400 Subject: [PATCH 5/5] lint, add comments --- app/packages/app/src/routing/RouterContext.ts | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/app/packages/app/src/routing/RouterContext.ts b/app/packages/app/src/routing/RouterContext.ts index 6f25d6d46f..92a2114e16 100644 --- a/app/packages/app/src/routing/RouterContext.ts +++ b/app/packages/app/src/routing/RouterContext.ts @@ -90,8 +90,13 @@ export const createRouter = ( hard: false, handleError, }); - } catch { - return; + } catch (e) { + if (e instanceof Resource) { + // skip the page change if a resource is thrown + return; + } + + throw e; } requestAnimationFrame(() => { @@ -210,6 +215,7 @@ const makeGetEntryResource = () => { handleError?: (error: unknown) => void; }): Resource> => { if (isReusable(location)) { + // throw the current resource (page) if it can be reused throw currentResource; }