feat: resolve preview merge record uses a fake one #15834

harshit078 · 2025-11-15T13:23:11Z

Description

This PR fixes the first issue out of fix: removeuseMergeRecordRelatationship and simplify dry run response #15486
created a Recoil atom mergePreviewRecordFamilyState that is used to store the preview record.
Preview items are now stored in mergePreviewRecordFamilyState which avoiding pollution of the recordStoreFamilyState

github-actions · 2025-11-15T13:34:57Z

🚀 Preview Environment Ready!

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

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

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts

greptile-apps · 2025-11-17T07:35:09Z

Greptile Summary

Introduces mergePreviewRecordFamilyState to store merge preview records separately from main record store, preventing pollution of recordStoreFamilyState
Updates record store selectors to check preview state first before falling back to main store for seamless preview display
Implements race condition handling with runId tracking, though the implementation has a critical stale closure bug

Confidence Score: 2/5

This PR has a critical race condition bug that will cause the protection logic to fail
The race condition handling logic in useMergePreview.ts has a stale closure bug where currentRunId references the old value from the effect closure, making all race condition checks ineffective. This defeats the purpose of the race condition protection.
Pay close attention to packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts - the race condition logic needs to be fixed using useRef

Important Files Changed

Filename	Overview
packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts	Added race condition handling with runId tracking, but implementation has stale closure bug that breaks the race condition checks

Sequence Diagram

sequenceDiagram
    participant User
    participant useMergePreview
    participant mergePreviewRecordFamilyState
    participant useMergeManyRecords
    participant Backend
    participant recordStoreSelectors

    User->>useMergePreview: "selects records to merge"
    useMergePreview->>useMergePreview: "computes previewSignature"
    useMergePreview->>useMergeManyRecords: "mergeManyRecords(preview: true)"
    useMergeManyRecords->>Backend: "mutation with dryRun: true, no-cache"
    Backend-->>useMergeManyRecords: "returns preview record"
    useMergeManyRecords-->>useMergePreview: "returns preview record"
    useMergePreview->>mergePreviewRecordFamilyState: "stores preview record"
    User->>recordStoreSelectors: "views merged preview"
    recordStoreSelectors->>mergePreviewRecordFamilyState: "checks for preview record first"
    mergePreviewRecordFamilyState-->>recordStoreSelectors: "returns preview data"
    recordStoreSelectors-->>User: "displays preview"
    User->>useMergeManyRecords: "confirms merge"
    useMergeManyRecords->>Backend: "mutation with dryRun: false"
    Backend-->>useMergeManyRecords: "returns merged record"
    useMergeManyRecords->>useMergeManyRecords: "triggers refetchQueries"

greptile-apps

_{7 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}
_{React with 👍 or 👎 to share your feedback on this new summary format}

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts

charlesBochet · 2025-11-20T10:23:49Z

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts

+  const [lastPreviewId, setLastPreviewId] = useState<string>('');
+  const [lastPreviewSignature, setLastPreviewSignature] = useState<string>('');
+  // eslint-disable-next-line @nx/workspace-no-state-useref
+  const currentRunIdRef = useRef(0);


@harshit078 we don't use useRef in the project

charlesBochet · 2025-11-20T10:24:00Z

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts

-    commandMenuNavigationMorphItemsByPage
-      .get(CommandMenuPages.MergeRecords)
-      ?.map((morphItem) => morphItem.recordId) ?? [];
+  const selectedRecordIds = useMemo(


useMemo does not seems necessary here?

charlesBochet · 2025-11-20T10:25:41Z

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts

+    ({ set }) =>
+      (record: ObjectRecord | null, previousId: string) => {
+        if (previousId !== '') {
+          set(mergePreviewRecordFamilyState(previousId), null);


I'm not sure about this, this does not seem robust, why do we need to clean the state?

charlesBochet · 2025-11-20T10:25:55Z

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts

+        if (previousId !== '') {
+          set(mergePreviewRecordFamilyState(previousId), null);
+        }
+        if (record !== null) {


use isDefined, isNonEmptyString helpers

charlesBochet · 2025-11-20T10:26:55Z

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts

  useEffect(() => {
    const fetchPreview = async () => {
-      if (selectedRecords.length < 2 || isMergeInProgress || isInitialized)
+      if (selectedRecordIds.length < 2 || isMergeInProgress) {


this code is not robust IMO, isInitialized seemed right. What issue are we trying to solve?

charlesBochet

@harshit078 ty

Feedbacks:

let's not use useRef. What we should aim for: hooks that are called synchronously. Here, I think we should add records to the mergePreviewFamilyState in useOpenMergeRecordsPageInCommandMenu for instance. this would remove the need for useRef, useEffect, useMemo and all other hacks.

I think we should not try to clean the mergePreviewFamilyState, it does not worth it.

Also, I'm not convinced by the fact that recordStore is now loading data from two different sources but why not, it does not harm too much

harshit078 · 2025-11-20T12:19:22Z

@charlesBochet Hi charles, thanks for the comments. I am super aware that Twenty ooesnt use useRef but I couldnt find another approach round it which would pass lint test and was ideal. I'll address the comments and refactor the approach thanks !

Copilot

Pull Request Overview

This PR introduces a dedicated Recoil atom mergePreviewRecordFamilyState to store merge preview records separately from the main record store, preventing pollution of recordStoreFamilyState during merge operations.

Key Changes:

Created mergePreviewRecordFamilyState to store preview records independently
Updated record store selectors to check preview state before falling back to main store
Refactored useMergePreview hook to manage preview records in the new state and handle race conditions with run IDs

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
`mergePreviewRecordFamilyState.ts`	New Recoil family state atom for storing merge preview records
`recordStoreFamilySelector.ts`	Updated to read from preview state first, then fall back to main store
`recordStoreIdentifierSelector.ts`	Added preview record check before reading from main store
`recordStoreIdentifiersSelector.ts`	Added preview record check in bulk identifier retrieval
`recordStoreFieldValueSelector.ts`	Enhanced to prioritize preview records for both regular and morph relation fields
`useMergePreview.ts`	Refactored to use new preview state with race condition handling via run IDs
`useMergeManyRecords.ts`	Improved conditional logic for preview vs. non-preview query options
`RecordTitleCellTextFieldDisplay.tsx`	Changed to use selector-based field value retrieval instead of direct record access

Comments suppressed due to low confidence (1)

packages/twenty-front/src/modules/object-record/record-store/states/selectors/recordStoreFamilySelector.ts:23

The set method of this selector doesn't handle preview records. When a field value is set, it always writes to recordStoreFamilyState even if the current value is being read from a preview record in mergePreviewRecordFamilyState. This could cause inconsistent behavior where reads come from the preview but writes go to the main store.

Consider adding logic to the set method to handle preview records appropriately, or document that this selector is read-only for preview records.

  set:
    <T>({ fieldName, recordId }: { fieldName: string; recordId: string }) =>
    ({ set }, newValue: T) =>
      set(recordStoreFamilyState(recordId), (prevState) =>

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

Copilot · 2025-11-20T12:24:44Z

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts

+    mergeSettings,
+    setPreviewRecordById,
+    lastPreviewId,
+    lastPreviewSignature,


[nitpick] The dependency array includes lastPreviewSignature which is compared inside the effect on line 80. This creates a circular dependency: the effect updates lastPreviewSignature, which triggers the effect again. However, the early return on line 80-82 prevents infinite loops. While this works, it's an unconventional pattern. Consider restructuring to avoid having state that's both read and written in the same effect appear in the dependency array.

Copilot · 2025-11-20T12:24:44Z

...t/src/modules/object-record/record-title-cell/components/RecordTitleCellTextFieldDisplay.tsx


-  const isEmpty =
-    recordValue?.[fieldDefinition.metadata.fieldName]?.trim() === '';
+  const isEmpty = fieldValue?.toString().trim() === '';


Using .toString() on fieldValue could produce unexpected results for complex field types (objects, arrays, null). For example, objects would display as [object Object]. Consider type checking and handling different field types appropriately, or use the original field value access pattern with proper typing.

Copilot · 2025-11-20T12:24:44Z

...t/src/modules/object-record/record-title-cell/components/RecordTitleCellTextFieldDisplay.tsx

-            recordValue?.[fieldDefinition.metadata.fieldName] ||
-            fieldDefinition.label
-          }
+          text={fieldValue?.toString() || fieldDefinition.label}


Similar to line 48, using .toString() on fieldValue can produce unexpected results for non-string field types. Objects would display as [object Object]. Consider ensuring the field value is properly typed as a string or handle different types appropriately.

Copilot · 2025-11-20T12:24:45Z

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts

  }, [
-    selectedRecords,
-    mergeSettings,
+    previewSignature,


The previewSignature is included in the dependency array but uses values that are already in the array (selectedRecordIds and mergeSettings.conflictPriorityIndex). Since previewSignature is derived from these values using useMemo, including it creates redundant dependencies. Remove previewSignature from the dependency array and keep only the primitive dependencies.

Suggested change

previewSignature,

Copilot · 2025-11-20T12:24:45Z

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts

+  // eslint-disable-next-line @nx/workspace-no-state-useref
+  const currentRunIdRef = useRef(0);


[nitpick] The comment eslint-disable-next-line @nx/workspace-no-state-useref suggests that using useRef here might violate a workspace rule. While the use of useRef for tracking run IDs is appropriate to prevent race conditions, consider if there's an alternative pattern that complies with your workspace rules or document why this exception is necessary.

charlesBochet · 2025-11-21T09:03:59Z

@harshit078 I'm going to close it for now, let's re-open when ready

feat: introduced mergePreview state and updated

f5d85c3

harshit078 changed the title ~~feat: introduced mergePreview state and updated~~ feat: resolve preview merge record uses a fake one Nov 15, 2025

Merge branch 'main' into fix-fake-record

9c2191a

FelixMalfait requested a review from neo773 November 15, 2025 19:27

harshit078 and others added 4 commits November 17, 2025 12:09

updated with mergePreview record state

c38de10

Merge branch 'main' into fix-fake-record

a58864a

fix failing tests

2840f34

Merge branch 'main' into fix-fake-record

69f2178

harshit078 marked this pull request as ready for review November 17, 2025 07:31

sentry bot reviewed Nov 17, 2025

View reviewed changes

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts Show resolved Hide resolved

greptile-apps bot reviewed Nov 17, 2025

View reviewed changes

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts Outdated Show resolved Hide resolved

packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts Show resolved Hide resolved

Merge branch 'main' into fix-fake-record

e194478

lucasbordeau self-requested a review November 17, 2025 09:18

fix comments

8615456

charlesBochet self-assigned this Nov 18, 2025

charlesBochet added the -PR: awaiting review label Nov 18, 2025

lucasbordeau removed their request for review November 19, 2025 08:50

charlesBochet reviewed Nov 20, 2025

View reviewed changes

charlesBochet requested changes Nov 20, 2025

View reviewed changes

charlesBochet added -PR: awaiting author and removed -PR: awaiting review labels Nov 20, 2025

Merge branch 'main' into fix-fake-record

01b0416

Copilot AI review requested due to automatic review settings November 20, 2025 12:19

Copilot started reviewing on behalf of harshit078 November 20, 2025 12:20 View session

Copilot finished reviewing on behalf of harshit078 November 20, 2025 12:21

Copilot AI reviewed Nov 20, 2025

View reviewed changes

charlesBochet closed this Nov 21, 2025

		// eslint-disable-next-line @nx/workspace-no-state-useref
		const currentRunIdRef = useRef(0);

feat: resolve preview merge record uses a fake one #15834

feat: resolve preview merge record uses a fake one #15834

Uh oh!

Conversation

harshit078 commented Nov 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

greptile-apps bot commented Nov 17, 2025

Greptile Summary

Confidence Score: 2/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

charlesBochet Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

charlesBochet Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

charlesBochet Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

charlesBochet Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

charlesBochet Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

charlesBochet left a comment

Choose a reason for hiding this comment

Uh oh!

harshit078 commented Nov 20, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

charlesBochet commented Nov 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

harshit078 commented Nov 15, 2025 •

edited

Loading

github-actions bot commented Nov 15, 2025 •

edited

Loading