Skip to content

Comments

fix(#233): improve multi-collection search filtering#241

Open
ambicuity wants to merge 2 commits intotobi:mainfrom
ambicuity:fix-multi-collection-search-233
Open

fix(#233): improve multi-collection search filtering#241
ambicuity wants to merge 2 commits intotobi:mainfrom
ambicuity:fix-multi-collection-search-233

Conversation

@ambicuity
Copy link

Fixes #233 and #217. This pull request modifies the core search queries in store.ts to natively construct SQL IN (?, ...) clauses instead of performing in-memory post-filtering. This fixes the bug where multiple collection flags might result in empty returns.

Copilot AI review requested due to automatic review settings February 21, 2026 15:25
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request addresses bugs #233 and #217 where multi-collection search with multiple -c flags would return empty results if any collection had no matches. The fix refactors the search implementation to use native SQL IN (?, ...) clauses instead of post-filtering results in memory, ensuring that results from collections with matches are returned even when other specified collections have no matches.

Changes:

  • Modified searchFTS and searchVec to accept string | string[] for collection filtering and construct SQL IN clauses for array inputs
  • Updated search commands in qmd.ts to pass collection arrays directly to search functions instead of post-filtering results
  • Added test case for multi-collection search to verify the fix

Reviewed changes

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

File Description
src/store.ts Updated searchFTS and searchVec function signatures and implementations to support array-based collection filtering with SQL IN clauses; updated interface types for HybridQueryOptions and StructuredSearchOptions
src/qmd.ts Removed post-filtering logic from search, vectorSearch, and querySearch functions; now passes collection arrays directly to store search functions
test/cli.test.ts Added test case for multi-collection search filtering; minor whitespace formatting changes in catch blocks

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +3232 to +3236
const collections = Array.isArray(options?.collection)
? options?.collection
: options?.collection
? [options?.collection]
: undefined;
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The structuredSearch function still iterates over collections one at a time (lines 3282-3283 for FTS and 3310-3311 for vector searches), defeating the purpose of the SQL IN clause optimization introduced in this PR. Instead of for (const coll of collectionList) and calling store.searchFTS(search.query, 20, coll), it should pass the collections array directly: store.searchFTS(search.query, 20, collections). The same applies to the vector search loop. This would make a single SQL query with IN (?, ?, ...) instead of multiple sequential queries.

Copilot uses AI. Check for mistakes.
test/cli.test.ts Outdated
Comment on lines 558 to 561
expect(exitCodeEmptyRef).toBe(0);
// The query should function smoothly even with nonexistent collections as long as valid ones are requested
expect(stdoutEmptyRef.toLowerCase()).toContain("meeting");
});
Copy link

Copilot AI Feb 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will fail because resolveCollectionFilter in src/qmd.ts (lines 1907-1925) exits the process with code 1 when it encounters a collection that doesn't exist. The function calls getCollectionFromYaml(name) and if it returns falsy, it prints an error and calls process.exit(1). This means the test at line 558 will expect exitCodeEmptyRef to be 1, not 0, and the process won't reach line 560 to check for "meeting" in the output.

Suggested change
expect(exitCodeEmptyRef).toBe(0);
// The query should function smoothly even with nonexistent collections as long as valid ones are requested
expect(stdoutEmptyRef.toLowerCase()).toContain("meeting");
});
// When a nonexistent collection is requested, qmd exits with error code 1.
expect(exitCodeEmptyRef).toBe(1);
});

Copilot uses AI. Check for mistakes.
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.

search: multi-collection -c filtering returns empty when any collection has no matches

1 participant