Skip to content

Fixed plain object in field value for workflow#17470

Merged
lucasbordeau merged 3 commits intomainfrom
fix/plain-object-in-format-result-field-value
Jan 27, 2026
Merged

Fixed plain object in field value for workflow#17470
lucasbordeau merged 3 commits intomainfrom
fix/plain-object-in-format-result-field-value

Conversation

@lucasbordeau
Copy link
Copy Markdown
Contributor

@lucasbordeau lucasbordeau commented Jan 27, 2026

This PR fixes a bug that arises in formatResult following up the recent refactor for DATE_TIME : #17407

In the case of workflows, we pass a plain object to formatResult :

{
  before: null, 
  after: '2026-01-27T10:59:15.525Z'
}

So a case has been added to handle this.

@Weiko are we ok with this more restrictive else-if part in this util ? We could also just put a continue for unknown shapes.

Copilot AI review requested due to automatic review settings January 27, 2026 11:00
@lucasbordeau lucasbordeau requested a review from Weiko January 27, 2026 11:00
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 1 file

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Jan 27, 2026

Greptile Overview

Greptile Summary

This PR fixes a bug in formatResult that was introduced in PR #17407 when strict DATE_TIME validation was added. The issue occurs when workflow event diffs pass plain objects with {before: ..., after: ...} structure through formatResult.

Changes:

  • Added Record<string, unknown> to the type union for DATE_TIME fields
  • Added else if (isPlainObject(rawUpdatedDateTime)) branch to handle plain object values
  • Improved error message to use JSON.stringify() for better debugging

Context:
The objectRecordChangedValues utility creates diff objects with {before, after} structure for changed fields. When these diffs flow through the event emitter system for workflow events, they eventually reach formatResult which now needs to handle this plain object case for DATE_TIME fields.

Minor issue:

  • One redundant variable assignment that could be simplified (style comment added)

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk
  • The fix correctly addresses the bug by adding support for plain object DATE_TIME values from workflow event diffs. The implementation is straightforward and follows the existing pattern. Score of 4 (not 5) due to: (1) minor style issue with redundant variable assignment, and (2) lack of test coverage for this new edge case. The fix itself is sound and solves the immediate problem.
  • No files require special attention - the change is localized and addresses a specific bug

Important Files Changed

Filename Overview
packages/twenty-server/src/engine/twenty-orm/utils/format-result.util.ts Adds handling for plain object DATE_TIME values (e.g. {before: null, after: '...'}) from workflow event diffs. Minor style issue with redundant variable assignment.

Sequence Diagram

sequenceDiagram
    participant WEM as WorkspaceEntityManager
    participant ORC as objectRecordChangedValues
    participant FMT as formatResult
    participant WF as Workflow Event

    WEM->>ORC: compare before/after records
    ORC->>ORC: detect changed DATE_TIME field
    ORC-->>WEM: return {fieldName: {before: null, after: '2026-01-27T10:59:15.525Z'}}
    WEM->>FMT: format event diff data
    Note over FMT: PR #17407 added strict<br/>DATE_TIME validation
    FMT->>FMT: check if DATE_TIME field
    FMT->>FMT: isPlainObject? (NEW)
    Note over FMT: This PR adds support<br/>for plain objects
    FMT-->>WEM: pass through plain object
    WEM->>WF: emit workflow event with diff
Loading

Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

// @ts-expect-error legacy noImplicitAny
newData[dateTimeField.name] = dateIsoString;
} else if (isPlainObject(rawUpdatedDateTime)) {
const plainObjectValue = rawUpdatedDateTime;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary intermediate variable, can assign rawUpdatedDateTime directly

Suggested change
const plainObjectValue = rawUpdatedDateTime;
newData[dateTimeField.name] = rawUpdatedDateTime;

Context Used: Context from dashboard - Avoid using inline returns in the codebase for better readability and maintainability. (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/twenty-server/src/engine/twenty-orm/utils/format-result.util.ts
Line: 213:213

Comment:
unnecessary intermediate variable, can assign `rawUpdatedDateTime` directly

```suggestion
      newData[dateTimeField.name] = rawUpdatedDateTime;
```

**Context Used:** Context from `dashboard` - Avoid using inline returns in the codebase for better readability and maintainability. ([source](https://app.greptile.com/review/custom-context?memory=ecab015d-82e3-4483-9a36-3ed0a14495bf))

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adjusts the formatResult DATE_TIME handling to support workflow use cases where DATE_TIME fields are represented as plain objects (e.g. { before, after }), avoiding spurious errors introduced by the previous refactor.

Changes:

  • Extends the inferred type of rawUpdatedDateTime within formatResult to include Record<string, unknown>.
  • Adds an isPlainObject branch so plain-object DATE_TIME values are passed through without conversion instead of triggering an error.
  • Updates the DATE_TIME validation error message to include JSON.stringify output and mention plain-object support.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 217 to 220
} else {
throw new Error(
`Invalid DATE_TIME field "${dateTimeField.name}", value: "${rawUpdatedDateTime}", it should be a string or Date instance, (current type : ${typeof rawUpdatedDateTime}).`,
`Invalid DATE_TIME field "${dateTimeField.name}", value: "${JSON.stringify(rawUpdatedDateTime)}", it should be a string, Date instance or plain object, (current type : ${typeof rawUpdatedDateTime}).`,
);
Copy link

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using JSON.stringify(rawUpdatedDateTime) inside the error message can itself throw (e.g., for values containing BigInt or circular references), which would mask the intended, more descriptive Error and surface a generic TypeError instead. It would be safer to stringify defensively (e.g., via a helper that catches JSON.stringify failures and falls back to String(value)), so that the error path is robust even when the value is of an unexpected type.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@Weiko Weiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 27, 2026

🚀 Preview Environment Ready!

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

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

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 5 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-shared/src/utils/strings/stringifySafely.ts">

<violation number="1" location="packages/twenty-shared/src/utils/strings/stringifySafely.ts:15">
P2: `JSON.stringify` returns `undefined` for unsupported top-level values (e.g., functions or symbols), so this line can return `undefined` despite the function promising a string. Capture the result and fall back to `String(value)` when JSON serialization yields `undefined`.</violation>
</file>

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

@lucasbordeau lucasbordeau disabled auto-merge January 27, 2026 13:04
@lucasbordeau lucasbordeau added this pull request to the merge queue Jan 27, 2026
Merged via the queue into main with commit 3265851 Jan 27, 2026
76 checks passed
@lucasbordeau lucasbordeau deleted the fix/plain-object-in-format-result-field-value branch January 27, 2026 13:33
@twenty-eng-sync
Copy link
Copy Markdown

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

1 similar comment
@twenty-eng-sync
Copy link
Copy Markdown

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants