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

fix: Multiple fileInputs in a repeatingSet #4188

Merged
merged 7 commits into from
Aug 13, 2024

Conversation

dsamojlenko
Copy link
Member

@dsamojlenko dsamojlenko commented Aug 12, 2024

Summary | Résumé

Fixes multiple fileInputs in a repeatingSet.

Previously, if you attempted to attach more than one file in a repeatingSet you would see the following error in the console and the form submit would fail:

image

Additionally, through testing it appears we'll need to set the maximum total file size lower to accommodate payload limits when sending to Notify. Dropped the overall bodySizeLimit for serverActions to 5MB, meaning the file size limit is actually about 4MB.

Test instructions

  • Create a form with a fileInput in a repeatingSet. ie: multiple-file-uploads-2024-08-12.json
  • Configure the form to send by email.
  • Test the form in Preview and attach more than one file in the repeatingSet.

Copy link
Contributor

@dsamojlenko dsamojlenko marked this pull request as ready for review August 13, 2024 13:51
@dsamojlenko dsamojlenko changed the title Fix for dynamic row keys fix: Multiple fileInputs in a repeatingSet Aug 13, 2024
Copy link
Contributor

@thiessenp-cds thiessenp-cds left a comment

Choose a reason for hiding this comment

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

Unrelated to this PR, I think this code could really help readability if it were refactored. Especially the deep nesting, including nested try-catches.

Copy link
Contributor

@thiessenp-cds thiessenp-cds left a comment

Choose a reason for hiding this comment

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

I tested in the Preview mode and forms-form mode with files under and over the combined 5MB limit, works as expected. This code also looks pretty isolated so a full app test probably isn't necessary (beyond the happy paths)
Nice work, LGTM :)

@dsamojlenko dsamojlenko merged commit 2d665f8 into develop Aug 13, 2024
13 checks passed
@dsamojlenko dsamojlenko deleted the fix/dynamic_row_attachments branch August 13, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants