feat: Sub2API-style in-place binary update (check / apply / rollback) - #6568
feat: Sub2API-style in-place binary update (check / apply / rollback)#6568neboyang wants to merge 5 commits into
Conversation
Add backend system-update APIs (check/apply/rollback) that download official GitHub release binaries, verify checksums, and atomically replace the running executable. Docker deployments are detected and guided to image pull instead. The admin maintenance UI now supports one-click update and rollback, with a GitHub fallback for check-only.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
WalkthroughAdds a system update service with GitHub release checks, validated binary installation, rollback support, authenticated API routes, and maintenance UI controls. ChangesSystem update and rollback
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Administrator
participant MaintenanceUI
participant API
participant SystemUpdateService
participant GitHub
participant LocalBinary
Administrator->>MaintenanceUI: Open maintenance update section
MaintenanceUI->>API: Check update
API->>SystemUpdateService: CheckUpdate
SystemUpdateService->>GitHub: Request release metadata
GitHub-->>SystemUpdateService: Return release and asset metadata
SystemUpdateService-->>API: Return update information
API-->>MaintenanceUI: Return normalized update data
Administrator->>MaintenanceUI: Select apply or rollback
MaintenanceUI->>API: Submit operation
API->>SystemUpdateService: Perform update or rollback
SystemUpdateService->>LocalBinary: Replace or restore executable
LocalBinary-->>SystemUpdateService: Return operation result
SystemUpdateService-->>API: Return status
API-->>MaintenanceUI: Return result and restart metadata
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
systemupdate/update_test.go (2)
122-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the hand-rolled helpers with the
stringspackage.
containsandhasSuffixduplicatestrings.Containsandstrings.HasSuffix. The nested closure incontainsalso adds cognitive cost for no benefit.As per coding guidelines: "Minimize nested function definitions and avoid package-level helpers with only one caller".
♻️ Proposed fix
-func contains(s, sub string) bool { - return len(s) >= len(sub) && (s == sub || len(sub) == 0 || - (func() bool { - for i := 0; i+len(sub) <= len(s); i++ { - if s[i:i+len(sub)] == sub { - return true - } - } - return false - })()) -} - -func hasSuffix(s, suffix string) bool { - return len(s) >= len(suffix) && s[len(s)-len(suffix):] == suffix -}Then call
strings.Containsandstrings.HasSuffixat the assertion sites.🤖 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 `@systemupdate/update_test.go` around lines 122 - 136, Remove the package-level contains and hasSuffix helpers in update_test.go, import the standard strings package, and replace their assertion-site usages with strings.Contains and strings.HasSuffix directly.Source: Coding guidelines
115-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the tautological deploy-mode test.
TestDetectDeployModeaccepts every value thatDetectDeployModecan return, so it cannot fail on a behavior change. Extract the detection inputs, for example the marker file path and the cgroup path, and assert each branch with explicit fixtures.As per coding guidelines: "avoid coverage-only, fake stress, timing, or implementation-detail tests".
🤖 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 `@systemupdate/update_test.go` around lines 115 - 120, Replace TestDetectDeployMode’s tautological allowed-values check with table-driven tests using explicit fixtures for each detection branch. Extract or inject the marker-file and cgroup-path inputs used by DetectDeployMode, then assert the expected binary, docker, and unknown results without relying on host state or implementation-detail coverage.Source: Coding guidelines
systemupdate/update.go (2)
276-294: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReuse one release list for validation and asset lookup.
ListRollbackVersionsalready callsfetchRecent, and line 291 calls it again with the same page size. Every rollback request therefore costs two GitHub API calls, and the two responses can differ. Fetch the releases once, then derive both the allow list and the target assets from that slice.🤖 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 `@systemupdate/update.go` around lines 276 - 294, Reuse the release slice returned by fetchRecent for both rollback-version validation and target asset lookup instead of calling ListRollbackVersions followed by another fetchRecent. Update the surrounding rollback flow to derive the allowed versions from that single result while preserving ErrRollbackVersionNotAllowed and existing error propagation behavior.
534-534: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the
common/json.godecoder wrapper for the GitHub release body.
systemupdate/update.go:534still usesencoding/jsondirectly. Replace it withcommon.DecodeJson(resLimitReader(resp), dest)so JSON decoding follows the required wrapper rule.🤖 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 `@systemupdate/update.go` at line 534, Update the response decoding at the GitHub release body handling point to call common.DecodeJson with resLimitReader(resp) and dest instead of using json.NewDecoder directly, preserving the existing return behavior.Sources: Coding guidelines, Learnings
router/api-router.go (1)
295-304: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd rate limiting and cache control to the update routes.
/applyand/rollbackstart a long download and replace the running binary. Neighbouring privileged routes attachmiddleware.CriticalRateLimit(), and state-changing admin routes attachmiddleware.DisableCache(). Apply the same middleware here so repeated clicks cannot queue several 15-minute operations, and so proxies do not cache the check response.🛡️ Proposed change
systemUpdateRoute := apiRouter.Group("/system-update") - systemUpdateRoute.Use(middleware.RootAuth()) + systemUpdateRoute.Use(middleware.RootAuth(), middleware.DisableCache()) { systemUpdateRoute.GET("/check", controller.CheckSystemUpdate) - systemUpdateRoute.POST("/apply", controller.PerformSystemUpdate) + systemUpdateRoute.POST("/apply", middleware.CriticalRateLimit(), controller.PerformSystemUpdate) systemUpdateRoute.GET("/rollback-versions", controller.ListSystemRollbackVersions) - systemUpdateRoute.POST("/rollback", controller.PerformSystemRollback) + systemUpdateRoute.POST("/rollback", middleware.CriticalRateLimit(), controller.PerformSystemRollback) }🤖 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 `@router/api-router.go` around lines 295 - 304, Update the systemUpdateRoute middleware configuration around controller.CheckSystemUpdate, PerformSystemUpdate, and PerformSystemRollback: apply middleware.CriticalRateLimit() to the long-running /apply and /rollback operations, and middleware.DisableCache() to the state-changing update routes, including /check to prevent cached responses. Preserve RootAuth and existing route paths.
🤖 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 `@controller/system_update.go`:
- Around line 96-105: Update the request parsing in the system update handler
around the local req struct to call ShouldBindJSON unconditionally instead of
checking Request.ContentLength; accept io.EOF as an empty request, while
returning the existing invalid-request error for other binding failures.
Preserve the subsequent trimmed req.Version handling.
- Around line 16-19: Move the systemupdate.CurrentVersion assignment out of the
package init path and set it only after common.InitEnv() has finalized
common.Version, preserving the actual VERSION value for updater responses. Use
the startup initialization flow or lazy assignment in GetSystemUpdateService,
and remove the premature init-time assignment.
In `@systemupdate/update_test.go`:
- Around line 8-28: Rewrite TestNormalizeAndCompareVersions as a deterministic
table-driven test with explicit version inputs and expected comparison results,
covering normalization, stable releases, and prerelease ordering. Use
testify/assert for non-fatal table-case checks and testify/require for any fatal
setup or subtest requirements, importing the appropriate testify packages;
ensure the table includes the prerelease numeric-order case and exposes the
current compareVersions failure noted in update.go.
In `@systemupdate/update.go`:
- Around line 699-712: Update the prerelease comparison logic in the version
comparison function containing aPre and bPre to split each prerelease on "." and
compare corresponding numeric identifiers as numbers, so values such as rc.9
sort below rc.10. Preserve the existing empty-prerelease precedence and semver
ordering for identifiers that are not numeric.
- Around line 190-205: Add a service-level mutex and non-blocking beginOp/endOp
guard to PerformUpdate, Rollback, and RollbackToVersion, covering each entire
apply or rollback sequence and returning ErrUpdateInProgress when another
operation is active. Ensure all exit paths release the guard, then map
ErrUpdateInProgress to HTTP 409 in the system update controller.
- Around line 404-409: Update the fallback error handling around os.Rename and
copyFile so the fmt.Errorf call wraps copyErr, the actual failure from copyFile,
instead of err from the rename attempt.
- Around line 397-401: Update the update flow around checksumURL and
selectReleaseAsset so an empty checksumURL returns an error before installing or
accepting the downloaded binary. Preserve verifyChecksum for non-empty URLs, and
only allow a missing-checksum path if it is explicitly gated by a documented
opt-in environment variable.
- Around line 555-567: Update the file-writing flow around os.Create and io.Copy
to flush and close the output explicitly before returning success, propagating
any Close error so callers do not verify an incomplete file. Remove the deferred
close and preserve the existing copy and size-limit errors.
- Around line 462-473: Restrict the fuzzy fallback in the asset-selection flow
to candidates whose normalized name contains the requested version, while
preserving the existing platform and checksum exclusions. Ensure this applies
across all platforms and architectures so assetMatchesPlatform cannot select a
different release; alternatively remove this fallback and rely exclusively on
candidateAssetNames.
In `@web/src/features/system-settings/maintenance/update-checker-section.tsx`:
- Around line 229-236: Update the rollback handler’s catch block to pass the
caught error through handleServerError instead of directly selecting
error.message or the generic rollback fallback, so backend 400 response messages
are surfaced consistently with the apply handler. Preserve the existing toast
display and finally block that resets rolling-back state.
- Around line 204-211: Update the error handling in the apply-update function
around the catch block to pass the caught error to the shared handleServerError
helper instead of deriving a local Error.message and calling toast.error
directly. Preserve the existing setApplying(false) cleanup in finally and ensure
the helper surfaces the backend response message.
- Around line 126-141: Update the fallback fetch in the update-checker request
flow to build the GitHub releases URL from a frontend configuration/environment
variable containing the repository identifier, rather than hardcoding
“Calcium-Ion/new-api”. Preserve the existing latest-release endpoint and request
behavior, and use the project’s established frontend config access pattern.
---
Nitpick comments:
In `@router/api-router.go`:
- Around line 295-304: Update the systemUpdateRoute middleware configuration
around controller.CheckSystemUpdate, PerformSystemUpdate, and
PerformSystemRollback: apply middleware.CriticalRateLimit() to the long-running
/apply and /rollback operations, and middleware.DisableCache() to the
state-changing update routes, including /check to prevent cached responses.
Preserve RootAuth and existing route paths.
In `@systemupdate/update_test.go`:
- Around line 122-136: Remove the package-level contains and hasSuffix helpers
in update_test.go, import the standard strings package, and replace their
assertion-site usages with strings.Contains and strings.HasSuffix directly.
- Around line 115-120: Replace TestDetectDeployMode’s tautological
allowed-values check with table-driven tests using explicit fixtures for each
detection branch. Extract or inject the marker-file and cgroup-path inputs used
by DetectDeployMode, then assert the expected binary, docker, and unknown
results without relying on host state or implementation-detail coverage.
In `@systemupdate/update.go`:
- Around line 276-294: Reuse the release slice returned by fetchRecent for both
rollback-version validation and target asset lookup instead of calling
ListRollbackVersions followed by another fetchRecent. Update the surrounding
rollback flow to derive the allowed versions from that single result while
preserving ErrRollbackVersionNotAllowed and existing error propagation behavior.
- Line 534: Update the response decoding at the GitHub release body handling
point to call common.DecodeJson with resLimitReader(resp) and dest instead of
using json.NewDecoder directly, preserving the existing return behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 761f50bf-4bac-4988-9f75-c988f8353d09
📒 Files selected for processing (5)
controller/system_update.gorouter/api-router.gosystemupdate/update.gosystemupdate/update_test.goweb/src/features/system-settings/maintenance/update-checker-section.tsx
| func (s *SystemUpdateService) PerformUpdate(ctx context.Context) error { | ||
| if DetectDeployMode() != "binary" { | ||
| return fmt.Errorf("%w: deploy_mode=%s (use docker compose pull/up for containers)", ErrUpdateNotSupportedEnv, DetectDeployMode()) | ||
| } | ||
| info, err := s.CheckUpdate(ctx, true) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !info.HasUpdate { | ||
| return ErrNoUpdateAvailable | ||
| } | ||
| if info.ReleaseInfo == nil { | ||
| return errors.New("release info missing") | ||
| } | ||
| return s.applyAssets(ctx, info.ReleaseInfo.Assets, info.LatestVersion) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Serialize update and rollback operations with a mutex.
PerformUpdate, Rollback and RollbackToVersion rename exePath, exePath+".backup" and exePath+".failed" without any lock. Two concurrent requests can interleave the renames. One sequence removes the backup and moves the live binary away, which leaves no executable on disk. The HTTP layer does not prevent concurrent calls.
Add a service-level mutex and take it for the whole apply or rollback sequence. Reject the second call instead of waiting, so the caller receives a clear "update already running" error.
🔒 Proposed guard
type SystemUpdateService struct {
repo string
token string
apiClient *http.Client
dlClient *http.Client
cacheMu sync.Mutex
cache *updateCacheEntry
currentVer string
+ opRunning atomic.Bool
}
+
+var ErrUpdateInProgress = errors.New("another update or rollback is in progress")
+
+func (s *SystemUpdateService) beginOp() error {
+ if !s.opRunning.CompareAndSwap(false, true) {
+ return ErrUpdateInProgress
+ }
+ return nil
+}
+
+func (s *SystemUpdateService) endOp() { s.opRunning.Store(false) }Call beginOp/endOp at the start of PerformUpdate, Rollback and RollbackToVersion, and map ErrUpdateInProgress to HTTP 409 in controller/system_update.go.
Also applies to: 208-232
🤖 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 `@systemupdate/update.go` around lines 190 - 205, Add a service-level mutex and
non-blocking beginOp/endOp guard to PerformUpdate, Rollback, and
RollbackToVersion, covering each entire apply or rollback sequence and returning
ErrUpdateInProgress when another operation is active. Ensure all exit paths
release the guard, then map ErrUpdateInProgress to HTTP 409 in the system update
controller.
| try { | ||
| const response = await fetch( | ||
| 'https://api.github.com/repos/Calcium-Ion/new-api/releases/latest', | ||
| { | ||
| headers: { | ||
| Accept: 'application/vnd.github+json', | ||
| 'User-Agent': 'new-api-dashboard', | ||
| }, | ||
| } | ||
| ) | ||
| if (!response.ok) { | ||
| throw new Error(t('Failed to contact GitHub releases API')) | ||
| } | ||
| const release = (await response.json()) as ReleaseInfo & { | ||
| tag_name?: string | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Is User-Agent a forbidden header name for the browser Fetch API?
💡 Result:
No, User-Agent is not a forbidden header name for the browser Fetch API [1][2]. Although it was previously restricted, it is no longer listed as a forbidden request header in the Fetch specification [3][4][5]. However, even though the header is not technically forbidden by the specification, browsers may still impose their own restrictions. For instance, Chrome is known to silently ignore or drop the User-Agent header when it is set in a Fetch request [1][2]. Therefore, while you can programmatically attempt to set it without triggering a forbidden header error, you cannot rely on the browser to send it as specified [1][2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_request_header
- 2: https://github.com/mdn/content/blob/main/files/en-us/glossary/forbidden_request_header/index.md
- 3: https://fetch.spec.whatwg.org/review-drafts/2024-12/
- 4: https://fetch.spec.whatwg.org/
- 5: https://github.com/whatwg/fetch/blob/09e95249f3669a335ca4551c7702888c46f7cce4/fetch.bs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate target file and relevant references"
fd -a 'update-checker-section\.tsx$|system-settings|new-api|\NEW_API_UPDATE_GITHUB_REPO|UPDATE_CHECK' . | sed 's#^\./##' | head -100
target="web/src/features/system-settings/maintenance/update-checker-section.tsx"
if [ -f "$target" ]; then
echo
echo "Outline"
ast-grep outline "$target" || true
echo
echo "Lines 1-190"
cat -n "$target" | sed -n '1,190p'
else
echo "target file not found"
fi
echo
echo "Search for NEW_API_UPDATE_GITHUB_REPO and hardcoded repo strings"
rg -n "NEW_API_UPDATE_GITHUB_REPO|Calcium-Ion/new-api|fetch\\(|User-Agent|Accept:" web || true
echo
echo "Check fetch User-Agent forbidden list via browser-provided data if available"
node - <<'JS'
let found = false
for (const key in headers && headers && headers.accept || {}) {} // no-op
try {
const req = new Request('http://localhost/');
for (const set of ['user-agent', 'User-Agent']) {
try {
req.headers.set(set, 'new-api-dashboard');
const val = req.headers.get(set);
console.log(`${set} set value: ${JSON.stringify(val)}`);
} catch (e) {
console.log(`${set} set error: ${e.name}: ${e.message}`);
}
}
found = true;
req.headers.delete('user-agent');
req.headers.delete('User-Agent');
} catch (e) {
console.log('Request API unavailable:', e.name, e.message);
}
console.log('Runtime Request headers probe completed:', found);
JSRepository: QuantumNous/new-api
Length of output: 617
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Target file"
target="web/src/features/system-settings/maintenance/update-checker-section.tsx"
if [ -f "$target" ]; then
echo "$target exists"
echo
echo "Outline"
ast-grep outline "$target" || true
echo
echo "Lines 1-220"
cat -n "$target" | sed -n '1,220p'
else
echo "$target not found"
fi
echo
echo "Search for relevant source references"
rg -n "NEW_API_UPDATE_GITHUB_REPO|Calcium-Hon/new-api|Calcium-Ion/new-api|new-api/releases/latest|User-Agent|Accept:" web || true
echo
echo "Runtime Fetch Request User-Agent probe"
node - <<'JS'
try {
const req = new Request('http://localhost/');
try {
req.headers.set('User-Agent', 'new-api-dashboard');
console.log('set User-Agent ok; value=', JSON.stringify(req.headers.get('User-Agent')));
} catch (e) {
console.log('set User-Agent error=', e.name + ': ' + e.message);
}
} catch (e) {
console.log('Request API unavailable, error=', e.name + ': ' + e.message);
}
JSRepository: QuantumNous/new-api
Length of output: 10746
🌐 Web query:
MDN forbidden headers User-Agent fetch Request set forbidden request header
💡 Result:
The User-Agent header is not a forbidden request header according to the Fetch standard [1][2]. While it was historically restricted, it has been removed from the forbidden header list in the specification [1][2]. However, in practice, browser behavior may vary: 1. Chromium Behavior: Historically, Chromium browsers have silently dropped the User-Agent header when set via fetch [1][2]. As of 2026, there are active efforts within the Chromium project to allow JavaScript to successfully set the User-Agent header [3]. Note that setting this header via JavaScript will trigger a CORS preflight request, as it is not a CORS-safelisted header [3]. 2. General Constraints: A forbidden request header is generally defined as an HTTP header that cannot be programmatically set or modified because the user agent must retain full control over it (e.g., to ensure security and protocol integrity) [1][2]. If you attempt to set a header that the browser considers forbidden, the fetch API or the Headers object will typically ignore the instruction or throw an error depending on the implementation and context [1][4][5]. Always check the specific browser's developer console if you observe that a header is being dropped or ignored despite being set in your request options.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Glossary/Forbidden_request_header
- 2: https://github.com/mdn/content/blob/main/files/en-us/glossary/forbidden_request_header/index.md
- 3: https://chromestatus.com/feature/5094781984309248
- 4: https://developer.mozilla.org/en-US/docs/Web/API/RequestInit
- 5: https://developer.mozilla.org/en-US/docs/Web/API/Headers
Use a configurable GitHub repo in the fallback URL.
This fallback points at Calcium-Ion/new-api even after the backend API fails. Forks should not show upstream release notes. Move the repo value to a frontend config/env variable instead of hardcoding it in the URL.
🤖 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 `@web/src/features/system-settings/maintenance/update-checker-section.tsx`
around lines 126 - 141, Update the fallback fetch in the update-checker request
flow to build the GitHub releases URL from a frontend configuration/environment
variable containing the repository identifier, rather than hardcoding
“Calcium-Ion/new-api”. Preserve the existing latest-release endpoint and request
behavior, and use the project’s established frontend config access pattern.
Source: Coding guidelines
| } catch (error) { | ||
| const message = | ||
| error instanceof Error | ||
| ? error.message | ||
| : t('Failed to check for updates') | ||
| error instanceof Error ? error.message : t('Update failed') | ||
| toast.error(message) | ||
| } finally { | ||
| setChecking(false) | ||
| setApplying(false) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Surface the backend message through handleServerError.
Axios rejects with a generic message such as "Request failed with status code 400". The backend message from /api/system-update/apply, for example the Docker guidance text, never reaches the user. Route the error through the shared handleServerError helper.
As per coding guidelines: "服务端错误统一使用 handleServerError".
🤖 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 `@web/src/features/system-settings/maintenance/update-checker-section.tsx`
around lines 204 - 211, Update the error handling in the apply-update function
around the catch block to pass the caught error to the shared handleServerError
helper instead of deriving a local Error.message and calling toast.error
directly. Preserve the existing setApplying(false) cleanup in finally and ensure
the helper surfaces the backend response message.
Source: Coding guidelines
Serialize apply/rollback with a mutex, require checksum verification, restrict fuzzy asset matching by version+platform, bind optional JSON without ContentLength, refresh version after InitEnv, improve download error handling and prerelease ordering, surface backend errors in the UI, and switch tests to testify table cases.
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 (2)
web/src/features/system-settings/maintenance/update-checker-section.tsx (2)
205-206: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBlock apply while a rollback is active.
Line 360 enables
handleApplyUpdatewhilerollingBackis true. A user can select a rollback version and then queue an apply request. The backend serializes both operations, so the later apply can restore the latest binary after the rollback completes.Disable the button and return early when either operation is active.
Proposed fix
const handleApplyUpdate = async () => { + if (applying || rollingBack) return setApplying(true) try { @@ - disabled={applying} + disabled={applying || rollingBack}Also applies to: 356-364
🤖 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 `@web/src/features/system-settings/maintenance/update-checker-section.tsx` around lines 205 - 206, Update handleApplyUpdate to return immediately when applying or rollingBack is already true, before setting applying or queuing the request. Update the apply button’s disabled condition to include both operation states, preventing apply while either operation is active.
296-308: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHide decorative icons from assistive technology.
These icons are adjacent to equivalent button text. Set
aria-hidden="true"on each icon.As per coding guidelines: “装饰性图标使用
aria-hidden="true"”.Also applies to: 351-363
🤖 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 `@web/src/features/system-settings/maintenance/update-checker-section.tsx` around lines 296 - 308, Add aria-hidden="true" to the decorative RefreshCcwIcon and Undo2Icon elements in the update-check and rollback buttons, including the corresponding icons in the additionally referenced section. Keep the existing button labels and icon styling unchanged.Source: Coding guidelines
🧹 Nitpick comments (2)
systemupdate/update.go (1)
541-562: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the project JSON decoder wrapper for the GitHub API response.
systemupdate/update.go:560currently callsjson.NewDecoder(...).Decode(...)directly, butcommon/json.goalready exposescommon.DecodeJson. Use that wrapper before decode if this path can contain custom common decoder behavior.🤖 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 `@systemupdate/update.go` around lines 541 - 562, The getJSON method currently decodes GitHub responses with the standard json.Decoder; replace that direct decode with the project’s common.DecodeJson wrapper from common/json.go, preserving the existing response validation and error propagation.Source: Coding guidelines
web/src/features/system-settings/maintenance/update-checker-section.tsx (1)
112-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd explicit return types to the new callbacks.
Add
Promise<void>to the async callbacks andvoidtogoToRelease. This keeps the TypeScript contracts explicit.As per coding guidelines: “参数和返回值应显式标注类型”.
Also applies to: 124-124, 205-205, 238-238, 261-261
🤖 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 `@web/src/features/system-settings/maintenance/update-checker-section.tsx` at line 112, Update the new callbacks in the maintenance update-checker section to declare explicit return types: add Promise<void> to each async callback, including loadRollbackVersions, and void to goToRelease. Preserve their existing implementations and behavior.Source: Coding guidelines
🤖 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 `@systemupdate/update.go`:
- Around line 137-140: Synchronize the CurrentVersion refresh in the
process-wide defaultSystemUpdateService using the same lock that protects
currentVer reads, writes, and cached UpdateInfo access. When the trimmed version
differs from the stored currentVer, update it and invalidate the dependent
update cache so stale results are not reused; preserve the existing behavior
when the version is unchanged or empty.
- Around line 740-798: Update comparePreRelease and parseLeadingInt to implement
complete SemVer prerelease ordering: classify identifiers as numeric only when
every character is a digit, compare numeric strings without int conversion, and
order numeric identifiers before nonnumeric ones. Preserve lexical comparison
for nonnumeric identifiers and return the shorter prerelease as lower when all
shared identifiers match; add coverage for rc.1alpha vs rc.1beta, rc.1 vs
rc.1.0, and numeric-versus-nonnumeric identifiers.
---
Outside diff comments:
In `@web/src/features/system-settings/maintenance/update-checker-section.tsx`:
- Around line 205-206: Update handleApplyUpdate to return immediately when
applying or rollingBack is already true, before setting applying or queuing the
request. Update the apply button’s disabled condition to include both operation
states, preventing apply while either operation is active.
- Around line 296-308: Add aria-hidden="true" to the decorative RefreshCcwIcon
and Undo2Icon elements in the update-check and rollback buttons, including the
corresponding icons in the additionally referenced section. Keep the existing
button labels and icon styling unchanged.
---
Nitpick comments:
In `@systemupdate/update.go`:
- Around line 541-562: The getJSON method currently decodes GitHub responses
with the standard json.Decoder; replace that direct decode with the project’s
common.DecodeJson wrapper from common/json.go, preserving the existing response
validation and error propagation.
In `@web/src/features/system-settings/maintenance/update-checker-section.tsx`:
- Line 112: Update the new callbacks in the maintenance update-checker section
to declare explicit return types: add Promise<void> to each async callback,
including loadRollbackVersions, and void to goToRelease. Preserve their existing
implementations and behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7adabe28-aa5b-4c51-a1ad-e90e2364edc9
📒 Files selected for processing (4)
controller/system_update.gosystemupdate/update.gosystemupdate/update_test.goweb/src/features/system-settings/maintenance/update-checker-section.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- controller/system_update.go
- systemupdate/update_test.go
|
Pushed another fix commit addressing the second CodeRabbit review:
Note: kept
|
- Guard currentVer refresh with cacheMu and invalidate update cache - Use locked currentVersion() helper for all version reads - Implement full SemVer prerelease ordering (numeric vs non-numeric) - Block concurrent apply/rollback in the maintenance UI - Add aria-hidden on decorative icons and explicit callback return types
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@systemupdate/update.go`:
- Around line 575-576: Update the JSON response decoding in the surrounding
getJSON flow to use the applicable wrapper from common/json.go instead of
directly calling encoding/json.NewDecoder(...).Decode. Remove the rationale that
preserves the stdlib decoder, while keeping the existing response handling and
error behavior unchanged.
- Around line 137-156: Update SystemUpdateService’s cache coordination around
fetchLatest and setCache by adding a generation counter protected by cacheMu.
Increment the generation whenever syncUpdateVersion detects a running-version
change, capture the current generation before fetching, and only store the
fetched UpdateInfo if the generation is unchanged; otherwise refresh the result
or return it without caching, preventing an in-flight older-version check from
restoring stale data.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba62511c-6405-4bb5-96c9-f58f01966655
📒 Files selected for processing (3)
systemupdate/update.gosystemupdate/update_test.goweb/src/features/system-settings/maintenance/update-checker-section.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- systemupdate/update_test.go
- web/src/features/system-settings/maintenance/update-checker-section.tsx
| // Keep stdlib decoder: importing common/json pulls a large dependency graph | ||
| // into this package for no behavioral difference (DecodeJson is a thin wrap). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use the required JSON decode wrapper.
getJSON performs application-level response decoding. The direct json.NewDecoder(...).Decode call violates the repository requirement. Replace it with the applicable decode wrapper from common/json.go.
As per coding guidelines, “All JSON marshal and unmarshal operations in business code must use the wrappers in common/json.go; do not directly call encoding/json operations.”
🤖 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 `@systemupdate/update.go` around lines 575 - 576, Update the JSON response
decoding in the surrounding getJSON flow to use the applicable wrapper from
common/json.go instead of directly calling encoding/json.NewDecoder(...).Decode.
Remove the rationale that preserves the stdlib decoder, while keeping the
existing response handling and error behavior unchanged.
Source: Coding guidelines
There was a problem hiding this comment.
Pull request overview
Adds a Sub2API-style in-place binary update workflow to the new-api admin experience, providing root-only endpoints for update checking/apply/rollback and wiring the maintenance UI to these APIs (with a GitHub browser fallback when the backend API is unavailable).
Changes:
- Added
systemupdateservice to fetch GitHub release metadata, select platform assets, verify checksums, and perform apply/rollback operations. - Added root-only controller endpoints + routing for check/apply/rollback and rollback-version listing.
- Updated the system maintenance UI to support update check, apply, rollback, and rollback-version selection, plus Docker-specific guidance and API fallback behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/features/system-settings/maintenance/update-checker-section.tsx | Switches update check to backend API, adds apply/rollback UI and rollback version listing with GitHub fallback. |
| systemupdate/update.go | Implements core update logic: GitHub release fetch, asset selection, download+checksum verification, atomic replacement, and rollback. |
| systemupdate/update_test.go | Adds unit tests for version comparison, asset selection, URL validation, and deploy-mode detection. |
| router/api-router.go | Registers root-only /api/system-update/* routes for check/apply/rollback flows. |
| controller/system_update.go | Adds HTTP handlers for system update/rollback with long-running operation timeouts and structured responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Keep stdlib decoder: importing common/json pulls a large dependency graph | ||
| // into this package for no behavioral difference (DecodeJson is a thin wrap). | ||
| return json.NewDecoder(resp.Body).Decode(dest) | ||
| } |
| func copyFile(src, dst string) error { | ||
| in, err := os.Open(src) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer in.Close() | ||
| out, err := os.Create(dst) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer out.Close() | ||
| if _, err := io.Copy(out, in); err != nil { | ||
| return err | ||
| } | ||
| return out.Close() | ||
| } |
| func syncUpdateVersion() { | ||
| // Must run after common.InitEnv() so VERSION env / build tags are final. | ||
| systemupdate.CurrentVersion = common.Version | ||
| } |
| <Undo2Icon className='me-2 h-4 w-4' aria-hidden='true' /> | ||
| {rollingBack ? t('Rolling back...') : t('Rollback last backup')} | ||
| </Button> |
- Add cache generation so in-flight checks cannot restore stale cache - Make version updates concurrency-safe via SetCurrentVersion/atomic - Sync and single-close path in copyFile for durable binary staging - Add missing maintenance update strings to all locale files
Follow-up (third review: CodeRabbit + Copilot)Pushed another fix commit:
Ready for maintainer review when convenient. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@systemupdate/update.go`:
- Around line 137-149: Synchronize access to defaultSystemUpdateService between
SetCurrentVersion and the initialization performed by GetSystemUpdateService.
Replace the unsynchronized package-level read with a shared atomic pointer or
reuse a service returned by GetSystemUpdateService, ensuring SetCurrentVersion
observes the initialized service and still performs its existing cache
invalidation under cacheMu.
In `@web/src/i18n/locales/en.json`:
- Around line 2-10: Move all newly added maintenance-update localization keys,
including the entries near the beginning and end of the file, inside the
existing translation object in en.json. Preserve their keys and values, and
ensure they are siblings of the other translation entries so
update-checker-section.tsx lookups resolve through the default translation
namespace.
In `@web/src/i18n/locales/fr.json`:
- Around line 2-10: Move all newly added maintenance-update localization keys,
including the entries near the top and those near the end of the file, inside
the existing translation object in the French locale. Preserve their French
values and the file’s JSON structure so default-namespace lookups resolve the
translated strings.
In `@web/src/i18n/locales/ja.json`:
- Around line 2-10: Move the maintenance-control entries shown in both the lines
2–10 block and the lines 5241–5242 block under the existing top-level
translation object in ja.json. Keep translation as a flat string-to-string
mapping, preserve all keys and Japanese values, and remove the duplicate
top-level placement.
In `@web/src/i18n/locales/ru.json`:
- Around line 2-10: Move the maintenance entries at web/src/i18n/locales/ru.json
lines 2-10 into the existing translation object, and move the update entries at
web/src/i18n/locales/ru.json lines 5240-5242 before that object’s closing brace.
Keep the locale structure as a top-level translation object containing flat
key-value mappings.
- Line 5: Update the Russian translation value for the in-place update message
in the locale data to replace the English “pull” with the Russian verb
“загрузить,” while preserving the rest of the translation and the Docker command
referenced elsewhere.
In `@web/src/i18n/locales/zh-TW.json`:
- Around line 2-10: In web/src/i18n/locales/zh-TW.json lines 2-10, move the
automatic-update and rollback entries into the existing flat translation
mapping; in web/src/i18n/locales/zh-TW.json lines 5240-5242, move the
update-completion entries before the translation mapping’s closing brace. Ensure
the entire locale remains wrapped as { "translation": { ... } } with
string-to-string entries only.
In `@web/src/i18n/locales/zh.json`:
- Around line 2-10: Move the manual-update, Docker, and rollback locale entries
at web/src/i18n/locales/zh.json lines 2-10 into the top-level translation
object. At web/src/i18n/locales/zh.json lines 5240-5242, move the closing brace
below "Update now" so both update entries remain inside translation; preserve
flat string mappings and valid JSON.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f0745608-8ca2-4ad2-8803-24d4a919728b
📒 Files selected for processing (9)
controller/system_update.gosystemupdate/update.goweb/src/i18n/locales/en.jsonweb/src/i18n/locales/fr.jsonweb/src/i18n/locales/ja.jsonweb/src/i18n/locales/ru.jsonweb/src/i18n/locales/vi.jsonweb/src/i18n/locales/zh-TW.jsonweb/src/i18n/locales/zh.json
- Move maintenance update strings into i18next translation namespace - Fix Russian wording for Docker image pull guidance - Publish SystemUpdateService via atomic.Pointer for SetCurrentVersion
Follow-up (fourth CodeRabbit review on f304ab2)Pushed another fix:
|
Summary
Adds an in-place binary update flow inspired by Sub2API, so binary installs can check for updates, apply them, and roll back from the admin UI.
Backend (
systemupdate+ controller)GET /api/system-update/checkPOST /api/system-update/applyGET /api/system-update/rollback-versionsPOST /api/system-update/rollbackRootAuth)NEW_API_UPDATE_GITHUB_REPO(defaultCalcium-Ion/new-api),UPDATE_GITHUB_TOKENFrontend
Tests
go test ./systemupdate/→ 5/5 PASSNotes
Test plan
Summary by CodeRabbit
New Features
Bug Fixes
Documentation