fix: hide scrollbar when not scrolling

[Gabko14]

Closes ghostly-250

Problem

The editor scrollbar stayed visible when the note fit without scrolling, and earlier attempts still produced brief flashes during paste and tab creation.

Fix

Replaced the SwiftUI TextEditor scrollbar patching with a small AppKit-backed editor that owns its NSScrollView directly and configures native overlay autohiding scrollbars.

This keeps the scrollbar hidden when content fits and lets macOS show it transiently while scrolling, without walking the SwiftUI view hierarchy after the fact.

Notes

This is an intentional exception to the usual SwiftUI-only preference in CLAUDE.md. The pure SwiftUI and post-hoc AppKit patching versions were still producing transient scrollbar glitches, so the editor now owns the native scroll view directly for deterministic behavior.

Tests

• xcodebuild test -project Ghostly.xcodeproj -scheme Ghostly -destination 'platform=macOS' -only-testing:GhostlyTests CODE_SIGN_IDENTITY='' CODE_SIGNING_REQUIRED=NO CODE_SIGNING_ALLOWED=NO
• Relaunched the built app for manual verification

[claude]

Review: fix/scrollbar-transient

Summary: This PR adds .scrollIndicators(.hidden) to the TextEditor in ContentView.swift to hide the scrollbar when not actively scrolling.

Assessment

Correct approach. The .scrollIndicators(.hidden) modifier is the right SwiftUI API for this — it hides scroll indicators by default while still showing them briefly during active scrolling. This matches native macOS behavior and keeps the editor looking clean.

No issues found:

• ✅ Single-line change, minimal blast radius
• ✅ Placement is correct (modifier chain on TextEditor)
• ✅ No performance, security, or logic concerns
• ✅ Works with the existing .scrollContentBackground(.hidden) modifier

Note: The PR references ghostly-250 (appears to be an external tracker) rather than a GitHub issue. No related GitHub issues were found.

Testing

Per CLAUDE.md, this is a UI-only change so manual verification via the test plan is appropriate:

• Open app with short text — no scrollbar visible
• Paste enough text to overflow — scrollbar appears while scrolling, then hides

Approved. Clean, focused fix. Ready to merge after manual verification.

[claude]

Review Summary

Issue: ghostly-250 — Hide scrollbar when not scrolling
Fix: Configures transient overlay scrollbars via both SwiftUI (.scrollIndicators) and AppKit (NSScrollView configuration)

Code Analysis

Approach: The PR correctly identifies that SwiftUI's .scrollIndicators(.automatic) alone doesn't guarantee overlay/transient behavior — it respects system preferences. To force transient scrollbars, the underlying NSScrollView must be configured with .scrollerStyle = .overlay and .autohidesScrollers = true. The dual approach makes sense.

EditorScrollBehavior utility: Clean, testable extraction following project patterns. The @MainActor annotation is correct for UI work.

Tests: Good coverage of the utility functions — finding scroll views, filtering logic, and configuration.

Suggestions

1. Minor: In EditorScrollBehavior.swift:223, the editorScrollViews(in:) method isn't marked @MainActor but accesses view hierarchy. Consider adding the annotation for consistency, though it currently works because it's called from @MainActor contexts.

2. Question: applyTransientScrollbars() is called on every onAppear. For a menu bar app that might open/close frequently, this re-applies settings each time. This is likely fine (idempotent), but worth noting.

Verdict

✅ Looks good. The fix correctly addresses the issue, follows project conventions, and includes appropriate tests. No blocking concerns.

[claude]

Code Review: fix/scrollbar-transient

Summary

This PR adds transient scrollbar behavior to the editor by configuring the underlying NSScrollView for overlay-style autohiding scrollbars. The approach is sound - using AppKit's native scrollbar configuration.

Issue Linkage

Linked to ghostly-250 (Hide scrollbar when not scrolling). The implementation matches the requirements.

---

Build Failure (Blocking)

The CI build is failing due to Swift 6 strict concurrency violations in EditorScrollBehavior.swift. Multiple static methods access @MainActor-isolated AppKit properties from non-isolated contexts:

Affected methods:

• editorScrollViews(in:) - accesses isDescendant(of:), subviews
• configure(_:) - mutates scrollerStyle, autohidesScrollers, hasVerticalScroller
• isEditorScrollView(_:) - accesses documentView
• allDescendants(of:) - accesses subviews
• containsTextView(in:) - accesses subviews
• depth(of:) - accesses superview

Fix: Add @MainActor to all static methods that interact with NSView/NSScrollView properties. Since all methods in this enum interact with AppKit views, the simplest fix is to mark the entire enum as @MainActor:

@MainActor
enum EditorScrollBehavior {
    // All methods automatically inherit @MainActor
}

---

Minor Observation

The PR description says .scrollIndicators(.hidden) but the code uses .scrollIndicators(.automatic). The code is correct (automatic is what enables transient behavior) - the description is just slightly off.

---

Tests

Good coverage of the utility logic. The tests correctly use @MainActor on the test suite.

---

Verdict: Please fix the @MainActor annotations to pass CI, then this is ready to merge.

[claude]

Code Review for PR #65

Summary

This PR aims to hide the scrollbar when not actively scrolling. It adds an EditorScrollBehavior utility that configures NSScrollView for overlay/transient scrollbars.

Issues

1. PR description does not match implementation

The description says:

Added .scrollIndicators(.hidden) to TextEditor in ContentView.swift

But the actual implementation uses AppKit NSScrollView configuration through view hierarchy traversal. The description should accurately reflect the approach taken.

2. Simpler SwiftUI solution may suffice

ContentView.swift:66 already has .scrollIndicators(.automatic). If the goal is transient scrollbars, consider:

• .scrollIndicators(.hidden) - hides completely (matches PR description)
• .scrollIndicators(.automatic) - already present, should auto-hide

The NSScrollView manipulation through view hierarchy traversal is fragile - SwiftUI updates could change internal structure and break this.

3. Fragile view hierarchy traversal

EditorScrollBehavior.allDescendants(of:) recursively traverses the entire view tree looking for NSTextView. This is an implementation detail of SwiftUI TextEditor that is not guaranteed to remain stable across macOS versions.

4. uniqueScrollViews is unnecessary complexity

Each NSTextView has exactly one enclosingScrollView, so deduplication via ObjectIdentifier set adds complexity without benefit. The mapping NSTextView -> enclosingScrollView is already 1:1.

5. Test coverage gap

The main entry point applyTransientScrollbars() (no parameters) is not tested because it depends on NSApplication.shared windows. Consider restructuring to make this testable, or document why it is acceptable to leave untested.

Suggestions

1. Try the pure SwiftUI approach first - Change .scrollIndicators(.automatic) to .scrollIndicators(.hidden) and test. If that achieves the desired behavior, remove EditorScrollBehavior entirely.

2. If AppKit is needed, simplify:

   • Remove uniqueScrollViews
   • Consider a more targeted approach than full tree traversal

3. Update PR description to accurately reflect the implementation.

What is Good

• Clean separation of scrollbar logic into dedicated utility
• Proper use of @MainActor for UI operations
• Tests follow project conventions (Swift Testing framework)
• Enum for stateless utility is appropriate

---

Reviewed by Claude Code

[claude]

Code Review

Issue Linkage

This PR references ghostly-250 from an external tracker. No linked GitHub issue was found.

---

Critical Concern: Violates Project Convention

CLAUDE.md explicitly states:

Native SwiftUI components (no NSViewRepresentable wrappers)

This PR introduces GhostlyTextEditor, an NSViewRepresentable wrapper around NSTextView. This directly contradicts a core architectural guideline.

Before proceeding, please confirm with the maintainer whether this deviation from the established pattern is intentional and acceptable.

---

PR Description is Misleading

The description says:

Added .scrollIndicators(.hidden) to TextEditor in ContentView.swift

But the implementation:

1. Removes the SwiftUI TextEditor entirely
2. Creates a custom GhostlyTextEditor using NSViewRepresentable
3. Configures NSScrollView with scrollerStyle = .overlay and autohidesScrollers = true

The description should accurately reflect this significant architectural change.

---

Did You Try Pure SwiftUI First?

The simpler approach would be:

TextEditor(text: transformedTextBinding)
    .scrollIndicators(.hidden)  // or .automatic

Have you verified that this native approach doesn't achieve the desired behavior? If SwiftUI's built-in modifiers work, they should be preferred per project conventions.

---

Code Quality Assessment

If the NSViewRepresentable approach is approved:

Good:

• Clean separation of configuration logic into EditorTextViewConfiguration
• Proper use of @MainActor on methods touching AppKit views
• Tests follow Swift Testing conventions with @Suite and @MainActor
• Focus state synchronization via Coordinator is well-implemented
• isProgrammaticUpdate flag prevents feedback loops

Minor suggestions:

1. GhostlyTextEditor.swift:89-95 - The async focus handling uses DispatchQueue.main.async. Consider using Swift Concurrency (Task { @MainActor in ... }) for consistency with modern Swift patterns.

2. The KeyboardShortcuts import was removed from ContentView.swift. Verify this was intentional and not used elsewhere in the file.

---

Testing

Tests cover the configuration utility well:

• Scrollbar overlay/autohide settings ✓
• Text view plain text configuration ✓
• Text update with selection clamping ✓

Gap: GhostlyTextEditor itself (the NSViewRepresentable) has no unit tests. The Coordinator's focus management and text synchronization logic could benefit from testing.

---

Summary

Aspect	Status
Builds	CI pending
Tests	Coverage exists for utility, not for view
Convention	⚠️ Violates "no NSViewRepresentable" guideline
Description	❌ Does not match implementation

Recommendation: Please clarify whether bypassing the SwiftUI-only constraint is acceptable for this use case, and update the PR description to accurately reflect the approach taken.

---

Reviewed by Claude Code

[claude]

PR Review: fix: hide scrollbar when not scrolling

Thanks for the detailed PR description explaining the rationale for this approach.

Summary

This PR replaces the SwiftUI TextEditor with a custom AppKit-backed GhostlyTextEditor to fix scrollbar visibility issues. The approach is well-justified given the documented limitations of SwiftUI's TextEditor scrollbar handling.

What I Reviewed

• EditorTextViewConfiguration.swift - Configuration utility
• GhostlyTextEditor.swift - NSViewRepresentable wrapper
• ContentView.swift - Integration changes
• EditorTextViewConfigurationTests.swift - Unit tests

Feedback

Overall: Looks good to merge ✓

The code is well-structured with good separation of concerns. A few observations:

1. Clean architecture - Good separation between configuration (EditorTextViewConfiguration) and the representable view (GhostlyTextEditor). This makes the configuration independently testable.

2. Proper MainActor usage - Correct use of @MainActor annotations throughout AppKit-interacting code.

3. Coordinator pattern - The performProgrammaticUpdate pattern correctly prevents feedback loops during text binding updates.

4. Selection handling - clampedSelectedRanges properly handles edge cases when text shrinks (e.g., from "Hello world" to "Hi").

5. Tests - Good coverage of the configuration utility. Tests follow project conventions using Swift Testing.

6. Memory management - Appropriate use of weak var textView in the coordinator.

Minor observation (not blocking):

• In applyFocusIfNeeded(), the deferred Task { @MainActor ... } handles the case where window isn't attached yet. This is a reasonable approach, though if focus issues surface later, this might be worth revisiting with a different timing mechanism (like viewDidMoveToWindow). Not a concern for now.

The PR achieves its goal with clean, testable code. The intentional deviation from SwiftUI-only is well-documented and justified.