Skip to content

Desktop: Implement native file handler on Mac#4106

Open
timon-schelling wants to merge 2 commits intomasterfrom
desktop-mac-open-file-handler
Open

Desktop: Implement native file handler on Mac#4106
timon-schelling wants to merge 2 commits intomasterfrom
desktop-mac-open-file-handler

Conversation

@timon-schelling
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


INSTALL_DELEGATE.call_once(|| {
let mtm = MainThreadMarker::new().expect("only ever called from main thread");
let delegate: Retained<GraphiteApplicationDelegate> = unsafe { msg_send![super(GraphiteApplicationDelegate::alloc(mtm).set_ivars(())), init] };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The use of super(...) inside msg_send! is incorrect here as it is intended for calling superclass implementations within a class method override. Additionally, msg_send_id! should be used for init to correctly handle the memory management of the returned object.

Suggested change
let delegate: Retained<GraphiteApplicationDelegate> = unsafe { msg_send![super(GraphiteApplicationDelegate::alloc(mtm).set_ivars(())), init] };
let delegate: Retained<GraphiteApplicationDelegate> = unsafe { msg_send_id![GraphiteApplicationDelegate::alloc(mtm).set_ivars(()), init] };

Comment on lines +50 to +53
let Some(app_event_scheduler) = APP_EVENT_SCHEDULER.lock().ok() else {
tracing::error!("Received macOS open URL event before the app event scheduler was initialized");
return;
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The error message here is misleading. APP_EVENT_SCHEDULER.lock().ok() only returns None if the mutex is poisoned, not if the scheduler is uninitialized. If the scheduler is uninitialized (i.e., the inner Option is None), the lock succeeds and the code correctly proceeds to buffer the paths in LAUNCH_DOCUMENTS.

Additionally, returning early on a poisoned lock prevents the paths from being added to the buffer. It is better to collect the paths first and then attempt to schedule the event if the scheduler is available.

fn application_open_urls(&self, _application: &NSApplication, urls: &NSArray<NSURL>) {
			let mut paths = Vec::new();

			for index in 0..urls.count() {
				let url = urls.objectAtIndex(index);
				if !url.isFileURL() {
					tracing::error!("Ignoring macOS open URL event for non-file URL: {:?}", url);
					continue;
				}

				let path = unsafe { CStr::from_ptr(url.fileSystemRepresentation().as_ptr()) };
				let path = PathBuf::from(OsStr::from_bytes(path.to_bytes()));

				paths.push(path);
			}

			if paths.is_empty() {
				return;
			}

			let mut pending_paths_to_open = LAUNCH_DOCUMENTS.lock().unwrap();
			pending_paths_to_open.extend(paths);

			if let Ok(scheduler_guard) = APP_EVENT_SCHEDULER.lock() {
				if let Some(app_event_scheduler) = scheduler_guard.as_ref() {
					app_event_scheduler.schedule(AppEvent::AddLaunchDocuments(std::mem::take(&mut pending_paths_to_open)));
				}
			}
		}

Comment thread desktop/src/app.rs
DesktopFrontendMessage::OpenLaunchDocuments => {
if self.cli.files.is_empty() {
let Some(launch_documents) = std::mem::take(&mut self.launch_documents) else {
tracing::error!("OpenLaunchDocuments should only be send once");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

There is a small typo in the error message: "send" should be "sent".

Suggested change
tracing::error!("OpenLaunchDocuments should only be send once");
tracing::error!("OpenLaunchDocuments should only be sent once");


use objc2::rc::Retained;
use objc2::runtime::ProtocolObject;
use objc2::{ClassType, MainThreadMarker, MainThreadOnly, define_class, msg_send};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Import msg_send_id to properly handle the initialization of the application delegate.

Suggested change
use objc2::{ClassType, MainThreadMarker, MainThreadOnly, define_class, msg_send};
use objc2::{ClassType, MainThreadMarker, MainThreadOnly, define_class, msg_send, msg_send_id};

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 6 files

Confidence score: 3/5

  • There is a concrete behavior mismatch in desktop/src/lib.rs: App::new uses cli.disable_ui_acceleration and ignores prefs.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 to None, 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.rs and desktop/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.

Comment thread desktop/src/lib.rs
app_event_scheduler,
prefs,
cli.files,
cli.disable_ui_acceleration,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/src/lib.rs, line 113:

<comment>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.</comment>

<file context>
@@ -102,7 +102,16 @@ pub fn start() {
+		app_event_scheduler,
+		prefs,
+		cli.files,
+		cli.disable_ui_acceleration,
+	);
 
</file context>
Suggested change
cli.disable_ui_acceleration,
disable_ui_acceleration,

unsafe impl NSApplicationDelegate for GraphiteApplicationDelegate {
#[unsafe(method(application:openURLs:))]
fn application_open_urls(&self, _application: &NSApplication, urls: &NSArray<NSURL>) {
let Some(app_event_scheduler) = APP_EVENT_SCHEDULER.lock().ok() else {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At desktop/src/window/mac/app.rs, line 50:

<comment>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.</comment>

<file context>
@@ -20,12 +36,66 @@ define_class!(
+	unsafe impl NSApplicationDelegate for GraphiteApplicationDelegate {
+		#[unsafe(method(application:openURLs:))]
+		fn application_open_urls(&self, _application: &NSApplication, urls: &NSArray<NSURL>) {
+			let Some(app_event_scheduler) = APP_EVENT_SCHEDULER.lock().ok() else {
+				tracing::error!("Received macOS open URL event before the app event scheduler was initialized");
+				return;
</file context>

@timon-schelling
Copy link
Copy Markdown
Member Author

timon-schelling commented May 5, 2026

!build desktop (Run ID 25389980031)

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

📦 Linux Build Complete for b877c2f
Download binary
Download Flatpak

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

📦 Mac Build Complete for b877c2f
Download binary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant