Review follow-up: a scroll restore that never ran, and six smaller lies

[jbachorik]

Follow-up to #14, from a code review that landed after it was merged. Seven findings, all real; the first is the one worth reading.

The scroll restores were dead code

Both SearchRail.rebuild and SkimView.rebuild captured the scroll position, cleared their children, and restored it a pulse later behind an if (vvalue == 0) guard. Instrumented, that guard never passes:

[r7] rebuild captured=0.5 vvalueNow=0.5
[r7]   deferred: vvalue=0.5 willRestore=false

A ScrollPane keeps its vvalue across a children swap by itself, so the position was never lost and the restore never ran. Both sites carried comments claiming to protect the reader's place, and the two tests written for the rail in #14 pass on the ScrollPane's own behaviour rather than on the mechanism they were written for.

Removed, along with the rebuild(boolean restoreScroll) parameter that existed only to steer it. The tests stay — they still guard the observable behaviour — with comments that name who actually delivers it.

The other six

• searchText expanded the rail and immediately asked for focus. While collapsed the query field is out of the scene graph, and expanding only puts it back when the 160ms animation finishes, so the focus was dropped. Exactly the trap / was fixed for in The Explorer stops fighting the reader #14; this caller predates that fix. Reachable on any window under the new 1100px threshold.
• Overlapping rail animations could strand expanded content in a 46px strip. Each collapse started a Timeline without stopping the previous one, and only the expand direction had an onFinished. A collapse overtaking an expand let the expand's finish swap the full content back in after the width had gone to 46px — with no ⌕ to get out of it. The Explorer stops fighting the reader #14 made this reachable by driving collapse from widthProperty().
• Dropping a repeated signature line dropped its changed-line marker. A member whose declaration changed was tagged · changed, pre-expanded, and then shown a body with nothing highlighted.
• Keeping the open file's row suppressed the rail's empty state. A query matching nothing rendered one ordinary-looking row and no "no file here matches" message — beside a footer reading 0 matches.
• A failed preference save left the checkbox showing a value that is not on disk, and required toggling twice to retry.
• saveAsync writes the whole record and has no production callers left. Package-private now, so a future caller cannot use it to reset a field — the exact bug updateAsync exists to prevent.

Verification

1295 tests, 0 failures, on top of current main.

Two pre-existing tests were corrected rather than accommodated: the signature-dedupe test's fixture happened to mark the signature line itself as changed, which the marker fix now legitimately keeps.

🤖 Generated with Claude Code