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..fc03862 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 1 Item 1" + When I scroll the source list to the bottom 5 times + Then the third page's items should have loaded + And no browse cursor should have been requested more than twice + 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..54af571 100644 --- a/apps/web/cypress/e2e/recipe-sources.ts +++ b/apps/web/cypress/e2e/recipe-sources.ts @@ -132,6 +132,82 @@ Given("browsing TheMealDB returns two pages of items", () => { }); }); +/** + * Three full pages, each addressed by a distinct `cursor` value the mock + * actually parses (not just "does the URL contain `cursor=`"). Pages are + * deliberately large enough to overflow the list's own scroll container so + * the sentinel starts below the fold — each `scrollTo("bottom")` then + * produces a real IntersectionObserver transition, the only thing that + * makes the observer fire again. 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 THREE_PAGE_SIZE = 15; +const threePageCursorsRequested: string[] = []; + +function threePagePage(pageNo: number, nextCursor: string | null) { + return { + items: Array.from({ length: THREE_PAGE_SIZE }, (_, index) => ({ + externalId: `p${pageNo}-${index}`, + title: `Page ${pageNo} Item ${index + 1}`, + picture: null, + url: `https://www.themealdb.com/meal/p${pageNo}-${index}`, + alreadyImported: false, + recipeId: null, + })), + nextCursor, + }; +} + +Given("browsing TheMealDB returns three pages, each addressed by its own cursor", () => { + threePageCursorsRequested.length = 0; + const pages: Record> = { + "": threePagePage(1, "cursor-2"), + "cursor-2": threePagePage(2, "cursor-3"), + "cursor-3": threePagePage(3, 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 } }); + }); +}); + +When("I scroll the source list to the bottom {int} times", (times: number) => { + for (let attempt = 0; attempt < times; attempt += 1) { + cy.get(".source-items-column .recipe-table-wrap").scrollTo("bottom", { + ensureScrollable: false, + }); + // IntersectionObserver callbacks land on their own task, and the next + // page's fetch + merge then needs a render — give both a beat before + // the next scroll (which needs fresh content below it to have anything + // new to reveal). + cy.wait(350); + } +}); + +Then("the third page's items should have loaded", () => { + // Page 3's marker sits ~30 rows down inside the list's own overflow + // container — scroll it in before asserting, like `common.steps.ts`'s + // ingredient-row check does. With the stale-closure loop it never loads + // at all: `handleLoadMore` keeps re-requesting cursor-2. + cy.contains(".recipe-table__name", "Page 3 Item 1").scrollIntoView().should("be.visible"); +}); + +Then("no browse cursor should have been requested more than twice", () => { + const counts = new Map(); + for (const cursor of threePageCursorsRequested) { + counts.set(cursor, (counts.get(cursor) ?? 0) + 1); + } + // React StrictMode double-invokes effects in the e2e dev server (see + // recipes.cy.ts), so the panel's initial fetch + one-page-ahead prefetch + // can each fire twice — hence "twice", not "once". The issue #38 loop + // blows well past that: every scroll re-requests the same stale cursor. + const offenders = [...counts.entries()].filter(([, n]) => n > 2); + expect(offenders, `cursor request counts: ${JSON.stringify([...counts])}`).to.have.length(0); +}); + // 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 `