Skip to content

Conversation

@rturnq
Copy link
Contributor

@rturnq rturnq commented Oct 24, 2025

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Oct 24, 2025

🦋 Changeset detected

Latest commit: de6e1d4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@marko/runtime-tags Patch

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

@coderabbitai
Copy link

coderabbitai bot commented Oct 24, 2025

Walkthrough

Adds a changeset and a patch to the runtime-tags HTML serializer: in writeRegistered, when scopeRef is falsy the code now assigns fnRef.init ||= access to initialize the registered function's initial accessor. Adds many new test fixtures and snapshots covering circular-object and circular-ordering scenarios (CSR, SSR, HTML, DOM, hydrate/resume snapshots, tag/template fixtures, and test step files).

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Description Check ❓ Inconclusive No pull request description was provided by the author. While the instruction emphasizes this is a lenient check, an empty description provides no information about the changeset and does not describe any part of the changes, even at a high level. Without any descriptive content, it is impossible to conclusively determine whether the description meets the criterion of being related to the changeset in some way. Consider adding a brief description of the changes to help reviewers understand the context and motivation for this fix. Even a simple one or two sentence explanation of what circular reference issue was addressed and why this fix resolves it would improve the PR's documentation.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "[Serializer] Ensure circular references to registered functions all all assigned" is directly related to the main functional change in this pull request. The changeset file indicates "a serializer fix to ensure circular references to registered functions are all assigned," and the key code change in packages/runtime-tags/src/html/serializer.ts implements exactly this by initializing fnRef.init when there is no scopeRef. The title clearly summarizes the main change and is specific enough for a reviewer to understand the primary intent. However, there is a grammatical error in the title ("all all" instead of "are all"), which slightly diminishes its polish but does not affect comprehension.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch serialize-circular

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 681661a and de6e1d4.

⛔ Files ignored due to path filters (12)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/.name-cache.json is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/csr-sanitized.expected.md is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/csr.expected.md is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/dom.expected/tags/child.js is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/dom.expected/tags/let-batch.js is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/dom.expected/template.hydrate.js is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/dom.expected/template.js is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/html.expected/tags/child.js is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/html.expected/tags/let-batch.js is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/html.expected/template.js is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/ssr-sanitized.expected.md is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/__snapshots__/ssr.expected.md is excluded by !**/__snapshots__/** and included by **/src/**
📒 Files selected for processing (4)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/tags/child.marko (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/tags/let-batch.marko (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/template.marko (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/test.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • packages/runtime-tags/src/tests/fixtures/return-serialize-circular-ordering/tags/let-batch.marko
  • packages/runtime-tags/src/tests/fixtures/return-serialize-circular-ordering/tags/child.marko
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/test.ts (1)
packages/runtime-tags/src/__tests__/utils/resolve.ts (1)
  • wait (4-8)
🔇 Additional comments (3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/test.ts (1)

1-3: LGTM!

The test fixture is straightforward and correctly exports test steps for exercising circular serialization ordering scenarios.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-ordering/template.marko (2)

3-5: Verify the unconditional if=true block is intentional.

The template uses an if=true block that will always be true. While this appears unusual, it may be intentional for testing how the serializer handles circular references within conditional scopes. Please confirm this is the intended test scenario and not an artifact that should be removed.


1-2: LGTM!

The value binding and setter setup correctly establishes the reactive state for testing circular serialization scenarios.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/resume.expected.md (1)

64-67: Add fenced code language for mutations block (MD040).

Specify a language to satisfy markdownlint.

Apply:

-# Mutations
-```
+# Mutations
+```text
 UPDATE html/body/div/#text "0" => "1"
-```
+```
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/ssr.expected.md (1)

37-47: Add fenced code language for mutations block (MD040).

Same nit as resume snapshot; suggest text.

Apply:

-# Mutations
-```
+# Mutations
+```text
 INSERT html
 INSERT html/head
 INSERT html/body
 INSERT html/body/div
 INSERT html/body/div/#text
 INSERT html/body/div/#comment
 INSERT html/body/script
 INSERT html/body/script/#text
-```
+```
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/tags/setter.js (1)

13-13: Setter value shape differs from the “-object” fixture; confirm cross-snapshot consistency.

Here you assign “setter” to a bare function: $setter2($scope, $setter($scope)). In the “return-serialize-circular-object” DOM/HTML snapshots, “setter” is an object with { fn: ... }. Please confirm that, for this fixture, HTML/SSR snapshots also expect a bare function and that the serializer fix initializes circular refs for both shapes (function and { fn }). If not, align the shape or add fixture coverage for both paths.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f591f61 and a704304.

📒 Files selected for processing (32)
  • .changeset/hungry-needles-shave.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/.name-cache.json (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/csr-sanitized.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/csr.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/tags/setter.js (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/template.hydrate.js (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/template.js (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/html.expected/tags/setter.js (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/html.expected/template.js (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/resume-sanitized.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/resume.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/ssr-sanitized.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/ssr.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/tags/setter.marko (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/template.marko (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/test.ts (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/.name-cache.json (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/csr-sanitized.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/csr.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/tags/setter.js (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.hydrate.js (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.js (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/html.expected/tags/setter.js (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/html.expected/template.js (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/resume-sanitized.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/resume.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/ssr-sanitized.expected.md (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/ssr.expected.md (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/html/serializer.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/test.ts (2)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/test.ts (1)
  • steps (3-3)
packages/runtime-tags/src/__tests__/utils/resolve.ts (1)
  • wait (4-8)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/html.expected/tags/setter.js (2)
packages/runtime-tags/src/__tests__/fixtures/basic-flush-here/__snapshots__/html.expected/template.js (1)
  • _ (4-4)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/html.expected/tags/setter.js (2)
  • $scope0_id (4-4)
  • setter (5-9)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/template.js (2)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/tags/setter.js (3)
  • $scope (6-9)
  • $setup (3-3)
  • $setup (3-3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.js (4)
  • $count (5-8)
  • $setup (9-14)
  • $setCount (18-18)
  • $setCount__script (15-17)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/tags/setter.js (3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/tags/setter.js (9)
  • $input_value__OR__setter (5-11)
  • $scope (6-9)
  • $setter2 (12-12)
  • $input_valueChange (13-15)
  • $input_valueChange (13-15)
  • $input_value (16-16)
  • $input_value (16-16)
  • $input (17-20)
  • $input (17-20)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/template.hydrate.js (2)
  • $input_value__OR__setter (2-6)
  • input_value (3-3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/html.expected/tags/setter.js (1)
  • setter (5-9)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/template.hydrate.js (3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/tags/setter.js (4)
  • $input_value__OR__setter (5-11)
  • $scope (6-9)
  • $input_value (16-16)
  • $input_value (16-16)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.hydrate.js (3)
  • $input_value__OR__setter (2-6)
  • input_value (3-3)
  • $count (12-15)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/template.js (2)
  • $count (5-8)
  • $setCount__script (15-17)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.hydrate.js (3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/tags/setter.js (4)
  • $input_value__OR__setter (5-11)
  • $scope (6-9)
  • $input_value (16-16)
  • $input_value (16-16)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/template.hydrate.js (3)
  • $input_value__OR__setter (2-6)
  • input_value (3-3)
  • $count (12-15)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.js (2)
  • $count (5-8)
  • $setCount__script (15-17)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/test.ts (2)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/test.ts (1)
  • steps (3-3)
packages/runtime-tags/src/__tests__/utils/resolve.ts (1)
  • wait (4-8)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/tags/setter.js (4)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/tags/setter.js (9)
  • $input_value__OR__setter (5-11)
  • $scope (6-9)
  • $setter2 (12-12)
  • $input_valueChange (13-13)
  • $input_valueChange (13-13)
  • $input_value (14-14)
  • $input_value (14-14)
  • $input (15-18)
  • $input (15-18)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/template.hydrate.js (2)
  • $input_value__OR__setter (2-6)
  • input_value (3-3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/html.expected/tags/setter.js (1)
  • setter (5-9)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/html.expected/tags/setter.js (1)
  • setter (5-7)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.js (3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/tags/setter.js (7)
  • $template (1-1)
  • $template (1-1)
  • $walks (2-2)
  • $walks (2-2)
  • $scope (6-9)
  • $setup (3-3)
  • $setup (3-3)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.hydrate.js (1)
  • $count (12-15)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/html.expected/template.js (2)
  • count (5-5)
  • setCount (10-15)
🪛 markdownlint-cli2 (0.18.1)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/ssr.expected.md

38-38: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/resume.expected.md

65-65: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/csr.expected.md

9-9: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


20-20: Multiple headings with the same content

(MD024, no-duplicate-heading)


21-21: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/csr.expected.md

9-9: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


20-20: Multiple headings with the same content

(MD024, no-duplicate-heading)


21-21: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

⏰ 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@20
  • GitHub Check: test: node@18
  • GitHub Check: test: node@22
🔇 Additional comments (32)
packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/test.ts (2)

1-1: LGTM!

The import is correct and necessary for the test fixture.


3-3: LGTM!

The test fixture correctly exports a steps array combining synchronous and asynchronous values for circular serialization testing.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/tags/setter.marko (1)

1-2: LGTM! Test fixture correctly exercises circular reference serialization.

This fixture appropriately creates a circular reference scenario by defining a setter that references input.valueChange and returns both the value and setter together. This pattern aligns with the PR objective to test serializer handling of circular references to registered functions.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/.name-cache.json (1)

1-14: LGTM! Auto-generated snapshot with reasonable variable name mappings.

This name cache snapshot contains appropriate mappings from internal variable identifiers to minified tokens, consistent with Marko's compilation patterns.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/template.marko (1)

1-4: LGTM! Test fixture appropriately exercises circular serialization with setter pattern.

The template correctly establishes a circular reference scenario by binding a setter to a local count variable, invoking it, and rendering the result. This pattern effectively tests the serializer's handling of circular references during hydration.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/ssr-sanitized.expected.md (1)

1-6: LGTM! SSR snapshot correctly shows initial state.

The snapshot appropriately captures the server-rendered output with the initial count value of "0" before any client-side script execution.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/resume-sanitized.expected.md (1)

1-14: LGTM! Resume snapshot correctly captures hydration state transitions.

The snapshot appropriately shows the progression from initial state ("0") to post-hydration state ("1") after setter invocation, validating that circular references are properly restored during the resume phase.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/resume-sanitized.expected.md (1)

1-14: LGTM! Snapshot correctly captures circular-object hydration behavior.

The resume snapshot for the circular-object variant appropriately documents the state transition during hydration, consistent with the related test fixtures.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/resume.expected.md (1)

1-71: LGTM! Comprehensive snapshot validates circular reference serialization.

This detailed snapshot appropriately captures the full hydration flow including walker runtime initialization, serialized circular references, and DOM mutations. The serialized state correctly shows function references and setter assignments being properly initialized, which validates the serializer fix.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/test.ts (1)

1-3: LGTM! Test configuration correctly defines multi-step rendering sequence.

The test setup appropriately defines a two-step rendering flow (initial render + async step) using the wait utility, which enables testing of both synchronous and asynchronous hydration scenarios. This pattern is consistent with the existing return-serialize-circular test fixture.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.hydrate.js (2)

22-22: Confirm init() sourcing.

This hydrate file ends with init(). Ensure the test harness guarantees init in scope for all targets to avoid flakiness.


15-15: The different call shapes are intentional and correspond to different source patterns.

The source fixture files confirm this is by design: circular/template.marko uses setCount() while circular-object/template.marko uses setCount.fn(). Each fixture tests a different calling convention, and the generated snapshots correctly reflect their respective sources. The code generation is working as intended—no standardization or changes are required.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/.name-cache.json (1)

1-14: Name-cache stability.

Looks fine. Please ensure the minified name cache is deterministic in CI to avoid flaky snapshot churn.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/html.expected/tags/setter.js (1)

8-12: Serialization gating for circular refs looks correct; verify symmetry.

Using _serialize_if($scope0_reason, /* input.valueChange */0) for input_value and gating setter on /* input.value */1 appears to break the cycle. Please confirm both directions are exercised in tests (value→setter and setter→valueChange).

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/html.expected/template.js (1)

7-15: Nice: reason mapping + resume wiring matches circular-assign fix.

_set_serialize_reason with input.value depending on count and the resumed valueChange looks consistent with ensuring both sides are assigned before use.

Optionally add a negative test where setter is referenced before input.valueChange to prove the fix prevents undefined access.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/tags/setter.js (1)

5-11: Use of the comma operator in _._return($scope, (input_value, setter)) — confirm intent.

The comma operator returns only setter (evaluating input_value as a side effect). This matches common Marko codegen patterns, but please confirm it’s intentional for this fixture’s OR-accessor and that it participates correctly in the circular assignment flow.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/ssr-sanitized.expected.md (1)

1-6: Snapshot looks correct and minimal.

Matches the sanitized SSR expectation for this fixture.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/csr-sanitized.expected.md (1)

1-14: CSR sanitized snapshot reads as expected (sync 0, async 1).

No issues spotted.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/tags/setter.marko (1)

1-2: Fixture matches the object-with-fn contract used in snapshots.

Looks good. Please just confirm the compiled HTML/DOM snapshots for this fixture show { fn: _._resume(...) } so the circular ref to the registered function is initialized pre-resume as intended.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/csr-sanitized.expected.md (1)

1-14: CSR sanitized snapshot content is consistent with the fixture.

No changes requested.

packages/runtime-tags/src/html/serializer.ts (1)

698-715: The initialization logic looks correct for handling circular references.

The change ensures that fnRef.init is set before calling writeProp(state, scope, parent, ""), which is necessary for circular reference scenarios where the function reference needs to be assigned later.

The logic flow is:

  1. Line 700 initializes fnRef.init with access if not already set
  2. Line 701 calls writeProp to serialize the scope
  3. Lines 704-715 then check if assignments were added during scope serialization
  4. If assignments were added (line 704), fnRef.init is updated to the full expression (line 710)
  5. If no assignments, the inline form is used (lines 713-714)

The ||= operator ensures this only sets init when it's falsy, preserving any existing value. The conditional logic at line 704 still works correctly because it checks if new assignments were added (assigns !== state.assigned.size), not whether fnRef.init exists.

However, the TODO comment expresses uncertainty. Consider either:

  • Removing the TODO if confident this is the correct solution after testing confirms it works
  • Or providing more context about what edge cases might break

Based on the test fixtures added, this appears to properly handle the circular reference case. You may want to verify there are no regressions in non-circular cases by running the full test suite.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/template.marko (1)

1-4: LGTM! Well-structured test fixture for circular reference serialization.

This test fixture effectively validates the circular reference handling by:

  • Declaring a count variable that's bound to a setter
  • The setter references the count via the binding
  • Invoking the setter function, which creates a circular dependency
  • Rendering the result to verify correct serialization and hydration

This directly exercises the code path fixed in the serializer.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/csr.expected.md (1)

1-23: Test snapshot looks correct.

The snapshot properly captures the expected render output and mutations for the circular reference test case, showing the initial render with count=0 and async update to count=1.

The static analysis tool flagged minor markdown formatting issues (missing language specs and duplicate headings), but these are acceptable for test snapshots.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/csr.expected.md (1)

1-23: Test snapshot looks correct.

The snapshot properly documents the expected render behavior for the circular object serialization test case.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/ssr.expected.md (1)

1-49: SSR snapshot looks correct.

The snapshot captures the expected SSR output including the serialized runtime state and hydration script for the circular reference scenario.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/html.expected/tags/setter.js (1)

1-21: Generated setter template module looks correct.

The template properly exports the setter with resume callback and scope serialization configuration for handling the circular reference during hydration.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/template.hydrate.js (1)

1-22: Hydration script looks correct.

The generated hydration script properly wires up the circular dependencies between the setter, count variable, and value change handlers.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/template.js (2)

15-18: Calling setCount.fn() matches the wrapped shape and avoids circular init hazards.

This is the expected counterpart to the plain fixture’s setCount() call. Parity with the HTML snapshot looks good.


9-14: Setup wires valueChange before initializing count — correct ordering.

_setter_input_valueChange(..., $valueChange($scope)) precedes $count($scope, 0), so updates propagate on resume. LGTM.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular/__snapshots__/dom.expected/template.js (1)

15-18: Fixture consistency confirmed—code is correct.

Verification confirms the plain circular fixture correctly uses setCount() (direct call) while the object circular fixture uses setCount.fn() (property call). The calling conventions are consistent with their respective designs.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/dom.expected/tags/setter.js (1)

13-15: Object-wrapping for circular serialization correctly implemented and verified across test fixtures.

The snapshots confirm the intended behavior: in the circular-object fixture, the function is properly wrapped as { fn: $setter($scope) } and called via setCount.fn(), while the non-circular fixture maintains the plain function form called as setCount(). This differential serialization strategy successfully enables early reference assignment while preserving callable access, resolving the circular object serialization issue.

packages/runtime-tags/src/__tests__/fixtures/return-serialize-circular-object/__snapshots__/html.expected/template.js (1)

16-25: Var-before-script ordering confirmed correct.

Verification of the snapshot file shows _._var() at line 16 properly precedes _._script() at line 18, avoiding TDZ hazards during resume when setCount is a registered object with fn.

@codecov
Copy link

codecov bot commented Oct 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.56%. Comparing base (280f4d3) to head (681661a).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2911   +/-   ##
=======================================
  Coverage   88.56%   88.56%           
=======================================
  Files         372      372           
  Lines       46327    46330    +3     
  Branches     3791     3791           
=======================================
+ Hits        41031    41034    +3     
  Misses       5262     5262           
  Partials       34       34           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rturnq rturnq force-pushed the serialize-circular branch from e766973 to de6e1d4 Compare October 26, 2025 00:24
@DylanPiercey DylanPiercey deleted the serialize-circular branch October 28, 2025 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants