fix(mirror): handle path-based template in test button (jsDelivr 400)#725
Conversation
…fix) User report (issue #698 follow-up): fastly.jsdelivr.net status dot shows ok / 42ms but tapping the 'Test mirror' button returns 'Mirror returned 400'. Root cause: the test code does template.replace("{url}", probeUrl), which is correct for whole-URL proxies like ghfast.top/{url}. But jsDelivr's template is https://fastly.jsdelivr.net/gh/{owner}/{repo}@{ref}/{path} — no {url} placeholder. The replace is a no-op and the literal braces hit jsDelivr, which returns 400 for the invalid path. Backend probe gets this right via a hardcoded cli/cli@v2.40.0/LICENSE URL. This patch teaches the client test to do the same substitution (cli/cli/v2.40.0/LICENSE) when the template lacks {url} so both shapes of mirror template are exercised correctly.
|
Looking for one thing? Review this PR in Change Stack to search files, summaries, diffs, and code without losing your place. 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 (2)
WalkthroughMirrorPickerViewModel.runTest() now builds test probe URLs from whole-URL ({url}) or decomposed ({owner}/{repo}/{ref}/{path}) templates with a wholeUrlProbe fallback and uses 5_000L.milliseconds for timeout. BundledMirrors records TrafficKind sets per mirror and adds a fastly_jsdelivr raw-file-only entry; MirrorRepositoryImpl drops the private val on appScope and has formatting-only adjustments. ChangesMirror URL Test Probe Dual Template Support
Bundled Mirrors TrafficKind Metadata & MirrorRepository Formatting
Sequence Diagram(s) sequenceDiagram
participant ViewModel as MirrorPickerViewModel
participant Template as MirrorTemplate
participant Network as NetworkClient
ViewModel->>Template: determine template shape (null / contains {url} / contains {owner})
ViewModel->>ViewModel: build targetUrl (use wholeUrlProbe or substitute placeholders)
ViewModel->>Network: send probe request to targetUrl (timeout 5_000L.milliseconds)
Network-->>ViewModel: response / HTTP error / DNS failure / other error
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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. Comment |
BundledMirrors is the catalog served when the backend's /v1/mirrors/list is unreachable. Backend MirrorPresets.ALL lists fastly_jsdelivr but the client bundle was never updated to match, so users running offline (or with a misconfigured network) saw a strictly smaller picker than users online — and lost access to jsDelivr entirely. Also extends the entry() helper with a trafficKinds parameter so this raw-file-only mirror is tagged correctly (release tarballs aren't served under jsDelivr's /gh/ path). Defaults to release_asset + raw_file for the whole-URL-proxy mirrors so the existing six entries keep their previous semantics.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.kt`:
- Around line 123-131: The else branch currently assumes any non-null, non-{url}
template should be decomposed into {owner}/{repo}/{ref}/{path}; instead, mimic
MirrorRewriter.applyTemplate(...) by first checking for the presence of the
decomposed marker (e.g. template.contains("{owner}")) before performing the
chained .replace calls — if the decomposed marker is missing return the template
as-is (or use wholeUrlProbe where appropriate); update the when branch handling
in MirrorPickerViewModel (the template/wholeUrlProbe logic) to guard the
replacement sequence with template.contains("{owner}") to avoid treating
malformed/unknown templates as decomposed.
🪄 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: b2657865-72e1-49f2-8272-fee14f99fdfa
📒 Files selected for processing (1)
feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.kt
Greptile SummaryThis PR fixes a 400 error when using the Test mirror button on
Confidence Score: 5/5Safe to merge — the targeted fix is correct, and the only inconsistency introduced is a cosmetic ordering difference between the cached and live-refreshed catalog. The core change — branching on template shape in
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[runTest called] --> B{preference type?}
B -->|Direct| C[template = null]
B -->|Custom| D[template = custom string]
B -->|Selected| E[template = catalog lookup]
C --> F{template null?}
D --> F
E --> F
F -->|Yes| G[targetUrl = wholeUrlProbe]
F -->|No| H{contains url?}
H -->|Yes| I[replace url with raw.githubusercontent.com probe]
H -->|No| J{contains owner?}
J -->|Yes| K[replace owner=cli, repo=cli, ref=v2.40.0, path=LICENSE]
J -->|No| G
G --> L[GET targetUrl withTimeoutOrNull 5s]
I --> L
K --> L
L -->|null| M[TestResult.Timeout]
L -->|UnresolvedAddress| N[TestResult.DnsFailure]
L -->|other exception| O[TestResult.Other]
L -->|200-299| P[TestResult.Success ms]
L -->|other status| Q[TestResult.HttpError status]
Reviews (3): Last reviewed commit: "Show active proxies first + some style r..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.kt (1)
130-143: 💤 Low valueTemplate-based probe URL construction correctly addresses path-based templates.
The logic now properly guards decomposed substitution with
template.contains("{owner}")(line 136) and provides awholeUrlProbefallback for unknown template shapes (line 142), fully addressing the past review concern.
Optional: Consider aligning
wholeUrlProbewith backend probe resource.The
wholeUrlProbeusesoctocat/Hello-World/master/READMEwhile the decomposed path usescli/cli@v2.40.0/LICENSE(matching the backend probe per the PR description). While the PR states {url} templates keep "existing behavior," usingcli/cli@v2.40.0/LICENSEfor both would improve consistency with the backend probe and make cross-template comparison easier.Suggested alignment
val wholeUrlProbe = - "https://raw.githubusercontent.com/octocat/Hello-World/master/README" + "https://raw.githubusercontent.com/cli/cli/v2.40.0/LICENSE"🤖 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 `@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.kt` around lines 130 - 143, The current probe uses wholeUrlProbe = "https://raw.githubusercontent.com/octocat/Hello-World/master/README" which mismatches the decomposed substitution values used in the template branch; update wholeUrlProbe so it aligns with the backend probe and the decomposed branch (e.g. use the cli/cli@v2.40.0/LICENSE raw URL) so targetUrl construction (the when block that checks template == null, template.contains("{url}"), and template.contains("{owner}")) produces consistent probe URLs across template shapes.
🤖 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.
Nitpick comments:
In
`@feature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.kt`:
- Around line 130-143: The current probe uses wholeUrlProbe =
"https://raw.githubusercontent.com/octocat/Hello-World/master/README" which
mismatches the decomposed substitution values used in the template branch;
update wholeUrlProbe so it aligns with the backend probe and the decomposed
branch (e.g. use the cli/cli@v2.40.0/LICENSE raw URL) so targetUrl construction
(the when block that checks template == null, template.contains("{url}"), and
template.contains("{owner}")) produces consistent probe URLs across template
shapes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1554ea8d-5c42-4540-a3c6-4f962202ed2d
📒 Files selected for processing (3)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/BundledMirrors.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/MirrorRepositoryImpl.ktfeature/tweaks/presentation/src/commonMain/kotlin/zed/rainxch/tweaks/presentation/mirror/MirrorPickerViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/data/src/commonMain/kotlin/zed/rainxch/core/data/mirror/BundledMirrors.kt
1 participant
Follow-up to #698 / OpenHub-Store/backend#25.
Symptom
`fastly.jsdelivr.net` shows status dot ok / 42ms, but tapping Test mirror returns "Mirror returned 400".
Root cause
`MirrorPickerViewModel.runTest()` does:
```kotlin
template.replace("{url}", probeUrl)
```
Correct for whole-URL proxies like `ghfast.top/{url}`. But jsDelivr's template is
```
https://fastly.jsdelivr.net/gh/{owner}/{repo}@{ref}/{path}
```
— no `{url}` placeholder. The replace is a no-op and the literal `{owner}`/`{repo}`/etc. braces hit jsDelivr as the request path, which returns 400.
Backend hourly probe (`MirrorStatusWorker.PROBE_ASSET`) gets this right via a hardcoded `cli/cli@v2.40.0/LICENSE` URL — which is why the status dot reports OK while the in-app test fails.
Fix
`runTest` now branches on which placeholder shape the template uses:
Summary by CodeRabbit
Bug Fixes
New Features