Skip to content

Migrate pagelayout position frontend#18229

Merged
Weiko merged 6 commits intomainfrom
c-migrate-pagelayout-position-frontend
Mar 2, 2026
Merged

Migrate pagelayout position frontend#18229
Weiko merged 6 commits intomainfrom
c-migrate-pagelayout-position-frontend

Conversation

@Weiko
Copy link
Copy Markdown
Member

@Weiko Weiko commented Feb 25, 2026

Context

Part 1 of migrating gridPosition in favor of typed position
FE should now always send both values to the BE and use both.

Next steps:

  • Update the backend to enforce and validate the new position field + DB migrations gridPositon -> position (type: GRID)
  • Cleanup frontend usage
  • Cleanup backend

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 12 files

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Feb 25, 2026

Greptile Summary

This PR migrates the frontend to use the new typed position field alongside the existing gridPosition field as part of a multi-step migration from gridPosition to position. The frontend now sends both values to the backend.

Key changes:

  • Added position field with layoutMode: GRID to all widget creation functions
  • Updated convertLayoutsToWidgets to populate both gridPosition and position
  • Updated convertPageLayoutDraftToUpdateInput to send position in GraphQL mutations
  • Modified convertPageLayoutToTabLayouts to prefer position over gridPosition when available
  • Added comprehensive test coverage for the new position field

Issues found:

  • convertLayoutsToWidgets.ts is missing the __typename property in the position object, which causes test failures and type inconsistencies

Confidence Score: 4/5

  • Safe to merge after fixing the missing __typename property
  • The migration strategy is sound and well-tested, with one syntax error that will cause test failures
  • Pay attention to convertLayoutsToWidgets.ts which needs the __typename fix

Important Files Changed

Filename Overview
packages/twenty-front/src/modules/page-layout/utils/convertLayoutsToWidgets.ts Added position field with layoutMode, but missing __typename property
packages/twenty-front/src/modules/page-layout/utils/convertPageLayoutDraftToUpdateInput.ts Added position field to GraphQL input (correctly without __typename)
packages/twenty-front/src/modules/page-layout/utils/convertPageLayoutToTabLayouts.ts Added fallback to use position field when available, with proper type checking
packages/twenty-front/src/modules/page-layout/utils/createDefaultGraphWidget.ts Added position field with __typename correctly included

Last reviewed commit: 9b45d69

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

12 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Copy link
Copy Markdown
Contributor

@Devessier Devessier left a comment

Choose a reason for hiding this comment

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

Left a few comments. I would like to discuss the best practice to create default widgets whose position makes sense with the layout mode they are used within.

Comment on lines +28 to +35
position: {
__typename: 'PageLayoutWidgetGridPosition',
layoutMode: PageLayoutTabLayoutMode.GRID,
row: gridPosition.row,
column: gridPosition.column,
rowSpan: gridPosition.rowSpan,
columnSpan: gridPosition.columnSpan,
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I feel we should instruct this function about the layout mode it should use. Indeed, we could create front component in record page layouts as well.

type: WidgetType.GRAPH,
configuration,
gridPosition,
position: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same thing here about layout mode

url,
},
gridPosition,
position: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same thing here about layout mode

body,
},
gridPosition,
position: {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same thing here about layout mode

@FelixMalfait
Copy link
Copy Markdown
Member

FelixMalfait commented Mar 2, 2026

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:15344

This environment will automatically shut down after 5 hours.

@Weiko Weiko enabled auto-merge March 2, 2026 13:40
@Weiko Weiko disabled auto-merge March 2, 2026 13:40
@Weiko Weiko merged commit 37bcb35 into main Mar 2, 2026
77 of 78 checks passed
@Weiko Weiko deleted the c-migrate-pagelayout-position-frontend branch March 2, 2026 13:42
@twenty-eng-sync
Copy link
Copy Markdown

Hey @Weiko! After you've done the QA of your Pull Request, you can mark it as done here. Thank you!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants