Skip to content

fix(desktop): ship assets/ so the packaged window icon resolves - #3451

Merged
Astro-Han merged 1 commit into
apache:mainfrom
ARE404:are404/fix-packaged-asset-path
Aug 21, 2026
Merged

fix(desktop): ship assets/ so the packaged window icon resolves#3451
Astro-Han merged 1 commit into
apache:mainfrom
ARE404:are404/fix-packaged-asset-path

Conversation

@ARE404

@ARE404 ARE404 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

The bug

createWindow resolves the window icon out of apps/desktop/assets:

icon: join(import.meta.dirname, '..', '..', 'assets', 'icon.png'),

files in electron-builder.config.mjs carries dist/**, dist-renderer/**
and package.json. It does not carry assets/, and assets/ is not in
extraResources either — so in a packaged app that path does not exist.

It fails silently: nativeImage reports an unreadable file as an empty
image rather than raising, and BrowserWindow then draws no icon at all. The
same trap the resources/status entry in that file already documents.

macOS hides it, because the dock uses the bundle icon electron-builder
generates at build time from icon: 'assets/icon.png'. Linux has no such
fallback for the window icon.

The fix

  • copy assets/ beside the app, the way resources/status already is
  • resolve the root against process.resourcesPath when packaged

The dev path is unchanged: two levels up from dist/main/ still lands on
apps/desktop.

Notes

Verification

  • tsc -p apps/desktop/tsconfig.main.json --noEmit clean
  • node --test dist/main/__tests__/desktop-assets.test.js — 2/2

I could not verify a packaged build end to end: this machine cannot start an
Electron GUI (it hangs during GUI init), so the dock/window icon was not
observed directly. The path resolution itself is covered by the tests.

`files` in the builder config carries `dist/`, `dist-renderer/` and
`package.json`. It does not carry `assets/`, but `createWindow` resolves the
window icon out of it:

    icon: join(import.meta.dirname, '..', '..', 'assets', 'icon.png')

In a packaged app that path does not exist. The failure is silent — Electron
reports an unreadable file as an empty NativeImage rather than raising — so
the window simply carries no icon, which is most visible on Linux where there
is no executable icon to fall back on.

Copy `assets/` beside the app the way `resources/status` already is, and
resolve the root against `process.resourcesPath` when packaged. The dev path
is unchanged.

@hqhq1025 hqhq1025 left a comment

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.

Review of exact head 4d3956a441a97765b506e6bf803884e758912a91.

No actionable code findings.

Problem and mechanism: the packaged BrowserWindow resolves assets/icon.png from the application resources tree, but the builder previously used that source image only to generate bundle/install icons and did not ship it as a runtime resource. The PR adds the single asset directory to extraResources and selects process.resourcesPath only in packaged builds while preserving the existing repository-relative development path.

The problem definition is correct and the solution follows first principles and Occam's razor. The runtime reads one file, the package now contains that file at the matching location, and development behavior is unchanged. assets/ currently contains only the 728 KB icon, so the directory copy does not introduce unrelated package contents. The small pure path helper is justified by making both runtime layouts testable without booting Electron.

Deletion/refactor assessment: I found no production code or tests that should be removed, and no deeper refactor is required. The two tests cover the meaningful path split without mirroring electron-builder internals.

Installed-runtime verification on this head:

  • full repository build passed;
  • Desktop typecheck, formatting, git diff --check, and the 2/2 focused asset tests passed;
  • an unsigned macOS arm64 unpacked app was built with electron-builder;
  • Maka.app/Contents/Resources/assets/icon.png exists and is byte-identical to the source image (SHA-256 1a4b892e4793d2e54fbb09ed5143b3875f642c33963182acc70127d07c18975a);
  • the packaged path helper resolves exactly to that installed file;
  • the Windows workflow has successfully packaged the installer/ZIP and passed the Windows release verifier.

Merge verdict: the patch itself is ready, but I am not approving while the required GitHub state is red/incomplete. Main CI failed one unrelated Settings/titlebar E2E after 38 tests passed; the exact failed test passed when rerun locally. The Windows upgrade/autoupdate workflow is still running. Re-run the failed CI and wait for the package workflow to finish; if both are green on the same head, this revision is ready to merge without further code changes.

Residual gap: I did not visually inspect a Linux window icon because this repository's release configuration currently packages macOS and Windows. The packaged resource contract itself is verified.

@Astro-Han Astro-Han left a comment

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.

Thanks for fixing the packaged window icon path. The extraResources entry now ships assets/ beside the packaged app, while process.resourcesPath is used only for packaged runs and the existing development path remains unchanged. The focused tests and packaged Windows release checks pass on this exact head, and I found no actionable issues.

AI-assisted review disclosure: OpenAI Codex assisted with the exact-head review of resource packaging, runtime path resolution, cross-platform behavior, tests, and release checks. I verified the final diff, packaged-resource path, current head, CI, and merge state; no unverified AI output is presented as a finding.

@Astro-Han
Astro-Han merged commit d62857a into apache:main Aug 21, 2026
2 of 3 checks passed
@jackwener

Copy link
Copy Markdown
Member

Post-merge review of PR head 4d3956a441a97765b506e6bf803884e758912a91 / merged commit d62857a8357e9160926726a2a13096bc2dc2b91d found one follow-up test gap, not a current runtime defect.

apps/desktop/src/main/__tests__/desktop-assets.test.ts verifies only the path helper. If the new extraResources entry is removed later, both unit tests still pass even though the original packaged-app bug has returned. The artifact verifier already owns this contract: scripts/verify-packaged-app.mjs:702-740 enumerates every required file and is called by both the macOS and Windows verifiers, but its required list omits assets/icon.png.

Please add join('assets', 'icon.png') to assertPackagedResources' required paths in a focused follow-up. That makes the shipped artifact itself fail verification when the source image is not copied, while the existing helper tests continue to cover the packaged/dev path split. The current merged implementation is otherwise sound; no rollback is indicated.

AI-assisted review disclosure: OpenAI Codex inspected the exact diff, current packaging configuration, runtime path resolution, focused tests, and macOS/Windows artifact verifier call sites. This comment is an automated follow-up finding and does not count as independent human review.

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.

4 participants