Conversation
|
Caution Review failedPull request was closed or merged during review 📝 Walkthrough🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can get early access to new features in CodeRabbit.Enable the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a03620f9c1
ℹ️ 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".
| case "AXTextField", "AXTextArea", "AXComboBox", "AXSearchField": | ||
| return true | ||
| case "AXWebArea", "AXGroup", "AXScrollArea": | ||
| return isEditable | ||
| default: | ||
| return false |
There was a problem hiding this comment.
Preserve
AXStaticText as a valid paste target
This whitelist drops AXStaticText, even though the previous implementation in this file explicitly accepted that role to support VS Code/Chrome-style editors. Because copyAndPasteIfPossible() now waits for isValidPasteTarget before sending ⌘V, any app whose focused editable control is exposed as AXStaticText will never auto-paste and will fall back to clipboard-only behavior instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dhavnii/Features/Clipboard/ClipboardManager.swift (1)
41-44: Rename this helper to match the broader predicate.
isTextFieldFocused()now returnstruefor editableAXWebArea/AXGroup/AXScrollAreatoo, so the name no longer describes what callers are actually checking. Something likeisValidPasteTargetFocused()would be less surprising.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dhavnii/Features/Clipboard/ClipboardManager.swift` around lines 41 - 44, The helper is misnamed: isTextFieldFocused() actually checks for any editable paste target (AXWebArea/AXGroup/AXScrollArea), so rename the function to isValidPasteTargetFocused() and update its doc comment accordingly; change the function declaration (isTextFieldFocused -> isValidPasteTargetFocused) and replace all call sites that reference isTextFieldFocused() with the new name to avoid breaking behavior, keeping the implementation (focusedElementState()?.isValidPasteTarget ?? false) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dhavnii/Features/Clipboard/ClipboardManager.swift`:
- Around line 83-110: The retry poller in attemptPasteWhenReady can paste into
whatever app is frontmost during retries and can overlap multiple chains; fix it
by introducing a per-request token (e.g., an autoPasteToken UUID stored on the
ClipboardManager when copyAndPasteIfPossible starts) that is captured by
attemptPasteWhenReady and checked on each retry and before simulatePaste(), and
ensure starting a new copyAndPasteIfPossible replaces/invalidates the stored
token so any previous retry chains bail out if their token no longer matches;
update copyAndPasteIfPossible, attemptPasteWhenReady, and where simulatePaste()
is called to compare the captured token against the current manager token and
abort stale chains.
---
Nitpick comments:
In `@dhavnii/Features/Clipboard/ClipboardManager.swift`:
- Around line 41-44: The helper is misnamed: isTextFieldFocused() actually
checks for any editable paste target (AXWebArea/AXGroup/AXScrollArea), so rename
the function to isValidPasteTargetFocused() and update its doc comment
accordingly; change the function declaration (isTextFieldFocused ->
isValidPasteTargetFocused) and replace all call sites that reference
isTextFieldFocused() with the new name to avoid breaking behavior, keeping the
implementation (focusedElementState()?.isValidPasteTarget ?? false) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e041eebf-7b49-4cae-a2cb-561aabfb1d19
📒 Files selected for processing (1)
dhavnii/Features/Clipboard/ClipboardManager.swift
Made-with: Cursor
Summary by CodeRabbit
Improvements
Refactor