Skip to content

fix(electron): configure app icons#622

Merged
Yumiue merged 4 commits into
1024XEngineer:mainfrom
Yumiue:html_progress_clean
May 11, 2026
Merged

fix(electron): configure app icons#622
Yumiue merged 4 commits into
1024XEngineer:mainfrom
Yumiue:html_progress_clean

Conversation

@Yumiue
Copy link
Copy Markdown
Collaborator

@Yumiue Yumiue commented May 11, 2026

构建图标脚本和配置

@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

Copy link
Copy Markdown

@fennoai fennoai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found 2 issues worth addressing: one runtime cleanup bug in the new icon generator, and one undocumented build prerequisite introduced by wiring that generator into build:electron.

Comment thread web/scripts/generate-icons.js Outdated
}
if (outputsAreFresh()) {
console.warn('No local image resizer found; existing icon.ico and icon.icns are up to date.')
process.exit(0)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.exit(0) here skips the outer finally, so the temp directory created at startup is never removed on the "no resizer, outputs already fresh" path. I reproduced this locally: after running the script on Linux without magick, /tmp/neocode-icons-* was left behind. Returning normally from resizeImages() (or moving the freshness check before mkdtempSync) would avoid leaking a temp dir on every invocation.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f37dbd: the fresh-output fallback now returns normally instead of calling process.exit(0), so the outer finally always removes the temporary directory.

Comment thread web/package.json Outdated
"dev:electron": "node scripts/clean-electron.js && node scripts/build-gateway.js && vite --mode electron",
"build": "tsc -b && vite build",
"build:electron": "node scripts/clean-electron.js && vite build --mode electron && node scripts/build-gateway.js && node scripts/verify-electron-preload.js && electron-builder --config electron-builder.config.cjs",
"build:electron": "node scripts/clean-electron.js && node scripts/generate-icons.js && vite build --mode electron && node scripts/build-gateway.js && node scripts/verify-electron-preload.js && electron-builder --config electron-builder.config.cjs",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes build:electron depend on the new icon-generation step, but I couldn't find any README/docs update about the new prerequisite. On Linux builders without ImageMagick the command only works while the checked-in icon.ico/icon.icns are newer than build/icon.png; once the source icon changes, build:electron starts hard-failing with No local image resizer found. Please document that dependency or make the build path independent of host tooling.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8f37dbd: build:electron no longer runs the icon generator, so the release build uses the checked-in icon assets without requiring host image tooling. I also documented generate:icons as a manual maintenance command and listed its Windows/macOS/Linux tool requirements.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Yumiue Yumiue merged commit 3a1c41f into 1024XEngineer:main May 11, 2026
3 checks passed
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.

1 participant