Follow focus on up/down and column moves to workspace#489
Merged
BarutSRB merged 13 commits intoJul 17, 2026
Conversation
Align the repository version gate with the installed formatter while preserving the existing exact-version check and SwiftLint pin.
Make Core own regular-application discovery, tracked-window overlays, metadata fallback, identity merging, and deterministic ordering. Keep IPC managed-only while reducing the SwiftUI picker to candidate rendering.
Route evidence-backed applications through persistent AX contexts and probe evidence-free regular apps with bounded one-shot enumeration. Carry captured WindowServer and geometry evidence into candidate selection so MainActor reduction performs no live AX reads, and promote only selected probes.
Make observed identity lookup pure, bind retry state to concrete evidence, bound alias history to two successful generations, and retire managed windows through one owner-local transaction. Rebind AX state across ID, element, and PID changes while preserving authoritative workspace and layout ownership.
Process accepted frame writes inline, ignore stale successes, and quarantine repeatedly unmanageable tiling incarnations through the shared retirement transaction. Rebind frame state by incarnation and preserve confirmed focus when background windows fail.
Record process, endpoint, classification, retry, replacement, and retirement lifecycles with generation-aware finalization targets. Capture bounded automatic AX evidence, freeze recorders before async finalization, and replace the manual focused-window dump path with phase-aware trace capture.
Build every issue attachment from fresh runtime diagnostics and current settings, then append only explicitly selected crash or trace evidence in bounded chunks. Keep selection session-only, degrade missing evidence to a fresh-only log, and serialize submissions with visible progress.
Move Window to Workspace Up/Down (and the Move Column to Workspace up/down and numbered bindings) always left focus on the source workspace, ignoring the "Follow Window to Monitor" setting that the numbered Move Window to Workspace path already honors. Route all three handlers through a shared finishWorkspaceMove helper that follows focus to the destination workspace when focusFollowsWindowToMonitor is enabled and stays on the source otherwise, matching moveFocusedWindow(toRawWorkspaceID:). Fixes BarutSRB#488 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1672bae8-7a6d-47d8-9efa-0a4d0979e675
📝 WalkthroughWalkthroughThe pull request restructures AX window admission, identity rebinding, full-rescan enumeration, retry handling, diagnostics capture, issue attachments, and related UI flows. It also centralizes workspace-move focus completion, adds regression coverage, updates classification fixtures, and changes release and packaging metadata. ChangesWindow admission and AX lifecycle
Diagnostics capture and issue attachments
Validation and supporting updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![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
🤖 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 `@Sources/OmniWM/Core/Controller/WorkspaceNavigationHandler.swift`:
- Around line 86-114: Remove the redundant applySessionPatch call from the
focusFollowsWindowToMonitor branch, since the moveWindow/moveColumn mutation
paths already update the destination workspace session. Move the source monitor
stopScrollAnimation call outside the settings conditional so it runs regardless
of focusFollowsWindowToMonitor, while preserving the existing sourceMonitor
lookup and transition behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb397e31-f03b-4031-8dce-ce1694784495
📒 Files selected for processing (1)
Sources/OmniWM/Core/Controller/WorkspaceNavigationHandler.swift
moveWindow/moveColumn already patch the destination workspace session (remembered focus token + niri viewport), so the extra applySessionPatch in finishWorkspaceMove was redundant. Also hoist stopScrollAnimation for the source monitor out of the follow branch so it runs regardless of the focusFollowsWindowToMonitor setting, matching moveFocusedWindow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1672bae8-7a6d-47d8-9efa-0a4d0979e675
Finish bounded AX discovery and acknowledged identity rebinds with generation-aware callback handling. Bound diagnostic capture and issue evidence, preserve rule snapshots, extend regression coverage, and allow packaging credentials to be overridden through the environment.
Owner
|
Thanks @RyanHecht — the root-cause fix and |
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (4)
Sources/OmniWM/Core/Ax/RunLoopJob.swift (1)
31-39: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument deadlock risks with re-entrant calls.
Because
body()executes while thestatemutex is held, any re-entrant calls withinbodytojob.isCancelled,job.checkCancellation(), orjob.cancel()will likely cause a deadlock (assumingMutexis a standard non-recursive lock).Consider adding a docstring to warn future callers that
bodymust be kept short and must not invoke other methods on thisRunLoopJobinstance.📝 Proposed docstring addition
+ /// Executes the provided closure atomically if the job has not been cancelled. + /// + /// - Warning: Do not call `isCancelled`, `checkCancellation()`, or `cancel()` from + /// within the closure, as it will deadlock. func performUnlessCancelled<T>(_ body: () throws -> T) throws -> T {🤖 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 `@Sources/OmniWM/Core/Ax/RunLoopJob.swift` around lines 31 - 39, Add documentation above RunLoopJob.performUnlessCancelled explaining that body executes while the state mutex is held, must remain short, and must not re-enter this RunLoopJob through isCancelled, checkCancellation(), or cancel() to avoid deadlock.Tests/OmniWMTests/CGSPhantomEventGuardTests.swift (1)
41-41: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that the tracked AX identity is preserved.
The trace proves the create event was seen without retry/rejection, but not that the existing AX reference or admission identity remained unchanged. Compare the stored identity before and after the event so the renamed regression test cannot pass after an unintended rebind.
Also applies to: 61-64
🤖 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 `@Tests/OmniWMTests/CGSPhantomEventGuardTests.swift` at line 41, Update testCGSCreateForTrackedWindowVerifiesIdentityWithoutMutation to capture the tracked AX reference or admission identity before dispatching the create event, then assert it is unchanged afterward. Keep the existing event-observation assertions and verify the same stored identity remains associated with the tracked window.Tests/OmniWMTests/WorkspaceMoveFocusBehaviorTests.swift (1)
29-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover upward adjacent moves as required by the issue.
Both adjacent-move regressions exercise only
.down, while the PR objective explicitly includes up and down window/column moves. Parameterize these tests over both directions, using a source workspace that has the corresponding neighbor.Also applies to: 116-117
🤖 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 `@Tests/OmniWMTests/WorkspaceMoveFocusBehaviorTests.swift` around lines 29 - 30, Update testNiriAdjacentWindowMoveHonorsFollowSettingWithEmptySourceAndDynamicDestination and the corresponding adjacent-move test to parameterize over both .up and .down directions. For each direction, configure the source workspace with the matching neighboring workspace and assert the existing follow-setting behavior for both false and true.Tests/OmniWMTests/WindowClassificationRegressionTests.swift (1)
35-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the mutated decision differ from the expected value.
Hard-coding
"floating"makes this test depend on the lexicographically first fixture remaining non-floating. Derive the mutation fromexpectedDecisioninstead.Proposed fix
let url = try XCTUnwrap(WindowClassificationFixtureLoader.fixtureURLs().first) var fixture = try WindowClassificationFixtureLoader.load(url) - fixture.observation.observedDecision.disposition = "floating" + fixture.observation.observedDecision.disposition = + fixture.expectedDecision.disposition == "floating" ? "managed" : "floating"🤖 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 `@Tests/OmniWMTests/WindowClassificationRegressionTests.swift` around lines 35 - 44, Update testObservedDecisionDoesNotDefineExpectedBehavior so the mutated observedDecision.disposition is derived from fixture.expectedDecision rather than hard-coded as "floating"; choose a value guaranteed to differ from the expected disposition while preserving the test’s assertion that recomputation matches expectedDecision and not the observation.
🤖 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 `@Sources/OmniWM/Core/Ax/AppAXContext.swift`:
- Around line 574-610: The AX notification update paths around bindWindowElement
and the corresponding sections near the missing-window cleanup must keep
observer mutations and subscribedWindows bookkeeping synchronized: update
subscribedWindows immediately after each successful removal or addition, without
an intervening cancellation check, and propagate failures from
addWindowNotifications instead of treating them as a silent false result. Apply
the same atomic ordering to the paths around removeMissingWindowSubscription and
all referenced subscription mutations.
In `@Sources/OmniWM/Core/Controller/AXEventHandler.swift`:
- Around line 627-629: Update ManagedWindowIdentityRebindResult.isHandled so
.pending returns false and only .committed returns true. In the
completeLiveStructuralReplacementCreate flow, ensure
finishAdmissionRetryAfterTracking is reached only after a committed identity
rebind, not while rekeyManagedReplacement(from:to:) remains deferred.
In `@Sources/OmniWM/Core/Controller/AXEventHandler`+ManagedWindowIdentity.swift:
- Around line 151-193: Update the rebind flow around
commitManagedWindowIdentityRebind and commitFrameApplicationStateForRebind so
workspace, frame, and managed identity state are not committed before AX
finalization succeeds. Stage the rebind and finalize AX state first, then commit
all three states atomically; if finalization fails, preserve the old state or
explicitly restore every staged change before retiring the retry and requesting
a rescan.
In `@Sources/OmniWM/Core/Controller/WindowAdmissionLifecycle.swift`:
- Around line 179-181: Update WindowIdentityAliasHistory.contains(_:and:) to
require lhs and rhs to be contained in the same generation, rather than
independently matching current or previous aliases. Preserve true only for
same-generation matches, unless an explicit overlap bridge records that the
generations represent the same window incarnation.
In `@Sources/OmniWM/Core/Controller/WindowAdmissionRetry.swift`:
- Around line 502-513: Update the .ruleReevaluation handling in the
WindowAdmissionRetry flow to store the created Task in AdmissionRetryState,
cancel or replace it during reset and shutdown, and capture the current
generation when launching it. Before applying a stale outcome or calling
scheduleTrackedTilingPromotionRetry, verify the task still belongs to the active
generation and has not been cancelled, preventing post-reset work from
rescheduling retries.
In `@Sources/OmniWM/Core/Controller/WindowAdmissionTracking.swift`:
- Around line 228-278: Update applyFramesParallel to defer retry decisions until
the terminal frame-result observer/callback reports the outcome, rather than
checking recentFrameWriteFailure immediately after applyFloatingCreateFrame. On
an actual .contextUnavailable result, warm the AX context and reapply the frame
while preserving the existing service guards and preventing premature retries.
- Around line 47-60: Update the existing-identity branch in the guard around
trackedToken and candidate.token to call finishAdmissionRetryAfterTracking
before returning. Ensure the admission retry is finalized when admissionReplaced
is recorded, while preserving the trace and early-return behavior.
In `@Sources/OmniWM/Core/Diagnostics/AutomaticAXSnapshot.swift`:
- Around line 250-258: Update selectWindowElement so a requested windowId that
is not found among the limited windows does not fall back to focusedWindow;
return nil instead. Preserve the existing focusedWindow result only when
windowId is absent, ensuring captured status cannot be reported for an unrelated
window.
In `@Sources/OmniWM/Core/Diagnostics/WindowAdmissionFinalizationTargets.swift`:
- Around line 90-97: Update the pending-target cleanup condition in the
finalization logic to also clear candidates whose action is .enumerationEmpty
when they match the same PID, nil window ID, and context; preserve the existing
.enumerationFailed handling so successful enumeration cannot retain a stale
empty result.
In `@Sources/OmniWM/Core/Diagnostics/WindowAdmissionTrace.swift`:
- Around line 663-673: Update pruneAuxiliaryState to include state.processes in
the pruning guard and filter it using the observed PID set, alongside
state.endpoints. Ensure process entries whose PIDs are absent from the records
snapshot are removed while retaining entries still referenced by records.
In `@Sources/OmniWM/Core/Diagnostics/WMController`+Diagnostics.swift:
- Around line 149-166: Update the private stream function to cap copied crash
evidence at the configured crash-log size limit, such as
RuntimeTraceLimits.captureBytes, tracking bytes written and stopping once the
limit is reached. Preserve the existing read-error mapping and ensure no more
than the configured maximum is written, including when the final chunk exceeds
the remaining allowance.
In `@Sources/OmniWM/Core/Intake/FactResolver.swift`:
- Around line 180-186: Update the focused-window resolution flow around
canonicalObservedWindowToken and handleActivationFactsResolved to return nil
immediately after recording a PID mismatch between elementPid and the requested
pid. Preserve normal token resolution for matching or unavailable process
identifiers, while preventing foreign AX elements from producing activation
facts under the requested PID.
In `@Sources/OmniWM/Core/Workspace/WorkspaceManager.swift`:
- Around line 2446-2449: Update the confirmedMissingKeys call in
WorkspaceManager to pass the current space topology instead of relying on its
empty default, ensuring inactive-Space windows remain protected during full
rescans.
In `@Sources/OmniWM/UI/DiagnosticsSettingsTab.swift`:
- Around line 278-288: Handle .writeFailed(reason) separately in startRecording
within Sources/OmniWM/UI/DiagnosticsSettingsTab.swift lines 278-288, displaying
the supplied reason instead of “Unexpected recording state”; keep .stopped on
the existing unexpected-state path. Apply the same case split and reason display
in the issue-report recording flow in
Sources/OmniWM/UI/ReportIssue/ReportIssueSettingsTab.swift lines 435-447.
In `@Sources/OmniWM/UI/RuleApplicationSection.swift`:
- Around line 90-99: Update selectApp(_:) so selecting an app with a bundleId
also clears or disables the existing app-name matcher state, including
appNameMatcherEnabled and appNameSubstring, while preserving the current
fallback behavior for apps without a bundle ID.
In `@Tests/OmniWMTests/DiagnosticsTraceRecorderTests.swift`:
- Around line 9-19: Update DiagnosticsEvidenceGate in
Tests/OmniWMTests/DiagnosticsTraceRecorderTests.swift:9-19 to latch release
state so wait() returns immediately when released beforehand, and update the
evidence test at Tests/OmniWMTests/DiagnosticsTraceRecorderTests.swift:188-233
to await confirmed gate entry before releasing it. Update the gate helper in
Tests/OmniWMTests/IssueReporterTests.swift:581-598 to latch releases and expose
an async waiter-count or entry handshake, then replace expectations and
Task.yield() scheduling assumptions in
Tests/OmniWMTests/IssueReporterTests.swift:340-404 with that deterministic
handshake.
In `@Tests/OmniWMTests/ManagedWindowIdentityTests.swift`:
- Around line 20-23: Update waitUntilEntered() to use a bounded, timeout-aware
wait or expectation instead of yielding indefinitely while entered is false.
Ensure all corresponding gate-entry waits identified in the diff fail
deterministically when provider entry does not occur, while preserving
successful completion once entered becomes true.
In `@Tests/OmniWMTests/WindowAdmissionRetryIdentityTests.swift`:
- Around line 14-40: In all four retry tests, register cancellation with defer
immediately after each successful retry scheduling call, using the corresponding
windowId and cancellation method. Apply this to both scheduleAdmissionRetry and
scheduleCandidateAdmissionRetry paths, and remove or avoid relying solely on the
final explicit cancellation so cancellation still occurs when a later XCTUnwrap
or assertion exits early.
In `@Tests/OmniWMTests/WindowAdmissionTestSupport.swift`:
- Around line 48-58: Update drainLayoutRefreshes(_:) to use a deadline or
bounded polling limit, avoiding an unbounded await on activeRefreshTask.value
and detecting continually requeued pending work. When the deadline is exceeded,
fail the test explicitly; retain the existing successful return once no active
task and no pending refresh remain.
---
Nitpick comments:
In `@Sources/OmniWM/Core/Ax/RunLoopJob.swift`:
- Around line 31-39: Add documentation above RunLoopJob.performUnlessCancelled
explaining that body executes while the state mutex is held, must remain short,
and must not re-enter this RunLoopJob through isCancelled, checkCancellation(),
or cancel() to avoid deadlock.
In `@Tests/OmniWMTests/CGSPhantomEventGuardTests.swift`:
- Line 41: Update testCGSCreateForTrackedWindowVerifiesIdentityWithoutMutation
to capture the tracked AX reference or admission identity before dispatching the
create event, then assert it is unchanged afterward. Keep the existing
event-observation assertions and verify the same stored identity remains
associated with the tracked window.
In `@Tests/OmniWMTests/WindowClassificationRegressionTests.swift`:
- Around line 35-44: Update testObservedDecisionDoesNotDefineExpectedBehavior so
the mutated observedDecision.disposition is derived from
fixture.expectedDecision rather than hard-coded as "floating"; choose a value
guaranteed to differ from the expected disposition while preserving the test’s
assertion that recomputation matches expectedDecision and not the observation.
In `@Tests/OmniWMTests/WorkspaceMoveFocusBehaviorTests.swift`:
- Around line 29-30: Update
testNiriAdjacentWindowMoveHonorsFollowSettingWithEmptySourceAndDynamicDestination
and the corresponding adjacent-move test to parameterize over both .up and .down
directions. For each direction, configure the source workspace with the matching
neighboring workspace and assert the existing follow-setting behavior for both
false and true.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c028c5a-2a59-407b-b5c8-0fadeedec64e
📒 Files selected for processing (106)
Info.plistMakefileScripts/package-app.shSources/OmniWM/Core/Ax/AXCallbackGenerationRegistry.swiftSources/OmniWM/Core/Ax/AXFrameApplicationLedger.swiftSources/OmniWM/Core/Ax/AXManager.swiftSources/OmniWM/Core/Ax/AXWindow.swiftSources/OmniWM/Core/Ax/AXWindowDump.swiftSources/OmniWM/Core/Ax/AXWindowEnumeration.swiftSources/OmniWM/Core/Ax/AppAXContext.swiftSources/OmniWM/Core/Ax/RunLoopJob.swiftSources/OmniWM/Core/Ax/Thread+RunLoop.swiftSources/OmniWM/Core/Config/AppRule.swiftSources/OmniWM/Core/Config/SettingsStore.swiftSources/OmniWM/Core/Controller/AXEventHandler+ManagedWindowIdentity.swiftSources/OmniWM/Core/Controller/AXEventHandler.swiftSources/OmniWM/Core/Controller/LayoutRefreshController+WindowLifecycle.swiftSources/OmniWM/Core/Controller/LayoutRefreshController.swiftSources/OmniWM/Core/Controller/RunningAppInventory.swiftSources/OmniWM/Core/Controller/ServiceLifecycleManager.swiftSources/OmniWM/Core/Controller/WMController.swiftSources/OmniWM/Core/Controller/WindowActionHandler.swiftSources/OmniWM/Core/Controller/WindowAdmissionIdentity.swiftSources/OmniWM/Core/Controller/WindowAdmissionLifecycle.swiftSources/OmniWM/Core/Controller/WindowAdmissionRetirement.swiftSources/OmniWM/Core/Controller/WindowAdmissionRetry.swiftSources/OmniWM/Core/Controller/WindowAdmissionTracking.swiftSources/OmniWM/Core/Controller/WorkspaceNavigationHandler.swiftSources/OmniWM/Core/Diagnostics/AutomaticAXSnapshot.swiftSources/OmniWM/Core/Diagnostics/BorderOpMetricsRecorder.swiftSources/OmniWM/Core/Diagnostics/DiagnosticsEventRecorder.swiftSources/OmniWM/Core/Diagnostics/DiagnosticsFileScanner.swiftSources/OmniWM/Core/Diagnostics/DiagnosticsRetention.swiftSources/OmniWM/Core/Diagnostics/FrameApplyRecorder.swiftSources/OmniWM/Core/Diagnostics/LogErrorTap.swiftSources/OmniWM/Core/Diagnostics/RawAXNotificationRecorder.swiftSources/OmniWM/Core/Diagnostics/RingBuffer.swiftSources/OmniWM/Core/Diagnostics/RuntimeTraceCaptureCoordinator.swiftSources/OmniWM/Core/Diagnostics/RuntimeTraceRecording.swiftSources/OmniWM/Core/Diagnostics/WMController+Diagnostics.swiftSources/OmniWM/Core/Diagnostics/WMController+TraceCapture.swiftSources/OmniWM/Core/Diagnostics/WMController+WindowDump.swiftSources/OmniWM/Core/Diagnostics/WindowAdmissionFinalizationTargets.swiftSources/OmniWM/Core/Diagnostics/WindowAdmissionTrace.swiftSources/OmniWM/Core/Diagnostics/WindowClassificationFixture.swiftSources/OmniWM/Core/Diagnostics/WindowClassificationObservation.swiftSources/OmniWM/Core/Diagnostics/WindowDiagnosticDump.swiftSources/OmniWM/Core/Intake/EventIntake.swiftSources/OmniWM/Core/Intake/EventInterpreter.swiftSources/OmniWM/Core/Intake/FactResolver.swiftSources/OmniWM/Core/Layout/Niri/NiriNode.swiftSources/OmniWM/Core/Rules/WindowRuleEngine.swiftSources/OmniWM/Core/Workspace/PlacementResolver.swiftSources/OmniWM/Core/Workspace/WindowModel.swiftSources/OmniWM/Core/Workspace/WorkspaceManager.swiftSources/OmniWM/UI/AppRuleEditor.swiftSources/OmniWM/UI/DiagnosticsSettingsTab.swiftSources/OmniWM/UI/ReportIssue/IssueWalkthroughCard.swiftSources/OmniWM/UI/ReportIssue/ReportIssueSettingsTab.swiftSources/OmniWM/UI/ReportIssue/ReportIssueViewModel.swiftSources/OmniWM/UI/RuleApplicationSection.swiftSources/OmniWM/UI/StatusBar/StatusMenuModel.swiftSources/OmniWM/UI/StatusBar/StatusMenuView.swiftTests/OmniWMTests/AXCallbackGenerationRegistryTests.swiftTests/OmniWMTests/AXFullRescanBoundaryTests.swiftTests/OmniWMTests/AppRuleTests.swiftTests/OmniWMTests/AutomaticAXSnapshotTests.swiftTests/OmniWMTests/CGSPhantomEventGuardTests.swiftTests/OmniWMTests/DiagnosticsRetentionTests.swiftTests/OmniWMTests/DiagnosticsTraceCaptureTests.swiftTests/OmniWMTests/DiagnosticsTraceRecorderTests.swiftTests/OmniWMTests/DurableParkTests.swiftTests/OmniWMTests/DwindleGroupFocusIntegrationTests.swiftTests/OmniWMTests/EventIntakeReplayTests.swiftTests/OmniWMTests/EventInterpreterCallbackGenerationTests.swiftTests/OmniWMTests/Fixtures/WindowClassification/01-standard-window-managed.jsonTests/OmniWMTests/Fixtures/WindowClassification/02-accessory-without-close-floating.jsonTests/OmniWMTests/Fixtures/WindowClassification/03-nonstandard-subrole-dialog-floating.jsonTests/OmniWMTests/Fixtures/WindowClassification/04-missing-fullscreen-button-floating.jsonTests/OmniWMTests/Fixtures/WindowClassification/05-user-rule-force-float.jsonTests/OmniWMTests/Fixtures/WindowClassification/06-browser-pip-builtin-floating.jsonTests/OmniWMTests/Fixtures/WindowClassification/07-user-rule-initial-niri-width.jsonTests/OmniWMTests/Fixtures/WindowClassification/README.mdTests/OmniWMTests/FloatingCreatePlacementTests.swiftTests/OmniWMTests/FullRescanWindowAdmissionTests.swiftTests/OmniWMTests/InputDiagnosticsTests.swiftTests/OmniWMTests/IssueReporterTests.swiftTests/OmniWMTests/ManagedFocusAdmissionTests.swiftTests/OmniWMTests/ManagedFocusAliasAdmissionTests.swiftTests/OmniWMTests/ManagedWindowIdentityTests.swiftTests/OmniWMTests/RunningAppInventoryTests.swiftTests/OmniWMTests/RuntimeArchitectureTests.swiftTests/OmniWMTests/WindowAdmissionFrameLifecycleTests.swiftTests/OmniWMTests/WindowAdmissionIdentityLifecycleTests.swiftTests/OmniWMTests/WindowAdmissionPolicyTests.swiftTests/OmniWMTests/WindowAdmissionRetryIdentityTests.swiftTests/OmniWMTests/WindowAdmissionRetryTests.swiftTests/OmniWMTests/WindowAdmissionTestSupport.swiftTests/OmniWMTests/WindowAdmissionTraceTests.swiftTests/OmniWMTests/WindowClassificationFixtureLoader.swiftTests/OmniWMTests/WindowClassificationRegressionTests.swiftTests/OmniWMTests/WindowClassificationReproducer.swiftTests/OmniWMTests/WindowModelTests.swiftTests/OmniWMTests/WindowRuleEngineTests.swiftTests/OmniWMTests/WorkspaceDeletionEngineCleanupTests.swiftTests/OmniWMTests/WorkspaceMoveFocusBehaviorTests.swift
💤 Files with no reviewable changes (4)
- Sources/OmniWM/Core/Diagnostics/WindowDiagnosticDump.swift
- Sources/OmniWM/Core/Ax/AXWindowDump.swift
- Sources/OmniWM/Core/Diagnostics/WMController+WindowDump.swift
- Sources/OmniWM/UI/AppRuleEditor.swift
Comment on lines
+574
to
+610
| if let subscribedElement = subscribedWindows[windowId], | ||
| !CFEqual(subscribedElement, element), | ||
| let observer = axObserver.value | ||
| { | ||
| try job.checkCancellation() | ||
| AppAXContext.removeWindowNotifications(observer: observer, element: subscribedElement) | ||
| try job.checkCancellation() | ||
| subscribedWindows[windowId] = nil | ||
| } | ||
| if subscribedWindows[windowId] == nil, let observer = axObserver.value { | ||
| try job.checkCancellation() | ||
| if AppAXContext.addWindowNotifications( | ||
| observer: observer, | ||
| element: element, | ||
| windowId: windowId | ||
| ) { | ||
| try job.checkCancellation() | ||
| subscribedWindows[windowId] = element | ||
| } | ||
| } | ||
| } | ||
|
|
||
| var deadIds: [Int] = [] | ||
| windows.forEachKey { existingId in | ||
| if !seenIds.contains(existingId) { | ||
| let existingElement = windows[existingId] | ||
| let didRemoveSubscription = AppAXContext.removeMissingWindowSubscription( | ||
| windowId: existingId, | ||
| existingElement: existingElement, | ||
| observer: axObserver.value, | ||
| subscribedWindows: subscribedWindows | ||
| let existingWindowIds = Array(windows.value.keys) | ||
| for existingId in existingWindowIds where !seenIds.contains(existingId) { | ||
| let existingElement = windows[existingId] | ||
| try job.checkCancellation() | ||
| let didRemoveSubscription = AppAXContext.removeMissingWindowSubscription( | ||
| windowId: existingId, | ||
| existingElement: existingElement, | ||
| observer: axObserver.value, | ||
| subscribedElement: subscribedWindows[existingId] | ||
| ) | ||
| try job.checkCancellation() | ||
| if didRemoveSubscription { | ||
| subscribedWindows[existingId] = nil | ||
| WindowAdmissionTrace.record( |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep AX subscription mutations and bookkeeping atomic.
These paths can remove/add an AX notification and then throw for cancellation before updating subscribedWindows. That leaves the cache disagreeing with the observer; bindWindowElement also silently accepts failed subscription adds. Update bookkeeping immediately with each AX mutation and propagate add failures.
Also applies to: 837-864, 911-924
🤖 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 `@Sources/OmniWM/Core/Ax/AppAXContext.swift` around lines 574 - 610, The AX
notification update paths around bindWindowElement and the corresponding
sections near the missing-window cleanup must keep observer mutations and
subscribedWindows bookkeeping synchronized: update subscribedWindows immediately
after each successful removal or addition, without an intervening cancellation
check, and propagate failures from addWindowNotifications instead of treating
them as a silent false result. Apply the same atomic ordering to the paths
around removeMissingWindowSubscription and all referenced subscription
mutations.
Comment on lines
627
to
629
| if completeLiveStructuralReplacementCreate(candidate) { | ||
| finishAdmissionRetryAfterTracking(windowId: windowId) | ||
| return |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
identity_file="$(fd -a '^AXEventHandler\+ManagedWindowIdentity\.swift$' . | head -n1)"
test -n "$identity_file" || exit 1
ast-grep outline "$identity_file" \
--match 'rekeyManagedWindowIdentity|completeManagedWindowIdentityRebind' \
--view expanded
rg -n -C8 '\brekeyManagedWindowIdentity\s*\(|\bfinishAdmissionRetryAfterTracking\s*\(' \
"$identity_file" \
Sources/OmniWM/Core/Controller/AXEventHandler.swift \
Sources/OmniWM/Core/Controller/WindowAdmissionRetry.swiftRepository: BarutSRB/OmniWM
Length of output: 16226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Sources/OmniWM/Core/Controller/AXEventHandler+ManagedWindowIdentity.swift"
# Show the result type and the handling semantics around the rebind helper.
rg -n -C6 'enum .*ManagedWindowIdentityRebindResult|isHandled|identityRebindPending|case \.pending|case \.committed|case \.ignored|rekeyManagedWindowIdentity' "$file" \
Sources/OmniWM/Core/Controller/AXEventHandler.swift \
Sources/OmniWM/Core/Controller/WindowAdmissionRetry.swiftRepository: BarutSRB/OmniWM
Length of output: 11122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Sources/OmniWM/Core/Controller/AXEventHandler+ManagedWindowIdentity.swift"
sed -n '1,140p' "$file" | cat -nRepository: BarutSRB/OmniWM
Length of output: 6623
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Sources/OmniWM/Core/Controller/AXEventHandler+ManagedWindowIdentity.swift"
rg -n -C8 'enum ManagedWindowIdentityRebindResult|case \.pending|var isHandled|isHandled\s*:' "$file"Repository: BarutSRB/OmniWM
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Sources/OmniWM/Core/Controller/AXEventHandler+ManagedWindowIdentity.swift"
# Find the exact result enum and any `isHandled` implementation.
rg -n -C12 'ManagedWindowIdentityRebindResult|isHandled|case pending|case committed|case rejected' "$file"Repository: BarutSRB/OmniWM
Length of output: 1229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
"Sources/OmniWM/Core/Controller/WindowAdmissionRetry.swift"
"Sources/OmniWM/Core/Controller/AXEventHandler.swift"
"Sources/OmniWM/Core/Controller/AXEventHandler+ManagedWindowIdentity.swift"
)
rg -n -C10 'finishAdmissionRetryAfterTracking|identityRebindPending|rebind.*pending|scheduled ?\? \.pending|isHandled|completeLiveStructuralReplacementCreate|rekeyManagedReplacement' "${files[@]}"Repository: BarutSRB/OmniWM
Length of output: 33676
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C12 'enum ManagedWindowIdentityRebindResult|struct ManagedWindowIdentityRebindResult|typealias ManagedWindowIdentityRebindResult|var isHandled|case pending|case committed|case rejected' SourcesRepository: BarutSRB/OmniWM
Length of output: 26106
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="Sources/OmniWM/Core/Controller/AXEventHandler+ManagedWindowIdentity.swift"
rg -n -C20 'scheduleManagedWindowIdentityRebind|retryManagedWindowIdentityRebind|completeManagedWindowIdentityRebind|finishManagedWindowIdentityRebind|ManagedWindowIdentityRebindResult|isHandled' "$file"Repository: BarutSRB/OmniWM
Length of output: 13183
Keep .pending out of isHandled. ManagedWindowIdentityRebindResult.isHandled currently treats .pending the same as .committed, so rekeyManagedReplacement(from:to:) can finish admission retry while the identity rebind is still deferred. Return false for .pending and only finish tracking on .committed.
🤖 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 `@Sources/OmniWM/Core/Controller/AXEventHandler.swift` around lines 627 - 629,
Update ManagedWindowIdentityRebindResult.isHandled so .pending returns false and
only .committed returns true. In the completeLiveStructuralReplacementCreate
flow, ensure finishAdmissionRetryAfterTracking is reached only after a committed
identity rebind, not while rekeyManagedReplacement(from:to:) remains deferred.
Comment on lines
+151
to
+193
| guard commitManagedWindowIdentityRebind( | ||
| from: oldWindow.token, | ||
| to: newWindow.token, | ||
| axRef: newWindow.axRef, | ||
| managedReplacementMetadata: managedReplacementMetadata | ||
| ) != nil else { | ||
| if let acknowledgement { | ||
| controller.axManager.rollbackWindowRebind( | ||
| acknowledgement, | ||
| newWindow: newWindow | ||
| ) | ||
| } | ||
| retireStaleManagedWindowIdentityRebind( | ||
| windowId: windowId, | ||
| retryGeneration: retryGeneration | ||
| ) | ||
| controller.layoutRefreshController.requestFullRescan(reason: .staleFullRescan) | ||
| return | ||
| } | ||
| controller.axManager.commitFrameApplicationStateForRebind( | ||
| from: oldWindow, | ||
| to: newWindow | ||
| ) | ||
| if let sizeConstraints { | ||
| controller.workspaceManager.setCachedConstraints(sizeConstraints, for: newWindow.token) | ||
| } | ||
|
|
||
| let finalized = if let provider = managedWindowIdentityRebindFinalizationProvider { | ||
| await provider(oldWindow, newWindow) | ||
| } else { | ||
| await controller.axManager.finalizeWindowRebindContextState( | ||
| from: oldWindow, | ||
| to: newWindow, | ||
| acknowledgement: acknowledgement | ||
| ) | ||
| } | ||
| guard finalized else { | ||
| retireStaleManagedWindowIdentityRebind( | ||
| windowId: windowId, | ||
| retryGeneration: retryGeneration | ||
| ) | ||
| controller.layoutRefreshController.requestFullRescan(reason: .staleFullRescan) | ||
| return |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not commit identity state before AX finalization succeeds.
Lines 151–175 destructively rekey workspace and frame state. If AX finalization then fails, only a rescan is requested, leaving the new token authoritative while AX binding may be incomplete and old frame state is already discarded. Use a two-phase commit or explicitly roll back all three states.
🤖 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 `@Sources/OmniWM/Core/Controller/AXEventHandler`+ManagedWindowIdentity.swift
around lines 151 - 193, Update the rebind flow around
commitManagedWindowIdentityRebind and commitFrameApplicationStateForRebind so
workspace, frame, and managed identity state are not committed before AX
finalization succeeds. Stage the rebind and finalize AX state first, then commit
all three states atomically; if finalization fails, preserve the old state or
explicitly restore every staged change before retiring the retry and requesting
a rescan.
Comment on lines
+179
to
+181
| func contains(_ lhs: AXWindowRef, and rhs: AXWindowRef) -> Bool { | ||
| contains(lhs) && contains(rhs) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect relevant file and related symbols
git ls-files 'Sources/OmniWM/Core/Controller/WindowAdmissionLifecycle.swift' 'Sources/OmniWM/**' | sed -n '1,120p'
echo '--- outline WindowAdmissionLifecycle.swift ---'
ast-grep outline Sources/OmniWM/Core/Controller/WindowAdmissionLifecycle.swift --view expanded || true
echo '--- relevant slice ---'
sed -n '1,260p' Sources/OmniWM/Core/Controller/WindowAdmissionLifecycle.swift
echo '--- search WindowIdentityAliasHistory ---'
rg -n "WindowIdentityAliasHistory|contains\\(_ lhs: AXWindowRef, and rhs: AXWindowRef\\)|contains\\(.*and.*\\)" Sources/OmniWM -SRepository: BarutSRB/OmniWM
Length of output: 15957
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- call site context ---'
sed -n '420,490p' Sources/OmniWM/Core/Controller/WindowAdmissionRetry.swift
echo '--- search alias model ---'
rg -n "struct FullRescanWindowIdentityAliases|enum FullRescanWindowIdentityAliases|FullRescanWindowIdentityAliases|WindowIdentityAliasGeneration|WindowIdentityAliasHistory" Sources/OmniWM/Core -S
echo '--- related slices around alias identity logic ---'
sed -n '1,220p' Sources/OmniWM/Core/Controller/WindowAdmissionIdentity.swiftRepository: BarutSRB/OmniWM
Length of output: 10562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- FullRescanWindowIdentityAliases definition ---'
sed -n '90,170p' Sources/OmniWM/Core/Ax/AXManager.swift
echo '--- alias population path ---'
sed -n '800,880p' Sources/OmniWM/Core/Ax/AXManager.swift
echo '--- any generation/overlap semantics nearby ---'
rg -n "overlap|generation|alias.*bridge|FullRescanWindowIdentityAliases|identityAliasesByWindowId" Sources/OmniWM/Core/Ax/AXManager.swift Sources/OmniWM/Core/Controller/WindowAdmissionIdentity.swift Sources/OmniWM/Core/Controller/WindowAdmissionLifecycle.swift -SRepository: BarutSRB/OmniWM
Length of output: 9291
Don’t treat aliases from different generations as the same window
WindowIdentityAliasHistory.contains(_ lhs:and:) currently returns true when lhs is in current and rhs is in previous (or vice versa). That can classify a window-ID reuse as the same incarnation even when the two refs never overlapped. Require both refs to match within the same generation, or track an explicit overlap bridge.
🤖 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 `@Sources/OmniWM/Core/Controller/WindowAdmissionLifecycle.swift` around lines
179 - 181, Update WindowIdentityAliasHistory.contains(_:and:) to require lhs and
rhs to be contained in the same generation, rather than independently matching
current or previous aliases. Preserve true only for same-generation matches,
unless an explicit overlap bridge records that the generations represent the
same window incarnation.
Comment on lines
+502
to
+513
| case let .ruleReevaluation(token, axRef): | ||
| Task { @MainActor [weak self] in | ||
| guard let self, let controller = self.controller else { return } | ||
| let outcome = await controller.reevaluateWindowRules(for: [.window(token)]) | ||
| if outcome.stale { | ||
| _ = self.scheduleTrackedTilingPromotionRetry( | ||
| token: token, | ||
| axRef: axRef, | ||
| reason: state.reason | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Track and generation-gate the rule-reevaluation task.
This unstructured task is not stored in AdmissionRetryState, so reset or cancellation cannot stop it. It may finish after shutdown and reschedule a retry. Store the task and verify its generation before applying or rescheduling the outcome.
🤖 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 `@Sources/OmniWM/Core/Controller/WindowAdmissionRetry.swift` around lines 502 -
513, Update the .ruleReevaluation handling in the WindowAdmissionRetry flow to
store the created Task in AdmissionRetryState, cancel or replace it during reset
and shutdown, and capture the current generation when launching it. Before
applying a stale outcome or calling scheduleTrackedTilingPromotionRetry, verify
the task still belongs to the active generation and has not been cancelled,
preventing post-reset work from rescheduling retries.
Comment on lines
+90
to
+99
| private func selectApp(_ app: RunningAppInfo) { | ||
| if let bundleId = app.bundleId { | ||
| draft.bundleId = bundleId | ||
| } else { | ||
| draft.bundleId = "" | ||
| draft.appNameMatcherEnabled = true | ||
| draft.appNameSubstring = app.appName | ||
| } | ||
| isPickerExpanded = false | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Synchronize the app-name matcher when changing the selected application.
After selecting an app without a bundle ID, choosing a bundle-ID app leaves the previous app name enabled. The resulting rule can retain criteria for two different applications.
Proposed fix
if let bundleId = app.bundleId {
draft.bundleId = bundleId
+ if draft.appNameMatcherEnabled {
+ draft.appNameSubstring = app.appName
+ }
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private func selectApp(_ app: RunningAppInfo) { | |
| if let bundleId = app.bundleId { | |
| draft.bundleId = bundleId | |
| } else { | |
| draft.bundleId = "" | |
| draft.appNameMatcherEnabled = true | |
| draft.appNameSubstring = app.appName | |
| } | |
| isPickerExpanded = false | |
| } | |
| private func selectApp(_ app: RunningAppInfo) { | |
| if let bundleId = app.bundleId { | |
| draft.bundleId = bundleId | |
| if draft.appNameMatcherEnabled { | |
| draft.appNameSubstring = app.appName | |
| } | |
| } else { | |
| draft.bundleId = "" | |
| draft.appNameMatcherEnabled = true | |
| draft.appNameSubstring = app.appName | |
| } | |
| isPickerExpanded = false | |
| } |
🤖 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 `@Sources/OmniWM/UI/RuleApplicationSection.swift` around lines 90 - 99, Update
selectApp(_:) so selecting an app with a bundleId also clears or disables the
existing app-name matcher state, including appNameMatcherEnabled and
appNameSubstring, while preserving the current fallback behavior for apps
without a bundle ID.
Comment on lines
+9
to
+19
| private actor DiagnosticsEvidenceGate { | ||
| private var continuation: CheckedContinuation<Void, Never>? | ||
|
|
||
| func wait() async { | ||
| await withCheckedContinuation { continuation = $0 } | ||
| } | ||
|
|
||
| func release() { | ||
| continuation?.resume() | ||
| continuation = nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make continuation gates latch releases and expose deterministic waiter entry.
Both helpers can discard release() before a continuation is installed, causing indefinite test hangs.
Tests/OmniWMTests/DiagnosticsTraceRecorderTests.swift#L9-L19: retain released state so a laterwait()returns immediately.Tests/OmniWMTests/DiagnosticsTraceRecorderTests.swift#L188-L233: wait for confirmed gate entry before releasing evidence.Tests/OmniWMTests/IssueReporterTests.swift#L581-L598: latch release state and provide an async waiter-count/entry handshake.Tests/OmniWMTests/IssueReporterTests.swift#L340-L404: replace expectations plusTask.yield()scheduling assumptions with that handshake.
📍 Affects 2 files
Tests/OmniWMTests/DiagnosticsTraceRecorderTests.swift#L9-L19(this comment)Tests/OmniWMTests/DiagnosticsTraceRecorderTests.swift#L188-L233Tests/OmniWMTests/IssueReporterTests.swift#L581-L598Tests/OmniWMTests/IssueReporterTests.swift#L340-L404
🤖 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 `@Tests/OmniWMTests/DiagnosticsTraceRecorderTests.swift` around lines 9 - 19,
Update DiagnosticsEvidenceGate in
Tests/OmniWMTests/DiagnosticsTraceRecorderTests.swift:9-19 to latch release
state so wait() returns immediately when released beforehand, and update the
evidence test at Tests/OmniWMTests/DiagnosticsTraceRecorderTests.swift:188-233
to await confirmed gate entry before releasing it. Update the gate helper in
Tests/OmniWMTests/IssueReporterTests.swift:581-598 to latch releases and expose
an async waiter-count or entry handshake, then replace expectations and
Task.yield() scheduling assumptions in
Tests/OmniWMTests/IssueReporterTests.swift:340-404 with that deterministic
handshake.
Comment on lines
+20
to
+23
| func waitUntilEntered() async { | ||
| while !entered { | ||
| await Task.yield() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the gate-entry wait.
A regression that prevents provider entry causes waitUntilEntered() to yield forever and stalls the entire test run. Use an expectation or timeout-aware entry wait so these tests fail deterministically.
Also applies to: 402-402, 509-509, 553-553, 582-582, 623-623, 703-703
🤖 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 `@Tests/OmniWMTests/ManagedWindowIdentityTests.swift` around lines 20 - 23,
Update waitUntilEntered() to use a bounded, timeout-aware wait or expectation
instead of yielding indefinitely while entered is false. Ensure all
corresponding gate-entry waits identified in the diff fail deterministically
when provider entry does not occur, while preserving successful completion once
entered becomes true.
Comment on lines
+14
to
+40
| XCTAssertTrue( | ||
| controller.axEventHandler.scheduleAdmissionRetry( | ||
| windowId: windowId, | ||
| expectedToken: nil, | ||
| reason: .windowInfoMissing, | ||
| trigger: .create | ||
| ) | ||
| ) | ||
| let initial = try XCTUnwrap(controller.axEventHandler.admissionRetryStateByWindowId[windowId]) | ||
| let token = WindowToken(pid: 467_974, windowId: Int(windowId)) | ||
| let axRef = AXWindowRef(element: AXUIElementCreateApplication(token.pid), windowId: token.windowId) | ||
|
|
||
| XCTAssertTrue( | ||
| controller.axEventHandler.scheduleCandidateAdmissionRetry( | ||
| windowId: windowId, | ||
| pid: token.pid, | ||
| axRef: axRef, | ||
| reason: .factsDeferred | ||
| ) | ||
| ) | ||
|
|
||
| let bound = try XCTUnwrap(controller.axEventHandler.admissionRetryStateByWindowId[windowId]) | ||
| XCTAssertEqual(bound.attempt, initial.attempt) | ||
| XCTAssertEqual(bound.generation, initial.generation) | ||
| XCTAssertEqual(bound.expectedToken, token) | ||
| XCTAssertTrue(CFEqual(bound.axRef?.element, axRef.element)) | ||
| controller.axEventHandler.cancelCreatedWindowRetry(windowId: windowId) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guarantee retry cancellation on early test exit.
Each retry is cancelled only at the end, but an intervening XCTUnwrap can throw first and leave its task active during later tests. Register cancellation with defer immediately after scheduling in all four tests.
Proposed pattern
XCTAssertTrue(
controller.axEventHandler.scheduleAdmissionRetry(
windowId: windowId,
expectedToken: nil,
reason: .windowInfoMissing,
trigger: .create
)
)
+ defer {
+ controller.axEventHandler.cancelCreatedWindowRetry(windowId: windowId)
+ }
let initial = try XCTUnwrap(controller.axEventHandler.admissionRetryStateByWindowId[windowId])
...
- controller.axEventHandler.cancelCreatedWindowRetry(windowId: windowId)Also applies to: 59-74, 82-106, 130-144
🤖 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 `@Tests/OmniWMTests/WindowAdmissionRetryIdentityTests.swift` around lines 14 -
40, In all four retry tests, register cancellation with defer immediately after
each successful retry scheduling call, using the corresponding windowId and
cancellation method. Apply this to both scheduleAdmissionRetry and
scheduleCandidateAdmissionRetry paths, and remove or avoid relying solely on the
final explicit cancellation so cancellation still occurs when a later XCTUnwrap
or assertion exits early.
Comment on lines
+48
to
+58
| static func drainLayoutRefreshes(_ controller: WMController) async { | ||
| while true { | ||
| if let task = controller.layoutRefreshController.layoutState.activeRefreshTask { | ||
| await task.value | ||
| continue | ||
| } | ||
| if controller.layoutRefreshController.layoutState.pendingRefresh == nil { | ||
| return | ||
| } | ||
| await Task.yield() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the refresh drain to prevent indefinite CI hangs.
A refresh task that never completes, or continually requeues pending work, leaves this helper blocked forever. Poll with a deadline and fail the test instead of awaiting an unbounded task.
Proposed fix
+import XCTest
+
static func drainLayoutRefreshes(_ controller: WMController) async {
- while true {
- if let task = controller.layoutRefreshController.layoutState.activeRefreshTask {
- await task.value
- continue
- }
- if controller.layoutRefreshController.layoutState.pendingRefresh == nil {
+ for _ in 0 ..< 500 {
+ let state = controller.layoutRefreshController.layoutState
+ if state.activeRefreshTask == nil,
+ state.activeRefresh == nil,
+ state.pendingRefresh == nil
+ {
return
}
- await Task.yield()
+ try? await Task.sleep(for: .milliseconds(10))
}
+ XCTFail("Layout refreshes did not quiesce within 5 seconds")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static func drainLayoutRefreshes(_ controller: WMController) async { | |
| while true { | |
| if let task = controller.layoutRefreshController.layoutState.activeRefreshTask { | |
| await task.value | |
| continue | |
| } | |
| if controller.layoutRefreshController.layoutState.pendingRefresh == nil { | |
| return | |
| } | |
| await Task.yield() | |
| } | |
| import XCTest | |
| static func drainLayoutRefreshes(_ controller: WMController) async { | |
| for _ in 0 ..< 500 { | |
| let state = controller.layoutRefreshController.layoutState | |
| if state.activeRefreshTask == nil, | |
| state.activeRefresh == nil, | |
| state.pendingRefresh == nil | |
| { | |
| return | |
| } | |
| try? await Task.sleep(for: .milliseconds(10)) | |
| } | |
| XCTFail("Layout refreshes did not quiesce within 5 seconds") | |
| } |
🤖 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 `@Tests/OmniWMTests/WindowAdmissionTestSupport.swift` around lines 48 - 58,
Update drainLayoutRefreshes(_:) to use a deadline or bounded polling limit,
avoiding an unbounded await on activeRefreshTask.value and detecting continually
requeued pending work. When the deadline is exceeded, fail the test explicitly;
retain the existing successful return once no active task and no pending refresh
remain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #488.
With "Follow Window to Monitor" enabled, sending a window to a numbered workspace (Opt+Shift+1-9) already carries focus across, but the up/down bindings (Ctrl+Opt+Shift+Up/Down) and the move-column-to-workspace bindings didn't — they always left focus on the source workspace, regardless of the setting.
The numbered path (
moveFocusedWindow(toRawWorkspaceID:)) branched onfocusFollowsWindowToMonitor, whilemoveWindowToAdjacentWorkspace,moveColumnToAdjacentWorkspace, andmoveColumnToWorkspacechecked nothing and always stayed put. This pulls the follow/stay logic out into a sharedfinishWorkspaceMovehelper and routes all three through it, so they follow focus when the setting is on and stay on the source when it's off.I reused the existing
focusFollowsWindowToMonitorsetting rather than adding a new one, since it already governs same-monitor workspace moves through the numbered path (and matches the intent from #84). No config or UI changes.Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
New Features
Chores