Conversation
There was a problem hiding this comment.
Review: Approved
Clean, focused fix that addresses the scrollbar lingering issue at the right layer.
What is good:
- Correct approach: Fixing this in
EditorTextViewConfiguration.updateTextkeeps scroll behavior coupled with text updates, rather than scattering workarounds across tab-switching logic - Belt-and-suspenders reset: Using both
scrollRangeToVisibleandcontentView.scroll(to: .zero)plusreflectScrolledClipViewensures reliable reset across AppKit edge cases - Solid test: The regression test properly simulates the scenario (scroll to bottom, update text, verify reset)
Minor observations (no action required):
-
Unconditional reset: Every
updateTextcall now resets scroll position. This is fine because:- User keystrokes do not go through this method (NSTextView handles native editing)
- The early return guard (
textView.string != text) skips no-op updates - Resetting to top on tab switch is reasonable UX
-
Force-unwrap in test (
textView.textContainer!): Safe here sincemakeScrollViewalways creates a valid text container, but could use optional binding for consistency with the surrounding code style.
Good work keeping the fix minimal and testable.
There was a problem hiding this comment.
Review: Changes Requested
Critical Issue: Unrelated changes bundled in PR
This PR claims to "reset editor scroll state on tab changes" but bundles 19 unrelated files alongside the 2 files that actually implement the fix. The majority of changes are beads issue tracking infrastructure that have nothing to do with the scrollbar fix:
Files that belong in this PR (2):
Ghostly/Utilities/EditorTextViewConfiguration.swift- the actual fixGhostlyTests/EditorTextViewConfigurationTests.swift- the test
Files that should NOT be in this PR (20):
.beads/directory changes (README, metadata, hooks, backup files).beads/dolt-monitor.pid,.beads/dolt-server.activity,.beads/dolt-server.port- runtime files with machine-specific data (PIDs, ports, timestamps) that should NEVER be committed.gitignorechangesAGENTS.mdadditions
Why this matters:
- PRs should be atomic - one logical change per PR makes review easier and rollbacks cleaner
- Runtime files committed -
.beads/dolt-monitor.pid(contains79528),.beads/dolt-server.port(contains14049),.beads/dolt-server.activity(timestamp) are machine-specific and will conflict/be stale on other machines - Review confusion - the existing approval review completely missed these unrelated files
The actual code fix looks good
The resetScrollState implementation is correct:
@MainActor
private static func resetScrollState(for textView: NSTextView) {
guard let scrollView = textView.enclosingScrollView else { return }
textView.scrollRangeToVisible(NSRange(location: 0, length: 0))
scrollView.contentView.scroll(to: .zero)
scrollView.reflectScrolledClipView(scrollView.contentView)
}The test is also reasonable, though the final assertion is outside the if let block which could technically pass vacuously if textView were nil (though it won't be in practice).
Recommended action:
- Remove all
.beads/changes from this PR (especially the runtime files) - Remove
AGENTS.mdand.gitignorechanges - Create a separate PR for beads integration if that's desired
- Add
.beads/dolt-monitor.pid,.beads/dolt-server.activity,.beads/dolt-server.portto.gitignore- these should never be tracked
Once the unrelated files are removed, this PR can be approved.
Summary
Testing