Allow JS root components to reinitialize on circuit restart#32
Conversation
WalkthroughThe changes implement per-renderer tracking for dynamic JavaScript root components by adding 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
Comment |
![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 currently throws when manager exists and
currentRendererId === rendererId, which is inverted; update the condition in the
block that references manager, currentRendererId and rendererId so it throws
only when a different renderer is attempting to enable JS root components (i.e.,
change the check to manager && currentRendererId !== rendererId), and update the
inline comment if needed to reflect the correct cross-renderer protection logic
in JSRootComponents.ts.
- Around line 136-139: jsComponentParametersByIdentifier is no longer
initialized, causing RootComponentsFunctions.add to dereference undefined;
inside the same initialization block where currentRendererId and manager are set
(the if (!hasInitializedJsComponents) branch), initialize
jsComponentParametersByIdentifier to the same empty collection type it used to
be (e.g., {} or new Map depending on original usage), then set
hasInitializedJsComponents = true so subsequent calls like
RootComponentsFunctions.add can safely read/write it.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Components/Web.JS/src/Rendering/JSRootComponents.tssrc/Components/Web.JS/src/Rendering/WebRendererInteropMethods.tssrc/Components/test/E2ETest/Tests/StatePersistanceJSRootTest.cssrc/Components/test/E2ETest/Tests/StatePersistenceTest.cssrc/Components/test/testassets/Components.TestServer/Program.cssrc/Components/test/testassets/Components.TestServer/RazorComponentEndpointsStartup.cs
💤 Files with no reviewable changes (2)
- src/Components/test/E2ETest/Tests/StatePersistanceJSRootTest.cs
- src/Components/test/testassets/Components.TestServer/Program.cs
🧰 Additional context used
🧬 Code graph analysis (2)
src/Components/Web.JS/src/Rendering/WebRendererInteropMethods.ts (1)
src/Components/Web.JS/src/Rendering/JSRootComponents.ts (1)
enableJSRootComponents(120-151)
src/Components/Web.JS/src/Rendering/JSRootComponents.ts (1)
src/JSInterop/Microsoft.JSInterop.JS/src/src/Microsoft.JSInterop.ts (1)
DotNetObject(697-718)
🔇 Additional comments (4)
src/Components/test/testassets/Components.TestServer/RazorComponentEndpointsStartup.cs (1)
54-55: Unconditional JS root registration looks fine here.src/Components/test/E2ETest/Tests/StatePersistenceTest.cs (1)
279-291: Nice coverage for dynamic JS root persistence across render modes.src/Components/Web.JS/src/Rendering/WebRendererInteropMethods.ts (1)
32-35: Passing rendererId into enableJSRootComponents is the right update.src/Components/Web.JS/src/Rendering/JSRootComponents.ts (1)
12-15: State tracking additions are reasonable for per-renderer lifecycle control.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Comment on lines
+126
to
130
| 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.
Guard condition is inverted; it throws on same renderer instead of different renderer.
This defeats the cross-renderer protection and may block same-renderer re-enablement depending on rendererId stability.
🔧 Suggested fix
- if (manager && currentRendererId === rendererId) {
+ 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 currently throws when manager exists and currentRendererId ===
rendererId, which is inverted; update the condition in the block that references
manager, currentRendererId and rendererId so it throws only when a different
renderer is attempting to enable JS root components (i.e., change the check to
manager && currentRendererId !== rendererId), and update the inline comment if
needed to reflect the correct cross-renderer protection logic in
JSRootComponents.ts.
Comment on lines
+136
to
+139
| 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); | ||
|
|
||
| if (!hasInitializedJsComponents) { |
There was a problem hiding this comment.
jsComponentParametersByIdentifier is never set after the change.
RootComponentsFunctions.add dereferences it, so this will throw at runtime once a dynamic root is added.
🔧 Suggested 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.
Suggested change
| 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); | |
| if (!hasInitializedJsComponents) { | |
| currentRendererId = rendererId; | |
| manager = managerInstance; | |
| jsComponentParametersByIdentifier = jsComponentParameters; | |
| if (!hasInitializedJsComponents) { |
🤖 Prompt for AI Agents
In `@src/Components/Web.JS/src/Rendering/JSRootComponents.ts` around lines 136 -
139, jsComponentParametersByIdentifier is no longer initialized, causing
RootComponentsFunctions.add to dereference undefined; inside the same
initialization block where currentRendererId and manager are set (the if
(!hasInitializedJsComponents) branch), initialize
jsComponentParametersByIdentifier to the same empty collection type it used to
be (e.g., {} or new Map depending on original usage), then set
hasInitializedJsComponents = true so subsequent calls like
RootComponentsFunctions.add can safely read/write it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Benchmark PR from agentic-review-benchmarks#7
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.