Conversation
|
❌ Version File Not Updated The version file was not modified in this PR. Please update the |
There was a problem hiding this comment.
Pull request overview
Improve macOS notarization robustness and align CI signing/notarization behavior for macOS builds.
Changes:
- Refactor notarization call to reuse
appPathand add error handling to skip notarization when the app isn’t signed. - Set
CSC_FOR_PULL_REQUEST: truein the macOS beta-release workflow build step.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| scripts/notarize.mjs | Adds appPath variable and wraps notarization in a try/catch to skip specific unsigned-app failures. |
| .github/workflows/beta-release.yml | Adjusts env to set CSC_FOR_PULL_REQUEST for macOS builds. |
| } | ||
|
|
||
| const appName = packager.appInfo.productFilename; | ||
| const appPath = `${appOutDir}/${appName}.app`; |
There was a problem hiding this comment.
Building file paths via string interpolation is error-prone across environments (path separators, trailing slashes). Prefer using Node’s path.join(appOutDir, ${appName}.app) (and ensure path is imported) to make path construction robust and consistent.
| } catch (error) { | ||
| // Skip notarization if app is not signed (common in PR builds with CSC_FOR_PULL_REQUEST=false) | ||
| if (error.message && error.message.includes('code object is not signed')) { | ||
| console.log( | ||
| '[notarize] Skipping notarization: app is not signed. This is expected in PR builds where code signing is disabled.' | ||
| ); | ||
| return; | ||
| } | ||
| // Re-throw other errors | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
In JavaScript, catch (error) can receive non-Error values (including null/undefined). Accessing error.message will throw if error is nullish, masking the original failure. Guard more defensively (e.g., check error && typeof error === 'object' and that 'message' in error before reading it), or normalize to an Error first.
| console.log(`[notarize] Notarization complete for ${appName}.app`); | ||
| } catch (error) { | ||
| // Skip notarization if app is not signed (common in PR builds with CSC_FOR_PULL_REQUEST=false) | ||
| if (error.message && error.message.includes('code object is not signed')) { |
There was a problem hiding this comment.
Branching on a substring match of an error message is brittle (message text can vary by toolchain/locale/version). If @electron/notarize provides structured properties (error codes/types) for this failure, prefer checking those; otherwise consider matching a more stable signal (e.g., known exit code / stderr pattern captured by the library) and include the original message in logs to aid diagnostics.
| if: matrix.os == 'macos-latest' | ||
| run: npm run build:${{ matrix.target }} | ||
| env: | ||
| CSC_FOR_PULL_REQUEST: true |
There was a problem hiding this comment.
Setting CSC_FOR_PULL_REQUEST: true unconditionally in a beta-release workflow can change signing/notarization behavior for non-PR builds. If the intent is only to affect PR builds, make this conditional on the event (e.g., github.event_name == 'pull_request') or set it at the job/step level only for PR runs to avoid unexpected signing behavior in release pipelines.
No description provided.