Add conversational search #774
Conversation
|
Warning Rate limit exceeded@Strift has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 50 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds ChatWorkspaces endpoint, settings/streaming delegates, DTOs for workspaces and prompts, client wiring (client->chats), HTTP postStream support, index-level chat/embedders settings, and PHPUnit tests for workspace CRUD, listing, reset, and streaming completions. (<=50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Client
participant Chats as ChatWorkspaces
participant HTTP as Http
App->>Client: client->chats->listWorkspaces()
Client->>Chats: listWorkspaces()
Chats->>HTTP: GET /chats
HTTP-->>Chats: 200 JSON
Chats-->>Client: ChatWorkspacesResults
Client-->>App: results
sequenceDiagram
participant App
participant Client
participant Chats as ChatWorkspaces
participant HTTP as Http
participant Stream as Stream
App->>Client: client->chatWorkspace("my-assistant")->updateSettings(payload)
Client->>Chats: workspace("my-assistant")->updateSettings(payload)
Chats->>HTTP: PATCH /chats/my-assistant/settings (JSON)
HTTP-->>Chats: 200 JSON -> ChatWorkspaceSettings
Chats-->>Client: ChatWorkspaceSettings
Client-->>App: settings
App->>Client: client->chatWorkspace("my-assistant")->streamCompletion(options)
Client->>Chats: workspace("my-assistant")->streamCompletion(options)
Chats->>HTTP: POST /chats/my-assistant/chat/completions (stream)
HTTP-->>Stream: chunked data
Stream-->>App: StreamInterface (read chunks)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (20)
src/Contracts/Http.php (1)
50-54: postStream contract addition looks good; consider documenting all thrown exceptions for parityThe implementation (src/Http/Client.php::postStream) can also throw ClientExceptionInterface and CommunicationException. To keep interface docs aligned and help consumers, consider adding these to the docblock.
Apply this docblock tweak:
/** * @throws ApiException * @throws JsonEncodingException + * @throws \Psr\Http\Client\ClientExceptionInterface + * @throws \Meilisearch\Exceptions\CommunicationException */ public function postStream(string $path, $body = null, array $query = []): \Psr\Http\Message\StreamInterface;src/Client.php (1)
59-59: Initialize $this->chats endpoint early; confirm typed property exists in the traitAssuming HandlesChatWorkspaces declares a typed public property for $chats, this avoids dynamic property issues on PHP 8.2+. If it's not declared there, we should add it.
I can open a small follow-up to add a typed property declaration if needed.
tests/Settings/ChatTest.php (1)
16-21: Optional: use heredoc/nowdoc for multi-line documentTemplate for readabilityThe embedded newline in a single-quoted string is fine but a heredoc/nowdoc improves readability of the template.
Option:
- 'documentTemplate' => '{% for field in fields %}{% if field.is_searchable and field.value != nil %}{{ field.name }}: {{ field.value }} -{% endif %}{% endfor %}', + 'documentTemplate' => <<<'TPL' +{% for field in fields %}{% if field.is_searchable and field.value != nil %}{{ field.name }}: {{ field.value }} +{% endif %}{% endfor %} +TPL,src/Http/Client.php (1)
148-156: Consider setting standard header casing and SSE Accept for streamingHeaders are case-insensitive, but using canonical casing is cleaner. Also, conversational streaming often returns text/event-stream; proactively sending Accept can improve content negotiation. If you prefer to keep postStream generic, we can add Accept at the caller level once the Http interface supports custom headers.
Apply:
- return $this->executeStream($request, ['Content-type' => 'application/json']); + return $this->executeStream($request, [ + 'Content-Type' => 'application/json', + 'Accept' => 'text/event-stream', + ]);src/Endpoints/Delegates/HandlesChatWorkspaces.php (2)
12-12: Public property may be too permissive; consider tightening mutabilityExposing
$chatsas a public mutable property invites accidental reassignments. If you need it publicly accessible (to keep$client->chats), consider making it effectively read-only (e.g., via a getter, or documenting/guarding the assignment in the owning class).
17-20: Method naming nit: prefer “list” over “get” for collection endpointsMinor naming nit to align with endpoint semantics:
listChatWorkspaces()would mirror the underlyinglistWorkspaces(). Not a blocker.src/Endpoints/Delegates/HandlesSettings.php (1)
505-541: Docblock is comprehensive; consider factoring the searchParameters shape into a reusable typeYou’ve inlined a large array shape for
searchParameterstwice (getChat/updateChat). Consider extracting a shared phpdoc typedef (via a dedicated value object/contract or a @phpstan-type alias) to avoid drift and keep it consistent with search settings elsewhere.src/Contracts/ChatWorkspacesResults.php (2)
13-20: Guard against missing offset/limit to avoid noticesIf the API omits offset/limit (e.g., on older servers or different defaults), accessing
$params['offset']/$params['limit']will trigger notices. Safer defaults maintain robustness without changing behavior when fields are present.Suggested change:
- $this->offset = $params['offset']; - $this->limit = $params['limit']; + $this->offset = $params['offset'] ?? 0; + $this->limit = $params['limit'] ?? \count($this->data);If the Chats list endpoint always returns offset/limit/total, feel free to keep strict access. Otherwise, defaulting avoids runtime notices.
55-58: Minor: count() duplicates parent::count()The parent Data already implements Countable. You can drop this override or delegate to
parent::count()to avoid duplication.- public function count(): int - { - return \count($this->data); - } + public function count(): int + { + return parent::count(); + }tests/Endpoints/ChatWorkspaceTest.php (3)
25-31: Enable experimental features once per test class to reduce overheadOptional: switch to setUpBeforeClass to toggle
/experimental-featuresonce, reducing per-test overhead. Not required, just cleaner/faster.
7-8: Avoid naming collision with the high-level Client; alias the HTTP Client importImporting
Meilisearch\Http\ClientasClientcan be confusing alongside$this->client(the high-level client). Alias it to improve readability.-use Meilisearch\Http\Client; +use Meilisearch\Http\Client as HttpClient;And update the instantiation:
- $http = new Client($this->host, getenv('MEILISEARCH_API_KEY')); + $http = new HttpClient($this->host, getenv('MEILISEARCH_API_KEY'));
91-131: Streaming test is pragmatic; consider a more defensive guard to reduce flakinessThe
maxChunkssafety is good. If the backend is slow or verbose, this can still flake. Consider:
- Lowering
maxChunks(e.g., 200) and/or- Skipping the test unless a known-good chat provider/config is available (env flag), or
- Parsing SSE “data:” lines to assert structured progress instead of raw length.
Not a blocker.
If CI occasionally flakes here, I can propose a provider-gated skip mechanism or SSE-aware parser for more deterministic assertions.
src/Contracts/ChatWorkspaceSettings.php (1)
75-87: toArray includes nulls; ensure it’s not used for PATCH bodies as-isIf this
toArray()ever feeds PATCH bodies, sending explicit nulls could clear server-side values. Current code paths send raw arrays directly, so this is fine; just noting for future usage.src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php (7)
16-23: URL-encode the workspace identifier when composing the pathWorkspace identifiers can contain characters that should be percent-encoded in URLs (spaces, slashes, unicode, etc.). Encode the path segment to avoid routing errors and subtle bugs.
Apply this diff:
- $response = $this->http->get('/chats/'.$this->workspaceName.'/settings'); + $workspace = rawurlencode($this->workspaceName); + $response = $this->http->get('/chats/'.$workspace.'/settings');
28-37: Verify allowed values forsource(string union looks potentially inconsistent with API naming)Double-check the provider identifiers against the server API. For example, docs often use lower-case
openaior hyphenatedazure-openai, andvllm(all lower-case). Current values ('openAi','azureOpenAi','vLlm') may not match server expectations.If needed, consider centralizing these values in an enum or constants to avoid drift.
I can propose an enum/constant set once the canonical values are confirmed.
41-48: Also encode the workspace in update pathSame reasoning as for GET: encode the workspace path segment.
- $response = $this->http->patch('/chats/'.$this->workspaceName.'/settings', $settings); + $workspace = rawurlencode($this->workspaceName); + $response = $this->http->patch('/chats/'.$workspace.'/settings', $settings);
55-62: Confirm DELETE semantics vs method name, and encode workspace
- The PR summary mentions “DELETE /chats/{workspace_uid}/settings — delete a workspace.” The method here is named
resetSettings()and the docblock says “Reset … to default values” while returningChatWorkspaceSettings. Please verify the server semantics:
- If it actually deletes the workspace (likely 204/empty body), consider renaming to
deleteWorkspace()and returnvoid/array.- If it resets settings and returns the resulting settings, then the name and return type are fine.
- Also encode the workspace segment as with GET/PATCH.
- $response = $this->http->delete('/chats/'.$this->workspaceName.'/settings'); + $workspace = rawurlencode($this->workspaceName); + $response = $this->http->delete('/chats/'.$workspace.'/settings');If the endpoint deletes the workspace and returns no content, adjust the signature and return accordingly:
- public function resetSettings(): ChatWorkspaceSettings + public function deleteWorkspace(): void { if (null === $this->workspaceName) { - throw new \InvalidArgumentException('Workspace name is required to reset settings'); + throw new \InvalidArgumentException('Workspace name is required to delete a workspace'); } - - $response = $this->http->delete('/chats/'.$this->workspaceName.'/settings'); - - return new ChatWorkspaceSettings($response); + $workspace = rawurlencode($this->workspaceName); + $this->http->delete('/chats/'.$workspace.'/settings'); }
67-72: Tighten/clarify the options docblock; the method can enforcestream
- You can restrict
roleto known values ('system'|'user'|'assistant'), which improves IDE/static analysis.- Since the method is “streamCompletion”, you can treat
streamas optional and document that the method forcesstream=true.- * @param array{ - * model: string, - * messages: array<array{role: string, content: string}>, - * stream: bool - * } $options The request body for the chat completion + * @param array{ + * model: string, + * messages: array<array{role: 'system'|'user'|'assistant', content: string}>, + * stream?: bool + * } $options The request body for the chat completion. Note: this method forces stream=true.
75-80: Force streaming flag and encode workspace in completion pathEnsure
streamis set to true (it’s easy to forget from the caller) and encode the workspace path segment.public function streamCompletion(array $options): \Psr\Http\Message\StreamInterface { if (null === $this->workspaceName) { throw new \InvalidArgumentException('Workspace name is required for chat completion'); } - return $this->http->postStream('/chats/'.$this->workspaceName.'/chat/completions', $options); + $options['stream'] = true; + $workspace = rawurlencode($this->workspaceName); + return $this->http->postStream('/chats/'.$workspace.'/chat/completions', $options); }
7-10: Add trait-level phpdoc for host expectations ($http, $workspaceName)Helps Psalm/PHPStan understand that the trait expects these members on the host class, improving static analysis without changing runtime behavior.
namespace Meilisearch\Endpoints\Delegates; use Meilisearch\Contracts\ChatWorkspaceSettings; +/** + * @property \Meilisearch\Contracts\Http $http + * @property string|null $workspaceName + */ trait HandlesChatWorkspaceSettings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
src/Client.php(3 hunks)src/Contracts/ChatWorkspaceSettings.php(1 hunks)src/Contracts/ChatWorkspacesResults.php(1 hunks)src/Contracts/Http.php(1 hunks)src/Endpoints/ChatWorkspaces.php(1 hunks)src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php(1 hunks)src/Endpoints/Delegates/HandlesChatWorkspaces.php(1 hunks)src/Endpoints/Delegates/HandlesSettings.php(2 hunks)src/Http/Client.php(2 hunks)tests/Endpoints/ChatWorkspaceTest.php(1 hunks)tests/Settings/ChatTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (11)
src/Endpoints/ChatWorkspaces.php (6)
src/Meilisearch.php (1)
Meilisearch(7-18)src/Contracts/ChatWorkspacesResults.php (2)
ChatWorkspacesResults(7-59)__construct(13-20)src/Contracts/Endpoint.php (1)
Endpoint(7-23)src/Contracts/ChatWorkspaceSettings.php (1)
__construct(18-30)src/Client.php (1)
__construct(50-71)src/Http/Client.php (2)
__construct(44-63)get(70-78)
src/Contracts/ChatWorkspacesResults.php (2)
src/Contracts/Data.php (1)
Data(7-50)src/Contracts/ChatWorkspaceSettings.php (2)
__construct(18-30)toArray(75-87)
src/Contracts/ChatWorkspaceSettings.php (2)
src/Contracts/Data.php (1)
Data(7-50)src/Contracts/ChatWorkspacesResults.php (2)
__construct(13-20)toArray(45-53)
src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php (3)
src/Contracts/ChatWorkspaceSettings.php (1)
ChatWorkspaceSettings(7-88)src/Contracts/Http.php (4)
get(17-17)patch(42-42)delete(48-48)postStream(54-54)src/Http/Client.php (4)
get(70-78)patch(122-130)delete(132-140)postStream(148-156)
src/Endpoints/Delegates/HandlesChatWorkspaces.php (2)
src/Contracts/ChatWorkspacesResults.php (1)
ChatWorkspacesResults(7-59)src/Endpoints/ChatWorkspaces.php (3)
ChatWorkspaces(12-37)listWorkspaces(26-31)workspace(33-36)
src/Contracts/Http.php (1)
src/Http/Client.php (1)
postStream(148-156)
tests/Endpoints/ChatWorkspaceTest.php (5)
src/Meilisearch.php (1)
Meilisearch(7-18)src/Http/Client.php (2)
Client(25-261)patch(122-130)src/Endpoints/ChatWorkspaces.php (2)
workspace(33-36)listWorkspaces(26-31)src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php (4)
updateSettings(39-48)getSettings(14-23)resetSettings(53-62)streamCompletion(73-80)src/Contracts/ChatWorkspaceSettings.php (8)
getSource(32-35)getOrgId(37-40)getProjectId(42-45)getApiVersion(47-50)getDeploymentId(52-55)getBaseUrl(57-60)getPrompts(70-73)getApiKey(62-65)
src/Client.php (1)
src/Endpoints/ChatWorkspaces.php (1)
ChatWorkspaces(12-37)
src/Http/Client.php (5)
src/Contracts/Http.php (1)
postStream(54-54)src/Http/Serialize/SerializerInterface.php (2)
serialize(21-21)unserialize(30-30)src/Http/Serialize/Json.php (2)
serialize(15-24)unserialize(26-35)src/Exceptions/ApiException.php (1)
ApiException(9-95)src/Exceptions/CommunicationException.php (1)
CommunicationException(7-13)
tests/Settings/ChatTest.php (4)
src/Endpoints/Indexes.php (1)
Indexes(24-278)src/Endpoints/Delegates/HandlesIndex.php (1)
index(25-28)src/Endpoints/Delegates/HandlesSettings.php (2)
getChat(542-545)updateChat(584-587)src/Endpoints/Delegates/HandlesTasks.php (1)
waitForTask(45-48)
src/Endpoints/Delegates/HandlesSettings.php (4)
src/Contracts/Http.php (2)
get(17-17)patch(42-42)src/Http/Client.php (2)
get(70-78)patch(122-130)src/Endpoints/Keys.php (1)
get(153-158)src/Endpoints/Tasks.php (1)
get(16-19)
🔇 Additional comments (12)
src/Client.php (1)
36-36: Good addition: exposes chat workspaces via HandlesChatWorkspacesWiring the chat workspaces endpoint through the trait is consistent with the existing client design.
tests/Settings/ChatTest.php (1)
53-57: Confirm HTTP verb for updating index chat settings (PATCH vs PUT) — PATCH is correctMeilisearch docs and the Settings API spec use PATCH for /indexes/{index_uid}/settings/chat; meilisearch.dev appears to have a typo that says PUT while its example uses PATCH. The current use of PATCH is canonical and tests are correct.
- tests/Settings/ChatTest.php (lines 53–57) — no change required.
- HandlesSettings::updateChat — uses PATCH — OK.
src/Endpoints/ChatWorkspaces.php (3)
20-24: Constructor and workspace scoping look goodStoring the optional workspace name here aligns with the trait-based operations that need it.
26-31: listWorkspaces: OK; assumes standard list payloadReturning ChatWorkspacesResults directly from the GET response is consistent with other endpoints. Ensure the API always includes results/offset/limit/total as expected by the DTO.
33-36: Fluent workspace() helper: LGTMThis mirrors the fluent style used elsewhere in the client.
src/Endpoints/Delegates/HandlesChatWorkspaces.php (1)
10-28: Solid trait and API surface; exposes chat workspaces cleanlyThe trait looks consistent with the rest of the client’s delegation pattern and provides a clear entry point via
$client->chats. The helper methods are thin and correct.src/Endpoints/Delegates/HandlesSettings.php (2)
421-445: Embedders settings endpoints look correct and consistentThe GET/PATCH/DELETE trio and @SInCE annotations align with the existing settings conventions. No issues spotted here.
542-587: No change — PATCH is the correct verb for updating index chat settingsMeilisearch docs specify PATCH for /indexes/{index_uid}/settings/chat (Update index chat settings). The code already uses PATCH, so the suggested change to PUT is incorrect — do not change the verb.
- Location to note:
- src/Endpoints/Delegates/HandlesSettings.php
- getChat(): returns GET for .../settings/chat (line ~544)
- updateChat(): returns PATCH for .../settings/chat (line ~586) — correct
Docs: https://www.meilisearch.com/docs/reference/api/settings — see "Update index chat settings"
Likely an incorrect or invalid review comment.
src/Contracts/ChatWorkspacesResults.php (1)
22-28: Return type annotation for results is precise and helpfulThe
@return array<int, array{uid: string}>keeps consumers aware of the shape. Nicely done.tests/Endpoints/ChatWorkspaceTest.php (1)
63-70: ListWorkspaces assertion is focused and correctAsserting the presence of the workspace by UID keeps the test minimal and robust. Looks good.
src/Contracts/ChatWorkspaceSettings.php (1)
18-30: DTO is clear and aligns with the API response shapeConstructor defaults and getters match what the endpoints return (including masked apiKey). The
promptsshape annotation is helpful for consumers.src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php (1)
9-11: LGTM: Well-scoped trait with clear, typed APIClean separation of workspace chat settings concerns, strict types, and typed returns are on point. The use of a PSR-7 stream for completions is appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Http/Client.php (1)
178-215: Resolved: correct exception is now caught during streaming error parsingCatching JsonDecodingException here addresses the previous bug where \JsonException was ineffective with the current serializer. This ensures we fall back to the raw body and throw ApiException as intended.
🧹 Nitpick comments (2)
src/Http/Client.php (2)
142-156: Add explicit SSE Accept header (and minor header casing nit) for streaming requestsGreat addition. To improve interoperability with proxies and servers that require content negotiation for streaming, set an explicit Accept header for SSE. Also, prefer canonical header casing for readability.
Apply:
- return $this->executeStream($request, ['Content-type' => 'application/json']); + return $this->executeStream($request, [ + 'Content-Type' => 'application/json', + 'Accept' => 'text/event-stream', + ]);
178-215: Broaden JSON Content-Type detection to include +json variants (e.g., application/problem+json)isJSONResponse currently matches only application/json. Many APIs return JSON errors as application/problem+json or other application/*+json types. Broadening detection improves error parsing on both streaming and non-streaming paths without affecting NDJSON.
Outside the selected range, update isJSONResponse:
- /** - * Checks if any of the header values indicate a JSON response. - * - * @param array $headerValues the array of header values to check - * - * @return bool true if any header value contains 'application/json', otherwise false - */ + /** + * Checks if any of the header values indicate a JSON response. + * Matches 'application/json' and any 'application/*+json' (e.g., application/problem+json). + * + * @param array $headerValues the array of header values to check + * + * @return bool true if any header value is JSON-based, otherwise false + */ private function isJSONResponse(array $headerValues): bool { - $filteredHeaders = array_filter($headerValues, static function (string $headerValue) { - return false !== strpos($headerValue, 'application/json'); - }); + $filteredHeaders = array_filter($headerValues, static function (string $headerValue) { + $value = strtolower($headerValue); + return false !== strpos($value, 'application/json') + || false !== strpos($value, '+json'); + }); return \count($filteredHeaders) > 0; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/Http/Client.php(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/Http/Client.php (6)
src/Contracts/Http.php (1)
postStream(54-54)src/Http/Serialize/SerializerInterface.php (2)
serialize(21-21)unserialize(30-30)src/Http/Serialize/Json.php (2)
serialize(15-24)unserialize(26-35)src/Exceptions/JsonDecodingException.php (1)
JsonDecodingException(7-13)src/Exceptions/ApiException.php (1)
ApiException(9-95)src/Exceptions/CommunicationException.php (1)
CommunicationException(7-13)
Co-authored-by: Tomas Norkūnas <norkunas.tom@gmail.com>
Co-authored-by: Tomas Norkūnas <norkunas.tom@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/Http/Client.php (2)
24-24: Good catch on importing StreamInterface (aligns with interface signature).This addresses the prior suggestion to import the PSR StreamInterface for the new streaming API.
186-216: Error-path JSON parsing on streams is correct; minor robustness nits.Catching JsonDecodingException fixes the previously reported leak. Two nits:
- Header name casing: we use 'Content-type' elsewhere—prefer 'Content-Type' consistently.
- JSON detection could be case-insensitive and include application/problem+json.
Outside this hunk, consider refining isJSONResponse to be case-insensitive and to detect problem+json:
private function isJSONResponse(array $headerValues): bool { foreach ($headerValues as $value) { if (stripos($value, 'application/json') !== false || stripos($value, 'application/problem+json') !== false) { return true; } } return false; }tests/Endpoints/ChatWorkspaceTest.php (1)
26-32: Guard missing MEILISEARCH_API_KEY in tests to aid local runs.If the env var is unset or empty, skip with a clear message (previously requested). This avoids confusing failures when running outside CI.
Apply:
- $http = new Client($this->host, getenv('MEILISEARCH_API_KEY')); + $apiKey = getenv('MEILISEARCH_API_KEY'); + if (!\is_string($apiKey) || '' === $apiKey) { + self::markTestSkipped('MEILISEARCH_API_KEY is not configured for chat tests.'); + } + $http = new Client($this->host, $apiKey);
🧹 Nitpick comments (4)
src/Http/Client.php (1)
143-157: Streaming POST: set Accept header and standardize header casing for SSE compatibility.Most SSE chat-completions endpoints expect Accept: text/event-stream; adding it improves interoperability. Also use canonical header casing.
Apply:
- return $this->executeStream($request, ['Content-type' => 'application/json']); + return $this->executeStream($request, [ + 'Content-Type' => 'application/json', + 'Accept' => 'text/event-stream', + ]);src/Contracts/ChatWorkspaceSettings.php (1)
100-106: Mirror the phpdoc fix in toArray() for static analysis consistency.Return-type shape should allow null for prompts.
Apply:
- * prompts?: array{ + * prompts?: array{ * system: string, * searchDescription: string, * searchQParam: string, * searchIndexUidParam: string - * } + * }|nulltests/Endpoints/ChatWorkspaceTest.php (2)
79-82: Make list assertion resilient to unrelated workspaces.Exact-equality is brittle if the backend contains more than one workspace. Assert presence instead.
Apply:
- self::assertSame([ - ['uid' => 'myWorkspace'], - ], $response->getResults()); + self::assertContains(['uid' => 'myWorkspace'], $response->getResults());
128-149: Reduce flakiness in streaming test by time-bounding instead of chunk-count failing.SSE streams may keep connections open; failing on chunk count can be flaky. Prefer a short deadline and exit when some data is received.
Apply:
- while (!$stream->eof() && $chunkCount < $maxChunks) { + $deadline = \microtime(true) + 15.0; // 15s safety deadline + while (!$stream->eof() && \microtime(true) < $deadline) { $chunk = $stream->read(8192); if ('' === $chunk) { // Small backoff to avoid tight loop on empty reads usleep(10_000); continue; } $receivedData .= $chunk; ++$chunkCount; } - - if ($chunkCount >= $maxChunks) { - self::fail('Test exceeded maximum chunk limit of '.$maxChunks); - } + // If the stream never closed, we still assert we received something within the deadline.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
src/Contracts/ChatWorkspacePromptsSettings.php(1 hunks)src/Contracts/ChatWorkspaceSettings.php(1 hunks)src/Contracts/ChatWorkspacesResults.php(1 hunks)src/Http/Client.php(3 hunks)tests/Endpoints/ChatWorkspaceTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Contracts/ChatWorkspacesResults.php
🧰 Additional context used
🧬 Code graph analysis (4)
src/Contracts/ChatWorkspacePromptsSettings.php (2)
src/Contracts/Data.php (1)
Data(7-50)src/Contracts/ChatWorkspaceSettings.php (2)
__construct(37-49)toArray(108-120)
src/Contracts/ChatWorkspaceSettings.php (2)
src/Contracts/ChatWorkspacePromptsSettings.php (3)
ChatWorkspacePromptsSettings(7-69)__construct(22-30)toArray(60-68)src/Contracts/Data.php (1)
Data(7-50)
tests/Endpoints/ChatWorkspaceTest.php (6)
src/Http/Client.php (2)
Client(26-262)patch(123-131)tests/TestCase.php (1)
TestCase(16-180)src/Endpoints/ChatWorkspaces.php (2)
workspace(33-36)listWorkspaces(26-31)src/Endpoints/Delegates/HandlesChatWorkspaceSettings.php (4)
updateSettings(39-48)getSettings(14-23)resetSettings(53-62)streamCompletion(73-80)src/Contracts/ChatWorkspaceSettings.php (8)
getSource(51-54)getOrgId(56-59)getProjectId(61-64)getApiVersion(66-69)getDeploymentId(71-74)getBaseUrl(76-79)getPrompts(86-89)getApiKey(81-84)src/Contracts/ChatWorkspacePromptsSettings.php (4)
getSystem(32-35)getSearchDescription(37-40)getSearchQParam(42-45)getSearchIndexUidParam(47-50)
src/Http/Client.php (6)
src/Contracts/Http.php (1)
postStream(54-54)src/Http/Serialize/SerializerInterface.php (2)
serialize(21-21)unserialize(30-30)src/Http/Serialize/Json.php (2)
serialize(15-24)unserialize(26-35)src/Exceptions/JsonDecodingException.php (1)
JsonDecodingException(7-13)src/Exceptions/ApiException.php (1)
ApiException(9-95)src/Exceptions/CommunicationException.php (1)
CommunicationException(7-13)
🪛 GitHub Check: phpstan-tests
src/Contracts/ChatWorkspaceSettings.php
[failure] 48-48:
Strict comparison using === between array{system: string, searchDescription: string, searchQParam: string, searchIndexUidParam: string} and null will always evaluate to false.
[failure] 18-18:
Property Meilisearch\Contracts\ChatWorkspaceSettings::$prompts (Meilisearch\Contracts\ChatWorkspacePromptsSettings|null) is never assigned null so it can be removed from the property type.
🪛 GitHub Actions: Tests
src/Contracts/ChatWorkspaceSettings.php
[error] 1-1: PHP CS Fixer lint found fixable issues in ChatWorkspaceSettings.php: remove unused import and adjust null checks (yoda_style, no_unused_imports, no_extra_blank_lines). Dry-run indicated 1 file can be fixed. Run 'vendor/bin/php-cs-fixer fix --config=.php-cs-fixer.dist.php' to apply fixes.
🔇 Additional comments (2)
src/Contracts/ChatWorkspacePromptsSettings.php (1)
7-69: DTO looks solid and matches usage (array-access + getters).Constructor initializes all required fields; toArray mirrors shape used by settings. No blockers.
src/Contracts/ChatWorkspaceSettings.php (1)
1-121: Use Composer’s lint scripts to fix coding style issuesThe project defines CS-Fixer commands in composer.json under the
lintandlint:fixscripts, so rather than invokingvendor/bin/php-cs-fixerdirectly (which isn’t present until dependencies are installed), run:
- To preview fixable issues:
composer lint- To apply fixes across the codebase (or a specific file):
composer lint:fix # or to target only ChatWorkspaceSettings.php: composer lint:fix -- src/Contracts/ChatWorkspaceSettings.phpThis will resolve the reported
no_unused_imports,yoda_style, andno_extra_blank_lineserrors insrc/Contracts/ChatWorkspaceSettings.php.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/Contracts/ChatWorkspaceSettings.php (4)
7-16: Make prompts nullable (constructor and property) to allow PATCHes without prompts.Some PATCH/GET flows won’t always include prompts; forcing a non-null DTO here is brittle and contradicts prior discussion. Make the property nullable with a sensible default.
- private ChatWorkspacePromptsSettings $prompts; + private ?ChatWorkspacePromptsSettings $prompts = null;
18-34: Docblock: allow prompts to be omitted or null.The current phpdoc requires
prompts, but the API can accept settings without prompts. Update the shape accordingly.- * apiKey?: string, - * prompts: array{ + * apiKey?: string, + * prompts?: array{ * system: string, * searchDescription: string, * searchQParam: string, * searchIndexUidParam: string - * } + * }|null
35-47: Constructor: guard prompts access; construct DTO only when present and non-null.Prevents undefined-index notices and aligns with the updated docblock.
- $this->prompts = new ChatWorkspacePromptsSettings($params['prompts']); + $this->prompts = (isset($params['prompts']) && null !== $params['prompts']) + ? new ChatWorkspacePromptsSettings($params['prompts']) + : null;
5-8: Unused same-namespace import is gone — CS issue addressed.Previous CS failure about importing
ChatWorkspacePromptsSettingsfrom the same namespace is resolved here. Thanks for cleaning it up.
🧹 Nitpick comments (5)
src/Contracts/ChatWorkspaceSettings.php (5)
84-87: Getter should return nullable type if prompts is optional.Keeps public API consistent with constructor and property.
- public function getPrompts(): ChatWorkspacePromptsSettings + public function getPrompts(): ?ChatWorkspacePromptsSettings
89-105: toArray() phpdoc: optional keys vs. nulls mismatch.The docblock marks optional keys with
?:, but implementation currently returns keys with null values. Decide on one behavior; if you filter nulls (recommended), keep?:in phpdoc. If not filtering, change tokey: string|null. Suggest adopting the filtering approach below and keeping?:.- * apiKey?: string, - * prompts: array{ + * apiKey?: string, + * prompts?: array{ * system: string, * searchDescription: string, * searchQParam: string, * searchIndexUidParam: string - * } + * }|null
106-118: Omit nulls in serialized payload to avoid unintended resets; include prompts only when set.Sending
"apiKey": null(or other nulls) might be interpreted as clearing a field server-side. Prefer omitting nulls entirely.- return [ - 'source' => $this->source, - 'orgId' => $this->orgId, - 'projectId' => $this->projectId, - 'apiVersion' => $this->apiVersion, - 'deploymentId' => $this->deploymentId, - 'baseUrl' => $this->baseUrl, - 'apiKey' => $this->apiKey, - 'prompts' => $this->prompts->toArray(), - ]; + $result = [ + 'source' => $this->source, + 'orgId' => $this->orgId, + 'projectId' => $this->projectId, + 'apiVersion' => $this->apiVersion, + 'deploymentId' => $this->deploymentId, + 'baseUrl' => $this->baseUrl, + 'apiKey' => $this->apiKey, + ]; + + if (null !== $this->prompts) { + $result['prompts'] = $this->prompts->toArray(); + } + + // Drop nulls to avoid accidental clears on PATCH. + return \array_filter($result, static fn ($v) => null !== $v);
79-82: Sensitive data: avoid accidental logging of apiKey.This DTO is often dumped to logs in client apps. Consider documenting that
toArray()is a request payload and should not be logged verbatim, or add a dedicatedtoRequestArray()that includesapiKeywhiletoArray()redacts it (e.g.,'apiKey' => $this->apiKey !== null ? '***' : null).
39-46: Optional: lightweight validation for requiredsource.If empty strings are invalid for
source, throw anInvalidArgumentExceptionearly; otherwise, add a phpdoc note about accepted values to guide callers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Contracts/ChatWorkspaceSettings.php(1 hunks)tests/Endpoints/ChatWorkspaceTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Endpoints/ChatWorkspaceTest.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Contracts/ChatWorkspaceSettings.php (2)
src/Contracts/Data.php (1)
Data(7-50)src/Contracts/ChatWorkspacePromptsSettings.php (3)
ChatWorkspacePromptsSettings(7-69)__construct(22-30)toArray(60-68)
Co-authored-by: Tomas Norkūnas <norkunas.tom@gmail.com>
Co-authored-by: Tomas Norkūnas <norkunas.tom@gmail.com>
|
Thanks @norkunas for the thorough review 🙌 I think this should finally be good |
| * stream: bool | ||
| * } $options The request body for the chat completion | ||
| */ | ||
| public function streamCompletion(array $options): \Psr\Http\Message\StreamInterface |
There was a problem hiding this comment.
But \Psr\Http\Message\StreamInterface is still not imported, right? :)
There was a problem hiding this comment.
Ugh, sorry, I missed that one 😅
Updated the code!
| public string $system; | ||
| public string $searchDescription; | ||
| /** | ||
| * @var non-empty-string | ||
| */ | ||
| public string $searchQParam; | ||
| /** | ||
| * @var non-empty-string | ||
| */ | ||
| public string $searchIndexUidParam; |
There was a problem hiding this comment.
these should be private, no?
|
|
||
| trait HandlesChatWorkspaces | ||
| { | ||
| public ChatWorkspaces $chats; |
There was a problem hiding this comment.
I tried to design a fluent API and then got lost in refactoring 😅 I fixed this now
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/Endpoints/Delegates/HandlesChatWorkspaces.php (1)
17-28: Public surface: confirm these helpers should be exposed (vs. only via $client->chats).Given Client also exposes $client->chats, verify we want both entry points. If not, consider narrowing visibility or deprecating one.
src/Contracts/ChatWorkspacePromptsSettings.php (2)
7-14: Tighten phpstan shape if empty strings are discouraged.If empty strings are undesirable for system/searchDescription (per prior discussion), declare them as non-empty-string in the phpstan array shape to push earlier feedback during static analysis.
Apply:
* @phpstan-type ChatWorkspacePromptsArray array{ - * system: string, - * searchDescription: string, + * system: non-empty-string, + * searchDescription: non-empty-string, * searchQParam: non-empty-string, * searchIndexUidParam: non-empty-string * }
17-26: Props are private — good alignment with encapsulation.This addresses the earlier comment that these should be private.
🧹 Nitpick comments (7)
src/Endpoints/Delegates/HandlesChatWorkspaces.php (4)
12-12: Document initialization of $chats (and consider readonly if supported).Clarify that the host class (e.g., Client) initializes this property; optional: make it readonly if PHP target allows.
- private ChatWorkspaces $chats; + /** + * Internal: initialized by the Client constructor. + * @internal + */ + private ChatWorkspaces $chats;
17-20: Consider renaming to listChatWorkspaces for API consistency.Matches ChatWorkspaces::listWorkspaces() naming and intent.
- public function getChatWorkspaces(): ChatWorkspacesResults + public function listChatWorkspaces(): ChatWorkspacesResultsNote: verify call sites/tests since this is public API.
17-20: Add @throws annotations consistent with other endpoints.This delegates an HTTP call and may throw transport/API exceptions; document them like in peer methods.
22-27: Strengthen param contract for $workspaceName.Declare non-empty-string in the docblock to match how workspaceName is used elsewhere.
- /** - * Get a specific chat workspace instance. - */ + /** + * Get a specific chat workspace instance. + * + * @param non-empty-string $workspaceName + */src/Contracts/ChatWorkspacePromptsSettings.php (3)
15-16: Prevent divergence between ArrayAccess data and private props.Because Data allows mutation via ArrayAccess, $this->data can get out of sync with typed props. Either don’t store the raw array or block mutation to keep this DTO immutable.
Apply:
class ChatWorkspacePromptsSettings extends Data { + // Keep immutable: block ArrayAccess mutation to avoid divergence with private props + public function offsetSet($offset, $value): void + { + throw new \LogicException('ChatWorkspacePromptsSettings is immutable.'); + } + + public function offsetUnset($offset): void + { + throw new \LogicException('ChatWorkspacePromptsSettings is immutable.'); + }Also applies to: 31-34
41-66: Naming nit: clarify Q param getter.Optional: rename getSearchQParam() to getSearchQueryParam() for clarity; this is new API and still experimental, so low blast radius.
15-26: Usefinalnow; deferreadonlyuntil PHP ≥ 8.1
- composer.json constrains PHP to ^7.4 || ^8.0, so
readonlyisn’t supported yet.- You can mark
class ChatWorkspacePromptsSettingsasfinaltoday.- Once you bump the minimum PHP requirement to ≥ 8.1, convert the private properties to
readonly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/Contracts/ChatWorkspacePromptsSettings.php(1 hunks)src/Endpoints/Delegates/HandlesChatWorkspaces.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/Contracts/ChatWorkspacePromptsSettings.php (2)
src/Contracts/Data.php (1)
Data(7-50)src/Contracts/ChatWorkspaceSettings.php (2)
__construct(56-68)toArray(148-160)
src/Endpoints/Delegates/HandlesChatWorkspaces.php (2)
src/Contracts/ChatWorkspacesResults.php (1)
ChatWorkspacesResults(7-90)src/Endpoints/ChatWorkspaces.php (3)
ChatWorkspaces(12-40)listWorkspaces(29-34)workspace(36-39)
🔇 Additional comments (2)
src/Endpoints/Delegates/HandlesChatWorkspaces.php (1)
1-9: Solid addition; types and structure look consistent.declare(strict_types=1), namespaces, and typed returns align with the codebase patterns.
src/Contracts/ChatWorkspacePromptsSettings.php (1)
70-77: Serialization shape looks correct.toArray() mirrors the declared phpstan shape and upstream ChatWorkspaceSettings expects this array. LGTM.
Pull Request
Related issue
Fixes #765
What does this PR do?
Add methods to interact with chat workspaces endpoints:
PATCH /chats/{workspace}/settings)GET /chats/{workspace}/settings)GET /chats)DELETE /chats/{workspace_uid}/settingsPOST chats/{workspace}/chat/completions)Add methods to interact with index chat settings:
GET /indexes/{index_uid}/settings/chatPUT /indexes/{index_uid}/settings/chatPR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
New Features
Settings
Tests