-
Notifications
You must be signed in to change notification settings - Fork 123
handle file upload async and sync render modes #4126
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughIn sync mode, value inputs are now type-checked: actual File objects (or objects with uid/name/size) are converted to full IStoredFile and dispatched; non-file values no longer immediately dispatch and instead trigger a fileFetcher.refetch when a newFileId exists. Async fetch response handling was broadened to run when not fetching. Changes
Sequence DiagramsequenceDiagram
participant E as Effect Hook
participant V as value (prop)
participant C as isFileObject check
participant B as Build & Dispatch
participant R as fileFetcher.refetch
E->>V: value change
E->>C: check File or uid/name/size?
alt is file-like
C->>B: build IStoredFile (uid,url,status,name,size,type,originFileObj)
B->>E: dispatch fetchFileInfoSuccessAction
else not file-like
C->>R: if newFileId present
alt newFileId present
R->>R: refetch({ queryParams: { id: newFileId } })
else no newFileId
R-->>E: no action
end
end
rect #f8f9fa
Note over E,R: Async fetch responses now processed when not fetching (covers async mode and manual sync refetches)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)shesha-reactjs/src/providers/storedFile/index.tsx (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shesha-reactjs/src/providers/storedFile/index.tsx (1)
146-154: Consider includingidin the constructedIStoredFilefor consistency.The async mode (lines 174-182) includes the
idproperty when constructingIStoredFile, but the sync mode omits it. IfnewFileIdis available (which could be fromvalue?.id), consider including it in the constructed object for consistency between modes.if (isFileObject) { const fileInfo: IStoredFile = { + id: newFileId || null, uid: value.uid || `sync-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`, url: null, status: 'done', name: value.name, size: value.size, type: value.type, - originFileObj: null, + originFileObj: value instanceof File ? value : null, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shesha-reactjs/src/providers/storedFile/index.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
shesha-reactjs/src/providers/storedFile/index.tsx (1)
shesha-reactjs/src/providers/storedFiles/contexts.ts (1)
IStoredFile(26-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-attempt
|
|
||
| if (isFileObject) { | ||
| const fileInfo: IStoredFile = { | ||
| uid: value.uid, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: uid will be undefined for File instances.
When value is a File instance, value.uid will be undefined because the native File API does not include a uid property. Since IStoredFile extends UploadFile (from antd), the uid is typically required for tracking files in the upload component. This will likely cause runtime issues with file display and tracking.
Generate a unique uid for File instances:
if (isFileObject) {
const fileInfo: IStoredFile = {
- uid: value.uid,
+ uid: value.uid || `sync-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`,
url: null,
status: 'done',
name: value.name,
size: value.size,
type: value.type,
originFileObj: null,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uid: value.uid, | |
| uid: value.uid || `sync-${Date.now()}-${Math.random().toString(36).substr(2, 9)}`, |
🤖 Prompt for AI Agents
In shesha-reactjs/src/providers/storedFile/index.tsx around line 147, the code
assigns uid: value.uid which will be undefined for native File instances; detect
when value.uid is falsy and generate a stable unique uid (for example
Date.now().toString() + Math.random() or a UUID) and assign that uid to the
IStoredFile object so UploadFile consumers always have a valid uid; ensure the
generated uid is a string and used consistently for tracking/display in the
Upload component.
| name: value.name, | ||
| size: value.size, | ||
| type: value.type, | ||
| originFileObj: null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
originFileObj should reference the actual File object.
When value is a File instance, originFileObj should preserve a reference to it rather than being set to null. This property is used by upload components to access the original file data.
- originFileObj: null,
+ originFileObj: value instanceof File ? value : null,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| originFileObj: null, | |
| originFileObj: value instanceof File ? value : null, |
🤖 Prompt for AI Agents
In shesha-reactjs/src/providers/storedFile/index.tsx around line 153,
originFileObj is being set to null but should reference the actual File when
value is a File instance; change the assignment so that originFileObj = value
when value instanceof File (otherwise keep null) so upload components can access
the original File object.
Summary by CodeRabbit