Repository navigation
Add an optional theme-aware header account menu - #2376
miaolinopenvox wants to merge 3 commits into
Conversation
✅ Deploy Preview for dashy-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
added
the
✨ New Feature
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Responsive header positioning can clip account actions and overlap the account trigger with navigation.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds an optional, theme-aware header account menu that displays identity and administrator status while reusing existing authentication actions.
Changes:
- Adds a lazy-loaded disclosure with keyboard support.
- Refreshes account information from local session changes and OIDC events.
- Adds configuration, translations, documentation, and targeted tests.
| File | Description |
|---|---|
| tests/unit/account-state.test.js | Tests account presentation decisions. |
| tests/unit/account-state-integration.test.js | Tests native session and expiry handling. |
| tests/unit/account-oidc-refresh.test.js | Tests refreshed OIDC identity and roles. |
| tests/components/account-native-actions.test.js | Tests existing authentication actions. |
| tests/components/account-menu.test.js | Tests disclosure interaction and refresh. |
| src/utils/config/ConfigSchema.json | Defines the optional configuration flag. |
| src/utils/auth/OidcAuth.js | Persists refreshed identity and notifies the store. |
| src/utils/auth/AccountState.js | Derives display-only account information. |
| src/components/Settings/AuthButtons.vue | Adds reusable presentation options. |
| src/components/PageStrcture/Header.vue | Integrates the lazy-loaded account menu. |
| src/components/PageStrcture/AccountMenu.vue | Implements the themed account disclosure. |
| src/assets/locales/zh-CN.json | Adds Simplified Chinese labels. |
| src/assets/locales/en.json | Adds English labels. |
| docs/configuring.md | Documents configuration and session behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .account-panel { | ||
| position: absolute; | ||
| top: calc(100% + 0.25rem); | ||
| right: 0; |
| <div class="header-actions"> | ||
| <Nav :links="pageInfo.navLinks" :user-hidden="!navVisible" class="nav" /> | ||
| <AccountMenu v-if="$store.getters.appConfig.showAccountMenu" /> |
lissy93
left a comment
lissy93
left a comment
There was a problem hiding this comment.
Hey @miaolinopenvox
Thanks for the PR, it looks good :)
Although there's room to refine is a bit, mostly:
- Try and keep the files this PR touches to a minimum, most of it should be able to be confined to AccountMenu.vue
- Be sure not to duplicate code/logic that we already have
- Careful with making changes to OIDC functionality in Auth.js
- Remove all those tests, and re-write based on testing the actual complexities
- Take a quick read through the code, some of it is much longer and more complex than it needs to be, and will add maintainability overhead for future developers
| ### Header account menu | ||
|
|
||
| Set `appConfig.showAccountMenu: true` to show account information beside the navigation. | ||
| Signed-out visitors see **Guest / Not signed in**. Signed-in users see their Dashy | ||
| username and, when applicable, a **Dashy admin** badge. Open the disclosure to use | ||
| Dashy's existing sign-in or sign-out action. OIDC and Keycloak sign-out may also | ||
| end the provider session. No authentication configured means no account menu, | ||
| even though an unprotected dashboard can allow configuration changes. | ||
|
|
||
| The menu uses the active theme's navigation and settings colors and supports long | ||
| names, narrow screens, keyboard navigation and Escape to close. Its state follows | ||
| Dashy's existing local session checks: auth changes, other-tab storage changes, | ||
| window focus, and a local check every 15 seconds for cookie/token expiry. These | ||
| checks do not contact the identity provider or guarantee immediate detection of | ||
| provider-side account revocation. The badge describes Dashy permissions, not an | ||
| identity provider's administrator role, and never authorizes server operations. |
There was a problem hiding this comment.
This should be in the auth docs instead maybe?
Or at least this file is the wrong place, since none of the other config options have a paragraph at the bottom.
| <span v-if="showGreeting && greeting" class="user-type-note">{{ greeting }}</span> | ||
| <span v-else-if="showGreeting && isGuest" class="user-type-note">{{ $t('settings.please-login') }}</span> |
There was a problem hiding this comment.
Would this change existing functionality? What is showGreeting from, I can't see where it's being passed in, so presumably it's always false and these 2 elements won't get shown anymore?
| import { userStateEnum } from '@/utils/config/defaults'; | ||
|
|
||
| /** Display-only snapshot. Server authorization never depends on this value. */ | ||
| export default function getAccountState(auth = {}) { |
There was a problem hiding this comment.
Is this file needed?
You could instead read $store.getters.userState for userType and call getCurrentUser() / isUserAdmin() directly. Saves duplicating logic which might diverge/change later.
| // Mirror token renewals into localStorage so Bearer attachment stays fresh | ||
| this.userManager.events.addUserLoaded((user) => { | ||
| if (user?.id_token) localStorage.setItem(localStorageKeys.ID_TOKEN, user.id_token); | ||
| if (user?.id_token) this.persistUserInfo(user); |
There was a problem hiding this comment.
This will now sign the user in twice.
There was a problem hiding this comment.
And changes here make me nervous. If I break something, it can brick OICD users out of their system, and OIDC has been really hard to test, since there's so many different OIDC providers/methods. So if possible, I want to keep as much of the logic the same as before, since at least we know that works 😅
| <span class="account-status">{{ $t(account.loggedIn | ||
| ? 'account-menu.signed-in' : 'account-menu.signed-out') }}</span> |
There was a problem hiding this comment.
I should update my check-locale script, so that we don't get that new warning that this translation is unused.
In the meantime if you want to solve the warning, you can write it like this:
account.loggedIn ? $t('account-menu.signed-in') : $t('account-menu.signed-out'))(basically just always use a raw string for the $t() param instead of a variable).
| setup(state); await wrapper.find('button').trigger('click'); | ||
| await vi.advanceTimersByTimeAsync(500); expect(actions.logout).toHaveBeenCalledOnce(); | ||
| }); | ||
| it('local logout invokes native logout, auth notification and login navigation', async () => { |
There was a problem hiding this comment.
Correct me if I am wrong, but is this sharing one mock across both OIDC and KC providers? Is that ok? Maybe, but it's unclear.
| fixture.loaded({ id_token: 'new-fixture-only', profile: { preferred_username: 'alice-renamed', groups: [] } }); | ||
| expect(localStorage.setItem).toHaveBeenCalledWith(localStorageKeys.USERNAME, 'alice-renamed'); | ||
| expect(localStorage.setItem).toHaveBeenCalledWith(localStorageKeys.ISADMIN, false); |
There was a problem hiding this comment.
Expects to have been called, but this would have happened, because you just called it. It should really assert AUTH_CHANGED properly.
| vi.mock('@/store', () => ({ default: { commit: fixture.commit } })); | ||
| vi.mock('@/utils/i18n', () => ({ default: { global: { t: key => key } } })); | ||
| vi.mock('@/utils/Toast', () => ({ toast: vi.fn() })); | ||
| vi.mock('@/utils/logging/CoolConsole', () => ({ statusMsg: vi.fn(), statusErrorMsg: vi.fn() })); | ||
| vi.mock('@/utils/logging/ErrorHandler', () => ({ default: vi.fn() })); | ||
| vi.mock('oidc-client-ts', () => ({ | ||
| WebStorageStateStore: class {}, | ||
| Log: { setLogger: vi.fn(), setLevel: vi.fn() }, | ||
| UserManager: class { | ||
| events = { addSilentRenewError() {}, addUserSignedOut() {}, addUserLoaded(fn) { fixture.loaded = fn; } }; | ||
| getUser() { return Promise.resolve(null); } | ||
| }, | ||
| })); | ||
| import { initOidcAuth } from '@/utils/auth/OidcAuth'; |
There was a problem hiding this comment.
Again, this isn't very clear. I think it's just checking what it just called was called? But no assertion that it worked, or value was correct.
There should be some warnings in the console which should help with this.
| } })); | ||
| let wrapper; | ||
| let store; | ||
| const guest = { name: '', loggedIn: false, admin: false, userType: 2 }; |
There was a problem hiding this comment.
userType: 2? I think it should be userStateEnum, and it's 4 which is keycloak enabled, not 2.
There was a problem hiding this comment.
Much of this is duplicated from account-state-integration. Can probably consolidate this.
|
Thanks Miao, but closing for now as no response on the feedback. |
Category
Feature
Overview
Dashy currently exposes account details inside its settings menu. This adds an optional header disclosure so users can see their current username, sign-in state and Dashy administrator status without opening settings.
Enable it with:
The option defaults to
false. The new control uses existing navigation/settings theme variables and delegates sign-in and sign-out toAuthButtons. It supports keyboard access, Escape/focus return, long names and narrow screens. When authentication is not configured, no identity or administrator badge is invented.Account changes refresh on existing auth notifications, cross-tab storage events and window focus, plus a local 15-second check for cookie/token expiry. This does not poll the identity provider or promise immediate upstream revocation detection. OIDC user-loaded events now persist refreshed username/group/role information and notify the store, rather than updating only the token. The badge is informational and does not change server authorization.
The optional component is lazy-loaded: eager loading exposed an existing configuration/store dependency cycle in the production bundle, which was caught in browser testing.
Issue Number
Related discussion: #1823 (current-user display in a comment). This does not claim to resolve the original OIDC administrator-permission issue.
Additional Info
appConfig.showAccountMenu, defaultfalse; documents behavior and adds English/Simplified Chinese labels. Existing UI remains unchanged when disabled.Screenshots below use a local synthetic configuration, not a production organization.