feat(metadata): deterministic universalIdentifiers for server-generated side-effects#21949
feat(metadata): deterministic universalIdentifiers for server-generated side-effects#21949Weiko wants to merge 10 commits into
Conversation
🔍 Visual Regression Review —
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 14 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/engine/metadata-modules/flat-command-menu-item/utils/build-navigation-flat-command-menu-item.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/flat-command-menu-item/utils/build-navigation-flat-command-menu-item.util.ts:75">
P2: Fallback UID derivation ignores `applicationUniversalIdentifier`, violating the PR's stated per-app two-level v5 rule. Any caller omitting `providedUniversalIdentifier` silently produces a UID that doesn't match the shared scheme.</violation>
</file>
<file name="packages/twenty-shared/src/application/deterministic-identifier/compute-deterministic-uuid.util.ts">
<violation number="1" location="packages/twenty-shared/src/application/deterministic-identifier/compute-deterministic-uuid.util.ts:5">
P2: Namespace parameter accepts any string but v5() requires a valid UUID. Publicly exported — a future caller passing a non-UUID string gets an opaque runtime TypeError.</violation>
</file>
<file name="packages/twenty-shared/src/application/deterministic-identifier/deterministic-identifier-discriminator.constant.ts">
<violation number="1" location="packages/twenty-shared/src/application/deterministic-identifier/deterministic-identifier-discriminator.constant.ts:46">
P2: navigationMenuItem discriminator can return '' when all inputs are null, causing UID collisions. Also, the ?? chain discards secondary identifiers, so two items that differ only in a later field would collide.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
🔍 Visual Regression Review —
|
| Story | Verdict | Confidence | Explained by | |
|---|---|---|---|---|
| 🟡 | ui-data-field-input-richtextfieldinput--default |
uncertain | 65% | — |
| 🟡 | modules-settings-playground-graphqlplayground--default |
uncertain | 60% | — |
| 🟡 | modules-auth-emailverificationsent--default |
uncertain | 55% | — |
| 🟡 | modules-settings-settingsoptioncardcontenttoggle--default |
uncertain | 55% | — |
| 🟡 | ui-input-imageinput-imageinput--default |
uncertain | 55% | — |
| 🟡 | modules-pagelayout-widgets-graphwidgetaggregatechart--default |
uncertain | 55% | — |
Changed stories
| Story | Diff % |
|---|---|
| ui-data-field-input-richtextfieldinput--default | 18% |
| modules-settings-playground-graphqlplayground--default | 3% |
| modules-auth-emailverificationsent--default | 1% |
| modules-settings-settingsoptioncardcontenttoggle--default | 1% |
| ui-input-imageinput-imageinput--default | 1% |
| modules-pagelayout-widgets-graphwidgetaggregatechart--default | 0% |
| ui-layout-draggablelist-draggablelist--default | 0% |
| modules-objectrecord-recordcalendar-month--default | 0% |
| modules-settings-datamodel-objects-forms-settingsdatamodelobjectaboutform--default | 0% |
View run details · advisory mode
🔍 Automated Pre-Review✅ No issues detected - This PR is ready for human review. Automated pre-review — human approval still required. |
Weiko
added
the
ci:allow-previous-version-upgrade-mutation
There was a problem hiding this comment.
1 issue found across 6 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/engine/metadata-modules/flat-command-menu-item/utils/build-navigation-flat-command-menu-item.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/flat-command-menu-item/utils/build-navigation-flat-command-menu-item.util.ts:75">
P2: Fallback UID derivation ignores `applicationUniversalIdentifier`, violating the PR's stated per-app two-level v5 rule. Any caller omitting `providedUniversalIdentifier` silently produces a UID that doesn't match the shared scheme.</violation>
</file>
<file name="packages/twenty-shared/src/application/deterministic-identifier/compute-deterministic-uuid.util.ts">
<violation number="1" location="packages/twenty-shared/src/application/deterministic-identifier/compute-deterministic-uuid.util.ts:5">
P2: Namespace parameter accepts any string but v5() requires a valid UUID. Publicly exported — a future caller passing a non-UUID string gets an opaque runtime TypeError.</violation>
</file>
<file name="packages/twenty-server/src/database/commands/upgrade-version-command/1-21/1-21-workspace-command-1775500013000-refactor-navigation-commands.command.ts">
<violation number="1" location="packages/twenty-server/src/database/commands/upgrade-version-command/1-21/1-21-workspace-command-1775500013000-refactor-navigation-commands.command.ts:205">
P2: Navigation-command universalIdentifier uses the old ad-hoc v5 formula instead of the new shared `getNavigationCommandUniversalIdentifier` helper, producing UIDs inconsistent with the PR's deterministic-ID standard and omitting the owner application UID from the computation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| workspaceId, | ||
| position: nextPosition++, | ||
| now, | ||
| universalIdentifier, |
There was a problem hiding this comment.
P2: Navigation-command universalIdentifier uses the old ad-hoc v5 formula instead of the new shared getNavigationCommandUniversalIdentifier helper, producing UIDs inconsistent with the PR's deterministic-ID standard and omitting the owner application UID from the computation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-server/src/database/commands/upgrade-version-command/1-21/1-21-workspace-command-1775500013000-refactor-navigation-commands.command.ts, line 205:
<comment>Navigation-command universalIdentifier uses the old ad-hoc v5 formula instead of the new shared `getNavigationCommandUniversalIdentifier` helper, producing UIDs inconsistent with the PR's deterministic-ID standard and omitting the owner application UID from the computation.</comment>
<file context>
@@ -202,6 +202,7 @@ export class RefactorNavigationCommandsCommand extends ActiveOrSuspendedWorkspac
workspaceId,
position: nextPosition++,
now,
+ universalIdentifier,
}),
);
</file context>
|
Discussed together, I think the overall approach is fine but we should create a battery of util per usecase and cover them with snapshot test to make sure we don't have regressions. |
There was a problem hiding this comment.
2 issues found across 45 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/database/commands/upgrade-version-command/1-21/1-21-workspace-command-1775500013000-refactor-navigation-commands.command.ts">
<violation number="1" location="packages/twenty-server/src/database/commands/upgrade-version-command/1-21/1-21-workspace-command-1775500013000-refactor-navigation-commands.command.ts:205">
P2: Navigation-command universalIdentifier uses the old ad-hoc v5 formula instead of the new shared `getNavigationCommandUniversalIdentifier` helper, producing UIDs inconsistent with the PR's deterministic-ID standard and omitting the owner application UID from the computation.</violation>
</file>
<file name="packages/twenty-server/src/engine/metadata-modules/index-metadata/utils/compute-flat-index-name.util.ts">
<violation number="1" location="packages/twenty-server/src/engine/metadata-modules/index-metadata/utils/compute-flat-index-name.util.ts:37">
P3: Duplicate index-field column resolution logic introduces drift risk between name generation and schema/index execution paths. A future rule change in one place can silently desynchronize deterministic names from actual indexed columns.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| }): string => { | ||
| const orderedIndexColumnNames = [...universalFlatIndexFieldMetadatas] | ||
| .sort((a, b) => a.order - b.order) | ||
| .map((flatIndexField) => { |
There was a problem hiding this comment.
P3: Duplicate index-field column resolution logic introduces drift risk between name generation and schema/index execution paths. A future rule change in one place can silently desynchronize deterministic names from actual indexed columns.
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/index-metadata/utils/compute-flat-index-name.util.ts, line 37:
<comment>Duplicate index-field column resolution logic introduces drift risk between name generation and schema/index execution paths. A future rule change in one place can silently desynchronize deterministic names from actual indexed columns.</comment>
<file context>
@@ -0,0 +1,101 @@
+}): string => {
+ const orderedIndexColumnNames = [...universalFlatIndexFieldMetadatas]
+ .sort((a, b) => a.order - b.order)
+ .map((flatIndexField) => {
+ const relatedFlatFieldMetadata = objectFlatFieldMetadatas.find(
+ (flatFieldMetadata) =>
</file context>
updated |
charlesBochet
requested review from
charlesBochet
and removed request for
charlesBochet
prastoin
left a comment
prastoin
left a comment
There was a problem hiding this comment.
As discussed and from my understanding, with the current implementation we're not gracefully and explicitly handling colliding deterministic universal identifier generation inside the same app
For example if a user declare two identical fields in its app we should early throw at the universal identifier level because I'm afraid it might be swallowed later in the engine
We should still focus on the "business" validation at the validator level but prevent buggy universal identifier override
charlesBochet
left a comment
charlesBochet
left a comment
There was a problem hiding this comment.
Left comments:
- I would add more tests to make sure we don't have regression on universalId generation
3 participants
Context
Server-generated "side-effect" entities created for every object (system fields, INDEX view, record-page fields view + view fields, search-vector index, navigation command, record page layout/tabs/widgets) were minted with random v4() ids. Because they were non-deterministic, nothing could reference them by id (e.g. point a view field at an object's createdAt field).
This PR introduces a single shared rule for deriving these ids deterministically via uuid v5, so the same (owner app, parent, kind) always yields the same id, making side-effects referable and reproducible.
This is the forward-only foundation (PR1). Follow-ups:
The rule
the id — two apps adding the same-named entity to a shared parent never collide.
Scope boundary: deterministic v5 applies to system side-effects (unique by construction) and, later, app-authored manifest entities (uniqueness enforced at SDK build time).
Entities created through the UI by the workspace "Custom" app (custom objects/views/fields) keep v4, their natural keys aren't unique and aren't enforced. A UI-created custom object keeps its v4 id; its side-effects are deterministic relative to that v4 parent.
Changes
twenty-shared: new application/deterministic-identifier/ module:
twenty-server: side-effect generators now derive universalIdentifier via the helpers (local id PKs stay v4()): system fields + name, INDEX view, record-page fields (fields-widget) view, default view fields, search-vector index, nav command, page layout/tabs/widgets. Index ids key off the generated Postgres index name; extracted computeFlatIndexNameOrThrow so the name (and therefore the id) is computed once with no placeholder.
Timeline
What actually changes
views, view fields, search index, nav command, page layout/tabs/widgets) instead of random v4().
What does NOT change
The one real-world impact / risk (existing workspaces)
The nav-command runtime lookup (findNavigationCommandMenuItemForObject) now computes the new formula, but existing workspaces' nav commands were stored with the old formula. So on an upgraded existing workspace, until the PR3 backfill:
What app developers get right now
Nothing usable yet. The helpers exist in twenty-shared but aren't re-exported from twenty-sdk (PR2), and app-authored objects still get SDK-derived ids in the old format until PR2
re-mints them. So "reference a server entity by deterministic id" doesn't work end-to-end until PR2