From f4b4fd8203eb8bf0f6cd285c9c0ae9c7ab65a994 Mon Sep 17 00:00:00 2001 From: xenia_grib Date: Tue, 10 Nov 2026 20:13:22 +0100 Subject: [PATCH 1/2] Refactor Apollo cache to manage search results with pagination and cursor handling - Updated cache logic to replace the current search page instead of merging results. - Introduced `searchKey` and `pageKey` functions for better cache key management. - Removed redundant `SEARCH_CATALOG_MORE_QUERY` and adjusted related logic in `ElementsPage`. - Implemented pagination controls in `ElementsPage` for navigating between pages. - Added utility functions for managing scroll positions during result loading. - Enhanced tests to cover new pagination behavior and ensure correct cache interactions. --- README.md | 29 ++- docs/AI_USAGE.md | 15 +- docs/API.md | 7 +- docs/SUSTAINABILITY.md | 18 +- e2e/atypical.e2e.ts | 7 +- e2e/elements-flow.e2e.ts | 72 +++++-- e2e/helpers.ts | 6 +- src/components/elements/ResultsTable.tsx | 9 +- src/lib/apollo/cache.ts | 59 ++++-- src/lib/resultsScroll.ts | 11 ++ src/lib/searchOperations.ts | 49 +---- src/pages/ElementsPage.tsx | 242 +++++++++++++++-------- src/tests/lib/apollo/cache.test.ts | 185 +++++------------ src/tests/pages/ElementsPage.test.tsx | 227 +++++++++++++-------- 14 files changed, 510 insertions(+), 426 deletions(-) create mode 100644 src/lib/resultsScroll.ts diff --git a/README.md b/README.md index 417aa2f..e02b921 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ This project is a web application for exploring the FIG Women's Artistic Gymnast The beta now has a working React frontend connected to the GraphQL API and PostgreSQL: -- **Elements:** search the catalog and load more results. Filter and sort controls are visible but are not connected yet. +- **Elements:** search, filter, sort, and page through the catalog. - **Element details:** open an item from the results and view its details and variants. - **Your gymnasts:** view and create gymnast profiles using the development test login, which accepts any email address. - **Backend:** the GraphQL API and seeded PostgreSQL catalog run on the VM. @@ -23,7 +23,7 @@ The calculator and About pages are placeholders. Adding items from the catalog t | Review | What to look at | | ------------------------------------------------------------------ | ---------------------------------------------------------------------------------- | -| [Running application](http://it2810-29.idi.ntnu.no/project2) | Search, load more, element details, and the Your gymnasts test login | +| [Running application](http://it2810-29.idi.ntnu.no/project2) | Search, pagination, element details, and the Your gymnasts test login | | [ElementsPage tests](src/tests/pages/ElementsPage.test.tsx) | Search, result paging, URL state, loading, and error behavior | | [Element detail tests](src/tests/pages/ElementDetailPage.test.tsx) | Catalog detail rendering and error/loading states | | [Gymnast page tests](src/tests/pages/YourGymnastsPage.test.tsx) | Test login, loading, profile creation, and error handling | @@ -50,9 +50,9 @@ The data is used for educational purposes. All rights to the Code of Points belo For everyone, without logging in: -- **Done:** Search the catalog and load more results. +- **Done:** Search the catalog and page through results. - **Done:** View details for an element or vault, including its code, difficulty, description, image, and variants. -- **Done:** Filter and sort results; the controls are currently visual placeholders. +- **Done:** Filter and sort results. - **Not done:** Build a routine in the calculator UI and see its D-value. The calculator page is currently a placeholder. For coaches (the development test login is the only login flow): @@ -211,21 +211,20 @@ const SEARCH = gql` } ` -const { data, fetchMore } = useQuery(SEARCH, { - variables: { search, first: 20 }, +const after = searchParams.getAll('cursor').at(-1) +const { data } = useQuery(SEARCH, { + variables: { search, first: 20, ...(after ? { after } : {}) }, }) -// "Load more": the next page is added to the list in the cache. -fetchMore({ variables: { after: data.searchCatalog.pageInfo.endCursor } }) ``` -- **Load more with `fetchMore`.** The cache keeps all the pages of one search in one list (the key is the search, filter and sort, not `first` or `after`). A query with `after` that is sent with `useQuery` or `client.query` is answered from the cache. `fetchMore` always asks the server, so use it for the next page. -- **`totalCount` only with the first page:** the first-page query asks for the count, and later-page queries omit it; the cache keeps the count when those pages are loaded. +- **Previous / Next pagination.** The current page number and its cursor history are in the URL (for example `?page=3&cursor=&cursor=`). This lets browser Back, reload, and shared links restore a page. A page number without its cursor history cannot restore a keyset page, so an invalid or incomplete page address is reset to page one. The backend remains cursor-based. +- **One cached page.** Apollo keeps one `searchCatalog` connection at a time and only reads it when the search, filters, sort, page size, and cursor all match. Replacing the connection and running `cache.gc()` collects rows from the prior page. The same complete query runs on each page so direct links have the fields the list needs; `totalCount` is therefore requested on each page and remains capped at 1001 by the server. - **Authentication.** Session handling (`devLogin`, `logout`, and `restoreSession`) is in `src/lib/apollo/client.ts`. The development test login accepts any email address and is the only login flow planned; do not use real or sensitive data with it. See [Test login on the virtual machine](docs/VM_SETUP.md#6-test-login-on-the-virtual-machine) and [Login for development](docs/API.md#login-for-development). The login page (`src/pages/LoginPage.tsx`) offers the demo coach and a test coach of your own. The app reads who is logged in in one place, `useAuth()` (`src/components/auth/useAuth.ts`), which the menu, the protected pages and the gymnasts page share. - **Errors.** `describeError(error)` (`src/lib/apollo/errors.ts`) turns an error from the API or the network into a message for the user, and says whether trying again can help. ## The Elements page: search, filters and sort -The Elements page (`/elements`, `src/pages/ElementsPage.tsx`) is where a coach searches the catalog. It sends one `searchCatalog` request with the search text, the filters and the sort, and shows the result as a table (cards on a phone) with "Load more elements". The search, the filters and the sort are all in the address (`?search=salto&apparatus=UB&group=UB-6&difficulty=A,B&sort=difficulty&dir=desc`), so a result can be shared and the back button works. +The Elements page (`/elements`, `src/pages/ElementsPage.tsx`) is where a coach searches the catalog. It shows one cursor page at a time in a table (cards on a phone) with Previous and Next controls. Search, filters, sort, page number, and the cursor stack are in the address (`?search=salto&apparatus=UB&group=UB-6&difficulty=A,B&sort=difficulty&dir=desc&page=2&cursor=`), so reload, sharing, and browser Back can restore a page. ### What the user can do @@ -236,7 +235,7 @@ The Elements page (`/elements`, `src/pages/ElementsPage.tsx`) is where a coach s ### Choices and why -- **Ready-made parts only.** The menus are the Base UI `Menu` and `Tooltip` that shadcn/ui is built on (already a dependency), so the arrow keys, Enter, Space, Escape, the type-ahead and "checked" are handled by a library that is tested for it. The address is `useSearchParams` from React Router. The Apollo cache already keeps one list per `search`, `filter` and `sort` (`keyArgs` in `src/lib/apollo/cache.ts`), so a new filter is a new list and "Load more" adds to the right one. +- **Ready-made parts only.** The menus are the Base UI `Menu` and `Tooltip` that shadcn/ui is built on (already a dependency), so the arrow keys, Enter, Space, Escape, the type-ahead and "checked" are handled by a library that is tested for it. The address is `useSearchParams` from React Router. The Apollo cache keeps only the current result page and matches it against the full query arguments, including the cursor. - **Scales with the catalog.** What the filters choose from is small and fixed: the 4 apparatus with their groups and difficulty values, fetched once (`CatalogStructure`, kept by the Apollo cache and by the server in memory for ten minutes) and the letters A-J. Nothing in the interface grows with the number of elements, and no filter or sort option is read from the results. - **Difficulty is a letter for elements and a band for vault.** An element has a value from 0.1 to 1.0, which the coach knows as a letter (A = 0.1 ... J = 1.0). A vault has a D-value from 1.6 to 6.4 and no letter. There are 23 different D-values, which is too many to choose between one by one, so the menu offers bands of one whole number (2.0-2.9, 3.0-3.9 ...). A band is sent to the API as the D-values that exist inside it, which `Apparatus.difficultyValues` lists, so the API needs no ranges. Only bands that have a vault are offered, and the chosen difficulty is dropped when the apparatus changes between vault and the others, because the scales differ. - **The sort is one list of outcomes.** "Code: low to high", "Difficulty: high to low", "Name: A to Z" ... instead of a field and a separate "Ascending"/"Descending". Practitioner guidance on sort menus (for example Vitaly Friedman's) is to name what the user gets and not the jargon, and the ARIA Authoring Practices put the sort state on the column header (`aria-sort`) for tables. Difficulty needs an apparatus, because element values and vault D-scores cannot be compared (see "Why difficulty is only sorted within an apparatus" in [docs/API.md](docs/API.md)), so its two choices are disabled until one is chosen, and the menu says why. On vault the difficulty sort is by the D-score. @@ -248,13 +247,13 @@ The Elements page (`/elements`, `src/pages/ElementsPage.tsx`) is where a coach s - **A filter that cannot be used is disabled, but still reachable.** The group filter before an apparatus is chosen uses `aria-disabled` and not the `disabled` attribute: a disabled button is skipped by the keyboard, and then nobody can find out why it is disabled. The reason is a tooltip (on hover, on keyboard focus, and when the button is pressed, because a tooltip does not open on a tap) and also its `aria-describedby`, so a screen reader says it. This is the pattern that PatternFly, Salt and Vivid recommend for a disabled control that has to explain itself. - **Each button says its name and its choice** ("Apparatus: Uneven bars", "Difficulty: A, B"), the menus have names, a chosen item is `aria-checked`, and the focus goes back to the button when a menu closes. "Clear filters" removes itself, so it moves the focus to the first filter. -- **The number of results is announced** (`role="status"`, "Showing 20 of 178"), and the results section is `aria-busy` while it loads. +- **Page changes are announced and focused.** A `role="status"` region says "Page N, showing X results", focus moves to the Results heading, and the results section is `aria-busy` while it loads. Pagination is a named navigation landmark with keyboard-operable Previous and Next buttons. - **Every page has a title** that says what the page is (`PageTitle`, "Elements – WAG CoP Planner"; WCAG 2.4.2). React 19 moves a `` to the head wherever it is rendered, so no library is needed. The element page names the element and the apparatus, also while it is loading or has failed. - **Checked** with axe-core (no violations in light and dark mode, with the menus open and at phone width), Lighthouse (100), keyboard-only use, contrast (about 15.9:1), touch target sizes, reflow at 320 px, text spacing and reduced motion. **Not checked** with a screen reader such as NVDA or VoiceOver: that has to be done by a person. ### Testing -- `src/tests/pages/ElementsPage.test.tsx` runs the page on the real Apollo Client with a stub for `fetch` that answers like the server: each filter, several values, the address in both directions, the disabled filters, clear, the sort list, the filters and the sort together, a wrong address, a structure that fails, and "Load more" with a filter. Each behavior was broken on purpose to check that a test fails without it. +- `src/tests/pages/ElementsPage.test.tsx` runs the page on the real Apollo Client with a stub for `fetch` that answers like the server: filters, sorting, URL cursor history, Previous/Next, page errors and retry, focus/status announcements, and a ten-page bounded DOM/cache check. - The backend tests (`backend/src/catalog/search.test.ts`, `backend/src/server/server.test.ts`) cover the vault filter, the range of the values and `Apparatus.difficultyValues`, and need the seeded database like the rest. - Not everything can be tested in jsdom (for example the tooltip, which Base UI opens from real pointer events). Those parts were checked in a real browser against the real backend. @@ -549,7 +548,7 @@ The assignment asks for "a form of automated end-to-end testing" of "a longer se | File | What it tests | | -------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `e2e/elements-flow.e2e.ts` | One long flow: search, filter by apparatus and group, "Load more", sort, open an element, go back, "Clear filters". It checks the address, the results, the title and the focus at each step. | +| `e2e/elements-flow.e2e.ts` | Search, filter by apparatus and group, paginate, sort, open an element, go back, and clear; a second journey checks page 2 scroll/cache restoration after opening an element. It checks the address, results, title, focus, scroll, and that Back does not refetch the search. | | `e2e/vault.e2e.ts` | The vault difficulty bands, and sorting vault by its D-score in both directions. | | `e2e/atypical.e2e.ts` | Atypical use: an address with filters that do not exist, a search without a result, the 80-character limit, a structure that fails to load (the request is made to fail), the disabled group filter, and keyboard-only use. | | `e2e/accessibility.e2e.ts` | axe-core on the page in its main states: filtered and sorted, a menu open, the reason for a disabled filter shown, vault, no result, dark mode, a phone (375 px) and reflow at 320 px. A violation fails the test. | diff --git a/docs/AI_USAGE.md b/docs/AI_USAGE.md index 05eb3fe..85355d4 100644 --- a/docs/AI_USAGE.md +++ b/docs/AI_USAGE.md @@ -258,10 +258,8 @@ - **What Claude did:** Wrote the client, the cache policy for `searchCatalog`, the authorization link (one refresh and one repeat on an expired token, one refresh at a time between tabs), the session, the error messages, the Vite proxy and the tests. - Checked it against the running backend: the first page, "load more" and the errors. - That check showed that a plain query with `after` is answered from the cache because - `after` is not part of the key, so the pages must be loaded with `fetchMore`. This is - now in the README and `docs/API.md`. + The original cache design accumulated pages; the pagination work below replaces it + with a current-page cache policy. ## Synthetic test data (`backend/src/seed/synthetic.ts`) @@ -481,11 +479,12 @@ - **What the team did:** Decided that making the page work is done in parts, each with its own issue and pull request, and that the first part is the search field and the results (#117). Chose that the pull request is stacked on the design (#114) until that is merged. - **What Claude did:** Wrote the query `searchCatalog` and the page with the results, the loading, error and empty states, the debounce and the search in the address. Moved the example rows to a fixture for the tests. Wrote the tests on the real Apollo Client with a stub for `fetch`, and checked them by breaking five behaviors one at a time (the tests fail without each of them). Checked in a browser against the real backend: the first 20 of 562, a search, no result, a search from the address, and 9 words. -## Load more elements (`src/components/elements/ElementsPage.tsx`, `src/lib/searchOperations.ts`) +## Cursor pagination on Elements (`src/pages/ElementsPage.tsx`, `src/lib/searchOperations.ts`) - **Tool:** Claude Code -- **What the team did:** Decided that "Load more elements" is disabled when everything is shown ("Showing N of N"), and that all the work to make the page work is one issue (#110), in several pull requests. -- **What Claude did:** Wrote the query for the next page (without `totalCount`, which the API counts only when it is asked), the button with its states (loading, disabled when everything is shown, error with a new try) and the tests, and checked the tests by breaking six behaviors one at a time. Checked in a browser against the real backend: 20, 40 and 60 of 562, one result (disabled at once), and 36 of 36 for a search that has two pages. +- **What the team did:** Replaced "Load more" with Previous/Next pagination to keep only one page of catalog rows in the DOM and Apollo cache. Chose cursor history in the URL so reload, shared links, and browser Back can restore a page without changing the backend's keyset API. +- **What Claude did:** Reused the complete catalog query for every page, kept the page number and cursor stack in the URL, replaced the cached connection and garbage-collected old normalized rows, added accessible focus/status updates and retry behavior, and preserved the scroll position across an element detail visit. Added component/cache coverage including ten pages and an E2E page-2 → detail → Back journey with a no-refetch assertion. The ten-page test measured 20 table rows, 20 card entries, and 20 cached catalog rows after paging; the prior accumulating design would hold 200 result rows and render 400 responsive DOM entries for the same ten pages. Updated README, API, and sustainability documentation, including why there are no numbered pages or "last page" (keyset cursors cannot jump, and offset paging gets slower with depth). Found and fixed two problems in the run of the e2e tests: the e2e sort checks counted an empty table (shown while the next page loads) as sorted, and the flow expected a full second page for a filter that has between 21 and 40 results. +- **Limits:** The e2e tests were run on the 562-entry seed (the synthetic rows were removed for the run). A screen reader was not used. ## The count of the search stops at 1001 (`backend/src/catalog/search.ts`) @@ -580,4 +579,4 @@ - The result did not make the code shorter: the files under `src/components/gymnasts/` have more lines than before, because of the new files and their comments. What changed is that each decision now has one place. - Changed outside the Gymnast page: the element page (colors, the badge, "Try in a routine", "Add to gymnast"), "Your gymnasts" (the wrapping of the title and the list), `variables.css` (`--card-hover`), and two shared parts that other pages use too: `Button` (the `icon` prop, which changes nothing when it is not given) and `DifficultyBadge` (its colors now follow the theme, also in the results of the Elements page). - Not done on purpose: moving the error messages of the other pages (the elements results, the element page, the login, "Your gymnasts") to the three layers, because those files belong to other pull requests and the change would conflict with them. The menus of the Elements page still have their own copies of the menu classes that `menuStyles.ts` now holds. -- **Limits:** The end-to-end tests need the database and the backend, and are run by hand: the course git server has no runners. A screen reader was not used: that has to be done by a person. Lighthouse was started on a production build in the last round, but the run did not finish (the computer was low on memory), so there are no Lighthouse scores from that round. "Your gymnasts" was fixed after the last check in a browser and has not been measured again, and the results table of the Elements page, which uses the changed badge, has not been looked at in dark mode. The test gymnast was made with the test login in the local database and deleted again. \ No newline at end of file +- **Limits:** The end-to-end tests need the database and the backend, and are run by hand: the course git server has no runners. A screen reader was not used: that has to be done by a person. Lighthouse was started on a production build in the last round, but the run did not finish (the computer was low on memory), so there are no Lighthouse scores from that round. "Your gymnasts" was fixed after the last check in a browser and has not been measured again, and the results table of the Elements page, which uses the changed badge, has not been looked at in dark mode. The test gymnast was made with the test login in the local database and deleted again. diff --git a/docs/API.md b/docs/API.md index a736208..db33efc 100644 --- a/docs/API.md +++ b/docs/API.md @@ -158,7 +158,7 @@ The requirement is that the solution works with any amount of data. The catalog - **Filter and sort need indexes.** `schema.prisma` declares an index for each sort field combined with `id` (`codeSort`, `difficulty`, and `hasName` with `nameSort`), and for the filter columns (apparatus, group). Without them the database sorts the whole catalog for every request. The name sort puts named items first in both directions (`hasName` descending), so ascending and descending each have their own index. The apparatus filter is often combined with a sort, so there are also indexes on apparatus with `difficulty` and with `codeSort`. The difficulty sort always has an apparatus filter, so it uses the index on apparatus and `difficulty`. The difficulty filter without an apparatus uses the index on `difficulty`. Without them, a filtered page walks past the rows of the other apparatus first, which is slow in one direction (for example vault sorted by ascending difficulty). - **Sort by name needs a rule for items without a name.** Most items have no name. A cursor does not accept `nulls` placement, so the sort uses a plain `hasName` field and a `nameSort` field that is never `null` (see Search without raw SQL). - **The text search must not scan the table.** A search for `%word%` cannot use an ordinary index. The search uses a word table with a B-tree index and a prefix match, which finds the rows that have a word starting with the search term without reading every row. A search that is correct but reads every row works on 500 rows and not on 5 million. -- **`totalCount` is only calculated when the client asks for it.** Counting a large result takes time: about 30 ms for the whole catalog and about 100 ms for a search in a test with 225 000 entries, and the cost grows with the catalog. The resolver for the field runs only if `totalCount` is in the query, so a client that scrolls without showing a total does not pay for it. The client should therefore ask for `totalCount` only with the first page of a new search or filter, and not with the pages that are loaded while scrolling. Together with the debounce on the search field, this keeps the count from running on every keystroke. **The count is capped:** it stops at 1001 (`MAX_TOTAL_COUNT` is 1000, and the database counts one more), so the cost does not grow with the catalog. A `totalCount` above 1000 means that there are more than 1000 matches, and the interface shows "1000+". The schema is the same (`Int!`), only the meaning of a value above 1000 is new. +- **`totalCount` is only calculated when the client asks for it.** Counting a large result takes time: about 30 ms for the whole catalog and about 100 ms for a search in a test with 225 000 entries. The resolver for the field runs only if `totalCount` is in the query. A client that does not need a total can omit it, or use `@include` to request it only on the first page. The Elements page currently uses the same complete query on every page so cached pages have all the selected fields and the list can show the total; therefore it recounts when each new page is fetched. **The count is capped:** it stops at 1001 (`MAX_TOTAL_COUNT` is 1000, and the database counts one more), so the cost does not grow with the catalog. A `totalCount` above 1000 means that there are more than 1000 matches, and the interface shows "1000+". The schema is the same (`Int!`), only the meaning of a value above 1000 is new. - **Elements and vaults are different tables, but one list.** A query cannot combine two tables into one sorted and paged result. The data model keeps them apart because they work differently, so the list is read from `CatalogEntry`, a read-only table with the shared columns that the seed script fills. It never gets out of date because the catalog never changes between seed runs. - **Variants are few.** A variant group has two or three members, so `variants` does not need paging. @@ -330,7 +330,7 @@ The scripts are in `backend/package.json`: `dev` and `start` run `src/index.ts` - **`imageUrl`** is `IMAGE_BASE_URL`, the apparatus (the first part of the id) and the file name. Moving the drawings only changes `IMAGE_BASE_URL`. - **`value` and `dValue`** are `Decimal` in the database and are converted to numbers at the end, where they are sent. - **A search result is a catalog entry** (one per code). The element or vault behind each result is loaded in one batch by DataLoader, so a page costs a fixed number of queries (4–6 in the tests) whether it has 5 or 100 results. -- **`totalCount` is only calculated when the query asks for it**, also through fragments, and not when `@skip` or `@include` leaves it out. A client that wants the count only on the first page writes `totalCount @include(if: $firstPage)`. +- **`totalCount` is only calculated when the query asks for it**, also through fragments, and not when `@skip` or `@include` leaves it out. A client that wants the count only on the first page can write `totalCount @include(if: $firstPage)`; the Elements page instead requests it on each page to keep the result query complete and show the total. - **Errors.** `BadUserInputError` and the other errors in `backend/src/errors.ts` get their code in `extensions.code`. Errors from GraphQL itself (an invalid query) keep their code. Anything else gives `INTERNAL_SERVER_ERROR` with the message "Internal server error": the details go to the log and never to the client. `formatError` reads `originalError` directly instead of using Apollo's `unwrapResolverError`, because that function fails when two copies of the `graphql` package are loaded. - **A request body above 100 kB is refused** (413). The CORS allow-list is `CORS_ORIGIN`; a browser on another address cannot call the API. - **`graphql` is pinned to version 16.** Apollo Server 5 needs `^16`, and the latest release is 17. @@ -362,7 +362,8 @@ These are the choices for the implementation. The server is built with them (iss Apollo Client is set up in `src/lib/apollo/` (issue #75). See "Calling the API from the frontend" in the README for how to use it. -- **`cache.ts`.** `searchCatalog` is a Relay connection. The key of the field is the search, filter and sort, so all the pages of one search are one list. The merge function puts a page with `after` after the pages already there, skips a page that arrives twice (by cursor), starts over when the first page (no `after`) arrives again, and keeps `totalCount` when a later page does not have it. `possibleTypes` lets a fragment on `CatalogItem` match `Element` and `Vault`. Elements and vaults are stored once by `__typename` and `id`, also when they are in several lists. Because `after` is not in the key, a plain query with `after` is a cache hit; `fetchMore` is the way to load the next page. +- **`cache.ts`.** `searchCatalog` is a Relay connection. The cache keeps one current page; its read policy only returns that page when search, filter, sort, page size, and `after` all match. A new page replaces the prior connection, and the Elements page runs Apollo garbage collection so normalized catalog rows no longer referenced by the current page can be removed. The full query (including `totalCount`) is used for every cursor page, so a direct page link receives the same complete fields. The URL stores the page number and the sequence of prior-page cursors; a page number without its cursor history is not sufficient to restore a keyset page, so an address with `?page=` but missing or mismatched cursors is reset to page 1. `possibleTypes` lets a fragment on `CatalogItem` match `Element` and `Vault`. +- **Previous / Next and "Page N", no numbered pages (issue #162).** Going back needs no API change: the cursor that starts a page is the `endCursor` of the page before it, so the client keeps the stack of earlier cursors (text only, no rows) and asks for the earlier page again. Jumping to page N, a list of numbered pages or "last page" cannot be done with keyset cursors: it would need offset paging, which gets slower the deeper the page (the database reads and drops every row before the offset), or a `before`/`last` extension of the API. "Page X of Y" would also only be exact below the `totalCount` cap of 1001. The team chose Previous / Next with the current page number, which keeps every page as cheap as the first and needs no backend change. Each cursor is about 100 characters, so the address grows by that much per page; that is a few kB after tens of pages, well within what browsers accept. - **`authLink.ts`, `session.ts`.** Adds `Authorization: Bearer <access token>`, and on `UNAUTHENTICATED` refreshes the session once and repeats the request once. The login operations (`Login`, `RefreshSession`, `Logout`) never start a refresh. Parts of the page that refresh at the same moment share one request, and tabs follow each other through `navigator.locks`, with the refresh token read inside the lock. A refresh token that the server refuses ends the login; a network error keeps it for a later try. The access token is kept in memory, and the refresh token in `localStorage` (see the decision in "Identifying the user"). - **`api.ts`.** The `login`, `refreshSession` and `logout` calls are plain `fetch` requests, not Apollo operations, because the refresh is called by the link itself. - **`errors.ts`.** One place for the messages of the error codes. diff --git a/docs/SUSTAINABILITY.md b/docs/SUSTAINABILITY.md index 3900b4c..6fa8151 100644 --- a/docs/SUSTAINABILITY.md +++ b/docs/SUSTAINABILITY.md @@ -10,17 +10,19 @@ With GraphQL, each operation selects the fields its page uses instead of downloa **Where:** [searchOperations.ts](../src/lib/searchOperations.ts), [catalogOperations.ts](../src/lib/catalogOperations.ts), and [gymnastOperations.ts](../src/lib/gymnastOperations.ts). -### Debounce search and fetch results in pages +### Debounce search and paginate one page at a time -The search waits **300 ms after the user stops typing** before updating the address and sending the query. It requests **20 results per page**, with a “Load more” action for additional results, rather than downloading the whole catalog at once. This avoids requests for every keystroke and limits the initial response. +The search waits **300 ms after the user stops typing** before updating the address and sending the query. It requests **20 results per page** and shows only that page, with Previous and Next controls. This avoids requests for every keystroke and prevents the DOM and Apollo cache from growing as the visitor explores a large catalog. The URL stores the page number and cursor history, so reload, sharing, and browser Back can restore a cursor page; a page number without its cursors is reset to page one. -**Where:** `SEARCH_DELAY_MS` and `PAGE_SIZE` in [searchOperations.ts](../src/lib/searchOperations.ts); the debounce, initial query, and “Load more” behavior in [ElementsPage.tsx](../src/pages/ElementsPage.tsx). +**Measurement:** after navigating through ten pages of 20 synthetic catalog items, the pagination test measured **20 table rows, 20 card entries, and 20 normalized catalog rows in Apollo's cache**. The table and cards are responsive alternatives, so both exist in the DOM while CSS displays only one: that is **40 result-entry DOM nodes (20 visible)**. In the previous accumulating design, the same ten pages held **200 result rows in the connection/cache** and rendered **200 table rows plus 200 card entries (400 result-entry DOM nodes)**. The before number follows the previous merge-and-render behavior at 20 items per page; the after numbers are asserted by the ten-page component/cache test. These are counts of result entries, not browser heap or energy measurements. + +**Where:** `SEARCH_DELAY_MS` and `PAGE_SIZE` in [searchOperations.ts](../src/lib/searchOperations.ts); URL cursor navigation, the page controls, and cache garbage collection in [ElementsPage.tsx](../src/pages/ElementsPage.tsx); page replacement in [cache.ts](../src/lib/apollo/cache.ts); ten-page checks in [ElementsPage.test.tsx](../src/tests/pages/ElementsPage.test.tsx) and [cache.test.ts](../src/tests/lib/apollo/cache.test.ts). ### Bound the cost of counting results -`totalCount` is requested on the first search page only. The server counts only when the query asks for it, and stops after **1,001 matches**; the interface displays that as “1000+”. This bounds how many matching rows the count operation needs to return or consider, rather than counting every result for very large searches. It does not mean every database query has a constant cost: query cost can still depend on the filter, indexes, and database. +Every page uses the same complete query, including `totalCount`, so a direct cursor link has all fields and Apollo does not consider the page incomplete. The server stops after **1,001 matches**; the interface displays that as “1000+”. The count is therefore requested again when a new page is fetched, but it remains capped. This bounds how many matching rows the count operation needs to return or consider, rather than counting every result for very large searches. It does not mean every database query has a constant cost: query cost can still depend on the filter, indexes, and database. -**Where:** the first-page and later-page GraphQL operations in [searchOperations.ts](../src/lib/searchOperations.ts); the `withTotalCount` check and `take: max + 1` cap in [search.ts](../backend/src/catalog/search.ts); the “1000+” display in `formatTotal` in [searchOperations.ts](../src/lib/searchOperations.ts). +**Where:** the query and capped total in [searchOperations.ts](../src/lib/searchOperations.ts) and [search.ts](../backend/src/catalog/search.ts). ### Use compact, lazy-loaded drawings and browser caching @@ -34,11 +36,11 @@ The catalog JSON is no longer imported into the frontend to build the element de **Where:** the API-backed detail query in [catalogOperations.ts](../src/lib/catalogOperations.ts) and its use in [ElementDetailPage.tsx](../src/pages/ElementDetailPage.tsx); the historical comparison is recorded in [AI_USAGE.md](./AI_USAGE.md). -### Reuse cached search results +### Bound cached search results -Apollo Client's in-memory cache stores search pages by search text, filters, and sort order. When a user returns to a search already loaded during the same app session, Apollo can display the cached results without requesting them again. Loading another page adds it to that search's cached result list. +Apollo Client's in-memory cache stores only the current `searchCatalog` page, including its cursor and query parameters. After a page replaces the prior connection, garbage collection removes normalized catalog rows no longer referenced by the current page. The current page is available from cache while navigating to an element and back; moving to another page replaces it, so browsing more pages does not accumulate catalog rows. -**Where:** Apollo is configured with the cache in [client.ts](../src/lib/apollo/client.ts); the search cache key and page-merging behavior are in [cache.ts](../src/lib/apollo/cache.ts). +**Where:** Apollo is configured with the cache in [client.ts](../src/lib/apollo/client.ts); the one-page read/replace policy is in [cache.ts](../src/lib/apollo/cache.ts). ### Offer a dark theme diff --git a/e2e/atypical.e2e.ts b/e2e/atypical.e2e.ts index cccd69d..024f88b 100644 --- a/e2e/atypical.e2e.ts +++ b/e2e/atypical.e2e.ts @@ -33,16 +33,15 @@ test('a group of another apparatus is left out, and a sort that needs an apparat ).toBeVisible() }) -test('a search without a result says so, and has nothing to load', async ({ +test('a search without results says so, and both pagination buttons are disabled', async ({ page, }) => { await page.goto('elements') await resultsLoaded(page) await page.getByRole('searchbox').fill('zzzqqqxxx') await expect(page.getByText('No elements match “zzzqqqxxx”.')).toBeVisible() - await expect( - page.getByRole('button', { name: 'Load more elements' }), - ).toHaveCount(0) + await expect(page.getByRole('button', { name: 'Previous' })).toBeDisabled() + await expect(page.getByRole('button', { name: 'Next' })).toBeDisabled() // With a filter the message says that the filters are part of it. await page.goto('elements?search=zzzqqqxxx&apparatus=FX') diff --git a/e2e/elements-flow.e2e.ts b/e2e/elements-flow.e2e.ts index 69d27c5..1387d07 100644 --- a/e2e/elements-flow.e2e.ts +++ b/e2e/elements-flow.e2e.ts @@ -1,5 +1,5 @@ // A longer sequence of interactions on the Elements page, as a coach would do it: search, -// narrow down with the filters, sort, load more, open an element, come back and clear. +// narrow down with the filters, paginate, open an element, come back and clear. // It checks the address, the results and the title at each step. It needs the seeded // catalog of the repository (562 entries). @@ -16,7 +16,7 @@ import { expectSearch, } from './helpers.ts' -test('search, filter, sort, load more, open an element and come back, then clear', async ({ +test('search, filter, paginate, open an element and come back, then clear', async ({ page, }) => { // 1. The page opens with the whole catalog, 20 at a time. @@ -28,10 +28,8 @@ test('search, filter, sort, load more, open an element and come back, then clear // 2. A search narrows it down, and is in the address after the user stops typing. await page.getByRole('searchbox').fill('salto') - await expect(page).toHaveURL(/search=salto/) - await expect - .poll(async () => (await counts(page)).total) - .toBeLessThan(all.total) + await expectSearch(page, /search=salto/) + await expect(page.getByRole('searchbox')).toHaveValue('salto') // 3. The apparatus filter narrows it more, and its menu closes after one choice. await openMenu(page, /^Apparatus/) @@ -44,16 +42,19 @@ test('search, filter, sort, load more, open an element and come back, then clear await expect.poll(async () => (await counts(page)).total).toBeGreaterThan(20) const floor = await counts(page) - // 4. "Load more" adds the next page to the list, until everything is shown. - const before = (await rowCodes(page)).length - await page.getByRole('button', { name: 'Load more elements' }).click() + // 4. Pagination replaces the visible page and records the cursor in the address. + await page.getByRole('button', { name: 'Next' }).click() + await expect(page).toHaveURL(/page=2&cursor=/) + const secondPage = Math.min(20, floor.total - 20) await expect( - page.getByText(`Showing ${floor.total} of ${floor.total}`), + page.getByText(`Page 2, showing ${secondPage} results`), ).toBeVisible() - expect((await rowCodes(page)).length).toBeGreaterThan(before) - await expect( - page.getByRole('button', { name: 'Load more elements' }), - ).toBeDisabled() + expect((await rowCodes(page)).length).toBe(secondPage) + await expect(page.getByRole('button', { name: 'Previous' })).toBeEnabled() + // On the seeded catalog this filter has two pages, so there is no third. + if (floor.total <= 40) { + await expect(page.getByRole('button', { name: 'Next' })).toBeDisabled() + } // 5. Sorting by difficulty starts from the first page, in the order that was asked for. await openMenu(page, /Sort by/) @@ -66,7 +67,7 @@ test('search, filter, sort, load more, open an element and come back, then clear await openMenu(page, 'Element group') await choose(page, 'menuitemradio', 'Saltos backward') await expectSearch(page, /group=FX-5/) - // The old results stay on the page until the new ones arrive, so wait for them. + // Wait for the new first page to arrive. await expect .poll(async () => { const codes = await rowCodes(page) @@ -109,7 +110,42 @@ test('search, filter, sort, load more, open an element and come back, then clear await expect( page.getByRole('button', { name: /Sort by Code: low to high/ }), ).toBeVisible() - await expect - .poll(async () => (await counts(page)).total) - .toBeGreaterThan(floor.total) + await expect(page.getByText('Page 1, showing 20 results')).toBeVisible() + expect((await rowCodes(page)).length).toBe(20) +}) + +test('page 2 survives opening an element and browser Back without a new search request', async ({ + page, +}) => { + const searchRequests: string[] = [] + page.on('request', (request) => { + if (request.postData()?.includes('SearchCatalog')) { + searchRequests.push(request.postData() ?? '') + } + }) + + await page.goto('elements') + await resultsLoaded(page) + await page.getByRole('button', { name: 'Next' }).click() + await expect(page).toHaveURL(/page=2&cursor=/) + await expect(page.getByText('Page 2, showing 20 results')).toBeVisible() + await expect.poll(() => searchRequests.length).toBe(2) + + const lastResult = page.locator('table tbody tr a').last() + await lastResult.scrollIntoViewIfNeeded() + const savedScroll = await page.evaluate(() => window.scrollY) + await lastResult.click() + await expect(page).toHaveURL(/\/elements\/[^/?]+/) + await expect(page.getByRole('heading', { level: 1 })).toBeVisible() + + await page.goBack() + await expect(page).toHaveURL(/page=2&cursor=/) + await expect(page.getByText('Page 2, showing 20 results')).toBeVisible() + await expect.poll(() => page.evaluate(() => window.scrollY)).toBe(savedScroll) + expect(searchRequests).toHaveLength(2) + + await page.reload() + await expect(page).toHaveURL(/page=2&cursor=/) + await expect(page.getByText('Page 2, showing 20 results')).toBeVisible() + await expect.poll(() => searchRequests.length).toBe(3) }) diff --git a/e2e/helpers.ts b/e2e/helpers.ts index d3a1037..e3e6ff5 100644 --- a/e2e/helpers.ts +++ b/e2e/helpers.ts @@ -65,7 +65,9 @@ export function search(page: Page): string { return decodeURIComponent(new URL(page.url()).search) } +// An empty list is not "sorted": while a new page loads the table has no rows, and a +// poll must wait for the rows to arrive. export const isDescending = (values: number[]) => - values.every((v, i) => i === 0 || v <= values[i - 1]) + values.length > 0 && values.every((v, i) => i === 0 || v <= values[i - 1]) export const isAscending = (values: number[]) => - values.every((v, i) => i === 0 || v >= values[i - 1]) + values.length > 0 && values.every((v, i) => i === 0 || v >= values[i - 1]) diff --git a/src/components/elements/ResultsTable.tsx b/src/components/elements/ResultsTable.tsx index ab91b33..030e1c6 100644 --- a/src/components/elements/ResultsTable.tsx +++ b/src/components/elements/ResultsTable.tsx @@ -1,8 +1,9 @@ import { ChevronRight } from 'lucide-react' -import { Link } from 'react-router-dom' +import { Link, useLocation } from 'react-router-dom' import DifficultyBadge from '@/components/ui/DifficultyBadge' import { getDifficultyLetter } from '@/lib/difficulty' import { groupLabel, type ElementListRow } from '@/lib/elementList' +import { saveResultsScroll } from '@/lib/resultsScroll' // On a wide screen the results are a table. On a phone and a small screen (below // the "md" breakpoint) they are cards below each other instead, because four @@ -33,6 +34,7 @@ const MORE_VARIANTS = * nothing else would show it. */ function ResultCards({ rows }: { rows: ElementListRow[] }) { + const location = useLocation() return ( <ul className="m-0 grid list-none grid-cols-1 gap-3 p-0 min-[600px]:grid-cols-2 md:hidden"> {rows.map((row) => ( @@ -45,6 +47,7 @@ function ResultCards({ rows }: { rows: ElementListRow[] }) { {/* The link covers the whole card (the ::after). */} <Link to={`/elements/${row.id}`} + onClick={() => saveResultsScroll(location.key, window.scrollY)} className="outline-none after:absolute after:inset-0" > {row.code} @@ -124,6 +127,7 @@ const ROW_LOOK = 'not-last:bg-(--surface) not-last:even:bg-(--table-row-alt) *:bg-(--surface) even:*:bg-(--table-row-alt) last:*:first:rounded-bl-lg last:*:last:rounded-br-lg' function ResultTableView({ rows }: { rows: ElementListRow[] }) { + const location = useLocation() return ( <div className="rounded-lg shadow-(--box-shadow) max-md:hidden"> <table @@ -184,6 +188,9 @@ function ResultTableView({ rows }: { rows: ElementListRow[] }) { anywhere in the row opens the element. */} <Link to={`/elements/${row.id}`} + onClick={() => + saveResultsScroll(location.key, window.scrollY) + } className="min-w-0 outline-none after:absolute after:inset-0" > {row.name && ( diff --git a/src/lib/apollo/cache.ts b/src/lib/apollo/cache.ts index 3e98823..c10e7dd 100644 --- a/src/lib/apollo/cache.ts +++ b/src/lib/apollo/cache.ts @@ -1,40 +1,61 @@ -// The cache of Apollo Client. `searchCatalog` is a Relay connection -// (edges, cursor, pageInfo). The cache keeps all the pages of one search in one -// list: "load more" (the same query with `after`) adds the next page to it, and -// a new search, filter or sort starts a new list. +// The cache keeps only the current search page. A different cursor replaces the +// previous connection; normalized rows from the replaced page are collected by +// Apollo's garbage collector. import { InMemoryCache, type TypePolicies } from '@apollo/client' -type Edge = { cursor?: string } type Connection = { - edges?: readonly Edge[] totalCount?: number | null - pageInfo?: unknown + __pageKey?: string + __searchKey?: string [key: string]: unknown } +type SearchArgs = Record<string, unknown> | null | undefined + +function searchKey(args: SearchArgs): string { + return JSON.stringify({ + search: args?.['search'] ?? null, + filter: args?.['filter'] ?? null, + sort: args?.['sort'] ?? null, + first: args?.['first'] ?? null, + }) +} + +function pageKey(args: SearchArgs): string { + return JSON.stringify({ + search: args?.['search'] ?? null, + filter: args?.['filter'] ?? null, + sort: args?.['sort'] ?? null, + first: args?.['first'] ?? null, + after: args?.['after'] ?? null, + }) +} + export const typePolicies: TypePolicies = { Query: { fields: { searchCatalog: { - // `first` and `after` are left out on purpose: they are the page, not the search. - keyArgs: ['search', 'filter', 'sort'], + keyArgs: false, + read(existing: Connection | undefined, { args }) { + return existing?.__pageKey === pageKey(args) ? existing : undefined + }, merge( existing: Connection | undefined, incoming: Connection, { args }, - ): Connection { - // The first page of a search (no cursor) replaces the list. - if (!existing || !args?.['after']) return incoming - const known = new Set(existing.edges?.map((edge) => edge.cursor)) - const added = (incoming.edges ?? []).filter( - (edge) => !known.has(edge.cursor), // the same page twice adds nothing - ) + ) { + const currentSearchKey = searchKey(args) return { ...incoming, - edges: [...(existing.edges ?? []), ...added], - // totalCount is only asked for with the first page. - totalCount: incoming.totalCount ?? existing.totalCount, + // Preserve a count if a caller deliberately omits it on later pages. + totalCount: + incoming.totalCount ?? + (existing?.__searchKey === currentSearchKey + ? existing.totalCount + : undefined), + __pageKey: pageKey(args), + __searchKey: currentSearchKey, } }, }, diff --git a/src/lib/resultsScroll.ts b/src/lib/resultsScroll.ts new file mode 100644 index 0000000..f97767c --- /dev/null +++ b/src/lib/resultsScroll.ts @@ -0,0 +1,11 @@ +const positions = new Map<string, number>() + +export function saveResultsScroll(key: string, top: number): void { + positions.set(key, top) +} + +export function takeResultsScroll(key: string): number | undefined { + const top = positions.get(key) + positions.delete(key) + return top +} diff --git a/src/lib/searchOperations.ts b/src/lib/searchOperations.ts index 2469210..7b377cd 100644 --- a/src/lib/searchOperations.ts +++ b/src/lib/searchOperations.ts @@ -143,15 +143,12 @@ type Page = { edges: { cursor: string; node: SearchNode }[] } -/** The first page of a search, with the number of results in all. */ +/** One search page, with the capped count of results in all. */ export type SearchCatalogResult = { searchCatalog: Page & { totalCount: number } } -/** A later page of a search. The count is not asked for again. */ -export type SearchCatalogMoreResult = { searchCatalog: Page } - -// The fields of one result, the same in both queries. +// The fields of one result. const RESULT = ` id code @@ -179,10 +176,7 @@ const RESULT = ` } ` -/** - * The first page of a search. It asks for `totalCount`, which the API counts only when - * it is asked (counting is expensive on a large result). - */ +/** One page of a search, including the capped total count for direct page links. */ export const SEARCH_CATALOG_QUERY: TypedDocumentNode< SearchCatalogResult, SearchVariables @@ -216,43 +210,6 @@ export const SEARCH_CATALOG_QUERY: TypedDocumentNode< } ` -/** - * The next page of a search ("Load more"). It is the same field with `after`, so the - * cache adds it to the pages that are shown (see src/lib/apollo/cache.ts), and it - * does not ask for `totalCount` again: the cache keeps the first one. - */ -export const SEARCH_CATALOG_MORE_QUERY: TypedDocumentNode< - SearchCatalogMoreResult, - SearchVariables -> = gql` - query SearchCatalogMore( - $search: String - $filter: CatalogFilter - $sort: CatalogSort - $first: Int! - $after: String - ) { - searchCatalog( - search: $search - filter: $filter - sort: $sort - first: $first - after: $after - ) { - pageInfo { - hasNextPage - endCursor - } - edges { - cursor - node { - ${RESULT} - } - } - } - } -` - /** A result of the search as a row of the table and of the cards. */ export function toListRow(node: SearchNode): ElementListRow { const isVault = node.__typename === 'Vault' diff --git a/src/pages/ElementsPage.tsx b/src/pages/ElementsPage.tsx index 4add436..0e90fe0 100644 --- a/src/pages/ElementsPage.tsx +++ b/src/pages/ElementsPage.tsx @@ -5,9 +5,9 @@ import { useState, type FormEvent, } from 'react' -import { useQuery } from '@apollo/client/react' -import { useSearchParams } from 'react-router-dom' -import { Plus, Search, X } from 'lucide-react' +import { useApolloClient, useQuery } from '@apollo/client/react' +import { useLocation, useSearchParams } from 'react-router-dom' +import { Search, X } from 'lucide-react' import { MultiFilterMenu, SingleFilterMenu, @@ -36,19 +36,59 @@ import { MAX_SEARCH_LENGTH, PAGE_SIZE, parseSort, - SEARCH_CATALOG_MORE_QUERY, SEARCH_CATALOG_QUERY, SEARCH_DELAY_MS, sortParams, toListRow, type Sort, } from '@/lib/searchOperations' +import { takeResultsScroll } from '@/lib/resultsScroll' const PILL = 'h-10 rounded-full border-0 bg-(--button-bg) px-4 text-sm font-medium text-(--button-text) shadow-(--box-shadow) transition-colors hover:bg-(--button-hover-bg) focus-visible:bg-(--button-hover-bg) dark:border-transparent dark:bg-(--button-bg) dark:hover:bg-(--button-hover-bg) hover:text-(--button-text) dark:hover:text-(--button-text) focus-visible:ring-0 focus-visible:outline-2 focus-visible:outline-solid focus-visible:outline-offset-2 focus-visible:outline-(--text-color) motion-reduce:transition-none' +type CursorPage = { page: number; cursors: string[]; valid: boolean } + +function cursorPageFrom(params: URLSearchParams): CursorPage { + const rawPage = params.get('page') + const cursors = params.getAll('cursor') + if (rawPage === null && cursors.length === 0) + return { page: 1, cursors: [], valid: true } + + const page = rawPage === null ? Number.NaN : Number(rawPage) + const valid = + Number.isSafeInteger(page) && + page > 1 && + page === cursors.length + 1 && + cursors.every((cursor) => cursor.length > 0) + return valid + ? { page, cursors, valid: true } + : { page: 1, cursors: [], valid: false } +} + +function clearCursorPage(params: URLSearchParams): URLSearchParams { + const next = new URLSearchParams(params) + next.delete('page') + next.delete('cursor') + return next +} + +function withCursorPage( + params: URLSearchParams, + cursors: string[], +): URLSearchParams { + const next = clearCursorPage(params) + if (cursors.length > 0) { + next.set('page', String(cursors.length + 1)) + for (const cursor of cursors) next.append('cursor', cursor) + } + return next +} + export default function ElementsPage() { const [searchParams, setSearchParams] = useSearchParams() + const location = useLocation() + const apolloClient = useApolloClient() // The search is in the address (?search=...), so a result can be shared and the // back button works. `text` is what is in the field, and `search` is what the @@ -58,6 +98,8 @@ export default function ElementsPage() { const [text, setText] = useState(searchParams.get('search') ?? '') const written = useRef(search) const apparatusButton = useRef<HTMLButtonElement>(null) + const resultsHeading = useRef<HTMLHeadingElement>(null) + const pageChangePendingFocus = useRef(false) // Every change to the address is made on the address as the last change left it. The // function form of setSearchParams only sees the address of the last render, so two @@ -86,17 +128,19 @@ export default function ElementsPage() { const next = new URLSearchParams(params) if (trimmed) next.set('search', trimmed) else next.delete('search') - return next + return clearCursorPage(next) }, { replace: true }, ) } - // A new sort is a new list (the cache keys it), so it starts from the first page. + // A new sort replaces the cached page, so it starts from the first page. function changeSort(chosen: Sort) { changeAddress( (params) => { const next = new URLSearchParams(params) + next.delete('page') + next.delete('cursor') next.delete('sort') next.delete('dir') for (const [key, value] of Object.entries(sortParams(chosen))) @@ -145,21 +189,34 @@ export default function ElementsPage() { searchParams.get('dir'), Boolean(filters.apparatusId), ) + const cursorPage = cursorPageFrom(searchParams) + const pageNumber = cursorPage.page + const after = cursorPage.cursors.at(-1) + + useEffect(() => { + if (!cursorPage.valid) { + changeAddress(clearCursorPage, { replace: true }) + } + // The address is the source of truth; `changeAddress` uses the latest ref. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [cursorPage.valid, location.search]) - // A changed filter is a new list (the cache keys it), so it starts from the first - // page. It is a new step in the history, so the back button undoes it. + // A changed filter replaces the cached page, so it starts from the first page. It is + // a new step in the history, so the back button undoes it. // The change is made on the filters of the address as it is when it is applied, and not // on the ones of the last render: two quick choices in the difficulty menu both count. function changeFilters(change: (current: ElementFilters) => ElementFilters) { changeAddress((params) => - withFilters(params, change(parseFilters(params, structure))), + clearCursorPage( + withFilters(params, change(parseFilters(params, structure))), + ), ) } const apparatus = structure.find((a) => a.id === filters.apparatusId) const group = apparatus?.groups.find((g) => g.id === filters.groupId) - // What the search is sent with. `after` is added by "Load more". + // The URL stores the cursor stack, so a page can be reloaded or shared. const searchVariables = { search: search || null, filter: toCatalogFilter(filters, structure), @@ -168,47 +225,54 @@ export default function ElementsPage() { ...(sort.field !== 'CODE' || sort.direction !== 'ASC' ? { sort } : {}), } - const { data, previousData, loading, error, refetch, fetchMore } = useQuery( - SEARCH_CATALOG_QUERY, - { - variables: { ...searchVariables, first: PAGE_SIZE }, - skip: filtersPending || filtersBlocked, - }, - ) + const queryVariables = { + ...searchVariables, + first: PAGE_SIZE, + ...(after ? { after } : {}), + } + const { data, loading, error, refetch } = useQuery(SEARCH_CATALOG_QUERY, { + variables: queryVariables, + skip: filtersPending || filtersBlocked, + }) // What the results area says when something failed: the structure of the filters when the // address needs it, otherwise the search. const shownError = filtersBlocked ? structureQuery.error : error const retry = filtersBlocked ? structureQuery.refetch : refetch - // While a new search is on its way the old results stay, so the page does not jump. - const shown = (data ?? previousData)?.searchCatalog + const shown = data?.searchCatalog const rows = shown?.edges.map((edge) => toListRow(edge.node)) ?? [] const hasNextPage = shown?.pageInfo.hasNextPage ?? false + const endCursor = shown?.pageInfo.endCursor - // "Load more": the next page is fetched with the cursor of the last one, and the - // cache adds it to the pages that are shown (src/lib/apollo/cache.ts). An error is - // for the search it happened in, and goes away with the next search. - const [loadingMore, setLoadingMore] = useState(false) - const [moreError, setMoreError] = useState<{ - search: string - message: string - } | null>(null) - - async function loadMore() { - const after = shown?.pageInfo.endCursor - if (!after || loadingMore) return - setLoadingMore(true) - setMoreError(null) - try { - await fetchMore({ - query: SEARCH_CATALOG_MORE_QUERY, - variables: { ...searchVariables, first: PAGE_SIZE, after }, - }) - } catch (problem) { - setMoreError({ search, message: describeError(problem).message }) - } finally { - setLoadingMore(false) - } + useEffect(() => { + if (data?.searchCatalog) apolloClient.cache.gc() + }, [apolloClient, data]) + + useEffect(() => { + if (!pageChangePendingFocus.current || loading || !shown) return + pageChangePendingFocus.current = false + resultsHeading.current?.focus() + }, [loading, pageNumber, shown]) + + useEffect(() => { + if (loading || !shown) return + const top = takeResultsScroll(location.key) + if (top !== undefined) window.scrollTo({ top }) + }, [location.key, loading, shown]) + + function changePage(cursors: string[]) { + pageChangePendingFocus.current = true + changeAddress((params) => withCursorPage(params, cursors)) + } + + function goToNextPage() { + if (!hasNextPage || !endCursor || loading) return + changePage([...cursorPage.cursors, endCursor]) + } + + function goToPreviousPage() { + if (pageNumber <= 1 || loading) return + changePage(cursorPage.cursors.slice(0, -1)) } function submit(event: FormEvent) { @@ -340,7 +404,9 @@ export default function ElementsPage() { aria-busy={loading} className="mt-8" > - <h2 id="results-heading">Results</h2> + <h2 id="results-heading" ref={resultsHeading} tabIndex={-1}> + Results + </h2> {shownError ? ( <div className="rounded-lg bg-(--surface) p-5 text-(--text-color) shadow-(--box-shadow)"> @@ -367,53 +433,61 @@ export default function ElementsPage() { > Loading elements… </p> - ) : rows.length === 0 ? ( - <p - role="status" - className="m-0 rounded-lg bg-(--surface) p-5 font-medium text-(--text-color) shadow-(--box-shadow)" - > - {search - ? `No elements match “${search}”${hasFilters(filters) ? ' with these filters' : ''}.` - : hasFilters(filters) - ? 'No elements match these filters.' - : 'There are no elements.'} - </p> ) : ( <> - <ResultsTable rows={rows} /> - <div className="mt-4 flex flex-wrap items-center justify-between gap-3"> + {rows.length === 0 ? ( <p - id="results-count" role="status" - aria-live="polite" - className="text-sm font-medium text-(--text-color)" + className="m-0 rounded-lg bg-(--surface) p-5 font-medium text-(--text-color) shadow-(--box-shadow)" > - Showing {rows.length} of {formatTotal(shown.totalCount)} - </p> - {/* Not pressable while the next page is on its way (a double click would - fetch it twice), and when everything is shown (Showing N of N): the - count says why. */} - <Button - type="button" - variant="outline" - className={PILL} - disabled={!hasNextPage || loadingMore || loading} - aria-busy={loadingMore} - aria-describedby="results-count" - onClick={() => void loadMore()} - > - <Plus aria-hidden="true" /> - {loadingMore ? 'Loading more…' : 'Load more elements'} - </Button> - </div> - {moreError?.search === search && ( - <p - role="alert" - className="mt-3 text-sm font-medium text-(--text-color)" - > - {moreError.message} Press the button to try again. + {search + ? `No elements match “${search}”${hasFilters(filters) ? ' with these filters' : ''}.` + : hasFilters(filters) + ? 'No elements match these filters.' + : 'There are no elements.'} </p> + ) : ( + <ResultsTable rows={rows} /> )} + <div className="mt-4 flex flex-wrap items-center justify-between gap-3"> + <div> + <p + role="status" + aria-live="polite" + className="text-sm font-medium text-(--text-color)" + > + {`Page ${pageNumber}, showing ${rows.length} results`} + </p> + {rows.length > 0 && ( + <p className="m-0 text-sm font-medium text-(--text-color)"> + {`Showing ${rows.length} of ${formatTotal(shown.totalCount)}`} + </p> + )} + </div> + <nav aria-label="Pagination" className="flex items-center gap-3"> + <Button + type="button" + variant="outline" + className={PILL} + disabled={pageNumber === 1 || loading} + onClick={goToPreviousPage} + > + Previous + </Button> + <span aria-current="page" className="text-sm font-medium"> + Page {pageNumber} + </span> + <Button + type="button" + variant="outline" + className={PILL} + disabled={!hasNextPage || loading} + onClick={goToNextPage} + > + Next + </Button> + </nav> + </div> </> )} </section> diff --git a/src/tests/lib/apollo/cache.test.ts b/src/tests/lib/apollo/cache.test.ts index 553267f..1e4277e 100644 --- a/src/tests/lib/apollo/cache.test.ts +++ b/src/tests/lib/apollo/cache.test.ts @@ -31,36 +31,6 @@ const SEARCH = gql` } } ` -// The same query without totalCount: it is only asked for with the first page. -const SEARCH_NEXT = gql` - query Search( - $search: String - $filter: CatalogFilter - $first: Int - $after: String - ) { - searchCatalog( - search: $search - filter: $filter - first: $first - after: $after - ) { - pageInfo { - hasNextPage - endCursor - } - edges { - cursor - node { - __typename - id - description - } - } - } - } -` - const edge = (n: number, type: 'Element' | 'Vault' = 'Element') => ({ __typename: 'CatalogEdge', cursor: `c${n}`, @@ -92,7 +62,7 @@ type Read = { } | null describe('the cache for searchCatalog', () => { - it('puts the next page after the first, so "load more" gives one list', () => { + it('replaces the cached page and answers only for matching cursor arguments', () => { const cache = createCache() const variables = { search: 'kip', first: 2 } cache.writeQuery({ @@ -101,130 +71,75 @@ describe('the cache for searchCatalog', () => { data: connection([edge(1), edge(2)], true, 5), }) cache.writeQuery({ - query: SEARCH_NEXT, + query: SEARCH, variables: { ...variables, after: 'c2' }, - data: connection([edge(3), edge(4)], true), - }) - cache.writeQuery({ - query: SEARCH_NEXT, - variables: { ...variables, after: 'c4' }, - data: connection([edge(5)], false), + data: connection([edge(3), edge(4)], true, 5), }) - const read = cache.readQuery<Read>({ query: SEARCH, variables })! - expect(read.searchCatalog.edges.map((e) => e.cursor)).toEqual([ - 'c1', - 'c2', - 'c3', - 'c4', - 'c5', - ]) - expect(read.searchCatalog.pageInfo).toEqual({ - __typename: 'PageInfo', - hasNextPage: false, - endCursor: 'c5', - }) - expect(read.searchCatalog.totalCount).toBe(5) // from the first page - }) - - it('adds nothing when the same page arrives twice', () => { - const cache = createCache() - const variables = { search: 'kip', first: 2 } - cache.writeQuery({ + expect(cache.readQuery<Read>({ query: SEARCH, variables })).toBeNull() + const read = cache.readQuery<Read>({ query: SEARCH, - variables, - data: connection([edge(1), edge(2)], true, 4), - }) - const next = { - query: SEARCH_NEXT, variables: { ...variables, after: 'c2' }, - data: connection([edge(3), edge(4)], false), - } - cache.writeQuery(next) - cache.writeQuery(next) - - const read = cache.readQuery<Read>({ query: SEARCH, variables })! - expect(read.searchCatalog.edges.map((e) => e.cursor)).toEqual([ - 'c1', - 'c2', - 'c3', - 'c4', - ]) + })! + expect(read.searchCatalog.edges.map((e) => e.cursor)).toEqual(['c3', 'c4']) + expect(read.searchCatalog.totalCount).toBe(5) }) - it('starts over when the first page of the same search arrives again', () => { + it('reuses a page when its cursor is revisited', () => { const cache = createCache() const variables = { search: 'kip', first: 2 } cache.writeQuery({ - query: SEARCH, - variables, - data: connection([edge(1), edge(2)], true, 4), - }) - cache.writeQuery({ - query: SEARCH_NEXT, variables: { ...variables, after: 'c2' }, - data: connection([edge(3), edge(4)], false), - }) - // A reload of the first page (the data may have changed). - cache.writeQuery({ - query: SEARCH, - variables, - data: connection([edge(1), edge(2)], true, 9), - }) - - const read = cache.readQuery<Read>({ query: SEARCH, variables })! - expect(read.searchCatalog.edges.map((e) => e.cursor)).toEqual(['c1', 'c2']) - expect(read.searchCatalog.totalCount).toBe(9) - }) - - it('keeps a list for each search, filter and sort', () => { - const cache = createCache() - cache.writeQuery({ - query: SEARCH, - variables: { search: 'kip', first: 2 }, - data: connection([edge(1), edge(2)], false, 2), - }) - cache.writeQuery({ query: SEARCH, - variables: { search: 'hang', first: 2 }, - data: connection([edge(7)], false, 1), + data: connection([edge(3), edge(4)], false, 5), }) - cache.writeQuery({ + const read = cache.readQuery<Read>({ query: SEARCH, - variables: { search: 'kip', filter: { apparatusId: 'UB' }, first: 2 }, - data: connection([edge(9)], false, 1), - }) - - const cursors = (variables: object) => - cache - .readQuery<Read>({ query: SEARCH, variables })! - .searchCatalog.edges.map((e) => e.cursor) - expect(cursors({ search: 'kip', first: 2 })).toEqual(['c1', 'c2']) - expect(cursors({ search: 'hang', first: 2 })).toEqual(['c7']) - expect( - cursors({ search: 'kip', filter: { apparatusId: 'UB' }, first: 2 }), - ).toEqual(['c9']) - // The page size is not part of the search. - expect(cursors({ search: 'kip', first: 50 })).toEqual(['c1', 'c2']) + variables: { ...variables, after: 'c2' }, + })! + expect(read.searchCatalog.edges.map((e) => e.cursor)).toEqual(['c3', 'c4']) }) - it('stores an element once, also when it is in two lists', () => { + it('stores only one page of rows after paging through ten pages', () => { const cache = createCache() - cache.writeQuery({ - query: SEARCH, - variables: { search: 'a', first: 2 }, - data: connection([edge(1), edge(2)], false, 2), - }) - cache.writeQuery({ - query: SEARCH, - variables: { search: 'b', first: 2 }, - data: connection([edge(2)], false, 1), - }) + let after: string | undefined + for (let page = 0; page < 10; page += 1) { + const start = page * 20 + 1 + const variables = { + search: 'kip', + first: 20, + ...(after ? { after } : {}), + } + const edges = Array.from({ length: 20 }, (_, index) => + edge(start + index), + ) + cache.writeQuery({ + query: SEARCH, + variables, + data: connection(edges, page < 9, 200), + }) + cache.gc() + after = edges.at(-1)!.cursor + } - const elements = Object.keys(cache.extract()).filter((key) => + const entities = Object.keys(cache.extract()).filter((key) => key.startsWith('Element:'), ) - expect(elements.sort()).toEqual(['Element:UB-1.1-a', 'Element:UB-1.2-a']) + expect(entities).toHaveLength(20) + expect( + cache + .readQuery<Read>({ + query: SEARCH, + variables: { search: 'kip', first: 20, after: 'c180' }, + }) + ?.searchCatalog.edges.map((e) => e.cursor), + ).toEqual(Array.from({ length: 20 }, (_, index) => `c${181 + index}`)) + expect( + cache.readQuery<Read>({ + query: SEARCH, + variables: { search: 'kip', first: 20 }, + }), + ).toBeNull() }) it('lets a fragment on the CatalogItem interface match an element and a vault', () => { diff --git a/src/tests/pages/ElementsPage.test.tsx b/src/tests/pages/ElementsPage.test.tsx index c75b6df..c16fb98 100644 --- a/src/tests/pages/ElementsPage.test.tsx +++ b/src/tests/pages/ElementsPage.test.tsx @@ -5,6 +5,7 @@ import { ApolloProvider } from '@apollo/client/react' import { + act, cleanup, fireEvent, render, @@ -14,13 +15,13 @@ import { } from '@testing-library/react' import { print } from 'graphql' import { MemoryRouter, useLocation, useNavigate } from 'react-router-dom' -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import type { ElementListRow } from '@/lib/elementList' import { MAX_SEARCH_LENGTH, PAGE_SIZE, - SEARCH_CATALOG_MORE_QUERY, SEARCH_CATALOG_QUERY, + SEARCH_DELAY_MS, } from '@/lib/searchOperations' import { createTestClient, @@ -34,7 +35,10 @@ import { import { exampleElementRows, exampleTotalCount } from '@/tests/elementRows' import ElementsPage from '@/pages/ElementsPage' -afterEach(cleanup) +afterEach(() => { + cleanup() + vi.useRealTimers() +}) /** What the server says for one row (an Element has `value`, a Vault has `dValue`). */ const nodeOf = (row: ElementListRow) => ({ @@ -193,7 +197,7 @@ function renderPage( </MemoryRouter> </ApolloProvider>, ) - return { calls, structureCalls } + return { calls, structureCalls, client } } const field = () => screen.getByRole<HTMLInputElement>('searchbox') @@ -251,13 +255,15 @@ describe('ElementsPage: the page', () => { ...screen.getAllByRole('link'), field(), ] - // The search field, the three filters, the sort button, "Load more" (there is a next + // The search field, the three filters, the sort button, pagination (there is a next // page), and each row twice: in the table and in a card (CSS shows only one of // them, and what is not shown cannot be reached with Tab). - expect(controls).toHaveLength(1 + 3 + 1 + 1 + 2 * 4) + expect(controls).toHaveLength(1 + 3 + 1 + 2 + 2 * 4) for (const control of controls) { expect(control.getAttribute('tabindex')).not.toBe('-1') - expect(control.hasAttribute('disabled')).toBe(false) + if (control.textContent !== 'Previous') { + expect(control.hasAttribute('disabled')).toBe(false) + } } for (const button of screen.getAllByRole('button')) { // outline-none on the base button hides the ring unless the style is set again. @@ -700,11 +706,11 @@ describe('ElementsPage: the filters', () => { await screen.findByRole('button', { name: 'Clear filters' }) }) - it('starts a changed filter from the first page, and loads more with the same filter', async () => { + it('starts a changed filter on page one and supports paging with the filter', async () => { const { calls } = renderPage(pagedServer(4)) await screen.findByText(/Showing 4 of/) - fireEvent.click(screen.getByRole('button', { name: 'Load more elements' })) - await screen.findByText(/Showing 7 of/) + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await screen.findByText(/Page 2, showing 3 results/) await openMenu('Apparatus') choose('menuitemradio', 'Uneven bars') @@ -714,8 +720,8 @@ describe('ElementsPage: the filters', () => { expect(last.variables).not.toHaveProperty('after') expect(last.variables['filter']).toEqual({ apparatusId: 'UB' }) - fireEvent.click(screen.getByRole('button', { name: 'Load more elements' })) - await screen.findByText(/Showing 7 of/) + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await screen.findByText(/Page 2, showing 3 results/) expect(calls.at(-1)!.variables).toMatchObject({ filter: { apparatusId: 'UB' }, after: expect.any(String), @@ -833,14 +839,14 @@ describe('ElementsPage: the sort', () => { ) }) - it('starts a new sort from the first page, and loads more with the same sort', async () => { + it('starts a new sort on page one and supports paging with the same sort', async () => { const { calls } = renderPage(pagedServer(4)) await screen.findByText(/Showing 4 of/) - fireEvent.click(screen.getByRole('button', { name: 'Load more elements' })) - await screen.findByText(/Showing 7 of/) + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await screen.findByText(/Page 2, showing 3 results/) expect(calls.at(-1)).toMatchObject({ - operationName: 'SearchCatalogMore', + operationName: 'SearchCatalog', variables: { after: expect.any(String) }, }) @@ -852,8 +858,8 @@ describe('ElementsPage: the sort', () => { expect(last.variables).not.toHaveProperty('after') expect(last.variables).toMatchObject({ sort: { field: 'NAME' } }) - fireEvent.click(screen.getByRole('button', { name: 'Load more elements' })) - await screen.findByText(/Showing 7 of/) + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await screen.findByText(/Page 2, showing 3 results/) expect(calls.at(-1)!.variables).toMatchObject({ sort: { field: 'NAME', direction: 'ASC' }, after: expect.any(String), @@ -1105,7 +1111,7 @@ describe('ElementsPage: the results from the API', () => { const empty = await screen.findByText('No elements match “zzz”.', {}, wait) expect(empty.getAttribute('role')).toBe('status') - expect(screen.queryByText(/Showing/)).toBeNull() + expect(screen.queryByText(/^Showing/)).toBeNull() }) it('shows the error with "Try again" when the server fails, and loads again when the user tries again', async () => { @@ -1142,16 +1148,24 @@ describe('ElementsPage: the search', () => { const { calls } = renderPage(serverWithRows) await screen.findByText(/Showing/) + vi.useFakeTimers() for (const text of ['y', 'yu', 'yur', 'yurchenko']) { fireEvent.change(field(), { target: { value: text } }) } - await waitFor(() => expect(calls).toHaveLength(2), wait) + await act(async () => { + await vi.advanceTimersByTimeAsync(SEARCH_DELAY_MS - 1) + }) + expect(calls).toHaveLength(1) + await act(async () => { + await vi.advanceTimersByTimeAsync(1) + }) + expect(calls).toHaveLength(2) expect(calls[1].variables).toMatchObject({ search: 'yurchenko', first: PAGE_SIZE, }) - expect(await screen.findByText('Showing 1 of 1', {}, wait)).toBeTruthy() + expect(screen.getByText('Showing 1 of 1')).toBeTruthy() }) it('puts the search in the address, so a result can be shared, and takes it out when the field is empty', async () => { @@ -1196,7 +1210,7 @@ describe('ElementsPage: the search', () => { expect(calls[1].variables).toMatchObject({ search: 'biles' }) }) - it('keeps the old results while a new search is on its way', async () => { + it('does not show results from the previous search while a new search is loading', async () => { const second = deferred<Answer>() renderPage(({ variables }) => variables['search'] ? second.promise : searchAnswer(exampleElementRows), @@ -1206,14 +1220,10 @@ describe('ElementsPage: the search', () => { fireEvent.change(field(), { target: { value: 'biles' } }) fireEvent.submit(screen.getByRole('search')) - // The new answer has not come, and the old results are still there. + // The new answer has not come, and the old results are no longer rendered. await waitFor(() => expect(address()).toBe('/elements?search=biles')) - expect( - screen.getByText( - `Showing ${exampleElementRows.length} of ${exampleElementRows.length}`, - ), - ).toBeTruthy() - expect(screen.queryByText('Loading elements…')).toBeNull() + expect(screen.getByText('Loading elements…')).toBeTruthy() + expect(document.querySelectorAll('tbody tr')).toHaveLength(0) second.resolve(searchAnswer([exampleElementRows[5]])) expect(await screen.findByText('Showing 1 of 1')).toBeTruthy() @@ -1238,31 +1248,34 @@ describe('ElementsPage: the search', () => { }) }) -describe('ElementsPage: Load more elements', () => { - const more = () => - screen.getByRole<HTMLButtonElement>('button', { - name: /Load more|Loading more/, - }) +describe('ElementsPage: pagination', () => { const tableRows = () => document.querySelectorAll('tbody tr') - it('fetches the next page with the cursor of the last row and adds it below, and the count follows', async () => { + it('replaces the first page with the next page and puts the cursor stack in the URL', async () => { const { calls } = renderPage(pagedServer(4)) await screen.findByText('Showing 4 of 7') expect(tableRows()).toHaveLength(4) - expect(more().disabled).toBe(false) + expect( + screen.getByRole('button', { name: 'Previous' }).hasAttribute('disabled'), + ).toBe(true) + expect( + screen.getByRole('button', { name: 'Next' }).hasAttribute('disabled'), + ).toBe(false) - fireEvent.click(more()) + fireEvent.click(screen.getByRole('button', { name: 'Next' })) - expect(await screen.findByText('Showing 7 of 7')).toBeTruthy() - expect(tableRows()).toHaveLength(7) - // The next page comes in the order of the server, after the first one. + expect(await screen.findByText(/Page 2, showing 3 results/)).toBeTruthy() + expect(tableRows()).toHaveLength(3) const codes = [...tableRows()].map( (row) => row.querySelector('td')?.textContent, ) - expect(codes).toEqual(exampleElementRows.map((row) => row.code)) + expect(codes).toEqual(exampleElementRows.slice(4).map((row) => row.code)) + expect(address()).toBe( + `/elements?page=2&cursor=${encodeURIComponent(exampleElementRows[3].id)}`, + ) expect(calls).toHaveLength(2) expect(calls[1]).toMatchObject({ - operationName: 'SearchCatalogMore', + operationName: 'SearchCatalog', variables: { search: null, first: PAGE_SIZE, @@ -1271,50 +1284,50 @@ describe('ElementsPage: Load more elements', () => { }) }) - it('does not ask for the count again with the next page', () => { - expect(print(SEARCH_CATALOG_QUERY)).toContain('totalCount') - expect(print(SEARCH_CATALOG_MORE_QUERY)).not.toContain('totalCount') + it('goes back to the prior page by requesting its cursor again', async () => { + const { calls } = renderPage(pagedServer(4)) + await screen.findByText('Showing 4 of 7') + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await screen.findByText(/Page 2, showing 3 results/) + expect(calls).toHaveLength(2) + + fireEvent.click(screen.getByRole('button', { name: 'Previous' })) + await screen.findByText(/Page 1, showing 4 results/) + expect(tableRows()).toHaveLength(4) + expect(calls).toHaveLength(3) + expect(calls[2].variables['after']).toBeUndefined() }) - it('is disabled and says so while the next page is on its way, so a double click fetches it once', async () => { + it('announces page changes and moves focus to the results heading', async () => { + renderPage(pagedServer(4)) + await screen.findByText('Showing 4 of 7') + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + expect(await screen.findByText('Page 2, showing 3 results')).toBeTruthy() + expect(await screen.findByRole('heading', { name: 'Results' })).toBe( + document.activeElement, + ) + }) + + it('keeps only the current page visible while a new page loads', async () => { const next = deferred<Answer>() const { calls } = renderPage((operation) => operation.variables['after'] ? next.promise : pagedServer(4)(operation), ) await screen.findByText('Showing 4 of 7') - fireEvent.click(more()) - await waitFor(() => expect(more().disabled).toBe(true)) - expect(more().textContent).toBe('Loading more…') - expect(more().getAttribute('aria-busy')).toBe('true') - fireEvent.click(more()) + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await waitFor(() => + expect(screen.getByText('Loading elements…')).toBeTruthy(), + ) + expect(tableRows()).toHaveLength(0) next.resolve(searchAnswer(exampleElementRows.slice(4), 7)) - expect(await screen.findByText('Showing 7 of 7')).toBeTruthy() + expect(await screen.findByText(/Page 2, showing 3 results/)).toBeTruthy() + expect(tableRows()).toHaveLength(3) expect(calls).toHaveLength(2) }) - it('is disabled when everything is shown (N of N), and the count is what describes it', async () => { - renderPage(pagedServer(10)) - const count = await screen.findByText('Showing 7 of 7') - - expect(more().disabled).toBe(true) - expect(more().getAttribute('aria-describedby')).toBe(count.id) - expect(count.id).not.toBe('') - }) - - it('becomes disabled when the last page has come', async () => { - renderPage(pagedServer(4)) - await screen.findByText('Showing 4 of 7') - - fireEvent.click(more()) - - await screen.findByText('Showing 7 of 7') - expect(more().disabled).toBe(true) - expect(more().textContent).toBe('Load more elements') - }) - - it('shows the error, keeps what is shown, and lets the user press the button again', async () => { + it('shows a page error and retries that page', async () => { let attempt = 0 const { calls } = renderPage((operation) => { if (operation.variables['after'] && ++attempt === 1) { @@ -1324,30 +1337,77 @@ describe('ElementsPage: Load more elements', () => { }) await screen.findByText('Showing 4 of 7') - fireEvent.click(more()) + fireEvent.click(screen.getByRole('button', { name: 'Next' })) const alert = await screen.findByRole('alert') expect(alert.textContent).toBe( - 'Something went wrong on our side. Try again. Press the button to try again.', + 'Something went wrong on our side. Try again.', ) - expect(tableRows()).toHaveLength(4) - expect(more().disabled).toBe(false) - - fireEvent.click(more()) - - expect(await screen.findByText('Showing 7 of 7')).toBeTruthy() + expect(tableRows()).toHaveLength(0) + fireEvent.click(screen.getByRole('button', { name: 'Try again' })) + expect(await screen.findByText(/Page 2, showing 3 results/)).toBeTruthy() expect(screen.queryByRole('alert')).toBeNull() expect(calls).toHaveLength(3) }) - it('starts again with the first page when the search changes, and the error of the old search goes', async () => { + it('restores a direct page link and sends the complete page query', async () => { + const cursor = exampleElementRows[3].id + const { calls } = renderPage( + pagedServer(4), + `/elements?page=2&cursor=${encodeURIComponent(cursor)}`, + ) + expect(await screen.findByText(/Page 2, showing 3 results/)).toBeTruthy() + expect(tableRows()).toHaveLength(3) + expect(calls[0].variables).toMatchObject({ + first: PAGE_SIZE, + after: cursor, + }) + expect(print(SEARCH_CATALOG_QUERY)).toContain('totalCount') + }) + + it('keeps the DOM and Apollo cache to one 20-result page after ten pages', async () => { + const catalog = Array.from({ length: 200 }, (_, index) => ({ + ...exampleElementRows[index % exampleElementRows.length], + id: `TEST-${index + 1}`, + code: `T${index + 1}`, + hasVariants: false, + })) + const { client } = renderPage(({ variables }) => { + const after = String(variables['after'] ?? '') + const start = after ? catalog.findIndex((row) => row.id === after) + 1 : 0 + return searchAnswer( + catalog.slice(start, start + PAGE_SIZE), + 200, + start + PAGE_SIZE < 200, + ) + }) + + await screen.findByText('Page 1, showing 20 results') + for (let page = 2; page <= 10; page += 1) { + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await screen.findByText(`Page ${page}, showing 20 results`) + } + + expect(document.querySelectorAll('tbody tr')).toHaveLength(20) + expect(document.querySelectorAll('ul li')).toHaveLength(20) + const cacheState = client.cache.extract() + if (typeof cacheState !== 'object' || cacheState === null) { + throw new Error('Expected Apollo cache.extract() to return an object') + } + const cachedCatalogRows = Object.keys(cacheState).filter((key) => + /^(Element|Vault):TEST-/.test(key), + ) + expect(cachedCatalogRows).toHaveLength(20) + }) + + it('starts again with page one when the search changes', async () => { const { calls } = renderPage((operation) => operation.variables['after'] && !operation.variables['search'] ? failure('INTERNAL_SERVER_ERROR', 'Something broke') : serverWithRowsPaged(operation), ) await screen.findByText('Showing 4 of 7') - fireEvent.click(more()) + fireEvent.click(screen.getByRole('button', { name: 'Next' })) await screen.findByRole('alert') fireEvent.change(field(), { target: { value: 'yurchenko' } }) @@ -1359,6 +1419,7 @@ describe('ElementsPage: Load more elements', () => { expect(last?.operationName).toBe('SearchCatalog') expect(last?.variables).toMatchObject({ search: 'yurchenko' }) expect(last?.variables['after']).toBeUndefined() + expect(address()).toBe('/elements?search=yurchenko') }) }) From 1d178551954a3259e595f49786a93fc8ee656e5f Mon Sep 17 00:00:00 2001 From: xenia_grib <xeniag@stud.ntnu.no> Date: Tue, 10 Nov 2026 21:19:05 +0100 Subject: [PATCH 2/2] Refactor pagination and result display in ElementsPage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Update pagination logic to use cursor-based navigation with `after` parameter instead of `cursor`. - Modify displayed result text to show ranges (e.g., "Showing 1–20 of 562") instead of total counts. - Adjust GraphQL queries to conditionally request total counts based on pagination state. - Enhance test cases to validate new pagination behavior and result display. - Ensure that the page state is preserved across navigations and reloads, improving user experience. --- README.md | 29 ++- docs/AI_USAGE.md | 1 + docs/API.md | 8 +- docs/SUSTAINABILITY.md | 6 +- e2e/atypical.e2e.ts | 2 +- e2e/elements-flow.e2e.ts | 97 ++++++++-- e2e/helpers.ts | 15 +- src/lib/searchOperations.ts | 16 +- src/pages/ElementsPage.tsx | 247 +++++++++++++++++--------- src/tests/pages/ElementsPage.test.tsx | 173 +++++++++++++----- 10 files changed, 429 insertions(+), 165 deletions(-) diff --git a/README.md b/README.md index e02b921..b43ecdc 100644 --- a/README.md +++ b/README.md @@ -194,8 +194,14 @@ import { gql } from '@apollo/client' import { useQuery } from '@apollo/client/react' const SEARCH = gql` - query Search($search: String, $first: Int, $after: String) { + query Search( + $search: String + $first: Int + $after: String + $withTotal: Boolean = true + ) { searchCatalog(search: $search, first: $first, after: $after) { + totalCount @include(if: $withTotal) pageInfo { hasNextPage endCursor @@ -211,20 +217,27 @@ const SEARCH = gql` } ` -const after = searchParams.getAll('cursor').at(-1) +// ?page=3&after=<cursor>: the cursor of the page is in the address, and the count +// comes with the first page and is carried to the next ones. +const after = searchParams.get('after') const { data } = useQuery(SEARCH, { - variables: { search, first: 20, ...(after ? { after } : {}) }, + variables: { + search, + first: 20, + ...(after ? { after } : {}), + withTotal: knownTotal === undefined, + }, }) ``` -- **Previous / Next pagination.** The current page number and its cursor history are in the URL (for example `?page=3&cursor=<end-of-page-1>&cursor=<end-of-page-2>`). This lets browser Back, reload, and shared links restore a page. A page number without its cursor history cannot restore a keyset page, so an invalid or incomplete page address is reset to page one. The backend remains cursor-based. -- **One cached page.** Apollo keeps one `searchCatalog` connection at a time and only reads it when the search, filters, sort, page size, and cursor all match. Replacing the connection and running `cache.gc()` collects rows from the prior page. The same complete query runs on each page so direct links have the fields the list needs; `totalCount` is therefore requested on each page and remains capped at 1001 by the server. +- **Previous / Next pagination.** The address has the page number and the cursor of that page (`?page=3&after=<cursor>`), so it does not grow with the page. The cursors of the earlier pages (for Previous) and the count are kept in the history entry (`location.state`), which Back, Forward and a reload keep. A shared link has no history entry: it opens the right page, and offers "First page" instead of "Previous". A page number without a cursor (`?page=3`) cannot restore a keyset page and opens page 1. The backend remains cursor-based. +- **One cached page, and one count per search.** Apollo keeps one `searchCatalog` connection at a time and only reads it when the search, filters, sort, page size, and cursor all match. Replacing the connection and running `cache.gc()` collects rows from the prior page. Every page asks for the same result fields, so a cached page is complete. `totalCount` is the same on every page of a search, so it is asked for with the first page only (`@include(if: $withTotal)`) and carried to the next pages. - **Authentication.** Session handling (`devLogin`, `logout`, and `restoreSession`) is in `src/lib/apollo/client.ts`. The development test login accepts any email address and is the only login flow planned; do not use real or sensitive data with it. See [Test login on the virtual machine](docs/VM_SETUP.md#6-test-login-on-the-virtual-machine) and [Login for development](docs/API.md#login-for-development). The login page (`src/pages/LoginPage.tsx`) offers the demo coach and a test coach of your own. The app reads who is logged in in one place, `useAuth()` (`src/components/auth/useAuth.ts`), which the menu, the protected pages and the gymnasts page share. - **Errors.** `describeError(error)` (`src/lib/apollo/errors.ts`) turns an error from the API or the network into a message for the user, and says whether trying again can help. ## The Elements page: search, filters and sort -The Elements page (`/elements`, `src/pages/ElementsPage.tsx`) is where a coach searches the catalog. It shows one cursor page at a time in a table (cards on a phone) with Previous and Next controls. Search, filters, sort, page number, and the cursor stack are in the address (`?search=salto&apparatus=UB&group=UB-6&difficulty=A,B&sort=difficulty&dir=desc&page=2&cursor=<cursor>`), so reload, sharing, and browser Back can restore a page. +The Elements page (`/elements`, `src/pages/ElementsPage.tsx`) is where a coach searches the catalog. It shows one cursor page at a time in a table (cards on a phone) with Previous and Next controls. Search, filters, sort, page number and the cursor of the page are in the address (`?search=salto&apparatus=UB&group=UB-6&difficulty=A,B&sort=difficulty&dir=desc&page=2&after=<cursor>`), so reload, sharing, and browser Back can restore a page. While the next page loads, the rows that are shown stay (dimmed), so the list and the controls do not disappear. ### What the user can do @@ -247,13 +260,13 @@ The Elements page (`/elements`, `src/pages/ElementsPage.tsx`) is where a coach s - **A filter that cannot be used is disabled, but still reachable.** The group filter before an apparatus is chosen uses `aria-disabled` and not the `disabled` attribute: a disabled button is skipped by the keyboard, and then nobody can find out why it is disabled. The reason is a tooltip (on hover, on keyboard focus, and when the button is pressed, because a tooltip does not open on a tap) and also its `aria-describedby`, so a screen reader says it. This is the pattern that PatternFly, Salt and Vivid recommend for a disabled control that has to explain itself. - **Each button says its name and its choice** ("Apparatus: Uneven bars", "Difficulty: A, B"), the menus have names, a chosen item is `aria-checked`, and the focus goes back to the button when a menu closes. "Clear filters" removes itself, so it moves the focus to the first filter. -- **Page changes are announced and focused.** A `role="status"` region says "Page N, showing X results", focus moves to the Results heading, and the results section is `aria-busy` while it loads. Pagination is a named navigation landmark with keyboard-operable Previous and Next buttons. +- **Page changes are announced and focused.** One line under the list says "Showing 21–40 of 562", and is a `role="status"` region that a screen reader hears as "Page 2, showing 21–40 of 562"; focus moves to the Results heading, and the results section is `aria-busy` while it loads. Pagination is a named navigation landmark with keyboard-operable Previous and Next buttons. - **Every page has a title** that says what the page is (`PageTitle`, "Elements – WAG CoP Planner"; WCAG 2.4.2). React 19 moves a `<title>` to the head wherever it is rendered, so no library is needed. The element page names the element and the apparatus, also while it is loading or has failed. - **Checked** with axe-core (no violations in light and dark mode, with the menus open and at phone width), Lighthouse (100), keyboard-only use, contrast (about 15.9:1), touch target sizes, reflow at 320 px, text spacing and reduced motion. **Not checked** with a screen reader such as NVDA or VoiceOver: that has to be done by a person. ### Testing -- `src/tests/pages/ElementsPage.test.tsx` runs the page on the real Apollo Client with a stub for `fetch` that answers like the server: filters, sorting, URL cursor history, Previous/Next, page errors and retry, focus/status announcements, and a ten-page bounded DOM/cache check. +- `src/tests/pages/ElementsPage.test.tsx` runs the page on the real Apollo Client with a stub for `fetch` that answers like the server: filters, sorting, the cursor in the address and the history in the state, Previous/Next and "First page" for a shared link, one count per search, the old rows while a page loads, page errors and retry, focus/status announcements, and a ten-page bounded DOM/cache check. - The backend tests (`backend/src/catalog/search.test.ts`, `backend/src/server/server.test.ts`) cover the vault filter, the range of the values and `Apparatus.difficultyValues`, and need the seeded database like the rest. - Not everything can be tested in jsdom (for example the tooltip, which Base UI opens from real pointer events). Those parts were checked in a real browser against the real backend. diff --git a/docs/AI_USAGE.md b/docs/AI_USAGE.md index 85355d4..08b91a6 100644 --- a/docs/AI_USAGE.md +++ b/docs/AI_USAGE.md @@ -484,6 +484,7 @@ - **Tool:** Claude Code - **What the team did:** Replaced "Load more" with Previous/Next pagination to keep only one page of catalog rows in the DOM and Apollo cache. Chose cursor history in the URL so reload, shared links, and browser Back can restore a page without changing the backend's keyset API. - **What Claude did:** Reused the complete catalog query for every page, kept the page number and cursor stack in the URL, replaced the cached connection and garbage-collected old normalized rows, added accessible focus/status updates and retry behavior, and preserved the scroll position across an element detail visit. Added component/cache coverage including ten pages and an E2E page-2 → detail → Back journey with a no-refetch assertion. The ten-page test measured 20 table rows, 20 card entries, and 20 cached catalog rows after paging; the prior accumulating design would hold 200 result rows and render 400 responsive DOM entries for the same ten pages. Updated README, API, and sustainability documentation, including why there are no numbered pages or "last page" (keyset cursors cannot jump, and offset paging gets slower with depth). Found and fixed two problems in the run of the e2e tests: the e2e sort checks counted an empty table (shown while the next page loads) as sorted, and the flow expected a full second page for a filter that has between 21 and 40 results. +- **Review round (team feedback with Claude Code review):** The team found that the list was replaced by "Loading elements…" while the next page loaded (with a 700 ms slow API the controls disappeared and the page jumped), that the text under the list said the page three times and that "Showing 2 of 562" now meant only the page, that `totalCount` was counted on every page although it is the same for the whole search, that an `eslint-disable` for `react-hooks` had been added, and that the address grew by about 100 characters per page (3067 characters on page 29). The team chose to keep the cursor stack in `location.state` and accept that a shared link loses the Previous history. Claude kept the old rows (dimmed, `aria-busy`) and the controls while a page loads, made one line "Showing 21–40 of 562" with the page number for screen readers, asked for the count once per search with `@include(if: $withTotal)`, removed the `eslint-disable` (the effect that resets a bad page address depends on `setSearchParams` only), put one cursor in the address and the rest in the history entry with "First page" for a link without history, and fixed a sort change that left the cursor in the address. Added component tests (one count per search, Previous through the history, a shared link, `?page=3` without a cursor) and e2e tests (a reload keeps Previous and the count, a link in a new tab, and a 700 ms slow next page that keeps the rows and the controls). - **Limits:** The e2e tests were run on the 562-entry seed (the synthetic rows were removed for the run). A screen reader was not used. ## The count of the search stops at 1001 (`backend/src/catalog/search.ts`) diff --git a/docs/API.md b/docs/API.md index db33efc..4a281b4 100644 --- a/docs/API.md +++ b/docs/API.md @@ -158,7 +158,7 @@ The requirement is that the solution works with any amount of data. The catalog - **Filter and sort need indexes.** `schema.prisma` declares an index for each sort field combined with `id` (`codeSort`, `difficulty`, and `hasName` with `nameSort`), and for the filter columns (apparatus, group). Without them the database sorts the whole catalog for every request. The name sort puts named items first in both directions (`hasName` descending), so ascending and descending each have their own index. The apparatus filter is often combined with a sort, so there are also indexes on apparatus with `difficulty` and with `codeSort`. The difficulty sort always has an apparatus filter, so it uses the index on apparatus and `difficulty`. The difficulty filter without an apparatus uses the index on `difficulty`. Without them, a filtered page walks past the rows of the other apparatus first, which is slow in one direction (for example vault sorted by ascending difficulty). - **Sort by name needs a rule for items without a name.** Most items have no name. A cursor does not accept `nulls` placement, so the sort uses a plain `hasName` field and a `nameSort` field that is never `null` (see Search without raw SQL). - **The text search must not scan the table.** A search for `%word%` cannot use an ordinary index. The search uses a word table with a B-tree index and a prefix match, which finds the rows that have a word starting with the search term without reading every row. A search that is correct but reads every row works on 500 rows and not on 5 million. -- **`totalCount` is only calculated when the client asks for it.** Counting a large result takes time: about 30 ms for the whole catalog and about 100 ms for a search in a test with 225 000 entries. The resolver for the field runs only if `totalCount` is in the query. A client that does not need a total can omit it, or use `@include` to request it only on the first page. The Elements page currently uses the same complete query on every page so cached pages have all the selected fields and the list can show the total; therefore it recounts when each new page is fetched. **The count is capped:** it stops at 1001 (`MAX_TOTAL_COUNT` is 1000, and the database counts one more), so the cost does not grow with the catalog. A `totalCount` above 1000 means that there are more than 1000 matches, and the interface shows "1000+". The schema is the same (`Int!`), only the meaning of a value above 1000 is new. +- **`totalCount` is only calculated when the client asks for it.** Counting a large result takes time: about 30 ms for the whole catalog and about 100 ms for a search in a test with 225 000 entries. The resolver for the field runs only if `totalCount` is in the query. A client that does not need a total can omit it, or use `@include` to request it only on the first page. The Elements page does that: the count is the same on every page of a search, so it asks with `totalCount @include(if: $withTotal)` on the first page only and carries the number to the later pages. **The count is capped:** it stops at 1001 (`MAX_TOTAL_COUNT` is 1000, and the database counts one more), so the cost does not grow with the catalog. A `totalCount` above 1000 means that there are more than 1000 matches, and the interface shows "1000+". The schema is the same (`Int!`), only the meaning of a value above 1000 is new. - **Elements and vaults are different tables, but one list.** A query cannot combine two tables into one sorted and paged result. The data model keeps them apart because they work differently, so the list is read from `CatalogEntry`, a read-only table with the shared columns that the seed script fills. It never gets out of date because the catalog never changes between seed runs. - **Variants are few.** A variant group has two or three members, so `variants` does not need paging. @@ -330,7 +330,7 @@ The scripts are in `backend/package.json`: `dev` and `start` run `src/index.ts` - **`imageUrl`** is `IMAGE_BASE_URL`, the apparatus (the first part of the id) and the file name. Moving the drawings only changes `IMAGE_BASE_URL`. - **`value` and `dValue`** are `Decimal` in the database and are converted to numbers at the end, where they are sent. - **A search result is a catalog entry** (one per code). The element or vault behind each result is loaded in one batch by DataLoader, so a page costs a fixed number of queries (4–6 in the tests) whether it has 5 or 100 results. -- **`totalCount` is only calculated when the query asks for it**, also through fragments, and not when `@skip` or `@include` leaves it out. A client that wants the count only on the first page can write `totalCount @include(if: $firstPage)`; the Elements page instead requests it on each page to keep the result query complete and show the total. +- **`totalCount` is only calculated when the query asks for it**, also through fragments, and not when `@skip` or `@include` leaves it out. A client that wants the count only on the first page writes `totalCount @include(if: $withTotal)`, as the Elements page does. - **Errors.** `BadUserInputError` and the other errors in `backend/src/errors.ts` get their code in `extensions.code`. Errors from GraphQL itself (an invalid query) keep their code. Anything else gives `INTERNAL_SERVER_ERROR` with the message "Internal server error": the details go to the log and never to the client. `formatError` reads `originalError` directly instead of using Apollo's `unwrapResolverError`, because that function fails when two copies of the `graphql` package are loaded. - **A request body above 100 kB is refused** (413). The CORS allow-list is `CORS_ORIGIN`; a browser on another address cannot call the API. - **`graphql` is pinned to version 16.** Apollo Server 5 needs `^16`, and the latest release is 17. @@ -362,8 +362,8 @@ These are the choices for the implementation. The server is built with them (iss Apollo Client is set up in `src/lib/apollo/` (issue #75). See "Calling the API from the frontend" in the README for how to use it. -- **`cache.ts`.** `searchCatalog` is a Relay connection. The cache keeps one current page; its read policy only returns that page when search, filter, sort, page size, and `after` all match. A new page replaces the prior connection, and the Elements page runs Apollo garbage collection so normalized catalog rows no longer referenced by the current page can be removed. The full query (including `totalCount`) is used for every cursor page, so a direct page link receives the same complete fields. The URL stores the page number and the sequence of prior-page cursors; a page number without its cursor history is not sufficient to restore a keyset page, so an address with `?page=` but missing or mismatched cursors is reset to page 1. `possibleTypes` lets a fragment on `CatalogItem` match `Element` and `Vault`. -- **Previous / Next and "Page N", no numbered pages (issue #162).** Going back needs no API change: the cursor that starts a page is the `endCursor` of the page before it, so the client keeps the stack of earlier cursors (text only, no rows) and asks for the earlier page again. Jumping to page N, a list of numbered pages or "last page" cannot be done with keyset cursors: it would need offset paging, which gets slower the deeper the page (the database reads and drops every row before the offset), or a `before`/`last` extension of the API. "Page X of Y" would also only be exact below the `totalCount` cap of 1001. The team chose Previous / Next with the current page number, which keeps every page as cheap as the first and needs no backend change. Each cursor is about 100 characters, so the address grows by that much per page; that is a few kB after tens of pages, well within what browsers accept. +- **`cache.ts`.** `searchCatalog` is a Relay connection. The cache keeps one current page; its read policy only returns that page when search, filter, sort, page size, and `after` all match. A new page replaces the prior connection, and the Elements page runs Apollo garbage collection so normalized catalog rows no longer referenced by the current page can be removed. Every page asks for the same result fields, so a cached page is complete; only `totalCount` is left out after the first page (`@include(if: $withTotal)`), and a page that is read without it does not need it. When a later page arrives without the count, the merge keeps the count of the same search. The address stores the page number and the cursor of that page (`?page=3&after=<cursor>`); a page number without its cursor is not sufficient to restore a keyset page, so `?page=3` alone opens page 1. `possibleTypes` lets a fragment on `CatalogItem` match `Element` and `Vault`. +- **Previous / Next and "Page N", no numbered pages (issue #162).** Going back needs no API change: the cursor that starts a page is the `endCursor` of the page before it, so the client keeps the stack of earlier cursors (text only, no rows) in the history entry (`location.state`) and asks for the earlier page again. Jumping to page N, a list of numbered pages or "last page" cannot be done with keyset cursors: it would need offset paging, which gets slower the deeper the page (the database reads and drops every row before the offset), or a `before`/`last` extension of the API. "Page X of Y" would also only be exact below the `totalCount` cap of 1001. The team chose Previous / Next with the current page number, which keeps every page as cheap as the first and needs no backend change. Only the cursor of the current page is in the address (about 100 characters), so the address does not grow with the page; the stack would have been about 3 kB on page 29. The price is that a shared link to page 5 has no history: it opens page 5 and offers "First page" instead of "Previous". Back, Forward and a reload keep the history entry. - **`authLink.ts`, `session.ts`.** Adds `Authorization: Bearer <access token>`, and on `UNAUTHENTICATED` refreshes the session once and repeats the request once. The login operations (`Login`, `RefreshSession`, `Logout`) never start a refresh. Parts of the page that refresh at the same moment share one request, and tabs follow each other through `navigator.locks`, with the refresh token read inside the lock. A refresh token that the server refuses ends the login; a network error keeps it for a later try. The access token is kept in memory, and the refresh token in `localStorage` (see the decision in "Identifying the user"). - **`api.ts`.** The `login`, `refreshSession` and `logout` calls are plain `fetch` requests, not Apollo operations, because the refresh is called by the link itself. - **`errors.ts`.** One place for the messages of the error codes. diff --git a/docs/SUSTAINABILITY.md b/docs/SUSTAINABILITY.md index 6fa8151..57efa80 100644 --- a/docs/SUSTAINABILITY.md +++ b/docs/SUSTAINABILITY.md @@ -12,7 +12,7 @@ With GraphQL, each operation selects the fields its page uses instead of downloa ### Debounce search and paginate one page at a time -The search waits **300 ms after the user stops typing** before updating the address and sending the query. It requests **20 results per page** and shows only that page, with Previous and Next controls. This avoids requests for every keystroke and prevents the DOM and Apollo cache from growing as the visitor explores a large catalog. The URL stores the page number and cursor history, so reload, sharing, and browser Back can restore a cursor page; a page number without its cursors is reset to page one. +The search waits **300 ms after the user stops typing** before updating the address and sending the query. It requests **20 results per page** and shows only that page, with Previous and Next controls. This avoids requests for every keystroke and prevents the DOM and Apollo cache from growing as the visitor explores a large catalog. The address stores the page number and the cursor of the page (about 100 characters, the same on every page), and the cursors of the earlier pages are kept in the history entry, so the address does not grow as the visitor pages on. A page number without its cursor is reset to page one. **Measurement:** after navigating through ten pages of 20 synthetic catalog items, the pagination test measured **20 table rows, 20 card entries, and 20 normalized catalog rows in Apollo's cache**. The table and cards are responsive alternatives, so both exist in the DOM while CSS displays only one: that is **40 result-entry DOM nodes (20 visible)**. In the previous accumulating design, the same ten pages held **200 result rows in the connection/cache** and rendered **200 table rows plus 200 card entries (400 result-entry DOM nodes)**. The before number follows the previous merge-and-render behavior at 20 items per page; the after numbers are asserted by the ten-page component/cache test. These are counts of result entries, not browser heap or energy measurements. @@ -20,9 +20,9 @@ The search waits **300 ms after the user stops typing** before updating the addr ### Bound the cost of counting results -Every page uses the same complete query, including `totalCount`, so a direct cursor link has all fields and Apollo does not consider the page incomplete. The server stops after **1,001 matches**; the interface displays that as “1000+”. The count is therefore requested again when a new page is fetched, but it remains capped. This bounds how many matching rows the count operation needs to return or consider, rather than counting every result for very large searches. It does not mean every database query has a constant cost: query cost can still depend on the filter, indexes, and database. +Counting is the expensive part of a search (about 30–100 ms, see [API.md](./API.md)), and the count is the same on every page of one search. It is therefore asked for **once per search**, with the first page (`totalCount @include(if: $withTotal)`), and carried to the next pages in the history entry; paging through ten pages counts once and not ten times. A shared link to a later page has no count with it, so that page asks for one. The server stops after **1,001 matches**; the interface displays that as “1000+”. This bounds how many matching rows the count operation needs to return or consider, rather than counting every result for very large searches. It does not mean every database query has a constant cost: query cost can still depend on the filter, indexes, and database. -**Where:** the query and capped total in [searchOperations.ts](../src/lib/searchOperations.ts) and [search.ts](../backend/src/catalog/search.ts). +**Where:** `withTotal` in [searchOperations.ts](../src/lib/searchOperations.ts) and [ElementsPage.tsx](../src/pages/ElementsPage.tsx); the `withTotalCount` check and the cap in [search.ts](../backend/src/catalog/search.ts). ### Use compact, lazy-loaded drawings and browser caching diff --git a/e2e/atypical.e2e.ts b/e2e/atypical.e2e.ts index 024f88b..8a3d72c 100644 --- a/e2e/atypical.e2e.ts +++ b/e2e/atypical.e2e.ts @@ -89,7 +89,7 @@ test('a structure that fails to load is an error with a retry, not unfiltered re await page.goto('elements?apparatus=UB&difficulty=A') await expect(page.getByRole('alert')).toBeVisible() // No results under an address that says Uneven bars, and nothing was searched. - await expect(page.getByText(/^Showing/)).toHaveCount(0) + await expect(page.getByText(/Showing \d/)).toHaveCount(0) expect(searches).toEqual([]) await expectSearch(page, '?apparatus=UB&difficulty=A') diff --git a/e2e/elements-flow.e2e.ts b/e2e/elements-flow.e2e.ts index 1387d07..9ba72e7 100644 --- a/e2e/elements-flow.e2e.ts +++ b/e2e/elements-flow.e2e.ts @@ -42,12 +42,12 @@ test('search, filter, paginate, open an element and come back, then clear', asyn await expect.poll(async () => (await counts(page)).total).toBeGreaterThan(20) const floor = await counts(page) - // 4. Pagination replaces the visible page and records the cursor in the address. + // 4. Pagination replaces the visible page and records its cursor in the address. await page.getByRole('button', { name: 'Next' }).click() - await expect(page).toHaveURL(/page=2&cursor=/) + await expect(page).toHaveURL(/page=2&after=/) const secondPage = Math.min(20, floor.total - 20) await expect( - page.getByText(`Page 2, showing ${secondPage} results`), + page.getByText(`Showing 21–${20 + secondPage} of ${floor.total}`), ).toBeVisible() expect((await rowCodes(page)).length).toBe(secondPage) await expect(page.getByRole('button', { name: 'Previous' })).toBeEnabled() @@ -60,7 +60,7 @@ test('search, filter, paginate, open an element and come back, then clear', asyn await openMenu(page, /Sort by/) await choose(page, 'menuitemradio', 'Difficulty: high to low') await expect(page).toHaveURL(/sort=difficulty&dir=desc/) - await expect(page.getByText(`Showing 20 of ${floor.total}`)).toBeVisible() + await expect(page.getByText(`Showing 1–20 of ${floor.total}`)).toBeVisible() expect(isDescending(await rowValues(page))).toBe(true) // 6. The group filter lists the groups of the apparatus, and every result is in the group. @@ -110,26 +110,32 @@ test('search, filter, paginate, open an element and come back, then clear', asyn await expect( page.getByRole('button', { name: /Sort by Code: low to high/ }), ).toBeVisible() - await expect(page.getByText('Page 1, showing 20 results')).toBeVisible() + await expect(page.getByText(/Showing 1–20 of/)).toBeVisible() expect((await rowCodes(page)).length).toBe(20) }) -test('page 2 survives opening an element and browser Back without a new search request', async ({ +test('page 2 survives opening an element, browser Back and a reload without losing its place', async ({ page, }) => { - const searchRequests: string[] = [] + const searches: { withTotal?: boolean; after?: string }[] = [] page.on('request', (request) => { - if (request.postData()?.includes('SearchCatalog')) { - searchRequests.push(request.postData() ?? '') - } + const body = request.postDataJSON() as { + operationName?: string + variables?: { withTotal?: boolean; after?: string } + } | null + if (body?.operationName === 'SearchCatalog') searches.push(body.variables!) }) await page.goto('elements') await resultsLoaded(page) await page.getByRole('button', { name: 'Next' }).click() - await expect(page).toHaveURL(/page=2&cursor=/) - await expect(page.getByText('Page 2, showing 20 results')).toBeVisible() - await expect.poll(() => searchRequests.length).toBe(2) + await expect(page).toHaveURL(/page=2&after=/) + await expect(page.getByText('Showing 21–40 of 562')).toBeVisible() + await expect.poll(() => searches.length).toBe(2) + // The count was made with the first page and is not asked for again. + expect(searches.map((s) => s.withTotal)).toEqual([true, false]) + // One cursor in the address, however deep the page. + expect(new URL(page.url()).search.length).toBeLessThan(200) const lastResult = page.locator('table tbody tr a').last() await lastResult.scrollIntoViewIfNeeded() @@ -138,14 +144,67 @@ test('page 2 survives opening an element and browser Back without a new search r await expect(page).toHaveURL(/\/elements\/[^/?]+/) await expect(page.getByRole('heading', { level: 1 })).toBeVisible() + // Back: the same page at the same place, from the cache, without a new request. await page.goBack() - await expect(page).toHaveURL(/page=2&cursor=/) - await expect(page.getByText('Page 2, showing 20 results')).toBeVisible() + await expect(page).toHaveURL(/page=2&after=/) + await expect(page.getByText('Showing 21–40 of 562')).toBeVisible() await expect.poll(() => page.evaluate(() => window.scrollY)).toBe(savedScroll) - expect(searchRequests).toHaveLength(2) + expect(searches).toHaveLength(2) + // A reload keeps the history entry, so the count and Previous still work. await page.reload() - await expect(page).toHaveURL(/page=2&cursor=/) - await expect(page.getByText('Page 2, showing 20 results')).toBeVisible() - await expect.poll(() => searchRequests.length).toBe(3) + await expect(page.getByText('Showing 21–40 of 562')).toBeVisible() + await expect.poll(() => searches.length).toBe(3) + expect(searches[2].withTotal).toBe(false) + await page.getByRole('button', { name: 'Previous' }).click() + await expect(page).toHaveURL(/\/elements$/) + await expect(page.getByText('Showing 1–20 of 562')).toBeVisible() +}) + +test('a shared link opens its page, and offers the first page instead of Previous', async ({ + page, +}) => { + await page.goto('elements') + await resultsLoaded(page) + await page.getByRole('button', { name: 'Next' }).click() + await expect(page.getByText('Showing 21–40 of 562')).toBeVisible() + await page.getByRole('button', { name: 'Next' }).click() + await expect(page.getByText('Showing 41–60 of 562')).toBeVisible() + const link = page.url() + + // A new tab has the address, but not the history entry. + const other = await page.context().newPage() + await other.goto(link) + await expect(other.getByText('Showing 41–60 of 562')).toBeVisible() + await expect(other.getByRole('button', { name: 'Previous' })).toHaveCount(0) + await other.getByRole('button', { name: 'First page' }).click() + await expect(other.getByText('Showing 1–20 of 562')).toBeVisible() + await other.close() +}) + +test('the page and its controls stay while a slow next page loads', async ({ + page, +}) => { + await page.route('**/graphql', async (route) => { + const body = route.request().postDataJSON() as { + variables?: { after?: string } + } | null + if (body?.variables?.after) await new Promise((r) => setTimeout(r, 700)) + await route.continue() + }) + + await page.goto('elements') + await resultsLoaded(page) + const firstCodes = await rowCodes(page) + await page.getByRole('button', { name: 'Next' }).click() + + // While the page is on its way: the old rows, the line and the controls are there. + await expect(page.getByRole('button', { name: 'Next' })).toBeDisabled() + expect(await rowCodes(page)).toEqual(firstCodes) + await expect(page.getByText('Showing 1–20 of 562')).toBeVisible() + await expect(page.getByText('Loading elements…')).toHaveCount(0) + + await expect(page.getByText('Showing 21–40 of 562')).toBeVisible() + expect(await rowCodes(page)).not.toEqual(firstCodes) + await expect(page.getByRole('heading', { name: 'Results' })).toBeFocused() }) diff --git a/e2e/helpers.ts b/e2e/helpers.ts index e3e6ff5..5584dd4 100644 --- a/e2e/helpers.ts +++ b/e2e/helpers.ts @@ -1,13 +1,18 @@ import { expect, type Locator, type Page } from '@playwright/test' -/** The line under the results: "Showing 20 of 562" (or "1000+"). */ -const showing = (page: Page): Locator => page.getByText(/^Showing \d+ of /) +/** + * The line under the results: "Showing 1–20 of 562" (or "1000+"). A screen reader also + * hears "Page 1, " before it, so the line does not start with "Showing". + */ +const showing = (page: Page): Locator => + page.getByText(/Showing \d+(–\d+)? of /) -/** Waits for the results and gives the numbers of "Showing 20 of 562". */ +/** Waits for the results and gives the numbers of "Showing 21–40 of 562". */ export async function counts(page: Page) { const text = (await showing(page).first().textContent({ timeout: 15_000 }))! - const [, shown, total] = /Showing (\d+) of (\d+)/.exec(text)! - return { shown: Number(shown), total: Number(total) } + const [, first, last, total] = /Showing (\d+)(?:–(\d+))? of (\d+)/.exec(text)! + const shown = last ? Number(last) - Number(first) + 1 : 1 + return { first: Number(first), shown, total: Number(total) } } /** The page has loaded the first results. */ diff --git a/src/lib/searchOperations.ts b/src/lib/searchOperations.ts index 7b377cd..27e74e8 100644 --- a/src/lib/searchOperations.ts +++ b/src/lib/searchOperations.ts @@ -116,6 +116,8 @@ export type SearchVariables = { sort?: Sort first: number after?: string | null + /** Whether to count the results (left out: yes). The count is the same on every page. */ + withTotal?: boolean } type NodeFields = { @@ -143,9 +145,9 @@ type Page = { edges: { cursor: string; node: SearchNode }[] } -/** One search page, with the capped count of results in all. */ +/** One search page, with the capped count of results in all when it is asked for. */ export type SearchCatalogResult = { - searchCatalog: Page & { totalCount: number } + searchCatalog: Page & { totalCount?: number } } // The fields of one result. @@ -176,7 +178,12 @@ const RESULT = ` } ` -/** One page of a search, including the capped total count for direct page links. */ +/** + * One page of a search. The page decides with `withTotal` whether the API counts the + * results: counting is the expensive part of a search (see docs/API.md), and the count is + * the same on every page, so it is asked for once per search and not for each page. The + * fields of the results are the same on every page, so a page that is cached is complete. + */ export const SEARCH_CATALOG_QUERY: TypedDocumentNode< SearchCatalogResult, SearchVariables @@ -187,6 +194,7 @@ export const SEARCH_CATALOG_QUERY: TypedDocumentNode< $sort: CatalogSort $first: Int! $after: String + $withTotal: Boolean = true ) { searchCatalog( search: $search @@ -195,7 +203,7 @@ export const SEARCH_CATALOG_QUERY: TypedDocumentNode< first: $first after: $after ) { - totalCount + totalCount @include(if: $withTotal) pageInfo { hasNextPage endCursor diff --git a/src/pages/ElementsPage.tsx b/src/pages/ElementsPage.tsx index 0e90fe0..7cedcbe 100644 --- a/src/pages/ElementsPage.tsx +++ b/src/pages/ElementsPage.tsx @@ -6,7 +6,11 @@ import { type FormEvent, } from 'react' import { useApolloClient, useQuery } from '@apollo/client/react' -import { useLocation, useSearchParams } from 'react-router-dom' +import { + useLocation, + useSearchParams, + type NavigateOptions, +} from 'react-router-dom' import { Search, X } from 'lucide-react' import { MultiFilterMenu, @@ -47,44 +51,89 @@ import { takeResultsScroll } from '@/lib/resultsScroll' const PILL = 'h-10 rounded-full border-0 bg-(--button-bg) px-4 text-sm font-medium text-(--button-text) shadow-(--box-shadow) transition-colors hover:bg-(--button-hover-bg) focus-visible:bg-(--button-hover-bg) dark:border-transparent dark:bg-(--button-bg) dark:hover:bg-(--button-hover-bg) hover:text-(--button-text) dark:hover:text-(--button-text) focus-visible:ring-0 focus-visible:outline-2 focus-visible:outline-solid focus-visible:outline-offset-2 focus-visible:outline-(--text-color) motion-reduce:transition-none' -type CursorPage = { page: number; cursors: string[]; valid: boolean } +// The page is in the address as `?page=3&after=<cursor>`: one cursor, so the address +// does not grow with the page. The cursors of the pages before it (for Previous) and the +// count of the search are kept in the history entry (`location.state`), which survives +// Back, Forward and a reload, but not a link that is shared or typed: such a link still +// opens the right page, and offers "First page" instead of "Previous". +type PageState = { cursors: string[]; total?: number } + +type CursorPage = { + page: number + after?: string + /** + * The cursors of the last pages up to this one, the last being `after`. All of them + * (pages 2 to `page`) when the user paged here; only `after` for a shared link. + */ + cursors: string[] + total?: number + valid: boolean +} -function cursorPageFrom(params: URLSearchParams): CursorPage { +function pageStateFrom(state: unknown): PageState | undefined { + if (typeof state !== 'object' || state === null) return undefined + const { cursors, total } = state as Record<string, unknown> + if (!Array.isArray(cursors) || !cursors.every((c) => typeof c === 'string')) + return undefined + return { + cursors: cursors as string[], + total: typeof total === 'number' ? total : undefined, + } +} + +function cursorPageFrom(params: URLSearchParams, state: unknown): CursorPage { const rawPage = params.get('page') - const cursors = params.getAll('cursor') - if (rawPage === null && cursors.length === 0) - return { page: 1, cursors: [], valid: true } - - const page = rawPage === null ? Number.NaN : Number(rawPage) - const valid = - Number.isSafeInteger(page) && - page > 1 && - page === cursors.length + 1 && - cursors.every((cursor) => cursor.length > 0) - return valid - ? { page, cursors, valid: true } - : { page: 1, cursors: [], valid: false } + const after = params.get('after') + const saved = pageStateFrom(state) + if (rawPage === null && after === null) + return { page: 1, cursors: [], total: saved?.total, valid: true } + + const page = Number(rawPage) + if (!Number.isSafeInteger(page) || page < 2 || !after) + return { page: 1, cursors: [], valid: false } + // The state belongs to this address only when it ends with its cursor. + const fits = + saved !== undefined && + saved.cursors.length <= page - 1 && + saved.cursors.at(-1) === after + return { + page, + after, + cursors: fits ? saved.cursors : [after], + total: fits ? saved.total : undefined, + valid: true, + } } function clearCursorPage(params: URLSearchParams): URLSearchParams { const next = new URLSearchParams(params) next.delete('page') - next.delete('cursor') + next.delete('after') return next } function withCursorPage( params: URLSearchParams, - cursors: string[], + page: number, + after: string | undefined, ): URLSearchParams { const next = clearCursorPage(params) - if (cursors.length > 0) { - next.set('page', String(cursors.length + 1)) - for (const cursor of cursors) next.append('cursor', cursor) + if (page > 1 && after) { + next.set('page', String(page)) + next.set('after', after) } return next } +/** "Showing 21–40 of 562" for page 2 of 20 rows. */ +function showingText(page: number, rows: number, total?: number): string { + const first = (page - 1) * PAGE_SIZE + 1 + const range = rows === 1 && first === 1 ? '1' : `${first}–${first + rows - 1}` + return total === undefined + ? `Showing ${range}` + : `Showing ${range} of ${formatTotal(total)}` +} + export default function ElementsPage() { const [searchParams, setSearchParams] = useSearchParams() const location = useLocation() @@ -110,7 +159,7 @@ export default function ElementsPage() { }, [searchParams]) function changeAddress( update: (params: URLSearchParams) => URLSearchParams, - options?: { replace: boolean }, + options?: NavigateOptions, ) { const next = update(latest.current) latest.current = next @@ -138,9 +187,7 @@ export default function ElementsPage() { function changeSort(chosen: Sort) { changeAddress( (params) => { - const next = new URLSearchParams(params) - next.delete('page') - next.delete('cursor') + const next = clearCursorPage(params) next.delete('sort') next.delete('dir') for (const [key, value] of Object.entries(sortParams(chosen))) @@ -189,17 +236,16 @@ export default function ElementsPage() { searchParams.get('dir'), Boolean(filters.apparatusId), ) - const cursorPage = cursorPageFrom(searchParams) + const cursorPage = cursorPageFrom(searchParams, location.state) const pageNumber = cursorPage.page - const after = cursorPage.cursors.at(-1) + const after = cursorPage.after + // An address with a page but without a usable cursor (`?page=3`) cannot be restored, + // because a keyset page is found by its cursor and not by its number: it opens page 1. useEffect(() => { - if (!cursorPage.valid) { - changeAddress(clearCursorPage, { replace: true }) - } - // The address is the source of truth; `changeAddress` uses the latest ref. - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [cursorPage.valid, location.search]) + if (!cursorPage.valid) + setSearchParams((params) => clearCursorPage(params), { replace: true }) + }, [cursorPage.valid, setSearchParams]) // A changed filter replaces the cached page, so it starts from the first page. It is // a new step in the history, so the back button undoes it. @@ -216,7 +262,6 @@ export default function ElementsPage() { const apparatus = structure.find((a) => a.id === filters.apparatusId) const group = apparatus?.groups.find((g) => g.id === filters.groupId) - // The URL stores the cursor stack, so a page can be reloaded or shared. const searchVariables = { search: search || null, filter: toCatalogFilter(filters, structure), @@ -229,50 +274,79 @@ export default function ElementsPage() { ...searchVariables, first: PAGE_SIZE, ...(after ? { after } : {}), + // The count is the same on every page of a search, so it is asked for once and then + // carried from page to page in the history entry. + withTotal: cursorPage.total === undefined, } - const { data, loading, error, refetch } = useQuery(SEARCH_CATALOG_QUERY, { - variables: queryVariables, - skip: filtersPending || filtersBlocked, - }) + const { data, previousData, loading, error, refetch } = useQuery( + SEARCH_CATALOG_QUERY, + { + variables: queryVariables, + skip: filtersPending || filtersBlocked, + }, + ) // What the results area says when something failed: the structure of the filters when the // address needs it, otherwise the search. const shownError = filtersBlocked ? structureQuery.error : error const retry = filtersBlocked ? structureQuery.refetch : refetch - const shown = data?.searchCatalog + // While the next page (or a new search) is on its way, the rows that are shown stay, + // dimmed, so the list and the page controls do not disappear and the page does not + // jump. `fresh` is set only when the page in the address has arrived. + const fresh = loading ? undefined : data?.searchCatalog + const shown = (data ?? previousData)?.searchCatalog const rows = shown?.edges.map((edge) => toListRow(edge.node)) ?? [] - const hasNextPage = shown?.pageInfo.hasNextPage ?? false - const endCursor = shown?.pageInfo.endCursor - + const hasNextPage = fresh?.pageInfo.hasNextPage ?? false + const endCursor = fresh?.pageInfo.endCursor + const total = data?.searchCatalog.totalCount ?? cursorPage.total + + // What the text under the list says is the page that is shown, not the one on its way. + const [shownPage, setShownPage] = useState({ page: pageNumber, total }) + if (fresh && (shownPage.page !== pageNumber || shownPage.total !== total)) + setShownPage({ page: pageNumber, total }) + + // A page replaces the one before it in the cache (src/lib/apollo/cache.ts); this + // removes the elements that only the old page pointed to. It runs once for each page + // that arrives, and walks the cache, which holds one page. useEffect(() => { - if (data?.searchCatalog) apolloClient.cache.gc() - }, [apolloClient, data]) + if (fresh) apolloClient.cache.gc() + }, [apolloClient, fresh]) useEffect(() => { - if (!pageChangePendingFocus.current || loading || !shown) return + if (!pageChangePendingFocus.current || !fresh) return pageChangePendingFocus.current = false resultsHeading.current?.focus() - }, [loading, pageNumber, shown]) + }, [fresh]) useEffect(() => { - if (loading || !shown) return + if (!fresh) return const top = takeResultsScroll(location.key) if (top !== undefined) window.scrollTo({ top }) - }, [location.key, loading, shown]) + }, [location.key, fresh]) + + // Previous needs the cursor of the page before. Page 2 comes after page 1, which needs + // none; otherwise it is the cursor before the last one, if the history has it. + const knowsPrevious = pageNumber === 2 || cursorPage.cursors.length >= 2 - function changePage(cursors: string[]) { + function changePage(page: number, cursors: string[]) { pageChangePendingFocus.current = true - changeAddress((params) => withCursorPage(params, cursors)) + const state: PageState = { cursors, total } + changeAddress((params) => withCursorPage(params, page, cursors.at(-1)), { + state, + }) } function goToNextPage() { - if (!hasNextPage || !endCursor || loading) return - changePage([...cursorPage.cursors, endCursor]) + if (!hasNextPage || !endCursor) return + changePage(pageNumber + 1, [...cursorPage.cursors, endCursor]) } function goToPreviousPage() { - if (pageNumber <= 1 || loading) return - changePage(cursorPage.cursors.slice(0, -1)) + if (pageNumber <= 1) return + // Without the history (a shared link) the page before is unknown: go to page 1. + if (knowsPrevious) + changePage(pageNumber - 1, cursorPage.cursors.slice(0, -1)) + else changePage(1, []) } function submit(event: FormEvent) { @@ -435,53 +509,64 @@ export default function ElementsPage() { </p> ) : ( <> - {rows.length === 0 ? ( - <p - role="status" - className="m-0 rounded-lg bg-(--surface) p-5 font-medium text-(--text-color) shadow-(--box-shadow)" - > - {search - ? `No elements match “${search}”${hasFilters(filters) ? ' with these filters' : ''}.` - : hasFilters(filters) - ? 'No elements match these filters.' - : 'There are no elements.'} - </p> - ) : ( - <ResultsTable rows={rows} /> - )} - <div className="mt-4 flex flex-wrap items-center justify-between gap-3"> - <div> + <div + className={ + fresh + ? undefined + : 'opacity-60 transition-opacity motion-reduce:transition-none' + } + > + {rows.length === 0 ? ( <p role="status" - aria-live="polite" - className="text-sm font-medium text-(--text-color)" + className="m-0 rounded-lg bg-(--surface) p-5 font-medium text-(--text-color) shadow-(--box-shadow)" > - {`Page ${pageNumber}, showing ${rows.length} results`} + {search + ? `No elements match “${search}”${hasFilters(filters) ? ' with these filters' : ''}.` + : hasFilters(filters) + ? 'No elements match these filters.' + : 'There are no elements.'} </p> + ) : ( + <ResultsTable rows={rows} /> + )} + </div> + <div className="mt-4 flex flex-wrap items-center justify-between gap-3"> + {/* One line for what is shown: "Showing 21–40 of 562". A screen reader + hears the page number with it when the page changes. */} + <p + role="status" + aria-live="polite" + className="m-0 text-sm font-medium text-(--text-color)" + > {rows.length > 0 && ( - <p className="m-0 text-sm font-medium text-(--text-color)"> - {`Showing ${rows.length} of ${formatTotal(shown.totalCount)}`} - </p> + <> + <span className="sr-only">{`Page ${shownPage.page}, `}</span> + {showingText(shownPage.page, rows.length, shownPage.total)} + </> )} - </div> + </p> <nav aria-label="Pagination" className="flex items-center gap-3"> <Button type="button" variant="outline" className={PILL} - disabled={pageNumber === 1 || loading} + // Not while a page is on its way: a double click would skip a page. + disabled={pageNumber === 1 || !fresh} onClick={goToPreviousPage} > - Previous + {knowsPrevious || pageNumber === 1 + ? 'Previous' + : 'First page'} </Button> <span aria-current="page" className="text-sm font-medium"> - Page {pageNumber} + Page {shownPage.page} </span> <Button type="button" variant="outline" className={PILL} - disabled={!hasNextPage || loading} + disabled={!hasNextPage || !fresh} onClick={goToNextPage} > Next diff --git a/src/tests/pages/ElementsPage.test.tsx b/src/tests/pages/ElementsPage.test.tsx index c16fb98..fcf3f82 100644 --- a/src/tests/pages/ElementsPage.test.tsx +++ b/src/tests/pages/ElementsPage.test.tsx @@ -201,6 +201,18 @@ function renderPage( } const field = () => screen.getByRole<HTMLInputElement>('searchbox') + +/** + * Waits until page `n` is shown: the line under the list starts with "Page n, " for a + * screen reader. (The test servers make pages of 4 rows, so the numbers of "Showing + * 21–23" are those of 20-row pages and are not what the tests look at.) + */ +const onPage = (n: number) => + screen.findByText( + (_, element) => + element?.getAttribute('role') === 'status' && + (element.textContent ?? '').startsWith(`Page ${n}, `), + ) const address = () => screen.getByTestId('address').textContent const wait = { timeout: 3000 } @@ -708,20 +720,20 @@ describe('ElementsPage: the filters', () => { it('starts a changed filter on page one and supports paging with the filter', async () => { const { calls } = renderPage(pagedServer(4)) - await screen.findByText(/Showing 4 of/) + await screen.findByText(/Showing 1–4 of/) fireEvent.click(screen.getByRole('button', { name: 'Next' })) - await screen.findByText(/Page 2, showing 3 results/) + await onPage(2) await openMenu('Apparatus') choose('menuitemradio', 'Uneven bars') - await screen.findByText(/Showing 4 of/) + await screen.findByText(/Showing 1–4 of/) const last = calls.at(-1)! expect(last.operationName).toBe('SearchCatalog') expect(last.variables).not.toHaveProperty('after') expect(last.variables['filter']).toEqual({ apparatusId: 'UB' }) fireEvent.click(screen.getByRole('button', { name: 'Next' })) - await screen.findByText(/Page 2, showing 3 results/) + await onPage(2) expect(calls.at(-1)!.variables).toMatchObject({ filter: { apparatusId: 'UB' }, after: expect.any(String), @@ -841,10 +853,10 @@ describe('ElementsPage: the sort', () => { it('starts a new sort on page one and supports paging with the same sort', async () => { const { calls } = renderPage(pagedServer(4)) - await screen.findByText(/Showing 4 of/) + await screen.findByText(/Showing 1–4 of/) fireEvent.click(screen.getByRole('button', { name: 'Next' })) - await screen.findByText(/Page 2, showing 3 results/) + await onPage(2) expect(calls.at(-1)).toMatchObject({ operationName: 'SearchCatalog', variables: { after: expect.any(String) }, @@ -852,14 +864,14 @@ describe('ElementsPage: the sort', () => { await openMenu(/Sort by/) chooseSort('Name: A to Z') - await screen.findByText(/Showing 4 of/) + await screen.findByText(/Showing 1–4 of/) const last = calls.at(-1)! expect(last.operationName).toBe('SearchCatalog') expect(last.variables).not.toHaveProperty('after') expect(last.variables).toMatchObject({ sort: { field: 'NAME' } }) fireEvent.click(screen.getByRole('button', { name: 'Next' })) - await screen.findByText(/Page 2, showing 3 results/) + await onPage(2) expect(calls.at(-1)!.variables).toMatchObject({ sort: { field: 'NAME', direction: 'ASC' }, after: expect.any(String), @@ -1036,7 +1048,7 @@ describe('ElementsPage: the results from the API', () => { expect( await screen.findByText( - `Showing ${exampleElementRows.length} of ${exampleTotalCount}`, + `Showing 1–${exampleElementRows.length} of ${exampleTotalCount}`, ), ).toBeTruthy() expect(calls).toHaveLength(1) @@ -1055,7 +1067,9 @@ describe('ElementsPage: the results from the API', () => { renderPage(() => searchAnswer(exampleElementRows, 1001, true)) expect( - await screen.findByText(`Showing ${exampleElementRows.length} of 1000+`), + await screen.findByText( + `Showing 1–${exampleElementRows.length} of 1000+`, + ), ).toBeTruthy() expect(screen.queryByText(/1001/)).toBeNull() }) @@ -1064,7 +1078,7 @@ describe('ElementsPage: the results from the API', () => { renderPage(() => searchAnswer(exampleElementRows, 1000, true)) expect( - await screen.findByText(`Showing ${exampleElementRows.length} of 1000`), + await screen.findByText(`Showing 1–${exampleElementRows.length} of 1000`), ).toBeTruthy() }) @@ -1183,7 +1197,7 @@ describe('ElementsPage: the search', () => { await waitFor(() => expect(address()).toBe('/elements'), wait) expect( await screen.findByText( - `Showing ${exampleElementRows.length} of ${exampleTotalCount}`, + `Showing 1–${exampleElementRows.length} of ${exampleTotalCount}`, {}, wait, ), @@ -1210,7 +1224,7 @@ describe('ElementsPage: the search', () => { expect(calls[1].variables).toMatchObject({ search: 'biles' }) }) - it('does not show results from the previous search while a new search is loading', async () => { + it('keeps the old results while a new search is on its way', async () => { const second = deferred<Answer>() renderPage(({ variables }) => variables['search'] ? second.promise : searchAnswer(exampleElementRows), @@ -1220,10 +1234,15 @@ describe('ElementsPage: the search', () => { fireEvent.change(field(), { target: { value: 'biles' } }) fireEvent.submit(screen.getByRole('search')) - // The new answer has not come, and the old results are no longer rendered. + // The new answer has not come, and the old results are still there. await waitFor(() => expect(address()).toBe('/elements?search=biles')) - expect(screen.getByText('Loading elements…')).toBeTruthy() - expect(document.querySelectorAll('tbody tr')).toHaveLength(0) + expect( + screen.getByText(`Showing 1–${exampleElementRows.length} of 7`), + ).toBeTruthy() + expect(document.querySelectorAll('tbody tr')).toHaveLength( + exampleElementRows.length, + ) + expect(screen.queryByText('Loading elements…')).toBeNull() second.resolve(searchAnswer([exampleElementRows[5]])) expect(await screen.findByText('Showing 1 of 1')).toBeTruthy() @@ -1251,9 +1270,9 @@ describe('ElementsPage: the search', () => { describe('ElementsPage: pagination', () => { const tableRows = () => document.querySelectorAll('tbody tr') - it('replaces the first page with the next page and puts the cursor stack in the URL', async () => { + it('replaces the first page with the next page and puts its cursor in the address', async () => { const { calls } = renderPage(pagedServer(4)) - await screen.findByText('Showing 4 of 7') + await screen.findByText('Showing 1–4 of 7') expect(tableRows()).toHaveLength(4) expect( screen.getByRole('button', { name: 'Previous' }).hasAttribute('disabled'), @@ -1264,14 +1283,14 @@ describe('ElementsPage: pagination', () => { fireEvent.click(screen.getByRole('button', { name: 'Next' })) - expect(await screen.findByText(/Page 2, showing 3 results/)).toBeTruthy() + expect(await onPage(2)).toBeTruthy() expect(tableRows()).toHaveLength(3) const codes = [...tableRows()].map( (row) => row.querySelector('td')?.textContent, ) expect(codes).toEqual(exampleElementRows.slice(4).map((row) => row.code)) expect(address()).toBe( - `/elements?page=2&cursor=${encodeURIComponent(exampleElementRows[3].id)}`, + `/elements?page=2&after=${encodeURIComponent(exampleElementRows[3].id)}`, ) expect(calls).toHaveLength(2) expect(calls[1]).toMatchObject({ @@ -1286,13 +1305,13 @@ describe('ElementsPage: pagination', () => { it('goes back to the prior page by requesting its cursor again', async () => { const { calls } = renderPage(pagedServer(4)) - await screen.findByText('Showing 4 of 7') + await screen.findByText('Showing 1–4 of 7') fireEvent.click(screen.getByRole('button', { name: 'Next' })) - await screen.findByText(/Page 2, showing 3 results/) + await onPage(2) expect(calls).toHaveLength(2) fireEvent.click(screen.getByRole('button', { name: 'Previous' })) - await screen.findByText(/Page 1, showing 4 results/) + await screen.findByText('Showing 1–4 of 7') expect(tableRows()).toHaveLength(4) expect(calls).toHaveLength(3) expect(calls[2].variables['after']).toBeUndefined() @@ -1300,29 +1319,34 @@ describe('ElementsPage: pagination', () => { it('announces page changes and moves focus to the results heading', async () => { renderPage(pagedServer(4)) - await screen.findByText('Showing 4 of 7') + await screen.findByText('Showing 1–4 of 7') fireEvent.click(screen.getByRole('button', { name: 'Next' })) - expect(await screen.findByText('Page 2, showing 3 results')).toBeTruthy() + expect(await onPage(2)).toBeTruthy() expect(await screen.findByRole('heading', { name: 'Results' })).toBe( document.activeElement, ) }) - it('keeps only the current page visible while a new page loads', async () => { + it('keeps the page and its controls while the next page loads, and cannot skip a page', async () => { const next = deferred<Answer>() const { calls } = renderPage((operation) => operation.variables['after'] ? next.promise : pagedServer(4)(operation), ) - await screen.findByText('Showing 4 of 7') + await screen.findByText('Showing 1–4 of 7') - fireEvent.click(screen.getByRole('button', { name: 'Next' })) - await waitFor(() => - expect(screen.getByText('Loading elements…')).toBeTruthy(), - ) - expect(tableRows()).toHaveLength(0) + const nextButton = screen.getByRole('button', { name: 'Next' }) + fireEvent.click(nextButton) + // The rows of page 1 stay (no layout jump), and the controls stay but are disabled: + // a second click would skip a page. + await waitFor(() => expect(nextButton.hasAttribute('disabled')).toBe(true)) + expect(tableRows()).toHaveLength(4) + expect(screen.getByText('Showing 1–4 of 7')).toBeTruthy() + expect(screen.getByRole('navigation', { name: 'Pagination' })).toBeTruthy() + expect(screen.queryByText('Loading elements…')).toBeNull() + fireEvent.click(nextButton) next.resolve(searchAnswer(exampleElementRows.slice(4), 7)) - expect(await screen.findByText(/Page 2, showing 3 results/)).toBeTruthy() + expect(await onPage(2)).toBeTruthy() expect(tableRows()).toHaveLength(3) expect(calls).toHaveLength(2) }) @@ -1335,7 +1359,7 @@ describe('ElementsPage: pagination', () => { } return pagedServer(4)(operation) }) - await screen.findByText('Showing 4 of 7') + await screen.findByText('Showing 1–4 of 7') fireEvent.click(screen.getByRole('button', { name: 'Next' })) @@ -1345,7 +1369,7 @@ describe('ElementsPage: pagination', () => { ) expect(tableRows()).toHaveLength(0) fireEvent.click(screen.getByRole('button', { name: 'Try again' })) - expect(await screen.findByText(/Page 2, showing 3 results/)).toBeTruthy() + expect(await onPage(2)).toBeTruthy() expect(screen.queryByRole('alert')).toBeNull() expect(calls).toHaveLength(3) }) @@ -1354,9 +1378,9 @@ describe('ElementsPage: pagination', () => { const cursor = exampleElementRows[3].id const { calls } = renderPage( pagedServer(4), - `/elements?page=2&cursor=${encodeURIComponent(cursor)}`, + `/elements?page=2&after=${encodeURIComponent(cursor)}`, ) - expect(await screen.findByText(/Page 2, showing 3 results/)).toBeTruthy() + expect(await onPage(2)).toBeTruthy() expect(tableRows()).toHaveLength(3) expect(calls[0].variables).toMatchObject({ first: PAGE_SIZE, @@ -1382,10 +1406,12 @@ describe('ElementsPage: pagination', () => { ) }) - await screen.findByText('Page 1, showing 20 results') + await screen.findByText(`Showing 1–20 of 200`) for (let page = 2; page <= 10; page += 1) { fireEvent.click(screen.getByRole('button', { name: 'Next' })) - await screen.findByText(`Page ${page}, showing 20 results`) + await screen.findByText( + `Showing ${(page - 1) * 20 + 1}–${page * 20} of 200`, + ) } expect(document.querySelectorAll('tbody tr')).toHaveLength(20) @@ -1400,13 +1426,80 @@ describe('ElementsPage: pagination', () => { expect(cachedCatalogRows).toHaveLength(20) }) + it('counts the results once per search, and carries the count to the later pages', async () => { + const { calls } = renderPage(pagedServer(2)) + await screen.findByText('Showing 1–2 of 7') + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await onPage(2) + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await onPage(3) + + expect(print(SEARCH_CATALOG_QUERY)).toContain( + 'totalCount @include(if: $withTotal)', + ) + expect(calls.map((call) => call.variables['withTotal'])).toEqual([ + true, + false, + false, + ]) + // The count is still shown on page 3, from the first page. + expect(screen.getByText(/of 7$/)).toBeTruthy() + }) + + it('keeps one cursor in the address and the earlier ones in the history, so Previous walks back', async () => { + renderPage(pagedServer(2)) + await screen.findByText('Showing 1–2 of 7') + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await onPage(2) + fireEvent.click(screen.getByRole('button', { name: 'Next' })) + await onPage(3) + expect(address()).toBe( + `/elements?page=3&after=${encodeURIComponent(exampleElementRows[3].id)}`, + ) + + fireEvent.click(screen.getByRole('button', { name: 'Previous' })) + await onPage(2) + expect(address()).toBe( + `/elements?page=2&after=${encodeURIComponent(exampleElementRows[1].id)}`, + ) + expect(tableRows().length).toBe(2) + fireEvent.click(screen.getByRole('button', { name: 'Previous' })) + await onPage(1) + expect(address()).toBe('/elements') + }) + + it('opens a shared link to a later page, and offers "First page" because the history is not in it', async () => { + const { calls } = renderPage( + pagedServer(2), + `/elements?page=3&after=${encodeURIComponent(exampleElementRows[3].id)}`, + ) + await onPage(3) + expect(tableRows().length).toBe(2) + // A link has no count with it, so this page asks for one. + expect(calls[0].variables['withTotal']).toBe(true) + expect(screen.queryByRole('button', { name: 'Previous' })).toBeNull() + + fireEvent.click(screen.getByRole('button', { name: 'First page' })) + await onPage(1) + expect(address()).toBe('/elements') + }) + + it('opens page 1 for a page number without a cursor, which cannot be restored', async () => { + const { calls } = renderPage(pagedServer(2), '/elements?page=3&search=a') + await onPage(1) + await waitFor(() => expect(address()).toBe('/elements?search=a')) + expect(calls.every((call) => call.variables['after'] === undefined)).toBe( + true, + ) + }) + it('starts again with page one when the search changes', async () => { const { calls } = renderPage((operation) => operation.variables['after'] && !operation.variables['search'] ? failure('INTERNAL_SERVER_ERROR', 'Something broke') : serverWithRowsPaged(operation), ) - await screen.findByText('Showing 4 of 7') + await screen.findByText('Showing 1–4 of 7') fireEvent.click(screen.getByRole('button', { name: 'Next' })) await screen.findByRole('alert')