Merged
Conversation
Closed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new Brand resource and full CRUD API surface to the Smithy model, plus IO and type definitions for Brand fields (ResourceName, ImageUrl) and registers Brand in the InAndOut service resources list. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
smithy/model/brand/brand-apis.smithy (1)
14-28: Consider adding aListBrandsoperation.The Brand resource has full CRUD operations, but there's no
listoperation defined. Clients typically need to retrieve all or paginated brands. If this is intentional for the initial implementation, consider adding it as a follow-up.📋 Example ListBrands operation
// In brand-io.smithy, add: structure ListBrandsInput { `@httpQuery`("maxResults") maxResults: Integer `@httpQuery`("nextToken") nextToken: String } structure ListBrandsOutput { `@required` brands: BrandSummaryList nextToken: String } list BrandSummaryList { member: BrandSummary } // In brand-apis.smithy, add to resource: resource Brand { // ... existing list: ListBrands } `@readonly` `@http`(method: "GET", uri: "/brands") `@paginated`(inputToken: "nextToken", outputToken: "nextToken", pageSize: "maxResults", items: "brands") operation ListBrands { input: ListBrandsInput output: ListBrandsOutput errors: [ InvalidInputError InternalServerError ] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/model/brand/brand-apis.smithy` around lines 14 - 28, The Brand resource lacks a list operation; add a paginated ListBrands operation by defining ListBrandsInput (with maxResults and nextToken as `@httpQuery`), ListBrandsOutput (with required brands and nextToken), a BrandSummary/BrandSummaryList type for list items, and wire it into the Brand resource with list: ListBrands; then declare the `@readonly` `@http` GET /brands operation with `@paginated`(inputToken="nextToken", outputToken="nextToken", pageSize="maxResults", items="brands") and include relevant errors (e.g., InvalidInputError, InternalServerError) so clients can retrieve paginated brand lists.smithy/model/common.smithy (1)
17-19: ImageUrl pattern may reject valid image URLs with query parameters.The current regex requires the URL to end with an image extension (
.jpg,.png, etc.), but many CDN-hosted images include query parameters for versioning or transformations (e.g.,https://cdn.example.com/logo.png?v=123orhttps://cdn.example.com/logo.jpg?width=200). These would fail validation.Also, the minimum length of 8 may be slightly low—even
http://x.jpgis 13 characters.♻️ Suggested pattern to allow query strings
-@pattern("^https?://[a-zA-Z0-9\\-._~:/?#\\[\\]@!$&'()*+,;=%]+\\.(jpg|jpeg|png|gif)$") -@length(min: 8, max: 255) +@pattern("^https?://[a-zA-Z0-9\\-._~:/?#\\[\\]@!$&'()*+,;=%]+\\.(jpg|jpeg|png|gif)(\\?[a-zA-Z0-9\\-._~:/?#\\[\\]@!$&'()*+,;=%]*)?$") +@length(min: 12, max: 255) string ImageUrl🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@smithy/model/common.smithy` around lines 17 - 19, The ImageUrl string type's `@pattern` currently requires the URL to end with an image extension and will reject valid URLs with query strings or fragments; update the `@pattern` on ImageUrl to allow an optional query string and fragment after the extension (ensure the extension like .jpg|.jpeg|.png|.gif appears before any ? or #) and adjust the `@length` annotation to a higher minimum (e.g., min: 13) to avoid too-short URLs; modify the `@pattern` annotation and the `@length`(min: ...) value on the ImageUrl definition accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@smithy/model/brand/brand-apis.smithy`:
- Around line 14-28: The Brand resource lacks a list operation; add a paginated
ListBrands operation by defining ListBrandsInput (with maxResults and nextToken
as `@httpQuery`), ListBrandsOutput (with required brands and nextToken), a
BrandSummary/BrandSummaryList type for list items, and wire it into the Brand
resource with list: ListBrands; then declare the `@readonly` `@http` GET /brands
operation with `@paginated`(inputToken="nextToken", outputToken="nextToken",
pageSize="maxResults", items="brands") and include relevant errors (e.g.,
InvalidInputError, InternalServerError) so clients can retrieve paginated brand
lists.
In `@smithy/model/common.smithy`:
- Around line 17-19: The ImageUrl string type's `@pattern` currently requires the
URL to end with an image extension and will reject valid URLs with query strings
or fragments; update the `@pattern` on ImageUrl to allow an optional query string
and fragment after the extension (ensure the extension like .jpg|.jpeg|.png|.gif
appears before any ? or #) and adjust the `@length` annotation to a higher minimum
(e.g., min: 13) to avoid too-short URLs; modify the `@pattern` annotation and the
`@length`(min: ...) value on the ImageUrl definition accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 136c4eb0-7cbb-4aeb-96d0-e20490586914
📒 Files selected for processing (5)
smithy/model/brand/brand-apis.smithysmithy/model/brand/brand-io.smithysmithy/model/brand/brand-types.smithysmithy/model/common.smithysmithy/model/main.smithy
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@smithy/model/brand/brand-types.smithy`:
- Around line 9-12: Remove the redundant `@resourceIdentifier`("brandId") trait
from the BrandSummary structure: edit the BrandSummary definition and delete the
`@resourceIdentifier` annotation on the brandId member so only `@required` remains
for the brandId field; locate the brandId member in the BrandSummary structure
to make this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 540464fe-973f-42e9-8413-b84fcbeec3f8
📒 Files selected for processing (1)
smithy/model/brand/brand-types.smithy
lefcos
approved these changes
Mar 7, 2026
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.
Summary by CodeRabbit