Repository navigation
Send a finite window location in the SkyLight key-window event - #187
Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe key-window event record now uses structured byte encoding with a finite window location, and ChangesKey-window event synthesis
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
Sources/Nehir/Core/PrivateAPIs.swift (1)
39-79: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a byte-level regression test for the event record.
Cover the 0x100-byte size, declared length, event type, window ID, finite
(-1, -1)location, zero padding, and mouseDown-to-mouseUp mutation. If posting is tested, fakeSLPSPostEventRecordTorather than addingForTestsbranches.As per coding guidelines, test hooks must observe behavior and must not alter Nehir-owned decisions; fake the OS boundary instead of the algorithm.
🤖 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/Nehir/Core/PrivateAPIs.swift` around lines 39 - 79, Add byte-level regression coverage for KeyWindowEventRecord.make and setEventType, asserting the 0x100-byte record, declared length, mouseDown default, encoded window ID, finite (-1, -1) location, and zero-filled padding; also verify mutation to mouseUp. If testing posting, fake the SLPSPostEventRecordTo OS boundary without adding ForTests branches or changing Nehir-owned decisions.Source: Coding guidelines
🤖 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/Nehir/Core/PrivateAPIs.swift`:
- Around line 82-85: Update makeKeyWindow around both SLPSPostEventRecordTo
calls to capture and check each OSStatus result instead of discarding it.
Surface either failure through the function’s existing return mechanism or
diagnostic path, while preserving the current mouse-down and mouse-up event
sequence.
---
Nitpick comments:
In `@Sources/Nehir/Core/PrivateAPIs.swift`:
- Around line 39-79: Add byte-level regression coverage for
KeyWindowEventRecord.make and setEventType, asserting the 0x100-byte record,
declared length, mouseDown default, encoded window ID, finite (-1, -1) location,
and zero-filled padding; also verify mutation to mouseUp. If testing posting,
fake the SLPSPostEventRecordTo OS boundary without adding ForTests branches or
changing Nehir-owned decisions.
🪄 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 Plus
Run ID: a06bb9bd-5251-4ffb-b3a5-46054a339482
📒 Files selected for processing (3)
.changeset/20260728144300-send-a-valid-finite-window-location-in-the-synth.md.provenance.jsonSources/Nehir/Core/PrivateAPIs.swift
| var eventBytes = KeyWindowEventRecord.make(windowId: windowId) | ||
| _ = SLPSPostEventRecordTo(&psn, &eventBytes) | ||
| eventBytes[0x08] = 0x02 | ||
| KeyWindowEventRecord.setEventType(.mouseUp, in: &eventBytes) | ||
| _ = SLPSPostEventRecordTo(&psn, &eventBytes) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 '\bSLPSPostEventRecordTo\b|postEventRecordFailed|FallbackFiringRecorder' SourcesRepository: apphane-dev/nehir
Length of output: 1414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PrivateAPIs.swift outline/section =="
sed -n '1,130p' Sources/Nehir/Core/PrivateAPIs.swift
echo
echo "== Search for fallback/diagnostic identifiers in whole repo =="
rg -n -C3 'FallbackFiringRecorder|fallback|postEvent|postEventRecordFailed|handleFailure|focusWindow|makeKeyWindow' Sources || true
echo
echo "== Find files containing focusWindow =="
rg -n 'func focusWindow|focusWindow\(' SourcesRepository: apphane-dev/nehir
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files under Sources/Nehir/Core =="
git ls-files Sources/Nehir/Core | sort
echo
echo "== imports/uses of PrivateAPIs or focusWindow in Core =="
rg -n -C2 'PrivateAPIs|focusWindow\(|makeKeyWindow|PostEvent|KeyWindow' Sources/Nehir/Core | head -n 200
echo
echo "== references to any logging/error/diagnostic reporting identifiers =="
rg -n 'Logger|logging|record.*error|postEventRecordFailed|fallback|Focus.*Failed' Sources/Nehir/Core Sources/Nehir/Input Sources/Nehir/App --glob '*.swift' | head -n 200Repository: apphane-dev/nehir
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== KeyboardFocusLifecycleCoordinator.focusWindow =="
sed -n '180,320p' Sources/Nehir/Core/Controller/KeyboardFocusLifecycleCoordinator.swift
echo
echo "== WindowActionHandler orderWindow and focusWindow fallback code =="
sed -n '210,250p' Sources/Nehir/Core/Controller/WindowActionHandler.swift
sed -n '720,750p' Sources/Nehir/Core/Controller/WindowActionHandler.swift
echo
echo "== focused private API availability and call path =="
sed -n '1,110p' Sources/Nehir/Core/PrivateAPIs.swiftRepository: apphane-dev/nehir
Length of output: 9061
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact callers of focusWindow(pid:windowId:windowRef:) =="
rg -n 'focusWindow\(pid:|focusWindow\([^)]*psn|focusWindowToWindow|windowRef.*focus' Sources || true
echo
echo "== nearby focusWindow implementations outside PrivateAPIs.swift =="
rg -n 'func focusWindow\(pid: pid_t|focusWindow\(pid:' Sources || trueRepository: apphane-dev/nehir
Length of output: 777
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WMController.swift section =="
sed -n '1,140p' Sources/Nehir/Core/Controller/WMController.swift
echo
echo "== all definitions/aliases for Nehir.focusWindow =="
rg -n '(^|[^A-Za-z0-9_])Nehir\.focusWindow|static\s+func\s+focusWindow|func\s+focusWindow\(' Sources/Nehir --glob '*.swift'Repository: apphane-dev/nehir
Length of output: 6214
Handle SLPSPostEventRecordTo failures in both event posts.
makeKeyWindow discards both OSStatus results, so the mouse click used to focus a window leaves the focused state indistinguishable from success when either post fails. Return/status-check the two posts and surface the failure to the caller or available diagnostic path.
🤖 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/Nehir/Core/PrivateAPIs.swift` around lines 82 - 85, Update
makeKeyWindow around both SLPSPostEventRecordTo calls to capture and check each
OSStatus result instead of discarding it. Surface either failure through the
function’s existing return mechanism or diagnostic path, while preserving the
current mouse-down and mouse-up event sequence.
There was a problem hiding this comment.
Verified. This is valid, but it predates this PR and is broader than the two posts: the main path also silently returns on GetProcessForPID and discards _SLPSSetFrontProcessWithOptions, while the command palette duplicates the sequence and returns success. There is no existing return or diagnostic channel to surface into, and changing the best-effort sequence or inventing a new fallback here would be an unverified behavioural change. I recorded a dedicated source-backed discovery (discovery/20260728-private-api-focus-statuses-are-silently-discarded.md) for a narrow observability patch: preserve all four raw statuses, surface them in runtime-trace diagnostics, and keep the current focus sequence until a real failure establishes safe recovery policy. Leaving this PR scoped to the byte-layout hardening.
There was a problem hiding this comment.
@Guria, agreed. The post-status concern is valid, but the complete failure-observability path spans GetProcessForPID, _SLPSSetFrontProcessWithOptions, both SLPSPostEventRecordTo calls, and the command-palette duplicate path. Keeping this PR limited to event-record byte-layout hardening is appropriate.
The recorded discovery’s narrow follow-up—capture all four raw statuses in runtime tracing while preserving the current best-effort sequence—is the right evidence-first path. No change requested here.
✏️ Learnings added
Learnt from: Guria
Repo: apphane-dev/nehir PR: 187
File: Sources/Nehir/Core/PrivateAPIs.swift:82-85
Timestamp: 2026-07-28T15:41:33.447Z
Learning: In `Sources/Nehir/Core/PrivateAPIs.swift`, failures from `GetProcessForPID`, `_SLPSSetFrontProcessWithOptions`, and both `SLPSPostEventRecordTo` calls are currently silently discarded; the command-palette path also duplicates the focus sequence and reports success. Any follow-up should be a narrow observability patch that preserves all four raw statuses in runtime-trace diagnostics without changing focus sequencing, fallback, or recovery policy until real failure evidence establishes a safe behavior.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
The synthesised key-window event record filled its window-location field (bytes 0x20..<0x30) with all 0xFF, which decodes as NaN in both CGPoint coordinates. A synthesised mouse-derived event should carry a valid position, so write a finite off-content point (-1, -1) instead and pad the record buffer to 0x100 bytes while still declaring the 0xF8 payload length. Name the previously magic field offsets while here. This is hardening, not a fix for an observed defect. It is adapted from an upstream change motivated by a report of Chromium web-app windows closing on focus, but that symptom does not reproduce here: on macOS 26.5.2 (25F84) with three Chromium web apps, neither this tree without the change nor the affected upstream release itself shows it, and the upstream report carries no diagnosis and no confirmation. The changeset is therefore level none. Record the upstream provenance for the SkyLight key-window event record
The record is handed to an undocumented SkyLight entry point, so its layout is a wire format with no compiler-checked contract: a wrong offset or width fails silently at runtime rather than at build time. Assert the 0x100 buffer size, the 0xF8 declared length, the key-window flag, the mouseDown default and the mouseUp mutation, the window-id round-trip, the zero padding past the declared length, and that no byte outside those fields is set. The window-location assertion is the point of the file: it must decode as a finite (-1, -1), never NaN. Offsets are restated as literals rather than reusing the production constants, which would assert only that the file agrees with itself. Verified to fail against the pre-change shape. Remove the source comment's unconfirmed claim that Chrome PWA windows close because of NaN coordinates; the upstream report has no diagnosis or confirmation, and its symptom does not reproduce here. Adapted from the upstream test that accompanies the ported change; recorded in .provenance.json.
Summary
makeKeyWindowsynthesises the SkyLight key-window event record that Nehir posts to make a specific window key. Its window-location field — bytes0x20..<0x30, read back as aCGPoint, i.e. two native-endianDoubles — was filled with0xFFin every byte, which decodes as NaN in both coordinates.This writes a finite off-content point
(-1, -1)there instead, pads the record buffer to0x100bytes while still declaring the0xF8payload length, and gives the previously magic offsets names (windowLocationOffset,keyWindowFlagOffset,windowIdOffset, …). Behaviour is otherwise unchanged: samemakeKeyWindow/focusWindowsignatures, same two posts (.mouseDownthen.mouseUp).The source change is in
Sources/Nehir/Core/PrivateAPIs.swift; byte-layout coverage lives in a new per-behaviour test file, with.provenance.jsonaudit entries for both adaptations and a level-nonechangeset.Why this is hardening, not a bug fix
A synthesised mouse-derived event should carry a valid position. Sending NaN is wrong on its own terms, and that is the whole justification here.
It is adapted from an upstream change (recorded in
.provenance.json) motivated by a report of Chromium web-app windows closing on focus. That symptom does not reproduce. Checked on macOS 26.5.2 (25F84) — the same build named in the upstream report — with three Chromium web apps open across two browsers (one Chrome PWA, two Helium PWAs):focus_direction_dispatch direction=right) onto the Chrome web app,managed_focus_requested→managed_focus_confirmed, focus away and back repeatedly. No destroy events; the window ends the capturephase=tiled hidden=nil observedVisible=true.startedServices=false, all three web apps admitted viacontext=startup_full_rescan, all three focused during the capture, all three alive at the end. This is the upstream report's headline scenario.The upstream issue has zero comments — no diagnosis, and no confirmation from the reporter that the change helped. So the causal chain "NaN location → Chromium mishandles it → window closes" is an undemonstrated hypothesis. The one variable left unexcluded is the Chromium build, which auto-updates.
Full write-up with the inlined evidence is on the planning branch:
discovery/20260728-skylight-key-window-nan-location-symptom-not-reproducible.md.Tests
The user explicitly approved byte-layout coverage after reviewing the implementation and the non-reproduction evidence.
Tests/NehirTests/SkyLightKeyWindowEventRecordTests.swiftis a new per-behaviour Swift Testing file adapted from the upstream companion test.It independently restates the byte offsets rather than reusing production constants, and pins:
0x100and declared length0xF8at0x04;0x3C;CGPoint(x: -1, y: -1)at0x20;0xF8and no unexpected non-zero byte elsewhere.A falsification run temporarily restored the pre-change
0xF8buffer and NaN location. The suite failed on both intended assertions (248 vs 256 bytes; decoded(nan, nan)vs finite(-1, -1)), then passed again after restoring the implementation.Release notes
Choose one:
.changeset/*.mdfor user-visible changes (mise run changeset -- patch "..."). — levelnone: the change is real but there is no user-visible behaviour to announce.mise run changeset:checkpasses on it, so nono release notelabel is needed.no release notelabel to make CI skip intentional.Validation
mise run check(format + lint + build + test) — passes after rebasing onto currentmain; 1491 tests in 130 suites.mise run license:check— passes; SPDX headers up to date.mise run changeset:check— "Changeset present."Pre-existing flake, unrelated to this branch.
IPCServerTestsintermittently dies with SIGTRAP:HotkeyCenter.deinitcallsMainActor.assumeIsolatedoff the main actor during teardown, reached fromWMController.deinit←IPCApplicationBridge.deinit←IPCConnection.deinit. Reproduced on the base commit without this change (failed on run 10 of 15 of the same filter). Not fixed here; worth its own issue.