Skip to content

fix(windows): preserve window state and Unicode focus metadata - #1409

Merged
Scriptwonder merged 1 commit into
CoplayDev:betafrom
SeojunKim-pumisj:fix/windows-focus-restore-1407
Oct 3, 2026
Merged

Scriptwonder merged 1 commit into
CoplayDev:betafrom
SeojunKim-pumisj:fix/windows-focus-restore-1407

Conversation

@SeojunKim-pumisj

@SeojunKim-pumisj SeojunKim-pumisj commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Description

Follow up on #1410's Windows focus fixes by preserving normal/maximized window state, supporting Unicode and long titles, and keeping PowerShell child processes hidden.

Rebased onto beta at 6abce3e0 after #1410 merged. The original HWND restoration is now in the base; this PR retains the remaining improvements and adapts its regression tests to the current project-specific targeting behavior.

Changes Made

  • Call ShowWindow(SW_RESTORE) only when IsIconic reports a minimized window. Normal and maximized windows retain their state.
  • Capture titles with Unicode Win32 APIs and a dynamically sized buffer; emit/decode UTF-8 JSON.
  • Hide PowerShell child processes for capture, project lookup, and activation. Project lookup also uses UTF-8 so non-ASCII project paths survive the JSON round trip.
  • Preserve fix(tests): recover jobs after reload, bound focus nudges, and fix CI #1410's exact project matching, strict HWND validation, foreground verification, opt-out, concurrency guard, cancellation restoration, and per-job focus policy.
  • Retain and adapt the original PR's Windows PowerShell regression tests, including a real Windows command-line parsing test for a Unicode project path. Native focus-changing calls are replaced with Win32 doubles in these tests.

Only Server/src/utils/focus_nudge.py and Server/tests/test_focus_nudge.py differ from the current base. Original contribution attribution is preserved.

Validation

At 3daf3d5b, on Windows / Python 3.13.7 with dependencies installed from the locked environment:

  • Full Python suite: 1481 passed, 3 skipped.
  • Focus helper and test-job focus policy: 109 passed; all also pass in the final full suite.
  • Tooling suite: 202 passed, with two existing AsyncMock warnings.
  • Final-head remote Python CI: 1466 passed, 18 skipped on Linux, plus 202 tooling tests passed. The additional 15 skips are the Windows-only cases exercised locally.
  • E2E workflow: the bridge job was skipped because this fork PR has no Unity license credentials; it is not an E2E pass.
  • Baseline comparison using the new PowerShell tests: the pre-fix beta helper failed the long-title and normal/maximized-window cases (3 failed, 4 passed); the updated helper passes these cases.
  • Read-only native foreground capture returned a valid HWND and string title. This check did not activate any window.
  • git diff --check: passed.

The PowerShell scripts execute on Windows, but focus-changing native calls in the regression tests are doubles. A live Unity desktop activation/restoration smoke test has not been performed. There are no C# or Unity package changes; no new local Unity EditMode/PlayMode run is claimed for this patch. Remote check results for the rebased head are listed above.

Related Issues

Relates to #1407 (Windows focus restoration); follows up on #1410. This PR does not close all symptoms in #1407.

Documentation

No tool/resource schema or generated reference changes.

Summary by CodeRabbit

  • Bug Fixes
    • Improved Windows focus restoration using saved window handles, including when window titles change or are duplicated.
    • Minimized windows are restored before activation, while invalid, closed, inaccessible, or unresponsive windows are handled safely.
    • Improved PowerShell execution reliability with UTF-8 output and hidden console windows.
  • Tests
    • Added coverage for Windows focus capture and restoration, Unity project selection, and activation edge cases.

@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a1aaf72b-0ed5-4bee-8cce-7485bbd5064a
📥 Commits

Reviewing files that changed from the base of the PR and between 61f0c80 and 3daf3d5.

📒 Files selected for processing (2)
  • Server/src/utils/focus_nudge.py
  • Server/tests/test_focus_nudge.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

Windows focus scripts now use explicit UTF-8 handling and suppress console windows. Foreground capture reads the measured window-title length. Activation restores a window only when it is minimized. Tests cover Windows subprocess settings and focus capture and restoration behavior.

Changes

Windows focus handling

Layer / File(s) Summary
Windows focus scripts
Server/src/utils/focus_nudge.py
The foreground query reads titles using the measured length and emits BOM-free UTF-8. Windows subprocesses use UTF-8 decoding where configured and suppress console creation. Activation restores a window only when it is minimized.
Windows focus tests
Server/tests/test_focus_nudge.py
Tests check subprocess options and simulate Win32 behavior. They cover foreground capture, Unity selection by project path, activation outcomes, saved-handle dispatch, and focus restoration.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: scriptwonder

Merge Risk: ⚪ Minimal · up to 3daf3

The change improves Windows focus capture and restoration, and no concrete merge-blocking issue was found.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 3daf3

The change improves window targeting and cleanup without demonstrating expanded permissions or external access. Window-identity edge cases and incomplete calling-context coverage remain, but no material security regression was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated sensitive outcome is foreground-window selection in the server process's Windows desktop context. The inspected changes do not demonstrate credential access, privilege escalation, or cross-tenant exposure; broader caller reachability was not completely established.

Trust Boundaries and Controls

  • observed — Captured window metadata crosses the PowerShell-to-Python boundary as JSON. Positive-integer validation precedes restoration, and Windows must report both successful foreground activation and the expected foreground HWND. PowerShell runs without profiles and non-interactively.

Resilience and Maintainability Implications

  • inferred — Handle validity and final foreground equality do not prove that a reused HWND still identifies the originally captured window. An unrelated window could become the restoration target after closure and handle reuse. This is a residual ownership limitation, not an established security regression: the base's title-based selection also lacked reliable ownership continuity.

Hardening Proposals

  • proposed — Consider binding the saved HWND to independently checked owner identity and declining restoration when ownership changes. Model handle reuse explicitly when evaluating that control; owner checks reduce exposure but should not be treated as an atomic lifetime guarantee.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The head addresses the Windows targeting and restoration objectives in [#1407]. Server/src/utils/focus_nudge.py captures the HWND and Unicode title, restores by HWND, and resolves Unity by exact pro… Change stall detection so healthy long-running tests do not trigger focus nudges. Stop repeated ineffective nudges at a defined limit and surface the required stuck status. Add regression tests for both behaviors.
Docstring Coverage ⚠️ Warning Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The changes in Server/src/utils/focus_nudge.py and Server/tests/test_focus_nudge.py concern Windows focus capture, activation, restoration, and regression coverage. They support the focus-related …
Title check ✅ Passed The title clearly summarizes the Windows focus changes by naming the preserved window state and Unicode focus metadata.
Description check ✅ Passed The description explains the changes, scope, validation results, related issues, and documentation impact. It does not reproduce every template heading or complete the Type of Change checkboxes, but i…
Full details: Linked Issues check

Explanation

The head addresses the Windows targeting and restoration objectives in [#1407]. Server/src/utils/focus_nudge.py captures the HWND and Unicode title, restores by HWND, and resolves Unity by exact project path; Server/tests/test_focus_nudge.py covers these behaviors. The head also supports the requested opt-out. However, should_nudge still nudges based on stale job updates while the editor is unfocused, so a healthy long test can still trigger a nudge. The head also has no limit on consecutive ineffective nudges or stuck_suspected outcome. These [#1407] requirements remain unmet.

  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov-commenter

codecov-commenter commented Sep 22, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

This replaces Windows title matching in the focus-nudge path with a saved HWND: capture returns {name, window_handle} as JSON, restore validates the handle, un-minimizes, and reports activation failure, backed by a PowerShell-level Win32 test double. Restoring by handle is the right fix — a window title is not stable identity — but the rewritten script turns a best-effort activation into a hard failure, and an HWND is not unique over time.

  1. The shared script now makes the whole nudge depend on SetForegroundWindow returning true. Server/src/utils/focus_nudge.py:443 exits non-zero when it returns false, and because that line sits in the shared script body it also governs the initial Unity activation. A freshly spawned powershell.exe holds no foreground rights, so the Win32 foreground lock will routinely deny the call — the old code ignored the return value, so _focus_app reported success. The consequence is at Server/src/utils/focus_nudge.py:596: the nudge now bails out logging "Failed to focus Unity" and never runs, i.e. the feature silently stops working. Your double returns true unless NUDGE_TEST_DENY is set (Server/tests/test_focus_nudge.py:54), so the suite cannot see this. Please confirm against a real editor; if the denial is real, log the failure instead of exiting.

  2. IsWindow proves only that some window occupies the handle. Server/src/utils/focus_nudge.py:439 checks liveness and Server/src/utils/focus_nudge.py:408 then focuses that handle unconditionally, after a focus_duration_s sleep of 3–12 s. Windows recycles HWNDs, so if the original window closed during the sleep the restore steals focus to an unrelated application. The title match at least failed closed. Comparing the current title against the captured one before focusing would keep the handle's precision without this failure mode.

Minor: the tests that actually execute the PowerShell skip off Windows (Server/tests/test_focus_nudge.py:23), and all three workflows run on ubuntu-latest, so TestWindowsFocusScripts never executes in CI while TestWindowsFocusRestore mocks subprocess.run wholesale.

Signed-off-by: pumisj <pumisj@naver.com>
@Scriptwonder
Scriptwonder force-pushed the fix/windows-focus-restore-1407 branch from 61f0c80 to 3daf3d5 Compare October 3, 2026 14:05
@Scriptwonder Scriptwonder changed the title fix(windows): restore focus to the original window by handle fix(windows): preserve window state and Unicode focus metadata Oct 3, 2026
@Scriptwonder
Scriptwonder merged commit 3bb0ace into CoplayDev:beta Oct 3, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants