Skip to content

fix: removeuseMergeRecordRelatationship and simplify dry run response#15486

Merged
etiennejouan merged 6 commits intotwentyhq:mainfrom
harshit078:fix-merge-frontend
Nov 5, 2025
Merged

fix: removeuseMergeRecordRelatationship and simplify dry run response#15486
etiennejouan merged 6 commits intotwentyhq:mainfrom
harshit078:fix-merge-frontend

Conversation

@harshit078
Copy link
Copy Markdown
Contributor

Description

Visual Appearance

Screen.Recording.2025-10-31.at.1.15.29.PM.mov

@harshit078 harshit078 marked this pull request as ready for review November 1, 2025 10:34
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.

Greptile Overview

Greptile Summary

This PR attempts to simplify the merge preview logic by removing the useMergeRecordRelationships hook, with the assumption that the backend now returns fully populated relation data in the dry run response.

Critical Issue Identified:

  • The backend's fetchRecordsToMerge method (in common-merge-many-query-runner.service.ts) uses TypeORM's repository.find() with only a select parameter, which does not load relation objects
  • It only fetches scalar fields and foreign key IDs (e.g., companyId) but not the actual related records (e.g., company details)
  • The processNestedRelations call that populates relations is only executed for actual merges (line 119-123), not for dry runs which return early (line 78-85)
  • The deleted useMergeRecordRelationships hook was the only code fetching complete relation data for the preview

Impact:

  • Merge preview will display incomplete relation information (only IDs, not the actual related record data like company name)
  • This will cause the bug described in issue #15201 to persist or worsen
  • Users won't see accurate preview data before confirming a merge

Required Fix:
Either the backend needs to be updated to populate relations in dry run responses (PR #15484 mentioned but not found/merged), or this frontend change should not be merged until the backend is ready.

Confidence Score: 0/5

  • This PR will break the merge preview feature by removing critical relation data fetching logic without a proper backend replacement
  • The removal of useMergeRecordRelationships eliminates the only code path that fetches complete relation objects for the merge preview. The backend's dry run response does not include populated relations - it only returns foreign key IDs. This is a critical regression that will cause merge previews to show incomplete data, directly impacting the user experience and potentially causing incorrect merge decisions
  • Both files require attention: useMergePreview.ts needs the relation fetching logic restored, and useMergeRecordRelationships.ts should not be deleted until backend properly handles relations in dry run

Important Files Changed

File Analysis

Filename Score Overview
packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergePreview.ts 1/5 Removed critical hook for fetching relation data, breaking merge preview with incomplete relation information
packages/twenty-front/src/modules/object-record/record-merge/hooks/useMergeRecordRelationships.ts 0/5 Deleted hook that was responsible for fetching and merging relation data into preview records

Sequence Diagram

sequenceDiagram
    participant UI as Merge Preview UI
    participant Hook as useMergePreview
    participant API as mergeManyRecords
    participant Backend as CommonMergeManyQueryRunner
    participant DB as Database

    UI->>Hook: Select records to merge
    Hook->>API: mergeManyRecords(preview: true)
    API->>Backend: Mutation with dryRun: true
    Backend->>DB: fetchRecordsToMerge (select only)
    Note over Backend,DB: Only fetches scalar fields + foreign key IDs<br/>(e.g., companyId, NOT company object)
    DB-->>Backend: Records with IDs only
    Backend->>Backend: performDeepMerge
    Backend->>Backend: createDryRunResponse
    Note over Backend: processRecord transforms data<br/>but relations are NOT populated
    Backend-->>API: Preview record (missing relation objects)
    API->>Hook: upsertRecordsInStore
    Note over Hook: REMOVED: useMergeRecordRelationships<br/>Previously fetched full relation data
    Hook-->>UI: Display preview with incomplete relations
Loading

2 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 3, 2025

🚀 Preview Environment Ready!

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

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

Copy link
Copy Markdown
Contributor

@etiennejouan etiennejouan left a comment

Choose a reason for hiding this comment

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

Preview on oneToMany relations does not work

here opportunities are not displayed but available in mergeMany payload response
Image

@harshit078
Copy link
Copy Markdown
Contributor Author

I realized the issue was in useMergePreview.ts and in common-merge-many-query-runner.service.ts

  • Now relations are populated on recordsToMerge before performDeepMerge runs as a result so performDeepMerge can merge them.
  • Frontend shows complete record coming from backend and makes no extra redudant call
Screen.Recording.2025-11-05.at.1.14.01.PM.mov

Copy link
Copy Markdown
Contributor

@etiennejouan etiennejouan left a comment

Choose a reason for hiding this comment

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

Hi @harshit078 its works well. Well done and thank you for your contribution !

@etiennejouan etiennejouan merged commit 003b04e into twentyhq:main Nov 5, 2025
61 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 5, 2025

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

Contributions

@etiennejouan
Copy link
Copy Markdown
Contributor

Even though this PR fixes the initial issue Stephanie reported, the merge feature could benefit from some other improvements (not a priority):

  • The preview merge record is a fake one—we should not insert it in the record store.
    My approach would be to use a specific preview record state, fetched by the preview record component (this would require modifications to how the FieldCard component fetches records—maybe too costly?).
    @harshit078 has a different and interesting approach:

We create a context that provides the preview record so that fake records never reach the store—they stay in the context. As a result, recordStoreFamilySelector will check the context first, and if a preview record exists in context with the requested ID, we return data from the context we created. I'm not sure if this would be best, but I think this could keep the fake records separate and prevent them from being inserted in the record store.

  • On the preview tab, you can edit relations—you should not be able to.
  • On the preview tab, the relation total is not displayed.

@charlesBochet have you any hint to share ?

@charlesBochet
Copy link
Copy Markdown
Member

  1. I think that most components / hooks will expect the recordStore to contain all related records to the merge preview record and it looks a bit too heavy to change this logic to make them cope with both recordStoreState and mergedRecordState
  2. It leaves us with two alternatives: a) modify recordStoreFamilySelector as suggested by @harshit078 but I'm not 100% sure that you can read a context from an atom. We could also introduce another state: virtualRecordStoreState and make the selector read from both. I still think it's a bit overkill. b) accepts that we can have fake records in the recordStore (we could even attach a type to each record in the record store but I'm not sure it's worth here). Why is it an issue?

@etiennejouan
Copy link
Copy Markdown
Contributor

I was worried that the fake record might interfere with other features.

This fake record is related (relation) to other real records. If there's ever a fetch performed in the store based on these relationships, it would come up. Definitely unlikely?

We could also say that we clean up this record when exiting the preview.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants