Optimizing Bookmarks w/ useBookmarks & applying it in Coach side panels #13282
Conversation
github-actions
bot
added
APP: Coach
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes managing bookmarks within side panels by transitioning from frequently fetching bookmarks via an API to maintaining them client‐side with a centralized composable.
- Introduces a new composable (useBookmarks) for handling bookmark operations.
- Updates various side panel components to use the injected bookmark state instead of a dedicated bookmarksFetch prop.
- Adjusts tests in the bookmark composable and refactors event emissions related to bookmark toggling.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/kolibri-common/composables/useBookmarks.js | Adds a new composable for centralized bookmark management, including methods for fetching, adding, removing, and toggling bookmarks. |
| packages/kolibri-common/composables/tests/useBookmarks.spec.js | Implements tests for the new composable, with one toggle test still skipped. |
| packages/kolibri-common/components/Cards/AccessibleResourceCard.vue | Changes the toggleBookmark event to pass the complete content node rather than its ID. |
| kolibri/plugins/coach/assets/src/views/quizzes/CreateExamPage/sidePanels/QuizResourceSelection/index.vue | Removes the bookmarksFetch prop and integrates useBookmarks to filter quiz content. |
| kolibri/plugins/coach/assets/src/views/lessons/LessonSummaryPage/sidePanels/LessonResourceSelection/index.vue | Updates bookmark integration by injecting useBookmarks and removing bookmarksFetch usage. |
| kolibri/plugins/coach/assets/src/views/common/resourceSelection/subPages/SelectionIndex.vue | Replaces usage of bookmarksCount with bookmarks.length from injected bookmark state. |
| kolibri/plugins/coach/assets/src/views/common/resourceSelection/subPages/SelectFromBookmarks.vue | Refactors to use injected bookmarks instead of relying on a bookmarksFetch object. |
| kolibri/plugins/coach/assets/src/views/common/resourceSelection/ContentCardList.vue | Refactors bookmark functionality to use the injected toggleBookmark and isBookmarked methods. |
| kolibri/plugins/coach/assets/src/composables/useResourceSelection.js | Removes legacy bookmarksFetch logic in favor of the new centralized bookmark management. |
Comments suppressed due to low confidence (1)
packages/kolibri-common/composables/tests/useBookmarks.spec.js:73
- The toggle bookmark test is currently skipped and its assertions reference the bookmark ID incorrectly. Consider enabling it and updating the assertions to use bookmarks.value.map(n => n.id) for proper matching.
it.skip('can toggle a bookmark', async () => {
| /** | ||
| * @param {BookmarkedContentNode} node | ||
| */ | ||
| function addBookmark(node) { |
There was a problem hiding this comment.
Consider adding error handling for the client call in addBookmark to manage potential API failures gracefully.
Copilot uses AI. Check for mistakes.
| /** | ||
| * @param {BookmarkedContentNode} node | ||
| */ | ||
| function removeBookmark(node) { |
There was a problem hiding this comment.
Consider adding error handling for the client call in removeBookmark to manage potential API failures gracefully.
Copilot uses AI. Check for mistakes.
| const fetchBookmarks = async params => { | ||
| const response = await ContentNodeResource.fetchBookmarks(params); | ||
| if (bookmarks?.annotator) { | ||
| const annotatedResults = await bookmarks.annotator(response.results); | ||
| return { | ||
| ...response, | ||
| results: annotatedResults, | ||
| count: annotatedResults.length, | ||
| }; | ||
| } | ||
| return response; | ||
| }; | ||
| const bookmarksFetch = useFetch({ | ||
| fetchMethod: () => | ||
| fetchBookmarks({ | ||
| params: { limit: 25, available: true, ...bookmarks?.filters }, | ||
| }), | ||
| fetchMoreMethod: more => | ||
| ContentNodeResource.fetchBookmarks({ | ||
| params: more, | ||
| }), | ||
| }); | ||
|
|
There was a problem hiding this comment.
The changes in this PR make it so we no longer paginate fetching Bookmarks because of the new strategy of loading them all once and maintaining the larger list client-side.
@rtibbles do you think this would be problematic overall? The ContentNodeResource.fetchBookmarks API response includes full ContentNode data, so that might get pretty hefty if the user has lots of bookmarks.
There was a problem hiding this comment.
@AlexVelezLl are there any consequences of removing this that I may have missed? I think I got all of the places that relied on this bookmarksFetch.
There was a problem hiding this comment.
do you think this would be problematic overall?
Yes (for the reasons you describe).
There was a problem hiding this comment.
Do you think it's more problematic than the fetching all of the user's bookmarks (sans ContentNode FWIW, so smaller payload) on every navigation?
One thing I just updated is so that we still have the View More button on Select from bookmarks to avoid rendering the whole list all at once if this fetch-once-and-maintain-locally approach ends up merging.
There was a problem hiding this comment.
The current API call seems significantly less likely to timeout, and if that is the concern, why don't we just cache the bookmarks call in the current code? For some reason we are completely bypassing the built in cache mechanisms in the APIResource layer by calling force: true when we fetch the bookmarks, and then not allowing it to handle cache invalidation by making direct calls to the endpoint when modifying the bookmarks.
It is possible that cache invalidation may not be working completely as expected for deletion in the APIResource layer, but it seems like it would be simpler just to fix that than this refactor.
The APIResource layer was specifically setup to allow us to do these repeated API calls on page navigation but returning a cached response if desired.
nucleogenesis
changed the title
Build Artifacts
|
Summary
We've been fetching the bookmarks over and over in the Coach side panel to keep things up-to-date with the user's behavior. This PR takes inspiration from @AlexVelezLl 's suggestion that we manage all of this client-side and only touch the API when the user updates their bookmarks.
This results in substantially fewer calls to the bookmarks API - see how there is a call on every navigation in this Before:
spammingthebookmarks.webm
Now the bookmarks API is only called on the initial side panel load and then when the user toggles a bookmark.
Notes
.bookmarkproperty which shows the user's proper Bookmark object.References
Closes #13077
Reviewer guidance
When you navigate the Lesson and Quiz resource side panel, keep your Dev Tools Network tab open and you can filter it by typing "book" into the filter bar. In the screencast above, you can see an API call made every time you navigate the topic tree - but when testing this PR you should not see that and only see a call on page load and when you toggle a bookmark.
Just go around using the side panel - select/deselect content and bookmarks. See that your decisions are reflected as expected. Go back to the root page and see that your # of bookmarks is reflected accurately as you make changes to your bookmarks.
Refresh the page after you make changes and see what happens - did it save your decision correctly?