fix: Expand Windows ~\\ home paths and hide phantom (session) entries in the desktop session list#5253
Conversation
… in the desktop session list
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @mvanhorn! The fix itself looks targeted and addresses a real issue (#5244).
However, the PR body doesn't follow our pull request template. The current body uses ## Summary and ## Changes headings, but the template requires:
- ## What this PR does — describe the change in prose
- ## Why it's needed — motivation and user-facing benefit
- ## Reviewer Test Plan — this is the big one. It needs:
- How to verify — steps a reviewer can follow to confirm the fix
- Evidence (Before & After) — screenshots, logs, or tmux output showing the bug and the fix (especially important for a Windows-specific issue)
- Tested on — which OSes you verified on (macOS / Windows / Linux)
- ## Risk & Scope — main risk, what's out of scope, breaking changes
- ## Linked Issues —
Fixes #5244(you have this, just needs the heading)
Could you restructure the PR body to follow the template? The Reviewer Test Plan section is especially important — without it, reviewers can't easily verify the Windows-specific path expansion fix or the phantom session filtering change.
中文说明
感谢贡献,@mvanhorn!修复本身看起来有针对性,解决了真实问题(#5244)。
但 PR 正文没有按照我们的 PR 模板 填写。当前使用的是 ## Summary 和 ## Changes,而模板要求:
- ## What this PR does — 用文字描述改动
- ## Why it's needed — 动机和用户可见的收益
- ## Reviewer Test Plan — 这是最重要的部分,需要包含:
- How to verify — 审查者如何验证修复
- Evidence (Before & After) — 截图、日志或 tmux 输出,展示修复前后的对比(对 Windows 特定问题尤为重要)
- Tested on — 你在哪些操作系统上验证过
- ## Risk & Scope — 主要风险、不在范围内的内容、破坏性变更
- ## Linked Issues —
Fixes #5244(已有,需要放在对应标题下)
请按照模板重新组织 PR 正文。Reviewer Test Plan 部分尤其重要——没有它,审查者很难验证 Windows 路径展开修复和幽灵会话过滤的改动。
— Qwen Code · qwen3.7-max
wenshao
left a comment
There was a problem hiding this comment.
No blocking issues found. The two fixes are targeted and correct — expandPath now expands ~\ using the same dual-separator split(/[/\\]+/) pattern already used in storage.ts/qwen-agent.ts/vscode paths.ts, and isUnresolvedQwenCanonicalMirror correctly drops the timestamp escape-hatch (timestamps were a false "keep" signal present on every mirror) while the awaited refreshExternalSessions in GET_SESSIONS still resolves content-bearing sessions before filtering. New tests pass and both packages typecheck. LGTM! ✅ — claude-opus-4-8[1m] via Qwen Code /qreview
|
@qwen-code /triage |
|
Thanks for the PR, @mvanhorn! Template looks good ✓ — the prior CHANGES_REQUESTED review flagged template headings, but the body has since been updated to follow the template correctly. On direction: clearly aligned. Two real Windows desktop bugs — phantom On approach: the scope feels right. The Moving on to code review. 🔍 中文说明感谢贡献,@mvanhorn! 模板完整 ✓ —— 之前的 CHANGES_REQUESTED 审查指出了模板标题问题,但正文已按模板正确更新。 方向:完全对齐。两个真实的 Windows 桌面端 bug——幽灵 方案:范围合理。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewReviewed the diff against my independent proposal. The PR matches what I'd do: Path expansion — the Session filtering — the core insight is correct: the timestamp short-circuit was a false "keep" signal since every mirror has timestamps. Removing it and routing through No correctness bugs, no security concerns, no AGENTS.md violations. The diff is focused — 4 files, +172/-12, no drive-by refactors. Test ResultsNew tests (PR-added) — all 7 pass ✅Regression check — base branch confirms tests catch the bugsStashing the PR fix and re-running the new tests: This confirms the new tests properly validate the fix rather than being tautological. Full suite — no new regressionsTypecheckReal-Scenario TestingN/A — both fixes are desktop-internal logic (Electron-process 中文说明代码审查对照我的独立方案审查了 diff。PR 的实现与我的思路一致: 路径展开 — 会话过滤 — 核心判断正确:时间戳短路是虚假的"保留"信号,因为每个镜像都有时间戳。移除它并委托给 无正确性 bug、无安全隐患、无 AGENTS.md 违规。Diff 聚焦——4 个文件、+172/-12,无顺手重构。 测试结果新增测试(PR 添加)— 全部 7 个通过 ✅路径展开 4/4 通过,会话过滤 3/3 通过(见上方输出)。 回归检查 — 基线分支确认测试能捕捉 bug暂存 PR 修复后重跑新测试:路径测试 2 通过 2 失败,会话测试 1 通过 2 失败——确认新测试确实验证了修复,而非自证正确。 完整套件 — 无新增回归shared: 2289 通过 · 15 失败(均为预存问题,与路径/会话无关)。server-core: 112 通过 · 1 失败(预存问题,与会话过滤无关)。 类型检查
真实场景测试N/A — 两处修复均为桌面端内部逻辑(Electron 进程的 — Qwen Code · qwen3.7-max |
|
This is a clean, focused bug fix. Two real Windows desktop bugs, two minimal code changes, two well-written test files. The approach matches what I'd do independently, the tests actually catch the bugs (verified against the base branch), and there's nothing in the diff that doesn't belong. The prior CHANGES_REQUESTED review from our bot was about template formatting — the author has since restructured the PR body to follow the template correctly. That concern is resolved. Approving. ✅ 中文说明这是一个干净、聚焦的 bug 修复。两个真实的 Windows 桌面端 bug,两处最小化的代码改动,两个编写良好的测试文件。方案与我的独立提案一致,测试确实能捕捉 bug(已与基线分支验证),diff 中每一处改动都有其必要性。 之前 bot 的 CHANGES_REQUESTED 审查是关于模板格式——作者已按模板重新组织了 PR 正文,该问题已解决。 批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
Appreciate the quick merge, @wenshao. Expanding the Windows home-path tilde and hiding the phantom entries cleans up the desktop client. |
|
Thanks @wenshao - expanding Windows home paths and hiding the phantom session entries are both nice quality-of-life fixes for the desktop client. |
What this PR does
This PR fixes two related defects that surface on the Windows desktop client after running certain skill/tool tasks: phantom conversations titled
(session)showing up in the session list, and a literal~folder being created under the app install directory.Windows
~\home-path expansion (packages/desktop/packages/shared/src/utils/paths.ts):expandPath()previously only expanded the Unix-style~/prefix. A Windows-style~\Documents\...or~\.craft-agent\...path fell through to the relative-path branch and was resolved against the process working directory — which on the desktop app is the install directory — creating a stray literal~folder. The fix extends the tilde branch to also match~\, splitting the remainder on either separator (/or\) and joining each segment against the home directory, so Windows-style home paths expand correctly. This mirrors the dual-separator handling already used elsewhere in the codebase.Hide empty
(session)placeholder mirrors in the desktop session list (packages/desktop/packages/server-core/src/sessions/SessionManager.ts): The phantom(session)entries are unresolved Qwen canonical-mirror placeholders with zero messages and no preview/content. The previousisUnresolvedQwenCanonicalMirror()logic short-circuited to "show this session" whenever any of itscreatedAt/lastUsedAt/lastMessageAttimestamps was non-zero — which these placeholders always have — so they were never filtered out. The fix:hasNoRenderableLocalMessages(), which considers both the loaded message array and the persistedmessageCount;managed.messages?.length ?? 0and by only treatingmanaged.nameas a title when it is actually a string.Net effect: empty
(session)placeholders are filtered out of the session list, while sessions with a real title or real message content continue to be shown.Why it's needed
On the Windows desktop client (Desktop v0.04), running skill/tool tasks produced extra
(session)conversations the user never created — they open with no real chat content, only tool-call/execution records or nearly empty — and also caused a literal~directory to be created under the app install directory. Per issue #5244, the expected behavior is:~\...should expand to the user home just like~/..., no literal~folder should be created under the install directory, and empty external sessions (zeromessageCount, no preview/prompt/title) should not appear as user-visible conversations in the main session list.Reviewer Test Plan
How to verify
expandPath('~\\.craft-agent\\foo')and confirm it returnsjoin(homedir(), '.craft-agent', 'foo')rather than a path under the current working directory; confirmexpandPath('~\\')resolves to the home directory; confirm the existing~,~/,$HOME,${HOME}cases are unchanged. These are covered by the new unit tests inpackages/desktop/packages/shared/src/utils/__tests__/paths.test.ts.(session)managed session with zero messages and non-zero timestamps and confirmgetSessions()no longer returns it; confirm sessions with a real title or real message content are still returned; confirmisUnresolvedQwenCanonicalMirror()does not throw on a malformed placeholder record. These are covered by the new unit tests inpackages/desktop/packages/server-core/src/sessions/SessionManager.test.ts.Evidence (Before & After)
N/A — non-visible logic/path-handling fixes; behavior is covered by the new unit tests above. The user-facing symptom (phantom
(session)rows in the desktop session list) is documented with screenshots in issue #5244; this change removes those rows by filtering empty placeholder mirrors.Tested on
✅ tested ·⚠️ not tested · N/A
Environment (optional)
Unit tests added for both fixes; validated through the repository CI matrix (Node 22.x on ubuntu/macos/windows-latest).
Risk & Scope
expandPath()helper and the desktopSessionManagerplaceholder-filtering logic. The path change only adds a new~\branch and broadens segment splitting; the session change only stops showing empty(session)placeholders that have a timestamp.Linked Issues
Fixes #5244
中文说明
这个 PR 做了什么
本 PR 修复了在 Windows 桌面端运行某些 skill / tool 任务后出现的两个相关缺陷:会话列表中出现名为
(session)的幽灵会话,以及在应用安装目录下创建出一个字面量~文件夹。Windows
~\home 路径展开(packages/desktop/packages/shared/src/utils/paths.ts):expandPath()之前只展开 Unix 风格的~/前缀。Windows 风格的~\Documents\...或~\.craft-agent\...路径不会被命中,落到相对路径分支,被解析到进程当前工作目录下——在桌面应用里就是安装目录——从而创建出一个多余的字面量~文件夹。修复方式是把波浪号分支扩展为同时匹配~\,把剩余部分按任意分隔符(/或\)切分后逐段与 home 目录拼接,使 Windows 风格的 home 路径能正确展开。这与代码库其他位置已有的双分隔符处理保持一致。在桌面会话列表中隐藏空的
(session)占位镜像(packages/desktop/packages/server-core/src/sessions/SessionManager.ts):这些幽灵(session)条目是未解析的 Qwen canonical-mirror 占位记录,消息数为零、没有预览/内容。之前的isUnresolvedQwenCanonicalMirror()逻辑只要createdAt/lastUsedAt/lastMessageAt中任意一个时间戳非零,就会短路成"显示该会话"——而这些占位记录总是带有时间戳——所以它们永远不会被过滤掉。本次修复:hasNoRenderableLocalMessages(),它会同时考虑已加载的消息数组和持久化的messageCount;managed.messages?.length ?? 0、并且只在managed.name确实是字符串时才将其当作标题,从而增强对畸形记录的健壮性。最终效果:空的
(session)占位记录被从会话列表中过滤掉,而拥有真实标题或真实消息内容的会话继续正常显示。为什么需要
在 Windows 桌面端(Desktop v0.04)上,运行 skill / tool 任务会产生用户从未主动创建的额外
(session)会话——打开后没有真实聊天内容,只有工具调用 / 执行记录,或几乎为空——同时还会在应用安装目录下创建出一个字面量~目录。根据 issue #5244,预期行为是:~\...应当像~/...一样展开到用户 home;安装目录下不应创建字面量~文件夹;messageCount为零且没有 preview / prompt / title 的外部 session 不应出现在主会话列表中作为用户可见的普通对话。审阅者测试计划
如何验证
expandPath('~\\.craft-agent\\foo'),确认它返回join(homedir(), '.craft-agent', 'foo'),而不是当前工作目录下的路径;确认expandPath('~\\')解析到 home 目录;确认已有的~、~/、$HOME、${HOME}用例行为不变。这些由packages/desktop/packages/shared/src/utils/__tests__/paths.test.ts中新增的单元测试覆盖。(session)受管会话,确认getSessions()不再返回它;确认拥有真实标题或真实消息内容的会话仍被返回;确认isUnresolvedQwenCanonicalMirror()在畸形占位记录上不抛异常。这些由packages/desktop/packages/server-core/src/sessions/SessionManager.test.ts中新增的单元测试覆盖。证据(前后对比)
N/A——这是不可见的逻辑 / 路径处理修复,行为由上面新增的单元测试覆盖。用户可见症状(桌面会话列表中的幽灵
(session)行)在 issue #5244 中已有截图记录;本次改动通过过滤空占位镜像移除了这些行。测试平台
✅ 已测试 ·⚠️ 未测试 · N/A
运行环境(可选)
为两处修复均新增了单元测试;通过仓库 CI 矩阵验证(Node 22.x,ubuntu / macos / windows-latest)。
风险与范围
expandPath()辅助函数和桌面SessionManager的占位过滤逻辑。路径改动只新增~\分支并扩展分段切分;会话改动只停止显示带时间戳的空(session)占位记录。关联 Issue
Fixes #5244