Add scrollbar visibility improvements and beads sync#59
Add scrollbar visibility improvements and beads sync#59
Conversation
There was a problem hiding this comment.
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/.gitignorereorganization (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
NotificationCenterobservers - Manually manages observer lifecycle
- Uses
DispatchWorkItemfor 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
weakreferences to avoid retain cycles - Clean observer cleanup in
detachObservers() @MainActorannotations are correct- Animation context usage is appropriate
Recommendation
Address the convention violations before merging:
- Either get approval to deviate from "no NSViewRepresentable" convention, or find an alternative approach
- Add unit tests for the coordinator logic
- Split unrelated
.beads/changes into a separate PR
Summary
GhostlyModifiers.swiftwith scrollbar visibility behavior for the editorTest plan