[7418] FE2 - PageNavOrder control#4670
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:
WalkthroughIntroduces a new PageNavOrder component that enables users to edit the order of page navigation items through a drag-and-drop interface. Adds supporting service layer functions for fetching and reordering navigation items, registers the component in the control map, and enhances SortableList with conditional sorting behavior. Changes
Sequence DiagramsequenceDiagram
participant User
participant PageNavOrder as PageNavOrder Component
participant Dialog as EnhancedDialog
participant SortableList
participant ContentService as Content Service
participant API
User->>PageNavOrder: Toggle "Include in Navigation" to Yes
PageNavOrder->>ContentService: getNavItemsOrder(siteId, currentPath)
ContentService->>API: GET /get-item-orders
API-->>ContentService: NavItem[] (with current order)
ContentService-->>PageNavOrder: PageNavItem[]
PageNavOrder->>Dialog: Open with nav items list
Dialog->>SortableList: Render items with drag handles
User->>SortableList: Drag and reorder items
SortableList->>SortableList: Update local item order
User->>Dialog: Click Save
Dialog->>PageNavOrder: Trigger save with new order
PageNavOrder->>ContentService: reorderNavItems(siteId, currentPath, before, after)
ContentService->>API: GET /reorder-items
API-->>ContentService: Success response
ContentService-->>PageNavOrder: Order updated
PageNavOrder->>User: Show success notification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
ui/app/src/services/content.ts (3)
1502-1508: Consider proper boolean typing for API shape (or map to booleans).disabled and placeInNav are strings. If the backend returns "true"/"false", prefer mapping to booleans in the client or declaring them as 'true' | 'false' to avoid misuse downstream.
Example mapping inside the fetch method (if feasible) to return boolean fields; otherwise narrow types to 'true' | 'false'.
1510-1516: Harden response mapping; default to an empty array.response?.response?.order may be undefined. Return [] to prevent nullish consumers from breaking.
export function getNavItemsOrder(site: string, path: string, order: string = 'default'): Observable<PageNavItem[]> { const qs = toQueryString({ site, path, order }); return get(`/studio/api/1/services/api/1/content/get-item-orders.json${qs}`).pipe( - map((response) => response?.response?.order), + map((response) => (response?.response?.order ?? []) as PageNavItem[]), catchError(errorSelectorApi1) ); }
1518-1524: Explicit return type and omit undefined params in query.
- Expose the return type (currently implicit any).
- Avoid sending null values; include before/after only when defined.
-export function reorderNavItems(site: string, path: string, before: string, after: string) { - const qs = toQueryString({ site, path, before, after }); - return get(`/studio/api/1/services/api/1/content/reorder-items.json${qs}`).pipe( - map((response) => response?.response?.orderValue), - catchError(errorSelectorApi1) - ); -} +export function reorderNavItems( + site: string, + path: string, + before?: string, + after?: string +): Observable<string | number> { + const params: Record<string, string> = { site, path }; + if (before) params.before = before; + if (after) params.after = after; + const qs = toQueryString(params); + return get(`/studio/api/1/services/api/1/content/reorder-items.json${qs}`).pipe( + map((response) => response?.response?.orderValue as string | number), + catchError(errorSelectorApi1) + ); +}Confirm the endpoint accepts GET with optional before/after and returns orderValue. If it requires POST, adjust accordingly.
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (1)
177-179: Disable Save until order is loaded (and optionally while submitting).Prevents invalid requests and improves UX.
-<PrimaryButton autoFocus onClick={handleUpdateOrder}> +<PrimaryButton + autoFocus + onClick={handleUpdateOrder} + disabled={!pagesOrderState.order || pagesOrderState.fetching} +> <FormattedMessage defaultMessage="Save" /> </PrimaryButton>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx(1 hunks)ui/app/src/components/FormsEngine/lib/controlMap.ts(1 hunks)ui/app/src/services/content.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (14)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/hooks/useEnhancedDialogState.ts (1)
useEnhancedDialogState(38-95)ui/app/src/hooks/useSpreadState.ts (1)
useSpreadState(19-25)ui/app/src/models/ApiResponse.ts (1)
ApiResponse(17-22)ui/app/src/components/FormsEngine/components/SortableList.tsx (2)
TItem(41-41)SortableList(266-335)ui/app/src/services/content.ts (3)
PageNavItem(1502-1508)getNavItemsOrder(1510-1516)reorderNavItems(1518-1524)ui/app/src/components/FormsEngine/lib/formsEngineContext.ts (1)
useItemContext(146-146)ui/app/src/hooks/useUpdateRefs.ts (1)
useUpdateRefs(21-25)ui/app/src/state/actions/system.ts (1)
showSystemNotification(121-124)ui/app/src/utils/system.ts (1)
pushErrorDialog(170-175)ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (1)
FormsEngineField(92-282)ui/app/src/components/EnhancedDialog/EnhancedDialog.tsx (1)
EnhancedDialog(43-135)ui/app/src/components/DialogBody/DialogBody.tsx (1)
DialogBody(23-36)ui/app/src/components/DialogFooter/DialogFooter.tsx (1)
DialogFooter(21-36)
ui/app/src/services/content.ts (2)
ui/app/src/utils/object.ts (1)
toQueryString(211-217)ui/app/src/utils/ajax.ts (2)
get(57-59)errorSelectorApi1(139-185)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Travis CI - Pull Request
🔇 Additional comments (1)
ui/app/src/components/FormsEngine/lib/controlMap.ts (1)
72-72: Lazy mapping looks good.The control now lazy-loads PageNavOrder; default export exists, so resolution should work.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (4)
97-99: Wrong event type and value conversion; breaks selection and model consistency.
RadioGrouponChangesignature is(event, value), notSelectChangeEvent. UsingSelectChangeEventand convertingevent.target.valueto boolean causes the selected radio not to match ("true"/"false"vs boolean) and can corrupt the field value.Apply this diff:
-const handleChange = (event: SelectChangeEvent) => { - setValue(event.target.value === 'true'); -}; +const handleChange = (_event: React.ChangeEvent<HTMLInputElement>, selected: string) => { + setValue(selected); // keep 'true' | 'false' strings +};Optionally narrow the prop type as suggested in lines 48-50.
100-119: Guard against empty order and avoid sending null before/after.
handleUpdateOrdercan run beforeorderis loaded, producingnull/NaNindices and sendingnullparams to the service.Apply this diff:
const handleUpdateOrder = () => { orderDialogState.onClose(); + if (!pagesOrderState.order || !pagesOrderState.order.length) { + return; + } - const currentItemIndex = pagesOrderState.order?.findIndex((item) => item.key === currentPath); + const currentItemIndex = pagesOrderState.order.findIndex((item) => item.key === currentPath); const previewItemIndex = currentItemIndex - 1; const nextItemIndex = currentItemIndex + 1; - const previewItemPath = previewItemIndex >= 0 ? pagesOrderState.order?.[previewItemIndex]?.key : null; + const previewItemPath = previewItemIndex >= 0 ? pagesOrderState.order[previewItemIndex]?.key : undefined; const nextItemPath = - nextItemIndex < (pagesOrderState.order?.length ?? 0) ? pagesOrderState.order?.[nextItemIndex]?.key : null; + nextItemIndex < pagesOrderState.order.length ? pagesOrderState.order[nextItemIndex]?.key : undefined; // TODO: Waiting for the new v2 API to handle reordering the full items list. Currently, this only reorders // the current item, and no other items can be re-arranged. reorderNavItems(siteId, currentPath, previewItemPath, nextItemPath).subscribe({Note: Ensure
reorderNavItemssignature accepts optionalbefore/afterparams (passundefinedfor missing neighbors).
129-139: Truthiness check shows "Edit Order" when value is "false".The string
"false"is truthy, so the button renders incorrectly. Compare explicitly to the expected true value.Apply this diff:
-{value && ( +{value === 'true' && ( <Button variant="text" startIcon={<ChangeCircleOutlinedIcon />} onClick={() => orderDialogState.onOpen()} disabled={readonly} sx={{ flex: 'none' }} > <FormattedMessage defaultMessage="Edit Order" /> </Button> )}
162-171: Avoid passing null to SortableList.
pagesOrderState.ordercan benull, which may cause issues inSortableList.Apply this diff:
<SortableList - items={pagesOrderState.order} + items={pagesOrderState.order ?? []} selectedItemId={currentPath} onChange={(fields: TItem<PageNavItem>[]) => setPagesOrderState({ order: fields }) } />
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (1)
48-50: Narrow the value type for type safety.The
valueprop should be constrained to'true' | 'false'instead ofstringto prevent invalid values and improve type safety.Apply this diff:
export interface PageNavOrderProps extends ControlProps { - value: string; + value: 'true' | 'false'; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (10)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/hooks/useEnhancedDialogState.ts (1)
useEnhancedDialogState(38-95)ui/app/src/hooks/useSpreadState.ts (1)
useSpreadState(19-25)ui/app/src/models/ApiResponse.ts (1)
ApiResponse(17-22)ui/app/src/components/FormsEngine/components/SortableList.tsx (2)
TItem(41-41)SortableList(266-335)ui/app/src/services/content.ts (3)
PageNavItem(1502-1508)getNavItemsOrder(1510-1516)reorderNavItems(1518-1524)ui/app/src/components/FormsEngine/lib/formsEngineContext.ts (1)
useItemContext(146-146)ui/app/src/hooks/useUpdateRefs.ts (1)
useUpdateRefs(21-25)ui/app/src/state/actions/system.ts (1)
showSystemNotification(121-124)ui/app/src/utils/system.ts (1)
pushErrorDialog(170-175)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Travis CI - Pull Request
- GitHub Check: Codacy Static Code Analysis
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (2)
113-120: Consider tracking the subscription for proper cleanup.While less critical than the useEffect case, the subscription should ideally be tracked so it can be unsubscribed if needed (e.g., if the dialog is closed or component unmounts during the request).
You can store the subscription in a ref and clean it up in a useEffect cleanup or when the dialog closes:
const subscriptionRef = useRef<Subscription | null>(null); // In handleUpdateOrder: subscriptionRef.current = reorderNavItems(siteId, currentPath, previewItemPath, nextItemPath).subscribe({ // ... handlers }); // Add cleanup: useEffect(() => { return () => { subscriptionRef.current?.unsubscribe(); }; }, []);
169-177: Consider explicit handling of undefined currentPath.While
SortableListwill safely handleundefinedforselectedItemId(comparison will be false), it's more explicit to passundefinedor an empty string to signal no selection.Consider:
<SortableList items={pagesOrderState.order ?? []} - selectedItemId={currentPath} + selectedItemId={currentPath ?? ''} onChange={(fields: TItem<PageNavItem>[]) =>
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (14)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/hooks/useEnhancedDialogState.ts (1)
useEnhancedDialogState(38-95)ui/app/src/hooks/useSpreadState.ts (1)
useSpreadState(19-25)ui/app/src/models/ApiResponse.ts (1)
ApiResponse(17-22)ui/app/src/components/FormsEngine/components/SortableList.tsx (2)
TItem(41-41)SortableList(266-335)ui/app/src/services/content.ts (3)
PageNavItem(1502-1508)getNavItemsOrder(1510-1516)reorderNavItems(1518-1524)ui/app/src/components/FormsEngine/lib/formsEngineContext.ts (1)
useItemContext(146-146)ui/app/src/hooks/useUpdateRefs.ts (1)
useUpdateRefs(21-25)ui/app/src/state/actions/system.ts (1)
showSystemNotification(121-124)ui/app/src/utils/system.ts (1)
pushErrorDialog(170-175)ui/app/src/components/FormsEngine/components/FormsEngineField.tsx (1)
FormsEngineField(92-282)ui/app/src/components/EnhancedDialog/EnhancedDialog.tsx (1)
EnhancedDialog(43-135)ui/app/src/components/DialogBody/DialogBody.tsx (1)
DialogBody(23-36)ui/app/src/components/DialogFooter/DialogFooter.tsx (1)
DialogFooter(21-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Travis CI - Pull Request
… currentPath and item not found in navItems
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (1)
160-164: Guard against missingcontextItembefore reading.label.
contextItemcan be undefined, socontextItem.labelwill throw and crash the dialog. Use optional chaining with a fallback (e.g.contextItem?.label ?? currentPath ?? 'this page') before passing toFormattedMessage.- page: contextItem.label + page: contextItem?.label ?? currentPath ?? formatMessage({ defaultMessage: 'this page' })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (10)
ui/app/src/components/FormsEngine/types.ts (1)
ControlProps(23-30)ui/app/src/hooks/useEnhancedDialogState.ts (1)
useEnhancedDialogState(38-95)ui/app/src/hooks/useSpreadState.ts (1)
useSpreadState(19-25)ui/app/src/models/ApiResponse.ts (1)
ApiResponse(17-22)ui/app/src/components/FormsEngine/components/SortableList.tsx (2)
TItem(41-41)SortableList(266-335)ui/app/src/services/content.ts (3)
PageNavItem(1502-1508)getNavItemsOrder(1510-1516)reorderNavItems(1518-1524)ui/app/src/components/FormsEngine/lib/formsEngineContext.ts (1)
useItemContext(146-146)ui/app/src/hooks/useUpdateRefs.ts (1)
useUpdateRefs(21-25)ui/app/src/state/actions/system.ts (1)
showSystemNotification(121-124)ui/app/src/utils/system.ts (1)
pushErrorDialog(170-175)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Travis CI - Pull Request
- GitHub Check: Codacy Static Code Analysis
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/lib/useSaveForm.tsx (1)
162-166: PreferObject.keys()overfor...infor safer iteration.Using
for...inwithout ahasOwnPropertycheck iterates over all enumerable properties, including inherited ones. While the risk is low here sincesystemPropsis a plain object, usingObject.keys()is safer and more explicit.Apply this diff to use a safer iteration pattern:
// Do not overwrite any existing value. - for (const key in systemProps) { - if (!(key in values)) { - values[key] = systemProps[key]; - } - } + Object.keys(systemProps).forEach((key) => { + if (!(key in values)) { + values[key] = systemProps[key]; + } + });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/app/src/components/FormsEngine/lib/useSaveForm.tsx(1 hunks)ui/app/src/services/content.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/app/src/services/content.ts
🧰 Additional context used
🧬 Code graph analysis (1)
ui/app/src/components/FormsEngine/lib/useSaveForm.tsx (1)
ui/app/src/components/FormsEngine/lib/formUtils.tsx (1)
createObjectWithSystemProps(425-451)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Travis CI - Pull Request
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (1)
ui/app/src/components/FormsEngine/lib/useSaveForm.tsx (1)
145-167: Non-destructive merge approach is correct and properly integrated.The implementation correctly preserves user-provided values while ensuring all required system metadata is present before save. The code properly handles both create and update modes, and the fallback logic for fileName and folderName maintains data integrity.
…o feature/7418-page-order # Conflicts: # ui/app/src/components/FormsEngine/lib/useSaveForm.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (1)
165-166:⚠️ Potential issue | 🟠 MajorGuard
contextItembefore readinglabel.Line 165 dereferences
contextItem.labeldirectly; this can throw when context is unavailable.🛡️ Suggested fix
<FormattedMessage defaultMessage={'Drag and Drop "{page}" to the desired location in the navigation structure.'} values={{ - page: contextItem.label + page: contextItem?.label ?? currentPath ?? 'this page' }} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx` around lines 165 - 166, The code dereferences contextItem.label directly (in PageNavOrder.tsx) which can throw when contextItem is undefined; update the usage where the object with page: contextItem.label is constructed to guard access — e.g., use optional chaining or a fallback value (contextItem?.label ?? '') or conditionally build the object only when contextItem is present; ensure the change targets the place that constructs the navigation payload so callers like the PageNavOrder component or its handler no longer read label from an undefined contextItem.
🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/components/SortableList.tsx (1)
251-252: Use a non-drag cursor for non-sortable rows.Line 251 always sets
cursor: 'grab', even whensortableis false. That advertises drag where dragging is disabled.♻️ Suggested tweak
sx={[ - { position: 'relative', borderRadius: 1, cursor: 'grab' }, + { position: 'relative', borderRadius: 1, cursor: sortable ? 'grab' : 'default' }, state.type === 'is-dragging' && { opacity: 0.4 } ]}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/app/src/components/FormsEngine/components/SortableList.tsx` around lines 251 - 252, The inline style always sets cursor: 'grab' which misleads when dragging is disabled; update the style generation in SortableList (the place using state.type and the style object with borderRadius/cursor) to use the component's sortable prop (or equivalent) to choose cursor: sortable ? 'grab' : 'default' (or 'auto'), preserving the existing conditional opacity for state.type === 'is-dragging' so non-sortable rows no longer show a grab cursor while sortable rows keep the drag affordance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx`:
- Around line 102-126: The primary "Close" flow is still invoking reorder logic
because handleUpdateOrder() is always called; to fix, prevent reorder when no
changes by either (A) adding an early return in handleUpdateOrder that checks
the changedOrder flag (e.g., if (!changedOrder) { orderDialogState.onClose();
return; }) before any reorderNavItems call, or (B) change the button's onClick
binding so when changedOrder is false it calls orderDialogState.onClose()
directly and only calls handleUpdateOrder() when changedOrder is true; reference
symbols: handleUpdateOrder, changedOrder, orderDialogState.onClose, and
reorderNavItems.
- Around line 99-101: handleChange currently always calls setValue and therefore
mutates form state even when the control is read-only; update the handleChange
in PageNavOrder.tsx to early-return when the control is read-only by checking
the component's read-only prop (e.g., props.readOnly | props.readonly |
props.isReadOnly) before calling setValue(selected === 'true'), so mutate state
only when not read-only.
---
Duplicate comments:
In `@ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx`:
- Around line 165-166: The code dereferences contextItem.label directly (in
PageNavOrder.tsx) which can throw when contextItem is undefined; update the
usage where the object with page: contextItem.label is constructed to guard
access — e.g., use optional chaining or a fallback value (contextItem?.label ??
'') or conditionally build the object only when contextItem is present; ensure
the change targets the place that constructs the navigation payload so callers
like the PageNavOrder component or its handler no longer read label from an
undefined contextItem.
---
Nitpick comments:
In `@ui/app/src/components/FormsEngine/components/SortableList.tsx`:
- Around line 251-252: The inline style always sets cursor: 'grab' which
misleads when dragging is disabled; update the style generation in SortableList
(the place using state.type and the style object with borderRadius/cursor) to
use the component's sortable prop (or equivalent) to choose cursor: sortable ?
'grab' : 'default' (or 'auto'), preserving the existing conditional opacity for
state.type === 'is-dragging' so non-sortable rows no longer show a grab cursor
while sortable rows keep the drag affordance.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ui/app/src/components/FormsEngine/components/SortableList.tsxui/app/src/components/FormsEngine/controls/PageNavOrder.tsxui/app/src/components/FormsEngine/lib/controlMap.tsui/app/src/services/content.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/app/src/components/FormsEngine/lib/controlMap.ts
- ui/app/src/services/content.ts
|
This needs the additionalFields work to be able to update the order tag in the xml |
Added PageNavOrder control and APIs for items retrieval and reordering
craftercms/craftercms#7418
Summary by CodeRabbit