feat: support deletion of code list from library #14419
Conversation
standeren
added
skip-releasenotes
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive implementation for deleting option lists across the frontend application. The changes span multiple files and components, adding a new mutation hook Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
solution/studio/designer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
frontend/libs/studio-content-library/mocks/mockPagesConfig.ts (1)
15-15: Consider adding a Jest mock function for better test assertions.While the empty function is a valid mock, using
jest.fn()would allow for better test assertions by enabling verification of whether and how the delete callback is called during tests.- onDeleteCodeList: () => {}, + onDeleteCodeList: jest.fn(),frontend/packages/shared/src/hooks/mutations/useDeleteOptionListMutation.ts (2)
6-20: Consider adding error handling and loading states.While the mutation implementation is solid, it would benefit from explicit error handling in the
onErrorcallback to manage failed deletions gracefully.return useMutation({ mutationFn: (optionListId: string) => deleteOptionList(org, app, optionListId).then(() => optionListId), onSuccess: (optionListId) => { setOptionListIdsQueryCache(queryClient, org, app, optionListId); void queryClient.invalidateQueries({ queryKey: [QueryKey.OptionLists, org, app] }); void queryClient.removeQueries({ queryKey: [QueryKey.OptionList, org, app, optionListId] }); }, + onError: (error) => { + console.error('Failed to delete option list:', error); + // Consider adding a toast notification or error state management here + }, meta, });
22-37: Optimize cache update logic.The cache update function could be more efficient by using a single operation instead of separate get and set operations.
-const setOptionListIdsQueryCache = ( +const updateOptionListIdsCache = ( queryClient: QueryClient, org: string, app: string, optionListId: string, ) => { - const currentOptionListIds = queryClient.getQueryData<string[]>([ - QueryKey.OptionListIds, - org, - app, - ]); - if (currentOptionListIds) { - const updatedOptionListIds = currentOptionListIds.filter((id) => id !== optionListId); - void queryClient.setQueryData([QueryKey.OptionListIds, org, app], updatedOptionListIds); - } + queryClient.setQueryData<string[]>( + [QueryKey.OptionListIds, org, app], + (old) => old?.filter((id) => id !== optionListId) ?? [] + ); };frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.tsx (1)
25-25: Consider making the callback more type-safe.The delete callback could benefit from a more specific type definition to ensure type safety throughout the component tree.
- onDeleteCodeList: (codeListId: string) => void; + onDeleteCodeList: (codeListId: string) => Promise<void>;frontend/packages/shared/src/hooks/mutations/useDeleteOptionListMutation.test.ts (1)
11-62: Enhance test coverage with edge cases.While the current tests are good, consider adding the following scenarios:
- Error handling tests
- Empty cache scenarios
- Race condition tests for concurrent deletions
Add these test cases:
test('Handles API errors correctly', async () => { queriesMock.deleteOptionList.mockRejectedValueOnce(new Error('API Error')); const { result } = renderHookWithProviders(() => useDeleteOptionListMutation(org, app) ); await expect(result.current.mutateAsync(optionsListId)) .rejects.toThrow('API Error'); }); test('Handles empty cache gracefully', async () => { const queryClient = createQueryClientMock(); const { result } = renderHookWithProviders( () => useDeleteOptionListMutation(org, app), { queryClient } ); await result.current.mutateAsync(optionsListId); expect(queryClient.getQueryData([QueryKey.OptionListIds, org, app])) .toEqual([]); });frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.tsx (1)
24-24: Consider documenting prop dependencies.While using the spread operator (
...rest) reduces prop drilling, it makes it harder to track which props are required by child components. Consider documenting the expected props in each component or using more explicit prop passing.Also applies to: 32-32, 48-48, 60-60, 114-114, 125-125
frontend/language/src/nb.json (1)
18-20: Fix typo in Norwegian translation.There's a typo in the translation key
app_content_library.code_lists.code_list_delete_enabled_title:
- "bibliotket" should be "biblioteket"
- "app_content_library.code_lists.code_list_delete_enabled_title": "Slett kodelisten fra bibliotket.", + "app_content_library.code_lists.code_list_delete_enabled_title": "Slett kodelisten fra biblioteket.",frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.tsx (1)
93-97: Consider adding error boundaries and type-safe translations.The
CodeListButtonscomponent is well-implemented with proper prop types and conditional rendering. However, consider these improvements:
- Wrap the component with an error boundary to gracefully handle potential rendering failures
- Consider using type-safe translation keys to prevent potential runtime errors
Example implementation:
// Create a type-safe translation key type type CodeListTranslationKey = | 'app_content_library.code_lists.code_list_delete_disabled_title' | 'app_content_library.code_lists.code_list_delete_enabled_title' | 'app_content_library.code_lists.code_list_delete'; // Add error boundary class CodeListButtonsErrorBoundary extends React.Component<{children: React.ReactNode}> { // ... implement error boundary methods } // Wrap the component function CodeListButtons({...props}: CodeListButtonsProps): React.ReactElement { return ( <CodeListButtonsErrorBoundary> {/* existing implementation */} </CodeListButtonsErrorBoundary> ); }Also applies to: 109-137
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx(3 hunks)frontend/language/src/nb.json(1 hunks)frontend/libs/studio-content-library/mocks/mockPagesConfig.ts(1 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.test.tsx(2 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.tsx(3 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.test.tsx(4 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.tsx(6 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.module.css(1 hunks)frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.tsx(5 hunks)frontend/packages/shared/src/api/mutations.ts(2 hunks)frontend/packages/shared/src/api/paths.js(1 hunks)frontend/packages/shared/src/hooks/mutations/index.ts(1 hunks)frontend/packages/shared/src/hooks/mutations/useDeleteOptionListMutation.test.ts(1 hunks)frontend/packages/shared/src/hooks/mutations/useDeleteOptionListMutation.ts(1 hunks)frontend/packages/shared/src/mocks/queriesMock.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.module.css
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (12)
frontend/packages/shared/src/hooks/mutations/index.ts (1)
2-2: LGTM!The export follows the established pattern and maintains alphabetical ordering.
frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx (1)
17-17: LGTM! Clean integration of delete functionality.The changes correctly integrate the delete functionality by:
- Adding the delete mutation hook
- Passing the delete handler to the ResourceContentLibraryImpl component
Also applies to: 28-28, 70-70
frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.tsx (1)
13-13: LGTM! Proper type definition for delete functionality.The onDeleteCodeList prop is correctly typed and integrated into the component's props interface.
frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.test.tsx (1)
11-11: Add test coverage for delete functionality.While the delete mock is properly set up, consider adding specific test cases to verify:
- Delete button behavior
- Error handling during deletion
- UI feedback during deletion
Would you like me to generate the additional test cases?
Also applies to: 155-155
frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.test.tsx (1)
141-159: LGTM! Comprehensive test coverage for delete functionality.The tests thoroughly cover:
- Delete button disabled state when code list is in use
- Delete button enabled state with correct titles
- Proper function calls with correct parameters
Also applies to: 231-243, 265-265
frontend/packages/shared/src/api/mutations.ts (1)
50-50: LGTM! The new mutation follows the established patterns.The
deleteOptionListmutation is well-implemented, following the same patterns as other delete operations in the file.Also applies to: 90-90
frontend/packages/shared/src/api/paths.js (1)
45-45: LGTM! Documentation update is accurate.The comment update correctly reflects that the endpoint now supports both Get and Delete operations.
frontend/packages/shared/src/mocks/queriesMock.ts (1)
223-223: LGTM! Mock implementation is consistent.The
deleteOptionListmock is properly implemented using jest.fn() and follows the established patterns in the file.frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.tsx (4)
1-6: LGTM! Well-organized imports and properly typed props.The imports are logically grouped, and the new
onDeleteCodeListprop is well-typed with a clear function signature.Also applies to: 22-22
32-32: LGTM! Clean and focused implementation.The delete handler follows the single responsibility principle and correctly passes the code list identifier.
Also applies to: 60-60
104-104: LGTM! Type update maintains consistency.The type change to
CodeListaligns with the import changes while maintaining immutable state updates.
Line range hint
1-137: Verify the deletion functionality's impact across the application.The implementation looks solid, but let's verify:
- The deletion state management in parent components
- The presence of appropriate confirmation dialogs
- The cleanup of related resources post-deletion
✅ Verification successful
Deletion functionality is properly implemented with appropriate safeguards
The implementation includes:
- Built-in confirmation dialog via StudioDeleteButton
- Prevention of deletion for code lists in use
- Clear user feedback through disabled states and tooltips
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for delete-related state management and confirmation dialogs rg -l "onDeleteCodeList|handleDeleteCodeList|deleteCodeList" frontend/libs/studio-content-library/src/Length of output: 671
Script:
#!/bin/bash # Check the implementation details echo "=== CodeListPage.tsx ===" rg "deleteCodeList|handleDeleteCodeList" -A 5 frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.tsx echo -e "\n=== StudioDeleteButton Implementation ===" ast-grep --pattern 'export function StudioDeleteButton' -l rg "StudioDeleteButton" -A 10 frontend/libs/studio-components/src/Length of output: 16383
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14419 +/- ##
=======================================
Coverage 95.66% 95.66%
=======================================
Files 1879 1880 +1
Lines 24405 24427 +22
Branches 2807 2809 +2
=======================================
+ Hits 23346 23368 +22
Misses 799 799
Partials 260 260 ☔ View full report in Codecov by Sentry. |
...io-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.tsx
Outdated
Show resolved
Hide resolved
...ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.module.css
Outdated
Show resolved
Hide resolved
...ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/EditCodeList/EditCodeList.module.css
Outdated
Show resolved
Hide resolved
Description
Add support for deletion of code lists (that are not in use) in the content library.
Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style