Skip to content

[WC-3361] FileUploader: enhance limits and feedback#2208

Open
yordan-st wants to merge 11 commits into
mainfrom
fix/WC-3361_fileuploader-limit-feedback
Open

[WC-3361] FileUploader: enhance limits and feedback#2208
yordan-st wants to merge 11 commits into
mainfrom
fix/WC-3361_fileuploader-limit-feedback

Conversation

@yordan-st
Copy link
Copy Markdown
Contributor

@yordan-st yordan-st commented May 11, 2026

Pull request type

Bug fix + Feature (behavior change — non-breaking)

Description

When the file upload limit was reached, the dropzone silently turned grey with no explanation. Dropping more files than the limit rejected the entire batch. There was no proper upload queue — excess files were marked as errors and "retried" via a promotion mechanism, conflating queueing with error state.

This PR addresses:

  • Silent disabled dropzone → now shows "Maximum file count of X reached."
  • Drop overflow now splits on remaining capacity. Only excess files are rejected; the rest upload normally.
  • Replaced the error/retry approach with a proper upload queue. Files waiting for a concurrent slot show a "Waiting..." state, not an error state.
  • New "Maximum concurrent uploads" property (XML key: maxFilesPerBatch) controls how many files upload simultaneously. Excess files queue and drain automatically.
  • New "Upload queued" text property for the waiting state message.
  • New "File limit reached" text property for the limit-reached message.
  • Rejected files (over total limit) auto-promote oldest-first when capacity opens.
  • Upload errors free both a concurrent slot and a total-files slot, keeping the queue draining.
  • maxFilesPerUpload property is now optional — empty or 0 means unlimited.
  • File list reordered: successful uploads shown above rejected files.

No longer depends on WC-3363. The dismissValidationErrors method was removed as part of this PR — rejected files are a distinct state from validation errors and are not dismissable. WC-3363 can merge independently.

What should be covered while testing?

Setup: File Uploader widget, files mode. Start with "Maximum number of files" = 5, "Maximum concurrent uploads" = 2.

File limit feedback:

  1. Upload 1 file → dropzone stays active, no warning shown
  2. Upload until total = 5 → dropzone turns grey AND message appears: "Maximum file count of 5 reached."
  3. Remove 1 file while at limit → dropzone re-enables, message disappears

Unlimited behavior:
5. Set total limit = 0 → dropzone never disables regardless of file count
6. Leave total limit empty → same as 0, unlimited

Concurrent upload queue:
7. Set concurrent uploads = 2, drop 5 files → exactly 2 show "Uploading..." with progress bar, remaining 3 show "Waiting..." without progress bar
8. Wait for 1 upload to complete → next queued file automatically starts uploading (now shows "Uploading...")
9. Leave concurrent uploads empty → all dropped files start uploading simultaneously

Total limit with auto-promotion:
10. Set total limit = 3, concurrent uploads = unlimited. Drop 5 files → 3 upload, 2 show as rejected with "Maximum file count of 3 reached."
11. Remove 1 uploaded file → oldest rejected file automatically starts uploading
12. Let an upload fail → that file shows error; oldest rejected file automatically starts uploading

List ordering:
13. Upload a mix of valid and invalid files → successful uploads appear above rejected files

Other:
14. Read-only mode → dropzone not rendered, no regression
15. Image upload mode → same queue and limit behavior applies

@yordan-st yordan-st force-pushed the fix/WC-3361_fileuploader-limit-feedback branch from 430b80b to 4d8b53f Compare May 11, 2026 15:02
@yordan-st yordan-st marked this pull request as ready for review May 12, 2026 13:00
@yordan-st yordan-st requested a review from a team as a code owner May 12, 2026 13:00
@yordan-st yordan-st force-pushed the fix/WC-3361_fileuploader-limit-feedback branch from a444b83 to 7d054a3 Compare May 12, 2026 13:01
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

@yordan-st yordan-st force-pushed the fix/WC-3361_fileuploader-limit-feedback branch from 9fe41dd to 09489e2 Compare May 12, 2026 13:45
@github-actions

This comment was marked as outdated.

@github-actions

This comment was marked as outdated.

Comment thread packages/pluggableWidgets/file-uploader-web/src/stores/FileUploaderStore.ts Outdated
@yordan-st yordan-st force-pushed the fix/WC-3361_fileuploader-limit-feedback branch from d06abf7 to 8fe5f51 Compare May 18, 2026 12:57
@github-actions
Copy link
Copy Markdown

AI Code Review

🔶 Changes requested — one or more medium-severity items must be addressed


What was reviewed

File Change
src/stores/FileUploaderStore.ts New maxFilesPerBatch, sortedFiles, warningMessage, retryLimitExceededFiles, MobX reaction for auto-retry
src/stores/FileStore.ts New errorType field; reset() action; markError now accepts error type
src/stores/__tests__/FileUploaderStore.spec.ts New test file — 253 lines covering new store behaviours
src/FileUploader.editorConfig.ts Import reorder only
src/FileUploader.editorPreview.tsx Import reorder only
src/FileUploader.xml maxFilesPerUpload made optional; new maxFilesPerBatch, uploadLimitReachedMessage, uploadBatchLimitExceededMessage
src/components/Dropzone.tsx maxFilesPerUpload prop removed (limit enforcement moved to store)
src/components/FileUploaderRoot.tsx Uses rootStore.sortedFiles and rootStore.warningMessage
src/utils/useRootStore.ts Adds dispose() cleanup on unmount
typings/FileUploaderProps.d.ts maxFilesPerUpload optional; maxFilesPerBatch, uploadLimitReachedMessage, uploadBatchLimitExceededMessage added
CHANGELOG.md Unreleased section updated

Skipped (out of scope): dist/, pnpm-lock.yaml

⚠️ CI check results could not be retrieved automatically — verify checks are green before merging.


Findings

🔶 Medium — maxFilesPerBatch not hidden in read-only mode in editorConfig

File: src/FileUploader.editorConfig.ts lines 22–26
Problem: When readOnlyMode is true, the editor config hides maxFilesPerUpload, maxFileSize, and objectCreationTimeout — all upload-time properties. The new maxFilesPerBatch property is upload-time too but is absent from this list, so Studio Pro will show it to users configuring a read-only widget, creating a confusing no-op setting.
Fix:

hidePropertiesIn(properties, values, [
    values.uploadMode === "files" ? "associatedImages" : "associatedFiles",
    "createImageAction",
    "createFileAction",
    "allowedFileFormats",
    "maxFilesPerUpload",
    "maxFilesPerBatch",   // ← add this
    "maxFileSize",
    "objectCreationTimeout"
]);

⚠️ Low — Tests push plain objects into store.files instead of using FileStore factories

File: src/stores/__tests__/FileUploaderStore.spec.ts lines 77–80, 96–99, 113–116, 132–138
Note: These tests cast raw objects ({ fileStatus: "existingFile" } as any) and push them into the MobX observable array. This bypasses the FileStore constructor and makeObservable call, so the pushed items are plain non-observable objects. It works for current assertions (count-based filtering), but breaks down for any future test that needs fileStatus changes on those objects to be reactive, and is fragile against FileStore structural refactors. Using the existing static factories is straightforward:

// instead of:
store.files.push({ fileStatus: "existingFile", _objectItem: obj("a") } as any);

// use:
const fileA = FileStore.existingFile(obj("a"), store);
store.files.push(fileA);

FileStore.existingFile calls fetchMxObject asynchronously — stub mx-data at the top of the test file if needed (as the DatasourceUpdateProcessor.spec.ts already does for the same module).


Positives

  • dispose() method added to FileUploaderStore and wired up in useRootStore via a cleanup useEffect — the MobX reaction no longer leaks on widget unmount.
  • New errorType discriminant on FileStore is properly registered as observable in makeObservable and correctly updated in all three creation paths (newFileWithError, markError, and the constructor).
  • Moving limit enforcement from react-dropzone's maxFiles option into the store is the right call — it eliminates the silent all-or-nothing rejection and gives fine-grained control over which files pass vs. error.
  • retryLimitExceededFiles is registered as a MobX action, so all state mutations inside it are batched and the retry reaction won't cascade.
  • CHANGELOG entry is thorough and covers fixed, added, and changed categories with user-facing language.
  • New XML properties use lowerCamelCase keys (maxFilesPerBatch, uploadLimitReachedMessage, uploadBatchLimitExceededMessage) consistent with the repo convention, and TypeScript typings are kept in sync.

yordan-st added 11 commits May 20, 2026 15:46
- Show "Maximum file count of X reached" message when dropzone is disabled
- Make maxFilesPerUpload optional (empty/0 = unlimited)
- Add maxFilesPerBatch property to cap server commits per drop event
- Split drops by remaining capacity — excess files shown as errors, not silently rejected
- Auto-retry limit/batch-exceeded files when capacity is freed
- Batch/limit-exceeded files survive dismissValidationErrors and re-queue correctly
- Retry order matches visual list order (newest first)
- Reorder file list: accepted files above rejected files
- Add "queued" and "rejected" statuses; remove "new", "removedAfterError"
- Remove errorType field and dismissValidationErrors
- maxFilesPerBatch (XML) → maxConcurrentUploads (TS)
- maxFilesPerUpload (XML) → maxTotalFiles (TS)
- processDrop is pure classifier; two reactions drain the queue
- Add uploadQueuedMessage XML property
- Remove uploadBatchLimitExceededMessage XML property
@yordan-st yordan-st force-pushed the fix/WC-3361_fileuploader-limit-feedback branch from 3d36ddc to 160f8a7 Compare May 20, 2026 13:46
@github-actions
Copy link
Copy Markdown

AI Code Review

⚠️ Approved with suggestions — low-severity items only, safe to merge


What was reviewed

File Change
src/stores/FileUploaderStore.ts New upload queue, concurrent-limit reactions, promotion logic
src/stores/FileStore.ts Status enum refactor (newqueued, removed removedAfterError, added rejected)
src/stores/TranslationsStore.ts Import order only
src/stores/__tests__/FileUploaderStore.spec.ts 814 lines of new unit tests
src/components/FileUploaderRoot.tsx Warning message logic moved from store to component
src/components/Dropzone.tsx Removed maxFilesPerUpload prop and maxFiles from useDropzone
src/components/FileEntry.tsx Removed removedAfterError from CSS class condition
src/components/UploadInfo.tsx New queued and rejected display cases
src/components/ActionButton.tsx Import order only
src/components/ActionsBar.tsx Import order only
src/utils/useRootStore.ts Added dispose() cleanup effect
src/utils/useTranslationsStore.tsx Import order only
src/utils/mx-data.ts Import order only
src/utils/parseAllowedFormats.ts Import order only
src/utils/prepareAcceptForDropzone.ts Import order only
src/FileUploader.xml New maxFilesPerBatch, uploadQueuedMessage, uploadLimitReachedMessage properties
src/FileUploader.editorConfig.ts Hide maxFilesPerBatch in image mode
src/FileUploader.editorPreview.tsx Import order only
typings/FileUploaderProps.d.ts maxFilesPerUpload → optional; new maxFilesPerBatch, uploadQueuedMessage, uploadLimitReachedMessage
CHANGELOG.md Fixed/Added/Changed entries for all behaviour changes
src/utils/__tests__/DatasourceUpdateProcessor.spec.ts Import order only
src/utils/__tests__/parseAllowedFormats.spec.ts Import order only

Skipped (out of scope): dist/, pnpm-lock.yaml

CI checks: could not fetch (tool approval required) — reviewer should confirm green before merge.


Findings

⚠️ Low — sortedFiles only moves validationError to end, not rejected

File: src/stores/FileUploaderStore.ts line ~564–570
Problem: The CHANGELOG and PR description state "successful uploads shown above rejected files." The sortedFiles computed only pushes validationError files (size/type rejection) to the end; files with "rejected" status (over the total cap) are not moved. For a single drop, the natural unshift order incidentally puts capacity-excess files below the accepted ones, but across multiple drops, rejected-cap files can appear interleaved with successful ones.
Fix: Extend the sort comparator to treat both validationError and rejected as trailing:

get sortedFiles(): FileStore[] {
    const isTrailing = (s: FileStatus) =>
        s === "validationError" || s === "rejected" ? 1 : 0;
    return [...this.files].sort((a, b) => isTrailing(a.fileStatus) - isTrailing(b.fileStatus));
}

Also add a matching test for rejected files sorting to the end.


⚠️ Low — Test stores never disposed; MobX reactions outlive tests

File: src/stores/__tests__/FileUploaderStore.spec.ts
Note: Every buildStore() call registers two MobX reactions that are never cleaned up because store.dispose() is never called. In practice this is harmless (stores are independent) but in a future test that mutates a "dead" store's observables the lingering reactions could fire unexpectedly. An afterEach dispose pattern would prevent this:

let store: FileUploaderStore;

beforeEach(() => { store = buildStore(); });
afterEach(() => store.dispose());

⚠️ Low — Direct fileStatus mutation in tests bypasses MobX actions

File: src/stores/__tests__/FileUploaderStore.spec.ts lines ~1232, ~1311
Note: Several tests mutate store.files[...].fileStatus = "done" as any directly outside an action. This works today because enforceActions is not set to "always", but it will throw if the jest config is ever tightened. Replace with the existing setQueued() / markMissing() action methods where possible, or wrap in runInAction(() => { ... }).


Positives

  • Replacing the error-state/retry promotion hack with proper "queued" / "rejected" state machine is a clean architectural improvement — state transitions are now unambiguous.
  • Using MobX reaction (not autorun) with explicit prevCount comparison for the promotion triggers is the right choice; it avoids firing on every render and provides a natural FIFO drain.
  • dispose() pattern on the store + useEffect cleanup in useRootStore is correctly wired — no reaction leak on widget unmount.
  • Test coverage is thorough: 814 new lines covering all status transitions, concurrent-limit scenarios, promotion ordering, and disposal. The end-to-end queue integration tests (upload error → slot freed → queued file promoted) are particularly valuable.
  • capacityFiles / capacityExcess split in processDrop correctly handles drops where only a subset fits — the whole batch is no longer rejected when the limit is partially exceeded.
  • XML required="false" on both new expression properties, with correct Integer return types and bilingual translations, follows the established widget convention.

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