chore: Move Unity process focus helpers into common package#1484
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughUnity process discovery and OS-specific window focus/restore logic are extracted from cli/common/clicore/focus.go into a new cli/common/unityprocess package with platform-specific implementations (darwin, windows via PowerShell scripts, unsupported fallback). Consumer packages (clicore, dispatcher, project-runner) and release-automation tooling are updated to reference the new package. ChangesUnity process/focus extraction and consumer wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant FocusUnityProcessWithRestore
participant OSHelper as osascript/PowerShell
Caller->>FocusUnityProcessWithRestore: request focus(pid)
FocusUnityProcessWithRestore->>OSHelper: read current frontmost/foreground handle
OSHelper-->>FocusUnityProcessWithRestore: previous handle
FocusUnityProcessWithRestore->>OSHelper: set frontmost/foreground(pid)
FocusUnityProcessWithRestore-->>Caller: RestoreFocusFunc closure
Caller->>FocusUnityProcessWithRestore: invoke restore()
FocusUnityProcessWithRestore->>OSHelper: restore previous handle
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/common/unityprocess/process_test.go (1)
125-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWhitespace-dependent negative assertion is brittle.
The negative check matches an exact 2-space-indented literal (
"\n $shown = ...") to prove the call isn't unconditional, while the positive check above expects 4-space indentation. If the.ps1template's formatting/indentation ever changes without changing behavior, this negative assertion could silently stop testing anything meaningful (false negative) rather than failing loudly.♻️ Suggested more robust check
- if strings.Contains(script, "\n $shown = [Win32Interop]::ShowWindowAsync($handle, 9)") { - t.Fatalf("restore script should not unconditionally restore the previous window: %s", script) - } + if strings.Count(script, "ShowWindowAsync($handle, 9)") != 1 { + t.Fatalf("expected exactly one conditional ShowWindowAsync call: %s", script) + }🤖 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 `@cli/common/unityprocess/process_test.go` around lines 125 - 141, The negative assertion in TestBuildRestoreWindowsForegroundWindowScriptRestoresOnlyMinimizedWindow is brittle because it depends on exact whitespace in the generated script. Update the test to verify the behavior using a whitespace-agnostic check against buildRestoreWindowsForegroundWindowScript, for example by asserting the ShowWindowAsync call is guarded inside the IsIconic conditional rather than matching a literal two-space-indented line.
🤖 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 `@cli/common/unityprocess/focus_windows.go`:
- Around line 10-33: The Windows focus helpers are dropping PowerShell stderr,
so errors from the scripts never reach callers. Update FocusUnityProcess,
FocusUnityProcessWithRestore, and restoreWindowsForegroundWindow to capture
stderr from exec.CommandContext (or unwrap *exec.ExitError) and append it to the
returned error before bubbling it up. Keep the fix localized around
buildFocusUnityProcessWindowsScript,
buildFocusUnityProcessWindowsWithRestoreScript, and
buildRestoreWindowsForegroundWindowScript so downstream err.Error() includes the
PowerShell throw text.
---
Nitpick comments:
In `@cli/common/unityprocess/process_test.go`:
- Around line 125-141: The negative assertion in
TestBuildRestoreWindowsForegroundWindowScriptRestoresOnlyMinimizedWindow is
brittle because it depends on exact whitespace in the generated script. Update
the test to verify the behavior using a whitespace-agnostic check against
buildRestoreWindowsForegroundWindowScript, for example by asserting the
ShowWindowAsync call is guarded inside the IsIconic conditional rather than
matching a literal two-space-indented line.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 73b81a9c-ee06-4225-823c-086933f50d94
⛔ Files ignored due to path filters (3)
cli/common/unityprocess/focus_unity_process.ps1is excluded by none and included by nonecli/common/unityprocess/focus_unity_process_with_restore.ps1is excluded by none and included by nonecli/common/unityprocess/restore_windows_foreground_window.ps1is excluded by none and included by none
📒 Files selected for processing (18)
cli/common/clicore/focus.gocli/common/clicore/focus_test.gocli/common/clicore/tool_readiness.gocli/common/unityprocess/focus_darwin.gocli/common/unityprocess/focus_unsupported.gocli/common/unityprocess/focus_windows.gocli/common/unityprocess/focus_windows_scripts.gocli/common/unityprocess/process.gocli/common/unityprocess/process_test.gocli/dispatcher/internal/dispatcher/launch.gocli/dispatcher/internal/dispatcher/launch_deps.gocli/dispatcher/shared-inputs-stamp.jsoncli/project-runner/internal/projectrunner/connection_retry.gocli/project-runner/internal/projectrunner/connection_retry_flow.gocli/project-runner/shared-inputs-stamp.jsoncli/release-automation/internal/automation/release_trigger_guard.gocli/release-automation/internal/automation/release_trigger_guard_test.goscripts/stamp-release-inputs.sh
💤 Files with no reviewable changes (1)
- cli/common/clicore/focus_test.go
There was a problem hiding this comment.
2 issues found across 21 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Summary
clicoreintocommon/unityprocesswhile preserving existing focus-window behavior..ps1templates instead of assembling the long scripts in Go string slices.User Impact
clicoredecomposition steps, making future focus behavior changes easier to review in smaller PRs.Changes
common/unityprocessfor Unity process discovery, macOS focus, Windows focus, and unsupported-platform focus handling.clicorecompatibility wrappers for existing callers while moving dispatcher/project-runner deps to the new package..ps1assets and refreshed dispatcher/project-runner shared input stamps.Verification
cd cli/common && go test ./unityprocess ./clicorecd cli/dispatcher && go test ./internal/dispatchercd cli/project-runner && go test ./internal/projectrunnercd cli/release-automation && go test ./internal/automation -run 'TestReleaseTriggerGuard(CommonPackageWhitelistsMatchGoDependencies|RequiresBothTriggersForSharedEmbeddedScripts)' -count=1scripts/check-go-cli.shcd cli/release-automation && go run ./cmd/check-release-triggers --base origin/v3-beta --head HEAD