fix: Handle errors in app content library #14430
Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to the Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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/ui-editor
TomasEng
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14430 +/- ##
=======================================
Coverage 95.68% 95.68%
=======================================
Files 1891 1891
Lines 24583 24588 +5
Branches 2823 2822 -1
=======================================
+ Hits 23523 23528 +5
Misses 799 799
Partials 261 261 ☔ View full report in Codecov by Sentry. |
TomasEng
force-pushed
the
split-app-contnet-library
branch
from
de49ab3 to
fea943f
Compare
TomasEng
force-pushed
the
split-app-contnet-library
branch
from
fea943f to
5a67361
Compare
TomasEng
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx (1)
78-80: Ensure consistent return type annotations for functionsThe function
handleUpdateCodeListIdexplicitly specifies a return typevoid, whilehandleUpdatedoes not. For consistency and readability, consider annotating the return type ofhandleUpdateor relying on type inference uniformly across your codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
frontend/app-development/features/appContentLibrary/AppContentLibrary.test.tsx(9 hunks)frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx(4 hunks)frontend/language/src/nb.json(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx
[warning] 112-112: frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx#L112
Added line #L112 was not covered by tests
🔇 Additional comments (2)
frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx (1)
112-112: Add unit tests to cover the 'hideDefaultError' handlingLine 112 introduces the
hideDefaultErroroption in theuseAddOptionListMutation. This logic is not covered by tests. Adding unit tests for this case will improve test coverage and ensure error handling works as expected.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 112-112: frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx#L112
Added line #L112 was not covered by testsfrontend/language/src/nb.json (1)
45-45: LGTM!The new translation key
app_content_library.fetch_erroris correctly added and follows the existing naming conventions.
frontend/app-development/features/appContentLibrary/AppContentLibrary.test.tsx
Outdated
Show resolved
Hide resolved
frontend/app-development/features/appContentLibrary/AppContentLibrary.test.tsx
Show resolved
Hide resolved
TomasEng
added
skip-releasenotes
…Library.test.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
The line that is not covered by tests according to Codecov belongs to code that was already present. It is out of scope of this pull request to fix that. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/app-development/features/appContentLibrary/AppContentLibrary.test.tsx (2)
156-165: Add JSDoc comments to describe the type and its properties.The type definition is clear, but adding documentation would improve maintainability.
Add JSDoc comments like this:
+/** + * Props for rendering the AppContentLibrary component in tests + */ type RenderAppContentLibraryProps = { + /** Optional service context overrides */ queries?: Partial<ServicesContextProps>; + /** Query client instance for managing test data */ queryClient?: QueryClient; };
168-182: Document the purpose of initializing OptionListsUsage with an empty array.The helper functions are well-structured, but it would be helpful to document why OptionListsUsage is initialized with an empty array.
Add a comment explaining the initialization:
function createQueryClientWithOptionsDataList( optionListDataList: OptionListData[] | undefined, ): QueryClient { const queryClient = createQueryClientMock(); queryClient.setQueryData([QueryKey.OptionLists, org, app], optionListDataList); + // Initialize usage data as empty since it's not relevant for these tests queryClient.setQueryData([QueryKey.OptionListsUsage, org, app], []); return queryClient; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app-development/features/appContentLibrary/AppContentLibrary.test.tsx(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build environment and run e2e test
- GitHub Check: CodeQL
- GitHub Check: Testing
🔇 Additional comments (3)
frontend/app-development/features/appContentLibrary/AppContentLibrary.test.tsx (3)
2-2: LGTM: Import changes are appropriate.The added imports support the new async testing functionality and type definitions.
Also applies to: 15-15
72-78: LGTM: Error handling test is well-structured.The test case properly verifies both the loading state removal and error message display. The use of a proper Error object for rejection is good practice.
Note: Based on the past review discussion, the current
waitForusage is correct as it passes the function reference without immediate execution (no parentheses).
55-55: LGTM: Test cases consistently use the new rendering helper.The refactoring improves test consistency by using
renderAppContentLibraryWithOptionListsacross all test cases.Also applies to: 67-67, 82-82, 92-92, 105-105, 117-117, 132-132
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx (2)
Line range hint
64-108: Consider memoizing event handlers for better performance.The handlers like
handleUpdateCodeListIdandhandleUpdatecould be memoized usinguseCallbackto prevent unnecessary re-renders of child components.- const handleUpdateCodeListId = (optionListId: string, newOptionListId: string): void => { + const handleUpdateCodeListId = useCallback((optionListId: string, newOptionListId: string): void => { updateOptionListId({ optionListId, newOptionListId }); - }; + }, [updateOptionListId]); - const handleUpdate = ({ title, codeList }: CodeListWithMetadata): void => { + const handleUpdate = useCallback(({ title, codeList }: CodeListWithMetadata): void => { updateOptionList({ optionListId: title, optionList: codeList }); - }; + }, [updateOptionList]);
110-130: Consider enhancing error handling with specific error messages.While the current error handling is good, it could be more informative by handling specific error cases (e.g., file size limits, format validation) instead of using a generic error message.
onError: (error: AxiosError<ApiError>) => { - if (isErrorUnknown(error)) { + if (error.response?.status === 413) { + toast.error(t('ux_editor.modal_properties_code_list_upload_size_error')); + } else if (error.response?.status === 415) { + toast.error(t('ux_editor.modal_properties_code_list_upload_format_error')); + } else if (isErrorUnknown(error)) { toast.error(t('ux_editor.modal_properties_code_list_upload_generic_error')); } },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx(3 hunks)
⏰ 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 (3)
frontend/app-development/features/appContentLibrary/AppContentLibrary.tsx (3)
Line range hint
1-28: LGTM! Well-organized imports.The imports are logically grouped and all imported entities are utilized in the code.
30-57: Excellent implementation of loading states and error handling!The component effectively:
- Separates data fetching from rendering
- Handles all possible query states
- Uses translations for user messages
Line range hint
99-105: Verify if empty image handlers are intentional.The images section contains empty handlers. Is this intentional or should there be actual implementation here?
✅ Verification successful
Empty image handlers are intentional
The empty handlers in the images section appear to be intentional, following React's best practices for defining unused or disabled features. This matches the pattern seen in the codebase where similar no-op handlers are used in mock configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for image-related functionality in the codebase rg -g '!*.{json,lock}' -i "onUpdateImage|handleUpdateImage" .Length of output: 1781
Script:
#!/bin/bash # Get the context of AppContentLibrary.tsx rg -B 10 -A 10 "onUpdateImage" ./frontend/app-development/features/appContentLibrary/AppContentLibrary.tsxLength of output: 670
|
Dear @coderabbitai, please don't encourage us to add comments to the code unless there's a really good reason to do so. Our team considers comments to be bad practice since they may become outdated and lead to more confusion than clarification. |
|
Tip For best results, initiate chat on the files or code changes. @TomasEng Thank you for this important clarification about the team's coding practices. I understand that comments should be avoided unless absolutely necessary, as they can become outdated and potentially misleading. I'll adjust my future reviews accordingly. ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. @TomasEng Thank you for this important clarification about the team's coding practices. I understand that comments should be avoided unless absolutely necessary, as they can become outdated and potentially misleading. I'll adjust my future reviews accordingly. 🧠 Learnings used |
There was a problem hiding this comment.
Looks good!
I have one question for future implementations tho; should we "disable" the whole library if hypothetically only one of the queries fails? 🤔 We might should consider splitting the queriers for each menu-item in the library so we are able to render the other library resources that does not have a failing query.
And maybe for this stage in the development when we only have one menu-item that is functional, in addition to the landingpage, we should be able to see the library, but the error should be shown when clicking on the code list menu item?
|
You've got some good suggestions there, @standeren. But I believe that would require a more extensive refactor of our library package, since it currently doesn't deal with pending and error states. Maybe we won't even need it when we start implementing it in other places. So I'm not sure if it's worth the effort, in particular because it will also increase the complexity of the library tool, including its API. |
Then we should consider a refactor in the future if/when the library grows in size. But I guess we can go for this simplification for now 🤝 |
standeren
added
the
skip-manual-testing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Since this is a small and simple PR, I tested it as well - works as expected 🤝 |
Description
This pull request splits the
AppContentLibrarycomponent in two, so that the second part and all its dependencies are only rendered when data has finished loading. It also adds an error message that is shown when the queries fail.Here is the error message:
It may of course be improved with more details, but that's a task for another pull request. This pull request solves the problem that the component may call functions with undefined values since it doesn't handle the
errorstate of the queries.Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
AppContentLibrarycomponent.