Skip to content
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

Add modal routing optimization #5014

Merged
merged 5 commits into from
Oct 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
211 changes: 129 additions & 82 deletions app/packages/app/src/routing/RouterContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ export const createRouter = <T extends OperationType>(
): Router<T> => {
const history = isNotebook() ? createMemoryHistory() : createBrowserHistory();

const getEntryResource = makeGetEntryResource<T>();

let currentEntryResource: Resource<Entry<T>>;
let nextCurrentEntryResource: Resource<Entry<T>>;

Expand All @@ -79,17 +81,27 @@ export const createRouter = <T extends OperationType>(
>();

const update = (location: FiftyOneLocation, action?: Action) => {
requestAnimationFrame(() => {
for (const [_, [__, onPending]] of subscribers) onPending?.();
});
currentEntryResource.load().then(({ cleanup }) => {
nextCurrentEntryResource = getEntryResource<T>(
environment,
routes,
location as FiftyOneLocation,
false,
handleError
);
try {
nextCurrentEntryResource = getEntryResource({
environment,
routes,
location: location as FiftyOneLocation,
hard: false,
handleError,
});
} catch (e) {
if (e instanceof Resource) {
// skip the page change if a resource is thrown
return;
}

throw e;
}

requestAnimationFrame(() => {
for (const [_, [__, onPending]] of subscribers) onPending?.();
});

const loadingResource = nextCurrentEntryResource;
loadingResource.load().then((entry) => {
Expand Down Expand Up @@ -135,13 +147,13 @@ export const createRouter = <T extends OperationType>(
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();
Expand Down Expand Up @@ -171,79 +183,114 @@ export const createRouter = <T extends OperationType>(
};
};

const getEntryResource = <T extends OperationType>(
environment: Environment,
routes: RouteDefinition<T>[],
location: FiftyOneLocation,
hard = false,
handleError?: (error: unknown) => void
): Resource<Entry<T>> => {
let route: RouteDefinition<T>;
let matchResult: MatchPathResult<T> | undefined = undefined;
for (let index = 0; index < routes.length; index++) {
route = routes[index];
const match = matchPath<T>(
location.pathname,
route,
location.search,
location.state
);

if (match) {
matchResult = match;
break;
const SKIP_EVENTS = new Set(["modal", "slice"]);

const makeGetEntryResource = <T extends OperationType>() => {
let currentLocation: FiftyOneLocation;
let currentResource: Resource<Entry<T>>;

const isReusable = (location: FiftyOneLocation) => {
if (currentLocation) {
return (
SKIP_EVENTS.has(location.state.event || "") ||
SKIP_EVENTS.has(currentLocation?.state.event || "")
);
}
}

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<T>[];
location: FiftyOneLocation;
hard: boolean;
handleError?: (error: unknown) => void;
}): Resource<Entry<T>> => {
if (isReusable(location)) {
// throw the current resource (page) if it can be reused
throw currentResource;
}
Comment on lines +217 to +220
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refactoring control flow instead of throwing currentResource

Throwing currentResource to manage control flow can be confusing and may complicate error handling. Consider refactoring to return early or use a different pattern to indicate resource reuse.

Apply this diff to refactor the control flow:

    if (isReusable(location)) {
-     // throw the current resource (page) if it can be reused
-     throw currentResource;
+     // return the current resource for reuse
+     return currentResource;
    }

Committable suggestion was skipped due to low confidence.


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<T>) => void;
const promise = new Promise<Entry<T>>((resolve) => {
resolveEntry = resolve;
});
const subscription = fetchQuery(
environment,
concreteRequest,
matchResult.variables || {},
{ fetchPolicy }
).subscribe({
next: (data) => {
const { state: _, ...rest } = location;
resolveEntry({
state: matchResult.variables as LocationState<T>,
...rest,
component,
data,
concreteRequest,
preloadedQuery,
cleanup: () => {
subscription?.unsubscribe();
},
});
},
error: (error) => handleError?.(error),
});
let route: RouteDefinition<T>;
let matchResult: MatchPathResult<T> | undefined = undefined;
for (let index = 0; index < routes.length; index++) {
route = routes[index];
const match = matchPath<T>(
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<T>) => void;
const promise = new Promise<Entry<T>>((resolve) => {
resolveEntry = resolve;
});
const subscription = fetchQuery(
environment,
concreteRequest,
matchResult.variables || {},
{ fetchPolicy }
).subscribe({
next: (data) => {
const { state: _, ...rest } = location;
resolveEntry({
state: matchResult.variables as LocationState<T>,
...rest,
component,
data,
concreteRequest,
preloadedQuery,
cleanup: () => {
subscription?.unsubscribe();
},
});
},
error: (error) => handleError?.(error),
});

return promise;
Comment on lines +258 to +285
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle promise rejection in error callback of fetchQuery

In the error callback of fetchQuery, the promise created for currentResource is neither resolved nor rejected. This may cause the promise to remain pending indefinitely, leading to potential memory leaks or unhandled promise rejections.

Apply this diff to reject the promise in the error callback:

              let resolveEntry: (entry: Entry<T>) => void;
+             let rejectEntry: (error: unknown) => void;
              const promise = new Promise<Entry<T>>((resolve, reject) => {
                resolveEntry = resolve;
+               rejectEntry = reject;
              });
              const subscription = fetchQuery(
                environment,
                concreteRequest,
                matchResult.variables || {},
                { fetchPolicy }
              ).subscribe({
                next: (data) => {
                  const { state: _, ...rest } = location;
                  resolveEntry({
                    state: matchResult.variables as LocationState<T>,
                    ...rest,
                    component,
                    data,
                    concreteRequest,
                    preloadedQuery,
                    cleanup: () => {
                      subscription?.unsubscribe();
                    },
                  });
                },
                error: (error) => {
+                 rejectEntry(error);
                  handleError?.(error);
                },
              });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let resolveEntry: (entry: Entry<T>) => void;
const promise = new Promise<Entry<T>>((resolve) => {
resolveEntry = resolve;
});
const subscription = fetchQuery(
environment,
concreteRequest,
matchResult.variables || {},
{ fetchPolicy }
).subscribe({
next: (data) => {
const { state: _, ...rest } = location;
resolveEntry({
state: matchResult.variables as LocationState<T>,
...rest,
component,
data,
concreteRequest,
preloadedQuery,
cleanup: () => {
subscription?.unsubscribe();
},
});
},
error: (error) => handleError?.(error),
});
return promise;
let resolveEntry: (entry: Entry<T>) => void;
let rejectEntry: (error: unknown) => void;
const promise = new Promise<Entry<T>>((resolve, reject) => {
resolveEntry = resolve;
rejectEntry = reject;
});
const subscription = fetchQuery(
environment,
concreteRequest,
matchResult.variables || {},
{ fetchPolicy }
).subscribe({
next: (data) => {
const { state: _, ...rest } = location;
resolveEntry({
state: matchResult.variables as LocationState<T>,
...rest,
component,
data,
concreteRequest,
preloadedQuery,
cleanup: () => {
subscription?.unsubscribe();
},
});
},
error: (error) => {
rejectEntry(error);
handleError?.(error);
},
});
return promise;

}
);
});

return currentResource;
};

return getEntryResource;
};

export const RouterContext = React.createContext<
Expand Down
2 changes: 1 addition & 1 deletion app/packages/app/src/routing/matchPath.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const compilePath = (path: string) =>
});

export type LocationState<T extends OperationType = OperationType> = {
event?: "modal";
event?: "modal" | "slice";
fieldVisibility?: State.FieldVisibilityStage;
groupSlice?: string;
modalSelector?: ModalSelector;
Expand Down
6 changes: 5 additions & 1 deletion app/packages/app/src/useEvents/useSetGroupSlice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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]
);
Expand Down
6 changes: 5 additions & 1 deletion app/packages/app/src/useWriters/onSetGroupSlice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});
Comment on lines +16 to +20
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using a constant or enum for the event type.

While the implementation is correct, the hardcoded "slice" string could be moved to a shared constant or enum to improve maintainability and type safety across the codebase.

Consider creating a shared events enum:

// routerEvents.ts
export enum RouterEvent {
  SLICE = "slice"
}

Then update the router push:

    router.push(pathname, {
      ...router.location.state,
-     event: "slice",
+     event: RouterEvent.SLICE,
      groupSlice: slice,
    });


if (env().VITE_NO_STATE) return;
commitMutation<setGroupSliceMutation>(environment, {
Expand Down
9 changes: 2 additions & 7 deletions app/packages/core/src/components/Grid/useRefreshers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
17 changes: 16 additions & 1 deletion app/packages/state/src/recoil/pathData/counts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
{
Expand All @@ -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) {
Expand Down
14 changes: 0 additions & 14 deletions app/packages/state/src/recoil/sidebarExpanded.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { subscribe } from "@fiftyone/relay";
import { DefaultValue, atom, atomFamily, selectorFamily } from "recoil";

export const sidebarExpandedStore = atomFamily<
Expand All @@ -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<
Expand All @@ -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<boolean, string>({
Expand Down
8 changes: 3 additions & 5 deletions e2e-pw/src/oss/poms/grid/index.ts
Original file line number Diff line number Diff line change
@@ -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";
Expand Down Expand Up @@ -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() {
Expand All @@ -80,7 +78,7 @@ export class GridPom {
}

async selectSlice(slice: string) {
await this.url.pageChange(() => this.sliceSelector.selectSlice(slice));
await this.sliceSelector.selectSlice(slice);
}

/**
Expand Down
Loading
Loading