Fix modal linking (#4618)

* id fixes, testing updates

* cleanup

* update for searchParams page load

* undo

app/packages/app/src/useEventSource.ts

@@ -58,7 +58,7 @@ const useEventSource = (
           dataset: getDatasetName(),
           group_id: getParam("groupId"),
           group_slice: getParam("slice"),
-          sample_id: getParam("sampleId"),
+          sample_id: getParam("id"),
           view: getParam("view"),
           workspace: getParam("workspace"),
         },

app/packages/app/src/useEvents/useStateUpdate.ts

@@ -29,8 +29,8 @@ const useStateUpdate: EventHandlerHook = ({
           : (payload.state.saved_view_slug as string),
         extra: {
           groupId: state.modalSelector?.groupId || null,
-          slice: state.groupSlice || null,
-          sampleId: state.modalSelector?.id || null,
+          id: state.modalSelector?.id || null,
+          slice: stateless ? getParam("slice") : state.groupSlice || null,
           workspace: state.workspace?._name || null,
         },
       });

app/packages/app/src/useEvents/utils.ts

@@ -7,6 +7,7 @@ import { env, toCamelCase } from "@fiftyone/utilities";
 import { atom } from "recoil";
 import type { DatasetPageQuery } from "../pages/datasets/__generated__/DatasetPageQuery.graphql";
 import type { LocationState } from "../routing";
+import { getParam } from "../utils";
 import { AppReadyState } from "./registerEvent";
 
 export const appReadyState = atom<AppReadyState>({
@@ -23,43 +24,102 @@ export const processState = (
       state.color_scheme as ColorSchemeInput
     );
 
-    if (env().VITE_NO_STATE) {
-      session.sessionGroupSlice = data.dataset?.defaultGroupSlice || undefined;
-      unsubscribe();
-      return;
-    }
-
-    session.sessionGroupSlice = state.group_slice as string;
-    session.selectedLabels = toCamelCase(
-      state.selected_labels as object
-    ) as State.SelectedLabel[];
-    session.selectedSamples = new Set(state.selected as string[]);
-    session.sessionSpaces = (state.spaces ||
-      session.sessionSpaces) as SpaceNodeJSON;
-    session.fieldVisibilityStage =
-      (state.field_visibility_stage as State.FieldVisibilityStage) || undefined;
+    session.sessionGroupSlice =
+      groupSlice || data.dataset?.defaultGroupSlice || undefined;
+    session.selectedLabels = resolveSelectedLabels(state);
+    session.selectedSamples = resolveSelected(state);
+    session.sessionSpaces = workspace;
+    session.fieldVisibilityStage = fieldVisibility;
     session.modalSelector = modalSelector;
 
     unsubscribe();
   });
 
-  if (env().VITE_NO_STATE) {
-    return { view: [], fieldVisibility: undefined };
-  }
-
-  const modalSelector =
-    state.group_id || state.sample_id
-      ? {
-          groupId: state.group_id as string,
-          id: state.sample_id as string,
-        }
-      : undefined;
+  const fieldVisibility = resolveFieldVisibility(state);
+  const groupSlice = resolveGroupSlice(state);
+  const modalSelector = resolveModalSelector(state);
+  const view = resolveView(state);
+  const workspace = resolveWorkspace(session, state);
 
   return {
-    fieldVisibility: state.field_visibility_stage as State.FieldVisibilityStage,
-    groupSlice: state.group_slice as string,
+    fieldVisibility,
+    groupSlice,
     modalSelector,
-    view: state.view || [],
-    workspace: state.spaces as SpaceNodeJSON,
+    view,
+    workspace,
   };
 };
+const resolveSelected = (state: { selected?: string[] }) => {
+  if (env().VITE_NO_STATE) {
+    return new Set<string>();
+  }
+
+  return new Set<string>(state.selected || []);
+};
+
+const resolveSelectedLabels = (state: { selected_labels?: string[] }) => {
+  if (env().VITE_NO_STATE) {
+    return [];
+  }
+
+  return (
+    (toCamelCase(state.selected_labels as object) as State.SelectedLabel[]) ||
+    []
+  );
+};
+
+const resolveFieldVisibility = (state: {
+  field_visibility?: State.FieldVisibilityStage;
+}) => {
+  if (env().VITE_NO_STATE) {
+    return undefined;
+  }
+
+  return state.field_visibility;
+};
+
+const resolveGroupSlice = (state: { group_slice?: string }) => {
+  if (env().VITE_NO_STATE) {
+    return getParam("slice") || undefined;
+  }
+
+  return state.group_slice;
+};
+
+const resolveModalSelector = (state: {
+  group_id?: string;
+  sample_id?: string;
+}) => {
+  if (env().VITE_NO_STATE) {
+    const groupId = getParam("groupId") || undefined;
+    const id = getParam("id") || undefined;
+
+    return groupId || id ? { groupId, id } : undefined;
+  }
+
+  return state.group_id || state.sample_id
+    ? {
+        groupId: state.group_id as string,
+        id: state.sample_id as string,
+      }
+    : undefined;
+};
+
+const resolveView = (state: { view?: object[] }) => {
+  if (env().VITE_NO_STATE) {
+    return [];
+  }
+
+  return state.view || [];
+};
+
+const resolveWorkspace = (
+  session: Session,
+  state: { spaces?: SpaceNodeJSON }
+) => {
+  if (env().VITE_NO_STATE) {
+    return session.sessionSpaces;
+  }
+
+  return state.spaces || session.sessionSpaces;
+};

app/packages/app/src/useSetters/onSetDataset.ts

@@ -48,7 +48,7 @@ const onSetDataset: RegisteredSetter =
         nextDataset: datasetName || null,
         extra: {
           groupId: null,
-          sampleId: null,
+          id: null,
           slice: null,
           workspace: null,
         },

app/packages/utilities/src/index.ts

@@ -4,16 +4,16 @@ import mime from "mime";
 import { isElectron } from "./electron";
 import { Field } from "./schema";
 
-export * from "./Resource";
 export * from "./color";
 export * from "./electron";
 export * from "./errors";
 export * from "./fetch";
+export * from "./order";
 export * from "./paths";
+export * from "./Resource";
 export * from "./schema";
 export * from "./styles";
 export * from "./type-check";
-export * from "./order";
 
 interface O {
   [key: string]: O | any;

e2e-pw/src/oss/fixtures/loader.ts

@@ -121,15 +121,15 @@ export class OssLoader extends AbstractFiftyoneLoader {
     datasetName: string,
     options?: WaitUntilGridVisibleOptions
   ) {
-    const { isEmptyDataset, savedView, withGrid } = options ?? {
+    const { isEmptyDataset, searchParams, withGrid } = options ?? {
       isEmptyDataset: false,
-      savedView: undefined,
+      searchParams: undefined,
       withGrid: true,
     };
 
     const forceDatasetFromSelector = async () => {
       await page.goto("/");
-      await page.getByTestId(`selector-Select dataset`).click();
+      await page.getByTestId("selector-Select dataset").click();
 
       if (datasetName) {
         await page.getByTestId(`selector-result-${datasetName}`).click();
@@ -141,8 +141,9 @@ export class OssLoader extends AbstractFiftyoneLoader {
       }
     };
 
-    if (savedView) {
-      await page.goto(`/datasets/${datasetName}?view=${savedView}`);
+    const search = searchParams ? searchParams.toString() : undefined;
+    if (search) {
+      await page.goto(`/datasets/${datasetName}?${search}`);
     } else {
       await page.goto(`/datasets/${datasetName}`);
     }
@@ -152,11 +153,12 @@ export class OssLoader extends AbstractFiftyoneLoader {
       await forceDatasetFromSelector();
     }
 
-    if (savedView) {
+    const view = searchParams?.get("view");
+    if (view) {
       const search = await page.evaluate(() => window.location.search);
 
       const params = new URLSearchParams(search);
-      if (params.get("view") !== savedView) {
+      if (params.get("view") !== view) {
         throw new Error(`wrong view: '${params.get("view")}'`);
       }
     }

e2e-pw/src/oss/specs/groups/dynamic-groups.spec.ts

@@ -54,7 +54,7 @@ extensionDatasetNamePairs.forEach(([extension, datasetName]) => {
     modal,
   }) => {
     await fiftyoneLoader.waitUntilGridVisible(page, datasetName, {
-      savedView: "dynamic-group",
+      searchParams: new URLSearchParams({ view: "dynamic-group" }),
     });
 
     await grid.assert.isEntryCountTextEqualTo("10 groups");
@@ -74,7 +74,7 @@ extensionDatasetNamePairs.forEach(([extension, datasetName]) => {
     modal,
   }) => {
     await fiftyoneLoader.waitUntilGridVisible(page, datasetName, {
-      savedView: "dynamic-group",
+      searchParams: new URLSearchParams({ view: "dynamic-group" }),
     });
     await grid.openFirstSample();
 

e2e-pw/src/oss/specs/groups/sparse-dynamic-groups.spec.ts

@@ -87,7 +87,7 @@ test(`left slice (default)`, async ({ fiftyoneLoader, page, grid, modal }) => {
 
 test(`right slice`, async ({ fiftyoneLoader, page, grid, modal }) => {
   await fiftyoneLoader.waitUntilGridVisible(page, datasetName, {
-    savedView: "group",
+    searchParams: new URLSearchParams({ view: "group" }),
   });
   await grid.selectSlice("right");
   await grid.assert.isEntryCountTextEqualTo("1 group with slice");

e2e-pw/src/oss/specs/modal/linking.spec.ts

@@ -0,0 +1,61 @@
+import { test as base } from "src/oss/fixtures";
+import { ModalPom } from "src/oss/poms/modal";
+import { getUniqueDatasetNameWithPrefix } from "src/oss/utils";
+
+const test = base.extend<{ modal: ModalPom }>({
+  modal: async ({ page, eventUtils }, use) => {
+    await use(new ModalPom(page, eventUtils));
+  },
+});
+
+const datasetName = getUniqueDatasetNameWithPrefix("linking");
+const groupDatasetName = getUniqueDatasetNameWithPrefix("group-linking");
+
+const id = "000000000000000000000000";
+
+test.beforeAll(async ({ fiftyoneLoader }) => {
+  await fiftyoneLoader.executePythonCode(`
+    from bson import ObjectId
+
+    import fiftyone as fo
+
+    dataset = fo.Dataset("${datasetName}")
+    dataset.persistent = True
+
+    sample = fo.Sample(_id=ObjectId("${id}"), filepath="sample.png")
+    dataset._sample_collection.insert_many(
+        [dataset._make_dict(sample, include_id=True)]
+    )
+
+
+    group_dataset = fo.Dataset("${groupDatasetName}")
+    group_dataset.persistent = True
+
+    group = fo.Group(id="${id}")
+    group_sample = fo.Sample(
+        filepath="group_sample.png", group=group.element("only")
+    )
+    group_dataset.add_sample(group_sample)`);
+});
+
+test(`sample linking`, async ({ page, fiftyoneLoader, modal }) => {
+  await fiftyoneLoader.waitUntilGridVisible(page, datasetName, {
+    searchParams: new URLSearchParams({ id }),
+  });
+
+  await modal.waitForSampleLoadDomAttribute(true);
+
+  await modal.assert.isOpen();
+  await modal.sidebar.assert.verifySidebarEntryText("id", id);
+});
+
+test(`group linking`, async ({ page, fiftyoneLoader, modal }) => {
+  await fiftyoneLoader.waitUntilGridVisible(page, groupDatasetName, {
+    searchParams: new URLSearchParams({ groupId: id }),
+  });
+
+  await modal.waitForSampleLoadDomAttribute(true);
+
+  await modal.assert.isOpen();
+  await modal.sidebar.assert.verifySidebarEntryText("group.id", id);
+});

e2e-pw/src/oss/specs/regression-tests/grid-page.spec.ts

@@ -69,7 +69,7 @@ test("modal dynamic group carousel has correct second page (all 21 samples)", as
   page,
 }) => {
   await fiftyoneLoader.waitUntilGridVisible(page, datasetName, {
-    savedView: "group",
+    searchParams: new URLSearchParams({ view: "group" }),
   });
   await grid.openFirstSample();
   await modal.group.setDynamicGroupsNavigationMode("carousel");

e2e-pw/src/oss/specs/smoke-tests/quickstart-groups-dynamic.spec.ts

@@ -54,7 +54,7 @@ test.describe("quickstart-groups", () => {
 
   test.beforeEach(async ({ page, fiftyoneLoader }) => {
     await fiftyoneLoader.waitUntilGridVisible(page, datasetName, {
-      savedView: "dynamic",
+      searchParams: new URLSearchParams({ view: "dynamic" }),
     });
   });
 

e2e-pw/src/shared/abstract-loader.ts

@@ -8,9 +8,9 @@ export type WaitUntilGridVisibleOptions = {
   isEmptyDataset?: boolean;
 
   /**
-   * Name of the saved view to be loaded.
+   * Search parameters to include
    */
-  savedView?: string;
+  searchParams?: URLSearchParams;
 
   /**
    * Whether to wait for the grid to be visible.