diff --git a/CLAUDE.md b/CLAUDE.md index 3bde1d5..bcfd9ad 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -12,6 +12,7 @@ The [`docs/learnings/`](docs/learnings/) directory contains hard-won technical k - [`cowork-vm-daemon.md`](docs/learnings/cowork-vm-daemon.md) — Cowork VM daemon lifecycle, respawn logic, crash diagnosis - [`plugin-install.md`](docs/learnings/plugin-install.md) — Anthropic & Partners plugin install flow, gate logic, backend endpoints, and DevTools recipes - [`apt-worker-architecture.md`](docs/learnings/apt-worker-architecture.md) — APT/DNF binary distribution via Cloudflare Worker + GitHub Releases, redirect chain, credential ownership, heartbeat runbook +- [`tray-rebuild-race.md`](docs/learnings/tray-rebuild-race.md) — why destroy + recreate on `nativeTheme` updates briefly duplicates the tray icon on KDE Plasma, and the in-place `setImage` + `setContextMenu` fast-path that avoids the SNI re-registration race ## Code Style diff --git a/README.md b/README.md index 94087d9..65d1b83 100644 --- a/README.md +++ b/README.md @@ -207,7 +207,9 @@ Special thanks to: - Diagnosing the session-start hook sudo blocking issue with three solution approaches - **[chukfinley](https://github.com/chukfinley)** for experimental Cowork mode support on Linux - **[CyPack](https://github.com/CyPack)** for orphaned cowork daemon cleanup on startup -- **[IliyaBrook](https://github.com/IliyaBrook)** for fixing the platform patch for Claude Desktop >= 1.1.3541 arm64 refactor +- **[IliyaBrook](https://github.com/IliyaBrook)** + - Fixing the platform patch for Claude Desktop >= 1.1.3541 arm64 refactor + - Fixing the duplicate tray icon on OS theme change with an in-place `setImage`/`setContextMenu` fast-path that avoids the KDE Plasma SNI re-registration race - **[MichaelMKenny](https://github.com/MichaelMKenny)** - Diagnosing the `$`-prefixed electron variable bug - Root cause analysis and workaround diff --git a/docs/learnings/tray-rebuild-race.md b/docs/learnings/tray-rebuild-race.md new file mode 100644 index 0000000..9d52689 --- /dev/null +++ b/docs/learnings/tray-rebuild-race.md @@ -0,0 +1,123 @@ +# Tray icon rebuild race on OS theme change + +Why destroy + delay + recreate isn't enough on KDE, and what the +in-place fast-path does differently. + +## The bug + +Claude Desktop's tray icon follows the OS theme via +`nativeTheme.on('updated', ...)` — every theme change re-runs the +tray rebuild function so the icon PNG can be switched. That rebuild +calls `tray.destroy()`, nulls the reference, sleeps 250 ms (added +earlier to bound DBus-teardown timing), then instantiates a fresh +`new Tray(image)`. + +Destroying the `Tray` deregisters the app's StatusNotifierItem from +the session bus (`org.kde.StatusNotifierWatcher.UnregisterItem`); +the new `Tray()` call registers a brand-new one. On KDE Plasma's +`systemtray` widget the window between "unregister signal emitted" +and "plasmoid observer reacts" can exceed 250 ms, during which both +the old SNI name and the new one coexist in the widget's internal +list — the user sees **two Claude icons side by side** until the +next session start. + +250 ms is genuinely enough on some setups (the delay was landed +because a larger gap was introducing a visible icon flash); it +isn't enough on others. Timing depends on the compositor version, +portal implementation, and presumably hardware speed, so widening +the delay is just moving the goalposts — the race is structural. + +## Triggers + +Any system-wide appearance change that makes Chromium emit +`nativeTheme::updated` trips the same code path. Verified triggers +in KDE System Settings: + +- **Appearance → Colors** (application colour scheme dropdown) +- **Appearance → Plasma Style** (panel/widget theme) +- **Appearance → Global Theme** (look-and-feel package) + +All three route through `org.freedesktop.appearance` / +`KGlobalSettings` signals that Chromium observes, so they all +re-enter the tray rebuild function and all reproduce the duplicate +icon. + +## The fix + +`patch_tray_inplace_update` (in `scripts/patches/tray.sh`) injects +a fast-path at the top of the rebuild function: + +```js +if (Nh && e !== false) { + Nh.setImage(pA.nativeImage.createFromPath(t)); + process.platform !== 'darwin' && Nh.setContextMenu(wAt()); + return; +} +``` + +When the tray already exists and isn't being disabled, the patch +updates the icon and the context menu on the **existing** +`StatusNotifierItem` — `setImage` and `setContextMenu` don't +re-register the SNI on DBus, they emit `NewIcon` / `LayoutUpdated` +signals, which the host consumes in-place. No race. + +The original destroy + recreate slow-path is kept intact for two +cases that legitimately require it: + +- **Initial creation** — `Nh` is `undefined`, so the fast-path + guard short-circuits and the slow path runs. +- **Disabling the tray** — `e === false` (user turned the tray off + via `menuBarEnabled` setting) means the tray should be destroyed + outright, not re-imaged. + +## Resilience to minifier churn + +Variable names (`Nh`, `pA`, `wAt`, `t`, `e`) drift between upstream +releases. All five are extracted dynamically in `tray.sh`: + +| Local | Extraction anchor | +|--|--| +| `tray_func` | `on("menuBarEnabled",()=>{ … })` | +| `tray_var` | `});let X=null;(async )?function ${tray_func}` | +| `electron_var` | already extracted earlier in `_common.sh` | +| `menu_func` | `${tray_var}.setContextMenu(X(` | +| `path_var` | `${tray_var}=new ${electron_var}.Tray(${electron_var}.nativeImage.createFromPath(X))` | +| `enabled_var` | `const X = fn("menuBarEnabled")` | + +Idempotency guard keys on the distinctive +`${tray_var}.setImage(${electron_var}.nativeImage.createFromPath(${path_var}))` +sequence using post-rename extracted names, so re-running the patch +on an already-patched asar is a no-op even after the minifier +churns. + +## Verification + +Reproduced on Fedora Linux 43 (KDE Plasma Desktop Edition) with +Plasma 6.6.4, `xdg-desktop-portal-kde` 6.6.4, Wayland session, +kernel 6.19.12. + +Steps on pristine `main` (before this patch): + +```bash +git clone https://github.com/aaddrick/claude-desktop-debian.git +cd claude-desktop-debian +./build.sh --build appimage --clean no +./claude-desktop-*-amd64.AppImage +# Then in KDE Settings → Appearance, flip any of Colors / +# Plasma Style / Global Theme. Two tray icons appear. +``` + +After the patch: one SNI stays registered for the app's lifetime, +icon updates in place on every theme change. + +## Pitfalls to watch for + +- **Fast-path runs inside the 3 s startup window too.** The + existing `_trayStartTime > 3e3` guard only gates the + `nativeTheme.on('updated')` → `tray_func()` call; once + `tray_func()` is running for any reason, our fast-path executes. + Fine — it's cheaper than the slow path even at startup. +- **macOS path is left untouched.** The condition + `process.platform !== 'darwin' && …setContextMenu` keeps the + Electron macOS tray model (right-click pops up a menu via + `popUpContextMenu(r)` with `r` captured at creation time) intact. diff --git a/scripts/patches/app-asar.sh b/scripts/patches/app-asar.sh index cec04c1..8f5b15f 100644 --- a/scripts/patches/app-asar.sh +++ b/scripts/patches/app-asar.sh @@ -77,6 +77,10 @@ console.log('Updated package.json: main entry and node-pty dependency'); # Patch tray icon selection patch_tray_icon_selection + # Inject fast-path that updates the tray icon in place on theme + # changes (avoids the KDE duplicate-SNI race on destroy+recreate) + patch_tray_inplace_update + # Patch menuBarEnabled to default to true when unset patch_menu_bar_default diff --git a/scripts/patches/tray.sh b/scripts/patches/tray.sh index f84c58b..f0f87e9 100644 --- a/scripts/patches/tray.sh +++ b/scripts/patches/tray.sh @@ -92,6 +92,144 @@ patch_tray_icon_selection() { echo '##############################################################' } +patch_tray_inplace_update() { + echo 'Patching tray rebuild to update in-place on theme change...' + local index_js='app.asar.contents/.vite/build/index.js' + + # Re-extract the tray variable name — `patch_tray_menu_handler` + # declares it `local` so it's not visible here. Same grep pattern. + local tray_func local_tray_var tray_var_re + local menu_func path_var enabled_var enabled_count + tray_func=$(grep -oP \ + 'on\("menuBarEnabled",\(\)=>\{\K\w+(?=\(\)\})' "$index_js") + if [[ -z $tray_func ]]; then + echo ' Could not find tray function — skipping' + echo '##############################################################' + return + fi + local_tray_var=$(grep -oP \ + "\}\);let \K\w+(?==null;(?:async )?function ${tray_func})" \ + "$index_js") + if [[ -z $local_tray_var ]]; then + echo ' Could not extract tray variable name — skipping' + echo '##############################################################' + return + fi + echo " Found tray variable: $local_tray_var" + + tray_var_re="${local_tray_var//\$/\\$}" + + menu_func=$(grep -oP "${tray_var_re}\.setContextMenu\(\K\w+(?=\(\))" \ + "$index_js" | head -1) + if [[ -z $menu_func ]]; then + echo ' Could not extract menu function name — skipping' + echo '##############################################################' + return + fi + echo " Found menu function: $menu_func" + + # Extract the icon-path local used in the original + # Nh = new pA.Tray(pA.nativeImage.createFromPath(X)) + # call. That `X` is the `const` assigned `path.join(resourcesDir(), + # suffix)` earlier in the function; minifier renames it between + # releases, so it needs to be extracted (not hardcoded). + path_var=$(grep -oP \ + "${tray_var_re}=new ${electron_var_re}\.Tray\(${electron_var_re}\.nativeImage\.createFromPath\(\K\w+(?=\))" \ + "$index_js" | head -1) + if [[ -z $path_var ]]; then + echo ' Could not extract icon-path var — skipping' + echo '##############################################################' + return + fi + echo " Found icon-path var: $path_var" + + # Extract the menuBarEnabled local. The injected fast-path needs to + # read the same local that the slow-path destroy/recreate block + # tests, so binding to the wrong site is silently broken. Bail if + # upstream ever ships >1 declaration site instead of taking the + # first one. + enabled_count=$(grep -cE \ + 'const \w+\s*=\s*\w+\("menuBarEnabled"\)' "$index_js") + if [[ $enabled_count -ne 1 ]]; then + echo " Expected 1 menuBarEnabled declaration, found" \ + "${enabled_count} — skipping" + echo '##############################################################' + return + fi + enabled_var=$(grep -oP \ + 'const \K\w+(?=\s*=\s*\w+\("menuBarEnabled"\))' "$index_js") + if [[ -z $enabled_var ]]; then + echo ' Could not extract menuBarEnabled var — skipping' + echo '##############################################################' + return + fi + echo " Found menuBarEnabled var: $enabled_var" + + # Idempotency guard: re-running the patch is a no-op once our + # fast-path is in place. Key on the distinctive + # "setImage(EL.nativeImage.createFromPath(PATH_VAR))" sequence + # using the (post-rename) extracted names — the destroy+recreate + # slow-path still exists below, so we can't just count occurrences + # of setImage. + local fast_path_marker + fast_path_marker="${local_tray_var}.setImage(${electron_var}.nativeImage.createFromPath(${path_var}))" + if grep -qF "$fast_path_marker" "$index_js"; then + echo ' In-place fast-path already present (idempotent)' + echo '##############################################################' + return + fi + + # Inject a fast-path before the existing destroy+recreate block: + # when the tray already exists and isn't being disabled, update it + # in place with setImage + setContextMenu. Skips the DBus race + # where Plasma briefly shows both the old (not yet unregistered) + # and the new StatusNotifierItem. Slow path is kept for initial + # creation and tray-disable. + if ! TRAY_VAR="$local_tray_var" EL_VAR="$electron_var" \ + MENU_FUNC="$menu_func" PATH_VAR="$path_var" \ + ENABLED_VAR="$enabled_var" \ + node -e " +const fs = require('fs'); +const p = 'app.asar.contents/.vite/build/index.js'; +const T = process.env.TRAY_VAR; +const E = process.env.EL_VAR; +const M = process.env.MENU_FUNC; +const P = process.env.PATH_VAR; +const V = process.env.ENABLED_VAR; +let code = fs.readFileSync(p, 'utf8'); + +// Anchor at the start of the existing destroy+recreate block, +// tolerating optional inner whitespace. +const reEsc = (s) => s.replace(/[.*+?^\${}()|[\\]\\\\]/g, '\\\\\$&'); +const anchor = new RegExp( + ';if\\\\(' + reEsc(T) + '&&\\\\(' + reEsc(T) + '\\\\.destroy\\\\(\\\\)' +); +if (!anchor.test(code)) { + console.error(' [FAIL] destroy-recreate anchor not found'); + process.exit(1); +} + +const fastPath = + 'if(' + T + '&&' + V + '!==false){' + + T + '.setImage(' + E + '.nativeImage.createFromPath(' + P + '));' + + 'process.platform!==\"darwin\"&&' + T + '.setContextMenu(' + M + '());' + + 'return' + + '}'; + +// Prefix the destroy block with the fast-path, keeping the matched +// portion ';if(TRAY&&(TRAY.destroy()' intact. +code = code.replace(anchor, (m) => ';' + fastPath + m.slice(1)); +fs.writeFileSync(p, code); +console.log(' [OK] Fast-path injected before destroy-recreate'); +"; then + echo 'Failed to inject tray in-place fast-path' >&2 + cd "$project_root" || exit 1 + exit 1 + fi + + echo '##############################################################' +} + patch_menu_bar_default() { echo 'Patching menuBarEnabled to default to true when unset...' local index_js='app.asar.contents/.vite/build/index.js'