[PR #215] [MERGED] fix: standardize TextNode remove method to use ID-based lookup to resolve "Child not found in children" error #1154

New issue

Closed

opened 2026-03-14 09:21:19 +03:00 by kerem · 0 comments

kerem commented

2026-03-14 09:21:19 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/anomalyco/opentui/pull/215
Author: @eli0shin
Created: 10/12/2025
Status: ✅ Merged
Merged: 10/12/2025
Merged by: @kommander

Base: main ← Head: use-id-for-text-remove

📝 Commits (3)

b4e3d3c refactor: standardize TextNode remove method to use ID-based lookup
6830dee remove redundant children iteration
4bae25b Merge branch 'main' into use-id-for-text-remove

📊 Changes

4 files changed (+15 additions, -22 deletions)

View changed files

📝 packages/core/src/renderables/Text.ts (+2 -5)
📝 packages/core/src/renderables/TextNode.test.ts (+3 -3)
📝 packages/core/src/renderables/TextNode.ts (+9 -5)
📝 packages/solid/src/reconciler.ts (+1 -9)

📄 Description

There is an architectural mismatch in the class hierarchy:

BaseRenderable declares abstract remove(id: string): void - expecting ID-based lookup
Renderable (which most components inherit from) implements this correctly:
- Uses renderableMapById.get(id) to look up child by ID
- Properly removes it from layout and internal data structures
TextNodeRenderable (which SpanRenderable inherits from) implements it incorrectly:
- Signature: remove(child: string | TextNodeRenderable): this
- Uses indexOf(child) to find the child - NO ID LOOKUP
- Expects a direct child reference or string literal

For example, this code works:

<box>
  {
    showTextA ?
      <text><span>text a</span></text> :
      <text><span>text b</span></text>
  }
</box>

But this code throws the error because the child of <text> is conditional

<box>
   <text>
    {
      showTextA ?
        <span>text a</span> :
        <span>text b</span>
    }
  </text>
</box>

The solid reconciler passes node reference directly for TextNodeRenderables and passes it to remove but the react one does not, which raises the question: Why are TextNodeRenderables different than other renderables? Why do they use remove by reference instead of by id?

This PR aligns TextNodeRenderable with other renderables and updates the solid reconciler to pass children to remove by id.

Resolves https://github.com/sst/opentui/issues/214

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/anomalyco/opentui/pull/215 **Author:** [@eli0shin](https://github.com/eli0shin) **Created:** 10/12/2025 **Status:** ✅ Merged **Merged:** 10/12/2025 **Merged by:** [@kommander](https://github.com/kommander) **Base:** `main` ← **Head:** `use-id-for-text-remove` --- ### 📝 Commits (3) - [`b4e3d3c`](https://github.com/anomalyco/opentui/commit/b4e3d3c7fa4f43a6c2820b50dba3bf7cb90cf4d6) refactor: standardize TextNode remove method to use ID-based lookup - [`6830dee`](https://github.com/anomalyco/opentui/commit/6830dee87cd804647f6017da650aab2d238c1bb7) remove redundant children iteration - [`4bae25b`](https://github.com/anomalyco/opentui/commit/4bae25bcba169296371f4b638c0ad4f2438935a1) Merge branch 'main' into use-id-for-text-remove ### 📊 Changes **4 files changed** (+15 additions, -22 deletions) <details> <summary>View changed files</summary> 📝 `packages/core/src/renderables/Text.ts` (+2 -5) 📝 `packages/core/src/renderables/TextNode.test.ts` (+3 -3) 📝 `packages/core/src/renderables/TextNode.ts` (+9 -5) 📝 `packages/solid/src/reconciler.ts` (+1 -9) </details> ### 📄 Description There is an architectural mismatch in the class hierarchy: 1. **BaseRenderable** declares `abstract remove(id: string): void` - expecting ID-based lookup 2. **Renderable** (which most components inherit from) implements this correctly: - Uses `renderableMapById.get(id)` to look up child by ID - Properly removes it from layout and internal data structures 3. **TextNodeRenderable** (which `SpanRenderable` inherits from) implements it incorrectly: - Signature: `remove(child: string | TextNodeRenderable): this` - Uses `indexOf(child)` to find the child - NO ID LOOKUP - Expects a direct child reference or string literal For example, this code works: ```javascript <box> { showTextA ? <text>text a</text> : <text>text b</text> } </box> ``` But this code throws the error because the child of `<text>` is conditional ```javascript <box> <text> { showTextA ? text a : text b } </text> </box> ``` The solid reconciler [passes node reference directly for TextNodeRenderables](https://github.com/sst/opentui/blob/7e53ea49e6f65040c0673b2fb8682b9602ddd1e9/packages/solid/src/reconciler.ts#L121) and passes it to `remove` [but the react one does not](https://github.com/sst/opentui/blob/7e53ea49e6f65040c0673b2fb8682b9602ddd1e9/packages/react/src/reconciler/host-config.ts#L59-L61), which raises the question: Why are `TextNodeRenderable`s different than other renderables? Why do they use remove by reference instead of by id? This PR aligns TextNodeRenderable with other renderables and updates the solid reconciler to pass children to remove by id. Resolves https://github.com/sst/opentui/issues/214 --- 🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.