-
Notifications
You must be signed in to change notification settings - Fork 9
Frames communication prototype #85
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?
Conversation
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.
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 = ( | |||
Copilot
AI
Sep 30, 2025
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.
Function name contains a typo: 'useHostExtensibityState' should be 'useHostExtensibilityState' (missing 'il' in 'Extensibility').
| @@ -0,0 +1,8 @@ | |||
| import { useExtensibilityState } from "./useExtensibilityState"; | |||
|
|
|||
| export const useExtensionExtensibityState = ( | |||
Copilot
AI
Sep 30, 2025
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.
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"; |
Copilot
AI
Sep 30, 2025
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.
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. | |||
Copilot
AI
Sep 30, 2025
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.
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); |
Copilot
AI
Sep 30, 2025
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.
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); |
Copilot
AI
Sep 30, 2025
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.
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]); |
Copilot
AI
Sep 30, 2025
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 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]); |
Copilot
AI
Sep 30, 2025
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 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]); |
Copilot
AI
Sep 30, 2025
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 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]); |
Copilot
AI
Sep 30, 2025
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 useCallback dependency array is missing key and scope dependencies. This could cause the callback to use stale values if these change.
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: