From 8ec0082307f09c0560d6cd2ccbf435a23b034acb Mon Sep 17 00:00:00 2001 From: Simon Klee Date: Thu, 1 Oct 2026 13:17:37 +0200 Subject: [PATCH] rendering: free slot placeholders and cached internal views Solid layout placeholders inherited a no-op destroyRecursively(), so removed or unmounted placeholders never freed their Yoga nodes. Destroy them on that path, free placeholders a host rejects, and make the slot forget destroyed placeholders so a reinserted slot never reuses a freed node. LineNumberRenderable now frees its gutter in clearTarget() and destroy(), which previously threw. DiffRenderable frees views it cached while detached and the default syntax style it creates. --- packages/core/src/renderables/Diff.test.ts | 50 ++++++++++++----- packages/core/src/renderables/Diff.ts | 24 ++++++-- .../src/renderables/LineNumberRenderable.ts | 35 +++--------- .../__tests__/LineNumberRenderable.test.ts | 21 +++++++ packages/solid/src/elements/slot.ts | 23 +++++++- packages/solid/src/reconciler.ts | 29 +++++----- packages/solid/tests/control-flow.test.tsx | 23 +++++++- packages/solid/tests/dynamic-portal.test.tsx | 36 +++++++++++- packages/solid/tests/line-number.test.tsx | 29 +++++++++- .../solid/tests/slot-reconciler-move.test.ts | 55 +++++++++++++++++-- 10 files changed, 255 insertions(+), 70 deletions(-) diff --git a/packages/core/src/renderables/Diff.test.ts b/packages/core/src/renderables/Diff.test.ts index 14c8fe5f..894420fc 100644 --- a/packages/core/src/renderables/Diff.test.ts +++ b/packages/core/src/renderables/Diff.test.ts @@ -2470,7 +2470,26 @@ test("DiffRenderable - gutter remains in correct position after updates", async } }) -test("DiffRenderable - properly cleans up listeners on destroy", async () => { +test("DiffRenderable - releases default syntax styles after replacing them with a borrowed style", () => { + const diff = new DiffRenderable(currentRenderer, { diff: simpleDiff, view: "split" }) + currentRenderer.root.add(diff) + const defaults = [diff["leftCodeRenderable"]!.syntaxStyle, diff["rightCodeRenderable"]!.syntaxStyle] + const borrowed = SyntaxStyle.create() + + try { + diff.syntaxStyle = borrowed + diff.destroyRecursively() + + for (const style of defaults) expect(() => style.getStyleCount()).toThrow("destroyed") + expect(borrowed.getStyleCount()).toBe(0) + } finally { + diff.destroyRecursively() + for (const style of defaults) style.destroy() + borrowed.destroy() + } +}) + +test.each(["destroy", "destroyRecursively"] as const)("DiffRenderable - %s frees cached views", async (cleanup) => { const syntaxStyle = SyntaxStyle.fromStyles({ default: { fg: RGBA.fromValues(1, 1, 1, 1) }, }) @@ -2503,19 +2522,24 @@ test("DiffRenderable - properly cleans up listeners on destroy", async () => { expect(leftCountBeforeDestroy).toBeGreaterThan(0) expect(rightCountBeforeDestroy).toBeGreaterThan(0) - // Destroy the diff - diffRenderable.destroyRecursively() + const sides = diffRenderable.getChildren() + diffRenderable.diff = "--- a/test.js\n+++ b/test.js\n@@ -a,b +c,d @@\n invalid" + await renderOnce() + const errorNodes = diffRenderable.getChildren() + expect(errorNodes).toHaveLength(2) + diffRenderable.diff = simpleDiff + diffRenderable.view = "unified" + await renderOnce() - // The LineNumberRenderables should have been destroyed - // Check that they're either null or destroyed - const leftSide = (diffRenderable as any).leftSide - const rightSide = (diffRenderable as any).rightSide - - if (leftSide) { - expect(leftSide.isDestroyed).toBe(true) - } - if (rightSide) { - expect(rightSide.isDestroyed).toBe(true) + try { + diffRenderable[cleanup]() + for (const node of [...sides, ...errorNodes, leftCodeRenderable, rightCodeRenderable]) { + expect(node.getLayoutNode().isFreed()).toBe(true) + expect(node.listenerCount("line-info-change")).toBe(0) + } + } finally { + for (const node of [...sides, ...errorNodes]) node.destroyRecursively() + syntaxStyle.destroy() } }) diff --git a/packages/core/src/renderables/Diff.ts b/packages/core/src/renderables/Diff.ts index 21b768cd..35f4af7d 100644 --- a/packages/core/src/renderables/Diff.ts +++ b/packages/core/src/renderables/Diff.ts @@ -66,6 +66,7 @@ export class DiffRenderable extends Renderable { private _fg?: RGBA private _filetype?: string private _syntaxStyle?: SyntaxStyle + private _defaultSyntaxStyle?: SyntaxStyle private _wrapMode?: "word" | "char" | "none" private _conceal: boolean private _selectionBg?: RGBA @@ -299,12 +300,27 @@ export class DiffRenderable extends Renderable { this._lineInfoChangeHandler = null } - public override destroyRecursively(): void { + protected override destroySelf(): void { this.detachLineInfoListeners() this.pendingRebuild = false this.leftSideAdded = false this.rightSideAdded = false - super.destroyRecursively() + for (const node of [this.leftSide, this.rightSide, this.errorTextRenderable, this.errorCodeRenderable]) { + node?.destroyRecursively() + } + this.leftSide = null + this.rightSide = null + this.leftCodeRenderable = null + this.rightCodeRenderable = null + this.errorTextRenderable = null + this.errorCodeRenderable = null + this._defaultSyntaxStyle?.destroy() + this._defaultSyntaxStyle = undefined + super.destroySelf() + } + + private getCodeSyntaxStyle(): SyntaxStyle { + return this._syntaxStyle ?? (this._defaultSyntaxStyle ??= SyntaxStyle.create()) } private buildErrorView(): void { @@ -342,7 +358,7 @@ export class DiffRenderable extends Renderable { id: this.id ? `${this.id}-error-code` : undefined, content: this._diff, filetype: "diff", - syntaxStyle: this._syntaxStyle ?? SyntaxStyle.create(), + syntaxStyle: this.getCodeSyntaxStyle(), wrapMode: this._wrapMode, conceal: this._conceal, width: "100%", @@ -379,7 +395,7 @@ export class DiffRenderable extends Renderable { filetype: this._filetype, wrapMode, conceal: this._conceal, - syntaxStyle: this._syntaxStyle ?? SyntaxStyle.create(), + syntaxStyle: this.getCodeSyntaxStyle(), width: "100%", height: "100%", ...(this._fg !== undefined && { fg: this._fg }), diff --git a/packages/core/src/renderables/LineNumberRenderable.ts b/packages/core/src/renderables/LineNumberRenderable.ts index 2b8a5068..fadb5eea 100644 --- a/packages/core/src/renderables/LineNumberRenderable.ts +++ b/packages/core/src/renderables/LineNumberRenderable.ts @@ -375,7 +375,6 @@ export class LineNumberRenderable extends Renderable { private _lineNumberOffset: number private _hideLineNumbers: Set private _lineNumbers: Map - private _isDestroying: boolean = false private handleLineInfoChange = (): void => { // When line info changes in the target, remeasure the gutter this.gutter?.remeasure() @@ -444,17 +443,7 @@ export class LineNumberRenderable extends Renderable { private setTarget(target: Renderable & LineInfoProvider): void { if (this.target === target) return - if (this.target) { - // Remove event listener from old target - this.target.off("line-info-change", this.handleLineInfoChange) - super.remove(this.target) - } - - if (this.gutter) { - super.remove(this.gutter) - this.gutter = null - } - + this.clearTarget() this.target = target // Listen for line info changes from target @@ -497,11 +486,6 @@ export class LineNumberRenderable extends Renderable { // Override remove to prevent removing gutter/target directly public override remove(child: BaseRenderable): void { - if (this._isDestroying) { - super.remove(child) - return - } - if (this.gutter && child === this.gutter) { throw new Error("LineNumberRenderable: Cannot remove gutter directly.") } @@ -511,18 +495,16 @@ export class LineNumberRenderable extends Renderable { super.remove(child) } - // Override destroyRecursively to properly clean up internal components public override destroyRecursively(): void { - this._isDestroying = true - - if (this.target) { - this.target.off("line-info-change", this.handleLineInfoChange) - } - + const target = this.target + this.clearTarget() + target?.destroyRecursively() super.destroyRecursively() + } - this.gutter = null - this.target = null + public override destroy(): void { + this.clearTarget() + super.destroy() } public clearTarget(): void { @@ -533,6 +515,7 @@ export class LineNumberRenderable extends Renderable { } if (this.gutter) { super.remove(this.gutter) + this.gutter.destroy() this.gutter = null } } diff --git a/packages/core/src/renderables/__tests__/LineNumberRenderable.test.ts b/packages/core/src/renderables/__tests__/LineNumberRenderable.test.ts index 93e9a7c0..1a23c706 100644 --- a/packages/core/src/renderables/__tests__/LineNumberRenderable.test.ts +++ b/packages/core/src/renderables/__tests__/LineNumberRenderable.test.ts @@ -110,6 +110,27 @@ afterEach(() => { }) describe("LineNumberRenderable", () => { + test.each(["clearTarget", "destroy"] as const)("%s frees the gutter and preserves the target", async (cleanup) => { + const { renderer } = await createTestRenderer({ width: 20, height: 5 }) + const target = new MockTextBuffer(renderer, { text: "content" }) + const numbers = new LineNumberRenderable(renderer, { target }) + renderer.root.add(numbers) + const gutter = numbers.getChildren()[0]! + + try { + numbers[cleanup]() + + expect(numbers.getChildren()).toHaveLength(0) + expect(target.parent).toBeNull() + expect(target.isDestroyed).toBe(false) + expect(target.listenerCount("line-info-change")).toBe(0) + expect(gutter.getLayoutNode().isFreed()).toBe(true) + } finally { + gutter.destroy() + target.destroy() + } + }) + test("renders line numbers correctly", async () => { const { renderer, renderOnce, captureCharFrame } = await createTestRenderer({ width: 20, diff --git a/packages/solid/src/elements/slot.ts b/packages/solid/src/elements/slot.ts index 9188ca50..a97e792e 100644 --- a/packages/solid/src/elements/slot.ts +++ b/packages/solid/src/elements/slot.ts @@ -83,8 +83,8 @@ export class TextSlotRenderable extends TextNodeRenderable { const slotParent = this.slotParent this.slotParent = undefined - slotParent?.destroy() - super.destroy() + this.parent?.remove(this) + slotParent?.didDestroySlotChild(this) } } @@ -145,6 +145,11 @@ export class LayoutSlotRenderable extends SlotBaseRenderable { this.freeYogaNode() } + // Parents and the reconciler release children through destroyRecursively(). + public override destroyRecursively(): void { + this.destroy() + } + public override destroy(): void { if (this.destroyed) { return @@ -154,8 +159,9 @@ export class LayoutSlotRenderable extends SlotBaseRenderable { const slotParent = this.slotParent this.slotParent = undefined + this.parent?.remove(this) this.freeYogaNode() - slotParent?.destroy() + slotParent?.didDestroySlotChild(this) } } @@ -372,11 +378,22 @@ export class SlotRenderable extends SlotBaseRenderable { } } + didDestroySlotChild(child: LayoutSlotRenderable | TextSlotRenderable): void { + const nodesByParent: Map = + child instanceof LayoutSlotRenderable ? this.layoutNodesByParent : this.textNodesByParent + for (const [parent, node] of nodesByParent) { + if (node !== child) continue + nodesByParent.delete(parent) + if (this.parent === parent) this.parent = this.getAttachedSlotParent() + } + } + public override destroy(): void { if (this.destroyed) { return } this.destroyed = true + this.parent = null const layoutNodes = new Set(this.layoutNodesByParent.values()) this.layoutNodesByParent.clear() diff --git a/packages/solid/src/reconciler.ts b/packages/solid/src/reconciler.ts index ffa87415..83620772 100644 --- a/packages/solid/src/reconciler.ts +++ b/packages/solid/src/reconciler.ts @@ -68,13 +68,14 @@ function _insertNode(parent: DomNode, node: DomNode, anchor?: DomNode): void { node instanceof TextNode, ) - if (node instanceof SlotRenderable) { - node.parent = parent - node = node.getSlotChild(parent) + const slot = node instanceof SlotRenderable ? node : undefined + if (slot) { + slot.parent = parent + node = slot.getSlotChild(parent) } if (anchor && anchor instanceof SlotRenderable) { - anchor = anchor.getSlotChild(parent) + anchor = anchor.getSlotChildForRemoval(parent) } if (isTextNodeRenderable(node)) { @@ -95,19 +96,19 @@ function _insertNode(parent: DomNode, node: DomNode, anchor?: DomNode): void { throw new Error("Tried to mount a non base renderable") } - if (!anchor) { - parent.add(node) - return - } - - const children = getNodeChildren(parent) - - const anchorIndex = children.indexOf(anchor) - if (anchorIndex === -1) { - log("[INSERT]", "Could not find anchor", logId(parent), logId(anchor), "[children]", ...children.map((c) => c.id)) + let anchorIndex: number | undefined + if (anchor) { + const children = getNodeChildren(parent) + anchorIndex = children.indexOf(anchor) + if (anchorIndex === -1) { + log("[INSERT]", "Could not find anchor", logId(parent), logId(anchor), "[children]", ...children.map((c) => c.id)) + } } parent.add(node, anchorIndex) + + // Hosts such as LineNumberRenderable ignore children they do not render. + if (slot && !node.parent) node.destroy() } function _removeNode(parent: DomNode, node: DomNode): void { diff --git a/packages/solid/tests/control-flow.test.tsx b/packages/solid/tests/control-flow.test.tsx index d1a8d011..eb9d8651 100644 --- a/packages/solid/tests/control-flow.test.tsx +++ b/packages/solid/tests/control-flow.test.tsx @@ -1,5 +1,5 @@ import { describe, expect, it, beforeEach, afterEach, test } from "bun:test" -import { testRender } from "../index.js" +import { LayoutSlotRenderable, testRender } from "../index.js" import { createSignal, createEffect, createMemo, For, Show, Switch, Match, Index, ErrorBoundary } from "solid-js" let testSetup: Awaited> @@ -215,7 +215,7 @@ describe("SolidJS Renderer - Control Flow Components", () => { expect(frame).not.toContain("Count is high") }) - it("should handle without fallback", async () => { + it.each(["replacement", "renderer destruction"])("should handle without fallback (%s)", async (cleanup) => { const [visible, setVisible] = createSignal(true) testSetup = await testRender( @@ -247,6 +247,25 @@ describe("SolidJS Renderer - Control Flow Components", () => { frame = testSetup.captureCharFrame() expect(frame).not.toContain("Visible content") expect(frame).toContain("Always visible") + + const placeholder = children.find((child) => child instanceof LayoutSlotRenderable)! + try { + expect(placeholder).toBeInstanceOf(LayoutSlotRenderable) + expect(placeholder.getLayoutNode().isFreed()).toBe(false) + + if (cleanup === "replacement") { + setVisible(true) + } else { + testSetup.renderer.destroy() + } + await new Promise((resolve) => process.nextTick(resolve)) + + expect(placeholder.parent).toBeNull() + expect(placeholder.getLayoutNode().isFreed()).toBe(true) + } finally { + testSetup.renderer.destroy() + placeholder?.destroy() + } }) it("should conditionally render content with in the correct order", async () => { diff --git a/packages/solid/tests/dynamic-portal.test.tsx b/packages/solid/tests/dynamic-portal.test.tsx index ee052eb5..971898e5 100644 --- a/packages/solid/tests/dynamic-portal.test.tsx +++ b/packages/solid/tests/dynamic-portal.test.tsx @@ -1,5 +1,5 @@ import { describe, expect, it, beforeEach, afterEach } from "bun:test" -import { testRender, Dynamic, Portal } from "../index.js" +import { testRender, Dynamic, LayoutSlotRenderable, Portal } from "../index.js" import { createSignal, Show } from "solid-js" import { createSpy } from "@opentui/core/testing" import type { BoxRenderable } from "@opentui/core" @@ -193,6 +193,40 @@ describe("SolidJS Renderer - Dynamic and Portal Components", () => { expect(frame).not.toContain("Portal content") }) + it("should give a reinserted portal marker a live placeholder", async () => { + const [showPortal, setShowPortal] = createSignal(true) + const nextTick = () => new Promise((resolve) => process.nextTick(resolve)) + let host!: BoxRenderable + + testSetup = await testRender( + () => { + const portal = ( + + Portal content + + ) + return ( + + {showPortal() ? portal : null} + After portal + + ) + }, + { width: 20, height: 5 }, + ) + + for (let i = 0; i < 3; i++) { + setShowPortal(false) + await nextTick() + setShowPortal(true) + await nextTick() + + const placeholder = host.getChildren()[0] + expect(placeholder).toBeInstanceOf(LayoutSlotRenderable) + expect(placeholder!.getLayoutNode().isFreed()).toBe(false) + } + }) + it("should handle multiple portals", async () => { testSetup = await testRender( () => ( diff --git a/packages/solid/tests/line-number.test.tsx b/packages/solid/tests/line-number.test.tsx index 5980fa0d..c31ea738 100644 --- a/packages/solid/tests/line-number.test.tsx +++ b/packages/solid/tests/line-number.test.tsx @@ -1,6 +1,6 @@ -import { describe, test, expect, beforeEach, afterEach } from "bun:test" +import { describe, test, expect, beforeEach, afterEach, spyOn } from "bun:test" import { testRender } from "../index.js" -import { SyntaxStyle } from "@opentui/core" +import { SyntaxStyle, Yoga } from "@opentui/core" import { MockTreeSitterClient } from "@opentui/core/testing" import { createSignal, Show } from "solid-js" @@ -74,6 +74,31 @@ console.log(test())` expect(frame).toContain(" 4 ") // Line number 4 }) + test("frees ignored conditional placeholders", async () => { + const createNode = spyOn(Yoga.default.Node, "create") + try { + testSetup = await testRender(() => ( + + + inactive + + target + + )) + await testSetup.renderOnce() + expect(testSetup.captureCharFrame()).toContain("target") + expect(createNode).toHaveBeenCalled() + + testSetup.renderer.destroy() + await new Promise((resolve) => process.nextTick(resolve)) + for (const { value } of createNode.mock.results) expect(value.isFreed()).toBe(true) + } finally { + testSetup?.renderer.destroy() + for (const { value } of createNode.mock.results) value.free() + createNode.mockRestore() + } + }) + test("handles conditional removal of line number element", async () => { const syntaxStyle = SyntaxStyle.fromStyles({ keyword: { fg: "#C792EA" }, diff --git a/packages/solid/tests/slot-reconciler-move.test.ts b/packages/solid/tests/slot-reconciler-move.test.ts index 3aa740ca..77fd1594 100644 --- a/packages/solid/tests/slot-reconciler-move.test.ts +++ b/packages/solid/tests/slot-reconciler-move.test.ts @@ -1,8 +1,9 @@ import { describe, expect, it } from "bun:test" -import { BoxRenderable, Yoga } from "@opentui/core" +import { BoxRenderable, LineNumberRenderable, TextRenderable, Yoga } from "@opentui/core" import { createTestRenderer } from "@opentui/core/testing" import { batch, createRoot, createSignal } from "solid-js" -import { createSlotNode, insert } from "../index.js" +import { createSlotNode, insert, insertNode } from "../index.js" +import { LayoutSlotRenderable } from "../src/elements/slot.js" type MoveOrder = "remove-then-insert" | "insert-then-remove" @@ -86,6 +87,38 @@ async function runMoveScenario(order: MoveOrder) { } describe("slot placeholder moves", () => { + it.each([false, true])("keeps rejected slots reusable (existing host: %p)", async (attached) => { + const setup = await createTestRenderer({ width: 20, height: 5 }) + const rejecting = new LineNumberRenderable(setup.renderer, {}) + setup.renderer.root.add(rejecting) + const slot = createSlotNode() + + try { + if (attached) insertNode(setup.renderer.root, slot) + const rejected = slot.getSlotChild(rejecting) + if (!(rejected instanceof LayoutSlotRenderable)) throw new Error("Expected layout placeholder") + const yoga = rejected.getLayoutNode() + insertNode(rejecting, slot) + + expect(yoga.isFreed()).toBe(true) + expect(slot.getSlotChildForRemoval(rejecting)).toBeUndefined() + + insertNode(rejecting, new TextRenderable(setup.renderer, { content: "target" }), slot) + expect(slot.getSlotChildForRemoval(rejecting)).toBeUndefined() + + insertNode(setup.renderer.root, slot) + const accepted = slot.getSlotChild(setup.renderer.root) + if (!(accepted instanceof LayoutSlotRenderable)) throw new Error("Expected layout placeholder") + expect(accepted.parent).toBe(setup.renderer.root) + expect(accepted.getLayoutNode().isFreed()).toBe(false) + setup.renderer.destroy() + expect(accepted.getLayoutNode().isFreed()).toBe(true) + } finally { + setup.renderer.destroy() + slot.destroy() + } + }) + it("recreates incompatible layout placeholders for remove-then-insert moves", async () => { const { controls, movedChild, originalChild, parentA, parentB, setup, slot } = await runMoveScenario("remove-then-insert") @@ -122,7 +155,7 @@ describe("slot placeholder moves", () => { } }) - it("promotes slot.parent back to another attached host when the newest placeholder is removed", async () => { + it.each(["remove", "destroy subtree"])("preserves the other attached slot host on %s", async (cleanup) => { const setup = await createTestRenderer({ width: 40, height: 10 }) const parentA = new BoxRenderable(setup.renderer, { id: "slot-parent-host-a", @@ -144,21 +177,33 @@ describe("slot placeholder moves", () => { slot.parent = parentA const childA = slot.getSlotChild(parentA) parentA.add(childA) + const yogaA = parentA.getLayoutNode().getChild(0)! slot.parent = parentB const childB = slot.getSlotChild(parentB) parentB.add(childB) + const yogaB = parentB.getLayoutNode().getChild(0)! expect(slot.parent).toBe(parentB) - parentB.remove(childB) - slot.didRemoveSlotChild(parentB, childB) + if (cleanup === "remove") { + parentB.remove(childB) + slot.didRemoveSlotChild(parentB, childB) + } else { + parentB.destroyRecursively() + } expect(slot.parent).toBe(parentA) expect(parentA.getChildren()[0]).toBe(childA) expect(parentB.getChildren()).toHaveLength(0) + expect(yogaA.isFreed()).toBe(false) + expect(yogaB.isFreed()).toBe(true) + + setup.renderer.destroy() + expect(yogaA.isFreed()).toBe(true) } finally { setup.renderer.destroy() + slot.destroy() } }) })