Conversation
Adde XCode previews. Added Drop Here graphics text.
There was a problem hiding this comment.
Pull request overview
Updates the macOS app icon to a new Icon Composer asset (“202602”) and wires it up in the Xcode project so the built app uses the new icon.
Changes:
- Add new Icon Composer asset bundle
Sentinel/202602.icon(withicon.json+ images). - Update Xcode build settings to use
ASSETCATALOG_COMPILER_APPICON_NAME = 202602and include the new icon asset in resources. - Minor Swift code changes (window-close observer,
@MainActoronAppState.shared, whitespace cleanup).
Reviewed changes
Copilot reviewed 6 out of 37 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Sentinel/Styles.swift | Minor whitespace cleanup in SwiftUI views. |
| Sentinel/SentinelApp.swift | Adds a window-close notification observer in AppDelegate. |
| Sentinel/AppState.swift | Marks the singleton shared as @MainActor. |
| Sentinel/202602.icon/icon.json | Defines the new Icon Composer asset layers/gradient and platform support. |
| Sentinel.xcodeproj/project.pbxproj | Adds the new .icon asset to the project and switches the app icon name to 202602. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Add window close notification observer | ||
| NotificationCenter.default.addObserver( | ||
| forName: NSWindow.willCloseNotification, | ||
| object: nil, | ||
| queue: .main | ||
| ) { _ in | ||
| if NSApp.windows.isEmpty { | ||
| NSApp.terminate(nil) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new NSWindow.willCloseNotification observer is redundant with applicationShouldTerminateAfterLastWindowClosed(_:), which already returns true. Also, checking NSApp.windows.isEmpty inside willClose is unlikely to ever be true because the closing window is still present when willClose fires, so this probably won’t terminate as intended. Prefer removing this observer entirely (keep the delegate method), or move the logic into applicationShouldTerminateAfterLastWindowClosed / use a post-close notification (or dispatch async) if custom behavior is needed.
|
|
||
| // Add window close notification observer | ||
| NotificationCenter.default.addObserver( |
There was a problem hiding this comment.
These changes (window-close observer) don’t appear related to the PR’s stated goal of updating the app icon. Consider moving this behavior change into a separate PR (or at least calling it out in the PR description) to keep review/rollback scoped to the icon update.
| // Add window close notification observer | ||
| NotificationCenter.default.addObserver( |
There was a problem hiding this comment.
Indentation is off here (the comment is indented more than the surrounding code), which makes the block harder to read and looks like an accidental tab/space change. Align it with the other statements in applicationDidFinishLaunching(_:).
|
|
||
| class AppState: ObservableObject { | ||
| static let shared = AppState() | ||
| @MainActor static let shared = AppState() |
There was a problem hiding this comment.
This PR is titled/described as an app icon update, but this change introduces a concurrency/actor-isolation behavior change for the AppState singleton. If this is intentional (e.g., to address main-thread publishing warnings), it should be called out in the PR description; otherwise consider moving it to a separate PR to keep scope focused.
| files = ( | ||
| C7BEBE212E846AB400B56DE9 /* Assets.xcassets in Resources */, | ||
| 3002E9352F38E9A5008068BA /* 202602.icon in Resources */, | ||
| C7374B672E79C8110015DEED /* Sentinel.icon in Resources */, |
There was a problem hiding this comment.
Now that the app icon build setting points to 202602, the old Sentinel.icon is likely no longer used by the main app target but is still being copied into the app’s Resources. If it’s not needed for anything else, removing it from the Resources build phase will avoid shipping two icon composer assets and reduce bundle size/confusion.
| C7374B672E79C8110015DEED /* Sentinel.icon in Resources */, |
1 participant
It took a long time, but this is the one.