Repository navigation
fix(mail): push filters into FTS search + migration engine + Spotlight body fallback - #158
Merged
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (49)
📝 WalkthroughWalkthroughAdds typed metadata schema, migrations, and predicate pushdown to the indexer; integrates schema evolution and backfill into store lifecycle; refactors macOS Mail search with SQL pushdown, Spotlight fallback, and new SQL helpers; introduces migration utilities, CLI quiet-mode detection, and logging/documentation updates. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (mail/search)
participant Store as Index Store
participant Source as IndexerSource
participant DB as SQLite DB
participant Search as Search Engine
CLI->>Store: search(query, {filters?, attach?, metadataFilters?})
Store->>DB: open DB (getDb) and run Migrator.run()
DB-->>Store: migrations applied/skipped
Store->>Source: metadataColumns()? (on index creation / after migrations)
Source-->>Store: declared MetadataColumnSpec[]
Store->>DB: applySourceMetadataSchema(table, declared)
DB-->>Store: schema updated (ALTER TABLE / CREATE INDEX)
alt new typed columns added
Store->>Source: populateMetadata(opts) (async-generator)
Source-->>Store: metadata batches
Store->>DB: backfillMetadataColumns(batch)
end
Store->>Search: execute FTS search with merged filters (sql+params, attach)
Search->>DB: fts.search(query, filters + attach subquery)
DB-->>Search: rows (typed columns + metadata_json)
Search->>Store: normalize metadata (merge typed columns + metadata_json)
Store-->>CLI: results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Review rate limit: 0/5 reviews remaining, refill in 7 minutes and 21 seconds. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive database infrastructure for the indexer, featuring a generic migration framework, support for typed metadata columns with automatic schema evolution, and data backfilling. It also refactors the macOS mail search to support index-based filtering with a Spotlight fallback. Feedback highlights critical performance and reliability improvements: specifically, avoiding type casting on indexed columns to preserve query efficiency, using pathToFileURL for robust database attachment URIs, and fixing pagination logic in metadata backfilling to prevent skipping rows with default empty string identifiers.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/search/drivers/sqlite-fts5/index.ts (1)
503-533:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHybrid Qdrant path currently drops SQL filters.
In the Qdrant-capable branch, Line 524 fetches docs only by ID, so
opts.filtersis ignored. Filtered hybrid queries can return out-of-scope results.Suggested fix
const hits = await qdrantStore.searchHybridAsync({ queryVector: queryResult.vector, queryText: opts.query, limit: opts.limit, }); + const filterClause = opts.filters?.sql ? ` AND ${opts.filters.sql}` : ""; + const filterParams = opts.filters?.params ?? []; // Resolve doc IDs back to full documents from SQLite content table const results: SearchResult<TDoc>[] = []; for (const hit of hits) { const doc = this.db - .query(`SELECT c.* FROM ${this.contentTableName} c WHERE c.${this.config.schema.idField} = ?`) - .get(hit.docId) as TDoc | null; + .query( + `SELECT c.* FROM ${this.contentTableName} c WHERE c.${this.config.schema.idField} = ?${filterClause}` + ) + .get(hit.docId, ...filterParams) as TDoc | null; if (doc) { results.push({ doc, score: hit.score, method: "rrf" }); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/search/drivers/sqlite-fts5/index.ts` around lines 503 - 533, The hybrid Qdrant branch (inside the block using this._vectorStore/searchHybridAsync and this.embedder.embed) is resolving docs by ID but ignoring opts.filters; update the post-search retrieval to apply the SQL filters by selecting rows WHERE id IN (...) AND <filters> instead of just WHERE id = ?, or alternately build a single SELECT that joins the candidate IDs with the content table and includes opts.filters; use this.contentTableName and this.config.schema.idField to form the query and bind parameters safely from the candidate IDs and opts.filters (preserve limit/ordering), and ensure you handle the case of many candidate IDs (e.g., chunking or an IN clause) so filtered hybrid results respect opts.filters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/indexer/lib/filters.test.ts`:
- Around line 65-67: Add a regression test that asserts buildMetadataPredicate
rejects an empty array for an "IN" op: update the test around the existing
it("rejects non-array IN values") case to also call buildMetadataPredicate("t",
[{ column: "x", op: "IN", value: [] }]) and expect it to throw (matching
/array/i or similar) so the code prevents generating an empty IN () SQL clause;
reference the buildMetadataPredicate function and the test case in
filters.test.ts when adding the assertion.
In `@src/indexer/lib/filters.ts`:
- Around line 57-64: The IN branch currently allows an empty array and can emit
invalid SQL like c.<column> IN (), so update the guard in the f.op === "IN"
handling to reject empty arrays: after confirming Array.isArray(f.value), check
f.value.length > 0 and throw a clear error (e.g., "Op IN requires a non-empty
array value.") if it is empty; this affects the block that builds the
placeholder string with const ph = f.value.map(() => "?").join(", ") and the
subsequent parts.push(`c.${f.column} IN (${ph})`) and params.push(...f.value).
In `@src/indexer/lib/store.ts`:
- Around line 285-292: The warning path in searchIndexReadonly calls
getPendingMigrations which currently bootstraps (mutates) the _migrations table
and can fail when opened readonly; change the logic so read-only callers do not
trigger migration-table setup: either (A) update getPendingMigrations to accept
a readonly flag and, when readonly is true, perform a non-mutating
existence/status check (do not call bootstrap/ensure migrations table), or (B)
here in the searchIndexReadonly flow check for the presence of the migrations
table first (e.g., tableExists('_migrations')) and only call
getPendingMigrations when the table exists; keep using pendingWarnedFor,
INDEXER_MIGRATIONS and logger.warn as before but avoid any code path that
creates or modifies migration bookkeeping when opened readonly.
- Around line 274-282: Replace encodeURI(attachPath) with a proper file URL
built from pathToFileURL(resolve(attachPath)).href: import { pathToFileURL }
from "node:url" and { resolve } from "path", then change the uri assignment to
use pathToFileURL(resolve(attachPath)).href (keep the existing mode/alias logic
and the db.run(`ATTACH DATABASE '${uri}' AS ${alias}`) call).
In `@src/macos/commands/mail/accounts.ts`:
- Around line 34-38: The informational log about unknown accounts (the
p.log.info call that prints `${unknownCount} account(s) ...`) must be gated by
the tool's shared output-mode (silent/quiet/verbose) handling so it doesn't emit
during --silent or piped runs; update the code around unknownCount/accounts to
check the existing output-mode flag or helper (e.g., p.opts.silent,
p.isSilent(), p.outputMode, or the project's shared output utility) and only
call p.log.info when output is allowed (e.g., not silent or when verbose),
preserving the same message text.
In `@src/utils/database/migrations.ts`:
- Around line 65-72: Wrap the migration application and its tracking insert in a
single DB transaction so they are atomic: start a transaction, call m.apply(db,
ctx) (measure performance before/after apply to compute ms), run the INSERT OR
REPLACE into MIGRATIONS_TABLE with scopedId/Date.now()/ms, and then COMMIT; on
any error during m.apply or the insert, ROLLBACK and rethrow the error. Update
the code around m.apply(...) and the db.run(...) insert to use the DB
transaction APIs (or explicit BEGIN/COMMIT/ROLLBACK) and ensure rollback happens
on exceptions so partial schema/data changes are not left untracked.
In `@src/utils/macos/MailDatabase.ts`:
- Around line 103-109: The mailbox LIKE pattern in listMessages() currently
builds `%${mailbox}%` raw and can overmatch; update the mailboxPattern to use
the centralized escapeLike function (e.g., mailboxPattern =
`%${escapeLike(mailbox)}%`) and modify the SQL to use the escaped pattern with
an ESCAPE clause (change "mb.url LIKE $mailbox" to "mb.url LIKE $mailbox ESCAPE
'\\'") so the parameter $mailbox receives the escaped pattern and SQL treats
backslash-escaped %, _, etc., literally.
In `@src/utils/search/types.test.ts`:
- Around line 24-25: The test only asserts predicate.sql but must also assert
predicate.params to catch parameter-binding regressions: update the test where
predicate: SearchFilterPredicate = opts.filters ?? { sql: "", params: [] } (and
the expect(predicate.sql).toBe("c.category = ?")) to add an assertion that
predicate.params equals the expected params array used in the test setup (e.g.,
the category value passed via opts), so verify both predicate.sql and
predicate.params for the SearchFilterPredicate returned by opts.filters.
---
Outside diff comments:
In `@src/utils/search/drivers/sqlite-fts5/index.ts`:
- Around line 503-533: The hybrid Qdrant branch (inside the block using
this._vectorStore/searchHybridAsync and this.embedder.embed) is resolving docs
by ID but ignoring opts.filters; update the post-search retrieval to apply the
SQL filters by selecting rows WHERE id IN (...) AND <filters> instead of just
WHERE id = ?, or alternately build a single SELECT that joins the candidate IDs
with the content table and includes opts.filters; use this.contentTableName and
this.config.schema.idField to form the query and bind parameters safely from the
candidate IDs and opts.filters (preserve limit/ordering), and ensure you handle
the case of many candidate IDs (e.g., chunking or an IN clause) so filtered
hybrid results respect opts.filters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ca3e9ba-9982-4a65-99a5-e23204fdbbbe
📒 Files selected for processing (35)
CLAUDE.mdsrc/indexer/lib/filters.test.tssrc/indexer/lib/filters.tssrc/indexer/lib/indexer-migrations.test.tssrc/indexer/lib/indexer-migrations.tssrc/indexer/lib/metadata-pushdown.e2e.test.tssrc/indexer/lib/metadata-schema.test.tssrc/indexer/lib/metadata-schema.tssrc/indexer/lib/sources/source.tssrc/indexer/lib/store.search-attach.test.tssrc/indexer/lib/store.tssrc/indexer/lib/types.tssrc/logger.tssrc/macos/commands/mail/accounts.tssrc/macos/commands/mail/search.tssrc/macos/lib/mail/emlx.test.tssrc/macos/lib/mail/emlx.tssrc/macos/lib/mail/search-filters.test.tssrc/macos/lib/mail/search-filters.tssrc/macos/lib/mail/search-label.tssrc/macos/lib/mail/spotlight.test.tssrc/macos/lib/mail/spotlight.tssrc/macos/lib/mail/sqlite-escape.test.tssrc/macos/lib/mail/sqlite.tssrc/utils/cli/index.tssrc/utils/cli/output-mode.test.tssrc/utils/cli/output-mode.tssrc/utils/database/migrations.test.tssrc/utils/database/migrations.tssrc/utils/macos/MacDatabase.tssrc/utils/macos/MailDatabase.tssrc/utils/macos/mail-sql.tssrc/utils/search/drivers/sqlite-fts5/index.tssrc/utils/search/types.test.tssrc/utils/search/types.ts
💤 Files with no reviewable changes (1)
- src/macos/lib/mail/sqlite.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/indexer/lib/metadata-schema.ts`:
- Around line 155-197: The update path currently binds every declared column
(using coerceMetadataValue) even when the source omitted a key, causing
undefined → NULL and overriding column defaults; fix by only including declared
columns that are present in the incoming metadata when building the UPDATE and
its parameters: inside the transaction loop in the block that iterates batches
from source.populateMetadata, compute presentColumns = columns.filter(c =>
Object.prototype.hasOwnProperty.call(metadata, c.name)), build a per-row SET
clause from presentColumns (e.g., presentColumns.map(c => `${c.name} =
?`).join(", ")), call coerceMetadataValue only for those present columns to form
typed params, then execute an UPDATE that appends metadata_json = ? and WHERE
source_id = ? (handling the case of zero present declared columns by updating
metadata_json only); keep existingBagBySourceId, declaredNames,
SafeJSON.stringify and sourceId logic unchanged.
In `@src/indexer/lib/storage.ts`:
- Around line 165-170: The TEST_INDEX_PREFIXES array used by
wipeAllTestIndexes() is too broad (e.g., "test_") and can delete real user
indexes; replace those entries with collision-resistant, test-owned prefixes
(for example only project-specific or CI-generated patterns such as "ci_test_",
"internal_e2e_test_", "tmp_idx_test_", or use a deterministic generated prefix)
and update any other usage sites referenced by wipeAllTestIndexes() so only
these narrow, unique prefixes are matched; ensure TEST_INDEX_PREFIXES and the
wipeAllTestIndexes() logic include a clear comment about their destructive
nature and do not include generic prefixes like "test_" or "integration_test_".
In `@src/macos/commands/mail/search.ts`:
- Around line 144-152: The announce function has redundant branches that call
logger.info(msg) in both the isStructuredOutput and quiet cases; simplify by
combining those conditions so that if (isStructuredOutput || quiet) you call
logger.info(msg) and otherwise call p.log.info(msg); update the announce
definition (referencing announce, isStructuredOutput, quiet, logger, and p.log)
accordingly to remove the duplicate branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9c15c034-a6c6-4e80-88ac-0c68057acd96
📒 Files selected for processing (15)
CLAUDE.mdbunfig.tomlsrc/indexer/lib/indexer.test.tssrc/indexer/lib/metadata-pushdown.e2e.test.tssrc/indexer/lib/metadata-schema.test.tssrc/indexer/lib/metadata-schema.tssrc/indexer/lib/storage.tssrc/indexer/lib/store-embedder.test.tssrc/indexer/lib/store.search-attach.test.tssrc/indexer/lib/store.tssrc/indexer/lib/test-cleanup-preload.tssrc/macos/commands/mail/search.tssrc/macos/lib/mail/sqlite-escape.test.tssrc/utils/macos/MailDatabase.tssrc/utils/macos/mail-sql.ts
genesiscz
force-pushed
the
feat/search-index-mail-improvements
branch
from
April 30, 2026 01:34
b034f2b to
f20cbde
Compare
Adds reusable migration framework under src/utils/database/migrations.ts with id+description+isApplied+apply contract, _migrations journal table scoped per (id, tableName), atomic BEGIN/COMMIT around each apply, and a Migrator adapter consumed by MacDatabase subclasses via getMigrator().
- INDEXER_MIGRATIONS list (source_id index, FTS5 unicode61 remove_diacritics 2,
metadata_json column) applied via the generic migration engine on
read-write opens.
- searchIndexReadonly accepts {filters, attach, metadataFilters}; ATTACHes
external read-only DBs via file: URI with mode=ro for cross-DB pushdown,
AND-merges typed and ad-hoc filters before BM25/cosine.
- Per-source typed metadata columns: applySourceMetadataSchema diffs declared
vs persisted columns and runs ALTER + CREATE INDEX; backfillMetadataColumns
pages distinct source_ids and UPDATEs all chunks per source. Shared
coerceMetadataValue (bool→0/1, Date→epoch ms) used by insertChunks too.
- buildMetadataPredicate emits c.<col> {=|<=|>=|BETWEEN|IN|LIKE} ? for
filter pushdown.
…ch body parts
- buildMailFilterPredicate emits c.source_id IN (SELECT CAST(m.ROWID AS TEXT)
FROM mailapp.messages ...) so the mail Envelope Index ATTACH-pushes
date/mailbox/account/receiver filters into the FTS query (7-55ms vs 300ms
broken pipeline at limit=50).
- mdfindMailRowids (Spotlight) covers email body when the FTS index is
missing; sanitizeSpotlightQuery strips NSPredicate metacharacters but
preserves Unicode letters incl. diacritics.
- searchMessages (in MailDatabase) uses ordered + any-order ANDed wildcard
patterns and drops the hard limit so pagination happens after merge with
Spotlight. listAccounts derives email from Sent-folder senders first then
falls back to non-Sent recipients (handles receive-only accounts; covers
Sent/Sent Items/Outbox/Drafts variants).
- Mail SQL helpers (escapeLike, LIKE_ESCAPE_CLAUSE, MESSAGE_SELECT,
buildFilters) live in src/utils/macos/mail-sql.ts so the LIKE-escape rule
can't drift across callers (regression-guarded by sqlite-escape.test.ts).
- Rich body extraction: parseEmlxBodyPartsFromFile returns {raw, html,
markdown, text}; new bodyHtml/bodyMarkdown/bodyRaw columns surface the
parts via 'tools macos mail show' and 'mail search'.
- Search label formatting centralized in search-label.ts; MailFilter
predicate dedupes the mailboxes JOIN via a joinedMb flag.
…ools/logs - bunfig.toml [test].preload runs wipeAllTestIndexes() once before any test file imports — covers every gt_*/phase2-* prefix tests own so a crashed prior run can't accumulate stale dirs in ~/.genesis-tools/indexer. _resetIndexerStorageForTesting() lets HOME-redirect tests force the singleton to re-read after they swap process.env.HOME. - Test prefixes renamed to gt_* (gt_e2e_test_, gt_indexer_test_, gt_integration_test_, gt_bench_) for collision resistance with real user index names. - isQuietOutput() helper for non-table output formats (JSON/TOON), wired through 'mail search' so --format json is pipeable. - logger writes day-stamped files under ~/.genesis-tools/logs/ instead of the repo's /logs/ dir. - CLAUDE.md: Database Infrastructure section documenting Storage, MacDatabase, Migrator, INDEXER_MIGRATIONS, and metadata-schema.
genesiscz
force-pushed
the
feat/search-index-mail-improvements
branch
from
April 30, 2026 01:39
f20cbde to
c288b84
Compare
9 tasks
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
1 participant
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.
Summary
Fixes the mail search pipeline where
tools macos mail searchreturned 2 RRF hits at--limit 50while--limit 20returned 0 + 20 unrelated metadata fallbacks. Filters were being applied after FTS instead of pushed into it; FTS had no diacritic folding; there was nosource_idindex; and the fallback path had no body coverage.ATTACH DATABASEof Mail.app's Envelope Index (read-only) +c.source_id IN (SELECT m.ROWID ...)predicate. Bench: 7-55ms vs 300ms broken pipeline.src/utils/database/migrations.ts) used for:source_idB-tree index, FTS5 rebuild withunicode61 remove_diacritics 2, andmetadata_jsoncolumn.metadata-schema.ts,filters.ts) — library-level, no first adopter yet.mdfindfor body coverage +%token%token%ordered + any-order ANDed LIKE patterns.src/utils/macos/mail-sql.ts(escapeLike,LIKE_ESCAPE_CLAUSE,MESSAGE_SELECT,buildFilters) so the SQLite ESCAPE rule can't drift between callers.source_ids andUPDATE WHERE source_id = ?so multi-chunk sources don't lose N-1 chunks; sharedcoerceMetadataValuehelper handles boolean/Date.tools macos mail accountsno longer shows "unknown" for receive-only accounts: derive emails from Sent-folder senders first, fall back to non-Sent type=0 recipients.--format jsonis pipeable (no Clack spinners on stdout) via newisQuietOutput()helper.Test plan
bun test src/macos/lib/mail/— sqlite-escape, search-filters, spotlight, emlxbun test src/indexer/lib/— migrations, metadata-schema, store.search-attach, metadata-pushdown.e2ebun test src/utils/database/migrations.test.ts src/utils/cli/output-mode.test.tstsgo --noEmitcleantools macos mail search "premier" --from 2026-01-01 --to 2026-03-10 --format json --columns id,date,from,subject --limit 50returns expected count of body matchestools macos mail accountsshows real email for every account (no "unknown")Summary by CodeRabbit
New Features
Bug Fixes
Infrastructure
Documentation