Fix crash on appearance change by tagliala · Pull Request #646 · gitx/gitx

tagliala · 2026-09-14T21:46:44Z

Author note: This PR was drafted by an AI coding assistant ("Big Pickle") based on crash-diagnosis and a local reproduction of the failure (SIGABRT on appearance toggle, crash reports from the shipped build on macOS 27). The proposed change was verified by building and toggling Dark ↔ Light with the patched binary; however it should be reviewed with the same scrutiny as any community contribution, and I'm happy to iterate on feedback.

Fixes #578

Root cause

NSApplication (PBDarkMode) overrode observeValueForKeyPath:ofObject:change:context: as a category method. This shadows AppKit's own implementation of that selector on NSApplication.

On macOS appearance changes (dark/light toggle or Increase Contrast), AppKit's NSSystemAppearanceProxy fires internal KVO notifications delivered to NSApp using AppKit's own contexts. Those deliveries landed in the category override, whose else branch called [super observeValueForKeyPath:...] — which resolved to NSObject's default implementation and raised the "message received but not handled" exception, aborting the app.

— the GitX frame sits between NSKeyValueNotifyObserver and the throwing -[NSObject(NSKeyValueObserving) observeValueForKeyPath:...], and disassembly confirms it is the removed else branch's [super ...] call.

Fix

Observe effectiveAppearance through a dedicated PBAppearanceObserverToken object instead of self. The category no longer implements observeValueForKeyPath: at all, so AppKit's own implementation on NSApplication is left untouched, while our KVO change is still delivered and re-posted as PBEffectiveAppearanceChanged.

Verification

Reproduced the SIGABRT on dark/light and Increase Contrast toggles with the release build, confirmed via the crash reports in ~/Library/Logs/DiagnosticReports.
Confirmed the crash frame offset (0x43380) disassembles to the removed else branch's [super observeValueForKeyPath:...] call.
Built and toggled system appearance (Dark → Light → Dark) with the fixed build: no crash, no new crash reports.

Notes

Also corrected stale comments in GitX-Bridging-Header.h and NSAppearance+PBDarkMode.swift that claimed the (never-compiled) Swift twin owns the implementations.

AI review

GPT-6 reviewed and approved commit 26fc6bbc0133cc3db2361109eaec624ce334f571 against the linked issue #578. Source review covered the AppKit KVO method collision, observer lifetime and re-registration, notification delivery to the web views, and Xcode source membership. No actionable findings.

No tests were run as part of this review, as requested. Both existing CI jobs passed for the same commit in tagliala/gitx#3. Runtime appearance-toggle verification is the author-reported verification above.

The NSApplication (PBDarkMode) category overrode observeValueForKeyPath:ofObject:change:context:, shadowing AppKit's own implementation on NSApplication. On appearance change (dark/light or Increase Contrast), AppKit delivers its internal KVO notifications to NSApp using its own contexts; the override's else branch then forwarded to super, which reached NSObject's default implementation and raised the "message received but not handled" exception, aborting the app. Observe effectiveAppearance through a dedicated token object instead, so AppKit's own implementation is left untouched and our KVO change is delivered without shadowing anything. Fix gitx#578

tagliala · 2026-09-14T22:06:48Z

@goneng this looks good on my machine an GPT-6 did not complain about the solution

If you can do some tests, it would be better. I have no idea of what the changes are about

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

goneng · 2026-09-15T19:19:30Z

@tagliala @hannesa2 LGTM, I think we can merge this.

I reviewed the diff and also checked the diagnosis against the runtime rather than
taking it on trust:

NSApplication does implement observeValueForKeyPath:ofObject:change:context:
itself (it is absent on NSResponder, present on NSObject), so the old category
really did shadow AppKit's implementation, and [super ...] landed on NSObject's
raising version. I reproduced the exact NSInternalInconsistencyException from Crash when switching display mode from dark to light on >= 1.5.1 #578
with a standalone build of the old code.
Built the new NSAppearance+PBDarkMode.m against the project header with
-Wall -Wextra (clean) and exercised it: appearance changes still post
PBEffectiveAppearanceChanged with the right object, [NSApp isDarkMode] is
correct on both transitions, and delivering a foreign-context KVO notification to
NSApp no longer raises.
The added removeObserver: on re-registration also quietly fixes a latent
duplicate-notification bug: the old code never removed its observation, so N calls
to registerObserverForAppearanceChanges: meant N notifications per change.
Confirmed the bridging-header comment is accurate - none of the four Swift twins
are in GitX.xcodeproj/project.pbxproj.

Two non-blocking notes for a follow-up, neither worth holding this up:

token.observer is now weak where the old code retained the observer. Safe as
written, since the only caller is ApplicationController, a top-level object of
MainMenu.xib that lives for the app's lifetime. Just worth knowing that if the
observer ever were deallocated, the if (observer) guard would silently stop all
appearance updates instead of failing visibly.
NSAppearance+PBDarkMode.swift is left as non-compiled dead code, which the comment
itself acknowledges. Deleting it seems cleaner than keeping it in sync by hand.

goneng

LGTM. Details in my comment above.

tagliala · 2026-09-15T19:22:34Z

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

I've attempted a review from Copilot here:

tagliala#3 (review)

but actually it wasn't able to review

goneng added the bugfix label Sep 14, 2026

tagliala marked this pull request as ready for review September 14, 2026 22:03

hannesa2 requested a lite review from Copilot September 15, 2026 17:10

Copilot AI reviewed Sep 15, 2026

hannesa2 requested a lite review from Copilot September 15, 2026 17:17

Copilot AI reviewed Sep 15, 2026

goneng approved these changes Sep 15, 2026

View reviewed changes

hannesa2 merged commit 99c700c into gitx:master Sep 15, 2026
3 checks passed

goneng mentioned this pull request Sep 19, 2026

Bug: Crash on color theme changing #662

Closed

Navigation Menu

Repository navigation

Fix crash on appearance change - #646

Fix crash on appearance change#646
hannesa2 merged 1 commit into
gitx:masterfrom
tagliala:fix/appearance-change-crash

tagliala commented Sep 14, 2026 •

edited

Loading

Uh oh!

tagliala commented Sep 14, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

goneng commented Sep 15, 2026

Uh oh!

goneng left a comment

Uh oh!

tagliala commented Sep 15, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

tagliala commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Root cause

Fix

Verification

Notes

AI review

Uh oh!

tagliala commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

goneng commented Sep 15, 2026

Uh oh!

goneng left a comment

Choose a reason for hiding this comment

Uh oh!

tagliala commented Sep 15, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tagliala commented Sep 14, 2026 •

edited

Loading

tagliala commented Sep 14, 2026 •

edited

Loading