aws-amplify / amplify-ui

Amplify UI is a collection of accessible, themeable, performant React (and more!) components that can connect directly to the cloud.
https://ui.docs.amplify.aws
Apache License 2.0
896 stars 285 forks source link

processFile in StorageManager causing endless uploads #5467

Closed OperationalFallacy closed 2 months ago

OperationalFallacy commented 2 months ago

Before creating a new issue, please confirm:

On which framework/platform are you having an issue?

React

Which UI component?

Storage (Storage Manager)

How is your app built?

Next.js

What browsers are you seeing the problem on?

Chrome, Safari

Which region are you seeing the problem in?

us-east-1

Please describe your bug.

I'm developing the Amplify Gen2 project, and I used StorageManager. It was working fine.

Now, if there is processFile={processFile} - it goes into an endless upload loop. Without processFile it works fine.

image image

This has to do with a weird local re-rendering, not the component itself, but I don't know how to approach it. Any advice is appreciated!

What's the expected behaviour?

It should upload a file with the processFile function.

Help us reproduce the bug!

I added some code samples, but it's probably a specific issue with the local environment.

Code Snippet

_app.tsx

"use client";
import type { AppProps } from "next/app";
import "@aws-amplify/ui-react/styles.css";
import "@fontsource-variable/inter";
import { Authenticator, useAuthenticator } from "@aws-amplify/ui-react";
import { Toaster } from "react-hot-toast";
import AuthForm from "../components/Auth";
import Layout, { Loader } from "../components/Layout";
import { useAuth } from "../hooks/useAuth";

const MyCustomApp = ({ Component, pageProps }: AppProps) => {
  const { handleLoginClick, handleCancelAuth, handleLogoutClick } = useAuth();

  const { authStatus } = useAuthenticator((context) => [context.authStatus]);
  console.log("MyCustomApp loaded...");
  return (
    <Layout
      handleLoginClick={handleLoginClick}
      handleLogoutClick={handleLogoutClick}
    >
      {authStatus === "configuring" && Loader}
      {authStatus !== "authenticated" && (
        <AuthForm handleCancelAuth={handleCancelAuth} />
      )}
      <>
        <Toaster toastOptions={{ duration: 5000 }} />
        <Component {...pageProps} />
      </>
    </Layout>
  );
};

export default function AppDefault(pageProps: AppProps) {
  return (
    <Authenticator.Provider>
      <MyCustomApp {...pageProps} />
    </Authenticator.Provider>
  );
}

index.tsx

"use client";
import { Flex, Text, useAuthenticator } from "@aws-amplify/ui-react";
import { StorageManager } from "@aws-amplify/ui-react-storage";
import { v4 as uuidv4 } from "uuid";

function AuthenticatedApp() {
  const processFile = ({ file, key }: { file: File; key: string }) => {
    console.debug("Running processFile", key);
    const fileExtension = file.name.split(".").pop();
    const new_key = `${uuidv4()}.${fileExtension}`;
    return { file, key: new_key };
  };

  return (
    <Flex direction="column" maxWidth="lg" margin="auto" padding="20px">
      <Text>My Documents</Text>
      <StorageManager
        acceptedFileTypes={["application/pdf"]}
        path={({ identityId }) => `private/${identityId}/`}
        maxFileCount={1}
        isResumable
        processFile={processFile}
        onFileRemove={({ key }) => {
          console.log(`File removed with key ${key}`);
        }}
        onUploadError={(error, { key }) => {
          console.error(`Error uploading file with key ${key}:`, error);
        }}
        onUploadSuccess={({ key }) => {
          console.log(`File uploaded successfully with key ${key}`);
        }}
        onUploadStart={({ key }) => {
          console.log(`File upload started with key ${key}`);
        }}
      />
    </Flex>
  );
}

export default function Home() {
  const { authStatus } = useAuthenticator((context) => [context.authStatus]);
  if (authStatus !== "authenticated") {
    return (
      <Flex justifyContent={"center"} wrap={"wrap"}>
        Not Authenticated!
      </Flex>
    );
  }

  return <AuthenticatedApp />;
}

Console log output

No response

Additional information and screenshots

No response

OperationalFallacy commented 2 months ago

This is an actual defect. Tested it with a few different versions.

"@aws-amplify/ui-react-storage@3.1.3" - this version is working as expected.

But 3.1.4 and up make re-auth and re-upload storms as described.

thaddmt commented 2 months ago

Thanks for the update! We're working on a repro now

calebpollman commented 2 months ago

@OperationalFallacy While we look in to this, can you try moving the processFile declaration outside of AuthenticatedApp?

+ const processFile = ({ file, key }: { file: File; key: string }) => {
+   console.debug("Running processFile", key);
+   const fileExtension = file.name.split(".").pop();
+   const new_key = `${uuidv4()}.${fileExtension}`;
+   return { file, key: new_key };
+ };

function AuthenticatedApp() {
-  const processFile = ({ file, key }: { file: File; key: string }) => {
-    console.debug("Running processFile", key);
-    const fileExtension = file.name.split(".").pop();
-    const new_key = `${uuidv4()}.${fileExtension}`;
-    return { file, key: new_key };
-  };

Curious if processFile having a stable reference would unblock you for the time being

OperationalFallacy commented 2 months ago

Yeah, still bombs

const processFile = ({ file, key }: { file: File; key: string }) => {
  return { file, key: "static-key-to-test-behavior" };
};

function AuthenticatedApp() {
thaddmt commented 2 months ago

Fix was released in the latest version 3.1.6 - https://www.npmjs.com/package/@aws-amplify/ui-react-storage?activeTab=versions

OperationalFallacy commented 2 months ago

I did a quick test: it doesn't get into an endless loop. However, it is still making two S3 put calls. Changing to 3.1.3 is making it work as expected.

thaddmt commented 2 months ago

Hm I am not able to repro so far, can you send an example of your network log?

OperationalFallacy commented 2 months ago

Hm I am not able to repro so far, can you send an example of your network log?

Thanks for checking. Network requests were two PUT requests to the bucket, I didn't see anything special.

Could it be the user hook messing with the calls? I saw that StorageManager is using it as well under the hood.

function AuthenticatedApp() {
  const { user } = useAuthenticator((context) => [context.user]);

  function processFile({ file, key }: { file: File; key: string }) {
    return {
      file,
      key,
      metadata: {
        aud: user.userId,
      },
    };
  }

        <StorageManager
        acceptedFileTypes={["pdf/*"]}
        path={({ identityId }) => `private/${identityId}/default/`}
        maxFileCount={1}
        isResumable
        processFile={processFile}
      />
reesscot commented 2 months ago

@OperationalFallacy We can't reproduce two PUT network calls. Do you mean 1 OPTIONS call and 1 PUT call? That's what we would expect to see

OperationalFallacy commented 2 months ago

Actually, no. Two put requests, exactly the same with two respective processFile calls

PUT /private/us-east-1%3A{identity}/default/test.webp HTTP/1.1
Content-Type: image/webp
Accept: */*
Authorization: AWS4-HMAC-SHA256 Credential=xxx
Sec-Fetch-Site: cross-site
Accept-Language: en-US,en;q=0.9
Accept-Encoding: gzip, deflate, br
Sec-Fetch-Mode: cors
Host: amplify-xxx.s3.us-east-1.amazonaws.com
Origin: http://localhost:3000
User-Agent: Mozilla/5.0 (Macintosh; xxx
Referer: http://localhost:3000/
Content-Length: 1616252
Connection: keep-alive
Sec-Fetch-Dest: empty
x-amz-meta-aud: someIdentityId <--- correctly set metadata 
x-amz-date: 20240727T023248Z
x-amz-security-token: xxx
x-amz-user-agent: aws-amplify/6.4.3 storage/1 framework/2 StorageManager ui-react-storage/3.1.6
x-amz-content-sha256: UNSIGNED-PAYLOAD

Tried to move the function from AuthenticatedApp()

function processFile({
  file,
  key,
  userId,
}: {
  file: File;
  key: string;
  userId: string;
}) {
  console.debug("Processing file", key);
  return {
    file,
    key,
    metadata: {
      aud: userId,
    },
  };
}

function AuthenticatedApp() {
...
      <StorageManager
        acceptedFileTypes={["pdf/*"]}
        path={({ identityId }) => `private/${identityId}/default/`}
        maxFileCount={1}
        isResumable
        processFile={({ file, key }) =>
          processFile({ file, key, userId: "someIdentityId" })
        }
      />

Dependencies

  "devDependencies": {
    "@aws-amplify/backend": "^1.0.0",
    "@aws-amplify/backend-cli": "^1.0.0",
    "@types/node": "^16",
    "@types/react": "^18.3.3",
    "@types/react-dom": "^18.3.0",
    "@typescript-eslint/eslint-plugin": "^7",
    "@typescript-eslint/parser": "^7",
    "aws-cdk": "^2.143.0",
    "aws-cdk-lib": "^2.143.0",
    "constructs": "^10.0.0",
    "esbuild": "^0.21.4",
    "eslint": "^8",
    "eslint-config-prettier": "^9.1.0",
    "eslint-import-resolver-typescript": "^3.6.1",
    "eslint-plugin-import": "^2.29.1",
    "eslint-plugin-prettier": "^5.2.1",
    "prettier": "^3.3.3",
    "projen": "^0.82.7",
    "ts-node": "^10.9.2",
    "tsx": "^4.15.0",
    "typescript": "^5.5.3"
  },
  "dependencies": {
    "@aws-amplify/geo": "^3.0.0",
    "@aws-amplify/ui-react": "^6.0.0",
    "@aws-amplify/ui-react-storage": "3.1.6",
    "@fontsource-variable/inter": "^5.0.15",
    "@fontsource/inter": "^5.0.15",
    "aws-amplify": "^6.0.0",
    "date-fns": "^3.6.0",
    "next": "^14.2.5",
    "react": "^18.3.1",
    "react-dom": "^18.3.1",
    "react-hot-toast": "2.4.1",
    "recoil": "^0.7.7"
  },
  "engines": {
    "node": ">= 16.14.0"
  },
  "license": "UNLICENSED",
  "version": "0.0.0",
  "packageManager": "yarn@4.3.1",

I'm good with my development, I can use slightly older version for now.

Thank you!

reesscot commented 2 months ago

@OperationalFallacy We still want to get to the bottom of this. Is it possible to create a minimum reproduction repo we can use to reproduce this issue? We're unable to reproduce with the example code provided. Are you using any other props on StorageManager than what is shown?

tytremblay commented 1 month ago

I'm also getting two POST requests when trying to upload a file with StorageManager.

image

This results in onUploadSuccess being called twice. If i remove the processFile property, it only makes one POST request.

Component:

'use client';
import { ThemeProvider as AmplifyThemeProvider } from '@aws-amplify/ui-react';
import { StorageManager } from '@aws-amplify/ui-react-storage';
import { H3 } from '@rtr/web-ui';
import { useTheme } from 'next-themes';
import path from 'path';
import { useCallback, useMemo } from 'react';
import { useNotifications } from '../../../store/NotificationsProvider';
import { useProjects } from '../../../store/ProjectsProvider';
import { useUser } from '../../../store/UserProvider';
import { colorTheme, convertColorMode } from '../../../theme';

// Not exported from ui-react-storage for some reason
interface ProcessFileParams {
  file: File;
  key: string;
}

function processFile({ file }: ProcessFileParams): ProcessFileParams {
  const fileName = file.name.replace('.zip', '');
  // https://ui.docs.amplify.aws/react/connected-components/storage/storagemanager#adding-metadata
  return { file, key: `${fileName}/${file.name}` };
}

export function UploadProject() {
  const { tenantId, userId } = useUser();
  const { addProject } = useProjects();
  const { addErrorNotification } = useNotifications();

  const { resolvedTheme } = useTheme();
  const colorScheme = convertColorMode(resolvedTheme);

  const basePath = useMemo(
    () => `${tenantId}/projects/${userId}/`,
    [userId, tenantId],
  );

  const handleUploadSuccess = useCallback(
    (event: { key?: string }) => {
      // Note: storage manager 'path' is prepended to the file 'key', which is generated by processFile.
      // here, it'll look like `[tenantId]/projects/[userId]/[projectName]/[fileName].zip`
      const key = event.key;
      if (!key) {
        addErrorNotification(
          'Project Upload Error',
          'An error occurred while uploading the project.  No file key was found after a successful upload.',
        );
        return;
      }

      if (!tenantId || !userId) {
        addErrorNotification(
          'Project Upload Error',
          'An error occurred while uploading the project.  Unable to determine user.',
        );
        return;
      }

      const fileData = path.parse(key);

      addProject({
        userId: userId,
        tenantId: tenantId,
        name: fileData.name,
        path: key,
        metadataPath: 'FIXME',
        resultsPath: 'FIXME',
      });
    },
    [addErrorNotification, addProject, tenantId, userId],
  );

  return (
    <div className="flex flex-col gap-4 mt-6">
      <H3>Upload a new optimization project</H3>
      {
        // the theme resolves late, often after the initial page render
        resolvedTheme ? (
          <AmplifyThemeProvider colorMode={colorScheme} theme={colorTheme}>
            {tenantId && userId ? (
              <StorageManager
                acceptedFileTypes={['.zip']}
                path={basePath}
                maxFileCount={1}
                processFile={processFile}
                displayText={{
                  dropFilesText: 'Drop project zip here or',
                }}
                onUploadSuccess={e => {
                  handleUploadSuccess(e);
                }}
                showThumbnails={false}
              />
            ) : (
              <div>Unable to determine user</div>
            )}
          </AmplifyThemeProvider>
        ) : (
          <></>
        )
      }
    </div>
  );
}
brandonwatson commented 1 month ago

I can also confirm that I have this problem, and had zeroed it back to the inclusion of the ProcessFile prop. In my case, all I am trying to do is change the file name.

from my package.json "@aws-amplify/ui-react-storage": "^3.3.1",

I tried updating to 3.3.2 with the same results.

LOGGING: inside processFile - file: kitchen2.png ImageUploadModal.tsx:48 LOGGING: inside processFile - file: kitchen2.png ImageUploadModal.tsx:59 LOGGING: inside handleUploadStart - key: protected/images/{identityId}/d58e4db3-5a08-4204-845f-0f7546b88e67/5c17eea8-1b96-4982-9ec6-0ee2bdc167d5/78cc86e6-d6c9-47af-acb5-9cc2126ad2ef.png ImageUploadModal.tsx:59 LOGGING: inside handleUploadStart - key: protected/images/{identityId}/d58e4db3-5a08-4204-845f-0f7546b88e67/5c17eea8-1b96-4982-9ec6-0ee2bdc167d5/5a574898-fb58-4bed-9d7d-e8a176d8e814.png ImageUploadModal.tsx:72 LOGGING: Upload successful key: protected/images/{identityId}/d58e4db3-5a08-4204-845f-0f7546b88e67/5c17eea8-1b96-4982-9ec6-0ee2bdc167d5/78cc86e6-d6c9-47af-acb5-9cc2126ad2ef.png, imageId: 78cc86e6-d6c9-47af-acb5-9cc2126ad2ef.png ImageUploadModal.tsx:72 LOGGING: Upload successful key: protected/images/{identityId}/d58e4db3-5a08-4204-845f-0f7546b88e67/5c17eea8-1b96-4982-9ec6-0ee2

    async function processFile({
        file,
    }: {
        file: File;
    }): Promise<{ file: File; key: string }> {
        console.log(`LOGGING: inside processFile - file: ${file.name}`);
        const fileExtension = file.name.split('.').pop();
        const newKey = `${uuidv4()}.${fileExtension}`;

        // Create a new File object with the new name
        const newFile = new File([file], newKey, { type: file.type });

        return { file: newFile, key: newKey };
    }