add support for event state + require event load to be serializable #6568
Conversation
WalkthroughThis PR adds a synchronized, event-driven state layer and related utilities to the events package. It introduces EventStateStore (per-channel registry, per-event trackers, subscribe/getSnapshot/getSnapshotWithDefault) and exposes a new hook useEventState that uses useSyncExternalStore to provide tear-free reads of the latest event payload. Two factory helpers—createUseSynchronizedEventState and createUseSynchronizedEventStateWithDefaults—are added to produce typed, synchronized hooks (with optional per-event defaults). Types are tightened with a new PlainData alias and EventGroup now restricts payloads to plain/serializable data. A demo component (StateDemo) illustrates usage, and extensive tests for store and hooks are added. Some JSDoc and examples were expanded. Sequence DiagramsequenceDiagram
participant Source as Event Dispatcher (e.g., CounterSource)
participant Bus as Event Bus
participant Store as EventStateStore
participant Hook as useEventState / factory hook
participant Component as Consumer (CounterDisplay)
Source->>Bus: dispatch("event:key", payload)
activate Bus
Bus->>Store: bus handler invoked with payload
activate Store
Store->>Store: update tracker.latestPayload
Store->>Store: notify subscribers (listener callbacks)
Store-->>Bus: (no direct response)
deactivate Store
deactivate Bus
Note right of Hook: Subscribed via useSyncExternalStore
Hook->>Store: subscription.getSnapshot() (render time)
Hook->>Component: returns latest payload (or default)
Component->>Component: render with synchronized state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
app/packages/events/src/demo-state.tsx(1 hunks)app/packages/events/src/hooks/createUseEventHandler.ts(2 hunks)app/packages/events/src/hooks/createUseSynchronizedEventState.test.tsx(1 hunks)app/packages/events/src/hooks/createUseSynchronizedEventState.ts(1 hunks)app/packages/events/src/hooks/index.ts(1 hunks)app/packages/events/src/hooks/useEventBus.ts(1 hunks)app/packages/events/src/hooks/useEventState.test.tsx(1 hunks)app/packages/events/src/hooks/useEventState.ts(1 hunks)app/packages/events/src/index.ts(1 hunks)app/packages/events/src/state/EventStateStore.test.ts(1 hunks)app/packages/events/src/state/EventStateStore.ts(1 hunks)app/packages/events/src/state/index.ts(1 hunks)app/packages/events/src/types/event.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/events/src/state/index.tsapp/packages/events/src/hooks/useEventBus.tsapp/packages/events/src/hooks/useEventState.tsapp/packages/events/src/types/event.tsapp/packages/events/src/hooks/createUseSynchronizedEventState.tsapp/packages/events/src/hooks/createUseEventHandler.tsapp/packages/events/src/hooks/useEventState.test.tsxapp/packages/events/src/hooks/createUseSynchronizedEventState.test.tsxapp/packages/events/src/index.tsapp/packages/events/src/state/EventStateStore.tsapp/packages/events/src/demo-state.tsxapp/packages/events/src/hooks/index.tsapp/packages/events/src/state/EventStateStore.test.ts
🧠 Learnings (3)
📚 Learning: 2024-11-06T20:52:30.520Z
Learnt from: benjaminpkane
Repo: voxel51/fiftyone PR: 5051
File: app/packages/utilities/src/index.ts:12-12
Timestamp: 2024-11-06T20:52:30.520Z
Learning: In `app/packages/utilities/src/index.ts`, when an export statement like `export * from "./Resource";` is moved within the file, it is not a duplication even if it appears in both the removed and added lines of a diff.
Applied to files:
app/packages/events/src/state/index.tsapp/packages/events/src/index.tsapp/packages/events/src/hooks/index.ts
📚 Learning: 2025-10-02T21:53:53.778Z
Learnt from: tom-vx51
Repo: voxel51/fiftyone PR: 6372
File: app/packages/core/src/client/annotationClient.ts:32-46
Timestamp: 2025-10-02T21:53:53.778Z
Learning: In `app/packages/core/src/client/annotationClient.ts`, the `delta` field in `PatchSampleRequest` intentionally does not accept simple scalar values (strings, numbers, booleans). The `AttributeField` type is correctly defined as `Record<string, unknown>` to enforce that all delta entries must be structured objects, not primitives.
Applied to files:
app/packages/events/src/types/event.ts
📚 Learning: 2025-04-23T15:22:03.452Z
Learnt from: benjaminpkane
Repo: voxel51/fiftyone PR: 5732
File: app/packages/core/src/components/Filters/use-query-performance-icon.tsx:26-38
Timestamp: 2025-04-23T15:22:03.452Z
Learning: React hooks (including useRecoilValue and other state management hooks) must be called unconditionally at the top level of a component or custom hook. They cannot be placed inside conditional statements, loops, or nested functions, as this violates React's Rules of Hooks and leads to unpredictable behavior.
Applied to files:
app/packages/events/src/demo-state.tsx
🧬 Code graph analysis (7)
app/packages/events/src/hooks/useEventState.ts (2)
app/packages/events/src/types/event.ts (1)
EventGroup(62-62)app/packages/events/src/state/EventStateStore.ts (1)
getEventStateStore(153-160)
app/packages/events/src/hooks/createUseSynchronizedEventState.ts (2)
app/packages/events/src/types/event.ts (1)
EventGroup(62-62)app/packages/events/src/hooks/useEventState.ts (1)
useEventState(59-71)
app/packages/events/src/hooks/useEventState.test.tsx (3)
app/packages/events/src/state/EventStateStore.ts (1)
__test__(165-167)app/packages/events/src/hooks/useEventState.ts (1)
useEventState(59-71)app/packages/events/src/hooks/useEventBus.ts (1)
useEventBus(17-28)
app/packages/events/src/hooks/createUseSynchronizedEventState.test.tsx (3)
app/packages/events/src/state/EventStateStore.ts (1)
__test__(165-167)app/packages/events/src/hooks/createUseSynchronizedEventState.ts (2)
createUseSynchronizedEventState(55-61)createUseSynchronizedEventStateWithDefaults(83-97)app/packages/events/src/hooks/useEventBus.ts (1)
useEventBus(17-28)
app/packages/events/src/state/EventStateStore.ts (1)
app/packages/events/src/types/event.ts (1)
EventGroup(62-62)
app/packages/events/src/demo-state.tsx (4)
app/packages/events/src/hooks/createUseSynchronizedEventState.ts (1)
createUseSynchronizedEventState(55-61)app/packages/events/src/hooks/createUseEventHandler.ts (1)
createUseEventHandler(44-56)app/packages/events/src/hooks/useEventBus.ts (1)
useEventBus(17-28)app/packages/events/src/hooks/useEventState.ts (1)
useEventState(59-71)
app/packages/events/src/state/EventStateStore.test.ts (1)
app/packages/events/src/state/EventStateStore.ts (2)
__test__(165-167)getEventStateStore(153-160)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.12)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.9)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: lint / eslint
- GitHub Check: test / test-app
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
🔇 Additional comments (10)
app/packages/events/src/hooks/useEventBus.ts (1)
9-12: Documentation enhancements are clear and helpful.The guidance on when to use each hook type (dispatch/subscribe vs side effects vs render state) provides valuable context for developers choosing between the event system APIs.
app/packages/events/src/hooks/createUseEventHandler.ts (1)
9-20: Documentation effectively contrasts handler vs state patterns.The explanation of when to use side-effect handlers versus synchronized state reading is clear and helps developers make the right architectural choice.
Also applies to: 33-38
app/packages/events/src/types/event.ts (2)
28-35: PlainData type provides compile-time guidance but can't catch all non-serializable types.The recursive type definition correctly restricts to primitives, arrays, and plain objects. However, TypeScript's structural typing means instances of classes (with only plain-data fields), Date objects, Map, Set, and other non-serializable types could match the structure at compile time. The documentation appropriately emphasizes the serialization requirement, but developers must follow this convention—there's no runtime enforcement.
This is a reasonable trade-off given TypeScript's type system limitations, but be aware that
as anyor structurally-matching non-plain objects can bypass the constraint.
62-62: EventGroup constraint properly tightens payload typing.Restricting event payloads to
PlainData | undefined | nullaligns with the serialization requirements for the state store. The constraint will catch most misuse at compile time.app/packages/events/src/index.ts (1)
3-3: Public API expansion is appropriate.Re-exporting the state module makes the EventStateStore and related utilities accessible to consumers.
app/packages/events/src/state/index.ts (1)
1-1: State module index follows standard patterns.app/packages/events/src/hooks/index.ts (1)
2-4: Public hook API properly expanded.The new synchronized state hooks are now accessible alongside existing event handler hooks.
app/packages/events/src/hooks/createUseSynchronizedEventState.test.tsx (2)
16-20: Proper test isolation with registry cleanup.Clearing the
__test__.registryinbeforeEachprevents test cross-contamination and ensures each test starts with a clean state.
66-91: Tearing prevention test correctly validates synchronized reads.Testing that three separate components reading the same event see identical values demonstrates the core benefit of
useSyncExternalStore—preventing visual inconsistencies during concurrent rendering.app/packages/events/src/hooks/useEventState.ts (1)
59-71: No changes needed—the review comment is incorrect.The
createSubscriptionmethod inEventStateStore.tsalready implements internal memoization. It caches subscription objects in aMapkeyed by event, returning the cached instance on subsequent calls (line 110:const existing = this.subscriptions.get(event); if (existing) return existing;).When
useEventStatecallsstore.createSubscription(event)on every render, it's performing a cheapMap.get()lookup, not allocating new subscription objects. The EventStateStore instance itself is also memoized per channel via the registry.The proposed
useMemowrapper is redundant. Tests confirm correctness, including tearing prevention across multiple components using the same event.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
app/packages/events/src/hooks/createUseSynchronizedEventState.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Files:
app/packages/events/src/hooks/createUseSynchronizedEventState.ts
🧬 Code graph analysis (1)
app/packages/events/src/hooks/createUseSynchronizedEventState.ts (2)
app/packages/events/src/types/event.ts (1)
EventGroup(62-62)app/packages/events/src/hooks/useEventState.ts (1)
useEventState(59-71)
🔇 Additional comments (4)
app/packages/events/src/hooks/createUseSynchronizedEventState.ts (4)
1-2: Imports look good.Clean dependencies, properly scoped.
4-54: Excellent documentation.The comparison with
createUseEventHandleris clear, and the examples correctly demonstrate null-safe access patterns.
55-61: Implementation is correct.Simple delegation with proper type preservation.
83-97: Implementation logic is sound.The past review concern has been addressed—return type correctly reflects the possibility of
undefined, and the unsafe cast has been removed. The fallback logic is correct.
| * function CounterDisplay() { | ||
| * // Always returns a value (never undefined), synchronized across all components | ||
| * const latestEvent = useCounterState("counter:updated"); | ||
| * return <div>Count: {latestEvent.count}</div>; | ||
| * } |
There was a problem hiding this comment.
Fix misleading JSDoc claim and unsafe example.
Line 77 claims "Always returns a value (never undefined)" but the return type is T[K] | undefined. This claim only holds when a default is provided for that specific event—users can call useCounterState with events that have no default, which returns undefined. The example's direct property access (latestEvent.count) would crash at runtime if no default exists.
Update the JSDoc to clarify the conditional guarantee:
* function CounterDisplay() {
- * // Always returns a value (never undefined), synchronized across all components
+ * // Returns a defined value when a default is provided for the event
* const latestEvent = useCounterState("counter:updated");
- * return <div>Count: {latestEvent.count}</div>;
+ * // Safe because we provided a default for "counter:updated"
+ * return <div>Count: {latestEvent.count}</div>;
* }🤖 Prompt for AI Agents
In app/packages/events/src/hooks/createUseSynchronizedEventState.ts around lines
76 to 80, the JSDoc incorrectly states "Always returns a value (never
undefined)" while the function signature returns T[K] | undefined, and the
example accesses latestEvent.count unsafely; update the JSDoc to state that the
hook returns the event value or undefined unless a default is provided for that
event, and update the example to either show providing a default or safely
handle undefined (e.g., optional chaining, nullish coalescing, or an explicit
guard) so property access cannot crash at runtime.
What changes are proposed in this pull request?
In our current setup, if event payload is used as React state, multiple components might get a different read during React's concurrent rendering. This PR introduces the concept of "event state" which uses useSyncExternalStore to make sure no tearing occurs.
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Documentation
Tests