spiral-banana-shop#40
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR replaces a dog-shop demonstration application with a banana-shop equivalent. Changes span configuration, routing, home page simplification, new banana domain operations (list, show, buy), a banana controller with three HTTP endpoints, matching request filters, and updated application metadata. Dog-related endpoints, controllers, and response types are removed. ChangesBanana Shop Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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: 4
🤖 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 `@poc/spiral-emit/app/src/Endpoint/Web/BananaController.php`:
- Around line 86-95: The list handler in BananaController::list currently
ignores the BananaListRequest and always returns the full static dataset; update
it to read filter and pagination parameters from the BananaListRequest (e.g.
variety, ripeness, limit, offset or their request getters), apply those filters
to the in-memory bananas array, compute the filtered total, then apply
offset/limit to produce the page, and finally construct BananaListResponse with
the paginated bananas and the filtered total so request filters and pagination
take effect.
- Around line 200-207: In buy(BuyBananaRequest $request): BuyBananaResponse,
replace the incorrect totalPriceCents: $request->budget with a computed total
using the requested quantity multiplied by the per-item price (e.g.
$request->quantity * $request->unitPriceCents or whatever per-unit field exists
on BuyBananaRequest); update the BuyBananaResponse constructor call to pass that
computed value (and if the per-unit price field may be missing, fall back to
using $request->budget or a sensible default). Ensure you reference the buy
method, BuyBananaRequest quantity and unit-price field, and the
BuyBananaResponse totalPriceCents parameter when making the change.
- Around line 154-157: The OpenAPI doc in BananaController does not mark the
required request fields even though the Term declarations (Term('variety'),
Term('quantity'), Term('budget')) are required; update the OA\RequestBody
annotation to include required=true and update the OA\JsonContent annotation to
include required={"variety","quantity","budget"} so the schema matches the Term
requirements and accurately documents input validation.
In
`@poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/InstructionEmitter.php`:
- Around line 27-28: The metadata object in InstructionEmitter (the 'id' and
'comment' fields) has inconsistent capitalization ("Banana Shop" vs "Banana shop
operations"); update one of them so both use the same capitalization convention
— e.g., change "Banana shop operations" to "Banana Shop operations" or change
"Banana Shop" to "Banana shop" — by editing the 'id' and/or 'comment' values in
the InstructionEmitter.php metadata block to match.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f636ac2-7bdd-4935-9889-8855ce5621db
⛔ Files ignored due to path filters (1)
aibraindata/state.dbis excluded by!**/*.db
📒 Files selected for processing (23)
poc/spiral-emit/app/config/cycle.phppoc/spiral-emit/app/config/swagger.phppoc/spiral-emit/app/src/Application/Bootloader/RoutesBootloader.phppoc/spiral-emit/app/src/Endpoint/Web/BananaController.phppoc/spiral-emit/app/src/Endpoint/Web/DogController.phppoc/spiral-emit/app/src/Endpoint/Web/HomeController.phppoc/spiral-emit/app/src/Endpoint/Web/OrderController.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BananaListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BananaShowRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BreedListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BuyBananaRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/DogShowRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderCancelRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderShowRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Response/BananaListResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/BananaShowResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/BreedListResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/BuyBananaResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/DogShowResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/OrderCancelResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/OrderListResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/OrderShowResponse.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/InstructionEmitter.php
💤 Files with no reviewable changes (11)
- poc/spiral-emit/app/src/Endpoint/Web/OrderController.php
- poc/spiral-emit/app/src/Endpoint/Web/Response/OrderShowResponse.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/OrderShowRequest.php
- poc/spiral-emit/app/src/Endpoint/Web/DogController.php
- poc/spiral-emit/app/src/Endpoint/Web/Response/OrderCancelResponse.php
- poc/spiral-emit/app/src/Endpoint/Web/Response/OrderListResponse.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/DogShowRequest.php
- poc/spiral-emit/app/src/Endpoint/Web/Response/DogShowResponse.php
- poc/spiral-emit/app/src/Endpoint/Web/Response/BreedListResponse.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/OrderCancelRequest.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/BreedListRequest.php
Comment on lines
+154
to
+157
| new Term('variety', 'Variety to buy', kind: Kind::String, required: true), | ||
| new Term('quantity', 'Number of bananas', kind: Kind::Integer, required: true), | ||
| new Term('budget', 'Maximum budget in cents', kind: Kind::Integer, required: true), | ||
| new Term('express', 'Express delivery', kind: Kind::Boolean), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "BananaController.php" -type fRepository: thumbrise/op
Length of output: 116
🏁 Script executed:
wc -l ./poc/spiral-emit/app/src/Endpoint/Web/BananaController.phpRepository: thumbrise/op
Length of output: 120
🏁 Script executed:
sed -n '150,190p' ./poc/spiral-emit/app/src/Endpoint/Web/BananaController.phpRepository: thumbrise/op
Length of output: 2012
Ensure required fields are marked in the OpenAPI request schema.
The Term declarations at lines 154-157 correctly mark variety, quantity, and budget as required. However, the OpenAPI schema at lines 172-183 does not mark these fields as required in either the RequestBody or JsonContent definition.
Update the OpenAPI schema to include:
required: trueon theOA\RequestBodyrequired: ['variety', 'quantity', 'budget']on theOA\JsonContent
This ensures the OpenAPI documentation accurately reflects the input validation requirements.
🤖 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 `@poc/spiral-emit/app/src/Endpoint/Web/BananaController.php` around lines 154 -
157, The OpenAPI doc in BananaController does not mark the required request
fields even though the Term declarations (Term('variety'), Term('quantity'),
Term('budget')) are required; update the OA\RequestBody annotation to include
required=true and update the OA\JsonContent annotation to include
required={"variety","quantity","budget"} so the schema matches the Term
requirements and accurately documents input validation.
Comment on lines
+27
to
+28
| id: 'Banana Shop', | ||
| comment: 'Banana shop operations', |
There was a problem hiding this comment.
Inconsistent capitalization of "Shop" in metadata fields.
The id field uses "Banana Shop" (capital 'S') while the comment field uses "Banana shop operations" (lowercase 's'). For consistency, both should use the same capitalization style.
📝 Proposed fix for consistency
- id: 'Banana Shop',
- comment: 'Banana shop operations',
+ id: 'Banana Shop',
+ comment: 'Banana Shop operations',📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| id: 'Banana Shop', | |
| comment: 'Banana shop operations', | |
| id: 'Banana Shop', | |
| comment: 'Banana Shop operations', |
🤖 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
`@poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/InstructionEmitter.php`
around lines 27 - 28, The metadata object in InstructionEmitter (the 'id' and
'comment' fields) has inconsistent capitalization ("Banana Shop" vs "Banana shop
operations"); update one of them so both use the same capitalization convention
— e.g., change "Banana shop operations" to "Banana Shop operations" or change
"Banana Shop" to "Banana shop" — by editing the 'id' and/or 'comment' values in
the InstructionEmitter.php metadata block to match.
thumbrise
force-pushed
the
spiral-banana-shop
branch
from
1ee90a3 to
70462ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@poc/spiral-emit/app/src/Endpoint/Web/BananaController.php`:
- Around line 106-107: The Term 'includeNutrition' is declared in the endpoint
metadata but never used in BananaController's handler and not present in the
response schema; either implement it or remove it (same issue repeats around the
block at 143-152). Fix by updating the BananaController action that handles this
endpoint to read the boolean includeNutrition from the incoming request (use the
same parameter name declared in Term('includeNutrition', ...)), and when true,
populate the response with a nutrition object/fields (and update the response
Term/schema to include that nutrition structure); alternatively remove the
Term('includeNutrition', ...) entries at both occurrences if you choose not to
support that option.
In `@poc/spiral-emit/app/src/Endpoint/Web/Request/BananaListRequest.php`:
- Around line 13-14: The property on BananaListRequest currently uses the
#[Path] attribute but should be bound from the query string; change the
attribute on the public ?string $variety property from #[Path] to #[Query] so
the /bananas list endpoint populates the variety filter from the query
parameters rather than a (nonexistent) path segment.
In `@poc/spiral-emit/app/src/Endpoint/Web/Response/BananaListResponse.php`:
- Line 10: The `@param` for $bananas documents a single nested shape instead of an
array of shapes; update the PHPDoc on the BananaListResponse constructor/method
parameter $bananas to an array-of-shapes form such as "`@param` array<int,
array{id: string, variety: string, ripeness: string, origin: string}> $bananas"
(or an equivalent "array<array{id:..., ...}>"/ "array{...}[]" accepted by your
static analyzer) so the docblock correctly represents a list of banana objects.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 96f15098-e76e-450e-92d1-6bf1b5e9f22b
⛔ Files ignored due to path filters (1)
aibraindata/state.dbis excluded by!**/*.db
📒 Files selected for processing (24)
poc/spiral-emit/app/config/cycle.phppoc/spiral-emit/app/config/swagger.phppoc/spiral-emit/app/src/Application/Bootloader/RoutesBootloader.phppoc/spiral-emit/app/src/Endpoint/Web/BananaController.phppoc/spiral-emit/app/src/Endpoint/Web/DogController.phppoc/spiral-emit/app/src/Endpoint/Web/HomeController.phppoc/spiral-emit/app/src/Endpoint/Web/OrderController.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BananaListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BananaShowRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BreedListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BuyBananaRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/DogShowRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderCancelRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderShowRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Response/BananaListResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/BananaShowResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/BreedListResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/BuyBananaResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/DogShowResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/OrderCancelResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/OrderListResponse.phppoc/spiral-emit/app/src/Endpoint/Web/Response/OrderShowResponse.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/InstructionEmitter.php
💤 Files with no reviewable changes (12)
- poc/spiral-emit/app/src/Endpoint/Web/Request/OrderListRequest.php
- poc/spiral-emit/app/src/Endpoint/Web/Response/OrderCancelResponse.php
- poc/spiral-emit/app/src/Endpoint/Web/DogController.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/OrderShowRequest.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/BreedListRequest.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/DogShowRequest.php
- poc/spiral-emit/app/src/Endpoint/Web/Response/BreedListResponse.php
- poc/spiral-emit/app/src/Endpoint/Web/Response/DogShowResponse.php
- poc/spiral-emit/app/src/Endpoint/Web/OrderController.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/OrderCancelRequest.php
- poc/spiral-emit/app/src/Endpoint/Web/Response/OrderShowResponse.php
- poc/spiral-emit/app/src/Endpoint/Web/Response/OrderListResponse.php
✅ Files skipped from review due to trivial changes (4)
- poc/spiral-emit/app/config/cycle.php
- poc/spiral-emit/app/src/Application/Bootloader/RoutesBootloader.php
- poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/InstructionEmitter.php
- poc/spiral-emit/app/config/swagger.php
Comment on lines
+13
to
+14
| #[Path] | ||
| public ?string $variety = null; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify request binding and route shape + interface symbol availability.
rg -n -C2 'class BananaListRequest|#\[Path\]|#\[Query\]|FilterInterface' poc/spiral-emit/app/src/Endpoint/Web/Request/BananaListRequest.php
rg -n -C2 'function list|Route\(route:\s*'\''/bananas'\''' poc/spiral-emit/app/src/Endpoint/Web/BananaController.php
fd -i 'FilterInterface.php'
rg -n -C2 'namespace Spiral\\Filters|interface FilterInterface' $(fd -i 'FilterInterface.php')Repository: thumbrise/op
Length of output: 856
Change #[Path] to #[Query] for the variety filter parameter.
The list endpoint route is /bananas (no path parameter), so variety should be populated from query string, not from path. The current #[Path] attribute won't populate this filter.
Suggested fix
- #[Path]
+ #[Query]
public ?string $variety = null;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[Path] | |
| public ?string $variety = null; | |
| #[Query] | |
| public ?string $variety = 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 `@poc/spiral-emit/app/src/Endpoint/Web/Request/BananaListRequest.php` around
lines 13 - 14, The property on BananaListRequest currently uses the #[Path]
attribute but should be bound from the query string; change the attribute on the
public ?string $variety property from #[Path] to #[Query] so the /bananas list
endpoint populates the variety filter from the query parameters rather than a
(nonexistent) path segment.
| final class BananaListResponse | ||
| { | ||
| /** | ||
| * @param array{array{id: string, variety: string, ripeness: string, origin: string}} $bananas |
There was a problem hiding this comment.
Fix the @param array shape for $bananas.
Line 10 currently documents a single nested shape, not a list. Use a list/array-of-shapes annotation to match actual payload.
Suggested fix
- * `@param` array{array{id: string, variety: string, ripeness: string, origin: string}} $bananas
+ * `@param` list<array{id: string, variety: string, ripeness: string, origin: string}> $bananas🤖 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 `@poc/spiral-emit/app/src/Endpoint/Web/Response/BananaListResponse.php` at line
10, The `@param` for $bananas documents a single nested shape instead of an array
of shapes; update the PHPDoc on the BananaListResponse constructor/method
parameter $bananas to an array-of-shapes form such as "`@param` array<int,
array{id: string, variety: string, ripeness: string, origin: string}> $bananas"
(or an equivalent "array<array{id:..., ...}>"/ "array{...}[]" accepted by your
static analyzer) so the docblock correctly represents a list of banana objects.
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
1 participant
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.
Summary by CodeRabbit
Release Notes
New Features
Removed Features
Updates