Conversation
- Detect user timezone to determine if likely in China - CN users fetch release assets from GitLab (lay2dev/clawpal) - Other users fetch from GitHub as before - Falls back to secondary source if primary fails
📦 PR Build Artifacts
|
📊 Test Coverage Report
Coverage measured by |
dev01lay2
left a comment
dev01lay2
left a comment
There was a problem hiding this comment.
PR #112 Review — Geo-aware download links
The overall approach is solid — timezone-based routing with GitLab as the CN mirror and a graceful fallback chain is exactly the right pattern. One blocking issue to address before merge:
🔴 BS: GitLab assets.links[].name is a label, not a filename
The GitLab Releases API returns assets.links where each link.name is a human-readable label set by the release creator (e.g. "macOS ARM64 Installer"), not the filename. The ASSET_MATCHERS all test name.endsWith('_aarch64.dmg') etc., so they'll silently miss every asset if the GitLab project uses descriptive link names.
For CN users this is the worst-case failure path: they can't reach GitHub API (that's why the mirror exists), GitLab assets all fail to match, buttons go grey, and the fallback link is GITLAB_RELEASES — which they can reach, so it's not catastrophic. But greyed download buttons are a confusing UX.
Fix options:
- Match on
urlinstead ofname(the download URL typically contains the filename):a => a.url.includes('_aarch64.dmg') - Or document that GitLab release links must be named with the exact filename as the label, and enforce that in the release process.
🟡 NBS: Redundant/invalid timezone entries in isLikelyCN()
Asia/Chongqing is a deprecated IANA alias for Asia/Shanghai — Intl.DateTimeFormat normalizes it to Asia/Shanghai, so the pattern never matches Chongqing. Similarly Asia/Harbin was removed from the IANA database and rolled into Asia/Shanghai. Both are harmless (covered by Shanghai) but add noise to the regex.
| function applyAssets(assetList, fallback) { | ||
| for (const [id, matcher] of Object.entries(ASSET_MATCHERS)) { | ||
| const el = document.getElementById(id); | ||
| if (!el) continue; |
There was a problem hiding this comment.
BS: release.assets?.links on GitLab returns objects where .name is the human-readable display label, not the filename. If the GitLab project's release links are named descriptively (e.g. "macOS ARM64") rather than with the raw filename, every matcher here will fail and CN users get greyed-out buttons.
Consider matching on .url which reliably contains the filename:
.then(release => (release.assets?.links || []).map(a => ({ name: a.url.split('/').pop() || a.name, url: a.url })));This extracts the filename from the URL, keeping matcher logic identical.
| } | ||
| }); | ||
| function isLikelyCN() { | ||
| try { |
There was a problem hiding this comment.
NBS: Asia/Chongqing and Asia/Harbin are deprecated IANA aliases — modern browsers normalize both to Asia/Shanghai. They'll never fire independently. Simplify to:
return /^Asia\/(Shanghai|Urumqi)$/.test(tz);
dev01lay2
left a comment
dev01lay2
left a comment
There was a problem hiding this comment.
PR #112 Re-review — Geo-aware download links (commit 31e0376)
The mirror-release.yml improvements are solid — skipping .sig/latest.json and adding --http1.1 for large-file stability both make sense. 👍
CI failures (cargo fmt on doctor.rs, missing TS types) are pre-existing base-branch issues, not caused by this PR.
🔴 BS (still unresolved): GitLab assets.links[].name is a human-readable label, not a filename
fetchGitLab() still maps a.name into the asset object:
(release.assets?.links || []).map(a => ({ name: a.name, url: a.url }))And ASSET_MATCHERS still matches on name.endsWith('_aarch64.dmg') etc. The GitLab Releases API assets.links[].name field is the label the release creator set (e.g. "macOS ARM64 Installer") — not the filename. These matchers will silently fail to match any GitLab asset, leaving all download buttons greyed-out for CN users.
Fix: match on url instead of name — the download URL reliably contains the filename:
// fetchGitLab: map url → name for matcher
(release.assets?.links || []).map(a => ({ name: a.url, url: a.url }))Or update applyAssets to try both name and url:
const asset = assetList.find(a =>
(matcher(a.name) || matcher(a.url)) && isTrustedUrl(a.url)
);Either approach works; the second is more robust without changing the GitLab fetch shape.
🟡 NBS (carry-over): Deprecated timezone entries in isLikelyCN()
Asia/Chongqing and Asia/Harbin are both deprecated IANA aliases that Intl.DateTimeFormat normalizes to Asia/Shanghai — they'll never match. Harmless (covered by Shanghai) but dead code.
| if (!asset) el.style.opacity = '0.4'; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
BS (unresolved): a.name here is the GitLab link label (set by the release creator), not the filename. The ASSET_MATCHERS (e.g. name.endsWith('_aarch64.dmg')) will never match a label like "macOS ARM64 Installer". CN users will see greyed-out download buttons.
Simplest fix — match on URL in the mapper:
(release.assets?.links || []).map(a => ({ name: a.url, url: a.url }))Or check both in applyAssets:
const asset = assetList.find(a => (matcher(a.name) || matcher(a.url)) && isTrustedUrl(a.url));2 participants
改动
下载链接根据用户地理位置自动分流：
实现方式
Asia/Shanghai等）判断是否在中国