-
Notifications
You must be signed in to change notification settings - Fork 74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: ensure new code lists are added on the cache from onUpdateOptionListMutation #14365
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14365 +/- ##
=======================================
Coverage 95.65% 95.65%
=======================================
Files 1877 1877
Lines 24385 24395 +10
Branches 2804 2805 +1
=======================================
+ Hits 23325 23335 +10
Misses 799 799
Partials 261 261 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! 🚀
I have 1 suggestion, can we decide on using 1 naming convention for options
?
We are currently using a mix of option
and options
for lists and IDs, I suggest we stick to using options
.
Agree. But I think that must be fixed over the whole solution? 🤔 |
bdd0120
to
70cb2e5
Compare
70cb2e5
to
19f6df1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 💯
📝 WalkthroughWalkthroughThe pull request involves renaming variables and updating function signatures in the Changes
Suggested Labels
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works nicely! But I noticed that a refresh will sort the overview, and adding a new code list does not. Could you add sorting when adding the new code list, so that the overview remains consistent?
add-new-codelist.mp4
This is actually done by intention. The idea was that if an app have a lot of code lists it might be frustrating that the one you just added is not visible after creation. The assumption comes from complaints on a similar behavior in the text-editor where we sorted newly added texts and the users struggled with finding it in the list after addition. These cases are of course not identical, so not sure it would be a problem 🤷 |
Good point! Lets keep it as is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/shared/src/hooks/mutations/useUpdateOptionListMutation.test.ts (1)
22-47
: Consider adding error case testsWhile the happy path is well tested, consider adding tests for error cases such as:
- Network failures
- Invalid option list data
- Cache update failures
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/packages/shared/src/hooks/mutations/useUpdateOptionListMutation.test.ts
(1 hunks)frontend/packages/shared/src/hooks/mutations/useUpdateOptionListMutation.ts
(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Typechecking and linting
- GitHub Check: Testing
- GitHub Check: CodeQL
🔇 Additional comments (6)
frontend/packages/shared/src/hooks/mutations/useUpdateOptionListMutation.ts (4)
26-38
: LGTM! Cache handling looks goodThe onSuccess callback correctly handles both updating existing lists and invalidating the cache. The naming is now consistent with the plural form used elsewhere.
45-56
: Well-structured code with good separation of concernsThe function effectively handles both cases (existing and new lists) with clear logic flow and good modularity.
83-88
: LGTM! Intentional UX decision for immediate visibilityThe implementation uses prepend to make new items immediately visible at the top, which aligns with the stated UX goals from the PR discussion.
58-66
: Consider using a dedicated ID field instead of titleUsing
title
as an identifier could be problematic as titles are not guaranteed to be unique and might change. Consider using a dedicated ID field for matching options lists.Let's check if there are any other places in the codebase where we might face similar issues:
frontend/packages/shared/src/hooks/mutations/useUpdateOptionListMutation.test.ts (2)
13-17
: LGTM! Consistent naming in test setupThe test data setup properly reflects the naming changes made in the main implementation.
49-77
: LGTM! Comprehensive test coverage for new functionalityThe tests effectively verify both the addition of new lists and the correct ordering (prepend behavior) in the cache.
Description
Ensure new code lists are added on the cache from onUpdateOptionListMutation.
This change will make new created code lists in the content library be visible immediately.
Related Issue(s)
Issue was introduced in this PR: #14269 And discovered in this PR: #14273
Verification
Summary by CodeRabbit
Refactor
Tests