fix: don't access inert block effects (#17882)

In #17837 we added logic to not schedule another batch during
resumption. The logic in there turns out to be flawed - it's dangerous
to keep accessing inert block effects, because if they're nested they
could access properties that no longer exist (because the outer if makes
the inner if obsolete).

So this PR basically reverts #17837 and instead schedules another batch
again under the assumption that this will only happen during the commit
phase, and all that's gonna happen is that it will schedule another
batch, which is safe.

Fixes #17866 Fixes #17878

This reverts commit 2f12b60701.

Co-authored-by: Rich Harris <rich.harris@vercel.com>

.changeset/light-jokes-draw.md

@@ -0,0 +1,5 @@
+---
+'svelte': patch
+---
+
+fix: don't access inert block effects

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

@@ -1,5 +1,4 @@
 /** @import { Effect, TemplateNode } from '#client' */
-import { INERT } from '#client/constants';
 import { Batch, current_batch } from '../../reactivity/batch.js';
 import {
 	branch,
@@ -88,7 +87,7 @@ export class BranchManager {
 			// effect is currently offscreen. put it in the DOM
 			var offscreen = this.#offscreen.get(key);
 
-			if (offscreen && (offscreen.effect.f & INERT) === 0) {
+			if (offscreen) {
 				this.#onscreen.set(key, offscreen.effect);
 				this.#offscreen.delete(key);
 
@@ -125,9 +124,6 @@ export class BranchManager {
 			//    or those that are already outroing (else the transition is aborted and the effect destroyed right away)
 			if (k === key || this.#outroing.has(k)) continue;
 
-			// don't destroy branches that are inside outroing blocks
-			if ((effect.f & INERT) !== 0) continue;
-
 			const on_destroy = () => {
 				const keys = Array.from(this.#batches.values());
 

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

@@ -301,26 +301,18 @@ export class Batch {
 			var is_branch = (flags & (BRANCH_EFFECT | ROOT_EFFECT)) !== 0;
 			var is_skippable_branch = is_branch && (flags & CLEAN) !== 0;
 
-			var inert = (flags & INERT) !== 0;
-			var skip = is_skippable_branch || this.#skipped_branches.has(effect);
+			var skip = is_skippable_branch || (flags & INERT) !== 0 || this.#skipped_branches.has(effect);
 
 			if (!skip && effect.fn !== null) {
 				if (is_branch) {
-					if (!inert) effect.f ^= CLEAN;
+					effect.f ^= CLEAN;
 				} else if ((flags & EFFECT) !== 0) {
 					effects.push(effect);
-				} else if ((flags & (RENDER_EFFECT | MANAGED_EFFECT)) !== 0 && (async_mode_flag || inert)) {
+				} else if (async_mode_flag && (flags & (RENDER_EFFECT | MANAGED_EFFECT)) !== 0) {
 					render_effects.push(effect);
 				} else if (is_dirty(effect)) {
+					if ((flags & BLOCK_EFFECT) !== 0) this.#maybe_dirty_effects.add(effect);
 					update_effect(effect);
-
-					if ((flags & BLOCK_EFFECT) !== 0) {
-						this.#maybe_dirty_effects.add(effect);
-
-						// if this is inside an outroing block, ensure that the block
-						// re-runs if the outro is later aborted
-						if (inert) set_signal_status(effect, DIRTY);
-					}
 				}
 
 				var child = effect.first;

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

@@ -45,7 +45,6 @@ import { Batch, collected_effects } from './batch.js';
 import { flatten, increment_pending } from './async.js';
 import { without_reactive_context } from '../dom/elements/bindings/shared.js';
 import { set_signal_status } from './status.js';
-import { async_mode_flag } from '../../flags/index.js';
 
 /**
  * @param {'$effect' | '$effect.pre' | '$inspect'} rune
@@ -679,10 +678,13 @@ function resume_children(effect, local) {
 	if ((effect.f & INERT) === 0) return;
 	effect.f ^= INERT;
 
-	// Mark branches as clean so that effects can be scheduled, but only in async mode
-	// (in legacy mode, effect resumption happens during traversal)
-	if (async_mode_flag && (effect.f & BRANCH_EFFECT) !== 0 && (effect.f & CLEAN) === 0) {
-		effect.f ^= CLEAN;
+	// If a dependency of this effect changed while it was paused,
+	// schedule the effect to update. we don't use `is_dirty`
+	// here because we don't want to eagerly recompute a derived like
+	// `{#if foo}{foo.bar()}{/if}` if `foo` is now `undefined
+	if ((effect.f & CLEAN) === 0) {
+		set_signal_status(effect, DIRTY);
+		Batch.ensure().schedule(effect); // Assumption: This happens during the commit phase of the batch, causing another flush, but it's safe
 	}
 
 	var child = effect.first;

packages/svelte/tests/runtime-runes/samples/transition-if-nested-access-property/_config.js

@@ -0,0 +1,14 @@
+import { tick } from 'svelte';
+import { test } from '../../test';
+import { raf } from '../../../animation-helpers';
+
+export default test({
+	async test({ assert, target }) {
+		const [btn] = target.querySelectorAll('button');
+
+		btn.click();
+		await tick();
+		raf.tick(100);
+		assert.htmlEqual(target.innerHTML, `<button>clear</button>`);
+	}
+});

packages/svelte/tests/runtime-runes/samples/transition-if-nested-access-property/main.svelte

@@ -0,0 +1,17 @@
+<script>
+	import { fade } from 'svelte/transition';
+
+	let data = $state({ id: 1 });
+</script>
+
+<button onclick={() => (data = null)}>clear</button>
+
+{#if data}
+	{#key data?.id}
+		<p transition:fade|global={{ duration: 100 }}>keyed</p>
+	{/key}
+
+	{#if data.id}
+		<p>sibling</p>
+	{/if}
+{/if}