From 5aca79139a4051703cf40382220a0fbc89a0b1cf Mon Sep 17 00:00:00 2001 From: Mayank Verma Date: Wed, 29 Jul 2026 19:05:25 +0530 Subject: [PATCH] gpui: Defer appearance change callback to avoid reentrant borrow (#61789) ### Objective PR #58902 made native macOS window chrome follow the selected theme, but AppKit can synchronously invoke `viewDidChangeEffectiveAppearance` while the settings observer holds an `App` borrow. `handle.update` then fails with `RefCell already borrowed`, leaving `Window::appearance()` and its observers stale. ### Steps to reproduce 1. Set Zed's theme mode to Dark. 2. Set macOS Appearance to Light. 3. Change Zed's theme mode to System. 4. Notice that the window chrome switches to light, but Zed's UI remains dark. https://github.com/user-attachments/assets/f91962cc-76ba-4d8e-b587-2a2343f365ba ### Expectation Zed should switch to its configured light theme when returning to System mode. ### Solution Defer `Window::appearance_changed` to the foreground executor so the current `App` borrow finishes before refreshing the cached appearance and notifying observers. ### Testing - Added a regression test. ### Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [ ] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - N/A --- crates/gpui/src/platform/test/window.rs | 21 ++++++++++-- crates/gpui/src/window.rs | 45 +++++++++++++++++++++---- crates/gpui_macos/src/window.rs | 3 +- 3 files changed, 59 insertions(+), 10 deletions(-) diff --git a/crates/gpui/src/platform/test/window.rs b/crates/gpui/src/platform/test/window.rs index 8894350edc..ea1edf5a91 100644 --- a/crates/gpui/src/platform/test/window.rs +++ b/crates/gpui/src/platform/test/window.rs @@ -33,8 +33,10 @@ pub(crate) struct TestWindowState { hover_status_change_callback: Option>, resize_callback: Option, f32)>>, moved_callback: Option>, + appearance_change_callback: Option>, input_handler: Option, is_fullscreen: bool, + appearance: WindowAppearance, } #[derive(Clone)] @@ -85,8 +87,10 @@ impl TestWindow { hover_status_change_callback: None, resize_callback: None, moved_callback: None, + appearance_change_callback: None, input_handler: None, is_fullscreen: false, + appearance: WindowAppearance::Light, }))) } @@ -113,6 +117,17 @@ impl TestWindow { self.0.lock().active_status_change_callback = Some(callback); } + pub fn simulate_appearance_change(&self, appearance: WindowAppearance) { + let mut lock = self.0.lock(); + lock.appearance = appearance; + let Some(mut callback) = lock.appearance_change_callback.take() else { + return; + }; + drop(lock); + callback(); + self.0.lock().appearance_change_callback = Some(callback); + } + pub fn simulate_input(&mut self, event: PlatformInput) -> bool { let mut lock = self.0.lock(); let Some(mut callback) = lock.input_callback.take() else { @@ -152,7 +167,7 @@ impl PlatformWindow for TestWindow { } fn appearance(&self) -> WindowAppearance { - WindowAppearance::Light + self.0.lock().appearance } fn display(&self) -> Option> { @@ -290,7 +305,9 @@ impl PlatformWindow for TestWindow { self.0.lock().hit_test_window_control_callback = Some(callback); } - fn on_appearance_changed(&self, _callback: Box) {} + fn on_appearance_changed(&self, callback: Box) { + self.0.lock().appearance_change_callback = Some(callback); + } fn draw(&self, scene: &Scene) { let scale_factor = self.scale_factor(); diff --git a/crates/gpui/src/window.rs b/crates/gpui/src/window.rs index 048988a431..c2e1737641 100644 --- a/crates/gpui/src/window.rs +++ b/crates/gpui/src/window.rs @@ -1688,11 +1688,19 @@ impl Window { } })); platform_window.on_appearance_changed(Box::new({ - let mut cx = cx.to_async(); + let cx = cx.to_async(); + let foreground_executor = cx.foreground_executor().clone(); move || { - handle - .update(&mut cx, |_, window, cx| window.appearance_changed(cx)) - .log_err(); + let mut cx = cx.clone(); + // Defer the update because changing the AppKit appearance may + // synchronously invoke this callback while App is already borrowed. + foreground_executor + .spawn(async move { + handle + .update(&mut cx, |_, window, cx| window.appearance_changed(cx)) + .log_err(); + }) + .detach(); } })); platform_window.on_button_layout_changed(Box::new({ @@ -6681,8 +6689,8 @@ mod tests { use crate::{ AppContext as _, Bounds, Context, FocusHandle, InteractiveElement as _, IntoElement, - ParentElement, Pixels, Render, Styled, TestAppContext, Window, WindowOptions, canvas, div, - px, size, + ParentElement, Pixels, Render, Styled, TestAppContext, Window, WindowAppearance, + WindowOptions, canvas, div, px, size, }; struct EmptyView; @@ -6743,6 +6751,31 @@ mod tests { .unwrap(); } + #[gpui::test] + fn test_appearance_change_runs_after_app_update(cx: &mut TestAppContext) { + let window = cx.add_window(|_, _| EmptyView); + let observed_appearance = Rc::new(Cell::new(None)); + let _subscription = window + .update(cx, { + let observed_appearance = observed_appearance.clone(); + move |_, window, _| { + window.observe_window_appearance(move |window, _| { + observed_appearance.set(Some(window.appearance())); + }) + } + }) + .unwrap(); + let test_window = cx.test_window(window.into()); + + cx.update(|_| { + test_window.simulate_appearance_change(WindowAppearance::Dark); + assert_eq!(observed_appearance.get(), None); + }); + cx.run_until_parked(); + + assert_eq!(observed_appearance.get(), Some(WindowAppearance::Dark)); + } + struct RootView { explicit_size: bool, child_bounds: Rc>>, diff --git a/crates/gpui_macos/src/window.rs b/crates/gpui_macos/src/window.rs index d094211516..cf9beb0228 100644 --- a/crates/gpui_macos/src/window.rs +++ b/crates/gpui_macos/src/window.rs @@ -2849,8 +2849,7 @@ extern "C" fn view_did_change_effective_appearance(this: &Object, _: Sel) { } // AppKit can relayout the standard traffic light buttons as part of - // applying a new appearance. Reapply GPUI's custom position after - // notifying appearance observers. + // applying a new appearance, so reapply GPUI's custom position. state.lock().move_traffic_light(); } }