Fix Settings window focus under Stage Manager - #2833
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs maintainer review before merge. Reviewed August 13, 2026, 4:28 PM ET / 20:28 UTC. ClawSweeper reviewWhat this changesThe PR keeps Settings on the active Stage Manager Space and fronts only newly presented Settings or Sparkle update windows during Dock promotion. Merge readinessKeep open for maintainer review: the focused Stage Manager proof is credible, but replacing the complete window-behavior mask needs an explicit compatibility decision beyond the tested open/close/reopen path. Likely related people: steipete, recent app/release-history contributor (medium confidence). Priority: P2 Review scores
Verification
How this fits togetherCodexBar is a macOS menu-bar app that temporarily uses regular-app activation while Settings or update dialogs are present. The Dock policy observes AppKit windows, then chooses Space behavior, activation, and focus for those dialogs. flowchart LR
A[Menu bar action] --> B[Settings or update window]
B --> C[Window appearance bridge]
C --> D[Dock policy]
D --> E[Space and focus decision]
E --> F[Interactive dialog]
Decision needed
Why: The branch proves the Stage Manager path but not the broader behavior displaced by an exact OptionSet replacement; that compatibility tradeoff needs product ownership rather than automated inference. Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain the narrow active-Space fix only after confirming the intended full-screen and multi-Space behavior, then validate the final head with the repository’s full test suite. Do we have a high-confidence way to reproduce the issue? No high-confidence current-main reproduction was independently established: the PR provides after-fix Stage Manager runtime evidence, but not a before-fix current-main failure trace. Is this the best way to solve the issue? Unclear: the active-Space approach is a plausible narrow fix, but wholesale replacement of collection behavior needs compatibility confirmation or a more preserving implementation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against daf02ae7a7a4. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (5 earlier review cycles)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7e1c5022c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.isAwaitingPresentedWindow = false | ||
| self.ensureRegularPolicy(activate: hasNewPresentedWindow) | ||
| if hasNewPresentedWindow { | ||
| for item in describedWindows where presentedWindowIDs.contains(ObjectIdentifier(item.window)) { |
There was a problem hiding this comment.
When Settings is already visible and the user opens a Sparkle dialog via About → Check for Updates, hasNewPresentedWindow becomes true for the new Sparkle window, but this loop orders every recognized window—including the existing Settings window—to the front. Because the iteration order of NSApp.windows does not identify which window was newly presented, Settings can become key again and cover the update dialog. Preserve the ID difference computed before updating self.presentedWindowIDs and present only those newly added windows.
Useful? React with 👍 / 👎.
Keep Settings on the active Space and key it when Dock promotion presents the window. Co-authored-by: Cursor <cursoragent@cursor.com>
Limit Dock promotion fronting to the newly detected window set so an update alert is not covered by Settings, and move the changelog note to 0.49.3 Unreleased. Co-authored-by: Cursor <cursoragent@cursor.com>
e7e1c50 to
dc64618
Compare
|
Addressed ClawSweeper / Codex review:
Focused tests: 19 passed. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Follow-up: runtime proof log gist (screenshots are local; table is in the PR body): https://gist.github.com/KGBos/75194a39a7c964de17ed14d79d98b371 Fresh package commit: @clawsweeper re-review |
clawsweeper
Bot
added
proof: sufficient
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
ClawSweeper status: review started. I am starting a fresh review of this pull request: Fix Settings window focus under Stage Manager This is item 1/1 in the current shard. Shard 0/1. This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking. Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted. |
2 participants
Summary
.moveToActiveSpace+.fullScreenAuxiliary.Review follow-ups addressed
DockIconPolicyDecision.newlyPresentedWindowIDs)SettingsWindowAppearanceTests|DockIconPolicyDecisionTests|SettingsWindowOpeningTests)dc646188)Stage Manager proof (fresh build
dc646188)Stage Manager
GloballyEnabled=1. Bundlecom.steipete.codexbar.1)0)Menu Bar1)0)Menu BarfocusedRuntime log: https://gist.github.com/KGBos/75194a39a7c964de17ed14d79d98b371
Test plan
swift test --filter 'SettingsWindowAppearanceTests|DockIconPolicyDecisionTests|SettingsWindowOpeningTests'— 19 passed.