Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRemote import size thresholds now derive from configured megabyte limits and produce specific per-file or total-limit errors, with updated coverage. File import failure notifications now use a shared formatter that localizes failure counts and deduplicates non-empty reasons before displaying toast descriptions. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. 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
🧹 Nitpick comments (1)
src/renderer/src/components/right-sidebar/file-explorer-import-feedback.test.ts (1)
14-24: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover trim-aware deduplication.
Add a reason such as
' Permission denied 'alongside'Permission denied'; the current test does not verify that trimming also prevents whitespace-only duplicates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 261e01a0-749e-40ab-a3ff-ee3ad2469b11
📥 Commits
Reviewing files that changed from the base of the PR and between 300ee19 and 8a11fbb48ce66058722b86b7685d955c8532ddc4.
📒 Files selected for processing (5)
src/main/ipc/filesystem-import.test.tssrc/main/ipc/filesystem-mutations.tssrc/renderer/src/components/right-sidebar/file-explorer-import-feedback.test.tssrc/renderer/src/components/right-sidebar/file-explorer-import-feedback.tssrc/renderer/src/components/right-sidebar/useFileExplorerImport.ts
8a11fbb to
c16443a
Compare
|
Taking a look! |
Sonner renders string descriptions with collapsed whitespace, so the newline-joined reasons showed as run-on text; pre-line restores the per-line format. Cap distinct reasons at 5 with an ellipsis line so path-bearing reasons (per-file EACCES) cannot fill the screen.
Top-level dropped files staged with relativePath '' rendered failure
reasons with an empty or generic label now that reasons reach the toast
('File exceeds...', "File changed during upload staging: ''"). Thread
the source basename as a display-only name so every staging message
names the real file while the staged entry keeps its '' relativePath;
same fallback for the directory root's escape/type messages.
Collapse interior whitespace per reason so a newline-bearing filename cannot defeat the line cap or forge an extra toast line; truncate only when the ellipsis actually saves a line; keep the multi-line description up for 60s (sonner's 4s default is unreadable for path-bearing reasons, matching the store-slice error toast precedent); pin the cap boundary.
|
Thanks @jae-heo — clean, well-scoped fix; correct root cause (the reason was already carried through the import results and only dropped at the toast seam). Verified end-to-end against a real SSH workspace, and pushed small follow-up commits on top:
Reproduction (before → after) Before (base): a failed drag-import into a read-only remote directory shows only the count — no way to tell why:
After (head): the toast surfaces the actionable reason:
Multi-file failures de-duplicate repeated reasons into one line:
Validation
|
Greptile SummaryThis PR surfaces actionable backend failure reasons in the file-explorer import error toast, which previously only showed a file count. It also improves size-limit error copy, fixes empty file labels in error messages for top-level dropped files and filesystem-root paths, and adds a
Confidence Score: 5/5Safe to merge — the change is narrowly scoped to error message copy and toast formatting with no routing, IPC protocol, security boundary, or data-path logic modified. The main-process changes carefully separate the display-only displayName path from the relativePath serialised into the staged-entry protocol. The descriptionClassName prop is confirmed valid in sonner 2.0.7. Deduplication and cap logic are fully covered by new unit tests including the exact truncation boundary. Regression tests verify oversized files are rejected before open() is called. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/renderer/src/components/right-sidebar/file-explorer-import-feedback.ts | New helper that formats the failure toast: deduplicates and normalises reasons, applies a 5-reason cap with ellipsis, and omits the description entirely when all reasons are blank. |
| src/renderer/src/components/right-sidebar/useFileExplorerImport.ts | Replaces inline toast construction with getFileExplorerImportFailureToast; adds descriptionClassName and 60s duration. Both are valid ExternalToast fields in sonner 2.0.7. |
| src/main/ipc/filesystem-mutations.ts | Introduces displayName option, separates entryRelativePath from displayPath, and updates assertRemoteUploadBudget with explicit MB figures. No change to security checks or protocol. |
| src/renderer/src/components/right-sidebar/file-explorer-import-feedback.test.ts | New test file covering single failure, newline collapsing, deduplication, empty description, dedup-before-cap, cap with ellipsis, and the truncation boundary. |
| src/main/ipc/filesystem-import.test.ts | Adds regression tests for 80 MB oversized file, filesystem-root display label, and updated expected messages for aggregate-size and path-escape failures. |
Reviews (2): Last reviewed commit: "fix(file-explorer): label filesystem-roo..." | Re-trigger Greptile
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d113ca3d-adeb-411f-97bc-92273e4112ca
📒 Files selected for processing (5)
src/main/ipc/filesystem-import.test.tssrc/main/ipc/filesystem-mutations.tssrc/renderer/src/components/right-sidebar/file-explorer-import-feedback.test.tssrc/renderer/src/components/right-sidebar/file-explorer-import-feedback.tssrc/renderer/src/components/right-sidebar/useFileExplorerImport.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/ipc/filesystem-import.test.ts
- src/renderer/src/components/right-sidebar/useFileExplorerImport.ts
|
Thanks for the thorough validation and follow-up improvements! The 60-second duration makes sense to me for multi-line, path-bearing error messages, so let's keep it. |
Electron validation — exact PR head 32d9488
|
|
@brennanb2025 Thanks again for the review and follow-up commits. This PR now conflicts with Before I do that work, could you confirm whether this fix is still wanted and whether you would like me to refresh the PR? If so, I will rebase it, port the changes to the new structure, and rerun the relevant checks. The fork PR workflows will then need maintainer approval to run. |
|
Thank you, @jae-heo! You were right that the explorer threw away the failure reason the import pipeline already returns. Main has changed a lot since this PR (the import handlers moved and the per-file limit is now 2 GB), so we rebuilt the fix in #33831 on the current code. That fix shows the shared reason in the file explorer, terminal drop and editor drop toasts, and keeps raw host paths out of the toast. You're credited as co-author on the commit. Thanks for the clear report and for pointing at the right seam! |
Summary
Fixes #9896.
Screenshots
No screenshot attached. This is a text-only extension of the existing error toast: its title is unchanged and its description now contains the actionable backend reason. No user data or local paths are included in this PR.
Testing
pnpm lint— full lint reaches pre-existing failures on currentmain: a non-exhaustive switch inskill-freshness-group.tsxand missing localization keys in unrelated files. Changed-file oxlint, type-aware oxlint, reliability gates, max-lines, localization coverage, styled-scrollbar, bundled-guide, and skill-manifest checks pass.pnpm typecheckpnpm test— 3,218 files passed; 34,127 tests passed; 7 files and 58 tests skippedpnpm buildAI Review Report
Reviewed the native desktop drop path, local import path, paired-runtime staging path, and SSH-backed import path. The actionable reason was already preserved through the main-process and authenticated runtime results but was discarded at the final renderer feedback seam. The change therefore leaves routing, ownership, destination planning, and the runtime RPC protocol unchanged.
The review checked repeated failures, empty reasons, an oversized top-level file, aggregate-size failures, and whether the file body is opened before rejecting an oversized file. Repeated reasons are deduplicated so a multi-file failure does not flood the toast, while distinct reasons remain visible.
Cross-platform review covered macOS, Linux, and Windows. No shortcut, shell command, platform path separator, native API, or Electron platform behavior changes. Local, paired-runtime, and SSH-backed imports continue to use their existing implementations.
Security Audit
Reviewed error provenance, input handling, paths, authentication, IPC, secrets, dependencies, and command execution.
No follow-up security work was identified.
Notes
ELI5
Failed remote file imports showed a vague toast with little detail. The error message now includes the real reasons (including size limits) so you know why the drop or upload failed.