fix(ui): keep scrollbar glyphs out of overlay copy by espetro · Pull Request #918 · tontinton/maki

espetro · 2026-08-31T17:31:23Z

Closes #917. Relates to #409, #248. Negative prior art: copilot-cli#4009.

Problem

Overlay-zone copy is cell-based, and modal scrollbars paint ▐ into the last content column of their area, so any full-width selection in /btw, usage modal, or help modal drags scrollbar glyphs into the clipboard. Chat copy is unaffected (state-based via segment cache).

Fix

At copy time, extract_selected_text scans each region's rightmost column in the live buffer (scrollbar::live_rails) and append_rows skips only the cells that still show the thumb glyph. Nothing is recorded at paint time, so a rail a modal later painted over stops excluding as soon as its cells show text again, and a hidden thumb leaves nothing to skip. This covers every scrollbar call site (modals, search_modal, file_picker, lua_float) without moving the rail into the border or shrinking the selectable region.

Testing

overlay_copy_skips_painted_rail_cells drives the real render path: draws the app with the help modal to a TestBackend, asserts the recorded rail cells hold ▐ and belong to the Overlay zone, then asserts extract_selected_text returns modal text with no ▐.
overlay_copy_reads_rails_from_the_live_buffer covers the stale-record case: a chat split's rail runs under the help modal, and copying the modal keeps the overpainted text intact.
cargo clippy -p maki-ui --all-targets -- -D warnings clean; cargo test -p maki-ui green.
Manual: /btw → select full width → paste contains no ▐.

espetro · 2026-08-31T21:04:30Z

Design update (commit 764753b): the border-rendered scrollbar is now thumb-only no more — the track renders as the border glyph itself (│ in panel border color) with a heavy ┃ thumb, inset 1 row from the corners. Border stays visually continuous; the thumb reads as the border thickening. This follows ratatui's documented border-placement pattern and avoids the overlay-flicker failure mode seen in tuios (reverted commit ad2142f). An earlier outward-bump variant was prototyped and discarded: no TUI framework ships a border detour, and the required connector glyphs read as floating chrome (screenshots in development notes).

tontinton · 2026-09-02T18:07:28Z

@espetro yep, you're right. Overlay zones register inset_border(popup), which is the same rect modals hand to the scrollbar, so the rail lands on the last content column.

I'd drop the mouse.rs half though. Once the rail moves to inner.right() it's outside the zone already, so shaving a column just eats content. And is_enabled() is the global toggle, not "did we paint a rail this frame", so status_area and queue_area lose a column too and they never had a scrollbar. It's also the only thing covering search_modal.rs:223, file_picker.rs:375 and lua_float.rs:525, which this PR doesn't convert.

Two options:

move every overlay scrollbar to the border column, delete the mouse.rs change. zones already register inner so nothing else is needed. cheap, but every new call site has to remember the rule, and it breaks again the moment a widget rect and its zone drift apart, which is how we got here.
record the rail rects painted this frame and skip those cells in extract_selected_text, e.g. an exclude on ContentRegion. doesn't care where a widget decides to paint.

I want 2. Take the border move on top of it if you like the look, just not as the thing keeping copy correct.

Rest:

render_vertical_scrollbar_in_border lost the is_enabled() early return, so /scrollbar off (Add option to hide non-interactive scrollbar #248) stops working for btw/usage/help.
rail math is content_len - rail.height with rail.height = inner.height - 2, but positions come from viewport_h = inner.height (help_modal.rs:210, usage_modal.rs:123). content_length ends up 2 too big and the thumb never reaches the bottom. pass the viewport height in.
extract_excludes_scrollbar_rail_column gets a region you already narrowed by hand, so it asserts the bounds check works. render a modal to a TestBackend and check the glyph sits at inner.right(), or drive the copy path (app/tests.rs:3365 has the scaffolding).

The │ + ┃ also makes 3 modals look different from chat and the pickers. fine by me if it's deliberate.

Record the cells each scrollbar repainted while drawing the frame and hand them to ContentRegion as exclude rects, so extract_selected_text skips rail glyphs wherever a widget decided to paint them. Replaces the column-shave approach: nothing assumes the rail sits in a fixed column, and selection/highlight geometry is left alone.

espetro · 2026-10-06T20:24:12Z

Thanks for the feedback @tontinton ; I reworked it as option 2: the paint record now carries copy correctness, and everything else is gone.

render_vertical_scrollbar snapshots the rail column, renders, then diffs and records the cells it repainted into a per-frame PAINTED_RAILS (thread-local, reset by begin_frame() at the top of App::view, since copy runs inside view()).
ContentRegion gained exclude: Vec<Rect>; append_rows skips those cells. copy_selection fills it from painted_rails() for the Overlay zone — and Input too, since its zone covers the rail column with the same latent leak.
render_vertical_scrollbar_in_border, the border move, the is_enabled() shave, and the tautological test are all dropped. One mechanism now covers every call site, including search_modal.rs, file_picker.rs, and lua_float.rs.
The is_enabled() early return never left the single render path, so /scrollbar off still works everywhere.
New test drives the real path: overlay_copy_skips_painted_rail_cells draws the app with the help modal to a TestBackend, asserts every recorded cell holds ▐, resolves the rail cell's zone to Overlay, and asserts extract_selected_text returns modal text with no ▐.

Rebased onto current main; the conflict is gone and it's mergeable again.

tontinton

Nice, much cleaner. One catch: painted_rails() also returns rails that a modal later painted over, so with a right split open, copying from help gives trl+B instead of Ctrl+B. Only skipping cells that still show ▐ at copy time fixes it, and then you can drop the before/after snapshot too.

Drop the per-frame PAINTED_RAILS record. A modal that overpaints cells another scrollbar painted left a stale record, so copying modal text lost characters. Copy now excludes only cells that still show the thumb glyph.

espetro · 2026-10-19T11:05:16Z

Thanks, you were right about the stale record. I dropped the PAINTED_RAILS thread-local, begin_frame and painted_rails entirely. At copy time scrollbar::live_rails now scans the copied area's rightmost column in the live buffer and excludes only cells that still show the thumb glyph, so cells a modal has overpainted with text are no longer excluded. I added a regression test with a chat split whose rail runs under the help modal, and cargo nextest run -p maki-ui is green. Pushed as 4a597d1.

tontinton · 2026-10-22T21:45:38Z

/review

maki-review

Verdict: Merge after one fix. Reading the rail from the live buffer at copy time is the right call and resolves the stale-record bug from the last review. Please also update the PR description: it still describes the dropped PAINTED_RAILS design.

Direction: The PR takes the best approach. Checking what the user actually sees at copy time works at every scrollbar call site without moving rails or shrinking zones.

maki-review · 2026-10-22T21:48:06Z

    pub line_breaks: LineBreaks,
+    /// Cells chrome still covers this frame (live scrollbar thumbs). Cells
+    /// inside are skipped no matter what the selection covers.
+    pub exclude: Vec<Rect>,


MEDIUM: The next copy path that forgets one line will leak the scrollbar glyph again, and the tests won't notice.

This field comes entirely from the buffer and area that copy already has. The new tests fill it in by hand, so deleting it from mouse.rs still passes.

Fix: drop the field and compute live_rails(buf, region.area) inside extract_selected_text.

Done in f824afd — ContentRegion.exclude is gone and extract_selected_text computes live_rails(buf, region.area) itself, so a copy path can't forget it. Tests no longer fill the field in by hand. Also updated the PR description, which still described the dropped PAINTED_RAILS design. @tontinton

ContentRegion.exclude was filled by hand at each copy site, so a path that forgot it would leak scrollbar glyphs again with no test noticing. extract_selected_text now computes live_rails from the buffer and region area it already has, and the field is gone.

tontinton · 2026-11-07T16:20:44Z

Thanks! Ended up fixing this a different way in a0bc4d9, so closing.

devin-ai-integration Bot force-pushed the fix/overlay-copy-scrollbar-rail branch from 764753b to 3251030 Compare October 6, 2026 19:17

devin-ai-integration Bot force-pushed the fix/overlay-copy-scrollbar-rail branch from 3251030 to 31594a5 Compare October 6, 2026 19:17

tontinton force-pushed the main branch from 3d22307 to 3436498 Compare October 10, 2026 15:19

tontinton requested changes Oct 18, 2026

View reviewed changes

fix(ui): read scrollbar rails from the live buffer at copy time

4a597d1

Drop the per-frame PAINTED_RAILS record. A modal that overpaints cells another scrollbar painted left a stale record, so copying modal text lost characters. Copy now excludes only cells that still show the thumb glyph.

maki-review Bot suggested changes Oct 22, 2026

View reviewed changes

tontinton closed this Nov 7, 2026

Navigation Menu

Repository navigation

fix(ui): keep scrollbar glyphs out of overlay copy - #918

fix(ui): keep scrollbar glyphs out of overlay copy#918
espetro wants to merge 3 commits into
tontinton:mainfrom
espetro:fix/overlay-copy-scrollbar-rail

espetro commented Aug 31, 2026 •

edited

Loading

Uh oh!

espetro commented Aug 31, 2026

Uh oh!

tontinton commented Sep 2, 2026 •

edited

Loading

Uh oh!

espetro commented Oct 6, 2026

Uh oh!

tontinton left a comment

Uh oh!

espetro commented Oct 19, 2026

Uh oh!

tontinton commented Oct 22, 2026

Uh oh!

maki-review Bot left a comment

Uh oh!

maki-review Bot Oct 22, 2026

Uh oh!

espetro Nov 6, 2026

Uh oh!

tontinton commented Nov 7, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

espetro commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Testing

Uh oh!

espetro commented Aug 31, 2026

Uh oh!

tontinton commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

espetro commented Oct 6, 2026

Uh oh!

tontinton left a comment

Choose a reason for hiding this comment

Uh oh!

espetro commented Oct 19, 2026

Uh oh!

tontinton commented Oct 22, 2026

Uh oh!

maki-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

maki-review Bot Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

espetro Nov 6, 2026

Choose a reason for hiding this comment

Uh oh!

tontinton commented Nov 7, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

espetro commented Aug 31, 2026 •

edited

Loading

tontinton commented Sep 2, 2026 •

edited

Loading