feat: implement field calculations for numeric fields#19088
feat: implement field calculations for numeric fields#19088mebishnusahu0595 wants to merge 1 commit intotwentyhq:mainfrom
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Welcome!
Hello there, congrats on your first PR! We're excited to have you contributing to this project. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 13 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/api/common/common-args-processors/data-arg-processor/data-arg-processor.service.ts">
<violation number="1" location="packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/data-arg-processor.service.ts:223">
P2: Formula evaluation errors are swallowed; the write proceeds with potentially stale or client-supplied calculated-field values when evaluation fails.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| fullRecord[field.name] = record[field.name]; | ||
| } catch (error) { | ||
| // Log error or set to null/error value | ||
| console.error(error.message); |
There was a problem hiding this comment.
P2: Formula evaluation errors are swallowed; the write proceeds with potentially stale or client-supplied calculated-field values when evaluation fails.
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/api/common/common-args-processors/data-arg-processor/data-arg-processor.service.ts, line 223:
<comment>Formula evaluation errors are swallowed; the write proceeds with potentially stale or client-supplied calculated-field values when evaluation fails.</comment>
<file context>
@@ -167,7 +172,61 @@ export class DataArgProcessorService {
+ fullRecord[field.name] = record[field.name];
+ } catch (error) {
+ // Log error or set to null/error value
+ console.error(error.message);
+ }
+ }
</file context>
| console.error(error.message); | |
| throw error; |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| flatObjectMetadata: FlatObjectMetadata; | ||
| existingRecords?: Partial<ObjectRecord>[]; | ||
| }): Promise<Partial<ObjectRecord>[]> { | ||
| const fields = Object.values(flatFieldMetadataMaps).flat(); |
There was a problem hiding this comment.
Bug: Object.values(flatFieldMetadataMaps) is used instead of Object.values(flatFieldMetadataMaps.byUniversalIdentifier). This results in an empty fields array, silently disabling the calculated fields feature.
Severity: HIGH
Suggested Fix
Change the line to const fields = Object.values(flatFieldMetadataMaps.byUniversalIdentifier).filter(isDefined); to correctly iterate over the field metadata objects stored in the byUniversalIdentifier property.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/data-arg-processor.service.ts#L194
Potential issue: The code attempts to get a list of field metadata by calling
`Object.values(flatFieldMetadataMaps)`. However, `flatFieldMetadataMaps` is an object
containing properties like `byUniversalIdentifier`, not a direct map of metadata. The
correct way to get the field metadata is by accessing the `byUniversalIdentifier`
property first. The current implementation results in the `fields` array being populated
with dictionary objects instead of field metadata, causing the subsequent filter to
always produce an empty array. Consequently, the `calculatedFields` array is always
empty, and the server-side calculated fields feature is silently never executed.
Did we get this right? 👍 / 👎 to inform future reviews.
| await this.globalWorkspaceOrmManager.getGlobalWorkspaceDataSource(); | ||
| const repository = dataSource.getRepository(flatObjectMetadata.nameSingular); | ||
|
|
||
| const existingRecord = await repository.findOne({ |
There was a problem hiding this comment.
Bug: A call to dataSource.getRepository() is made in computeArgs before the required workspace context is set, which will cause a runtime crash on every updateOne operation.
Severity: CRITICAL
Suggested Fix
The logic that calls dataSource.getRepository() needs to be moved to a point in the execution flow after the workspace context has been established, likely inside the executeInWorkspaceContext callback, similar to existing patterns in the codebase.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-server/src/engine/api/common/common-query-runners/common-update-one-query-runner.service.ts#L79
Potential issue: In the `computeArgs` method, `dataSource.getRepository()` is called
before the workspace context is established. The `getRepository` method's call chain
leads to `getWorkspaceContext()`, which relies on `AsyncLocalStorage` to retrieve a
context. Since `computeArgs` is executed before `executeInWorkspaceContext` sets this
context, `getWorkspaceContext()` will throw an error. This will cause a runtime crash on
every `updateOne` operation, regardless of whether calculated fields are involved.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Pull request overview
Implements numeric Field Calculations using Excel-style formulas, enabling server-side recomputation of calculated number fields and marking them read-only in the UI.
Changes:
- Added
calculationFormulato number field metadata/settings (shared + front types). - Introduced
FieldCalculationService(expr-eval) and integrated calculation into server arg processing. - Added settings UI input for formulas and client-side read-only protection for calculated fields.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| yarn.lock | Locks new expr-eval dependency. |
| packages/twenty-server/package.json | Adds expr-eval dependency for formula evaluation. |
| packages/twenty-shared/src/types/FieldMetadataSettings.ts | Extends number field settings with calculationFormula. |
| packages/twenty-server/src/engine/api/common/field-calculation/field-calculation.service.ts | New service for parsing/evaluating formulas and dependency sorting. |
| packages/twenty-server/src/engine/api/common/field-calculation/field-calculation.service.spec.ts | Unit tests for evaluation and dependency sorting/cycle detection. |
| packages/twenty-server/src/engine/api/common/common-args-processors/common-args-processors.ts | Registers FieldCalculationService provider. |
| packages/twenty-server/src/engine/api/common/common-args-processors/data-arg-processor/data-arg-processor.service.ts | Computes calculated fields during arg processing (create/update). |
| packages/twenty-server/src/engine/api/common/common-query-runners/common-update-one-query-runner.service.ts | Attempts to load existing record to support update-time calculations. |
| packages/twenty-front/src/modules/settings/components/SettingsOptions/SettingsOptionCardContentInput.tsx | New settings card layout component for an inline input. |
| packages/twenty-front/src/modules/settings/data-model/fields/forms/number/components/SettingsDataModelFieldNumberForm.tsx | Adds formula input to number field settings form. |
| packages/twenty-front/src/modules/object-record/record-field/ui/types/FieldMetadata.ts | Adds calculationFormula to number field UI metadata type. |
| packages/twenty-front/src/modules/object-record/record-field/ui/hooks/usePersistField.ts | Prevents persisting values for calculated number fields. |
| packages/twenty-front/src/modules/object-record/read-only/utils/isRecordFieldReadOnly.ts | Marks fields with calculationFormula as read-only in UI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <Separator /> | ||
| <SettingsOptionCardContentInput | ||
| Icon={IconFunction} | ||
| title={t`Calculation formula`} | ||
| description={t`Excel-like formula (e.g. Price * Quantity)`} | ||
| > | ||
| <TextInput | ||
| value={value?.calculationFormula ?? ''} | ||
| onChange={(formula) => | ||
| onChange({ ...value, calculationFormula: formula }) | ||
| } | ||
| disabled={disabled} | ||
| placeholder={t`Enter formula`} | ||
| /> | ||
| </SettingsOptionCardContentInput> |
There was a problem hiding this comment.
Now that settings includes calculationFormula, the existing onChange handlers for number type / decimals (above) must preserve it. As written, those handlers replace the whole settings object (only setting type/decimals), so a user-entered formula will be cleared when changing type or decimals; update them to merge with the current value.
| return fields.filter( | ||
| (field) => | ||
| field.type === FieldMetadataType.NUMBER && | ||
| isDefined(field.settings?.calculationFormula), | ||
| ); |
There was a problem hiding this comment.
calculationFormula is treated as "defined" even when it is an empty string, which makes the backend consider the field calculated while other code paths treat empty string as "no formula" (e.g. if (formula) checks). Consider filtering by a non-empty (trimmed) formula instead (and/or normalizing empty strings to undefined when saving settings) so backend and frontend read-only/calculation behavior stays consistent.
| return fields.filter( | |
| (field) => | |
| field.type === FieldMetadataType.NUMBER && | |
| isDefined(field.settings?.calculationFormula), | |
| ); | |
| return fields.filter((field) => { | |
| if (field.type !== FieldMetadataType.NUMBER) { | |
| return false; | |
| } | |
| const formula = field.settings?.calculationFormula; | |
| if (!isDefined(formula)) { | |
| return false; | |
| } | |
| if (typeof formula === 'string' && formula.trim() === '') { | |
| return false; | |
| } | |
| return true; | |
| }); |
| decimals: | ||
| fieldMetadataItem?.settings?.decimals ?? DEFAULT_DECIMAL_VALUE, | ||
| type: fieldMetadataItem?.settings?.type ?? 'number', | ||
| calculationFormula: fieldMetadataItem?.settings?.calculationFormula ?? '', | ||
| }} |
There was a problem hiding this comment.
calculationFormula defaults to an empty string even though the schema marks it optional. If the backend distinguishes between "unset" and "set", consider normalizing '' to undefined when persisting to avoid saving empty formulas (and to align with truthy checks used for read-only behavior).
| return this.calculateFields({ | ||
| processedRecords, | ||
| flatFieldMetadataMaps, | ||
| flatObjectMetadata, | ||
| existingRecords: arguments[0].existingRecords, | ||
| }); |
There was a problem hiding this comment.
existingRecords is accessed via arguments[0].existingRecords instead of being destructured from the process parameter object. This is brittle and makes the method harder to refactor; destructure existingRecords in the function signature and pass it through explicitly.
| try { | ||
| record[field.name] = this.fieldCalculationService.evaluate( | ||
| formula, | ||
| fullRecord, | ||
| ); | ||
| fullRecord[field.name] = record[field.name]; | ||
| } catch (error) { | ||
| // Log error or set to null/error value | ||
| console.error(error.message); | ||
| } |
There was a problem hiding this comment.
Calculation errors are swallowed and only logged with console.error, which can leave calculated fields stale/incorrect without notifying API consumers. Consider using Nest's logger and returning a structured error (e.g. CommonQueryRunnerException with a user-friendly message) or explicitly setting the calculated field to null/invalid to avoid silent failures.
| const dataSource = | ||
| await this.globalWorkspaceOrmManager.getGlobalWorkspaceDataSource(); | ||
| const repository = dataSource.getRepository(flatObjectMetadata.nameSingular); | ||
|
|
||
| const existingRecord = await repository.findOne({ | ||
| where: { id: args.id }, | ||
| }); |
There was a problem hiding this comment.
computeArgs runs before executeInWorkspaceContext (see CommonBaseQueryRunnerService.execute), but this code performs a DB read using a global datasource/repository here. This can execute outside the intended workspace context and also bypasses the rolePermissionConfig that is normally passed to getRepository(...) in common-base-query-runner.service.ts:318-321. Move this fetch into a code path that runs within the workspace context (e.g. inside run where repository is available) or wrap it in executeInWorkspaceContext and ensure rolePermissionConfig is applied.
| fieldMetadataItem.isUIReadOnly || | ||
| fieldReadOnlyByPermissions | ||
| fieldReadOnlyByPermissions || | ||
| !!(fieldMetadataItem.settings as any)?.calculationFormula |
There was a problem hiding this comment.
Avoid as any here; it removes type safety around read-only behavior. Prefer narrowing settings to a typed shape (e.g. as { calculationFormula?: string } | null | undefined) or extending the FieldMetadataItem['settings'] union so calculationFormula can be accessed without any.
| !!(fieldMetadataItem.settings as any)?.calculationFormula | |
| !!(fieldMetadataItem.settings as { calculationFormula?: string } | null | undefined) | |
| ?.calculationFormula |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e171d416d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const fields = Object.values(flatFieldMetadataMaps).flat(); | ||
| const objectFields = fields.filter( |
There was a problem hiding this comment.
In calculateFields, fields is built with Object.values(flatFieldMetadataMaps).flat(), but flatFieldMetadataMaps is a metadata maps object (byUniversalIdentifier, universalIdentifierById, etc.), not an array of FlatFieldMetadata. This means objectFields ends up empty and no calculated fields are ever evaluated, so formulas configured in metadata will not run on create/update. Iterate over flatFieldMetadataMaps.byUniversalIdentifier (or an equivalent helper) to collect actual field entries.
Useful? React with 👍 / 👎.
| } catch (error) { | ||
| // Log error or set to null/error value | ||
| console.error(error.message); |
There was a problem hiding this comment.
When formula evaluation throws, the code only logs the error and continues, which lets the mutation succeed while leaving calculated fields stale or unchanged. In practice, a typo in a formula or a missing dependency value will silently corrupt consistency between source fields and computed fields, with no actionable error returned to the caller. This should raise a validation/query exception (or apply an explicit fallback) instead of swallowing the failure.
Useful? React with 👍 / 👎.
|
This needs deeper product input, if we introduce computed fields I think we would do it for more than just for the numeric fields, it needs to be built in a more abstract way. Thanks for trying this but it's unlikely we accept a feature without prior validation/product work |
Description
This PR implements Field Calculations for numeric fields, allowing users to define Excel-style formulas (e.g.,
Price * (1 - Discount / 100)) in the field settings. Calculated fields are automatically updated on the server-side whenever their dependencies change and are treated as read-only in the UI.Key Changes:
calculationFormulaproperty toFieldMetadataNumberSettings.FieldCalculationServiceusingexpr-evalfor formula parsing and evaluation.DataArgProcessorServiceto trigger calculations during record creation and updates.Testing:
FieldCalculationServicecovering basic arithmetic, complex formulas, and dependency sorting.Closes #19087