Skip to content

Improve search #1925

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Improve search #1925

wants to merge 24 commits into from

Conversation

flevi29
Copy link
Collaborator

@flevi29 flevi29 commented Apr 18, 2025

Pull Request

Related issue

Fixes #1767

What does this PR do?

Migration

client.index(...).search now has only two parameters, it requires q parameter to be inside the old second parameter, which is now the first parameter.

- client.index('movies').search('American ninja')
+ client.index('movies').search({ q: 'American ninja' })

multiSearch no longer accepts TypeScript generics.

- client.multiSearch<FederatedMultiSearchParams, YourType>({ federation: {}, queries: [/* ... */] });
- client.multiSearch<MultiSearchParams, YourType>({ queries: [/* ... */] });
+ const { hits } = client.multiSearch({ federation: {}, queries: [/* ... */] });
+ hits as FederatedSearchHit<YourType>[];
+ const { results } = client.multiSearch({ queries: [/* ... */] });
+ for (const { indexUid, hits } of results) {
+   if (indexUid === "yourIndexUid") {
+     hits as SearchHit<YourType>[];
+   }
+ }

Types:

  • MatchingStrategies -> MatchingStrategy
    • no longer a runtime constant
  • Filter -> FilterExpression
  • deleted Query
  • deleted Highlight
  • deleted Crop
  • SearchForFacetValuesParams -> FacetSearchQuery
  • FacetHit -> FacetValueHit
  • SearchForFacetValuesResponse -> FacetSearchResult
  • HybridSearch -> HybridQuery
  • SearchParams -> SearchQuery
  • SearchRequestGET -> SearchQueryGet
  • MultiSearchQuery -> SearchQueryWithIndex
  • MultiSearchQueryWithFederation -> SearchQueryWithIndexAndFederation
  • MultiSearchParams -> MultiSearch
  • FederatedMultiSearchParams -> FederatedSearch
  • CategoriesDistribution -> FieldDistribution
  • deleted Facet
  • deleted FacetDistribution
  • RankingScoreDetails -> ScoreDetails
  • Hit -> SearchHit
  • deleted Hits
  • FacetStat -> FacetStats
  • deleted FacetStats
  • FacetsByIndex -> FederatedFacets
  • SearchResponse -> SearchResult
  • deleted MultiSearchResult
  • MultiSearchResponse -> SearchResults
  • MultiSearchResponseOrSearchResponse -> SearchResultsOrFederatedSearchResult
  • SearchSimilarDocumentsParams -> SimilarQuery

Summary by CodeRabbit

  • New Features

    • Added support for managing experimental features with new enable/disable methods.
    • Introduced federated multi-search, similarity search methods, and enhanced network configuration options.
    • Expanded and refined type definitions for advanced search parameters and responses, including hybrid, vector, and federated search.
  • Bug Fixes

    • Standardized search method calls to use a single options object with a q property for query strings, improving API consistency.
  • Documentation

    • Updated examples and API references to align with the unified search method signatures and parameter formats.
  • Refactor

    • Refactored search methods and internal logic for improved type safety and simplified parameter handling.
    • Removed legacy and redundant type definitions and deprecated test suites.
  • Tests

    • Rewrote and streamlined search tests for comprehensive coverage and reliability.
    • Enhanced test utilities and added a detailed film dataset for testing.
  • Chores

    • Updated ESLint rules to enforce assertion usage in tests.
    • Consolidated and expanded type exports for better external accessibility.

@flevi29 flevi29 added the breaking-change The related changes are breaking for the users label Apr 18, 2025
@flevi29 flevi29 mentioned this pull request Apr 25, 2025
@flevi29 flevi29 mentioned this pull request Apr 25, 2025
3 tasks
Copy link

codecov bot commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.70%. Comparing base (77505c0) to head (fc0bd5d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1925      +/-   ##
==========================================
- Coverage   99.03%   95.70%   -3.34%     
==========================================
  Files          18       17       -1     
  Lines        1449     1442       -7     
  Branches      305      279      -26     
==========================================
- Hits         1435     1380      -55     
- Misses         14       62      +48     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

coderabbitai bot commented May 14, 2025

Walkthrough

This update refactors the MeiliSearch JavaScript client’s search API, unifying search method signatures to accept a single options object with a q property for the query string and other parameters. It introduces new and updated types for search parameters and responses, adds experimental feature controls, and modernizes multi-search and federated search APIs with precise TypeScript overloads. Test suites and code samples are updated to match the new API, while legacy types and redundant tests are removed.

Changes

File(s) / Group Change Summary
.code-samples.meilisearch.yaml, README.md, tests/env/node/getting_started.cjs, tests/env/node/search_example.cjs, tests/env/typescript-node/src/index.ts, tests/token.test.ts Updated all search method usages and documentation to use a single object argument with q for the query string, removing positional string arguments.
src/indexes.ts, src/meilisearch.ts Refactored search-related methods to use typed query objects, added overloads, improved type safety, and standardized parameter serialization. Added experimental feature endpoints and refactored multi-search with overloads for federated search.
src/types/search-parameters.ts, src/types/search-response.ts, src/types/network.ts, src/types/shared.ts, src/types/token.ts Added new or updated type definitions for search parameters, responses, network, utility types, and token rules. Removed legacy types from src/types/types.ts.
src/types/index.ts Re-exported new and updated type modules, expanded available types.
src/utils.ts Added utility function stringifyRecordKeyValues for serializing object values.
src/http-requests.ts Simplified value handling in URL parameter appending, removing special Date handling.
eslint.config.js Added Vitest assertion rule for test files.
playgrounds/javascript/src/meilisearch.ts Updated search usage and types to new object-based API.
tests/client.test.ts Enabled/disabled experimental network feature in test lifecycle hooks.
tests/embedders.test.ts Removed dataset and tests for vector/hybrid search and similar documents.
tests/facet_search.test.ts, tests/get_search.test.ts, tests/typed_search.test.ts Deleted legacy test suites for facet, GET, and typed search.
tests/search.test.ts Completely refactored and modernized search tests to use new API, types, and features with broader coverage.
tests/utils/meilisearch-test-utils.ts Added new assertion helpers and a typed object keys utility.
tests/utils/search.ts, tests/utils/test-data/films.ts Introduced new test utilities and a detailed film dataset for search testing.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant Client (JS)
  participant MeiliSearch Server

  User->>Client (JS): index.search({ q, ...params })
  Client (JS)->>MeiliSearch Server: POST /indexes/:uid/search { q, ...params }
  MeiliSearch Server-->>Client (JS): SearchResult
  Client (JS)-->>User: SearchResult

  User->>Client (JS): client.multiSearch({ queries: [...] })
  Client (JS)->>MeiliSearch Server: POST /multi-search { queries }
  MeiliSearch Server-->>Client (JS): MultiSearchResponse or FederatedSearchResult
  Client (JS)-->>User: MultiSearchResponse or FederatedSearchResult

  User->>Client (JS): client.getExperimentalFeatures()
  Client (JS)->>MeiliSearch Server: GET /experimental-features
  MeiliSearch Server-->>Client (JS): RuntimeTogglableFeatures
  Client (JS)-->>User: RuntimeTogglableFeatures

  User->>Client (JS): client.updateExperimentalFeatures(features)
  Client (JS)->>MeiliSearch Server: PATCH /experimental-features { features }
  MeiliSearch Server-->>Client (JS): RuntimeTogglableFeatures
  Client (JS)-->>User: RuntimeTogglableFeatures
Loading

Poem

In the warren of code, a search was reborn,
With queries now bundled, old habits are shorn.
Types are precise, and overloads delight,
Experimental toggles hop into sight.
Tests pruned and rewritten, the garden grows neat—
This rabbit approves: the update’s a treat!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 371289d and fc0bd5d.

📒 Files selected for processing (2)
  • src/types/search-response.ts (1 hunks)
  • tests/utils/search.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/utils/search.ts

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@flevi29 flevi29 marked this pull request as ready for review May 14, 2025 09:09
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (11)
src/meilisearch.ts (1)

220-238: Consider harmonising parameter naming & add a short JSDoc block

The overload signatures use the parameter identifiers multiSearch / federatedSearch, whereas the implementation that follows accepts a generic body.
Although this is perfectly valid TypeScript, the sudden change of identifier breaks the mental mapping for a reader and hurts IDE “rename-symbol” refactors.

Also, the public method currently lacks any JSDoc even though the rest of the class is meticulously documented.

-  /** {@link https://www.meilisearch.com/docs/reference/api/multi_search} */
-  multiSearch(
+  /**
+   * Performs a multi-index or federated search request.
+   * {@link https://www.meilisearch.com/docs/reference/api/multi_search}
+   */
+  multiSearch(

[Naming consistency & documentation]

tests/search.test.ts (2)

70-74: Potentially incomplete network clean-up in afterAll

beforeAll enables the network feature with a non-empty self/remotes object.
afterAll resets the network with an empty object, but does not disable the feature itself (network: false).

If other suites run after this one, the residual toggle may leak state and provoke 401s or unexpected routing. Consider restoring the previous value, e.g.

 await index.delete().waitTask();
-await client.updateNetwork({});
+await client.updateExperimentalFeatures({ network: false });
+await client.updateNetwork({});

436-450: Guard against undefined _vectors to avoid false positives

assertIsVectors assumes vectors is always defined, yet the API returns null (or absence) when retrieveVectors=false. Calling this helper accidentally in such cases would throw in the helper instead of failing the test meaningfully.

-function assertIsVectors(vectors: SearchHit<Film>["_vectors"]) {
+function assertIsVectors(vectors: SearchHit<Film>["_vectors"] | undefined) {
+  assert.isDefined(vectors, "expected _vectors to be present in hit");
tests/utils/test-data/films.ts (1)

1-14: Reduce repetition & bundle size by extracting common literals

The dataset repeats the exact same Apple-iTunes provider object hundreds of times, inflating the test bundle.
Creating a small helper, e.g. const APPLE_ITUNES = { name: 'Apple iTunes', logo: '/q6tl6Ib6X5FT80RMlcDbexIo4St.jpg' }, and re-using it with the spread operator would make this file shorter, easier to edit, and less error-prone.

Purely a maintainability suggestion – no functional impact.

tests/utils/search.ts (1)

27-39: Guard against undefined when destructuring searchQuery

If callers ever do searchGet.searchMethod(undefined) the destructuring
const { hybrid, ...rest } = searchQuery; throws at runtime.
A tiny defensive tweak removes that foot-gun without altering typings:

-    searchMethod: (searchQuery) => {
-      const { hybrid, ...rest } = searchQuery;
+    searchMethod: (searchQuery = {}) => {
+      const { hybrid, ...rest } = searchQuery;
src/indexes.ts (2)

126-134: Skip calling the helper when no query params are provided

stringifyRecordKeyValues(undefined, ["filter"]) works only if the
helper defends against undefined. Making that explicit improves
readability and prevents future refactors from breaking:

-      params: stringifyRecordKeyValues(searchQuery, ["filter"]),
+      params:
+        searchQuery !== undefined
+          ? stringifyRecordKeyValues(searchQuery, ["filter"])
+          : undefined,

298-299: Type-only cast hides useful compiler checks

params as Omit<typeof params, "filter"> always collapses to {} once
params is undefined, discarding the real shape of DocumentsQuery.
That defeats the purpose of typing the request parameters.

Instead, keep the original type when filter is absent:

-          params: params as Omit<typeof params, "filter">,
+          // Safe: we are in the branch where `filter` is `undefined`
+          params: params as Exclude<DocumentsQuery<D>, { filter: unknown }>,

This preserves the other query-string keys (fields, limit, …) and
lets TypeScript flag accidental misspellings.

src/types/types.ts (3)

112-116: Name collision risk – two different Pagination shapes exist now

src/types/search-parameters.ts already exports a Pagination with { page; hitsPerPage; }.
Introducing another Pagination here ({ offset; limit; }) with the same identifier but different shape can confuse consumers and tooling (auto-imports, code completion). Renaming this one to something like OffsetPagination or namespacing it would avoid ambiguity.


170-177: retrieveVectors flag lacks documentation & version guard

The new retrieveVectors?: boolean is great, but:

  1. Please add a JSDoc comment explaining that it retrieves stored vectors and requires Meilisearch ≥ 1.4 (or the correct minimum).
  2. Consider gating it behind a feature flag type‐side (e.g., conditional on ExperimentalFeatures) so users on older servers don’t inadvertently compile code that will 400.

370-375: Possible type duplication – FieldDistribution defined in multiple places

FieldDistribution is now declared here, but it also exists in src/types/search-response.ts (imported by other modules). Having two identical declarations in different modules leads to structural-typing compatibility but breaks instanceof checks and confuses documentation generators. Prefer a single, shared definition in shared.ts and re-export from here if needed.

src/types/search-parameters.ts (1)

77-78: Consider addressing the TODO comment

There's a TODO comment about potentially using generic records to type certain settings. This could be addressed in this PR or at least include more context about why it's being deferred.

Either implement the suggested approach or add more context to the TODO about the trade-offs and when this might be addressed.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 438da05 and 3671911.

⛔ Files ignored due to path filters (1)
  • tests/__snapshots__/search.test.ts.snap is excluded by !**/*.snap
📒 Files selected for processing (29)
  • .code-samples.meilisearch.yaml (15 hunks)
  • README.md (6 hunks)
  • eslint.config.js (1 hunks)
  • playgrounds/javascript/src/meilisearch.ts (2 hunks)
  • src/http-requests.ts (1 hunks)
  • src/indexes.ts (3 hunks)
  • src/meilisearch.ts (3 hunks)
  • src/types/experimental-features.ts (1 hunks)
  • src/types/index.ts (1 hunks)
  • src/types/network.ts (1 hunks)
  • src/types/search-parameters.ts (1 hunks)
  • src/types/search-response.ts (1 hunks)
  • src/types/shared.ts (2 hunks)
  • src/types/token.ts (1 hunks)
  • src/types/types.ts (7 hunks)
  • src/utils.ts (1 hunks)
  • tests/client.test.ts (1 hunks)
  • tests/embedders.test.ts (0 hunks)
  • tests/env/node/getting_started.cjs (1 hunks)
  • tests/env/node/search_example.cjs (1 hunks)
  • tests/env/typescript-node/src/index.ts (1 hunks)
  • tests/facet_search.test.ts (0 hunks)
  • tests/get_search.test.ts (0 hunks)
  • tests/search.test.ts (1 hunks)
  • tests/token.test.ts (2 hunks)
  • tests/typed_search.test.ts (0 hunks)
  • tests/utils/meilisearch-test-utils.ts (4 hunks)
  • tests/utils/search.ts (1 hunks)
  • tests/utils/test-data/films.ts (1 hunks)
💤 Files with no reviewable changes (4)
  • tests/get_search.test.ts
  • tests/typed_search.test.ts
  • tests/embedders.test.ts
  • tests/facet_search.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (9)
src/types/token.ts (1)
src/types/search-parameters.ts (1)
  • SearchQuery (147-149)
playgrounds/javascript/src/meilisearch.ts (1)
src/types/search-parameters.ts (1)
  • SearchQueryWithOffsetLimit (126-126)
tests/client.test.ts (1)
tests/utils/meilisearch-test-utils.ts (3)
  • getClient (275-275)
  • clearAllIndexes (265-265)
  • config (266-266)
tests/utils/meilisearch-test-utils.ts (1)
src/http-requests.ts (1)
  • T (192-257)
src/types/search-parameters.ts (3)
src/types/shared.ts (4)
  • PascalToCamelCase (21-21)
  • SafeOmit (29-29)
  • RequiredKeys (8-10)
  • NonNullKeys (4-6)
src/types/types.ts (1)
  • Locale (327-327)
src/http-requests.ts (1)
  • T (192-257)
src/meilisearch.ts (4)
src/types/search-parameters.ts (3)
  • MultiSearch (199-199)
  • FederatedSearch (189-192)
  • MultiSearchOrFederatedSearch (202-202)
src/types/types.ts (1)
  • ExtraRequestInit (24-24)
src/types/search-response.ts (3)
  • SearchResults (135-137)
  • FederatedSearchResult (110-114)
  • SearchResultsOrFederatedSearchResult (140-142)
src/types/experimental-features.ts (1)
  • RuntimeTogglableFeatures (6-14)
src/types/search-response.ts (4)
src/types/shared.ts (2)
  • RecordAny (2-2)
  • DeepStringRecord (23-27)
src/http-requests.ts (1)
  • T (192-257)
src/types/search-parameters.ts (1)
  • Pagination (45-50)
src/types/types.ts (2)
  • FieldDistribution (374-374)
  • MeiliSearchErrorResponse (460-467)
src/types/types.ts (1)
src/types/search-parameters.ts (2)
  • Pagination (45-50)
  • FilterExpression (59-59)
tests/search.test.ts (7)
tests/utils/search.ts (5)
  • client (13-13)
  • INDEX_UID (12-12)
  • index (14-14)
  • search (16-126)
  • assertFacetDistributionAndStatsAreCorrect (128-146)
tests/utils/meilisearch-test-utils.ts (4)
  • HOST (271-271)
  • MASTER_KEY (272-272)
  • assert (143-146)
  • objectKeys (264-264)
src/meilisearch.ts (2)
  • index (89-91)
  • multiSearch (229-238)
tests/utils/test-data/films.ts (2)
  • FILMS (16-887)
  • Film (1-14)
src/types/search-parameters.ts (5)
  • SearchQuery (147-149)
  • MatchingStrategy (20-20)
  • FilterExpression (59-59)
  • SearchQueryWithIndexAndFederation (158-161)
  • Federation (177-182)
src/indexes.ts (4)
  • search (105-114)
  • searchGet (125-134)
  • searchSimilarDocuments (149-158)
  • searchSimilarDocumentsGet (161-170)
src/types/search-response.ts (4)
  • SearchResultWithOffsetLimit (88-89)
  • SearchResultWithPagination (86-87)
  • SearchHit (29-38)
  • ExplicitVectors (23-26)
🪛 GitHub Check: codecov/patch
src/utils.ts

[warning] 24-25: src/utils.ts#L24-L25
Added lines #L24 - L25 were not covered by tests

src/meilisearch.ts

[warning] 428-431: src/meilisearch.ts#L428-L431
Added lines #L428 - L431 were not covered by tests

🔇 Additional comments (65)
tests/env/node/search_example.cjs (1)

27-29: Updated search method call signature correctly implemented

The search method call has been updated to use the new unified signature that accepts a single options object with the query string under the q key, aligning with the API changes mentioned in the PR objectives.

eslint.config.js (1)

44-49: Good addition of test assertion verification

Adding the vitest/expect-expect rule ensures all test files include at least one assertion call, which prevents false-positive tests that don't actually verify anything. This is a good practice for maintaining test quality.

src/http-requests.ts (1)

25-25:

✅ Verification successful

Simplified URL parameter handling

The parameter handling has been simplified by removing special Date object processing and using a more straightforward string conversion approach. This reduces code complexity while maintaining functionality.


🏁 Script executed:

#!/bin/bash
# Check if Date objects were handled elsewhere in the codebase

# Look for Date handling in parameter processing or serialization
rg -A 2 -B 2 "instanceof Date" --type ts

Length of output: 42


🏁 Script executed:

#!/bin/bash
# Locate the http-requests.ts file
HTTP_FILE=$(fd http-requests.ts)
echo "Inspecting: $HTTP_FILE"

# Display the first 100 lines to review parameter handling context
sed -n '1,100p' "$HTTP_FILE"

# Search within the file for any Date-specific logic
rg -n "Date" "$HTTP_FILE"

Length of output: 3276


🏁 Script executed:

#!/bin/bash
# Locate the type definition file for URLSearchParamsRecord
TYPE_FILE=$(fd --full-path "src/types/index.js")
echo "Inspecting: $TYPE_FILE"

# Show the definition of URLSearchParamsRecord
rg -n "URLSearchParamsRecord" -C 5 "$TYPE_FILE"

Length of output: 163


🏁 Script executed:

#!/bin/bash
# Find the TypeScript file defining URLSearchParamsRecord
FILE=$(rg -l "URLSearchParamsRecord" --type ts)
echo "Definition file: $FILE"

# Show the URLSearchParamsRecord type declaration
rg -n "URLSearchParamsRecord" -C 5 "$FILE"

Length of output: 369


🏁 Script executed:

#!/bin/bash
# Properly iterate over files containing the type and show its declaration
FILES=$(rg -l "URLSearchParamsRecord" --type ts)
for file in $FILES; do
  echo "---- $file ----"
  rg -n "URLSearchParamsRecord" -C 5 "$file"
done

Length of output: 1822


No Date-specific logic found; Date handling removal is safe

I searched the entire codebase and did not find any instanceof Date checks or other Date-specific serialization elsewhere, and the URLSearchParamsRecord type only permits primitives (string, number, boolean, etc.), so dropping the special Date branch has no unintended impact.

– No other Date handling detected in src/http-requests.ts or related files
– The simplified String(val) conversion covers all permitted param types

tests/token.test.ts (2)

335-336: Updated search method signature for token validation test

The search method call has been updated to use the new unified signature with a q parameter, maintaining the test's ability to verify that searching with invalid token permissions correctly triggers an API key error.


353-354: Updated search method signature for token validation test

The search method call has been updated to use the new unified signature with a q parameter, maintaining the test's ability to verify that searching with incorrect index permissions correctly triggers an API key error.

src/types/token.ts (3)

1-1: Well-structured type import update

The import has been correctly changed from Filter to SearchQuery to align with the renamed types mentioned in the PR objectives. This ensures consistency with the new type system.


4-4: Good use of type utility for better type safety

Using Pick<SearchQuery, "filter"> to extract only the filter property from the larger SearchQuery type is a good practice. It ensures type safety while keeping the interface minimal.


11-11: Improved documentation reference

Simplifying the reference to the core implementation with a direct mention of meilisearch_auth::SearchRules instead of a GitHub URL makes the documentation more maintainable and less prone to becoming outdated.

tests/env/node/getting_started.cjs (3)

30-30: Updated search method signature as per PR changes

The search method call has been correctly updated to use the new signature with a single parameter object containing the query string under the q property.


32-36: Proper implementation of filtered search with new signature

The filtered search now correctly uses the new unified parameter object pattern, properly including both the query string and filter parameters in a single object.


38-42: Faceted search correctly updated to new API format

This faceted search correctly implements the new parameter structure, using an object with q, filter (as an array), and facets properties. This is consistent with the API signature changes described in the PR objectives.

tests/client.test.ts (2)

41-45: Added test setup for experimental network feature

Good addition of a beforeAll hook to enable the experimental network feature. This supports testing the new network-related functionality mentioned in the PR objectives.


47-51: Proper cleanup of experimental settings after tests

The afterAll hook correctly disables the experimental feature before cleaning up indexes, ensuring tests don't leave side effects that could impact other test suites.

src/types/index.ts (1)

1-6: Good organization of expanded type exports

These new exports provide a well-structured approach to exposing the refactored and new TypeScript types mentioned in the PR objectives. This ensures that all the new search-related types, experimental features, and network types are properly accessible throughout the codebase.

src/utils.ts (2)

19-35: Nice utility function implementation!

This generic utility function for stringifying specific keys in a record is well-typed and elegantly implemented. The type parameters ensure type safety while the mapped return type accurately preserves the original types for untouched keys and correctly annotates stringified values as strings.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 24-25: src/utils.ts#L24-L25
Added lines #L24 - L25 were not covered by tests


37-42: Export organization is clean and clear

The exports are neatly organized, making it clear what utilities are available from this module.

src/types/network.ts (2)

1-9: Well-documented Remote type

The type definition includes helpful documentation links to the official API reference and corresponding Rust implementation. The structure with an optional API key accommodates different authentication scenarios.


11-19: Network type looks good

The Network type is well-structured to model both self-reference and remote connections. The documentation is clear and links to the appropriate references.

playgrounds/javascript/src/meilisearch.ts (3)

1-4: Import updated correctly to include the new type

The import has been properly updated to include the SearchQueryWithOffsetLimit type, which aligns with the PR objective to refactor the search method signatures.


42-47: Search method signature updated appropriately

The implementation now correctly uses the new search method signature with a single parameter object containing q and other properties, as described in the PR objectives.


49-51: Console output format updated to reflect parameter changes

The string template has been appropriately updated to display the new search parameter object structure.

.code-samples.meilisearch.yaml (7)

17-20: Search parameter format updated correctly

The date guide filter example has been updated to use the new search method signature with a single object parameter containing q and filter properties.


24-27: Sort example properly converted

The date guide sort example now correctly uses the object format with q and sort properties.


83-85: Basic search examples correctly updated

Both the POST and GET search examples have been properly updated to use the new object format with the q property.


307-310: Filter examples consistently updated

All filter examples now correctly use the object parameter format with q and filter properties, maintaining consistency across the codebase.

Also applies to: 312-315, 317-320


462-462: Getting started example correctly updated

The getting started search example correctly demonstrates the new API format.


771-774: Advanced parameters example updated properly

The example for showRankingScoreDetails has been correctly updated to the new parameter format.


789-795: Hybrid search example is consistent with new format

The hybrid search example follows the new parameter structure correctly, maintaining consistency with other examples.

src/types/experimental-features.ts (1)

1-14: Well-structured type definition for experimental features.

The RuntimeTogglableFeatures type is well-documented with references to API docs and backend structure. The optional boolean/null properties provide flexibility for feature toggling.

tests/env/typescript-node/src/index.ts (4)

27-27: Proper use of generic type parameter.

Good use of generics to type the index with the Movie interface, providing type safety for search operations and hits processing.


34-36: Search signature change correctly implemented.

The code correctly uses the new search API that accepts a single parameter object with q property for the query string, aligning with the PR objectives to unify search method signatures.


41-43: Correctly using updated search method.

The search implementation follows the new pattern documented in the PR objectives, using a single parameter object instead of separate query and options parameters.


45-53: Proper type handling for hits.

The code correctly handles optional properties in search hits using the nullable chain operator, improving type safety. This is a good practice when working with search results.

tests/utils/meilisearch-test-utils.ts (5)

3-3: Good addition of Task type import.

Importing the Task type provides better type safety for the new isTaskSuccessful helper function.


96-96: Added complementary RESOLVED symbol.

Adding the RESOLVED symbol complements the existing NOT_RESOLVED symbol, creating a consistent pattern for assertion messages.


131-137: Useful assertion helper for promises.

The resolves assertion helper provides a clean way to assert that promises resolve without rejection, complementing the existing rejects helper for comprehensive promise testing.


138-141: Task validation helper improves test readability.

The isTaskSuccessful assertion encapsulates common task success validation logic, making tests more concise and readable while enforcing consistent validation.


259-261: Type-safe object keys extraction utility.

The objectKeys function provides a type-safe way to extract keys from objects with string keys, enhancing type safety in testing code.

src/types/shared.ts (5)

1-2: Direct type definition improves maintainability.

Defining RecordAny directly instead of importing it simplifies dependency management and makes the type more accessible.


4-10: Useful utility types for refining object properties.

The NonNullKeys and RequiredKeys utility types provide powerful tools for transforming object types in a type-safe way, especially useful for API parameter validation.


20-21: String case conversion utility with attribution.

The PascalToCamelCase utility type with attribution to its source is a good practice. This utility will be helpful for converting between naming conventions in APIs.


23-27: Recursive type mapping for deep objects.

The DeepStringRecord type provides a robust way to recursively convert all properties in nested objects to strings, which is useful for serialization and display purposes.


29-29: Type alias enhances code maintainability.

The SafeOmit type alias provides semantic clarity and future-proofing. While currently a direct alias for Omit, it establishes a pattern that could be extended with additional safety checks if needed.

src/meilisearch.ts (1)

426-441: Add unit-tests for the new experimental-features endpoints

Code-coverage shows 0 % for the freshly added getExperimentalFeatures & updateExperimentalFeatures paths.
A simple happy-path test (mocking the underlying HttpRequests) would keep these endpoints from going stale or accidentally breaking.

🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 428-431: src/meilisearch.ts#L428-L431
Added lines #L428 - L431 were not covered by tests

src/types/types.ts (2)

15-18: Potential regression – URLSearchParamsRecord can no longer accept Date instances

Date was removed from the union. A lot of user-land code (including our own integration tests for tasks/jobs) relies on passing Date objects directly to be subsequently stringified via URLSearchParams.
Please double-check usages across the repo – if any still expect to pass a Date, they will now fail to compile.

-  string | string[] | number | number[] | boolean | null | undefined
+  | string
+  | string[]
+  | number
+  | number[]
+  | boolean
+  | Date            // keep backward-compat
+  | null
+  | undefined

183-185: Breaking change – DocumentsDeletionQuery.filter is now mandatory

filter moved from optional to required. This is logically correct (the endpoint refuses an empty body), but it is a breaking change for TypeScript users that previously called:

index.deleteDocuments({});

If we intend to keep the stricter typing, flag it prominently in the release notes; otherwise make it optional and let runtime validation handle the error.

README.md (5)

202-202: LGTM: Updated search example with new API signature

The search example now correctly uses the object parameter syntax with the q property instead of passing the query string directly, aligning with the PR's API changes.


230-233: LGTM: Search parameters example updated correctly

The example now properly demonstrates how to use the new search API with a single parameter object containing both the query and additional search parameters.


276-279: LGTM: Filter example updated with new API signature

The filter example correctly uses the new search API pattern with a single parameter object.


304-308: LGTM: Placeholder search example updated properly

The placeholder search example now correctly uses the new object parameter syntax while maintaining all the necessary properties.


350-350: LGTM: Abortable search example updated correctly

The abortable search example now uses the object parameter style for the query while keeping the request config as the second parameter.

src/types/search-parameters.ts (14)

1-8: LGTM: Proper imports for utility types

The file correctly imports utility types from shared.js and types.js that are used to build the comprehensive type system for search parameters.


9-14: LGTM: Well-documented HybridQuery type

The HybridQuery type is properly defined with the necessary properties and includes a reference to the corresponding Rust type in the Meilisearch backend.


15-21: LGTM: MatchingStrategy type with proper transformation

The MatchingStrategy type correctly uses PascalToCamelCase to transform the Pascal case enum values from the backend to camel case for JavaScript, ensuring consistent typing while matching the expected API format.


22-37: LGTM: FederationOptions type with experimental flag

The FederationOptions type is well-documented with links to the API documentation and backend type. The experimental remote property is properly marked with a comment, and the undocumented queryPosition property includes a private remark.


38-51: LGTM: Clear separation of pagination approaches

The code correctly separates the two pagination approaches (offset/limit and page-based) into distinct types, each with proper documentation links to the API reference.


52-59: LGTM: Flexible FilterExpression type

The FilterExpression type properly handles the various formats that filters can be expressed in, with appropriate documentation links and private remarks about how it's handled in the backend.


61-117: LGTM: Well-structured search query segments

The search query components are logically divided into segments that can be composed together, with comprehensive documentation links for each property. This approach makes the types more maintainable and easier to understand.


120-125: LGTM: Elegant type transformation for GET requests

The IntoGet type transformation elegantly handles the differences between POST and GET request formats, particularly for nested hybrid search parameters that need to be flattened for URL parameters.


126-154: LGTM: Comprehensive search query type combinations

These types provide a complete set of options for different pagination approaches and request methods, enabling strongly-typed API interactions while preserving flexibility.


139-145: LGTM: Well-documented required pagination constraint

The enforcement of required pagination is well-documented with a clear explanation of why this constraint exists, relating it to the backend implementation and response shape guarantee.


156-171: LGTM: Search query types with index integration

These types correctly extend the base search query types to include index UID, enabling multi-search functionality while preserving type safety.


173-202: LGTM: Well-structured federation types

The federation-related types are well-structured and properly documented, providing a comprehensive type system for multi-search and federated search capabilities.


204-214: LGTM: FacetSearchQuery with proper composition

The FacetSearchQuery type correctly composes with the shared facet and search query segment, ensuring consistency across different query types.


215-226: LGTM: SimilarQuery with enforced offset and limit

The SimilarQuery type properly composes multiple segments and uses NonNullKeys to enforce the presence of offset and limit properties, ensuring the API works correctly with this query type.

Comment on lines +24 to +25
return;
}
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Add test coverage for the undefined case

The static analysis tool indicates that the undefined handling branch is not covered by tests.

Consider adding a test case that specifically verifies the behavior when undefined is passed:

test('stringifyRecordKeyValues handles undefined input', () => {
  const result = stringifyRecordKeyValues(undefined, ['filter']);
  expect(result).toBeUndefined();
});

🏁 Script executed:

#!/bin/bash
# Check if there are any tests for this utility function
rg "stringifyRecordKeyValues" --type=ts -A 5 -B 5

Length of output: 2078


Add missing test for the undefined branch in stringifyRecordKeyValues

The if (v === undefined) return; path in src/utils.ts isn’t covered by any existing tests. Please add a test to verify that passing undefined yields undefined.

• Location: src/utils.ts (undefined‐handling branch around lines 24–25)
• Suggested test (e.g. in src/utils.test.ts or your repo’s test suite):

import { stringifyRecordKeyValues } from './utils';

test('stringifyRecordKeyValues handles undefined input', () => {
  expect(stringifyRecordKeyValues(undefined, ['filter'])).toBeUndefined();
});
🧰 Tools
🪛 GitHub Check: codecov/patch

[warning] 24-25: src/utils.ts#L24-L25
Added lines #L24 - L25 were not covered by tests

Comment on lines +224 to +229
): Promise<SearchResults>;
multiSearch(
federatedSearch: FederatedSearch,
init?: ExtraRequestInit,
): Promise<FederatedSearchResult>;
async multiSearch(
Copy link

@coderabbitai coderabbitai bot May 14, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Parameter should be partial for update operations

updateNetwork() right above correctly accepts a Partial<Network>.
updateExperimentalFeatures() should mirror this behaviour – the server accepts a sparse payload with only the toggles you want to touch. Requiring the full RuntimeTogglableFeatures object forces callers to redundantly send the whole structure.

-  async updateExperimentalFeatures(
-    runtimeTogglableFeatures: RuntimeTogglableFeatures,
+  async updateExperimentalFeatures(
+    runtimeTogglableFeatures: Partial<RuntimeTogglableFeatures>,
   ): Promise<RuntimeTogglableFeatures> {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
): Promise<SearchResults>;
multiSearch(
federatedSearch: FederatedSearch,
init?: ExtraRequestInit,
): Promise<FederatedSearchResult>;
async multiSearch(
async updateExperimentalFeatures(
runtimeTogglableFeatures: Partial<RuntimeTogglableFeatures>,
): Promise<RuntimeTogglableFeatures> {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is outside of the scope of this PR at the moment.

Copy link

Choose a reason for hiding this comment

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

@flevi29 Understood! It makes sense to keep this PR focused on its primary objectives. Thanks for considering the suggestion.

Comment on lines 80 to 99
name: index.searchSimilarDocuments.name,
searchMethod: async ({
offset,
limit,
...searchQuery
}: SearchQueryWithOffsetLimit) => {
const { hits, ...result } = await index.searchSimilarDocuments({
id: 607,
embedder: "default",
offset: offset ?? undefined,
limit: limit ?? undefined,
...searchQuery,
});

return {
...result,
query: searchQuery.q ?? "",
hits: hits as SearchHit<Film>[],
};
},
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Avoid sending unsupported properties to searchSimilarDocuments

SimilarQuery intentionally has a much smaller surface than a full SearchQuery; spreading searchQuery here risks forwarding keys such as hybrid, vector, attributesToSearchOn, etc., that the endpoint neither documents nor accepts. When such keys are present the request will still compile (excess property checks are bypassed on spreads) but the HTTP call will silently drop them, making the test utility lie about what is really under test.

Consider whitelisting only the properties that are part of SimilarQuery:

-      const { hits, ...result } = await index.searchSimilarDocuments({
-        id: 607,
-        embedder: "default",
-        offset: offset ?? undefined,
-        limit: limit ?? undefined,
-        ...searchQuery,
-      });
+      const { hits, ...result } = await index.searchSimilarDocuments({
+        id: 607,
+        embedder: "default",
+        offset: offset ?? undefined,
+        limit: limit ?? undefined,
+        filter: searchQuery.filter ?? undefined,
+        rankingScoreThreshold: searchQuery.rankingScoreThreshold ?? undefined,
+        // add further whitelisted props when the API starts supporting them
+      } satisfies SimilarQuery);

This keeps the helper faithful to the public API surface and prevents accidental usage drift when new fields appear in SearchQuery.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
name: index.searchSimilarDocuments.name,
searchMethod: async ({
offset,
limit,
...searchQuery
}: SearchQueryWithOffsetLimit) => {
const { hits, ...result } = await index.searchSimilarDocuments({
id: 607,
embedder: "default",
offset: offset ?? undefined,
limit: limit ?? undefined,
...searchQuery,
});
return {
...result,
query: searchQuery.q ?? "",
hits: hits as SearchHit<Film>[],
};
},
name: index.searchSimilarDocuments.name,
searchMethod: async ({
offset,
limit,
...searchQuery
}: SearchQueryWithOffsetLimit) => {
const { hits, ...result } = await index.searchSimilarDocuments({
id: 607,
embedder: "default",
offset: offset ?? undefined,
limit: limit ?? undefined,
filter: searchQuery.filter ?? undefined,
rankingScoreThreshold: searchQuery.rankingScoreThreshold ?? undefined,
// add further whitelisted props when the API starts supporting them
} satisfies SimilarQuery);
return {
...result,
query: searchQuery.q ?? "",
hits: hits as SearchHit<Film>[],
};
},

Comment on lines +158 to +161
export type SimilarResult = {
hits: SearchHit[];
id: string;
} & ProcessingTime &
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

SimilarResult.id should allow number as well

SimilarQuery.id is declared as string | number, but SimilarResult
narrows the type down to string. When callers pass a numeric id the
response will still contain that number, forcing them to cast or as unknown as. Widen the type so the request/response cycle stays
consistent:

-  id: string;
+  id: string | number;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export type SimilarResult = {
hits: SearchHit[];
id: string;
} & ProcessingTime &
export type SimilarResult = {
hits: SearchHit[];
id: string | number;
} & ProcessingTime &

@@ -431,13 +425,13 @@ We welcome all contributions, big and small! If you want to know more about this
#### [Make a search request](https://www.meilisearch.com/docs/reference/api/search)

```ts
client.index<T>('xxx').search(query: string, options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
client.index<T>('xxx').search(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Type name in API reference needs to be updated

The type SearchParams is referenced in the API documentation, but according to the PR description, this type has been renamed to SearchQuery.

-client.index<T>('xxx').search(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
+client.index<T>('xxx').search(options: SearchQuery = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
client.index<T>('xxx').search(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
client.index<T>('xxx').search(options: SearchQuery = {}, config?: Partial<Request>): Promise<SearchResponse<T>>

```

#### [Make a search request using the GET method (slower than the search method)](https://www.meilisearch.com/docs/reference/api/search#search-in-an-index-with-get-route)

```ts
client.index<T>('xxx').searchGet(query: string, options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
client.index<T>('xxx').searchGet(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Type name in API reference needs to be updated

The type SearchParams is referenced in the API documentation for searchGet, but according to the PR description, this type has been renamed to SearchQuery.

-client.index<T>('xxx').searchGet(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
+client.index<T>('xxx').searchGet(options: SearchQueryGet = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
client.index<T>('xxx').searchGet(options: SearchParams = {}, config?: Partial<Request>): Promise<SearchResponse<T>>
client.index<T>('xxx').searchGet(options: SearchQueryGet = {}, config?: Partial<Request>): Promise<SearchResponse<T>>

@flevi29 flevi29 mentioned this pull request May 19, 2025
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (1)
tests/utils/search.ts (1)

80-101: Avoid sending unsupported properties to searchSimilarDocuments

SimilarQuery intentionally has a more limited set of properties than a full SearchQuery. Spreading searchQuery on line 92 risks forwarding keys that the endpoint doesn't support, which could lead to silently dropped parameters.

Consider explicitly including only the supported properties:

-      const { hits, ...result } = await index.searchSimilarDocuments({
-        id: 607,
-        embedder: "default",
-        offset: offset ?? undefined,
-        limit: limit ?? undefined,
-        ...searchQuery,
-      });
+      const { hits, ...result } = await index.searchSimilarDocuments({
+        id: 607,
+        embedder: "default",
+        offset: offset ?? undefined,
+        limit: limit ?? undefined,
+        filter: searchQuery.filter ?? undefined,
+        rankingScoreThreshold: searchQuery.rankingScoreThreshold ?? undefined,
+        // add further whitelisted props when the API supports them
+      });

This makes the helper more accurate to the actual API surface and prevents accidental usage of unsupported properties.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 3c9c438 and 8262774.

📒 Files selected for processing (1)
  • tests/utils/search.ts (1 hunks)
🔇 Additional comments (5)
tests/utils/search.ts (5)

1-16: Good job setting up a clean test environment

The imports look appropriately organized and the test setup with a random UUID for the index UID provides good test isolation.


25-41: Clean implementation of search wrappers

The handling of the hybrid property in the searchGet wrapper is well implemented, correctly transforming it into hybridSemanticRatio and hybridEmbedder parameters.


42-79: Well-typed search implementations

Good job using TypeScript's type system to ensure type safety in both the multiSearch and federatedMultiSearch implementations. The satisfies operator ensures proper type checking while maintaining inference capabilities.


124-127: Good type assertion for the array

The use of as const satisfies ensures both literal type preservation and type compatibility, which is excellent for maintaining type safety in tests.


129-147: Well-structured assertion function

The assertFacetDistributionAndStatsAreCorrect function is well-implemented, providing thorough validation of the facet distribution and stats structures.

Comment on lines 102 to 123
{
name: index.searchSimilarDocumentsGet.name,
searchMethod: async ({
offset,
limit,
...searchQuery
}: SearchQueryWithOffsetLimit) => {
const { hits, ...result } = await index.searchSimilarDocumentsGet({
id: 607,
embedder: "default",
offset: offset ?? undefined,
limit: limit ?? undefined,
...searchQuery,
});

return {
...result,
query: searchQuery.q ?? "",
hits: hits as SearchHit<Film>[],
};
},
},
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Apply the same fix to searchSimilarDocumentsGet

The same issue applies here - spreading searchQuery on line 114 could include unsupported properties.

Apply a similar fix:

-      const { hits, ...result } = await index.searchSimilarDocumentsGet({
-        id: 607,
-        embedder: "default",
-        offset: offset ?? undefined,
-        limit: limit ?? undefined,
-        ...searchQuery,
-      });
+      const { hits, ...result } = await index.searchSimilarDocumentsGet({
+        id: 607,
+        embedder: "default",
+        offset: offset ?? undefined,
+        limit: limit ?? undefined,
+        filter: searchQuery.filter ?? undefined,
+        rankingScoreThreshold: searchQuery.rankingScoreThreshold ?? undefined,
+        // add further whitelisted props when the API supports them
+      });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{
name: index.searchSimilarDocumentsGet.name,
searchMethod: async ({
offset,
limit,
...searchQuery
}: SearchQueryWithOffsetLimit) => {
const { hits, ...result } = await index.searchSimilarDocumentsGet({
id: 607,
embedder: "default",
offset: offset ?? undefined,
limit: limit ?? undefined,
...searchQuery,
});
return {
...result,
query: searchQuery.q ?? "",
hits: hits as SearchHit<Film>[],
};
},
},
{
name: index.searchSimilarDocumentsGet.name,
searchMethod: async ({
offset,
limit,
...searchQuery
}: SearchQueryWithOffsetLimit) => {
const { hits, ...result } = await index.searchSimilarDocumentsGet({
id: 607,
embedder: "default",
offset: offset ?? undefined,
limit: limit ?? undefined,
filter: searchQuery.filter ?? undefined,
rankingScoreThreshold: searchQuery.rankingScoreThreshold ?? undefined,
// add further whitelisted props when the API supports them
});
return {
...result,
query: searchQuery.q ?? "",
hits: hits as SearchHit<Film>[],
};
},
},
🤖 Prompt for AI Agents
In tests/utils/search.ts between lines 102 and 123, the
searchSimilarDocumentsGet method spreads the entire searchQuery object, which
may include unsupported properties. To fix this, explicitly pick only the
supported properties from searchQuery before spreading them into the
searchSimilarDocumentsGet call, similar to the fix applied elsewhere. This
ensures only valid parameters are passed to the search method.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (5)
tests/search.test.ts (5)

256-257: Consider implementing the TODO for more thorough testing of ranking score details.

The current test only verifies that _rankingScoreDetails is an object, but doesn't validate its structure or contents. This could be enhanced with more specific assertions about the expected properties and their types.

-      // TODO: This could be more thoroughly tested and typed
-      assert.typeOf(_rankingScoreDetails, "object");
+      assert.typeOf(_rankingScoreDetails, "object");
+      // Verify the expected structure of ranking score details
+      assert.hasAllKeys(_rankingScoreDetails, ["words", "typo", "proximity", "attribute", "sort", "exactness"]);
+      for (const key in _rankingScoreDetails) {
+        assert.typeOf(_rankingScoreDetails[key], "number", `${key} should be a number`);
+      }

450-451: Improve type safety with a proper type guard instead of type assertion.

The current code uses a type assertion which bypasses TypeScript's type checking. Consider using a type guard function to ensure type safety.

-  const {
-    default: { embeddings, ...restOfObj },
-  } = vectors as Record<string, ExplicitVectors>;
+  // Type guard to ensure vectors has the expected structure
+  function isExplicitVectorsRecord(obj: unknown): obj is Record<string, ExplicitVectors> {
+    return obj !== null && typeof obj === 'object' && 'default' in obj && 
+           typeof obj.default === 'object' && 'embeddings' in obj.default;
+  }
+  
+  assert.isTrue(isExplicitVectorsRecord(vectors), 'vectors does not have the expected structure');
+  
+  const {
+    default: { embeddings, ...restOfObj },
+  } = vectors;

96-99: Use deepEqual instead of sameMembers for array comparison.

When testing the exact order of array elements matters for search results, sameMembers only checks that the arrays contain the same elements regardless of order. If order is important for search results, consider using deepEqual instead.

-      assert.sameMembers(
+      assert.deepEqual(
        hits.map((v) => v.id),
        [95, 348, 607, 608],
-      );
+      );

If order doesn't matter, add a comment explaining this to make the intent clear:

+      // Order of search results doesn't matter, only that all expected IDs are present
       assert.sameMembers(
         hits.map((v) => v.id),
         [95, 348, 607, 608],
       );

33-68: Refactor test setup for better maintainability.

The setup is quite lengthy and could be made more maintainable by extracting some logic into helper functions. This would make the test file cleaner and easier to maintain.

Consider moving the index configuration and document loading to a helper function in the utils directory:

// In utils/search.js
export async function setupTestIndex() {
  await client.updateExperimentalFeatures({ network: true });
  await client.updateNetwork({
    self: INDEX_UID,
    remotes: { [INDEX_UID]: { url: HOST, searchApiKey: MASTER_KEY } },
  });

  await index
    .updateSettings({
      filterableAttributes: [
        "id",
        "genres",
        "popularity",
        "release_date",
        "_geo",
      ],
      sortableAttributes: ["popularity"],
      embedders: {
        default: {
          source: "userProvided",
          dimensions: 1,
        },
      },
    })
    .waitTask()
    .then((task) => {
      assert.isTaskSuccessful(task);
    });

  await index
    .addDocuments(FILMS)
    .waitTask()
    .then((task) => {
      assert.isTaskSuccessful(task);
    });
}

// Then in the test file:
beforeAll(async () => {
  await setupTestIndex();
});

172-188: Improve test robustness by validating all possible expected values.

The current test only checks for inclusion of expected values in the search results, but doesn't validate all possible values that could be returned. This could allow unexpected values to pass the test.

-        assert.includeDeepMembers(
-          [
-            { overview: "♥☻Earth☺♥", popularity: "34.945" },
-            { overview: "♥☻earth☺♥", popularity: "41.947" },
-          ],
-          [{ overview, popularity }],
-        );
+        const expectedValues = [
+          { overview: "♥☻Earth☺♥", popularity: "34.945" },
+          { overview: "♥☻earth☺♥", popularity: "41.947" },
+        ];
+        const found = expectedValues.some(expected => 
+          expected.overview === overview && expected.popularity === popularity
+        );
+        assert.isTrue(found, `Unexpected formatted values: ${JSON.stringify({ overview, popularity })}`);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 8262774 and 371289d.

📒 Files selected for processing (2)
  • tests/search.test.ts (1 hunks)
  • tests/utils/search.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • tests/utils/search.ts
🔇 Additional comments (4)
tests/search.test.ts (4)

75-84: Great test organization using parameterized tests!

The use of test.concurrent.for to test multiple search methods with the same parameters is an excellent approach that reduces code duplication and ensures consistent testing across methods.


301-305: Good use of type-safe object keys extraction.

Using objectKeys<MatchingStrategy> is a smart way to ensure type safety when working with the possible matching strategies. This approach guarantees that the test will be updated if the available matching strategies change.


399-445: Well-structured federation test.

The federation test is comprehensive and well-structured, testing multiple aspects of the federation feature including weights, facet distribution, and error handling. The assertions are thorough and validate the expected response structure.


447-461: Well-implemented vector validation function.

The assertIsVectors function thoroughly validates the structure and content of vector data, ensuring proper typing and expected fields. This is a good example of modular test code that can be reused across test cases.

Comment on lines +435 to +436
// TODO: Maybe could get an error response for this, to validate it against
assert.deepEqual(remoteErrors, {});
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Address the TODO for testing error handling in remote responses.

The test currently assumes there are no remote errors but doesn't validate the error handling functionality.


🏁 Script executed:

#!/bin/bash
# Check for any error handling implementations in the codebase
rg -A 5 "remoteErrors" --type ts --type js

Length of output: 978


#!/bin/bash
# Display lines 420–460 of tests/search.test.ts for context
file=$(fd search.test.ts)
sed -n '420,460p' "$file"

Add tests for error handling in multiSearch responses

The existing TODO indicates we need to verify that remoteErrors is populated when one or more queries fail. Please update the test in tests/search.test.ts (around lines 435–436) to include a failing scenario:

  • Simulate or mock an error in one of the federated searches (e.g. by using an invalid indexUid or intercepting the HTTP call).
  • Call client.multiSearch({ queries, federation }) so that one query succeeds and one fails.
  • Assert that remoteErrors contains the expected error entries, for example:
    // After invoking multiSearch with one invalid query
    assert.ok(remoteErrors["invalidIndex"], "Expected an error for ‘invalidIndex’");
    assert.strictEqual(remoteErrors["invalidIndex"].code, "index_not_found");
    assert.match(
      remoteErrors["invalidIndex"].message,
      /Index .* not found/,
      "Error message should mention missing index"
    );
  • Ensure that valid hits and facets are still returned for the successful queries.

This will confirm the client correctly surfaces remote errors while returning available results.

🤖 Prompt for AI Agents
In tests/search.test.ts around lines 435 to 436, the test currently only asserts
that remoteErrors is empty and does not verify error handling. Update the test
to simulate a failing federated search by mocking or using an invalid indexUid
in one of the queries passed to client.multiSearch. Then assert that
remoteErrors contains the expected error object for the failing query, checking
the error code and message. Also verify that valid hits and facets are still
returned for successful queries. This will properly test that remote errors are
surfaced while results are returned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
1 participant