fix: async issues #2884
fix: async issues #2884
Conversation
🦋 Changeset detectedLatest commit: 710cead 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 |
WalkthroughAwait/placeholder cleanup now copies pending effects to a local variable and clears the original before draining; Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2884 +/- ##
=======================================
Coverage 88.14% 88.15%
=======================================
Files 371 371
Lines 46115 46128 +13
Branches 3684 3688 +4
=======================================
+ Hits 40649 40662 +13
Misses 5431 5431
Partials 35 35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (3)
.sizes/dom.js (1)
1209-1214: Detaching and draining pending placeholder effects is correctCapturing tryWithPlaceholder.H, clearing the holder, then running with checkPending=true avoids double-run and respects pending-try gating. Consider setting H to undefined instead of [] to prevent accidental reuse and reduce allocations.
Apply this diff:
- let pendingEffects2 = tryWithPlaceholder.H; - pendingEffects2 && - ((tryWithPlaceholder.H = []), - runEffects(pendingEffects2, !0)); + const pendingEffects2 = tryWithPlaceholder.H; + if (pendingEffects2) { + tryWithPlaceholder.H = undefined; + runEffects(pendingEffects2, true); + }packages/runtime-tags/src/html/inlined-runtimes.ts (1)
41-93: Reworked reorder runtime looks sound; please verify op '#' orderingThe new placeholder lifecycle and replaceWith(...container.childNodes) path are clear and fix prior timing bugs. One edge case to confirm: on Line 57, (placeholders[id] = placeholder).i++ assumes placeholder is already set by encountering the T tag for the same id. If a '#' op can be observed before the corresponding 'T', this would throw. If ordering is guaranteed, we’re good; otherwise add a dev-guard in MARKO_DEBUG.
Option (debug-only): guard increment to catch misuse during development.
- (placeholders[id] = placeholder).i++; + if (MARKO_DEBUG && !placeholders[id] && !placeholder) { + throw new Error("Unexpected '#' before placeholder init for id: " + id); + } + (placeholders[id] = placeholder || placeholders[id]).i++;Also minor nit: avoid appending an empty string when no style is found to prevent an extra empty Text node.
- runtime.d.head.append( - runtime.d.querySelector("style[" + runtime.i + "]") || "" - ); + const style = runtime.d.querySelector("style[" + runtime.i + "]"); + style && runtime.d.head.append(style);.changeset/ripe-pandas-do.md (1)
5-5: Tiny grammar fixHyphenate “client-side” and consider “batched effects”.
Apply this diff:
-Fix issue where client side placeholders were not clearing batched effects, causing extra execution. +Fix issue where client‑side placeholders were not clearing batched effects, causing extra execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.changeset/ripe-pandas-do.md(1 hunks).changeset/spotty-trees-design.md(1 hunks).sizes.json(1 hunks).sizes/dom.js(2 hunks)packages/runtime-tags/src/__tests__/fixtures/async-deep-recursive/__snapshots__/ssr.expected.md(2 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reorder-nested-batched-resolve/__snapshots__/ssr.expected.md(2 hunks)packages/runtime-tags/src/__tests__/fixtures/async-state/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/catch-single-reject-async/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/placeholder-single/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/placeholders-nested/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/try-effects-async/__snapshots__/csr.expected.md(0 hunks)packages/runtime-tags/src/__tests__/fixtures/try-effects-async/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/dom/control-flow.ts(1 hunks)packages/runtime-tags/src/dom/queue.ts(1 hunks)packages/runtime-tags/src/html/inlined-runtimes.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/runtime-tags/src/tests/fixtures/try-effects-async/snapshots/csr.expected.md
🧰 Additional context used
🧬 Code graph analysis (3)
packages/runtime-tags/src/dom/control-flow.ts (1)
packages/runtime-tags/src/dom/queue.ts (2)
pendingEffects(18-18)runEffects(97-104)
packages/runtime-tags/src/dom/queue.ts (1)
packages/runtime-tags/src/common/types.ts (1)
Scope(15-27)
.sizes/dom.js (2)
packages/runtime-tags/src/dom/scope.ts (1)
removeAndDestroyBranch(50-53)packages/runtime-tags/src/dom/queue.ts (1)
runEffects(97-104)
🪛 LanguageTool
.changeset/ripe-pandas-do.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...tags": patch --- Fix issue where client side placeholders were not clearing batc...
(QB_NEW_EN_HYPHEN)
[grammar] ~5-~5: There might be a mistake here.
Context: ...atched effects, causing extra execution.
(QB_NEW_EN)
⏰ 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 (14)
.sizes.json (1)
10-11: Slight size increase acknowledgedMin and Brotli grew slightly. Confirm within bundle budget and expected for this fix.
.changeset/spotty-trees-design.md (1)
5-5: Good changesetClear description and correct semver level.
packages/runtime-tags/src/html/inlined-runtimes.ts (1)
38-38: Walker minified branch OKSignature/flow preserved; just minified variable renames.
.sizes/dom.js (1)
1712-1737: Confirm explicit checkPending argument on runEffects call sites
Verify that allrunEffectscalls at queue.ts:73,191; template.ts:129,140; compat.ts:44; control-flow.ts:60,148 pass the secondcheckPendingflag when required.packages/runtime-tags/src/dom/control-flow.ts (1)
144-149: LGTM! Defensive retention-and-clear pattern prevents effect re-execution.The local capture of
tryWithPlaceholder.___effectsfollowed by immediate clearing ensures that any effects added during execution don't get lost or re-run. The null-check guards against undefined, and passingtrueas the second argument correctly forces pending-check behavior as implemented in_enable_catchwithin queue.ts.packages/runtime-tags/src/__tests__/fixtures/catch-single-reject-async/__snapshots__/ssr.expected.md (1)
57-62: Expected test update reflecting runtime changes.The mutation sequence adjustments align with the updated placeholder effect draining behavior introduced in control-flow.ts. The reordering of the
tremoval and text node insertions correctly reflects the new DOM mutation timing.packages/runtime-tags/src/__tests__/fixtures/async-state/__snapshots__/ssr.expected.md (1)
80-92: Consolidated mutation sequence reflects batched placeholder handling.The new removal directive and consolidated insertions correctly represent the updated effect draining behavior. This aligns with the retention-and-clear pattern implemented in the runtime.
packages/runtime-tags/src/__tests__/fixtures/placeholders-nested/__snapshots__/ssr.expected.md (1)
99-100: Nested placeholder unwinding correctly reflected.The consecutive removals and consolidated insertions accurately represent the updated placeholder lifecycle for nested scenarios. This matches the expected behavior from the runtime changes.
Also applies to: 110-110
packages/runtime-tags/src/__tests__/fixtures/try-effects-async/__snapshots__/ssr.expected.md (1)
98-112: Async effect draining mutations updated correctly.The mutation consolidation and reordered placeholder removal align with the retention-and-clear pattern for effects in try/catch scenarios with async operations.
packages/runtime-tags/src/__tests__/fixtures/async-deep-recursive/__snapshots__/ssr.expected.md (1)
114-134: Recursive placeholder unwinding sequence updated.The shifted timing of placeholder removal operations correctly represents the updated effect draining for deeply recursive async scenarios. The mutations properly reflect the sequential unwinding of nested placeholders.
packages/runtime-tags/src/dom/queue.ts (1)
97-104: Progressive API enhancement pattern implemented correctly.The wrapper with type assertion exposes the optional
checkPendingparameter while preserving the base implementation. The actual conditional behavior is deferred to the_enable_catchwrapper (lines 171-194), which is a clean progressive enhancement pattern.packages/runtime-tags/src/__tests__/fixtures/placeholder-single/__snapshots__/ssr.expected.md (1)
74-84: Single placeholder mutation sequence updated as expected.The consolidated text node insertion and reordered placeholder removal correctly reflect the updated effect draining behavior for the single-placeholder case.
packages/runtime-tags/src/__tests__/fixtures/async-reorder-nested-batched-resolve/__snapshots__/ssr.expected.md (2)
114-114: Verify CSR snapshot matches SSR placeholder‐cleanup ordering
SSR mutations now include REMOVE t at lines 114, 129, 134, 142. Please confirm the CSR snapshot reflects the same REMOVE t sequence:rg -n -A200 -B20 '^# Mutations' packages/runtime-tags/src/__tests__/fixtures/async-reorder-nested-batched-resolve/__snapshots__/csr.expected.md
134-134: Confirm distinct placeholders for duplicate removals
The twoREMOVE t after html/body/script0entries at lines 129 and 134 are identical—ensure they refer to separate placeholder instances, not a duplicated removal. Consider enhancing the mutation log to include a unique marker key for each placeholder to avoid confusion in snapshots.
DylanPiercey
force-pushed
the
async-issues
branch
from
b3a7415 to
775f037
Compare
DylanPiercey
force-pushed
the
async-issues
branch
from
775f037 to
710cead
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.changeset/ripe-pandas-do.md (1)
5-5: Consider hyphenating "client-side" for clarity.The phrase "client side placeholders" could be written as "client-side placeholders" to follow standard English hyphenation conventions for compound adjectives.
Apply this diff:
-Fix issue where client side placeholders were not clearing batched effects, causing extra execution. +Fix issue where client-side placeholders were not clearing batched effects, causing extra execution.packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/resume.expected.md (1)
49-53: Consider adding a language identifier to the mutations code block.The mutations code block at line 49 lacks a language identifier. Adding one (e.g.,
diffortext) would improve syntax highlighting and documentation clarity.Apply this diff:
# Mutations -``` +```text REMOVE html/body/#comment0 before #commentpackages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/csr-sanitized.expected.md (1)
5-7: Consider adding language identifier for better documentation.The HTML code block at line 5 lacks a language identifier. Adding
htmlwould improve syntax highlighting.Apply this diff:
# Render ASYNC -``` +```html Rejected Bpackages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/ssr.expected.md (1)
2-14: Consider adding language identifiers to code blocks.Multiple code blocks in this snapshot lack language identifiers (lines 2, 7, 12), which would improve syntax highlighting and documentation clarity.
Example for the first block:
# Write -```html +```html <!--M_[--><!--M_!^b--><script>WALKER_RUNTIME("M")("_");...Note: The duplicate "Write" headings (lines 1, 6, 11) are standard for this test format and can be safely ignored.
packages/runtime-tags/src/html/inlined-runtimes.ts (1)
74-84: Consider adding inline comment for terse removal loop.The loop at lines 76-82 is quite compact and uses the comma operator to both remove nodes and check the termination condition. While correct, a brief inline comment would aid future maintainability.
Example comment:
c(start = runtime.l["^" + id]) { if (--placeholderRoot.i) return 1; + // Remove all siblings between start and id marker for ( ; (nextSibling =
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
.changeset/ripe-pandas-do.md(1 hunks).changeset/spotty-trees-design.md(1 hunks).sizes.json(1 hunks).sizes/dom.js(2 hunks)packages/runtime-tags/src/__tests__/fixtures/async-deep-recursive/__snapshots__/ssr.expected.md(2 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/.name-cache.json(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/csr-sanitized.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/csr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/dom.expected/template.hydrate.js(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/dom.expected/template.js(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/html.expected/template.js(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/resume-sanitized.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/resume.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/ssr-sanitized.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/template.marko(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/test.ts(1 hunks)packages/runtime-tags/src/__tests__/fixtures/async-reorder-nested-batched-resolve/__snapshots__/ssr.expected.md(2 hunks)packages/runtime-tags/src/__tests__/fixtures/async-state/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/catch-single-reject-async/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/placeholder-single/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/placeholders-nested/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/__tests__/fixtures/try-effects-async/__snapshots__/csr.expected.md(0 hunks)packages/runtime-tags/src/__tests__/fixtures/try-effects-async/__snapshots__/ssr.expected.md(1 hunks)packages/runtime-tags/src/dom/control-flow.ts(1 hunks)packages/runtime-tags/src/dom/queue.ts(1 hunks)packages/runtime-tags/src/html/inlined-runtimes.ts(1 hunks)packages/runtime-tags/src/html/writer.ts(10 hunks)
💤 Files with no reviewable changes (1)
- packages/runtime-tags/src/tests/fixtures/try-effects-async/snapshots/csr.expected.md
✅ Files skipped from review due to trivial changes (3)
- packages/runtime-tags/src/tests/fixtures/async-reject-then-resolve-isolated-boundaries/template.marko
- packages/runtime-tags/src/tests/fixtures/async-reject-then-resolve-isolated-boundaries/snapshots/.name-cache.json
- packages/runtime-tags/src/tests/fixtures/async-reject-then-resolve-isolated-boundaries/snapshots/csr.expected.md
🚧 Files skipped from review as they are similar to previous changes (10)
- .changeset/spotty-trees-design.md
- packages/runtime-tags/src/dom/control-flow.ts
- packages/runtime-tags/src/tests/fixtures/async-reject-then-resolve-isolated-boundaries/snapshots/resume-sanitized.expected.md
- packages/runtime-tags/src/dom/queue.ts
- packages/runtime-tags/src/tests/fixtures/catch-single-reject-async/snapshots/ssr.expected.md
- packages/runtime-tags/src/tests/fixtures/try-effects-async/snapshots/ssr.expected.md
- packages/runtime-tags/src/tests/fixtures/async-reject-then-resolve-isolated-boundaries/snapshots/dom.expected/template.hydrate.js
- packages/runtime-tags/src/tests/fixtures/async-reject-then-resolve-isolated-boundaries/test.ts
- packages/runtime-tags/src/tests/fixtures/async-reject-then-resolve-isolated-boundaries/snapshots/ssr-sanitized.expected.md
- .sizes.json
🧰 Additional context used
🧬 Code graph analysis (3)
.sizes/dom.js (2)
packages/runtime-tags/src/dom/scope.ts (1)
removeAndDestroyBranch(50-53)packages/runtime-tags/src/dom/queue.ts (1)
runEffects(97-104)
packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/dom.expected/template.js (1)
packages/runtime-tags/src/__tests__/utils/resolve.ts (2)
rejectAfter(30-38)resolveAfter(22-28)
packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/html.expected/template.js (1)
packages/runtime-tags/src/__tests__/utils/resolve.ts (2)
resolveAfter(22-28)rejectAfter(30-38)
🪛 LanguageTool
.changeset/ripe-pandas-do.md
[grammar] ~5-~5: Use a hyphen to join words.
Context: ...tags": patch --- Fix issue where client side placeholders were not clearing batc...
(QB_NEW_EN_HYPHEN)
[grammar] ~5-~5: There might be a mistake here.
Context: ...atched effects, causing extra execution.
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.18.1)
packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/csr-sanitized.expected.md
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/resume.expected.md
49-49: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/ssr.expected.md
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
25-25: Multiple headings with the same content
(MD024, no-duplicate-heading)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
33-33: Multiple headings with the same content
(MD024, no-duplicate-heading)
45-45: Multiple headings with the same content
(MD024, no-duplicate-heading)
46-46: 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 (15)
packages/runtime-tags/src/__tests__/fixtures/async-state/__snapshots__/ssr.expected.md (1)
80-92: LGTM! Mutation sequence correctly reflects updated placeholder lifecycle.The updated mutation sequence, including the new
REMOVE t after html/body/#comment5operation, correctly reflects the changes to how placeholder effects are cleared and nodes are reordered during SSR rendering..sizes/dom.js (1)
1209-1213: LGTM! Critical fix for async placeholder effect ordering.The changes correctly address issue #2879 by isolating placeholder effects before draining:
- Effect isolation: Copying
tryWithPlaceholder.Hto localpendingEffects2prevents re-entrant modifications during drain- Container reset: Clearing
tryWithPlaceholder.H = []before draining ensures new effects added during execution go to a fresh container- Correct drain sequence: Calling
runEffects(pendingEffects2, !0)withcheckPending=trueensures proper runtime/HTML flush orderingThis prevents the scenario where runtime scripts flush before required HTML when promise rejection occurs first, which was causing browser crashes.
The minimal size increase (15 bytes min, 12 bytes brotli) is acceptable for this critical fix.
packages/runtime-tags/src/__tests__/fixtures/placeholder-single/__snapshots__/ssr.expected.md (1)
74-84: LGTM! Mutation sequence update is consistent with the fix.The updated mutation sequence at line 74 (
REMOVE t after html/body/#text4) correctly reflects the changes to placeholder lifecycle management, consistent with similar updates across other test fixtures.packages/runtime-tags/src/__tests__/fixtures/placeholders-nested/__snapshots__/ssr.expected.md (1)
99-100: Verify duplicate REMOVE operations across snapshots.Duplicate
REMOVE t after …entries appear in three snapshot files. Confirm whether this is intended idempotent behavior or if the mutation tracking/snapshot generation should deduplicate redundant operations.packages/runtime-tags/src/__tests__/fixtures/async-deep-recursive/__snapshots__/ssr.expected.md (1)
114-114: Flush order realigned; looks correct.Moving t removal to occur after script flush matches the runtime changes and fixes the misordered flush on rejection.
Also applies to: 134-134
packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/html.expected/template.js (1)
1-33: Good coverage for reject-before-resolve.This fixture validates the problematic order. Please also ensure there's a test with an nested inside a (from #2879) to lock the original repro.
Do you already have a nested inside snapshot in this PR? If not, I can help add one mirroring the linked repro.
packages/runtime-tags/src/html/writer.ts (3)
821-826: Schedule walk after rejected branch rewrite.Setting cur.needsWalk in the catch-path ensures the WALK runtime runs after converting the placeholder to the end marker. This is the crucial fix for the rejection ordering bug.
1035-1057: Aggregate needsWalk correctly during consume.Propagating needsWalk across contiguous chunks ensures one walker emission per flush even after chain merges.
1084-1086: Walker emission is now reliably gated by effects/reorders and per-chunk flags.
- Consuming state.walkOnNextFlush in flushScript, setting it in flushHTML when this.needsWalk is true, and also forcing needsWalk when effects/reorders are present ensures a single, timely WALK call per flush.
- This sequencing aligns with the snapshot changes and resolves premature script flushing.
Also applies to: 1120-1122, 1218-1227
packages/runtime-tags/src/__tests__/fixtures/async-reorder-nested-batched-resolve/__snapshots__/ssr.expected.md (1)
114-114: Consistent reordering across nested batched resolve.The updated t removal timing after script flush matches the fix and keeps SSR mutations coherent.
Also applies to: 134-134
packages/runtime-tags/src/__tests__/fixtures/async-reject-then-resolve-isolated-boundaries/__snapshots__/dom.expected/template.js (1)
5-6: Duplicate _._enable_catch() calls.Looks redundant; if idempotent it’s harmless, but confirm this is intentional from the compiler/runtime debug output.
If unintentional, I can help adjust the generator to emit it once per module.
packages/runtime-tags/src/html/inlined-runtimes.ts (4)
38-38: LGTM! Minified walker runtime matches debug version.The variable renaming and reordering in the minified WALKER_RUNTIME_CODE correctly corresponds to the debug version, preserving the tree-walking and marker-processing logic.
41-41: LGTM! Early-exit guard prevents re-initialization.The guard
if (runtime.j) return;correctly prevents re-initialization of the runtime, which is important for avoiding duplicate placeholder setup or event handler registration.
69-85: LGTM! Counter-based placeholder lifecycle fixes async flush ordering.The placeholder object with increment/decrement counters (
iproperty) and thec()completion callback provides deterministic coordination for nested async boundaries. This directly addresses the issue from #2879 where promise rejections caused incorrect flush order. The counter approach ensures all placeholder segments are ready before replacement occurs, preventing runtime scripts from flushing before required HTML.
56-61: Verify placeholder initialization order before increment
At inlined-runtimes.ts:57,placeholder.iis incremented onop == "#", assuming the placeholder was set by a prior "T" tag. Confirm the walker never invokesruntime.xwith"#"for anidbefore its corresponding "T" tag—or add a guard to handle out-of-order ops.
| public needsWalk = false; | ||
| public reorderId: string | null = null; |
There was a problem hiding this comment.
🧩 Analysis chain
Per-chunk walk flag added.
needsWalk on Chunk is a clear, minimal mechanism to carry walk intent through consumption/flush.
To avoid stale references, please verify no remaining uses of the previous flag name exist:
🏁 Script executed:
#!/bin/bash
# Find stale references to a previous per-chunk walk flag and confirm new usage.
rg -nP '\bhasWalk\b|needsWalk\b|walkOnNextFlush\b' -g '!**/node_modules/**' -SLength of output: 1313
Remove stale walkOnNextFlush references
Stale walkOnNextFlush usages remain and must be replaced with needsWalk:
- packages/runtime-class/src/runtime/helpers/tags-compat/runtime-html.js:48
- packages/runtime-tags/src/html/writer.ts:855, 1084–1085, 1223, 1282
🤖 Prompt for AI Agents
In packages/runtime-tags/src/html/writer.ts around lines 855, 1084–1085, 1223,
and 1282 (file header shows needsWalk declared at 966–967), replace all
references to the stale property/method name walkOnNextFlush with the new
boolean property needsWalk; update any assignments, checks, and toggles (e.g.,
walkOnNextFlush = true -> needsWalk = true; if (walkOnNextFlush) -> if
(needsWalk)) and ensure logic semantics remain identical, leaving the existing
reorderId handling unchanged.
resolves #2879