Merge pull request #4062 from Spinnich/fix/gallery-window-skip-total

perf(roms): let a gallery window fetch skip the library count

Author: gantoine

backend/endpoints/roms/__init__.py

@@ -27,6 +27,7 @@
 from fastapi.responses import Response
 from fastapi_pagination import resolve_params
 from fastapi_pagination.limit_offset import LimitOffsetPage, LimitOffsetParams
+from fastapi_pagination.types import GreaterEqualZero
 from pydantic import BaseModel, Field
 from sqlalchemy.exc import IntegrityError
 from starlette.responses import FileResponse
@@ -324,6 +325,7 @@ class CustomLimitOffsetParams(LimitOffsetParams):
 
 
 class CustomLimitOffsetPage[T: BaseModel](LimitOffsetPage[T]):
+    total: GreaterEqualZero | None
     char_index: dict[str, int]
     rom_id_index: list[int]
     filter_values: RomFiltersDict
@@ -348,6 +350,17 @@ def get_roms(
             )
         ),
     ] = True,
+    with_total: Annotated[
+        bool,
+        Query(
+            description=(
+                "Whether to count the full result set. Set to false when the caller"
+                " already knows the total, e.g. paging through a gallery it has"
+                " sized; total then comes back null, unless the rom id index is"
+                " being built and already carries it."
+            )
+        ),
+    ] = True,
     search_term: Annotated[
         str | None,
         Query(description="Search term to filter roms."),
@@ -831,9 +844,20 @@ def _transform(items: Sequence[Rom]) -> list[SimpleRomSchema]:
                 for item in items
             ]
 
+        def resolve_total() -> int | None:
+            if with_rom_id_index:
+                # The index already spans the result set, so the count is free.
+                return len(rom_id_index)
+            # Without the index the count is its own scan of the filtered set,
+            # so a caller scrolling a gallery it already sized opts out.
+            return (
+                db_rom_handler.get_rom_count(query=query, session=session)
+                if with_total
+                else None
+            )
+
         params = resolve_params()
         if with_rom_id_index:
-            total = len(rom_id_index)
             page_ids = list(rom_id_index[params.offset : params.offset + params.limit])
             if page_ids:
                 page_rows = session.scalars(query.where(Rom.id.in_(page_ids))).all()
@@ -847,12 +871,11 @@ def _transform(items: Sequence[Rom]) -> list[SimpleRomSchema]:
             page_items = list(
                 session.scalars(query.offset(params.offset).limit(params.limit)).all()
             )
-            total = db_rom_handler.get_rom_count(query=query, session=session)
 
         return CustomLimitOffsetPage.create(
             _transform(page_items),
             params,
-            total=total,
+            total=resolve_total(),
             char_index=char_index_dict,
             rom_id_index=list(rom_id_index),
             filter_values=filter_values,

backend/tests/endpoints/roms/test_rom.py

@@ -313,6 +313,63 @@ def test_get_roms_without_rom_id_index(
     assert items[0]["id"] == rom.id
 
 
+def test_get_roms_without_total(
+    client: TestClient, access_token: str, rom: Rom, platform: Platform
+):
+    params = {
+        "platform_id": platform.id,
+        "limit": 15,
+        "with_rom_id_index": False,
+    }
+
+    with patch.object(
+        db_rom_handler, "get_rom_count", wraps=db_rom_handler.get_rom_count
+    ) as get_rom_count:
+        response = client.get(
+            "/api/roms",
+            headers={"Authorization": f"Bearer {access_token}"},
+            params={**params, "with_total": False},
+        )
+        assert response.status_code == status.HTTP_200_OK
+
+        # The point of the opt-out: no second scan of the filtered set.
+        get_rom_count.assert_not_called()
+
+        body = response.json()
+        assert body["total"] is None
+
+        items = body["items"]
+        assert len(items) == 1
+        assert items[0]["id"] == rom.id
+
+        # Control: the count still runs for callers that ask for it.
+        response = client.get(
+            "/api/roms",
+            headers={"Authorization": f"Bearer {access_token}"},
+            params=params,
+        )
+        assert response.status_code == status.HTTP_200_OK
+        assert response.json()["total"] == 1
+        get_rom_count.assert_called_once()
+
+
+def test_get_roms_keeps_total_from_the_rom_id_index(
+    client: TestClient, access_token: str, rom: Rom, platform: Platform
+):
+    # The index already carries the count, so opting out of the separate
+    # count query costs the caller nothing there.
+    response = client.get(
+        "/api/roms",
+        headers={"Authorization": f"Bearer {access_token}"},
+        params={"platform_id": platform.id, "with_total": False},
+    )
+    assert response.status_code == status.HTTP_200_OK
+
+    body = response.json()
+    assert body["total"] == 1
+    assert body["rom_id_index"] == [rom.id]
+
+
 def test_get_roms_filter_by_metadata_providers(
     client: TestClient, access_token: str, rom: Rom, platform: Platform
 ):

frontend/src/__generated__/models/CustomLimitOffsetPage_SimpleRomSchema_.ts

@@ -183,10 +183,10 @@ export interface GetRomsParams {
   playerCountsLogic?: string | null;
   metadataProvidersLogic?: string | null;
   tagsLogic?: string | null;
-  // Skip the char index / filter-value / id-index aggregations server-side
   withCharIndex?: boolean;
   withFilterValues?: boolean;
   withRomIdIndex?: boolean;
+  withTotal?: boolean;
   // Cancel an in-flight request
   signal?: AbortSignal;
 }
@@ -238,6 +238,7 @@ async function getRoms({
   withCharIndex = undefined,
   withFilterValues = undefined,
   withRomIdIndex = undefined,
+  withTotal = undefined,
   signal = undefined,
 }: GetRomsParams) {
   const params = {
@@ -351,6 +352,7 @@ async function getRoms({
     ...(withRomIdIndex !== undefined
       ? { with_rom_id_index: withRomIdIndex }
       : {}),
+    ...(withTotal !== undefined ? { with_total: withTotal } : {}),
   };
 
   return api.get<GetRomsResponse>(`/roms`, {

frontend/src/services/api/rom.ts

@@ -165,6 +165,7 @@ describe("galleryRoms windowed fetch", () => {
     expect(params.withCharIndex).toBeUndefined();
     expect(params.withFilterValues).toBeUndefined();
     expect(params.withRomIdIndex).toBeUndefined();
+    expect(params.withTotal).toBeUndefined();
   });
 
   it("does not clobber the filter drawer when filter values are skipped", async () => {
@@ -225,6 +226,48 @@ describe("galleryRoms windowed fetch", () => {
     expect(store.charIndex).toEqual({ A: 0, B: 10 });
   });
 
+  // Skipping the id index used to make the backend fall back to a full COUNT,
+  // so every scroll batch re-counted the library for a total the page already
+  // had (issue #4053).
+  it("skips the total on a window the bootstrap already sized", async () => {
+    getRoms.mockImplementation((params: { limit?: number }) => {
+      if (params.limit === 1) {
+        return Promise.resolve({
+          data: { total: 500, items: [], char_index: {}, rom_id_index: [] },
+        });
+      }
+      // The backend returns a null total when the count is skipped.
+      return Promise.resolve({
+        data: { total: null, items: [], char_index: {}, rom_id_index: [] },
+      });
+    });
+    const store = storeGalleryRoms();
+
+    await store.fetchInitialMetadata();
+    expect(store.total).toBe(500);
+
+    store.syncVisibleWindows([72]);
+    await flushPromises();
+
+    const windowCall = getRoms.mock.calls.find((c) => c[0].offset === 72);
+    expect(windowCall?.[0].withTotal).toBe(false);
+    // The null total must not blank the size the bootstrap established.
+    expect(store.total).toBe(500);
+  });
+
+  // The very first window doubles as the bootstrap when nothing has loaded
+  // yet, so it still has to bring the total back with it.
+  it("asks for the total on the first window when no bootstrap ran", async () => {
+    getRoms.mockResolvedValue(windowResponse(0, 300));
+    const store = storeGalleryRoms();
+
+    store.syncVisibleWindows([0]);
+    await flushPromises();
+
+    expect(getRoms.mock.calls[0][0].withTotal).toBeUndefined();
+    expect(store.total).toBe(300);
+  });
+
   it("does not mark a window loaded when the context is invalidated mid-apply", async () => {
     // Controllable frame yield so we can interleave a context switch between
     // the batched-apply's frames.

frontend/src/v2/stores/galleryRoms.test.ts

@@ -523,7 +523,10 @@ export default defineStore("v2GalleryRoms", {
       // object when these are skipped, so re-applying it would wipe the
       // populated values and blank the AlphaStrip / filter drawer. The id
       // index is a full-library scan we already paid for in the bootstrap, so
-      // window fetches opt out of recomputing it.
+      // window fetches opt out of recomputing it. Dropping it makes the backend
+      // count the result set separately instead, which is the same scan under
+      // another name for a total the bootstrap already gave us, so opt out of
+      // that too and keep the window fetch to just its page of covers.
       const withAggregations = !this.metadataLoaded;
 
       try {
@@ -535,6 +538,7 @@ export default defineStore("v2GalleryRoms", {
                 withCharIndex: false,
                 withFilterValues: false,
                 withRomIdIndex: false,
+                withTotal: false,
               }),
           signal: controller.signal,
         });
@@ -547,9 +551,10 @@ export default defineStore("v2GalleryRoms", {
 
         const data = response.data;
         // Only apply the full metadata when this window actually fetched the
-        // aggregations (the very first window before the bootstrap resolved).
+        // aggregations (a window reached before the bootstrap resolved).
         // Otherwise char_index / filter_values come back empty and would
-        // clobber what the bootstrap populated, so just refresh `total`.
+        // clobber what the bootstrap populated; `total` comes back null and
+        // the guard below leaves the established size alone.
         if (offset === 0 && withAggregations) {
           this._applyMetadata(data, galleryFilter, platformsStore);
         } else if (data.total !== null && data.total !== undefined) {