From e686903170b350c5076caf4bac2a872a29338dac Mon Sep 17 00:00:00 2001 From: Aaron Sinclair Date: Sat, 15 Nov 2025 10:32:40 -0800 Subject: [PATCH] Fix reactive conditional rendering with proper branch swapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements efficient reactive conditionals that pre-render branches once and swap DOM nodes without recreation, enabling fast switching with thousands of elements. Key Fixes: - Runtime now calls reevaluate_conditional_*() on signal changes - Match/case swap logic uses phi nodes to load correct old case - Previously hardcoded case_0 as old value, causing accumulation - Branch render functions now wrap all content in container div Implementation: - Pre-render both if/else branches at init - Store roots in globals (@cond_N_then_root, @cond_N_else_root) - On signal change, swap DOM nodes via js_replace_at_marker() - Match statements use switch/phi pattern to select old case Tests: - Added test_reactive_conditionals.py (5 new tests) - Updated test_llvm_codegen_conditional.py for reactive infrastructure - All 35 tests passing Performance: No re-rendering on switches, perfect for dynamic content. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- examples/runtime/ritz-runtime.js | 52 ++++++-- ritz/llvm_codegen_signals.py | 73 +++++++---- tests/test_llvm_codegen_conditional.py | 19 +-- tests/test_reactive_conditionals.py | 169 +++++++++++++++++++++++++ 4 files changed, 272 insertions(+), 41 deletions(-) create mode 100644 tests/test_reactive_conditionals.py diff --git a/examples/runtime/ritz-runtime.js b/examples/runtime/ritz-runtime.js index 4292517..3a23d1b 100644 --- a/examples/runtime/ritz-runtime.js +++ b/examples/runtime/ritz-runtime.js @@ -115,6 +115,22 @@ class RitzRuntime { this.updateSignalDisplay(signalName, value); } } + + // Re-evaluate all reactive conditionals + for (const [name, fn] of Object.entries(exports)) { + if (name.startsWith('reevaluate_conditional_') && typeof fn === 'function') { + fn(); + this._log(`[Reactive] ${name}()`); + } + } + + // Re-evaluate all reactive match statements + for (const [name, fn] of Object.entries(exports)) { + if (name.startsWith('reevaluate_match_') && typeof fn === 'function') { + fn(); + this._log(`[Reactive] ${name}()`); + } + } } updateSignalDisplay(signalName, value) { @@ -154,22 +170,33 @@ class RitzRuntime { // Replace DOM node at marker position const marker = this.nodes.get(markerId); if (!marker) { - console.error(`Marker ${markerId} not found`); + console.error(`[replace_at_marker] Marker ${markerId} not found`); return; } const parent = marker.parentNode; if (!parent) { - console.error(`Marker has no parent`); + console.error(`[replace_at_marker] Marker ${markerId} has no parent`); return; } - // Remove old node if it exists (0 means no old node) + this._log(`[replace_at_marker] marker=${markerId}, old=${oldNodeId}, new=${newNodeId}`); + + // Remove old node if it exists (0 means no old node to remove) if (oldNodeId !== 0) { const oldNode = this.nodes.get(oldNodeId); - if (oldNode && oldNode.parentNode) { + if (!oldNode) { + console.warn(`[replace_at_marker] Old node ${oldNodeId} not found in registry`); + } else if (!oldNode.parentNode) { + console.warn(`[replace_at_marker] Old node ${oldNodeId} not in DOM`); + } else if (oldNode.parentNode !== parent) { + console.warn(`[replace_at_marker] Old node ${oldNodeId} in wrong parent`); oldNode.parentNode.removeChild(oldNode); - this._log(`Removed old node ${oldNodeId}`); + this._log(`[replace_at_marker] Removed ${oldNodeId} from wrong parent`); + } else { + // Remove from correct parent + parent.removeChild(oldNode); + this._log(`[replace_at_marker] Removed ${oldNodeId} from correct parent`); } } @@ -177,11 +204,20 @@ class RitzRuntime { if (newNodeId !== 0) { const newNode = this.nodes.get(newNodeId); if (!newNode) { - console.error(`New node ${newNodeId} not found`); + console.error(`[replace_at_marker] New node ${newNodeId} not found`); return; } - parent.insertBefore(newNode, marker.nextSibling); - this._log(`Inserted node ${newNodeId} at marker ${markerId}`); + + // Insert right after the marker + // If newNode is already in DOM elsewhere, this will move it + const nextSibling = marker.nextSibling; + if (nextSibling) { + parent.insertBefore(newNode, nextSibling); + this._log(`[replace_at_marker] Inserted ${newNodeId} before sibling`); + } else { + parent.appendChild(newNode); + this._log(`[replace_at_marker] Appended ${newNodeId} (no sibling)`); + } } } diff --git a/ritz/llvm_codegen_signals.py b/ritz/llvm_codegen_signals.py index 627c6db..f53016f 100644 --- a/ritz/llvm_codegen_signals.py +++ b/ritz/llvm_codegen_signals.py @@ -522,21 +522,41 @@ class SignalLLVMIRGenerator: # Swap to this case self.lines.append(f"swap_{case_idx}:") - old_var = self._new_var() - new_var = self._new_var() - marker_var2 = self._new_var() - old_case_var = self._new_var() - # Load the old case root based on current state - # We need to use a phi or switch to load the right old root - # For simplicity, just create a minimal subtree swap + # Load marker and new case root + marker_var2 = self._new_var() + new_var = self._new_var() self.lines.append(f" %{marker_var2} = load i32, i32* @cond_{cond_id}_marker") self.lines.append(f" %{new_var} = load i32, i32* @cond_{cond_id}_case_{case_idx}_root") - # Approximate: load old root based on current value - # This is simplified - ideally we'd use a switch or phi - self.lines.append(f" %{old_var} = load i32, i32* @cond_{cond_id}_case_0_root") + # Use switch to load the correct old case root based on current value + old_var = self._new_var() + switch_labels = [f"load_old_{case_idx}_{i}" for i in range(num_cases)] + if default_idx is not None: + self.lines.append(f" switch i32 %{current_var}, label %{switch_labels[default_idx]} [") + else: + self.lines.append(f" switch i32 %{current_var}, label %{switch_labels[0]} [") + + for i in range(num_cases): + self.lines.append(f" i32 {i}, label %{switch_labels[i]}") + self.lines.append(" ]") + + # Emit load blocks and phi merge + phi_inputs = [] + for i in range(num_cases): + self.lines.append(f"{switch_labels[i]}:") + tmp_var = self._new_var() + self.lines.append(f" %{tmp_var} = load i32, i32* @cond_{cond_id}_case_{i}_root") + phi_inputs.append((tmp_var, switch_labels[i])) + self.lines.append(f" br label %merge_{case_idx}") + + # Merge point with phi to get old_var + self.lines.append(f"merge_{case_idx}:") + phi_operands = ", ".join([f"[ %{var}, %{label} ]" for var, label in phi_inputs]) + self.lines.append(f" %{old_var} = phi i32 {phi_operands}") + + # Now do the actual replacement self.lines.append(f" call void @js_replace_at_marker(i32 %{marker_var2}, i32 %{old_var}, i32 %{new_var})") self.lines.append(f" store i32 {case_idx}, i32* @cond_{cond_id}_current") self.lines.append(" br label %done") @@ -551,27 +571,30 @@ class SignalLLVMIRGenerator: self._emit_branch_render_function(cond_id, f"case_{case_idx}", case.get("body", [])) def _emit_branch_render_function(self, cond_id: int, branch_name: str, body: list[dict[str, Any]]) -> None: - """Emit a function that renders a branch and returns its root element.""" + """Emit a function that renders a branch and returns its root element. + + Wraps all branch content in a container div to handle multiple elements. + """ self.lines.append(f"define i32 @_render_cond_{cond_id}_{branch_name}() {{") self.lines.append("entry:") - # Render branch body - root_var = None + # Create wrapper div for the branch content + wrapper_var = self._new_var() + div_str = self._get_or_create_string("div") + self.lines.append(f" %{wrapper_var} = call i32 @js_create_element(i8* getelementptr inbounds ({self._string_type('div')}, {self._string_type('div')}* {div_str}, i32 0, i32 0))") + + # Push wrapper onto parent stack so child elements append to it + self.parent_element_stack.append(wrapper_var) + + # Render all statements in the branch body for stmt in body: - var = self._emit_statement(stmt) - if root_var is None and var: - root_var = var + self._emit_statement(stmt) - # Return root element - if root_var: - self.lines.append(f" ret i32 %{root_var}") - else: - # Empty branch - create empty div - empty_var = self._new_var() - div_str = self._get_or_create_string("div") - self.lines.append(f" %{empty_var} = call i32 @js_create_element(i8* getelementptr inbounds ({self._string_type('div')}, {self._string_type('div')}* {div_str}, i32 0, i32 0))") - self.lines.append(f" ret i32 %{empty_var}") + # Pop wrapper from parent stack + self.parent_element_stack.pop() + # Return the wrapper element + self.lines.append(f" ret i32 %{wrapper_var}") self.lines.append("}") self.lines.append("") diff --git a/tests/test_llvm_codegen_conditional.py b/tests/test_llvm_codegen_conditional.py index f18bc1c..3fbe71b 100644 --- a/tests/test_llvm_codegen_conditional.py +++ b/tests/test_llvm_codegen_conditional.py @@ -6,7 +6,7 @@ from ritz.llvm_codegen_signals import generate_llvm_ir def test_if_statement_codegen(): - """Test LLVM IR generation for if statement.""" + """Test LLVM IR generation for if statement with reactive rendering.""" source = """ from ritz.syntax import * @@ -27,13 +27,15 @@ def Toggle(): # Should have conditional branch assert "br i1" in llvm_ir - # Should have basic blocks for then/else/merge - assert "if.then." in llvm_ir - assert "if.end." in llvm_ir + # Should have reactive conditional infrastructure + assert "@cond_0_marker" in llvm_ir + assert "_init_conditional_0" in llvm_ir + assert "_render_cond_0_then" in llvm_ir + assert "_render_cond_0_else" in llvm_ir def test_if_else_codegen(): - """Test LLVM IR generation for if/else.""" + """Test LLVM IR generation for if/else with reactive rendering.""" source = """ from ritz.syntax import * @@ -54,9 +56,10 @@ def Toggle(): # Should have conditional branch assert "br i1" in llvm_ir - # Should have both then and else blocks - assert "if.then." in llvm_ir - assert "if.else." in llvm_ir + # Should have reactive infrastructure with both branches + assert "_render_cond_0_then" in llvm_ir + assert "_render_cond_0_else" in llvm_ir + assert "reevaluate_conditional_0" in llvm_ir def test_if_comparison_codegen(): diff --git a/tests/test_reactive_conditionals.py b/tests/test_reactive_conditionals.py new file mode 100644 index 0000000..747ac96 --- /dev/null +++ b/tests/test_reactive_conditionals.py @@ -0,0 +1,169 @@ +"""Tests for reactive conditional rendering with signal updates.""" + +import pytest +from ritz.parser import parse_component +from ritz.llvm_codegen_signals import generate_llvm_ir + + +def test_reactive_conditional_infrastructure(): + """Test that reactive conditional infrastructure is generated.""" + source = """ +from ritz.syntax import * + +@component +def Toggle(): + visible = signal(1) + + if visible: + with P(): + yield "Visible" + else: + with P(): + yield "Hidden" +""" + ir = parse_component(source) + llvm_ir = generate_llvm_ir(ir) + + # Should have marker global + assert "@cond_0_marker = global i32 0" in llvm_ir + + # Should have current state global + assert "@cond_0_current = global i32 0" in llvm_ir + + # Should have root globals for both branches + assert "@cond_0_then_root = global i32 0" in llvm_ir + assert "@cond_0_else_root = global i32 0" in llvm_ir + + # Should have init function + assert "define void @_init_conditional_0()" in llvm_ir + + # Should have reevaluate function (for signal updates) + assert "define void @reevaluate_conditional_0()" in llvm_ir + + # Should have branch render functions + assert "define i32 @_render_cond_0_then()" in llvm_ir + assert "define i32 @_render_cond_0_else()" in llvm_ir + + +def test_reactive_conditional_init_logic(): + """Test that init function pre-renders both branches.""" + source = """ +from ritz.syntax import * + +@component +def Toggle(): + visible = signal(1) + + if visible: + with P(): + yield "Shown" + else: + with P(): + yield "Hidden" +""" + ir = parse_component(source) + llvm_ir = generate_llvm_ir(ir) + + # Init should call both render functions + assert "call i32 @_render_cond_0_then()" in llvm_ir + assert "call i32 @_render_cond_0_else()" in llvm_ir + + # Init should store both roots + assert "store i32" in llvm_ir and "@cond_0_then_root" in llvm_ir + assert "store i32" in llvm_ir and "@cond_0_else_root" in llvm_ir + + # Init should insert active branch at marker + assert "call void @js_replace_at_marker" in llvm_ir + + +def test_reactive_conditional_swap_logic(): + """Test that reevaluate function swaps branches when condition changes.""" + source = """ +from ritz.syntax import * + +@component +def Toggle(): + visible = signal(0) + + if visible: + with P(): + yield "On" + else: + with P(): + yield "Off" +""" + ir = parse_component(source) + llvm_ir = generate_llvm_ir(ir) + + # Reevaluate should re-evaluate the condition + assert "reevaluate_conditional_0" in llvm_ir + + # Should check if condition changed + assert "icmp ne" in llvm_ir + + # Should have swap logic + assert "swap:" in llvm_ir or "swap_to_then:" in llvm_ir + assert "swap_to_then:" in llvm_ir + assert "swap_to_else:" in llvm_ir + + +def test_multiple_reactive_conditionals(): + """Test multiple if statements in same component get unique IDs.""" + source = """ +from ritz.syntax import * + +@component +def MultiToggle(): + flag1 = signal(1) + flag2 = signal(0) + + if flag1: + with P(): + yield "First on" + + if flag2: + with P(): + yield "Second on" +""" + ir = parse_component(source) + llvm_ir = generate_llvm_ir(ir) + + # Should have two separate conditionals + assert "@cond_0_marker" in llvm_ir + assert "@cond_1_marker" in llvm_ir + + # Should have init functions for both + assert "define void @_init_conditional_0()" in llvm_ir + assert "define void @_init_conditional_1()" in llvm_ir + + # Should have reevaluate functions for both + assert "define void @reevaluate_conditional_0()" in llvm_ir + assert "define void @reevaluate_conditional_1()" in llvm_ir + + +def test_reactive_conditional_with_comparison(): + """Test reactive conditional with comparison operator.""" + source = """ +from ritz.syntax import * + +@component +def Counter(): + count = signal(0) + + if count > 5: + with P(): + yield "High" + else: + with P(): + yield "Low" +""" + ir = parse_component(source) + llvm_ir = generate_llvm_ir(ir) + + # Should generate icmp for comparison + assert "icmp" in llvm_ir + assert "sgt" in llvm_ir # signed greater than + + # Should still have reactive infrastructure + assert "@_init_conditional_0()" in llvm_ir + assert "@reevaluate_conditional_0()" in llvm_ir