Item nav order APIs update#3916
Merged
sumerjabri merged 12 commits intoJun 9, 2026
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR migrates content item ordering from v1 (legacy neighbor-based persistence via ChangesContent ordering v1 to v2 migration
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 7
🧹 Nitpick comments (1)
src/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.java (1)
31-36: Consider allowing optional boundary anchors to support first/last position reorders.The current requirement for both
pathBeforeandpathAfterto be non-empty prevents reordering items to first or last position, where one anchor is naturally absent. When implementing the reorder service, consider making one anchor optional and validating that at least one is provided.Optional DTO adjustment for future implementation
import jakarta.validation.constraints.NotEmpty; +import jakarta.validation.constraints.AssertTrue; import org.craftercms.commons.validation.annotations.param.ValidExistingContentPath; +import org.springframework.util.StringUtils; @@ `@NotEmpty` `@ValidExistingContentPath` private String parentPath; - `@NotEmpty` + `@ValidExistingContentPath` private String pathBefore; - `@NotEmpty` + `@ValidExistingContentPath` private String pathAfter; + + `@AssertTrue`(message = "Either pathBefore or pathAfter must be provided") + public boolean hasAnchor() { + return StringUtils.hasText(pathBefore) || StringUtils.hasText(pathAfter); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.java` around lines 31 - 36, Rework the ReorderItemRequest DTO so reordering can target first/last: remove the `@NotEmpty` requirement from at least one of the two fields (pathBefore and pathAfter) and add a class-level validation that enforces "at least one anchor present" (e.g., implement a custom validator annotation like `@AtLeastOneAnchor` or add an isValid() method used by a class-level `@AssertTrue`) that checks (pathBefore != null && !pathBefore.isEmpty()) || (pathAfter != null && !pathAfter.isEmpty()); keep or adapt `@ValidExistingContentPath` checks to only run when the corresponding field is non-empty. Ensure validators reference the ReorderItemRequest class and the pathBefore/pathAfter fields so the service handling reorders can accept null/empty for one anchor but not both.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/api/studio-api.yaml`:
- Around line 5482-5483: The schema property nextOrder is declared as type
"integer" which will reject valid fractional ordering values; update the OpenAPI
schema entries declaring nextOrder (and any identical order-like fields at the
other occurrence) to use type "number" instead of "integer" so calculated or
between-item order values are accepted; locate the nextOrder definitions in the
YAML (the shown block and the similar block at the other occurrence) and change
their type to "number" while preserving any existing descriptions or
constraints.
- Around line 5475-5491: The response objects for these new TODO endpoints are
missing an interim 501 Not Implemented entry; update the response blocks that
currently list '200', '400', '401', '404', and '500' (including the shown block
and the two other new endpoint response blocks introduced alongside it) by
adding a '501' response entry that references the shared NotImplemented response
(e.g., "501": $ref: '#/components/responses/NotImplemented') so clients see the
interim behavior.
- Line 5460: The OpenAPI spec has a duplicate operationId "getNextItemOrder"
(declared twice), which breaks codegen; rename one of the operations to a unique
identifier (e.g., "getNextItemOrderV2" or a more specific name like
"getNextItemOrderForCollection") and update any references that rely on that
operationId (generated client method names, $ref usages, or tooling configs) so
they point to the new name; ensure the operationId change is applied in the
operation object where "operationId: getNextItemOrder" is declared and verify
downstream codegen and tests compile against the new identifier.
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java`:
- Around line 376-377: The endpoint mappings in the controller are out of sync
with the OpenAPI spec; update the mapping constants and `@GetMapping/`@PostMapping
annotations so the routes use the spec's paths (e.g., "/item/next-order",
"/item/orders", "/item/reorder") instead of the current ".../order/..."
variants: rename or adjust GET_ITEM_NEXT_ORDER and its handler getNextItemOrder,
the corresponding constant and handler for the item orders endpoint (referenced
around the getItemOrders method), and the reorder endpoint constant/handler
(around the reorderItems/reorder method) so each mapping and constant exactly
match the OpenAPI contract.
- Around line 376-380: The three REST handlers currently returning null (e.g.,
getNextItemOrder) must call the appropriate content service methods to compute
the values and return them wrapped in a Result payload instead of null; locate
getNextItemOrder (and the two adjacent unimplemented methods referenced) and
replace the TODO/null with calls to your domain service (e.g.,
contentService.getNextItemOrder(siteId, parentPath) or the equivalent method
names used in this class), handle/translate any checked exceptions into proper
error Results, and return Result.success(theComputedValue) (or the project’s
standard Result factory) so the endpoints produce valid responses.
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java`:
- Around line 70-72: The path constants GET_ITEM_NEXT_ORDER, GET_ITEMS_ORDER,
and REORDER_ITEM currently use "/order" segments but must match the OpenAPI v2
spec; update the constant values in RequestMappingConstants to SITE_ID +
"/item/next-order", SITE_ID + "/item/orders", and SITE_ID + "/item/reorder"
respectively so the controller-registered routes (GET_ITEM_NEXT_ORDER,
GET_ITEMS_ORDER, REORDER_ITEM) align with the published API paths.
In
`@src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java`:
- Around line 529-531: The method updateNavOrder(String siteId, String path,
Document document) currently returns false unconditionally which silently allows
stale nav-order data; change it to fail fast by throwing an
UnsupportedOperationException (or IllegalStateException) with a clear message
like "updateNavOrder not implemented — legacy nav-order logic removed" inside
ContentServiceInternalImpl.updateNavOrder so callers (page writes/copies and
updateNavOrderForMove) cannot proceed silently; ensure the exception text names
updateNavOrder and suggests the replacement work is required.
---
Nitpick comments:
In
`@src/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.java`:
- Around line 31-36: Rework the ReorderItemRequest DTO so reordering can target
first/last: remove the `@NotEmpty` requirement from at least one of the two fields
(pathBefore and pathAfter) and add a class-level validation that enforces "at
least one anchor present" (e.g., implement a custom validator annotation like
`@AtLeastOneAnchor` or add an isValid() method used by a class-level `@AssertTrue`)
that checks (pathBefore != null && !pathBefore.isEmpty()) || (pathAfter != null
&& !pathAfter.isEmpty()); keep or adapt `@ValidExistingContentPath` checks to only
run when the corresponding field is non-empty. Ensure validators reference the
ReorderItemRequest class and the pathBefore/pathAfter fields so the service
handling reorders can accept null/empty for one anchor but not both.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac1fffea-e171-4114-974f-f75a65826175
📒 Files selected for processing (23)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.javasrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xmlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovysrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
💤 Files with no reviewable changes (17)
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.java
- src/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovy
- src/main/resources/crafter/studio/database-context.xml
- src/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.java
- src/main/resources/crafter/studio/studio-services-context.xml
- src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
- src/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovy
- src/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xml
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.java
jmendeza
force-pushed
the
feature/6061-item-orders
branch
from
fa7060e to
b9e01ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/main/api/studio-api.yaml`:
- Around line 5345-5353: The 200 responses return bare payload fields (e.g.,
nextOrder, order and array item schemas) instead of the v2 ApiResponse envelope;
update each affected response schema (the ones returning nextOrder and the
array/item responses referenced around the diff) to wrap the existing schema
under a top-level object property named response (i.e., replace the current
schema with an object that has properties.response equal to the original schema
and preserves type/description), so clients receive { response:
<originalPayload> } consistent with ApiResponse; ensure the referenced fields
nextOrder and any array item schemas are moved unchanged into response.
In
`@src/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.java`:
- Around line 31-36: ReorderItemRequest currently requires both pathBefore and
pathAfter which prevents boundary moves; decide if boundary reorders are
allowed, then either (A) if boundaries supported: remove the `@NotEmpty`
constraint from pathBefore/pathAfter (make them nullable) and add a class-level
validator (e.g., an `@Constraint` on ReorderItemRequest or an `@AssertTrue` boolean
method) that enforces the business rule (at least one present or exactly one
present depending on the requirement) and update the reorderItem endpoint
contract to reflect the rule, or (B) if boundaries are not supported: keep
`@NotEmpty` but add documentation to the reorderItem API contract stating both
neighbors are required. Ensure validators reference
ReorderItemRequest.pathBefore and .pathAfter and the endpoint reorderItem in
tests/contract docs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b14727f-70cd-4317-9296-34bb814ef90f
📒 Files selected for processing (23)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.javasrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xmlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovysrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
💤 Files with no reviewable changes (17)
- src/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovy
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.java
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.java
- src/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xml
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
- src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
- src/main/resources/crafter/studio/database-context.xml
- src/main/resources/crafter/studio/studio-services-context.xml
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java
- src/main/webapp/default-site/scripts/api/ServiceFactory.groovy
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
jmendeza
force-pushed
the
feature/6061-item-orders
branch
4 times, most recently
from
a55aa73 to
a751208
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java (1)
379-387:⚠️ Potential issue | 🔴 CriticalEndpoints are still exposed as stubs returning
null.Both
getItemsOrder(...)andreorderItem(...)are mapped but unimplemented, so these APIs currently return invalid responses instead of aResultpayload backed by service logic.🤖 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/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java` around lines 379 - 387, Implement the two stub endpoints to delegate to the content service and return a proper Result: in getItemsOrder(String siteId, String parentPath) call the service method that retrieves item order (e.g., contentService.getItemsOrder(siteId, parentPath) or equivalent), wrap the returned data into a successful Result payload and return it; in reorderItem(String siteId, ReorderItemRequest request) call the service method that applies the reorder (e.g., contentService.reorderItem(siteId, request)), handle validation/exceptions as before in this controller, and return a Result indicating success (or the updated order) instead of null; keep existing annotations (`@ValidSiteId`, `@ValidExistingContentPath`, `@Valid`) and reuse the controller’s standard Result creation pattern/methods for consistent response shape.src/main/api/studio-api.yaml (2)
5414-5415:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn
orderasnumber(double), notinteger.At Line 5415,
orderis declared asinteger, which can reject valid fractional ordering values produced by reorder calculations.Suggested fix
order: - type: integer + type: number + format: double🤖 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/main/api/studio-api.yaml` around lines 5414 - 5415, The OpenAPI schema property named "order" is currently declared as type: integer which will reject fractional reorder values; change the "order" property in the studio-api YAML schema from type: integer to type: number and (optionally) add format: double so fractional ordering values are accepted (update the property definition for "order" accordingly).
5349-5356:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a top-level v2 response envelope instead of per-item
response.At Line 5350 the 200 schema is an array, and at Line 5354 each item contains
response. This makes the API contract inconsistent with the v2 envelope pattern used elsewhere ({ response, ... }) and complicates generated clients.Suggested minimal shape fix
'200': description: OK content: application/json: schema: - type: array - items: - type: object - properties: - response: - $ref: '#/components/schemas/ApiResponse' - path: - type: string - order: - type: number - format: double - label: - type: string + type: object + properties: + response: + $ref: '#/components/schemas/ApiResponse' + items: + type: array + items: + type: object + properties: + path: + type: string + order: + type: number + format: double + label: + type: string🤖 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/main/api/studio-api.yaml` around lines 5349 - 5356, The 200 response currently defines an array whose items each contain a per-item "response" field (referenced as ApiResponse), which violates the v2 top-level envelope; change the schema so the operation returns a single object with a top-level "response" property whose value is the array of item objects. Concretely, replace the existing schema: type: array / items: { type: object / properties: { response: $ref: '#/components/schemas/ApiResponse', ... } } with schema: type: object / properties: { response: { type: array, items: { type: object, properties: { /* move item properties here except per-item response */ } } } } so ApiResponse is referenced at the top-level "response" instead of per-item.
🤖 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.
Duplicate comments:
In `@src/main/api/studio-api.yaml`:
- Around line 5414-5415: The OpenAPI schema property named "order" is currently
declared as type: integer which will reject fractional reorder values; change
the "order" property in the studio-api YAML schema from type: integer to type:
number and (optionally) add format: double so fractional ordering values are
accepted (update the property definition for "order" accordingly).
- Around line 5349-5356: The 200 response currently defines an array whose items
each contain a per-item "response" field (referenced as ApiResponse), which
violates the v2 top-level envelope; change the schema so the operation returns a
single object with a top-level "response" property whose value is the array of
item objects. Concretely, replace the existing schema: type: array / items: {
type: object / properties: { response: $ref: '#/components/schemas/ApiResponse',
... } } with schema: type: object / properties: { response: { type: array,
items: { type: object, properties: { /* move item properties here except
per-item response */ } } } } so ApiResponse is referenced at the top-level
"response" instead of per-item.
In
`@src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java`:
- Around line 379-387: Implement the two stub endpoints to delegate to the
content service and return a proper Result: in getItemsOrder(String siteId,
String parentPath) call the service method that retrieves item order (e.g.,
contentService.getItemsOrder(siteId, parentPath) or equivalent), wrap the
returned data into a successful Result payload and return it; in
reorderItem(String siteId, ReorderItemRequest request) call the service method
that applies the reorder (e.g., contentService.reorderItem(siteId, request)),
handle validation/exceptions as before in this controller, and return a Result
indicating success (or the updated order) instead of null; keep existing
annotations (`@ValidSiteId`, `@ValidExistingContentPath`, `@Valid`) and reuse the
controller’s standard Result creation pattern/methods for consistent response
shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6c55bed0-f18b-4dca-8965-7c0e4157c51f
📒 Files selected for processing (23)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.javasrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xmlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovysrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
💤 Files with no reviewable changes (17)
- src/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.java
- src/main/resources/crafter/studio/database-context.xml
- src/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovy
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java
- src/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovy
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.java
- src/main/resources/crafter/studio/studio-services-context.xml
- src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
- src/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.java
- src/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovy
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xml
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/org/craftercms/studio/model/rest/content/ReorderItemRequest.java
- src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/default-site/scripts/api/ServiceFactory.groovy
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
jmendeza
force-pushed
the
feature/6061-item-orders
branch
from
a751208 to
e577469
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/main/api/studio-api.yaml`:
- Around line 10750-10792: The base schema ReorderItemRequestBase currently
permits all three type values which allows invalid combinations; restrict the
subtype schemas by adding a concrete type enum in each allOf: update
ReorderItemReferencePathRequest to include a properties.type enum limited to the
reference-path variants (e.g., ["addBefore","addAfter"]) and update
ReorderItemInsertBetweenRequest to include properties.type enum
["insertBetween"] (or equivalent values you use for insertBetween), so each
derived schema narrows the allowed type and enforces correct discriminator
behavior.
In
`@src/main/java/org/craftercms/studio/model/rest/content/order/ReorderItemRequest.java`:
- Around line 34-35: Replace the `@NotEmpty` annotation with `@NotBlank` on all path
fields in the ReorderItemRequest class so whitespace-only strings are rejected
up-front; specifically update the annotations on parentPath, referencePath,
previousPath, nextPath (and the fifth path field annotated similarly) to use
javax.validation.constraints.NotBlank instead of NotEmpty so constraint
validation returns a 400 for blank paths before reaching
`@ValidExistingContentPath`.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50430f18-2688-4255-8792-2fde9650fdcf
📒 Files selected for processing (23)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.javasrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/rest/content/order/ReorderItemRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xmlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovysrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
💤 Files with no reviewable changes (17)
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequence.java
- src/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.java
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovy
- src/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovy
- src/main/resources/crafter/studio/database-context.xml
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.java
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
- src/main/resources/crafter/studio/studio-services-context.xml
- src/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xml
- src/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovy
- src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.java
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java
- src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java
- src/main/webapp/default-site/scripts/api/ServiceFactory.groovy
jmendeza
force-pushed
the
feature/6061-item-orders
branch
2 times, most recently
from
cc20aad to
816c727
Compare
jmendeza
force-pushed
the
feature/6061-item-orders
branch
from
816c727 to
f2a4381
Compare
jmendeza
force-pushed
the
feature/6061-item-orders
branch
from
3b00094 to
506cf21
Compare
Member
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java`:
- Line 2197: The move operation calls updateNavOrderForCopyOrMove(...) with the
wrong root path: change the argument passed from sourcePath to repoPath in
moveInternal(...) so updateNavOrderForCopyOrMove(repoPath, sourcePath,
lifecycleContents, sourcePathChildren) uses the destination root for matching;
also apply the same correction at the other similar call site (around the block
corresponding to lines 2409-2424) so both move-related invocations pass repoPath
as the target root parameter.
- Around line 997-1002: The parent-equality check in reorderItem is executed
even when previousPath or nextPath is null (add-before/add-after cases), causing
InvalidParametersException; fix by only computing/ comparing parents when both
previousPath and nextPath are non-null: move or guard the calls to
getParentUrl(previousPath)/getParentUrl(nextPath) and the CS.equals check behind
a conditional like "if (previousPath != null && nextPath != null) { ... }" (or
equivalently check previousParent/nextParent for null) so add-before/add-after
requests are allowed; update the code in ContentServiceInternalImpl.reorderItem
accordingly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 872fb5e0-a91f-4713-b9bf-4541c339ea38
📒 Files selected for processing (35)
src/main/api/studio-api.yamlsrc/main/java/org/craftercms/studio/api/v1/constant/StudioConstants.javasrc/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.javasrc/main/java/org/craftercms/studio/api/v1/exception/ContentNotFoundException.javasrc/main/java/org/craftercms/studio/api/v1/service/content/ContentService.javasrc/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.javasrc/main/java/org/craftercms/studio/api/v2/service/content/ContentService.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ContentController.javasrc/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.javasrc/main/java/org/craftercms/studio/controller/rest/v2/ResultConstants.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v1/util/ContentUtils.javasrc/main/java/org/craftercms/studio/impl/v2/repository/GitContentRepositoryImpl.javasrc/main/java/org/craftercms/studio/impl/v2/repository/blob/BlobAwareContentRepository.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.javasrc/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javasrc/main/java/org/craftercms/studio/model/rest/content/order/ItemOrder.javasrc/main/java/org/craftercms/studio/model/rest/content/order/ReorderItemRequest.javasrc/main/resources/crafter/studio/database-context.xmlsrc/main/resources/crafter/studio/database/createDDL.sqlsrc/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.18-to-5.0.0.19.sqlsrc/main/resources/crafter/studio/studio-services-context.xmlsrc/main/resources/crafter/studio/upgrade/pipelines.yamlsrc/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xmlsrc/main/webapp/default-site/scripts/api/ContentServices.groovysrc/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/api/ServiceFactory.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovysrc/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovysrc/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovysrc/test/java/org/craftercms/studio/impl/v1/util/ContentUtilsTest.javasrc/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
💤 Files with no reviewable changes (17)
- src/test/java/org/craftercms/studio/impl/v1/util/ContentUtilsTest.java
- src/main/resources/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.xml
- src/main/webapp/default-site/scripts/api/impl/content/SpringPageNavigationOrderServices.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/DmPageNavigationOrderService.java
- src/main/webapp/default-site/scripts/rest/api/1/content/get-item-orders.get.groovy
- src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
- src/main/resources/crafter/studio/database-context.xml
- src/main/java/org/craftercms/studio/impl/v1/service/content/DmPageNavigationOrderServiceImpl.java
- src/main/webapp/default-site/scripts/api/ContentServices.groovy
- src/main/java/org/craftercms/studio/api/v1/dal/NavigationOrderSequenceMapper.java
- src/main/webapp/default-site/scripts/api/PageNavigationOrderServices.groovy
- src/main/webapp/default-site/scripts/rest/api/1/content/get-next-item-order.get.groovy
- src/main/webapp/default-site/scripts/api/ServiceFactory.groovy
- src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
- src/main/webapp/default-site/scripts/rest/api/1/content/reorder-items.get.groovy
- src/main/webapp/default-site/scripts/api/impl/content/SpringContentServices.groovy
- src/main/api/studio-api.yaml
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/craftercms/studio/controller/rest/v2/ResultConstants.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/craftercms/studio/controller/rest/v2/RequestMappingConstants.java
- src/main/resources/crafter/studio/studio-services-context.xml
- src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java
Member
|
@coderabbitai, we're done with development. Please give it a once-over before we send this to a human. Make sure to itemize any issues and you can ignore Travis CI. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Inspect ContentServiceInternalImpl for the new ordering methods
rg -n "getItemsOrder\|reorderItem\|updateNavOrder\|getItemOrder\|pageNavOrderIncrement\|DEFAULT_PAGE_NAV_ORDER" src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java | head -80
echo "---FULL METHOD---"
ast-grep --pattern 'private $_ updateNavOrder($$$) {
$$$
}'
echo "---getItemsOrder impl---"
ast-grep --pattern 'public $_ getItemsOrder($$$) {
$$$
}'
echo "---getItemOrder impl---"
ast-grep --pattern 'private $_ getItemOrder($$$) {
$$$
}'
echo "---reorderItem impl---"
ast-grep --pattern 'public $_ reorderItem($$$) {
$$$
}'Length of output: 996 🏁 Script executed: #!/bin/bash
# Inspect ReorderItemRequest model
cat src/main/java/org/craftercms/studio/model/rest/content/order/ReorderItemRequest.javaLength of output: 2539 🏁 Script executed: #!/bin/bash
# Inspect ContentController new endpoints
rg -n "getItemsOrder\|reorderItem\|GET_ITEMS_ORDER\|REORDER_ITEM" src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java -A 20 -B 5Length of output: 451 🏁 Script executed: #!/bin/bash
# Inspect ItemOrder model
cat src/main/java/org/craftercms/studio/model/rest/content/order/ItemOrder.javaLength of output: 1509 🏁 Script executed: #!/bin/bash
# Check for any remaining references to NavigationOrderSequence
rg -rn "NavigationOrderSequence\|navigationOrderSequence\|navigation_order_sequence" --type java --type xml --type sql --type groovy
echo "---Check for any unused import in ContentService v1---"
cat src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java | tail -30Length of output: 1212 🏁 Script executed: #!/bin/bash
# Inspect ContentServiceInternalImpl for reorderItem and getItemsOrder full implementations
cat src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java | grep -n "getItemsOrder\|getItemOrder\|reorderItem\|updateNavOrder\|pageNavOrder\|DEFAULT_PAGE_NAV\|ItemOrder\|ReorderItem" | head -60Length of output: 3004 🏁 Script executed: #!/bin/bash
# Check DB upgrade script
cat src/main/resources/crafter/studio/database/upgrade/5.0.x/5.0.0.18-to-5.0.0.19.sql | head -120
echo "---createDDL.sql nav order section---"
rg -n "navigation_order\|5.0.0.19\|5.0.0.18" src/main/resources/crafter/studio/database/createDDL.sql | head -20
echo "---pipelines.yaml---"
cat src/main/resources/crafter/studio/upgrade/pipelines.yaml | grep -A5 -B5 "5.0.0.18\|5.0.0.19"Length of output: 5602 🏁 Script executed: #!/bin/bash
# Check for missing parentPath in ReorderItemRequest & API YAML schema alignment
rg -n "parentPath\|siteId\|ReorderItemRequest\|ReorderItemReferencePathRequest\|ReorderItemInsertBetweenRequest" src/main/api/studio-api.yaml | head -40
echo "---API YAML for reorderItem endpoint---"
rg -n "reorderItem\|reorder\|getItemsOrder\|/order" src/main/api/studio-api.yaml -A 30 | head -120Length of output: 477 🏁 Script executed: #!/bin/bash
# Check v2 ContentService interface for new methods
cat src/main/java/org/craftercms/studio/api/v2/service/content/ContentService.java | grep -A5 "getItemsOrder\|reorderItem"
# Check v2 ContentServiceImpl for new methods
cat src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java | grep -A15 "getItemsOrder\|reorderItem"Length of output: 1472 🏁 Script executed: #!/bin/bash
# Check if getItemOrders was left in v1 ContentService (AI summary says reorderItems was removed but getItemOrders remains)
rg -n "getItemOrders\|getItemsOrder\|reorderItems\|reorderItem" src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java
echo "---v1 ContentServiceImpl - check for computeReorder still present---"
rg -n "computeReorder" src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java -A10
echo "---check for computeReorder usage---"
rg -rn "computeReorder" --type javaLength of output: 1435 🏁 Script executed: #!/bin/bash
# Inspect test for ContentServiceInternalImpl
cat src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java | grep -n "StudioConfiguration\|studioConfiguration\|pageNavOrder\|reorderItem\|getItemsOrder\|ItemOrder" | head -40
# Check if there are any tests for the new controller endpoints
rg -rn "getItemsOrder\|reorderItem" --type java src/test/
echo "---Check for any test for ContentController---"
fd "ContentControllerTest" src/test/Length of output: 1480 🏁 Script executed: #!/bin/bash
# Check for potential NPE in ContentServiceInternalImpl - does getItemsOrder handle null items?
cat src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java | grep -n -A 30 "public.*getItemsOrder\|private.*getItemOrder\|private.*updateNavOrder"Length of output: 3355 🏁 Script executed: #!/bin/bash
# Get full reorderItem protected method and getItemOrder
sed -n '953,1040p' src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javaLength of output: 4144 🏁 Script executed: #!/bin/bash
# Check ContentServiceImpl getItemsOrder annotations
rg -n "getItemsOrder\|reorderItem" src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java -B 10Length of output: 432 🏁 Script executed: #!/bin/bash
# Check if ReorderItemRequest is missing parentPath (the POST endpoint uses siteId from path but where does parentPath come from?)
rg -n "parentPath\|reorderItem\|ReorderItemRequest" src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java -A 15 -B 5
echo "---full ContentController new endpoints---"
cat src/main/java/org/craftercms/studio/controller/rest/v2/ContentController.java | grep -n "getItemsOrder\|reorderItem\|order" | head -20Length of output: 1231 🏁 Script executed: #!/bin/bash
# Check API YAML for the new endpoints
grep -n "order\|reorder\|getItemsOrder\|reorderItem" src/main/api/studio-api.yaml | head -60Length of output: 2489 🏁 Script executed: #!/bin/bash
# Check the full reorderItem test coverage
sed -n '895,960p' src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.javaLength of output: 2901 🏁 Script executed: #!/bin/bash
# Check if v1 getItemOrders still has an implementation in ContentServiceImpl (v1)
rg -n "getItemOrders\|DmOrderTO" src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java -A5 | head -40Length of output: 469 🏁 Script executed: #!/bin/bash
# Look for computeReorder references and any dead code
rg -rn "computeReorder\|getOrders\b" --type java | head -20
# Check if there's a dead reorderItem(String, String, String) overload in ContentServiceInternalImpl
sed -n '977,1030p' src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javaLength of output: 3066 🏁 Script executed: #!/bin/bash
# Check the full API YAML for the new order endpoints
python3 -c "
import re
with open('src/main/api/studio-api.yaml') as f:
content = f.read()
# Find the sections around 'order'
matches = [(m.start(), m.end()) for m in re.finditer(r'/api/2/content/\{siteId\}/order', content)]
for start, end in matches:
print(content[max(0,start-50):start+2000])
print('---')
"Length of output: 4541
🔴 High Priority1.
// ContentServiceImpl.java — as-is
public List<ItemOrder> getItemsOrder(`@SiteId` String siteId, `@ContentPath` String parentPath) throws ServiceLayerException {
return contentServiceInternal.getItemsOrder(siteId, parentPath);
}Should match the pattern used by `@Override`
`@RequireSiteReady`
`@HasPermission`(type = DefaultPermission.class, action = PERMISSION_CONTENT_READ)
public List<ItemOrder> getItemsOrder(`@SiteId` String siteId, `@ContentPath` String parentPath) throws ServiceLayerException {
return contentServiceInternal.getItemsOrder(siteId, parentPath);
}2.
Recommend wrapping the parse in // inside getItemOrder, replacing the bare parse:
try {
order = Double.parseDouble(orderString);
} catch (NumberFormatException e) {
throw new ServiceLayerException(
format("Invalid order value '%s' in descriptor for site '%s' path '%s'", orderString, siteId, path), e);
}🟡 Medium Priority3. Dead code:
4. v1 The Groovy REST script ( 5. No unit tests for
🟢 Low Priority6. List<ContentItem> pages = itemDao.getChildrenByPath(site.getId(), parentPath, null, null,
List.of(CONTENT_TYPE_PAGE), null, null, null, null, 0, Integer.MAX_VALUE);For very large sites this could cause memory pressure. Consider capping at a reasonable max (e.g. 10,000) or at minimum adding a 7. No sibling validation for
That's the full list. Items 1 and 2 should definitely be fixed before merge; the rest can be addressed in this PR or tracked for a follow-up. 🧠 Learnings used |
sumerjabri
requested changes
Jun 6, 2026
sumerjabri
left a comment
sumerjabri
left a comment
Member
There was a problem hiding this comment.
Please see the CR comments.
Member
Author
|
@CodeRabbit reassess your last review now that there are new commits |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# 1. Check getItemsOrder annotations in v2 ContentServiceImpl
echo "=== ContentServiceImpl v2 getItemsOrder & reorderItem annotations ==="
rg -n "getItemsOrder\|reorderItem\|RequireSiteReady\|HasPermission\|RequireContentExists\|SiteId\|ContentPath" \
src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java -B3 -A3 | head -80
echo ""
echo "=== Full getItemsOrder method in ContentServiceImpl ==="
grep -n "getItemsOrder" src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java -A 10
echo ""
echo "=== Full reorderItem method in ContentServiceImpl ==="
grep -n "reorderItem" src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.java -A 10Length of output: 1909 🏁 Script executed: #!/bin/bash
# 2. Check getItemOrder for NumberFormatException handling
echo "=== getItemOrder in ContentServiceInternalImpl ==="
grep -n "getItemOrder\|parseDouble\|NumberFormat\|catch" \
src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java -A 3 -B 1 | head -80
echo ""
echo "=== reorderItem internal implementations ==="
grep -n "reorderItem\|previousPath\|nextPath\|referencePath\|AddBefore\|AddAfter\|Insert" \
src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java -A 5 | head -80Length of output: 7622 🏁 Script executed: #!/bin/bash
# 3. Check computeReorder dead code in v1 ContentServiceImpl
echo "=== computeReorder in v1 ContentServiceImpl ==="
grep -n "computeReorder" src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java -A 5
echo ""
echo "=== getItemOrders in v1 ContentService interface ==="
grep -n "getItemOrders\|getItemsOrder" src/main/java/org/craftercms/studio/api/v1/service/content/ContentService.java -A 5
echo ""
echo "=== getItemOrders in v1 ContentServiceImpl ==="
grep -n "getItemOrders\|getItemsOrder" src/main/java/org/craftercms/studio/impl/v1/service/content/ContentServiceImpl.java -A 5Length of output: 1373 🏁 Script executed: #!/bin/bash
# 4. Check unit tests for getItemsOrder and reorderItem
echo "=== Tests in ContentServiceInternalImplTest ==="
grep -n "getItemsOrder\|reorderItem\|ItemOrder\|computeReorder\|`@Test`" \
src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java | head -60
echo ""
echo "=== Count test methods ==="
grep -c "`@Test`" src/test/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImplTest.java
echo ""
# 5. Check sibling validation for AddBefore/AddAfter
echo "=== areSiblings usage in ContentServiceInternalImpl ==="
grep -n "areSiblings\|AddBefore\|AddAfter\|ReferenceReorder\|referencePath" \
src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java -A 5 | head -50
echo ""
# 6. Check paging limit
echo "=== Integer.MAX_VALUE or paging in getItemsOrder ==="
grep -n "MAX_VALUE\|getChildrenByPath\|CONTENT_TYPE_PAGE" \
src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java -A 3 | head -30Length of output: 6600 🏁 Script executed: #!/bin/bash
# Get the full getItemsOrder implementation
echo "=== Full getItemsOrder region ==="
grep -n "getItemsOrder" src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javaLength of output: 484 🏁 Script executed: #!/bin/bash
# Get line ranges for getItemsOrder and reorderItem implementations in ContentServiceInternalImpl
echo "=== Line numbers for key methods ==="
grep -n "public.*getItemsOrder\|public.*reorderItem\|private.*getItemOrder\|private.*updateNavOrder\|private.*reorderItem" \
src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.javaLength of output: 804 🏁 Script executed: #!/bin/bash
# Get full getItemOrder implementation
echo "=== getItemOrder full body ==="
sed -n '956,985p' src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
echo ""
echo "=== MAX_CHILDREN_FOR_GET_NAV_ORDER constant ==="
grep -n "MAX_CHILDREN_FOR_GET_NAV_ORDER" \
src/main/java/org/craftercms/studio/impl/v2/service/content/internal/ContentServiceInternalImpl.java
echo ""
echo "=== Check getItemsOrder annotations in v2 ContentServiceImpl - full context ==="
sed -n '350,370p' src/main/java/org/craftercms/studio/impl/v2/service/content/ContentServiceImpl.javaLength of output: 2970
The only remaining item is the low-priority sibling check ( 🧠 Learnings used |
Member
Author
|
@CodeRabbit |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
sumerjabri
approved these changes
Jun 9, 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.
craftercms/craftercms#6061
Item nav order APIs update
Summary by CodeRabbit
Release Notes
New Features
Breaking Changes
Chores