Skip to content

fix(desktop): resolve Windows drive paths and spaces in chat links - #1197

Closed
Totopo27 wants to merge 2 commits into
vastsa:mainfrom
Totopo27:fix/chat-windows-paths-and-spaces
Closed

Totopo27 wants to merge 2 commits into
vastsa:mainfrom
Totopo27:fix/chat-windows-paths-and-spaces

Conversation

@Totopo27

Copy link
Copy Markdown
Contributor

Summary

Fixes #1169 by properly parsing Windows drive paths and paths containing spaces in chat messages and tool records, ensuring they resolve to interactive workspace file links.

Motivation & Root Cause

As documented in #1169:

  1. When a user clicks a file path in a tool record such as C:\demo project\readme.md, the UI reported: "没有匹配 C:/demo project/readme.md 的文件" ("No matching file...").
    • Root cause: toWorkspaceRel in apps/desktop/src/lib/chat-links.ts only checked for leading slashes (path.startsWith("/")). On Windows, drive-anchored paths like C:\demo project\readme.md fell through to the relative path branch without stripping the workspace root prefix, passing C:/demo project/readme.md as the relative path to fsResolveRef.
  2. In markdown inline code (InlineCode) and conversational text, Windows paths or paths with spaces (e.g. C:\demo project\readme.md or /Users/me/my project/page.md) were rendered as plain non-clickable code.
    • Root cause: FILE_TOKEN_RE and SCAN_RE only allowed tokens matching [\p{L}\p{N}_@+.-], rejecting Windows drive letters (C:), backslashes, and paths with spaces.

Key Changes

  • apps/desktop/src/lib/chat-links.ts:
    • Updated toWorkspaceRel to detect both POSIX absolute paths (/) and Windows drive absolute paths ([a-zA-Z]:/). Normalizes backslashes, compares roots case-insensitively for drive letters, and safely slices the workspace-relative path.
    • Updated FILE_TOKEN_RE to allow Windows drive anchors and valid directory/file names with spaces.
    • Updated SCAN_RE to capture Windows drive-prefixed paths whole without fragmentation.
  • apps/desktop/test/chat-links.test.mjs:
    • Added unit regression tests for parseFileRef, toWorkspaceRel, resolvePreviewTarget, and splitChatText with Windows native paths, mixed slashes, spaces, and POSIX paths with spaces.
  • apps/desktop/test/chat-ref-resolve.test.mjs:
    • Added regression test confirming resolution of Windows absolute paths with spaces against workspace roots.

Verification

  • node apps/desktop/test/chat-links.test.mjs: 26/26 passed.
  • node apps/desktop/test/chat-ref-resolve.test.mjs: 22/22 passed.
  • node apps/desktop/test/tool-row-file-refs.test.mjs: 8/8 passed.
  • node apps/desktop/test/verified-chat-files.test.mjs: 7/7 passed.
  • node apps/desktop/test/transcript-file-chips.test.mjs: 4/4 passed.
  • pnpm lint: Checked 97 files, style tokens OK.
  • pnpm check:pr-base: Passed against upstream main.

Normalize Windows absolute drive paths and tolerate valid path spaces
in chat link parsing and workspace-relative resolution.

Previously, toWorkspaceRel only stripped POSIX leading slashes against
the workspace root, leaving Windows drive paths unstripped and causing
file resolution to report missing files for valid workspace items.
Additionally, FILE_TOKEN_RE and SCAN_RE rejected paths with Windows
drive letters, backslashes, or spaces, leaving inline code paths inert.

Fixes vastsa#1169
@vastsa

vastsa commented Sep 29, 2026

Copy link
Copy Markdown
Owner

I confirmed the reported bug is real, but this PR is not yet root-complete, so I cannot merge it as-is. The new FILE_TOKEN_RE accepts spaces only when parseFileRef receives the full string; the actual plain-text path is first selected by SCAN_RE, which still does not include spaces in anchored paths. Direct reproduction against this PR:\n\n- splitChatText("See C:\\demo project\\readme.md now", "C:/demo project") produces a plain prefix ending in ...\\r plus a link for eadme.md.\n- splitChatText("See C:/demo project/readme.md now", "C:/demo project") links project/readme.md, not the full path.\n- /Users/me/my project/page.md is likewise split and links only project/page.md.\n\nAlso, linkifyMdastTree still explicitly skips inlineCode, so the issue’s reported final-response inline-code path remains non-clickable and is not covered by the PR. The focused chat-links.test.mjs passes 26/26, but it does not test these splitChatText/inline-code paths. Please complete the scanner and clarify/cover the inline-code behavior before requesting merge; the access-boundary behavior must remain unchanged.

… resolution

Complete chat link scanning for workspace-anchored paths containing
spaces (both Windows drive paths and POSIX absolute roots).

Previously, splitChatText fragmented anchored paths with spaces because
the static SCAN_RE terminated at whitespace. Now, getScanRegex builds
root prefix patterns so anchored paths with spaces are captured whole
and evaluated up to the likely file path boundary.

Additionally, confirmed and documented that Markdown's InlineCode
component already previews file paths via resolvePreviewTarget, and
added regression coverage in chat-links.test.mjs.
@Totopo27

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough verification and sharp catch on splitChatText!

I've completed the implementation and addressed both points:

  1. Anchored Path Scanning with Spaces:

    • Updated scanner generation (getScanRegex) to dynamically match against workspace root prefixes ([cC]:/demo project, [cC]:\\demo project, /Users/me/my project).
    • Integrated scanAnchoredCandidate to parse candidate tokens up to the valid filename/extension boundary (isLikelyFilePath), preventing fragmentation into trailing words.
    • Added tests covering the exact cases you identified:
      • splitChatText("See C:\\demo project\\readme.md now", "C:/demo project") -> links C:\demo project\readme.md
      • splitChatText("See C:/demo project/readme.md now", "C:/demo project") -> links C:/demo project/readme.md
      • splitChatText("Check /Users/me/my project/page.md here", "/Users/me/my project") -> links /Users/me/my project/page.md
  2. Inline Code Behavior (linkifyMdastTree & InlineCode):

    • Clarified and verified the inline code path: linkifyMdastTree deliberately skips AST inlineCode nodes because the React component InlineCode (apps/desktop/src/components/Markdown.tsx) natively handles preview targets directly via resolvePreviewTarget(text, root, baseDir) and renders interactive buttons (.chat-code-link).
    • Because resolvePreviewTarget previously returned null for Windows drive paths and spaces, inline code paths remained inert. With the updated parseFileRef and toWorkspaceRel, InlineCode now resolves and opens these paths interactively without mutating markdown AST structure.
    • Added regression coverage verifying linkifyMdastTree leaves inlineCode untouched while resolvePreviewTarget classifies it.

All tests in chat-links.test.mjs pass cleanly (including the new anchored space tests). Ready for re-review!

yexisu pushed a commit to yexisu/PI-Desktop that referenced this pull request Sep 29, 2026
`main` is currently red, and it is not a flake:

    CI vastsa#1197  166ada6  failure   Typecheck Desktop
    CI vastsa#1195  967f3c2  failure   Typecheck Desktop
    CI vastsa#1192  db16f9c  failure   Typecheck Desktop
    CI vastsa#1189  4fedb8e  success   <- last green main run I can see

All three failures are the same line:

    electron/main/user-login-path.ts(28,74): error TS2345: Argument of type
    'string | undefined' is not assignable to parameter of type 'PathLike'.

**Root cause.** `Boolean(candidate) && existsSync(candidate)` is inside a type
predicate:

    const shell = [process.env.SHELL, "/bin/zsh", "/bin/bash", "/bin/sh"].find(
      (candidate): candidate is string => Boolean(candidate) && existsSync(candidate),
    );

A type predicate constrains what the *caller* sees; it does not narrow the
parameter inside its own body, and `Boolean(candidate)` is not a narrowing call.
So the body still has `string | undefined` and `existsSync` rejects it.

**Fix.** Narrow with a real check, which keeps the predicate at the call site and
satisfies the body:

    (candidate): candidate is string =>
      typeof candidate === "string" && existsSync(candidate),

The file arrived with `574c78a9` ("fix(mcp): spawn stdio servers with the
login-shell PATH"). That commit is 1 ahead / 8 behind `4fedb8e4` — it landed on
`main` after the last green run, which is where main turns red.

**Verification** (Windows 11; `pnpm install --filter @pi-desktop/desktop...
--ignore-scripts`, `pnpm --filter "@pi-desktop/desktop^..." build`):

- Pristine file, the CI command `pnpm --filter @pi-desktop/desktop typecheck`
  → exit 2 with `user-login-path.ts(28,74): error TS2345` — the same file,
  line and column as CI. With the fix → **exit 0**.
- `pnpm lint` → exit 0.
- `node scripts/check-architecture.mjs` → Architecture check passed.
- `node --test test/user-login-path.test.mjs` → 4/4 pass.
- Full `apps/desktop` suite: 2452 tests; 43 fail with the pristine file and 42
  with the fix. Both failure sets are the same macOS-only group (codesign, DMG,
  notarization, ssh askpass, process groups) plus a couple of load-sensitive
  timeout-budget tests; I could not make those two pass on either revision, and
  nothing in this change can reach them. Recording it rather than claiming the
  suite is green.
@Totopo27

Copy link
Copy Markdown
Contributor Author

Closing as superseded by merged PR #1222, which resolved #1169 in upstream.

@Totopo27 Totopo27 closed this Sep 30, 2026

This branch was previously deployed

1 inactive deployment
Preview — 4452873b Deployed Sep 29, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] 聊天无法完整识别含空格的文件路径,Windows 路径还有盘符与反斜杠问题

2 participants