fix(dashboard): show loading indicator when uploading assets#4940
fix(dashboard): show loading indicator when uploading assets#4940latifniz wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughChannel switching now preserves the current route when navigating between channels, with fallback routing when no matching target route exists. Asset uploads expose pending state, disable the upload button, show a loading icon, and display an upload overlay while processing. 🚥 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 |
1b37f51 to
1776783
Compare
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)
packages/dashboard/src/lib/components/shared/asset/asset-gallery.tsx (1)
249-254: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd an
onErrorhandler to theuseMutationcall.The mutation only defines
onSuccessbut noonError. As per coding guidelines,useMutationcalls should include bothonSuccessandonErrorhandlers. WithoutonError, upload failures are silently swallowed — the spinner stops but the user gets no feedback that the upload failed.♻️ Proposed fix
const { mutate: createAssets, isPending: isUploading } = useMutation({ mutationFn: api.mutate(createAssetsDocument), onSuccess: () => { queryClient.invalidateQueries({ queryKey }); }, + onError: (error) => { + // Show error notification to the user + console.error('Asset upload failed:', error); + }, });🤖 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 `@packages/dashboard/src/lib/components/shared/asset/asset-gallery.tsx` around lines 249 - 254, Add an onError handler to the useMutation call creating createAssets in the asset gallery, using the existing notification or error-reporting pattern to provide clear user feedback when asset upload fails while preserving the current onSuccess invalidation behavior.Source: Coding guidelines
🧹 Nitpick comments (1)
packages/dashboard/src/lib/components/shared/asset/asset-gallery.tsx (1)
447-452: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd
role="status"to the upload overlay for screen reader accessibility.The overlay visually communicates upload progress but lacks an ARIA live region. Screen reader users won't be notified when the upload starts or completes. Adding
role="status"(which impliesaria-live="polite") addresses this with minimal effort.♿ Proposed fix
{isUploading && ( - <div className="absolute inset-0 bg-background/70 backdrop-blur-sm z-10 flex flex-col items-center justify-center rounded-md"> + <div role="status" className="absolute inset-0 bg-background/70 backdrop-blur-sm z-10 flex flex-col items-center justify-center rounded-md"> <Loader2 className="h-10 w-10 text-primary animate-spin mb-3" /> <p className="text-center font-medium"><Trans>Uploading assets...</Trans></p> </div> )}🤖 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 `@packages/dashboard/src/lib/components/shared/asset/asset-gallery.tsx` around lines 447 - 452, Add role="status" to the upload overlay div rendered by the isUploading condition in the asset gallery, so screen readers announce upload state changes.
🤖 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 `@packages/dashboard/src/lib/components/shared/asset/asset-gallery.tsx`:
- Around line 249-254: Add an onError handler to the useMutation call creating
createAssets in the asset gallery, using the existing notification or
error-reporting pattern to provide clear user feedback when asset upload fails
while preserving the current onSuccess invalidation behavior.
---
Nitpick comments:
In `@packages/dashboard/src/lib/components/shared/asset/asset-gallery.tsx`:
- Around line 447-452: Add role="status" to the upload overlay div rendered by
the isUploading condition in the asset gallery, so screen readers announce
upload state changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8b1632cf-e9cb-4aee-b6d1-525a04a1c3ef
📒 Files selected for processing (2)
packages/dashboard/src/lib/components/layout/channel-switcher.tsxpackages/dashboard/src/lib/components/shared/asset/asset-gallery.tsx
grolmus
left a comment
There was a problem hiding this comment.
Nice UX improvement — the disabled button + spinner + overlay reads well, and thanks for sticking to semantic tokens (bg-background, text-primary). Two things before this can merge:
dashboard i18n syncis red — the new<Trans>Uploading assets…</Trans>string hasn't been extracted into the translation catalogs. Frompackages/dashboard, run:(orbun run i18n:extractnpm run i18n:extract --workspace=@vendure/dashboardfrom the repo root), then commit the updatedpackages/dashboard/src/i18n/locales/*.pofiles. The new message will land untranslated in the non-English catalogs — that's expected; the check just needs the catalogs to matchlingui extractoutput.- The branch is ~32 commits behind
master— please merge the latestmasterin (the "Update branch" button on this PR is the easiest way) so CI runs against current code.
Heads-up: #4941 also edits asset-gallery.tsx, so whichever of the two merges second will need a quick rebase.
Once i18n is green this LGTM.
Description
When uploading assets via Catalog → Assets, there was zero visual feedback during the upload process. The UI appeared completely frozen until all files finished uploading, then they appeared all at once. This was confusing especially when uploading multiple large files.
What changed
Added upload progress feedback to the
AssetGallerycomponent:Root cause
useMutationfrom React Query exposes anisPendingstate but it was never being used. The fix simply destructuresisPendingand wires it up to the UI.Notes
True per-file progress (e.g. percentage bar) is not possible with the current architecture because
api.mutateuses the Fetch API which does not expose upload progress events. This fix gives the best feedback possible without rewriting the upload mechanism to use XHR.The overlay also shows in the asset picker dialog (used when selecting assets on product/category pages) which is correct behaviour.
Checklist
Fixes issue #4936
here is video after fix ( this is local host so it takes less than a second but loader is visible, the issue i reported was in prod so it took a lot of time , see video in the issue as well)
fixloader.mp4
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.