Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a frames communication prototype that introduces extensible state management capabilities for cross-frame communication between host and guest components in the UIX framework. The implementation provides a centralized state management system that allows data synchronization between different UI frames.
Key changes:
- Adds extensible state management system with store manager and pub/sub mechanism
- Implements React hooks and components for both host and guest environments
- Creates new
uix-guest-reactpackage for React-specific guest functionality - Establishes communication channels between UI frames for state broadcasting
Reviewed Changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/uix-host/src/port.ts | Adds uiFrames tracking array to Port class |
| packages/uix-host-react/src/hooks/useExtensibilityState.tsx | Implements host-side state management hook |
| packages/uix-host-react/src/components/ExtensibleState/* | Creates extensible state provider components |
| packages/uix-host-react/src/components/GuestUIFrame.tsx | Registers UI frames for communication |
| packages/uix-guest/src/guest-ui.ts | Adds broadcast capabilities to guest UI |
| packages/uix-guest-react/* | Complete new package for React guest components |
| packages/uix-core/src/store/* | Core state management infrastructure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -0,0 +1,8 @@ | |||
| import { useExtensibilityState } from "./useExtensibilityState"; | |||
|
|
|||
| export const useHostExtensibilityState = ( | |||
There was a problem hiding this comment.
Function name contains a typo: 'useHostExtensibityState' should be 'useHostExtensibilityState' (missing 'il' in 'Extensibility').
| @@ -0,0 +1,8 @@ | |||
| import { useExtensibilityState } from "./useExtensibilityState"; | |||
|
|
|||
| export const useExtensionExtensibityState = ( | |||
There was a problem hiding this comment.
Function name contains a typo: 'useExtensionExtensibityState' should be 'useExtensionExtensibilityState' (missing 'il' in 'Extensibility').
| export const useExtensionExtensibityState = ( | |
| export const useExtensionExtensibilityState = ( |
| export * from "./useHostExtensibityState.js"; | ||
| export * from "./useExtensionExtensibityState.js"; |
There was a problem hiding this comment.
Export paths contain typos: 'ExtensibityState' should be 'ExtensibilityState' in both file references (missing 'il' in 'Extensibility').
| export * from "./useHostExtensibityState.js"; | |
| export * from "./useExtensionExtensibityState.js"; | |
| export * from "./useHostExtensibilityState.js"; | |
| export * from "./useExtensionExtensibilityState.js"; |
| @@ -0,0 +1 @@ | |||
| This is the `@adobe/uix-host-react` package alone. See [/README.md](../../README.md) for details. | |||
There was a problem hiding this comment.
README incorrectly refers to '@adobe/uix-host-react' but this is the uix-guest-react package. Should be '@adobe/uix-guest-react'.
| This is the `@adobe/uix-host-react` package alone. See [/README.md](../../README.md) for details. | |
| This is the `@adobe/uix-guest-react` package alone. See [/README.md](../../README.md) for details. |
| export const ExtensibleStateProvider = ({ | ||
| children, | ||
| }: PropsWithChildren<unknown>) => { | ||
| const [storeManager, setStoreManager] = useState(null); |
There was a problem hiding this comment.
Using null as initial state is not type-safe. Consider using useState<ExtensibleStoreManagerInterface | null>(null) or a more appropriate initial value to maintain type safety.
| const [storeManager, setStoreManager] = useState(null); | |
| const [storeManager, setStoreManager] = useState<ExtensibleStoreManager | null>(null); |
| const [connection, setConnection] = useState( | ||
| undefined as GuestUI<VirtualApi> | ||
| ); | ||
| const [storeManager, setStoreManager] = useState(null); |
There was a problem hiding this comment.
Using null as initial state is not type-safe. Consider using useState<ExtensibleStoreManagerInterface | null>(null) or a more appropriate initial value to maintain type safety.
| const [storeManager, setStoreManager] = useState(null); | |
| const [storeManager, setStoreManager] = useState<ExtensibleStoreManager | null>(null); |
| }); | ||
| setState(def); | ||
| } | ||
| }, [storeManager]); |
There was a problem hiding this comment.
The useEffect dependency array is missing key and defaultState dependencies. This could cause the effect to not re-run when these values change, potentially leading to stale closures or incorrect behavior.
| }, [storeManager]); | |
| }, [storeManager, key, defaultState]); |
| setState(value); | ||
| } | ||
| })(); | ||
| }, [connection]); |
There was a problem hiding this comment.
The useEffect dependency array is missing key, defaultState, scope, and storeManager dependencies. This could cause the effect to not re-run when these values change, potentially leading to stale closures or incorrect behavior.
| }, [connection]); | |
| }, [connection, key, defaultState, scope, storeManager]); |
| }); | ||
| setState(def); | ||
| } | ||
| }, [storeManager]); |
There was a problem hiding this comment.
The useCallback dependency array is missing key dependency. This could cause the callback to use stale values of key if it changes.
| }, [storeManager]); | |
| }, [storeManager, key]); |
| setState(value); | ||
| } | ||
| })(); | ||
| }, [connection]); |
There was a problem hiding this comment.
The useCallback dependency array is missing key and scope dependencies. This could cause the callback to use stale values if these change.
1 participant
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: