Desktop: Register the Graphite app as a file handler on Mac#4106
Desktop: Register the Graphite app as a file handler on Mac#4106timon-schelling merged 3 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements native macOS document handling, enabling the application to open files through system events like double-clicking or dragging files onto the app icon. Key changes include the addition of a custom NSApplicationDelegate to intercept openURLs events, a refactor of the App struct to manage launch documents independently of the CLI, and the configuration of document types and MIME types in the macOS bundle. Review feedback identifies critical issues with Objective-C memory management and macro usage, suggests improvements to the robustness of the URL event buffering logic, and notes a minor typo in a log message.
There was a problem hiding this comment.
2 issues found across 6 files
Confidence score: 3/5
- There is a concrete behavior mismatch in
desktop/src/lib.rs:App::newusescli.disable_ui_accelerationand ignoresprefs.disable_ui_acceleration, which can incorrectly treat acceleration as enabled and undermine the intended fallback path when acceleration should be off. - In
desktop/src/window/mac/app.rs, the lock/error handling is misleading (.lock().ok()maps poison toNone, not uninitialized state), which raises a real risk of incorrect diagnostics and potentially wrong control flow around scheduler state. - Given both issues are medium severity (5/10) with solid confidence (7/10), this looks like some user-impacting risk rather than a merge-blocking failure, so a targeted fix pass would improve safety before merging.
- Pay close attention to
desktop/src/lib.rsanddesktop/src/window/mac/app.rs- acceleration-source precedence and mutex/scheduler state handling need to be corrected.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="desktop/src/lib.rs">
<violation number="1" location="desktop/src/lib.rs:113">
P2: App::new is passed `cli.disable_ui_acceleration`, which ignores the `prefs.disable_ui_acceleration` value. This makes the app think acceleration is enabled even when prefs disabled it, so the UI-acceleration failure watchdog can still exit the app unnecessarily. Pass the computed `disable_ui_acceleration` instead.</violation>
</file>
<file name="desktop/src/window/mac/app.rs">
<violation number="1" location="desktop/src/window/mac/app.rs:50">
P2: The error message here is misleading: `.lock().ok()` returns `None` only when the mutex is **poisoned**, not when the scheduler is uninitialized (which is the inner `Option` being `None`). More importantly, returning early here means the file paths from this event are never buffered into `LAUNCH_DOCUMENTS`, so they are lost entirely if the mutex is poisoned. Consider collecting paths into `LAUNCH_DOCUMENTS` before attempting to acquire this lock, so that paths are always buffered regardless of the scheduler lock state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
!build desktop (Run ID 25389980031) |
|
|
b877c2f to
8c5b931
Compare
ba590de to
63a3b1e
Compare
63a3b1e to
50eb2a8
Compare
closes #3575