Report focus events for normal-screen terminal apps#11946
Conversation
Co-Authored-By: Warp <agent@warp.dev>
warp-focus-reporting-demo.mp4 |
|
Maintainers: could someone please add the |
|
This PR is not linked to an issue that is marked with Issue-state enforcement details:
Readiness check:
To continue, link this PR to a same-repo issue such as Powered by Oz |
There was a problem hiding this comment.
This PR is not linked to an issue that is marked with ready-to-implement.
Issue-state enforcement details:
-
Associated same-repo issues checked: #11945
-
Required readiness label:
ready-to-implement
Readiness check:
- #11945: missing
ready-to-implement; readiness labels present: none
To continue, link this PR to a same-repo issue such as Closes #123 in the PR description, and make sure that issue has ready-to-implement.
Powered by Oz
|
Thanks for marking #11945 Could someone re-run the Oz review / issue-state check or dismiss the stale “changes requested” review from before the label was added? |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR updates terminal focus reporting so DECSET 1004 events are gated by the active terminal mode rather than only alt-screen state, and adds a regression test for normal-screen focus events.
Concerns
- No blocking concerns found. Visual evidence is included in the PR description, and no approved spec context was provided for additional implementation checks.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
…es (#472) ## What Strip `<...>` segments from each `**Owners**:` bullet value in `warp-ownership/ownership-areas/*.md` before extracting `@handles`, so emails like `<name@warp.dev>` no longer contribute a bogus `warp` owner. ## Why The ownership-area bullets use the form `@handle <email@warp.dev>`. The previous owner-handle regex was unanchored, so it greedily picked up `@warp` from the email domain in addition to the real handle. `_resolve_reviewer_from_ownership_area` then random-picked uniformly across the polluted owner list. When the choice landed on `warp` (or another email-domain match), `pr.create_review_request(reviewers=[\"warp\"])` 422s on GitHub (\"Reviews may only be requested from collaborators\") and the workflow silently degraded to \"no human review was requested\" — leaving non-member approved PRs without an assigned reviewer. Real-world example: PR [warpdotdev/warp#11946](warpdotdev/warp#11946) (`recommended_area: \"Shell Compatibility\"`). ## How - `oz/ownership.py`: added an `_ANGLE_SEGMENT_RE` substitution that strips `<...>` segments before `_OWNER_HANDLE_RE.finditer`. Same dedup/order behavior; just no longer counts email domains as handles. - `tests/test_ownership.py`: one regression test (`test_ignores_handles_inside_angle_bracketed_emails`) using the real `@handle <email@warp.dev>` shape. ## Verification `python -m unittest tests.test_ownership` → 16/16 pass, including the new test. Generated with [Oz](https://staging.warp.dev/conversation/3843f759-01a1-43a5-ae12-65f04335d59d).
|
@zachbai quick update: this PR should be ready for review now. The linked issue has the needed readiness label, the PR checklist is updated, and the latest Oz review did not call out anything else to address. I think we’re just waiting for the reviewer-triggered review pipeline at this point. Thanks! |
|
@zachbai thanks again for reviewing and approving this! All checks are passing now and the PR looks clean/mergeable from my side. Whenever you have a chance, could you please help merge it or route it to whoever should do the final merge? Really appreciate it. |
Description
Fixes normal-screen DECSET 1004 focus reporting. Warp already parsed
ESC[?1004hand emitted focus events for alt-screen apps, but the focus-reporting gate requiredmodel.is_alt_screen_active(). Normal-screen foreground apps that enable focus reporting, such as a long-running CLI TUI/probe, never receivedESC[Oon blur orESC[Ion focus.This changes the gate to use the active terminal mode via
model.is_term_mode_set(TermMode::FOCUS_IN_OUT), so focus events are emitted for whichever screen/mode currently owns the foreground process.Linked Issue
Fixes #11945
ready-to-specorready-to-implement.ready-to-implement.Testing
./script/runManual:
FOCUS_OUTthenFOCUS_IN.Automated:
cargo test -p warp focus_reporting_writes_focus_events_in_normal_screencargo check -p warp --libcargo fmt --package warp --checkgit diff --checkcargo clippy -p warp --lib -- -D warningsScreenshots / Videos
https://github.com/BobbyWang0120/warp/releases/download/focus-reporting-demo-assets/warp-focus-reporting-demo.mp4
Demo: a normal-screen DECSET 1004 probe receives
FOCUS_OUTafter Warp loses focus andFOCUS_INafter Warp regains focus.Agent Mode
Co-Authored-By: Warp agent@warp.dev