Handle ObjectMetadataItem, FieldMetadataItem and NavigationMenuItem with SSE events#18235
Conversation
| if (!isDefined(objectMetadataItem)) { | ||
| return null; | ||
| } | ||
|
|
||
| const namePlural = objectMetadataItem?.namePlural; |
There was a problem hiding this comment.
Bug: The NavigationMenuItemSSEEffect component imports refreshObjectMetadataItems but never calls it, leading to potential state inconsistencies when handling SSE updates for navigation items.
Severity: MEDIUM
Suggested Fix
In NavigationMenuItemSSEEffect.tsx, call await refreshObjectMetadataItems() before refetching the navigation menu items. This will ensure that the object metadata is up-to-date when processing navigation item SSE events, preventing items from being silently filtered due to stale state.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-front/src/modules/navigation-menu-item/utils/sortNavigationMenuItems.ts#L39-L43
Potential issue: When a `navigationMenuItem` Server-Sent Event (SSE) is processed by
`NavigationMenuItemSSEEffect`, it updates the navigation menu items but fails to refresh
the associated object metadata. This is because the `refreshObjectMetadataItems`
function is imported but never called. Consequently, if a new navigation item references
object metadata that is not yet present in the client's state, the
`sortNavigationMenuItems` function will silently filter it out. This can cause
navigation items to temporarily disappear from the UI until the object metadata is
updated through other means.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Pull request overview
This pull request enhances the SSE (Server-Sent Events) handling infrastructure by adding support for ObjectMetadataItem, FieldMetadataItem, and NavigationMenuItem entities. The changes enable real-time synchronization of metadata changes across the application by listening to SSE events and updating the application state accordingly.
Changes:
- Renamed existing SSE effect components (
ViewMetadataSSEEffect→ViewSSEEffect,ViewFieldMetadataSSEEffect→ViewFieldSSEEffect) for naming consistency - Added three new SSE effect components:
ObjectMetadataItemSSEEffect,FieldMetadataSSEEffect, andNavigationMenuItemSSEEffectto handle real-time updates - Extended the metadata store state to include
navigationMenuItemsas a tracked entity - Refactored
sortNavigationMenuItemsto handle missing object metadata items gracefully by returning null instead of throwing errors
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/twenty-front/src/modules/sse-db-event/components/SSEProvider.tsx | Updated imports and component references to use renamed components and added new SSE effect components |
| packages/twenty-front/src/modules/navigation-menu-item/utils/sortNavigationMenuItems.ts | Refactored to inline object metadata lookup logic and handle missing items gracefully |
| packages/twenty-front/src/modules/metadata-store/states/metadataStoreState.ts | Added 'navigationMenuItems' to the list of tracked metadata entities |
| packages/twenty-front/src/modules/metadata-store/effect-components/ViewSSEEffect.tsx | Renamed from ViewMetadataSSEEffect for consistency |
| packages/twenty-front/src/modules/metadata-store/effect-components/ViewFieldSSEEffect.tsx | Renamed from ViewFieldMetadataSSEEffect for consistency |
| packages/twenty-front/src/modules/metadata-store/effect-components/ObjectMetadataItemSSEEffect.tsx | New component to handle ObjectMetadataItem SSE events and refresh navigation menu items |
| packages/twenty-front/src/modules/metadata-store/effect-components/NavigationMenuItemSSEEffect.tsx | New component to handle NavigationMenuItem SSE events |
| packages/twenty-front/src/modules/metadata-store/effect-components/FieldMetadataSSEEffect.tsx | New component to handle FieldMetadata SSE events |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/twenty-front/src/modules/navigation-menu-item/utils/sortNavigationMenuItems.ts
Outdated
Show resolved
Hide resolved
...es/twenty-front/src/modules/metadata-store/effect-components/NavigationMenuItemSSEEffect.tsx
Outdated
Show resolved
Hide resolved
| store.set( | ||
| prefetchNavigationMenuItemsState.atom, | ||
| navigationMenuItemsResult.data?.navigationMenuItems ?? [], | ||
| ); |
There was a problem hiding this comment.
After updating the navigation menu items state, the metadata store should also be updated for consistency with the NavigationMenuItemSSEEffect implementation. The updateDraft and applyChanges methods should be called with the new navigation menu items data to keep the metadata store in sync.
| store.set( | |
| prefetchNavigationMenuItemsState.atom, | |
| navigationMenuItemsResult.data?.navigationMenuItems ?? [], | |
| ); | |
| const newNavigationMenuItems = | |
| navigationMenuItemsResult.data?.navigationMenuItems ?? []; | |
| store.set(prefetchNavigationMenuItemsState.atom, newNavigationMenuItems); | |
| updateDraft('navigationMenuItems', newNavigationMenuItems); | |
| applyChanges(); |
Greptile SummaryThis PR adds Server-Sent Events (SSE) handling for Key Changes:
Minor Issues:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant SSE as SSE Server
participant Provider as SSEProvider
participant FieldEffect as FieldMetadataSSEEffect
participant ObjEffect as ObjectMetadataItemSSEEffect
participant NavEffect as NavigationMenuItemSSEEffect
participant Store as Jotai Store
participant MetaStore as MetadataStore
SSE->>Provider: SSE Event Stream
rect rgb(200, 220, 250)
Note over FieldEffect: Field Metadata Changed
FieldEffect->>FieldEffect: Listen to fieldMetadata events
SSE->>FieldEffect: fieldMetadata event
FieldEffect->>FieldEffect: refreshObjectMetadataItems()
FieldEffect->>Store: get objectMetadataItemsState
FieldEffect->>MetaStore: updateDraft('objectMetadataItems')
FieldEffect->>MetaStore: applyChanges()
end
rect rgb(220, 250, 200)
Note over ObjEffect: Object Metadata Changed
ObjEffect->>ObjEffect: Listen to objectMetadata events
SSE->>ObjEffect: objectMetadata event
ObjEffect->>ObjEffect: refreshObjectMetadataItems()
ObjEffect->>Store: get objectMetadataItemsState
ObjEffect->>MetaStore: updateDraft('objectMetadataItems')
ObjEffect->>MetaStore: applyChanges()
ObjEffect->>ObjEffect: findManyNavigationMenuItemsLazy()
ObjEffect->>Store: Check with isDeeplyEqual
ObjEffect->>Store: set prefetchNavigationMenuItemsState (if changed)
end
rect rgb(250, 220, 200)
Note over NavEffect: Navigation Menu Changed
NavEffect->>NavEffect: Listen to navigationMenuItem events
SSE->>NavEffect: navigationMenuItem event
NavEffect->>NavEffect: findManyNavigationMenuItemsLazy()
NavEffect->>Store: get prefetchNavigationMenuItemsState
NavEffect->>Store: Check with isDeeplyEqual
NavEffect->>Store: set prefetchNavigationMenuItemsState (if changed)
NavEffect->>MetaStore: updateDraft('navigationMenuItems')
NavEffect->>MetaStore: applyChanges()
end
Last reviewed commit: 20fc48d |
|
|
||
| const store = useStore(); | ||
|
|
||
| const { refreshObjectMetadataItems } = useRefreshObjectMetadataItems(); |
There was a problem hiding this comment.
Unused import - refreshObjectMetadataItems is destructured but never called in this component
| const { refreshObjectMetadataItems } = useRefreshObjectMetadataItems(); | |
| const { updateDraft, applyChanges } = useMetadataStore(); |
There was a problem hiding this comment.
1 issue found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/twenty-front/src/modules/metadata-store/effect-components/ObjectMetadataItemSSEEffect.tsx">
<violation number="1" location="packages/twenty-front/src/modules/metadata-store/effect-components/ObjectMetadataItemSSEEffect.tsx:58">
P2: Guard against undefined navigation menu query results before overwriting the prefetch state; otherwise a failed/empty query will clear existing items to `[]`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ) { | ||
| store.set( | ||
| prefetchNavigationMenuItemsState.atom, | ||
| navigationMenuItemsResult.data?.navigationMenuItems ?? [], |
There was a problem hiding this comment.
P2: Guard against undefined navigation menu query results before overwriting the prefetch state; otherwise a failed/empty query will clear existing items to [].
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/twenty-front/src/modules/metadata-store/effect-components/ObjectMetadataItemSSEEffect.tsx, line 58:
<comment>Guard against undefined navigation menu query results before overwriting the prefetch state; otherwise a failed/empty query will clear existing items to `[]`.</comment>
<file context>
@@ -0,0 +1,65 @@
+ ) {
+ store.set(
+ prefetchNavigationMenuItemsState.atom,
+ navigationMenuItemsResult.data?.navigationMenuItems ?? [],
+ );
+ }
</file context>
…ortNavigationMenuItems.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
...es/twenty-front/src/modules/metadata-store/effect-components/ObjectMetadataItemSSEEffect.tsx
Show resolved
Hide resolved
| if ( | ||
| !isDeeplyEqual( | ||
| existingNavigationMenuItems, | ||
| navigationMenuItemsResult.data?.navigationMenuItems, | ||
| ) | ||
| ) { | ||
| store.set( | ||
| prefetchNavigationMenuItemsState.atom, | ||
| navigationMenuItemsResult.data?.navigationMenuItems ?? [], | ||
| ); | ||
| } |
There was a problem hiding this comment.
Bug: If the findManyNavigationMenuItemsLazy query fails, the navigation menu items state is incorrectly cleared, causing the navigation menu to disappear from the UI.
Severity: HIGH
Suggested Fix
Add a guard to check if navigationMenuItemsResult.data?.navigationMenuItems is defined before performing the isDeeplyEqual comparison and subsequent state update. This will prevent clearing the state on a query error. For example: if (!isDefined(navigationMenuItemsResult.data?.navigationMenuItems)) { return; }.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
packages/twenty-front/src/modules/metadata-store/effect-components/ObjectMetadataItemSSEEffect.tsx#L42-L60
Potential issue: In `ObjectMetadataItemSSEEffect`, if the
`findManyNavigationMenuItemsLazy` query returns an error (e.g., due to a network issue),
`navigationMenuItemsResult.data` will be `undefined`. The code proceeds to compare the
existing state with `undefined` using `isDeeplyEqual`, which returns `false`. This
incorrectly triggers a state update, `store.set(...,
navigationMenuItemsResult.data?.navigationMenuItems ?? [])`, which resolves to setting
the navigation menu items to an empty array. This causes the navigation menu to
disappear from the UI until the data is successfully refetched. A similar component,
`NavigationMenuItemSSEEffect`, correctly handles this case with a guard.
|
Hey @lucasbordeau! After you've done the QA of your Pull Request, you can mark it as done here. Thank you! |
This PR allows to handle ObjectMetadataItem, FieldMetadataItem and NavigationMenuItem SSE events.