fix(desktop): use a supported macOS bundle icon size - #3353
Draft
sunrioa wants to merge 1 commit into
Draft
Conversation
Refs apache#3352 Refs apache#1919 Refs apache#1920 Generated-by: OpenAI Codex
jackwener
reviewed
Aug 20, 2026
jackwener
left a comment
Member
There was a problem hiding this comment.
Automated Codex review on exact head 654ef0c506b521c5641656bd2e4145e2b1585dce found no actionable code issue.
The bug still exists on current main: both packaged macOS icon reads request Electron's unsupported large size, matching the native IconLoader::ReadIcon() crash evidence in #3352. Centralizing the supported { size: 'normal' } option in loadNativeBundleIcon fixes both call sites without changing the unpackaged bypass, and the focused test would fail if the old option returned.
Required conclusions:
- Optimal for the actual problem: yes.
- Production code to delete: none identified.
- Tests to delete/replace: none identified.
- Deeper refactor: no; the shared icon-loading helper is the correct owner.
- Ready to merge: no; the PR is Draft, has no hosted required
test, and has no independent human review. - Residual risk/gaps: the package-only native crash has not been smoked in a signed/notarized macOS bundle; a maintainer should decide whether the Electron contract plus focused test is sufficient or require that package evidence before leaving Draft.
This changes user-visible packaged-app behavior and therefore requires independent human review under CONTRIBUTING.md. No security, licensing, governance, or public-contract effect was identified.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Use Electron's macOS-supported
normalfile-icon size for the packaged permission overlay. Both native bundle-icon reads now share the same option, while unpackaged development continues to skip native icon loading.Add a regression test covering the unpackaged bypass and the packaged
normaloption.Fixes #3352
Refs #1919
Refs #1920
Verification
Passed locally:
corepack npm --workspace @maka/desktop test— 979 passedcorepack npm --workspace @maka/desktop run e2e— 34 passed, 1 skippedcorepack npm run lint— passedcorepack npm run format:check— passedcorepack npm run build— passedcorepack npm run typecheck— passedcorepack npm exec -- knip --workspace apps/desktop— passedcorepack npm exec -- knip --workspace packages/ui— passedcorepack npm run astryx:theme -- --check— passednode scripts/audit-alignment.mjs— passednode --test --test-concurrency=1 scripts/ci-test-plan.test.mjs— 21 passedcorepack npm run windows:inventory— passedgit diff --check— passedcorepack npm testwas also attempted, but was not green on this machine for failures outside this Desktop change:NODE_NO_WARNINGS=1.Not run:
AI use
Select exactly one:
Tool(s) and scope:
OpenAI Codex assisted with crash-log analysis, implementation, the regression test, local verification, and drafting this description. I reviewed the final diff and test results.
Checklist
Does this PR entail a change in behavior?