feat: 14508/delete code list for org #14630
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughA new organization-level code list management feature is introduced. A backend controller with complete CRUD operations is added along with its service interface and implementation. Several controllers and Git repository methods are updated to accept a dynamic options folder path. On the frontend, multiple components, hooks, and tests are refactored with new subroute support and improved routing. Additionally, the shared package now includes new API endpoints, hooks, queries, type definitions, and mock data for managing both code lists and text resources. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
area/dashboard
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 8
🧹 Nitpick comments (46)
frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.ts (1)
12-17: Consider enhancing error handling and cache management.The mutation could be more robust with these improvements:
- Add error handling with
onErrorcallback- Consider using cache invalidation instead of direct updates
- Add type safety for the title parameter
Here's a suggested implementation:
+type DeleteCodeListParams = { + title: string; +}; export const useDeleteOrgCodeListMutation = (org: string) => { const queryClient = useQueryClient(); const { deleteCodeListForOrg } = useServicesContext(); - const mutationFn = (title: string) => deleteCodeListForOrg(org, title); + const mutationFn = ({ title }: DeleteCodeListParams) => deleteCodeListForOrg(org, title); return useMutation({ mutationFn, - onSuccess: (newData: CodeListsResponse) => { - queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); + onSuccess: () => { + queryClient.invalidateQueries([QueryKey.OrgCodeLists, org]); }, + onError: (error) => { + console.error('Failed to delete code list:', error); + // Handle error (e.g., show toast notification) + } }); };frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.test.tsx (4)
34-39: Consider parameterizing the useParams mock.The current useParams mock returns a static value. Consider making it configurable to test different organization contexts.
jest.mock('react-router-dom', () => ({ ...jest.requireActual('react-router-dom'), - useParams: () => ({ - selectedContext: 'testOrg', - }), + useParams: jest.fn().mockReturnValue({ + selectedContext: 'testOrg', + }), }));
72-80: Add test coverage for error scenarios.The deletion test only covers the success path. Consider adding test cases for:
- Failed deletion (API error)
- Cancellation of deletion
- Empty code list name
Example test case for error handling:
it('handles deletion error gracefully', async () => { const user = userEvent.setup(); const error = new Error('Failed to delete'); queriesMock.deleteCodeListForOrg.mockRejectedValueOnce(error); renderOrgContentLibraryWithCodeLists(); await goToLibraryPage(user, 'code_lists'); const deleteButton = screen.getByRole('button', { name: deleteCodeListButtonTextMock }); await user.click(deleteButton); expect(queriesMock.deleteCodeListForOrg).toHaveBeenCalledTimes(1); // Add expectations for error handling UI elements });
91-95: Enhance type definition with more specific types.The queries type could be more specific by explicitly listing the expected query methods.
type RenderOrgContentLibraryProps = { - queries?: Partial<ServicesContextProps>; + queries?: Pick<ServicesContextProps, 'deleteCodeListForOrg' | /* other specific methods */>; queryClient?: QueryClient; };
113-119: Add error handling for query client setup.The function should handle undefined codeListDataList gracefully and validate the data structure.
function createQueryClientWithOptionsDataList( codeListDataList: CodeListData[] | undefined, ): QueryClient { const queryClient = createQueryClientMock(); + if (!codeListDataList?.length) { + return queryClient; + } + if (!codeListDataList.every(list => list.title && Array.isArray(list.data))) { + throw new Error('Invalid code list data structure'); + } queryClient.setQueryData([QueryKey.OrgCodeLists, org], codeListDataList); return queryClient; }frontend/dashboard/app/App.tsx (2)
25-39: Consider adding a default case for the switch statement.
AlthoughmergeQueryStatuseslikely returns'pending','error', or'success', adding adefault:branch could handle unexpected statuses (e.g.,'idle'). This ensures the component always returns a valid JSX element.
92-103: Handle unknown subroutes with a fallback.
Currently, there is no fallback for subroutes that do not match the known basenames. Consider adding a default case returning a 404-like component or redirect.backend/src/Designer/Controllers/Organisation/CodeListController.cs (3)
36-58: Returning 204 No Content on NotFoundException might obscure actual errors.
Consider distinguishing genuinely empty code lists from actual file-not-found issues, e.g., return an empty array (200 OK) vs. 204 No Content or 404. This clarifies responses to consumers and reduces ambiguity.
103-126: Potential large file upload handling.
While the current approach is valid, consider future improvements for handling extremely large uploads, such as file size checks or streaming validations.
128-150: Deleting code list on success returns a 200 with plain text.
A 204 (No Content) is commonly used for successful deletions, but returning 200 with a custom message is also acceptable. This is more of a style preference than a bug.backend/src/Designer/Services/Implementation/Organisation/CodeListService.cs (1)
134-152: Upload from file is well-handled with basic validations.
Verifying label and value are present is good, though consider potential memory constraints on large files.frontend/dashboard/hooks/useSubRoute/useSubroute.ts (1)
5-8: LGTM! Consider adding JSDoc comments.The hook implementation is clean and follows React hooks best practices.
Consider adding JSDoc comments to document the hook's purpose and return value:
+/** + * Custom hook to get the current subroute from URL parameters. + * @returns {string} The current subroute or default subroute if not specified. + */ export function useSubroute(): string { const { subroute = defaultSubroute } = useParams(); return subroute; }frontend/dashboard/pages/PageLayout/DashboardHeader/dashboardHeaderMenuItems.ts (2)
3-6: Consider making the enum public.The
HeaderMenuItemKeyenum could be useful in other components for type-safe key references.Apply this diff:
-enum HeaderMenuItemKey { +export enum HeaderMenuItemKey { OrgLibrary = 'orgLibrary', AppDashboard = 'appDashboard', }
8-12: Consider using i18n type for the name property.The
nameproperty appears to be an i18n key. Consider using a type to ensure type safety for translation keys.Apply this diff if you have a type for i18n keys:
export interface HeaderMenuItem { key: HeaderMenuItemKey; link: string; - name: string; + name: TranslationKey; // Replace with your i18n key type }frontend/packages/shared/src/hooks/queries/useOrgCodeListsQuery.test.ts (1)
7-12: Add more comprehensive test cases.While the current test verifies the basic functionality, consider adding test cases for:
- Error handling
- Loading state
- Returned data validation
Example test cases to add:
it('handles error states', async () => { queriesMock.getCodeListsForOrg.mockRejectedValueOnce(new Error('Failed to fetch')); const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); await waitFor(() => expect(result.current.isError).toBe(true)); expect(result.current.error.message).toBe('Failed to fetch'); }); it('shows loading state', () => { const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); expect(result.current.isLoading).toBe(true); }); it('returns the fetched data', async () => { const mockData = { codeLists: [] }; queriesMock.getCodeListsForOrg.mockResolvedValueOnce(mockData); const { result } = renderHookWithProviders(() => useOrgCodeListsQuery(org)); await waitFor(() => expect(result.current.isSuccess).toBe(true)); expect(result.current.data).toEqual(mockData); });frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.ts (2)
7-7: Consider using a more descriptive variable name.The variable name 'q' is not as descriptive as it could be. Consider using 'queryClient' for better readability and consistency with other hooks.
- const q = useQueryClient(); + const queryClient = useQueryClient();
9-18: Add error handling and cache invalidation.The mutation lacks error handling and might benefit from cache invalidation instead of direct cache updates. Consider:
- Adding an onError callback to handle failures gracefully
- Using invalidateQueries instead of setQueryData to ensure stale data is properly handled
return useMutation({ mutationFn: async () => { const textResourcesWithLanguage: ITextResourcesWithLanguage[] = await createTextResourcesForOrg(org, language); return textResourcesWithLanguage; }, - onSuccess: (textResourcesWithLanguage) => - q.setQueryData([QueryKey.TextResourcesForOrg, org], textResourcesWithLanguage), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: [QueryKey.TextResourcesForOrg, org] }); + }, + onError: (error: Error) => { + console.error('Failed to create text resources:', error); + // Handle error appropriately (e.g., show notification) + }, });frontend/packages/shared/src/hooks/mutations/useUploadOrgCodeListMutation.ts (1)
16-21: Add error handling and cache invalidation.Similar to the text resources mutation, this hook would benefit from error handling and cache invalidation.
return useMutation({ mutationFn, - onSuccess: (newData: CodeListsResponse) => { - queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: [QueryKey.OrgCodeLists, org] }); }, + onError: (error: Error) => { + console.error('Failed to upload code list:', error); + // Handle error appropriately (e.g., show notification) + }, });frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.ts (1)
6-19: Apply consistent improvements with other hooks.For consistency with other hooks, consider:
- Using 'queryClient' instead of 'q'
- Adding error handling
- Using cache invalidation
export const useUpdateTextResourcesForOrgMutation = (org: string, language: string) => { - const q = useQueryClient(); + const queryClient = useQueryClient(); const { updateTextResourcesForOrg } = useServicesContext(); return useMutation({ mutationFn: async (payload: ITextResource[]) => { const textResourcesWithLanguage: ITextResourcesWithLanguage[] = await updateTextResourcesForOrg(org, language, payload); return textResourcesWithLanguage; }, - onSuccess: (textResourcesWithLanguage) => - q.setQueryData([QueryKey.TextResourcesForOrg, org], textResourcesWithLanguage), + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: [QueryKey.TextResourcesForOrg, org] }); + }, + onError: (error: Error) => { + console.error('Failed to update text resources:', error); + // Handle error appropriately (e.g., show notification) + }, }); };frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.ts (1)
16-21: Add error handling and cache invalidation.For consistency with other hooks, consider adding error handling and using cache invalidation.
return useMutation({ mutationFn, - onSuccess: (newData: CodeListsResponse) => { - queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: [QueryKey.OrgCodeLists, org] }); }, + onError: (error: Error) => { + console.error('Failed to create code list:', error); + // Handle error appropriately (e.g., show notification) + }, });frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.ts (1)
9-22: Consider adding error handling.While the mutation is well-implemented, it would benefit from error handling to manage failed updates gracefully.
Consider adding
onErrorcallback:return useMutation({ mutationFn, onSuccess: (newData: CodeListsResponse) => { queryClient.setQueryData([QueryKey.OrgCodeLists, org], newData); }, + onError: (error) => { + // Handle error (e.g., show error notification) + console.error('Failed to update code list:', error); + }, });frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx (1)
11-23: Add type safety to ResourceContentLibraryImpl configuration.The configuration object would benefit from explicit type annotations.
Consider adding type annotations:
-const { getContentResourceLibrary } = new ResourceContentLibraryImpl({ +type ContentLibraryConfig = { + pages: { + codeList: { + props: { + codeListsData: CodeListData[]; + onDeleteCodeList: (id: string) => void; + onUpdateCodeListId: (id: string) => void; + onUpdateCodeList: (data: CodeListData) => void; + onUploadCodeList: (file: File) => void; + }; + }; + }; +}; + +const { getContentResourceLibrary } = new ResourceContentLibraryImpl<ContentLibraryConfig>({frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.test.ts (1)
9-17: Add test cases for error scenarios and edge cases.While the happy path is tested, consider adding test cases for:
- Error handling when the API call fails
- Edge cases with empty or invalid language codes
- Loading state verification
Example test case for error handling:
it('handles API errors gracefully', async () => { queriesMock.getTextResourcesForOrg.mockRejectedValueOnce(new Error('API Error')); const { result } = renderHookWithProviders(() => useTextResourcesForOrgQuery(org, languageMock) ); await waitFor(() => expect(result.current.isError).toBe(true)); expect(result.current.error).toBeDefined(); });frontend/packages/shared/src/hooks/mutations/useCreateTextResourcesForOrgMutation.test.ts (1)
12-22: Enhance test coverage with payload validation and error scenarios.The current test verifies the function call but lacks:
- Validation of the actual mutation payload
- Error handling scenarios
- Loading state verification
Example test cases:
it('handles API errors gracefully', async () => { queriesMock.createTextResourcesForOrg.mockRejectedValueOnce(new Error('API Error')); const { result } = renderHookWithProviders(() => useCreateTextResourcesForOrgMutation(org, languageMock) ); result.current.mutate(); await waitFor(() => expect(result.current.isError).toBe(true)); expect(result.current.error).toBeDefined(); }); it('sets loading state during mutation', async () => { const { result } = renderHookWithProviders(() => useCreateTextResourcesForOrgMutation(org, languageMock) ); result.current.mutate(); expect(result.current.isLoading).toBe(true); await waitFor(() => expect(result.current.isSuccess).toBe(true)); });frontend/packages/shared/src/hooks/mutations/useUpdateTextResourcesForOrgMutation.test.ts (1)
12-28: Enhance test structure and coverage.Consider the following improvements:
- Add
beforeEachcleanup- Test error scenarios
- Test with different text resource variations
describe('useUpdateTextResourcesForOrgMutation', () => { beforeEach(jest.clearAllMocks); it('Calls updateTextResourcesForOrg with correct arguments and payload', async () => { // ... existing test ... }); it('handles API errors gracefully', async () => { queriesMock.updateTextResourcesForOrg.mockRejectedValueOnce(new Error('API Error')); const { result } = renderHookWithProviders(() => useUpdateTextResourcesForOrgMutation(org, languageMock) ); result.current.mutate(textResourcesMock); await waitFor(() => expect(result.current.isError).toBe(true)); expect(result.current.error).toBeDefined(); }); it('handles empty text resources array', async () => { const { result } = renderHookWithProviders(() => useUpdateTextResourcesForOrgMutation(org, languageMock) ); result.current.mutate([]); await waitFor(() => expect(result.current.isSuccess).toBe(true)); expect(queriesMock.updateTextResourcesForOrg).toHaveBeenCalledWith(org, languageMock, []); }); });frontend/packages/shared/src/mocks/codeListsResponse.ts (1)
42-45: Consider adding error details to the mock.The
codeListWithErrormock could be enhanced with specific error details to better simulate error scenarios.const codeListWithError: CodeListData = { title: 'codeListWithError', hasError: true, + errorMessage: 'Failed to parse code list', + errorCode: 'PARSE_ERROR' };frontend/packages/shared/src/hooks/mutations/useUploadOrgCodeListMutation.test.ts (2)
13-22: Consider testing edge cases in file data.The test data only covers the happy path. Consider adding test cases for:
- Empty code list
- Invalid JSON format
- Missing required fields
const fileData: CodeList = [ { value: 'test-value', label: 'test-label', }, ]; +const emptyFileData: CodeList = []; +const invalidFileData = 'invalid-json'; +const incompleteFileData: CodeList = [{ value: 'test-value' }];
41-54: Consider testing error scenarios.The cache update test only covers successful scenarios. Consider adding test cases for error handling and cache invalidation on failure.
+ it('Handles errors correctly', async () => { + const queryClient = createQueryClientMock(); + const error = new Error('Upload failed'); + const uploadCodeListForOrg = jest.fn(() => Promise.reject(error)); + const { result } = renderHookWithProviders(() => useUploadOrgCodeListMutation(org), { + queryClient, + queries: { uploadCodeListForOrg }, + }); + + await expect(result.current.mutateAsync(file)).rejects.toThrow(error); + expect(queryClient.getQueryData([QueryKey.OrgCodeLists, org])).toBeUndefined(); + });frontend/packages/shared/src/hooks/mutations/useUpdateOrgCodeListMutation.test.ts (2)
12-27: Consider testing more code list variations.The test data could be expanded to include:
- Code lists with all optional fields
- Code lists with special characters in title
- Code lists with different data structures
+const complexCodeList: CodeListData = { + title: 'complex-title-with-special-chars-!@#$', + data: [ + { + value: 'test-value', + label: 'test-label', + description: 'test-description', + helpText: 'test-help-text' + } + ] +};
43-57: Consider testing optimistic updates.The cache update test could be enhanced to verify optimistic updates and rollback behavior.
+ it('Handles optimistic updates and rollbacks', async () => { + const queryClient = createQueryClientMock(); + queryClient.setQueryData([QueryKey.OrgCodeLists, org], [oldCodeList]); + const error = new Error('Update failed'); + const updateCodeListForOrg = jest.fn(() => Promise.reject(error)); + const { result } = renderHookWithProviders(() => useUpdateOrgCodeListMutation(org), { + queryClient, + queries: { updateCodeListForOrg }, + }); + + await expect(result.current.mutateAsync(updatedCodeList)).rejects.toThrow(error); + const cachedData = queryClient.getQueryData([QueryKey.OrgCodeLists, org]); + expect(cachedData).toEqual([oldCodeList]); + });frontend/packages/shared/src/hooks/mutations/useCreateOrgCodeListMutation.test.ts (2)
29-43: Add error handling test cases.The test suite should include cases for error scenarios to ensure robust error handling.
Consider adding these test cases:
it('Handles API errors gracefully', async () => { const error = new Error('API Error'); const createCodeListForOrg = jest.fn(() => Promise.reject(error)); const { result } = renderHookWithProviders(() => useCreateOrgCodeListMutation(org), { queries: { createCodeListForOrg }, }); await expect(result.current.mutateAsync(newCodeList)).rejects.toThrow(error); }); it('Handles invalid input data', async () => { const { result } = renderHookWithProviders(() => useCreateOrgCodeListMutation(org)); const invalidData = { title: '', data: [] }; await expect(result.current.mutateAsync(invalidData)).rejects.toThrow(); });
45-59: Add test for optimistic updates.The cache update test could be enhanced to verify optimistic updates.
Consider adding this test case:
it('Updates cache optimistically', async () => { const queryClient = createQueryClientMock(); queryClient.setQueryData([QueryKey.OrgCodeLists, org], [existingCodeList]); const createCodeListForOrg = jest.fn(() => new Promise((resolve) => setTimeout(resolve, 100))); const { result } = renderHookWithProviders(() => useCreateOrgCodeListMutation(org), { queryClient, queries: { createCodeListForOrg }, }); const promise = result.current.mutateAsync(newCodeList); const immediateData = queryClient.getQueryData([QueryKey.OrgCodeLists, org]); expect(immediateData).toContain(newCodeList); await promise; });frontend/packages/shared/src/mocks/textResourcesMock.ts (1)
3-64: Refactor repetitive text resource definitions.The current implementation has repetitive code for similar text resources. Consider using a factory function or array mapping for better maintainability.
Here's a suggested refactor:
const createTextResources = ( prefix: string, count: number, valuePrefix: string ): Record<string, ITextResource> => Array.from({ length: count }, (_, i) => i + 1).reduce( (acc, num) => ({ ...acc, [`${prefix}${num}TextResource`]: { id: `${prefix}${num}`, value: `${valuePrefix} ${num}`, }, }), {} ); export const { label1TextResource, label2TextResource, label3TextResource, label4TextResource, label5TextResource, } = createTextResources('label', 5, 'Ledetekst'); export const { description1TextResource, description2TextResource, description3TextResource, description4TextResource, description5TextResource, } = createTextResources('description', 5, 'Beskrivelse'); export const { helpText1TextResource, helpText2TextResource, helpText3TextResource, helpText4TextResource, helpText5TextResource, } = createTextResources('helpText', 5, 'Hjelpetekst');frontend/packages/shared/src/types/QueryKey.ts (1)
33-34: Group related query keys together.The new keys
OrgCodeListsandTextResourcesForOrgshould be grouped with related organization and text resource keys for better maintainability.Apply this organization:
OptionListIds = 'OptionListIds', - OrgCodeLists = 'OrgCodeLists', OrgList = 'OrgList', Organizations = 'Organizations', + OrgCodeLists = 'OrgCodeLists', ProcessTaskDataType = 'ProcessTaskDataType', // ... TextLanguages = 'TextLanguages', TextResources = 'TextResources', + TextResourcesForOrg = 'TextResourcesForOrg', Widgets = 'Widgets', - TextResourcesForOrg = 'TextResourcesForOrg',Also applies to: 54-54
frontend/dashboard/pages/CreateService/CreateService.tsx (1)
91-91: Consider validating subroute before href construction.The href construction looks good, but consider validating that both
subrouteandselectedContextare non-empty strings to avoid potential "///" in the URL.- href: `/${subroute}/${selectedContext}`, + href: `/${[subroute, selectedContext].filter(Boolean).join('/')}`,frontend/libs/studio-pure-functions/src/StringUtils/StringUtils.ts (1)
89-89: Enhance removeLeadingSlash to handle multiple leading slashes.The current regex
/^\//gonly removes a single leading slash. Consider updating it to handle multiple leading slashes.- static removeLeadingSlash = (str: string): string => str.replace(/^\//g, ''); + static removeLeadingSlash = (str: string): string => str.replace(/^\/+/g, '');frontend/dashboard/hooks/guards/useContextRedirectionGuard.ts (2)
104-107: Move DashboardRoute type definition to the top.Consider moving the
DashboardRoutetype definition to the top of the file with other type definitions for better organization and readability.-type DashboardRoute = { - selectedContextType: string; - subroute: string; -}; +// Move this block to line 14, just after UseRedirectionGuardResult
54-57: Add JSDoc comments to helper functions.Consider adding JSDoc comments to helper functions to improve code documentation and maintainability.
+/** + * Checks if the user has access to the target context. + * @param selectedContextType - The type of context to check. + * @param organizations - List of organizations to check against. + * @returns True if the user has access to the target context. + */ const hasAccessToTargetContext = ( selectedContextType: string, organizations: Organization[], ): boolean => hasAccessToContext(getTargetContext(selectedContextType), organizations); +/** + * Gets the target context based on the current context. + * @param currentContext - The current context. + * @returns The target context or stored context if current is None. + */ const getTargetContext = ( currentContext: SelectedContextType | string, ): SelectedContextType | string => currentContext === SelectedContextType.None ? getSelectedContextFromStorage() : currentContext; +/** + * Retrieves the selected context from session storage. + * @returns The stored context or Self if none exists. + */ const getSelectedContextFromStorage = (): string => { return typedSessionStorage.getItem('dashboard::selectedContext') || SelectedContextType.Self; };Also applies to: 66-69, 71-73
frontend/dashboard/app/App.test.tsx (1)
83-89: Consider adding error case tests for getLibraryHeading and getFavouriteAppListHeading.The helper functions could benefit from additional test coverage for cases where the headings are not found.
+it('should handle missing library heading', () => { + expect(() => getLibraryHeading()).toThrow(); +}); + +it('should handle missing favourite app list heading', () => { + expect(() => getFavouriteAppListHeading()).toThrow(); +});Also applies to: 91-96
frontend/dashboard/pages/PageLayout/DashboardHeader/DashboardHeader.tsx (2)
43-45: Add prop types validation for TopNavigationMenuProps.Consider adding prop types validation using PropTypes or TypeScript's required modifier.
type TopNavigationMenuProps = { - menuItem: HeaderMenuItem; + menuItem: HeaderMenuItem & Required<Pick<HeaderMenuItem, 'name' | 'link'>>; };
47-63: Consider memoizing TopNavigationMenuItem component.Since this component is used in a map function, memoizing it could prevent unnecessary re-renders.
-function TopNavigationMenuItem({ menuItem }: TopNavigationMenuProps): React.ReactElement { +const TopNavigationMenuItem = React.memo(function TopNavigationMenuItem( + { menuItem }: TopNavigationMenuProps +): React.ReactElement { const selectedContext: string = useSelectedContext(); const { t } = useTranslation(); const path: string = `${menuItem.link}/${selectedContext}`; return ( <StudioPageHeader.HeaderLink color='dark' variant='regular' renderLink={(props) => ( <NavLink to={path} {...props}> {t(menuItem.name)} </NavLink> )} /> ); -} +});backend/src/Designer/Services/Interfaces/Organisation/ICodeListService.cs (2)
33-34: Fix incorrect XML documentation.The documentation for GetCodeLists method incorrectly states it gets a single options list.
/// <summary> - /// Gets an options list from the app repository with the specified optionListId. + /// Gets all options lists from the app repository. /// </summary>
67-68: Fix incorrect XML documentation.The documentation for UploadCodeList method incorrectly describes adding a new option.
/// <summary> - /// Adds a new option to the option list. + /// Uploads an options list from a file. /// </summary>backend/src/Designer/Controllers/Preview/InstancesController.cs (2)
32-32: Consider centralizing theOptionsFolderPathconstant.The same constant is duplicated across multiple files. Consider moving it to a shared constants file to maintain DRY principles and ensure consistency.
189-189: Address the TODO comment about dynamic options list.The TODO comment indicates missing functionality for language and source-based options list retrieval.
Would you like me to help implement the dynamic options list functionality based on language and source parameters?
frontend/dashboard/pages/CreateService/CreateService.test.tsx (1)
328-339: Consider using an enum value for organization.Instead of hardcoding 'ttd' as the organization ID, consider using a constant or enum value for better maintainability and type safety.
- const selectedContext = 'ttd'; + const selectedContext = SelectedContextType.Organization;
🛑 Comments failed to post (8)
frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.ts (1)
1-7: 🛠️ Refactor suggestion
Convert class with static members to a standalone function.
The class only contains static members, which adds unnecessary complexity. Consider refactoring to a standalone function for better maintainability and tree-shaking.
Apply this diff to convert the class to a standalone function:
-export class FileUtils { - static convertToFormData = (file: File): FormData => { - const formData = new FormData(); - formData.append('file', file); - return formData; - }; -} +export const convertToFormData = (file: File): FormData => { + const formData = new FormData(); + formData.append('file', file); + return formData; +};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export const convertToFormData = (file: File): FormData => { const formData = new FormData(); formData.append('file', file); return formData; };🧰 Tools
🪛 Biome (1.9.4)
[error] 1-7: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
frontend/libs/studio-pure-functions/src/FileUtils/FileUtils.test.ts (1)
13-14:
⚠️ Potential issueUse appropriate Jest matchers for File objects.
The
toBematcher checks for reference equality which may not work reliably with File objects returned from FormData.get(). Consider using more appropriate matchers to verify the file properties.Apply this diff to improve the test:
- expect(retrievedFile).toBe(file); + expect(retrievedFile).toBeInstanceOf(File); + expect((retrievedFile as File).name).toBe(fileName); + expect((retrievedFile as File).type).toBe(fileType); + expect((retrievedFile as File).size).toBe(fileContent.length);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const retrievedFile = formData.get('file'); expect(retrievedFile).toBeInstanceOf(File); expect((retrievedFile as File).name).toBe(fileName); expect((retrievedFile as File).type).toBe(fileType); expect((retrievedFile as File).size).toBe(fileContent.length);frontend/packages/shared/src/hooks/queries/useTextResourcesForOrgQuery.ts (1)
11-14:
⚠️ Potential issueInclude dynamic parameters in the query key.
The query key should include both
organdlanguageparameters to ensure proper caching and avoid stale data.Apply this diff to fix the query key:
return useQuery<ITextResourcesWithLanguage>({ - queryKey: [QueryKey.TextResourcesForOrg], + queryKey: [QueryKey.TextResourcesForOrg, org, language], queryFn: () => getTextResourcesForOrg(org, language), });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.return useQuery<ITextResourcesWithLanguage>({ queryKey: [QueryKey.TextResourcesForOrg, org, language], queryFn: () => getTextResourcesForOrg(org, language), });frontend/packages/shared/src/hooks/mutations/useUploadOrgCodeListMutation.ts (1)
11-14: 🛠️ Refactor suggestion
Add file type validation.
The mutationFn accepts any file type without validation. Consider adding file type checks to prevent invalid uploads.
const mutationFn = (file: File) => { + const allowedTypes = ['text/csv', 'application/json']; + if (!allowedTypes.includes(file.type)) { + throw new Error('Invalid file type. Only CSV and JSON files are supported.'); + } const formData = FileUtils.convertToFormData(file); return uploadCodeListForOrg(org, formData); };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const mutationFn = (file: File) => { const allowedTypes = ['text/csv', 'application/json']; if (!allowedTypes.includes(file.type)) { throw new Error('Invalid file type. Only CSV and JSON files are supported.'); } const formData = FileUtils.convertToFormData(file); return uploadCodeListForOrg(org, formData); };frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.tsx (1)
17-19:
⚠️ Potential issueImplement missing callback functions.
Several callback functions are empty. These should be implemented to handle their respective operations.
Consider implementing these callbacks using the appropriate mutation hooks:
-onUpdateCodeListId: () => {}, -onUpdateCodeList: () => {}, -onUploadCodeList: () => {}, +onUpdateCodeListId: (id: string) => { + // Use appropriate mutation hook +}, +onUpdateCodeList: (data: CodeListData) => { + // Use useUpdateOrgCodeListMutation +}, +onUploadCodeList: (file: File) => { + // Use appropriate upload mutation +},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.onUpdateCodeListId: (id: string) => { // Use appropriate mutation hook }, onUpdateCodeList: (data: CodeListData) => { // Use useUpdateOrgCodeListMutation }, onUploadCodeList: (file: File) => { // Use appropriate upload mutation },frontend/dashboard/dashboardTestUtils.tsx (1)
32-34: 🛠️ Refactor suggestion
Consider using MemoryRouter instead of BrowserRouter for tests.
BrowserRouter manipulates the browser's URL and requires a browser environment, which can make tests less isolated and potentially flaky. MemoryRouter is the recommended choice for testing as it keeps routing contained in memory.
- <BrowserRouter> + <MemoryRouter> <ServicesContextProvider {...queries}>{children}</ServicesContextProvider> - </BrowserRouter> + </MemoryRouter>Don't forget to update the import:
-import { BrowserRouter } from 'react-router-dom'; +import { MemoryRouter } from 'react-router-dom';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// At the top of the file, update the import: -import { BrowserRouter } from 'react-router-dom'; +import { MemoryRouter } from 'react-router-dom'; // ... other code ... // Within the component rendering (lines 32-34 updated) <MemoryRouter> <ServicesContextProvider {...queries}>{children}</ServicesContextProvider> </MemoryRouter> // ... remaining code ...frontend/packages/shared/src/hooks/mutations/useDeleteOrgCodeListMutation.test.ts (1)
46-52:
⚠️ Potential issueFix incorrect hook usage in test.
The test is using
useCreateOrgCodeListMutationinstead ofuseDeleteOrgCodeListMutation, and the parameters are inconsistent with the delete operation.Apply this fix:
- const { result } = renderHookWithProviders(() => useCreateOrgCodeListMutation(org), { + const { result } = renderHookWithProviders(() => useDeleteOrgCodeListMutation(org), { queryClient, - queries: { createCodeListForOrg }, + queries: { deleteCodeListForOrg }, }); - await result.current.mutateAsync({ title: codeListToDelete.title }); + await result.current.mutateAsync(codeListToDelete.title);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const { result } = renderHookWithProviders(() => useDeleteOrgCodeListMutation(org), { queryClient, queries: { deleteCodeListForOrg }, }); await result.current.mutateAsync(codeListToDelete.title);frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeListPage.tsx (1)
30-30: 💡 Verification agent
🧩 Analysis chain
Verify handling of undefined codeListsUsages in CodeLists component.
The
codeListsUsagesprop has been made optional, but it's being passed directly to theCodeListscomponent. Ensure thatCodeListsproperly handles the case whencodeListsUsagesis undefined.
🏁 Script executed:
#!/bin/bash # Search for CodeLists component implementation to verify undefined prop handling ast-grep --pattern 'type CodeListsProps = { $$$ codeListsUsages$_: $_ $$$ }'Length of output: 84
I'll run a broader search to locate the definition and usage of the
codeListsUsagesprop within theCodeListscomponent to ensure it handles an undefined value correctly.
🏁 Script executed:
#!/bin/bash # Search for "codeListsUsages" in the codebase and display a few context lines rg "codeListsUsages" -C 3Length of output: 21595
Action Required: Handle Undefined
codeListsUsagesin CodeLists ComponentIt appears that the
CodeListscomponent passescodeListsUsagesdirectly togetCodeListSourcesByIdwithout a default value or any guard against an undefined value. SincecodeListsUsagesis now optional in the parent (CodeListPage), a scenario could arise where it is undefined, causing a runtime error (asundefined.findwould be invoked). To resolve this, please ensure thatcodeListsUsagesis defaulted to an empty array (or otherwise safely handled) in theCodeListscomponent to prevent such issues.
- Location:
frontend/libs/studio-content-library/src/ContentLibrary/LibraryBody/pages/CodeListPage/CodeLists/CodeLists.tsx- Change Suggestion: Default the
codeListsUsagesprop value (e.g., using destructuring default values or a conditional) to prevent passing undefined to functions expecting an array.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## org-library-mvp #14630 +/- ##
================================================
Coverage 95.73% 95.73%
================================================
Files 1919 1919
Lines 24948 24951 +3
Branches 2838 2838
================================================
+ Hits 23885 23888 +3
Misses 802 802
Partials 261 261 ☔ View full report in Codecov by Sentry. |
frontend/dashboard/pages/OrgContentLibrary/OrgContentLibrary.test.tsx
Outdated
Show resolved
Hide resolved
Description
Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Tests