perf(bookmarks): answer the wishlist from one aggregation - #112
Merged
udaycodespace merged 1 commit intoAug 27, 2026
Merged
Conversation
getSavedCourses read the user's entire bookmark collection on every request, populated a course for each row, and then filtered, sorted, counted and sliced in Node — to return a page of twelve. serializeCourse and its per-row regex ran over every saved course to produce those twelve, the category list was rebuilt by walking all of them into a Set, and title-asc called localeCompare across the full list. The load was multiplied by how the client uses it: BookmarksProvider calls the endpoint on mount and after every clear, and SavedCourses re-runs it on each bookmark-change event, so saving five courses in a row meant five full reads. It is the same defect udaycodespace#96 fixed for the admin user and course lists. One pipeline now: match the user on the indexed userId, look up the course projecting eight fields rather than a document, keep the rows whose course is gone, add the computed availability, access type, numeric price and search text, then facet the rows, the count and the category list. The category list stays deliberately unfiltered — it populates the dropdown, and building it from the filtered set would remove every other option from the control the user just used. The facet therefore sits before the filter stages, which run inside the rows and total branches only. Also casts the user id explicitly. find() casts a string id against the schema and aggregate() does not, so matching the token's string id against an ObjectId field would have read as an empty wishlist rather than as an error. The response body is unchanged, rejection messages included, so the wishlist page needed no edit.
udaycodespace
self-requested a review
August 27, 2026 06:50
Owner
|
@MOHITKOURAV01 LGTM! Looks good to me. Backend tests are passing (354/354). Approved. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GET /api/bookmarksread the user's entire bookmark collection, populated a course for every row, and then filtered, sorted, counted and sliced in Node — to return a page of twelve. This replaces it with one aggregation.Related Issue
Closes #107
What changed?
backend/utils/bookmarkListing.js(new) — query validation, the expressions for availability / access type / numeric price / search text, the pipeline builder, and the row shaper. 17 exports, all unit tested.backend/controllers/courseBookmarkController.js—getSavedCoursesbecomes parse → build → run → respond.serializeCourse,parsePrice,isPaidCourseand two local helpers are gone; the pipeline expresses what they did. The other four handlers are untouched.backend/tests/bookmark-listing.test.js(new) — 40 tests, pure and end-to-end.docs/issue-107-saved-courses-query.md(new) — write-up.No frontend change. The response body is byte-compatible, rejection messages included, and there is a test asserting the key set.
The pipeline
$matchthe user on the indexed{ userId: 1, createdAt: -1 }→$lookupthe course projecting eight fields rather than a document →$unwindpreserving rows whose course is gone →$addFieldsavailability, access type, numeric price and search text →$facet{ rows, total, categories }.The load mattered more than the row count suggests:
BookmarksProvidercalls this on mount and after every clear, andSavedCourses.jsxre-runs it on eachlearnhub:bookmark-changeevent — so saving five courses in a row meant five full reads.Two details worth a reviewer's attention
The category list is deliberately not filtered
categoriespopulates the category dropdown, and the old code built it from all the user's bookmarks, before any filter ran. Building it from the filtered set would remove every other option from the control the user just used, with no way back.So the
$facetsits before the filter stages, and the filters run inside therowsandtotalbranches only. There is a test asserting the list is identical filtered and unfiltered.This does mean the
$lookupis bounded by the user's bookmark count rather than by the page — the category list cannot be computed without the join. That is inherent to the feature, not to this implementation. Everything else (filtering, sorting, counting, slicing, shaping) now happens in the database, and the join returns eight projected fields rather than whole course documents.aggregate()does not cast idsfind()casts a string id against the schema;aggregate()does not, andgetUserIdreturnsreq.user._id.toString().$match: { userId: "<hex>" }against anObjectIdfield matches nothing — silently, as an empty wishlist rather than as an error. My first run of the new tests failed on exactly this; the cast is now explicit, with a test that a real token's string id still finds the user's bookmarks.Type
Areas touched
Testing
cd backend && npm test→ 354 pass, 0 fail (314 before, 40 added).There is no lint script in
backend/; the box is left unticked rather than ticked falsely.Test steps
/saved-courses→ it responds instead of hanging.db.system.profile: the query for a twelve-row page now shows a$limitstage. Previously it reported the user's full bookmark count with nolimitat all.pagination.totalItemscounts every match, not the twelve on screen.Unavailablefilter finds it.Screenshots
The wishlist renders identically; the change is what it costs to render.
Edge cases checked
Other edge case details
$unwindwithpreserveNullAndEmptyArrays, or the row would vanish instead of showing as unavailable, which is the entire point of thedeletedavailability filter. Tested both ways round.escapeRegexstays on both the search and the category path; there is a test that?search=(returns no rows rather than a 500._id. Two courses saved in the same millisecond (ordinary — the wishlist saves in bursts) could otherwise swap places between requests and make a card appear on both page one and page two, or on neither.{ locale: "en", strength: 2 }so it behaves like thelocaleCompareit replaces rather than sorting every uppercase title first. Only the two title sorts pay for the collation."1,299"reads as1299, not1. Tested through the price sort.NaNand not an error.Checklist
CONTRIBUTING.mdNotes
isPaidCoursewas/\d/.test(...)— any digit anywhere. That loose rule is kept rather than tightened: changing it would silently move courses between the free and paid filters on data users have already saved.ACCESS_EXPRESSIONreproduces it exactly.No new index was needed:
courseBookmarkModelalready carries{ userId: 1, createdAt: -1 }, which covers the leading$matchand the default sort.$lookupneeds a collection name, not a model, so it is read offCourse.collection.namerather than hardcoded — a change to Mongoose's pluralisation cannot silently make the join return nothing.