fix(triage_client): correct scrollback row indices after a clear or a resize - #137
Merged
Merged
Conversation
`CircularList.trimStart` advances the list's start without advancing the absolute start that every element measures its index from. Its only caller is `Buffer.clearScrollback`, which runs on `ESC[3J`, so after anything runs `clear` the buffer holds a mixed index space: surviving lines report a row too high by the number trimmed, while lines written afterwards report correctly. A scrolled-up viewport therefore jumps, and a held selection renders and copies from the wrong lines. Upstream has this only as PR 225, unmerged since 2026-05-08 against a release that is two years old, so `dependency_overrides` now points at a fork carrying it. The retire-stale-selections half could have been done locally, but the drift also drives TerminalScrollAnchor, which pins the viewport at `line.index * lineHeight`; that is arithmetic inside xterm's own data model, where no amount of client code can reach it. The fork takes PR 225's narrowed approach and deliberately does not detach the trimmed lines. Detaching looks more complete and was already tried and walked back: `CellAnchor.y` and `.offset` guard `_owner!.index` with `assert(attached)` alone, so in release builds, where the assert is stripped, an anchor still holding one dereferences a null index and throws. That version passes every debug test and crashes on device. Leaving them attached makes retiring stale holders our job, which is the rest of this change. `attached` cannot be the signal, since not detaching is the point; instead a cleared line now sits before the start of the buffer and so reports a negative row. The scroll anchor releases on that, and the pane both stops offering a copy and clears the selection, which takes the stale highlight with it. Tested by driving a real `ESC[3J` through the emulator. The override was confirmed to be load-bearing by re-running those tests without it: a surviving row reads 16 rather than 0, and the anchor stays pinned at 30.0 instead of releasing.
There was a problem hiding this comment.
Pull request overview
This PR fixes a row-indexing inconsistency triggered by ESC[3J (scrollback clear) in the Flutter client’s terminal stack. It pins xterm to a fork containing an upstream-but-unreleased fix so BufferLine.index stays consistent after a clear, then updates local scroll anchoring and selection/copy handling to correctly treat cleared-out lines as stale (identified via negative indices) rather than relying on attached.
Changes:
- Add a
dependency_overridespin to a forkedxterm.dartthat advances the absolute start index duringtrimStart, keepingBufferLine.indexcorrect after scrollback clear. - Retire/ignore stale selections after scrollback clear via a new
terminalSelectionIsLivehelper and updated copy-button/selection logic in the terminal pane. - Add unit tests that drive a real
ESC[3Jto verify surviving indices, scroll-anchor behavior, and stale-selection detection.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| flutter/triage_client/test/terminal/scrollback_clear_test.dart | New tests covering ESC[3J effects on line indices, scroll anchoring, and selection liveness. |
| flutter/triage_client/pubspec.yaml | Pins xterm to a git fork via dependency_overrides, with detailed rationale and removal note. |
| flutter/triage_client/pubspec.lock | Locks xterm to a specific git commit (resolved-ref) for reproducible builds. |
| flutter/triage_client/lib/widgets/terminal_pane_stub.dart | Clears/filters selections that refer to cleared rows so the copy UI and highlight don’t desync. |
| flutter/triage_client/lib/terminal/terminal_selection.dart | Adds terminalSelectionIsLive helper used to detect selections spanning cleared (negative-index) rows. |
| flutter/triage_client/lib/terminal/terminal_scroll_anchor.dart | Drops the scroll anchor when the anchored line’s index goes negative after scrollback clear. |
| devlog/plans/000118-01-xterm-scrollback-clear.md | Plan write-up describing the issue, fork rationale, and implementation steps. |
| devlog/000118-fix-xterm-scrollback-clear.md | Branch devlog recording decisions, verification, and the dependency pin. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The override branched from upstream master, which carries three unreleased commits beyond v4.0.0, two of them behavioural: colour 15 rendering in palette_builder.dart, and Android enter-key handling in terminal_view.dart. The pubspec described it as one fix and pubspec.lock still reports 4.0.0, so nothing signalled that colour and keyboard behaviour were changing alongside a scrollback fix. Rebranched from the v4.0.0 tag, so lib/ differs from the release by exactly one file, and repinned by commit rather than branch name: a branch ref lets a force-push swap the terminal emulator this app ships with no diff here. Also records what the fix does not cover. A width-changing resize after a clear resurrects the cleared lines, because replaceWith computes cyclic indices from a _startIndex that trimStart leaves non-zero, and every live row then reports a negative index. Both guards added here misfire in that path, conservatively. The resurrection is pre-existing and present in stock 4.0.0.
The pubspec said the pin was the release plus one commit; it is two, the fix and a dev-dependency removal. That second one is genuinely consumer-invisible, since pub does not resolve dev_dependencies of a non-root package, but a supply-chain reviewer reading the comment would expect the diff to show one commit and it does not. Says lib/ carries one change instead, which is the claim that matters and is verifiable. Also points the superseded pinning decision at the entry that replaced it, so a reader looking for the rationale does not stop at a commit that is no longer in the build.
A width change reflows through `replaceWith`, which adopted the new lines against a rotated start and only reset that start afterwards. The buffer came back rotated against itself: every row answered with another row's line, and where the reflow was shorter than the rotation the leading slots were never written, so cleared lines reappeared there or reading row 0 threw. Nothing about this needs a `clear`. `push` rotates the array every time it evicts from a full scrollback, so writing past `maxLines` and then resizing the window is enough, which makes it a better candidate for the duplicated content and missing history than the clear path was. Repins the xterm override onto a fork commit carrying that fix and adds six cases covering clear-then-width-change, five of which fail against the previous pin. The guards this branch added for cleared rows were misfiring on that path, treating live rows as dead, so the rule they rest on now holds on resize as well.
iamstuffed
changed the title
Repins onto the fork commit that corrects the second fix's comments. The previous wording claimed a short replacement leaves the leading slots unwritten; measured on the unfixed code, a list of 4 rotated by 2 with a replacement of 3 reads back as the third element, then a null dereference, then the first. Slot 0 is written and row 1 throws, so a row can hand back the wrong new line, a cleared one, or throw. Three counts in the pubspec and devlog were also wrong: master carries five commits beyond the tag rather than three, the fork's tests fail 9 against stock rather than 7, and the devlog recorded a pin two commits behind the one shipped. The pubspec no longer states a commit count at all, having had it wrong three times; it says what `lib/` contains and points at `git diff v4.0.0..<ref> -- lib/`. Strengthens the scroll-anchor case, which still passed against the old pin: the offset reads 20 either way because the rotation gives that line the same absolute index, so it now also asserts which line sits at the row. All six width-change cases now fail against the previous pin.
The enumeration replacing the commit count was itself incomplete: two of the branch's commits are comment corrections, and one of those edits circular_buffer.dart rather than only tests.
The override comment enumerated two fixes in `lib/`, but the second carries a third behavioural edit it only gestured at: `replaceWith` clears the slots it reuses without detaching what they held, because after a trim those are lines the first fix deliberately left attached. Detaching them is the release-build crash that fix exists to avoid, so it is load-bearing rather than incidental. Also records in the devlog that both `terminalSelectionIsLive` call sites in the pane are uncovered: they are mobile-only, so the widget harness never reaches them, and the Verification section previously read as though the whole change was tested.
The comment correction this branch claimed at 02:32 never reached `lib/`. A control run that verified the claim ended by checking the file out from HEAD in the same shell, reverting the uncommitted edit, so the fork commit carried only its tests while its message and this repo's pubspec both said the wording had been fixed. Also corrects the devlog's reason for the pane's `terminalSelectionIsLive` call sites being untested. It said they are mobile-only, but the pane's `_isMobile` has no `runningUnderFlutterTest` carve-out and the harness reports android. They are unreached because no widget test creates a selection and the pane renders the FLUTTER_TEST fallback view.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in xterm.dart's
IndexAwareCircularBuffer, both corruptingBufferLine.index, which is what every scrollback consumer resolves a row through.TerminalScrollAnchorpins the viewport atline.index * lineHeight, so a scrolled-up viewport jumps; selection anchors resolve rows through it, so a held selection renders and copies from the wrong lines.1.
trimStartleaves a mixed index spacetrimStartadvances the list's start without advancing the absolute start that every element measures its index from. Its only caller isBuffer.clearScrollback, which runs onESC[3J, so after anything runsclearsurviving lines report a row too high by the number trimmed while lines written afterwards report correctly.2.
replaceWithrotates the buffer against itselfA width change reflows through
replaceWith, which adopts the new lines through_getCyclicIndex(offset by_startIndex) and only resets_startIndexafterwards. It therefore writes at one rotation and reads back at another.This one needs no
clearat all.pushrotates the array every time it evicts from a full scrollback, so writing pastmaxLinesand then resizing the window is enough. Measured against stock 4.0.0:Terminal(maxLines: 30), 60 lines written, then a width change: row 0 reports 29 and row 11 reports 10.trimStartdropped, or throwNull check operator used on a null valuefromoperator [], and which row does which is not worth predicting. Rotate a list of 4 by 2 and replace with 3 and it reads back as the third replacement element, then a throw, then the first.That second case is a crash in the shipped release, reachable with nothing but a full scrollback and a resize. It is a better candidate for the originally reported duplicated content and missing history than the clear path was.
It also mattered to defect 1's fix: this branch identifies a cleared line by its negative row, and before this the guards misfired on the resize path, treating live rows as dead.
Why a fork
Defect 1 exists upstream only as PR #225, unmerged since 2026-05-08 against a release that is two years old. A pull request is not something a build can depend on, so
dependency_overridespoints athyeons-lab/xterm.dart, pinned by commit rather than branch so a force-push cannot swap the emulator with no diff here. Defect 2 is not in #225; it needs its own upstream issue, which is noted as a follow-up.The retire-stale-selections work could have been done locally. The deciding factor was the scroll anchor: that arithmetic lives inside xterm's data model, where no client code can reach it.
Branched from the
v4.0.0tag rather than master, which carries five commits beyond the tag (four non-merge), two of them behavioural: colour rendering and Android enter-key handling. Against the release,lib/differs by exactly one file.Why the narrow fix
The intuitive patch for defect 1 also detaches the trimmed lines. That version was already tried and walked back, and I wrote it before finding #225:
CellAnchor.yand.offsetguard_owner!.indexwithassert(attached)alone, so in release builds, where the assert is stripped, an anchor still holding one dereferences a null index and throws. It passes every debug test and crashes on device.The same constraint shapes defect 2's fix. Resetting the start before adopting means
_adoptChildwould detach whatever occupies each slot, and after a trim those are exactly the lines that must stay attached. So the leftover slots are cleared without detaching, and the contract holds through a reflow.Leaving them attached makes retiring stale holders our job, which is the rest of this change.
attachedcannot be the signal, since not detaching is the point; a cleared line instead sits before the start of the buffer and reports a negative row.Verification
Eleven tests drive a real
ESC[3Jand real resizes through the emulator rather than simulating a trim. All six covering clear-then-width-change fail against the previous pin (5 pass, 6 fail of 11), includingterminalSelectionIsLivereporting a row that is genuinely present as dead.Run against stock 4.0.0's
circular_buffer.dart, nine of the fork's tests fail, so every test the fork adds is load-bearing. 121 tests there, 307 here,flutter analyzeclean.Not covered, and worth stating: both
terminalSelectionIsLivecall sites in the pane can be removed with the suite green. No widget test creates a selection, and the pane renders theFLUTTER_TESTfallback rather than xterm'sTerminalView. The emulator-level behaviour they rest on is covered; the wiring is not.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.