Allow JS root components to reinitialize on circuit restart - #7
Conversation
📝 WalkthroughWalkthroughThe changes implement per-renderer tracking for dynamic JavaScript root components by introducing a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Components/Web.JS/src/Rendering/JSRootComponents.ts`:
- Around line 126-130: The guard in JSRootComponents.ts incorrectly throws when
the same renderer re-attaches; update the condition in the block that checks
manager and renderer ids so it only throws when a different renderer is present
(i.e., keep the check for manager but change the comparison between
currentRendererId and rendererId to detect inequality), allowing same-renderer
re-enable; refer to the variables manager, currentRendererId, and rendererId in
the JSRootComponents module to locate and fix the condition.
- Around line 136-138: When reassigning the renderer/manager (the lines that set
currentRendererId = rendererId and manager = managerInstance), also refresh the
cached component metadata by updating jsComponentParametersByIdentifier from the
new manager; e.g., call the managerInstance method that returns current
component parameter metadata (or invoke the existing refresh helper) and assign
its result to jsComponentParametersByIdentifier so dynamic root additions read
fresh metadata after re-init.
| 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.
Fix renderer-id guard to allow same-renderer re-enable.
Line 126 currently throws when the same renderer re-attaches, which blocks circuit restart (the scenario this change targets). Flip the comparison to only throw on a different renderer id.
🔧 Proposed fix
- if (manager && currentRendererId === rendererId) {
+ if (manager && currentRendererId !== rendererId) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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.'); | |
| } | |
| 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.'); | |
| } |
🤖 Prompt for AI Agents
In `@src/Components/Web.JS/src/Rendering/JSRootComponents.ts` around lines 126 -
130, The guard in JSRootComponents.ts incorrectly throws when the same renderer
re-attaches; update the condition in the block that checks manager and renderer
ids so it only throws when a different renderer is present (i.e., keep the check
for manager but change the comparison between currentRendererId and rendererId
to detect inequality), allowing same-renderer re-enable; refer to the variables
manager, currentRendererId, and rendererId in the JSRootComponents module to
locate and fix the condition.
| currentRendererId = rendererId; | ||
| manager = managerInstance; | ||
| jsComponentParametersByIdentifier = jsComponentParameters; | ||
|
|
||
| // Call the registered initializers. This is an arbitrary subset of the JS component types that are registered | ||
| // on the .NET side - just those of them that require some JS-side initialization (e.g., to register them | ||
| // as custom elements). | ||
| for (const [initializerIdentifier, componentIdentifiers] of Object.entries(jsComponentInitializers)) { | ||
| const initializerFunc = DotNet.findJSFunction(initializerIdentifier, 0) as JSComponentInitializerCallback; | ||
| for (const componentIdentifier of componentIdentifiers) { | ||
| const parameters = jsComponentParameters[componentIdentifier]; | ||
| initializerFunc(componentIdentifier, parameters); | ||
|
|
There was a problem hiding this comment.
Refresh JS component parameter metadata on re-enable.
Line 137 updates the manager but never updates jsComponentParametersByIdentifier, so dynamic root additions can read undefined or stale metadata after re-init.
🛠️ Proposed fix
currentRendererId = rendererId;
manager = managerInstance;
+ jsComponentParametersByIdentifier = jsComponentParameters;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| currentRendererId = rendererId; | |
| manager = managerInstance; | |
| jsComponentParametersByIdentifier = jsComponentParameters; | |
| // Call the registered initializers. This is an arbitrary subset of the JS component types that are registered | |
| // on the .NET side - just those of them that require some JS-side initialization (e.g., to register them | |
| // as custom elements). | |
| for (const [initializerIdentifier, componentIdentifiers] of Object.entries(jsComponentInitializers)) { | |
| const initializerFunc = DotNet.findJSFunction(initializerIdentifier, 0) as JSComponentInitializerCallback; | |
| for (const componentIdentifier of componentIdentifiers) { | |
| const parameters = jsComponentParameters[componentIdentifier]; | |
| initializerFunc(componentIdentifier, parameters); | |
| currentRendererId = rendererId; | |
| manager = managerInstance; | |
| jsComponentParametersByIdentifier = jsComponentParameters; |
🤖 Prompt for AI Agents
In `@src/Components/Web.JS/src/Rendering/JSRootComponents.ts` around lines 136 -
138, When reassigning the renderer/manager (the lines that set currentRendererId
= rendererId and manager = managerInstance), also refresh the cached component
metadata by updating jsComponentParametersByIdentifier from the new manager;
e.g., call the managerInstance method that returns current component parameter
metadata (or invoke the existing refresh helper) and assign its result to
jsComponentParametersByIdentifier so dynamic root additions read fresh metadata
after re-init.
3 participants
Benchmark PR from agentic-review-benchmarks#7
Summary by CodeRabbit
New Features
Tests