fix(desktop): accept uppercase icon URL schemes - #5470
Conversation
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Review Summary
Scope: 8 files, +177/-31 — case-insensitive icon URL scheme detection for the desktop app.
What works well:
- The regex
/^https?:\/\//icorrectly handles case-insensitive http/https matching - All 9 inline
startsWithchecks across 5 files are properly replaced with the centralizedisIconUrl()predicate - The
useRefremoval inuseWorkspaceIconis a correct cleanup (it was dead code) - All 21 tests pass (icon-constants, workspace-icon-url, icon-cache)
- Defense in depth:
openUrlinSourceInfoPagestill passes throughisSafeExternalUrl()downstream - No security issues detected — no SSRF, no path traversal, no credential exposure
Minor observations (not blockers):
isIconUrl()inSourceInfoPage.tsx:322handleOpenUrl is used on a general source URL (not an icon URL). Functionally correct, but the name may surprise future readers ifisIconUrlever gains icon-specific validation.isIconUrl(str: string)has a stricter signature than the siblingisEmoji(str: string | undefined). All call sites guard correctly, but consistency with the established pattern would be more defensive.
CI: 16/16 checks passing.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @tt-a1i! The fix itself looks well-targeted, but the PR body doesn't follow our PR template, which blocks proper review.
A few things to fix:
- Headings — the template expects
## What this PR doesand## Reviewer Test Plan, but the PR uses## Summaryand## Test plan. Please use the template headings so reviewers can find things quickly. - Missing sections —
## Why it's needed,## Risk & Scope,### Tested on(OS matrix), and### Evidence (Before & After)are all absent. Even short answers are fine — they help reviewers assess impact and risk. - 中文说明 — the template has a
<details>中文说明</details>block for a bilingual translation. Please include it. - Linked Issues —
Fixes #5469is buried in the Summary; it should be in a## Linked Issuessection so it auto-closes properly.
The actual change (case-insensitive isIconUrl + reusing the shared predicate) is small and well-scoped. Once the template is filled in, this should move through review quickly.
中文说明
感谢 PR,@tt-a1i!修复本身方向正确,但 PR 描述没有按照 PR 模板 填写,会影响审查进度。
需要补充的内容:
- 标题名称 — 模板要求
## What this PR does和## Reviewer Test Plan,当前用的是## Summary和## Test plan,请改为模板中的标题。 - 缺少的章节 —
## Why it's needed、## Risk & Scope、### Tested on(操作系统矩阵)、### Evidence (Before & After)均未填写,即使简短回答也有助于审查。 - 中文说明 — 模板中有
<details>中文说明</details>区块,请补充。 - 关联 Issue —
Fixes #5469放在了 Summary 里面,应放在## Linked Issues区块以便自动关闭。
实际改动(isIconUrl 大小写不敏感 + 复用共享谓函数)小而精准,模板补齐后审查应该很快。
— Qwen Code · qwen3.7-max
wenshao
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
The case-insensitive regex /^https?:\/\//i is correct, all call sites are consistently updated, and tests cover the new behavior at unit, integration, and cache levels.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
updated the PR description to match the template. thanks. |
✅ Local verification — safe to merge (one non-blocking test note)I verified this PR locally before merging. Tested PR head Results — all green
The change is a clean, consistent refactor: Before / after — load-bearing for the predicate and the icon-cache pathsReverting the 5 source files to pre-fix while keeping the new tests, 3 of the new tests fail (and pass after): That is the bug: pre-fix,
|
| 检查项 | 命令 | 结果 |
|---|---|---|
| 单元测试(3 个文件) | bun test icon-constants / workspace-icon-url / icon-cache |
✅ 21 个通过 |
| 类型(shared) | bun run typecheck:shared |
✅ 通过 |
| Lint(electron) | bun x eslint(4 个文件) |
✅ 通过 |
| Lint(shared) | bun x eslint(4 个文件) |
✅ 通过(0 error;2 个 storage.ts 既有告警) |
| 格式 | prettier --check(8 个文件) |
✅ 通过 |
| 空白/冲突 | git diff --check |
✅ 通过 |
这是一次干净、一致的重构:isIconUrl 改为大小写不敏感(/^https?:\/\//i),并把 4 个文件中 9 处内联的 startsWith('http://')||startsWith('https://') 调用点统一改用该共享判定函数。useWorkspaceIcon 中提取的 const workspaceIconUrl = workspace?.iconUrl 不改变行为(仅增加了可选链)。
前后对比 —— 对判定函数与 icon-cache 路径是“关键且必要的”
把 5 个源文件回退到修复前、但保留新增测试,有 3 个新测试失败(修复后通过):
(fail) icon URL detection > treats http and https schemes as case-insensitive
(fail) remote icon URLs > returns source icon URLs with uppercase schemes directly
(fail) remote icon URLs > returns skill icon URLs with uppercase schemes directly
这正是该 bug:修复前 'HTTPS://…'.startsWith('https://') 为 false,大写的远程图标会被拒绝。✔️
⚠️ 不阻塞的发现:workspace-icon-url.test.ts 并非“关键且必要”
该测试(“保留大写远程图标 URL 协议,而不是回退到本地图标”)在修复前与修复后的 storage.ts 上都通过——我把 storage.ts 回退后它仍然通过,因此它并不能捕捉该处的回归。
根因:用例把本地 icon.svg 写到了工作区声明的 rootPath(<configDir>/workspace),但在存在 config-defaults.json 时,getWorkspaces() 会把实际生效的 rootPath 重新解析为 <CONFIG_DIR>/workspaces/<slug>:
effectiveRootPath=/tmp/…/workspaces/a
icon exists at effRoot/icon.svg=false ← 用例把图标写到了 …/workspace,而不是 …/workspaces/a
于是 findWorkspaceIcon() 找不到图标,用例本想触发的“本地图标回退”根本没有发生,断言(URL 不变)无论大小写检查是否敏感都成立。
底层的 storage.ts 改动本身是正确的——它复用了同一个 isIconUrl 判定函数,而该函数有直接的单元测试(关键且必要)。只是这一个集成测试没有真正覆盖它。建议(不阻塞):强化该用例,让“本地图标回退”真正触发(把 icon.svg 放到 getWorkspaces() 实际解析出的目录,或针对该解析后的路径断言)——这样在没有修复时该测试就会失败。
扩展行为验证(我增补的 19 个用例,全部通过)
通过真实的 isIconUrl(无 mock):混合大小写(HtTpS://)、大写的主机/路径/查询串、以及仅含协议头均被接受;file://、FILE://、ftp://、data:、httpsx://、单斜杠 https:/、前导空白、http: 均被拒绝(失败即关闭)。该正则以 ^ 锚定且不做 trim——与此前的 startsWith 行为一致。
关于 typecheck:electron
它确实失败(exit 2),但仅涉及 4 个与本 PR 无关的文件(auto-update.ts、settings-default-thinking.test.ts、session-delete-navigation.test.ts、skills-loading.test.ts);本 PR 改动的 3 个 electron 文件类型干净。这些都是既有问题,与作者的说明一致。
结论
修复正确、精简、一致,核心判定函数也有良好覆盖。可以合并。建议(不阻塞)收紧 workspace-icon-url.test.ts,使 storage.ts 路径真正被守护。LGTM 👍
|
@qwen-code /triage |
|
Thanks for the PR! Template looks good ✓ — all required sections present, bilingual, test plan included. On direction: this is a clear, real bug — uppercase On approach: the scope is tight and minimal. The core change is a single-line fix in 中文说明感谢贡献! 模板完整 ✓ — 所有必需章节齐全,包含双语说明和测试计划。 方向:这是一个真实且明确的 bug — 按照 RFC 3986,大写的 方案:范围紧凑且最小化。核心改动是 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading diff): I would fix Comparison with the diff: the PR matches this exactly — and goes one step further by cleaning up the Findings:
Real-Scenario TestingThis PR is entirely in the desktop Electron renderer process ( The core behavioral evidence was captured by reverting the source files to pre-fix while keeping the new tests — 3 tests fail, confirming the bug and the fix: All 3 pass after the fix. ✔️ 中文说明代码审查独立方案(读 diff 前):把 与 diff 对比: PR 完全匹配该方案——并且进一步清理了 发现:
真实场景测试本 PR 完全位于桌面 Electron 渲染进程( 核心行为证据:将源文件回退到修复前、保留新增测试——3 个测试失败,确认了 bug 和修复的有效性。修复后全部通过。✔️ — Qwen Code · qwen3.7-max |
ReflectionThis is a textbook example of a well-scoped bug fix. The motivation is clear (uppercase URL schemes are valid but rejected), the fix is minimal (one regex change + call-site consolidation), and the tests prove both the bug and the fix. Going back to my independent proposal: the PR matches it exactly and adds a welcome hook cleanup. I wouldn't have done it differently. The code is straightforward — no over-abstraction, no speculative features, no drive-by refactors beyond what touches the same paths. Every change in the diff is needed for the stated goal. The shared The one non-blocking finding (workspace-icon-url test fixture mismatch) was already surfaced by @wenshao and doesn't affect the safety of merging — the After seeing the revert-and-rerun evidence (3 tests fail before, pass after), the results match what the PR promised. If I had to maintain this in six months, I'd thank the author for consolidating 9 scattered Verdict: LGTM — safe to merge. ✅ 中文说明总结这是一个教科书级别的、范围恰当的 bug 修复。动机清晰(大写 URL 协议合法但被拒绝),修复最小化(一行正则改动 + 调用点统一),测试同时证明了 bug 和修复的有效性。 回到我的独立方案:PR 完全匹配,并附带了合理的 hook 清理。我不会用不同的方式来做。 代码简洁明了——没有过度抽象、没有投机性功能、没有超出同一路径范围的顺手重构。diff 中的每一处改动都是为了实现声明的目标。共享的 唯一不阻塞的发现(workspace-icon-url 测试 fixture 路径不匹配)已由 @wenshao 提出,不影响合并安全性—— 看到回退-重跑证据后(修复前 3 个测试失败,修复后通过),结果与 PR 的承诺一致。如果六个月后需要维护这段代码,我会感谢作者把 9 处分散的 结论:LGTM — 可以合并。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
What this PR does
http://andhttps://schemes case-insensitively.Why it's needed
Desktop icon handling currently checks for lowercase
http/httpsschemes in several places. URLs such asHTTPS://example.com/icon.pngare valid, but they can be treated as non-URL icon values and fall back incorrectly. Centralizing the check also keeps the desktop call sites consistent.Reviewer Test Plan
How to verify
HTTP://andHTTPS://icon URLs are accepted by the shared predicate and by workspace icon fallback logic.bun test packages/desktop/packages/shared/src/utils/__tests__/icon-constants.test.ts packages/desktop/packages/shared/src/config/__tests__/workspace-icon-url.test.ts packages/desktop/apps/electron/src/renderer/lib/__tests__/icon-cache.test.ts.bun run typecheck:shared.npx prettier --check packages/desktop/packages/shared/src/utils/icon-constants.ts packages/desktop/packages/shared/src/utils/__tests__/icon-constants.test.ts packages/desktop/packages/shared/src/config/storage.ts packages/desktop/packages/shared/src/config/__tests__/workspace-icon-url.test.ts packages/desktop/apps/electron/src/renderer/hooks/useWorkspaceIcon.ts packages/desktop/apps/electron/src/renderer/lib/icon-cache.ts packages/desktop/apps/electron/src/renderer/lib/__tests__/icon-cache.test.ts packages/desktop/apps/electron/src/renderer/pages/SourceInfoPage.tsx.bun x eslint src/renderer/hooks/useWorkspaceIcon.ts src/renderer/lib/icon-cache.ts src/renderer/lib/__tests__/icon-cache.test.ts src/renderer/pages/SourceInfoPage.tsxfrompackages/desktop/apps/electron.bun x eslint src/utils/icon-constants.ts src/utils/__tests__/icon-constants.test.ts src/config/storage.ts src/config/__tests__/workspace-icon-url.test.tsfrompackages/desktop/packages/shared.git diff --check.Evidence (Before & After)
Before: uppercase HTTP(S) icon URLs were not consistently recognized by lowercase-only
startsWithchecks and could fall through to non-URL icon handling. After: the shared predicate uses case-insensitive scheme matching, and the regression tests cover uppercase URL schemes in the shared predicate, workspace icon fallback, and icon cache paths.Known local validation note:
bun run typecheck:electronstill fails on pre-existingauto-update.tsandsettings-default-thinking.test.tstype errors unrelated to this PR.Tested on
Environment (optional)
Local package tests, typecheck, prettier, eslint, and
git diff --checkon macOS.Risk & Scope
isIconUrlis now shared across more desktop URL-handling paths, so future icon-specific behavior should avoid making the predicate narrower than generic HTTP(S) icon/source URL detection needs.Linked Issues
Fixes #5469
中文说明
What this PR does
http://和https://协议大小写不敏感。Why it's needed
desktop 里有多处只检查小写
http/https协议。HTTPS://example.com/icon.png这类 URL 是合法的,但之前可能被当成非 URL 图标值处理并错误 fallback。把判断集中到共享函数里,也能让 desktop 相关调用点保持一致。Reviewer Test Plan
How to verify
HTTP://和HTTPS://图标 URL 会被共享判断函数和 workspace 图标 fallback 逻辑接受。bun test packages/desktop/packages/shared/src/utils/__tests__/icon-constants.test.ts packages/desktop/packages/shared/src/config/__tests__/workspace-icon-url.test.ts packages/desktop/apps/electron/src/renderer/lib/__tests__/icon-cache.test.ts。bun run typecheck:shared。git diff --check命令。Evidence (Before & After)
修复前:大写 HTTP(S) 图标 URL 不能被小写限定的
startsWith检查稳定识别,可能落到非 URL 图标处理逻辑。修复后:共享判断函数使用大小写不敏感的协议匹配,回归测试覆盖了共享判断函数、workspace 图标 fallback 和 icon cache 路径里的大写 URL 协议。本地验证说明:
bun run typecheck:electron仍会因为既有的auto-update.ts和settings-default-thinking.test.ts类型错误失败,和这个 PR 无关。Tested on
Environment (optional)
在 macOS 上运行了本地 package tests、typecheck、prettier、eslint 和
git diff --check。Risk & Scope
isIconUrl现在被更多 desktop URL 处理路径复用,后续如果增加图标专属校验,需要避免把它收窄到影响通用 HTTP(S) 图标/source URL 判断。Linked Issues
Fixes #5469
AI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.