Skip to content

feat(food-search): All Providers aggregated search (mobile, phase 2)#1654

Merged
CodeWithCJ merged 1 commit into
CodeWithCJ:mainfrom
jsandai:feature/unified-food-search-phase2
Jun 27, 2026
Merged

feat(food-search): All Providers aggregated search (mobile, phase 2)#1654
CodeWithCJ merged 1 commit into
CodeWithCJ:mainfrom
jsandai:feature/unified-food-search-phase2

Conversation

@jsandai

@jsandai jsandai commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Description

What problem does this PR solve?
Phase 2 of the unified search (#1496). Phase 1 streamed in a single online provider; this adds an "All Providers" mode so a single search returns results from every configured food provider at once, without the user picking a source first.

How did you implement the solution?
Selecting "All Providers" in the source switcher fans the search out across all active food providers in parallel (useAllProvidersSearch, built on React Query useQueries). Each provider is its own query, so results stream in independently and a slow or failing provider does not block the others (partial results). The view shows Top Matches (each provider's top results interleaved round-robin, capped, badged with the source) and a By Source section with one collapsible accordion per provider. Each accordion has a result-count badge, a per-provider loading spinner and skeleton, an errored state ("Couldn't load" + tap to retry), an empty state ("No results"), and a "show all N" link that opens that single provider's full paginated view. Providers are colour-coded (a tinted badge in Top Matches and a dot in By Source) via a small providerColor helper (curated map + deterministic fallback). The fan-out is entirely client-side, so there is no new backend.

Linked Issue: Closes #1653

How to Test

  1. Have two or more active food providers configured (Open Food Facts and Swiss Food Database are seeded by default and need no credentials).
  2. Open the food search, type a query (for example "salmon").
  3. Tap the source selector in the External Results header and choose "All Providers".
  4. Verify Top Matches shows interleaved results badged by source, and By Source lists each provider with a result count.
  5. Tap a provider to expand it; tap "show all N" to open that provider's full results.
  6. Confirm a provider that errors or returns nothing shows "Couldn't load" (with retry) or "No results" respectively, and the others still render.

PR Type

  • Issue (bug fix)
  • New Feature
  • Refactor
  • Documentation

Checklist

All PRs:

  • [MANDATORY - ALL] Integrity & License: I certify this is my own work, free of malicious code, and I agree to the License terms.

New features only:

  • [MANDATORY for new feature] Alignment: I have raised a GitHub issue and it was reviewed/approved by maintainers or it was approved on Discord.

Frontend changes (SparkyFitnessFrontend/):

  • [MANDATORY for Frontend changes] Quality: I have run pnpm run validate and it passes.
  • [MANDATORY for Frontend changes] Translations: I have only updated the English (en) translation file.

Backend changes (SparkyFitnessServer/):

  • [MANDATORY for Backend changes] Code Quality: I have run typecheck, lint, and tests. New files use TypeScript, new endpoints have Zod schemas, and new endpoints include tests.
  • [MANDATORY for Backend changes] Database Security: I have updated rls_policies.sql for any new user-specific tables.

UI changes (components, screens, pages):

  • [MANDATORY for UI changes] Screenshots: I have attached Before/After screenshots below.

Mobile changes (SparkyFitnessMobile/):

  • [MANDATORY for Mobile changes] Tested on device or emulator: I have verified the changes work on iOS or Android.

Screenshots

Screenshot_20260626_173750_SparkyFitness

Notes for Reviewers

Mobile only. Web parity is intentionally deferred to a follow-up so the mobile UX can settle first; no SparkyFitnessFrontend/ or SparkyFitnessServer/ changes here (the Frontend and Backend checklist items above are left unchecked because they do not apply). Tests: FoodSearchScreen.test.tsx updated and passing; typecheck and lint clean on the changed files.

A few deliberate choices worth your eye, all easy to change:

  • "Show all N" switches into that single provider's existing full view in-place rather than pushing a new screen (reuses the phase 1 single-provider search). Can be a pushed screen if preferred.
  • By Source accordions start collapsed (Top Matches gives instant results).
  • An errored provider shows a muted "Couldn't load" + retry rather than being hidden, so the user knows it was tried and can recover.
  • Header label follows content: single provider reads "External Results", All Providers reads "Top Matches" then "By Source", with the source switcher constant across both.

@github-actions github-actions Bot added enhancement New feature or request mobile labels Jun 26, 2026
@github-actions

Copy link
Copy Markdown

PR Validation Results

Change Detection

  • 📱 Mobile changes detected

✅ All checks passed. Thank you!

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an aggregated 'All Providers' search mode in the food search screen, allowing parallel queries across multiple food providers. It adds a new hook useAllProvidersSearch, query keys, and a providerColor utility to support this feature, along with UI updates for top matches and provider-specific accordions. Feedback on the changes highlights that the manual reference-stability logic using useRef inside useMemo in useAllProvidersSearch is a React anti-pattern and redundant, as React Query v5's useQueries already provides referential stability.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread SparkyFitnessMobile/src/hooks/useAllProvidersSearch.ts Outdated
@jsandai jsandai force-pushed the feature/unified-food-search-phase2 branch from eda214d to 74cd177 Compare June 26, 2026 23:03
@jsandai

jsandai commented Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

@gemini-code-assist

Good catch on the render-phase ref reads/writes – that part is a legitimate anti-pattern, so I've removed the useRef/useMemo stability block.

One correction on the premise, though: in React Query v5 useQueries only returns a referentially stable array when you pass a combine function. Without combine the result is rebuilt on every render. QueriesObserver.getOptimisticResult returns #trackResult(...), which is matches.map(...) (a fresh array), and replaceEqualDeep structural sharing is applied only inside #combineResult, i.e. the combine branch. So depending on queries directly would hand back a new array reference each render, and the two FoodSearchScreen memos derived from providerResults would recompute on every keystroke.

So rather than drop the optimization, I moved the projection into useQueries' own combine. combine is memoised with useCallback (it is compared by identity and only closes over providers), and React Query runs replaceEqualDeep on its output, which gives genuinely stable providerResults across renders the idiomatic way, with no refs touched during render. The loading semantics (isFetching && items.length === 0) and the index-to-provider pairing are unchanged, and the hook ends up ~15 lines shorter.

Typecheck, lint, and the FoodSearchScreen tests pass.

@gemini-code-assist

Copy link
Copy Markdown
Contributor

That's a solid improvement. Moving the projection into the combine function is exactly the right way to handle referential stability in React Query v5. It keeps the hook clean and avoids the pitfalls of manual useMemo dependencies on the raw useQueries result array. The loading logic also looks robust for preventing UI flickers during background updates. Nice work.

@CodeWithCJ

Copy link
Copy Markdown
Owner

@jsandai @apedley is this ready to be merged?

@jsandai

jsandai commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@jsandai @apedley is this ready to be merged?

I'd like Andrew's eyes on it first

@apedley apedley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theres an issue with the header on food search screen only for iOS that actually came from phase 1. We need headerShown: false in the options for createStackScreenOptions or else we get double headers.

New logic untested, ie. the interleave and empty/error/etc states.

Result rows does not use the provider_id to determine the key, they are only in the section. Instead it uses item.online.source. There's a possible collision there.

If someone selects All, deletes the text in the input, and taps scan it forwards __all__ as the provider to FoodScan. I'm not sure why it's forwarding it.. maybe in case we want to have a provider override in the future.

// before each By Source provider. Known providers get a curated brand-ish
// colour; anything else gets a deterministic hue so new providers still get a
// stable colour with no maintenance.
const PROVIDER_COLORS: Record<string, string> = {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to not hardcode hex colors. Use uniwind colors from global.css for consistency and theme awareness. There are category colors there that are used for settings icons that would work well here.

@apedley

apedley commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

UX issue maybe from phase 1: if I search for something I have a lot of but want to find another of, the entire screen can be taken up by "your foods" making it seem like search didn't work

@apedley

apedley commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Theres an issue with the header on food search screen only for iOS that actually came from phase 1. We need headerShown: false in the options for createStackScreenOptions or else we get double headers.

Sorry forgot the screenshot:

simulator_screenshot_A0532DE5-5E2F-4F6D-8021-5B12D544513C

@jsandai jsandai force-pushed the feature/unified-food-search-phase2 branch from 74cd177 to 257a465 Compare June 27, 2026 02:58
@jsandai

jsandai commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough pass. All addressed in the latest push:

iOS double header. Fixed. FoodSearch now passes headerShown: false in its stack options, so iOS no longer draws the native header on top of the screen's own. Android already defaulted to headerShown: false, which is why it only showed on iOS. Good catch that it traces back to phase 1.

Result row key collision. Fixed. The online row key now includes the provider id (online-${providerId ?? source}-${id}-${index}) instead of keying on item.online.source alone, so two providers that share a provider_type can no longer collide.

__all__ forwarded to FoodScan. Fixed. The scan handler now forwards undefined when the selected source is the All Providers sentinel, so the scanner falls back to its default provider rather than receiving __all__. (There was no future-override use; it was just leaking the sentinel.)

Hardcoded hex colours. Fixed. providerColor is now a useProviderColor hook that resolves the theme category palette from global.css via useCSSVariable (the same --color-cat-* vars the settings icons use, plus a few other distinct theme hues so more than five active providers still separate), so it's theme-aware instead of hardcoded hex. A provider is mapped onto the palette by a stable hash of its type, so each source keeps a consistent colour with no upkeep. I kept it a deterministic assignment rather than guessing specific pairings; if you want particular providers pinned to particular category colours, say the word and I will map them explicitly. One thing worth your eye on device: the Top Matches badge tint is now a 13% opacity overlay of the themed colour (rather than the old hex+alpha trick), so the exact tint depends on the theme value.

Untested new logic. I pulled the Top Matches interleave out into a pure interleaveTopMatches helper and added unit tests covering the round-robin order, the cap, the per-provider rank limit, source tagging, and skipping empty/failed providers. I have not been able to run this on an iOS device, so a sanity pass there before merge would be ideal.

@jsandai

jsandai commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

UX issue maybe from phase 1: if I search for something I have a lot of but want to find another of, the entire screen can be taken up by "your foods" making it seem like search didn't work

Good catch, and you are right that this predates phase 2. The screen is one SectionList that stacks Your Foods, then Your Meals, then the online section (Top Matches / By Source in All mode), and the local sections are unbounded. So a query that matches a lot of your own foods fills the screen and pushes the online results below the fold, which reads as "search didn't find anything new." It affects single-provider mode too, not just All Providers.

A few ways we could go:

A (my preference): cap Your Foods / Your Meals to the first few matches with a "Show all N" expander, reusing the same affordance the By Source accordions and provider "show all" already use. That keeps the online section within a screen of the top and stays consistent with the existing UX language. I would lean toward making it conditional, so the cap only kicks in when there are online results to show and a pure local search is never artificially truncated.

B (could complement A): keep the section headers sticky and add a count to them (e.g. "Your Foods (23)") so it is always obvious there is more below, including the online header. On its own it just signals the online results exist rather than bringing them up, but paired with A it reinforces it.

C (considered, not keen): a fixed max-height inner scroll on the local sections. It fights the SectionList model and feels awkward on mobile, so I would rather not.

Reordering to put online first is off the table since local-first is intentional.

Two things I would want your call on before I build it:

  1. Scope: fold this into this PR, or do it as a small fast-follow? It is a phase-1-rooted layout issue that also touches single-provider mode, so it is arguably its own concern, but it is the same screen we are in here. Happy either way.
  2. Design specifics: the cap count, the expander wording, and whether you agree with making the cap conditional on online results existing.

Once you pick the shape I will wire it up.

@apedley

apedley commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Did you run into rate limiting at all with open food facts? The two rate limiters use different query keys so toggling between all and OFF could maybe cause a rate limit but I haven't hit it. Although OFF is so slow I usually don't wait around for it to load.

Regarding the hashmap of colors. It's maybe a little too creative. Any collisions undercut the whole point and even with 3 providers theres a much higher chance of that than you'd think. Its like the birthday paradox if you're familiar. Google says 34%. Assigning by index works better. colors[i % colors.length] over the providers list. This won't be stable for different ordering of providers, but this is for quick glance grouping so that isn't a requirement. This would also assign a color to rarely used providers not on the list like Norish Edamam Tandoor or Nutritionx.

Also regarding the colors, the contrast on the badges is too low for at least the color-exercise. Check it out here on ios it's the neon green.

image

If you need to add a category color feel free.

Now for capping results..
Option A with a condition sounds good. Go ahead and put it in this PR it's not too big. As for wording and cap limit, I'm going to leave it up to you. 5 or 6 sounds good I think but use your best judgment.

@CodeWithCJ

CodeWithCJ commented Jun 27, 2026

Copy link
Copy Markdown
Owner

Fatsecret has some limits. Though we will not hit that in practical for selfhosted env, hitting all the providers for every search will not be ideal.

May I suggest to have these options so we don't take out what is feasible today?

  • search only local
  • search online with all/selected providers
  • search both local & online.

Sorry if this was already been discussed. Probably a checkbox/radio button. I know this is how we had previously. but instead of multiple tabs, in single search itself having the checkbox/radio will it make hybrid of both?

OpenFoodFacts cloud already running into issues, we will also be contributing more.

@CodeWithCJ

CodeWithCJ commented Jun 27, 2026

Copy link
Copy Markdown
Owner

Purpose of this PR itself is to unify the search. Not sure my previous comment is not valid one. I am open to your suggestions.

@apedley

apedley commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Fatsecret has some limits. Though we will not hit that in practical for selfhosted env, hitting all the providers for every search will not be ideal.

Hitting all providers is the only way to get that info. To each provider it looks exactly the same as if it were searched individually.

May I suggest to have these options so we don't take out what is feasible today?

  • search only local
  • search online with all/selected providers
  • search both local & online.

These options are there just automated to be more like retail apps where you don’t need to remember where to search. Even so, “All Providers” currently requires switching to it explicitly - it’s not available as a default.

OpenFoodFacts cloud already running into issues, we will also be contributing more.

OFF will actually be in better shape during this. It has its own rate limit that’s tighter than the default.

@jsandai

jsandai commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

@apedley

All good calls, working through them now.

OFF rate limiting. You're right, and it's worse than "maybe": the single-provider hook and the All Providers hook each construct their own RateLimiter(8, 60_000), so they're two independent budgets. OFF could see up to 16/min if someone toggles between a single OFF search and All. I'm consolidating them into one shared module-level limiter so both modes draw from a single 8/min budget. The query keys stay separate on purpose (the All Providers key was split out to dodge an infinite-query cache-shape clash from earlier), so the two modes won't share React Query cache, but they'll now share the rate budget, which is the part that matters for not hammering OFF. Agree that OFF is slow overall, and I don't usually wait on it either.

Colours, hash to index. Agreed, switching to colors[i % colors.length] keyed off the provider's position in the active list. You're right about the birthday paradox, a collision defeats the whole point and the odds are higher than they feel. Stability across reordering isn't a requirement for glance grouping, and as you noted this also gives a colour to the rarely-used providers that aren't individually mapped.

Colours, contrast. Good catch on the neon green, that was --color-exercise, which I'd borrowed from the macro/activity vars that aren't tuned for badge contrast. Dropping those and moving to the --color-cat-* set, which is built for exactly this. To give index assignment enough headroom that wrap-around collisions don't show up on a realistic setup, I'm adding three new contrast-matched category colours to global.css (a blue, a teal, and an amber), taking it to eight total alongside the existing slate/pink/violet/orange/green. Eight covers any realistic provider count collision-free while staying distinct enough to tell apart at a glance; past that the hues start blurring and undercut the purpose as much as a collision would. They'll match the existing cat vars' saturation so they read on the 13% badge tint, in both light and dark.

Capping local results. Thanks, will do, in this PR. Option A with the condition: capping Your Foods and Your Meals to six rows with a "Show all N" expander, and only when there are online results to show, so a pure local search is never truncated.

I'll push all of this together and flag anything that needs an iOS eye, since I can't run it on a device here.


@CodeWithCJ

Thanks, this is a fair thing to be careful about, and it's worth being explicit about how the load actually behaves here.

A few things that should ease the concern:

All Providers is opt-in, not the default. A normal search still hits a single provider; the user has to deliberately switch to "All Providers" to fan out. So the multi-provider path only runs when someone asks for it, not on every search.

To each provider, an All Providers search looks identical to a normal individual search, one request, same shape. There's no amplification per provider; it's the same call we'd make if the user had picked that provider manually. So we're not generating a heavier or unusual load pattern, just running the existing per-provider searches in parallel.

For OFF specifically, requests are rate-limited, and per Andrew's review I'm consolidating that into a single shared limiter (tighter than OFF's own default) that covers both single and All modes, so even heavy toggling can't push us past a safe rate.

And there's already an escape hatch for a setup that wants to avoid an external provider's load entirely: providers carry an active flag, and search only ever queries active ones, so deactivating a provider in the configuration removes it from search completely. A rate- or bandwidth-sensitive install can run a purely local search today without any new control.

On the explicit local / online / both selector: that's essentially what the unified search already does automatically. Local and the chosen online provider both come back in one search (the retail-app pattern that #1496 is aiming for), so the user doesn't have to remember where to look. Reintroducing an explicit mode toggle would step back toward the multi-tab decision the redesign is trying to remove. That said, the building blocks are all here, so if you'd want a "search scope" control in the future (say, an advanced toggle to restrict to local-only for low-bandwidth or rate-sensitive setups), that's a small follow-up rather than a rework. Happy to go that way if you'd prefer it.

@jsandai jsandai force-pushed the feature/unified-food-search-phase2 branch from 257a465 to 189b3c2 Compare June 27, 2026 15:49
@jsandai

jsandai commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Pushed all of this.

  • Colours are now assigned by list position (palette[i % length]) instead of a hash, so no birthday-paradox collisions. Added a small buildProviderColorMap helper with unit tests covering the index assignment, distinct colours up to the palette size, and the wrap past it.
  • Dropped the borrowed non-category vars (including the low-contrast neon green, which was --color-exercise) and moved to the --color-cat-* set, adding three new contrast-tuned category colours (blue, teal, amber) in light/dark/amoled so the palette is eight wide. The badge tint stays a 13% overlay of the themed colour.
  • OFF now uses a single shared rate limiter across the single-provider and All Providers hooks, so the two modes draw from one budget.
  • Local results: Your Foods and Your Meals are capped to six rows with a "Show all N" expander, and only when online results are also showing, so a pure local search is never truncated. The cap resets on a new query.

Two things I could not check myself since I have no iOS device: the new category colours' contrast on the badge (especially the blue/teal/amber on dark), and the local "Show all" behaviour. A quick look when you get a chance would be appreciated.

@apedley

apedley commented Jun 27, 2026

Copy link
Copy Markdown
Contributor
  • externalFoodRateLimiter.ts is 8 lines 5 of them comments. The rest exports the function it imported with 2 constants. Claude can be silly like that sometimes but and this honestly made me laugh. We do need a singleton but move it either to externalFoodSearchApi.ts or rateLimiter.ts. Either makes sense to me.
  • For the number of matches, lets have the minimum be the number of providers so people see at least one from every provider they have configured

After those 2 quick things we're ready for part 3

@jsandai

jsandai commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Ha, fair, that file was overkill for one export. Moved the singleton into externalFoodSearchApi.ts next to the OFF calls and deleted the standalone file; both hooks import it from there now.

Top Matches now never caps below the number of providers that returned results, so every configured provider with a hit shows at least one match. The round robin already emits each provider's first item before any second item, so that guarantee just falls out of raising the floor. Added a couple of tests for it (eight providers all appear; empty or errored providers don't count toward the minimum).

@jsandai jsandai force-pushed the feature/unified-food-search-phase2 branch from 189b3c2 to 9331edb Compare June 27, 2026 18:55
@apedley

apedley commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Looks good. Some tests are failing though

Adds an 'All Providers' mode to the unified food search: a single search
fans out across every active food provider in parallel (useAllProvidersSearch
via React Query useQueries), each provider streaming in independently so a
slow or failing provider does not block the others.

- Top Matches: interleaves each provider's top results (round-robin by
  rank, capped), each row badged with its source in a per-provider colour.
- By Source: one collapsible accordion per provider with a colour dot, a
  result-count badge, a per-provider loading spinner and skeleton rows, and
  a 'show all N' link that switches into that single provider's full view.
  Errored providers show a muted 'Couldn't load' + tap-to-retry; providers
  that return nothing show 'No results'.
- Per-provider signature colours (providerColor: curated map + deterministic
  fallback) differentiate sources at a glance.
- The Top Matches header doubles as the All Providers / single-provider
  switcher, consistent with the phase 1 source selector.

Single-provider search is disabled while All Providers is active. Phase 1
behaviour is otherwise unchanged.
@jsandai jsandai force-pushed the feature/unified-food-search-phase2 branch from 9331edb to 24db2a8 Compare June 27, 2026 21:53
@jsandai

jsandai commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Fixed, green now. The limiter move was the culprit: I'd put the singleton in externalFoodSearchApi.ts, which the useExternalFoodSearch test mocks, so offRateLimiter came back undefined and the OFF query threw, cascading into the neighbouring tests. Moved it to rateLimiter.ts instead since nothing mocks that, and ran the full suite this time. 164 suites / 2540 tests passing.

@CodeWithCJ

Copy link
Copy Markdown
Owner

Hope this is ready to merge.

@CodeWithCJ CodeWithCJ merged commit 97dc4e5 into CodeWithCJ:main Jun 27, 2026
9 checks passed
@jsandai jsandai deleted the feature/unified-food-search-phase2 branch June 28, 2026 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request mobile

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: All Providers aggregated food search (unified search phase 2)

3 participants