feat(vite-plugin): enable clone optimization for components with child instances#43
feat(vite-plugin): enable clone optimization for components with child instances#43senrecep wants to merge 3 commits intodashersw:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements partial-clone optimization for components containing child components. The compiler now generates static HTML templates with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vite-plugin-gea/src/generate-clone.ts (1)
131-139:⚠️ Potential issue | 🔴 CriticalDo not use a hard-coded
<div>as the universal child-slot placeholder.This changes the parsed DOM before cloning. In valid templates like
<p><Child /></p>,<tbody><Row /></tbody>, or<svg><Icon /></svg>, the browser will reparent or discard the<div>duringtemplate.innerHTMLparsing, so the generatedchildPathno longer points at the slot andreplaceChildwill patch the wrong node or fail entirely. Please emit a context-valid placeholder here, or bail out of clone optimization for restricted parents.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-plugin-gea/src/generate-clone.ts` around lines 131 - 139, The function jsxToStaticHtml currently returns a hard-coded "<div data-gea-child-slot></div>" for component children which breaks parsing in restricted parents; modify jsxToStaticHtml to derive a context-valid placeholder from the parent tag (use elementPath[elementPath.length - 1] or the immediate parent's tag from getJSXTagName) and emit a placeholder using that same tag name with the data-gea-child-slot attribute, but if the parent is a restricted container where inserting a child element would be invalid (examples: p, tbody, thead, tfoot, tr, colgroup, svg-specific contexts, select/options, etc.) then bail out by returning null to skip clone optimization; update references to isComp/data-gea-child-slot logic accordingly in jsxToStaticHtml so replaceChild targets a context-correct node.
🧹 Nitpick comments (1)
packages/vite-plugin-gea/tests/optimization-tests.test.ts (1)
774-854: Add at least one render-level regression for the new clone path.These assertions only check emitted strings, so they still pass if the parsed skeleton is wrong or
replaceChildtargets the wrong node at runtime. A small test that actually mounts a compiled parent with child components—ideally under a restrictive container like<p>or<tbody>—would cover the risky part of this change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-plugin-gea/tests/optimization-tests.test.ts` around lines 774 - 854, The current tests only assert emitted strings from transformComponentSource (checking for __tpl, __cloneTemplate, data-gea-child-slot, replaceChild, etc.) but do not actually mount the compiled output to verify runtime behavior; add at least one render-level regression that compiles a parent component with a child (use transformComponentSource to get the compiled module or the plugin pipeline), mounts it into a restrictive container (e.g., a <p> or <tbody> element) and asserts the real DOM: the child component's .el is actually inserted in place of the placeholder and the parent markup structure is correct; target the same examples used in the existing tests (e.g., components referencing Header/Panel/Footer) and assert DOM node identity/position and that replaceChild was effective at runtime rather than just string presence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/vite-plugin-gea/src/generate-clone.ts`:
- Around line 131-139: The function jsxToStaticHtml currently returns a
hard-coded "<div data-gea-child-slot></div>" for component children which breaks
parsing in restricted parents; modify jsxToStaticHtml to derive a context-valid
placeholder from the parent tag (use elementPath[elementPath.length - 1] or the
immediate parent's tag from getJSXTagName) and emit a placeholder using that
same tag name with the data-gea-child-slot attribute, but if the parent is a
restricted container where inserting a child element would be invalid (examples:
p, tbody, thead, tfoot, tr, colgroup, svg-specific contexts, select/options,
etc.) then bail out by returning null to skip clone optimization; update
references to isComp/data-gea-child-slot logic accordingly in jsxToStaticHtml so
replaceChild targets a context-correct node.
---
Nitpick comments:
In `@packages/vite-plugin-gea/tests/optimization-tests.test.ts`:
- Around line 774-854: The current tests only assert emitted strings from
transformComponentSource (checking for __tpl, __cloneTemplate,
data-gea-child-slot, replaceChild, etc.) but do not actually mount the compiled
output to verify runtime behavior; add at least one render-level regression that
compiles a parent component with a child (use transformComponentSource to get
the compiled module or the plugin pipeline), mounts it into a restrictive
container (e.g., a <p> or <tbody> element) and asserts the real DOM: the child
component's .el is actually inserted in place of the placeholder and the parent
markup structure is correct; target the same examples used in the existing tests
(e.g., components referencing Header/Panel/Footer) and assert DOM node
identity/position and that replaceChild was effective at runtime rather than
just string presence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a245405-c2a6-4d99-88a3-630596c7ed2f
📒 Files selected for processing (4)
.changeset/clone-optimization-child-components.mdpackages/vite-plugin-gea/src/generate-clone.tspackages/vite-plugin-gea/src/transform-component.tspackages/vite-plugin-gea/tests/optimization-tests.test.ts
💤 Files with no reviewable changes (1)
- packages/vite-plugin-gea/src/transform-component.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/vite-plugin-gea/src/generate-clone.ts (2)
46-53: Consider expanding the restricted elements set for thoroughness.The set covers major categories (phrasing, table structure, lists, SVG), but some phrasing content elements are missing (e.g.,
button,label,code,kbd,abbr). These would also be invalid containers for arbitrary placeholder elements.That said, the practical impact is likely minimal since component children are typically placed in flow content containers like
divorsection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-plugin-gea/src/generate-clone.ts` around lines 46 - 53, RESTRICTED_CLONE_PARENTS currently omits several phrasing/content elements that should forbid inserting arbitrary placeholders; update the Set named RESTRICTED_CLONE_PARENTS to also include missing inline/phrasing tags such as button, label, code, kbd, abbr (and any other relevant inline controls like output, samp, var, sub, sup) so the generator avoids inserting clones into those invalid parents; locate the constant RESTRICTED_CLONE_PARENTS in generate-clone.ts and add the additional tag names to the Set.
597-619: Cursor-based instance matching relies on implicit traversal order consistency.The function assumes
getDirectChildElementstraversal order matches the DFS order used whencomponentInstanceswas populated incollectComponentTags. While both use the same filtering utility and should match in typical cases, the implicit coupling is fragile: any divergence in traversal logic would silently produce wrong instance variable pairings, and there is no bounds checking if a cursor exceeds the instances array.Consider adding a defensive check or using the existing
dfsIndexstored in eachChildComponentfor explicit matching:Suggested improvement
if (tag && isComponentTag(tag)) { const instances = componentInstances.get(tag) const cursor = instanceCursors.get(tag) ?? 0 const instance = instances?.[cursor] - if (instance) { + if (!instance) { + console.warn(`[gea] Missing component instance for ${tag} at cursor ${cursor}`) + } else { instanceCursors.set(tag, cursor + 1) slots.push({ childPath: [...path, idx], instanceVar: instance.instanceVar }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-plugin-gea/src/generate-clone.ts` around lines 597 - 619, collectComponentSlotPatches currently relies on implicit traversal order and a cursor (instanceCursors) to pick the matching ChildComponent from componentInstances which is fragile; change the matching to use the explicit dfsIndex on each ChildComponent (imported type ChildComponent) to find the instance whose dfsIndex equals the DFS position for the current child (or otherwise use a Map from dfsIndex→instance when building componentInstances), and add a defensive bounds check before using an instance (if no match, skip pushing slot or log/warn and continue) so you never read past the instances array; keep the existing instanceCursors logic only if you still need counts, but prefer explicit dfsIndex matching in collectComponentSlotPatches and ensure you reference getDirectChildElements, collectComponentTags, componentInstances, instanceCursors, and dfsIndex when implementing the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/vite-plugin-gea/src/generate-clone.ts`:
- Around line 46-53: RESTRICTED_CLONE_PARENTS currently omits several
phrasing/content elements that should forbid inserting arbitrary placeholders;
update the Set named RESTRICTED_CLONE_PARENTS to also include missing
inline/phrasing tags such as button, label, code, kbd, abbr (and any other
relevant inline controls like output, samp, var, sub, sup) so the generator
avoids inserting clones into those invalid parents; locate the constant
RESTRICTED_CLONE_PARENTS in generate-clone.ts and add the additional tag names
to the Set.
- Around line 597-619: collectComponentSlotPatches currently relies on implicit
traversal order and a cursor (instanceCursors) to pick the matching
ChildComponent from componentInstances which is fragile; change the matching to
use the explicit dfsIndex on each ChildComponent (imported type ChildComponent)
to find the instance whose dfsIndex equals the DFS position for the current
child (or otherwise use a Map from dfsIndex→instance when building
componentInstances), and add a defensive bounds check before using an instance
(if no match, skip pushing slot or log/warn and continue) so you never read past
the instances array; keep the existing instanceCursors logic only if you still
need counts, but prefer explicit dfsIndex matching in
collectComponentSlotPatches and ensure you reference getDirectChildElements,
collectComponentTags, componentInstances, instanceCursors, and dfsIndex when
implementing the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 519078e1-7f12-4689-8ee3-4c98197b404a
📒 Files selected for processing (2)
packages/vite-plugin-gea/src/generate-clone.tspackages/vite-plugin-gea/tests/optimization-tests.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vite-plugin-gea/tests/optimization-tests.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vite-plugin-gea/src/generate-clone.ts`:
- Around line 46-53: The current RESTRICTED_CLONE_PARENTS list still allows many
parents that will reparse/forbid same-tag children (e.g., li, dt, dd, option,
textarea), breaking placeholder navigation; update generate-clone.ts to stop
relying on this blacklist and instead use a whitelist of known-safe parent tags
(e.g., div, span, section, article, header, footer, main, nav, figure) or change
the insertion technique to use a non-parsed container (e.g., serialize the
placeholder inside a <template> or use document.createElement('template') and
insert the placeholder there) so the placeholder isn't reparsed before childPath
runs; replace or supplement RESTRICTED_CLONE_PARENTS (and the same logic
referenced around lines 149-151) with the whitelist/templating approach and
update the check that decides whether to emit `<${parentTag}...>` accordingly.
- Around line 597-619: collectComponentSlotPatches is failing to advance per-tag
cursors for component instances nested inside component children, causing desync
with componentInstances; update the function to mirror the full DFS traversal so
nested component descendants are "consumed" even when they don't produce a slot.
Concretely, add a small helper or a boolean parameter (e.g., consumeOnly) to
collectComponentSlotPatches so that when you encounter a component element (tag
&& isComponentTag) you both (1) handle the direct-slot logic (push into slots
and increment instanceCursors for that tag) and (2) recursively traverse that
component element's children in consumeOnly mode to walk the same DFS and
increment instanceCursors for any nested component tags without adding slots;
ensure you reference collectComponentSlotPatches, componentInstances,
instanceCursors, getDirectChildElements and isComponentTag when making the
change.
In `@packages/vite-plugin-gea/tests/optimization-tests.test.ts`:
- Around line 860-862: The variable `app` is typed only as `{ dispose: () =>
void } | undefined` but is later used with `app.render(root)`; update the `app`
declaration to include the `render` method so TypeScript knows `app` has both
`render(el: HTMLElement): void` and `dispose(): void` (i.e., change the type of
`app` to a union that includes an object with `render: (el: HTMLElement) =>
void` and `dispose: () => void` or the equivalent interface), and apply the same
fix where `app.render` is invoked (around the code referencing `app.render`).
- Line 14: The test imports loadRuntimeModules and compileJsxComponent at the
top but also redeclares local implementations later, causing duplicate symbol
errors; remove the local duplicate definitions for loadRuntimeModules and
compileJsxComponent (the blocks that reimplement those functions) and use the
imported versions, or alternatively delete the import and keep the local
implementations—preferably delete the local re-declarations so the top-level
import of loadRuntimeModules and compileJsxComponent is the single definition
used throughout the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9606cb7b-a27f-495a-8bee-4a650f067a0c
📒 Files selected for processing (2)
packages/vite-plugin-gea/src/generate-clone.tspackages/vite-plugin-gea/tests/optimization-tests.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/vite-plugin-gea/src/generate-clone.ts (1)
593-619:⚠️ Potential issue | 🟠 MajorThe DFS-desync fix still looks incomplete for expression-wrapped descendants.
Line 601 still consumes slots from
getDirectChildElements(), butcollectComponentTags()inpackages/vite-plugin-gea/src/transform-jsx.tsalso descends through JSX expression containers and callback returns. IfgetDirectChildElements()does not mirror that wider walk, cases like<Wrapper>{<Item />}</Wrapper><Item />will still assign the sibling slot to the wronginstanceVar. Please either reuse the same traversal rules here or add a regression test for expression/callback-wrapped nested components.Run this read-only check to compare the two traversals. Expected result: if
getDirectChildElements()does not descend intoJSXExpressionContainer/ callback-body JSX the same waycollectComponentTags()does, the cursor-desync path is still reachable.#!/bin/bash set -euo pipefail printf '\n=== collectComponentTags traversal ===\n' rg -n -C3 'export function collectComponentTags|visitExpr|JSXExpressionContainer|LogicalExpression|ConditionalExpression|CallExpression' packages/vite-plugin-gea/src/transform-jsx.ts printf '\n=== getDirectChildElements traversal ===\n' rg -n -C3 'export function getDirectChildElements|function getDirectChildElements|JSXExpressionContainer|LogicalExpression|ConditionalExpression|CallExpression' packages/vite-plugin-gea/src/utils.ts printf '\n=== consume-only slot traversal ===\n' rg -n -C3 'function collectComponentSlotPatches|getDirectChildElements' packages/vite-plugin-gea/src/generate-clone.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/vite-plugin-gea/src/generate-clone.ts` around lines 593 - 619, collectComponentSlotPatches is still only using getDirectChildElements (which doesn't descend into JSXExpressionContainer/callback bodies) while collectComponentTags in transform-jsx.ts does a wider traversal, causing cursor desync for expression-wrapped descendants; fix by making collectComponentSlotPatches use the same traversal rules as collectComponentTags — either call/reuse the shared traversal helper used by collectComponentTags (e.g., visitExpr/collect logic) or extend getDirectChildElements to descend into JSXExpressionContainer, LogicalExpression, ConditionalExpression and CallExpression the same way, update the call sites in collectComponentSlotPatches to use that expanded walk (keep the existing consumeOnly handling), and add a regression test for nested expression/callback-wrapped components (e.g., <Wrapper>{<Item/>}</Wrapper><Item/>) to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/vite-plugin-gea/src/generate-clone.ts`:
- Around line 593-619: collectComponentSlotPatches is still only using
getDirectChildElements (which doesn't descend into
JSXExpressionContainer/callback bodies) while collectComponentTags in
transform-jsx.ts does a wider traversal, causing cursor desync for
expression-wrapped descendants; fix by making collectComponentSlotPatches use
the same traversal rules as collectComponentTags — either call/reuse the shared
traversal helper used by collectComponentTags (e.g., visitExpr/collect logic) or
extend getDirectChildElements to descend into JSXExpressionContainer,
LogicalExpression, ConditionalExpression and CallExpression the same way, update
the call sites in collectComponentSlotPatches to use that expanded walk (keep
the existing consumeOnly handling), and add a regression test for nested
expression/callback-wrapped components (e.g.,
<Wrapper>{<Item/>}</Wrapper><Item/>) to prevent regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6dc5c67b-f909-4fd2-8670-da8564cce708
📒 Files selected for processing (3)
.changeset/fix-clone-parent-whitelist-and-slot-cursor.mdpackages/vite-plugin-gea/src/generate-clone.tspackages/vite-plugin-gea/tests/optimization-tests.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vite-plugin-gea/tests/optimization-tests.test.ts
f24970d to
a5fe99d
Compare
…d instances Remove the componentInstances.size === 0 guard so components with child component instances can use the clone optimization path. The compiler emits a data-gea-child-slot placeholder in the static HTML skeleton and generates parentNode.replaceChild(this.child.el, placeholder) calls in __cloneTemplate to wire up child elements at mount time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
06db1a9 to
27182d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/vite-plugin-gea/src/codegen/gen-clone.ts`:
- Around line 500-507: The current clone-slot collection in gen-clone.ts records
every component instance (using componentInstances, instanceCursors and pushing
into slots with instance.instanceVar) but gen-children.ts only initializes
non-lazy children (it returns early when child.lazy), causing generated code to
dereference this.<instanceVar>.el for lazy children; update the collection logic
to detect lazy slot children (use the same child.lazy condition from
gen-children.ts) and either skip adding those instances to slots (bail out of
the clone optimization for lazy slot children) or throw a clear error during
slot collection to fail fast instead of producing an unreplaced placeholder.
Ensure references to instanceVar are only produced for children guaranteed to be
constructed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 331ce720-a189-48cf-b14e-d44ce1989fb9
📒 Files selected for processing (4)
.changeset/partial-clone-optimization.mdpackages/vite-plugin-gea/src/codegen/gen-clone.tspackages/vite-plugin-gea/src/codegen/generator.tspackages/vite-plugin-gea/tests/optimization-tests.test.ts
💤 Files with no reviewable changes (1)
- packages/vite-plugin-gea/src/codegen/generator.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/partial-clone-optimization.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vite-plugin-gea/tests/optimization-tests.test.ts
| const instances = componentInstances.get(tag) | ||
| const cursor = instanceCursors.get(tag) ?? 0 | ||
| const instance = instances?.[cursor] | ||
| if (instance) { | ||
| instanceCursors.set(tag, cursor + 1) | ||
| if (!consumeOnly) { | ||
| slots.push({ childPath: [...path, idx], instanceVar: instance.instanceVar }) | ||
| } |
There was a problem hiding this comment.
Skip clone-slot patching for lazy child components.
This records every component instance as replaceable, but packages/vite-plugin-gea/src/codegen/gen-children.ts:138-167 only initializes non-lazy children in the constructor (if (child.lazy) return). The generated call at Lines 645-650 will therefore dereference this.<instanceVar>.el before the instance exists for lazy children and fail during mount. Please bail out of clone optimization for lazy slot children, or make slot collection fail hard instead of silently producing an unreplaced placeholder.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/vite-plugin-gea/src/codegen/gen-clone.ts` around lines 500 - 507,
The current clone-slot collection in gen-clone.ts records every component
instance (using componentInstances, instanceCursors and pushing into slots with
instance.instanceVar) but gen-children.ts only initializes non-lazy children (it
returns early when child.lazy), causing generated code to dereference
this.<instanceVar>.el for lazy children; update the collection logic to detect
lazy slot children (use the same child.lazy condition from gen-children.ts) and
either skip adding those instances to slots (bail out of the clone optimization
for lazy slot children) or throw a clear error during slot collection to fail
fast instead of producing an unreplaced placeholder. Ensure references to
instanceVar are only produced for children guaranteed to be constructed.
Lazy children are not initialized at mount time, so dereferencing this.<instanceVar>.el for them produces a runtime error. Skip lazy instances when collecting component slot patches to ensure only guaranteed-constructed children get replaceChild calls. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Measures 1000 mounts of a parent component containing a child instance. Uses the clone path (with child slot placeholder) introduced in PR dashersw#43. Reports per-mount time and dispose time via the existing report() mechanism. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
6234430 to
a0d7215
Compare
…nent definitions Delete the local re-declarations of loadRuntimeModules and compileJsxComponent in optimization-tests.test.ts and use the imports from ./helpers/compile. Also add symbolsModule to loadRuntimeModules in helpers/compile.ts so all callers get consistent module bindings (component, store, symbols). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
componentInstances.size === 0guard frompreCloneEligibleso components with child component instances are now eligible for clone template optimizationdata-gea-child-slotplaceholder divs for each child component position in the template__cloneTemplatereplaces each placeholder viaparentNode.replaceChildusing the child instance's.elpropertycollectComponentSlotPatchesto map child component instance vars to their slot positionsCloneComponentSlotPatchtype to carrychildPath+instanceVarthrough code generationHow it works
Before this change, any component with child component instances fell back to the slower
innerHTMLpath. Now the compiler emits a partial clone template:And in
__cloneTemplate:Test plan
optimization-tests.test.tsverify__tpl,__cloneTemplate, child instance vars,replaceChild, anddata-gea-child-slotin static HTMLmainare unrelated to this change (chat CRLF issue, todo counter)npm test -w @geajs/vite-pluginpasses all previously-passing testsSummary by CodeRabbit
Release Notes
New Features
Tests