Item search query parameter modernisation (#1040)

* await labels and locations properly

* update query params with every search

* don't persist default settings in query params

* conceptualize optional parameters

* add run script for development

* lint

* consider typescript

* remove run.sh

* capitalize QueryParamValue

* make query parameter updates predictable

This reverts commit 5c0c48cea5.

* capitalize typename again

---------

Co-authored-by: Benji <benji@DG-SM-7059.local>
Co-authored-by: Benji <benji@mac.home.internal>
Co-authored-by: Benji <benji@dg-sm-7059.home.internal>

frontend/layouts/default.vue

@@ -368,12 +368,12 @@
   ]);
 
   const labelStore = useLabelStore();
+  labelStore.ensureAllLabelsFetched();
 
   const locationStore = useLocationStore();
+  locationStore.ensureLocationsFetched();
 
   onMounted(() => {
-    labelStore.refresh();
-    locationStore.refreshChildren();
     locationStore.refreshParents();
     locationStore.refreshTree();
   });

frontend/pages/items.vue

@@ -17,6 +17,7 @@
   import BaseContainer from "@/components/Base/Container.vue";
   import SearchFilter from "~/components/Search/Filter.vue";
   import ItemViewSelectable from "~/components/Item/View/Selectable.vue";
+  import type { LocationQueryRaw } from "vue-router";
 
   const { t } = useI18n();
 
@@ -37,7 +38,41 @@
   const items = ref<ItemSummary[]>([]);
   const total = ref(0);
 
-  const page1 = useRouteQuery("page", 1);
+  // Using useRouteQuery directly has two downsides
+  // 1. It persists the default value in the query string
+  // 2. The ref returned by useRouteQuery updates asynchronously after calling the setter.
+  //    This can cause unintuitive behaviors.
+  // -> We copy query parameters into separate refs on page load and update the query explicitly via `router.push`.
+  type QueryParamValue = string | string[] | number | boolean;
+  type QueryRef = Ref<boolean | string | string[] | number, boolean | string | string[] | number>;
+  const queryParamDefaultValues: Record<string, QueryParamValue> = {};
+  function useOptionalRouteQuery(key: string, defaultValue: string): Ref<string>;
+  function useOptionalRouteQuery(key: string, defaultValue: string[]): Ref<string[]>;
+  function useOptionalRouteQuery(key: string, defaultValue: number): Ref<number>;
+  function useOptionalRouteQuery(key: string, defaultValue: boolean): Ref<boolean>;
+  function useOptionalRouteQuery(key: string, defaultValue: QueryParamValue): QueryRef {
+    queryParamDefaultValues[key] = defaultValue;
+    if (typeof defaultValue === "string") {
+      const val = useRouteQuery(key, defaultValue);
+      return ref(val.value);
+    }
+    if (Array.isArray(defaultValue)) {
+      const val = useRouteQuery(key, defaultValue);
+      return ref(val.value);
+    }
+    if (typeof defaultValue === "number") {
+      const val = useRouteQuery(key, defaultValue);
+      return ref(val.value);
+    }
+    if (typeof defaultValue === "boolean") {
+      const val = useRouteQuery(key, defaultValue);
+      return ref(val.value);
+    }
+
+    throw Error(`Invalid query value type ${typeof defaultValue}`);
+  }
+
+  const page1 = useOptionalRouteQuery("page", 1);
 
   const page = computed({
     get: () => page1.value,
@@ -46,14 +81,15 @@
     },
   });
 
-  const query = useRouteQuery("q", "");
-  const advanced = useRouteQuery("advanced", false);
-  const includeArchived = useRouteQuery("archived", false);
-  const fieldSelector = useRouteQuery("fieldSelector", false);
-  const negateLabels = useRouteQuery("negateLabels", false);
-  const onlyWithoutPhoto = useRouteQuery("onlyWithoutPhoto", false);
-  const onlyWithPhoto = useRouteQuery("onlyWithPhoto", false);
-  const orderBy = useRouteQuery("orderBy", "name");
+  const query = useOptionalRouteQuery("q", "");
+  const includeArchived = useOptionalRouteQuery("archived", false);
+  const fieldSelector = useOptionalRouteQuery("fieldSelector", false);
+  const negateLabels = useOptionalRouteQuery("negateLabels", false);
+  const onlyWithoutPhoto = useOptionalRouteQuery("onlyWithoutPhoto", false);
+  const onlyWithPhoto = useOptionalRouteQuery("onlyWithPhoto", false);
+  const orderBy = useOptionalRouteQuery("orderBy", "name");
+  const qLoc = useOptionalRouteQuery("loc", []);
+  const qLab = useOptionalRouteQuery("lab", []);
 
   const preferences = useViewPreferences();
   const pageSize = computed(() => preferences.value.itemsPerTablePage);
@@ -63,23 +99,14 @@
 
   onMounted(async () => {
     loading.value = true;
-    // Wait until locations and labels are loaded
-    let maxRetry = 10;
-    while (!labels.value || !locations.value) {
-      await new Promise(resolve => setTimeout(resolve, 100));
-      if (maxRetry-- < 0) {
-        break;
-      }
-    }
     searchLocked.value = true;
-    const qLoc = route.query.loc as string[];
+    await Promise.all([locationsStore.ensureLocationsFetched(), labelStore.ensureAllLabelsFetched()]);
     if (qLoc) {
-      selectedLocations.value = locations.value.filter(l => qLoc.includes(l.id));
+      selectedLocations.value = locations.value.filter(l => qLoc.value.includes(l.id));
     }
 
-    const qLab = route.query.lab as string[];
     if (qLab) {
-      selectedLabels.value = labels.value.filter(l => qLab.includes(l.id));
+      selectedLabels.value = labels.value.filter(l => qLab.value.includes(l.id));
     }
 
     queryParamsInitialized.value = true;
@@ -111,7 +138,7 @@
 
   const locationsStore = useLocationStore();
 
-  const locationFlatTree = await useFlatLocations();
+  const locationFlatTree = useFlatLocations();
 
   const locations = computed(() => locationsStore.allLocations);
 
@@ -181,19 +208,19 @@
   });
 
   watch(onlyWithoutPhoto, (newV, oldV) => {
-    if (newV && onlyWithPhoto) {
+    if (newV && onlyWithPhoto.value) {
+      // this triggers the watch on onlyWithPhoto
       onlyWithPhoto.value = false;
-    }
-    if (newV !== oldV) {
+    } else if (newV !== oldV) {
       search();
     }
   });
 
   watch(onlyWithPhoto, (newV, oldV) => {
-    if (newV && onlyWithoutPhoto) {
+    if (newV && onlyWithoutPhoto.value) {
+      // this triggers the watch on onlyWithoutPhoto
       onlyWithoutPhoto.value = false;
-    }
-    if (newV !== oldV) {
+    } else if (newV !== oldV) {
       search();
     }
   });
@@ -220,29 +247,6 @@
     return data;
   }
 
-  watch(advanced, (v, lv) => {
-    if (v === false && lv === true) {
-      selectedLocations.value = [];
-      selectedLabels.value = [];
-      fieldTuples.value = [];
-
-      console.log("advanced", advanced.value);
-
-      router.push({
-        query: {
-          advanced: route.query.advanced,
-          q: query.value,
-          page: page.value,
-          archived: includeArchived.value ? "true" : "false",
-          negateLabels: negateLabels.value ? "true" : "false",
-          onlyWithoutPhoto: onlyWithoutPhoto.value ? "true" : "false",
-          onlyWithPhoto: onlyWithPhoto.value ? "true" : "false",
-          orderBy: orderBy.value,
-        },
-      });
-    }
-  });
-
   async function search() {
     if (searchLocked.value) {
       return;
@@ -258,6 +262,42 @@
       }
     }
 
+    const push_query: Record<string, string | string[] | number | boolean | undefined> = {
+      archived: includeArchived.value,
+      fieldSelector: fieldSelector.value,
+      negateLabels: negateLabels.value,
+      onlyWithoutPhoto: onlyWithoutPhoto.value,
+      onlyWithPhoto: onlyWithPhoto.value,
+      orderBy: orderBy.value,
+      page: page.value,
+      q: query.value,
+      loc: locIDs.value,
+      lab: labIDs.value,
+      fields: fields,
+    };
+
+    for (const key in push_query) {
+      const val = push_query[key];
+      const defaultVal = queryParamDefaultValues[key];
+      if (
+        (Array.isArray(val) &&
+          Array.isArray(defaultVal) &&
+          val.length == defaultVal.length &&
+          val.every(v => (defaultVal as string[]).includes(v))) ||
+        val === queryParamDefaultValues[key]
+      ) {
+        push_query[key] = undefined;
+      }
+
+      // Empirically seen to be unnecessary but according to router.push types,
+      // booleans are not supported. This might be more stable.
+      if (typeof push_query[key] === "boolean") {
+        push_query[key] = String(val);
+      }
+    }
+
+    await router.push({ query: push_query as LocationQueryRaw });
+
     const { data, error } = await api.items.getAll({
       q: query.value || "",
       locations: locIDs.value,
@@ -308,27 +348,6 @@
       }
     }
 
-    // Push non-reactive query fields
-    await router.push({
-      query: {
-        // Reactive
-        advanced: "true",
-        archived: includeArchived.value ? "true" : "false",
-        fieldSelector: fieldSelector.value ? "true" : "false",
-        negateLabels: negateLabels.value ? "true" : "false",
-        onlyWithoutPhoto: onlyWithoutPhoto.value ? "true" : "false",
-        onlyWithPhoto: onlyWithPhoto.value ? "true" : "false",
-        orderBy: orderBy.value,
-        page: page.value,
-        q: query.value,
-
-        // Non-reactive
-        loc: locIDs.value,
-        lab: labIDs.value,
-        fields,
-      },
-    });
-
     // Reset Pagination
     page.value = 1;
 
@@ -345,19 +364,6 @@
       }
     }
 
-    await router.push({
-      query: {
-        archived: "false",
-        fieldSelector: "false",
-        page: 1,
-        orderBy: "name",
-        q: "",
-        loc: [],
-        lab: [],
-        fields,
-      },
-    });
-
     await search();
   }
 

frontend/stores/labels.ts

@@ -5,6 +5,7 @@ export const useLabelStore = defineStore("labels", {
   state: () => ({
     allLabels: null as LabelOut[] | null,
     client: useUserApi(),
+    refreshAllLabelsPromise: null as Promise<void> | null,
   }),
   getters: {
     /**
@@ -13,19 +14,20 @@ export const useLabelStore = defineStore("labels", {
      * response.
      */
     labels(state): LabelOut[] {
-      if (state.allLabels === null) {
-        this.client.labels.getAll().then(result => {
-          if (result.error) {
-            console.error(result.error);
-          }
-
-          this.allLabels = result.data;
-        });
-      }
       return state.allLabels ?? [];
     },
   },
   actions: {
+    async ensureAllLabelsFetched() {
+      if (this.allLabels !== null) {
+        return;
+      }
+
+      if (this.refreshAllLabelsPromise === null) {
+        this.refreshAllLabelsPromise = this.refresh().then(() => {});
+      }
+      await this.refreshAllLabelsPromise;
+    },
     async refresh() {
       const result = await this.client.labels.getAll();
       if (result.error) {

frontend/stores/locations.ts

@@ -8,6 +8,7 @@ export const useLocationStore = defineStore("locations", {
     Locations: null as LocationOutCount[] | null,
     client: useUserApi(),
     tree: null as TreeItem[] | null,
+    refreshLocationsPromise: null as Promise<void> | null,
   }),
   getters: {
     /**
@@ -29,20 +30,20 @@ export const useLocationStore = defineStore("locations", {
       return state.parents ?? [];
     },
     allLocations(state): LocationOutCount[] {
-      if (state.Locations === null) {
-        this.client.locations.getAll({ filterChildren: false }).then(result => {
-          if (result.error) {
-            console.error(result.error);
-            return;
-          }
-
-          this.Locations = result.data;
-        });
-      }
       return state.Locations ?? [];
     },
   },
   actions: {
+    async ensureLocationsFetched() {
+      if (this.Locations !== null) {
+        return;
+      }
+
+      if (this.refreshLocationsPromise === null) {
+        this.refreshLocationsPromise = this.refreshChildren().then(() => {});
+      }
+      await this.refreshLocationsPromise;
+    },
     async refreshParents(): ReturnType<LocationsApi["getAll"]> {
       const result = await this.client.locations.getAll({ filterChildren: true });
       if (result.error) {