Add CLIProxyAPI binary self-update flow#39
Merged
Conversation
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughCLIProxyAPI 바이너리를 앱(Sparkle) 업데이트와 분리해 자가 업데이트하는 기능이 추가됩니다. 버전 파싱, GitHub 릴리스 조회, 아카이브 검증, active/pending 바이너리 저장소, 업데이트 서비스 상태머신 및 About 설정/Dashboard UI가 구현됩니다. 또한 DEBUG 빌드용 관리 경로 분리( ChangesCLIProxyAPI 바이너리 자가 업데이트
Estimated code review effort: 5 (Critical) | ~120 minutes 개발 빌드 경로 분리 및 기본 포트
Estimated code review effort: 3 (Moderate) | ~25 minutes 설계/계획 문서
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant Service as CLIProxyAPIUpdateService
participant Release as CLIProxyAPIReleaseClient
participant Verifier as CLIProxyAPIArchiveVerifier
participant Store as CLIProxyAPIBinaryStore
participant Server as ProxyServiceManager
App->>Service: checkAutomaticallyOnLaunch()
Service->>Release: latestRelease()
Release-->>Service: CLIProxyAPIRelease
Service-->>App: availableUpdate 표시
App->>Service: downloadAvailableUpdate()
Service->>Release: downloadArchive()
Release-->>Service: archive Data
Service->>Verifier: verify(archiveData, release)
Verifier-->>Service: CLIProxyAPIBinaryVerificationResult
Service->>Store: savePending(binary, manifest)
Service-->>App: pendingUpdate 표시
App->>Service: applyPendingNow()
Service->>Store: applyPending()
Store-->>Service: active 승격 완료
App->>Server: restartServer() (실행 중일 때)
Possibly related PRs
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an in-app update mechanism for the CLIProxyAPI binary, allowing it to be updated independently of the main CLIProxyManager app. It includes background update checks, checksum verification, and immediate or deferred update application, with UI controls integrated into the 'About' settings and automatic prompts on the dashboard. Additionally, debug builds are isolated to a separate dev directory. The code review feedback focuses on improving robustness and memory efficiency: adding a User-Agent header to GitHub API requests, supporting GoReleaser-style filename prefixes in the checksum parser, using a streaming approach to compute SHA256 hashes instead of loading large binaries into memory, and displaying the actual failure reason in the UI when an update check fails.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } | ||
|
|
||
| public func latestRelease() async throws -> CLIProxyAPIRelease { | ||
| let data = try await httpClient.get(latestReleaseURL, headers: ["Accept": "application/vnd.github+json"]) |
There was a problem hiding this comment.
GitHub's REST API strictly requires a valid User-Agent header for all requests. Without it, requests can be rejected with a 403 Forbidden error. Adding a custom User-Agent header (such as the app's name) ensures reliable API communication.
let data = try await httpClient.get(latestReleaseURL, headers: [
"Accept": "application/vnd.github+json",
"User-Agent": "CLIProxyManager"
])
Comment on lines
+79
to
+88
| static func checksum(for assetName: String, in data: Data) -> String? { | ||
| guard let text = String(data: data, encoding: .utf8) else { return nil } | ||
| for line in text.split(whereSeparator: { $0 == "\n" || $0 == "\r" }) { | ||
| let parts = line.split(whereSeparator: { $0 == " " || $0 == "\t" }).map(String.init) | ||
| if parts.count >= 2, parts[1] == assetName { | ||
| return parts[0] | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Standard checksum files (like those generated by sha256sum or GoReleaser) sometimes prefix binary filenames with a * character (e.g., hash *filename). Stripping this prefix dynamically makes the checksum parser more robust against different generation formats.
Suggested change
| static func checksum(for assetName: String, in data: Data) -> String? { | |
| guard let text = String(data: data, encoding: .utf8) else { return nil } | |
| for line in text.split(whereSeparator: { $0 == "\n" || $0 == "\r" }) { | |
| let parts = line.split(whereSeparator: { $0 == " " || $0 == "\t" }).map(String.init) | |
| if parts.count >= 2, parts[1] == assetName { | |
| return parts[0] | |
| } | |
| } | |
| return nil | |
| } | |
| static func checksum(for assetName: String, in data: Data) -> String? { | |
| guard let text = String(data: data, encoding: .utf8) else { return nil } | |
| for line in text.split(whereSeparator: { $0 == "\n" || $0 == "\r" }) { | |
| let parts = line.split(whereSeparator: { $0 == " " || $0 == "\t" }).map(String.init) | |
| if parts.count >= 2 { | |
| let filename = parts[1].hasPrefix("*") ? String(parts[1].dropFirst()) : parts[1] | |
| if filename == assetName { | |
| return parts[0] | |
| } | |
| } | |
| } | |
| return nil | |
| } |
Comment on lines
+178
to
+186
| private func validateBinary(at url: URL, manifest: CLIProxyAPIBinaryManifest) throws { | ||
| if try Data(contentsOf: url).sha256HexDigest() != manifest.vendoredBinarySha256 { | ||
| throw CLIProxyAPIBinaryStoreError.binaryChecksumMismatch | ||
| } | ||
| let size = try url.resourceValues(forKeys: [.fileSizeKey]).fileSize ?? -1 | ||
| if size != manifest.vendoredBinarySizeBytes { | ||
| throw CLIProxyAPIBinaryStoreError.binarySizeMismatch | ||
| } | ||
| } |
There was a problem hiding this comment.
Loading the entire binary file into memory via Data(contentsOf:) to compute its SHA256 hash is memory-inefficient and can cause memory spikes, especially for larger binaries (around 40MB+). Using a streaming approach with FileHandle is much more memory-efficient.
Suggested change
| private func validateBinary(at url: URL, manifest: CLIProxyAPIBinaryManifest) throws { | |
| if try Data(contentsOf: url).sha256HexDigest() != manifest.vendoredBinarySha256 { | |
| throw CLIProxyAPIBinaryStoreError.binaryChecksumMismatch | |
| } | |
| let size = try url.resourceValues(forKeys: [.fileSizeKey]).fileSize ?? -1 | |
| if size != manifest.vendoredBinarySizeBytes { | |
| throw CLIProxyAPIBinaryStoreError.binarySizeMismatch | |
| } | |
| } | |
| private func validateBinary(at url: URL, manifest: CLIProxyAPIBinaryManifest) throws { | |
| if try url.sha256HexDigest() != manifest.vendoredBinarySha256 { | |
| throw CLIProxyAPIBinaryStoreError.binaryChecksumMismatch | |
| } | |
| let size = try url.resourceValues(forKeys: [.fileSizeKey]).fileSize ?? -1 | |
| if size != manifest.vendoredBinarySizeBytes { | |
| throw CLIProxyAPIBinaryStoreError.binarySizeMismatch | |
| } | |
| } |
Comment on lines
+230
to
+234
| extension Data { | ||
| func sha256HexDigest() -> String { | ||
| SHA256.hash(data: self).map { String(format: "%02x", $0) }.joined() | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a streaming SHA256 helper extension on URL to allow memory-efficient file hashing without loading the entire file into memory.
Suggested change
| extension Data { | |
| func sha256HexDigest() -> String { | |
| SHA256.hash(data: self).map { String(format: "%02x", $0) }.joined() | |
| } | |
| } | |
| extension Data { | |
| func sha256HexDigest() -> String { | |
| SHA256.hash(data: self).map { String(format: "%02x", $0) }.joined() | |
| } | |
| } | |
| extension URL { | |
| func sha256HexDigest() throws -> String { | |
| let fileHandle = try FileHandle(forReadingFrom: self) | |
| defer { try? fileHandle.close() } | |
| var sha256 = SHA256() | |
| let bufferSize = 64 * 1024 | |
| while let data = try fileHandle.read(upToCount: bufferSize), !data.isEmpty { | |
| sha256.update(data: data) | |
| } | |
| return sha256.finalize().map { String(format: "%02x", $0) }.joined() | |
| } | |
| } |
Comment on lines
+77
to
+95
| let binaryData = try Data(contentsOf: binaryURL) | ||
| let size = try binaryURL.resourceValues(forKeys: [.fileSizeKey]).fileSize ?? binaryData.count | ||
| let manifest = CLIProxyAPIBinaryManifest( | ||
| name: "cliproxyapi", | ||
| version: metadata.version, | ||
| commit: metadata.commit, | ||
| builtAt: metadata.builtAt, | ||
| sourceKind: .userUpdated, | ||
| source: release.assetURL.absoluteString, | ||
| upstreamRepository: "router-for-me/CLIProxyAPI", | ||
| upstreamTag: release.tagName, | ||
| upstreamAsset: release.assetName, | ||
| upstreamAssetSha256: release.assetSha256, | ||
| vendoredBinaryName: "cliproxyapi", | ||
| vendoredBinarySha256: binaryData.sha256HexDigest(), | ||
| vendoredBinarySizeBytes: size, | ||
| vendoredFromArchivePath: "cli-proxy-api", | ||
| downloadedAt: ISO8601DateFormatter().string(from: Date()) | ||
| ) |
There was a problem hiding this comment.
Avoid loading the entire binary into memory to compute its size and SHA256 hash. Using the streaming sha256HexDigest() helper on URL is much more memory-efficient.
let size = try binaryURL.resourceValues(forKeys: [.fileSizeKey]).fileSize ?? 0
let manifest = try CLIProxyAPIBinaryManifest(
name: "cliproxyapi",
version: metadata.version,
commit: metadata.commit,
builtAt: metadata.builtAt,
sourceKind: .userUpdated,
source: release.assetURL.absoluteString,
upstreamRepository: "router-for-me/CLIProxyAPI",
upstreamTag: release.tagName,
upstreamAsset: release.assetName,
upstreamAssetSha256: release.assetSha256,
vendoredBinaryName: "cliproxyapi",
vendoredBinarySha256: binaryURL.sha256HexDigest(),
vendoredBinarySizeBytes: size,
vendoredFromArchivePath: "cli-proxy-api",
downloadedAt: ISO8601DateFormatter().string(from: Date())
)
Comment on lines
+194
to
+195
| case .failed: | ||
| return "Current version: \(currentVersion) · Last check failed." |
There was a problem hiding this comment.
Pattern-matching the associated error message in the .failed state allows displaying the actual failure reason to the user, which significantly improves usability.
Suggested change
| case .failed: | |
| return "Current version: \(currentVersion) · Last check failed." | |
| case .failed(let message): | |
| return "Current version: \(currentVersion) · Last check failed: \(message)" |
Comment on lines
+63
to
+71
| XCTAssertEqual( | ||
| cliproxyAPIUpdateDescription( | ||
| currentVersion: "7.2.41", | ||
| state: .failed("Network unavailable"), | ||
| availableUpdate: nil, | ||
| pendingUpdate: nil | ||
| ), | ||
| "Current version: 7.2.41 · Last check failed." | ||
| ) |
There was a problem hiding this comment.
Update the unit test to assert the new, more descriptive failure message format.
Suggested change
| XCTAssertEqual( | |
| cliproxyAPIUpdateDescription( | |
| currentVersion: "7.2.41", | |
| state: .failed("Network unavailable"), | |
| availableUpdate: nil, | |
| pendingUpdate: nil | |
| ), | |
| "Current version: 7.2.41 · Last check failed." | |
| ) | |
| XCTAssertEqual( | |
| cliproxyAPIUpdateDescription( | |
| currentVersion: "7.2.41", | |
| state: .failed("Network unavailable"), | |
| availableUpdate: nil, | |
| pendingUpdate: nil | |
| ), | |
| "Current version: 7.2.41 · Last check failed: Network unavailable" | |
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
Tests/CLIProxyManagerAppTests/AutomaticShellInstallServiceTests.swift (1)
7-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value컴파일 타임 분기 테스트라 항상 한쪽 브랜치만 실행됨
#if DEBUG분기가 테스트 자체 내부에도 있어서, 실제로 실행되는 빌드 설정과 다른 쪽 어서션은 절대 검증되지 않습니다. 유닛 테스트가 보통 Debug 구성으로 빌드/실행되는 점을 감안하면#else분기(Release 시 정상 설치)는 CI에서 사실상 검증되지 않을 가능성이 높습니다. 큰 문제는 아니지만, Release 구성으로도 테스트를 한 번 돌리는 CI job이 있는지 확인해볼 가치가 있습니다.🤖 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 `@Tests/CLIProxyManagerAppTests/AutomaticShellInstallServiceTests.swift` around lines 7 - 21, The testRuntimeDefaultDoesNotInstallInDebugBuild test is using a compile-time `#if` DEBUG branch, so only one assertion path is ever exercised in a given build and the Release expectation is not really validated. Move the Debug/Release behavior coverage out of a single conditional test in AutomaticShellInstallServiceTests and either split it into separate build-specific tests or ensure CI runs the test suite in both Debug and Release configurations. Keep the assertions around AutomaticShellInstallService.runtimeDefault and StubShellInstaller, but make sure both install and non-install behavior are actually exercised by the test matrix.Sources/CLIProxyManagerApp/Services/CLIProxyAPIUpdateService.swift (1)
152-166: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
applyPendingNow()는 실패 시recordFailure를 호출하지 않아 상태/에러 메시지가 일관되지 않음
check(suppressDeferredVersion:)와downloadAvailableUpdate()는 실패 시recordFailure(error)를 호출해state를.failed로 전환하고lastErrorMessage/영속화된lastFailureMessage/lastFailureAt을 갱신합니다. 반면applyPendingNow()는store.applyPending()이 던지는 오류를 그대로 다시 던지기만 하고 이런 상태 갱신을 하지 않습니다. 적용 실패(예: 매니페스트 쓰기 실패, 체크섬 불일치) 시 UI가 이 서비스의state/lastErrorMessage에만 의존한다면 실패 정보가 누락될 수 있습니다.♻️ 제안: recordFailure로 상태 일관성 확보
func applyPendingNow() throws { refreshStoredStatus() - try store.applyPending() + do { + try store.applyPending() + } catch { + recordFailure(error) + throw error + } pendingUpdate = nil availableUpdate = nil🤖 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 `@Sources/CLIProxyManagerApp/Services/CLIProxyAPIUpdateService.swift` around lines 152 - 166, `CLIProxyAPIUpdateService.applyPendingNow()` handles `store.applyPending()` failures without updating the service failure state, unlike `check(suppressDeferredVersion:)` and `downloadAvailableUpdate()`. Wrap the pending-apply flow in the same failure handling path by calling `recordFailure(error)` when `store.applyPending()` throws, so `state`, `lastErrorMessage`, and persisted failure metadata stay consistent. Keep the existing success path in `applyPendingNow()` unchanged, and use the existing `recordFailure` helper to centralize error-state updates.Sources/CLIProxyManagerCore/Proxy/ProxyHealthClient.swift (1)
32-39: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win대용량 아카이브 다운로드 시 불필요한 디스크 캐싱 가능성.
requestCachePolicy = .reloadIgnoringLocalCacheData는 응답을 읽을 때 로컬 캐시를 무시할 뿐, 응답이 이후URLCache.shared에 저장되는 것까지 막지는 않습니다. 릴리스 아카이브는 한 번만 사용되는 대용량(수십MB) 바이너리이므로, 캐시에 불필요하게 저장되어 디스크를 낭비하거나 다른 캐시 항목을 밀어낼 수 있습니다.♻️ 제안: URL 캐시 비활성화
public static func makeExternalUpdateSession() -> URLSession { let configuration = URLSessionConfiguration.default configuration.timeoutIntervalForRequest = 30 configuration.timeoutIntervalForResource = 300 configuration.requestCachePolicy = .reloadIgnoringLocalCacheData + configuration.urlCache = nil return URLSession(configuration: configuration) }🤖 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 `@Sources/CLIProxyManagerCore/Proxy/ProxyHealthClient.swift` around lines 32 - 39, The URLSession created in makeExternalUpdateSession still allows large release archive responses to be cached on disk even with reloadIgnoringLocalCacheData. Update the URLSessionConfiguration in ProxyHealthClient.makeExternalUpdateSession to disable caching for this one-shot download session, so archive fetches do not get stored in URLCache.shared. Keep the timeout settings as-is and make the cache behavior explicit in the same configuration block.Tests/CLIProxyManagerCoreTests/CLIProxyAPIReleaseClientTests.swift (1)
118-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win테스트 헬퍼 중복 (
XCTAssertThrowsErrorAsync)동일한 헬퍼 함수가
CLIProxyAPIArchiveVerifierTests.swift에도 그대로 복사되어 있습니다. 공용 테스트 유틸리티 파일로 추출하면 유지보수가 쉬워집니다.🤖 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 `@Tests/CLIProxyManagerCoreTests/CLIProxyAPIReleaseClientTests.swift` around lines 118 - 130, The async test helper XCTAssertThrowsErrorAsync is duplicated in CLIProxyAPIReleaseClientTests and CLIProxyAPIArchiveVerifierTests, so extract it into a shared test utility used by both suites. Move the helper into a common test support file, keep the same async throwing assertion behavior, and update the tests to call that shared helper instead of maintaining separate copies.Sources/CLIProxyManagerCore/Proxy/CLIProxyAPIArchiveVerifier.swift (1)
79-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
"router-for-me/CLIProxyAPI"매직 스트링 중복이 저장소 식별자는
CLIProxyAPIReleaseClient.swift(컨텍스트 스니펫 기준 최신 릴리스 URL 구성에 사용)에도 동일하게 하드코딩되어 있습니다. 공유 상수로 추출하면 향후 저장소 변경 시 한 곳만 수정하면 됩니다.♻️ 제안: 공유 상수로 추출
+public enum CLIProxyAPIRepository { + public static let identifier = "router-for-me/CLIProxyAPI" +}- upstreamRepository: "router-for-me/CLIProxyAPI", + upstreamRepository: CLIProxyAPIRepository.identifier,🤖 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 `@Sources/CLIProxyManagerCore/Proxy/CLIProxyAPIArchiveVerifier.swift` around lines 79 - 95, The repository identifier is hardcoded in multiple places, so extract the shared value into a common constant and reuse it here in CLIProxyAPIArchiveVerifier when building CLIProxyAPIBinaryManifest, as well as in CLIProxyAPIReleaseClient; replace the inline "router-for-me/CLIProxyAPI" string with that shared symbol so future repository changes only need one update.
🤖 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 `@docs/superpowers/plans/2026-07-04-cliproxyapi-update-about-ui.md`:
- Around line 35-37: Update the plan text so it matches the current branch
state: `AboutSettingsView` already takes `viewModel` and
`cliProxyAPIUpdateService` and includes the `CLIProxyAPI binary` row, so the
current wording should not imply that this UI still lives in
`ServerSettingsView`. Revise the affected bullet in the plan doc to reflect the
present `GeneralSettingsView`/`AboutSettingsView` setup, or explicitly label it
as describing the previous state; use the `AboutSettingsView` and
`ServerSettingsView` references to make the scope clear.
In `@docs/superpowers/specs/2026-07-04-cliproxyapi-update-about-ui-design.md`:
- Around line 198-206: `cliProxyAPIPendingUpdatePromptTitle`의 함수 서명을 문서와 호출부에서
하나로 맞추세요. 현재 문서의 선언에는 `currentVersion:`이 포함돼 있지만, `GeneralSettingsView.swift`
스니펫과 단계별 수정안은 `pendingUpdate:`만 사용하므로, `cliProxyAPIPendingUpdatePromptTitle` 정의와
이를 호출하는 쪽 모두 같은 매개변수 형태로 정리해 계약 불일치를 없애세요.
In `@Sources/CLIProxyManagerApp/Views/DashboardView.swift`:
- Around line 131-158: The CLIProxyAPI update apply flow is duplicated in
DashboardView and GeneralSettingsView, and the success message has already
drifted between the two implementations. Extract the shared apply logic from the
button action into a single DashboardViewModel method such as
applyCLIProxyAPIPendingUpdate(using:), keeping the try applyPendingNow(),
conditional restartServer(), and success/error settingsMessage handling in one
place. Then have both views call that shared method so the update behavior and
user-facing text stay consistent.
- Around line 108-130: In DashboardView’s confirmationDialog download action,
handle the failure path from CLIProxyAPIUpdateService.downloadAvailableUpdate()
instead of only checking pendingUpdate; since the service records failures in
state and does not throw, add explicit user feedback when pendingUpdate remains
nil after the Task completes, similar to the apply flow’s settingsMessage
handling. Use the existing symbols showCLIProxyAPIUpdatePrompt,
showCLIProxyAPIApplyPrompt, cliProxyAPIUpdateService.state, and
downloadAvailableUpdate() to surface an error message or alert so the user can
tell the download failed.
In `@Sources/CLIProxyManagerApp/Views/GeneralSettingsView.swift`:
- Around line 352-379: This confirmationDialog apply flow duplicates the same
apply/restart logic already present in DashboardView, and the success messages
have already drifted apart. Extract the shared apply behavior into a common
method on DashboardViewModel (or a shared helper used by both
GeneralSettingsView and DashboardView), then have this Button action call that
shared entry point and keep the status messaging consistent from a single place.
In `@Sources/CLIProxyManagerCore/Proxy/CLIProxyAPIBinaryStore.swift`:
- Around line 50-122: `CLIProxyAPIBinaryStore`의 `applyPending()`와
`prepareActiveBinary()`가 동일한 파일 집합(`clipProxyBinary`, `activeClipProxyManifest`,
`pendingClipProxyDirectory`)을 갱신하므로, 두 경로를 같은 잠금으로 직렬화하도록 수정하세요.
`ProxyServiceManager.lifecycleLock`이 `ProxyServiceManager` 내부만 보호하는 현재 구조를 점검하고,
`CLIProxyAPIUpdateService.applyPendingNow()`와
`prepareActiveBinary(bundledBinaryURL:bundledManifestURL:)`가 공통으로 획득하는 파일 락 또는
단일 직렬화 진입점을 추가해 동시 실행을 막으세요. 이동/복구/삭제가 섞이지 않도록 락 획득과 해제를 두 메서드의 전체 작업 범위에 적용하세요.
---
Nitpick comments:
In `@Sources/CLIProxyManagerApp/Services/CLIProxyAPIUpdateService.swift`:
- Around line 152-166: `CLIProxyAPIUpdateService.applyPendingNow()` handles
`store.applyPending()` failures without updating the service failure state,
unlike `check(suppressDeferredVersion:)` and `downloadAvailableUpdate()`. Wrap
the pending-apply flow in the same failure handling path by calling
`recordFailure(error)` when `store.applyPending()` throws, so `state`,
`lastErrorMessage`, and persisted failure metadata stay consistent. Keep the
existing success path in `applyPendingNow()` unchanged, and use the existing
`recordFailure` helper to centralize error-state updates.
In `@Sources/CLIProxyManagerCore/Proxy/CLIProxyAPIArchiveVerifier.swift`:
- Around line 79-95: The repository identifier is hardcoded in multiple places,
so extract the shared value into a common constant and reuse it here in
CLIProxyAPIArchiveVerifier when building CLIProxyAPIBinaryManifest, as well as
in CLIProxyAPIReleaseClient; replace the inline "router-for-me/CLIProxyAPI"
string with that shared symbol so future repository changes only need one
update.
In `@Sources/CLIProxyManagerCore/Proxy/ProxyHealthClient.swift`:
- Around line 32-39: The URLSession created in makeExternalUpdateSession still
allows large release archive responses to be cached on disk even with
reloadIgnoringLocalCacheData. Update the URLSessionConfiguration in
ProxyHealthClient.makeExternalUpdateSession to disable caching for this one-shot
download session, so archive fetches do not get stored in URLCache.shared. Keep
the timeout settings as-is and make the cache behavior explicit in the same
configuration block.
In `@Tests/CLIProxyManagerAppTests/AutomaticShellInstallServiceTests.swift`:
- Around line 7-21: The testRuntimeDefaultDoesNotInstallInDebugBuild test is
using a compile-time `#if` DEBUG branch, so only one assertion path is ever
exercised in a given build and the Release expectation is not really validated.
Move the Debug/Release behavior coverage out of a single conditional test in
AutomaticShellInstallServiceTests and either split it into separate
build-specific tests or ensure CI runs the test suite in both Debug and Release
configurations. Keep the assertions around
AutomaticShellInstallService.runtimeDefault and StubShellInstaller, but make
sure both install and non-install behavior are actually exercised by the test
matrix.
In `@Tests/CLIProxyManagerCoreTests/CLIProxyAPIReleaseClientTests.swift`:
- Around line 118-130: The async test helper XCTAssertThrowsErrorAsync is
duplicated in CLIProxyAPIReleaseClientTests and CLIProxyAPIArchiveVerifierTests,
so extract it into a shared test utility used by both suites. Move the helper
into a common test support file, keep the same async throwing assertion
behavior, and update the tests to call that shared helper instead of maintaining
separate copies.
🪄 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: 9241af23-976c-46ed-9bda-44addd039f54
📒 Files selected for processing (39)
README.mdSources/CLIProxyManagerApp/BundledProxyBinary.swiftSources/CLIProxyManagerApp/CLIProxyManagerApp.swiftSources/CLIProxyManagerApp/Services/AutomaticShellInstallService.swiftSources/CLIProxyManagerApp/Services/CLIProxyAPIUpdateService.swiftSources/CLIProxyManagerApp/ViewModels/DashboardViewModel.swiftSources/CLIProxyManagerApp/Views/DashboardView.swiftSources/CLIProxyManagerApp/Views/GeneralSettingsView.swiftSources/CLIProxyManagerApp/Views/SettingsSheets.swiftSources/CLIProxyManagerApp/Views/SettingsView.swiftSources/CLIProxyManagerCore/Config/AppConfig.swiftSources/CLIProxyManagerCore/Config/ManagedPaths.swiftSources/CLIProxyManagerCore/Proxy/CLIProxyAPIArchiveVerifier.swiftSources/CLIProxyManagerCore/Proxy/CLIProxyAPIBinaryManifest.swiftSources/CLIProxyManagerCore/Proxy/CLIProxyAPIBinaryStore.swiftSources/CLIProxyManagerCore/Proxy/CLIProxyAPIReleaseClient.swiftSources/CLIProxyManagerCore/Proxy/CLIProxyAPIVersion.swiftSources/CLIProxyManagerCore/Proxy/ProxyHealthClient.swiftSources/CLIProxyManagerCore/Proxy/ProxyServiceManager.swiftTests/CLIProxyManagerAppTests/AutomaticShellInstallServiceTests.swiftTests/CLIProxyManagerAppTests/CLIProxyAPIUpdateServiceTests.swiftTests/CLIProxyManagerAppTests/CLIProxyAPIUpdateUITests.swiftTests/CLIProxyManagerAppTests/DashboardViewModelTests.swiftTests/CLIProxyManagerAppTests/ProviderSettingsViewModelTests.swiftTests/CLIProxyManagerCoreTests/AppConfigStoreTests.swiftTests/CLIProxyManagerCoreTests/AppConfigTests.swiftTests/CLIProxyManagerCoreTests/CLIProxyAPIArchiveVerifierTests.swiftTests/CLIProxyManagerCoreTests/CLIProxyAPIBinaryManifestTests.swiftTests/CLIProxyManagerCoreTests/CLIProxyAPIBinaryStoreTests.swiftTests/CLIProxyManagerCoreTests/CLIProxyAPIReleaseClientTests.swiftTests/CLIProxyManagerCoreTests/CLIProxyAPIVersionTests.swiftTests/CLIProxyManagerCoreTests/ProxyServiceManagerTests.swiftTests/CLIProxyManagerCoreTests/ShellFunctionRendererTests.swiftdocs/superpowers/plans/2026-07-01-cliproxyapi-binary-self-update.mddocs/superpowers/plans/2026-07-02-dev-managed-path.mddocs/superpowers/plans/2026-07-04-cliproxyapi-update-about-ui.mddocs/superpowers/specs/2026-07-01-cliproxyapi-binary-self-update-design.mddocs/superpowers/specs/2026-07-02-dev-managed-path-design.mddocs/superpowers/specs/2026-07-04-cliproxyapi-update-about-ui-design.md
Comment on lines
+131
to
+158
| .confirmationDialog( | ||
| cliProxyAPIPendingUpdatePromptTitle(pendingUpdate: cliProxyAPIUpdateService.pendingUpdate), | ||
| isPresented: $showCLIProxyAPIApplyPrompt, | ||
| titleVisibility: .visible | ||
| ) { | ||
| Button(cliProxyAPIApplyButtonTitle( | ||
| pendingUpdate: cliProxyAPIUpdateService.pendingUpdate, | ||
| isServerRunning: viewModel.serverControlState.isRunning | ||
| )) { | ||
| Task { | ||
| do { | ||
| try cliProxyAPIUpdateService.applyPendingNow() | ||
| if viewModel.serverControlState.isRunning { | ||
| await viewModel.restartServer() | ||
| } | ||
| viewModel.settingsMessage = "CLIProxyAPI binary updated. Restarting the app is not required." | ||
| } catch { | ||
| viewModel.settingsMessage = "CLIProxyAPI update failed: \(error.localizedDescription)" | ||
| } | ||
| } | ||
| } | ||
| Button("Apply on next server start") { | ||
| viewModel.settingsMessage = "CLIProxyAPI update will be applied on next server start." | ||
| } | ||
| Button("Cancel", role: .cancel) {} | ||
| } message: { | ||
| Text(cliProxyAPIPendingUpdatePromptMessage(currentVersion: cliProxyAPIUpdateService.currentVersionText)) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
GeneralSettingsView.swift(352-379)와 apply 로직이 그대로 중복되어 이미 문구가 갈라짐.
두 파일 모두 try applyPendingNow() → 서버 실행 중이면 restartServer() → 성공/실패 메시지 설정이라는 동일한 흐름을 독립적으로 구현했습니다. 그 결과 성공 메시지가 서로 다릅니다: 여기는 "CLIProxyAPI binary updated. Restarting the app is not required.", GeneralSettingsView는 "CLIProxyAPI update applied."입니다. DashboardViewModel에 공용 메서드(예: applyCLIProxyAPIPendingUpdate(using:))로 추출해 두 뷰에서 재사용하면 문구 불일치와 향후 유지보수 시 로직 이탈 위험을 없앨 수 있습니다.
♻️ 제안 리팩터
// DashboardViewModel.swift
func applyCLIProxyAPIPendingUpdate(using service: CLIProxyAPIUpdateService) async {
do {
try service.applyPendingNow()
if serverControlState.isRunning {
await restartServer()
}
settingsMessage = "CLIProxyAPI binary updated. Restarting the app is not required."
} catch {
settingsMessage = "CLIProxyAPI update failed: \(error.localizedDescription)"
}
}이후 양쪽 뷰의 버튼 클로저는 Task { await viewModel.applyCLIProxyAPIPendingUpdate(using: cliProxyAPIUpdateService) }로 단순화합니다.
🤖 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 `@Sources/CLIProxyManagerApp/Views/DashboardView.swift` around lines 131 - 158,
The CLIProxyAPI update apply flow is duplicated in DashboardView and
GeneralSettingsView, and the success message has already drifted between the two
implementations. Extract the shared apply logic from the button action into a
single DashboardViewModel method such as applyCLIProxyAPIPendingUpdate(using:),
keeping the try applyPendingNow(), conditional restartServer(), and
success/error settingsMessage handling in one place. Then have both views call
that shared method so the update behavior and user-facing text stay consistent.
Co-Authored-By: Claude <noreply@anthropic.com>
Owner
Author
|
Review feedback addressed in
Verification:
CodeRabbit is currently rate-limited/pending; the branch is pushed and ready for the next bot review when available. |
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.
Summary
~/.cliproxy-manager/devwith default dev port18318Verification
DEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer swift test— 351 tests passedDEVELOPER_DIR=/Applications/Xcode.app/Contents/Developer swift build -c debug --product CLIProxyManager --scratch-path /tmp/cliproxy-manager-dev-open-build/tmp/CLIProxyManagerDev.app; production snapshot unchangedport=18318,autostartServer=false; production18317listener unchanged7.2.41and confirmed pending update state for7.2.50testing🤖 Generated with Claude Code
Summary by CodeRabbit
새 기능
버그 수정