Typehint Batch results - #928
Open
Strift wants to merge 8 commits into
Open
Conversation
Strift
added
enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBatch endpoints now return immutable, typed ChangesBatch response typing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HandlesBatches
participant Batches
participant Batch
participant BatchesResults
Client->>HandlesBatches: Request batch or batch list
HandlesBatches->>Batches: Fetch batch response
Batches->>Batch: Convert raw payload with Batch::fromArray()
Batch-->>Batches: Return typed Batch objects
Batches->>BatchesResults: Build paginated response
BatchesResults-->>Client: Return typed results and pagination
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Contracts/Batch.php (1)
170-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the
finishedAtpresence check.You can use
isset()to concisely check if a key exists and its value is not null, which simplifies the conditional logic.♻️ Proposed refactor
- \array_key_exists('finishedAt', $data) && null !== $data['finishedAt'] - ? new \DateTimeImmutable($data['finishedAt']) : null, + isset($data['finishedAt']) ? new \DateTimeImmutable($data['finishedAt']) : null,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Contracts/Batch.php` around lines 170 - 171, Update the finishedAt initialization in the Batch construction logic to replace the array_key_exists-and-null comparison with an isset($data['finishedAt']) check, while preserving creation of DateTimeImmutable for non-null values and null otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Contracts/Batch.php`:
- Around line 131-137: Update the PHPDoc for the toArray() method in Batch to
return the existing RawBatch type alias instead of array<mixed>,
preserving the method signature and implementation.
---
Nitpick comments:
In `@src/Contracts/Batch.php`:
- Around line 170-171: Update the finishedAt initialization in the Batch
construction logic to replace the array_key_exists-and-null comparison with an
isset($data['finishedAt']) check, while preserving creation of DateTimeImmutable
for non-null values and null otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7954aad5-95cc-41c3-9863-b58e1889f39d
📒 Files selected for processing (5)
src/Contracts/Batch.phpsrc/Contracts/BatchesResults.phpsrc/Endpoints/Batches.phpsrc/Endpoints/Delegates/HandlesBatches.phptests/Endpoints/BatchesTest.php
Strift
force-pushed
the
php-181-typehint-batches-results
branch
from
July 17, 2026 08:10
472ba25 to
7f37146
Compare
Strift
marked this pull request as ready for review
July 17, 2026 08:10
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Contracts/BatchesResults.php (1)
88-106: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnsure
toArray()returns a fully scalar array instead of objects.The
toArray()method currently assigns$this->datadirectly to theresultskey, which containsBatchobjects. BecauseBatchproperties are private and it does not implementJsonSerializable, callingjson_encode($batchesResults->toArray())will serialize the objects as empty{}structures, resulting in data loss.You should map the
Batchobjects to their raw array representations to ensuretoArray()consistently returns a deeply primitive array.♻️ Proposed fix
/** * `@return` array{ - * results: list<Batch>, + * results: list<RawBatch>, * from: non-negative-int, * limit: non-negative-int, * next: non-negative-int, * total: non-negative-int * } */ public function toArray(): array { return [ - 'results' => $this->data, + 'results' => array_map(fn (Batch $batch) => $batch->toArray(), $this->data), 'next' => $this->next, 'limit' => $this->limit, 'from' => $this->from, 'total' => $this->total, ]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Contracts/BatchesResults.php` around lines 88 - 106, Update BatchesResults::toArray() so the results value maps each Batch object to its raw array representation before returning the array. Preserve the existing pagination fields and ensure the return type documentation reflects a list of scalar array data rather than list<Batch>, allowing json_encode() to retain each batch’s contents.
🧹 Nitpick comments (1)
src/Contracts/Batch.php (1)
16-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using precise array shapes for nested stats.
If the keys for
progressTrace,writeChannelCongestion, andinternalDatabaseSizesare known or bounded, define their specific shapes instead of usingarray<string, mixed>. As per coding guidelines,src/Contracts/**/*.phpshould use precise PHPStan array shapes. If they are intentionally arbitrary or open-ended by the Meilisearch API, you can leave them as is.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Contracts/Batch.php` around lines 16 - 18, Update the PHPStan annotations in the Batch contract for progressTrace, writeChannelCongestion, and internalDatabaseSizes to use precise nested array shapes when their supported keys are known or bounded; retain array<string, mixed> only for fields intentionally open-ended by the Meilisearch API.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Contracts/BatchesResults.php`:
- Around line 88-106: Update BatchesResults::toArray() so the results value maps
each Batch object to its raw array representation before returning the array.
Preserve the existing pagination fields and ensure the return type documentation
reflects a list of scalar array data rather than list<Batch>, allowing
json_encode() to retain each batch’s contents.
---
Nitpick comments:
In `@src/Contracts/Batch.php`:
- Around line 16-18: Update the PHPStan annotations in the Batch contract for
progressTrace, writeChannelCongestion, and internalDatabaseSizes to use precise
nested array shapes when their supported keys are known or bounded; retain
array<string, mixed> only for fields intentionally open-ended by the Meilisearch
API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d563b0b-bbd9-41d9-814d-aa918230a2d1
📒 Files selected for processing (5)
src/Contracts/Batch.phpsrc/Contracts/BatchesResults.phpsrc/Endpoints/Batches.phpsrc/Endpoints/Delegates/HandlesBatches.phptests/Endpoints/BatchesTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Endpoints/BatchesTest.php
- src/Endpoints/Batches.php
norkunas
approved these changes
Jul 17, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Contracts/BatchStats.php`:
- Around line 10-20: The RawBatchStats definition at
src/Contracts/BatchStats.php lines 10-20 and the corresponding constructor
parameter PHPDocs at src/Contracts/BatchStats.php lines 23-31 use array<string,
mixed> for progressTrace, writeChannelCongestion, and internalDatabaseSizes.
Replace each with precise PHPStan array shapes or constrained value types
matching the Meilisearch API specification, and keep the type definitions
consistent between the alias and constructor documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf451bb9-69ef-4513-ad8f-9860bcb09131
📒 Files selected for processing (10)
src/Contracts/Batch.phpsrc/Contracts/BatchEmbedderRequests.phpsrc/Contracts/BatchProgress.phpsrc/Contracts/BatchProgressStep.phpsrc/Contracts/BatchStats.phpsrc/Contracts/BatchesResults.phptests/Contracts/BatchProgressTest.phptests/Contracts/BatchStatsTest.phptests/Contracts/BatchesResultsTest.phptests/Endpoints/BatchesTest.php
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Contracts/Batch.php
- tests/Endpoints/BatchesTest.php
- src/Contracts/BatchesResults.php
Map /batches responses to a Batch contract so getBatch/getBatches return typed objects instead of raw arrays. Co-authored-by: Cursor <cursoragent@cursor.com>
Strift
force-pushed
the
php-181-typehint-batches-results
branch
from
August 4, 2026 06:59
326e076 to
41004c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Contracts/Batch.php`:
- Around line 138-142: Update Batch::fromArray to access the required nullable
duration and progress keys directly, matching the declared RawBatch shape.
Remove optional coalescing for progress while preserving its nullable
BatchProgress::fromArray conversion, and keep optional coalescing only for
batchStrategy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d4eccb4-8320-4d45-906e-7a6081614d45
📒 Files selected for processing (12)
src/Contracts/Batch.phpsrc/Contracts/BatchEmbedderRequests.phpsrc/Contracts/BatchProgress.phpsrc/Contracts/BatchProgressStep.phpsrc/Contracts/BatchStats.phpsrc/Contracts/BatchesResults.phpsrc/Endpoints/Batches.phpsrc/Endpoints/Delegates/HandlesBatches.phptests/Contracts/BatchProgressTest.phptests/Contracts/BatchStatsTest.phptests/Contracts/BatchesResultsTest.phptests/Endpoints/BatchesTest.php
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/Contracts/BatchProgressTest.php
- src/Contracts/BatchEmbedderRequests.php
- src/Contracts/BatchProgress.php
- tests/Contracts/BatchesResultsTest.php
- src/Endpoints/Delegates/HandlesBatches.php
- src/Contracts/BatchStats.php
- tests/Contracts/BatchStatsTest.php
- src/Endpoints/Batches.php
- src/Contracts/BatchesResults.php
- tests/Endpoints/BatchesTest.php
Align duration and progress reads with the RawBatch shape, keeping optional coalescing only for batchStrategy. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Related issue
Fixes #743
What does this PR do?
PR checklist
Please check if your PR fulfills the following requirements:
Thank you so much for contributing to Meilisearch!
Summary by CodeRabbit
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes