[7418] AWSFileUpload control#4827
Open
jvega190 wants to merge 5 commits intocraftercms:developfrom
Open
Conversation
Contributor
WalkthroughAdds a new Forms Engine control component Changes
Sequence DiagramsequenceDiagram
autonumber
actor User
participant AwsFileUpload as AwsFileUpload<br/>(Component)
participant SingleFileUpload as SingleFileUpload<br/>(Component)
participant Uppy as Uppy/XHRUpload
participant S3API as S3 Upload API
participant FormEngine as Forms Engine
User->>AwsFileUpload: Open form / select file
AwsFileUpload->>SingleFileUpload: provide endpoint, siteId/profileId, props, callbacks
User->>SingleFileUpload: Click "Choose File" / confirm
SingleFileUpload->>Uppy: enqueue file & start upload (PUT/POST)
Uppy->>S3API: Upload file
S3API-->>Uppy: Return upload response (item list)
Uppy-->>SingleFileUpload: onComplete
SingleFileUpload-->>AwsFileUpload: invoke onComplete handler with result
AwsFileUpload->>AwsFileUpload: parse first successful item -> AwsFile{key,bucket,url}
AwsFileUpload->>FormEngine: setValue(AwsFile)
FormEngine-->>AwsFileUpload: new value prop
AwsFileUpload->>User: render s3:// location and preview (image/video/asset)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
ui/app/src/components/SingleFileUpload/SingleFileUpload.tsx (1)
411-428: EmptyTypographyrendered whenfileis null.When
showFileDetailsis true but no file is selected, an emptyTypographyelement withmb: 2is rendered, causing unnecessary margin. Consider moving the file check outside.♻️ Suggested improvement
- {showFileDetails && ( - <Typography variant="subtitle1" component="h2" sx={{ mb: 2 }}> - {file && ( - <Box - component="em" - className={`single-file-upload--filename ${fileNameErrorClass}`} - sx={{ - overflow: 'hidden', - textOverflow: 'ellipsis', - whiteSpace: 'nowrap' - }} - title={file.name} - > - {file.name} - </Box> - )} - </Typography> - )} + {showFileDetails && file && ( + <Typography variant="subtitle1" component="h2" sx={{ mb: 2 }}> + <Box + component="em" + className={`single-file-upload--filename ${fileNameErrorClass}`} + sx={{ + overflow: 'hidden', + textOverflow: 'ellipsis', + whiteSpace: 'nowrap' + }} + title={file.name} + > + {file.name} + </Box> + </Typography> + )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/SingleFileUpload/SingleFileUpload.tsx` around lines 411 - 428, When showFileDetails is true but file is null the code still renders an empty Typography (mb: 2); change the condition so Typography is only rendered when both showFileDetails and file are truthy (i.e., move the file check to wrap the Typography element), keeping the inner Box (single-file-upload--filename) and its title/file.name logic intact; update references in SingleFileUpload.tsx around showFileDetails and file so no empty Typography is produced.ui/app/src/components/FormsEngine/controls/AWSFileUpload.tsx (1)
124-133: Considersandboxattribute for iframe security.While the URL comes from a trusted S3 response, in security-sensitive environments consider adding a
sandboxattribute (e.g.,sandbox="allow-same-origin") to restrict iframe capabilities. Note that this may affect PDF rendering in some browsers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/controls/AWSFileUpload.tsx` around lines 124 - 133, Add a sandbox attribute to the iframe created in AWSFileUpload.tsx (the Box with component="iframe" used when fileType === 'asset' and src={value.url}) to limit iframe capabilities; set sandbox to an appropriate value such as "allow-same-origin" (or "allow-same-origin allow-scripts" if needed for rendering) and document the caveat that this may affect PDF rendering in some browsers so test after change.ui/app/src/components/FormsEngine/lib/valueRetrievers.ts (1)
233-235: Add type annotations for consistency and safety.The function lacks parameter and return type annotations. Consider adding them for consistency with other typed extractors and to clarify that the return value may be
undefinedwhen the array is empty.♻️ Suggested type annotations
-export function awsFileExtractor(value) { - return arrayFieldExtractor(value)[0]; +export function awsFileExtractor(value: unknown): unknown { + return arrayFieldExtractor(value)[0]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/valueRetrievers.ts` around lines 233 - 235, Add TypeScript annotations to awsFileExtractor: annotate the parameter type to match the input expected by arrayFieldExtractor (e.g., the same type used by other extractors) and set the return type to include undefined (e.g., FileType | undefined) to reflect that arrayFieldExtractor(value)[0] may be absent; ensure the function signature aligns with arrayFieldExtractor's parameter type and the project’s File/Attachment type names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/AWSFileUpload.tsx`:
- Around line 63-67: The bucket field in the AwsFile construction (awsFile:
AwsFile) can become "bucketName/undefined" when item.bucketName is truthy but
item.prefix is nullish; update the bucket assembly to only append the prefix
when item.prefix is non-null/defined and non-empty (e.g., check item.prefix !==
undefined && item.prefix !== null && item.prefix !== '' or use a truthy check)
so the bucket becomes either "bucketName/prefix" or just "bucketName" (and still
fall back to item.bucket ?? '' when item.bucketName is falsy); modify the code
that builds the bucket string near where awsFile is created to include that
conditional.
- Around line 121-123: In AWSFileUpload.tsx the video source type is built from
value.url.match(/\.(.+)$/)?.[1] which can be undefined and yield
"video/undefined"; update the logic that generates the MIME subtype for the
<source> element (where value.url is used) to safely extract the extension and
fall back to a sensible default (e.g., "mp4") or map common extensions to proper
MIME subtypes before building `video/...`, so the type becomes
`video/<ext-or-fallback>` instead of possibly `video/undefined`.
- Around line 82-98: AWSFileUpload renders a parent <form
id="asset_upload_form"> and also uses <SingleFileUpload>, but SingleFileUpload
itself renders another <form id="asset_upload_form"> (see SingleFileUpload JSX
around line 378), causing nested forms and duplicate IDs; fix by removing the
inner form wrapper from SingleFileUpload so it no longer emits a <form> (keep
only the input/button elements and attach handlers like onComplete/disabled to
those elements) and rely on the parent AWSFileUpload form, or if you must keep a
form in SingleFileUpload, rename its id and add a prop (e.g., formTarget/formId)
to pass a unique id from AWSFileUpload to avoid duplication and nesting (update
SingleFileUpload’s interface and usages accordingly).
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/controls/AWSFileUpload.tsx`:
- Around line 124-133: Add a sandbox attribute to the iframe created in
AWSFileUpload.tsx (the Box with component="iframe" used when fileType ===
'asset' and src={value.url}) to limit iframe capabilities; set sandbox to an
appropriate value such as "allow-same-origin" (or "allow-same-origin
allow-scripts" if needed for rendering) and document the caveat that this may
affect PDF rendering in some browsers so test after change.
In `@ui/app/src/components/FormsEngine/lib/valueRetrievers.ts`:
- Around line 233-235: Add TypeScript annotations to awsFileExtractor: annotate
the parameter type to match the input expected by arrayFieldExtractor (e.g., the
same type used by other extractors) and set the return type to include undefined
(e.g., FileType | undefined) to reflect that arrayFieldExtractor(value)[0] may
be absent; ensure the function signature aligns with arrayFieldExtractor's
parameter type and the project’s File/Attachment type names.
In `@ui/app/src/components/SingleFileUpload/SingleFileUpload.tsx`:
- Around line 411-428: When showFileDetails is true but file is null the code
still renders an empty Typography (mb: 2); change the condition so Typography is
only rendered when both showFileDetails and file are truthy (i.e., move the file
check to wrap the Typography element), keeping the inner Box
(single-file-upload--filename) and its title/file.name logic intact; update
references in SingleFileUpload.tsx around showFileDetails and file so no empty
Typography is produced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 17acf4b8-1a62-46b7-b948-cefbae949cdf
📒 Files selected for processing (5)
ui/app/src/components/FormsEngine/controls/AWSFileUpload.tsxui/app/src/components/FormsEngine/lib/controlMap.tsui/app/src/components/FormsEngine/lib/valueRetrievers.tsui/app/src/components/FormsEngine/lib/valueSerializers.tsui/app/src/components/SingleFileUpload/SingleFileUpload.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/AWSFileUpload.tsx`:
- Around line 147-153: The getAwsFileType function's regexes fail on signed S3
URLs because they require the extension at end-of-string; update the three
patterns in getAwsFileType to allow an optional query string after the extension
(e.g., make them match extension followed by either ? or end-of-string) so URLs
like file.mp4?X-Amz-Signature=... are correctly detected as 'video'; adjust the
regexes for 'image', 'video', and 'asset' accordingly while keeping
case-insensitive matching.
- Around line 121-123: The current MIME type extraction in AWSFileUpload.tsx
(where <source src={value.url} type={`video/${value.url.match(/\.(.+)$/)?.[1] ??
'mp4'}`} /> is used) can capture query strings and fragments; replace the regex
with one that only captures the file extension before any ? or # (e.g.,
/\.([A-Za-z0-9]+)(?:[?#].*)?$/) and use that capture for the MIME subtype
(falling back to 'mp4' if no match) so URLs like video.mp4?token=... produce
'video/mp4' instead of 'video/mp4?token=...'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3bf6f0e2-805d-4180-8485-fa6fc2aeb6aa
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/AWSFileUpload.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/lib/valueSerializers.ts (1)
42-42: Minor:prepareAwsFileduplicatesprepareArray.Both functions return
{ item: value }. If you keep the single-object wrapping as-is, you can dropprepareAwsFileand point this entry atprepareArrayto reduce duplication. (If you adopt the array-wrap fix suggested on lines 220–222, this becomes moot.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/lib/valueSerializers.ts` at line 42, The 'aws-file-upload' serializer currently calls prepareAwsFile which duplicates prepareArray by returning { item: value }; remove that duplication by changing the mapping for the 'aws-file-upload' key to use prepareArray instead of prepareAwsFile and delete the now-unused prepareAwsFile function (or keep it only if referenced elsewhere). Update any imports/exports if prepareAwsFile was exported so there are no unused symbols remaining.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/app/src/components/FormsEngine/lib/valueSerializers.ts`:
- Line 42: The 'aws-file-upload' serializer currently calls prepareAwsFile which
duplicates prepareArray by returning { item: value }; remove that duplication by
changing the mapping for the 'aws-file-upload' key to use prepareArray instead
of prepareAwsFile and delete the now-unused prepareAwsFile function (or keep it
only if referenced elsewhere). Update any imports/exports if prepareAwsFile was
exported so there are no unused symbols remaining.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c6bfba4-2233-467e-809b-2cf43459ab2a
📒 Files selected for processing (2)
ui/app/src/components/FormsEngine/controls/AWSFileUpload.tsxui/app/src/components/FormsEngine/lib/valueSerializers.ts
✅ Files skipped from review due to trivial changes (1)
- ui/app/src/components/FormsEngine/controls/AWSFileUpload.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
craftercms/craftercms#7418
Summary by CodeRabbit
New Features
Improvements