Skip to content

Conversation

@rturnq
Copy link
Contributor

@rturnq rturnq commented Oct 25, 2025

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Oct 25, 2025

🦋 Changeset detected

Latest commit: 2922ce9

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 25, 2025

Walkthrough

This change adds hoisting relationship tracking to the runtime-tags translator. A new hoistedTo field was added to the Section interface to track references hoisted to a section. The reference tracking logic in trackHoistedReference was updated to populate this field when hoisted bindings are created, establishing bidirectional tracking between sections and their hoisted references. Signal setup resolution was modified to branch on the hoistedTo state. Test fixtures were added, including a template demonstrating nested hoisting with conditional blocks and a corresponding test file.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The pull request description is completely empty, containing no information about the changeset or the rationale behind these changes. While the check is designed to be lenient, the pass criterion requires that the description be "related in some way to the changeset." An empty description fails to meet this criterion as it provides no relation to any part of the changeset whatsoever, and it does not describe any aspect of the changes being made.
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.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix: duplicate setup when hoisting" clearly and accurately summarizes the main objective of this changeset. The PR introduces structural changes to track hoisted references, adds a new field to the Section interface, and modifies signal resolution logic—all in service of fixing an issue where duplicate setup signals were being created during hoisting. The title is concise, uses proper conventional commit formatting, and is specific enough that a teammate scanning the history would immediately understand the primary fix.
✨ 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 fix-hoist-setup

📜 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 280f4d3 and 2922ce9.

⛔ Files ignored due to path filters (11)
  • .changeset/mean-pianos-march.md is excluded by none and included by none
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/.name-cache.json is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/csr-sanitized.expected.md is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/csr.expected.md is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/dom.expected/template.hydrate.js is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/dom.expected/template.js is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/html.expected/template.js is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/resume-sanitized.expected.md is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/resume.expected.md is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/ssr-sanitized.expected.md is excluded by !**/__snapshots__/** and included by **/src/**
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/__snapshots__/ssr.expected.md is excluded by !**/__snapshots__/** and included by **/src/**
📒 Files selected for processing (5)
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/template.marko (1 hunks)
  • packages/runtime-tags/src/__tests__/fixtures/hoist-only/test.ts (1 hunks)
  • packages/runtime-tags/src/translator/util/references.ts (1 hunks)
  • packages/runtime-tags/src/translator/util/sections.ts (2 hunks)
  • packages/runtime-tags/src/translator/util/signals.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/runtime-tags/src/translator/util/sections.ts (1)
packages/runtime-tags/src/translator/util/references.ts (1)
  • ReferencedBindings (112-112)
🔇 Additional comments (6)
packages/runtime-tags/src/translator/util/references.ts (1)

380-380: LGTM! Bidirectional hoisting relationship correctly established.

This change completes the bidirectional tracking between sections and their hoisted bindings, mirroring the existing pattern on line 379.

packages/runtime-tags/src/translator/util/sections.ts (2)

65-65: LGTM! New field follows existing patterns.

The hoistedTo field addition complements the existing hoisted field, enabling reverse lookup of hoisting relationships.


137-137: LGTM! Correct initialization.

Initialization to undefined is consistent with other optional Section fields.

packages/runtime-tags/src/__tests__/fixtures/hoist-only/test.ts (1)

1-1: LGTM! Standard test fixture pattern.

This minimal test configuration is typical for compiler snapshot/code generation tests.

packages/runtime-tags/src/translator/util/signals.ts (1)

1294-1296: LGTM! Fixes duplicate setup in hoisted contexts.

The branching logic correctly ensures a setup signal is created when a section has bindings hoisted to it, preventing the duplicate setup issue described in the PR title. When hoistedTo is truthy, getSignal() ensures the signal exists; otherwise, the original behavior is preserved.

packages/runtime-tags/src/__tests__/fixtures/hoist-only/template.marko (1)

1-12: LGTM! Comprehensive hoisting test case.

This template effectively tests nested hoisting scenarios with three levels of scope (top-level, first <if>, nested <if>), each defining hoisted constants. This validates that the hoistedTo tracking correctly prevents duplicate setup across multiple hoisting levels.


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.

@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 (2922ce9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2916   +/-   ##
=======================================
  Coverage   88.56%   88.56%           
=======================================
  Files         372      372           
  Lines       46327    46332    +5     
  Branches     3791     3793    +2     
=======================================
+ Hits        41031    41036    +5     
  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.

@DylanPiercey DylanPiercey merged commit 5963a78 into main Oct 28, 2025
11 checks passed
@DylanPiercey DylanPiercey deleted the fix-hoist-setup branch October 28, 2025 20:07
@github-actions github-actions bot mentioned this pull request Oct 25, 2025
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