Skip to content

feat(search): weighted FTS ranking + searchable message bodies - #294

Open
salmonumbrella wants to merge 1 commit into
maathimself:mainfrom
salmonumbrella:search/split-1-fts-bodies-rebased
Open

feat(search): weighted FTS ranking + searchable message bodies#294
salmonumbrella wants to merge 1 commit into
maathimself:mainfrom
salmonumbrella:search/split-1-fts-bodies-rebased

Conversation

@salmonumbrella

@salmonumbrella salmonumbrella commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Split 1/3 of #283, per the review request there. Keyword-search improvements only — no infrastructure changes, no new dependencies, runs on stock postgres:16-alpine.

Summary

Search results are ranked by relevance instead of date-only, and message bodies become searchable across the whole mailbox.

  • Weighted lexical ranking — subject > sender > body (setweight A–D + ts_rank_cd), prefix matching as you type, quoted phrases, served by a GIN index over a trigger-maintained search_fts column. Filter-only queries stay date-ordered.
  • Resumable backfill — fast-DDL migrations 0041–0043 are crash-idempotent and boot-safe; search_fts fills via a background drainer, and not-yet-backfilled rows fall back to the previous query path. The legacy search_vector column stays until a follow-up removes it after real-install validation.
  • Body materialization — a provider-gated background IMAP drainer fills body_text (Apple/Yahoo/generic on; Gmail/PurelyMail/Microsoft off given their throttle history), with pacing, poison-message forward progress, and per-account circuit breakers.
  • The REST response carries a constant mode: 'lexical' field — the stable contract the semantic follow-up PR extends with real modes.

Review fixes

  • Rebased on current main; migrations are now 0041–0043.
  • Connection refusals stop the body drainer immediately and arm its circuit breaker, including after partial batch progress.
  • Searches without a selected account again respect include_in_unified_inbox.
  • The initial FTS backfill count runs in a dedicated transaction with SET LOCAL statement_timeout = 0, so large mailboxes do not restart the same count every 30 seconds.

Testing

  • Local Node 22: backend 1,080 passed; frontend 1,553 passed; both linters clean; production build passed.
  • Disposable exe.dev CrabBox, clean Ubuntu host with Node 22: same full test, lint, and production-build matrix passed.

Contributor License Agreement

By submitting this pull request I confirm that:

  • I have read and agree to the Contributor License Agreement.
  • My contribution is my own original work (or I have identified any third-party material and confirmed it is compatible with the CLA).
  • I have the right to submit this contribution under the terms of the CLA.

@maathimself

Copy link
Copy Markdown
Owner

Thanks, this is great. The security side is clean too, I checked the FTS and SQL paths hard and there's no injection, and the auth and per-user scoping are right.

A few things before I can merge:

Blocking

  1. Rebase and renumber the migrations. 0035/0036/0037 collide with main (0035_oidc_rp_initiated_logout, 0036_unified_inbox_accounts, 0037_message_delivery_addresses). Bump yours to 0040+.

  2. The body drainer has no connection-refusal handling. The snippet indexer stops and backs off hard the moment a provider refuses a connection (isConnectionRefusal in imapManager.js) specifically so it doesn't pile on. bodyBackfill.js has nothing equivalent, so at an iCloud connection limit one batch fires ~100 fresh logins and the run retries to ~300 before giving up, which escalates the block and can starve the live IDLE/sync connection. This is a real problem I've hit with iCloud. Please add an isConnectionRefusal fast-stop, and make the breaker refusal-aware, right now it only backs off when a run makes zero progress. The description says "circuit breakers comparable to the snippet indexer" but that isn't the case yet.

  3. Unified inbox scoping got dropped. searchService searches all enabled accounts and no longer honors include_in_unified_inbox, so an account a user deliberately excluded from the unified inbox now shows up in unified search. Please restore the include_in_unified_inbox filter for the unified (no specific account) case.

Should fix

  1. On a large mailbox the ftsBackfill initial count(*) runs on the normal pool with the 30s statement_timeout, so it can throw and retry every 5 minutes forever, which means body search never starts on exactly the big mailboxes that need it. Run that count off the timeout-free path, or bound/drop it.

Everything else I found is minor and can come later. Once 1 through 3 are in I'll re-review.

Results ranked by relevance instead of date-only: subject > sender > body
(setweight A-D + ts_rank_cd), prefix matching as you type, quoted phrases,
served by a GIN index over a trigger-maintained search_fts column. Backfill
runs as a resumable background drainer (fast-DDL migrations 0035-0037, no
boot-blocking rewrite); not-yet-backfilled rows fall back to the previous
query path. Body text is materialized by a provider-gated background IMAP
drainer with circuit breakers, poison-message forward progress, and
progress reporting. Filter-only queries stay date-ordered.

No infrastructure changes - runs on stock postgres:16-alpine.

Split 1/3 of maathimself#283.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@salmonumbrella
salmonumbrella force-pushed the search/split-1-fts-bodies-rebased branch from 42fc2d4 to ef15a6d Compare August 8, 2026 06:46
@salmonumbrella

Copy link
Copy Markdown
Contributor Author

fixed the four review points in ef15a6d:

  • rebased on current main; migrations are now 0041–0043
  • connection refusals stop the body drainer immediately and arm the breaker, including after partial progress
  • unified search again respects include_in_unified_inbox
  • the initial FTS count uses a dedicated transaction with SET LOCAL statement_timeout = 0

verification:

  • local Node 22: backend 1,080 tests, frontend 1,553 tests, both linters, frontend production build
  • clean disposable exe.dev CrabBox: same full matrix passed

CI note: both GitHub jobs currently stop at the audit step because unchanged current-main lockfiles resolve nanoid 3.3.16 and the new advisory requires 3.3.17. This PR has no package or lockfile diff; the same main commit passed CI yesterday. I kept that dependency repair separate from this search PR.

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.

2 participants