Skip to content

fix: reset editor scroll state on tab changes#67

Open
Gabko14 wants to merge 3 commits intomainfrom
fix/new-tab-scrollbar-reset
Open

fix: reset editor scroll state on tab changes#67
Gabko14 wants to merge 3 commits intomainfrom
fix/new-tab-scrollbar-reset

Conversation

@Gabko14
Copy link
Owner

@Gabko14 Gabko14 commented Mar 1, 2026

Summary

  • reset the reused AppKit editor scroll view after programmatic text swaps so a visible scrollbar does not linger into a fresh tab
  • keep the fix at the editor configuration layer instead of adding another SwiftUI tab-switch workaround
  • add a regression test for reused scroll-view reset behavior

Testing

  • xcodebuild test -project Ghostly.xcodeproj -scheme Ghostly -destination 'platform=macOS' -only-testing:GhostlyTests CODE_SIGN_IDENTITY='' CODE_SIGNING_REQUIRED=NO CODE_SIGNING_ALLOWED=NO

Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: Approved

Clean, focused fix that addresses the scrollbar lingering issue at the right layer.

What is good:

  • Correct approach: Fixing this in EditorTextViewConfiguration.updateText keeps scroll behavior coupled with text updates, rather than scattering workarounds across tab-switching logic
  • Belt-and-suspenders reset: Using both scrollRangeToVisible and contentView.scroll(to: .zero) plus reflectScrolledClipView ensures 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):

  1. Unconditional reset: Every updateText call 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
  2. Force-unwrap in test (textView.textContainer!): Safe here since makeScrollView always 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.

Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fix
  • GhostlyTests/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
  • .gitignore changes
  • AGENTS.md additions

Why this matters:

  1. PRs should be atomic - one logical change per PR makes review easier and rollbacks cleaner
  2. Runtime files committed - .beads/dolt-monitor.pid (contains 79528), .beads/dolt-server.port (contains 14049), .beads/dolt-server.activity (timestamp) are machine-specific and will conflict/be stale on other machines
  3. 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:

  1. Remove all .beads/ changes from this PR (especially the runtime files)
  2. Remove AGENTS.md and .gitignore changes
  3. Create a separate PR for beads integration if that's desired
  4. Add .beads/dolt-monitor.pid, .beads/dolt-server.activity, .beads/dolt-server.port to .gitignore - these should never be tracked

Once the unrelated files are removed, this PR can be approved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant