Merge pull request #4113 from Spinnich/fix/gallery-random-stale-platform

fix(v2): drop a random pick after leaving the platform

Author: gantoine

frontend/src/v2/views/Gallery/Collection.test.ts

@@ -237,6 +237,37 @@ describe("Collection view random rom", () => {
     expect(push).not.toHaveBeenCalled();
   });
 
+  it("stays quiet when a pick fails after the view moved to another collection", async () => {
+    let failPick: (reason: Error) => void = () => {};
+    getRandomRom.mockReturnValueOnce(
+      new Promise((_resolve, reject) => {
+        failPick = reject;
+      }),
+    );
+
+    const wrapper = await mountView();
+    await wrapper.get("button.random").trigger("click");
+
+    routeState.params = { collection: "2" };
+    routeGuards.forEach((guard) =>
+      guard({ name: "collection", params: { collection: "2" } }),
+    );
+    await flushPromises();
+
+    failPick(new Error("boom"));
+    await flushPromises();
+
+    expect(snackbarError).not.toHaveBeenCalled();
+    expect(push).not.toHaveBeenCalled();
+
+    // The button still works on the collection the user landed on.
+    getRandomRom.mockResolvedValue({ data: rom(7) });
+    await wrapper.get("button.random").trigger("click");
+    await flushPromises();
+
+    expect(push).toHaveBeenCalledWith({ name: "rom", params: { rom: 7 } });
+  });
+
   it("ignores a click while a pick is in flight", async () => {
     let resolvePick: (value: { data: SimpleRom }) => void = () => {};
     getRandomRom.mockReturnValue(

frontend/src/v2/views/Gallery/Collection.vue

@@ -288,17 +288,18 @@ async function onRandomGame() {
   if (!c || randomLoading.value) return;
   randomLoading.value = true;
   const scopeId = c.id;
+  // A pick from the collection the user just left leads nowhere useful.
+  const stale = () => currentCollection.value?.id !== scopeId;
   try {
     const { data } = await romApi.getRandomRom(randomScope());
-    // A pick from the collection the user just left leads nowhere useful.
-    if (currentCollection.value?.id !== scopeId) return;
+    if (stale()) return;
     if (!data) {
       snackbar.info(t("collection.empty"));
       return;
     }
     router.push({ name: ROUTES.ROM, params: { rom: data.id } });
   } catch {
-    snackbar.error(t("platform.random-rom-error"));
+    if (!stale()) snackbar.error(t("platform.random-rom-error"));
   } finally {
     randomLoading.value = false;
   }

frontend/src/v2/views/Gallery/Platform.test.ts

@@ -13,13 +13,18 @@ const {
   getRoms,
   getPlatform,
   push,
+  routeGuards,
   snackbarError,
   snackbarInfo,
 } = vi.hoisted(() => ({
   getRandomRom: vi.fn(),
   getRoms: vi.fn(),
   getPlatform: vi.fn(),
   push: vi.fn(),
+  routeGuards: [] as ((to: {
+    name: string;
+    params: Record<string, string>;
+  }) => unknown)[],
   snackbarError: vi.fn(),
   snackbarInfo: vi.fn(),
 }));
@@ -41,7 +46,9 @@ vi.mock("vue-router", async (importOriginal) => ({
   ...(await importOriginal<typeof import("vue-router")>()),
   useRoute: () => routeState,
   useRouter: () => ({ push, replace: vi.fn() }),
-  onBeforeRouteUpdate: vi.fn(),
+  // Captured rather than dropped: calling the guard is how a test moves
+  // the view to another platform, which is what the stale check watches.
+  onBeforeRouteUpdate: vi.fn((guard) => routeGuards.push(guard)),
 }));
 
 vi.mock("@/plugins/router", () => ({
@@ -104,13 +111,13 @@ vi.mock("@/v2/composables/useSnackbar", () => ({
   useSnackbar: () => ({ error: snackbarError, info: snackbarInfo }),
 }));
 
-function platform(id: number): Platform {
+function platform(id: number, name = "Super Nintendo"): Platform {
   return {
     id,
-    name: "Super Nintendo",
-    display_name: "Super Nintendo",
-    slug: "snes",
-    fs_slug: "snes",
+    name,
+    display_name: name,
+    slug: `platform-${id}`,
+    fs_slug: `platform-${id}`,
     rom_count: 83000,
   } as Platform;
 }
@@ -119,9 +126,41 @@ function rom(id: number): SimpleRom {
   return { id, name: "Chrono Trigger" } as SimpleRom;
 }
 
+/** Resolves the promise the next `getRandomRom` call returns, on demand. */
+function deferRandomRom() {
+  let settle: (value: { data: SimpleRom | null }) => void = () => {};
+  getRandomRom.mockReturnValueOnce(
+    new Promise((resolve) => {
+      settle = resolve;
+    }),
+  );
+  return (pick: SimpleRom | null) => settle({ data: pick });
+}
+
+/** Rejects the promise the next `getRandomRom` call returns, on demand. */
+function deferRandomRomFailure() {
+  let fail: (reason: Error) => void = () => {};
+  getRandomRom.mockReturnValueOnce(
+    new Promise((_resolve, reject) => {
+      fail = reject;
+    }),
+  );
+  return () => fail(new Error("boom"));
+}
+
+/** Moves the view to another platform the way the router would. */
+async function navigateTo(platformId: number) {
+  routeState.params = { platform: String(platformId) };
+  routeState.path = `/platform/${platformId}`;
+  routeGuards.forEach((guard) =>
+    guard({ name: "platform", params: { platform: String(platformId) } }),
+  );
+  await flushPromises();
+}
+
 async function mountView() {
   const platforms = storePlatforms();
-  platforms.set([platform(1)]);
+  platforms.set([platform(1), platform(2, "Mega Drive")]);
   const galleryRoms = storeGalleryRoms();
   vi.spyOn(galleryRoms, "fetchInitialMetadata").mockResolvedValue();
 
@@ -136,7 +175,13 @@ describe("Platform view random rom", () => {
   beforeEach(() => {
     setActivePinia(createPinia());
     vi.clearAllMocks();
-    getPlatform.mockResolvedValue({ data: platform(1) });
+    routeState.name = "platform";
+    routeState.path = "/platform/1";
+    routeState.params = { platform: "1" };
+    routeGuards.length = 0;
+    getPlatform.mockImplementation((id: number) =>
+      Promise.resolve({ data: platform(id) }),
+    );
     getRoms.mockResolvedValue({ data: { items: [], total: 0 } });
   });
 
@@ -178,6 +223,61 @@ describe("Platform view random rom", () => {
     expect(push).not.toHaveBeenCalled();
   });
 
+  // Issue #4104: the pick is scoped to the platform that was on screen when
+  // the button was clicked, so following it after the user moved on drops
+  // them into a game from a gallery they already left.
+  it("drops a pick that lands after the view moved to another platform", async () => {
+    const resolvePick = deferRandomRom();
+
+    const wrapper = await mountView();
+    await wrapper.get("button.random").trigger("click");
+
+    await navigateTo(2);
+
+    resolvePick(rom(42));
+    await flushPromises();
+
+    expect(push).not.toHaveBeenCalled();
+  });
+
+  it("stays quiet when a pick fails after the view moved to another platform", async () => {
+    const failPick = deferRandomRomFailure();
+
+    const wrapper = await mountView();
+    await wrapper.get("button.random").trigger("click");
+
+    await navigateTo(2);
+
+    failPick();
+    await flushPromises();
+
+    expect(snackbarError).not.toHaveBeenCalled();
+    expect(push).not.toHaveBeenCalled();
+
+    // The button still works on the platform the user landed on.
+    getRandomRom.mockResolvedValue({ data: rom(7) });
+    await wrapper.get("button.random").trigger("click");
+    await flushPromises();
+
+    expect(push).toHaveBeenCalledWith({ name: "rom", params: { rom: 7 } });
+  });
+
+  // A same-platform reload swaps in a fresh `Platform` record, so the check
+  // has to compare ids rather than object identity.
+  it("still navigates when the view reloaded the same platform", async () => {
+    const resolvePick = deferRandomRom();
+
+    const wrapper = await mountView();
+    await wrapper.get("button.random").trigger("click");
+
+    await navigateTo(1);
+
+    resolvePick(rom(42));
+    await flushPromises();
+
+    expect(push).toHaveBeenCalledWith({ name: "rom", params: { rom: 42 } });
+  });
+
   it("ignores a click while a pick is in flight", async () => {
     let resolvePick: (value: { data: SimpleRom }) => void = () => {};
     getRandomRom.mockReturnValue(

frontend/src/v2/views/Gallery/Platform.vue

@@ -333,15 +333,21 @@ async function onRandomGame() {
   const p = currentPlatform.value;
   if (!p || randomLoading.value) return;
   randomLoading.value = true;
+  const scopeId = p.id;
+  // The pick belongs to the platform that was on screen when the button was
+  // clicked; following it after the user moved on would drop them into a
+  // game from a gallery they already left.
+  const stale = () => currentPlatform.value?.id !== scopeId;
   try {
-    const { data } = await romApi.getRandomRom({ platformIds: [p.id] });
+    const { data } = await romApi.getRandomRom({ platformIds: [scopeId] });
+    if (stale()) return;
     if (!data) {
       snackbar.info(t("platform.random-rom-empty"));
       return;
     }
     router.push({ name: ROUTES.ROM, params: { rom: data.id } });
   } catch {
-    snackbar.error(t("platform.random-rom-error"));
+    if (!stale()) snackbar.error(t("platform.random-rom-error"));
   } finally {
     randomLoading.value = false;
   }