Skip to content

fix(hub): stop re-filtering server search results in the merged list - #674

Open
RonenMars wants to merge 1 commit into
fix/hub-merged-search-server-sourcefrom
fix/merged-search-client-refilter
Open

fix(hub): stop re-filtering server search results in the merged list#674
RonenMars wants to merge 1 commit into
fix/hub-merged-search-server-sourcefrom
fix/merged-search-client-refilter

Conversation

@RonenMars

@RonenMars RonenMars commented Aug 12, 2026

Copy link
Copy Markdown
Owner

Completes the second half of #646.
#657 made the merged hub list take its conversations from /api/search while a query is active; this stops the client from throwing those results away again.

Stacked on #657 — merge that one first, GitHub will retarget this to main.

The defect

/api/search matches message bodies.
MergedClassicList then re-filtered what it returned on title and preview only, so a conversation the server correctly matched deep in its message history was fetched over the network and silently discarded on the device.
That is precisely the case server-side search exists to serve, and it failed invisibly — an empty result list, not an error.

Mechanism

MergedClassicList now takes a conversationsFromServer boolean alongside the searchQuery it already received.
The value is Boolean(debouncedConvSearch) — the same condition mergedClassicItems uses to choose between the server results and the paged set, so the flag cannot drift from the source it describes.
During the 300ms debounce window the flag is still false and the items are still the paged set, which stays consistent.

When the flag is set, conversation rows pass through unfiltered because the server already matched them.
When it is not, they filter on title and preview exactly as before.
Session rows filter client-side in both regimes, since /api/search does not cover sessions.

The predicate moved out of the useMemo into an exported mergedItemMatchesQuery so the regression test exercises the real code rather than a copy of it — __tests__/unit/components/sessions/collapsedServerLockout.test.ts mirrors its production expression by hand, and a mirror cannot fail when production changes.
No other behaviour moved.

Tests

__tests__/unit/merged-search-filter.test.ts, three cases:

  • a server-matched conversation whose query hits neither title nor preview is kept
  • the paged set still filters on title and preview
  • session rows still filter by the query while conversations are server-backed

Fails-without-the-fix, verified by deleting the if (conversationsFromServer) return true line and re-running rather than by assumption:

● mergedItemMatchesQuery › keeps a server-matched conversation whose query hits neither title nor preview
    Expected: true
    Received: false
Tests: 1 failed, 2 passed, 3 total

Restored, all three pass.

Verification

  • npx jest --ci --watchman=false --runInBand over merged-search-filter, conversation-search-anchor, searchHighlight, conversationHref, collapsedServerLockout — 5 suites, 80 tests, all pass.
  • npx tsc --noEmit --pretty false — 14 errors, all TS2345, matching the Expo Router typed-route baseline tracked in P1: npm run typecheck is red locally and green in CI #606. .expo/types/router.d.ts was generated first, since without it tsc reports a false clean 0.
  • npx eslint app/index.tsx __tests__/unit/merged-search-filter.test.ts — clean.

No suite was excluded in the end.
__tests__/unit/hooks/useConversations.test.tsx initially looked hung — it sits at 0% CPU after finishing — but it passes all 40 of its tests in under 3 seconds and then simply refuses to exit, which docs/troubleshooting.md already documents as an open handle and already prescribes --forceExit for.
Re-run with that flag it is green: 3 suites, 52 tests.

The specific handle, since the doc names the class but not this instance: test-utils/createWrapper builds a QueryClient that is never cleared, so each cached query schedules React Query's setTimeout(gcTime) when it goes unused and node will not exit while it is pending.
Running only the partial failure tests takes 1.4s and the process then exits on its own at 305s — the 5-minute default gcTime, to the second.
Running only the retention gcTime tests takes 0.9s and is still alive at 400s, because useConversation sets a 7-day gcTime (hooks/useConversations.ts:423), and that is the timer the whole file ends up waiting on.
Clearing the client in a global afterEach would remove the need for --forceExit, but that is shared test infrastructure and does not belong in this PR.

Shared fixture: unchanged

e2e/fixtures/search-results.json was deliberately left alone.
It is shared with e2e/06_search_anchor.yaml, and that flow passes with this bug present: the anchor's preview is "Where did we set the wombat timeout for the retry loop?", which contains the query, so the old client-side predicate kept it either way.
A green run of that flow is not evidence for this fix, and extending the fixture would have changed what the flow exercises.

On-device check for whoever runs it next

No simulator was used here, per the task constraints.

  1. Mock suite up (node e2e/mock-server.js), hub in merged mode — the default, stores/settings.ts:133.
  2. Open search and type a term that appears only in a conversation's message bodies, not in its title or preview.
  3. The conversation should now appear in the list. Before this change it was fetched and then dropped.
  4. Confirm session rows still narrow as you type, and that clearing the query restores the full list.

Refs #646.

/api/search matches message bodies, but MergedClassicList re-checked each conversation on title and preview only.
A conversation the server correctly matched deep in its message history was fetched over the network and then silently discarded on the device, which is exactly the case server-side search exists to serve.

Tell the list which regime it is in with a conversationsFromServer prop, derived from the same debouncedConvSearch condition mergedClassicItems uses to pick its source.
Server-backed conversations are already filtered, so the client returns them untouched; the paged set filters as before.
Session rows keep filtering client-side in both regimes because /api/search does not cover sessions.

The predicate moved to an exported mergedItemMatchesQuery so the regression test exercises the real code rather than a copy of it.
Reverting the conversationsFromServer branch makes the first test fail and the other two pass, confirmed by running it.

e2e/06_search_anchor.yaml cannot detect this defect, because its fixture preview contains the query, so the shared fixture is unchanged.
tsc holds at the 14-error TS2345 baseline tracked in #606; eslint clean.

Completes the work started in #657.
Refs #646.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant