feat(dashboard): Overhaul shell navigation and identity - #4995
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Dashboard Preview: https://admin-dashboard-akp0g5ko3-vendure.vercel.app |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change adds public and private asset visibility, administrator avatar storage and GraphQL operations, shared media handling, dashboard channel colors, and global navigation shortcuts. It updates generated GraphQL and TypeScript types, dashboard UI components, end-to-end tests, API documentation, manifests, and localization catalogs. Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 review |
✅ Action performedReview finished.
|
michaelbromley
left a comment
michaelbromley
left a comment
There was a problem hiding this comment.
Thorough review across four areas: the core admin-avatar/stored-media backend, the core asset-service refactor and asset visibility, the Dashboard shell navigation/shortcuts, and the Dashboard identity/channel-colors/profile work. The feature work is substantial and mostly well-built, but there are a couple of issues I'd want resolved before this merges, plus a batch of concrete fixes on the backend avatar path.
Blockers
1. A shortcut conflict can crash the entire Dashboard, and the type system doesn't prevent it.
define-dashboard-extension.ts throws from inside a bare useEffect (main.tsx:148) when shortcut validation finds a conflict. There is no ErrorBoundary anywhere in the dashboard, so an unhandled throw from a passive effect takes down the whole React root rather than failing in a scoped way. It is also trivially reachable: NavMenuSection.items[].shortcut uses the unrestricted NavigationShortcut type (which includes the reserved letters a c d m o p s), while only the routes[].navMenuItem path gets the compile-time-restricted DashboardExtensionNavigationShortcut. So navSections: [{ items: [{ shortcut: 'p' }] }] compiles cleanly and then crashes at runtime. buildShortcutMap already handles the identical conflict case by disabling the shortcut and logging a console.warn — this path should follow that pattern instead of throwing, and both nav-registration paths should share the restricted type.
2. The new G-chord navigation fails WCAG 2.1 SC 2.1.4 (Character Key Shortcuts).
nav-main.tsx binds bare single letter/digit keys as global shortcuts that are always live app-wide, with no toggle, no remap, and no focus-scoping. Screen-reader users in browse mode rely on those same single keys for their own quick navigation. SC 2.1.4 requires at least one of: the shortcut can be turned off, it can be remapped, or it is only active when a specific component has focus. None of those are currently provided.
High — security and data integrity
3. The private-asset boundary is enforced only inside AssetService, so it is bypassable through Asset-typed custom-field relations, including on the shop API.
Only asset.service.ts findOne/findAll filter on visibility === PUBLIC. The custom-field-relation resolver queries the Asset repository directly and never goes through AssetService. Asset relation custom fields are a normal, documented pattern (the test suite itself uses a shop-facing one on Order). Writing a private avatar's ID (sequential and enumerable) into such a field exposes the full private Asset — source, preview, mimeType — to anyone who can read that field on the storefront. This needs enforcement at a lower layer, or an explicit documented restriction with a guard rail.
4. Avatar upload bypasses Permission.CreateAsset, and SVG passes the imageOnly check.
setActiveAdministratorAvatar gates on Permission.Owner only, so an administrator with no granted permissions (the new e2e test proves this) can persist a file to storage without holding CreateAsset. The imageOnly option does not exclude image/svg+xml. The UI renders only the rasterized preview, so the current UI path is safe, but the raw source file is stored unmodified and is exposed via GraphQL. A low-privilege admin can upload a <script>-bearing SVG that executes for anyone who opens avatar.source directly. The new part is that this capability is now reachable regardless of assigned permissions.
Medium
5. Missing i18n key: administrator.service.ts throws error.mime-type-not-permitted, which does not exist in any locale file including en.json. Every rejected avatar upload surfaces the raw key instead of a translated message.
6. Read-modify-write race in setAvatar: two concurrent setActiveAdministratorAvatar calls both read the same previous avatar and each create a new Asset plus two storage files; the last save wins and the other Asset row and files are permanently orphaned with no cleanup path.
7. softDelete on an Administrator never calls deletePrivate on a still-set avatar, and onDelete: SET NULL never fires because the Asset row is not removed. Repeatedly creating and deleting admins with avatars accumulates orphaned private assets and files indefinitely.
8. The asset-server-plugin URL-prefix builder now trusts the client-supplied X-Forwarded-Host header with no allowlist, and getFirstHeaderValue picks the first (least-trusted) entry. This is host-header injection: a spoofed value wins over the real Host and poisons every asset source/preview URL rendered into the UI. There is no config to scope trust to a known proxy.
9. Channel colors are written as the entire color map to one global settings blob, and the server-side set() is a find-or-create-then-overwrite with no per-key merge or locking. Two admins or two tabs editing colors around the same time will silently clobber each other's changes with no error or toast.
Low and quality
setActiveAdministratorAvataris declaredAdministrator!but returnsundefinedwhen there is no active admin, producing a generic "cannot return null" error (inherited fromupdateActiveAdministrator).- It throws a generic
UserInputErroron aMimeTypeErrorinstead of returning the typedCreateAssetResultunion the rest of the Asset API uses. - Global keydown/keyup/blur listeners are torn down and reattached on nearly every sidebar interaction because
startNavigationChord's deps includeopenand section state. Reading those via refs would remove the churn. - Chord state can be left un-restored if the user clicks a nav link while still holding
G(overwrites freshly-computed open sections with the stale pre-chord snapshot), or if the viewport flips to mobile mid-chord (cleanup removes the listeners but never cancels the chord). canEdit/isAvailableinuse-channel-colors.tsaretruebefore the query resolves (react-queryisErrorstarts false), so every channel flashes "neutral" on first render, including ones that already have a color. Needs anisLoadingguard, and because the hook is instantiated in four separate places the gate has to be fixed in all of them.- The avatar upload button never shows a disabled/busy state while uploading, because the
disabled:CSS classes do not apply to the<label>render target. Cosmetic only; clicks are safely blocked in JS. The existingasset-gallerydropzone pattern avoids this. - Private assets are silently dropped from bulk
delete/assignToChannel/updateEntityAssets, sodelete()can returnDELETEDwhen nothing actually happened.
Nits
service.module.tsbreaks the file's alphabetical import order forStoredMediaService(likely a lint failure).administrator.resolver.tsdouble-fetches the same Administrator (findOneByUserIdthensetAvatarcallsfindOne).dashboard.plugin.tsself-spreads thevendure.dashboardsettings key before assigning it, which reads like defensive code for a scenario that should not occur.validateChannelColorsnever prunes colors for deleted channels, so the blob grows forever.nav-user-identity.spec.tsxasserts the absence of old classes rather than the presence of the new ones, which is a weak regression guard.
Verified sound
For the record, these held up under review: StoredMediaService is a faithful relocation of the old asset write/preview/MIME logic with the stream-guard, peek-then-write, and three-signal MIME validation all preserved; update() rejects non-public assets and UpdateAssetInput does not expose visibility, so there is no path to flip private to public through the API; the products.spec.ts refactor is more correct than the wait it replaced; and the themed bg-chart-* classes and the "hide controls without the plugin" test premise both check out.
michaelbromley
added
the
T3: Systemic
|
T3: Systemic — new public API + extends a core concept (patterns 5 & 6).
|
dlhck
changed the title
Start both stored-media deletions together after commit, auto-open empty configurable-operation list arguments, and update dashboard E2E interactions for the new shell controls.\n\nRelates to #4987
… explore-dashboard-shell-ux
The filter's first empty argument now opens automatically, so the previous click ambiguously matched the open chip and its nested add button. Assert the open state before selecting a facet value.\n\nRelates to #3548
|
|
@michaelbromley I’ve decided to take the rename/reframe route rather than introduce authenticated asset delivery in this PR. I’ve removed So avatar URLs retain the delivery and security characteristics of the configured |
2 participants
Description
This overhauls Dashboard shell identity with configurable channel colors, dedicated administrator avatar media, profile controls, and shared icon fallbacks.
It adds permission-filtered
Gnavigation chords, conflict handling, keytips, temporary sidebar expansion and restoration, and a platform-specific sidebar-toggle hint while preserving extension compatibility and plugin-less fallbacks.Core, Dashboard, unit/e2e, and production-build checks validate storage lifecycle, permissions, settings persistence, shortcuts, and the updated shell.
Breaking changes
No breaking changes.
Screenshots
Not included.
Checklist
📌 Always:
👍 Most of the time:
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.