Skip to content

fix: block submit while file upload is pending #701

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.

Sign up for GitHub

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

Open
k08200 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: block submit while file upload is pending #701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e16a37d
fix: block submit while file upload is pending
k08200 Apr 30, 2026
e1d062f
fix: mark uploaded file value as readonly
k08200 Apr 30, 2026
283fc0d
fix: block pending file upload submits
k08200 Apr 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions editor/components/extension/file-upload.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,7 @@ export const UploadedFileValue = forwardRef<
name={name}
required={required}
value={value}
readOnly
className={cn("sr-only", className)}
/>
</>
Expand Down
45 changes: 45 additions & 0 deletions editor/components/formfield/file-upload-field/__tests__/file-upload-validation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { describe, expect, test } from "vitest";
import { shouldBlockSubmitForFileUpload } from "../file-upload-validation";

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);
});
});
Comment on lines +4 to +45
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 30, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.

51 changes: 47 additions & 4 deletions editor/components/formfield/file-upload-field/file-upload-field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"use client";

import React, { useMemo, useState, useEffect } from "react";
import React, { useMemo, useState, useEffect, useRef } from "react";
import {
FileUploader,
FileUploaderTrigger,
Expand All @@ -16,6 +16,7 @@ import { DropzoneOptions } from "react-dropzone";
import { UploadStatus, useFileUploader } from "./use-file-uploader";
import type { FileUploaderFn } from "./uploader";
import Image from "next/image";
import { shouldBlockSubmitForFileUpload } from "./file-upload-validation";

type Accept = {
[key: string]: string[];
Expand Down Expand Up @@ -59,6 +60,8 @@ export const FileUploadField = ({
onFilesChange,
isMultipartFile,
}: FileUploadDropzoneProps) => {
const uploadedFileValueRef = useRef<HTMLInputElement>(null);

useEffect(() => {
if (isMultipartFile) return;
if (!uploader) {
Expand All @@ -69,7 +72,7 @@ export const FileUploadField = ({
}, [isMultipartFile, uploader]);

const [files, setFiles] = useState<File[]>([]);
const { getStatus, data } = useFileUploader({
const { getStatus, data, isUploading } = useFileUploader({
files,
uploader,
autoUpload: true,
Expand All @@ -94,6 +97,45 @@ export const FileUploadField = ({
() => data.map((info) => info.path).filter(Boolean) as string[],
[data]
);
const uploadedFilesValue = uploadedFilesPaths.join(",");

const shouldBlockSubmit = shouldBlockSubmitForFileUpload({
isMultipartFile,
selectedFilesCount: files.length,
uploadedFilesCount: uploadedFilesPaths.length,
isUploading,
});

const validationMessage = shouldBlockSubmit
? "Please wait for the selected file upload to finish."
: "";
Comment on lines +109 to +111
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 30, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

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.


useEffect(() => {
uploadedFileValueRef.current?.setCustomValidity(validationMessage);
}, [validationMessage]);

useEffect(() => {
const input = uploadedFileValueRef.current;
const form = input?.form;

if (!input || !form || isMultipartFile) return;

const handleSubmit = (event: SubmitEvent) => {
if (!shouldBlockSubmit) return;

input.setCustomValidity(validationMessage);
event.preventDefault();
event.stopPropagation();
event.stopImmediatePropagation();
input.reportValidity();
};

form.addEventListener("submit", handleSubmit, true);

return () => {
form.removeEventListener("submit", handleSubmit, true);
};
}, [isMultipartFile, shouldBlockSubmit, validationMessage]);

return (
<FileUploader
Expand All @@ -114,9 +156,10 @@ export const FileUploadField = ({
/>
{!isMultipartFile && (
<UploadedFileValue
ref={uploadedFileValueRef}
name={name}
required={required}
value={uploadedFilesPaths}
required={required || shouldBlockSubmit}
value={uploadedFilesValue}
/>
)}
</>
Expand Down
11 changes: 11 additions & 0 deletions editor/components/formfield/file-upload-field/file-upload-validation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export function shouldBlockSubmitForFileUpload(args: {
isMultipartFile?: boolean;
selectedFilesCount: number;
uploadedFilesCount: number;
isUploading: boolean;
}): boolean {
if (args.isMultipartFile) return false;
if (args.selectedFilesCount === 0) return false;

return args.isUploading || args.uploadedFilesCount < args.selectedFilesCount;
}
Loading