feat: automatic update detection and release pipeline#12
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 2 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
📝 WalkthroughWalkthroughAdds a complete update detection and distribution system: new update service that fetches release manifests, server API and caching, frontend updater module/UI and scheduled checks, build pipeline producing AppImage/MSIX and Pages metadata, tests, docs, and packaging templates. Changes
Sequence DiagramsequenceDiagram
participant App as App Init / Scheduler
participant Frontend as Frontend (UI)
participant API as Backend API (/api/v1/update)
participant Updater as Update Service
participant Manifest as Remote Manifest (Pages)
App->>Frontend: scheduleAutoUpdateCheck()
Frontend->>API: GET /api/v1/update
API->>Updater: Check(currentVersion)
Updater->>Manifest: HTTP GET stable.json (2s timeout)
Manifest-->>Updater: JSON manifest
Updater->>Updater: normalize & compare versions
alt update available
Updater-->>API: Result{available:true,kind,action_url,...}
API-->>Frontend: 200 OK with UpdateInfo
Frontend->>Frontend: set updateInfo signal
Frontend->>Frontend: render update chip / settings
Frontend->>Frontend: user -> openUpdateAction() or dismissAvailableUpdate()
else no update
Updater-->>API: Result{available:false,kind:"none"}
API-->>Frontend: 200 OK
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
frontend/src/main.tsx (1)
96-96: Isolate updater scheduling failures from app bootstrap.Line 96 runs inside the main bootstrap
try; if scheduling throws, startup flips to global error state. Consider guarding it locally so update checks are best-effort.Proposed refactor
watchVersions(); - scheduleAutoUpdateCheck(); + try { + scheduleAutoUpdateCheck(); + } catch (err: unknown) { + appendLog('system', `Update scheduler disabled: ${errMessage(err)}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/main.tsx` at line 96, The call to scheduleAutoUpdateCheck is currently inside the main bootstrap try block and can throw, flipping the app into a global error state; move or wrap the scheduleAutoUpdateCheck invocation in its own local try/catch so update scheduling is best-effort: call scheduleAutoUpdateCheck() inside a small try block, catch and log the error (using the same logger used during bootstrap) without rethrowing, so failures in scheduleAutoUpdateCheck do not affect the surrounding bootstrap flow or error state.frontend/static/style.css (1)
97-109: Add explicit keyboard focus styles for the new update controls.Defining
:focus-visiblefor.update-chipand.update-chip-dismisswill make focus indication consistent with the rest of the custom-themed UI.♿ Suggested CSS patch
.update-chip:hover{background:color-mix(in srgb,var(--accent) 18%,var(--surface-1));border-color:var(--accent)} +.update-chip:focus-visible{ + outline:1px solid color-mix(in srgb,var(--accent) 60%,transparent); + box-shadow:0 0 0 2px var(--accent-glow); +} .update-chip-dismiss{ border:none;background:transparent;color:var(--text-muted);font-size:10px;font-weight:600; font-family:var(--font-mono);cursor:pointer;padding:3px 4px;border-radius:var(--r-sm); } .update-chip-dismiss:hover{background:var(--surface-2);color:var(--text)} +.update-chip-dismiss:focus-visible{ + outline:1px solid color-mix(in srgb,var(--accent) 60%,transparent); + box-shadow:0 0 0 2px var(--accent-glow); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/static/style.css` around lines 97 - 109, Add explicit keyboard focus-visible styles for the interactive controls .update-chip and .update-chip-dismiss by defining :focus-visible rules that match the rest of the theme (use the existing theme tokens such as --accent, --focus, --focus-ring, --r-sm or similar) to provide a visible outline/box-shadow and ensure outline: none is not removing visibility; ensure the styles include clear contrast (e.g., a 2px solid or a subtle ring via box-shadow) and preserve current border-radius and padding so keyboard focus looks consistent with other components.internal/server/server.go (1)
22-29: Inject updater dependency (or manifest URL) instead of hardcoding it inNewServer.Hard-wiring
DefaultManifestURLand constructing the concrete updater in the constructor makes testing and non-prod environments harder than necessary.♻️ Suggested refactor
type Server struct { mu sync.RWMutex mcDir string appVersion string @@ - updater *appupdate.Service + updater updaterChecker mux *http.ServeMux frontend fs.FS } + +type updaterChecker interface { + Check(currentVersion string) (appupdate.Result, error) +} -func NewServer(mcDir string, appVersion string, cfg *config.Config, instances *instance.InstanceStore, frontend fs.FS) *Server { +func NewServer(mcDir string, appVersion string, cfg *config.Config, instances *instance.InstanceStore, frontend fs.FS, updater updaterChecker) *Server { + if updater == nil { + updater = appupdate.NewService(appupdate.DefaultManifestURL, runtime.GOOS, runtime.GOARCH) + } s := &Server{ @@ - updater: appupdate.NewService(appupdate.DefaultManifestURL, runtime.GOOS, runtime.GOARCH), + updater: updater,Also applies to: 47-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/server.go` around lines 22 - 29, The constructor NewServer currently hardcodes DefaultManifestURL and constructs the concrete appupdate.Service inside, making tests brittle; change NewServer to accept an injected updater (appupdate.Service) or a manifestURL string as an additional parameter and set the server.updater field from that parameter instead of creating it inside NewServer; update all NewServer call sites and tests to pass a mock or alternate updater (or manifest URL) and remove any use of DefaultManifestURL within NewServer so the creation responsibility is moved to callers or test helpers.internal/server/api.go (1)
127-134: Consider short-TTL caching for update checks.This path does a remote lookup on each request; a small in-memory TTL cache would reduce latency and external dependency pressure while keeping update freshness acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/api.go` around lines 127 - 134, Add a short in-memory TTL cache to Server.handleUpdate so s.updater.Check(appVersion) is not called on every request; create a package-level or Server-scoped cached struct (e.g., type updateCache { mu sync.RWMutex; data *UpdateResult; ts time.Time }) and a TTL constant (e.g., 1m). In handleUpdate, first read-lock and return cached data via writeJSON if ts+TTL is valid; otherwise escalate to write-lock, call s.updater.Check(s.appVersion), update the cache fields (data and ts) and then writeJSON; on upstream error use writeError as currently done. Ensure all cache accesses use the mutex and cache key includes s.appVersion if multiple versions must be supported.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 140-142: Replace the mutable download+chmod+run sequence (the
curl, chmod, and ARCH=... APPIMAGE_EXTRACT_AND_RUN lines) with a reproducible
build step that clones AppImage/AppImageKit and runs its ci/build-in-docker.sh
to produce appimagetool, then invoke that built binary on dist/appimage/AppDir
to create dist/croopor-linux-amd64.AppImage; alternatively swap to using
linuxdeploy or appimage-builder instead of appimagetool. Specifically: remove
the curl + chmod + APPIMAGE_EXTRACT_AND_RUN invocation and add steps to git
clone https://github.com/AppImage/AppImageKit, checkout a pinned commit or tag,
run ci/build-in-docker.sh (or use the recommended tool), and use the resulting
appimagetool binary to package (the current packaging invocation is the code to
replace). Ensure you pin the AppImageKit commit/tag for reproducibility and
update workflow envs to point to the locally built appimagetool.
In `@frontend/src/updater.ts`:
- Around line 92-101: scheduleAutoUpdateCheck currently sets a one-shot timer
(autoCheckTimer) that calls checkForUpdates once and then stops; change it to a
recurring scheduler so it retries quickly if startup/install/launch are busy and
otherwise schedules the next daily check after a completed check. Implement
logic inside scheduleAutoUpdateCheck (or a helper used by it) to: clear any
existing autoCheckTimer, compute whether to set a short retry timeout (e.g.,
AUTO_CHECK_RETRY_MS) when bootstrapState.value !== 'ready' or
installState.value.status !== 'idle' or launchState.value.status !== 'idle',
otherwise call checkForUpdates({ silent: true }) and on its completion schedule
the next run after AUTO_CHECK_INTERVAL_MS; keep using autoCheckTimer to track
the pending window and ensure timers are nulled before/after firing so multiple
timers cannot be armed. Reference scheduleAutoUpdateCheck, autoCheckTimer,
checkForUpdates, AUTO_CHECK_DELAY_MS (replace/augment with retry/interval
constants) when making the changes.
- Around line 83-85: The timestamp is being updated in the finally block so any
failed poll still sets lastUpdateCheckAt; move the call to stampUpdateCheck()
out of the finally and only invoke it after a confirmed successful check (e.g.,
after a successful fetch/response handling path) while still clearing
pendingCheck = null in finally; update the code around stampUpdateCheck(),
pendingCheck and the function performing the poll so only successful completion
calls stampUpdateCheck().
In `@internal/update/update.go`:
- Around line 164-176: The parseStableVersion function accepts prefixes because
fmt.Sscanf tolerates trailing characters; change it to explicitly validate that
raw matches exactly three dot-separated numeric components:
normalizeVersion(raw), then split on '.' (or use a strict regexp) and convert
each part with strconv.Atoi into stableVersion.major/minor/patch, returning an
error if there are not exactly three numeric parts or any conversion fails (and
reject any trailing non-numeric characters); update imports to include strconv
and add regression tests for inputs like "1.2.3.4" and "1.2.3foo" to assert
parseStableVersion returns an error.
In `@packaging/windows/AppxManifest.xml.template`:
- Around line 2-6: The Package root element in the manifest is missing the
IgnorableNamespaces attribute which causes validation failures; update the
Package element (the declaration that currently includes xmlns, xmlns:uap,
xmlns:desktop and xmlns:rescap) to add IgnorableNamespaces listing the declared
extension prefixes (e.g., "uap desktop rescap") so the manifest consumer will
ignore those extension namespaces during validation.
---
Nitpick comments:
In `@frontend/src/main.tsx`:
- Line 96: The call to scheduleAutoUpdateCheck is currently inside the main
bootstrap try block and can throw, flipping the app into a global error state;
move or wrap the scheduleAutoUpdateCheck invocation in its own local try/catch
so update scheduling is best-effort: call scheduleAutoUpdateCheck() inside a
small try block, catch and log the error (using the same logger used during
bootstrap) without rethrowing, so failures in scheduleAutoUpdateCheck do not
affect the surrounding bootstrap flow or error state.
In `@frontend/static/style.css`:
- Around line 97-109: Add explicit keyboard focus-visible styles for the
interactive controls .update-chip and .update-chip-dismiss by defining
:focus-visible rules that match the rest of the theme (use the existing theme
tokens such as --accent, --focus, --focus-ring, --r-sm or similar) to provide a
visible outline/box-shadow and ensure outline: none is not removing visibility;
ensure the styles include clear contrast (e.g., a 2px solid or a subtle ring via
box-shadow) and preserve current border-radius and padding so keyboard focus
looks consistent with other components.
In `@internal/server/api.go`:
- Around line 127-134: Add a short in-memory TTL cache to Server.handleUpdate so
s.updater.Check(appVersion) is not called on every request; create a
package-level or Server-scoped cached struct (e.g., type updateCache { mu
sync.RWMutex; data *UpdateResult; ts time.Time }) and a TTL constant (e.g., 1m).
In handleUpdate, first read-lock and return cached data via writeJSON if ts+TTL
is valid; otherwise escalate to write-lock, call s.updater.Check(s.appVersion),
update the cache fields (data and ts) and then writeJSON; on upstream error use
writeError as currently done. Ensure all cache accesses use the mutex and cache
key includes s.appVersion if multiple versions must be supported.
In `@internal/server/server.go`:
- Around line 22-29: The constructor NewServer currently hardcodes
DefaultManifestURL and constructs the concrete appupdate.Service inside, making
tests brittle; change NewServer to accept an injected updater
(appupdate.Service) or a manifestURL string as an additional parameter and set
the server.updater field from that parameter instead of creating it inside
NewServer; update all NewServer call sites and tests to pass a mock or alternate
updater (or manifest URL) and remove any use of DefaultManifestURL within
NewServer so the creation responsibility is moved to callers or test helpers.
🪄 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: fb0d4862-d4ef-49a2-bca4-18fc0169e8e6
📒 Files selected for processing (21)
.github/workflows/release.yml.goreleaser.ymlREADME.mddocs/ARCHITECTURE.mddocs/CONVENTIONS.mdfrontend/src/components/App.tsxfrontend/src/components/SettingsView.tsxfrontend/src/main.tsxfrontend/src/state.tsfrontend/src/store.tsfrontend/src/types.tsfrontend/src/updater.tsfrontend/static/style.cssinternal/server/api.gointernal/server/server.gointernal/update/update.gointernal/update/update_test.gomain.gopackaging/linux/croopor.desktoppackaging/windows/AppxManifest.xml.templatepackaging/windows/croopor.appinstaller.template
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
internal/update/update.go (3)
51-59: Consider removing the client-level timeout.The HTTP client has a 2s timeout (line 53), but
fetchManifestalso creates a context with a 2s timeout (line 105). Since the context timeout is per-request and more precise, the client-level timeout is redundant.♻️ Suggested simplification
func NewService(manifestURL, platform, arch string) *Service { return &Service{ - client: &http.Client{Timeout: 2 * time.Second}, + client: &http.Client{}, manifestURL: manifestURL, platform: platform, arch: arch, now: time.Now, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update.go` around lines 51 - 59, The Service constructor NewService sets an http.Client with a 2s Timeout which is redundant because fetchManifest already uses a per-request context timeout; remove the client-level timeout (i.e., stop constructing client as &http.Client{Timeout: 2 * time.Second} and instead use &http.Client{} or nil timeout) so the per-request context in fetchManifest controls request deadlines and avoids double timeouts.
148-154: Consider returning "none" when AppImageURL is empty for consistency.When
entry.AppImageURLis empty, this returnskind = "appimage"with an empty URL. The caller handles this correctly by checking for emptyactionURL, but it would be more consistent to return"none"here (similar to the Windows fallback pattern or the!okcase).♻️ Suggested improvement
case "linux": entry, ok := manifest.Linux[arch] if !ok { return "none", "", "" } actionURL := strings.TrimSpace(entry.AppImageURL) + if actionURL == "" { + return "none", "", "" + } return "appimage", actionURL, "Download AppImage"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update.go` around lines 148 - 154, In the "linux" case handling (see manifest.Linux and entry.AppImageURL), ensure you treat an empty or whitespace-only AppImageURL as no action: trim entry.AppImageURL and if it's empty return "none", "", "" instead of returning kind "appimage" with an empty URL; otherwise return "appimage", the trimmed URL, "Download AppImage".
123-126: Limit response body size to prevent potential memory exhaustion.The JSON decoder reads directly from the unbounded response body. While the manifest URL points to a controlled GitHub Pages endpoint, a compromised CDN or MITM attack could serve an arbitrarily large response. Consider limiting to a reasonable size (e.g., 1MB is generous for a manifest).
🛡️ Suggested fix using io.LimitReader
Add
"io"to imports, then:- var manifest Manifest - if err := json.NewDecoder(resp.Body).Decode(&manifest); err != nil { + const maxManifestSize = 1 << 20 // 1 MB + var manifest Manifest + if err := json.NewDecoder(io.LimitReader(resp.Body, maxManifestSize)).Decode(&manifest); err != nil { return nil, fmt.Errorf("decode update manifest: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update.go` around lines 123 - 126, The JSON decoder currently reads directly from resp.Body (json.NewDecoder(resp.Body)), which can be unbounded; change it to decode from a size-limited reader using io.LimitReader (e.g., io.LimitReader(resp.Body, 1<<20)) and add the "io" import; optionally define a constant like maxManifestSize = 1<<20 and use json.NewDecoder(io.LimitReader(resp.Body, maxManifestSize)) when decoding the manifest variable to prevent memory exhaustion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yml:
- Around line 207-235: The release step currently writes and uploads a
non-arch-specific asset name ($appInstallerPath -> croopor.appinstaller) causing
name collisions between matrix jobs; change the generated asset name to include
the architecture (e.g., set $appInstallerPath to Join-Path $dist
"croopor-windows-${goarch}.appinstaller" and update the $appInstaller creation
so it writes that file), and update the upload step file entry to
dist/windows-${{ matrix.goarch }}/croopor-windows-${{ matrix.goarch
}}.appinstaller so each matrix job uploads a unique asset (alternatively
serialize uploads or use overwrite_files: true if you intentionally want a
single asset).
- Around line 219-246: The workflow currently creates a new self-signed cert
with New-SelfSignedCertificate (and Export-Certificate) each run which breaks
MSIX trust and update continuity; replace that block to load a persistent PFX
from secrets (e.g., decode GITHUB_SECRET_SIGNING_PFX to a file and use its
password from GITHUB_SECRET_SIGNING_PFX_PASSWORD) and call signtool.exe with the
/f <pfxPath> (and /p <password> and /fd SHA256) instead of using $cert. Keep
makeappx usage for packaging, remove per-run certificate export ($cerPath) and
the upload of a transient cert artifact (windows-cert-*) so the workflow uses
the long-lived PFX across releases and preserves update trust.
In `@frontend/src/updater.ts`:
- Around line 117-135: Both early-return branches stop re-arming the auto-check
timer when shouldAutoCheck() is false; ensure a replacement timer is scheduled
instead. In scheduleAutoUpdateCheck() and at the start of runAutoUpdateCheck(),
replace the immediate returns when !shouldAutoCheck() with calls to
queueAutoUpdateCheck(AUTO_CHECK_INTERVAL_MS) (or call it first, then return) so
a future re-evaluation is queued; keep existing logic that clears/sets
autoCheckTimer and avoid creating duplicate timers by preserving the
autoCheckTimer check in scheduleAutoUpdateCheck().
In `@internal/server/api.go`:
- Around line 135-158: The manual "Check for updates" route currently always
returns the warm cache; modify the handler to accept a cache-bypass flag (e.g.
query param "force" or "bypass") and when that flag is present skip both
readCachedUpdate() and the s.updateCache TTL short-circuit so the handler always
calls s.updater.Check(s.appVersion). After a successful forced check still
update s.updateCache (updateCacheEntry fields) and return the fresh result; keep
existing locking via s.updateCacheMu and error handling with writeError. Use the
presence/value of r.URL.Query().Get("force") (or similar) to implement the
bypass logic around the cached branches.
---
Nitpick comments:
In `@internal/update/update.go`:
- Around line 51-59: The Service constructor NewService sets an http.Client with
a 2s Timeout which is redundant because fetchManifest already uses a per-request
context timeout; remove the client-level timeout (i.e., stop constructing client
as &http.Client{Timeout: 2 * time.Second} and instead use &http.Client{} or nil
timeout) so the per-request context in fetchManifest controls request deadlines
and avoids double timeouts.
- Around line 148-154: In the "linux" case handling (see manifest.Linux and
entry.AppImageURL), ensure you treat an empty or whitespace-only AppImageURL as
no action: trim entry.AppImageURL and if it's empty return "none", "", ""
instead of returning kind "appimage" with an empty URL; otherwise return
"appimage", the trimmed URL, "Download AppImage".
- Around line 123-126: The JSON decoder currently reads directly from resp.Body
(json.NewDecoder(resp.Body)), which can be unbounded; change it to decode from a
size-limited reader using io.LimitReader (e.g., io.LimitReader(resp.Body,
1<<20)) and add the "io" import; optionally define a constant like
maxManifestSize = 1<<20 and use json.NewDecoder(io.LimitReader(resp.Body,
maxManifestSize)) when decoding the manifest variable to prevent memory
exhaustion.
🪄 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: 32c273ac-8a76-4170-b825-4e9f53f8637c
📒 Files selected for processing (21)
.github/workflows/release.yml.goreleaser.ymlREADME.mddocs/ARCHITECTURE.mddocs/CONVENTIONS.mdfrontend/src/components/App.tsxfrontend/src/components/SettingsView.tsxfrontend/src/main.tsxfrontend/src/state.tsfrontend/src/store.tsfrontend/src/types.tsfrontend/src/updater.tsfrontend/static/style.cssinternal/server/api.gointernal/server/server.gointernal/update/update.gointernal/update/update_test.gomain.gopackaging/linux/croopor.desktoppackaging/windows/AppxManifest.xml.templatepackaging/windows/croopor.appinstaller.template
✅ Files skipped from review due to trivial changes (9)
- docs/CONVENTIONS.md
- docs/ARCHITECTURE.md
- README.md
- packaging/linux/croopor.desktop
- frontend/src/state.ts
- .goreleaser.yml
- packaging/windows/croopor.appinstaller.template
- frontend/src/store.ts
- packaging/windows/AppxManifest.xml.template
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/main.tsx
- internal/server/server.go
- frontend/src/types.ts
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
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.
adds the first real updater slice.
notes:
Summary by CodeRabbit
New Features
Packaging
Documentation