From 445880c0065abed0d5547f03ddb445ae8534daa9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= <deluan@navidrome.org>
Date: Thu, 17 Jul 2025 11:00:12 -0400
Subject: [PATCH] fix(ui): prevent disabled Show in Playlist menu item from
 triggering actions (#4356)

* fix: prevent disabled Show in Playlist menu item from triggering actions

Fixed bug where clicking on the disabled 'Show in Playlist' menu item would unintentionally trigger music playback and replace the queue. The menu item now properly prevents event propagation when disabled and takes no action.

This resolves the issue where users would accidentally start playing music when clicking on the greyed-out menu option. The fix includes:
- Custom onClick handler that stops event propagation for disabled state
- Proper styling to maintain visual disabled state while allowing event handling
- Comprehensive test coverage for the disabled behavior

* style: clean up disabled menu item styling code

Simplified the arrow function for disabled onClick handler and changed inline style from empty object to undefined when not needed. Also added a CSS class for disabled menu items for potential future use.

These changes improve code readability and follow React best practices by using undefined instead of empty objects for conditional styles.
---
 ui/src/common/SongContextMenu.jsx      | 25 ++++++++++++++++-----
 ui/src/common/SongContextMenu.test.jsx | 31 +++++++++++++++++++++++---
 2 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/ui/src/common/SongContextMenu.jsx b/ui/src/common/SongContextMenu.jsx
index 32fbeb24..5eff495b 100644
--- a/ui/src/common/SongContextMenu.jsx
+++ b/ui/src/common/SongContextMenu.jsx
@@ -31,6 +31,9 @@ const useStyles = makeStyles({
   noWrap: {
     whiteSpace: 'nowrap',
   },
+  disabledMenuItem: {
+    pointerEvents: 'auto',
+  },
 })
 
 const MoreButton = ({ record, onClick, info }) => {
@@ -233,19 +236,29 @@ export const SongContextMenu = ({
         open={open}
         onClose={handleMainMenuClose}
       >
-        {Object.keys(options).map(
-          (key) =>
+        {Object.keys(options).map((key) => {
+          const showInPlaylistDisabled =
+            key === 'showInPlaylist' && !playlists.length
+          return (
             options[key].enabled && (
               <MenuItem
                 value={key}
                 key={key}
-                onClick={handleItemClick}
-                disabled={key === 'showInPlaylist' && !playlists.length}
+                onClick={
+                  showInPlaylistDisabled
+                    ? (e) => e.stopPropagation()
+                    : handleItemClick
+                }
+                disabled={showInPlaylistDisabled}
+                style={
+                  showInPlaylistDisabled ? { pointerEvents: 'auto' } : undefined
+                }
               >
                 {options[key].label}
               </MenuItem>
-            ),
-        )}
+            )
+          )
+        })}
       </Menu>
       <Menu
         anchorEl={playlistAnchorEl}
diff --git a/ui/src/common/SongContextMenu.test.jsx b/ui/src/common/SongContextMenu.test.jsx
index ee6a358d..a30da859 100644
--- a/ui/src/common/SongContextMenu.test.jsx
+++ b/ui/src/common/SongContextMenu.test.jsx
@@ -10,6 +10,8 @@ vi.mock('../dataProvider', () => ({
 
 vi.mock('react-redux', () => ({ useDispatch: () => vi.fn() }))
 
+const getPlaylistsMock = vi.fn()
+
 vi.mock('react-admin', async (importOriginal) => {
   const actual = await importOriginal()
   return {
@@ -18,9 +20,7 @@ vi.mock('react-admin', async (importOriginal) => {
       window.location.hash = `#${url}`
     },
     useDataProvider: () => ({
-      getPlaylists: vi.fn().mockResolvedValue({
-        data: [{ id: 'pl1', name: 'Pl 1' }],
-      }),
+      getPlaylists: getPlaylistsMock,
       inspect: vi.fn().mockResolvedValue({
         data: { rawTags: {} },
       }),
@@ -32,6 +32,9 @@ describe('SongContextMenu', () => {
   beforeEach(() => {
     vi.clearAllMocks()
     window.location.hash = ''
+    getPlaylistsMock.mockResolvedValue({
+      data: [{ id: 'pl1', name: 'Pl 1' }],
+    })
   })
 
   it('navigates to playlist when selected', async () => {
@@ -79,4 +82,26 @@ describe('SongContextMenu', () => {
 
     expect(mockOnClick).not.toHaveBeenCalled()
   })
+
+  it('does nothing when "Show in Playlist" is disabled', async () => {
+    getPlaylistsMock.mockResolvedValue({ data: [] })
+    const mockOnClick = vi.fn()
+    render(
+      <TestContext>
+        <div onClick={mockOnClick}>
+          <SongContextMenu record={{ id: 'song1', size: 1 }} resource="song" />
+        </div>
+      </TestContext>,
+    )
+
+    fireEvent.click(screen.getAllByRole('button')[1])
+    await waitFor(() =>
+      screen.getByText(/resources\.song\.actions\.showInPlaylist/),
+    )
+
+    fireEvent.click(
+      screen.getByText(/resources\.song\.actions\.showInPlaylist/),
+    )
+    expect(mockOnClick).not.toHaveBeenCalled()
+  })
 })