feedback

apps/web/modules/core/rate-limit/rate-limit-configs.test.ts

@@ -132,6 +132,8 @@ describe("rateLimitConfigs", () => {
         { config: rateLimitConfigs.api.client, identifier: "client-api-key" },
         { config: rateLimitConfigs.api.syncUserIdentification, identifier: "sync-user-id" },
         { config: rateLimitConfigs.actions.emailUpdate, identifier: "user-profile" },
+        { config: rateLimitConfigs.storage.upload, identifier: "storage-upload" },
+        { config: rateLimitConfigs.storage.delete, identifier: "storage-delete" },
       ];
 
       for (const { config, identifier } of testCases) {
@@ -175,5 +177,23 @@ describe("rateLimitConfigs", () => {
         expect(exceededResult.data.allowed).toBe(false);
       }
     });
+
+    test("should properly configure storage upload rate limit", async () => {
+      const config = rateLimitConfigs.storage.upload;
+
+      // Verify configuration values
+      expect(config.interval).toBe(60); // 1 minute
+      expect(config.allowedPerInterval).toBe(5); // 5 requests per minute
+      expect(config.namespace).toBe("storage:upload");
+    });
+
+    test("should properly configure storage delete rate limit", async () => {
+      const config = rateLimitConfigs.storage.delete;
+
+      // Verify configuration values
+      expect(config.interval).toBe(60); // 1 minute
+      expect(config.allowedPerInterval).toBe(5); // 5 requests per minute
+      expect(config.namespace).toBe("storage:delete");
+    });
   });
 });

apps/web/modules/core/rate-limit/rate-limit-configs.ts

@@ -32,7 +32,6 @@ export const rateLimitConfigs = {
 
   storage: {
     upload: { interval: 60, allowedPerInterval: 5, namespace: "storage:upload" }, // 5 per minute
-    download: { interval: 60, allowedPerInterval: 5, namespace: "storage:download" }, // 5 per minute
     delete: { interval: 60, allowedPerInterval: 5, namespace: "storage:delete" }, // 5 per minute
   },
 };

packages/storage/src/client.test.ts

@@ -268,4 +268,72 @@ describe("client.ts", () => {
       expect(result).toBeUndefined();
     });
   });
+
+  describe("getCachedS3Client (singleton)", () => {
+    test("returns the same instance on multiple calls and constructs once", async () => {
+      vi.doMock("./constants", () => ({
+        ...mockConstants,
+      }));
+
+      const { getCachedS3Client } = await import("./client");
+      const typedGetCachedS3Client = getCachedS3Client as unknown as () => S3Client;
+
+      const first = typedGetCachedS3Client();
+      const second = typedGetCachedS3Client();
+
+      expect(first).toBeDefined();
+      expect(second).toBeDefined();
+      expect(first).toBe(second);
+      expect(mockS3Client).toHaveBeenCalledTimes(1);
+    });
+
+    test("returns undefined when env is invalid and does not construct client", async () => {
+      vi.doMock("./constants", () => ({
+        ...mockConstants,
+        S3_ACCESS_KEY: undefined,
+        S3_SECRET_KEY: undefined,
+      }));
+
+      const { getCachedS3Client } = await import("./client");
+      const typedGetCachedS3Client = getCachedS3Client as unknown as () => S3Client;
+
+      const client = typedGetCachedS3Client();
+      expect(client).toBeUndefined();
+      expect(mockS3Client).not.toHaveBeenCalled();
+    });
+
+    test("createS3Client uses cached instance when available", async () => {
+      vi.doMock("./constants", () => ({
+        ...mockConstants,
+      }));
+
+      const { getCachedS3Client, createS3Client } = await import("./client");
+      const typedGetCachedS3Client = getCachedS3Client as unknown as () => S3Client;
+
+      const cached = typedGetCachedS3Client();
+      const created = createS3Client();
+
+      expect(cached).toBeDefined();
+      expect(created).toBe(cached);
+      expect(mockS3Client).toHaveBeenCalledTimes(1);
+    });
+
+    test("createS3Client returns provided client even if cache exists", async () => {
+      vi.doMock("./constants", () => ({
+        ...mockConstants,
+      }));
+
+      const { getCachedS3Client, createS3Client } = await import("./client");
+      const typedGetCachedS3Client = getCachedS3Client as unknown as () => S3Client;
+      const cached = typedGetCachedS3Client();
+      expect(cached).toBeDefined();
+
+      const injected = new S3Client({});
+      const result = createS3Client(injected);
+
+      expect(result).toBe(injected);
+      // One construction for cached, one for injected
+      expect(mockS3Client).toHaveBeenCalledTimes(2);
+    });
+  });
 });

packages/storage/src/client.ts

@@ -10,6 +10,9 @@ import {
   S3_SECRET_KEY,
 } from "./constants";
 
+// Cached singleton instance of S3Client
+let cachedS3Client: S3Client | undefined;
+
 /**
  * Create an S3 client from environment variables
  * @returns A Result containing the S3 client or an error: S3CredentialsError | UnknownError
@@ -39,18 +42,28 @@ export const createS3ClientFromEnv = (): Result<S3Client, StorageError> => {
   }
 };
 
+/**
+ * Get a cached singleton S3 client instance. Lazily initializes from env on first successful call.
+ * Subsequent calls return the same instance.
+ */
+export const getCachedS3Client = (): S3Client | undefined => {
+  if (!cachedS3Client) {
+    const result = createS3ClientFromEnv();
+    if (result.ok) {
+      cachedS3Client = result.data;
+    }
+  }
+  return cachedS3Client;
+};
+
 /**
  * Create an S3 client from an existing client or from environment variables
  * @param s3Client - An existing S3 client
  * @returns An S3 client or undefined if the S3 credentials are not set in the environment variables or if there is an error creating the client
  */
 export const createS3Client = (s3Client?: S3Client): S3Client | undefined => {
-  if (!s3Client) {
-    const result = createS3ClientFromEnv();
-    if (result.ok) return result.data;
-
-    return undefined;
+  if (s3Client) {
+    return s3Client;
   }
-
-  return s3Client;
+  return getCachedS3Client();
 };