fix: remove broken stale request check and handle network errors in autocomplete

[Sherley-Sonali]

⚠️ Acceptance Requirements

• ✅ Linked to Auto-completion is often very slow #6160
• ✅ Assigned to @Sherley-Sonali

What

• Removed the broken time-based stale request check in _getSuggestions.
  start.difference(DateTime.now()).inSeconds always returns a negative value,
  so the condition > 5 never triggered. This was dead code.
• Stale request ordering is already handled correctly by AutocompleteManager
  in openfoodfacts-dart, this confirms the "to be confirmed" from the issue discussion.
• Fixed silent catch block: on network failure, the loading spinner now always
  hides and empty results are returned cleanly instead of leaving the user
  with a stuck spinner.

Screenshot or video

No visual change — this is a logic fix. Spinner now correctly disappears
on network failure instead of staying indefinitely.

Fixes bug(s)

• Fixes: Auto-completion is often very slow #6160

Part of

• Server-side issues (tracker) #1462

[monsieurtanuki]

Hi @Sherley-Sonali!
The first step would be to format your code, cf. dart format .

[monsieurtanuki]

Hi @Sherley-Sonali!

Please have a look at my comments.

I guess the code would indeed need some refactoring. But I'm not sure you're actually fixing the initial issue.

The assumption that caching the lists in static variables isn't good enough, offline (or worse: with low connectivity), and with recurring queries.
I cannot even find that caching in the code, beyond mere local variables - would you help me?

A solution would be to store (and reuse) the lists locally, e.g. in SQFlite in a new table.

Then, when the user types in something:

• let's have a look in the local database
• if we find something in the database, we return it, while refreshing the database with the values returned by the call to the server - will be reused for the next time
• if there's nothing in the database, we also call the server, store the list and return it

What do you think of that?

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 9.10%. Comparing base (4d9c7fc) to head (018aa19).
⚠️ Report is 1360 commits behind head on develop.

Files with missing lines	Patch %	Lines
...ib/pages/input/smooth_autocomplete_text_field.dart	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           develop   #7441      +/-   ##
==========================================
- Coverage     9.54%   9.10%   -0.44%     
==========================================
  Files          325     625     +300     
  Lines        16411   36636   +20225     
==========================================
+ Hits          1567    3337    +1770     
- Misses       14844   33299   +18455     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Sherley-Sonali]

You're right. _suggestions is just an in-memory map that doesn't survive across sessions.

The persistent SQFlite approach would work. I'd create a new DaoAutocomplete following the existing DAO pattern in lib/database/, with columns (query, language, results_json, last_updated). On each keystroke: check the table first and return immediately if found, then fire the server request in the background to refresh it for next time. If nothing in the table, fall to the server as today.
One thing I noticed: when offline with no cached results for a new query, DaoProduct already has product names locally from scans, searches, and BackgroundTaskOffline: that could serve as a natural third tier. I'd design DaoAutocomplete with that extensibility in mind without over-scoping this PR.

On your suggestions: yes to both, acceptin now.

[monsieurtanuki]

@Sherley-Sonali Given that creating SQFlite tables is prone to minor errors with big consequences, please share your table structure here and explain typical use cases before PRing anything.

[teolemon]

Hi @Sherley-Sonali

Just a quick nudge on the reviewer's comments above. We’d love to get this merged, but we need those updates to move to the next stage.

If we don't hear back within 10 days, I'll close this for now. You can always reopen it later when you have more time to tackle the feedback!

[Copilot]

Pull request overview

This PR fixes autocomplete request handling in Smooth App by removing a dead “stale request” check and ensuring the loading spinner is cleared when suggestion fetches fail (e.g., network errors), aligning behavior with AutocompleteManager ordering/caching.

Changes:

• Remove the broken time-based stale-request check in _getSuggestions.
• Ensure network failures return empty results and always stop the loading spinner.
• Add unit tests covering AutocompleteManager caching/error behavior (and update pubspec.lock).

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File	Description
packages/smooth_app/lib/pages/input/smooth_autocomplete_text_field.dart	Removes dead stale-request logic; updates error handling/loading behavior for autocomplete suggestions.
packages/smooth_app/test/pages/autocomplete_test.dart	Adds tests for AutocompleteManager caching, concurrency, and error scenarios.
packages/smooth_app/pubspec.lock	Updates transitive dependency resolutions.

[Copilot]

_setLoading(false) is now executed unconditionally in finally. If multiple _getSuggestions calls overlap (common while typing), an older request finishing first will clear the spinner even though a newer request is still in-flight. Consider tying the loading state to the latest query (e.g., only clear when search == _searchInput, or use a monotonically increasing requestId/counter and only clear for the latest request).

Suggested change

	_setLoading(false);
	}
	if (_suggestions[search]?.isEmpty ?? true && search == _searchInput) {
	_setLoading(false);
	}
	if (search == _searchInput) {
	_setLoading(false);
	}
	}

[Copilot]

This async error assertion is likely incorrect: manager.getSuggestions returns a Future, so wrapping it in () => ... won’t throw synchronously. Use an async-aware expectation (e.g., expect(manager.getSuggestions('bo'), throwsA(...)), or await expectLater(...)) so the test actually verifies the failure mode.

Suggested change

	expect(() => manager.getSuggestions('bo'), throwsException);
	await expectLater(manager.getSuggestions('bo'), throwsException);

[Copilot]

The test name/intent (“returns most recent cached result when out of order”) isn’t being asserted: both requests use the same delay and the expectations only check isNotEmpty. This can pass even if out-of-order/stale-result handling is broken. Either rename the test to what it actually verifies, or make it deterministic by forcing out-of-order completion (different delays) and asserting the manager returns/keeps the latest-query result.

[Sherley-Sonali]

Thanks for the nudge.

Here's the proposed table structure for DaoAutocomplete:

Table: autocomplete_cache

Column	Type	Notes
query	TEXT	Normalised, trimmed, lowercase query string
language	TEXT	BCP-47 language code e.g. en, fr
results	TEXT	JSON-encoded List<String> of suggestion strings
updated_ms	INTEGER	Unix timestamp in ms - LocalDatabase.nowInMillis()

Primary key: (query, language) ON CONFLICT REPLACE - matching the pattern in DaoProduct. Same query in different languages returns different suggestions, so collapsing on query alone would corrupt results for multilingual users. ON CONFLICT REPLACE means background refreshes write cleanly without separate UPDATE logic.

Typical use cases:

1. Cache hit, fresh: User types "choco" → row found, updated_ms within TTL → return results immediately, fire background server request to refresh the row. User never waits.

2. Cache hit, stale: Same - still return cached results immediately, refresh in background. Stale suggestions are better than a blank dropdown.

3. Cache miss, online: "mango" not in table → call AutocompleteManager.getSuggestions as today → insert row → return results. Next session this query is a cache hit.

4. Cache miss, offline: "mango" not in table, no connectivity → fall back to a prefix query on DaoProduct, which already holds product names from scans, previous searches, and BackgroundTaskOffline. No new data needed - only a new query path.

One thing worth noting while tracing the package: AutocompleteManager in openfoodfacts-dart already handles stale request ordering and in-session caching - DaoAutocomplete adds persistence across sessions
without touching that logic. The interception happens entirely in _getSuggestions in smooth-app, before the dart package is called.

Migration: new table added as a version 10 increment in local_database.dart, following the versioned onUpgrade pattern in DaoProduct. I can share the full migration script here before writing any feature code.

Does this structure look right before I proceed?

[monsieurtanuki]

Does this structure look right before I proceed?

Quickly said (because it's late tonight): no.
Your autocomplete is supposed to work for different "types" (e.g. categories, brands), which is not reflected in your table columns.
And you're supposed to handle different results depending on the env (TEST or PROD).
Maybe an additional reference table with an autoincrement id, with somehow the root url of the query as a unique varchar key (which would pack language, env and type). And reuse that id in "your" table, instead of "language".
What do you think about it?

[Sherley-Sonali]

That's a good catch - I missed the type dimension entirely when I proposed (query, language).

Tracing all the instantiation sites, AutocompleteManager wraps four distinct Autocompleter implementations, each hitting a different endpoint:

• TaxonomyNameAutocompleter - search API, fixed English, for brands
• TagTypeAutocompleter - main OFF API, per tagType + language + country (5 tag types used in smooth-app: ORIGINS, EMB_CODES, LABELS, CATEGORIES, COUNTRIES)
• FolksonomyKeysAutocompleter - folksonomy API, no dynamic params
• FolksonomyValuesAutocompleter - folksonomy API, but also takes a dynamic keyProvider() - the current folksonomy key being edited

The two-table structure makes sense. One thought on root_url: for FolksonomyValuesAutocompleter the effective cache context includes the runtime folksonomy key (e.g. "packaging", "origin") - not just the base URL. Two calls with the same query but different folksonomy keys return completely different results and must not share a cache entry.

So I'd suggest calling the column namespace instead of root_url, constructed by each Autocompleter from all parameters that affect its output - base URL + type + language + env, and for folksonomy values additionally the current key. This keeps the schema stable: the two-table structure stays unchanged, but namespace construction
is owned by each Autocompleter rather than derived from the URL alone.
For the table structure specifically:

autocomplete_namespace

Column	Type	Notes
id	INTEGER	Primary key, autoincrement
namespace	TEXT	Unique - base URL + type + language + env + dynamic params

autocomplete_cache

Column	Type	Notes
namespace_id	INTEGER	FK to autocomplete_namespace.id
query	TEXT	Normalised, trimmed, lowercase
results	TEXT	JSON-encoded List<String>
updated_ms	INTEGER	LocalDatabase.nowInMillis()

Primary key: (namespace_id, query) ON CONFLICT REPLACE Index: UNIQUE INDEX on autocomplete_namespace(namespace) for fast namespace resolution.
On table sizes: autocomplete_namespace stays at 7 rows for a regular user (5 tag types + 1 brand + 1 folksonomy keys), growing only if the user actively uses folksonomy values. autocomplete_cache reaches 200-400 rows in steady state under a 30-day TTL. The integer namespace_id foreign key means all cache lookups after namespace
resolution are integer + short string comparisons on a sub-500-row table - essentially instant.

What do you think?

[monsieurtanuki]

@Sherley-Sonali Looks good to me, especially namespace.

For the moment, code-wise, just focus on the tables: creation and population.

Don't play with updated_ms and TTL, just populate that field. I don't like the name btw, please stick to whatever name we already used in the other tables for timestamps.

Not interested in your "4. Cache miss, offline" either. That's not relevant here. Or I didn't understand.

[Sherley-Sonali]

Working on table creation/population.
Since namespace really belongs to each Autocompleter, my inclination is to add something like getCacheNamespace() in openfoodfacts-dart and use it here - but that expands scope.
Keep it local for this PR, or move that change upstream first?

[monsieurtanuki]

@Sherley-Sonali It wouldn't be appropriate to code it in the off-dart package: it would delay the process, without any added value and potentially adding confusion to off-dart.
If needed, you can use extension.

I know that if I had to code it, I would probably create an abstract class with two methods:

abstract class ServerSuggestion {
  String getNamespace(String soFar);
  Future<List<String>> getSuggestionsFromServer(String soFar);
}

And then a small class for the cache:

class SuggestionCache {
  SuggestionCache(this.serverSuggestion);
  final ServerSuggestion serverSuggestion;

  static Map<String, List<String>> _suggestions = {};

  Future<List<String>> getSuggestions(String soFar) async {
    final String namespace = serverSuggestion.getNamespace(soFar);
    List<String>? result = _suggestions[namespace];
    if (result != null) {
      return result;
    }
    result = await dao.getSuggestions(namespace);
    if (result != null) {
      _suggestions[namespace] = result;
      return result;
    }
    // TODO add try/catch and something elegant for exceptions
    result = await serverSuggestion.getSuggestionsFromServer(soFar);
    if (result != null) {
      await dao.putSuggestions(namespace, result);
      _suggestions[namespace] = result;
      return result;
    }
  }
}

With that, I have 2 rather dumb classes, that do not care about how to get the data from the server.

And then, just implements some ServerSuggestion.

I don't know if it was what you were heading for.

[Sherley-Sonali]

@monsieurtanuki PR updated with table creation and population.

Database layer: Two new tables. autocomplete_namespace normalises the cache key . autocomplete_cache stores JSON encoded results. The namespace lookup has an in-memory cache so repeated lookups in the same session avoid database hits.

Namespace abstraction: ServerSuggestion implementations use ProductQuery globals to build their own namespace. The UI only passes what identifies the autocompleter (tagType, taxonomyNames, or keyProvider).

UI: SmoothAutocompleteTextField now takes an optional serverSuggestion. When provided, it uses serverSuggestion.getSuggestionsFromServer for fetching and stores results after server response using serverSuggestion.getNamespace.

Not in this PR: Reading from cache before server call, SuggestionCache orchestration, TTL eviction. Those will follow separately.

[monsieurtanuki]

Thank you @Sherley-Sonali for your work.
Please have a look at my comments.

[monsieurtanuki]

It would be more clear if you had one dao per table. Like in the rest of the project btw.

[monsieurtanuki]

I don't think it's the DAO's job to cache data. Just make DAO what they're meant for: simple access to database.

In the rest of the code, I imagine something like "oh we're looking for suggestions of category in English, let's get the namespace for it from DaoNamespace, and while we're on the page let's use that same namespace while we're on looking for suggestions."

[Sherley-Sonali]

DAOs are now plain DB access; caching logic moved to new SuggestionCache class

[monsieurtanuki]

Are there cases when it's not available?

[monsieurtanuki]

Please create a separate method for that, in that page or in a distinct class, like I suggested earlier: "get the suggestions, either from the static, or from the database, or from the server".

[Sherley-Sonali]

SuggestionCache now owns namespace ID cache and orchestrates memory → DB → server lookup

[Sherley-Sonali]

Addressed review comments - split DaoAutocomplete into DaoNamespace and DaoAutocompleteCache (one class per table), introduced SuggestionCache to handle namespace ID caching and three-tier lookup (memory → DB → server) in the UI layer, renamed getResults/storeResults to get/put, removed redundant index.

[monsieurtanuki]

Hi @Sherley-Sonali!
Looks better! Please have a look at my comments.

[Sherley-Sonali]

Addressed all comments: removed soFar from getNamespace, added static results cache in SuggestionCache, removed redundant conflict algorithm.

[monsieurtanuki]

Thank you @Sherley-Sonali for your good work.
Still some comments, please have a look at them.

[monsieurtanuki]

Suggested change

	static final Map<String, List<String>> _resultsCache =
	<String, List<String>>{};
	static final Map<int, Map<String, List<String>>> _resultsCache =
	<int, Map<String, List<String>>>{};

That would be cleaner, instead of using your final String cacheKey = '$namespaceId|$soFar';
Just putting whatever is related to the namespace in its specific Map<String, List<String>>.
Then we'd be able to say: just clean that namespace.

[monsieurtanuki]

Suggested change

	Future<List<String>> getSuggestions(final String soFar) async {
	final String namespace = serverSuggestion.getNamespace();
	final int namespaceId = await _getNamespaceId(namespace);
	int? _namespaceId;
	Future<List<String>> getSuggestions(final String soFar) async {
	final String namespace = serverSuggestion.getNamespace();
	_namespaceId ??= await _getNamespaceId(namespace);

Just a suggestion.

[monsieurtanuki]

Lousy naming with the ending underscore.
What about staticCache, dbCache and serverData?

[monsieurtanuki]

Suggested change

	final DaoAutocompleteCache daoCache = DaoAutocompleteCache(localDatabase);
	_daoCache ??= DaoAutocompleteCache(localDatabase);

cf. _namespaceId: we don't have to recreate an object all the time.
In this case, could even be done in the constructor.

[monsieurtanuki]

That's typically why I asked you to create 2 separate files. Which for some reason you ignored, and I'm a bit puzzled with that.
When I review, the whole file is considered as changed, and I have to read again ALL the file, when you may have simply edited one class.

[monsieurtanuki]

I don't agree at all: knowing that there's no result is important.
Imagine looking for gqehbjvlesqmbnqvhd again and again.
If we know there are no results, let's cache that there's no result.