fix(hub): stop re-filtering server search results in the merged list - #674
Open
RonenMars wants to merge 1 commit into
Open
fix(hub): stop re-filtering server search results in the merged list#674RonenMars wants to merge 1 commit into
RonenMars wants to merge 1 commit into
Conversation
/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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Completes the second half of #646.
#657 made the merged hub list take its conversations from
/api/searchwhile 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/searchmatches message bodies.MergedClassicListthen re-filtered what it returned ontitleandpreviewonly, 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
MergedClassicListnow takes aconversationsFromServerboolean alongside thesearchQueryit already received.The value is
Boolean(debouncedConvSearch)— the same conditionmergedClassicItemsuses 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/searchdoes not cover sessions.The predicate moved out of the
useMemointo an exportedmergedItemMatchesQueryso the regression test exercises the real code rather than a copy of it —__tests__/unit/components/sessions/collapsedServerLockout.test.tsmirrors 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:titlenorpreviewis kepttitleandpreviewFails-without-the-fix, verified by deleting the
if (conversationsFromServer) return trueline and re-running rather than by assumption:Restored, all three pass.
Verification
npx jest --ci --watchman=false --runInBandovermerged-search-filter,conversation-search-anchor,searchHighlight,conversationHref,collapsedServerLockout— 5 suites, 80 tests, all pass.npx tsc --noEmit --pretty false— 14 errors, allTS2345, 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.tswas 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.tsxinitially 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, whichdocs/troubleshooting.mdalready documents as an open handle and already prescribes--forceExitfor.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/createWrapperbuilds aQueryClientthat is never cleared, so each cached query schedules React Query'ssetTimeout(gcTime)when it goes unused and node will not exit while it is pending.Running only the
partial failuretests takes 1.4s and the process then exits on its own at 305s — the 5-minute defaultgcTime, to the second.Running only the
retention gcTimetests takes 0.9s and is still alive at 400s, becauseuseConversationsets a 7-daygcTime(hooks/useConversations.ts:423), and that is the timer the whole file ends up waiting on.Clearing the client in a global
afterEachwould 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.jsonwas deliberately left alone.It is shared with
e2e/06_search_anchor.yaml, and that flow passes with this bug present: the anchor'spreviewis "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.
node e2e/mock-server.js), hub in merged mode — the default,stores/settings.ts:133.Refs #646.