Circular serialize tag var issue #2917
Merged
Conversation
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
🦋 Changeset detectedLatest commit: 16f3867 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds test fixture directories return-serialize-circular and circular-tag-var-function-serialize with templates, child components calling setters, and test.ts exporting steps that include wait(1). Refactors HTML serializer: removes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (160)
📒 Files selected for processing (9)
🚧 Files skipped from review as they are similar to previous changes (5)
🧰 Additional context used🧬 Code graph analysis (2)packages/runtime-tags/src/html/serializer.ts (1)
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/test.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (15)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/runtime-tags/src/html/serializer.ts (2)
592-607: <!-- [scratchpad_start]
[task review_serializer_1/1 verify_concern_1/3 complete]
Analyzed full assignment and flush flow[observations]
- Reference class (line 306-320): has init, assigns (null or string array), parent, accessor, flush, and bufPos fields
- state.flush incremented after each stringify (line 351)
- state.assigned cleared after writeAssigned (line 490): state.assigned = new Set()
- writeAssigned called at lines: 429, 444, 546 - multiple times per stringify
- ref.flush tracks which flush cycle the ref was created in (line 1596)
- In writeReferenceOr (lines 592-607):
- Line 595-597: if ref.assigns truthy, just calls addAssignment, NO state.assigned.add()
- Line 598-602: if isCircular, calls state.assigned.add(ref) BEFORE addAssignment
- Inconsistency confirmed: isCircular path adds ref back to assigned set, but ref.assigns path doesn't
[analysis]
Review comment concern is VALID:
- writeAssigned (line 490) clears state.assigned set with new Set()
- Can be called multiple times within one stringify
- If ref.assigns already truthy (has prior assignments), appending new assignments (line 596) without re-adding ref to state.assigned means writeAssigned won't write them
- isCircular branch (line 600) correctly adds ref back to state.assigned, but ref.assigns branch should too
- This is a logic bug: after state.assigned is drained, if same ref needs more assignments, they won't be flushed
[analysis_conclusion]
The review comment accurately identifies a missing state.assigned.add(ref) call after line 596. The suggested fix matches what the isCircular branch does on line 600.[done]
Review comment is correct and identifies a legitimate bug pattern.
</scratchpad_end] -->Ensure re-queued assignments always flush
When
refalready exists andref.assignsis truthy, you append another assignment but don't re-addreftostate.assigned. The inconsistency with theisCircularbranch (which does callstate.assigned.add(ref)beforeaddAssignment) indicates a bug. IfwriteAssignedpreviously drained the set during the same stringify cycle, new assignments won't be emitted.Consider adding the
refback to the assigned set when appending more assignments:if (ref) { if (parent) { if (ref.assigns) { - addAssignment(ref, ensureId(state, parent) + toAccess(accessor)); + addAssignment(ref, ensureId(state, parent) + toAccess(accessor)); + state.assigned.add(ref); return false; } else if (isCircular(parent, ref)) { ensureId(state, ref); state.assigned.add(ref); addAssignment(ref, ensureId(state, parent) + toAccess(accessor)); return false; } }
634-668: Add handler for root-level registered functions with scopesThe current implementation doesn't properly serialize registered functions at the root level (when
parentis null) if they have a scope. This occurs whenwritePropis called withparent=null(e.g., at lines 423, 500, 528, 650 of serializer.ts), and the value is a registered function with an associated scope. The else-branch currently only pushesaccessto the buffer, which omits the scope invocation and breaks the closure.Add an
else if (scope)branch to handle this case by inlining the scope reference:function writeRegistered( state: State, val: WeakKey, parent: Reference | null, accessor: string, { access, scope }: Registered, ) { if (scope && parent) { const fnRef = new Reference( parent, accessor, state.flush, state.buf.length, ); state.refs.set(val, fnRef); let scopeRef = state.refs.get(scope); let shouldWrite = true; if (scopeRef) { shouldWrite = false; } else { shouldWrite = writeProp(state, scope, null, ""); scopeRef = state.refs.get(scope); } if (scopeRef) { fnRef.init = access + "(" + ensureId(state, scopeRef) + ")"; } else { fnRef.init = access + "()"; } state.assigned.add(fnRef); addAssignment(fnRef, ensureId(state, parent) + toAccess(accessor)); return shouldWrite; + } else if (scope) { + let scopeRef = state.refs.get(scope); + if (!scopeRef) { + writeProp(state, scope, null, ""); + scopeRef = state.refs.get(scope); + } + state.buf.push( + scopeRef ? access + "(" + ensureId(state, scopeRef) + ")" : access + "()", + ); } else { state.buf.push(access); } return true; }Add a test case for this scenario at packages/runtime-tags/src/tests/serializer.test.ts to verify root-level scoped function serialization.
🧹 Nitpick comments (2)
packages/runtime-tags/src/html/writer.ts (1)
137-144: Leverage registry to avoid allocating a new tag-var object when already registeredYou can fulfill the TODO by checking the registry for an existing value for this
registryIdand reusing it, avoiding an extra{}allocation and preserving identity when possible. One approach is to add/get-by-id support in the serializer and prefer that value.Example shape:
import { register as serializerRegister, + getById as serializerGetById, // hypothetical helper } from "./serializer"; export function _var( parentScopeId: number, scopeOffsetAccessor: Accessor, childScopeId: number, registryId: string, ) { _scope_with_id(parentScopeId)[scopeOffsetAccessor] = _scope_id(); - // TODO: if the return value is already registered, use that. - _scope_with_id(childScopeId)[AccessorProp.TagVariable] = _resume( - {}, - registryId, - parentScopeId, - ); + const existing = serializerGetById?.(registryId); + _scope_with_id(childScopeId)[AccessorProp.TagVariable] = existing ?? + _resume({}, registryId, parentScopeId); }packages/runtime-tags/src/__tests__/serializer.test.ts (1)
4-4: Tests updated to remove getter API—please add a scoped-root caseConsider adding a test that serializes a scoped registered function as the root value (no parent), e.g.
register("fn", builder(scope), scope); assertStringify(builder(scope), '_._.fn(_.a={...})', {_: { fn: builder }})to validate the no-parent path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (13)
.changeset/all-bobcats-return.mdis excluded by none and included by nonepackages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/.name-cache.jsonis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/dom.expected/tags/child.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/html.expected/tags/child.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/ssr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**
📒 Files selected for processing (6)
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/tags/child.marko(1 hunks)packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/template.marko(1 hunks)packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/test.ts(1 hunks)packages/runtime-tags/src/__tests__/serializer.test.ts(1 hunks)packages/runtime-tags/src/html/serializer.ts(3 hunks)packages/runtime-tags/src/html/writer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/test.ts (1)
packages/runtime-tags/src/__tests__/utils/resolve.ts (1)
wait(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test: node@22
- GitHub Check: test: node@20
- GitHub Check: test: node@18
🔇 Additional comments (3)
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/tags/child.marko (1)
1-1: LGTM for the fixtureSimple trigger is fine for exercising the serialization path.
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/test.ts (1)
1-3: Fixture scaffolding looks good
steps = [{}, wait(1)]matches the pattern used elsewhere.packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/template.marko (1)
1-5: LGTM for reproducing the circular tag-var/function caseCompact and focused; good for regression coverage.
DylanPiercey
force-pushed
the
circular-serialize-tag-var-issue
branch
from
e4d7e56 to
f3efc57
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/test.ts (1)
3-3: LGTM!The test steps configuration is correctly structured for the fixture. The empty object as the first step and
wait(1)as the second step appear intentional for testing the circular serialization scenario.Consider adding a brief comment explaining the purpose of each step for improved clarity:
+// First step: initial state, second step: wait for async serialization export const steps = [{}, wait(1)];packages/runtime-tags/src/html/writer.ts (2)
138-144: Reusing existing registration in _varYou noted the TODO. You can avoid re-registering by checking
getRegisteredfor the existing value and using its accessor when available. I can draft a small helper if you want to pursue this now.
16-21: Consolidate scoped and unscoped registrations through the instance methodFor consistency, route all registrations through
$chunk.boundary.state.serializer.register()with an optional scope parameter.Apply this diff:
@@ -import { - register as serializerRegister, - Serializer, - setDebugInfo, -} from "./serializer"; +import { Serializer, setDebugInfo } from "./serializer"; @@ export function _resume<T extends WeakKey>( - return scopeId === undefined - ? serializerRegister(id, val) - : $chunk.boundary.state.serializer.register( - id, - val, - _scope_with_id(scopeId), - ); + const scope = scopeId === undefined ? undefined : _scope_with_id(scopeId); + return $chunk.boundary.state.serializer.register(id, val, scope);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (146)
.changeset/all-bobcats-return.mdis excluded by none and included by nonepackages/runtime-tags/src/__tests__/fixtures/async-deep-recursive/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-deep-recursive/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-before-and-after-isolated-boundaries/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-before-and-after-isolated-boundaries/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reorder-nested-batched-resolve/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reorder-nested-batched-resolve/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/at-tags-dynamic/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/at-tags-dynamic/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-attrs/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-attrs/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-alias/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-alias/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-same-source-alias-within-pattern/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-same-source-alias-within-pattern/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-same-source-alias/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-same-source-alias/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-renderBody/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-renderBody/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-nested-for/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-nested-for/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/bind-to-input/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/bind-to-input/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/body-content/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/body-content/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-reject-async/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-success-async/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-success-async/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-success-sync/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-success-sync/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-throw-sync/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-throw-sync/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/.name-cache.jsonis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/dom.expected/tags/child.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/html.expected/tags/child.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/ssr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-for-shallow/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-for-shallow/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-if-deep/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-if-deep/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-if-shallow/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-if-shallow/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-for-deep/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-for-deep/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-for-shallow/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-for-shallow/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-if-deep/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-if-deep/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-if-shallow/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-if-shallow/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/controllable-checked-spread/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/controllable-checked-spread/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/controllable-select-dynamic-spread/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/controllable-select-dynamic-spread/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-args/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-args/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-attributes/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-attributes/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-single-arg/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-single-arg/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-assignment/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-assignment/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-intersection/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-intersection/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-args-tag-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-args-tag-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-input-intersection/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-input-intersection/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-var-assignment/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-var-assignment/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/function-references-normalize/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/function-references-normalize/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/function-references-optional-member-normalize/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/function-references-optional-member-normalize/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-attr-tag/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-attr-tag/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-from-dynamic/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-from-dynamic/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-many/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-many/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-same-scope/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-same-scope/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var-from-dynamic/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var-from-dynamic/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var-many/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var-many/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-native-tag-var-same-scope/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-native-tag-var-same-scope/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-native-tag-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-native-tag-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/html-comment-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/html-comment-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/let-tag-controllable-child/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/let-tag-controllable-child/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/multiple-bound-values/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/multiple-bound-values/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-effect-child/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-effect-child/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/native-tag-spread-content/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/native-tag-spread-content/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/nested-for-if-stateful/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/nested-for-if-stateful/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/no-render-content-conditional/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/no-render-content-conditional/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/no-render-content-subtree/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/no-render-content-subtree/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholder-single/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholder-single/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholder-skipped/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholder-skipped/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholders-nested/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholders-nested/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/returns-within-define-tag/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/returns-within-define-tag/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/tag-var-with-serialize-reason/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/tag-var-with-serialize-reason/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/toggle-nested-2/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/toggle-nested-2/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/toggle-nested-3/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/toggle-nested-3/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/try-effects-catch/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/try-effects-catch/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/try-single-throw-sync/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/try-single-throw-sync/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/translator-interop/src/__tests__/fixtures/interop-basic-tags-to-class/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/translator-interop/src/__tests__/fixtures/interop-basic-tags-to-class/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/translator-interop/src/__tests__/fixtures/interop-event-handler-render-body-tags-to-class/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/translator-interop/src/__tests__/fixtures/interop-event-handler-render-body-tags-to-class/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**
📒 Files selected for processing (5)
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/tags/child.marko(1 hunks)packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/template.marko(1 hunks)packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/test.ts(1 hunks)packages/runtime-tags/src/html/serializer.ts(10 hunks)packages/runtime-tags/src/html/writer.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/runtime-tags/src/tests/fixtures/circular-tag-var-function-serialize/template.marko
- packages/runtime-tags/src/tests/fixtures/circular-tag-var-function-serialize/tags/child.marko
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/test.ts (1)
packages/runtime-tags/src/__tests__/utils/resolve.ts (1)
wait(4-8)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test: node@18
- GitHub Check: test: node@22
- GitHub Check: test: node@20
🔇 Additional comments (6)
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/test.ts (1)
1-1: LGTM!The import correctly references the
waitutility from the test utilities module.packages/runtime-tags/src/html/serializer.ts (5)
612-623: Circular/alias handling looks correctQueuing assignments on circular access and deferring them via
state.assignsis a clean replacement for init/getter flows. Good use ofensureIdandisCircular.
299-307: State shape changes improve clarityTracking
registeredseparately fromassignsand addingregisterInstanceIdmakes ordering explicit and easier to reason about.Also applies to: 311-313
392-399: API consolidation for registrationAdding
Serializer.registerand threadinginstanceIdthroughregisteris a solid step toward instance-scoped behavior.Also applies to: 402-415
1731-1733: Deterministic ordering of scoped refsSorting by
instanceIdensures evaluation order matches registration order within a serializer instance. Simple and effective.
492-507: Verify scope type validation in scoped registrationThe review comment correctly identifies a defensive programming gap. The
register()function acceptsscope?: unknownwith no type validation, and the fallback inwriteAssigned()at line 502 usesref.assigns whenscopeRefis missing—which would be semantically wrong if a primitive scope were passed.All current call sites are safe (passing objects/functions or undefined), but the suggested fixes are appropriate defensive measures:
- Add a type guard to require scope be an object or function when provided
- Replace the fallback with a debug-time check and use
"void 0"(undefined) in productionThese prevent accidental misuse without breaking existing behavior.
DylanPiercey
force-pushed
the
circular-serialize-tag-var-issue
branch
2 times, most recently
from
1c61ecf to
8b171f2
Compare
DylanPiercey
force-pushed
the
circular-serialize-tag-var-issue
branch
2 times, most recently
from
cf06cbc to
1b37d6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/runtime-tags/src/html/writer.ts (1)
138-143: TODO: Optimize redundant registrations.The comment suggests checking whether the return value is already registered before creating a new registration. This could avoid redundant work in the serializer.
Consider whether this optimization should be implemented now or tracked separately. Do you want me to help verify if the current approach causes issues with duplicate registrations, or generate a script to search for similar patterns in the codebase?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (158)
.changeset/all-bobcats-return.mdis excluded by none and included by nonepackages/runtime-tags/src/__tests__/fixtures/async-deep-recursive/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-deep-recursive/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-before-and-after-isolated-boundaries/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-before-and-after-isolated-boundaries/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reorder-nested-batched-resolve/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/async-reorder-nested-batched-resolve/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/at-tags-dynamic/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/at-tags-dynamic/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-attrs/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-attrs/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-alias/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-alias/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-same-source-alias-within-pattern/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-same-source-alias-within-pattern/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-same-source-alias/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input-same-source-alias/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-input/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-renderBody/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-component-renderBody/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-nested-for/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/basic-nested-for/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/bind-to-input/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/bind-to-input/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/body-content/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/body-content/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-reject-async/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-success-async/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-success-async/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-success-sync/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-success-sync/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-throw-sync/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/catch-single-throw-sync/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/.name-cache.jsonis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/dom.expected/tags/child.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/html.expected/tags/child.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/ssr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-for-shallow/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-for-shallow/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-if-deep/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-if-deep/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-if-shallow/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-n-child-if-shallow/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-for-deep/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-for-deep/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-for-shallow/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-for-shallow/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-if-deep/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-if-deep/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-if-shallow/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/cleanup-single-child-if-shallow/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/controllable-checked-spread/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/controllable-checked-spread/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/controllable-select-dynamic-spread/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/controllable-select-dynamic-spread/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-args/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-args/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-attributes/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-attributes/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-single-arg/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-parameters-from-single-arg/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-assignment/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-assignment/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-intersection/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var-intersection/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/custom-tag-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-args-tag-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-args-tag-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-input-intersection/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-input-intersection/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-var-assignment/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/dynamic-tag-var-assignment/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/function-references-normalize/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/function-references-normalize/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/function-references-optional-member-normalize/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/function-references-optional-member-normalize/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-attr-tag/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-attr-tag/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-from-dynamic/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-from-dynamic/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-many/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-many/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-same-scope/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var-same-scope/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-custom-tag-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var-from-dynamic/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var-from-dynamic/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var-many/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var-many/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-dynamic-tag-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-native-tag-var-same-scope/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-native-tag-var-same-scope/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-native-tag-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/hoist-native-tag-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/html-comment-var/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/html-comment-var/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/let-tag-controllable-child/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/let-tag-controllable-child/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/multiple-bound-values/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/multiple-bound-values/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-effect-child/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/native-tag-ref-effect-child/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/native-tag-spread-content/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/native-tag-spread-content/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/nested-for-if-stateful/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/nested-for-if-stateful/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/no-render-content-conditional/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/no-render-content-conditional/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/no-render-content-subtree/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/no-render-content-subtree/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholder-single/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholder-single/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholder-skipped/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholder-skipped/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholders-nested/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/placeholders-nested/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/.name-cache.jsonis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/csr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/csr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/tags/setter.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.hydrate.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/html.expected/tags/setter.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/html.expected/template.jsis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/resume-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/ssr-sanitized.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/returns-within-define-tag/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/returns-within-define-tag/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/tag-var-with-serialize-reason/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/tag-var-with-serialize-reason/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/toggle-nested-2/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/toggle-nested-2/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/toggle-nested-3/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/toggle-nested-3/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/try-effects-catch/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/try-effects-catch/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/try-single-throw-sync/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/runtime-tags/src/__tests__/fixtures/try-single-throw-sync/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/translator-interop/src/__tests__/fixtures/interop-basic-tags-to-class/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/translator-interop/src/__tests__/fixtures/interop-basic-tags-to-class/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/translator-interop/src/__tests__/fixtures/interop-event-handler-render-body-tags-to-class/__snapshots__/resume.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**packages/translator-interop/src/__tests__/fixtures/interop-event-handler-render-body-tags-to-class/__snapshots__/ssr.expected.mdis excluded by!**/__snapshots__/**and included by**/src/**
📒 Files selected for processing (9)
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/tags/child.marko(1 hunks)packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/template.marko(1 hunks)packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/test.ts(1 hunks)packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/tags/setter.marko(1 hunks)packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/template.marko(1 hunks)packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/test.ts(1 hunks)packages/runtime-tags/src/__tests__/serializer.test.ts(1 hunks)packages/runtime-tags/src/html/serializer.ts(11 hunks)packages/runtime-tags/src/html/writer.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/runtime-tags/src/tests/fixtures/circular-tag-var-function-serialize/tags/child.marko
- packages/runtime-tags/src/tests/serializer.test.ts
- packages/runtime-tags/src/tests/fixtures/circular-tag-var-function-serialize/test.ts
🧰 Additional context used
🧬 Code graph analysis (3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/test.ts (1)
packages/runtime-tags/src/__tests__/utils/resolve.ts (1)
wait(4-8)
packages/runtime-tags/src/html/writer.ts (1)
packages/runtime-tags/src/html.ts (1)
_scope_with_id(63-63)
packages/runtime-tags/src/html/serializer.ts (1)
packages/runtime-tags/src/html/writer.ts (1)
Boundary(942-990)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test: node@22
- GitHub Check: test: node@18
- GitHub Check: test: node@20
🔇 Additional comments (15)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/template.marko (1)
1-4: LGTM! Test fixture for circular serialization.This test fixture appropriately exercises circular serialization by binding a setter to a reactive variable and immediately invoking it during rendering.
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/tags/setter.marko (1)
1-2: LGTM! Test fixture for return serialization with setter.The fixture correctly defines a setter pattern that will be returned and exercises the circular serialization logic.
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/test.ts (1)
1-3: LGTM! Test orchestration for async rendering.The test steps correctly use the wait utility to coordinate async rendering stages.
packages/runtime-tags/src/__tests__/fixtures/circular-tag-var-function-serialize/template.marko (1)
1-5: LGTM! Test fixture for circular tag-variable-function serialization.This fixture appropriately tests circular references through tag variables and function callbacks within conditional scopes.
packages/runtime-tags/src/html/writer.ts (1)
151-157: LGTM! Properly routes scoped registrations through per-instance serializer.The _resume function correctly delegates to the per-instance
serializer.registerwhen a scopeId is provided, enabling instance-scoped registration tracking introduced in the serializer refactoring.packages/runtime-tags/src/html/serializer.ts (10)
10-15: LGTM! Replaced getter flag with instanceId for per-instance tracking.The change from
getter: booleantoinstanceId: numberenables deterministic ordering of registered references, which is essential for the new per-instance registration mechanism.
295-307: LGTM! State additions support per-instance registration tracking.The additions of
registerInstanceIdcounter andregisteredcollection enable the new per-instance registration mechanism. The rename frommutationstomutatedimproves consistency withassigned.
392-399: LGTM! Serializer instance method enables scoped registration with instanceId.The new
registermethod correctly computesinstanceIdonly for scoped registrations (when scope is provided), ensuring deterministic ordering of per-instance registered references.
402-415: LGTM! Register function updated to accept optional instanceId.The function signature extension is backward compatible, defaulting
instanceIdto 0 when not provided, which maintains existing behavior for non-scoped registrations.
440-453: LGTM! writeRoot correctly handles registered references.The condition properly includes
state.registered.lengthalongsideassignedandmutatedchecks, ensuring registered references are processed during root serialization.
480-511: LGTM! writeAssigned properly handles three phases of reference processing.The rewrite cleanly separates assigned, registered, and mutated reference processing. The sorting by
instanceId(line 494) ensures deterministic output for per-instance registrations.
614-628: LGTM! writeReferenceOr correctly handles existing assignments.The new check at line 615 prevents duplicate serialization when a reference already has pending assignments, correctly adding to the assignment queue and returning false to skip re-serialization.
649-677: LGTM! writeRegistered properly defers scoped registrations.The rewrite correctly handles two paths:
- With parent and scope (lines 656-671): Defers serialization by queuing the reference in
state.registeredand adding an assignment. Only writes the scope if not yet seen.- Without parent/scope (lines 672-673): Immediately emits the registered access path.
The deferred pattern aligns with the overall serialization architecture.
1731-1733: LGTM! Comparator ensures deterministic ordering of registered references.The
compareRegisteredReferencesfunction correctly sorts byinstanceId, ensuring consistent serialization order for per-instance registrations.
492-507: Fallback behavior is intentional but verify it matches intended design.The fallback at line 502 is by design: when
scopeRefis undefined (the scope object wasn't serialized), the code usesref.assigns![0]as the fallback scope argument. This is safe becauseref.assignsis guaranteed to contain at least one element—items added tostate.registeredat line 665 immediately triggeraddAssignment()which initializes the array with[assign](line 1650).The fallback uses the first assignment identifier (parent reference + accessor) when the actual scope reference isn't available. While this appears intentional and correct, confirm this matches your intended behavior for handling incompletely serialized scope references.
DylanPiercey
force-pushed
the
circular-serialize-tag-var-issue
branch
from
1b37d6f to
16f3867
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2917 +/- ##
==========================================
- Coverage 88.56% 88.56% -0.01%
==========================================
Files 372 372
Lines 46332 46299 -33
Branches 3793 3783 -10
==========================================
- Hits 41036 41006 -30
+ Misses 5262 5259 -3
Partials 34 34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.