fix(cua-driver/linux): prefer focused pyatspi editable#2030
Conversation
|
@injaneity is attempting to deploy a commit to the Cua Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
ChangesFocused editable selection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
8545981 to
2cfb118
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs (1)
102-145: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: single-pass walk and narrower exception handling.
Two minor points on the helpers:
- In the no-focused-editable case, the tree is traversed twice (
find_focused_editablethenfind_editable), and each node incurs per-node AT-SPI D-Bus calls (queryEditableText,getState). A single recursive walk that records the first focused-editable and the first editable would halve the round trips on the fallback path. The unuseddepthparameter could be dropped as well.- The bare
except:clauses also swallowKeyboardInterrupt/SystemExit; preferexcept Exception:.Both are non-blocking given this only runs in the AT-SPI fallback path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs` around lines 102 - 145, The AT-SPI helpers in find_focused_editable, find_editable, has_editable, and is_focused currently do extra work and use overly broad exception handling. Refactor the traversal so a single recursive walk can record the first focused editable and first editable node instead of scanning the tree twice, and remove the unused depth parameter from the recursive helpers. Also replace the bare except blocks in these helpers with except Exception so KeyboardInterrupt and SystemExit are not swallowed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs`:
- Around line 152-162: The Python fallback in find_editable is missing the
document-scope preference that Rust’s pick_editable uses, so it can choose the
browser chrome instead of the page field. Update find_editable to track whether
an editable is inside a document/web-doc context and prefer those candidates
before falling back to tree order, using the existing focused-editable pass and
the fallback loop in this section to mirror Rust tiering.
---
Nitpick comments:
In `@libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs`:
- Around line 102-145: The AT-SPI helpers in find_focused_editable,
find_editable, has_editable, and is_focused currently do extra work and use
overly broad exception handling. Refactor the traversal so a single recursive
walk can record the first focused editable and first editable node instead of
scanning the tree twice, and remove the unused depth parameter from the
recursive helpers. Also replace the bare except blocks in these helpers with
except Exception so KeyboardInterrupt and SystemExit are not swallowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4c725a0d-de04-4e70-b5a5-6b784c7e6581
📒 Files selected for processing (1)
libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs
| # Prefer focused editable across the whole window before falling | ||
| # back to first editable in tree order. | ||
| for win in app: | ||
| editable = find_editable(win) | ||
| editable = find_focused_editable(win) | ||
| if editable: | ||
| break | ||
| if editable is None: | ||
| for win in app: | ||
| editable = find_editable(win) | ||
| if editable: | ||
| break |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm pick_editable's tiering and whether any other pyatspi path mirrors in_web_doc.
rg -nP 'in_web_doc|pick_editable|find_editable|find_focused_editable' libs/cua-driver/rust/crates/platform-linux/src/atspi/Repository: trycua/cua
Length of output: 2092
Align Python fallback logic with Rust's in_web_doc tier
Verified that pick_editable explicitly prioritizes document-scoped editables. The current Python fallback (find_editable) lacks an in_web_doc filter, causing it to select the first editable in tree order (often the address bar) instead of the page field for unfocused browser windows.
Please update find_editable to track/document scope and prefer editables within a document role to match the Rust tiering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs` around lines 152
- 162, The Python fallback in find_editable is missing the document-scope
preference that Rust’s pick_editable uses, so it can choose the browser chrome
instead of the page field. Update find_editable to track whether an editable is
inside a document/web-doc context and prefer those candidates before falling
back to tree order, using the existing focused-editable pass and the fallback
loop in this section to mirror Rust tiering.
2cfb118 to
cd892bd
Compare
|
Updated per review:
Validation:
|
Summary
Testing
Fixes #1947
Summary by CodeRabbit