From ae8b736d07f37d9c81a8476d38eba44f606f3504 Mon Sep 17 00:00:00 2001 From: Deluan Date: Mon, 7 Sep 2026 21:03:00 -0400 Subject: [PATCH] fix(ui): keep the user menu avatar in sync after upload or removal avatarTag was a login-time snapshot, so uploading an avatar left the menu on the old icon and removing one left it pointing at getAvatar, which now falls through to the Navidrome logo placeholder. The edit page now updates the stored tag when the record is the logged-in user, and the menu re-reads the identity on refresh. --- ui/src/common/ImageUploadOverlay.jsx | 4 +- ui/src/layout/UserMenu.jsx | 19 +++++++--- ui/src/layout/UserMenu.test.jsx | 54 +++++++++++++++++++++----- ui/src/setupTests.js | 3 ++ ui/src/user/UserEdit.jsx | 14 +++++++ ui/src/user/UserEdit.test.jsx | 57 ++++++++++++++++++++++++++-- 6 files changed, 132 insertions(+), 19 deletions(-) diff --git a/ui/src/common/ImageUploadOverlay.jsx b/ui/src/common/ImageUploadOverlay.jsx index 119426c73..03144ddc0 100644 --- a/ui/src/common/ImageUploadOverlay.jsx +++ b/ui/src/common/ImageUploadOverlay.jsx @@ -88,7 +88,7 @@ export const ImageUploadOverlay = ({ body: formData, }) notify(msg.uploaded, 'success') - if (onImageChange) onImageChange() + if (onImageChange) onImageChange(true) refresh() } catch (err) { notify(msg.uploadError, 'warning') @@ -117,7 +117,7 @@ export const ImageUploadOverlay = ({ method: 'DELETE', }) notify(msg.removed, 'success') - if (onImageChange) onImageChange() + if (onImageChange) onImageChange(false) refresh() } catch (err) { notify(msg.removeError, 'warning') diff --git a/ui/src/layout/UserMenu.jsx b/ui/src/layout/UserMenu.jsx index 49c64928d..1b465ed35 100644 --- a/ui/src/layout/UserMenu.jsx +++ b/ui/src/layout/UserMenu.jsx @@ -7,7 +7,7 @@ import { useState, } from 'react' import PropTypes from 'prop-types' -import { useTranslate, useGetIdentity } from 'react-admin' +import { useTranslate, useGetIdentity, useVersion } from 'react-admin' import { Tooltip, IconButton, @@ -52,15 +52,24 @@ const UserMenu = (props) => { const [anchorEl, setAnchorEl] = useState(null) const translate = useTranslate() const { loaded, identity } = useGetIdentity() + const version = useVersion() + const [avatarTag, setAvatarTag] = useState(null) const classes = useStyles(props) const dispatch = useDispatch() const { children, label, icon, logout } = props - // identity.id is the username (see authProvider.getIdentity); avatarTag is only set when an avatar was uploaded - const avatarUrl = identity?.avatarTag - ? `${subsonic.getAvatarUrl(identity.id)}&_=${identity.avatarTag}` - : identity?.avatar + // The tag is only set when an avatar was uploaded, and it changes mid-session on upload or + // removal, so the identity is re-read on every refresh instead of trusting the login snapshot. + useEffect(() => { + authProvider.getIdentity().then((user) => setAvatarTag(user?.avatarTag)) + }, [version]) + + // identity.id is the username (see authProvider.getIdentity) + const avatarUrl = + avatarTag && identity?.id + ? `${subsonic.getAvatarUrl(identity.id)}&_=${avatarTag}` + : identity?.avatar useEffect(() => { if (config.devActivityPanel) { diff --git a/ui/src/layout/UserMenu.test.jsx b/ui/src/layout/UserMenu.test.jsx index 15c42e81b..4a21b94ca 100644 --- a/ui/src/layout/UserMenu.test.jsx +++ b/ui/src/layout/UserMenu.test.jsx @@ -1,6 +1,6 @@ import React from 'react' import { render, screen } from '@testing-library/react' -import { describe, it, expect, vi } from 'vitest' +import { describe, it, expect, vi, beforeEach } from 'vitest' import UserMenu from './UserMenu' vi.mock('../subsonic', () => ({ @@ -14,34 +14,42 @@ vi.mock('react-redux', () => ({ })) let mockIdentity +let mockVersion vi.mock('react-admin', () => ({ useTranslate: () => (x) => x, useGetIdentity: () => ({ loaded: true, identity: mockIdentity }), + useVersion: () => mockVersion, })) const renderUserMenu = (identity) => { mockIdentity = identity - render(Logout} />) + return render(Logout} />) } describe('', () => { - it('uses the uploaded avatar when the identity has an avatarTag', () => { - renderUserMenu({ id: 'u1', username: 'deluan', avatarTag: 'abc123' }) - expect(screen.getByRole('img')).toHaveAttribute( + beforeEach(() => { + localStorage.clear() + mockVersion = 1 + }) + + it('uses the uploaded avatar when there is an avatar tag', async () => { + localStorage.setItem('avatarTag', 'abc123') + renderUserMenu({ id: 'deluan', fullName: 'Deluan' }) + expect(await screen.findByRole('img')).toHaveAttribute( 'src', expect.stringContaining('getAvatar'), ) }) it('falls back to the generic icon with no avatar and no gravatar', () => { - renderUserMenu({ id: 'u1', username: 'deluan' }) + renderUserMenu({ id: 'deluan', fullName: 'Deluan' }) expect(screen.queryByRole('img')).not.toBeInTheDocument() }) - it('uses the gravatar url when there is no avatarTag', () => { + it('uses the gravatar url when there is no avatar tag', () => { renderUserMenu({ - id: 'u1', - username: 'deluan', + id: 'deluan', + fullName: 'Deluan', avatar: 'https://gravatar/u1', }) expect(screen.getByRole('img')).toHaveAttribute( @@ -49,4 +57,32 @@ describe('', () => { 'https://gravatar/u1', ) }) + + it('picks up an avatar uploaded during the session', async () => { + const { rerender } = renderUserMenu({ id: 'deluan', fullName: 'Deluan' }) + expect(screen.queryByRole('img')).not.toBeInTheDocument() + + localStorage.setItem('avatarTag', 'newtag') + mockVersion = 2 + rerender(Logout} />) + + expect(await screen.findByRole('img')).toHaveAttribute( + 'src', + expect.stringContaining('newtag'), + ) + }) + + it('goes back to the generic icon when the avatar is removed during the session', async () => { + localStorage.setItem('avatarTag', 'abc123') + const { rerender } = renderUserMenu({ id: 'deluan', fullName: 'Deluan' }) + expect(await screen.findByRole('img')).toBeInTheDocument() + + localStorage.removeItem('avatarTag') + mockVersion = 2 + rerender(Logout} />) + + await vi.waitFor(() => + expect(screen.queryByRole('img')).not.toBeInTheDocument(), + ) + }) }) diff --git a/ui/src/setupTests.js b/ui/src/setupTests.js index ddb999f3c..7cb46e09c 100644 --- a/ui/src/setupTests.js +++ b/ui/src/setupTests.js @@ -14,6 +14,9 @@ const localStorageMock = (function () { setItem: function (key, value) { store[key] = value.toString() }, + removeItem: function (key) { + delete store[key] + }, clear: function () { store = {} }, diff --git a/ui/src/user/UserEdit.jsx b/ui/src/user/UserEdit.jsx index b6523bc2b..567709832 100644 --- a/ui/src/user/UserEdit.jsx +++ b/ui/src/user/UserEdit.jsx @@ -87,6 +87,19 @@ const AvatarField = () => { const canEdit = config.enableUserAvatarUpload && (isAdmin || isMyself) const classes = useAvatarStyles() + const handleImageChange = useCallback( + (hasImage) => { + if (!isMyself) return + // Only a cache-buster: the server sends the authoritative ETag, so any changing value works + if (hasImage) { + localStorage.setItem('avatarTag', Date.now().toString()) + } else { + localStorage.removeItem('avatarTag') + } + }, + [isMyself], + ) + if (!record?.id) return null return ( @@ -104,6 +117,7 @@ const AvatarField = () => { entityType="user" entityId={record.id} hasUploadedImage={!!record.uploadedImage} + onImageChange={handleImageChange} canEdit={canEdit} messages={{ uploaded: 'message.avatarUploaded', diff --git a/ui/src/user/UserEdit.test.jsx b/ui/src/user/UserEdit.test.jsx index 0f5dd5a1c..52e955ed7 100644 --- a/ui/src/user/UserEdit.test.jsx +++ b/ui/src/user/UserEdit.test.jsx @@ -1,5 +1,5 @@ import * as React from 'react' -import { render, screen } from '@testing-library/react' +import { render, screen, fireEvent } from '@testing-library/react' import UserEdit from './UserEdit' import config from '../config' import { describe, it, expect, vi, afterEach } from 'vitest' @@ -86,8 +86,23 @@ vi.mock('./DeleteUserButton', () => ({ vi.mock('../common', () => ({ Title: ({ subTitle }) =>
{subTitle}
, - ImageUploadOverlay: ({ canEdit, messages }) => - canEdit ? : null, + ImageUploadOverlay: ({ canEdit, messages, onImageChange }) => + canEdit ? ( + <> + + + + ) : null, })) vi.mock('../subsonic', () => ({ @@ -235,6 +250,42 @@ describe('', () => { expect(screen.getByLabelText('message.uploadAvatar')).toBeInTheDocument() }) + it('stores a new avatar tag when the user uploads their own avatar', () => { + config.enableUserAvatarUpload = true + renderUserEdit({ id: 'u1', userName: 'deluan' }, { isMyself: true }) + + fireEvent.click(screen.getByLabelText('message.uploadAvatar')) + + expect(localStorage.getItem('avatarTag')).toBeTruthy() + }) + + it('removes the avatar tag when the user removes their own avatar', () => { + config.enableUserAvatarUpload = true + localStorage.setItem('avatarTag', 'oldtag') + renderUserEdit( + { id: 'u1', userName: 'deluan', uploadedImage: 'u1_deluan.png' }, + { isMyself: true }, + ) + + fireEvent.click(screen.getByLabelText('message.removeAvatar')) + + expect(localStorage.getItem('avatarTag')).toBeNull() + }) + + it('does not touch the admin own tag when editing another user', () => { + config.enableUserAvatarUpload = true + renderUserEdit( + { id: 'u1', userName: 'deluan', uploadedImage: 'u1_deluan.png' }, + { isMyself: false, role: 'admin' }, + ) + localStorage.setItem('avatarTag', 'mytag') + + fireEvent.click(screen.getByLabelText('message.uploadAvatar')) + fireEvent.click(screen.getByLabelText('message.removeAvatar')) + + expect(localStorage.getItem('avatarTag')).toEqual('mytag') + }) + it('hides the control when the feature is off and the viewer is not an admin', () => { config.enableUserAvatarUpload = false renderUserEdit(