Implement onboarding, update management, and version 2.1.0 enhancements#39
Open
vordenken wants to merge 46 commits into
Open
Implement onboarding, update management, and version 2.1.0 enhancements#39vordenken wants to merge 46 commits into
vordenken wants to merge 46 commits into
Conversation
Update version to 2.1.0 in manifest and project files Expose update controller in AppDelegate for onboarding Enable automatic update checks in Info.plist Implement onboarding pages for user guidance Enhance main view with update settings and controls Refactor update controller to manage beta updates Handle onboarding completion and settings in ViewController Create build and release documentation Establish security policy for vulnerability reporting Update appcast for new versioning scheme Add Renovate configuration for dependency management Set version to 2.1.0 in semver file
Reviewer's GuideImplements a three-step onboarding flow with persisted state, exposes and wires update/beta-channel settings through a new UpdateController API into the WebView UI, introduces a GitHub Actions-based automated build-and-release pipeline around semver.txt and Sparkle, and fixes the keyboard PiP toggle to use the W3C PiP API to avoid CSS-sized PiP windows. Sequence diagram for onboarding flow and update settings persistencesequenceDiagram
actor User
participant WebViewHTMLJS as WebView_HTML_JS
participant WKWebView
participant ViewController
participant UpdateController
participant SPUUpdater
participant SFSafariApplication
User->>WKWebView: Launch AutoPiP app
WKWebView->>ViewController: webView(_:didFinish:)
ViewController->>WKWebView: evaluateJavaScript("setVersion")
ViewController->>WKWebView: evaluateJavaScript("startOnboarding()")
User->>WebViewHTMLJS: Click "Done – Open Safari"
WebViewHTMLJS->>WebViewHTMLJS: Collect onb-auto-check/onb-auto-download/onb-beta
WebViewHTMLJS->>WKWebView: postMessage("onboarding-done:" + JSON)
WKWebView->>ViewController: userContentController(_:didReceive:)
ViewController->>ViewController: handleOnboardingDone(jsonString)
ViewController->>UpdateController: set automaticallyChecksForUpdates
ViewController->>UpdateController: set automaticallyDownloadsUpdates
ViewController->>UpdateController: set isBetaUpdatesEnabled
UpdateController->>SPUUpdater: automaticallyChecksForUpdates / automaticallyDownloadsUpdates
UpdateController->>SPUUpdater: resetUpdateCycleAfterShortDelay()
ViewController->>ViewController: UserDefaults["OnboardingCompleted"] = true
ViewController->>SFSafariApplication: showPreferencesForExtension
SFSafariApplication-->>ViewController: completion(error?)
ViewController->>SFSafariApplication: openApplication (fallback if error)
ViewController->>NSApplication: terminate(nil)
Sequence diagram for update and beta channel setting changes in main viewsequenceDiagram
actor User
participant WebViewHTMLJS as WebView_HTML_JS
participant WKWebView
participant ViewController
participant UpdateController
participant UpdaterDelegate
participant SPUUpdater
User->>WKWebView: Reopen AutoPiP app
WKWebView->>ViewController: webView(_:didFinish:)
ViewController->>WKWebView: evaluateJavaScript("setVersion")
ViewController->>ViewController: showMainView()
ViewController->>WKWebView: evaluateJavaScript("show(enabled,useSettings)")
ViewController->>UpdateController: read automaticallyChecksForUpdates / automaticallyDownloadsUpdates / isBetaUpdatesEnabled
ViewController->>WKWebView: evaluateJavaScript("setUpdateSettings(settings)")
User->>WebViewHTMLJS: Toggle "Include beta updates"
WebViewHTMLJS->>WKWebView: postMessage("set-beta:true")
WKWebView->>ViewController: userContentController(_:didReceive:)
ViewController->>UpdateController: isBetaUpdatesEnabled = true
UpdateController->>UserDefaults: set(BetaUpdatesEnabled,true)
UpdateController->>SPUUpdater: resetUpdateCycleAfterShortDelay()
SPUUpdater->>UpdaterDelegate: allowedChannels(for:)
UpdaterDelegate-->>SPUUpdater: Set(["beta"]) (when BetaUpdatesEnabled)
Sequence diagram for keyboard PiP toggle using W3C API with WebKit fallbacksequenceDiagram
actor User
participant ContentScript as content_js
participant VideoElement as HTMLVideoElement
participant BrowserPiP as Browser_PiP_System
User->>ContentScript: Press keyboard shortcut (⌥P)
ContentScript->>ContentScript: togglePiP()
alt PiP currently active
ContentScript->>ContentScript: disablePiP()
else No PiP active
ContentScript->>VideoElement: check typeof video.requestPictureInPicture
alt requestPictureInPicture available
ContentScript->>VideoElement: requestPictureInPicture()
VideoElement-->>BrowserPiP: Open PiP at intrinsic size
VideoElement-->>ContentScript: Promise reject on error
ContentScript->>ContentScript: catch(error)
ContentScript->>ContentScript: setWebkitPresentationMode(video,"picture-in-picture")
else Only WebKit API available
ContentScript->>ContentScript: setWebkitPresentationMode(video,"picture-in-picture")
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAutoPiP 2.1.0 adds onboarding with configurable Sparkle updates, a W3C PiP fallback, updated version metadata, an automated macOS release workflow, appcast entries, and repository issue, security, Renovate, and documentation configuration. ChangesOnboarding and Update Settings
Automated Release Pipeline
Repository Configuration and Community Files
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WKWebView
participant ViewController
participant UpdateController
participant SafariExtensionState
WKWebView->>ViewController: webView(_:didFinish:)
ViewController->>WKWebView: setVersion(v)
ViewController->>WKWebView: startOnboarding() or show()
WKWebView->>ViewController: onboarding-done or update command
ViewController->>UpdateController: apply settings or check updates
ViewController->>SafariExtensionState: query extension state
SafariExtensionState-->>ViewController: enabled state
ViewController->>WKWebView: setUpdateSettings(settings)
sequenceDiagram
participant Workflow
participant Keychain
participant Xcode
participant SparkleTools
participant Appcast
participant GitHubRelease
Workflow->>Keychain: import signing certificate
Workflow->>Xcode: archive app and create DMG
Workflow->>SparkleTools: sign DMG
SparkleTools-->>Workflow: EdDSA signature
Workflow->>Appcast: insert and deduplicate release item
Workflow->>GitHubRelease: create release and upload DMG
Workflow->>Appcast: push updated feed
Workflow->>Keychain: delete temporary keychain
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- There are several hard-coded UserDefaults keys (
"OnboardingCompleted","BetaUpdatesEnabled", etc.) scattered across ViewController, the updater delegate, and JavaScript; consider centralizing these keys in a single Swift enum/struct (and mirroring them in JS constants) to avoid typos and make future changes safer. - The string-based message protocol between the web view and
ViewController(e.g."set-auto-check:true","open-url:" + href,"onboarding-done:" + JSON) is starting to get complex; you might want to move to a small structured JSON message format withtype/payload` fields to make parsing more robust and easier to extend.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There are several hard-coded UserDefaults keys (`"OnboardingCompleted"`, `"BetaUpdatesEnabled"`, etc.) scattered across ViewController, the updater delegate, and JavaScript; consider centralizing these keys in a single Swift enum/struct (and mirroring them in JS constants) to avoid typos and make future changes safer.
- The string-based message protocol between the web view and `ViewController` (e.g. `"set-auto-check:true"`, `"open-url:" + href`, `"onboarding-done:" + JSON) is starting to get complex; you might want to move to a small structured JSON message format with `type`/`payload` fields to make parsing more robust and easier to extend.
## Individual Comments
### Comment 1
<location path="AutoPiP/Resources/Script.js" line_range="118-120" />
<code_context>
+ webkit.messageHandlers.controller.postMessage("check-for-updates");
+ });
+
+ document.getElementById('auto-check-toggle').addEventListener('change', function() {
+ webkit.messageHandlers.controller.postMessage("set-auto-check:" + this.checked);
+ var dl = document.getElementById('auto-download-toggle');
+ dl.disabled = !this.checked;
+ if (!this.checked) dl.checked = false;
</code_context>
<issue_to_address>
**issue (bug_risk):** Keep native `autoDownload` flag in sync when disabling it via the `auto-check` toggle.
In this handler you disable and uncheck `auto-download-toggle` only in the DOM, but you never send `set-auto-download:false` to the native side as is done in onboarding. That leaves Sparkle’s `automaticallyDownloadsUpdates` still `true` after a user turns off auto-checking.
You can align behavior by also posting the native message when auto-check is turned off:
```js
document.getElementById('auto-check-toggle').addEventListener('change', function() {
webkit.messageHandlers.controller.postMessage("set-auto-check:" + this.checked);
var dl = document.getElementById('auto-download-toggle');
dl.disabled = !this.checked;
if (!this.checked) {
dl.checked = false;
webkit.messageHandlers.controller.postMessage("set-auto-download:false");
}
});
```
</issue_to_address>
### Comment 2
<location path="BUILD.md" line_range="55" />
<code_context>
+### GitHub App for Branch Protection
+
+If `main` has branch protection, the workflow needs a GitHub App to push the appcast update. Create a GitHub App with **Contents: Read & Write** permission, install it on the repo, then:
+- Add variable `CLienT_ID` with the App's client ID
+- Add secret `APP_PRIVATE_KEY` with the App's private key
+- Add the App to the branch protection bypass list
</code_context>
<issue_to_address>
**issue (typo):** Typo in variable name: `CLienT_ID` should be `CLIENT_ID` for consistency
Below you use `CLIENT_ID` in the secrets section. Please update this bullet to `CLIENT_ID` as well so the variable name is consistent and avoids configuration confusion.
```suggestion
- Add variable `CLIENT_ID` with the App's client ID
```
</issue_to_address>
### Comment 3
<location path="BUILD.md" line_range="49" />
<code_context>
+- Sets the version in the Xcode project and `manifest.json`
+- Builds, archives, and creates a DMG
+- Signs the DMG with the code signing certificate and Sparkle EdDSA
+- Creates a git tag and GitHub Release with changelog + installation instructions
+- Updates `appcast.xml` (with changelog as `<description>`) on the working branch and `main`
+
</code_context>
<issue_to_address>
**nitpick (typo):** Consider capitalizing "Git" as a proper noun
Here, "git" should be capitalized as "Git" to match its proper name and stay consistent with "GitHub" in the same sentence.
```suggestion
- Creates a Git tag and GitHub Release with changelog + installation instructions
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
.github/workflows/build-release.yml (1)
18-18: Pin GitHub Actions to commit SHAs for supply-chain security.These actions use tag-pinning (
@v*) instead of immutable commit SHAs, which increases supply-chain attack risk. Consider enabling Renovate'shelpers:pinGitHubActionDigestspreset (or upgrading toconfig:best-practices) to automatically pin actions to full-length commit SHAs with version comments for maintainability.Lines: 18, 68, 74, 341, 400
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-release.yml at line 18, The GitHub Actions in this workflow are pinned to semantic version tags (such as `@v6` in actions/checkout@v6) instead of immutable commit SHAs, which introduces supply-chain security risks. Replace each action version tag reference with the full commit SHA of that action version, and include the version tag as a comment for maintainability (for example, replace `@v6` with @<full-commit-sha> # v6). Alternatively, enable Renovate's helpers:pinGitHubActionDigests preset or upgrade to config:best-practices to automatically manage this pinning going forward.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/ISSUE_TEMPLATE/config.yml:
- Line 1: In the `.github/ISSUE_TEMPLATE/config.yml` file, rename the
configuration key from `blank_issues` to `blank_issues_enabled`. GitHub's issue
template configuration schema uses `blank_issues_enabled` as the correct key
name to control whether blank issues are available in the issue template
chooser. Update this key name to match the expected schema.
In @.github/workflows/build-release.yml:
- Around line 8-10: The workflow-level permissions are granting write access to
all jobs via `contents: write`, which violates the principle of least privilege.
Remove the workflow-level `contents: write` permission to revert to the default
read-only state, then add a job-level permissions section specifically to the
`build-and-release` job that grants `contents: write` only to that job. This
ensures the `check` job and any other jobs retain read-only permissions while
only the `build-and-release` job has the necessary write access.
- Around line 124-130: The tag existence check in the workflow hard-fails if a
tag already exists, which prevents idempotent reruns when a release partially
succeeds but a later step fails. Instead of exiting with an error when the tag
is detected, modify the logic to handle the existing tag case gracefully by
updating or reconciling the existing release and appcast rather than aborting.
This allows subsequent workflow reruns to recover from failures and complete the
release process without being blocked by the previously-created tag.
- Around line 24-33: Refactor the GitHub Actions workflow to prevent template
injection and spoofing vulnerabilities by moving GitHub context expressions from
direct `run:` script expansion to the `env:` section. Replace the bot detection
logic that uses `github.event.head_commit.author.name` (which is user-controlled
and spoofable) with `github.actor` instead. Define environment variables in the
`env:` block above the `run:` step, then reference them as shell variables
within the script. Apply this same hardening pattern at lines 93 and 417 where
similar direct template expansion and bot detection logic exists.
In `@AutoPiP/UpdateController.swift`:
- Around line 30-38: The setters for automaticallyChecksForUpdates and
automaticallyDownloadsUpdates in UpdateController do not enforce the constraint
that auto-download must be gated by auto-check (auto-download cannot be true if
auto-check is false). Update the setter for automaticallyChecksForUpdates to set
automaticallyDownloadsUpdates to false whenever auto-check is disabled, and
update the setter for automaticallyDownloadsUpdates to prevent enabling
auto-download when auto-check is false (either by rejecting the change or
forcing auto-check to true). This ensures the dependency is enforced at the
controller layer and prevents callers from creating invalid states.
In `@BUILD.md`:
- Line 55: Fix the variable name typo in the GitHub App setup documentation at
line 55 of BUILD.md. The variable name is currently written as `CLienT_ID` with
inconsistent capitalization, which will cause workflow configuration to fail.
Correct this to the proper constant naming convention `CLIENT_ID` to match the
actual variable name used in the application configuration.
In `@README.md`:
- Line 40: The update note in the README at the "Updating" section states that
Sparkle automatically checks for updates once per day as unconditional behavior.
Since auto-update checks are now user-configurable via a toggle, revise this
sentence to clarify that automatic daily checks only occur when the user has
enabled this feature. Reference the new toggle in the statement to make it clear
that users can control whether this automatic checking behavior is active.
---
Nitpick comments:
In @.github/workflows/build-release.yml:
- Line 18: The GitHub Actions in this workflow are pinned to semantic version
tags (such as `@v6` in actions/checkout@v6) instead of immutable commit SHAs,
which introduces supply-chain security risks. Replace each action version tag
reference with the full commit SHA of that action version, and include the
version tag as a comment for maintainability (for example, replace `@v6` with
@<full-commit-sha> # v6). Alternatively, enable Renovate's
helpers:pinGitHubActionDigests preset or upgrade to config:best-practices to
automatically manage this pinning going forward.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a48e89bb-c91e-48e8-8bc8-32b3bd951291
📒 Files selected for processing (22)
.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/pull_request_template.md.github/workflows/build-release.ymlAutoPiP Extension/Resources/content.jsAutoPiP Extension/Resources/manifest.jsonAutoPiP.xcodeproj/project.pbxprojAutoPiP.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolvedAutoPiP/AppDelegate.swiftAutoPiP/Info.plistAutoPiP/Resources/Base.lproj/Main.htmlAutoPiP/Resources/Script.jsAutoPiP/Resources/Style.cssAutoPiP/UpdateController.swiftAutoPiP/ViewController.swiftBUILD.mdREADME.mdSECURITY.mdappcast.xmlrenovate.jsonsemver.txt
…hen checks are turned off
…sabled when auto-check is turned off
…s and enhance versioning logic
… paths-ignore comments
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
❌ Failed to create PR with unit tests: AGENT_CHAT: Failed to open pull request |
Owner
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The GitHub Actions trigger uses
paths-ignore: ['.github/**']while the comment above says “this workflow itself still trigger a build”; if you want changes to workflow files to trigger builds, remove.github/**frompaths-ignoreor narrow it (e.g. only ignore.github/ISSUE_TEMPLATE/**). - The WKWebView
evaluateJavaScript("setUpdateSettings(...)" )call inshowMainView()runs outside of aDispatchQueue.main.asyncblock, unlike the extension state call; consider dispatching it explicitly to the main thread to avoid any potential threading issues with WKWebView.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The GitHub Actions trigger uses `paths-ignore: ['.github/**']` while the comment above says “this workflow itself still trigger a build”; if you want changes to workflow files to trigger builds, remove `.github/**` from `paths-ignore` or narrow it (e.g. only ignore `.github/ISSUE_TEMPLATE/**`).
- The WKWebView `evaluateJavaScript("setUpdateSettings(...)" )` call in `showMainView()` runs outside of a `DispatchQueue.main.async` block, unlike the extension state call; consider dispatching it explicitly to the main thread to avoid any potential threading issues with WKWebView.
## Individual Comments
### Comment 1
<location path=".github/workflows/build-release.yml" line_range="143-150" />
<code_context>
+ echo '```' >> "$GITHUB_STEP_SUMMARY"
+ fi
+
+ - name: Check for existing tag
+ run: |
+ TAG="${{ steps.version.outputs.tag }}"
+ if git rev-parse "refs/tags/${TAG}" >/dev/null 2>&1; then
+ echo "::warning::Tag ${TAG} already exists — assuming partial release, will reconcile (re-upload asset, refresh appcast)."
+ echo "tag_exists=true" >> "$GITHUB_OUTPUT"
+ else
+ echo "tag_exists=false" >> "$GITHUB_OUTPUT"
+ fi
+
</code_context>
<issue_to_address>
**nitpick:** The `Check for existing tag` step computes an output that is never consumed, and the step has no `id`.
This step writes `tag_exists` to `$GITHUB_OUTPUT`, but without an `id` the output can’t be used and isn’t referenced anywhere else. Either assign an `id` and consume `steps.<id>.outputs.tag_exists` where needed, or remove the unused output to keep the workflow simpler and clearer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+143
to
+150
| - name: Check for existing tag | ||
| run: | | ||
| TAG="${{ steps.version.outputs.tag }}" | ||
| if git rev-parse "refs/tags/${TAG}" >/dev/null 2>&1; then | ||
| echo "::warning::Tag ${TAG} already exists — assuming partial release, will reconcile (re-upload asset, refresh appcast)." | ||
| echo "tag_exists=true" >> "$GITHUB_OUTPUT" | ||
| else | ||
| echo "tag_exists=false" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
nitpick: The Check for existing tag step computes an output that is never consumed, and the step has no id.
This step writes tag_exists to $GITHUB_OUTPUT, but without an id the output can’t be used and isn’t referenced anywhere else. Either assign an id and consume steps.<id>.outputs.tag_exists where needed, or remove the unused output to keep the workflow simpler and clearer.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/build-release.yml (1)
26-28: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winShallow
fetch-depth: 2can make thesemver.txtdiff silently fail (and silently skip real releases).For multi-commit pushes to
main,github.event.beforemay not be present in a depth-2 clone, sogit diff "$BEFORE" "$HEAD_SHA"fails. Since the script has noset -e, the failure is swallowed andgrep -qsees empty output, causingshould-build=false— a realsemver.txtchange onmaincould be silently skipped rather than triggering a release.🔧 Proposed fix
- uses: actions/checkout@v6 with: - fetch-depth: 2 + fetch-depth: 0Also applies to: 61-64
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-release.yml around lines 26 - 28, Update the checkout steps used by the release-detection logic to fetch sufficient history for github.event.before, replacing the shallow fetch-depth: 2 configuration in both referenced locations. Ensure git diff "$BEFORE" "$HEAD_SHA" can resolve the comparison commits for multi-commit pushes to main.
♻️ Duplicate comments (1)
.github/workflows/build-release.yml (1)
143-151: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
tag_existsoutput is unreachable and the "will reconcile" claim isn't implemented.This step lacks an
id:, sotag_existscan never be read viasteps.<id>.outputs.tag_exists— no downstream step consumes it. The warning promises reconciliation ("re-upload asset, refresh appcast"), but nothing branches on this value.Additionally, for beta releases the tag is always the next auto-incremented number (Lines 118-129), so a retry after a partial failure will practically never hit this "already exists" path for the failed tag — it just mints a brand-new beta tag/release each retry instead of reconciling the broken one.
🔧 Suggested direction
- name: Check for existing tag + id: check-tag run: |Then either skip re-tagging when
tag_exists == 'true'and reuse the same tag, or reorder beta-number computation to check for an existing unfinished release for the current commit before incrementing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-release.yml around lines 143 - 151, Fix the release retry flow around the “Check for existing tag” step: make its tag_exists output consumable by assigning the step an id, then implement downstream branching so an existing tag reuses the current tag and skips re-tagging while reconciling the release assets/appcast. For beta releases, check for an existing unfinished release for the current commit before generating the next auto-incremented tag, rather than always creating a new tag on retry.
🧹 Nitpick comments (2)
.github/workflows/build-release.yml (2)
182-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
BUILDnumber calculation across two steps.Both steps independently derive
BUILDfromgithub.run_number + 10. Currently consistent, but this duplication risks silent drift between the app's embeddedCURRENT_PROJECT_VERSIONand the appcast'ssparkle:versionif either formula is edited without the other.♻️ Suggested refactor
Compute
BUILDonce in the "Read version and changelog" step and emit it assteps.version.outputs.build, then reference${{ steps.version.outputs.build }}in both later steps instead of recomputing.Also applies to: 241-244
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-release.yml around lines 182 - 187, Compute the build number once in the “Read version and changelog” step and expose it as the `build` output of `steps.version`. Update both later steps to reference `${{ steps.version.outputs.build }}` instead of independently deriving `github.run_number + 10`, keeping the embedded version and appcast version synchronized.
49-53: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBeta appcast entries accumulate indefinitely; every feature-branch push triggers a full macOS build+release.
The dedup logic only strips entries whose
sparkle:shortVersionStringexactly matches the bareVERSION, which never matches beta strings like"2.1.0-beta19". Combined with "always build" on any non-doc push to a feature branch (Line 49-53),appcast.xmlaccumulates a permanent, unbounded set of beta<item>entries per version (already 7 for2.1.0), and every such push consumes a full macOS runner for build/sign/DMG/release.Consider pruning older beta entries for the same base version (e.g., keep only the last N), and/or gating beta builds on something narrower than "any push" to reduce cost and appcast growth.
Also applies to: 344-357
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/build-release.yml around lines 49 - 53, Update the feature-branch condition using REF_NAME so beta releases are not built for every non-documentation push; gate them on the narrower release-trigger criteria used by the workflow. Also update the appcast deduplication logic to recognize beta versions by their base VERSION and retain only the most recent bounded number of beta entries per base version, preventing unbounded accumulation while preserving the latest beta releases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-release.yml:
- Around line 76-78: Update the workflow concurrency configuration around the
existing concurrency block to serialize release-critical runs across all
branches, rather than grouping by github.ref. Use a single stable workflow-wide
group while preserving cancel-in-progress behavior, so beta tag generation and
the main-branch appcast push cannot run concurrently.
- Around line 7-16: Update the paths-ignore list in the workflow trigger
configuration to stop excluding all of .github/**, while continuing to ignore
the intended documentation and repository metadata files. Ensure changes to
.github/workflows/build-release.yml itself remain eligible to trigger the
workflow as stated by the comment.
---
Outside diff comments:
In @.github/workflows/build-release.yml:
- Around line 26-28: Update the checkout steps used by the release-detection
logic to fetch sufficient history for github.event.before, replacing the shallow
fetch-depth: 2 configuration in both referenced locations. Ensure git diff
"$BEFORE" "$HEAD_SHA" can resolve the comparison commits for multi-commit pushes
to main.
---
Duplicate comments:
In @.github/workflows/build-release.yml:
- Around line 143-151: Fix the release retry flow around the “Check for existing
tag” step: make its tag_exists output consumable by assigning the step an id,
then implement downstream branching so an existing tag reuses the current tag
and skips re-tagging while reconciling the release assets/appcast. For beta
releases, check for an existing unfinished release for the current commit before
generating the next auto-incremented tag, rather than always creating a new tag
on retry.
---
Nitpick comments:
In @.github/workflows/build-release.yml:
- Around line 182-187: Compute the build number once in the “Read version and
changelog” step and expose it as the `build` output of `steps.version`. Update
both later steps to reference `${{ steps.version.outputs.build }}` instead of
independently deriving `github.run_number + 10`, keeping the embedded version
and appcast version synchronized.
- Around line 49-53: Update the feature-branch condition using REF_NAME so beta
releases are not built for every non-documentation push; gate them on the
narrower release-trigger criteria used by the workflow. Also update the appcast
deduplication logic to recognize beta versions by their base VERSION and retain
only the most recent bounded number of beta entries per base version, preventing
unbounded accumulation while preserving the latest beta releases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 502aa711-b6df-45bb-8b0f-60db1e614cd9
📒 Files selected for processing (6)
.github/ISSUE_TEMPLATE/config.yml.github/workflows/build-release.ymlAutoPiP/Resources/Script.jsAutoPiP/UpdateController.swiftREADME.mdappcast.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/ISSUE_TEMPLATE/config.yml
- AutoPiP/UpdateController.swift
- AutoPiP/Resources/Script.js
Comment on lines
+7
to
+16
| # Skip the workflow entirely when only docs/repo-meta/appcast files changed. | ||
| # Source, semver.txt and this workflow itself still trigger a build. | ||
| - '**.md' | ||
| - 'LICENSE' | ||
| - 'PRIVACY.md' | ||
| - 'SECURITY.md' | ||
| - 'renovate.json' | ||
| - '.gitignore' | ||
| - '.github/**' | ||
| - 'appcast.xml' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
paths-ignore: .github/** contradicts the stated intent.
The comment states "this workflow itself still trigger a build," but .github/** matches .github/workflows/build-release.yml itself. A push that only edits this workflow file would be skipped entirely by GitHub's path filtering (not just gated by the check job), making it impossible to trigger a normal push-based test of workflow changes (aside from workflow_dispatch).
🔧 Proposed fix
- - '.github/**'
+ - '.github/ISSUE_TEMPLATE/**'
+ - '.github/pull_request_template.md'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Skip the workflow entirely when only docs/repo-meta/appcast files changed. | |
| # Source, semver.txt and this workflow itself still trigger a build. | |
| - '**.md' | |
| - 'LICENSE' | |
| - 'PRIVACY.md' | |
| - 'SECURITY.md' | |
| - 'renovate.json' | |
| - '.gitignore' | |
| - '.github/**' | |
| - 'appcast.xml' | |
| # Skip the workflow entirely when only docs/repo-meta/appcast files changed. | |
| # Source, semver.txt and this workflow itself still trigger a build. | |
| - '**.md' | |
| - 'LICENSE' | |
| - 'PRIVACY.md' | |
| - 'SECURITY.md' | |
| - 'renovate.json' | |
| - '.gitignore' | |
| - '.github/ISSUE_TEMPLATE/**' | |
| - '.github/pull_request_template.md' | |
| - 'appcast.xml' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-release.yml around lines 7 - 16, Update the
paths-ignore list in the workflow trigger configuration to stop excluding all of
.github/**, while continuing to ignore the intended documentation and repository
metadata files. Ensure changes to .github/workflows/build-release.yml itself
remain eligible to trigger the workflow as stated by the comment.
Comment on lines
+76
to
+78
| concurrency: | ||
| group: build-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Per-ref concurrency doesn't protect shared resources across branches.
build-${{ github.ref }} only serializes runs on the same ref. Two simultaneously-running feature branches (or a feature branch + main) can still race on:
- Beta tag numbering (Lines 118-129): both read the same
git tag -l "v${VERSION}-beta*"snapshot and can compute the sameBETA_NUM, racing to create/publish the same tag. - The
main-branch appcast push (Lines 433-467): two branches pushingappcast.xmltomainconcurrently can hit a non-fast-forward rejection with no retry/rebase, failing the job after the release/DMG already succeeded.
Consider a workflow-wide (not per-ref) concurrency group for the release-critical portion, or add fetch+rebase retry around the main push and re-check tag existence immediately before tag creation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-release.yml around lines 76 - 78, Update the
workflow concurrency configuration around the existing concurrency block to
serialize release-critical runs across all branches, rather than grouping by
github.ref. Use a single stable workflow-wide group while preserving
cancel-in-progress behavior, so beta tag generation and the main-branch appcast
push cannot run concurrently.
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
1 participant
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.
🚀 Release v2.1.0 — Onboarding & Automated Releases
This PR promotes v2.1.0 from beta to stable after 13 beta releases and extensive testing.
✨ What's New
🎨 Onboarding Flow
First-time users are now guided through a 3-step setup:
State is persisted via
UserDefaults(OnboardingCompletedflag) — the flow only runs once per user.⚙️ Configurable Update Settings
Update preferences are now first-class settings, exposed via the onboarding flow and the main app view:
All settings are wired through
UpdateControllerto the underlying SparkleSPUUpdater.🔧 Under the Hood
🤖 Automated Builds & Releases
Releases are now fully automated via GitHub Actions (
.github/workflows/build-release.yml):semver.txtis the single source of truth for version + changelogfeature/*→ auto-incrementedv2.1.0-beta{N}(never overwritten)mainwith changedsemver.txt→ stablev2.1.0releaseappcast.xml🧪 Beta Release Channel
Users can now opt in to beta updates through Sparkle's channel mechanism (
allowedChannelsreturns["beta"]whenBetaUpdatesEnabledis true). The appcast marks beta releases with a visible warning banner.🐛 Bugfix: Keyboard PiP small in corner
Fixed a bug where the keyboard shortcut (
⌥P) opened PiP at the video element's CSS-rendered size instead of its intrinsic dimensions. The toggle now prefers the W3CrequestPictureInPicture()API and falls back to the WebKit API on failure. This affected YouTube's mini-player (which renders the<video>element very small) and other CSS-scaled video players.🧪 Testing
This release has been validated through 13 beta releases with no regressions reported. Manual test plan:
📝 Notes
appcast.xmlonmainwill be auto-updated by the workflow after merge — the beta banner will be removed and thesparkle:channelwill be set tostable.✅ Checklist
semver.txt,manifest.json, and Xcode projectSummary by Sourcery
Introduce a guided onboarding flow, configurable update channel and settings, and automated build-and-release pipeline for the AutoPiP v2.1.0 release.
New Features:
Bug Fixes:
Enhancements:
Build:
CI:
Documentation:
Chores:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation