Skip to content

feat(upload/transformFiles): remove unnecessary validation - #1058

Merged
davedbase merged 4 commits into
solidjs-community:mainfrom
an-jello:upload/transform-file-remove-uneeded-checks
Nov 11, 2026
Merged

feat(upload/transformFiles): remove unnecessary validation#1058
davedbase merged 4 commits into
solidjs-community:mainfrom
an-jello:upload/transform-file-remove-uneeded-checks

Conversation

@an-jello

@an-jello an-jello commented Sep 10, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

The current transformFiles function does an unnecessary check for a valid file index number / file existing in an index. This PR removes it.

I've consulted the W3C reference:

  • Indices of FileList "are the numbers in the range zero to one less than the number of File objects"

Summary by CodeRabbit

  • Bug Fixes
    • Improved drag-and-drop state tracking so the dragging indicator updates correctly when entering, leaving, or dropping files.
    • Improved file processing for dropped or selected files, including safer handling of empty input.
  • API Changes
    • Removed the onDragStart, onDragEnd, and onDrag dropzone callbacks.
    • Continued support for onDragEnter, onDragLeave, and onDragOver callbacks.

@changeset-bot

changeset-bot Bot commented Sep 10, 2026 •
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 46ede36

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@solid-primitives/upload Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change StackReview Change Stack

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 83196ee3-9d50-4bc0-890f-853357ee1be8
Commits

Reviewing files that changed from the base of the PR and between 1137924 and 46ede36.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review

No actionable comments were generated in the recent review.

Walkthrough

The upload package updates createDropzone drag-state handling, removes obsolete drag callbacks, simplifies transformFiles, and adds release changesets.

Changes: Upload behavior

Layer / File(s) Summary
Dropzone drag state and callback contract
packages/upload/src/types.ts, packages/upload/src/createDropzone.ts, .changeset/afraid-mails-cry.md		 createDropzone now updates isDragging on drag enter, drag leave, and drop. It removes dragstart, dragend, and drag listeners and callbacks.
FileList transformation
packages/upload/src/helpers.ts, .changeset/fine-kids-grin.md		 transformFiles returns early for null input and iterates FileList indices without numeric validation or missing-file checks.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: High · previous review 04c31

File uploads may fail when FileList indexing yields an absent entry because the new transformation path can pass it to URL.createObjectURL. This should be resolved before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (2 skipped: 2… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)

Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the primary change: removing unnecessary validation from transformFiles.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (2 skipped: 2 unsupported.)

Finishing Touches

🧪 Generate unit tests (beta)

  • Create PR with unit tests
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 1d819740-49ce-42b4-a3d3-bbce17acdeaa

📥 Commits

Reviewing files that changed from the base of the PR and between c7b608c and c1e461a.

📒 Files selected for processing (5)

  • .changeset/afraid-mails-cry.md
  • .changeset/fine-kids-grin.md
  • packages/upload/src/createDropzone.ts
  • packages/upload/src/helpers.ts
  • packages/upload/src/types.ts
💤 Files with no reviewable changes (1)

  • packages/upload/src/types.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

@an-jello
an-jello force-pushed the upload/transform-file-remove-uneeded-checks branch from c1e461a to 04c31ee Compare September 10, 2026 15:40
@an-jello

an-jello commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Whoops, was attempting to use PR stacking but failed :/

Rebased the changes above to be on top of main again

@davedbase

davedbase commented Nov 6, 2026

Copy link
Copy Markdown
Member

@an-jello would you mind applying the same fix to 2.0?

@an-jello

an-jello commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

@davedbase Holy crap this PR was made a long time ago.

I haven't had the chance to play with Solid 2.0 so I'm not comfortable per-se to port this PR to it. Though, looking at the change, it doesn't look like the change would break anything... (🤞)

@davedbase
davedbase merged commit f4c4e24 into solidjs-community:main Nov 11, 2026
3 checks passed
@davedbase davedbase mentioned this pull request Nov 11, 2026
Merged
davedbase added a commit that referenced this pull request Nov 11, 2026
@davedbase @royeden @claude
fix(upload, raf): fix transformFiles crash and #824 follow-ups
9864461 
upload: transformFiles iterated the FileList with for...in, which also
visits its `length` and `item` members, so it threw on every call
(regression from #1058, staged but unpublished as 0.1.6). Use
Array.from(files, ...) and add tests.

raf (follow-up to #824, unreleased):
- useGlobalRAF registers a separate entry per call, so two owners
  registering the same callback no longer unregister each other
- remove() only stops the shared loop when it removed the last callback
- createCallbacksSet's dispatcher is typed (...args: Parameters<T>) => void
- document that createScheduledLoop's scheduler must repeat the callback
- fix tests whose assertions ran after the test ended, dispose leaked
  roots, add regression tests
- add the new primitives to primitive.list and add a changeset

Co-authored-by: Roy Eden <roy.ede@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@an-jello @davedbase