-
Notifications
You must be signed in to change notification settings - Fork 5k
fix: refresh virtualized table when field metadata is updated ( #16388 ) #17214
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
base: main
Are you sure you want to change the base?
fix: refresh virtualized table when field metadata is updated ( #16388 ) #17214
Conversation
Greptile Summary
Important Files Changed
Confidence score: 4/5
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant UpdateHook as "useUpdateOneFieldMetadataItem"
participant GraphQL as "GraphQL API"
participant MetadataRefresh as "refreshObjectMetadataItems"
participant ViewRefresh as "refreshCoreViewsByObjectMetadataId"
participant StateManager as "Recoil State"
participant VirtualizedTable as "RecordTableVirtualizedInitialDataLoadEffect"
participant ResetVirtualization as "resetVirtualizationBecauseDataChanged"
User->>Frontend: "Update field metadata"
Frontend->>UpdateHook: "updateOneFieldMetadataItem()"
UpdateHook->>GraphQL: "updateOneFieldMetadataItemMutation()"
GraphQL-->>UpdateHook: "Response"
UpdateHook->>MetadataRefresh: "refreshObjectMetadataItems()"
UpdateHook->>ViewRefresh: "refreshCoreViewsByObjectMetadataId()"
UpdateHook->>StateManager: "setLastFieldMetadataItemUpdate()"
VirtualizedTable->>StateManager: "Monitor lastFieldMetadataItemUpdate"
StateManager-->>VirtualizedTable: "Field update detected"
VirtualizedTable->>VirtualizedTable: "Check if field is in current view"
VirtualizedTable->>ResetVirtualization: "resetVirtualizationBecauseDataChanged()"
ResetVirtualization-->>VirtualizedTable: "Table refreshed"
VirtualizedTable-->>Frontend: "Table displays updated field metadata"
|
Greptile found no issues!From now on, if a review finishes and we haven't found any issues, we will not post anything, but you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:57146 This environment will automatically shut down when the PR is closed or after 5 hours. |
There was a problem hiding this 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 4 files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @carbonFibreCode, thank you so much for your contribution. I've tested it and it works well.
Concerning the implementation strategy, we plan to have an app-scoped state holding all sorts of metadata updates—a higher level one than the one you built.
Another issue I see: when first loading another table view based on another view showing the field, it would go through the useEffect (RecordTableVirtualizedInitialDataLoadEffect.tsx file), resetting virtualization when we don't need it to.
@lucasbordeau, what do you think? Would you have more precise guidance to help fix this? Or too complicated to fix without context/experience on twenty codebase ?
| } | ||
|
|
||
| if ( | ||
| lastFieldMetadataItemUpdate.timestamp === lastProcessedFieldMetadataUpdate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lastFieldMetadataItemUpdate.timestamp < lastProcessedFieldMetadataUpdate Shouldn't be more logic like that ?
hey @etiennejouan thanks for review, i am working on it right now to follow the implementation strategy for using appScoped state, open to @lucasbordeau 's comment |
@carbonFibreCode |
waiting for @lucasbordeau 's review, In the mean time i tried switching to using a generic lastAppOperationState as requested, for the table refresh logic, I opted for a pure event driven approach, if a relevant event hasn't been processed by the component, we trigger a refresh. |
| visibleRecordFields, | ||
| ]); | ||
|
|
||
| const lastFieldMetadataItemUpdate = useRecoilValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic works for me, could you just extract this into a separate effect component ? We try to keep one useEffect per component to avoid side effects that can happen when multiple useEffects share the same reactive dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah sure, i'll do it and push the changes by 9:30 PM IST (4:30 PM CET) -> i hope correct about the time zone.. haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @lucasbordeau, I’ve updated the PR with the separate effect component as you suggested and all checks are passing. Would you mind having another look and sharing any further guidance, given the current blocked: tech spec needed label?
...-server/src/engine/workspace-manager/workspace-cleaner/services/cleaner.workspace-service.ts
Show resolved
Hide resolved
...-server/src/engine/workspace-manager/workspace-cleaner/services/cleaner.workspace-service.ts
Show resolved
Hide resolved
dec4d06 to
45a3502
Compare
| @@ -0,0 +1,13 @@ | |||
| import { createState } from 'twenty-ui/utilities'; | |||
|
|
|||
| export type LastFieldMetadataItemUpdate = { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I think it's a good pattern :
- The table updates only when it mounts back after settings change, so that's asynchronous as it should be
- We reset virtualization as we already do for other update events, and it is designed to handle this frequently right now. (We would like to optimize those small state effects but that requires unlocking other improvements before.)
- We already use this pattern elsewhere
Two changes though :
- Could you use an uuid v4 instead of a timestamp for the referential integrity of this object ?
- Extract this type in a standalone file
| import { ContextStoreComponentInstanceContext } from '@/context-store/states/contexts/ContextStoreComponentInstanceContext'; | ||
| import { createComponentState } from '@/ui/utilities/state/component-state/utils/createComponentState'; | ||
|
|
||
| export const lastProcessedFieldMetadataUpdateComponentState = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lastProcessedFieldMetadataUpdateIdComponentState => use an id property instead
| } | ||
|
|
||
| if ( | ||
| lastFieldMetadataItemUpdate.timestamp === lastProcessedFieldMetadataUpdate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use id here for comparison
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmmmm.... ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lucasbordeau pushed the changes
- now eventId is compared as per the request
- pulled out the types in separate file as per existing design pattern
45a3502 to
9d6c673
Compare
lucasbordeau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice contribution.
I've just added some minor renaming to make it more aligned with our conventions.
Thanks for the feedback, Lucas, i've noted those naming conventions and will make sure to apply them to my future contributions. Appreciate the guidance on this one. |
Fixes #16388
Now we have used the metadata fetching from network using resetVirtualizationBecauseDataChanged(), as we navigate back to the table after updating the field content. As the virtualised table uses the "cache-first" strategy, it fails to give fresh data from the data base
Please let me know i we need to add the loading state (and it's UI requirements) while it fetches the cell content, as currently the cell is empty until it's populated by fresh data
Screen.Recording.2026-01-17.at.5.31.26.PM.mov