Sign up
View stargazersSign up

fix(desktop): avoid temporary icon copies on Linux launches#19589

merged8
main
GitHub fork: fix/linux-second-instance-icon-tmp
opened22d agobyOonly21mil
4 files changed
View on GitHubTwo-way sync enabledPR last imported 16d ago

Packaged Linux launches copy the application icon out of app.asar before checking the single-instance lock. Electron extracts that copy through a temporary file, which can remain after a rejected secondary launch exits.

Both existing icon installers now read binary bytes and write the persistent icon only when it is missing or different. Optional icon errors still allow desktop registration to finish. The URL handler reuses the existing byte comparison helper; main's theme-icon installation remains intact.

This addresses the confirmed launch-time path reported in #17737. It does not establish that the separately reported idle-time file growth is fixed.

Verification on the current head:

  • Two focused suites reduced from 37 to 27 tests, retaining main's coverage and removing overlapping byte-length and platform matrices.
  • Current production: 27 passed. The same tests against main production: 7 failed, 20 passed. Restored current production: 27 passed.
  • Desktop typecheck, targeted lint/formatting, and git diff --check pass.
  • Independent whole-diff reviews by Claude Opus 5.5 and GPT-6 Sol found no blockers in one round.
sh
1pnpm exec vp test run apps/desktop/src/app/DesktopPreReadyPlatform.test.ts apps/desktop/src/app/DesktopLinuxUrlHandler.test.ts
2pnpm --filter @t3tools/desktop exec tsc-rs --noEmit
3pnpm exec vp lint apps/desktop/src/app/DesktopPreReadyPlatform.ts apps/desktop/src/app/DesktopPreReadyPlatform.test.ts apps/desktop/src/app/DesktopLinuxUrlHandler.ts apps/desktop/src/app/DesktopLinuxUrlHandler.test.ts

Native verification limit: Electron 44.4.5 was downloaded, but could not start on this headless Linux VM because libcups, cairo, GTK, and Pango libraries are missing. The tests exercise filesystem behavior with a mocked ASAR source; a final packaged Linux repeated-launch trace remains for a machine with Electron support. macOS, Windows, iOS, Android, and the Electron desktop UI were not exercised. No visual layout changes; test-level before/after proof is provided above.

Thanks to @only21mil for the original fix and packaged Fedora investigation in #19589. Maintainer takeover preserves the contributor commits and adds a merge of main, focused tests, and a smaller implementation. Human approval and CI on the pushed head remain required; unmerged.

Tests: GPT-6 Astra; implementation: GPT-6.1 Sol; review: Claude Opus 5.5 and GPT-6 Sol. Codex and Claude harnesses in T3 Code.

Checks

21 successful · 10 skipped
All checks passed
06a2d2a

Added 2 commits

  • test(desktop): cover ASAR-free Linux icon install228919b
  • fix(desktop): install the Linux icon without extracting it from the ASARa12ed61
CcoderabbitaiBot22d ago
Synced from GitHub

Review in Change Stack →

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 11 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 03730695-32fc-4de0-ae41-43b020ec5e89
📥 Commits

Reviewing files that changed from the base of the PR and between 94f72a5fc53c3c2d3d1122802ffacf6a14f4c0c5 and 06a2d2adee51c2c2d3a29f4d74d0689248adde3e.

📒 Files selected for processing: 4
  • apps/desktop/src/app/DesktopLinuxUrlHandler.test.ts
  • apps/desktop/src/app/DesktopLinuxUrlHandler.ts
  • apps/desktop/src/app/DesktopPreReadyPlatform.test.ts
  • apps/desktop/src/app/DesktopPreReadyPlatform.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: e9b18e58-7c4e-48e4-b45f-aff2adad59d2
📥 Commits

Reviewing files that changed from the base of the PR and between 228919b6b9a2972aadc0e663ecc7824115915e91 and 94f72a5fc53c3c2d3d1122802ffacf6a14f4c0c5.

📒 Files selected for processing: 3
  • apps/desktop/src/app/DesktopLinuxUrlHandler.test.ts
  • apps/desktop/src/app/DesktopLinuxUrlHandler.ts
  • apps/desktop/src/app/DesktopPreReadyPlatform.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.

📝 Walkthrough

Packaged Linux URL-handler registration and pre-ready setup now compare icon bytes before writing. Both paths skip writes when the installed icon matches and use mode 0o600 for icon writes. Icon read or write failures do not prevent desktop-entry setup.

Changes: Linux icon installation

Layer / File(s)Summary
URL-handler icon registration
apps/desktop/src/app/DesktopLinuxUrlHandler.ts, apps/desktop/src/app/DesktopLinuxUrlHandler.test.ts		Packaged registration reads and compares icon bytes, then writes only when needed. Tests cover differing and matching bytes, repeated registration, filesystem errors, and cases that must not access icons.
Pre-ready icon installation
apps/desktop/src/app/DesktopPreReadyPlatform.ts, apps/desktop/src/app/DesktopPreReadyPlatform.test.ts		Packaged setup writes the icon only when the destination is missing or differs. Tests cover filesystem errors, unpackaged development setup, and non-Linux setup.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to 94f72

No actionable merge-blocking risk remains for the Linux icon-install change after normal checks.

Pre-merge checks: Passed 4
Check nameStatusExplanation
Linked Issues checkPassedIssue [#17737] requires Linux launches to stop accumulating temporary icon copies. At the reviewed head, both icon installers read the packaged PNG as bytes and write the persistent destination only w…
Out of Scope Changes checkPassedThe change remains limited to the two Linux icon-install paths and their focused tests. The tests directly support issue [#17737] by covering binary reads, conditional writes, failure handling, and no…
Title checkPassedThe title clearly and concisely describes the main change: avoiding temporary Linux icon copies during desktop launches.
Description checkPassedThe description provides the problem, implementation details, linked issue, scope, verification results, test commands, known limitations, and agent attribution. Although it does not use the template …
Full details: Linked Issues check

Explanation

Issue [#17737] requires Linux launches to stop accumulating temporary icon copies. At the reviewed head, both icon installers read the packaged PNG as bytes and write the persistent destination only when it is missing or differs. The pre-ready path uses readFileSync and writeFileSync; the URL-handler path uses readFile and writeFile. New files use mode 0o600, and existing files retain their permissions. Focused tests cover missing, changed, equal, read-failure, and write-failure cases. The reported AppImage check found no matching temporary files across the primary and ten secondary launches.

Full details: Out of Scope Changes check

Explanation

The change remains limited to the two Linux icon-install paths and their focused tests. The tests directly support issue [#17737] by covering binary reads, conditional writes, failure handling, and non-Linux or unpackaged behavior. No unrelated production behavior or unrelated file changes are identified.

Full details: Description check

Explanation

The description provides the problem, implementation details, linked issue, scope, verification results, test commands, known limitations, and agent attribution. Although it does not use the template headings explicitly, it is substantially complete and directly related to the change.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

Jjet22d ago

could to fix this pr? the screenshots are blurred for some reason

Oonly21mil22d ago
Synced from GitHub

Thanks for flagging this. Two of the screenshots were fully blurred, one was a dim thumbnail, and the rest were full-screen captures that GitHub scaled down. I replaced all nine with four sharp, captioned crops of the setup dialog at native resolution. I also corrected the note on hidden windows: only the candidate window was hidden, and it was not restored in this environment.

Added 1 commit

  • Merge upstream main into fix/linux-second-instance-icon-tmp94f72a5
JJulius Marminge19d ago

Note

Grok responding on behalf of Julius.

Closing as superseded. #17737 was closed as not planned (upstream / Chromium–GNOME, not a T3-side write). Thanks for the investigation and the ASAR-free icon install approach — if that issue is reopened with a confirmed app-side path, this can be revisited.

JJulius Marminge19d ago

Note

Grok responding on behalf of Julius.

Reopening. Closing this was a mistake on our side: #17737 is reopened, since the icon copy out of app.asar in DesktopPreReadyPlatform.ts is confirmed on main.

JJulius Marminge18d ago

Taking over this one as maintainer for batch desktop-app-11. I will keep this focused on the ASAR icon-copy leak, trim duplicate regression coverage, and push follow-up commits here while preserving the original work and author credit.

Added 5 commits

  • Merge remote-tracking branch 'origin/main' into fix/linux-second-instance-icon-tmpb778ccc
  • test(desktop): trim Linux icon installation regressions5f2a921
  • fix(desktop): avoid temporary ASAR icon extractionde78093
  • Merge remote-tracking branch 'origin/main' into L70-19589bb59152
  • Merge remote-tracking branch 'origin/main' into L70-1958906a2d2a
JJulius Marminge17d ago

Reverting this in #22222 together with the other desktop fixes the takeover campaign merged yesterday. Nothing was wrong with the review of this change; it was merged on CI and code audits only, and after #20907 from the same batch broke existing installs the maintainer decided that desktop changes do not ship without being run on a real desktop build. To bring it back: open a fresh PR from the original commit, run it on a real build that already has user state on each platform it touches, and put the before/after proof in the body.