Build and publish safari browser extension - #681
Conversation
TODO: Setup browser extension project in Apple developer
tobiasdiez
left a comment
There was a problem hiding this comment.
Thanks a lot!
Looks like this PR contains a few unrelated/unnecessary or otherwise strange changes (AI generated??). Can you please revise this, or at least explain the need for the changes.
The main issue is the whole scripts/makefile setup. In the wxt world, you solve this by "modules" that hook into the build progress and modify/postprocess the output. You might be able to reuse https://github.com/rxliuli/wxt-module-safari-xcode or at least get inspiration how its done.
| }, | ||
| commands: { | ||
| _execute_page_action: { | ||
| _execute_action: { |
There was a problem hiding this comment.
Chrome was giving me warnings about this
There was a problem hiding this comment.
safari is giving me errors with this
There was a problem hiding this comment.
There is a conceptual difference between an action and a page action...
There was a problem hiding this comment.
Maybe but it's not working in safari. I tested all other browsers, they work
| }, | ||
| description: | ||
| "The JabRef browser extension imports new bibliographic information directly from the browser into JabRef.", | ||
| developer: { |
There was a problem hiding this comment.
for safari maybe... did you tested the other browsers?
Can you do this please, and then configure the securities in the gh actions to actually be able to test the publish/notarize workflow. |
Add workflow for maual safari tests
* origin/main: Add workflow for maual tests
set unique safari build number
* origin/main: set unique safari build number
|
Uploading to apple app store connect works now. |
|
So you went through the code and reviewed/agreed with all those changes yourself? I don't want to invest a lot of time in reviewing AI created nonsense. |
|
I reviewed and tested all generated code. I added some more explanations to the Swift Bridge and its purpose |
Safari build/publish. The PR merges cleanly upstream; the conflicts are sibling collisions on this integration branch, resolved as unions: - release.yml, test.yml: #681 turns publish into a matrix over browser-stores/safari/safari-distribution, so the experimental artifact upload is now gated on the browser-stores target - it is the only one producing .output/*.zip. build-bridge, attach-bridge, bridge-smoke and experimental-prerelease keep their own jobs. - package.json: experimental's newer vue/vue-tsc/wxt over #681's older base, plus its esbuild, spawn-sync and wxt-module-safari-xcode. - pnpm-lock.yaml: regenerated with pnpm 10.34.1 rather than hand-merged. - README.md: both new sections kept. - safari-signing-test.yml: new in #681; git's modify/delete was rename detection, the file exists at neither the merge base nor experimental. Verified: pnpm install --frozen-lockfile and vitest (1420 passed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AiaGjSfwh9uqDGtbW8tLfU
Conflicts were the version bumps main made to the pnpm/setup-node steps this branch replaced with the setup-browser-extension composite action: kept the composite action and moved the bumps into it, so it matches lint.yml. package.json takes main's vue, typescript, vitest and wxt and keeps this branch's esbuild, spawn-sync and wxt-module-safari-xcode; pnpm-lock.yaml regenerated. One fix beyond the conflicts, needed because main is now on wxt 0.21: wxt 0.21 resolves `modules` entries with Node's import.meta.resolve, whose importer argument is ignored, so the relative module paths resolved inside wxt itself and `wxt prepare` failed. Absolute file URLs work in both versions. Verified: pnpm install, pnpm lint, pnpm test (1420 passed), pnpm build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015DK61Z1E3cKxFbEun5UdD6
There was a problem hiding this comment.
🟡 Changes recommended
The Safari native bridge currently relies on launching /Applications/JabRef.app/... from a sandboxed app/extension, which is likely to fail under App Sandbox constraints and break native messaging in production.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR adds a Safari build pipeline for the JabRef Browser Extension, including WXT Safari/Xcode project generation, local signing/notarization helpers, and GitHub Actions workflows to package and publish the Safari app.
Changes:
- Add Safari as a WXT target and generate an Xcode project/app bundle via
wxt-module-safari-xcode, with post-processing hooks. - Introduce local scripts + entitlements for signing and notarization, plus documentation for Safari build/release steps.
- Extend CI/release workflows to build, package, notarize, and publish the Safari extension (and upload a direct-distribution artifact).
File summaries
| File | Description |
|---|---|
wxt.config.ts |
Adds Safari target, Safari Xcode configuration, and WXT module pipeline for Safari build/project patching. |
scripts/sign_safari_local.sh |
Adds local Developer ID signing script for the generated Safari .app. |
scripts/notarize_safari_local.sh |
Adds local notarization + stapling + zip packaging script for the signed Safari .app. |
scripts/SafariWebExtensionHandler.swift |
Adds a Swift native-message bridge handler for Safari WebExtension native messaging. |
scripts/JabRef Browser Extension.entitlements |
Adds app entitlements used during signing. |
scripts/JabRef Browser Extension Extension.entitlements |
Adds extension entitlements used during signing. |
README.md |
Documents Safari dev/build/sign/notarize flows and CI/release secret requirements. |
CONTRIBUTING.md |
Adds Safari dev/build instructions plus CI/notarization notes. |
modules/jabref-safari-prepare.module.ts |
Post-processes Safari build output (manifest cleanup + background page generation + staging). |
modules/jabref-safari-xcode.module.ts |
Copies the Swift handler into the generated Xcode project after build. |
package.json |
Adds Safari scripts (dev/build/app packaging/sign/notarize/zip) and adds Safari module dependency. |
pnpm-workspace.yaml |
Allows pnpm build scripts for esbuild and spawn-sync. |
pnpm-lock.yaml |
Locks new dependencies (notably wxt-module-safari-xcode, esbuild, spawn-sync). |
.github/workflows/test.yml |
Adds a macOS matrix job to validate pnpm safari:build-app. |
.github/workflows/release.yml |
Adds Safari packaging, signing, notarization, App Store Connect publishing, and release asset upload. |
.github/workflows/safari-signing-test.yml |
Adds a manual workflow to test Safari signing/publish without modifying releases. |
.github/actions/setup-browser-extension/action.yml |
Adds a composite action to standardize Node/pnpm setup across workflows. |
Review details
Suppressed comments (1)
.github/workflows/release.yml:205
- This checkout step pins actions/checkout to v6.0.3, but the workflow also uses v7.0.1 in other jobs. Prefer a single pinned SHA/version across the workflow for consistency and security updates.
uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3
- Files reviewed: 16/17 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| let process = Process() | ||
| process.executableURL = executableURL | ||
| process.arguments = arguments | ||
|
|
||
| let stdout = Pipe() | ||
| let stderr = Pipe() | ||
| process.standardOutput = stdout | ||
| process.standardError = stderr | ||
|
|
||
| do { | ||
| try process.run() | ||
| } catch { | ||
| throw JabRefBridgeError.processLaunchFailed(error.localizedDescription) |
There was a problem hiding this comment.
Apple App Store does not accept extensions without sandbox, only locally it is possible. so the extension can still use the http endpoint
@koppor Maybe we can solve this with the bridge in future ?
tobiasdiez
left a comment
There was a problem hiding this comment.
Sorry that it took me so long to review, but honestly speaking it's AI slop at its finest. I never understood why you guys were so annoyed by AI-created PRs in the JabRef repo, but now I better understand the pain.
My comments are perhaps formulated a bit meaner/more direct than if I were to address a person. I hope Claude can look over this.
| - name: Build Safari app | ||
| if: ${{ matrix.target == 'safari' && github.event_name != 'pull_request' }} | ||
| run: pnpm safari:build-app | ||
|
|
There was a problem hiding this comment.
Can you please walk me through how this works together with the step "Download signed Safari distribution artifact" and the safari-webext-publish-action action in the next step.
There was a problem hiding this comment.
We first build the *.app which is the foundation for both local distribution and app store distribution
However, they require different notarization and entitlements
And we separet notarization as its own job so it can be requeue when it fails for some reason. Can happen when apple is having issues
|
|
||
| - name: Build Safari app | ||
| if: ${{ matrix.target == 'safari-build' }} | ||
| run: pnpm safari:build-app |
There was a problem hiding this comment.
Always a good idea to have 3 workflows to run the same command...
There was a problem hiding this comment.
Different workflows serve different purpose
| Normally, you simply install the extension from the browser store and are ready to go. | ||
|
|
||
| > [Firefox](https://addons.mozilla.org/en-US/firefox/addon/jabref/?src=external-github) - [Chrome](https://chrome.google.com/webstore/detail/jabref-browser-extension/bifehkofibaamoeaopjglfkddgkijdlh) - [Edge](https://microsoftedge.microsoft.com/addons/detail/pgkajmkfgbehiomipedjhoddkejohfna) - [Vivaldi](https://chrome.google.com/webstore/detail/jabref-browser-extension/bifehkofibaamoeaopjglfkddgkijdlh) | ||
| > [Firefox](https://addons.mozilla.org/en-US/firefox/addon/jabref/?src=external-github) - [Chrome](https://chrome.google.com/webstore/detail/jabref-browser-extension/bifehkofibaamoeaopjglfkddgkijdlh) - [Edge](https://microsoftedge.microsoft.com/addons/detail/pgkajmkfgbehiomipedjhoddkejohfna) - [Vivaldi](https://chrome.google.com/webstore/detail/jabref-browser-extension/bifehkofibaamoeaopjglfkddgkijdlh) - Safari (build from source) |
There was a problem hiding this comment.
I thought you have publishing to the apple store now working?
There was a problem hiding this comment.
Publishing to the apple store will require review from apple. This is an alternative way to provide the extension for download as a simple app
| }, | ||
| commands: { | ||
| _execute_page_action: { | ||
| _execute_action: { |
There was a problem hiding this comment.
There is a conceptual difference between an action and a page action...
| }, | ||
| description: | ||
| "The JabRef browser extension imports new bibliographic information directly from the browser into JabRef.", | ||
| developer: { |
There was a problem hiding this comment.
for safari maybe... did you tested the other browsers?
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
* origin/safari: Potential fix for pull request finding Potential fix for pull request finding
|
First of all, thanks for the review, but you are still talking to a human. Sorry that the PR quality is not the best; my focus was on getting it working first. I reviewed and tested the changes and also verified with firefox/chrome. Regarding page browser actions, seems like your knowledge regarding manifestv3 is not up to date |
* origin/main: docs: add git submodule hint (#697) chore: add .idea to .gitignore (#698) chore(deps): update dependency oxfmt to v0.61.0 (#709) chore(deps): update dependency vue-tsc to v3.3.8 (#705) chore(deps): update tailwindcss monorepo to v4.3.3 (#707) chore(deps): update dependency globals to v17.9.0 (#719) # Conflicts: # CONTRIBUTING.md # package.json # pnpm-lock.yaml
* main: fix: Chrome native messaging (#700)
Uh oh!
There was an error while loading. Please reload this page.