diff --git a/Docs/editor-investigation.md b/Docs/editor-investigation.md index fafde78..20b83ba 100644 --- a/Docs/editor-investigation.md +++ b/Docs/editor-investigation.md @@ -152,6 +152,55 @@ Conclusion: This may be necessary in the long term, but it should not be the fir The key finding is that Sapling can preserve a single source buffer and still present different active and inactive line states. This reduces risk compared with maintaining separate source and render buffers. +## Finding #1 — Selection Feedback Loop + +Root cause: + +The first `NSTextView` bridge treated every selection notification as user intent, even when the selection change was caused by Sapling itself while restoring selection after an attribute pass. + +The recursive path was: + +```text +textViewDidChangeSelection() +-> selection binding update +-> EditorState update +-> @Published state refresh +-> SwiftUI updateNSView() +-> applyHybridAttributes() +-> setSelectedRange() +-> textViewDidChangeSelection() +``` + +This created an unbounded feedback loop and could crash immediately after launch while the hybrid styling pass and SwiftUI refresh cycle bounced selection updates back and forth. + +Architecture impact: + +- Selection is not just editor state; it is also a native text system side effect. +- Attribute rendering can produce selection notifications even when the user's logical selection did not change. +- SwiftUI refreshes must not be allowed to feed native programmatic changes back into `EditorState` as if they were user edits. +- Hybrid editing remains viable, but the platform adapter must own reentrancy control. + +Chosen solution: + +- Programmatic text, selection, and attribute changes are wrapped in a coordinator transaction. +- Selection delegate callbacks are ignored while that transaction is active. +- `EditorCoordinator.updateSelection` and `updateSource` ignore no-op updates before publishing. +- Attribute passes restore selection only if the text view actually changed it. +- The bridge skips redundant restyling when both source text and active line are unchanged. + +This keeps the canonical source and selection model in `EditorState`, while making the native adapter responsible for distinguishing user-originated changes from TextKit side effects. + +Alternative solutions considered: + +- Temporary delegate removal: workable, but broader than needed and easy to forget when adding new native callbacks. +- Debounced rendering: useful for future performance work, but it would only delay the recursion instead of fixing the feedback path. +- Never restoring selection after attributes: avoids this crash, but risks cursor drift if TextKit changes selection during editing. +- Moving selection outside published editor state: reduces SwiftUI refresh pressure, but weakens the architecture because active-line rendering depends on selection. + +Result: + +The app now survives launch, typing, arrow-key movement, and select-all smoke testing without re-entering the selection loop. + ## AttributedString and NSAttributedString Swift `AttributedString` is useful for renderer-facing APIs and SwiftUI previews. diff --git a/decisions.md b/decisions.md index 423eba1..6ae932d 100644 --- a/decisions.md +++ b/decisions.md @@ -394,3 +394,4 @@ Negative: - AppKit and UIKit bridging adds platform-specific code. - The editor abstraction must prevent the rest of the app from depending directly on NSTextView or UITextView. - A future custom editor engine may still be required if line replacement or overlay rendering cannot preserve cursor correctness. +- Native adapter updates must isolate user-originated selection changes from programmatic text, selection, and attribute changes to avoid SwiftUI/TextKit feedback loops.