[codex] Add Cycle Monitors content rotation#302
Open
ABaldetti wants to merge 113 commits into
Open
Conversation
Replace the Niri axis solver, viewport geometry helpers, and monitor restore assignment matcher with Zig implementations behind a small C ABI surface. Add a dedicated COmniWMKernels header target, a reproducible kernel build script, focused Swift and Zig regression coverage, and the documentation/build updates needed to rebuild and test the new library.
Stabilize the layout and CLI regression coverage after the Zig kernel port. Enable animations explicitly in the shared layout-plan fixture, make the scratchpad async reveal test use the controller-local AX frame override after setup refreshes settle, add deterministic viewport settling in the Niri animation-toggle regressions, widen the injected CLI watch timeouts under aggregate load, and ignore Zig build cache output.
Pin shared build metadata for the macOS target, Zig toolchain, and Ghostty archive digest. Add reusable preflight helpers plus canonical make targets for build, test, verify, and release-check workflows. Refactor Zig and packaging scripts to consume the pinned metadata and fail fast on provenance or configuration mismatches.
Introduce per-target SwiftLint configs and target-specific thresholds so the stricter root lint policy can be rolled out without forcing legacy-heavy modules to fail immediately. Clean up the surfaced violations across Core, UI, IPC, Quake Terminal, and tests by replacing unsafe casts and `try!` usage, removing unnecessary `async`, tightening access control, and marking unsupported `init(coder:)` paths unavailable. Refresh the contributor docs with the supported `make build`, `make test`, `make verify`, and `make release-check` workflows, and align the AX/Niri regressions with the cleanup.
Rewrite the scratchpad async fronting test to use the real async AX frame-apply path instead of the synchronous frameApplyOverrideForTests hook. Add a small layout-plan test helper that installs an AppAXContext and disables the default synchronous override for async-sensitive tests. This keeps the production scratchpad code unchanged while stabilizing the previously failing scratchpad test and the full Swift suite.
Move the pure Dwindle frame solver behind the COmniWMKernels C ABI and have Swift flatten the workspace tree into a kernel snapshot before reapplying solved frames to existing nodes. This removes the Swift gap and min-size recursion, adds the new Zig solver entry point, and expands Dwindle regression coverage for single-window, fullscreen, gap, placeholder, and min-size behaviors. Tested with: swift test --filter DwindleLayoutEngineTests
Fix typo in README for 'Scratchpad'
Stop using pending reveal transactions for workspaceInactive reveals so cross-workspace activation no longer re-hides windows or re-suppresses frame writes after transient verification misses. Keep delayed reveal verification for floating and scratchpad-style async restores, and add regression coverage for the layout refresh, inactive-workspace activation, and workspace-bar focus paths. Fixes BarutSRB#204 Fixes BarutSRB#207
Move Niri's deterministic bulk projection/layout slice out of Swift and into the Zig kernel library behind `omniwm_niri_layout_solve`. Swift keeps ownership of the Niri tree, viewport state, monitor selection, and AppKit policy while flattening the current columns and windows into compact snapshot arrays, invoking one bulk solve, and applying the returned canonical/rendered rects, resolved spans, and hidden-edge classifications back onto the existing nodes. This removes the old Swift-side projection math for container rects, window frames, visibility and overflow handling, and single-window aspect fitting, adds ABI coverage for empty and bounded caller-allocated buffers, and updates the architecture docs to document the Niri leaf-kernel boundary. Tests: - zig build test - swift test --filter NiriLayoutEngineTests - swift test --filter NiriConstraintSolverTests - swift test --filter ViewportGeometryTests - swift test --filter MonitorRestoreAssignmentsTests - swift test --filter NiriLayoutKernelABITests
Move the deterministic reconcile reducer and restore-intent solve behind a compact Zig-backed C ABI. Keep RuntimeStore, Planner, event normalization, trace recording, invariant validation, and runtime object mutation Swift-owned while StateReducer becomes a thin marshal/decode seam over omniwm_reconcile_plan and omniwm_reconcile_restore_intent. Thread persisted hydration through the same solve, add direct ABI coverage plus Zig unit tests for reconcile behavior, and update architecture docs to document the new leaf-kernel boundary. Testing: - zig build test - swift test --filter ReconcileStateTests - swift test --filter ReconcileKernelABITests - make kernels-test - make test
Move keyboard focus border ownership and lifecycle decisions into the BorderCoordinator reconcile loop so border visibility, ordering, and fallback behavior stay consistent across focus changes, CGS events, workspace transitions, and fullscreen/teardown paths. - track managed and fallback owners with generation-scoped state, fallback leases, live-motion revalidation, and bounded trace records so stale frame/teardown events are ignored instead of reviving old borders - derive ordering metadata and corner radius from validated window server state while keeping BorderManager and BorderWindow focused on rendering and preventing hidden config changes from replaying stale target state - route WMController, AXEventHandler, layout refresh, workspace, service lifecycle, and Niri animation border updates through explicit reconcile sources and add regression tests plus architecture notes for the new behavior
Use one exact snappy spring preset for normal-mode animations. Remove balanced/gentle presets and per-call spring tuning. Make Reduce Motion resolve to the exact reducedMotion preset. Switch Niri movement, workspace switching, overview animation, and window-close motion to plain snappy. Add tests for spring config resolution, Niri config wiring, and close-animation settling. Update architecture docs to reflect the new preset model.
Replace Dwindle's cubic window motion with a dedicated no-bounce spring configuration and thread display refresh rate through workspace snapshots so motion settles consistently across monitors. Snap canonical and animated Dwindle frames to physical pixels, seed new split insertions from the split edge, and make fullscreen exclusive per workspace to avoid overlapping fullscreen leaves. Unify Dwindle settings application through the handler snapshot context, remove the unused cubic animation path, and harden AX frame writes by rejecting invalid target geometry without retrying. Add regression coverage for Dwindle animation lifecycle, pixel snapping, refresh-rate plumbing, insertion seeds, fullscreen exclusivity, and AX invalid-frame rejection. Verified with: - swift test --filter 'DwindleLayoutEngineTests|SpringAnimationTests|AXManagerTests' - swift test --skip-build
Record hotkeys directly from the responder chain instead of relying on a local event monitor. This lets the recorder capture command-based key equivalents before AppKit swallows them, which fixes top-row workspace shortcuts on Czech/QWERTZ and other non-QWERTY layouts. Add regression tests that verify command plus top-row keys are stored by physical key code, including through performKeyEquivalent. Fixes BarutSRB#171
Refresh resolved Niri monitor settings whenever global Niri config changes so monitors inheriting defaults update immediately from the settings window. Route monitor-specific Niri refreshes through the same helper and add regression coverage for live center-focused-column and single-window-aspect-ratio propagation.
Accept AXStandardWindow subrole windows during AX enumeration even when the AX role is non-standard or missing, so Emacs-style windows remain tracked across active-space changes and wake/unlock full rescans. Fixes BarutSRB#197
Route quake terminal hide/show through the shared focus pipeline so manual close restores the latest valid focus target instead of replaying a stale app snapshot. Add regression coverage for manual close, focus-loss auto-hide, and external focus fallback paths. Fixes BarutSRB#212
Move the deterministic Overview projection path out of Swift and into the existing Zig kernels library. Replace the generic workspace and Niri overview projection math in OverviewLayoutCalculator with a bulk omniwm_overview_projection_solve FFI call. The new kernel owns frame normalization, scale fitting, projected window and column geometry, drop-zone generation, total content height, and scroll bounds, while Swift keeps snapshot extraction, search, navigation, thumbnails, and result application. Extend the checked-in C ABI with explicit overview snapshot and result structs, add the new Zig solver module, and update the universal archive build step to run ranlib after lipo so the rebuilt static library links cleanly. Add direct ABI regression coverage plus extra Overview projection characterization tests, and verify the change with: - make kernels-build - make kernels-test - swift test --filter OverviewProjectionKernelABITests - swift test --filter Overview
Replace the deterministic WindowRuleEngine base-decision tree with a compact Zig-backed window decision kernel behind omniwm_window_decision_solve. Keep rule compilation, regex matching, title gating, metadata ownership, and manual overrides in Swift while reattaching workspace and rule-effect metadata after the kernel decode. Add ABI coverage, decision regression tests, higher-level admission/manual-override coverage, and architecture notes for the new leaf-kernel boundary.
Seed native fullscreen restore snapshots before command-driven, direct-activation, and full-rescan fullscreen transitions so restored windows can replay their managed geometry after exiting AppKit fullscreen. Keep restore records alive through the first relayout commit, suppress Niri and Dwindle animations during that restore pass, and force-apply the captured frame before clearing lifecycle state. Expand AX event, refresh routing, and layout refresh tests to cover command-driven, direct-activation, replacement-token, and restore-plan finalization flows.
Normalize refresh, focus, and activation controller inputs into an explicit orchestration snapshot/event/plan boundary and route deterministic planning through OrchestrationCore. Thin WMController, LayoutRefreshController, and AXEventHandler to adapter/executor roles, keep runtime ownership and platform effects in Swift, and add seam-level regression tests for refresh coalescing, focus supersede/defer, and native fullscreen restore activation planning.
Replace the Swift orchestration reducer with a Swift encoder/decoder around the new `omniwm_orchestration_step` C ABI. The Zig kernel now owns the deterministic refresh and managed-focus state transitions, while Swift continues to own macOS-facing effects like AX focus, borders, workspace activation, refresh tasks, and retry scheduling. Add the orchestration ABI surface to `COmniWMKernels`, including flattened refresh/focus snapshots, activation observations, window-removal payloads, decisions, ordered actions, and ABI layout reporting. The Swift wrapper grows caller-owned output buffers on `BUFFER_TOO_SMALL` and decodes the returned snapshot, decision, and action plan back into existing OmniWM types. Mirror kernel-owned managed-focus state back into `FocusBridgeCoordinator` and `WorkspaceManager`, and route activation handling through normalized kernel events. This moves retry budget progression, retry exhaustion, owned-application fallback, native fullscreen restore activation, and managed activation confirmation into the reducer boundary. Preserve refresh merge behavior across the port, including affected workspace sets, window-removal payloads, post-layout attachments, visibility follow-ups, and cancelled-refresh restart state. Expand orchestration and ABI tests to cover the new kernel boundary and update architecture docs for the kernel-backed orchestration flow.
Centralize OmniWM status item persistence repair so the main icon and Hidden Bar separator both recover from invalid AppKit preferred-position and visibility restore state before install. Configure both owned status items as mandatory visible surfaces, clear hidden visibility preferences during unsafe-ordering rebuilds, and document targeted recovery for older builds without resetting settings.toml. Fixes BarutSRB#277. Refs BarutSRB#188. Verification: - make build - swift test --filter MenuBarRecoveryTests - git diff --check
Replace global watermark rejection for delayed runtime callbacks with scoped validation against the pending focus request, frame write, or affected workspace transition. Record pending frame writes as runtime transactions with request ids, route workspace transition post-actions through WMRuntime, and expand workspace navigation affected-workspace sets so unrelated mutations no longer cancel valid confirmations. Add Swift and Zig regressions for focus confirmation, AX frame write outcome, workspace transition, and navigation scope behavior.
Add direct runtime-toggle coverage for workspace layout round trips so the user-facing toggle path preserves graph membership, focus, monitor assignment, and Niri viewport state. Rename the graph audit helper to describe the layout-switch invariant it enforces and document the intentionally ignored fields.
Preserve manually moved app workspace placement during automatic rule reevaluation and sibling window admission while keeping explicit rule apply as the path that reapplies workspace defaults to existing windows. Add regression coverage for new sibling windows, automatic reevaluation, and explicit IPC rule apply behavior. Update CLI docs and README to describe initial default placement semantics. Tests: ./Scripts/build-preflight.sh build debug && LIBRARY_PATH="/Users/barut/OmniWM/OmniWM/Frameworks/GhosttyKit.xcframework/macos-arm64_x86_64" swift test --filter "AXEventHandlerTests/newSiblingWindowFollowsMovedAppWorkspaceWhileAutomaticReevaluationPreservesMovedWindow|IPCRuleRouterTests/applyPidExplicitlyReappliesWorkspaceRuleToExistingWindows|IPCRuleRouterTests/applyFocusedReevaluatesManagedFocusAndReturnsUpdatedRulesProjection|IPCRuleRouterTests/applyWindowSupportsExplicitOpaqueIdsAndStableValidationFailures" Fixes BarutSRB#274
Restore workspace-inactive floating windows when their workspace becomes active, let rescue clear stale workspace-inactive hidden records, and trust verified AX frame readback when an app reports a setter error after applying the requested frame. Fixes BarutSRB#224.
Route focused floating-window workspace transfers through workspace graph reassignment instead of requiring a layout-engine node. Preserve floating mode/state across IPC and runtime move-to-workspace flows, and keep layout-plan tests isolated from live platform hooks. Closes BarutSRB#250
Stop clamping the cursor back to the last monitor when movement occurs outside the configured mouse-warp axis. Valid edge warps still run, while ordinary cross-monitor movement now updates the last monitor instead of bouncing the cursor. Replace clamp-specific mouse warp tests with regressions for permissive non-axis movement and outside-all-monitor no-op behavior.
…s SLS misses Some Electron-based apps (verified: WeChat / com.tencent.xinWeChat and ChatGPT Atlas / com.openai.atlas) are silently omitted by SLSWindowQueryWindows(cid, [], _) — the empty-array enumeration that queryAllVisibleWindows() relies on. Their windows are returned when queried by WID directly, but never appear in the empty-array enumeration regardless of the flags bitmap (verified: 0x0, 0x1, 0x2, 0x4, 0x8, 0x10, 0x20, 0x80, 0x100, 0x200, 0x400, 0x800, 0xffffffff). Because the affected pid never makes it into pidsWithWindows, the rest of OmniWM never sees these apps at all — no admission, no AX context, no layout. Restarting OmniWM does not help. Augment pidsWithWindows from the public CGWindowList API as a complement, restricted to layer==0 (regular app windows) and alpha>0 (actually rendered) to avoid Dock / menubar / wallpaper / tooltips. This is additive only — apps already discovered via SLS are unaffected. Closes BarutSRB#290
…XValue error wrappers
AXUIElementCopyMultipleAttributeValues with options=0 returns *typed
error wrappers* (not nil) for missing attributes. Empirically observed
for AXFullScreenButton on bilibili (com.bilibili.bilibiliPC): an
AXValue containing kAXValueAXErrorType (error -25212 / kAXErrorNoValue),
which is neither NSError nor CFError and slips past the original
nil/NSError filter.
The downstream code only ever calls hasResolvedAttribute() on
close/fullscreen/zoom/minimize button attributes — all of which must
be AXUIElements when present — so accept ONLY genuine AXUIElement
values. Anything else (AXValue error wrapper, CFError, etc.) is
correctly treated as 'attribute absent / unusable'.
Before the fix, the AXValue wrapper was misclassified as 'has button',
which then tripped the type-check guard at line ~538 and set
attributeFetchSucceeded=false on what is otherwise a perfectly normal
window. The heuristic kernel saw attributeFetchFailed and returned
disposition=undecided, leaving bilibili windows deferred and floating.
After the fix:
attributeFetchSucceeded: True (was: False)
admissionOutcome: tracked-floating (was: deferred)
heuristicReasons: ['missingFullscreenButton'] (was: ['attributeFetchFailed'])
The remaining 'floating' disposition is now a legitimate heuristic
decision ('no fullscreen button → probably a tool window') that users
can override with a per-app rule (layout = "tile").
Verified no regression: re-tested every previously-working app
(Claude, Spotify, Cursor, Mail, Calendar, Ghostty, WeChat) — none
changed disposition. Working apps always returned a real AXUIElement
for present buttons, which the new strict check still accepts.
Closes BarutSRB#291
…w-invisibility-cgwindow-augment
…ttribute-strict-axuielement
Drop the static-viewport-preserving Niri window-removal path, including reveal-side/recovery policy fields and layout focus intents. Remove the AX same-app focus preemption/suppression hooks that fed that recovery path, simplify orchestration/topology ABI payloads, and prune the regression tests tied to the removed policy.
Make immediate relayouts check for real Niri viewport/window/column animation work before taking the scroll-animation path, pruning stale display registrations when none remains. When active Niri animations are cancelled, settle viewport and engine motion, apply final frames on demand, and unregister display-link work. Cancel existing animations when starting an Alt-drag move and add regressions for final-frame application and stale scroll registrations.
📝 WalkthroughWalkthroughAdds a new ChangesMonitor Workspace Cycling Feature
Script Maintenance
Sequence DiagramsequenceDiagram
participant User as rgba(64,64,255,0.5)
participant CLI_IPC as rgba(0,128,128,0.5)
participant WMRuntime as rgba(0,200,0,0.5)
participant WorkspaceNavHandler as rgba(200,100,0,0.5)
participant NiriLayoutEngine as rgba(128,0,128,0.5)
participant WorkspaceSession as rgba(200,0,0,0.5)
User->>CLI_IPC: cycle-monitors command
CLI_IPC->>WMRuntime: promote to layoutMutationAction.cycleMonitors
WMRuntime->>WorkspaceNavHandler: cycleMonitors(source)
WorkspaceNavHandler->>WorkspaceNavHandler: gather active workspaces & eligible entries
WorkspaceNavHandler->>WorkspaceNavHandler: save Niri viewport state
alt can bulk-rotate with Niri
WorkspaceNavHandler->>NiriLayoutEngine: rotateWorkspaceContents(workspaceIds)
NiriLayoutEngine-->>WorkspaceNavHandler: success/failure
else fallback reassignment
WorkspaceNavHandler->>WorkspaceNavHandler: manually move/reassign windows
end
WorkspaceNavHandler->>WorkspaceSession: apply session snapshot/patches (viewport/focus)
WorkspaceNavHandler-->>CLI_IPC: ExternalCommandResult.executed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
ABaldetti
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Tests/OmniWMTests/WorkspaceSessionKernelABITests.swift (1)
448-513: ⚡ Quick winSplit this test into arrange/act/assert helpers to satisfy function length lint and keep intent crisp.
The test is solid, but it exceeds the configured body-length threshold and is getting noisy to scan. Extract monitor/workspace fixture creation or assertions into small local helpers.
🤖 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/WorkspaceSessionKernelABITests.swift` around lines 448 - 513, The test `project treats visible monitor cycle as current projection without changing homes` is too long; split into arrange/act/assert helpers to satisfy lint: extract monitor and workspace setup into an "arrange" helper (use makeWorkspaceSessionUUID, makeWorkspaceSessionMonitor, makeWorkspaceSessionWorkspace and WorkspaceSessionKernelStringTable), move the call to withWorkspaceSessionOutput/callWorkspaceSessionKernel into an "act" helper that returns status, output and projectionBuffer, and put the expectation checks (the `#expect` lines validating output.outcome and projectionBuffer entries) into an "assert" helper; replace the inlined arrays and assertions in the test body with calls to these small helpers so the test function body is short and intent remains clear.Tests/OmniWMTests/Transcripts/Goldens/MonitorWorkspaceCycleTests.swift (2)
8-20: 💤 Low valueDrop unused
async throwsto satisfyasync_without_await.
transcriptIsGoldenStabledoes not call anyawaitortryexpression; SwiftLint flags this. The second test legitimately needsasync throws, but this one can be a plainfunc.♻️ Proposed change
- `@Test` `@MainActor` func transcriptIsGoldenStable() async throws { + `@Test` `@MainActor` func transcriptIsGoldenStable() {🤖 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/Transcripts/Goldens/MonitorWorkspaceCycleTests.swift` around lines 8 - 20, transcriptIsGoldenStable currently declares "async throws" but contains no await/try; remove the unnecessary async throws from the function signature of transcriptIsGoldenStable (leave the `@Test` and `@MainActor` attributes and body intact) so it becomes a synchronous test function; do not change the other test that legitimately requires async throws (e.g., the other test in this file that relies on await/try).
58-64: ⚡ Quick winStrengthen post-cycle assertions to verify window relocation.
The current end-state checks confirm only that workspace-to-monitor bindings are unchanged, which is the unconditional invariant of the feature. Consider also asserting that
primaryTokennow resolves to workspace"2"andsecondaryTokento workspace"1", so a regression that no-ops the rotation still satisfies the monitor binding invariant but fails the relocation assertion.🤖 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/Transcripts/Goldens/MonitorWorkspaceCycleTests.swift` around lines 58 - 64, Add assertions after driver.run() that verify the window tokens were actually relocated: assert that primaryToken now resolves to workspaceTwoId and secondaryToken to workspaceOneId. Use the same runtime/controller APIs used elsewhere in the test to resolve a token's workspace (e.g., runtime.controller.workspaceManager.workspaceId(for: primaryToken) == workspaceTwoId and runtime.controller.workspaceManager.workspaceId(for: secondaryToken) == workspaceOneId), placing these checks alongside the existing activeWorkspace(on: primaryMonitorId)/activeWorkspace(on: secondaryMonitorId) expectations.
🤖 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.
Nitpick comments:
In `@Tests/OmniWMTests/Transcripts/Goldens/MonitorWorkspaceCycleTests.swift`:
- Around line 8-20: transcriptIsGoldenStable currently declares "async throws"
but contains no await/try; remove the unnecessary async throws from the function
signature of transcriptIsGoldenStable (leave the `@Test` and `@MainActor` attributes
and body intact) so it becomes a synchronous test function; do not change the
other test that legitimately requires async throws (e.g., the other test in this
file that relies on await/try).
- Around line 58-64: Add assertions after driver.run() that verify the window
tokens were actually relocated: assert that primaryToken now resolves to
workspaceTwoId and secondaryToken to workspaceOneId. Use the same
runtime/controller APIs used elsewhere in the test to resolve a token's
workspace (e.g., runtime.controller.workspaceManager.workspaceId(for:
primaryToken) == workspaceTwoId and
runtime.controller.workspaceManager.workspaceId(for: secondaryToken) ==
workspaceOneId), placing these checks alongside the existing activeWorkspace(on:
primaryMonitorId)/activeWorkspace(on: secondaryMonitorId) expectations.
In `@Tests/OmniWMTests/WorkspaceSessionKernelABITests.swift`:
- Around line 448-513: The test `project treats visible monitor cycle as current
projection without changing homes` is too long; split into arrange/act/assert
helpers to satisfy lint: extract monitor and workspace setup into an "arrange"
helper (use makeWorkspaceSessionUUID, makeWorkspaceSessionMonitor,
makeWorkspaceSessionWorkspace and WorkspaceSessionKernelStringTable), move the
call to withWorkspaceSessionOutput/callWorkspaceSessionKernel into an "act"
helper that returns status, output and projectionBuffer, and put the expectation
checks (the `#expect` lines validating output.outcome and projectionBuffer
entries) into an "assert" helper; replace the inlined arrays and assertions in
the test body with calls to these small helpers so the test function body is
short and intent remains clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30e846c6-170f-41b5-b4e2-97631faf7109
📒 Files selected for processing (26)
Scripts/check-direct-mutation-callers.shSources/OmniWM/Core/Config/SettingsStore.swiftSources/OmniWM/Core/Controller/WorkspaceNavigationHandler.swiftSources/OmniWM/Core/Input/ActionCatalog.swiftSources/OmniWM/Core/Input/HotkeyBinding.swiftSources/OmniWM/Core/Input/InputBindingTrigger.swiftSources/OmniWM/Core/Layout/Niri/NiriLayoutEngine+WorkspaceOps.swiftSources/OmniWM/Core/Runtime/RuntimeControllerOperations.swiftSources/OmniWM/Core/Runtime/WMCommand.swiftSources/OmniWM/Core/Runtime/WMRuntime.swiftSources/OmniWM/IPC/IPCCommandRouter.swiftSources/OmniWMIPC/IPCModels.swiftTests/OmniWMTests/ActionCatalogTests.swiftTests/OmniWMTests/CLIParserTests.swiftTests/OmniWMTests/IPCCommandRouterTests.swiftTests/OmniWMTests/IPCModelsTests.swiftTests/OmniWMTests/KeyboardHotkeyThroughRuntimeTests.swiftTests/OmniWMTests/NiriLayoutEngineTests.swiftTests/OmniWMTests/SettingsStoreTests.swiftTests/OmniWMTests/Transcripts/Goldens/MonitorWorkspaceCycleTests.swiftTests/OmniWMTests/Transcripts/Goldens/MonitorWorkspaceCycleTranscript.swiftTests/OmniWMTests/WorkspaceManagerTests.swiftTests/OmniWMTests/WorkspaceSessionKernelABITests.swiftZig/omniwm_kernels/src/ipc_manifest.jsonZig/omniwm_kernels/src/workspace_session.zigdocs/IPC-CLI.md
💤 Files with no reviewable changes (1)
- Tests/OmniWMTests/WorkspaceManagerTests.swift
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 `@Tests/OmniWMTests/WorkspaceSessionKernelABITests.swift`:
- Around line 463-476: In assertVisibleMonitorCycleProjection, before asserting
home_monitor_id, effective_monitor_id, and projected_monitor_id on each
projectionBuffer entry, add explicit assertions that has_home_monitor_id,
has_effective_monitor_id, and has_projected_monitor_id are true for
result.projectionBuffer[0] and result.projectionBuffer[1]; i.e., assert the
corresponding has_* flags on each entry (has_home_monitor_id,
has_effective_monitor_id, has_projected_monitor_id) first, then verify the ID
values as currently done in assertVisibleMonitorCycleProjection.
🪄 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: 7298f1be-0c67-4bb7-91ca-04e30698666e
📒 Files selected for processing (2)
Tests/OmniWMTests/Transcripts/Goldens/MonitorWorkspaceCycleTests.swiftTests/OmniWMTests/WorkspaceSessionKernelABITests.swift
✅ Files skipped from review due to trivial changes (1)
- Tests/OmniWMTests/Transcripts/Goldens/MonitorWorkspaceCycleTests.swift
Comment on lines
+463
to
+476
| private func assertVisibleMonitorCycleProjection( | ||
| _ result: VisibleMonitorCycleProjectionResult | ||
| ) { | ||
| #expect(result.status == OMNIWM_KERNELS_STATUS_OK) | ||
| #expect(result.output.outcome == UInt32(OMNIWM_WORKSPACE_SESSION_OUTCOME_APPLY)) | ||
| #expect(result.projectionBuffer.count == 2) | ||
| guard result.projectionBuffer.count == 2 else { return } | ||
| #expect(result.projectionBuffer[0].home_monitor_id == 10) | ||
| #expect(result.projectionBuffer[0].effective_monitor_id == 10) | ||
| #expect(result.projectionBuffer[0].projected_monitor_id == 20) | ||
| #expect(result.projectionBuffer[1].home_monitor_id == 20) | ||
| #expect(result.projectionBuffer[1].effective_monitor_id == 20) | ||
| #expect(result.projectionBuffer[1].projected_monitor_id == 10) | ||
| } |
There was a problem hiding this comment.
Add has_* flag assertions before trusting projection field values.
The assertion helper checks home_monitor_id, effective_monitor_id, and projected_monitor_id directly, but never asserts the corresponding presence flags (has_home_monitor_id, has_effective_monitor_id, has_projected_monitor_id). Every other projection-validating block in this file (e.g. Lines 611–615, 673–677, 1162–1166) guards the value checks with an explicit has_* assertion first. Without them, this test could pass even if the kernel emits the "right" field values but leaves has_* as 0 — or if a zero-initialised entry coincidentally matches.
🛡️ Proposed fix
`#expect`(result.projectionBuffer.count == 2)
guard result.projectionBuffer.count == 2 else { return }
+ `#expect`(result.projectionBuffer[0].has_home_monitor_id == 1)
`#expect`(result.projectionBuffer[0].home_monitor_id == 10)
+ `#expect`(result.projectionBuffer[0].has_effective_monitor_id == 1)
`#expect`(result.projectionBuffer[0].effective_monitor_id == 10)
+ `#expect`(result.projectionBuffer[0].has_projected_monitor_id == 1)
`#expect`(result.projectionBuffer[0].projected_monitor_id == 20)
+ `#expect`(result.projectionBuffer[1].has_home_monitor_id == 1)
`#expect`(result.projectionBuffer[1].home_monitor_id == 20)
+ `#expect`(result.projectionBuffer[1].has_effective_monitor_id == 1)
`#expect`(result.projectionBuffer[1].effective_monitor_id == 20)
+ `#expect`(result.projectionBuffer[1].has_projected_monitor_id == 1)
`#expect`(result.projectionBuffer[1].projected_monitor_id == 10)🤖 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/WorkspaceSessionKernelABITests.swift` around lines 463 -
476, In assertVisibleMonitorCycleProjection, before asserting home_monitor_id,
effective_monitor_id, and projected_monitor_id on each projectionBuffer entry,
add explicit assertions that has_home_monitor_id, has_effective_monitor_id, and
has_projected_monitor_id are true for result.projectionBuffer[0] and
result.projectionBuffer[1]; i.e., assert the corresponding has_* flags on each
entry (has_home_monitor_id, has_effective_monitor_id, has_projected_monitor_id)
first, then verify the ID values as currently done in
assertVisibleMonitorCycleProjection.
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
7 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.
Summary
This adds the new
cycleMonitorscommand surface for rotating the visible contents of active monitor workspaces.When multiple monitors have active workspaces, Cycle Monitors sorts the monitors by position and moves the normal managed windows/layout from each active workspace to the next monitor's active workspace, wrapping the last monitor back to the first. The workspace IDs and configured monitor assignments stay fixed.
Behavior
cycleMonitorsaction for hotkeys, runtime commands, IPC, CLI, settings, and command metadataTests And Docs
Validation
Passed:
git diff --checkScripts/check-direct-mutation-callers.shmake kernels-testScripts/check-kernel-abi-goldens.shviamake verifyScripts/check-transcript-coverage.shviamake verifyswift test --filter 'WorkspaceManagerTests|IPCModelsTests|ActionCatalogTests|IPCCommandRouterTests|NiriLayoutEngineTests/rotateWorkspaceContents|MonitorWorkspaceCycleTests' --disable-sandboxBlocked locally:
make verifywas run and reachedmake build, then stopped inScripts/build-preflight.shbecause the local ignored Ghostty archive does not match the pinned digest inScripts/build-metadata.env.Frameworks/GhosttyKit.xcframeworkorScripts/build-metadata.env.fc3f8846ff2fa307dbdeed618331455832bd42b43b1d9f98039a1c90b7a89e5ba91353aff21976810b14c472d650bcc63f0568821367e90866381473f8a50edfNote: no app/package rebuild was performed after the final source cleanup.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests