From cdd6dd5ca88c11ff98ac0ebb1acb680a7b024a99 Mon Sep 17 00:00:00 2001 From: Tim Hostetler <6970899+thostetler@users.noreply.github.com> Date: Mon, 13 Apr 2026 19:11:55 -0400 Subject: [PATCH 1/3] fix(libraries): prevent adding documents to read-only libraries - Read-only libraries are now visually disabled in the AddToLibraryModal table and cannot be selected - Switch from mutate to mutateAsync so Promise.all correctly waits for the API response; previously it resolved immediately (void), always triggering the success banner regardless of the 403 response - Mock handler returns 403 for any add/remove attempt on a read-only library - Add tests covering selection guard and the regression case --- .../Libraries/AddToLibraryModal.tsx | 14 +- src/components/Libraries/LibraryListTable.tsx | 190 ++++++++++-------- .../__tests__/AddToLibraryModal.test.tsx | 101 ++++++++++ src/mocks/handlers/libraries.ts | 5 + 4 files changed, 216 insertions(+), 94 deletions(-) create mode 100644 src/components/Libraries/__tests__/AddToLibraryModal.test.tsx diff --git a/src/components/Libraries/AddToLibraryModal.tsx b/src/components/Libraries/AddToLibraryModal.tsx index 21c3f95fe..797cf0d6e 100644 --- a/src/components/Libraries/AddToLibraryModal.tsx +++ b/src/components/Libraries/AddToLibraryModal.tsx @@ -58,9 +58,9 @@ export const AddToLibraryModal = ({ const clearSelections = useStore((state) => state.clearSelected); - const { mutate: editDocs, isLoading: isEditingDocs } = useEditLibraryDocuments(); + const { mutateAsync: editDocs, isLoading: isEditingDocs } = useEditLibraryDocuments(); - const { mutate: addDocsByQuery, isLoading: isAddingDocs } = useAddDocumentsByQuery(); + const { mutateAsync: addDocsByQuery, isLoading: isAddingDocs } = useAddDocumentsByQuery(); const toast = useToast(); @@ -171,6 +171,8 @@ const AddToExistingLibraryPane = ({ const libraries = librariesData?.libraries ?? []; + const readOnlyLibIds = libraries.filter((l) => l.permission === 'read').map((l) => l.id); + const entries = librariesData?.count ?? 0; const handleSortChange = (sort: ILibraryListTableSort) => { @@ -188,11 +190,12 @@ const AddToExistingLibraryPane = ({ }; const handleSelectLibrary = (id: LibraryIdentifier) => { + if (readOnlyLibIds.includes(id)) { + return; + } if (selectedLibs.includes(id)) { - // deselect setSelectedLibs((prev) => prev.filter((l) => l !== id)); } else { - // select setSelectedLibs((prev) => [...prev, id]); } }; @@ -231,9 +234,10 @@ const AddToExistingLibraryPane = ({ showIndex={false} showSettings={false} showDescription={false} - hideCols={['public', 'num_users', 'permission', 'date_created']} + hideCols={['public', 'num_users', 'date_created']} selectable selected={selectedLibs} + disabledIds={readOnlyLibIds} onChangeSort={handleSortChange} onChangePageIndex={handlePageIndexChange} onChangePageSize={handlePageSizeChange} diff --git a/src/components/Libraries/LibraryListTable.tsx b/src/components/Libraries/LibraryListTable.tsx index fcedc4e1d..0c1121636 100644 --- a/src/components/Libraries/LibraryListTable.tsx +++ b/src/components/Libraries/LibraryListTable.tsx @@ -110,6 +110,7 @@ export interface ILibraryListTableProps extends TableProps { showDescription?: boolean; selected?: LibraryIdentifier[]; selectable?: boolean; + disabledIds?: LibraryIdentifier[]; onChangeSort: (sort: ILibraryListTableSort) => void; onChangePageIndex: (index: number) => void; onChangePageSize: (size: NumPerPageType) => void; @@ -130,6 +131,7 @@ export const LibraryListTable = (props: ILibraryListTableProps) => { showDescription = true, selected = [], selectable = false, + disabledIds = [], onChangeSort, onChangePageIndex, onChangePageSize, @@ -247,97 +249,107 @@ export const LibraryListTable = (props: ILibraryListTableProps) => { date_last_modified, }, index, - ) => ( - onLibrarySelect(id)} - tabIndex={0} - onKeyDown={(e) => { - if (e.key === 'Enter' || e.key === ' ') { - onLibrarySelect(id); + ) => { + const isDisabled = disabledIds.includes(id); + return ( + - {!isMobile && (selectable || showIndex) && ( - - {showIndex && `${pageSize * pageIndex + index + 1} `} - {selectable && ( - - )} - - )} - {allHiddenCols.indexOf('public') === -1 && ( - - {isPublic ? ( - - - - ) : ( - - - - )} - - )} - {allHiddenCols.indexOf('num_users') === -1 && ( - - {num_users === 1 ? ( - - - - ) : ( - - !isDisabled && onLibrarySelect(id)} + tabIndex={isDisabled ? -1 : 0} + onKeyDown={(e) => { + if (!isDisabled && (e.key === 'Enter' || e.key === ' ')) { + onLibrarySelect(id); + } + }} + role="row" + aria-selected={selected.includes(id)} + aria-disabled={isDisabled} + backgroundColor={selected.includes(id) ? colors.highlightBackground : 'transparent'} + color={selected.includes(id) ? colors.highlightForeground : colors.text} + style={{ backgroundColor: colors.highlightBackground, color: colors.highlightForeground }} + > + {!isMobile && (selectable || showIndex) && ( + + {showIndex && `${pageSize * pageIndex + index + 1} `} + {selectable && ( + - - )} - - )} - {allHiddenCols.indexOf('name') === -1 && ( - - {name} - {showDescription && {description}} - - )} - {allHiddenCols.indexOf('num_documents') === -1 && {num_documents}} - {allHiddenCols.indexOf('owner') === -1 && {owner}} - {allHiddenCols.indexOf('permission') === -1 && {permission}} - {allHiddenCols.indexOf('date_last_modified') === -1 && ( - - - - )} - {showSettings && !isMobile && ( - -
- handleDeleteLibrary(id)} - onSetting={() => handleSettings(id)} - disableDelete={permission !== 'owner'} - /> -
- - )} - - ), + )} + + )} + {allHiddenCols.indexOf('public') === -1 && ( + + {isPublic ? ( + + + + ) : ( + + + + )} + + )} + {allHiddenCols.indexOf('num_users') === -1 && ( + + {num_users === 1 ? ( + + + + ) : ( + + + + )} + + )} + {allHiddenCols.indexOf('name') === -1 && ( + + {name} + {showDescription && {description}} + + )} + {allHiddenCols.indexOf('num_documents') === -1 && {num_documents}} + {allHiddenCols.indexOf('owner') === -1 && {owner}} + {allHiddenCols.indexOf('permission') === -1 && {permission}} + {allHiddenCols.indexOf('date_last_modified') === -1 && ( + + + + )} + {showSettings && !isMobile && ( + +
+ handleDeleteLibrary(id)} + onSetting={() => handleSettings(id)} + disableDelete={permission !== 'owner'} + /> +
+ + )} + + ); + }, )} diff --git a/src/components/Libraries/__tests__/AddToLibraryModal.test.tsx b/src/components/Libraries/__tests__/AddToLibraryModal.test.tsx new file mode 100644 index 000000000..8c1c544e8 --- /dev/null +++ b/src/components/Libraries/__tests__/AddToLibraryModal.test.tsx @@ -0,0 +1,101 @@ +import { render } from '@/test-utils'; +import { describe, expect, test, vi } from 'vitest'; +import { AddToLibraryModal } from '../AddToLibraryModal'; + +const mocks = vi.hoisted(() => ({ + useRouter: vi.fn(() => ({ + query: { q: 'star' }, + asPath: '/search?q=star', + push: vi.fn(), + events: { on: vi.fn(), off: vi.fn() }, + })), +})); + +vi.mock('next/router', () => ({ useRouter: mocks.useRouter })); + +describe('AddToLibraryModal', () => { + const defaultProps = { + bibcodes: ['2021ApJ...000A...1X'], + isOpen: true, + onClose: vi.fn(), + }; + + test('read-only library cannot be selected', async () => { + const { user, findByRole, findByText } = render(); + + // Wait for libraries to load — the table should appear + await findByRole('table'); + + // Library "003" from mock data has permission "read" — its name is "003" + const readOnlyRow = await findByText('003'); + await user.click(readOnlyRow); + + // Submit button should still be disabled because nothing was selected + const submitButton = await findByRole('button', { name: /submit/i }); + expect(submitButton).toBeDisabled(); + }); + + test('read-only library row is visually marked as disabled', async () => { + const { findByRole, findAllByRole } = render(); + + await findByRole('table'); + + const rows = await findAllByRole('row'); + // Find the row for library "003" (read-only) + const readOnlyRow = rows.find( + (row) => row.textContent?.includes('003') && row.getAttribute('aria-disabled') === 'true', + ); + expect(readOnlyRow).toBeDefined(); + }); + + test('writable library can be selected and enables submit', async () => { + const { user, findByRole, findByText } = render(); + + await findByRole('table'); + + // Library "001" has permission "admin" — should be selectable + const writableRow = await findByText('001'); + await user.click(writableRow); + + const submitButton = await findByRole('button', { name: /submit/i }); + expect(submitButton).not.toBeDisabled(); + }); + + test('does not invoke success callback when adding to a library returns 403', async () => { + // This tests the mutateAsync fix: Promise.all should reject when API returns 403, + // so onClose(true) must NOT be called — previously it was called immediately because + // mutate() returned void (not a Promise), causing Promise.all to resolve right away. + const { server } = await import('@/mocks/server'); + const { rest } = await import('msw'); + const { ApiTargets } = await import('@/api/models'); + const onClose = vi.fn(); + + server.use( + rest.post(`*${ApiTargets.DOCUMENTS}/:id`, (_req, res, ctx) => { + return res(ctx.status(403), ctx.json({ error: 'Insufficient permissions' })); + }), + ); + + const { user, findByRole, findByText, queryByText } = render( + , + ); + + await findByRole('table'); + + // Select writable library "001" + const writableRow = await findByText('001'); + await user.click(writableRow); + + const submitButton = await findByRole('button', { name: /submit/i }); + await user.click(submitButton); + + // Allow async mutations to settle + await new Promise((r) => setTimeout(r, 100)); + + // Success callback (onClose(true)) must NOT have been called + expect(onClose).not.toHaveBeenCalledWith(true); + + // Success banner text must NOT appear anywhere + expect(queryByText(/paper\(s\) added/i)).not.toBeInTheDocument(); + }); +}); diff --git a/src/mocks/handlers/libraries.ts b/src/mocks/handlers/libraries.ts index 9e96cfe89..d638d2e6e 100644 --- a/src/mocks/handlers/libraries.ts +++ b/src/mocks/handlers/libraries.ts @@ -168,6 +168,11 @@ export const librariesHandlers = [ apiHandlerRoute(ApiTargets.DOCUMENTS, '/:id'), async (req, res, ctx) => { const id = req.params.id; + const library = libraries.find((l) => l.id === id); + + if (library?.permission === 'read') { + return res(ctx.status(403), ctx.json({ error: 'Insufficient permissions' })); + } if (req.body.action === 'remove') { // remove docs From 9a1ffcf83aa6b6debbfb65ed2ecb85dd887765e3 Mon Sep 17 00:00:00 2001 From: Tim Hostetler <6970899+thostetler@users.noreply.github.com> Date: Mon, 13 Apr 2026 19:46:29 -0400 Subject: [PATCH 2/3] fix(libraries): restore hidden permission column in add-to-library modal --- src/components/Libraries/AddToLibraryModal.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/Libraries/AddToLibraryModal.tsx b/src/components/Libraries/AddToLibraryModal.tsx index 797cf0d6e..e7f6bb6a1 100644 --- a/src/components/Libraries/AddToLibraryModal.tsx +++ b/src/components/Libraries/AddToLibraryModal.tsx @@ -234,7 +234,7 @@ const AddToExistingLibraryPane = ({ showIndex={false} showSettings={false} showDescription={false} - hideCols={['public', 'num_users', 'date_created']} + hideCols={['public', 'num_users', 'permission', 'date_created']} selectable selected={selectedLibs} disabledIds={readOnlyLibIds} From 7029529c2f7b472810670110a5a96528fb2adbec Mon Sep 17 00:00:00 2001 From: Tim Hostetler <6970899+thostetler@users.noreply.github.com> Date: Mon, 13 Apr 2026 19:51:53 -0400 Subject: [PATCH 3/3] fix(libraries): address review feedback on row styling and aria labels - Remove unconditional inline style from Tr that overrode conditional backgroundColor/color props, causing disabled rows to appear highlighted - Fix malformed aria-label template string on selectable Checkbox - Replace flaky setTimeout in test with waitFor on submit button loading state --- src/components/Libraries/LibraryListTable.tsx | 3 +-- .../Libraries/__tests__/AddToLibraryModal.test.tsx | 7 ++++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/components/Libraries/LibraryListTable.tsx b/src/components/Libraries/LibraryListTable.tsx index 0c1121636..067258370 100644 --- a/src/components/Libraries/LibraryListTable.tsx +++ b/src/components/Libraries/LibraryListTable.tsx @@ -273,7 +273,6 @@ export const LibraryListTable = (props: ILibraryListTableProps) => { aria-disabled={isDisabled} backgroundColor={selected.includes(id) ? colors.highlightBackground : 'transparent'} color={selected.includes(id) ? colors.highlightForeground : colors.text} - style={{ backgroundColor: colors.highlightBackground, color: colors.highlightForeground }} > {!isMobile && (selectable || showIndex) && ( @@ -284,7 +283,7 @@ export const LibraryListTable = (props: ILibraryListTableProps) => { isDisabled={isDisabled} pointerEvents="none" tabIndex={-1} - aria-label={`library ${selected.includes(id)} ? 'selected' ? 'not selected`} + aria-label={selected.includes(id) ? 'library selected' : 'library not selected'} /> )} diff --git a/src/components/Libraries/__tests__/AddToLibraryModal.test.tsx b/src/components/Libraries/__tests__/AddToLibraryModal.test.tsx index 8c1c544e8..18a106985 100644 --- a/src/components/Libraries/__tests__/AddToLibraryModal.test.tsx +++ b/src/components/Libraries/__tests__/AddToLibraryModal.test.tsx @@ -1,4 +1,4 @@ -import { render } from '@/test-utils'; +import { render, waitFor } from '@/test-utils'; import { describe, expect, test, vi } from 'vitest'; import { AddToLibraryModal } from '../AddToLibraryModal'; @@ -89,8 +89,9 @@ describe('AddToLibraryModal', () => { const submitButton = await findByRole('button', { name: /submit/i }); await user.click(submitButton); - // Allow async mutations to settle - await new Promise((r) => setTimeout(r, 100)); + // Wait for the mutation to settle — the button exits its loading state once + // the 403 response is received and the error handler runs + await waitFor(() => expect(submitButton).not.toBeDisabled()); // Success callback (onClose(true)) must NOT have been called expect(onClose).not.toHaveBeenCalledWith(true);