fix(frontend): a learner past the first page could not see the rest of their courses - #83
Merged
Merged
Conversation
…f their courses
Roadmap D3, the last of the audit's five defects that was open work.
`/me/courses/` is cursor-paginated; `MyCourses` read `page.results` and ignored the
rest, so a learner with more enrolments than one page saw part of their list and
nothing suggested more existed.
**The root cause was a type.** The client declared
`request<{ results: Enrollment[] }>` — a hand-written response shape, which
invariant 16 calls a bug — and because it omitted `next`, the second page was not so
much ignored as **invisible**. Nothing in the code had to decide to drop it. It now
uses the generated `PaginatedEnrollmentList`, so losing a field would be deliberate.
**And the finding that generalises: `next` must not be followed.** Django builds
that URL from the request's Host header, and we sit behind the Next.js rewrite,
which forwards its own destination as Host — the same fact `base.py` records as the
reason `CSRF_TRUSTED_ORIGINS` is required. So `next` names `api` or the internal
hostname, never the origin the browser is on. Following it verbatim would request
the wrong host from the browser and in production would be an attempt on an
internal name.
`cursorFrom` takes the `cursor` parameter and discards the origin. The test
fixtures deliberately carry `http://api/...` so a regression fails rather than
passing on a convenient localhost URL — and one test asserts no request URL
contains that host. **This is the first paginated endpoint the frontend consumes**,
so the rule is written into `specs/frontend-state-and-api.md` §8 rather than left in
one component.
A "load more" button, not page numbers: a learner's own course list is short enough
to scan, and cursor pagination gives no total, so numbered pages could not say how
many there are even if they were wanted.
**A failed "load more" keeps what is on screen.** Moving to the failed state would
replace a usable list with an error, which costs the learner information they
already had. The cursor is retained, so retrying is one click.
Four provocations, each failing the right tests: using the whole `next` URL,
replacing instead of appending, destroying the list on a failed load, and showing
the control on a single-page response.
One adjustment worth noting: two assertions use `toBeInTheDocument` rather than
`toBeVisible`, because cards sit inside `StaggerList` and carry `opacity: 0` until
an `IntersectionObserver` that `vitest.setup.ts` stubs to a no-op fires.
`motion.test.tsx` records that behaviour deliberately; asserting visibility here
would be asserting a motion library's runtime rather than this component's logic.
455 frontend tests across 40 files, tsc, eslint, verify:css, verify:a11y and
verify:static all pass, and `npm run types` produces no diff — the generated types
and the committed schema still agree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Roadmap D3 — the last of the audit's five defects that was open work.
/me/courses/is cursor-paginated;MyCoursesreadpage.resultsand ignored the rest, so a learner with more enrolments than one page saw part of their list with nothing to suggest more existed.The root cause was a type
The client declared
request<{ results: Enrollment[] }>— a hand-written response shape, which invariant 16 calls a bug. Because it omittednext, the second page wasn't so much ignored as invisible: no code had to decide to drop it.It now uses the generated
PaginatedEnrollmentList, so losing a field would be a deliberate act rather than an absence nobody could see.The finding that generalises:
nextmust not be followedDjango builds that URL from the request's Host header. We sit behind the Next.js rewrite, which forwards its own destination as Host — the same fact
base.pyrecords as the reasonCSRF_TRUSTED_ORIGINSis required. Sonextnamesapior the internal hostname, never the origin the browser is on.Following it verbatim would request the wrong host from the browser, and in production would be an attempt on an internal name.
cursorFromextracts thecursorparameter and discards the origin. The test fixtures deliberately carryhttp://api/...so a regression fails rather than passing on a convenient localhost URL, and one test asserts no request URL contains that host.This is the first paginated endpoint the frontend consumes, so the rule is written into
specs/frontend-state-and-api.md§8 rather than left inside one component. Any future one inherits it.The interaction
A "load more" button, not page numbers — a learner's own course list is short enough to scan, and cursor pagination gives no total, so numbered pages couldn't say how many there are even if they were wanted.
A failed "load more" keeps what's on screen. Moving to the failed state would replace a usable list with an error, costing the learner information they already had. The cursor is retained, so retrying is one click.
Provocations
nextURL instead of the cursorOne adjustment worth noting
Two assertions use
toBeInTheDocumentrather thantoBeVisible, because cards sit insideStaggerListand carryopacity: 0until anIntersectionObserverthatvitest.setup.tsstubs to a no-op fires.motion.test.tsxrecords that behaviour deliberately — asserting visibility here would be asserting a motion library's runtime, not this component's logic.455 frontend tests across 40 files; tsc, eslint,
verify:css,verify:a11y,verify:staticall pass; andnpm run typesproduces no diff, so the generated types and the committed schema still agree.🤖 Generated with Claude Code