fix(ui): stop the session player shaking at the scrollbar threshold - #7125
geovannewashington wants to merge 1 commit into
Conversation
|
Claude finished @geovannewashington's task in 6m 35s —— View job Code Review CompleteReviewed 2 files. 1 inline issue posted. Standards: 1 violation, 0 judgement calls. Spec: 0. Correctness: 0. StandardsThe new test at SpecEvery requirement in #7121 is met. The gutter is reserved on the same element that carries Additional notesThe production change itself is sound and minimal. Two things the author may want to weigh, neither posted inline because they did not clear the review's confidence bar:
If you've addressed the feedback and want a new review, tag |
The player is created with fit: "width", so asciinema derives the font size from the container width and the rendered height falls out of that. Nothing checks the height against the container, so a recording whose height lands near the container's creates a loop: the content overflows, the scrollbar takes its width, the narrower container shrinks the font, the shorter content no longer overflows, the scrollbar goes away, and it starts over. Reserving the gutter keeps the container width constant whether the scrollbar is showing or not, so the loop has nothing to oscillate on. A 160x35 recording hits this at common window heights, which is why it showed up on sessions recorded inside tmux: the status bar takes a row, so a 160x36 terminal records 160x35. No test comes with it. jsdom computes no layout, so nothing in the console suite can observe the shaking, and an assertion on the class string would fail on a rename that preserved the fix while passing if overflow-y-auto were dropped. Fixes: #7121
78e4cae to
5a9af34
Compare
|
/review |
|
I'll analyze this and get back to you. |
|
Claude finished @otavio's task in 2m 46s —— View job Code Review CompleteReviewed 1 file on all three axes: Standards (repo rules, language conventions, tests, code smells), Spec (does it implement the issue?) and Correctness (security and runtime defects). No new issues found. The code looks good as-is. The previous round's inline finding — the test asserting the If you push additional changes and want a new review, tag |

What
Reserve the scrollbar gutter on the session player's scroll container so replays stop shaking.
Why
The player is created with
fit: "width", so asciinema derives the font size from the container width and the rendered height falls out of that, never checked against the container. When a recording's height lands on the container's, width and height feed each other: the content overflows, the scrollbar takes its width, the narrower container shrinks the font, the shorter content fits, the scrollbar goes away, and it starts over.A 160x35 recording hits this at common window heights, which is why it showed up on sessions recorded inside tmux: the status bar takes a row, so a 160x36 terminal records 160x35.
Changes
SessionPlayer.tsx:scrollbar-gutter: stableon the scroll container, so its width is identical whether or not a scrollbar is showing.No test accompanies it. The first draft asserted the class string on the container reached through the asciinema mock, which pinned the spelling rather than the behaviour: it failed on a rename that preserved the fix and passed if
overflow-y-autowere dropped while the gutter class stayed, which is the regression it appeared to guard. jsdom computes no layout, so no seam in this suite can observe the shaking. The reason the gutter is there is in the commit message.Testing
Unit: the existing 17 cases pass unchanged. eslint and
tsc -bclean.Manual, dev stack: a 160x35 tmux recording at the window height where it shook plays steadily. Deleting the class from the element in devtools brings the shaking back, typing it in stops it again, same window and same recording. That A/B is the only check that can see the behaviour, and it is what this change rests on.
Closes #7121