Skip to content

Add scrollbar visibility improvements and beads sync#59

Closed
Gabko14 wants to merge 2 commits intomainfrom
codex/scrolling-bar
Closed

Add scrollbar visibility improvements and beads sync#59
Gabko14 wants to merge 2 commits intomainfrom
codex/scrolling-bar

Conversation

@Gabko14
Copy link
Owner

@Gabko14 Gabko14 commented Feb 22, 2026

Summary

  • Add GhostlyModifiers.swift with scrollbar visibility behavior for the editor
  • Update beads gitignore with merge artifact patterns
  • Sync beads issue data

Test plan

  • Verify scrollbar appears on hover/scroll in editor
  • Confirm no regressions in existing editor behavior

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.

PR Review

Critical Issues

1. NSViewRepresentable violates project conventions

The CLAUDE.md explicitly states:

Native SwiftUI components (no NSViewRepresentable wrappers)

This PR introduces TextEditorScrollbarBehavior: NSViewRepresentable which goes against the established architecture. If custom scrollbar behavior is needed, consider alternative approaches that stay within SwiftUI.

2. No tests provided

Per CLAUDE.md:

Nothing ships without automated tests. Before creating a PR, you must: 1. Write unit tests for all logic/model changes

This PR adds 241 lines of new logic but no tests. The scrollbar coordinator has testable logic (e.g., canScrollVertically, the observer setup/teardown, fade timing).

Other Concerns

3. Unrelated changes bundled together

The PR mixes two unrelated concerns:

  • Scrollbar visibility improvements (the feature)
  • .beads/.gitignore reorganization (tooling)

These should be separate commits or PRs for cleaner history and easier review.

4. Complexity for the use case

The implementation is ~240 lines of AppKit interop code that:

  • Walks the view hierarchy to locate NSScrollView
  • Sets up multiple NotificationCenter observers
  • Manually manages observer lifecycle
  • Uses DispatchWorkItem for fade debouncing

This adds significant maintenance burden. Is the native scrollbar behavior insufficient? What specific problem does this solve?

5. No linked issue

Without a linked issue, it's unclear what problem this solves or what the acceptance criteria are.

Code Quality Notes

The implementation itself is well-structured:

  • Proper weak references to avoid retain cycles
  • Clean observer cleanup in detachObservers()
  • @MainActor annotations are correct
  • Animation context usage is appropriate

Recommendation

Address the convention violations before merging:

  1. Either get approval to deviate from "no NSViewRepresentable" convention, or find an alternative approach
  2. Add unit tests for the coordinator logic
  3. Split unrelated .beads/ changes into a separate PR

@Gabko14 Gabko14 closed this Feb 22, 2026
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