Skip to content

[7418] FE2 - PageNavOrder control#4670

Draft
jvega190 wants to merge 19 commits intocraftercms:developfrom
jvega190:feature/7418-page-order
Draft

[7418] FE2 - PageNavOrder control#4670
jvega190 wants to merge 19 commits intocraftercms:developfrom
jvega190:feature/7418-page-order

Conversation

@jvega190
Copy link
Copy Markdown
Member

@jvega190 jvega190 commented Oct 8, 2025

Added PageNavOrder control and APIs for items retrieval and reordering
craftercms/craftercms#7418

Summary by CodeRabbit

  • New Features
    • Added page navigation order management allowing users to edit and reorder navigation items
    • Introduced drag-and-drop interface for customizing navigation item order
    • Added toggle to include or exclude pages from navigation menu

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Oct 8, 2025

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Introduces 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

Cohort / File(s) Summary
New PageNavOrder Component
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx, ui/app/src/components/FormsEngine/lib/controlMap.ts
Introduces PageNavOrder component with a radio toggle for navigation inclusion and an EnhancedDialog containing a SortableList for drag-and-drop reordering. Registers component in control map.
Navigation Service Layer
ui/app/src/services/content.ts
Adds PageNavItem interface and two public functions: getNavItemsOrder() to fetch current navigation item ordering, and reorderNavItems() to persist reordering changes via API.
SortableList Enhancement
ui/app/src/components/FormsEngine/components/SortableList.tsx
Adds optional onlySelectedSortable prop to conditionally restrict drag-sort behavior to only the selected item, with drag handles rendered only for sortable items.

Sequence Diagram

sequenceDiagram
    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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 11.11% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly references ticket #7418 and describes the main change: introducing the PageNavOrder control component for the FE2 forms engine.
Description check ✅ Passed The description provides a concise summary of changes and links to the related issue, though it could be more detailed about what PageNavOrder does and the scope of changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between cd14b55 and 3886f65.

📒 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.

Comment thread ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx Outdated
Comment thread ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx
Comment thread ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

RadioGroup onChange signature is (event, value), not SelectChangeEvent. Using SelectChangeEvent and converting event.target.value to 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.

handleUpdateOrder can run before order is loaded, producing null/NaN indices and sending null params 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 reorderNavItems signature accepts optional before/after params (pass undefined for 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.order can be null, which may cause issues in SortableList.

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 value prop should be constrained to 'true' | 'false' instead of string to 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3886f65 and 6c83938.

📒 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

Comment thread ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx Outdated
Comment thread ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 SortableList will safely handle undefined for selectedItemId (comparison will be false), it's more explicit to pass undefined or 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

📥 Commits

Reviewing files that changed from the base of the PR and between 6c83938 and 0fde973.

📒 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

Comment thread ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx
Comment thread ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (1)

160-164: Guard against missing contextItem before reading .label.

contextItem can be undefined, so contextItem.label will throw and crash the dialog. Use optional chaining with a fallback (e.g. contextItem?.label ?? currentPath ?? 'this page') before passing to FormattedMessage.

-								page: contextItem.label
+								page: contextItem?.label ?? currentPath ?? formatMessage({ defaultMessage: 'this page' })
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0fde973 and 9a15552.

📒 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

Comment thread ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
ui/app/src/components/FormsEngine/lib/useSaveForm.tsx (1)

162-166: Prefer Object.keys() over for...in for safer iteration.

Using for...in without a hasOwnProperty check iterates over all enumerable properties, including inherited ones. While the risk is low here since systemProps is a plain object, using Object.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

📥 Commits

Reviewing files that changed from the base of the PR and between 3f0a3bf and d2834c5.

📒 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.

Comment thread ui/app/src/components/FormsEngine/lib/useSaveForm.tsx
jvega190 added 2 commits March 2, 2026 11:41
…o feature/7418-page-order

# Conflicts:
#	ui/app/src/components/FormsEngine/lib/useSaveForm.tsx
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx (1)

165-166: ⚠️ Potential issue | 🟠 Major

Guard contextItem before reading label.

Line 165 dereferences contextItem.label directly; 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 when sortable is 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

📥 Commits

Reviewing files that changed from the base of the PR and between d2834c5 and 2abc622.

📒 Files selected for processing (4)
  • ui/app/src/components/FormsEngine/components/SortableList.tsx
  • ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx
  • ui/app/src/components/FormsEngine/lib/controlMap.ts
  • ui/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

Comment thread ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx
Comment thread ui/app/src/components/FormsEngine/controls/PageNavOrder.tsx
@jvega190
Copy link
Copy Markdown
Member Author

jvega190 commented Mar 3, 2026

This needs the additionalFields work to be able to update the order tag in the xml

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant