fix: delete the default page layout tabs and widgets when deleting the default relation fields#19994
Conversation
📊 API Changes ReportGraphQL Schema ChangesGraphQL Schema Changes[error] Error: Unable to read JSON file: /home/runner/work/twenty/twenty/main-schema-introspection.json: Not valid JSON content GraphQL Metadata Schema ChangesGraphQL Metadata Schema Changes[error] Error: Unable to read JSON file: /home/runner/work/twenty/twenty/main-metadata-schema-introspection.json: Not valid JSON content REST API Analysis ErrorError OutputREST Metadata API Analysis ErrorError Output |
… timeline activities
![sentry[bot]](https://avatars.github.com/in/12637?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.github.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/modules/timeline/repositories/timeline-activity.repository.ts">
<violation number="1" location="packages/twenty-server/src/modules/timeline/repositories/timeline-activity.repository.ts:225">
P2: Morph-field guard is unsafe because it matches by field name globally, not by the timeline object/active field, so unrelated fields can cause false positives and bypass the intended protection.</violation>
</file>
<file name="packages/twenty-server/src/engine/metadata-modules/field-metadata/services/field-metadata.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/field-metadata/services/field-metadata.service.ts:201">
P1: Relation-tab cleanup is over-broad: tabs are deleted solely because they contain a matching relation widget, without ensuring the tab is default/system-only or free of unrelated widgets.</violation>
<violation number="2" location="packages/twenty-server/src/engine/metadata-modules/field-metadata/services/field-metadata.service.ts:234">
P1: Both the existing `flatPageLayoutWidgetsToDelete` spread and the new `relationPageLayoutWidgetsToDelete` spread produce the same `pageLayoutWidget` key. When both conditions are true, JavaScript object spread causes the second to overwrite the first, silently dropping `flatPageLayoutWidgetsToDelete`. Merge both arrays into a single `pageLayoutWidget.flatEntityToDelete` entry to ensure all widgets are deleted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
abusarah-tech
changed the title
There was a problem hiding this comment.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-server/src/modules/timeline/repositories/timeline-activity.repository.ts">
<violation number="1" location="packages/twenty-server/src/modules/timeline/repositories/timeline-activity.repository.ts:226">
P1: The new morph-field existence check compares `object.id` against `field.objectMetadataUniversalIdentifier`, mixing ID domains and likely returning false for valid fields.</violation>
</file>
<file name="packages/twenty-server/src/engine/metadata-modules/field-metadata/services/field-metadata.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/field-metadata/services/field-metadata.service.ts:211">
P1: Tab deletion checks only relation-widget subset, so tabs containing other active widgets can be incorrectly deleted.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| relationPageLayoutWidgetsToDelete | ||
| .filter( | ||
| (widget) => | ||
| widgetsCountByPageLayoutTabIds[widget.pageLayoutTabId] === 1, |
There was a problem hiding this comment.
P1: Tab deletion checks only relation-widget subset, so tabs containing other active widgets can be incorrectly deleted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/engine/metadata-modules/field-metadata/services/field-metadata.service.ts, line 211:
<comment>Tab deletion checks only relation-widget subset, so tabs containing other active widgets can be incorrectly deleted.</comment>
<file context>
@@ -192,8 +192,24 @@ export class FieldMetadataService extends TypeOrmQueryService<FieldMetadataEntit
relationPageLayoutWidgetsToDelete
+ .filter(
+ (widget) =>
+ widgetsCountByPageLayoutTabIds[widget.pageLayoutTabId] === 1,
+ )
.map((widget) => widget.pageLayoutTabId)
</file context>
There was a problem hiding this comment.
though we are checking also if it is a default widget too so not sure if this issue is valid
|
Hi, FieldMetadataService.deleteOneField deletes all page-layout widgets for the object whose widget configurationType matches the deleted relation field name (e.g. TIMELINE/TASKS), which can remove unrelated user-created/custom widgets. Severity: action required | Category: correctness How to fix: Restrict deletions to defaults Agent prompt to fix - you can give this to your LLM of choice:
Found by Qodo code review |
2 participants
This PR should solve the issue #19990.
A summary of the code changes:
deleteOneFieldinfield-metadata.service.ts, we fetch the existing page layout tabs and widgetsRELATIONand are matching the current default layout tabs