From 7b682bced9be2e75a94db2bab09f4e19e6e85d3f Mon Sep 17 00:00:00 2001 From: martmull Date: Thu, 2 Jul 2026 10:03:52 +0200 Subject: [PATCH] feat(shared): require defaultValue on non-nullable field manifests (#22419) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Context Follow-up to #22362, which made `isNullable` manifest changes actually apply (including a nullable → non-nullable backfill). This models the `isNullable` / `defaultValue` relationship directly in the `FieldManifest` type. ## Rule - A **non-nullable** field (`isNullable: false`) must declare a `defaultValue`, so the column always has a value to fall back on (e.g. for the backfill on the nullable → non-nullable transition). - A **nullable** or **unspecified** field may omit `defaultValue`. ## Changes - Split `RegularFieldManifest` into a base shape plus a discriminated nullability union. The union keeps `isNullable` free once a `defaultValue` is supplied, so helpers that always provide one can still pass a dynamic `boolean` `isNullable`. - `defaultValue` keeps its rich per-type `FieldMetadataDefaultValue` (POSITION → number, ACTOR → composite) rather than a bare `string`. - `RelationFieldManifest` is rebased on the shared base and keeps `isNullable` / `defaultValue` optional, since relation join columns are always nullable by design. - Narrowed `buildEstimateFieldManifest` in the manifest-update integration test to satisfy the stricter type. ## Verification Environment couldn't install the monorepo deps (registry connections aborting), so `nx typecheck` wasn't run here. Validated the union structure with standalone `tsc` synthetic tests mirroring every construction pattern in the codebase: - ✅ nullable/no-default, no-`isNullable`, non-nullable with string/number/composite defaults, dynamic-boolean-with-default, and the `DistributiveOmit` path into `ObjectFieldManifest` - ✅ non-nullable **without** a default is correctly rejected with a clear "defaultValue is missing but required" error Recommend a full `nx typecheck twenty-shared twenty-sdk twenty-server` in CI to confirm against full project resolution. https://claude.ai/code/session_01VnbrgBB3kNGP876qaKPYDL --- _Generated by [Claude Code](https://claude.ai/code/session_01VnbrgBB3kNGP876qaKPYDL)_ Review in cubic --- ...-manifest-update-field.integration-spec.ts | 50 ++++++++++++------- .../src/application/fieldManifestType.ts | 41 +++++++-------- .../twenty-ui/src/input/Slider/Slider.tsx | 1 - 3 files changed, 53 insertions(+), 39 deletions(-) diff --git a/packages/twenty-server/test/integration/metadata/suites/application/successful-manifest-update-field.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/application/successful-manifest-update-field.integration-spec.ts index 3e47c2ebf9..038a4a3feb 100644 --- a/packages/twenty-server/test/integration/metadata/suites/application/successful-manifest-update-field.integration-spec.ts +++ b/packages/twenty-server/test/integration/metadata/suites/application/successful-manifest-update-field.integration-spec.ts @@ -87,23 +87,37 @@ const buildReferenceFieldManifest = (isNullable: boolean): FieldManifest => ({ // coerces null/omitted TEXT values to '' via the null-equivalent processor, // so a TEXT column can never actually hold NULL. NUMBER preserves NULL, which // is what the nullable -> non-nullable backfill needs to act on. -const buildEstimateFieldManifest = ({ - isNullable, - defaultValue, -}: { - isNullable: boolean; - defaultValue?: number; -}): FieldManifest => ({ - universalIdentifier: TEST_NUMBER_FIELD_ID, - type: FieldMetadataType.NUMBER, - name: 'estimate', - label: 'Estimate', - description: 'Ticket estimate', - icon: 'IconNumber', - isNullable, - ...(isDefined(defaultValue) ? { defaultValue } : {}), - objectUniversalIdentifier: TEST_OBJECT.universalIdentifier, -}); +const buildEstimateFieldManifest = ( + params: + | { isNullable: true; defaultValue?: number } + | { isNullable: false; defaultValue: number }, +): FieldManifest => { + const commonEstimateFields = { + universalIdentifier: TEST_NUMBER_FIELD_ID, + type: FieldMetadataType.NUMBER as const, + name: 'estimate', + label: 'Estimate', + description: 'Ticket estimate', + icon: 'IconNumber', + objectUniversalIdentifier: TEST_OBJECT.universalIdentifier, + }; + + if (params.isNullable) { + return { + ...commonEstimateFields, + isNullable: true, + ...(isDefined(params.defaultValue) + ? { defaultValue: params.defaultValue } + : {}), + }; + } + + return { + ...commonEstimateFields, + isNullable: false, + defaultValue: params.defaultValue, + }; +}; const createTicketRecord = async (data: Record) => { const response = await makeGraphqlAPIRequest( @@ -415,7 +429,7 @@ describe('Manifest update - fields', () => { description: 'Unique external identifier', icon: 'IconId', isUnique: true, - isNullable: false, + isNullable: true, objectUniversalIdentifier: TEST_OBJECT.universalIdentifier, }, ], diff --git a/packages/twenty-shared/src/application/fieldManifestType.ts b/packages/twenty-shared/src/application/fieldManifestType.ts index 75c4486a39..1d93f3f4f9 100644 --- a/packages/twenty-shared/src/application/fieldManifestType.ts +++ b/packages/twenty-shared/src/application/fieldManifestType.ts @@ -7,7 +7,7 @@ import { type RelationAndMorphRelationFieldMetadataType, } from '@/types'; -export type RegularFieldManifest< +type BaseRegularFieldManifest< T extends FieldMetadataType = Exclude< FieldMetadataType, RelationAndMorphRelationFieldMetadataType @@ -18,36 +18,37 @@ export type RegularFieldManifest< label: string; description?: string; icon?: string; - /** - * Default value in the canonical metadata format. - * - * Literal string defaults must be wrapped in single quotes inside the - * string (e.g. `"'Draft'"`), including string sub-fields of composite - * defaults (e.g. `{ source: "'MANUAL'" }`) and SELECT/MULTI_SELECT values. - * Unquoted strings are reserved for computed defaults such as `'uuid'` - * and `'now'`; any other unquoted string raises a validation warning. - */ - defaultValue?: FieldMetadataDefaultValue; options?: FieldMetadataOptions; universalSettings?: FieldMetadataUniversalSettings; - isNullable?: boolean; - // When false, this field is not editable through the generic UI isUIEditable?: boolean; - /** - * @deprecated Use defineIndex({ isUnique: true, fields: [...] }) instead. - * Indexes are the SDK primitive for uniqueness — they support both single- - * and multi-column unique constraints with a single, consistent API. This - * field still works but will be removed in a future release. - */ isUnique?: boolean; objectUniversalIdentifier: string; }; +type RegularFieldManifestNullability = + | { + defaultValue: FieldMetadataDefaultValue; + isNullable?: boolean; + } + | { + defaultValue?: FieldMetadataDefaultValue; + isNullable?: true; + }; + +export type RegularFieldManifest< + T extends FieldMetadataType = Exclude< + FieldMetadataType, + RelationAndMorphRelationFieldMetadataType + >, +> = BaseRegularFieldManifest & RegularFieldManifestNullability; + export type RelationFieldManifest< T extends RelationAndMorphRelationFieldMetadataType = RelationAndMorphRelationFieldMetadataType, -> = Omit, 'universalSettings' | 'type'> & { +> = Omit, 'universalSettings' | 'type'> & { type: T; + isNullable?: boolean; + defaultValue?: FieldMetadataDefaultValue; relationTargetFieldMetadataUniversalIdentifier: string; relationTargetObjectMetadataUniversalIdentifier: string; universalSettings: FieldMetadataUniversalSettings; diff --git a/packages/twenty-ui/src/input/Slider/Slider.tsx b/packages/twenty-ui/src/input/Slider/Slider.tsx index d8adf3b85c..f79996b0a9 100644 --- a/packages/twenty-ui/src/input/Slider/Slider.tsx +++ b/packages/twenty-ui/src/input/Slider/Slider.tsx @@ -40,7 +40,6 @@ const getSliderProgress = ({ return Math.min(1, Math.max(0, (value - min) / (max - min))); }; - const getSliderFill = ({ max, min,