Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the A2UI core to be framework-agnostic by abstracting reactivity through a new FrameworkSignal interface and utilizing generics across the state and rendering layers. The React renderer has been updated to provide a Preact-based signal implementation, and the explorer app and tests have been adjusted accordingly. Feedback highlights several critical areas for improvement: the FrameworkSignal interface should be extended to support batching to prevent performance regressions; encapsulation should be respected in the GenericBinder rather than using type casts to access private members; the lifecycle management of AbortController in DataContext needs to be restored to prevent resource leaks; and the effect type definition should be updated to support cleanup functions.
|
|
||
| batch(() => { | ||
| this.updateSignal(normalizedPath); | ||
| this.updateSignal(normalizedPath); |
There was a problem hiding this comment.
The removal of the batch wrapper in notifySignals and notifyAllSignals is a significant change that may impact performance and consistency. Batching ensures that multiple signal updates are processed as a single atomic operation, which is essential to avoid redundant re-computations of derived signals and multiple re-renders of components. Since the core is now framework-agnostic, the FrameworkSignal interface should be extended to include a batch method (e.g., batch: (fn: () => void) => void) so that each framework implementation can provide its own optimization.
| } else if ((bindingRef.current as unknown as {context: ComponentContext}).context !== context) { | ||
| bindingRef.current = new GenericBinder<Props, 'react'>(context, api.schema); | ||
| } else if ( | ||
| (bindingRef.current as unknown as {context: ComponentContext<'react'>}).context !== context |
There was a problem hiding this comment.
Accessing the private context property of GenericBinder via a type cast to unknown is a violation of encapsulation. This makes the code fragile and dependent on internal implementation details. It would be better to expose a public getter for the context or a method like hasContext(context: ComponentContext<SK>): boolean on the GenericBinder class.
| return this.frameworkSignal.isSignal(result) | ||
| ? result | ||
| : this.frameworkSignal.wrap(result); | ||
| } |
There was a problem hiding this comment.
In the branch for function calls with no arguments, the AbortController created for the function execution is no longer being aborted when the signal is disposed. The previous implementation (see line 206 in the LEFT side) attached an unsubscribe method to the signal to handle this. Without a similar mechanism or an effect to manage the controller's lifecycle, asynchronous operations started by the function will leak when the context is disposed.
| wrap: <T>(val: T) => new Signal<T>(val), | ||
| unwrap: <T>(val: Signal<T>) => val.value, | ||
| set: <T>(signal: Signal<T>, value: T) => (signal.value = value), | ||
| effect: (fn: () => void) => effect(fn), |
There was a problem hiding this comment.
The type definition for the effect parameter fn is () => void, but the implementation in data-context.ts (e.g., line 227) passes a function that returns a cleanup function. To avoid type errors and ensure proper resource management, the FrameworkSignal interface and this implementation should be updated to support cleanup functions: fn: () => void | (() => void).
| effect: (fn: () => void) => effect(fn), | |
| effect: (fn: () => void | (() => void)) => effect(fn), |
2 participants
Description
Attempting to fix React CI for #1009; do not merge this.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.