Add tagged multi-platform desktop release workflow and unified artifact builder - #135
Merged
Merged
MacroscopeApp / Macroscope - Correctness Check
succeeded
Mar 2, 2026 in 1m 24s
No issues identified (29 code objects reviewed).
• Merge Base:
1af43db
• Head:8c2797a
Details
| ✅ | File Path | Comments Posted |
|---|---|---|
| ➖ | bun.lock |
|
| ➖ | package.json |
|
| ➖ | .github/workflows/release.yml |
|
| ➖ | README.md |
|
| ➖ | docs/release.md |
|
| ✅ | scripts/build-desktop-artifact.ts |
0 |
Filtered Issues Details
scripts/build-desktop-artifact.ts
- line 142: The function
resolveBooleanFlagintroduces a strictness regression where boolean CLI flags set tofalseare ignored in favor of the environment variable default (which might also befalse, or potentiallytrueif defaults change). In the previous implementation (implied by typical CLI parsers), passing--skip-build=falseexplicitly would disable the flag. However,Option.filter(flag, Boolean)filters outfalsevalues, treatingSome(false)asNone. ThenOption.getOrElsereturns the environment value. This means if a user explicitly passesfalsevia CLI but the environment variable or default istrue, the CLI argument is ignored. While the current defaults for these flags arefalse, if a user provides an environment variable likeT3CODE_DESKTOP_SKIP_BUILD=trueand then tries to override it with--skip-build=falseon the command line, thefalsewill be filtered out, and it will resolve totrue. [ Already posted ] - line 234: Similar to the
sipsissue, theiconutilcommand ingenerateMacIconSetinjects${iconsetDir}and${targetIcns}directly into the shell string without quotes. If the temporary directory path or the target path contains spaces,iconutilwill receive malformed arguments and fail. [ Already posted ] - line 257: The
stageMacIconsfunction introduces a runtime crash for cross-compilation scenarios. It unconditionally executes the macOS-exclusive command-line toolssipsandiconutilwhenever the target platform is'mac', regardless of the host OS. If the script is executed on Linux or Windows (allowed by the CLIplatformargument),ChildProcess.spawnwill fail with anENOENTerror when attempting to runsips, causing the build to fail. [ Already posted ] - line 536: The
getDefaultArchfunction blindly accessesconfig.archChoicesand potentially returnsundefinedif the array is empty, butgetDefaultArchis typed to return a non-undefinedBuildArch.Type. While the current static config has non-empty arrays, the function's fallback logicreturn config.archChoices[0] ?? "x64";relies onarchChoices[0]existing. IfarchChoiceswere empty (valid per array type but not per logic),undefinedwould be returned where a string is expected, potentially causing issues downstream. However, a more immediate issue is ingetDefaultArchlogic: ifprocess.archdoes not match the platform's options (e.g. running on an ARM machine but building for a platform that only supports x64 inPLATFORM_CONFIG), it falls back toconfig.archChoices[0]. This change forces a specific architecture fromPLATFORM_CONFIGwhich might mismatch the host ifprocess.archis different, but the more critical runtime crash risk is inbuildDesktopArtifact. Specifically, insidebuildDesktopArtifact, the code constructs a command string:bunx --bun electron-builder ... --${options.arch}. IfgetDefaultArchreturned a fallback that isn't compatible with the current machine (e.g. trying to builduniversalorarm64on a strictly x64 machine without cross-compilation tools set up, or vice versa),electron-builderwill fail. But the most specific runtime crash introduced here is related touniversalsupport. The newBuildArchincludesuniversal. IfgetDefaultArchor the user selectsuniversal, the command becomes... --universal. Electron-builder supports--universalfor macOS (--mac), but if a user (or default logic) selectsuniversalfor Linux or Windows (which do not support universal binaries in the same way via that flag),electron-builderwill fail at runtime.PLATFORM_CONFIGformacincludesuniversal, butlinux/windo not. However, if a user manually passes--arch universalvia CLI for a Windows build,resolveBuildOptionsaccepts it (as it's inBuildArchliterals).buildDesktopArtifactthen executeselectron-builder ... --universal. This flag is invalid for Windows/Linux in standard electron-builder usage and will cause the subprocess to exit with non-zero code, triggering aBuildScriptError. [ Already posted ] - line 536: In
resolveBuildOptions, thetargetresolution usesPLATFORM_CONFIG[platform].defaultTargetas a fallback. TheBuildPlatformschema definesmac,linux,win. IfdetectHostBuildPlatformreturnsundefined(e.g. on unknown OS) and no platform is provided via CLI/Env, the script errors out early. However, if a platform IS provided, saywin,defaultTargetbecomesnsis. The issue arises inbuildDesktopArtifactwhereelectron-builderis invoked. The code executes:electron-builder ${platformConfig.cliFlag} ${options.target} .... If the user provides atargetthat is valid for one platform but not the selected one (e.g.platform=winbuttarget=dmg), or ifBuildPlatformallowsuniversalarch but the target doesn't support it,electron-builderwill crash. Specifically related to the review objects:BuildArchnow allowsuniversal. If a user runsbuild-desktop-artifact --platform win --arch universal,resolveBuildOptionssucceeds.buildDesktopArtifactthen runselectron-builder --win nsis --universal. Electron builder for Windows does not support--universal(it's a Mac concept). This will cause the build command to fail at runtime. [ Already posted ] - line 571: The
resolveBuildOptionsfunction will throw aTypeErrorat runtime when accessingPLATFORM_CONFIG[platform]ifplatformis undefined. WhiledetectHostBuildPlatformhandlesdarwin/linux/win32, it returnsundefinedfor other platforms (e.g.aix,sunos,freebsd). If the user runs this script on an unsupported platform without providing aplatformflag or env var,mergeOptionsreturnsundefined. The subsequent checkif (!platform)catches this, preventing the crash. However, there is a subtlety:mergeOptionsuses the third argument as a default. Ifinput.platformisNone,env.platformisNone, anddetectHostBuildPlatformreturnsundefined, thenplatformbecomesundefined. The checkif (!platform)correctly handles this. [ Already posted ]
Loading