Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MPDX-8440] Remove TNT import file upload restriction #1180

Merged
merged 2 commits into from
Nov 11, 2024

Conversation

canac
Copy link
Contributor

@canac canac commented Nov 11, 2024

Description

The Amplify maximum body size for lambda file uploads of 1MB is too small for some users' multi-MB TNT uploads. Instead of uploading TNT exports to the lambda, upload them directly to the API and remove the file size restriction.

I also cleaned up a few things in the TntConnect component:

  • Give variable a more descriptive name
  • Remove a redundant call to revokeObjectURL

Testing

I downloaded and used this fixture to test the upload.

MPDX-8440

https://secure.helpscout.net/conversation/2756145432/1255279/
https://secure.helpscout.net/conversation/2756616046/1255472/

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

* Give variable a more descriptive name
* Remove a redundant call to revokeObjectURL
@canac canac added the Preview Environment Add this label to create an Amplify Preview label Nov 11, 2024
@canac canac requested a review from dr-bizz November 11, 2024 15:52
@canac canac self-assigned this Nov 11, 2024
Copy link

Preview branch generated at https://hs-1255279-tnt-import-size.d3dytjb8adxkk5.amplifyapp.com

},
body: form,
},
).catch(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fetch throws if there was a network error such that it couldn't make the request it all. It resolves to a response with ok: false if there was an HTTP error (i.e. 4xx or 5xx status code). That's why I switched this error message to be more accurate.

Copy link

Bundle sizes [mpdx-react]

Compared against 62fca45

No significant changes found

@@ -174,10 +178,6 @@ const TntConnect: React.FC<Props> = ({ accountListId }: Props) => {
return;
}

if (tntFile) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old blob will already be released by the useEffect cleanup function on line 164.

Copy link
Contributor

@dr-bizz dr-bizz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great! As I didn't build it, I will let @caleballdrin test the UI as he built the UI and had a lot of the TNT files.

@dr-bizz
Copy link
Contributor

dr-bizz commented Nov 11, 2024

@caleballdrin Can you test the UI of this PR by uploading some of your test TNT files?

@canac canac changed the title [HS-1255279] Remove TNT import file upload restriction [MPDX-8440] Remove TNT import file upload restriction Nov 11, 2024
Copy link
Contributor

@caleballdrin caleballdrin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested it and it worked! However, I didn't try it with a file over 1mb.

@canac
Copy link
Contributor Author

canac commented Nov 11, 2024

It worked with the 13.5MB one in the HelpScout ticket https://secure.helpscout.net/api/v1/conversations/2756145432/attachments/733246685/file

@canac canac merged commit 5643e76 into main Nov 11, 2024
19 checks passed
@canac canac deleted the hs-1255279-tnt-import-size branch November 11, 2024 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Preview Environment Add this label to create an Amplify Preview
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants