Allow JS root components to reinitialize on circuit restart#7
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7bb6a08b45
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // accept the new manager. The old manager's DotNetObjectReference is no longer valid anyway | ||
| // because the old circuit is gone. We don't dispose the old manager - doing so would cause | ||
| // JSDisconnectedException because the circuit that created it no longer exists. | ||
| currentRendererId = rendererId; | ||
| manager = managerInstance; |
There was a problem hiding this comment.
After this change jsComponentParametersByIdentifier is never updated when JS root components are enabled. RootComponentsFunctions.add later does jsComponentParametersByIdentifier[componentIdentifier], which will throw a TypeError the first time a dynamic root component is added in any session (including the initial one) because the variable remains undefined. This breaks dynamic JS root components entirely after this commit; the assignment should be restored during enablement.
Useful? React with 👍 / 👎.
| if (manager && currentRendererId === rendererId) { | ||
| // A different renderer type (e.g., Server vs WebAssembly) is trying to enable JS root components. | ||
| // This is a multi-host scenario which is not supported for dynamic root components. | ||
| throw new Error('Dynamic root components have already been enabled.'); |
There was a problem hiding this comment.
The guard in enableJSRootComponents throws when currentRendererId === rendererId, which is exactly the “same renderer re-enables (e.g., circuit restart)” scenario described in the comment below. That means a circuit restart on the same renderer still throws “already enabled,” so the intended reinitialization never happens. The comparison should reject different renderer IDs instead, otherwise re-enable for the same renderer remains broken.
Useful? React with 👍 / 👎.
2 participants
Benchmark PR from agentic-review-benchmarks#7