fix(scroll): correctly calculate absolute Y for nested regions#126
Conversation
jacob314
changed the title
jacob314
force-pushed
the
fix_nested_scroll
branch
3 times, most recently
from
c06d007 to
36717e0
Compare
|
I've reviewed the PR and applied some fixes locally instead to clean things up:
(Note: The Otherwise, the structural changes and fixes in this PR look good! |
jacob314
force-pushed
the
fix_nested_scroll
branch
2 times, most recently
from
386ac29 to
4400482
Compare
more fixes. refactor(sticky): optimize sticky node lookup and remove debug flag - Removed `debugRainbow: true` from `examples/sticky/index.ts` which was an artifact. - Refactored `identifyActiveStickyNodes` in `src/render-sticky.ts` to use `originalIndex` during population, eliminating the O(N^2) `indexOf(find(...))` lookup. - Extracted next sticky node discovery into a `findNextStickyNode` helper to reduce duplication.
jacob314
force-pushed
the
fix_nested_scroll
branch
from
4400482 to
842c373
Compare
jacob314
force-pushed
the
fix_nested_scroll
branch
from
9a20d85 to
e5b5329
Compare
| rows, | ||
| }), | ||
| { | ||
| debugRainbow: true, |
There was a problem hiding this comment.
intentionally added for easier debugging that this sample doesn't introduce inefficient rendering.
| } | ||
| }, [recordFilename, startRecording]); | ||
|
|
||
| const [innerStates, setInnerStates] = useState< |
There was a problem hiding this comment.
tweaks to this sample to reproduce an issue noticed in Gemini CLI
| }); | ||
|
|
||
| for (const region of this.sceneManager.regions.values()) { | ||
| const processRegionForScroll = ( |
There was a problem hiding this comment.
Defining this recursive helper as a closure inside the render() method—which is called on every frame results in unnecessary object allocations and increases garbage collection pressure. Maybe refactor this into a private class method?
| const region = this.sceneManager.getRegion(node.id); | ||
| if (!region) return; | ||
|
|
||
| const absX = Math.round(region.x + offsetX); |
There was a problem hiding this comment.
would it be safer to accumulate the offsets as floats and only apply Math.round() at the final step before usage?
2 participants
This PR resolves an issue where scrolling a nested child component would trigger excessive repaints of the entire screen.
Cause:
Previously,
TerminalBufferWorkercalculated the absolute Y coordinate (absY) of nested scrollable regions by making an incorrect assumption that all regions were positioned directly relative to the root camera.Solution:
The fix introduces a recursive
processRegionForScrollfunction that correctly accumulatesscrollTopandscrollLeftoffsets from all ancestor regions, ensuring thatabsXandabsYare accurately computed for deeply nested scrollable children. This effectively eliminates the excessive screen repainting bug.Additional Changes:
test/repro-nested-scroll-repaint.test.tsx) to guarantee the number of repainted lines remains within the expected bound (<= 6), preventing future regressions.examples/sticky/sticky.tsxto remove code duplication around inner scrolling states, consolidating them into a single state record.