Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix view groups update after field metadata update #10995

Merged
merged 2 commits into from
Mar 19, 2025

Conversation

lucasbordeau
Copy link
Contributor

@lucasbordeau lucasbordeau commented Mar 18, 2025

This PR fixes a difficult to reproduce bug, where a race condition appears if we go back to a table with view groups before the update field metadata logic finishes its work.

In order to reproduce this bug on localhost, you'll have to simulate a slow network in your browser.

The problem was that the view groups are initialized only once by useLoadRecordIndexStates, in an effect component : RecordIndexLoadBaseOnContextStoreEffect. And that this component as an internal state loadedViewId, which prevents subsequent updates of view groups by useLoadRecordIndexStates, because it considers that loading already happened, even if it's actually stale data.

So instead of creating other states to burden the effect component with complex state management, the solution was to add the only needed update in a synchronous way directly in updateOneFieldMetadataItem logic. This way we are sure that our boards and tables with view groups get updated eventually, for each field metadata update, even if the requests take time.

Fixes #10947
Fixes #10944

…etadataItem

- Modified useSetRecordGroups to enforce objectMetadataItem as args instead of context
Copy link
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.

PR Summary

This PR fixes a race condition where view groups could become stale after field metadata updates by implementing synchronous updates in the updateOneFieldMetadataItem function.

  • Added direct view group updates in useUpdateOneFieldMetadataItem.ts to ensure synchronous updates after field metadata changes
  • Modified useSetRecordGroups to accept objectMetadataItemId directly instead of relying on context store
  • Added objectMetadataItem parameter to setRecordGroupsFromViewGroups calls to maintain metadata context during updates
  • Removed dependency on effect component's internal state management to prevent stale data issues
  • Added proper view record fetching after metadata updates to ensure groups stay in sync

5 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile

@ijreilly
Copy link
Collaborator

this fixes #10947 right ? I just tested and it seems to !

@ijreilly
Copy link
Collaborator

This PR fixes a difficult to reproduce bug, where a race condition appears if we go back to a table with view groups before the update field metadata logic finishes its work.

In order to reproduce this bug on localhost, you'll have to simulate a slow network in your browser.

The problem was that the view groups are initialized only once by useLoadRecordIndexStates, in an effect component : RecordIndexLoadBaseOnContextStoreEffect. And that this component as an internal state loadedViewId, which prevents subsequent updates of view groups by useLoadRecordIndexStates, because it considers that loading already happened, even if it's actually stale data.

So instead of creating other states to burden the effect component with complex state management, the solution was to add the only needed update in a synchronous way directly in updateOneFieldMetadataItem logic. This way we are sure that our boards and tables with view groups get updated eventually, for each field metadata update, even if the requests take time.

@charlesBochet did we not already witnessed issues with the slow update of fieldMetadata ? it rings a bell !

@lucasbordeau
Copy link
Contributor Author

Indeed it's the same problem, I updated the description.

Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Agree with all of it. On the back end we call that "related records" updates when we update the object metadata

@charlesBochet charlesBochet merged commit 62a5881 into main Mar 19, 2025
47 checks passed
@charlesBochet charlesBochet deleted the fix/view-groups-stale-after-field-metadata-update branch March 19, 2025 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants