Skip to content

Implement page layout override#18472

Merged
Weiko merged 10 commits intomainfrom
c--implement-page-layout-override
Mar 9, 2026
Merged

Implement page layout override#18472
Weiko merged 10 commits intomainfrom
c--implement-page-layout-override

Conversation

@Weiko
Copy link
Copy Markdown
Member

@Weiko Weiko commented Mar 6, 2026

No description provided.

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 41 files

@FelixMalfait
Copy link
Copy Markdown
Member

FelixMalfait commented Mar 6, 2026

🚀 Preview Environment Ready!

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

This environment will automatically shut down after 5 hours.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Mar 6, 2026

Greptile Summary

This PR introduces a page-layout override system that allows workspace custom applications to override specific properties (title, position, icon, conditionalDisplay) of standard-application-owned page layout tabs and widgets without mutating the originals. Changes span a DB migration (adding a nullable overrides JSONB column to both tables), a new OverridableEntity base class, three core utility functions (isCallerOverridingEntity, sanitizeOverridableEntityInput, resolveOverridableEntityProperty), and updated entity/DTO/service wiring throughout the page layout stack.

Key changes:

  • New migration adds overrides jsonb column to pageLayoutTab and pageLayoutWidget tables
  • OverridableEntity<T> abstract base class keeps override data in a JSONB column typed to per-entity override shapes
  • isCallerOverridingEntity gates the override path: overrides are only applied when the caller is the workspace-custom app and the entity was created by a different (standard) application
  • sanitizeOverridableEntityInput separates overridable properties from the direct-update payload and manages the overrides JSONB object
  • Properties marked isOverridable: true in ALL_ENTITY_PROPERTIES_CONFIGURATION_BY_METADATA_NAME: tab (title, position, icon) and widget (title, position, conditionalDisplay)

Issues found:

  • The "reset override" branch in sanitizeOverridableEntityInput uses strict reference equality (===) which is correct for primitive fields but silently fails for widget object-typed overridable fields (position, conditionalDisplay), meaning stale overrides for those fields can never be cleared
  • sanitizeOverridableEntityInput mutates the caller-supplied updatedEditableProperties object in-place while also returning it — the mutation could surprise callers holding a separate reference
  • isCallerOverridingEntity uses an implicit arrow-function return, which conflicts with the project's style convention
  • No unit tests for the three new override utility functions despite their non-trivial branching

Confidence Score: 2/5

  • The PR is architecturally sound but has a correctness bug in override-clearing logic for object-typed widget properties that will cause silent data staleness, plus a surprising input mutation pattern and missing test coverage.
  • The PR is safe to merge functionally for primitive-typed overrides (all tab properties: title, position, icon). However, there is a correctness bug for widget object-typed properties (position and conditionalDisplay): strict reference equality (===) cannot clear stale overrides for object fields, meaning once an override is set for these properties, it can never be removed even when reset to the base value. Additionally, sanitizeOverridableEntityInput mutates its input parameter in-place, which violates the principle of least surprise. The missing unit tests for three core override utility functions add maintenance risk. Style violations are minor in comparison.
  • packages/twenty-server/src/engine/metadata-modules/utils/sanitize-overridable-entity-input.util.ts requires the most attention due to the object equality bug for widget overridable properties and the input mutation pattern.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Client calls updatePageLayoutTab / updatePageLayoutWidget"] --> B["Fetch workspaceCustomFlatApplication"]
    B --> C["Fetch existing FlatEntity from cache"]
    C --> D["extractAndSanitizeObjectStringFields\n(get editable properties)"]
    D --> E["isCallerOverridingEntity()\ncaller == workspaceCustomApp?\nentity belongs to standard app?"]
    E -->|"false (own entity)"| F["sanitizeOverridableEntityInput\nshouldOverride=false\npreserve existing overrides"]
    E -->|"true (standard entity)"| G["sanitizeOverridableEntityInput\nshouldOverride=true"]
    G --> H{"For each overridable property\nin updatedEditableProperties"}
    H -->|"value == base entity value"| I["Remove property from overrides\n(clear stale override)"]
    H -->|"value != base entity value"| J["Add/update property in overrides\nRemove from direct update payload"]
    H -->|"property not in update"| K["Keep existing override value"]
    I & J & K --> L["Merge: overrides=computed,\nupdatedEditableProperties=sanitized"]
    F --> L
    L --> M["validateBuildAndRunWorkspaceMigration"]
    M --> N["Persist to DB: entity updated,\noverrides JSONB column updated"]
Loading

Last reviewed commit: 22d420a

@Weiko Weiko closed this Mar 6, 2026
@Weiko Weiko reopened this Mar 9, 2026
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.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@Weiko Weiko closed this Mar 9, 2026
@Weiko Weiko reopened this Mar 9, 2026
@Weiko
Copy link
Copy Markdown
Member Author

Weiko commented Mar 9, 2026

@greptileai trigger

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.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

Copy link
Copy Markdown
Contributor

@thomtrp thomtrp left a comment

Choose a reason for hiding this comment

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

LGTM

overrides,
} as FlatPageLayoutWidget;

if (updatedEditableProperties.objectMetadataId !== undefined) {
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.

nit: isDefined for consistency?

@Weiko Weiko enabled auto-merge March 9, 2026 16:52
@Weiko Weiko added this pull request to the merge queue Mar 9, 2026
Merged via the queue into main with commit c433b2b Mar 9, 2026
85 checks passed
@Weiko Weiko deleted the c--implement-page-layout-override branch March 9, 2026 17:29
@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!

1 similar comment
@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants