fix: revert replace AWS SDK with Bun native S3 utilities (#684)

app/backend/src/acceptVisualChanges.ts

@@ -1,3 +1,4 @@
+import { S3Client } from './s3Client';
 import {
   BASE_IMAGE_NAME,
   BASE_IMAGES_DIRECTORY,
@@ -9,7 +10,6 @@ import { TRPCError } from '@trpc/server';
 import { AcceptVisualChangesInput } from './schema';
 import { getKeysFromS3 } from './getKeysFromS3';
 import { updateCommitStatus } from './updateCommitStatus';
-import { copyS3File } from './copyS3File';
 
 export const acceptVisualChanges = async ({
   commitHash,
@@ -65,11 +65,16 @@ export const updateBaseImages = async (s3Paths: string[], bucket: string) => {
   const baseImagePaths = getBaseImagePaths(newImagePaths);
   return await Promise.all(
     baseImagePaths.map(async (path, index) => {
-      const sourcePath = newImagePaths[index];
-      if (!sourcePath) {
+      const copySource = newImagePaths[index];
+      if (!copySource) {
         throw new Error(`Source path not found for index ${index}`);
       }
-      await copyS3File(sourcePath, path, bucket);
+      await S3Client.copyObject({
+        Bucket: bucket,
+        CopySource: `${bucket}/${copySource}`,
+        Key: path,
+        ACL: 'bucket-owner-full-control'
+      });
     })
   );
 };

app/backend/src/getKeysFromS3.ts

@@ -1,15 +1,13 @@
 import { NEW_IMAGES_DIRECTORY } from 'shared';
-import { s3Client } from './s3Client';
+import { S3Client } from './s3Client';
 
 // Info on working with nested object path prefixes: https://realguess.net/2014/05/24/amazon-s3-delimiter-and-prefix/#Prefix
 export const getKeysFromS3 = async (hash: string, bucket: string) => {
-  const { contents } = await s3Client.list(
-    {
-      prefix: `${NEW_IMAGES_DIRECTORY}/${hash}/`
-    },
-    { bucket }
-  );
+  const response = await S3Client.listObjectsV2({
+    Bucket: bucket,
+    Prefix: `${NEW_IMAGES_DIRECTORY}/${hash}/`
+  });
 
-  const keys = contents?.map(item => item.key) ?? [];
+  const keys = response?.Contents?.map(content => content.Key ?? '') ?? [];
   return keys.filter(path => path && !path.includes('actions-runner'));
 };

app/backend/src/getTemporaryObjectUrl.ts

@@ -1,7 +1,10 @@
-import { s3Client } from './s3Client';
+import { S3Client } from './s3Client';
+import { getSignedUrl } from '@aws-sdk/s3-request-presigner';
+import { GetObjectCommand } from '@aws-sdk/client-s3';
 
 export const getTemporaryObjectUrl = async (key: string, bucket: string) => {
-  return s3Client.presign(key, { bucket, expiresIn: oneHour });
+  const command = new GetObjectCommand({ Bucket: bucket, Key: key });
+  return getSignedUrl(S3Client, command, { expiresIn: oneHour });
 };
 
 const oneHour = 3600;

app/backend/src/s3Client.ts

@@ -1,7 +1,3 @@
-import { S3Client } from 'bun';
-import { defaultProvider } from '@aws-sdk/credential-provider-node';
+import { S3 } from '@aws-sdk/client-s3';
 
-// Create S3Client with credentials loaded from ~/.aws/credentials and ~/.aws/config files
-// This automatically picks up credentials, SSO tokens, and handles role assumptions without checking environment variables
-const credentials = process.env.CI ? undefined : await defaultProvider()();
-export const s3Client = new S3Client(credentials);
+export const S3Client = new S3();

app/backend/test/acceptVisualChanges.test.ts

@@ -5,10 +5,16 @@ import {
   acceptVisualChanges
 } from '../src/acceptVisualChanges';
 import { BASE_IMAGES_DIRECTORY, NEW_IMAGES_DIRECTORY } from 'shared';
-import { beforeEach, describe, expect, it, mock } from 'bun:test';
+import { afterEach, describe, expect, it, mock } from 'bun:test';
 
-const copyS3FileMock = mock(() => Promise.resolve());
-const s3ListMock = mock(() => Promise.resolve({ contents: [] }));
+const copyObjectMock = mock();
+const listObjectsV2Mock = mock();
+mock.module('../src/s3Client', () => ({
+  S3Client: {
+    copyObject: copyObjectMock,
+    listObjectsV2: listObjectsV2Mock
+  }
+}));
 const listCommitStatusesForRefMock = mock(() => ({
   data: [
     {
@@ -18,18 +24,6 @@ const listCommitStatusesForRefMock = mock(() => ({
     }
   ]
 }));
-const updateCommitStatusMock = mock();
-
-mock.module('../src/copyS3File', () => ({
-  copyS3File: copyS3FileMock
-}));
-
-mock.module('../src/s3Client', () => ({
-  s3Client: {
-    list: s3ListMock
-  }
-}));
-
 mock.module('../src/getOctokit', () => ({
   getOctokit: mock(() => ({
     rest: {
@@ -39,21 +33,20 @@ mock.module('../src/getOctokit', () => ({
     }
   }))
 }));
-
+const updateCommitStatusMock = mock();
 mock.module('../src/updateCommitStatus', () => ({
   updateCommitStatus: updateCommitStatusMock
 }));
-
 mock.module('@octokit/rest', () => ({
   Octokit: mock()
 }));
 
-beforeEach(() => {
-  mock.clearAllMocks();
-});
-
 const pathPrefix = `${NEW_IMAGES_DIRECTORY}/030928b2c4b48ab4d3b57c8e0b0f7a56db768ef5`;
 describe('acceptVisualChanges', () => {
+  afterEach(() => {
+    mock.clearAllMocks();
+  });
+
   describe('filterNewImages', () => {
     it('should filter only the new images from the given paths', () => {
       const newImage = `${pathPrefix}/SMALL/pdpPage/new.png`;
@@ -103,16 +96,18 @@ describe('acceptVisualChanges', () => {
         ],
         expectedBucket
       );
-      expect(copyS3FileMock).toHaveBeenCalledWith(
-        `${pathPrefix}/SMALL/pdpPage/new.png`,
-        `${BASE_IMAGES_DIRECTORY}/SMALL/pdpPage/base.png`,
-        expectedBucket
-      );
-      expect(copyS3FileMock).toHaveBeenCalledWith(
-        `${pathPrefix}/SMALL/srpPage/new.png`,
-        `${BASE_IMAGES_DIRECTORY}/SMALL/srpPage/base.png`,
-        expectedBucket
-      );
+      expect(copyObjectMock).toHaveBeenCalledWith({
+        Bucket: expectedBucket,
+        CopySource: `${expectedBucket}/${pathPrefix}/SMALL/pdpPage/new.png`,
+        Key: `${BASE_IMAGES_DIRECTORY}/SMALL/pdpPage/base.png`,
+        ACL: 'bucket-owner-full-control'
+      });
+      expect(copyObjectMock).toHaveBeenCalledWith({
+        Bucket: expectedBucket,
+        CopySource: `${expectedBucket}/${pathPrefix}/SMALL/srpPage/new.png`,
+        Key: `${BASE_IMAGES_DIRECTORY}/SMALL/srpPage/base.png`,
+        ACL: 'bucket-owner-full-control'
+      });
     });
 
     it('should throw error if other required checks have not yet passed', async () => {
@@ -142,7 +137,8 @@ describe('acceptVisualChanges', () => {
         })
       ).rejects.toThrow();
 
-      expect(copyS3FileMock).not.toHaveBeenCalled();
+      expect(listObjectsV2Mock).not.toHaveBeenCalled();
+      expect(copyObjectMock).not.toHaveBeenCalled();
       expect(updateCommitStatusMock).not.toHaveBeenCalled();
     });
 
@@ -156,7 +152,8 @@ describe('acceptVisualChanges', () => {
         owner: 'owner'
       });
 
-      expect(copyS3FileMock).not.toHaveBeenCalled();
+      expect(listObjectsV2Mock).not.toHaveBeenCalled();
+      expect(copyObjectMock).not.toHaveBeenCalled();
       expect(updateCommitStatusMock).toHaveBeenCalled();
     });
   });

app/backend/test/fetchCurrentPage.test.ts

@@ -1,39 +1,25 @@
 import { fetchCurrentPage } from '../src/fetchCurrentPage';
-import { beforeEach, describe, expect, it, mock } from 'bun:test';
+import { describe, expect, it, mock } from 'bun:test';
 import { NEW_IMAGES_DIRECTORY } from 'shared';
 
 mock.module('../src/getTemporaryObjectUrl', () => ({
-  getTemporaryObjectUrl: mock(() => Promise.resolve('url'))
+  getTemporaryObjectUrl: mock(() => 'url')
 }));
-
 const pathPrefix = `${NEW_IMAGES_DIRECTORY}/hash`;
-const listMock = mock(async () => ({
-  contents: [
-    { key: `${pathPrefix}/SMALL/srpPage/base.png` },
-    { key: `${pathPrefix}/SMALL/srpPage/diff.png` },
-    { key: `${pathPrefix}/SMALL/srpPage/new.png` },
-    { key: `${pathPrefix}/EXTRA_LARGE/pdpPage/new.png` }
+const listObjectsV2Mock = mock(() => ({
+  Contents: [
+    { Key: `${pathPrefix}/SMALL/srpPage/base.png` },
+    { Key: `${pathPrefix}/SMALL/srpPage/diff.png` },
+    { Key: `${pathPrefix}/SMALL/srpPage/new.png` },
+    { Key: `${pathPrefix}/EXTRA_LARGE/pdpPage/new.png }` }
   ]
 }));
-
 mock.module('../src/s3Client', () => ({
-  s3Client: {
-    list: listMock
+  S3Client: {
+    listObjectsV2: listObjectsV2Mock
   }
 }));
 
-beforeEach(() => {
-  mock.clearAllMocks();
-  listMock.mockImplementation(async () => ({
-    contents: [
-      { key: `${pathPrefix}/SMALL/srpPage/base.png` },
-      { key: `${pathPrefix}/SMALL/srpPage/diff.png` },
-      { key: `${pathPrefix}/SMALL/srpPage/new.png` },
-      { key: `${pathPrefix}/EXTRA_LARGE/pdpPage/new.png` }
-    ]
-  }));
-});
-
 describe('fetchCurrentPage', () => {
   it('should get first page of images', async () => {
     const result = await fetchCurrentPage({

app/backend/test/getGroupedKeys.test.ts

@@ -1,35 +1,26 @@
 import { NEW_IMAGES_DIRECTORY } from 'shared';
 import { getGroupedKeys } from '../src/getGroupedKeys';
-import { beforeEach, describe, expect, it, mock } from 'bun:test';
-
-const listMock = mock(
-  async (): Promise<{ contents: Array<{ key: string }> }> => ({
-    contents: []
-  })
-);
+import { describe, expect, it, mock } from 'bun:test';
 
+const listObjectsV2Mock = mock();
 mock.module('../src/s3Client', () => ({
-  s3Client: {
-    list: listMock
+  S3Client: {
+    listObjectsV2: listObjectsV2Mock
   }
 }));
 
 const pathPrefix = `${NEW_IMAGES_DIRECTORY}/hash`;
 
-beforeEach(() => {
-  mock.clearAllMocks();
-});
-
 describe('getGroupedKeys', () => {
   it('returns only the keys where there is a base, new, and diff', async () => {
-    listMock.mockImplementationOnce(async () => ({
-      contents: [
-        { key: `${pathPrefix}/EXTRA_LARGE/srpPage/base.png` },
-        { key: `${pathPrefix}/SMALL/srpPage/base.png` },
-        { key: `${pathPrefix}/EXTRA_LARGE/pdpPage/base.png` },
-        { key: `${pathPrefix}/EXTRA_LARGE/pdpPage/diff.png` },
-        { key: `${pathPrefix}/EXTRA_LARGE/pdpPage/new.png` },
-        { key: 'ome/actions-runner/something' }
+    listObjectsV2Mock.mockImplementationOnce(() => ({
+      Contents: [
+        { Key: `${pathPrefix}/EXTRA_LARGE/srpPage/base.png` },
+        { Key: `${pathPrefix}/SMALL/srpPage/base.png` },
+        { Key: `${pathPrefix}/EXTRA_LARGE/pdpPage/base.png` },
+        { Key: `${pathPrefix}/EXTRA_LARGE/pdpPage/diff.png` },
+        { Key: `${pathPrefix}/EXTRA_LARGE/pdpPage/new.png` },
+        { Key: 'ome/actions-runner/something' }
       ]
     }));
     const paths = await getGroupedKeys('hash', 'bucket');
@@ -46,11 +37,11 @@ describe('getGroupedKeys', () => {
   });
 
   it('returns keys where there is a new image but no base image', async () => {
-    listMock.mockImplementationOnce(async () => ({
-      contents: [
-        { key: `${pathPrefix}/EXTRA_LARGE/srpPage/base.png` },
-        { key: `${pathPrefix}/SMALL/pdpPage/new.png` },
-        { key: `${pathPrefix}/EXTRA_LARGE/pdpPage/base.png` }
+    listObjectsV2Mock.mockImplementationOnce(() => ({
+      Contents: [
+        { Key: `${pathPrefix}/EXTRA_LARGE/srpPage/base.png` },
+        { Key: `${pathPrefix}/SMALL/pdpPage/new.png` },
+        { Key: `${pathPrefix}/EXTRA_LARGE/pdpPage/base.png` }
       ]
     }));
     const paths = await getGroupedKeys('hash', 'bucket');
@@ -63,15 +54,15 @@ describe('getGroupedKeys', () => {
   });
 
   it('returns multiple pages', async () => {
-    listMock.mockImplementationOnce(async () => ({
-      contents: [
-        { key: `${pathPrefix}/EXTRA_LARGE/srpPage/base.png` },
-        { key: `${pathPrefix}/SMALL/srpPage/base.png` },
-        { key: `${pathPrefix}/SMALL/srpPage/diff.png` },
-        { key: `${pathPrefix}/SMALL/srpPage/new.png` },
-        { key: `${pathPrefix}/EXTRA_LARGE/pdpPage/base.png` },
-        { key: `${pathPrefix}/EXTRA_LARGE/pdpPage/diff.png` },
-        { key: `${pathPrefix}/EXTRA_LARGE/pdpPage/new.png` }
+    listObjectsV2Mock.mockImplementationOnce(() => ({
+      Contents: [
+        { Key: `${pathPrefix}/EXTRA_LARGE/srpPage/base.png` },
+        { Key: `${pathPrefix}/SMALL/srpPage/base.png` },
+        { Key: `${pathPrefix}/SMALL/srpPage/diff.png` },
+        { Key: `${pathPrefix}/SMALL/srpPage/new.png` },
+        { Key: `${pathPrefix}/EXTRA_LARGE/pdpPage/base.png` },
+        { Key: `${pathPrefix}/EXTRA_LARGE/pdpPage/diff.png` },
+        { Key: `${pathPrefix}/EXTRA_LARGE/pdpPage/new.png` }
       ]
     }));
     const paths = await getGroupedKeys('hash', 'bucket');
@@ -96,18 +87,18 @@ describe('getGroupedKeys', () => {
   });
 
   it('tells us if the commit hash was not associated with a visual regression test failure', async () => {
-    listMock.mockImplementationOnce(async () => ({ contents: [] }));
+    listObjectsV2Mock.mockImplementationOnce(() => ({ Contents: [] }));
     expect(getGroupedKeys('hash', 'bucket')).rejects.toThrow(
       'The commit hash was not associated with any visual regression test failures'
     );
   });
 
   it('tells us if there are no new or diff images associated with the commit hash', async () => {
-    listMock.mockImplementationOnce(async () => ({
-      contents: [
-        { key: `${pathPrefix}/EXTRA_LARGE/srpPage/base.png` },
-        { key: `${pathPrefix}/SMALL/srpPage/base.png` },
-        { key: `${pathPrefix}/EXTRA_LARGE/pdpPage/base.png` }
+    listObjectsV2Mock.mockImplementationOnce(() => ({
+      Contents: [
+        { Key: `${pathPrefix}/EXTRA_LARGE/srpPage/base.png` },
+        { Key: `${pathPrefix}/SMALL/srpPage/base.png` },
+        { Key: `${pathPrefix}/EXTRA_LARGE/pdpPage/base.png` }
       ]
     }));
     expect(getGroupedKeys('hash', 'bucket')).rejects.toThrow(