The Explorer stops fighting the reader - #14
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Remove inlined FQN javafx.scene.Node (already imported as Node) - Remove redundant per-child setVisible() loop; lines.setVisible() already handles rendering visibility inheritance in JavaFX - Fix test assertions to check container visibility/managed state instead of individual children's own visibility property Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both Task 1 and Task 2 shipped a per-child setVisible loop that existed only to satisfy these assertions; JavaFX inherits visibility for rendering, so the container's own flags are the whole of a collapse. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Remove per-child visibility loop from buildFileRow (Task 1 removed it for
good reason: JavaFX inheritance makes it redundant state sync). Fix test
assertion in aCollapsedGroupStaysCollapsedWhenTheRailRebuilds to check
container visibility via lookup(".result-match-lines"), matching the pattern
established in theMatchGroupCaretIsVisibleAndCollapsesTheGroup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rebuild() was overwriting user scrolls that landed between capture and deferred restore. Fix: only restore if vvalue is still 0 (the rebuild's clear operation left it there). Non-zero means user or something else moved it and must win. Add test: aScrollMadeDuringARebuildIsNotOverwrittenByTheRestore verifies that a scroll made during the same FX pulse as rebuild survives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…the file Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ler to an end The span-clamp copied from topLine() floors a divisor, not a multiplier; flooring it at 1 turned a ~120px wheel delta into a snap to vvalue 0 or 1 whenever a body had nothing to scroll. redispatchWheel now leaves vvalue alone (and does not consume) when the rows do not overflow the viewport. The proportional wheel test now fires on a paragraph's own line-number graphic rather than the CodeArea itself, since Flowless's VirtualFlow is a private child inside the CodeArea and dispatch never descends into a target's own children -- the old target could never reach Flowless's handler at all, fix or no fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Drops the duplicated first line from an expanded skim body when it repeats the header's signature and something is left underneath it, so an open member no longer wastes a row echoing what the header already shows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two hand-picked pixel margins in SkimViewTest went stale in a row as row height and body line counts shifted, and running the class repeatedly turned up a second test relying on the same pattern that was intermittently failing from a shared expansion-state leak between test methods. Both now size their viewport to a quarter of the real, already-laid-out row height instead of a literal, so future row-height or content changes don't require a retune. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… middle Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…survives the next pulse rebuild()'s deferred vvalue restore (Task 3) is right for refresh() repainting the same document, but show() loading a brand new one has no reader's place worth preserving. Generalized the revealLine-only `revealing` flag to `scrollClaimed` and had show() set it around its own rebuild(), the same way revealLine() already did, so the restore it used to queue can no longer fire a pulse later and undo a jumpToLine reveal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Skim-by-default (the delta's design) stays the default, but it now has a Settings > Explorer checkbox to turn off. The preference lives in UserConfig (~/.drydock/config.json), read-modify-write everywhere it is touched so it survives an unrelated worktrees-directory edit, and is cached in MainWorkspace so the FX thread never blocks reading it on file open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
skimRow's save was fire-and-forget, contradicting the plan's own rule that every user-triggered async op shows progress and clears it on every completion path. It now disables the checkbox for the write's duration and surfaces a failure via UiErrors, same shape as the sibling worktreesRow. Also fixes SettingsModalSkimRowTest, whose fake returned false for a control that starts unselected -- a listener wired before the load landed would have passed the test by having nothing to fire on. Confirmed (then reverted) that the corrected test does fail against that ordering bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Auto-collapses the file rail below 1100px (unless the reader collapsed it themselves), elides the breadcrumb from the left below 900px via a new UiFormats.breadcrumbSegments(Path, int) overload shared with Review, and floors the skim/full/editable header controls so they stop collapsing to unreadable "…" buttons. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pses on the next resize tick The width listener's narrow branch collapsed the rail unconditionally while only the wide branch respected the reader's choice, so a manual expand while narrow was undone by the very next width change. Replaces the one-way collapsedByReader flag with a nullable railOverride: the reader's choice wins until the window crosses the collapse threshold, at which point control reverts to automatic. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The containment assertion only pinned bound-implies-advertised, the direction that had rotted. A row advertised for a key nobody wired up would have slipped through unnoticed. Turn it into an equality between what installShortcuts binds and what the overlay's own-binding rows advertise, with a named, commented exemption set for the rows that are legitimately bound elsewhere (⌘[ / ⌘], Esc) or aren't a key at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…pansion, and one less duplicated preference read Final whole-branch review of the eleven-task Explorer usability branch found four cross-task interactions no single-task review could see: - SkimView.rebuild()'s deferred vvalue restore could overwrite a revealLine landing in the pulse between an ordinary refresh() and its own restore -- findings and diff-overlay refreshes arrive over MCP while the reader is reading, so this is a real race. Guarded exactly like SearchRail.rebuild() already guards the identical race. - SessionExplorerView.searchText() expanded the rail without claiming it via railOverride, so the next width tick below the threshold took it away again -- the one caller that predates the claiming mechanism added for the manual ⌕ button. - SkimView.show() left stale per-line-number expansion state across documents, which could now pull open or fold the wrong member in a differently-shaped file since rebuild() consults it to override folding. - MainWorkspace's constructor inlined refreshExplorerPreferences()'s body instead of calling it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An instrumented run showed setSkim's anchor is read from a CodeArea that has not been laid out: opening a 68-line file it asked for line 42, and revealLine faithfully scrolled there. The correction added earlier only ran when no line was requested, and the follow-up go-to-line could not undo it, because revealLine did nothing at all for a line no member covers -- line 1 is the package declaration. So revealLine now resolves forward to the next member down, which also makes go-to-line work for imports, blank lines and class headers instead of being a dead gesture there, and the open path always places the view itself before the requested line is applied on top. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Opening a file at line 1 landed on the first declaration and expanded it -- and onMemberRead counts that as the reader having read it, which the trail and the dwell sampler both believe. A line the reader did not point at gets scrolled to, not opened. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four mechanisms that had each accumulated a caller-must-remember contract: - rebuild()'s scroll restore was steered by a scrollClaimed field that two callers set and reset around it in a try/finally; it is a parameter now, so the field and both dances are gone and a third caller cannot inherit the wrong default. - Every manual rail collapse/expand paired railOverride with setRailCollapsed by hand, five times. One caller had already forgotten it and shipped a bug. They route through readerSetRailCollapsed now. - Both UserConfig setters spelled out the read-modify-write that keeps the other preferences alive; updateUserConfig holds it once. - Six tests copy-pasted the derive-a-viewport-height fixture, comment and all. And two pieces of work nobody was waiting for: the breadcrumb rebuilt itself on every width tick when only crossing 900px changes anything, and the rail queued a deferred scroll restore on every keystroke even with nothing to restore. revealLine also looked the same member up three times; it resolves once and asks Member.covers whether the reader actually pointed at it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three from the code review, none of them cosmetic: The pre-layout anchor on the open path was worse than a wrong scroll. revealLine expands the member it lands in and reports it through onMemberRead, so every changed file opened with a member nobody chose already unfolded and permanently marked read -- a minimap tick and a dwell signal for code no one had looked at. scrollToTop() afterwards could never undo that. setSkim does not compute the anchor now. Slash was dead whenever the rail was collapsed: showCollapsed takes the search field out of the scene graph, so requestFocus did nothing while the keystroke was still consumed. This branch built that trap itself, by auto-collapsing under 1100px and advertising the key in the same PR. It expands first and the focus request waits for the field to come back. And the config read-modify-write was two executor tasks with a gap between them, so two overlapping preference edits could both read before either wrote. UserConfig.updateAsync makes it one task on the single thread that already serialises writes. A fourth finding -- resolving forward onto a folded private helper leaving the reader at the top -- is real and reproduced, and a comment at the site says so. An attempted fix did not move the viewport either, so it wants investigation rather than another guess. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Chasing the review's folded-helper finding turned up something wider underneath it: revealLine's scroll was not landing at all whenever the rebuild changed the content's height -- which is most reveals, since revealing usually opens a body. Instrumented, on a minimap-click-equivalent: revealLine computes target=0.41 and calls setVvalue(0.41), and the value a pulse later is 0.05. ScrollPaneSkin's next layout re-derives vvalue to preserve the previous ABSOLUTE offset, so a synchronous write against changed content is overwritten every time. It is re-applied on the next pulse now; the synchronous write stays first so the common case still does not flicker. The existing test never caught this because it asserts only that vvalue moved off zero, and the overwrite leaves exactly that -- the old offset re-expressed against taller content. The new test pins the position. The folded-helper case the review actually reported is improved but NOT finished, and the code says so at the site: resolving forward onto a helper with no row of its own now falls back to the group row that holds it, which lands when its test runs alone and does not when the same test runs in the full suite. That test was removed rather than left to flake. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…passes The remaining known-broken behaviour from the review, finished rather than deferred. A reveal's scroll is overwritten by ScrollPaneSkin's next layout, which re-derives vvalue to preserve the previous absolute offset. The last commit re-applied it once on the next pulse; that landed when a test ran alone and missed inside the full suite, because how many passes it takes is not something this code can know. The request is held now and re-applied on each layout until the value is still on target at the START of a later pass -- reading it back immediately after setting it always agrees, which is why the earlier attempt cleared itself too soon. Bounded at eight passes, and any deliberate scroll drops it, so it can never argue with the reader. That also finishes the folded-helper case the review reported: resolving forward onto a helper with no row of its own falls back to the group row that holds it, and now that the write survives, it lands. Two smaller ones from the same review: a signature line carrying a trailing comment no longer duplicates itself in the body, because the comparison strips comments the way the signature it compares against was built; and a rebuild restores the reader's pixel offset rather than the same fraction of a document that changed height. The scroll tests wait for the outcome now instead of counting pulses. Two behaviours are noted in the code as untested and why: the pixel restore, and topLine's exact-edge boundary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It rendered, but as a dim 11px glyph among a checkbox, a change dot and a filename -- and it is the one thing in the row that does not open the file, so looking like the row is exactly wrong. Full text colour, 13px bold, and a resting plate, with the accent on hover. Only rows carrying content matches have one, so this is never many at once. Checked in both themes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Eleven usability defects in the Session Explorer, found by driving the real app rather than reading the code, then fixed, reviewed and re-verified in the app.
The report that started this was "it is more chaotic than before: sections collapse and expand seemingly randomly, the scrolling is totally weird, and there are no mnemonics". Each of those turned out to be a real defect with a specific cause.
What was wrong, and what it is now
rebuild()recreated every row expanded, and it runs on every keystroke, file open and findings refreshVirtualFlowconsumes every wheel event, even when a body sized to its content has nothing to scrollsetSkimanchored on aCodeAreathat had not been laid out — it asked for line 42 of a 68-line filerevealLinesilently no-op'd for any line no member covers — imports, blank lines, class headersScrollPaneSkinre-derives vvalue on the next layout to preserve the old absolute offset, overwriting the scroll…,…,e…where the controls should be/,dandswere bound and advertised nowhere — an AGENTS.md rule violationPlus: skim-by-default is now a setting (
~/.drydock/config.json, Settings → Explorer) rather than something the Explorer does to you.Notes for review
UserConfig.updateAsync— the read-modify-write was two executor tasks, so two overlapping preference edits could each read before either wrote. It is one task on the thread that already serialises writes.revealLineexpands what it lands on and reports it throughonMemberRead, so every changed file opened with a member nobody chose unfolded and permanently marked read — a minimap tick and a dwell signal for code no one had looked at.vvalue > 0, which the very overwrite they were meant to catch satisfies, and one could not fail at all.Verification
1269 tests, 0 failures. Every fix was verified RED-then-GREEN by reverting it.
The scroll tests wait for the outcome rather than counting FX pulses — fixed pulse counts were behind three separate order-dependent failures in this class.
Screenshots of the running app before and after, at 1600×1000 and 1000×760, in both themes.
Two behaviours are deliberately untested, and say so in the code: the pixel-vs-fraction scroll restore (a test restored reliably alone and timed out in the full suite), and
topLine()'s exact-edge boundary (rows carry 1px spacing, so two edges never coincide — a test could only pass by construction).Two need a human, because no harness can drive them: the mouse wheel over open code, and a click on a search match that lands inside an untouched private helper.
🤖 Generated with Claude Code