Skip to content

[Fix] Bug with one to many update in table #16340#17416

Merged
lucasbordeau merged 5 commits intotwentyhq:mainfrom
carbonFibreCode:fix/oneToManyUpdate
Jan 26, 2026
Merged

[Fix] Bug with one to many update in table #16340#17416
lucasbordeau merged 5 commits intotwentyhq:mainfrom
carbonFibreCode:fix/oneToManyUpdate

Conversation

@carbonFibreCode
Copy link
Copy Markdown
Contributor

@carbonFibreCode carbonFibreCode commented Jan 24, 2026

fixes #16340

we are updating the cache partially to prevent the flow of the corrupted fields into the cache

the fields get corrupted during the mutation process potentially overriding the recoil cache as we are passing the newRecordCache directly in upsertRecordsInStore, the newRecordCache is thin and hence the recoil wipes out the fields that are undefined or empty

we prevent this by extracting the partial data ( only the data which is being updated in the field ) and passing it to upsertRecordsInStore, as it only touched the specific fields and updates the data, leaving the other fields untouched.

Screen.Recording.2026-01-24.at.10.40.29.PM.mov

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Jan 24, 2026

Greptile Overview

Greptile Summary

This PR fixes a cache corruption issue in one-to-many relationship updates. Previously, when attaching or detaching relations, the entire cached record (including all fields and nested relations) was passed to upsertRecordsInStore. This caused problems because getRecordFromCache returns the full record with all relations at depth 1, and some of those fields could be corrupted during the mutation process.

The fix extracts only the essential fields before upserting to the store:

  • id - record identifier
  • __typename - GraphQL type information
  • [fieldNameOnTargetRecord] - only the specific relation field being updated

This ensures that only the field being modified is updated in the store, preventing unintended side effects on other fields and relations. The change is applied consistently in both triggerAttachRelationOptimisticEffect and triggerDetachRelationOptimisticEffect.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The fix is well-targeted and addresses a specific cache corruption issue without changing the overall flow. The implementation is consistent across both attach and detach functions, uses the same pattern (extracting only necessary fields), and prevents the propagation of potentially corrupted data to the record store. The change is minimal and focused on the root cause.
  • No files require special attention

Important Files Changed

Filename Overview
packages/twenty-front/src/modules/apollo/optimistic-effect/utils/triggerAttachRelationOptimisticEffect.ts Fixed cache corruption by passing only the updated field to upsertRecordsInStore instead of the entire cached record with all its relations
packages/twenty-front/src/modules/apollo/optimistic-effect/utils/triggerDetachRelationOptimisticEffect.ts Fixed cache corruption by passing only the updated field to upsertRecordsInStore instead of the entire cached record with all its relations

Sequence Diagram

sequenceDiagram
    participant Client
    participant Cache as Apollo Cache
    participant AttachFn as triggerAttachRelationOptimisticEffect
    participant GetRecord as getRecordFromCache
    participant Upsert as upsertRecordsInStore
    participant Store as Record Store

    Client->>AttachFn: Attach relation (sourceId, targetId, fieldName)
    AttachFn->>Cache: cache.modify() - Update relation field
    Note over Cache: Adds/updates relation reference in cache
    AttachFn->>GetRecord: Fetch updated record from cache
    GetRecord->>Cache: Read fragment with all fields
    Cache-->>GetRecord: Return record with ALL fields & relations
    Note over AttachFn: BEFORE: Passed entire record (corrupted fields)<br/>AFTER: Extract only id, __typename, and updated field
    AttachFn->>AttachFn: Create partialRecordForStore<br/>{id, __typename, [fieldName]}
    AttachFn->>Upsert: upsertRecordsInStore(partialRecordForStore)
    Upsert->>Store: Merge partial record with existing store
    Note over Store: Only specified field updated,<br/>other fields preserved
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.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

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 2 files

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Jan 24, 2026

🚀 Preview Environment Ready!

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

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

@carbonFibreCode carbonFibreCode changed the title Bug with one to many update in table #16340 [Fix] Bug with one to many update in table #16340 Jan 25, 2026
@lucasbordeau lucasbordeau self-requested a review January 26, 2026 09:36
@lucasbordeau lucasbordeau self-assigned this Jan 26, 2026
@lucasbordeau
Copy link
Copy Markdown
Contributor

Hi @carbonFibreCode, I've been testing your solution but it felt like it missed the point because it did not fix the root cause but only avoided the side effect of the root cause, it could be enough for a fix, but in this part of the app, it is better to fix the root cause because the side effects could arise later and elsewhere as it is a central part.

The root cause in this case was related to the special treatment we have for activities, which have a junction activityTarget table, ie noteTargets or taskTargets.

We already fixed a similar issue related to the computation of the gql fields of a note or task for regular find requests, in order to optimize our queries.

Here we have the opposite problem, we miss the label identifier fields of a note or task, so it overwrites the note or task in the record store with the id field only.

So the root cause here is that we had to fix the generation of the record gql fields, inside getRecordFromCache, in order to return the required fields for computing the chip.

Here are both the Apollo cache read fragment before and after this fix, inside getRecordFromCache :

fragment PersonFragment on Person {
  __typename
  id
  noteTargets {
    edges {
      node {
        __typename
        id
      }
    }
  }
  taskTargets {
    edges {
      node {
        __typename
        id
      }
    }
  }
  // Other fields ...
}
fragment PersonFragment on Person {
  __typename
  id
  noteTargets {
    edges {
      node {
        __typename
        id
        note {
          __typename
          id
          title
        }
      }
    }
  }
  taskTargets {
    edges {
      node {
        __typename
        id
        task {
          __typename
          id
          title
        }
      }
    }
  }
  // Other fields ...
}

@lucasbordeau
Copy link
Copy Markdown
Contributor

Also I wanted to keep your fix as it seems like a good idea to only update what's relevant, but it introduced regression while doing a QA on relation modification, notably detaching a relation from a card, so I think it's safer to let what's in the cache overwrite the record store and only add the relevant missing fields to not risk introducing regression.

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 3 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/apollo/optimistic-effect/utils/triggerDetachRelationOptimisticEffect.ts">

<violation number="1">
P2: Passing the full cached record into the store can overwrite unrelated fields with undefined when the cache contains partial data.</violation>
</file>

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

@carbonFibreCode
Copy link
Copy Markdown
Contributor Author

Also I wanted to keep your fix as it seems like a good idea to only update what's relevant, but it introduced regression while doing a QA on relation modification, notably detaching a relation from a card, so I think it's safer to let what's in the cache overwrite the record store and only add the relevant missing fields to not risk introducing regression.

Hey @lucasbordeau, thanks a lot for digging into this and fixing the root cause.
I was initially leaning towards the same approach of fetching the data along with activityTargetGqlFields as mentioned in the issue discussion, but the optimization felt better at that moment and I completely overlooked the potential regressions, especially given this is such a central part of the app.

Your explanation and the updated fragments make a lot of sense . I appreciate you taking the time to walk through the reasoning.

@lucasbordeau lucasbordeau added this pull request to the merge queue Jan 26, 2026
Merged via the queue into twentyhq:main with commit 4a5ffcc Jan 26, 2026
61 checks passed
@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!

camilo-agudelo-uma pushed a commit to innovation-grupo-uma/twenty-uma that referenced this pull request Feb 2, 2026
…7416)

fixes twentyhq#16340 

we are updating the cache partially to prevent the flow of the corrupted
fields into the cache

the fields get corrupted during the mutation process potentially
overriding the recoil cache as we are passing the `newRecordCache`
directly in `upsertRecordsInStore`, the newRecordCache is thin and hence
the recoil wipes out the fields that are undefined or empty



we prevent this by extracting the partial data ( only the data which is
being updated in the field ) and passing it to `upsertRecordsInStore`,
as it only touched the specific fields and updates the data, leaving the
other fields untouched.


https://github.com/user-attachments/assets/5256bef7-70c3-47b3-b2ce-dd02ee1a2de8

---------

Co-authored-by: Arun kumar <arunkumar@Aruns-MacBook-Air.local>
Co-authored-by: Lucas Bordeau <bordeau.lucas@gmail.com>
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.

Bug with one to many update in table

2 participants