fix: block submit while file upload is pending#701
Open
k08200 wants to merge 3 commits intogridaco:mainfrom
Open
fix: block submit while file upload is pending#701k08200 wants to merge 3 commits intogridaco:mainfrom
k08200 wants to merge 3 commits intogridaco:mainfrom
Conversation
|
@k08200 is attempting to deploy a commit to the Grida Team on Vercel. A member of the Team first needs to authorize it. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
WalkthroughBlocks form submission while non-multipart file uploads are in progress by adding Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant FormComp as FileUploadField
participant Uploader as useFileUploader
participant Browser as Browser/FormValidation
User->>FormComp: Select files / Submit form
FormComp->>Uploader: request selectedFilesCount, uploadedFilesCount, isUploading, isMultipartFile
Uploader-->>FormComp: return upload state
FormComp->>FormComp: shouldBlock = shouldBlockSubmitForFileUpload(...)
alt shouldBlock == true
FormComp->>Browser: set custom validity on UploadedFileValue (prevent submit)
Browser-->>User: block submission / show validation
else shouldBlock == false
Browser-->>User: allow submission
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@editor/components/formfield/file-upload-field/__tests__/file-upload-validation.test.ts`:
- Around line 4-45: Add a new test in the
describe("shouldBlockSubmitForFileUpload") block that exercises the branch where
uploadedFilesCount < selectedFilesCount while isUploading is false: call
shouldBlockSubmitForFileUpload with selectedFilesCount: 2, uploadedFilesCount:
1, isUploading: false (and isMultipartFile omitted or false) and assert it
returns true so the helper blocks submit when uploads stopped but counts
mismatch; place it alongside the other tests for clarity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 479215b4-4a22-449a-95e6-bb3517ca46b3
📒 Files selected for processing (3)
editor/components/formfield/file-upload-field/__tests__/file-upload-validation.test.tseditor/components/formfield/file-upload-field/file-upload-field.tsxeditor/components/formfield/file-upload-field/file-upload-validation.ts
Comment on lines
+4
to
+45
| describe("shouldBlockSubmitForFileUpload", () => { | ||
| test("does not block an untouched optional upload field", () => { | ||
| expect( | ||
| shouldBlockSubmitForFileUpload({ | ||
| selectedFilesCount: 0, | ||
| uploadedFilesCount: 0, | ||
| isUploading: false, | ||
| }) | ||
| ).toBe(false); | ||
| }); | ||
|
|
||
| test("blocks when a selected file has not produced an uploaded path yet", () => { | ||
| expect( | ||
| shouldBlockSubmitForFileUpload({ | ||
| selectedFilesCount: 1, | ||
| uploadedFilesCount: 0, | ||
| isUploading: true, | ||
| }) | ||
| ).toBe(true); | ||
| }); | ||
|
|
||
| test("allows submit after each selected file has an uploaded path", () => { | ||
| expect( | ||
| shouldBlockSubmitForFileUpload({ | ||
| selectedFilesCount: 2, | ||
| uploadedFilesCount: 2, | ||
| isUploading: false, | ||
| }) | ||
| ).toBe(false); | ||
| }); | ||
|
|
||
| test("does not block multipart file uploads", () => { | ||
| expect( | ||
| shouldBlockSubmitForFileUpload({ | ||
| isMultipartFile: true, | ||
| selectedFilesCount: 1, | ||
| uploadedFilesCount: 0, | ||
| isUploading: true, | ||
| }) | ||
| ).toBe(false); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add a test for count mismatch when isUploading is false.
There’s no assertion for the uploadedFilesCount < selectedFilesCount branch when upload activity has stopped, which is part of the helper contract.
Proposed test addition
describe("shouldBlockSubmitForFileUpload", () => {
+ test("blocks when selected files still lack uploaded paths even if not actively uploading", () => {
+ expect(
+ shouldBlockSubmitForFileUpload({
+ selectedFilesCount: 2,
+ uploadedFilesCount: 1,
+ isUploading: false,
+ })
+ ).toBe(true);
+ });
+
test("does not block an untouched optional upload field", () => {
expect(
shouldBlockSubmitForFileUpload({
selectedFilesCount: 0,
uploadedFilesCount: 0,📝 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.
Suggested change
| describe("shouldBlockSubmitForFileUpload", () => { | |
| test("does not block an untouched optional upload field", () => { | |
| expect( | |
| shouldBlockSubmitForFileUpload({ | |
| selectedFilesCount: 0, | |
| uploadedFilesCount: 0, | |
| isUploading: false, | |
| }) | |
| ).toBe(false); | |
| }); | |
| test("blocks when a selected file has not produced an uploaded path yet", () => { | |
| expect( | |
| shouldBlockSubmitForFileUpload({ | |
| selectedFilesCount: 1, | |
| uploadedFilesCount: 0, | |
| isUploading: true, | |
| }) | |
| ).toBe(true); | |
| }); | |
| test("allows submit after each selected file has an uploaded path", () => { | |
| expect( | |
| shouldBlockSubmitForFileUpload({ | |
| selectedFilesCount: 2, | |
| uploadedFilesCount: 2, | |
| isUploading: false, | |
| }) | |
| ).toBe(false); | |
| }); | |
| test("does not block multipart file uploads", () => { | |
| expect( | |
| shouldBlockSubmitForFileUpload({ | |
| isMultipartFile: true, | |
| selectedFilesCount: 1, | |
| uploadedFilesCount: 0, | |
| isUploading: true, | |
| }) | |
| ).toBe(false); | |
| }); | |
| }); | |
| describe("shouldBlockSubmitForFileUpload", () => { | |
| test("blocks when selected files still lack uploaded paths even if not actively uploading", () => { | |
| expect( | |
| shouldBlockSubmitForFileUpload({ | |
| selectedFilesCount: 2, | |
| uploadedFilesCount: 1, | |
| isUploading: false, | |
| }) | |
| ).toBe(true); | |
| }); | |
| test("does not block an untouched optional upload field", () => { | |
| expect( | |
| shouldBlockSubmitForFileUpload({ | |
| selectedFilesCount: 0, | |
| uploadedFilesCount: 0, | |
| isUploading: false, | |
| }) | |
| ).toBe(false); | |
| }); | |
| test("blocks when a selected file has not produced an uploaded path yet", () => { | |
| expect( | |
| shouldBlockSubmitForFileUpload({ | |
| selectedFilesCount: 1, | |
| uploadedFilesCount: 0, | |
| isUploading: true, | |
| }) | |
| ).toBe(true); | |
| }); | |
| test("allows submit after each selected file has an uploaded path", () => { | |
| expect( | |
| shouldBlockSubmitForFileUpload({ | |
| selectedFilesCount: 2, | |
| uploadedFilesCount: 2, | |
| isUploading: false, | |
| }) | |
| ).toBe(false); | |
| }); | |
| test("does not block multipart file uploads", () => { | |
| expect( | |
| shouldBlockSubmitForFileUpload({ | |
| isMultipartFile: true, | |
| selectedFilesCount: 1, | |
| uploadedFilesCount: 0, | |
| isUploading: true, | |
| }) | |
| ).toBe(false); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@editor/components/formfield/file-upload-field/__tests__/file-upload-validation.test.ts`
around lines 4 - 45, Add a new test in the
describe("shouldBlockSubmitForFileUpload") block that exercises the branch where
uploadedFilesCount < selectedFilesCount while isUploading is false: call
shouldBlockSubmitForFileUpload with selectedFilesCount: 2, uploadedFilesCount:
1, isUploading: false (and isMultipartFile omitted or false) and assert it
returns true so the helper blocks submit when uploads stopped but counts
mismatch; place it alongside the other tests for clarity.
Contributor
Author
|
Behavior verification update: I ran a temporary Next dev route with Playwright in Chromium. For a non-multipart |
Contributor
Author
|
Follow-up: added |
Contributor
Author
2026-05-01.1.01.31.movIs it right?? |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@editor/components/formfield/file-upload-field/file-upload-field.tsx`:
- Around line 109-111: The validation text currently uses shouldBlockSubmit
only, causing the message to always read "Please wait…" even when uploads
failed; update the logic that sets validationMessage (in file-upload-field
component where validationMessage, shouldBlockSubmit and isUploading are
defined) to branch on isUploading: when shouldBlockSubmit && isUploading keep
the existing "Please wait for the selected file upload to finish." otherwise
when shouldBlockSubmit && !isUploading show a clear recovery message (e.g.,
"File upload incomplete or failed. Please retry or remove the file.") so users
aren’t misled; ensure you reference and use the existing isUploading flag and
preserve existing empty-string behavior when shouldBlockSubmit is false.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e90062e-6d39-40fc-ab43-b8b21b2c4fef
📒 Files selected for processing (1)
editor/components/formfield/file-upload-field/file-upload-field.tsx
Comment on lines
+109
to
+111
| const validationMessage = shouldBlockSubmit | ||
| ? "Please wait for the selected file upload to finish." | ||
| : ""; |
There was a problem hiding this comment.
Validation message is inaccurate when upload is not in progress
shouldBlockSubmit is also true when uploads are incomplete due to non-uploading states (e.g., failed upload), but the message always says “Please wait…”. This can mislead users about recovery steps. Consider branching the message by isUploading vs mismatch-only states.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/components/formfield/file-upload-field/file-upload-field.tsx` around
lines 109 - 111, The validation text currently uses shouldBlockSubmit only,
causing the message to always read "Please wait…" even when uploads failed;
update the logic that sets validationMessage (in file-upload-field component
where validationMessage, shouldBlockSubmit and isUploading are defined) to
branch on isUploading: when shouldBlockSubmit && isUploading keep the existing
"Please wait for the selected file upload to finish." otherwise when
shouldBlockSubmit && !isUploading show a clear recovery message (e.g., "File
upload incomplete or failed. Please retry or remove the file.") so users aren’t
misled; ensure you reference and use the existing isUploading flag and preserve
existing empty-string behavior when shouldBlockSubmit is false.
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.
Summary
Verification
Note: pnpm --dir editor typecheck currently fails on unrelated missing static image modules under app/(www), app/(site), app/(workbench), and components/stripe.
Closes #137
Summary by CodeRabbit
Bug Fixes
Improvements
Tests