Improve Settings
#1864
Improve Settings
#1864
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1864 +/- ##
==========================================
- Coverage 98.83% 98.10% -0.74%
==========================================
Files 18 19 +1
Lines 1549 1266 -283
Branches 334 280 -54
==========================================
- Hits 1531 1242 -289
- Misses 18 24 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
flevi29
added
breaking-change
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and improves the handling of Settings by separating and documenting settings types, centralizing settings update methods under a new "setting" property on the Index class, and significantly refactoring the tests related to settings. Key changes include:
- Moving and redefining Settings types into a dedicated settings module.
- Updating API calls to use the new client.index(...).setting.* method pattern.
- Removing or refactoring a number of tests to align with the new settings updates and experimental features support.
Reviewed Changes
Copilot reviewed 46 out of 46 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/*.test.ts | Updates to test files to use new settings methods and removal of redundant tests |
| src/types/*.ts | Refactoring of settings types and removal of duplicated definitions |
| src/settings.ts, src/meilisearch.ts | Introduction of new settings functions and experimental features endpoints |
| README.md, .code-samples.meilisearch.yaml | Documentation updates to reflect new settings API usage |
Comments suppressed due to low confidence (3)
tests/faceting.test.ts:1
- Several test files (e.g. faceting, filterable_attributes, facet_search_settings, distinct_attribute, displayed_attributes, dictionary) have been removed. Please ensure that the new or remaining tests fully cover all the settings functionalities previously verified to avoid gaps in test coverage.
Entire file removed
src/meilisearch.ts:459
- The new getExperimentalFeatures and updateExperimentalFeatures endpoints have been added using the 'patch' method for updates. Verify that these endpoints are fully documented, have clear versioning implications, and that there are adequate tests for their behavior.
New experimental features endpoints added
README.md:262
- Make sure that all usage examples and code samples in the documentation and YAML files accurately reflect the new settings API changes and provide guidance for users during migration.
Documentation examples updated to use client.index('myIndex').setting.*
WalkthroughAdds a typed settings facade accessible as Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Client as MeiliSearch Client
participant Index as Index
participant Settings as Index.settings (SettingFns)
participant HTTP as HTTP Layer
Dev->>Client: client.index("movies")
Client->>Index: resolve Index("movies")
Note over Index: constructor calls makeSettingFns(...) → #settings
Dev->>Index: settings.updateFilterableAttributes([...])
Index->>Settings: delegate updateFilterableAttributes(...)
Settings->>HTTP: PUT /indexes/:uid/settings/filterable-attributes (body)
HTTP-->>Settings: EnqueuedTask
Settings-->>Dev: EnqueuedTask response
Dev->>Index: settings.getFilterableAttributes()
Settings->>HTTP: GET /indexes/:uid/settings/filterable-attributes
HTTP-->>Settings: value
Settings-->>Dev: value
Dev->>Index: settings.resetFilterableAttributes()
Settings->>HTTP: DELETE /indexes/:uid/settings/filterable-attributes
HTTP-->>Settings: EnqueuedTask
Settings-->>Dev: EnqueuedTask response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
.code-samples.meilisearch.yaml (1)
6-10: Documentation samples now rely on the newsettingproperty – double-check the promise handling.All updated snippets still use the fire-and-forget style (
client.index(...).setting.updateXxx(...)) without awaiting the returnedEnqueuedTaskPromise.
While brevity is fine for docs, omitting eitherawaitor.then()can mislead users who need the taskUid for subsequent polling or error handling.Consider adding a short comment or one minimal example showing the idiomatic pattern:
const task = await client.index('movies') .setting.updateSynonyms({ ... }); await task.waitTask(); // <- most common real-world needThis helps newcomers understand that a task object is produced, not an immediate success result.
(No change required in every snippet—one prominent example in the “settings” section is enough.)Also applies to: 14-16, 20-24
src/settings.ts (2)
31-33:camelToKebabCasemisses consecutive upper-case segments.Example:
"URLPath"➜"u-r-lpath"instead of"url-path".
A simple fix:-return str.replace(/[A-Z]/g, (letter) => `-${letter.toLowerCase()}`); +return str + .replace(/([a-z0-9])([A-Z])/g, "$1-$2") // word boundary + .toLowerCase();Not blocking, but improves future-proofing.
48-63: Minor optimisation & readability
return await httpRequest.get({ path });→ justreturn httpRequest.get({ path });
(awaitin areturnadds an unnecessary micro-task).Using
for..of Object.entriesalready provides the strongly-typed key (name: string),
but ifoptsis frozen the loop could instead iterate overObject.keys(opts)
to avoid constructing entry tuples. Marginal win, non-blocking.tests/settings.test.ts (3)
269-273:deleteinside tight test loops – replace withundefinedto satisfy linter and avoid shape-deopt.The Biome linter flags the
delete output.sortFacetValuesBy["*"];pattern.
In hot code this can de-optimise objects; in tests the perf hit is negligible, but you can appease the linter and keep intent clear:-const star = output.sortFacetValuesBy["*"]; -delete output.sortFacetValuesBy["*"]; +const star = output.sortFacetValuesBy["*"]; +output.sortFacetValuesBy["*"] = undefined;(or clone the object before mutation).
🧰 Tools
🪛 Biome (1.9.4)
[error] 271-272: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
503-507: Reassigning the loop variable shadows immutable data – prefer a new name.
mappedSetting = mappedSetting.map(...)mutates the parameter captured from
describe.for. While legal (function parameters are mutable), it can confuse
readers becausemappedSettingwas perceived as an input, not a working copy.- mappedSetting = mappedSetting.map(...) + const mappedSettingWithLabel = mappedSetting.map(...)This improves clarity and prevents accidental reuse of the transformed value
elsewhere in the block.
510-521: Race-condition risk: relying onwaitTask({ timeout: 30_000 })in serial loops slows CI.Each individual setting test waits up to 30 s. With ~40 settings this can add 20 min worst-case.
If the Meilisearch instance supports batch updates, consider:
- Parallelising the per-setting tasks with
Promise.allSettled.- Lowering timeout via server-side per-cluster config.
Not a blocker, but may considerably speed up CI.
src/indexes.ts (1)
574-583: Expose thesettingfacade asreadonlyat type levelThe getter returns a fresh reference each call; consumers could still re-assign it (
index.setting = …) unless TS is told it’s immutable.- get setting() { + get setting(): Readonly<SettingFns> { return this.#setting; }Small addition but it prevents accidental mutation and documents intent.
src/types/settings.ts (1)
133-139:EmbeddingSettingsdouble-inherits fromSubEmbeddingSettings
EmbeddingSettingsalready containssearchEmbedderandindexingEmbedder
fields typed asSubEmbeddingSettings, but the final intersection
& SubEmbeddingSettingsmerges the entire parent again, creating duplicate
properties and making some keys (source,model, …) ambiguous in the
resulting type.Unless a “root-level shortcut” is explicitly required, consider removing the
intersection to avoid confusion:-export type EmbeddingSettings = PartialAndNullable<{ … }> & SubEmbeddingSettings; +export type EmbeddingSettings = PartialAndNullable<{ … }>;If both levels are intentional, a code comment explaining why will help future
maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (1)
tests/__snapshots__/settings.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (43)
.code-samples.meilisearch.yaml(17 hunks)README.md(3 hunks)src/indexes.ts(4 hunks)src/settings.ts(1 hunks)src/types/index.ts(1 hunks)src/types/settings.ts(1 hunks)src/types/shared.ts(1 hunks)src/types/task_and_batch.ts(2 hunks)src/types/types.ts(0 hunks)tests/dictionary.test.ts(0 hunks)tests/displayed_attributes.test.ts(0 hunks)tests/distinct_attribute.test.ts(0 hunks)tests/documents.test.ts(4 hunks)tests/dump.test.ts(1 hunks)tests/embedders.test.ts(0 hunks)tests/env/node/getting_started.cjs(1 hunks)tests/facet_search_settings.test.ts(0 hunks)tests/faceting.test.ts(0 hunks)tests/filterable_attributes.test.ts(0 hunks)tests/get_search.test.ts(1 hunks)tests/index.test.ts(1 hunks)tests/keys.test.ts(1 hunks)tests/localized_attributes.test.ts(0 hunks)tests/non_separator_tokens.test.ts(0 hunks)tests/pagination.test.ts(0 hunks)tests/prefix_search_settings.test.ts(0 hunks)tests/proximity_precision.test.ts(0 hunks)tests/ranking_rules.test.ts(0 hunks)tests/raw_document.test.ts(1 hunks)tests/search.test.ts(3 hunks)tests/search_cutoff_ms.test.ts(0 hunks)tests/searchable_attributes.test.ts(0 hunks)tests/separator_tokens.test.ts(0 hunks)tests/settings.test.ts(1 hunks)tests/snapshots.test.ts(1 hunks)tests/sortable_attributes.test.ts(0 hunks)tests/stop_words.test.ts(0 hunks)tests/synonyms.test.ts(0 hunks)tests/task.test.ts(1 hunks)tests/token.test.ts(1 hunks)tests/typed_search.test.ts(2 hunks)tests/typo_tolerance.test.ts(0 hunks)tests/utils/meilisearch-test-utils.ts(3 hunks)
💤 Files with no reviewable changes (21)
- tests/separator_tokens.test.ts
- tests/non_separator_tokens.test.ts
- tests/localized_attributes.test.ts
- tests/search_cutoff_ms.test.ts
- tests/typo_tolerance.test.ts
- tests/faceting.test.ts
- tests/sortable_attributes.test.ts
- tests/displayed_attributes.test.ts
- tests/embedders.test.ts
- tests/pagination.test.ts
- tests/searchable_attributes.test.ts
- tests/prefix_search_settings.test.ts
- tests/filterable_attributes.test.ts
- tests/facet_search_settings.test.ts
- tests/proximity_precision.test.ts
- tests/synonyms.test.ts
- tests/dictionary.test.ts
- src/types/types.ts
- tests/ranking_rules.test.ts
- tests/distinct_attribute.test.ts
- tests/stop_words.test.ts
🧰 Additional context used
🧬 Code Graph Analysis (7)
src/types/task_and_batch.ts (1)
src/types/settings.ts (1)
UpdatableSettings(170-211)
tests/typed_search.test.ts (1)
src/meilisearch.ts (1)
index(86-88)
tests/utils/meilisearch-test-utils.ts (2)
src/types/task_and_batch.ts (1)
Task(122-131)src/http-requests.ts (1)
T(199-264)
tests/documents.test.ts (2)
tests/env/esm/src/index.js (1)
client(4-4)src/meilisearch.ts (1)
index(86-88)
src/settings.ts (5)
src/types/settings.ts (1)
SingleUpdatableSettings(217-217)src/types/task_and_batch.ts (1)
EnqueuedTaskPromise(137-143)src/http-requests.ts (1)
HttpRequests(140-290)src/task.ts (1)
HttpRequestsWithEnqueuedTaskPromise(192-196)src/types/types.ts (1)
RecordAny(10-10)
src/types/settings.ts (1)
src/types/shared.ts (1)
PascalToCamelCase(10-10)
src/indexes.ts (4)
src/settings.ts (2)
makeSettingFns(36-66)SettingFns(15-25)src/errors/meilisearch-error.ts (1)
MeiliSearchError(1-3)src/types/settings.ts (2)
Settings(224-224)UpdatableSettings(170-211)src/types/task_and_batch.ts (1)
EnqueuedTaskPromise(137-143)
🪛 Biome (1.9.4)
tests/settings.test.ts
[error] 271-272: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (46)
tests/keys.test.ts (1)
3-3: Consistent import path update
The import forErrorStatusCodehas been correctly updated from the old types entrypoint to the new consolidated../src/index.js. This aligns with the refactoring of type exports.tests/snapshots.test.ts (1)
2-2: Consistent import path update
TheErrorStatusCodeimport path has been updated to../src/index.js, matching the centralized exports. No test logic was affected.tests/dump.test.ts (1)
2-2: Consistent import path update
TheErrorStatusCodeimport was correctly changed to the unified../src/index.jsmodule. Test assertions remain valid.tests/task.test.ts (1)
2-2: Consistent import path update
The import forErrorStatusCodenow points to the rootindex.jsentrypoint, in line with the refactoring. All task tests continue to reference the correct error codes.tests/index.test.ts (1)
2-2: Consistent import path update
UpdatedErrorStatusCodeimport to the new../src/index.jsentrypoint. This keeps type imports in sync with the consolidated exports.tests/raw_document.test.ts (1)
7-7: Import path updated to reflect refactored type exports.The import path for
ContentTypeEnumhas been updated from"../src/types/index.js"to"../src/index.js"to align with the consolidated export structure. This change is part of the broader settings refactoring described in the PR objectives.tests/get_search.test.ts (1)
2-2: Import path updated to reflect refactored type exports.The import path for
ErrorStatusCodehas been updated from"../src/types/index.js"to"../src/index.js"to align with the consolidated type export structure. This change is consistent with similar updates across other files in the PR.tests/env/node/getting_started.cjs (1)
22-22: API usage updated to use new settings interface.This line changes the method call from
index.updateFilterableAttributestoindex.setting.updateFilterableAttributes, implementing the new API pattern where all settings methods are accessed via thesettingproperty. This change aligns with the PR objective to consolidate setting manipulation methods under a dedicated property.src/types/index.ts (1)
2-2: Added export for new settings types.This change exports all types from the settings module, making them available throughout the codebase. This is a crucial part of the refactoring described in the PR objectives, where setting types have been reorganized and improved.
tests/token.test.ts (1)
302-302: API pattern change is correctThe change to use
setting.updateFilterableAttributesinstead of directly callingupdateFilterableAttributesfollows the new API pattern, where all index settings methods are now accessed through the unifiedsettingproperty.tests/search.test.ts (3)
10-10: Import paths updated correctlyThe import paths for
ErrorStatusCode,MatchingStrategies, and related types have been consolidated to use the main entry point from"../src/index.js"instead of specific file paths, which is a cleaner approach.Also applies to: 14-14
1217-1219: API pattern change is correctThe change to use
setting.updateLocalizedAttributesinstead of directly callingupdateLocalizedAttributesaligns with the new API pattern, where all index settings methods are now accessed through the unifiedsettingproperty.
1425-1425: Consider addressing the TODO commentThere's a new TODO comment about an assertion error with promising not resolving. This might indicate a test issue that should be addressed.
Could you provide more details about this assertion error or check if this is related to the API changes in this PR?
src/types/task_and_batch.ts (1)
1-1: Type import and extension updated correctlyThe change from importing
SettingstoUpdatableSettingsand updating theTaskDetailstype to extendUpdatableSettingsinstead is aligned with the PR's objective to make settings types more generic and better match the Rust source code types.This is a breaking change that requires consumers to use
UpdatableSettingsinstead ofSettingswhen working with tasks, but it provides a more accurate type definition reflecting what fields can actually be updated.Also applies to: 100-100
tests/typed_search.test.ts (2)
9-9: Import paths updated correctlyThe import paths have been consolidated to use the main entry point from
"../src/index.js"instead of specific file paths, which improves maintainability.
121-123: API pattern change and type parameter update are correctTwo important changes have been made:
The generic type parameter
<Movie>has been removed from theindex()call, which aligns with the implementation insrc/meilisearch.tswhere theindex()method is generic.The method call now uses
.setting.updateFilterableAttributes()instead of calling it directly on the index object, following the new API pattern where all settings methods are accessed through the unifiedsettingproperty.These changes are part of the broader refactoring to improve the organization and clarity of the settings API.
tests/documents.test.ts (4)
143-146: API update for FilterableAttributes methodThe method for updating filterable attributes has been changed to use the new
settingproperty on the index, aligning with the PR's goal of consolidating all settings methods under a unified interface.
494-496: API update for FilterableAttributes methodConsistent with the earlier change, this code now uses
setting.updateFilterableAttributesinstead of directly calling the method on the index.
512-514: API update for FilterableAttributes methodAnother instance of the updated API pattern for setting filterable attributes through the
settingproperty.
688-688: API update for FilterableAttributes methodThis change completes the migration to the new settings API pattern within this test file.
src/types/shared.ts (1)
9-10: New type utility for Pascal-to-camel case conversionAdded a utility type that converts PascalCase to camelCase using TypeScript's built-in
Uncapitalizeutility. This supports the settings refactoring by providing proper type conversions when generating setting method names.The source attribution via StackOverflow comment is good practice for referencing external solutions.
tests/utils/meilisearch-test-utils.ts (4)
3-3: Added Task type importUpdated the import to include the
Tasktype, which is needed for the new task validation helper function.
130-133: Added utility for task status verificationNew helper method that validates if a task completed successfully by checking for null error and "succeeded" status. This simplifies testing the outcome of settings operations that return tasks.
251-253: Added type-safe object keys extraction utilityNew helper function that returns an array of keys from an object with proper typing, useful for working with settings in a type-safe manner.
This function ensures the returned keys array maintains the type information from the input object, allowing for better type checking when working with settings keys.
256-256: Added ObjectKeys to exportsMakes the new utility function available for use in other test files.
README.md (20)
265-268: Updated code example to use new settings APIExample for updating filterable attributes now uses the new
settingproperty, matching the implementation changes.
713-726: Updated pagination settings documentationDocumentation for pagination settings methods now reflects the new pattern of accessing these methods through the
settingproperty.
733-746: Updated synonyms settings documentationDocumentation for synonyms settings now reflects the new API pattern.
753-766: Updated stop words settings documentationDocumentation for stop words settings now uses the new
settingproperty pattern.
773-786: Updated ranking rules settings documentationDocumentation for ranking rules settings now uses the new
settingproperty pattern.
793-806: Updated distinct attribute settings documentationDocumentation for distinct attribute settings now reflects the new API pattern.
813-826: Updated searchable attributes settings documentationDocumentation for searchable attributes settings now uses the new
settingproperty pattern.
833-846: Updated displayed attributes settings documentationDocumentation for displayed attributes settings now reflects the new API pattern.
853-866: Updated filterable attributes settings documentationDocumentation for filterable attributes settings now uses the new
settingproperty pattern.
873-886: Updated sortable attributes settings documentationDocumentation for sortable attributes settings now reflects the new API pattern.
893-906: Updated faceting settings documentationDocumentation for faceting settings now uses the new
settingproperty pattern.
913-926: Updated typo tolerance settings documentationDocumentation for typo tolerance settings now reflects the new API pattern.
934-947: Updated separator tokens settings documentationDocumentation for separator tokens settings now uses the new
settingproperty pattern.
954-967: Updated non-separator tokens settings documentationDocumentation for non-separator tokens settings now reflects the new API pattern.
974-987: Updated dictionary settings documentationDocumentation for dictionary settings now uses the new
settingproperty pattern.
994-1007: Updated proximity precision settings documentationDocumentation for proximity precision settings now reflects the new API pattern.
1014-1027: Updated facet search settings documentationDocumentation for facet search settings now uses the new
settingproperty pattern.
1034-1047: Updated prefix search settings documentationDocumentation for prefix search settings now reflects the new API pattern.
1054-1067: Updated embedders settings documentationDocumentation for embedders settings now uses the new
settingproperty pattern.
1074-1087: Updated search cutoff settings documentationDocumentation for search cutoff settings now reflects the new API pattern.
src/types/settings.ts (1)
103-106: Potential typo:"OpenAi"should be"OpenAI"In
EmbedderSourcethe variant is spelled"OpenAi", but the public
documentation (and most provider identifiers) use"OpenAI".
A mismatch would make type-level validation pass yet fail at runtime if the
backend expects the canonical spelling.Please verify the exact casing used by the API and update the literal to avoid
hard-to-trace 400 responses.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.code-samples.meilisearch.yaml (1)
402-405: Outdated API in sample: missing.settingnamespace
updateTypoTolerancemust be called viaindex.setting. Fix the sample to avoid broken docs.Apply this diff:
-typo_tolerance_guide_5: |- - client.index('movies').updateTypoTolerance({ +typo_tolerance_guide_5: |- + client.index('movies').setting.updateTypoTolerance({ disableOnNumbers: true })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.code-samples.meilisearch.yaml(17 hunks)README.md(1 hunks)src/indexes.ts(4 hunks)src/types/experimental-features.ts(1 hunks)src/types/index.ts(1 hunks)src/types/settings.ts(1 hunks)src/types/task_and_batch.ts(2 hunks)src/types/types.ts(0 hunks)tests/multi_modal_search.test.ts(2 hunks)tests/settings.test.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/types/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/types/index.ts
- README.md
🧰 Additional context used
🧬 Code graph analysis (5)
src/types/settings.ts (2)
src/types/shared.ts (2)
NonNullableDeepRecordValues(11-18)PascalToCamelCase(21-21)src/types/types.ts (3)
RecordAny(10-10)ChatSettings(587-592)SearchParams(239-263)
src/indexes.ts (3)
src/settings.ts (2)
makeSettingFns(36-66)SettingFns(15-25)src/types/settings.ts (2)
Settings(270-270)UpdatableSettings(212-257)src/types/task_and_batch.ts (1)
EnqueuedTaskPromise(157-163)
tests/settings.test.ts (5)
tests/utils/meilisearch-test-utils.ts (3)
getClient(369-369)assert(231-234)objectKeys(358-358)src/meilisearch.ts (1)
index(92-94)src/types/settings.ts (8)
SingleUpdatableSettings(263-263)RankingRuleView(193-197)ProximityPrecisionView(38-40)FacetValuesSort(69-69)EmbedderSource(105-107)PrefixSearchSettings(188-190)VectorStoreBackend(209-209)UpdatableSettings(212-257)src/types/task_and_batch.ts (1)
EnqueuedTaskPromise(157-163)src/indexes.ts (1)
setting(583-585)
tests/multi_modal_search.test.ts (1)
src/types/settings.ts (1)
EmbeddingSettings(163-169)
src/types/task_and_batch.ts (1)
src/types/settings.ts (1)
UpdatableSettings(212-257)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests (Node.js 20)
- GitHub Check: integration-tests (Node.js 22)
🔇 Additional comments (8)
src/types/experimental-features.ts (1)
1-11: Mapped experimental features: concise and type-safeUnion + mapped type reduces drift and enforces allowed keys. LGTM.
Also applies to: 19-20
tests/multi_modal_search.test.ts (1)
3-3: Type alignment for embedder configUsing
satisfies EmbeddingSettingsis correct and prevents silent shape drift. LGTM.Also applies to: 120-120
src/types/task_and_batch.ts (1)
1-2: TaskDetails now extends UpdatableSettings — correct API reflectionSwitching from
SettingstoUpdatableSettingsmatches server “details” semantics (optional, nullable per-key) and fixes overly strict typing.IndexSwapcompact form is fine. LGTM.Also applies to: 110-110, 117-136
src/indexes.ts (4)
68-96: Settings facade initialization looks exhaustive and correctThe
MakeSettingsRecordmap covers all currentSingleUpdatableSettingskeys with appropriate verbs. LGTM.
142-146: searchGet: fail fast on non-string filters (duplicate of prior review)This still silently drops non-string, non-array filters. Prefer throwing for any unsupported type.
Will this be addressed by #1925 as previously noted?
556-567: Keep top-level get/update/reset settings — good for bulk opsBulk settings endpoints preserved alongside the new facade. LGTM.
576-585: Newsettinggetter API surfacePublic getter exposing generated per-setting fns is clean and discoverable. LGTM.
tests/settings.test.ts (1)
39-53: Data-driven settings tests are solid and future-proofMatrix-based assertions plus
satisfies Required<UpdatableSettings>catch drift and validate both single and bulk endpoints. LGTM.Also applies to: 511-529, 531-605, 606-635
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/settings.test.ts (1)
539-550: Avoid unnecessary .bind(this).makeSettingFns closures don’t use this; binding can be dropped to reduce noise.
Apply:
- const getSetting = index.setting[`get${capitalizedKey}`].bind( - index, - ) as () => Promise<SingleUpdatableSettings[keyof SingleUpdatableSettings]>; + const getSetting = index.setting[ + `get${capitalizedKey}` + ] as () => Promise<SingleUpdatableSettings[keyof SingleUpdatableSettings]>; - const updateSetting = index.setting[`update${capitalizedKey}`].bind( - index, - ) as ( + const updateSetting = index.setting[ + `update${capitalizedKey}` + ] as ( v: SingleUpdatableSettings[keyof SingleUpdatableSettings], ) => EnqueuedTaskPromise; - const resetSetting = index.setting[`reset${capitalizedKey}`].bind(index); + const resetSetting = index.setting[`reset${capitalizedKey}`];src/types/settings.ts (1)
28-30: Consider centralizing PartialAndNullable in shared types.Minor consistency win: export it from src/types/shared.ts and import here.
Apply:
-import type { - PascalToCamelCase, - NonNullableDeepRecordValues, -} from "./shared.js"; +import type { + PascalToCamelCase, + NonNullableDeepRecordValues, + PartialAndNullable, +} from "./shared.js"; @@ -/** Map properties of a record to be optional and nullable. */ -type PartialAndNullable<T> = { [P in keyof T]?: T[P] | null };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/indexes.ts(4 hunks)src/types/settings.ts(1 hunks)src/types/shared.ts(1 hunks)tests/settings.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/types/shared.ts
🧰 Additional context used
🧬 Code graph analysis (3)
src/indexes.ts (3)
src/settings.ts (2)
makeSettingFns(36-66)SettingFns(15-25)src/types/settings.ts (2)
Settings(268-268)UpdatableSettings(210-255)src/types/task_and_batch.ts (1)
EnqueuedTaskPromise(157-163)
tests/settings.test.ts (6)
tests/utils/meilisearch-test-utils.ts (3)
getClient(369-369)assert(231-234)objectKeys(358-358)src/meilisearch.ts (1)
index(92-94)src/http-requests.ts (1)
T(230-274)src/types/settings.ts (8)
SingleUpdatableSettings(261-261)RankingRuleView(191-195)ProximityPrecisionView(36-38)FacetValuesSort(67-67)EmbedderSource(103-105)PrefixSearchSettings(186-188)VectorStoreBackend(207-207)UpdatableSettings(210-255)src/types/task_and_batch.ts (1)
EnqueuedTaskPromise(157-163)src/indexes.ts (1)
setting(583-585)
src/types/settings.ts (2)
src/types/shared.ts (2)
PascalToCamelCase(22-22)NonNullableDeepRecordValues(12-19)src/types/types.ts (2)
RecordAny(10-10)SearchParams(239-263)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests (Node.js 22)
- GitHub Check: integration-tests (Node.js 20)
🔇 Additional comments (8)
src/indexes.ts (4)
68-96: SettingFns mapping looks complete and correct.Exhaustive over SingleUpdatableSettings, and HTTP verbs match expected semantics. Nicely wired into makeSettingFns.
140-147: searchGet: fail fast on non‑string filter shapes (avoid silent drops).Currently non‑string and non‑array filters are coerced to undefined, dropping caller criteria. Throw for any non‑string type.
Apply:
- const parseFilter = (filter?: Filter): string | undefined => { - if (typeof filter === "string") return filter; - else if (Array.isArray(filter)) { - throw new MeiliSearchError( - "The filter query parameter should be in string format when using searchGet", - ); - } else return undefined; - }; + const parseFilter = (filter?: Filter): string | undefined => { + if (typeof filter === "string") return filter; + if (Array.isArray(filter)) { + throw new MeiliSearchError( + "The filter query parameter should be in string format when using searchGet", + ); + } + if (filter != null) { + throw new MeiliSearchError( + `Unsupported filter type for searchGet: ${typeof filter}. Only string filters are allowed.`, + ); + } + return undefined; + };
556-574: Settings endpoints (get/update/reset) API remains consistent.Endpoints and verbs align with server routes; return types are correct.
579-585: Good public facade for per‑setting methods.Getter cleanly exposes the generated SettingFns without leaking internals.
tests/settings.test.ts (2)
511-519: Experimental features toggling: LGTM.Enabling required flags upfront and cleaning up after keeps tests deterministic.
606-635: Comprehensive resetSettings assertion.Covers new keys (chat, vectorStore) and expected null/["*"] defaults.
src/types/settings.ts (2)
1-6: Reuse of shared NonNullableDeepRecordValues is correct.Prevents array/object corruption; good consolidation in shared.ts.
209-269: Settings/UpdatableSettings surface looks aligned with server.Keys cover the full set (including chat, vectorStore); shapes and unions match docs.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
tests/settings.test.ts (1)
533-541: Tighten types for per-key accessors (optional)You can preserve type safety by parameterizing on TKey to avoid
keyof Settingunions for get/update.Example:
const getSetting = index.settings[`get${capitalizedKey}`] as <K extends keyof Setting>(this: unknown) => Promise<Setting[K]>; const updateSetting = index.settings[`update${capitalizedKey}`] as <K extends keyof Setting>(v: Setting[K]) => EnqueuedTaskPromise;src/types/settings.ts (2)
261-267: Make Settings fully required at the top level
Settingsshould represent the complete, non-null shape returned by getSettings. Currently keys can remain optional. Require the first layer.Apply this diff:
-export type Settings = NonNullableDeepRecordValues<UpdatableSettings>; +export type Settings = NonNullableDeepRecordValues<Required<UpdatableSettings>>;
172-177: Use Locale type for locales (optional consistency)Elsewhere
SearchParamsusesLocale[]. Align here for consistency and better type safety.Apply this diff:
-import type { RecordAny, SearchParams } from "./types.js"; +import type { RecordAny, SearchParams, Locale } from "./types.js"; @@ - locales: string[]; + locales: Locale[];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.code-samples.meilisearch.yaml(17 hunks)README.md(1 hunks)src/indexes.ts(4 hunks)src/settings.ts(1 hunks)src/types/settings.ts(1 hunks)src/types/shared.ts(1 hunks)tests/documents.test.ts(4 hunks)tests/env/node/getting_started.cjs(1 hunks)tests/search.test.ts(3 hunks)tests/settings.test.ts(1 hunks)tests/token.test.ts(1 hunks)tests/typed_search.test.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- tests/documents.test.ts
- tests/env/node/getting_started.cjs
- .code-samples.meilisearch.yaml
- tests/search.test.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/settings.ts (5)
src/types/settings.ts (1)
Setting(259-259)src/types/task_and_batch.ts (1)
EnqueuedTaskPromise(157-163)src/http-requests.ts (1)
HttpRequests(139-366)src/task.ts (1)
HttpRequestsWithEnqueuedTaskPromise(181-185)src/types/types.ts (1)
RecordAny(10-10)
tests/settings.test.ts (5)
tests/utils/meilisearch-test-utils.ts (3)
getClient(369-369)assert(231-234)objectKeys(358-358)src/meilisearch.ts (1)
index(92-94)src/types/settings.ts (8)
Setting(259-259)RankingRuleView(189-193)ProximityPrecisionView(34-36)FacetValuesSort(65-65)EmbedderSource(101-103)PrefixSearchSettings(184-186)VectorStoreBackend(205-205)UpdatableSettings(208-253)src/types/task_and_batch.ts (1)
EnqueuedTaskPromise(157-163)src/indexes.ts (1)
settings(583-585)
tests/typed_search.test.ts (3)
tests/env/node/getting_started.cjs (1)
index(10-10)src/meilisearch.ts (1)
index(92-94)tests/env/node/search_example.cjs (2)
index(16-16)index(26-26)
src/types/settings.ts (2)
src/types/shared.ts (3)
PascalToCamelCase(25-25)PartialAndNullable(12-12)NonNullableDeepRecordValues(15-22)src/types/types.ts (2)
RecordAny(10-10)SearchParams(239-263)
src/indexes.ts (3)
src/settings.ts (2)
makeSettingFns(38-68)SettingFns(11-21)src/types/settings.ts (2)
Settings(266-266)UpdatableSettings(208-253)src/types/task_and_batch.ts (1)
EnqueuedTaskPromise(157-163)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests (Node.js 20)
- GitHub Check: integration-tests (Node.js 22)
🔇 Additional comments (9)
src/settings.ts (1)
50-61: Union-typed get/update leak specific setting types (acknowledging prior discussion).Both get${Name} and update${Name} are typed as Setting[keyof typeof opts], i.e., a union of all setting payloads/returns. This undermines the per-setting compile-time guarantees exposed by SettingFns. If you want to keep the current approach (as discussed), consider at least casting each assigned function to the specific SettingFns member instead of casting the whole object at the end to reduce accidental shape drift.
src/indexes.ts (3)
68-96: Per-setting methods wiring looks correct; double-check HTTP verbs for new endpoints.The mapping to PUT/PATCH looks consistent, including chat and vectorStore as PATCH. Given earlier doc inconsistencies around vector-store, please verify against the server version used in CI.
139-147: Fail fast on unsupported filter shapes in searchGet (avoid silent drops).Non-string filters other than arrays are silently ignored. Prefer throwing for any non-string (non-null/undefined) input to prevent unnoticed criteria loss.
Apply this diff:
- const parseFilter = (filter?: Filter): string | undefined => { - if (typeof filter === "string") return filter; - else if (Array.isArray(filter)) { - throw new MeiliSearchError( - "The filter query parameter should be in string format when using searchGet", - ); - } else return undefined; - }; + const parseFilter = (filter?: Filter): string | undefined => { + if (typeof filter === "string") return filter; + if (Array.isArray(filter)) { + throw new MeiliSearchError( + "The filter query parameter should be in string format when using searchGet", + ); + } + if (filter != null) { + throw new MeiliSearchError( + `Unsupported filter type for searchGet: ${typeof filter}. Only string filters are allowed.`, + ); + } + return undefined; + };
576-585: Nice public facade for per-setting operations.Getter cleanly exposes the generated methods while keeping the field private/read-only.
tests/token.test.ts (1)
301-304: Test updated to new settings facade path — OK.Using index(...).settings.updateFilterableAttributes(...) aligns with the new API surface.
src/types/shared.ts (1)
11-13: Utility type addition looks good.PartialAndNullable is clear and matches the settings typing needs.
tests/typed_search.test.ts (1)
121-123: Migration to settings facade — OK.Switched to index(uid).settings.updateFilterableAttributes(...) correctly.
tests/settings.test.ts (1)
427-433: Locale code “eng” may be invalid — verify accepted valuesMany Meilisearch builds expect BCP 47 tags like "en" or "en-US". Using "eng" could fail.
Please confirm supported locale codes in your target server version. If needed, switch to "en".
src/types/settings.ts (1)
195-203: ChatSettings declared only once
Verified that settings.ts is the sole definition and it’s exported exactly once via the barrel.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/settings.test.ts (2)
571-571: Add timeouts to waitTask on reset flows to reduce flakesAlign with other waits using 60s timeout.
Apply this diff:
- const task = await resetSetting().waitTask(); + const task = await resetSetting().waitTask({ timeout: 60_000 });- const task = await index.updateSettings({ [castKey]: null }).waitTask(); + const task = await index + .updateSettings({ [castKey]: null }) + .waitTask({ timeout: 60_000 });- const task = await index.resetSettings().waitTask(); + const task = await index.resetSettings().waitTask({ timeout: 60_000 });Also applies to: 597-597, 605-605
549-553: Avoid reassigning the mappedSetting parameter for clarityUse a new local (e.g., cases) instead of reassigning the function parameter.
Apply this diff:
- mappedSetting = mappedSetting.map(([a, b]) => [ + const cases = mappedSetting.map(([a, b]) => [ a, b === undefined ? "" : ` with ${b}`, ]); - test.for(mappedSetting)( + test.for(cases)( "update and get methods%s", async ([{ input, assertion }]) => { const task = await updateSetting(input).waitTask({ timeout: 60_000 }); assert.isTaskSuccessful(task); assert.strictEqual(task.type, "settingsUpdate"); const taskSetting = task.details?.[castKey]; assert.isDefined(taskSetting); assertion(input, taskSetting); const setting = await getSetting(); assertion(input, setting); }, ); @@ - test.for(mappedSetting)( + test.for(cases)( `${index.updateSettings.name} and ${index.getSettings.name} methods%s`, async ([{ input, assertion }]) => { const task = await index .updateSettings({ [castKey]: input }) .waitTask({ timeout: 60_000 });Also applies to: 554-568, 577-594
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/settings.test.ts(1 hunks)tests/utils/meilisearch-test-utils.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-25T08:05:45.241Z
Learnt from: flevi29
PR: meilisearch/meilisearch-js#1864
File: tests/settings.test.ts:564-569
Timestamp: 2025-09-25T08:05:45.241Z
Learning: In Vitest/Chai assertions, assert.oneOf uses strict equality comparison and doesn't work for array comparison since arrays are compared by reference, not content. For checking if a value is either null or deep-equals an array like ["*"], use conditional logic with separate null check and assert.sameMembers for the array case.
Applied to files:
tests/settings.test.ts
🧬 Code graph analysis (1)
tests/settings.test.ts (4)
tests/utils/meilisearch-test-utils.ts (3)
getClient(374-374)assert(231-234)objectKeys(363-363)src/types/settings.ts (8)
Setting(259-259)RankingRuleView(189-193)ProximityPrecisionView(34-36)FacetValuesSort(65-65)EmbedderSource(101-103)PrefixSearchSettings(184-186)VectorStoreBackend(205-205)UpdatableSettings(208-253)src/types/task_and_batch.ts (1)
EnqueuedTaskPromise(157-163)src/indexes.ts (1)
settings(583-585)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration-tests (Node.js 20)
- GitHub Check: integration-tests (Node.js 22)
🔇 Additional comments (5)
tests/utils/meilisearch-test-utils.ts (1)
347-354: TypedobjectKeyshelper looks great.The generic constraint keeps the runtime helper aligned with our string literal unions, and the cast is justified by the mapped type requirement. Nicely done.
tests/settings.test.ts (4)
270-279: Wildcard facet sort default handling looks goodAllowing "*": "alpha" | "count" | undefined prevents brittle assertions across server versions.
570-575: Fix reset assertion: allow null or deep-equal ["*"] (oneOf/includeDeepMembers are unsuitable)oneOf uses strict equality; includeDeepMembers is semantically wrong here. Use a conditional with sameMembers.
Apply this diff:
test("reset method", async () => { const task = await resetSetting().waitTask(); - assert.includeDeepMembers([null, ["*"]], [task.details?.[castKey]]); + const resetValue = task.details?.[castKey]; + if (resetValue === null) { + // ok + } else { + assert.sameMembers(resetValue, ["*"]); + } assert.isTaskSuccessful(task); assert.strictEqual(task.type, "settingsUpdate"); });
596-601: Same reset assertion issue for index.updateSettings({ key: null })Mirror the conditional check to handle null or deep-equal ["*"].
Apply this diff:
test(`reset with ${index.updateSettings.name} method`, async () => { const task = await index.updateSettings({ [castKey]: null }).waitTask(); - assert.includeDeepMembers([null, ["*"]], [task.details?.[castKey]]); + const resetValue = task.details?.[castKey]; + if (resetValue === null) { + // ok + } else { + assert.sameMembers(resetValue, ["*"]); + } assert.isTaskSuccessful(task); assert.strictEqual(task.type, "settingsUpdate"); });
403-413: Do not couple apiKey output shape to input; allow redaction/omissionServers may redact secrets; comparing typeof input vs output is brittle. Just validate output shape.
Apply this diff:
- const { apiKey: inputApiKey, ...restOfInputEmbeddingSettings } = + const { apiKey: _inputApiKey, ...restOfInputEmbeddingSettings } = inputEmbeddingSettings; const { apiKey: outputApiKey, ...restOfOutputEmbeddingSettings } = outputEmbeddingSettings; @@ assert.deepEqual( restOfInputEmbeddingSettings, restOfOutputEmbeddingSettings, ); - assert(typeof inputApiKey === typeof outputApiKey); + // apiKey may be redacted/omitted by the server + assert.isTrue(outputApiKey == null || typeof outputApiKey === "string");
|
Hello @Strift ! It's been almost 8 months since I've opened this PR. Can you please give it a review? I know features are more important to you guys, and I want to work on those too, but I want to first improve some things so that work on those features becomes a lot easier, and also so I don't have to keep updating this and other PRs. If we get this PR moving, I'll work on some features as well. cc @brunoocasali , @curquiza |
|
Hey @flevi29, Sorry to keep the PR hanging for so long. Unfortunately, it's hard for Meilisearch to prioritize refactoring that includes breaking changes when they have new features to integrate and other SDKs to maintain. I only have limited bandwidth, and my time allocation follows from those priorities. It's unfortunate, because I think these improvements are much-welcome to the SDK and make our work easier. I would love to integrate them sooner. |
|
I understand. I might open a few missing feature PRs in the coming days. |
3 participants
Pull Request
What does this PR do?
Settingstypessettingof theIndexclassvectorStoreSettingto experimental settingsMigration
client.index(uid).updateSettingsnow requires the typeUpdatableSettingsinstead of justSettingsSummary by CodeRabbit
New Features
Refactor
Documentation
Tests