Document E5 mirror system design and implementation plan#469
Merged
rainxchzed merged 32 commits intomainfrom Apr 30, 2026
Merged
Document E5 mirror system design and implementation plan#469rainxchzed merged 32 commits intomainfrom
rainxchzed merged 32 commits intomainfrom
Conversation
…interceptor reads
…ROR_REWRITE bypass
…O_MIRROR_REWRITE bypass
…6 after completion
…Dialog, DeployYourOwnHint
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
WalkthroughAdds a mirror/proxy system: mirror catalog fetching & persistence, mirror picker UI and navigation, proxy rewrite interceptor, multi-source racing downloader, slow-download detector with auto-suggest UI, SHA‑256 digest verification, and DI wiring including platform digest verifiers and a test HttpClient. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MirrorPickerVM
participant MirrorRepo
participant ProxyManager
participant TestHttpClient
participant Remote
User->>MirrorPickerVM: open screen
MirrorPickerVM->>MirrorRepo: observeCatalog() & observePreference()
MirrorRepo-->>MirrorPickerVM: catalog + preference
User->>MirrorPickerVM: OnSelectMirror(mirror)
MirrorPickerVM->>MirrorRepo: setPreference(Selected(id))
MirrorRepo->>ProxyManager: preference change
ProxyManager->>ProxyManager: update current template
User->>MirrorPickerVM: OnTestConnection
MirrorPickerVM->>TestHttpClient: GET probe (5s, NO_MIRROR_REWRITE absent)
TestHttpClient->>Remote: request
alt 2xx
Remote-->>TestHttpClient: success
MirrorPickerVM->>User: show Success(latency)
else timeout / DNS / HTTP error
MirrorPickerVM->>User: show corresponding result
end
sequenceDiagram
participant DownloadFlow
participant MultiSourceDownloader
participant DirectDownloader
participant MirrorDownloader
participant WinnerDeferred
participant SlowDetector
participant FileSystem
DownloadFlow->>MultiSourceDownloader: download(githubUrl)
alt template available
par direct
MultiSourceDownloader->>DirectDownloader: download(directUrl, bypassMirror=true)
DirectDownloader->>WinnerDeferred: tryCompleteOnSuccess()
DirectDownloader->>SlowDetector: emit progress
DirectDownloader->>FileSystem: write chunks
and mirror
MultiSourceDownloader->>MirrorDownloader: download(rewrittenUrl)
MirrorDownloader->>WinnerDeferred: tryCompleteOnSuccess()
MirrorDownloader->>SlowDetector: emit progress
MirrorDownloader->>FileSystem: write chunks
end
WinnerDeferred-->>MultiSourceDownloader: winner chosen
MultiSourceDownloader->>Loser: cancelAndJoin()
else no template
MultiSourceDownloader->>DirectDownloader: download(directUrl, bypassMirror=true)
end
SlowDetector->>DownloadFlow: suggestMirror (maybe)
FileSystem-->>DownloadFlow: complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
# Conflicts: # composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/ViewModelsModule.kt # core/presentation/src/commonMain/composeResources/values/strings.xml # feature/tweaks/presentation/build.gradle.kts # feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksAction.kt # feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksViewModel.kt
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (5)
feature/tweaks/presentation/build.gradle.kts (1)
19-19: Changeapi()toimplementation()for Ktor dependency.The
testHttpClient: HttpClientdependency inMirrorPickerViewModel.ktis private and used only internally. Since no public declarations in this module expose Ktor types, theapi(libs.ktor.client.core)unnecessarily leaks Ktor to downstream modules. Switch toimplementation(...)to limit transitive exposure.Suggested change
- api(libs.ktor.client.core) + implementation(libs.ktor.client.core)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/tweaks/presentation/build.gradle.kts` at line 19, The buildscript currently declares the Ktor client as api(libs.ktor.client.core) which unnecessarily exposes Ktor types to downstream modules; change that to implementation(libs.ktor.client.core). Locate the dependency declaration in build.gradle.kts and replace api(...) with implementation(...); this aligns with how the Ktor HttpClient is used privately in MirrorPickerViewModel.kt (testHttpClient: HttpClient) and prevents leaking Ktor types transitively.core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/ProxyManager.kt (1)
63-64: 💤 Low valueNon-atomic check-then-act on
mirrorCollectorJob.The idempotency check
mirrorCollectorJob?.isActive == truefollowed by assignment creates a race window. IfstartMirrorCollectoris called concurrently, both calls could pass the check and launch duplicate collectors.Given the relevant code snippet shows this is called once during DI initialization (
SharedModule.kt:143-157), this is likely safe in practice, but consider using aMutexorsynchronizedfor defensive correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/ProxyManager.kt` around lines 63 - 64, The idempotency check in startMirrorCollector on the mirrorCollectorJob field is non-atomic and can race; wrap the check-and-assign in a mutual-exclusion block (e.g., add a Mutex like mirrorCollectorMutex and perform the if (mirrorCollectorJob?.isActive == true) return and subsequent mirrorCollectorJob = ... inside mirrorCollectorMutex.withLock { ... } or use a synchronized(this) block) so only one coroutine/thread can observe-and-create the job; update the startMirrorCollector method to acquire the mutex before checking mirrorCollectorJob and releasing it after assignment to prevent duplicate collectors.feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.kt (2)
16-16: ⚖️ Poor tradeoffPresentation layer imports data layer class.
MirrorRewriteris imported fromcore.data.network, which violates clean architecture's dependency rule (presentation should only depend on domain). Consider movingMirrorRewriterto domain or exposing a use case that encapsulates the template application logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.kt` at line 16, MirrorPickerViewModel imports the data-layer class MirrorRewriter, violating the presentation→domain dependency rule; remove the direct dependency by either (A) moving MirrorRewriter (or its template-application logic) into the domain module as a domain service, or (B) create a domain use-case (e.g., ApplyMirrorTemplateUseCase) that exposes a method used by MirrorPickerViewModel; update MirrorPickerViewModel to depend only on the new domain use-case or domain service (referencing MirrorPickerViewModel and MirrorRewriter by name) and adjust DI/constructors to inject the domain abstraction instead of the data-layer class.
139-145: ⚡ Quick win
refresh()silently swallows errors fromrefreshCatalog().If
mirrorRepository.refreshCatalog()throws,isRefreshingremainstrueand the user sees no feedback. Consider wrapping in try-catch to reset state and optionally emit an error event.♻️ Suggested fix
private fun refresh() { viewModelScope.launch { _state.update { it.copy(isRefreshing = true) } - mirrorRepository.refreshCatalog() - _state.update { it.copy(isRefreshing = false) } + try { + mirrorRepository.refreshCatalog() + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + _events.send(MirrorPickerEvent.RefreshFailed(e.message ?: "Unknown error")) + } finally { + _state.update { it.copy(isRefreshing = false) } + } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.kt` around lines 139 - 145, The refresh() coroutine currently calls mirrorRepository.refreshCatalog() without error handling so exceptions can leave _state.isRefreshing stuck true and provide no user feedback; wrap the call inside a try-catch inside viewModelScope.launch, set _state.update { it.copy(isRefreshing = false) } in a finally block to ensure the flag is cleared, and in the catch block emit an error event or update state with an error message (via the same _state or an event channel) so callers of MirrorPickerViewModel see the failure; reference refresh(), viewModelScope.launch, mirrorRepository.refreshCatalog(), _state.update and ensure the finally always clears isRefreshing.core/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/DesktopDigestVerifier.kt (1)
23-26: 💤 Low valuePrefer idiomatic EOF check
< 0instead of<= 0.While functionally equivalent here (buffer size is non-zero), the standard idiom for
InputStream.read()EOF detection isread < 0orread == -1. This improves readability and aligns with Java conventions.♻️ Suggested fix
while (true) { val read = stream.read(buf) - if (read <= 0) break + if (read < 0) break digest.update(buf, 0, read) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/DesktopDigestVerifier.kt` around lines 23 - 26, Update the EOF check in DesktopDigestVerifier's read loop to use the idiomatic InputStream EOF detection (read < 0 or read == -1) instead of using `read <= 0`; locate the loop where `val read = stream.read(buf)` and replace the `if (read <= 0) break` check with `if (read < 0) break` to follow Java/Kotlin conventions and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.kt`:
- Around line 312-314: The current navigation calls to
navController.navigate(GithubStoreGraph.MirrorPickerScreen) can push duplicate
MirrorPickerScreen entries; update both call sites (the one using
onNavigateToMirrorPicker around the navController.navigate call and the second
occurrence mentioned at lines 414-417) to pass launchSingleTop = true in the
navigate options so repeated taps will not create duplicate destinations in the
back stack; locate the two
navController.navigate(GithubStoreGraph.MirrorPickerScreen) invocations and add
the launchSingleTop flag in their navigation options.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/download/MultiSourceDownloaderImpl.kt`:
- Around line 41-56: The flow builder in MultiSourceDownloaderImpl launches
child coroutines (e.g., the directJob launched around downloader.download and
using emit(progress)) which violates Flow's context preservation; replace the
outer flow { ... } with channelFlow { ... } and inside the launched coroutines
use send(progress) instead of emit(progress), keep the winnerSignal checks and
the same exception handling logic (e.g., in the directJob/cachedJob catch blocks
rethrow only if winnerSignal reports that source) and add the
kotlinx.coroutines.flow.channelFlow import; this ensures concurrent emissions
from the child coroutines (directJob, cachedJob) are safe and preserves the
original winnerSignal behavior.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/download/SlowDownloadDetectorImpl.kt`:
- Around line 44-59: The mutex.withLock in SlowDownloadDetectorImpl currently
holds the lock while potentially calling recordSlowEvent which does I/O
(preferences.data.first()), causing contention; change the critical section to
only update and trim the in-memory samples and compute the decision variables
(e.g., first, last, elapsedSec, deltaBytes, bytesPerSec, windowFull) inside
mutex.withLock, then capture a simple boolean or necessary snapshot (e.g., now
and decision flag) and release the lock; after the lock, if the captured flag
indicates a slow event, call recordSlowEvent(now) (and any preference/proxy
checks) outside the lock so preference.data.first() is not invoked while holding
the mutex.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/MirrorRepositoryImpl.kt`:
- Around line 64-70: Before overwriting _catalog, capture its current value into
a local variable (e.g., val previousCatalog = _catalog.value) and use
previousCatalog when resolving removed-mirror display names (instead of using
configs/new value); then assign _catalog.value = configs and proceed to
preferences update and checkSelectedMirrorStillExists(configs). Make the same
change at the other update site that sets _catalog (the block that currently
updates and immediately handles removed mirrors around
checkSelectedMirrorStillExists) so removed-mirror handling can look up names
from previousCatalog rather than the new configs.
- Around line 64-65: MirrorRepositoryImpl currently persists remote mirror
entries without rejecting reserved sentinel IDs ("direct", "__custom__"), which
can collide with observePreference's control-flow decoding; update the code so
that when building configs from response.mirrors (used to set _catalog.value)
you first map each entry via toDomain() and then filter out any with id in the
reserved set, and apply the same filtering when decoding/consuming mirror IDs
inside observePreference so preference decoding ignores any remote entries whose
id equals "direct" or "__custom__"; reference MirrorRepositoryImpl, the _catalog
assignment, and the observePreference logic when making these changes.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.kt`:
- Line 103: The mirror-rewrite interceptor enabled by installMirrorRewrite() can
rewrite request URLs to non-GitHub hosts but the client still attaches
Authorization; update the interceptor implementation used by HttpClientFactory
so it detects when the request host changes during rewrite (compare
originalUrl.host vs rewrittenUrl.host in the installMirrorRewrite interceptor)
and remove the Authorization header (or clear
call.request.headers[HttpHeaders.Authorization]) before proceeding if the hosts
differ; ensure this logic runs immediately after URL rewrite and before the
request is dispatched so tokens are never sent to rewritten/non-GitHub hosts.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/ProxyManager.kt`:
- Line 3: The import of java.util.concurrent.atomic.AtomicReference in
ProxyManager is JVM-only; replace it with a KMP-safe atomic from
kotlinx.atomicfu: remove the java import, add kotlinx-atomicfu as a dependency,
and change the AtomicReference<T> usage in ProxyManager (the atomic reference
variable(s) and any compareAndSet/get/set calls) to use atomicfu's
atomic(initial) and .value (or atomicfu's compareAndSet) APIs so the logic
(reads, writes, CAS) behaves the same across targets; alternatively, if you
prefer no library, make the field `@Volatile` and implement a small synchronized
wrapper around get/set/CAS in the ProxyManager class.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.kt`:
- Line 261: In DefaultDownloadOrchestrator (the runCatching call that currently
uses java.io.File(filePath).delete()), replace the JVM-only java.io.File usage
with a KMP-safe abstraction: either call an injected platform
FileDeleter/FileSystem service or an expect/actual function (e.g., expect fun
deleteFile(path: String): Boolean) and call that inside runCatching, or use
okio.FileSystem if available; update the constructor or class to accept the
injected file utility and replace references to java.io.File(filePath).delete()
with the platform-agnostic deleteFile/fileSystem.delete call so compilation
succeeds on non-JVM targets.
In `@core/presentation/src/commonMain/composeResources/values/strings.xml`:
- Line 868: The current mirror_picker_description string incorrectly states the
mirror proxies GitHub API calls; update the string (mirror_picker_description)
to accurately describe that the mirror only rewrites download hosts for release
assets and explicitly does not proxy GitHub API calls (i.e., it affects
downloads only, not API requests). Make the wording concise and user-facing,
e.g. explain it's used for downloading release assets and that API calls remain
direct to GitHub.
In `@docs/superpowers/specs/2026-04-29-e5-mirror-system-design.md`:
- Line 41: There are four unlabeled fenced code blocks in the spec that trigger
markdownlint MD040; locate each triple-backtick block used for examples and add
an appropriate language identifier (e.g., text, kotlin) immediately after the
opening ``` so each block becomes ```text or ```kotlin, ensuring all unlabeled
fenced blocks are tagged to satisfy MD040.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/AutoSuggestMirrorViewModel.kt`:
- Around line 11-47: Refactor AutoSuggestMirrorViewModel to follow the
State/Action/Event pattern: replace the raw _isVisible/isVisible with a single
MutableStateFlow<State> where State is a data class (e.g., AutoSuggestState(val
isVisible: Boolean)), add a sealed interface Action (e.g., MaybeLater,
DontAskAgain, PickOneClicked, Dismiss) and expose a public handleAction(action:
Action) method that performs what the current public methods do (call
mirrorRepository.snoozeAutoSuggest or dismissAutoSuggestPermanently and update
state), and add a Channel<Event> (or MutableSharedFlow) for one-off outcomes
(e.g., ShowSnoozedConfirmation, ShowPickedMirror) which you emit instead of
returning via methods; also wire the detector.suggestMirror collector to update
the State.isVisible inside viewModelScope.launch rather than setting _isVisible
directly. Ensure to keep existing calls to mirrorRepository.snoozeAutoSuggest
and dismissAutoSuggestPermanently inside actions handling.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerRoot.kt`:
- Around line 98-101: In MirrorPickerRoot update the Icon call that currently
sets contentDescription = null (Icons.AutoMirrored.Filled.ArrowBack) so it
provides an accessible, localized label; replace the null with a call to
stringResource (e.g., stringResource(R.string.back) or a suitable key like
R.string.navigate_back) and add the matching string resource entry if missing so
the back button is announced to assistive technologies.
---
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/ProxyManager.kt`:
- Around line 63-64: The idempotency check in startMirrorCollector on the
mirrorCollectorJob field is non-atomic and can race; wrap the check-and-assign
in a mutual-exclusion block (e.g., add a Mutex like mirrorCollectorMutex and
perform the if (mirrorCollectorJob?.isActive == true) return and subsequent
mirrorCollectorJob = ... inside mirrorCollectorMutex.withLock { ... } or use a
synchronized(this) block) so only one coroutine/thread can observe-and-create
the job; update the startMirrorCollector method to acquire the mutex before
checking mirrorCollectorJob and releasing it after assignment to prevent
duplicate collectors.
In
`@core/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/DesktopDigestVerifier.kt`:
- Around line 23-26: Update the EOF check in DesktopDigestVerifier's read loop
to use the idiomatic InputStream EOF detection (read < 0 or read == -1) instead
of using `read <= 0`; locate the loop where `val read = stream.read(buf)` and
replace the `if (read <= 0) break` check with `if (read < 0) break` to follow
Java/Kotlin conventions and improve readability.
In `@feature/tweaks/presentation/build.gradle.kts`:
- Line 19: The buildscript currently declares the Ktor client as
api(libs.ktor.client.core) which unnecessarily exposes Ktor types to downstream
modules; change that to implementation(libs.ktor.client.core). Locate the
dependency declaration in build.gradle.kts and replace api(...) with
implementation(...); this aligns with how the Ktor HttpClient is used privately
in MirrorPickerViewModel.kt (testHttpClient: HttpClient) and prevents leaking
Ktor types transitively.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.kt`:
- Line 16: MirrorPickerViewModel imports the data-layer class MirrorRewriter,
violating the presentation→domain dependency rule; remove the direct dependency
by either (A) moving MirrorRewriter (or its template-application logic) into the
domain module as a domain service, or (B) create a domain use-case (e.g.,
ApplyMirrorTemplateUseCase) that exposes a method used by MirrorPickerViewModel;
update MirrorPickerViewModel to depend only on the new domain use-case or domain
service (referencing MirrorPickerViewModel and MirrorRewriter by name) and
adjust DI/constructors to inject the domain abstraction instead of the
data-layer class.
- Around line 139-145: The refresh() coroutine currently calls
mirrorRepository.refreshCatalog() without error handling so exceptions can leave
_state.isRefreshing stuck true and provide no user feedback; wrap the call
inside a try-catch inside viewModelScope.launch, set _state.update {
it.copy(isRefreshing = false) } in a finally block to ensure the flag is
cleared, and in the catch block emit an error event or update state with an
error message (via the same _state or an event channel) so callers of
MirrorPickerViewModel see the failure; reference refresh(),
viewModelScope.launch, mirrorRepository.refreshCatalog(), _state.update and
ensure the finally always clears isRefreshing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc126eb7-c15e-44a1-9596-e667d01a42bf
📒 Files selected for processing (54)
composeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/di/ViewModelsModule.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/AppNavigation.ktcomposeApp/src/commonMain/kotlin/zed/rainxch/githubstore/app/navigation/GithubStoreGraph.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/di/PlatformModule.android.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/network/AndroidDigestVerifier.ktcore/data/src/androidMain/kotlin/zed/rainxch/core/data/services/AndroidDownloader.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/di/SharedModule.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/download/MultiSourceDownloaderImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/download/SlowDownloadDetectorImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/AssetNetwork.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/dto/MirrorListResponse.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/mappers/AssetNetwork.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/BundledMirrors.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/MirrorPersistence.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/MirrorRepositoryImpl.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/HttpClientFactory.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/MirrorApiClient.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/MirrorRewriteInterceptor.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/MirrorRewriter.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/ProxyManager.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/di/PlatformModule.jvm.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/network/DesktopDigestVerifier.ktcore/data/src/jvmMain/kotlin/zed/rainxch/core/data/services/DesktopDownloader.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/GithubAsset.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/MirrorConfig.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/MirrorPreference.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/MirrorStatus.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/model/MirrorType.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/network/DigestVerifier.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/network/Downloader.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/network/SlowDownloadDetector.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/MirrorRepository.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/system/MultiSourceDownloader.ktcore/presentation/src/commonMain/composeResources/values/strings.xmldocs/superpowers/plans/2026-04-29-e5-mirror-system.mddocs/superpowers/specs/2026-04-29-e5-mirror-system-design.mdfeature/tweaks/presentation/build.gradle.ktsfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksAction.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksRoot.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/TweaksViewModel.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/components/sections/Network.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/AutoSuggestMirrorViewModel.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerAction.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerEvent.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerRoot.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerState.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/components/AutoSuggestMirrorSheet.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/components/CustomMirrorDialog.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/components/DeployYourOwnHint.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/components/MirrorRow.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/components/StatusDot.kt
Comment on lines
+44
to
+59
| mutex.withLock { | ||
| val now = Clock.System.now().toEpochMilliseconds() | ||
| samples.addLast(now to progress.bytesDownloaded) | ||
| while (samples.isNotEmpty() && samples.first().first < now - sustainedMs) { | ||
| samples.removeFirst() | ||
| } | ||
| if (samples.size >= 2) { | ||
| val first = samples.first() | ||
| val last = samples.last() | ||
| val elapsedSec = (last.first - first.first).coerceAtLeast(1L) / 1000.0 | ||
| val deltaBytes = (last.second - first.second).coerceAtLeast(0L) | ||
| val bytesPerSec = (deltaBytes / elapsedSec).toLong() | ||
| val windowFull = (last.first - first.first) >= sustainedMs - 500 | ||
| if (windowFull && bytesPerSec < thresholdBytesPerSec) { | ||
| recordSlowEvent(now) | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid suspending I/O while holding the hot-path mutex.
At Line 44, mutex.withLock wraps logic that can reach preferences.data.first() via recordSlowEvent (Lines 58-59), which serializes progress handling and can backlog onProgress jobs during fast updates. Keep the lock scope to in-memory queue math only, then perform preference/proxy checks outside the critical section.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/download/SlowDownloadDetectorImpl.kt`
around lines 44 - 59, The mutex.withLock in SlowDownloadDetectorImpl currently
holds the lock while potentially calling recordSlowEvent which does I/O
(preferences.data.first()), causing contention; change the critical section to
only update and trim the in-memory samples and compute the decision variables
(e.g., first, last, elapsedSec, deltaBytes, bytesPerSec, windowFull) inside
mutex.withLock, then capture a simple boolean or necessary snapshot (e.g., now
and decision flag) and release the lock; after the lock, if the captured flag
indicates a slow event, call recordSlowEvent(now) (and any preference/proxy
checks) outside the lock so preference.data.first() is not invoked while holding
the mutex.
Comment on lines
+64
to
+65
| val configs = response.mirrors.map { it.toDomain() } | ||
| _catalog.value = configs |
Contributor
There was a problem hiding this comment.
Reject reserved sentinel IDs from remote mirror entries.
Backend mirror IDs are persisted directly (Line 64), but observePreference reserves special IDs (direct, __custom__) for control flow (Lines 75-83). If a remote entry collides with a sentinel, preference decoding becomes ambiguous and can switch behavior incorrectly.
Also applies to: 75-83
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/MirrorRepositoryImpl.kt`
around lines 64 - 65, MirrorRepositoryImpl currently persists remote mirror
entries without rejecting reserved sentinel IDs ("direct", "__custom__"), which
can collide with observePreference's control-flow decoding; update the code so
that when building configs from response.mirrors (used to set _catalog.value)
you first map each entry via toDomain() and then filter out any with id in the
reserved set, and apply the same filtering when decoding/consuming mirror IDs
inside observePreference so preference decoding ignores any remote entries whose
id equals "direct" or "__custom__"; reference MirrorRepositoryImpl, the _catalog
assignment, and the observePreference logic when making these changes.
| val mismatch = digestVerifier.verify(filePath, expectedDigest) | ||
| if (mismatch != null) { | ||
| Logger.w { "Orchestrator: digest mismatch for ${spec.asset.name}: $mismatch" } | ||
| runCatching { java.io.File(filePath).delete() } |
Contributor
There was a problem hiding this comment.
Platform-specific java.io.File used in commonMain code.
java.io.File is JVM-specific and will fail compilation on non-JVM targets (iOS, JS, etc.). Since this is in commonMain, use an expect/actual pattern or a KMP-compatible file deletion abstraction.
🔧 Suggested approach
Either inject a platform-agnostic file utility or use expect/actual:
- runCatching { java.io.File(filePath).delete() }
+ runCatching { fileSystem.delete(filePath) }Or use okio.FileSystem which is KMP-compatible if already a dependency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/services/DefaultDownloadOrchestrator.kt`
at line 261, In DefaultDownloadOrchestrator (the runCatching call that currently
uses java.io.File(filePath).delete()), replace the JVM-only java.io.File usage
with a KMP-safe abstraction: either call an injected platform
FileDeleter/FileSystem service or an expect/actual function (e.g., expect fun
deleteFile(path: String): Boolean) and call that inside runCatching, or use
okio.FileSystem if available; update the constructor or class to accept the
injected file utility and replace references to java.io.File(filePath).delete()
with the platform-agnostic deleteFile/fileSystem.delete call so compilation
succeeds on non-JVM targets.
|
|
||
| **New files:** | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Add language identifiers to fenced code blocks (MD040).
Lines 41, 90, 216, and 355 use unlabeled fenced blocks. Please tag them (for example text or kotlin) to satisfy markdownlint and keep docs CI clean.
Also applies to: 90-90, 216-216, 355-355
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/superpowers/specs/2026-04-29-e5-mirror-system-design.md` at line 41,
There are four unlabeled fenced code blocks in the spec that trigger
markdownlint MD040; locate each triple-backtick block used for examples and add
an appropriate language identifier (e.g., text, kotlin) immediately after the
opening ``` so each block becomes ```text or ```kotlin, ensuring all unlabeled
fenced blocks are tagged to satisfy MD040.
Comment on lines
+11
to
+47
| class AutoSuggestMirrorViewModel( | ||
| private val detector: SlowDownloadDetector, | ||
| private val mirrorRepository: MirrorRepository, | ||
| ) : ViewModel() { | ||
| private val _isVisible = MutableStateFlow(false) | ||
| val isVisible = _isVisible.asStateFlow() | ||
|
|
||
| init { | ||
| viewModelScope.launch { | ||
| detector.suggestMirror.collect { | ||
| _isVisible.value = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fun onMaybeLater() { | ||
| _isVisible.value = false | ||
| viewModelScope.launch { | ||
| mirrorRepository.snoozeAutoSuggest(24L * 60 * 60 * 1000) | ||
| } | ||
| } | ||
|
|
||
| fun onDontAskAgain() { | ||
| _isVisible.value = false | ||
| viewModelScope.launch { | ||
| mirrorRepository.dismissAutoSuggestPermanently() | ||
| } | ||
| } | ||
|
|
||
| fun onPickOneClicked() { | ||
| _isVisible.value = false | ||
| } | ||
|
|
||
| fun dismiss() { | ||
| _isVisible.value = false | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Align this ViewModel with the required State/Action/Event contract.
This ViewModel uses direct public methods instead of the project-standard Action/Event flow pattern. Please expose a state model, handle input via a sealed action type, and emit one-off outcomes through an event channel.
♻️ Refactor sketch
+data class AutoSuggestMirrorState(
+ val isVisible: Boolean = false,
+)
+
+sealed interface AutoSuggestMirrorAction {
+ data object MaybeLater : AutoSuggestMirrorAction
+ data object DontAskAgain : AutoSuggestMirrorAction
+ data object PickOne : AutoSuggestMirrorAction
+ data object Dismiss : AutoSuggestMirrorAction
+}
+
+sealed interface AutoSuggestMirrorEvent {
+ data object NavigateToMirrorPicker : AutoSuggestMirrorEvent
+}As per coding guidelines, "Implement State/Action/Event pattern for all ViewModels: state in MutableStateFlow, actions as sealed interface, events in Channel".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/AutoSuggestMirrorViewModel.kt`
around lines 11 - 47, Refactor AutoSuggestMirrorViewModel to follow the
State/Action/Event pattern: replace the raw _isVisible/isVisible with a single
MutableStateFlow<State> where State is a data class (e.g., AutoSuggestState(val
isVisible: Boolean)), add a sealed interface Action (e.g., MaybeLater,
DontAskAgain, PickOneClicked, Dismiss) and expose a public handleAction(action:
Action) method that performs what the current public methods do (call
mirrorRepository.snoozeAutoSuggest or dismissAutoSuggestPermanently and update
state), and add a Channel<Event> (or MutableSharedFlow) for one-off outcomes
(e.g., ShowSnoozedConfirmation, ShowPickedMirror) which you emit instead of
returning via methods; also wire the detector.suggestMirror collector to update
the State.isVisible inside viewModelScope.launch rather than setting _isVisible
directly. Ensure to keep existing calls to mirrorRepository.snoozeAutoSuggest
and dismissAutoSuggestPermanently inside actions handling.
Comment on lines
+98
to
+101
| Icon( | ||
| imageVector = Icons.AutoMirrored.Filled.ArrowBack, | ||
| contentDescription = null, | ||
| ) |
Contributor
There was a problem hiding this comment.
Provide an accessible label for the back button icon.
At Line 100, contentDescription = null leaves the navigation action unlabeled for assistive tech. Add a localized back/navigation description.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerRoot.kt`
around lines 98 - 101, In MirrorPickerRoot update the Icon call that currently
sets contentDescription = null (Icons.AutoMirrored.Filled.ArrowBack) so it
provides an accessible, localized label; replace the null with a call to
stringResource (e.g., stringResource(R.string.back) or a suitable key like
R.string.navigate_back) and add the matching string resource entry if missing so
the back button is announced to assistive technologies.
…longer imports data
…ources Standardizes string syntax across `strings.xml` and localized versions for Turkish, French, and Italian. Replaces escaped single quotes (`\'`) with unescaped apostrophes (`'`) in various UI strings, including language picker descriptions, Personal Access Token instructions, and telemetry settings. This is a syntax-only cleanup that does not change the translated text.
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 by CodeRabbit