Fix multi file upload#2331
Open
rxtted wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR migrates file upload URL generation from a server-side method to client-side API requests. The ChangesUpload URL Generation Migration
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
🧹 Nitpick comments (2)
resources/views/filament/server/pages/list-files.blade.php (1)
14-21: ⚡ Quick winAdd response structure validation for resilience.
The function accesses
(await r.json()).attributes.urlwithout validating that these nested properties exist. If the API response structure is unexpected or malformed, this will throw aTypeErrorwith a cryptic message that gets caught and displayed as "Failed to get upload token", making debugging difficult.🛡️ Suggested improvement for defensive error handling
async fetchUploadUrl() { const r = await fetch(`/api/client/servers/${this.serverUuid}/files/upload`, { headers: { Accept: 'application/json', 'X-Requested-With': 'XMLHttpRequest' }, credentials: 'same-origin', }); if (!r.ok) throw new Error(`upload url request failed (${r.status})`); - return (await r.json()).attributes.url; + const data = await r.json(); + if (!data?.attributes?.url) { + throw new Error('Invalid upload URL response format'); + } + return data.attributes.url; },🤖 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 `@resources/views/filament/server/pages/list-files.blade.php` around lines 14 - 21, The fetchUploadUrl function currently returns (await r.json()).attributes.url without validating the response shape; update fetchUploadUrl to parse the JSON into a variable, verify that the top-level object is present and that json.attributes is an object and json.attributes.url is a non-empty string (or a valid URL), and throw a clear, descriptive Error including the parsed response when the shape is invalid so callers see why "Failed to get upload token" happened; keep the existing r.ok check and return the validated url when present.resources/views/filament/server/pages/file-upload.blade.php (1)
10-17: ⚡ Quick winAdd response structure validation for resilience.
The function accesses
(await r.json()).attributes.urlwithout validating that these nested properties exist. If the API response structure is unexpected or malformed, this will throw aTypeErrorwith a cryptic message that gets caught and displayed as "Failed to get upload token", making debugging difficult.🛡️ Suggested improvement for defensive error handling
async fetchUploadUrl() { const r = await fetch(`/api/client/servers/${this.serverUuid}/files/upload`, { headers: { Accept: 'application/json', 'X-Requested-With': 'XMLHttpRequest' }, credentials: 'same-origin', }); if (!r.ok) throw new Error(`upload url request failed (${r.status})`); - return (await r.json()).attributes.url; + const data = await r.json(); + if (!data?.attributes?.url) { + throw new Error('Invalid upload URL response format'); + } + return data.attributes.url; },🤖 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 `@resources/views/filament/server/pages/file-upload.blade.php` around lines 10 - 17, The fetchUploadUrl method currently assumes the JSON response contains attributes.url; change it to parse the JSON, validate that the parsed object has a top-level attributes object and a string url (e.g., check typeof body === 'object' && body.attributes && typeof body.attributes.url === 'string'), and if not throw a descriptive Error (including the actual response body or a snippet) so callers see a clear message instead of a cryptic TypeError; keep the existing fetch and status check around serverUuid but replace the direct return of (await r.json()).attributes.url with this validated extraction and error path.
🤖 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.
Nitpick comments:
In `@resources/views/filament/server/pages/file-upload.blade.php`:
- Around line 10-17: The fetchUploadUrl method currently assumes the JSON
response contains attributes.url; change it to parse the JSON, validate that the
parsed object has a top-level attributes object and a string url (e.g., check
typeof body === 'object' && body.attributes && typeof body.attributes.url ===
'string'), and if not throw a descriptive Error (including the actual response
body or a snippet) so callers see a clear message instead of a cryptic
TypeError; keep the existing fetch and status check around serverUuid but
replace the direct return of (await r.json()).attributes.url with this validated
extraction and error path.
In `@resources/views/filament/server/pages/list-files.blade.php`:
- Around line 14-21: The fetchUploadUrl function currently returns (await
r.json()).attributes.url without validating the response shape; update
fetchUploadUrl to parse the JSON into a variable, verify that the top-level
object is present and that json.attributes is an object and json.attributes.url
is a non-empty string (or a valid URL), and throw a clear, descriptive Error
including the parsed response when the shape is invalid so callers see why
"Failed to get upload token" happened; keep the existing r.ok check and return
the validated url when present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c603892-b90f-4578-98e2-d5a6e759bea1
📒 Files selected for processing (3)
app/Filament/Server/Resources/Files/Pages/ListFiles.phpresources/views/filament/server/pages/file-upload.blade.phpresources/views/filament/server/pages/list-files.blade.php
💤 Files with no reviewable changes (1)
- app/Filament/Server/Resources/Files/Pages/ListFiles.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #2326
multi-file uploads were 404ing after the first file. $wire.getUploadUrl() goes through livewire's request batcher, so the parallel uploadFile() loop (maxConcurrent=3) handed every concurrent caller the same jwt. wings invalidates the token after first use, so files 2+ all hit 404.
fetching the existing /api/client/servers/{uuid}/files/upload route directly bypasses the batcher, each fetch is independent so every upload gets its own jwt. the livewire helper got dropped along with its imports, nothing was calling it after the swap.
the other option was pre-fetching urls sequentially via $wire.getUploadUrl() before the parallel loop. stays in the livewire idiom and the diff is smaller, but it's one round-trip per file before any upload starts (a 100-file batch sits for like 5s before sending the first byte). went with the http route for the parallel-friendliness, i'm open to the sequential shape if people prefer.
i noticed that the upload pipeline is duplicated across file-upload.blade and list-files.blade. i didn't touch it, n just patched both.
tested on panel b35 + wings b25: parallel batches all succeed, weird filenames get handled, nested folders via drag-drop, also forced a 403 on the url fetch to confirm the failure UX still surfaces a per-file error.