[7231] Validate uploading files existence#4914
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds overwrite confirmation to upload flows by checking remote path existence before upload. Single-file upload now branches between policy confirmation, overwrite confirmation, and error handling. The Uppy dashboard plugin, UI, locale types, and file item actions were updated to surface overwrite-required state and actions. A placeholder button was removed from the upload dialog. ChangesOverwrite confirmation for uploads
UploadDialogContainer cleanup
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/uppy/src/plugins/Dashboard.jsx (1)
343-378: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftConfirm policy-corrected uploads as pending
confirmAllshould mark the dashboard as pending when an allowed file starts uploading. Theallowedbranch callscheckPathAndUpload(...)without anonUploadStartedcallback, soonPendingChanges(true)never fires for that path.validateAndRetryhas the same gap if single-file retries are meant to update pending state too. Mirror thevalidateFilesPolicycallback/pending-check flow here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/uppy/src/plugins/Dashboard.jsx` around lines 343 - 378, confirmAll in Dashboard.jsx does not mark policy-corrected allowed uploads as pending because the allowed branch calls checkPathAndUpload without any upload-start callback, so onPendingChanges(true) is never triggered. Update confirmAll to mirror the validateFilesPolicy flow by passing an onUploadStarted handler into checkPathAndUpload for the allowed-file path, and review validateAndRetry so its retry path also updates pending state consistently when an upload starts.
🧹 Nitpick comments (1)
ui/uppy/src/components/FileItem/Buttons/index.jsx (1)
85-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the click handler.
onClick={() => onClick()}wraps the callback unnecessarily. UseonClick={onClick}directly.♻️ Proposed refactor
- onClick={() => onClick()} + onClick={onClick}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/uppy/src/components/FileItem/Buttons/index.jsx` at line 85, The click handler in the Buttons component is wrapped unnecessarily with an arrow function, causing avoidable indirection. Update the onClick prop in the FileItem Buttons JSX to pass the existing onClick callback directly instead of wrapping it, and keep the same handler reference used in this component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/uppy/src/components/FileItem/Buttons/index.jsx`:
- Around line 128-133: The two ConfirmActionIcon render branches in
Buttons/index.jsx are not mutually exclusive, so validateAndRetry and
confirmOverwrite can both appear when overwriteRequired is true and
suggestedName is still present. Update the conditional rendering around the
file.meta.validating checks so the overwrite flow takes priority, for example by
preventing the validate-and-retry branch from rendering when
file.meta.overwriteRequired is set, while keeping confirmOverwrite tied to
overwriteRequired.
In `@ui/uppy/src/components/FileItem/FileInfo/index.jsx`:
- Around line 96-107: The refactored warning icon in warningIcon() is missing
the data-ga-event-* tracking attributes that the inline SVG still has in
renderPolicyWarning(). Update warningIcon() to include the same
data-ga-event-category, data-ga-event-action, and data-ga-event-label attributes
so both branches preserve analytics behavior.
---
Outside diff comments:
In `@ui/uppy/src/plugins/Dashboard.jsx`:
- Around line 343-378: confirmAll in Dashboard.jsx does not mark
policy-corrected allowed uploads as pending because the allowed branch calls
checkPathAndUpload without any upload-start callback, so onPendingChanges(true)
is never triggered. Update confirmAll to mirror the validateFilesPolicy flow by
passing an onUploadStarted handler into checkPathAndUpload for the allowed-file
path, and review validateAndRetry so its retry path also updates pending state
consistently when an upload starts.
---
Nitpick comments:
In `@ui/uppy/src/components/FileItem/Buttons/index.jsx`:
- Line 85: The click handler in the Buttons component is wrapped unnecessarily
with an arrow function, causing avoidable indirection. Update the onClick prop
in the FileItem Buttons JSX to pass the existing onClick callback directly
instead of wrapping it, and keep the same handler reference used in this
component.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0c9a6fa6-03bf-4a5d-950a-9ba371378f79
📒 Files selected for processing (10)
ui/app/src/components/SingleFileUpload/SingleFileUpload.tsxui/app/src/components/UploadDialog/UploadDialogContainer.tsxui/app/src/components/UppyDashboard/UppyDashboard.tsxui/uppy/src/components/Dashboard.jsxui/uppy/src/components/FileItem/Buttons/index.jsxui/uppy/src/components/FileItem/FileInfo/index.jsxui/uppy/src/components/FileItem/index.jsxui/uppy/src/components/FileList.jsxui/uppy/src/plugins/Dashboard.jsxui/uppy/types/generatedLocale.d.ts
…ending changes are tracked during uploads.
|
Outside diff range comment addressed |
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 4914 --repo craftercms/studio-ui --json title,body,files,commits,additions,deletions,changedFiles | head -c 3000
echo "----DIFF----"
gh pr diff 4914 --repo craftercms/studio-ui > /tmp/pr.diff
wc -l /tmp/pr.diffLength of output: 3412 🏁 Script executed: #!/bin/bash
set -euo pipefail
cat -n /tmp/pr.diffLength of output: 26225 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n 'checkPathExistence' -C5 ui/app/src/services/content.ts
echo "---preact check---"
head -30 ui/uppy/src/components/FileItem/FileInfo/index.jsx
echo "---package.json check for preact---"
rg -n '"preact"|"react"' ui/uppy/package.jsonLength of output: 1803
A few things worth a look before human review:
Nothing here is blocking — the core feature is well-implemented and consistent across bulk/single upload flows. These are refinement suggestions for you or the human reviewer to weigh in on. ✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/uppy/src/plugins/Dashboard.jsx (1)
202-204: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftSilent overwrite on existence-check failure.
When
checkPathExistenceerrors, the handler callsstartUpload(), so a failed check silently proceeds with the upload and can overwrite an existing file without user confirmation. This diverges from the single-file flow, which surfaces an error instead of continuing. Consider marking the file invalid (or routing to an error state) onerrorrather than uploading.Want me to draft an error-handling path that mirrors the single-file flow (flag the file / surface an error instead of uploading)?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/uppy/src/plugins/Dashboard.jsx` around lines 202 - 204, The `checkPathExistence` error handler in `Dashboard.jsx` is incorrectly falling back to `startUpload()`, which lets uploads continue after a failed existence check. Update the `error` callback in that existence-check flow to route the file into an error/invalid state instead of starting the upload, mirroring the single-file handling path so the user is notified and the file is not overwritten silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@ui/uppy/src/plugins/Dashboard.jsx`:
- Around line 202-204: The `checkPathExistence` error handler in `Dashboard.jsx`
is incorrectly falling back to `startUpload()`, which lets uploads continue
after a failed existence check. Update the `error` callback in that
existence-check flow to route the file into an error/invalid state instead of
starting the upload, mirroring the single-file handling path so the user is
notified and the file is not overwritten silently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae60d3d3-5ab6-42ea-95aa-069778ad9d20
📒 Files selected for processing (3)
ui/uppy/src/components/FileItem/Buttons/index.jsxui/uppy/src/components/FileItem/FileInfo/index.jsxui/uppy/src/plugins/Dashboard.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/uppy/src/components/FileItem/Buttons/index.jsx
- ui/uppy/src/components/FileItem/FileInfo/index.jsx
…mplementing onPathExistenceError callback and refining path existence checks during uploads.
|
Outside diff range comments addressed |
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 4914 --repo craftercms/studio-ui --json commits -q '.commits[-6:] | .[] | .oid + " " + .messageHeadline'
echo "----"
gh pr diff 4914 --repo craftercms/studio-ui > /tmp/pr.diff
wc -l /tmp/pr.diffLength of output: 999 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n 'checkPathAndUpload|finalizeExistenceChecks|pendingExistenceChecks|confirmAll|onPendingChanges' -A6 ui/uppy/src/plugins/Dashboard.jsx | head -300Length of output: 4188 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '170,225p' ui/uppy/src/plugins/Dashboard.jsx
echo "---confirmAll full---"
sed -n '362,412p' ui/uppy/src/plugins/Dashboard.jsxLength of output: 3251 All three concerns from the earlier review appear addressed now:
One remaining lighter-weight item: no new/updated tests were added for the async existence-check + confirm/skip/error flows, but as previously noted this is non-blocking for a draft PR headed to human review. Everything else — the ✏️ Learnings added
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ui/uppy/src/plugins/Dashboard.jsx`:
- Around line 204-208: The fallback error message in Dashboard.jsx is hardcoded
in English, so non-English users see untranslated copy. Update the detail
fallback inside the Dashboard plugin’s error handling to pull this string from
locale.strings or externalMessages instead of using the literal text. Keep the
existing error-preference chain in place, and use the Dashboard localization
hooks/symbols already available in the component to resolve the translated
fallback.
- Around line 256-259: The finalize flow in Dashboard’s
validateFilesPolicy/confirmAll path can run twice when checkPathExistence
resolves synchronously, so make the onPendingChanges/onComplete completion path
idempotent. Add a local guard in the relevant Dashboard.jsx logic so the
finalize callback can only fire once even if pending counts reach zero during
the loop and again after the loop, and keep the behavior tied to the existing
validateFilesPolicy, confirmAll, and onPendingChanges flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 98536331-fcbd-45c8-97a0-7de0c1fbd5e2
📒 Files selected for processing (2)
ui/app/src/components/UppyDashboard/UppyDashboard.tsxui/uppy/src/plugins/Dashboard.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/app/src/components/UppyDashboard/UppyDashboard.tsx
|
@coderabbitai can you check if everything looks good? Please give it a final review before I hand over for human review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 4914 --repo craftercms/studio-ui --json commits,updatedAt -q '.commits[-5:] | .[] | .oid + " " + .messageHeadline'
echo "----"
gh pr diff 4914 --repo craftercms/studio-ui > /tmp/pr.diff
wc -l /tmp/pr.diffLength of output: 859 🏁 Script executed: #!/bin/bash
set -euo pipefail
grep -n '^diff --git' /tmp/pr.diff
echo "---"
grep -n 'test' /tmp/pr.diff -i | head -20Length of output: 1603 🏁 Script executed: #!/bin/bash
set -euo pipefail
sed -n '417,644p' /tmp/pr.diffLength of output: 7113 All caught up — I re-pulled the latest diff and compared it against the state from my last review pass. No new commits since then introduce regressions on the three previously-flagged concerns, and the additional commits (
The Only remaining non-blocking note carried over from before: still no new/updated tests for the async existence-check + confirm/skip/error flows (bulk and single upload). Not blocking for handoff, but worth flagging to the human reviewer or a fast-follow. Everything else looks good — this is ready to hand off for human review. 🐇✅ |
1 participant
craftercms/craftercms#7231
Summary by CodeRabbit
New Features
Bug Fixes