Refactor request deduplication and address PR #1636 review comments

- Rename `getCachedRequestPromise` to `deduplicatePromiseForRequest` to better reflect its purpose.
- Rename `getRequestScopedDapper` to `getDapperForRequest`.
- Improve cache management:
  - Immediately handle aborted requests to prevent race conditions.
  - Remove rejected promises from cache to allow retries.
  - Use `WeakMap.delete` for safer cache clearing instead of array splicing.
- Add docstrings explaining usage, performance implications, and limitations (e.g., only caching "get" methods).
- Expand test coverage:
  - Verify promise reference equality.
  - Ensure different arguments create distinct cache entries.
  - Test behavior with aborted signals.
- Remove- Rename `getCachedRequestPromise` to `deduplicatePromiseForRequest` to better reflect its purpose.
- Rename `getRequestScopedDapper` to `getDapperForRequest`.
- Improve cache management:
  - Immediately handle aborted requests to prevent race conditions.
  - Remove rejected promises from cache to allow retries.
  - Use `WeakMap.delete` for safer cache clearing instead of array splicing.
- Add docstrings explaining usage, performance implications, and limitations (e.g., only caching "get" methods).
- Expand test coverage:
  - Verify promise reference equality.
  - Ensure different arguments create distinct cache entries.
  - Test behavior with aborted signals.
- Remove resolved TODO in entry.client.tsx

Author: Oksamies

apps/cyberstorm-remix/app/entry.client.tsx

@@ -20,7 +20,6 @@ const publicEnvVariables = getPublicEnvVariables([
   "VITE_COOKIE_DOMAIN",
 ]);
 
-// TODO: Check if this is synchronous or not
 Sentry.init({
   dsn: publicEnvVariables.VITE_CLIENT_SENTRY_DSN,
   integrations: [

apps/cyberstorm-remix/cyberstorm/utils/__tests__/dapperSingleton.test.ts

@@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
 import {
   initializeClientDapper,
   getClientDapper,
-  getRequestScopedDapper,
+  getDapperForRequest,
   resetDapperSingletonForTest,
 } from "../dapperSingleton";
 import { DapperTs } from "@thunderstore/dapper-ts";
@@ -69,17 +69,17 @@ describe("dapperSingleton", () => {
     });
   });
 
-  describe("getRequestScopedDapper", () => {
+  describe("getDapperForRequest", () => {
     it("returns client dapper if no request is provided", () => {
       initializeClientDapper();
-      const dapper = getRequestScopedDapper();
+      const dapper = getDapperForRequest();
       expect(dapper).toBe(window.Dapper);
     });
 
     it("returns a proxy if request is provided", () => {
       initializeClientDapper();
       const request = new Request("http://localhost");
-      const dapper = getRequestScopedDapper(request);
+      const dapper = getDapperForRequest(request);
       expect(dapper).not.toBe(window.Dapper);
       // It should be a proxy
       expect(dapper).toBeInstanceOf(DapperTs);
@@ -88,24 +88,24 @@ describe("dapperSingleton", () => {
     it("caches the proxy for the same request", () => {
       initializeClientDapper();
       const request = new Request("http://localhost");
-      const dapper1 = getRequestScopedDapper(request);
-      const dapper2 = getRequestScopedDapper(request);
+      const dapper1 = getDapperForRequest(request);
+      const dapper2 = getDapperForRequest(request);
       expect(dapper1).toBe(dapper2);
     });
 
     it("creates different proxies for different requests", () => {
       initializeClientDapper();
       const request1 = new Request("http://localhost");
       const request2 = new Request("http://localhost");
-      const dapper1 = getRequestScopedDapper(request1);
-      const dapper2 = getRequestScopedDapper(request2);
+      const dapper1 = getDapperForRequest(request1);
+      const dapper2 = getDapperForRequest(request2);
       expect(dapper1).not.toBe(dapper2);
     });
 
     it("intercepts 'get' methods and caches promises", async () => {
       initializeClientDapper();
       const request = new Request("http://localhost");
-      const dapper = getRequestScopedDapper(request);
+      const dapper = getDapperForRequest(request);
 
       // Mock the underlying method on window.Dapper
       const mockGetCommunities = vi
@@ -125,7 +125,7 @@ describe("dapperSingleton", () => {
     it("does not intercept non-'get' methods", async () => {
       initializeClientDapper();
       const request = new Request("http://localhost");
-      const dapper = getRequestScopedDapper(request);
+      const dapper = getDapperForRequest(request);
 
       // Mock a non-get method
       // postTeamCreate is a good candidate

apps/cyberstorm-remix/cyberstorm/utils/__tests__/requestCache.test.ts

@@ -1,18 +1,18 @@
 import { describe, it, expect, vi } from "vitest";
-import { getCachedRequestPromise } from "../requestCache";
+import { deduplicatePromiseForRequest } from "../requestCache";
 
 describe("requestCache", () => {
   it("caches result per request", async () => {
     const mockFn = vi.fn().mockResolvedValue("result");
     const request = new Request("http://localhost");
 
-    const result1 = await getCachedRequestPromise(
+    const result1 = await deduplicatePromiseForRequest(
       "mockFn",
       mockFn,
       ["arg1"],
       request
     );
-    const result2 = await getCachedRequestPromise(
+    const result2 = await deduplicatePromiseForRequest(
       "mockFn",
       mockFn,
       ["arg1"],
@@ -24,13 +24,79 @@ describe("requestCache", () => {
     expect(mockFn).toHaveBeenCalledTimes(1);
   });
 
+  it("returns the exact same promise instance", async () => {
+    const mockFn = vi.fn().mockResolvedValue("result");
+    const request = new Request("http://localhost");
+
+    const promise1 = deduplicatePromiseForRequest(
+      "mockFn",
+      mockFn,
+      ["arg1"],
+      request
+    );
+    const promise2 = deduplicatePromiseForRequest(
+      "mockFn",
+      mockFn,
+      ["arg1"],
+      request
+    );
+
+    expect(promise1).toBe(promise2);
+  });
+
+  it("distinguishes between calls with different arguments", async () => {
+    const mockFn = vi.fn().mockImplementation(async (arg) => arg);
+    const request = new Request("http://localhost");
+
+    const result1 = await deduplicatePromiseForRequest(
+      "mockFn",
+      mockFn,
+      ["arg1"],
+      request
+    );
+    const result2 = await deduplicatePromiseForRequest(
+      "mockFn",
+      mockFn,
+      ["arg2"],
+      request
+    );
+
+    expect(result1).toBe("arg1");
+    expect(result2).toBe("arg2");
+    expect(mockFn).toHaveBeenCalledTimes(2);
+  });
+
+  it("removes rejected promises from cache", async () => {
+    const mockFn = vi
+      .fn()
+      .mockRejectedValueOnce(new Error("fail"))
+      .mockResolvedValueOnce("success");
+    const request = new Request("http://localhost");
+
+    // First call fails
+    await expect(
+      deduplicatePromiseForRequest("mockFn", mockFn, ["arg1"], request)
+    ).rejects.toThrow("fail");
+
+    // Second call should retry and succeed
+    const result = await deduplicatePromiseForRequest(
+      "mockFn",
+      mockFn,
+      ["arg1"],
+      request
+    );
+
+    expect(result).toBe("success");
+    expect(mockFn).toHaveBeenCalledTimes(2);
+  });
+
   it("does not share cache between requests", async () => {
     const mockFn = vi.fn().mockResolvedValue("result");
     const request1 = new Request("http://localhost");
     const request2 = new Request("http://localhost");
 
-    await getCachedRequestPromise("mockFn", mockFn, ["arg1"], request1);
-    await getCachedRequestPromise("mockFn", mockFn, ["arg1"], request2);
+    await deduplicatePromiseForRequest("mockFn", mockFn, ["arg1"], request1);
+    await deduplicatePromiseForRequest("mockFn", mockFn, ["arg1"], request2);
 
     expect(mockFn).toHaveBeenCalledTimes(2);
   });
@@ -40,15 +106,15 @@ describe("requestCache", () => {
     const request = new Request("http://localhost");
 
     expect(() =>
-      getCachedRequestPromise("default", mockFn, [], request)
+      deduplicatePromiseForRequest("default", mockFn, [], request)
     ).toThrow("Must be named functions to support caching.");
   });
 
   it("allows caching if a valid name is provided", async () => {
     const mockFn = async () => "result";
     const request = new Request("http://localhost");
 
-    const result = await getCachedRequestPromise(
+    const result = await deduplicatePromiseForRequest(
       "customLabel",
       mockFn,
       [],
@@ -64,12 +130,12 @@ describe("requestCache", () => {
       signal: controller.signal,
     });
 
-    await getCachedRequestPromise("mockFn", mockFn, ["arg1"], request);
+    await deduplicatePromiseForRequest("mockFn", mockFn, ["arg1"], request);
 
     // Abort the request
     controller.abort();
 
-    await getCachedRequestPromise("mockFn", mockFn, ["arg1"], request);
+    await deduplicatePromiseForRequest("mockFn", mockFn, ["arg1"], request);
 
     expect(mockFn).toHaveBeenCalledTimes(2);
   });
@@ -84,16 +150,13 @@ describe("requestCache", () => {
     // Abort before first use
     controller.abort();
 
-    // First use: populates cache (unavoidable currently as we push after check)
-    await getCachedRequestPromise("mockFn", mockFn, ["arg1"], request);
+    // First use: should NOT cache (or cache is cleared immediately)
+    await deduplicatePromiseForRequest("mockFn", mockFn, ["arg1"], request);
     expect(mockFn).toHaveBeenCalledTimes(1);
 
-    // Second use: should be cleared/ignored and re-executed?
-    // Currently it returns cached value because listener wasn't attached.
-    await getCachedRequestPromise("mockFn", mockFn, ["arg1"], request);
+    // Second use: should be re-executed because cache was not persisted/cleared
+    await deduplicatePromiseForRequest("mockFn", mockFn, ["arg1"], request);
 
-    // If consistent with above, it should be 2.
-    // If inconsistent (bug), it is 1.
     expect(mockFn).toHaveBeenCalledTimes(2);
   });
 });

apps/cyberstorm-remix/cyberstorm/utils/dapperSingleton.ts

@@ -1,7 +1,7 @@
 import { DapperTs } from "@thunderstore/dapper-ts";
 import { type RequestConfig } from "@thunderstore/thunderstore-api";
 import { getSessionTools } from "../security/publicEnvVariables";
-import { getCachedRequestPromise } from "./requestCache";
+import { deduplicatePromiseForRequest } from "./requestCache";
 
 type ConfigFactory = () => RequestConfig;
 
@@ -54,7 +54,14 @@ export function getClientDapper(): DapperTs {
   return window.Dapper;
 }
 
-export function getRequestScopedDapper(request?: Request): DapperTs {
+/**
+ * Returns a Dapper instance scoped to the request.
+ * If a request is provided, it returns a proxy that deduplicates "get" requests.
+ * If no request is provided (e.g. client-side navigation), it returns the global client Dapper instance.
+ *
+ * @param request - The Request object to scope the Dapper instance to.
+ */
+export function getDapperForRequest(request?: Request): DapperTs {
   if (!request) {
     return getClientDapper();
   }
@@ -72,12 +79,13 @@ export function getRequestScopedDapper(request?: Request): DapperTs {
         return value;
       }
 
+      // We only cache methods starting with "get" to avoid caching mutations or other side effects.
       if (typeof propertyName !== "string" || !propertyName.startsWith("get")) {
         return value.bind(target);
       }
 
       return (...args: unknown[]) =>
-        getCachedRequestPromise(
+        deduplicatePromiseForRequest(
           propertyName,
           (...innerArgs: unknown[]) =>
             (value as (...i: unknown[]) => Promise<unknown>).apply(

apps/cyberstorm-remix/cyberstorm/utils/requestCache.ts

@@ -9,36 +9,43 @@ type CacheEntry = {
 };
 
 const requestScopedCaches = new WeakMap<Request, CacheEntry[]>();
+
 function getCache(request: Request) {
+  if (request.signal?.aborted) {
+    requestScopedCaches.delete(request);
+    return [];
+  }
+
   let cache = requestScopedCaches.get(request);
   if (!cache) {
     cache = [];
     requestScopedCaches.set(request, cache);
 
     const signal = request.signal;
     if (signal) {
-      if (signal.aborted) {
-        cache.splice(0, cache.length);
-      } else {
-        signal.addEventListener(
-          "abort",
-          () => {
-            cache?.splice(0, cache.length);
-          },
-          { once: true }
-        );
-      }
+      signal.addEventListener(
+        "abort",
+        () => {
+          requestScopedCaches.delete(request);
+        },
+        { once: true }
+      );
     }
   }
 
-  if (request.signal?.aborted) {
-    cache.splice(0, cache.length);
-  }
-
   return cache;
 }
 
-export function getCachedRequestPromise<Args extends unknown[], Result>(
+/**
+ * Deduplicates concurrent requests for the same function and arguments within the scope of a single Request.
+ * This is useful for clientLoaders where we might trigger the same data fetch multiple times.
+ *
+ * @param funcName - The name of the function (used as part of the cache key). Must not be empty, "anonymous", or "default".
+ * @param func - The function to execute.
+ * @param inputs - The arguments to pass to the function.
+ * @param request - The Request object to scope the cache to.
+ */
+export function deduplicatePromiseForRequest<Args extends unknown[], Result>(
   funcName: string,
   func: (...inputs: Args) => Promise<Result>,
   inputs: Args,
@@ -48,6 +55,9 @@ export function getCachedRequestPromise<Args extends unknown[], Result>(
     throw new Error("Must be named functions to support caching.");
   }
 
+  // Performance note: We use a linear search over the cache entries.
+  // Since the number of unique requests per page load is usually small, this is acceptable.
+  // We use lodash/isEqual for deep comparison of arguments, which can be expensive for large objects.
   const cache = getCache(request);
   for (const cacheEntry of cache) {
     if (
@@ -59,8 +69,22 @@ export function getCachedRequestPromise<Args extends unknown[], Result>(
   }
 
   const promise = func(...inputs) as Promise<Result>;
+
+  // Remove from cache if it rejects to allow retrying
+  promise.catch(() => {
+    const currentCache = requestScopedCaches.get(request);
+    if (currentCache) {
+      const index = currentCache.findIndex(
+        (entry) => entry.promise === promise
+      );
+      if (index !== -1) {
+        currentCache.splice(index, 1);
+      }
+    }
+  });
+
   cache.push({
-    funcName: funcName,
+    funcName,
     inputs,
     promise,
   });