Skip to content

fix(web): keep folder drops out of attachment uploads - #12019

Closed
StiensWout wants to merge 1 commit into
pingdotgg:mainfrom
StiensWout:t3code/fix-folder-drop-upload
Closed

StiensWout wants to merge 1 commit into
pingdotgg:mainfrom
StiensWout:t3code/fix-folder-drop-upload

Conversation

@StiensWout

@StiensWout StiensWout commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Dropping a folder creates a failed attachment and blocks Send. Filter directory entries in the shared workspace drop handler before they reach the uploader. Mixed drops still attach real files, and a folder drop explains how to reference an accessible path.

Fixes #11961.

The same handler covers chat, sidebar thread rows, sidebar search results, and the legacy sidebar on web and desktop. Files without MIME types and drops without entry metadata keep working.

Verification

  • Nine focused drop-handler tests pass. Three new cases failed against the original behavior before the fix.
  • Web typecheck, targeted lint, formatting, and git diff --check pass.
  • Chromium 154, real filesystem folder and text file dropped through Input.dispatchDragEvent, against an isolated T3 server:
Drop Before After
Folder + text file Directory upload fails with ERR_FILE_NOT_FOUND; Send disabled Only the text file uploads, HTTP 204; no failed attachment; Send enabled
Folder only, with an existing draft Creates a failed attachment Zero upload requests; composer contents unchanged; Send enabled

Browser inspection confirmed isDirectory: true for the folder and false for the text file. Desktop uses the same handler; a packaged desktop build was not separately exercised.

Before

Folder becomes a failed attachment and disables Send

After

Only the text file is attached, the folder limitation is explained, and Send remains enabled

Prepared for Wout by gpt-6 in Codex through T3 Code.

Summary by CodeRabbit

  • Bug Fixes
    • Improved drag-and-drop file handling in chat workspaces.
    • Folders are now rejected with an informational notification instead of being attached.
    • Mixed drops attach only valid files, including files without a MIME type.
    • Files remain supported when drag metadata is unavailable.
    • Rejected folders are no longer recovered through fallback file data.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Sep 16, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at cdc4657

Macroscope's review found this PR approvable — This is a focused bug fix that prevents folder entries from reaching attachment uploads while preserving normal and mixed file drops. The shared handler change is covered by targeted tests and introduces no schema, security, deployment, default-setting, or static-analysis configuration changes.

You can add or adjust custom eligibility rules. Learn more.

@macroscopeapp macroscopeapp Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All clear

Posted via Macroscope — Effect Service Conventions

@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: ed5389e0-6deb-4c0e-81bb-b7259354088b

📥 Commits

Reviewing files that changed from the base of the PR and between 53612cc and cdc4657.

📒 Files selected for processing (2)
  • apps/web/src/components/chat/workspaceFileDrop.test.ts
  • apps/web/src/components/chat/workspaceFileDrop.ts

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


📝 Walkthrough

Walkthrough

The workspace drop handler now inspects drag items, rejects folders, preserves valid files, and reports skipped folders. Tests cover mixed drops, MIME-less files, missing entry metadata, and fallback behavior.

Changes

Workspace Drop Filtering

Layer / File(s) Summary
Drop item contract
apps/web/src/components/chat/workspaceFileDrop.ts
WorkspaceFileDragEvent now optionally exposes drag items with file retrieval and directory metadata.
Drop filtering and validation
apps/web/src/components/chat/workspaceFileDrop.ts, apps/web/src/components/chat/workspaceFileDrop.test.ts
onDrop skips directory entries, collects valid files, shows an informational toast when folders are skipped, and avoids adding an empty file collection. Tests cover mixed drops, MIME-less files, missing entry metadata, and fallback behavior.

Priority: ⬇️ Low

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

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: ⚪ Minimal · up to cdc46

The folder filtering behavior preserves valid file drops and does not reattach rejected folders through the fallback path. No actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: preventing folder drops from becoming attachment uploads.
Description check ✅ Passed The description explains what changed, why it changed, affected surfaces, verification results, and UI behavior with before-and-after screenshots. It does not include the template's Checklist section,…
Linked Issues check ✅ Passed The PR addresses the coding requirements in #11961. workspaceFileDrop.ts reads dataTransfer.items, skips entries whose webkitGetAsEntry().isDirectory is true, and does not call addFiles for fo…
Out of Scope Changes check ✅ Passed The changes are limited to the shared workspace drop handler and its automated tests. The toast, filtering logic, fallback behavior, and test updates directly support #11961. No unrelated product beha…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@StiensWout

Copy link
Copy Markdown
Contributor Author

Reviewed the non-blocking docstring-coverage warning. The changed handler has a nearby comment explaining why directory metadata must be read synchronously during the drop event. Extra docstrings for this existing handler and its test helper would repeat the code; the repository's guidance favors comments that explain behavior or constraints. No code change is needed for that warning.

gpt-6 via Codex through T3 Code, responding for Wout.

@juliusmarminge

Copy link
Copy Markdown
Member

Superseded by #12001, which already fixed #11961 and merged to main. Closing this PR as leftover hygiene.

@juliusmarminge

Copy link
Copy Markdown
Member

Superseded by #12001, which already landed the folder-drop fix for #11961 (path chips on local, refused on remote). Closing this PR as superseded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Dropped folders become failed file uploads

2 participants