Eclipse Messages behind an external window when no Beeper window is found#92
Conversation
Reworks EclipsingWindowCoordinator's anchor resolution following review: - Model the anchor as a resolved-value AnchorWindow built via per-source factories, replacing the struct-of-optionals that filled fields differently per path. - Resolve all main-thread-affined AppKit reads (NSApp.windows, NSWorkspace, NSScreen, NSWindow) eagerly inside a main-thread hop, since makeAutomatable runs on a background queue. - Fix the Cocoa<->screen coordinate flip to use the primary display height rather than the window's own screen, correcting positioning and screen lookup on mixed-height multi-monitor setups. Extract the flip into a shared NSRect.flippedBetweenCocoaAndScreenSpace() helper and route OnboardingManager through it (also fixing its NSScreen.main usage). - For the external-window fallback: select the topmost (frontmost) window instead of largest-by-area, drop the fixed min-size pre-filter in favor of the uniform encompass guard, and skip near-transparent windows. Relies on BetterSwiftAX 0.1.3, where Window.listDescriptions now skips individual malformed descriptors instead of throwing the whole list. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughRefactors eclipse placement to compute geometry from a resolved AnchorWindow in screen/AX space, adds an NSRect flip helper for Cocoa↔screen coordinates, and updates a dependency. EclipsingWindowCoordinator now resolves an Electron-first anchor with an external-window fallback and uses its screen-frame for positioning. ChangesWindow Coordinate Space and Anchor Positioning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Hardens EclipsingWindowCoordinator.makeAutomatable so automation no longer hard-throws when there is no Electron/Beeper window: it now falls back to an external on-screen window as the eclipse anchor. Along the way, it consolidates anchor data into a resolved AnchorWindow value, eagerly hops AppKit reads to the main thread (since makeAutomatable runs off-main), and centralizes the Cocoa↔screen-space flip into a shared NSRect helper that OnboardingManager also adopts. Also bumps BetterSwiftAX 0.1.2 → 0.1.3 so Window.listDescriptions is resilient to malformed entries.
Changes:
- Add external-window fallback for the eclipse anchor with
AnchorWindow.electron/.externalfactories and a main-thread-synchronous resolver. - Extract
NSRect.flippedBetweenCocoaAndScreenSpace()(using primary-display height) and use it from both eclipsing and onboarding paths. - Replace the fixed min-size pre-filter with the uniform
shouldOnlyEclipseIfEncompassesguard, skip near-transparent/fading windows, and pin BetterSwiftAX to 0.1.3 inPackage.resolved.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swift | Introduce AnchorWindow, external-window fallback, main-thread hop, and shared screen/coordinate helpers. |
| src/IMessage/Sources/IMessage/OnboardingManager.swift | Use shared flippedBetweenCocoaAndScreenSpace() instead of ad-hoc NSScreen.main-based flip. |
| src/IMessage/Sources/IMessage/Extensions.swift | Add reusable NSRect.flippedBetweenCocoaAndScreenSpace() helper based on primary display height. |
| Package.resolved | Update BetterSwiftAX pin from 0.1.2 to 0.1.3 and normalize repo URL casing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![indent[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| /// CGWindow bounds). The flip is about the primary display's height, so it is | ||
| /// its own inverse and is correct regardless of which display the rect is on. | ||
| func flippedBetweenCocoaAndScreenSpace() -> NSRect { | ||
| guard let primaryHeight = NSScreen.screens.first?.frame.height else { return self } |
There was a problem hiding this comment.
Silent no-op when no displays are attached: returning self here means callers (OnboardingManager, EclipsingWindowCoordinator.screenFrame(for:)) silently use an unflipped rect. The same guard appears in EclipsingWindowCoordinator.screen(containing:). In practice macOS without displays is rare, but if it ever happens (headless session, screen sleep race) the misplacement will be hard to diagnose. Consider a log.warning(...) before falling through, matching the warning style already used in screenFrame(for:).
|
|
||
| if let description = externalEclipsingAnchorWindow(messagesPID: messagesPID) { | ||
| let anchor = AnchorWindow.external(description) | ||
| log.notice("falling back to external frontmost window for eclipsing: \(anchor.description)") |
There was a problem hiding this comment.
Repeat log.notice on every automation: makeAutomatable is invoked on each prepareForAutomation call (see MessagesController.swift), so this .notice fires every time Messages automation runs while Beeper is closed. Existing logging style in this file uses .debug for repeated state and .notice for transitions. Suggest demoting to .debug or stashing the last-chosen anchor on the coordinator and only emitting .notice when it changes.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swift (1)
75-99:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude the configured offsets in the “encompasses” check.
Lines 75-77 only validate
targetSize, but Lines 94-95 can still move the final rect outside the anchor. With non-zeroeclipsingOffsetX/Y(or a tight right-aligned anchor),shouldOnlyEclipseIfEncompassescan pass even though the target window still protrudes.🤖 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 `@src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swift` around lines 75 - 99, The current encompasses guard only checks targetSize against anchorWindow.screenFrame but ignores Self.eclipsingOffsetX/Y and right-alignment, so the final targetRect created in the targetOrigin closure can protrude; compute the same targetOrigin logic (respecting Self.eclipsingAlignment and adding Self.eclipsingOffsetX and Self.eclipsingOffsetY) before the guard and then validate that anchorWindow.screenFrame fully contains the resulting NSRect(origin: targetOrigin, size: targetSize) (or use the existing encompasses helper) when Self.shouldOnlyEclipseIfEncompasses is true; update the guard and error log to use that adjusted rect check so the decision matches how targetRect is actually constructed.
🤖 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
`@src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swift`:
- Around line 69-70: makeAutomatable is dereferencing NSScreen (accessing
screen.frame/visibleFrame) on the automation thread via AnchorWindow.screen;
change AnchorWindow to store plain NSRect/strings for screenFrame and
screenVisibleFrame (populate these on the main thread where AnchorWindow is
created—see where AnchorWindow.screen is set around the population at ~154-160,
169-181) and update makeAutomatable to read those stored rects/formatted values
instead of accessing NSScreen. Also fix the shouldOnlyEclipseIfEncompasses
logic: when computing whether the eclipsing window fits, include
eclipsingOffsetX, eclipsingOffsetY and alignment adjustments so the final
targetRect (not just anchorWindow.screenFrame.size vs targetSize) is tested for
containment/fit before early-return; update any checks that reference
anchorWindow.screenFrame.size and targetSize to use the computed targetRect
bounds.
---
Outside diff comments:
In
`@src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swift`:
- Around line 75-99: The current encompasses guard only checks targetSize
against anchorWindow.screenFrame but ignores Self.eclipsingOffsetX/Y and
right-alignment, so the final targetRect created in the targetOrigin closure can
protrude; compute the same targetOrigin logic (respecting
Self.eclipsingAlignment and adding Self.eclipsingOffsetX and
Self.eclipsingOffsetY) before the guard and then validate that
anchorWindow.screenFrame fully contains the resulting NSRect(origin:
targetOrigin, size: targetSize) (or use the existing encompasses helper) when
Self.shouldOnlyEclipseIfEncompasses is true; update the guard and error log to
use that adjusted rect check so the decision matches how targetRect is actually
constructed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21ce808c-76a3-43b6-946a-024bed4e423f
📒 Files selected for processing (4)
Package.resolvedsrc/IMessage/Sources/IMessage/Extensions.swiftsrc/IMessage/Sources/IMessage/OnboardingManager.swiftsrc/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swift
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-05-03T17:00:19.662Z
Learnt from: KishanBagaria
Repo: beeper/platform-imessage PR: 69
File: src/IMessage/Sources/IMessage/EventWatcher/EventWatcher+Updates.swift:89-93
Timestamp: 2026-05-03T17:00:19.662Z
Learning: In the beeper/platform-imessage Swift codebase, keep message IDs (`PlatformSDK.MessageID`) as raw GUIDs. When mapping from DB/event rows to `message.id`, set the ID directly from `msgRow.guid` (no GUID→public-ID hashing or transformation). For multi-part messages, append the part index as `_<part.index>` to the GUID-derived ID. During code review, if changes touch message ID creation/mapping, ensure this raw GUID + optional `_<part.index>` suffix behavior is preserved.
Applied to files:
src/IMessage/Sources/IMessage/OnboardingManager.swiftsrc/IMessage/Sources/IMessage/Extensions.swiftsrc/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swift
🪛 SwiftLint (0.63.2)
src/IMessage/Sources/IMessage/WindowCoordination/WindowCoordinators/EclipsingWindowCoordinator.swift
[Warning] 270-270: Magic numbers should be replaced by named constants
(no_magic_numbers)
Without it, the external-window fallback could pick the Messages window itself, no-opping the eclipse. The coordinator already assumes an `app` is being coordinated, so refuse to proceed when it isn't set. Generated with [Indent](https://indent.com) Co-Authored-By: KishanBagaria <KishanBagaria@users.noreply.github.com>
AnchorWindow.screen was an NSScreen?, and makeAutomatable read screen.frame/visibleFrame from the background automation queue. Capture both frames as NSRect? inside the onMain hop so consumers only touch plain values. Generated with [Indent](https://indent.com) Co-Authored-By: KishanBagaria <KishanBagaria@users.noreply.github.com>
Summary
When the Messages app needs to be made automatable,
EclipsingWindowCoordinatorbriefly shows it behind the Beeper (Electron) window. Previously, if no Electron window could be found, this hard-threw and automation failed. This branch adds a fallback to an external on-screen window as the eclipse anchor, plus a round of hardening on anchor selection and coordinate handling.Changes
AnchorWindow. Modeled via per-source factories (.electron/.external) instead of a struct-of-optionals filled differently per path.NSApp.windows,NSWorkspace,NSScreen,NSWindow) are resolved eagerly inside a main-thread hop, sincemakeAutomatableruns on a background queue.NSRect.flippedBetweenCocoaAndScreenSpace()helper;OnboardingManagernow uses it too (also fixing itsNSScreen.mainusage).shouldOnlyEclipseIfEncompassesguard, and skips near-transparent/fading windows.Dependency
Bumps BetterSwiftAX 0.1.2 → 0.1.3 (beeper/BetterSwiftAX#2), where
Window.listDescriptionsnow skips individual malformed window descriptors instead of throwing the entire list.Testing
swift build --target IMessagepasses. Multi-monitor and no-Electron-window paths have not been exercised at runtime.🤖 Generated with Claude Code