-
Couldn't load subscription status.
- Fork 101
Add sharding feature #2037
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
Add sharding feature #2037
Conversation
WalkthroughUpdates network and tasks typings, adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Client Code
participant SDK as MeiliSearch SDK
participant HTTP as HTTP Client
participant API as Meilisearch Server
Dev->>SDK: updateNetwork(options: UpdatableNetwork)
SDK->>HTTP: PATCH /network (body: options)
HTTP->>API: PATCH /network
API-->>HTTP: 200 OK (Network)
HTTP-->>SDK: Network
SDK-->>Dev: Promise<Network>
rect rgba(230,250,230,0.4)
note right of API: Network may include remotes.writeApiKey and sharding flag
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ 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. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/types/shared.ts (1)
9-12: Ensure DeepPartial preserves array shapesWhen
DeepPartialhits an array, it currently turns that array into an object with optional numeric keys, soDeepPartial<string[]>no longer acceptsstring[].UpdatableNetwork(and any future callers) will immediately lose type safety as soon asNetworkgains an array member (e.g., a list of shard names or roles). A small tweak keeps array members as arrays while still recursing into their element type.-export type DeepPartial<T> = T extends object - ? { [TKey in keyof T]?: DeepPartial<T[TKey]> } - : T; +export type DeepPartial<T> = T extends ReadonlyArray<infer U> + ? ReadonlyArray<DeepPartial<U>> + : T extends Array<infer U> + ? Array<DeepPartial<U>> + : T extends object + ? { [TKey in keyof T]?: DeepPartial<T[TKey]> } + : T;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/meilisearch.ts(2 hunks)src/types/index.ts(1 hunks)src/types/network.ts(1 hunks)src/types/shared.ts(1 hunks)src/types/task-and-batch.ts(2 hunks)src/types/types.ts(1 hunks)tests/client.test.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
tests/client.test.ts (2)
src/types/network.ts (1)
UpdatableNetwork(17-17)tests/utils/meilisearch-test-utils.ts (1)
assert(231-234)
src/types/network.ts (1)
src/types/shared.ts (1)
DeepPartial(10-12)
src/types/task-and-batch.ts (1)
src/types/types.ts (1)
MeiliSearchErrorResponse(825-832)
src/meilisearch.ts (1)
src/types/network.ts (2)
UpdatableNetwork(17-17)Network(11-15)
⏰ 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 (2)
src/meilisearch.ts (1)
387-389: Deep-partial typing unlocks granular updatesSwitching to
UpdatableNetworklets callers patch just the sharding flag or remote write keys without rebuilding full objects, while the request flow stays unchanged. Looks solid.src/types/network.ts (1)
1-18: Network typings now cover sharding and write keysThe dedicated
Remote/Networktypes model the new API surface, and theDeepPartialalias makes partial network updates ergonomic. Nice addition.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2037 +/- ##
==========================================
- Coverage 98.83% 98.83% -0.01%
==========================================
Files 18 18
Lines 1549 1547 -2
Branches 334 334
==========================================
- Hits 1531 1529 -2
Misses 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this 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 (1)
tests/utils/assertions/error.ts (1)
5-10: Relax the key-count assertion to future-proof error checksHard-coding
assert.lengthOf(..., 4)will break as soon as the API grows an optional field (e.g.details). Dropping that length check keeps the helper resilient while still validating the required properties.- assert.lengthOf(Object.keys(error), 4); const { message, code, type, link } = error;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
tests/tasks-and-batches.test.ts(3 hunks)tests/utils/assert.ts(1 hunks)tests/utils/assertions/error.ts(1 hunks)tests/utils/assertions/promise.ts(1 hunks)tests/utils/assertions/tasks-and-batches.ts(1 hunks)tests/utils/meilisearch-test-utils.ts(2 hunks)tests/utils/object.ts(1 hunks)tests/utils/tasks-and-batches.ts(0 hunks)
💤 Files with no reviewable changes (1)
- tests/utils/tasks-and-batches.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-04T06:44:01.679Z
Learnt from: flevi29
PR: meilisearch/meilisearch-js#2012
File: tests/client.test.ts:13-13
Timestamp: 2025-09-04T06:44:01.679Z
Learning: The meilisearch-js project supports Node.js 20 and above, so JSON imports should use the modern `with { type: "json" }` syntax rather than the deprecated `assert { type: "json" }` syntax.
Applied to files:
tests/utils/meilisearch-test-utils.ts
🧬 Code graph analysis (3)
tests/utils/assert.ts (3)
tests/utils/assertions/error.ts (1)
errorAssertions(4-12)tests/utils/assertions/promise.ts (1)
promiseAssertions(6-43)tests/utils/assertions/tasks-and-batches.ts (1)
tasksAndBatchesAssertions(38-229)
tests/utils/assertions/error.ts (1)
src/types/types.ts (1)
MeiliSearchErrorResponse(825-832)
tests/utils/assertions/tasks-and-batches.ts (3)
tests/utils/object.ts (1)
objectKeys(1-3)src/types/task-and-batch.ts (7)
TaskStatus(33-35)TaskType(42-56)EnqueuedTask(97-103)Batch(236-245)TasksResults(194-194)BatchesResults(252-252)Task(156-167)tests/utils/assertions/error.ts (1)
errorAssertions(4-12)
⏰ 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 (6)
tests/tasks-and-batches.test.ts (3)
5-12: Consolidated helper imports keep these tests tidyPulling
assertandobjectEntriesfrom the barrel keeps the helper surface compact.
35-101: Nice exhaustive coverage of the query variantsEnumerating every supported filter alongside the
*fallbacks gives the parameterized runs great coverage.
194-215: Stricter result-shape validation is spot onValidating with
isTasksOrBatchesResultsbefore drilling into items will surface regressions aroundtotal/from/nextwith minimal boilerplate.tests/utils/object.ts (1)
1-8: Typed entry/key helpers make the parameterized tests clearerThese narrowed wrappers eliminate downstream casts when feeding
describe.for, keeping the suites expressive.tests/utils/assert.ts (1)
6-18: I like the single custom assert surfaceMerging vitest’s assert with the domain-specific suites keeps call sites focused while preserving the base assertions.
tests/utils/assertions/promise.ts (1)
6-43: Promise helpers cover the usual edge casesCapturing the thrown error and supporting optional message matchers will save a lot of repetitive test boilerplate.
|
Will merge after #2050 fixes tests |
Pull Request
Related issue
Fixes #2001
What does this PR do?
Summary by CodeRabbit
New Features
Refactor
Chores
Tests