From c48625cb0cc5e7987ef7bdce79cf88f5e49a2f58 Mon Sep 17 00:00:00 2001 From: Nicolas Date: Sat, 5 Sep 2026 20:20:43 +0200 Subject: [PATCH] fix(recipes): stoppe le scroll infini des sources au lieu de boucler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deux volets de l'issue #38. **Bug de bouclage** — l'`IntersectionObserver` de `SourceItemTable` n'a que `[hasMore]` comme dépendance et ne se réabonne donc jamais pendant un scroll normal (`hasMore` reste `true`). Il figeait la toute première closure `onLoadMore`, laquelle rappelait indéfiniment le même curseur périmé — la liste s'allongeait en boucle avec les mêmes éléments et `nextCursor` n'atteignait jamais son vrai `null`. - `SourceItemTable` lit maintenant `onLoadMore` via une ref réassignée à chaque rendu : l'observer appelle toujours la closure fraîche, sans réabonnement inutile (intention d'origine du `biome-ignore` conservée, le commentaire n'étant plus nécessaire). - `RecipeSourcesPanel` dédoublonne le merge par `externalId` et traite « cette page n'a rien apporté de nouveau » comme une fin de liste même si la source renvoie encore un `nextCursor` — garde-fou contre une éventuelle heuristique de fin imparfaite côté adaptateur. **Couverture recherche vide** — `theMealDbAdapter.list()` tapait `/search.php?s=` même sans requête, ce que TheMealDB borne à ~25 recettes (`nextCursor` toujours `null`). Une requête vide déroule désormais la liste des catégories (`/list.php?c=list` puis `/filter.php?c=…`), une catégorie par pas de curseur, pour un vrai « tout parcourir ». Tests : nouveau scénario Cypress à 3 pages avec curseurs distincts réellement vérifiés (aucun curseur redemandé) ; nouveaux cas Mocha pour la marche par catégories (première page + curseur, suivi du curseur, arrêt sur la dernière catégorie, page vide au-delà, requête blanche = requête vide). Closes #38 Co-Authored-By: Claude Sonnet 5 --- apps/api/src/sources/the-meal-db.ts | 105 ++++++++++---- .../test/recipe-sources/the-meal-db.test.ts | 129 ++++++++++++++++++ apps/web/cypress/e2e/recipe-sources.feature | 12 ++ apps/web/cypress/e2e/recipe-sources.ts | 41 ++++++ .../recipes/sources/RecipeSourcesPanel.tsx | 21 ++- .../recipes/sources/SourceItemTable.tsx | 18 ++- specs/backend-architecture.md | 15 +- 7 files changed, 304 insertions(+), 37 deletions(-) diff --git a/apps/api/src/sources/the-meal-db.ts b/apps/api/src/sources/the-meal-db.ts index e819bab..cedd8e1 100644 --- a/apps/api/src/sources/the-meal-db.ts +++ b/apps/api/src/sources/the-meal-db.ts @@ -66,20 +66,91 @@ function detailUrl(idMeal: string): string { return `https://www.themealdb.com/meal/${idMeal}`; } +/** Category-list response shape (`/list.php?c=list`) — same `meals` envelope as everything else, but each row only carries `strCategory`. */ +interface TheMealDbCategoryListResponse { + meals: { strCategory: string | null }[] | null; +} + +/** Narrows to meals that actually carry a name — a titleless row isn't worth surfacing in a browse list. */ +function hasName(meal: TheMealDbMeal): meal is TheMealDbMeal & { strMeal: string } { + return Boolean(meal.strMeal); +} + +/** Maps one raw meal (from `/search.php` or `/filter.php`) into the generic list-item shape. */ +function toListItem(meal: TheMealDbMeal & { strMeal: string }) { + return { + externalId: meal.idMeal, + title: meal.strMeal, + picture: meal.strMealThumb, + url: detailUrl(meal.idMeal), + }; +} + +/** + * Name search (`/search.php?s=`) — the only mode TheMealDB's free tier + * offers for a non-empty query. One response holds every match, so there's + * never a next page. + */ +async function searchByName(query: string): Promise { + try { + const data = await fetchTheMealDb( + `/search.php?s=${encodeURIComponent(query)}`, + ); + return { items: (data.meals ?? []).filter(hasName).map(toListItem), nextCursor: null }; + } catch (err) { + throw err; // see fetchTheMealDb()'s catch comment above + } +} + +/** + * "Browse everything" (an empty/omitted query). TheMealDB's free tier has + * no catalog-wide listing endpoint — `/search.php?s=` only answers with a + * fixed ~25-meal sample — so this walks the category list instead: + * `/list.php?c=list` for the categories, then `/filter.php?c={category}` + * for one category at a time, exposing **one category per cursor step** so + * the infinite scroll keeps pulling real, distinct pages instead of + * stalling on that sample (issue #38). The category list is re-fetched on + * each step rather than cached in module state — it's a single small + * request, it keeps this adapter stateless like the others, and the walk + * is paced one step per scroll anyway. + */ +async function listByCategory(cursor: string | null | undefined): Promise { + try { + const parsed = Number.parseInt(cursor ?? "0", 10); + const startAt = Number.isNaN(parsed) || parsed < 0 ? 0 : parsed; + + const categoryList = await fetchTheMealDb("/list.php?c=list"); + const categories = (categoryList.meals ?? []) + .map((row) => row.strCategory) + .filter((name): name is string => Boolean(name)); + + const category = categories[startAt]; + if (category === undefined) { + // Cursor has walked past the last category — the clean end of the list. + return { items: [], nextCursor: null }; + } + + const data = await fetchTheMealDb( + `/filter.php?c=${encodeURIComponent(category)}`, + ); + const nextCursor = startAt + 1 < categories.length ? String(startAt + 1) : null; + return { items: (data.meals ?? []).filter(hasName).map(toListItem), nextCursor }; + } catch (err) { + throw err; // see fetchTheMealDb()'s catch comment above + } +} + /** * TheMealDB (themealdb.com) — a free, public recipe API (no scraping: the * publisher's own structured JSON, hence `official: true`). The first real * `RecipeSourceAdapter` implementation, proving the generic contract * (recipe-source-adapter.ts) end to end against a live source. * - * `list()` is search-only — TheMealDB has no dedicated "browse everything" - * endpoint on its free tier. An omitted `query` searches for an empty - * string, which TheMealDB happens to answer with a small default sample - * (~25 meals) rather than nothing — close enough to this contract's - * "omitted `query` means browse everything" convention - * (`RecipeSourceListParams.query`) to lean on as-is, though it's a fixed - * sample, not the whole catalog. Search isn't paginated either — one - * response holds every match, so `nextCursor` is always `null`. + * `list()` has two modes (see the helpers above): a non-empty `query` is a + * name search (`/search.php?s=`, single unpaginated response, + * `nextCursor: null`); an empty/omitted `query` — this contract's "browse + * everything" — walks the category list one category per cursor step, + * since the free tier has no real catalog-wide endpoint. */ export const theMealDbAdapter: RecipeSourceAdapter = { key: SOURCE_KEY, @@ -93,22 +164,8 @@ export const theMealDbAdapter: RecipeSourceAdapter = { async list(params: RecipeSourceListParams): Promise { try { - const query = params.query ?? ""; - const data = await fetchTheMealDb( - `/search.php?s=${encodeURIComponent(query)}`, - ); - const meals = data.meals ?? []; - return { - items: meals - .filter((meal): meal is TheMealDbMeal & { strMeal: string } => Boolean(meal.strMeal)) - .map((meal) => ({ - externalId: meal.idMeal, - title: meal.strMeal, - picture: meal.strMealThumb, - url: detailUrl(meal.idMeal), - })), - nextCursor: null, - }; + const query = (params.query ?? "").trim(); + return query.length > 0 ? await searchByName(query) : await listByCategory(params.cursor); } catch (err) { throw err; // see fetchTheMealDb()'s catch comment above } diff --git a/apps/api/test/recipe-sources/the-meal-db.test.ts b/apps/api/test/recipe-sources/the-meal-db.test.ts index d79d694..8bf995a 100644 --- a/apps/api/test/recipe-sources/the-meal-db.test.ts +++ b/apps/api/test/recipe-sources/the-meal-db.test.ts @@ -16,6 +16,28 @@ function stubFetch(body: unknown, status = 200) { globalThis.fetch = (async () => new Response(JSON.stringify(body), { status })) as typeof fetch; } +/** + * Route-aware variant of {@link stubFetch} for the empty-query category + * walk, which fires several different requests (`/list.php?c=list`, then a + * `/filter.php?c=…`) within a single `list()` call. Matches by URL + * substring; an unstubbed path throws rather than silently returning junk. + * Returns the list of requested URLs, so a test can assert *which* + * endpoint was hit (e.g. category walk vs. `/search.php`). + */ +function stubFetchByUrl(routes: Record, status = 200): string[] { + const calls: string[] = []; + globalThis.fetch = (async (input: string | URL | Request) => { + const url = typeof input === "string" ? input : input.toString(); + calls.push(url); + const route = Object.keys(routes).find((fragment) => url.includes(fragment)); + if (route === undefined) { + throw new Error(`stubFetchByUrl: no route matches ${url}`); + } + return new Response(JSON.stringify(routes[route]), { status }); + }) as typeof fetch; + return calls; +} + const baseMeal: TheMealDbMeal = { idMeal: "52795", strMeal: "Chicken Handi", @@ -109,6 +131,113 @@ describe("theMealDbAdapter", () => { expect((err as RecipeSourceFetchError).cause).to.be.instanceOf(Error); } }); + + // Empty/omitted query = "browse everything". The free tier has no + // catalog-wide endpoint, so the adapter walks categories one per + // cursor step (issue #38) rather than settling for `/search.php?s=`'s + // fixed ~25-meal sample. + describe("empty query — walks categories", () => { + const CATEGORIES = { + meals: [{ strCategory: "Beef" }, { strCategory: "Chicken" }, { strCategory: "Dessert" }], + }; + + it("returns the first category's meals, cursor pointing at the next category", async () => { + stubFetchByUrl({ + "/list.php?c=list": CATEGORIES, + "/filter.php?c=Beef": { + meals: [ + { idMeal: "1", strMeal: "Beef Pie", strMealThumb: "https://img.test/1.jpg" }, + { idMeal: "2", strMeal: "Beef Stew", strMealThumb: null }, + ], + }, + }); + + const result = await theMealDbAdapter.list({}); + + expect(result.items).to.deep.equal([ + { + externalId: "1", + title: "Beef Pie", + picture: "https://img.test/1.jpg", + url: "https://www.themealdb.com/meal/1", + }, + { + externalId: "2", + title: "Beef Stew", + picture: null, + url: "https://www.themealdb.com/meal/2", + }, + ]); + expect(result.nextCursor).to.equal("1"); + }); + + it("follows the cursor into the next category", async () => { + const calls = stubFetchByUrl({ + "/list.php?c=list": CATEGORIES, + "/filter.php?c=Chicken": { + meals: [{ idMeal: "3", strMeal: "Chicken Handi", strMealThumb: null }], + }, + }); + + const result = await theMealDbAdapter.list({ cursor: "1" }); + + expect(calls.some((u) => u.includes("/filter.php?c=Chicken"))).to.equal(true); + expect(result.items.map((i) => i.externalId)).to.deep.equal(["3"]); + expect(result.nextCursor).to.equal("2"); + }); + + it("stops (null cursor) on the last category", async () => { + stubFetchByUrl({ + "/list.php?c=list": CATEGORIES, + "/filter.php?c=Dessert": { + meals: [{ idMeal: "9", strMeal: "Flan", strMealThumb: null }], + }, + }); + + const result = await theMealDbAdapter.list({ cursor: "2" }); + + expect(result.nextCursor).to.be.null; + }); + + it("returns an empty final page once the cursor is past the last category", async () => { + stubFetchByUrl({ "/list.php?c=list": CATEGORIES }); + + const result = await theMealDbAdapter.list({ cursor: "3" }); + + expect(result.items).to.deep.equal([]); + expect(result.nextCursor).to.be.null; + }); + + it("treats a blank query the same as no query — category walk, never /search.php", async () => { + const calls = stubFetchByUrl({ + "/list.php?c=list": CATEGORIES, + "/filter.php?c=Beef": { + meals: [{ idMeal: "1", strMeal: "Beef Pie", strMealThumb: null }], + }, + }); + + await theMealDbAdapter.list({ query: " " }); + + expect(calls.some((u) => u.includes("/search.php"))).to.equal(false); + expect(calls.some((u) => u.includes("/list.php?c=list"))).to.equal(true); + }); + + it("skips a titleless meal in a category page, like search does", async () => { + stubFetchByUrl({ + "/list.php?c=list": CATEGORIES, + "/filter.php?c=Beef": { + meals: [ + { idMeal: "1", strMeal: null, strMealThumb: null }, + { idMeal: "2", strMeal: "Beef Stew", strMealThumb: null }, + ], + }, + }); + + const result = await theMealDbAdapter.list({}); + + expect(result.items.map((i) => i.externalId)).to.deep.equal(["2"]); + }); + }); }); describe("fetchDetail", () => { diff --git a/apps/web/cypress/e2e/recipe-sources.feature b/apps/web/cypress/e2e/recipe-sources.feature index ee6a3ae..aee6fb6 100644 --- a/apps/web/cypress/e2e/recipe-sources.feature +++ b/apps/web/cypress/e2e/recipe-sources.feature @@ -56,6 +56,18 @@ Feature: Browsing external recipe sources And I should see the source item "Beef Wellington" And I should not see "Voir plus" + Scenario: Walks several pages with distinct cursors, then stops, without re-requesting a page + Given the recipe catalog contains nothing + And the sources reference list has options + And the household has enabled TheMealDB + And browsing TheMealDB returns three pages, each addressed by its own cursor + When I visit "/recettes" + And I click the button "TheMealDB" + Then I should see the source item "Page One Dish" + And I should see the source item "Page Two Dish" + And I should see the source item "Page Three Dish" + And no browse cursor should have been requested more than once + Scenario: Offers a retry when loading the next page fails Given the recipe catalog contains nothing And the sources reference list has options diff --git a/apps/web/cypress/e2e/recipe-sources.ts b/apps/web/cypress/e2e/recipe-sources.ts index 01e50d2..47664c1 100644 --- a/apps/web/cypress/e2e/recipe-sources.ts +++ b/apps/web/cypress/e2e/recipe-sources.ts @@ -132,6 +132,47 @@ Given("browsing TheMealDB returns two pages of items", () => { }); }); +/** + * Three real pages, each addressed by a distinct `cursor` value the mock + * actually parses (not just "does the URL contain `cursor=`"). Records + * every cursor it serves so the scenario can assert none was requested + * twice — the tell-tale of the issue #38 loop, where a stale closure + * re-requests the same page forever instead of advancing. + */ +const threePageCursorsRequested: string[] = []; + +Given("browsing TheMealDB returns three pages, each addressed by its own cursor", () => { + threePageCursorsRequested.length = 0; + const item = (externalId: string, title: string) => ({ + externalId, + title, + picture: null, + url: `https://www.themealdb.com/meal/${externalId}`, + alreadyImported: false, + recipeId: null, + }); + const pages: Record = { + "": { items: [item("p1", "Page One Dish")], nextCursor: "cursor-2" }, + "cursor-2": { items: [item("p2", "Page Two Dish")], nextCursor: "cursor-3" }, + "cursor-3": { items: [item("p3", "Page Three Dish")], nextCursor: null }, + }; + cy.intercept("GET", "**/sources/theMealDb/browse*", (req) => { + const cursor = new URL(req.url).searchParams.get("cursor") ?? ""; + threePageCursorsRequested.push(cursor); + req.reply({ statusCode: 200, body: pages[cursor] ?? { items: [], nextCursor: null } }); + }); +}); + +Then("no browse cursor should have been requested more than once", () => { + // Let the panel settle (auto-scroll + one-page-ahead prefetch) before + // asserting. A looping IntersectionObserver would re-hit "" or "cursor-2" + // here; the latest-ref fix keeps each cursor to a single request. + cy.wait(600).then(() => { + const seen = [...threePageCursorsRequested]; + expect(seen.length, `cursors requested: ${JSON.stringify(seen)}`).to.equal(new Set(seen).size); + }); +}); + // The panel prefetches the next page as soon as page 1 is on screen (before // anyone's actually waited on it), so the *first* request for it is that // prefetch — this is what actually fails "once", not a request triggered by diff --git a/apps/web/src/features/recipes/sources/RecipeSourcesPanel.tsx b/apps/web/src/features/recipes/sources/RecipeSourcesPanel.tsx index 3b86981..f38e7ae 100644 --- a/apps/web/src/features/recipes/sources/RecipeSourcesPanel.tsx +++ b/apps/web/src/features/recipes/sources/RecipeSourcesPanel.tsx @@ -236,11 +236,22 @@ export function RecipeSourcesPanel({ .then(({ items, nextCursor }) => { nextPagePrefetchRef.current = null; isLoadingMoreRef.current = false; - setBrowseState((prev) => - prev.status === "loaded" - ? { status: "loaded", items: [...prev.items, ...items], nextCursor } - : prev, - ); + setBrowseState((prev) => { + if (prev.status !== "loaded") return prev; + // Merge deduplicated by `externalId`, and treat "this page added + // nothing genuinely new" as the end of the list even if the + // source still returned a non-null `nextCursor`. Belt-and-braces + // with `SourceItemTable`'s latest-ref fix against the issue #38 + // loop (a stale cursor re-fetching the same page forever), and a + // guard against a source whose own end-of-catalog heuristic is + // imperfect (e.g. 750g's "page with no cards"). + const seen = new Set(prev.items.map((item) => item.externalId)); + const freshItems = items.filter((item) => !seen.has(item.externalId)); + if (freshItems.length === 0) { + return { status: "loaded", items: prev.items, nextCursor: null }; + } + return { status: "loaded", items: [...prev.items, ...freshItems], nextCursor }; + }); setLoadMoreStatus("idle"); }) .catch(() => { diff --git a/apps/web/src/features/recipes/sources/SourceItemTable.tsx b/apps/web/src/features/recipes/sources/SourceItemTable.tsx index df7ee14..dc562a1 100644 --- a/apps/web/src/features/recipes/sources/SourceItemTable.tsx +++ b/apps/web/src/features/recipes/sources/SourceItemTable.tsx @@ -45,12 +45,24 @@ export function SourceItemTable({ const { t } = useTranslation(); const scrollContainerRef = useRef(null); const sentinelRef = useRef(null); + // Keeps the observer callback pointed at the *current* render's + // `onLoadMore` without re-subscribing the observer on every render. + // `RecipeSourcesPanel` re-declares `handleLoadMore` — and the + // `browseState.nextCursor` it closes over — on every render; the effect + // below only re-runs when `hasMore` flips (which it doesn't during a + // normal scroll). Calling `onLoadMore` directly would therefore pin the + // observer to the very first render's closure forever, re-requesting + // that stale cursor page after page so `nextCursor` never reaches its + // real `null` and the list loops (issue #38). Reading through a ref + // reassigned each render fixes that while keeping the "subscribe once" + // intent. + const onLoadMoreRef = useRef(onLoadMore); + onLoadMoreRef.current = onLoadMore; - // biome-ignore lint/correctness/useExhaustiveDependencies: `onLoadMore` is deliberately excluded — RecipeSourcesPanel passes a fresh function identity on every render, and re-subscribing the observer on every single render (rather than only when hasMore actually flips) would be wasteful busywork for no behavioral difference. useEffect(() => { const root = scrollContainerRef.current; const sentinel = sentinelRef.current; - if (!hasMore || !onLoadMore || !root || !sentinel) return; + if (!hasMore || !root || !sentinel) return; // `rootMargin` starts loading the next page slightly before the // sentinel actually reaches the visible edge — combined with @@ -59,7 +71,7 @@ export function SourceItemTable({ // need it, not just "as soon as" they do. const observer = new IntersectionObserver( (entries) => { - if (entries.some((entry) => entry.isIntersecting)) onLoadMore(); + if (entries.some((entry) => entry.isIntersecting)) onLoadMoreRef.current?.(); }, { root, rootMargin: "200px 0px" }, ); diff --git a/specs/backend-architecture.md b/specs/backend-architecture.md index a4e1e3f..948a883 100644 --- a/specs/backend-architecture.md +++ b/specs/backend-architecture.md @@ -559,11 +559,16 @@ pas encore de ligne `Source` — jamais une erreur). - **`the-meal-db.ts`** — `key: "theMealDb"`, `official: true`, `locale: "en"`. API publique gratuite (`https://www.themealdb.com/api/json/v1/${API_KEY}`, `THE_MEAL_DB_API_KEY` env var, défaut `"1"` = clé de test partagée - documentée par TheMealDB). `list()` n'a qu'une recherche (`/search.php?s=`), - pas de vrai "tout parcourir" côté gratuit — une requête vide renvoie un - petit échantillon fixe (~25 recettes), non paginé (`nextCursor` toujours - `null`). `parse()` reconstruit les ingrédients depuis les paires plates - `strIngredient1..20`/`strMeasure1..20`. + documentée par TheMealDB). `list()` a deux modes : une `query` non vide est + une recherche par nom (`/search.php?s=`, réponse unique non paginée, + `nextCursor: null`) ; une `query` vide/absente ("tout parcourir") **déroule + la liste des catégories** — `/list.php?c=list` une fois, puis + `/filter.php?c={catégorie}` une catégorie à la fois, le `cursor` étant + l'index de catégorie (une catégorie = une "page"). Le palier gratuit n'a + pas de vrai endpoint catalogue global ; `/search.php?s=` (vide) ne renvoie + qu'un échantillon fixe (~25 recettes), donc le scroll infini s'arrêtait + aussitôt (issue #38). `parse()` reconstruit les ingrédients depuis les + paires plates `strIngredient1..20`/`strMeasure1..20`. - **`json-ld-recipe.ts`** — `key: "jsonLdRecipe"`, `official: false`, scraper générique schema.org/`Recipe` (extraction regex des blocs `