fix: enable save button when changing currency default value by mabdullahabaid · Pull Request #16864 · twentyhq/twenty

mabdullahabaid · 2025-12-30T17:02:21Z

Problem

When editing a currency field’s default value (for example, changing from USD to UYU), the Save button remained disabled. However, if the format setting was changed first (short → full → short), the Save button would then work correctly for currency changes.

Root Cause

The form was using React Hook Form’s values mode, which did not properly track dirty state for the defaultValue and settings fields.

Solution

Switched to the defaultValues + reset() pattern:

Initialize the form with defaultValues (using placeholder values)
Call reset() when fieldMetadataItem loads

This correctly tracks dirty state by comparing against the most recent reset values.

Note

A similar pattern is used in:

useWebhookForm
useImapSmtpCaldavConnectionForm

Those hooks call reset() via query callbacks. In this case, reset() is called inside a useEffect because the data comes from synchronous Recoil state rather than an async query.

Found two solutions to the problem, created commits for both. I feel the useEffect pattern makes more sense here since it's cleaner in terms of code.

greptile-apps

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

mabdullahabaid · 2025-12-30T17:13:33Z

packages/twenty-front/src/pages/settings/data-model/SettingsObjectFieldEdit.tsx

    resolver: zodResolver(settingsFieldFormSchema()),
-    values: {
-      icon: fieldMetadataItem?.icon ?? 'Icon',
-      type: fieldMetadataItem?.type as SettingsFieldType,
-      label: fieldMetadataItem?.label ?? '',
-      description: fieldMetadataItem?.description,
-      isLabelSyncedWithName: fieldMetadataItem?.isLabelSyncedWithName ?? true,
-      settings: fieldMetadataItem?.settings,
+    defaultValues: {
+      icon: 'Icon',
+      type: FieldMetadataType.TEXT as SettingsFieldType,
+      label: '',
+      description: null,
+      isLabelSyncedWithName: true,
+      settings: undefined,
+      defaultValue: undefined,
    },
  });


The name field is intentionally excluded and it has been the case before the changes. It's managed by a child component (SettingsDataModelFieldIconLabelForm) which has its own Controller with defaultValue={fieldMetadataItem?.name}.

cubic-dev-ai

No issues found across 1 file

github-actions · 2025-12-30T17:13:29Z

🚀 Preview Environment Ready!

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

This environment will automatically shut down when the PR is closed or after 5 hours.

cursor · 2026-01-06T11:45:08Z

You have run out of free Bugbot PR reviews for this billing cycle. This will reset on February 15.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

FelixMalfait · 2026-01-06T11:48:46Z

The first commit seemed like a better solution to me, we're trying to get rid of all useEffect.
But in the first commit you seem to do a lot of casting/transformation so what I would do in that case is try to go back to the root (why isn't it properly formatted/typed?) and fix that. If it cannot be fixed cleanly then justify why exactly

FelixMalfait · 2026-01-06T11:50:29Z

Just read the description I had missed it sorry, you mention useWebhookForm/useImapSmtpCaldavConnectionForm but they don't need an effect do they?

mabdullahabaid · 2026-01-06T12:56:13Z

You're right — those hooks don't need a useEffect because they call reset() directly in their query's onCompleted callback when async data arrives.

In this case, fieldMetadataItem comes from synchronous Recoil state (via useFieldMetadataItem), so there's no query callback to hook into. The useEffect watches for when fieldMetadataItem becomes defined and then calls reset().

However, I see your broader point: if we're trying to eliminate useEffect across the codebase, this comparison doesn't really justify adding a new one here. Will do some reworking here to figure out how to use the first pattern better.

…alization and remove useEffect

mabdullahabaid · 2026-01-06T14:45:06Z

packages/twenty-front/src/pages/settings/data-model/SettingsObjectFieldEdit.tsx

      description: fieldMetadataItem?.description,
      isLabelSyncedWithName: fieldMetadataItem?.isLabelSyncedWithName ?? true,
-      settings: fieldMetadataItem?.settings,
+      settings,
+      defaultValue,
    },
  });


We use the values prop to avoid useEffect, consistent with the codebase patterns. React Hook Form performs deep comparison, so isDirty is not reset when values are unchanged.

cubic-dev-ai

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="packages/twenty-front/src/modules/object-metadata/utils/getFieldMetadataItemInitialValues.ts">

<violation number="1" location="packages/twenty-front/src/modules/object-metadata/utils/getFieldMetadataItemInitialValues.ts:25">
P2: Missing validation for `currencyCode` when `defaultValue` exists. The existing `useCurrencySettingsFormInitialValues.ts` hook validates that `currencyCode` is a non-empty string (after stripping quotes) and falls back to USD if invalid. This function should apply the same validation to handle cases where `defaultValue` exists but `currencyCode` is missing or empty.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

cubic-dev-ai · 2026-01-06T14:06:38Z

packages/twenty-front/src/modules/object-metadata/utils/getFieldMetadataItemInitialValues.ts

+  const defaultValue = fieldMetadataItem.defaultValue
+    ? {
+        amountMicros: fieldMetadataItem.defaultValue.amountMicros ?? null,
+        currencyCode: fieldMetadataItem.defaultValue.currencyCode,


P2: Missing validation for currencyCode when defaultValue exists. The existing useCurrencySettingsFormInitialValues.ts hook validates that currencyCode is a non-empty string (after stripping quotes) and falls back to USD if invalid. This function should apply the same validation to handle cases where defaultValue exists but currencyCode is missing or empty.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/object-metadata/utils/getFieldMetadataItemInitialValues.ts, line 25: <comment>Missing validation for `currencyCode` when `defaultValue` exists. The existing `useCurrencySettingsFormInitialValues.ts` hook validates that `currencyCode` is a non-empty string (after stripping quotes) and falls back to USD if invalid. This function should apply the same validation to handle cases where `defaultValue` exists but `currencyCode` is missing or empty.</comment> <file context> @@ -0,0 +1,33 @@ + const defaultValue = fieldMetadataItem.defaultValue + ? { + amountMicros: fieldMetadataItem.defaultValue.amountMicros ?? null, + currencyCode: fieldMetadataItem.defaultValue.currencyCode, + } + : { </file context>

✅ Addressed in be42bfc

…InitialValues to use helper

mabdullahabaid · 2026-01-06T15:35:18Z

packages/twenty-front/src/modules/object-metadata/utils/getFieldMetadataItemInitialValues.ts

+        currencyCode: isNonEmptyString(
+          stripSimpleQuotesFromString(
+            fieldMetadataItem.defaultValue.currencyCode,
+          ),
+        )
+          ? fieldMetadataItem.defaultValue.currencyCode
+          : applySimpleQuotesToString(CurrencyCode.USD),


packages/twenty-front/src/pages/settings/data-model/SettingsObjectFieldEdit.tsx

      label: fieldMetadataItem?.label ?? '',
      description: fieldMetadataItem?.description,
      isLabelSyncedWithName: fieldMetadataItem?.isLabelSyncedWithName ?? true,
-      settings: fieldMetadataItem?.settings,
+      settings,
+      defaultValue,
    },
  });


…taItemInitialValues

mabdullahabaid · 2026-01-06T16:41:01Z

packages/twenty-front/src/pages/settings/data-model/SettingsObjectFieldEdit.tsx

      label: fieldMetadataItem?.label ?? '',
      description: fieldMetadataItem?.description,
      isLabelSyncedWithName: fieldMetadataItem?.isLabelSyncedWithName ?? true,
-      settings: fieldMetadataItem?.settings,
+      settings,
+      defaultValue,
    },
  });



The values prop is appropriate here because:

It syncs the form with external state (fieldMetadataItem)

React Hook Form performs deep comparison, so isDirty should work correctly

It avoids useEffect, following the codebase pattern

mabdullahabaid · 2026-01-06T16:40:06Z

Introduced a helper, removed useEffect and made sure the default currency has the format needed by the form schema (i.e. single quoted like 'USD').

Also made the code DRY so that the default value initialization/formatting happens at a single place for both creating a new currency field and updating an existing one.

twenty-eng-sync · 2026-01-06T18:25:30Z

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

twenty-eng-sync · 2026-01-06T18:25:32Z

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

github-actions · 2026-01-06T18:27:30Z

Thanks @mabdullahabaid for your contribution!
This marks your 90th PR on the repo. You're top 1% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

…Save 按鈕 dirty state (twentyhq#16864)

mabdullahabaid added 2 commits December 30, 2025 21:44

fix: initialize currency defaultValue and settings in form values

eadc43b

refactor: use defaultValues + reset pattern for field edit form

7c39547

greptile-apps bot reviewed Dec 30, 2025

View reviewed changes

mabdullahabaid requested a review from FelixMalfait December 30, 2025 17:02

sentry bot reviewed Dec 30, 2025

View reviewed changes

cubic-dev-ai bot reviewed Dec 30, 2025

View reviewed changes

mabdullahabaid added the -PR: awaiting review label Dec 30, 2025

mabdullahabaid and others added 2 commits December 30, 2025 22:14

Merge branch 'main' into issue-16792

d36745b

Merge branch 'main' into issue-16792

1162eab

fix: initialize currency field values via helper to handle quote seri…

ed2f584

…alization and remove useEffect

sentry bot reviewed Jan 6, 2026

View reviewed changes

cubic-dev-ai bot reviewed Jan 6, 2026

View reviewed changes

fix: add currencyCode validation and refactor useCurrencySettingsForm…

be42bfc

…InitialValues to use helper

sentry bot reviewed Jan 6, 2026

View reviewed changes

refactor: simplify currencyCode normalization logic in getFieldMetada…

2d1e075

…taItemInitialValues

sentry bot reviewed Jan 6, 2026

View reviewed changes

chore: improvements

aece73e

FelixMalfait approved these changes Jan 6, 2026

View reviewed changes

FelixMalfait added this pull request to the merge queue Jan 6, 2026

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 6, 2026

FelixMalfait added this pull request to the merge queue Jan 6, 2026

Merged via the queue into main with commit a46619e Jan 6, 2026
68 checks passed

FelixMalfait deleted the issue-16792 branch January 6, 2026 18:25

NotYen pushed a commit to NotYen/twenty-ym that referenced this pull request Jan 30, 2026

[Fix, Add] 手動合併開源 bugfix：圖表 toggle 無法點擊 (twentyhq#16874)、Currency 欄位 …

466d89f

…Save 按鈕 dirty state (twentyhq#16864)

Conversation

mabdullahabaid commented Dec 30, 2025

Problem

Root Cause

Solution

Note

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

mabdullahabaid Dec 30, 2025

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Dec 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursor bot commented Jan 6, 2026

Uh oh!

FelixMalfait commented Jan 6, 2026

Uh oh!

FelixMalfait commented Jan 6, 2026

Uh oh!

mabdullahabaid commented Jan 6, 2026

Uh oh!

This comment was marked as outdated.

Uh oh!

mabdullahabaid Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

Uh oh!

cubic-dev-ai bot Jan 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

mabdullahabaid Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

Uh oh!

mabdullahabaid Jan 6, 2026

Choose a reason for hiding this comment

Uh oh!

mabdullahabaid commented Jan 6, 2026

Uh oh!

Uh oh!

Uh oh!

twenty-eng-sync bot commented Jan 6, 2026

Uh oh!

twenty-eng-sync bot commented Jan 6, 2026

Uh oh!

github-actions bot commented Jan 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions bot commented Dec 30, 2025 •

edited

Loading

cubic-dev-ai bot Jan 6, 2026 •

edited

Loading