WithVelopackUpdates(repoUrl) enables it: RunApp runs the Velopack
startup hook, a background check downloads newer releases after launch,
and IUpdateService lets the UI apply on restart. Feeds from Gitea or
Forgejo releases, or GitHub when the URL points there. Lifted from the
implementation proven in CheapShotcutRandomizer.

forgejo-actions commented

2026-08-20 21:33:20 +00:00

🤖 Claude AI Code Review

Code Review: Add Velopack auto-update support

1. Summary

This PR integrates Velopack for auto-updates. It adds:

A WithVelopackUpdates(repoUrl) builder method
An IUpdateService / UpdateService pair that checks, downloads, and applies updates
Velopack startup hook wiring in RunApp()
A background check kicked off from BlazorHostWindow
Documentation and TODO updates

The design is clean, the docs are honest about the security tradeoffs, and the abstraction (IUpdateService) is well-scoped. Nicely done overall.

2. Code Quality

Strengths:

IUpdateService is minimal and well-documented, including thread-marshalling warnings on StateChanged.
Re-entrancy guard via Interlocked.CompareExchange on _checkInProgress is correct.
Host-based GitHub detection (vs. substring) is a thoughtful touch and the comment explains why.
Best-effort error handling with logging is appropriate for background updates.
Docs candidly flag the attack-surface implications — excellent.

Minor:

UpdateReady is a plain auto-property written inside the lock but read without one — see below.
CreateSource calls new Uri(repoUrl) without validation, but this is guarded by WithVelopackUpdates's Uri.TryCreate, so it's fine in practice.

3. Potential Issues

🔴 `UpdateReady` read/write memory visibility

UpdateReady is written inside _stateLock but read outside it (e.g., from Blazor circuits). The plain auto-property has no memory barrier on the read path, so a reader thread may observe a stale value indefinitely.

public bool UpdateReady { get; private set; }  // read without lock elsewhere

Consider volatile backing field or reading under the lock:

private volatile bool _updateReady;
public bool UpdateReady => _updateReady;

Same concern applies to PendingVersion => _pendingUpdate?.TargetFullRelease?.Version?.ToString() — it reads _pendingUpdate outside the lock. The comment claims "guard the trio so a reader never sees a half-published update," but the readers don't actually take the lock, defeating the intent.

🟡 Fire-and-forget on `Task.Run` swallows nothing but relies on service contract

_ = Task.Run(updateService.CheckAndDownloadAsync);

This is safe only because CheckAndDownloadAsync guarantees it never throws. That contract is documented, and the implementation wraps everything in try/catch — good. But Interlocked reset uses Volatile.Write while the finally is reached; if an exception escaped before the try (it can't here), the guard would stick. Acceptable as-is.

🟡 `VelopackApp.Build().Run()` placement

Velopack docs recommend running the hook as the very first thing in Main, before any other framework initialization. Here it runs inside RunApp(), which may execute after other setup depending on the consumer's Main. For install/update/uninstall hooks that exit early this is usually fine, but document that consumers shouldn't do meaningful work before RunApp(), or expose the hook separately. Worth confirming against Velopack's guidance.

🟡 `GiteaSource` / `GithubSource` created with `null` token

Documented as a known limitation (no private feeds). Fine, but note that GithubSource(repoUrl, null, false) — the third arg prerelease: false — means prereleases are silently excluded. This is a reasonable default but isn't surfaced in the API or docs.

🟢 Duplicate `UpdateManager` construction

CheckAndDownloadAsync creates a fresh UpdateManager each call. Minor allocation; not a problem given check frequency.

4. Recommendations

Fix the memory visibility of UpdateReady/_pendingUpdate reads (volatile or lock the getters). This is the one item I'd want addressed before merge.
Clarify RunApp() ordering in docs — tell consumers not to run logic before it when updates are enabled, or verify Velopack tolerates the current placement.
Consider exposing prerelease support (or at least documenting the current false).
Optionally expose the last check error/state for diagnostics; currently failures are debug-logged and invisible to the UI, which is fine but limits troubleshooting.
Consider whether CheckAndDownloadAsync should surface a "check failed" signal so UIs can distinguish "no update" from "couldn't reach feed" — not required, just a UX nicety.

5. Verdict

COMMENT (leaning APPROVE)

This is a solid, well-documented feature with good security awareness. The only substantive concern is the memory-visibility bug on UpdateReady/_pendingUpdate, where the stated locking intent isn't fully realized on the read path. Address that (a one-line volatile change) and this is a clean approve.

📊 Tokens: 6438 input + 1796 output | 💰 Cost: ~$0.0771
Generated by Claude Opus 4.8 via Gitea Actions

## 🤖 Claude AI Code Review # Code Review: Add Velopack auto-update support ## 1. Summary This PR integrates [Velopack](https://velopack.io/) for auto-updates. It adds: - A `WithVelopackUpdates(repoUrl)` builder method - An `IUpdateService` / `UpdateService` pair that checks, downloads, and applies updates - Velopack startup hook wiring in `RunApp()` - A background check kicked off from `BlazorHostWindow` - Documentation and TODO updates The design is clean, the docs are honest about the security tradeoffs, and the abstraction (`IUpdateService`) is well-scoped. Nicely done overall. ## 2. Code Quality **Strengths:** - `IUpdateService` is minimal and well-documented, including thread-marshalling warnings on `StateChanged`. - Re-entrancy guard via `Interlocked.CompareExchange` on `_checkInProgress` is correct. - Host-based GitHub detection (vs. substring) is a thoughtful touch and the comment explains *why*. - Best-effort error handling with logging is appropriate for background updates. - Docs candidly flag the attack-surface implications — excellent. **Minor:** - `UpdateReady` is a plain auto-property written inside the lock but read without one — see below. - `CreateSource` calls `new Uri(repoUrl)` without validation, but this is guarded by `WithVelopackUpdates`'s `Uri.TryCreate`, so it's fine in practice. ## 3. Potential Issues ### 🔴 `UpdateReady` read/write memory visibility `UpdateReady` is written inside `_stateLock` but read outside it (e.g., from Blazor circuits). The plain auto-property has no memory barrier on the read path, so a reader thread may observe a stale value indefinitely. ```csharp public bool UpdateReady { get; private set; } // read without lock elsewhere ``` Consider `volatile` backing field or reading under the lock: ```csharp private volatile bool _updateReady; public bool UpdateReady => _updateReady; ``` Same concern applies to `PendingVersion => _pendingUpdate?.TargetFullRelease?.Version?.ToString()` — it reads `_pendingUpdate` outside the lock. The comment claims "guard the trio so a reader never sees a half-published update," but the readers don't actually take the lock, defeating the intent. ### 🟡 Fire-and-forget on `Task.Run` swallows nothing but relies on service contract ```csharp _ = Task.Run(updateService.CheckAndDownloadAsync); ``` This is safe *only because* `CheckAndDownloadAsync` guarantees it never throws. That contract is documented, and the implementation wraps everything in try/catch — good. But `Interlocked` reset uses `Volatile.Write` while the finally is reached; if an exception escaped before the try (it can't here), the guard would stick. Acceptable as-is. ### 🟡 `VelopackApp.Build().Run()` placement Velopack docs recommend running the hook as *the very first thing* in `Main`, before any other framework initialization. Here it runs inside `RunApp()`, which may execute after other setup depending on the consumer's `Main`. For install/update/uninstall hooks that exit early this is usually fine, but document that consumers shouldn't do meaningful work before `RunApp()`, or expose the hook separately. Worth confirming against Velopack's guidance. ### 🟡 `GiteaSource` / `GithubSource` created with `null` token Documented as a known limitation (no private feeds). Fine, but note that `GithubSource(repoUrl, null, false)` — the third arg `prerelease: false` — means prereleases are silently excluded. This is a reasonable default but isn't surfaced in the API or docs. ### 🟢 Duplicate `UpdateManager` construction `CheckAndDownloadAsync` creates a fresh `UpdateManager` each call. Minor allocation; not a problem given check frequency. ## 4. Recommendations 1. **Fix the memory visibility** of `UpdateReady`/`_pendingUpdate` reads (volatile or lock the getters). This is the one item I'd want addressed before merge. 2. **Clarify `RunApp()` ordering** in docs — tell consumers not to run logic before it when updates are enabled, or verify Velopack tolerates the current placement. 3. Consider exposing `prerelease` support (or at least documenting the current `false`). 4. Optionally expose the last check error/state for diagnostics; currently failures are debug-logged and invisible to the UI, which is fine but limits troubleshooting. 5. Consider whether `CheckAndDownloadAsync` should surface a "check failed" signal so UIs can distinguish "no update" from "couldn't reach feed" — not required, just a UX nicety. ## 5. Verdict **COMMENT** (leaning APPROVE) This is a solid, well-documented feature with good security awareness. The only substantive concern is the **memory-visibility bug on `UpdateReady`/`_pendingUpdate`**, where the stated locking intent isn't fully realized on the read path. Address that (a one-line `volatile` change) and this is a clean approve. --- 📊 **Tokens**: 6438 input + 1796 output | 💰 **Cost**: ~$0.0771 *Generated by Claude Opus 4.8 via Gitea Actions*

cheapnud added 1 commit

2026-08-20 21:36:44 +00:00

Harden the update service after review

Claude PR Review / AI Code Review (pull_request) Successful in 35s

Details

Build & Test / Build and Test (pull_request) Successful in 2m21s

Details

bd4f17f7ff

Guard the cross-thread state with a lock, add a re-entrancy gate to the
background check, detect GitHub by URL host instead of substring,
validate the repo URL up front, and document the trust model.

cheapnud merged commit c5ec3b2f5a into master

2026-08-20 21:39:17 +00:00

cheapnud deleted branch f/velopack-updates

2026-08-20 21:39:17 +00:00

cheapnud referenced this pull request from a commit

2026-08-20 21:39:19 +00:00

Merge pull request 'Add Velopack auto-update support' (#1) from f/velopack-updates into master

No reviewers

No labels

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

cheapnud/CheapAvaloniaBlazor!1

No description provided.

Rows
Columns

Add Velopack auto-update support #1