Add HighlightedTextManager and pull highlighted text logic into it

[timlenardo]

This is part 3 of my refactoring series.

In this PR, I pull the highlighted text logic out of the AccessibilityNotificationManager and into a new HighlightedTextManager. This will be necessary as we add other ways to detect highlighted text in the future. For instance, when detecting highlighted text with vision models in non-accessibility apps like Google Docs, we'll need that logic to be outside the AccessibilityNotificationManager, because it will use notifications like mouse drags, or shift+arrow key presses.

This also introduces the pattern that I have advocated for in a few of our engineering meetings: there should be "state managers" that are in charge of calculating specific states that we are interested in, like which text is highlighted, or which window is foregrounded. These need to be separate from the notification managers, like AccessibilityNotificationManager, the MouseNotificationManager, and the KeystrokeNotificationManager, because we'll often need to use notifications from different managers to determine the correct answer. These states should also be outside of the OnitPanelState, since we'll want them in different features, like QuickEdit or Typeahead. This new pattern accomplishes both.

Note: I've left the code 'as is' to make the review process easier. However, the HighligtedTextManager code now contains some code related to the CaratPositionManager, which should be stripped out. I will do this in a follow-up PR.

[greptile-apps]

Greptile Summary

This PR refactors the highlighted text detection logic from AccessibilityNotificationsManager into a new dedicated HighlightedTextManager. The change establishes an architectural pattern where "state managers" are responsible for calculating specific application states (like which text is highlighted), while "notification managers" handle raw system events.

The new HighlightedTextManager is implemented as a singleton that subscribes to accessibility notifications and manages text selection state through debouncing and validation. Key components moved include:

• HighlightedTextCoordinator, HighlightedTextWorker, and HighlightedTextValidator moved from Accessibility/HighlightedText/ to StateManagers/HighlightedText/
• New HighlightedTextConfig for centralized configuration
• New delegate methods added to AccessibilityNotificationsDelegate for selection and focus changes
• Integration updated in QuickEditManager to use the new manager

This separation enables future features where highlighted text detection may come from multiple sources (accessibility notifications, mouse events, vision models for non-accessibility apps like Google Docs). The manager maintains existing functionality while providing better modularity and cross-feature reusability outside of UI-specific components like OnitPanelState.

Confidence score: 2/5

• This PR contains several bugs and architectural issues that need to be addressed before merging
• Critical compilation error with undefined lastCaretPositionChangeTimestamp property, mixed concerns with caret positioning logic, and potential threading issues
• Files that need more attention: AccessibilityNotificationsManager.swift, HighlightedTextManager.swift, PanelStateTetheredManager+Delegates.swift

_{13 files reviewed, 3 comments}

_{Edit Code Review Bot Settings | Greptile}

[Niduank]

@timlenardo I replaced the // wut by this call.
Without this call, unhighlight is not working on Notes, iTerm2.

I know that it breaks the separation of layers.
The other way to handle it is to make AXUIElement optional in handleSelectionChange, delegate etc ...
I started doing that but stopped when I reached this code in AccessibilityNotificationsManager:

private func notifyDelegates(for element: AXUIElement, _ notification: (AccessibilityNotificationsDelegate) -> Void) {
       guard let elementPid = element.pid() else {
           // If we can't determine PID, notify all delegates
           notifyDelegates(notification)
           return
       }

       let processType = determineProcessType(for: elementPid)

       for case let delegate as AccessibilityNotificationsDelegate in delegates.allObjects {
           switch processType {
           case .ownProcess:
               if delegate.wantsNotificationsFromOnit {
                   notification(delegate)
               }
           case .ignoreProcess:
               if delegate.wantsNotificationsFromIgnoredProcesses {
                   notification(delegate)
               }
           case .eligible:
               notification(delegate)
           }
       }
   }

[timlenardo]

I see the problem. My only other idea is:

1. Add a 'selectedText' optional String to handleSelectionChange, pass it through the delegate's didChangeSelection method.
2. Update HighlightedTextWorker to always return a valid AXUIElement, even if there's no selected text. This could be the window element if no selectedText elements are found.

I think this would be nicer, because a) we can completely dissociate HighlightedTextManger from AccessibilityNotificationManager b) we may avoid some race conditions by reading the selectedText() as soon as the notification comes through. Currently, it's up to the AXNotifDelegate to read the selectedText(), but if there's anything asynchronous in the callstack, the value may change. What do you think?

[lk340]

It seems like didChangeSelection is only used by the HighlightedTextManager (all other implementations are stubs), in which case, can't we just make it so that AXUIElement can be nil for :

• AccessibilityNotificationsManager.handleSelectionChange()
• AccessibilityNotificationsDelegate.accessibilityManager(_ manager..., didChangeSelection: ...)
• HighlightedTextManager.handleSelectionChange()

And then, in HighlightedTextManager.handleSelectionChange(), we can add a guard clause that sets selectedText to nil when element is nil:

    func handleSelectionChange(for element: AXUIElement?) {
        guard let element = element else {
            PanelStateCoordinator.shared.state.pendingInput = nil
            PanelStateCoordinator.shared.state.trackedPendingInput = nil
            self.selectedText = nil
            return
        }

        guard HighlightedTextValidator.isValid(element: element) else { return }

        ...
    }

And then all we'd have to do in AccessibilityNotificationsManager.handleAppActivation() is replace

                Task { @MainActor in
                    if let element = unsafeElement {
                        self.handleSelectionChange(for: element)
                    } else {
                        HighlightedTextManager.shared.processSelectedText(nil)
                    }
                }

with

                Task { @MainActor in
                    self.handleSelectionChange(for: unsafeElement)
                }

And update the guard clause in notifyDelegate from:

`guard let elementPid = element.pid() else { ... }`

to

`guard let element = element, let elementPid = element.pid() else { ... }`

And then update the stubs to take in an optional AXUIElement for the didChangeSelection notification.

I don't think this should break anything, as HighlightedTextManager is the only one that's actually processing the kAXSelectedTextChangedNotification.

Is this what you were referring to @Niduank ?

[timlenardo]

@lk340 - the issue is in notifyDelegates in AccessibilityNotificationsManager, where we check the processID before sending the notification. If we have a nil AXUIElement, we won't know which process it's from, so we will always send the notification. That would be okay for now, since only Notes and iTerm2 use the HighlightedTextPoller. But it would break if want to add any app using the HighlightedTextPoller to the ignoredApps list. In this case, we'd send the notification to all delegates, even if the app should be ignored.

[lk340]

This should be moved to the top of the file with the other private properties for better readability

[lk340]

We should remove the vestigial textSelectionDebounceInterval from AccessibilityNotificationsManager+Config.swift.

[lk340]

Maybe this comment is needed here as a future reminder (in which case, I would use our TODO pattern), but I'm just pointing out that it's here and should maybe be removed if it's safe!

[lk340]

Forgot to add a quick comment about this, but we should also make the methods inside of HighlightedTextManager private to signify that these operations are isolated to this class.

[timlenardo]

@lk340 unfortunately, we can't make them private, because then they're inaccessible in the extensions. I don't know why that's the case in Swift, doesn't make sense to me!