Implement basic edition for record page layouts#15237
Conversation
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR introduces basic editing capabilities for record page layouts, specifically for Company objects. The implementation adds three new actions (Edit, Save, Cancel) to the side panel and enables drag-and-drop reordering of widgets in a vertical list layout mode. The feature is gated behind a feature flag (IS_RECORD_PAGE_LAYOUT_ENABLED) and uses a draft/persisted state pattern via Recoil. The architecture separates view and edit modes into distinct components: PageLayoutVerticalListViewer displays widgets in read-only mode, while PageLayoutVerticalListEditor enables DnD reordering using @hello-pangea/dnd. A new PageLayoutContent component orchestrates the switching between grid and vertical-list layouts based on feature flags and edit mode state. The implementation currently hardcodes a default layout ID for Company objects while waiting for backend support to store custom layouts per record.
Important Files Changed
Changed Files
| Filename | Score | Overview |
|---|---|---|
| packages/twenty-front/src/modules/page-layout/components/PageLayoutVerticalListEditor.tsx | 2/5 | Adds drag-and-drop editor for widgets with critical issue: useId() generates invalid droppableId containing colons |
| packages/twenty-front/src/modules/action-menu/actions/record-actions/single-record/record-page-layout-actions/components/SaveRecordPageLayoutSingleRecordAction.tsx | 3/5 | Implements save action for page layouts but lacks error handling for async save operation |
| packages/twenty-front/src/modules/action-menu/actions/record-actions/single-record/record-page-layout-actions/components/CancelRecordPageLayoutSingleRecordAction.tsx | 3/5 | Implements cancel action; potential undefined pageLayoutId for non-Company objects when backend not ready |
| packages/twenty-front/src/modules/action-menu/actions/record-actions/single-record/record-page-layout-actions/components/EditRecordPageLayoutSingleRecordAction.tsx | 3/5 | Enables edit mode toggle; lacks defensive checks for null selectedRecord from Recoil state |
| packages/twenty-front/src/modules/action-menu/hooks/useRegisteredActions.ts | 3/5 | Adds feature flag check but relies on getActionConfig to properly filter actions |
| packages/twenty-front/src/modules/page-layout/constants/DefaultPageLayout.ts | 3/5 | Adds duplicate Tasks widget at same grid position for testing DnD, which will cause overlap |
| packages/twenty-front/src/modules/page-layout/components/PageLayoutContent.tsx | 4/5 | Orchestrates view/edit mode switching between vertical-list and grid layouts with proper feature flag checks |
| packages/twenty-front/src/modules/action-menu/actions/record-actions/constants/RecordPageLayoutActionsConfig.tsx | 4/5 | Configures three new layout actions with proper registration logic; TODO comments indicate backend integration pending |
| packages/twenty-front/src/modules/action-menu/actions/utils/getActionConfig.ts | 4/5 | Adds required feature flag parameter to control action visibility; currently hardcoded to Company objects |
| packages/twenty-front/src/modules/page-layout/components/PageLayoutVerticalListViewer.tsx | 5/5 | Simple viewer component rendering widgets in vertical list with unnecessary wrapper div (minor code smell) |
| packages/twenty-front/src/modules/page-layout/hooks/useReorderPageLayoutWidgets.ts | 5/5 | Implements DnD reordering logic with proper null checks and immutable state updates |
| packages/twenty-front/src/modules/page-layout/components/PageLayoutLeftPanel.tsx | 5/5 | Simple refactor from PageLayoutGridLayout to PageLayoutContent component with no issues |
| packages/twenty-front/src/modules/page-layout/components/PageLayoutRendererContent.tsx | 5/5 | Replaces PageLayoutGridLayout with PageLayoutContent for expanded functionality support |
Confidence score: 2/5
- This PR has significant issues that could cause runtime errors and should not be merged without addressing the critical problems identified below.
- Score lowered due to: (1)
useId()generating invalid droppableId with colons in PageLayoutVerticalListEditor, (2) missing error handling for async save operations that could leave UI in inconsistent state, (3) potential null/undefined issues with selectedRecord and pageLayoutId for non-Company objects, (4) duplicate widgets at same grid position causing overlap, and (5) reliance on backend support that isn't fully implemented yet (per TODO comments). - Pay close attention to
packages/twenty-front/src/modules/page-layout/components/PageLayoutVerticalListEditor.tsx(fixuseId()usage for droppableId),SaveRecordPageLayoutSingleRecordAction.tsx(add error handling), and all three action components that handle selectedRecord/pageLayoutId (add defensive null checks).
Sequence Diagram
sequenceDiagram
participant User
participant RecordShowPage
participant ActionMenu
participant EditAction
participant SaveAction
participant CancelAction
participant PageLayoutState
participant PageLayoutContent
participant VerticalListEditor
participant DragDropContext
User->>RecordShowPage: "Navigate to record page"
RecordShowPage->>ActionMenu: "Render actions based on config"
ActionMenu->>ActionMenu: "Check if IS_RECORD_PAGE_LAYOUT_ENABLED"
ActionMenu->>ActionMenu: "Load RECORD_PAGE_LAYOUT_ACTIONS_CONFIG"
User->>EditAction: "Click Edit Layout"
EditAction->>PageLayoutState: "setIsPageLayoutInEditMode(true)"
PageLayoutState->>PageLayoutContent: "Update edit mode state"
PageLayoutContent->>VerticalListEditor: "Render editor view"
VerticalListEditor->>DragDropContext: "Initialize drag-and-drop"
User->>VerticalListEditor: "Drag widget"
VerticalListEditor->>DragDropContext: "Handle drag event"
DragDropContext->>VerticalListEditor: "onDragEnd(result)"
VerticalListEditor->>PageLayoutState: "reorderWidgets(result)"
PageLayoutState->>PageLayoutState: "Update pageLayoutDraftState"
User->>SaveAction: "Click Save Layout"
SaveAction->>PageLayoutState: "savePageLayout()"
SaveAction->>PageLayoutState: "setIsPageLayoutInEditMode(false)"
PageLayoutState->>PageLayoutContent: "Update edit mode state"
PageLayoutContent->>PageLayoutContent: "Switch to viewer mode"
User->>CancelAction: "Click Cancel Edition"
CancelAction->>PageLayoutState: "resetDraftPageLayoutToPersistedPageLayout()"
CancelAction->>PageLayoutState: "setIsPageLayoutInEditMode(false)"
PageLayoutState->>PageLayoutContent: "Update edit mode state"
PageLayoutContent->>PageLayoutContent: "Switch to viewer mode"
Context used:
- Context from
dashboard- Avoid using fragments when there is only one child component in a return statement. (source)
13 files reviewed, 15 comments
packages/twenty-front/src/modules/page-layout/constants/DefaultPageLayout.ts
Show resolved
Hide resolved
packages/twenty-front/src/modules/action-menu/hooks/useRegisteredActions.ts
Show resolved
Hide resolved
| {widgets.map((widget) => ( | ||
| <div key={widget.id}> | ||
| <WidgetRenderer widget={widget} /> | ||
| </div> | ||
| ))} |
There was a problem hiding this comment.
style: Remove the redundant wrapper div around WidgetRenderer - it serves no purpose and can be omitted
Context Used: Context from dashboard - Avoid using fragments when there is only one child component in a return statement. (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/page-layout/components/PageLayoutVerticalListViewer.tsx
Line: 20:24
Comment:
**style:** Remove the redundant wrapper div around WidgetRenderer - it serves no purpose and can be omitted
**Context Used:** Context from `dashboard` - Avoid using fragments when there is only one child component in a return statement. ([source](https://app.greptile.com/review/custom-context?memory=51414064-3127-4b1d-ad7e-62ce2c3739e9))
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| const newWidgets = Array.from(tab.widgets ?? []); | ||
| const [removed] = newWidgets.splice(result.source.index, 1); | ||
| newWidgets.splice(result.destination!.index, 0, removed); |
There was a problem hiding this comment.
style: Non-null assertion on result.destination is redundant since line 26 already returns if it's undefined
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/page-layout/hooks/useReorderPageLayoutWidgets.ts
Line: 34:34
Comment:
**style:** Non-null assertion on `result.destination` is redundant since line 26 already returns if it's undefined
How can I resolve this? If you propose a fix, please make it concise.| const newWidgets = Array.from(tab.widgets ?? []); | ||
| const [removed] = newWidgets.splice(result.source.index, 1); | ||
| newWidgets.splice(result.destination!.index, 0, removed); |
There was a problem hiding this comment.
style: Reordering logic is identical to useRecordIndexOptionsForTable - consider extracting to a shared reorder utility function
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/page-layout/hooks/useReorderPageLayoutWidgets.ts
Line: 32:34
Comment:
**style:** Reordering logic is identical to `useRecordIndexOptionsForTable` - consider extracting to a shared reorder utility function
How can I resolve this? If you propose a fix, please make it concise.| const pageLayoutId = | ||
| objectMetadataItem.nameSingular === CoreObjectNameSingular.Company | ||
| ? DEFAULT_PAGE_LAYOUT_ID | ||
| : selectedRecord?.pageLayoutId; |
There was a problem hiding this comment.
logic: pageLayoutId can be undefined if selectedRecord is null or doesn't have pageLayoutId. Hook on line 25 may not handle undefined properly. Does useSetIsPageLayoutInEditMode handle undefined pageLayoutId gracefully?
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/action-menu/actions/record-actions/single-record/record-page-layout-actions/components/EditRecordPageLayoutSingleRecordAction.tsx
Line: 22:22
Comment:
**logic:** pageLayoutId can be undefined if selectedRecord is null or doesn't have pageLayoutId. Hook on line 25 may not handle undefined properly. Does useSetIsPageLayoutInEditMode handle undefined pageLayoutId gracefully?
How can I resolve this? If you propose a fix, please make it concise.| const pageLayoutId = | ||
| objectMetadataItem.nameSingular === CoreObjectNameSingular.Company | ||
| ? DEFAULT_PAGE_LAYOUT_ID | ||
| : selectedRecord?.pageLayoutId; |
There was a problem hiding this comment.
logic: Fallback logic uses pageLayoutId from selectedRecord for non-Company objects, but selectedRecord might be null(line 15 allows this). Should verify selectedRecord exists before accessing pageLayoutId.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/action-menu/actions/record-actions/single-record/record-page-layout-actions/components/EditRecordPageLayoutSingleRecordAction.tsx
Line: 19:22
Comment:
**logic:** Fallback logic uses pageLayoutId from selectedRecord for non-Company objects, but selectedRecord might be null(line 15 allows this). Should verify selectedRecord exists before accessing pageLayoutId.
How can I resolve this? If you propose a fix, please make it concise.| shouldBeRegistered: ({ selectedRecord }) => | ||
| isDefined(selectedRecord) && | ||
| !selectedRecord?.isRemote && | ||
| !isDefined(selectedRecord?.deletedAt), | ||
| // TODO: Once backend is ready, uncomment the line below | ||
| // isDefined(selectedRecord?.pageLayoutId), |
There was a problem hiding this comment.
style: All three actions (EDIT_LAYOUT, SAVE_LAYOUT, CANCEL_LAYOUT_EDITION) have identical shouldBeRegistered logic. Consider extracting this to a shared constant or function to avoid duplication and ensure consistent behavior. Should these three actions have different registration conditions, particularly around edit mode state?
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/action-menu/actions/record-actions/constants/RecordPageLayoutActionsConfig.tsx
Line: 26:31
Comment:
**style:** All three actions (EDIT_LAYOUT, SAVE_LAYOUT, CANCEL_LAYOUT_EDITION) have identical `shouldBeRegistered` logic. Consider extracting this to a shared constant or function to avoid duplication and ensure consistent behavior. Should these three actions have different registration conditions, particularly around edit mode state?
How can I resolve this? If you propose a fix, please make it concise.| // TODO: Once backend is ready, uncomment the line below | ||
| // isDefined(selectedRecord?.pageLayoutId), |
There was a problem hiding this comment.
logic: The backend integration TODO should likely check for pageLayoutId OR ensure a default layout exists. Currently when uncommented, records without a pageLayoutId would never show these actions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/action-menu/actions/record-actions/constants/RecordPageLayoutActionsConfig.tsx
Line: 30:31
Comment:
**logic:** The backend integration TODO should likely check for `pageLayoutId` OR ensure a default layout exists. Currently when uncommented, records without a `pageLayoutId` would never show these actions.
How can I resolve this? If you propose a fix, please make it concise.| [PageLayoutSingleRecordActionKeys.EDIT_LAYOUT]: { | ||
| key: PageLayoutSingleRecordActionKeys.EDIT_LAYOUT, | ||
| label: msg`Edit Layout`, | ||
| shortLabel: msg`Edit`, | ||
| isPinned: true, | ||
| position: 0, | ||
| Icon: IconPencil, | ||
| type: ActionType.Standard, | ||
| scope: ActionScope.RecordSelection, | ||
| shouldBeRegistered: ({ selectedRecord }) => | ||
| isDefined(selectedRecord) && | ||
| !selectedRecord?.isRemote && | ||
| !isDefined(selectedRecord?.deletedAt), | ||
| // TODO: Once backend is ready, uncomment the line below | ||
| // isDefined(selectedRecord?.pageLayoutId), | ||
| availableOn: [ActionViewType.SHOW_PAGE], | ||
| component: <EditRecordPageLayoutSingleRecordAction />, | ||
| }, | ||
| [PageLayoutSingleRecordActionKeys.SAVE_LAYOUT]: { | ||
| key: PageLayoutSingleRecordActionKeys.SAVE_LAYOUT, | ||
| label: msg`Save Layout`, | ||
| shortLabel: msg`Save`, | ||
| isPinned: true, | ||
| position: 1, | ||
| Icon: IconDeviceFloppy, | ||
| type: ActionType.Standard, | ||
| scope: ActionScope.RecordSelection, | ||
| shouldBeRegistered: ({ selectedRecord }) => | ||
| isDefined(selectedRecord) && | ||
| !selectedRecord?.isRemote && | ||
| !isDefined(selectedRecord?.deletedAt), | ||
| // TODO: Once backend is ready, uncomment the line below | ||
| // isDefined(selectedRecord?.pageLayoutId), | ||
| availableOn: [ActionViewType.SHOW_PAGE], | ||
| component: <SaveRecordPageLayoutSingleRecordAction />, | ||
| }, | ||
| [PageLayoutSingleRecordActionKeys.CANCEL_LAYOUT_EDITION]: { | ||
| key: PageLayoutSingleRecordActionKeys.CANCEL_LAYOUT_EDITION, | ||
| label: msg`Cancel Edition`, | ||
| shortLabel: msg`Cancel`, | ||
| isPinned: true, | ||
| position: 2, | ||
| Icon: IconX, | ||
| type: ActionType.Standard, | ||
| scope: ActionScope.RecordSelection, | ||
| shouldBeRegistered: ({ selectedRecord }) => | ||
| isDefined(selectedRecord) && | ||
| !selectedRecord?.isRemote && | ||
| !isDefined(selectedRecord?.deletedAt), | ||
| // TODO: Once backend is ready, uncomment the line below | ||
| // isDefined(selectedRecord?.pageLayoutId), | ||
| availableOn: [ActionViewType.SHOW_PAGE], | ||
| component: <CancelRecordPageLayoutSingleRecordAction />, | ||
| }, |
There was a problem hiding this comment.
logic: The Save and Cancel actions should likely only be visible during edit mode, while Edit should only show when NOT in edit mode. Without state-based registration, all three actions may appear simultaneously.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-front/src/modules/action-menu/actions/record-actions/constants/RecordPageLayoutActionsConfig.tsx
Line: 17:70
Comment:
**logic:** The Save and Cancel actions should likely only be visible during edit mode, while Edit should only show when NOT in edit mode. Without state-based registration, all three actions may appear simultaneously.
How can I resolve this? If you propose a fix, please make it concise.| const pageLayoutId = | ||
| objectMetadataItem.nameSingular === CoreObjectNameSingular.Company | ||
| ? DEFAULT_PAGE_LAYOUT_ID | ||
| : selectedRecord?.pageLayoutId; | ||
|
|
||
| const { savePageLayout } = useSavePageLayout(pageLayoutId); |
There was a problem hiding this comment.
Bug: For non-Company records, selectedRecord?.pageLayoutId can be undefined, causing hooks like useSavePageLayout to throw an unhandled error and crash the action.
(Severity: High 0.70 | Confidence: 0.95)
🔍 Detailed Analysis
For non-Company records, the pageLayoutId is assigned from selectedRecord?.pageLayoutId, which can be undefined. This value is then passed to the useSavePageLayout and useSetIsPageLayoutInEditMode hooks. Both hooks use useAvailableComponentInstanceIdOrThrow, which throws an unhandled error if its input is not a non-empty string and no context is available. This will cause the UI action to crash when a user attempts to save the layout for a non-Company record where the pageLayoutId is not yet provided by the backend, as indicated by TODO comments in the code.
💡 Suggested Fix
Add a null check to ensure pageLayoutId is defined before passing it to the useSavePageLayout and useSetIsPageLayoutInEditMode hooks. Alternatively, re-enable the commented-out isDefined(selectedRecord?.pageLayoutId) check in RecordPageLayoutActionsConfig.tsx to prevent the action from being available when the ID is missing.
🤖 Prompt for AI Agent
Fix this bug. In
packages/twenty-front/src/modules/action-menu/actions/record-actions/single-record/record-page-layout-actions/components/SaveRecordPageLayoutSingleRecordAction.tsx
at lines 20-25: For non-Company records, the `pageLayoutId` is assigned from
`selectedRecord?.pageLayoutId`, which can be `undefined`. This value is then passed to
the `useSavePageLayout` and `useSetIsPageLayoutInEditMode` hooks. Both hooks use
`useAvailableComponentInstanceIdOrThrow`, which throws an unhandled error if its input
is not a non-empty string and no context is available. This will cause the UI action to
crash when a user attempts to save the layout for a non-Company record where the
`pageLayoutId` is not yet provided by the backend, as indicated by TODO comments in the
code.
Did we get this right? 👍 / 👎 to inform future reviews.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:54929 This environment will automatically shut down when the PR is closed or after 5 hours. |
CleanShot.2025-10-21.at.18.05.56.mp4