ci(CUA-656): update nix-wayland comment — cursor-click-gif tests now GREEN - #2028
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThe workflow comments for the Native-Wayland reproduction suite were rewritten to describe the EIS-backed compositor injection path, restate the trigger scope, and change the suite framing to blocking status. ChangesNative-Wayland workflow comments
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 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 |
…acked fix The cursor-click-gif tests (kde, xfce-labwc, gnome, xfce-sway) all pass since the EIS-backed cua-compositor injection path landed in #2000. Remove the stale 'TDD RED suite' / 'X11-only today' / 'EXPECTED TO FAIL' language that no longer reflects reality. Closes CUA-656.
e79a178 to
aac6064
Compare
r33drichards
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
libs/cua-driver/rust/crates/platform-linux/src/input/mod.rs (2)
1394-1423: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSerialize scratch-keycode remaps.
change_keyboard_mappingmutates process-external X server state, and every caller uses the sameSCRATCH_KC. Concurrent Unicodetype_textcalls can overwrite each other’s mapping or capture a temporary mapping asoriginal_scratch.Consider guarding the remap/send/restore section with a process-wide mutex for Unicode paths.
🤖 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/input/mod.rs` around lines 1394 - 1423, The Unicode scratch-keycode path in type_text currently mutates shared X server state via change_keyboard_mapping on the fixed SCRATCH_KC, so concurrent calls can race and restore the wrong mapping. Add a process-wide mutex around the remap/send/restore sequence in the type_text flow (including the original_scratch capture, the temporary mapping, key sending, and restoration) so only one Unicode remap using char_to_keycode_shift can run at a time.
1416-1463: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore the scratch mapping even when event delivery fails.
After
change_keyboard_mappingsucceeds, any later?onsend_event/flushreturns before Line 1460 restores keycode 8, leaving the X server keyboard map globally remapped.Proposed structure
- conn.send_event(false, window, EventMask::KEY_PRESS, &press)?; - sleep(Duration::from_millis(KEY_DELAY_MS)); - conn.send_event(false, window, EventMask::KEY_RELEASE, &release)?; - conn.flush()?; - - // Restore the scratch keycode mapping after a Unicode remap. - if keycode == 0 && !original_scratch.is_empty() { - conn.change_keyboard_mapping(1, SCRATCH_KC, original_scratch.len() as u8, &original_scratch)?; - conn.flush()?; - } + let send_result = (|| -> Result<()> { + conn.send_event(false, window, EventMask::KEY_PRESS, &press)?; + sleep(Duration::from_millis(KEY_DELAY_MS)); + conn.send_event(false, window, EventMask::KEY_RELEASE, &release)?; + conn.flush()?; + Ok(()) + })(); + + if keycode == 0 && !original_scratch.is_empty() { + let restore_result = (|| -> Result<()> { + conn.change_keyboard_mapping( + 1, + SCRATCH_KC, + original_scratch.len() as u8, + &original_scratch, + )?; + conn.flush()?; + Ok(()) + })(); + send_result?; + restore_result?; + } else { + send_result?; + }🤖 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/input/mod.rs` around lines 1416 - 1463, The Unicode scratch-key remap in the input path is only restored after the event sends succeed, so an error from `send_event` or `flush` can leave `SCRATCH_KC` globally remapped. Update the Unicode injection logic in the function that builds `effective_kc` and emits the `KeyPressEvent`/`KeyReleaseEvent` so the original mapping is always restored even on failure, for example by wrapping the event delivery and restore in a guard or cleanup path that runs before returning any error.
🤖 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 `@nix/cua-driver/tests/electron-cjk-input.nix`:
- Around line 380-386: The CJK input assertion in the test is too permissive
because the success branch in the readback check uses containment instead of
exact equality, so extra or duplicated characters can still pass. Update the
validation in the CJK input flow around the readback handling (including the
later assertion path that depends on this result) to require `readback_value` to
exactly match `CJK_TEXT`, and keep the partial/empty failure messages unchanged
so the test enforces the contract it claims.
---
Outside diff comments:
In `@libs/cua-driver/rust/crates/platform-linux/src/input/mod.rs`:
- Around line 1394-1423: The Unicode scratch-keycode path in type_text currently
mutates shared X server state via change_keyboard_mapping on the fixed
SCRATCH_KC, so concurrent calls can race and restore the wrong mapping. Add a
process-wide mutex around the remap/send/restore sequence in the type_text flow
(including the original_scratch capture, the temporary mapping, key sending, and
restoration) so only one Unicode remap using char_to_keycode_shift can run at a
time.
- Around line 1416-1463: The Unicode scratch-key remap in the input path is only
restored after the event sends succeed, so an error from `send_event` or `flush`
can leave `SCRATCH_KC` globally remapped. Update the Unicode injection logic in
the function that builds `effective_kc` and emits the
`KeyPressEvent`/`KeyReleaseEvent` so the original mapping is always restored
even on failure, for example by wrapping the event delivery and restore in a
guard or cleanup path that runs before returning any error.
🪄 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: b7e43a6e-7ff0-48dc-8c54-a3b0c6259a8d
📒 Files selected for processing (5)
.github/workflows/nix-build.ymlflake.nixlibs/cua-driver/rust/crates/platform-linux/src/input/mod.rsnix/cua-driver/tests/electron-cjk-input.nixnix/cua-driver/tests/wayland/cursor-click-gif.nix
| # Assertion: the CJK characters must be present in the input | ||
| if readback_value and CJK_TEXT in readback_value: | ||
| print("CJK_INPUT_OK: CJK text found in input field", flush=True) | ||
| elif readback_value and len(readback_value) > 0: | ||
| print(f"CJK_INPUT_PARTIAL: got {readback_value!r}, expected {CJK_TEXT!r}", flush=True) | ||
| else: | ||
| print(f"CJK_INPUT_FAIL: input empty, expected {CJK_TEXT!r}", flush=True) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the exact CJK match the test claims.
Line 381 passes when the field merely contains CJK_TEXT, so duplicated or extra characters still produce CJK_INPUT_OK and satisfy Line 545. Use equality to match the stated exact-readback contract.
Proposed fix
- if readback_value and CJK_TEXT in readback_value:
+ if readback_value == CJK_TEXT:
print("CJK_INPUT_OK: CJK text found in input field", flush=True)Also applies to: 542-548
🤖 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 `@nix/cua-driver/tests/electron-cjk-input.nix` around lines 380 - 386, The CJK
input assertion in the test is too permissive because the success branch in the
readback check uses containment instead of exact equality, so extra or
duplicated characters can still pass. Update the validation in the CJK input
flow around the readback handling (including the later assertion path that
depends on this result) to require `readback_value` to exactly match `CJK_TEXT`,
and keep the partial/empty failure messages unchanged so the test enforces the
contract it claims.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Linux visual regression artifactsMatrix jobs now run independently. Download visual artifacts from this workflow run.
|
2 participants
Summary
Update the
nix-wayland.ymlworkflow comment to reflect that the Wayland CI suite is now green (all cursor-click-gif tests pass), removing stale "TDD RED suite" / "X11-only today" / "EXPECTED TO FAIL" language.Background
The 3 originally failing tests (
kde / cursor-click-gif,xfce-labwc / cursor-click-gif,gnome / cursor-click-gif) were fixed when:Root cause: The test relied on host compositor focus routing for keyboard delivery after a virtual pointer click. In headless CI,
labwc(without tiling config) opens floating windows at arbitrary positions — the click at(120, 120)may miss the target.xfce-swayworked becauseworkspace_layout stackingfills the output.Fix in commit
6be1de83(PR chore(deps): apply security dependency rollup #2000): Changedcursor-click-gif.nixtoeis = true— the EIS-backed nestedcua-compositorpath. Theclick()call is retained to exercise the virtual pointer path, buttype_text/press_keyuse the deterministic focus-free injection path.Confirmed green: All 4
cursor-click-gifcombinations pass on main (run 28139436182).Change
One comment block in
.github/workflows/nix-wayland.yml— removes outdated "TDD RED suite" framing.References
6be1de83(PR chore(deps): apply security dependency rollup #2000)Summary by CodeRabbit