fix: add clipboard fallback for Write Mode selection reading (#259)#387
fix: add clipboard fallback for Write Mode selection reading (#259)#387postoso wants to merge 4 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f46be944e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let pasteboard = NSPasteboard.general | ||
| let snapshot = PasteboardSnapshot.capture(from: pasteboard) | ||
| let changeCountBeforeCopy = pasteboard.changeCount |
There was a problem hiding this comment.
This fallback snapshots and restores NSPasteboard.general without coordinating with TypingService.withTemporaryPasteboardString, whose reliable-paste path keeps a temporary string on the same pasteboard until its async restore completes under pasteboardSessionSemaphore. If the user starts another rewrite during that restore window, TypingService's restore can change changeCount while this code is waiting for Cmd+C, causing the previous clipboard/temporary insertion text to be treated as the selected context and then restoring the wrong snapshot. Please run this copy fallback under the same pasteboard session/lock or otherwise wait for Fluid's pending pasteboard restore to finish before sampling changeCount.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 47516f8a8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // timeout we proceed unguarded (no worse than the uncoordinated behavior this guard | ||
| // replaces), the main run loop drains, the paste's `main.sync` completes, and the | ||
| // session frees. We always release before returning (the deferred `endExclusive`). | ||
| let acquiredSession = PasteboardSession.tryBeginExclusive(timeoutMicros: Self.pasteboardSessionWaitMicros) |
There was a problem hiding this comment.
In the Edit/Write hotkey path this selection capture runs on the MainActor via RewriteModeService.captureSelectedText(), so if a previous reliable-paste/clipboard insertion is still holding PasteboardSession for its up-to-5s verification window (common when the target app cannot expose readable AX text and verification times out), this semaphore wait blocks the main thread for the full 5.5s before recording can start and the app appears unresponsive. Please avoid synchronously waiting the whole restore window on the main thread; use a non-blocking/short wait fallback or move the capture off-main.
Useful? React with 👍 / 👎.
| // Always restore the user's previous pasteboard, then defend that restore against a | ||
| // late synthetic-copy write (a slow/busy target app processing our Cmd+C after the | ||
| // read timeout would otherwise clobber the restored clipboard — see the method doc). | ||
| self.restoreClipboardDefensively(snapshot, to: pasteboard) |
There was a problem hiding this comment.
When the target app ignores Cmd+C because there is no selection, didChange is false, but this still clears and rewrites the user's previous clipboard and then watches for late writes. Since PasteboardSnapshot only restores types for which data(forType:) was captured, entering Write Mode with no selection can unnecessarily damage complex clipboard contents even though Fluid never observed a copy; restore only after an actual pasteboard change, or wait for a late change before restoring.
Useful? React with 👍 / 👎.
postoso
force-pushed
the
fix/259-writemode-clipboard
branch
from
536c073 to
110b975
Compare
|
Rebased onto 1.6.0 (current main). Clean replay, no conflicts, the fix is unchanged. xcodebuild builds green and swiftlint --strict is clean. Ready to pull. |
…ev#259) Write/Rewrite mode could not read selected text from external apps (Zed, VS Code, Chrome/Docs, Obsidian) because TextSelectionService was AX-tree-only: when kAXFocusedUIElementAttribute returns nothing for Electron / GPU-rendered editors, both AX strategies fail and selection capture returned nil, silently degrading to a fresh prompt instead of rewriting the selection. Add a third fallback in getSelectedText(): synthesize Cmd+C, poll the pasteboard changeCount until the target app writes (bounded timeout, since Cmd+C is async), read the copied string, then always restore the user's previous pasteboard contents. An empty selection produces no pasteboard write, so it times out and returns nil, preserving the existing write-mode behavior. Extract the existing pasteboard snapshot/restore logic from TypingService into a shared PasteboardSnapshot value type so both the paste-insertion path and the new selection-read path use one full-fidelity implementation instead of duplicating it.
…t-aware Cmd+C (altic-dev#259) Address two issues found in code review of the synthetic-Cmd+C selection fallback added in d25796b. 1. Late-write clipboard clobber. getSelectedTextViaClipboard() restored the user's pasteboard unconditionally right after the 300ms copy-wait timeout. A slow/busy target app could process the synthetic Cmd+C *after* that restore, and its delayed pasteboard write would clobber the restored clipboard — leaving the copied selection where the user's content belongs, breaking the "always restore the user's clipboard" guarantee. Replace the unconditional restore with restoreClipboardDefensively(_:to:): restore, record the change count our own restore produced, then poll a short bounded settle window; any further write (the app's late copy) is re-restored. Doubly bounded (200ms total / max 3 re-restores) so it cannot hang, and every exit path ends having just re-restored the user's snapshot so it cannot leave the selection on the clipboard. Synchronous on the main thread, consistent with the existing copy-wait; only paid on this last-resort fallback path. 2. Cmd+C key code was not layout-aware. postSyntheticCopy() hard-coded kVK_ANSI_C, so on Dvorak/AZERTY/QWERTZ it posted Cmd+<wrong-char> and never copied. Extract TypingService's existing TIS/UCKeyTranslate layout lookup (used for Cmd+V) into a shared LayoutAwareKeyCode enum and route both the Cmd+V paste path and the new Cmd+C path through it, re-evaluated per call so runtime layout switches are picked up, with the ANSI key code as fallback. Add LayoutAwareKeyCode unit tests (resolves Latin chars, falls back for an unmappable character, deterministic across calls). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… session (altic-dev#259) The Write/Rewrite selection-read fallback (TextSelectionService. getSelectedTextViaClipboard) snapshotted and restored NSPasteboard.general around a synthetic Cmd+C without coordinating with TypingService's paste path. TypingService.withTemporaryPasteboardString keeps a temporary string on the same pasteboard and defers the user-clipboard restore onto a background queue, holding pasteboardSessionSemaphore until that async restore completes (up to ~5s later). If a new rewrite's selection read sampled changeCount while a prior paste's restore was still pending, the paste's writes were mistaken for the user's copy (wrong selection captured) and the two snapshot/restores fought over the real pasteboard. Extract the semaphore + restore queue out of TypingService into a shared PasteboardSession type (mirroring the PasteboardSnapshot / LayoutAwareKeyCode extractions) so both subsystems share one mutual-exclusion primitive instead of racing parallel locks. The selection read now acquires the same session (bounded wait) around its snapshot -> Cmd+C -> read -> restore sequence, so it cannot sample changeCount or touch the pasteboard while a paste session is in flight. The acquire is a timed wait (tryBeginExclusive). This is load-bearing for deadlock-freedom: the paste path resolves the layout-aware Cmd+V key code via LayoutAwareKeyCode, which does DispatchQueue.main.sync when off-main, so a background paste can hold the session while waiting on the main thread; a blocking acquire on the main-thread selection read would let the two wait on each other. The timeout breaks that inversion (on expiry the read proceeds unguarded, no worse than the prior uncoordinated behavior) and bounds any main-thread stall. TypingService keeps identical behavior (pure rename to the shared session). Adds PasteboardSession unit tests; LayoutAwareKeyCode tests still pass.
Use a short non-blocking pasteboard session wait and skip the synthetic-copy selection fallback on contention to avoid main-thread stalls. Only restore the pasteboard after an observed copy write, preserving complex clipboard contents when Cmd+C times out without changing the pasteboard.
postoso
force-pushed
the
fix/259-writemode-clipboard
branch
from
110b975 to
c7fbc7f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7fbc7fdb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| layoutPtr, | ||
| keyCode, | ||
| UInt16(kUCKeyActionDisplay), | ||
| 0, |
There was a problem hiding this comment.
For input sources with a separate Command layer, such as macOS's Dvorak-QWERTY ⌘ layout, the key that produces c/v with no modifiers is not the key that produces the Copy/Paste menu equivalent while Command is down. Because this scan passes modifier state 0 but the callers later post the returned key with .maskCommand, the clipboard fallback can send a different shortcut instead of Cmd+C/Cmd+V on those layouts. Please translate using the Command modifier state that will be posted, or otherwise preserve the ANSI key for command-preserving layouts.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7fbc7fdb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
|
|
||
| // Read the copied selection only if the pasteboard actually changed. | ||
| let copiedText = didChange ? pasteboard.string(forType: .string) : nil |
There was a problem hiding this comment.
When the AX paths fail in editors that copy the current line on an empty selection (for example VS Code with its default empty-selection clipboard behavior), this fallback sees the Cmd+C pasteboard change and returns that line as if it were selected text. That makes pressing the Write/Edit hotkey with only a caret enter rewrite mode using the current line as context instead of the intended no-selection Write Mode path.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7fbc7fdb5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let cKey = LayoutAwareKeyCode.virtualKeyCode(for: "c", qwertyFallback: sentinel) | ||
| let vKey = LayoutAwareKeyCode.virtualKeyCode(for: "v", qwertyFallback: sentinel) |
There was a problem hiding this comment.
This test is tied to whatever input source is active on the machine running the suite. When a developer or CI host has a non-Latin input method selected, no GUI keyboard layout data available, or any input source where TISGetInputSourceProperty(...UnicodeKeyLayoutData) is nil, LayoutAwareKeyCode correctly returns the supplied fallback by design, but the sentinel here makes that path fail the test even though production behavior is valid. Consider forcing a known test layout, or making this assertion conditional on layout data being available.
Useful? React with 👍 / 👎.
1 participant
What
Adds a clipboard (synthetic Cmd+C) fallback so Write/Rewrite mode can read the selected text in apps whose Accessibility tree doesn't expose it — Electron / GPU-rendered editors (Zed, VS Code, Chrome/Docs, Obsidian). Previously selection capture returned nil there and the feature silently degraded to a fresh prompt instead of rewriting the selection.
How
TextSelectionService.getSelectedText()gains a third fallback after the two AX strategies: synthesize Cmd+C, poll the pasteboardchangeCountuntil the target app writes (bounded timeout, since Cmd+C is async), read the copied string, then restore the user's previous pasteboard.TypingServiceinto a sharedPasteboardSnapshotvalue type so the paste path and this read path share one implementation.Robustness
UCKeyTranslatemechanism the Cmd+V paste path uses (now extracted into a sharedLayoutAwareKeyCode), so it lands on the correct physical key on non-QWERTY layouts (Dvorak/AZERTY/…), falling back to the ANSI key code when layout data is unavailable.Verification
swiftlint --strictclean;xcodebuild buildsucceeds; unit tests for the shared layout-aware key-code util pass. (The pasteboard settle-watch involves real async copy timing that isn't unit-simulable; its correctness is argued from the bounded change-count bookkeeping.)Closes #259