fix: run effects in pending snippets (#17719)

Boundaries are buggy: if the `pending` snippet contains state, changes
to that state [won't cause
updates](https://svelte.dev/playground/hello-world?version=5.51.2#H4sIAAAAAAAAE22SQW-DMAyF_0qUTSpoE92uFJh223Hate0hJWaNliZRYsoqxH9fQtJW6noD-33Pz4aRKnYAWtIPkFKTQVvJSQZcIPCcPtNOSHC0XI8UTyboQsHXE_VuTOGOIDHUdszBvXqrFYJCb0Mr11phsNmoDUpAYsFpeQTrSE3W25Uv-0bXqxaFVsT0bp8dmewhJ2PobNB7OSQcOrAWuKc-rT4IB8UgcP91dsvyVZRf_IvZK8tJ3VzoInXTiCuDvVVXlYkT5u50k9DtRYfZJd11XGq8FSlKAsPOre4V-uSPDhlC9hIE1fJ6GFXtekRvrlUrRftTjzF25J5q8jrN9xOqtXDwhw14RO7jc5bIzI-3-vihyp3358yeZmFlmpENTGD8CIu0GV_kU7U0TdxmfHBKGON3MqC4UN9ZPsVDBHzOe1bjuEzaad7230j_nyD8Ii3R9jBt_RsTchCK07Jj0sH0B6hNF6aqAgAA):

```svelte
<script>
	let resolvers = [];

	function push(value) {
		const deferred = Promise.withResolvers();
		resolvers.push(() => deferred.resolve(value));
		return deferred.promise;
	}

	function shift() {
		resolvers.shift()?.();
	}

	let count = $state(0);
</script>

<button onclick={() => count += 1}>
	increment
</button>

<button onclick={shift}>
	shift
</button>

<svelte:boundary>
	<p>{await push('resolved')}</p>

	{#snippet pending()}
		<p>{count}</p>
	{/snippet}
</svelte:boundary>
```

The issue is that the boundary's `this.#effect` has the
`BOUNDARY_EFFECT` flag, and `this.#pending_effect` is a child thereof.
Instead, `this.#main_effect` should have the flag. (It turns out
`this.#failed_effect` _also_ needs the flag, because errors that occur
in a `failed` snippet cause the boundary to re-render in its `failed`
state, which I found somewhat confusing to be honest. Probably the right
choice though.)

I was able to simplify the code a bit, too.

~~(Actually now that I think about it do we need `this.#effect` at all?
Will check.)~~

### Before submitting the PR, please make sure you do the following

- [x] It's really useful if your PR references an issue where it is
discussed ahead of time. In many cases, features are absent for a
reason. For large changes, please create an RFC:
https://github.com/sveltejs/rfcs
- [x] Prefix your PR title with `feat:`, `fix:`, `chore:`, or `docs:`.
- [x] This message body should clearly illustrate what problems it
solves.
- [x] Ideally, include a test that fails without this PR but passes with
it.
- [x] If this PR changes code within `packages/svelte/src`, add a
changeset (`npx changeset`).

### Tests and linting

- [x] Run the tests with `pnpm test` and lint the project with `pnpm
lint`

---------

Co-authored-by: Simon H <[email protected]>

Author: Rich-Harris

.changeset/many-dolls-argue.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: run effects in pending snippets

packages/svelte/src/internal/client/dom/blocks/boundary.js

@@ -1,6 +1,5 @@
 /** @import { Effect, Source, TemplateNode, } from '#client' */
 import {
-	BLOCK_EFFECT,
 	BOUNDARY_EFFECT,
 	COMMENT_NODE,
 	DIRTY,
@@ -53,7 +52,7 @@ import { set_signal_status } from '../../reactivity/status.js';
  * }} BoundaryProps
  */
 
-var flags = EFFECT_TRANSPARENT | EFFECT_PRESERVED | BOUNDARY_EFFECT;
+var flags = EFFECT_TRANSPARENT | EFFECT_PRESERVED;
 
 /**
  * @param {TemplateNode} node
@@ -98,15 +97,10 @@ export class Boundary {
 	/** @type {DocumentFragment | null} */
 	#offscreen_fragment = null;
 
-	/** @type {TemplateNode | null} */
-	#pending_anchor = null;
-
 	#local_pending_count = 0;
 	#pending_count = 0;
 	#pending_count_update_queued = false;
 
-	#is_creating_fallback = false;
-
 	/** @type {Set<Effect>} */
 	#dirty_effects = new Set();
 
@@ -142,51 +136,31 @@ export class Boundary {
 	constructor(node, props, children) {
 		this.#anchor = node;
 		this.#props = props;
-		this.#children = children;
 
-		this.parent = /** @type {Effect} */ (active_effect).b;
+		this.#children = (anchor) => {
+			var effect = /** @type {Effect} */ (active_effect);
 
-		this.is_pending = !!this.#props.pending;
+			effect.b = this;
+			effect.f |= BOUNDARY_EFFECT;
 
-		this.#effect = block(() => {
-			/** @type {Effect} */ (active_effect).b = this;
+			children(anchor);
+		};
+
+		this.parent = /** @type {Effect} */ (active_effect).b;
 
+		this.#effect = block(() => {
 			if (hydrating) {
-				const comment = this.#hydrate_open;
+				const comment = /** @type {Comment} */ (this.#hydrate_open);
 				hydrate_next();
 
-				const server_rendered_pending =
-					/** @type {Comment} */ (comment).nodeType === COMMENT_NODE &&
-					/** @type {Comment} */ (comment).data === HYDRATION_START_ELSE;
-
-				if (server_rendered_pending) {
+				if (comment.data === HYDRATION_START_ELSE) {
 					this.#hydrate_pending_content();
 				} else {
 					this.#hydrate_resolved_content();
-
-					if (this.#pending_count === 0) {
-						this.is_pending = false;
-					}
 				}
 			} else {
-				var anchor = this.#get_anchor();
-
-				try {
-					this.#main_effect = branch(() => children(anchor));
-				} catch (error) {
-					this.error(error);
-				}
-
-				if (this.#pending_count > 0) {
-					this.#show_pending_snippet();
-				} else {
-					this.is_pending = false;
-				}
+				this.#render();
 			}
-
-			return () => {
-				this.#pending_anchor?.remove();
-			};
 		}, flags);
 
 		if (hydrating) {
@@ -206,19 +180,24 @@ export class Boundary {
 		const pending = this.#props.pending;
 		if (!pending) return;
 
+		this.is_pending = true;
 		this.#pending_effect = branch(() => pending(this.#anchor));
 
 		queue_micro_task(() => {
-			var anchor = this.#get_anchor();
+			var fragment = (this.#offscreen_fragment = document.createDocumentFragment());
+			var anchor = create_text();
+
+			fragment.append(anchor);
 
 			this.#main_effect = this.#run(() => {
 				Batch.ensure();
 				return branch(() => this.#children(anchor));
 			});
 
-			if (this.#pending_count > 0) {
-				this.#show_pending_snippet();
-			} else {
+			if (this.#pending_count === 0) {
+				this.#anchor.before(fragment);
+				this.#offscreen_fragment = null;
+
 				pause_effect(/** @type {Effect} */ (this.#pending_effect), () => {
 					this.#pending_effect = null;
 				});
@@ -228,17 +207,28 @@ export class Boundary {
 		});
 	}
 
-	#get_anchor() {
-		var anchor = this.#anchor;
+	#render() {
+		try {
+			this.is_pending = this.has_pending_snippet();
+			this.#pending_count = 0;
+			this.#local_pending_count = 0;
+
+			this.#main_effect = branch(() => {
+				this.#children(this.#anchor);
+			});
 
-		if (this.is_pending) {
-			this.#pending_anchor = create_text();
-			this.#anchor.before(this.#pending_anchor);
+			if (this.#pending_count > 0) {
+				var fragment = (this.#offscreen_fragment = document.createDocumentFragment());
+				move_effect(this.#main_effect, fragment);
 
-			anchor = this.#pending_anchor;
+				const pending = /** @type {(anchor: Node) => void} */ (this.#props.pending);
+				this.#pending_effect = branch(() => pending(this.#anchor));
+			} else {
+				this.is_pending = false;
+			}
+		} catch (error) {
+			this.error(error);
 		}
-
-		return anchor;
 	}
 
 	/**
@@ -262,7 +252,8 @@ export class Boundary {
 	}
 
 	/**
-	 * @param {() => Effect | null} fn
+	 * @template T
+	 * @param {() => T} fn
 	 */
 	#run(fn) {
 		var previous_effect = active_effect;
@@ -285,20 +276,6 @@ export class Boundary {
 		}
 	}
 
-	#show_pending_snippet() {
-		const pending = /** @type {(anchor: Node) => void} */ (this.#props.pending);
-
-		if (this.#main_effect !== null) {
-			this.#offscreen_fragment = document.createDocumentFragment();
-			this.#offscreen_fragment.append(/** @type {TemplateNode} */ (this.#pending_anchor));
-			move_effect(this.#main_effect, this.#offscreen_fragment);
-		}
-
-		if (this.#pending_effect === null) {
-			this.#pending_effect = branch(() => pending(this.#anchor));
-		}
-	}
-
 	/**
 	 * Updates the pending count associated with the currently visible pending snippet,
 	 * if any, such that we can replace the snippet with content once work is done
@@ -383,7 +360,7 @@ export class Boundary {
 
 		// If we have nothing to capture the error, or if we hit an error while
 		// rendering the fallback, re-throw for another boundary to handle
-		if (this.#is_creating_fallback || (!onerror && !failed)) {
+		if (!onerror && !failed) {
 			throw error;
 		}
 
@@ -423,31 +400,18 @@ export class Boundary {
 				e.svelte_boundary_reset_onerror();
 			}
 
-			// If the failure happened while flushing effects, current_batch can be null
-			Batch.ensure();
-
-			this.#local_pending_count = 0;
-
 			if (this.#failed_effect !== null) {
 				pause_effect(this.#failed_effect, () => {
 					this.#failed_effect = null;
 				});
 			}
 
-			// we intentionally do not try to find the nearest pending boundary. If this boundary has one, we'll render it on reset
-			// but it would be really weird to show the parent's boundary on a child reset.
-			this.is_pending = this.has_pending_snippet();
+			this.#run(() => {
+				// If the failure happened while flushing effects, current_batch can be null
+				Batch.ensure();
 
-			this.#main_effect = this.#run(() => {
-				this.#is_creating_fallback = false;
-				return branch(() => this.#children(this.#anchor));
+				this.#render();
 			});
-
-			if (this.#pending_count > 0) {
-				this.#show_pending_snippet();
-			} else {
-				this.is_pending = false;
-			}
 		};
 
 		queue_micro_task(() => {
@@ -462,10 +426,16 @@ export class Boundary {
 			if (failed) {
 				this.#failed_effect = this.#run(() => {
 					Batch.ensure();
-					this.#is_creating_fallback = true;
 
 					try {
 						return branch(() => {
+							// errors in `failed` snippets cause the boundary to error again
+							// TODO Svelte 6: revisit this decision, most likely better to go to parent boundary instead
+							var effect = /** @type {Effect} */ (active_effect);
+
+							effect.b = this;
+							effect.f |= BOUNDARY_EFFECT;
+
 							failed(
 								this.#anchor,
 								() => error,
@@ -475,8 +445,6 @@ export class Boundary {
 					} catch (error) {
 						invoke_error_boundary(error, /** @type {Effect} */ (this.#effect.parent));
 						return null;
-					} finally {
-						this.#is_creating_fallback = false;
 					}
 				});
 			}

packages/svelte/src/internal/client/reactivity/batch.js

@@ -293,16 +293,19 @@ export class Batch {
 				}
 			}
 
-			var parent = effect.parent;
-			effect = effect.next;
-
-			while (effect === null && parent !== null) {
-				if (parent === pending_boundary) {
+			while (effect !== null) {
+				if (effect === pending_boundary) {
 					pending_boundary = null;
 				}
 
-				effect = parent.next;
-				parent = parent.parent;
+				var next = effect.next;
+
+				if (next !== null) {
+					effect = next;
+					break;
+				}
+
+				effect = effect.parent;
 			}
 		}
 	}

packages/svelte/tests/runtime-runes/samples/async-boundary-pending-live/_config.js

@@ -0,0 +1,38 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+
+export default test({
+	html: `
+		<button>increment</button>
+		<button>shift</button>
+		<p>0</p>
+	`,
+
+	async test({ assert, target }) {
+		const [increment, shift] = target.querySelectorAll('button');
+
+		increment.click();
+		await tick();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>increment</button>
+				<button>shift</button>
+				<p>1</p>
+			`
+		);
+
+		shift.click();
+		await tick();
+
+		assert.htmlEqual(
+			target.innerHTML,
+			`
+				<button>increment</button>
+				<button>shift</button>
+				<p>resolved</p>
+			`
+		);
+	}
+});

packages/svelte/tests/runtime-runes/samples/async-boundary-pending-live/main.svelte

@@ -0,0 +1,31 @@
+<script>
+	let resolvers = [];
+
+	function push(value) {
+		const deferred = Promise.withResolvers();
+		resolvers.push(() => deferred.resolve(value));
+		return deferred.promise;
+	}
+
+	function shift() {
+		resolvers.shift()?.();
+	}
+
+	let count = $state(0);
+</script>
+
+<button onclick={() => count += 1}>
+	increment
+</button>
+
+<button onclick={shift}>
+	shift
+</button>
+
+<svelte:boundary>
+	<p>{await push('resolved')}</p>
+
+	{#snippet pending()}
+		<p>{count}</p>
+	{/snippet}
+</svelte:boundary>