fix(desktop): read macOS notification authorization in process - #4982
fix(desktop): read macOS notification authorization in process#4982hqhq1025 wants to merge 2 commits into
Conversation
The initial change passed builds but omitted the Knip entry and generated-addon boundary. Register the npm command, reuse the declared Playwright dependency, and exempt only the generated native import. The existing CI Knip gate now passes locally; packaged loading remains smoke-tested.
|
Fixed the CI Knip failures in 17c2924. The smoke script now has an npm entry, imports Electron automation from the declared @playwright/test dependency, and the Desktop Knip configuration exempts only the generated ../native/notification-settings.node import. Reproduced the original failures locally, then verified that npx knip --workspace apps/desktop passes. The npm smoke command also passed with eight concurrent native reads in an ad-hoc signed packaged Electron app. Changed-file lint and diff checks passed. Pre-push self-review found no additional issue; production notification behavior is unchanged. Hosted CI is rerunning. |
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for querying under the running app's bundle identity without prompting or replacing its notification delegate. One P2 remains around the probe's concurrency and failure boundary; details inline.
A separate validation limitation: the new packaged smoke constructs a miniature app, so it does not prove the real signed Maka packaging/IPC path. Prefer extending the existing real-package verifier rather than growing parallel smoke setup. We found no demonstrated packaging defect, and are not treating that missing verification as a separate product bug.
中文
在当前应用身份内只读查询的方向正确。剩余P2是通知探针的并发与故障影响范围。另有验证限制:手工mini-app smoke不能证明真实签名Maka包的路径,建议复用已有真实包verifier;未发现已证打包缺陷,不把验证缺口另定为产品bug。
AI-assisted review by two fresh reviewers, reconciled by the coordinating Codex agent. Native OS-stall behavior was source-traced, not reproduced against a stalled macOS service.
| ); | ||
| handleReconnectableRead(deps.ipcMain, "capabilities:getSnapshot", async () => { | ||
| const snapshot = permissions(); | ||
| const snapshot = await permissions(); |
There was a problem hiding this comment.
[P2] Keep the native notification wait out of unrelated capability/health reads
Reasonable OS-delay path: the native implementation occupies a libuv worker while waiting up to three seconds for the Apple callback. Each call creates fresh async work. Permission Center already requests permissions and capabilities concurrently, and focus/visibility refreshes can overlap; health uses this same complete snapshot too. Yet capability/health only consume accessibility and screen-recording permissions. A delayed notification service therefore blocks unrelated reads and duplicate refreshes can occupy the shared worker pool; queued calls do not start their three-second timeout until a worker becomes available.
Please restrict the notification probe to the permission consumer that needs it, and share an in-flight notification read at the existing reader boundary, clearing it on settlement so later refreshes remain fresh. A delayed-reader regression can verify overlap is coalesced and capability/health do not wait on an unused notification result.
中文
系统通知回调延迟时,每次读取都占一个libuv worker等待最多3秒。权限页会并发查permissions/capabilities,焦点刷新又可重叠,health也走同一路径;后两者却不消费通知结果。建议将通知查询限制到真正需要它的消费者,并在现有reader边界合并未完成的读取,settle后清除。这样能去掉无关故障耦合,并避免重复刷新排满共享线程池。
There was a problem hiding this comment.
Thanks. The independent audit of 17c29247 confirms this is a narrow dependency issue, not a reason to replace the native bridge. Querying in the actual Electron application identity is the right ownership boundary. The existing reader tests passed, and a delayed-reader probe showed eight overlapping snapshot requests issuing eight reads.
The smallest correction is to remove notification from capability/health dependencies and share an in-flight notification read at the existing reader, clearing it on settlement rather than caching authorization. Keep the native timeout and actual build/dev/package seams. The standalone mini-app smoke can have local diagnostic value, but it does not replace checking the signed Maka app's real permissions IPC.
中文
独立复核仍支持这个局部 P2,native 进程内身份边界正确,无需重写。reader 测试通过,延迟探针中 8 次并发触发 8 次读取。最小修法是删 capability/health 的无关通知依赖,并在既有 reader 合并未完成读取、结束即清;保留 timeout 和真实构建入口。mini-app smoke 有局部诊断价值,但不能代替正式签名 Maka 的真实 IPC 验收。
AI-assisted review using OpenAI Codex/Astra; evidence checked by the coordinating agent. This is not an independent human review.
Summary
Read macOS notification authorization through a small Node-API module loaded in Electron's main process. The Permission Center currently checks
Notification.isSupported()and then always reportsunknown, even when macOS can report the application's authorization.UNUserNotificationCenterunder the running application's bundle identity, without a helper process, consent prompt, notification delivery probe, or delegate replacement.The startup candidate multiplication encountered during local testing is already handled by
candidateInFlight/attempt.exitedon the base revision7779d69a7. This PR does not duplicate that fix.Validation
unknownto nativenot_determined.app.isPackaged === true; eight concurrent reads returnednot_determined.Screenshots
Same 1280 x 900 viewport, Chinese locale, light theme, Permission Center, settings-general synthetic fixture, default zoom. Before uses the base revision's compiled permission snapshot; after uses this revision's implementation. Neither screenshot uses mocked permission data. Evidence is stored separately from the product diff.
Before
After
Pre-Push Review
Self-review and an independent read-only code review found no actionable issue. Reviewed native callback ownership after timeout, semaphore synchronization, Node-API cleanup, ASAR loading, architecture-matched release runners, asynchronous callers, test quality, and diff scope. No deeper refactor or additional abstraction was justified.
Limits