Skip to content

fix: support nested StaticRender objects and optimize StaticRender API to ensure elements are really removed from the DOM.#99

Open
jacob314 wants to merge 3 commits into
masterfrom
branch_nested_static2
Open

fix: support nested StaticRender objects and optimize StaticRender API to ensure elements are really removed from the DOM.#99
jacob314 wants to merge 3 commits into
masterfrom
branch_nested_static2

Conversation

@jacob314

@jacob314 jacob314 commented Apr 8, 2026

Copy link
Copy Markdown
Owner

I tried multiple ways to optimize this and this one appears to be have been the most robust while performant.

I was also tempted to create a version that aligned a bit more when Ink's existing rendering support
but performance was poor when I tried to implement that way. I'm not exactly clear why it didn't work well but performance
was bad using the nestedStatic example to test performance. Starting up that example took multiple seconds while it is
basically instant with this approach.

Nested static items isn't a truly crucial feature but is a good test to make sure the static rendering is robust.

The most useful part of this change is switching so we fully evict statically rendered items from the virtual DOM. We weren't
doing enough of that previously resulting in performance struggles seen in GeminiCLI handling a DOM with a large number of statically rendered elements.

Moved StaticRender's layout evaluation to prepareYogaTree inside ink.tsx. This avoids repeatedly evaluating static subtrees during the main layout loop, treating nested and cached StaticRender components as efficient, fixed-size leaf nodes.

This fixes an issue where nested scrollable regions with overflowToBackbuffer=true would not render their newly revealed lines into the backbuffer because the root region artificially clipped the rendering canvas to the terminal height. By passing overrideHeight and isExpanded=true to the root region composeNode call during getLinesForScroll, the scrollable region receives the expanded context and correctly renders its off-screen lines, which are then successfully captured and written to the terminal history.

Tests to reproduce issues.

Comment thread src/renderer.ts
trackSelection,
} = options;

const callBeforeRender = (n: DOMElement) => {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was no longer needed

refactor: simplify StaticRender layout computation

Moved StaticRender's layout evaluation to prepareYogaTree inside ink.tsx. This avoids repeatedly evaluating static subtrees during the main layout loop, treating nested and cached StaticRender components as efficient, fixed-size leaf nodes.

fix: address PR review feedback

fix: lint errors

chore: update selection snapshot

refactor: Update StaticRender to take a callback for children

Tweak example.

fix(worker): expand root region when rendering backbuffer lines

This fixes an issue where nested scrollable regions with overflowToBackbuffer=true would not render their newly revealed lines into the backbuffer because the root region artificially clipped the rendering canvas to the terminal height. By passing overrideHeight and isExpanded=true to the root region composeNode call during getLinesForScroll, the scrollable region receives the expanded context and correctly renders its off-screen lines, which are then successfully captured and written to the terminal history.

Tests to reproduce issues.
@jacob314

jacob314 commented Apr 8, 2026

Copy link
Copy Markdown
Owner Author

Overall, this is an excellent PR. Evaluating the layout for the static subtree inside prepareYogaTree and treating it as a fixed-size leaf node in the main layout loop is a fantastic optimization. Fully evicting statically rendered items from the virtual DOM via the isRendered state is an elegant way to reduce memory overhead and significantly improves performance.

Here are a few points to consider:

1. Breaking API Change

Changing children from ReactNode to () => ReactNode on <StaticRender> is a breaking API change.

  • If you intend for this to be a breaking change, please consider appending a ! to the commit type in the PR title (e.g., fix!: support nested StaticRender objects...) or adding a BREAKING CHANGE: footer to the commit message to adhere strictly to Conventional Commits.
  • Alternative Backward Compatibility: You could support both patterns to avoid breaking existing code, while still encouraging the new pattern for performance:
    export type Props = {
        readonly children: ReactNode | (() => ReactNode);
        // ...
    };
    // ...
    {isRendered ? null : (typeof children === 'function' ? children() : children)}

2. Potential Memory Leak in Region.node

The cached Region object retains a reference to the original DOM node (rootRegion.node). While the isRendered state successfully drops the React elements, just ensure that Region.node isn't inadvertently keeping a detached DOM tree in memory. Since the ink-static-render DOM element itself remains mounted, this is likely fine, but worth a quick double-check.

3. Excellent isExpanded Propagation

The logic introduced in composeNode to propagate the isExpanded state via inExpandedContext && Boolean(childRegion?.overflowToBackbuffer) is robust. It correctly ensures that only the specific scrollable regions with overflowToBackbuffer activated are expanded and captured for terminal history.

Great work on this! The tests and snapshots are looking solid and correctly follow the repository's testing standards (using SVG snapshots and headless xterm where appropriate).

@jacob314 jacob314 force-pushed the branch_nested_static2 branch from 17618a7 to 2287716 Compare April 8, 2026 07:19
Update cachedStickyHeaders mapping in render-node-to-output to set 'node: undefined' to drop the retained reference to the DOM element after computing the sticky header layout cache. Update all usages to handle the optional stickyNode property and resolve max-params typescript errors.
prisis added a commit to visulima/visulima that referenced this pull request Apr 8, 2026
…nk#99

Change StaticRender children from ReactNode to () => ReactNode so React
stops reconciling the subtree once the cached render is ready. Add
internal_onRendered callback on DOMElement, invoked by prepareYogaTree()
after caching, which sets isRendered state to skip children on subsequent
renders.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add instruction to explicitly avoid destructured objects for resolving max-params linting errors in hot-path rendering code, instead preferring to ignore the linter rule due to GC allocation overhead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant