feat: Add text resources to app content library #14722
Conversation
📝 WalkthroughWalkthroughThis PR updates the AppContentLibrary feature to incorporate enhanced support for text resources. Changes include the introduction of new type definitions, query and mutation hooks for text resources, updated component and test implementations, and modifications to data setups. Tests are refactored to use a consolidated rendering function and additional scenarios validate text resource handling. Utility functions for converting text resources to mutation arguments and associated tests are also added. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
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
solution/studio/designer
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14722 +/- ##
=======================================
Coverage 95.76% 95.77%
=======================================
Files 1916 1919 +3
Lines 24988 25043 +55
Branches 2859 2859
=======================================
+ Hits 23930 23985 +55
Misses 799 799
Partials 259 259 ☔ View full report in Codecov by Sentry. |
TomasEng
force-pushed
the
add-text-resources-to-app-content-library
branch
from
2e51047 to
69e3490
Compare
TomasEng
force-pushed
the
add-text-resources-to-app-content-library
branch
from
69e3490 to
15368c3
Compare
TomasEng
added
team/studio-domain1
area/content-library
![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/app-development/features/appContentLibrary/utils/convertTextResourceToMutationArgs.test.ts (1)
4-14: Consider adding more test cases for better coverage.While the happy path is well tested, consider adding test cases for:
- Empty string values
- Missing optional fields
- Validation of required fields
frontend/app-development/features/appContentLibrary/test-data/optionListDataList.ts (2)
52-55: Fix inconsistent naming.The variable name
codeList2Datais inconsistent with the pattern used inoptionList1Data. Consider renaming to maintain consistency:-export const codeList2Data: OptionListData = { +export const optionList2Data: OptionListData = { title: optionList2Name, data: optionList2, };
15-34: Add type annotations for intermediate constants.Consider adding explicit type annotations for the intermediate constants:
-const optionList1: OptionList = [ +const optionList1: OptionList = [ -const optionList2: OptionList = [ +const optionList2: OptionList = [Also applies to: 41-50
frontend/app-development/features/appContentLibrary/test-data/textResources.ts (2)
3-38: Consider using a more maintainable data structure.The current implementation has a large number of individual constants. Consider using a more structured approach:
-const label1Id = 'label1'; -const description1Id = 'description1'; // ... more constants ... +const resourceIds = { + labels: { + label1: 'label1', + label2: 'label2', + label3: 'label3', + label4: 'label4', + }, + descriptions: { + description1: 'description1', + // ... more descriptions + }, + helpTexts: { + helpText1: 'helpText1', + // ... more help texts + }, +} as const;
40-63: Simplify resource creation using a factory function.The creation of text resources follows a repetitive pattern. Consider using a factory function:
const createTextResource = (id: string, value: string): ITextResource => ({ id, value }); export const resources = { nb: { labels: { label1: createTextResource(resourceIds.labels.label1, 'Ledetekst 1'), // ... more labels }, // ... more categories }, en: { // ... similar structure for English }, } as const;frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx (2)
38-71: Consider adding error handling for text resources query.While the implementation looks good, consider adding specific error handling for text resources query failures to provide more detailed feedback to users.
switch (status) { case 'pending': return <StudioPageSpinner spinnerTitle={t('general.loading')} />; case 'error': - return <StudioPageError message={t('app_content_library.fetch_error')} />; + const errorMessage = textResourcesStatus === 'error' + ? t('app_content_library.text_resources_fetch_error') + : t('app_content_library.fetch_error'); + return <StudioPageError message={errorMessage} />; case 'success': return (
111-135: Consider adding loading state for mutation operations.While the implementation is correct, consider adding loading states during text resource updates to provide better user feedback.
const handleUpdateTextResource = useCallback( (textResourceWithLanguage: TextResourceWithLanguage): void => { + toast.info(t('app_content_library.updating_text_resource')); const mutationArgs = convertTextResourceToMutationArgs(textResourceWithLanguage); - updateTextResource(mutationArgs); + updateTextResource(mutationArgs, { + onSuccess: () => toast.success(t('app_content_library.text_resource_updated')), + onError: () => toast.error(t('app_content_library.text_resource_update_failed')), + }); }, [updateTextResource], );frontend/app-development/features/appContentLibrary/AppContentLibrary.test.tsx (1)
148-167: Consider adding error case test for text resource updates.While the success case is well tested, consider adding a test for the error case to ensure proper error handling.
it('Handles errors when updating text resources', async () => { const upsertTextResources = jest.fn().mockRejectedValue(new Error('Update failed')); const language = 'nb'; const textResource = label1ResourceNb; const textResourceWithLanguage: TextResourceWithLanguage = { language, textResource }; renderAppContentLibraryWithData({ queries: { upsertTextResources } }); retrieveConfig().codeList.props.onUpdateTextResource(textResourceWithLanguage); await waitFor(expect(upsertTextResources).toHaveBeenCalled); expect(upsertTextResources).toHaveBeenCalledTimes(1); // Add expectations for error handling once implemented });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/app-development/features/appContentLibrary/AppContentLibrary.test.tsx(7 hunks)frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx(7 hunks)frontend/app-development/features/appContentLibrary/test-data/optionListDataList.ts(1 hunks)frontend/app-development/features/appContentLibrary/test-data/textResources.ts(1 hunks)frontend/app-development/features/appContentLibrary/utils/convertTextResourceToMutationArgs.test.ts(1 hunks)frontend/app-development/features/appContentLibrary/utils/convertTextResourceToMutationArgs.ts(1 hunks)
🔇 Additional comments (8)
frontend/app-development/features/appContentLibrary/utils/convertTextResourceToMutationArgs.ts (1)
4-13: LGTM! Clean and well-typed implementation.The function effectively maps text resource data to mutation arguments with proper TypeScript types.
frontend/app-development/features/appContentLibrary/test-data/optionListDataList.ts (1)
15-39: LGTM! Well-structured test data.Good separation of concerns and clear organization of option list data.
frontend/app-development/features/appContentLibrary/test-data/textResources.ts (1)
65-93: LGTM! Well-organized language-specific arrays.The organization of resources into language-specific arrays is clean and maintainable.
frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx (2)
1-37: LGTM! Clean and well-organized imports.The imports are logically grouped and the new text resource related imports are correctly placed.
103-109: LGTM! Well-implemented text resource update handler.The
handleUpdateTextResourcecallback is correctly memoized with useCallback and has proper dependency tracking.frontend/app-development/features/appContentLibrary/AppContentLibrary.test.tsx (3)
37-40: LGTM! Clear and simple mock implementation.The mock implementation is straightforward and uses a testid for easy querying.
71-75: LGTM! Good test coverage for text resources.The test verifies that text resources are correctly passed to the component.
189-195: LGTM! Well-structured test data setup.The
createQueryClientWithDatafunction properly initializes all required query data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
frontend/app-development/features/appContentLibrary/test-data/optionListDataList.ts (3)
15-39: Add JSDoc documentation for better maintainability.Consider adding JSDoc documentation to describe the purpose and structure of the option list data. This will improve maintainability and help other developers understand the test data's purpose.
+/** + * Test data for the first option list. + * Contains three items with full text resource references (label, description, and help text). + */ const optionList1: OptionList = [
41-55: Document the intentional differences between option lists.The second option list has a different structure (missing description and helpText). If this is intentional for testing different scenarios, please document this.
+/** + * Test data for the second option list. + * Intentionally contains minimal item properties (only value and label) + * to test handling of incomplete option list items. + */ const optionList2: OptionList = [
57-57: Add type annotation for better clarity.Consider adding a type annotation to the exported array for better code clarity.
-export const optionListDataList = [optionList1Data, optionList2Data]; +export const optionListDataList: OptionListData[] = [optionList1Data, optionList2Data];frontend/app-development/features/appContentLibrary/test-data/textResources.ts (3)
3-14: Consider grouping related constants using an object or enum.The ID constants could be better organized to reduce repetition and improve maintainability.
Consider this alternative structure:
-const label1Id = 'label1'; -const description1Id = 'description1'; -const helpText1Id = 'helpText1'; -const label2Id = 'label2'; -const description2Id = 'description2'; -const helpText2Id = 'helpText2'; -const label3Id = 'label3'; -const description3Id = 'description3'; -const helpText3Id = 'helpText3'; -const label4Id = 'label4'; -const description4Id = 'description4'; -const helpText4Id = 'helpText4'; +const TEXT_RESOURCE_IDS = { + 1: { + label: 'label1', + description: 'description1', + helpText: 'helpText1', + }, + 2: { + label: 'label2', + description: 'description2', + helpText: 'helpText2', + }, + 3: { + label: 'label3', + description: 'description3', + helpText: 'helpText3', + }, + 4: { + label: 'label4', + description: 'description4', + helpText: 'helpText4', + }, +} as const;
16-39: Consider using a factory function to generate text resources.The current implementation is repetitive and could be simplified using a factory function approach.
Consider this alternative structure:
const createTextResource = (id: string, translations: { nb: string; en: string }): { nb: ITextResource; en: ITextResource } => ({ nb: { id, value: translations.nb }, en: { id, value: translations.en }, }); const resources = { label1: createTextResource('label1', { nb: 'Ledetekst 1', en: 'Label 1' }), description1: createTextResource('description1', { nb: 'Beskrivelse 1', en: 'Description 1' }), helpText1: createTextResource('helpText1', { nb: 'Hjelpetekst 1', en: 'Help text 1' }), // ... repeat for other resources }; export const { label1: { nb: label1ResourceNb, en: label1ResourceEn }, description1: { nb: description1ResourceNb, en: description1ResourceEn }, // ... repeat for other exports } = resources;
41-69: Consider adding validation for resource completeness.The current implementation doesn't ensure that all required resources are present for each language.
Consider adding runtime validation:
const validateResources = (resources: ITextResource[], expectedIds: string[]) => { const missingIds = expectedIds.filter(id => !resources.find(r => r.id === id)); if (missingIds.length > 0) { throw new Error(`Missing text resources for ids: ${missingIds.join(', ')}`); } }; const expectedIds = [label1Id, description1Id, helpText1Id, /* ... */]; validateResources(textResourcesNb, expectedIds); validateResources(textResourcesEn, expectedIds);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/app-development/features/appContentLibrary/test-data/optionListDataList.ts(1 hunks)frontend/app-development/features/appContentLibrary/test-data/textResources.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
🔇 Additional comments (2)
frontend/app-development/features/appContentLibrary/test-data/optionListDataList.ts (1)
1-14: LGTM! Well-organized imports.The imports are properly organized and use TypeScript's recommended 'type' import syntax for type definitions.
frontend/app-development/features/appContentLibrary/test-data/textResources.ts (1)
71-74: LGTM!The final export structure is clean and properly typed.
Description
This pull request adds text resources and text resource mutations to
AppContentLibraryand applies them to the code list editor.This pull request might look quite large, but test data alone accounts for more than 150 of the added lines.
Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Bug Fixes