diff --git a/docs/plans/2026-09-27-files-changes-uiux.md b/docs/plans/2026-09-27-files-changes-uiux.md index 46f6b87..59853bd 100644 --- a/docs/plans/2026-09-27-files-changes-uiux.md +++ b/docs/plans/2026-09-27-files-changes-uiux.md @@ -476,3 +476,21 @@ ChangesTab ② 变动页 Git 视角以树显示：`src/client/changes`（单子目录链压缩成一行，带计数胶囊 1）→ `GitLens.tsx`（M 徽标），已暂存组 `src` → `app.ts`。 - 单文件页回归：14 个 spec / 91 用例绿（含新增 `file-tree-archive.spec.tsx` 9 例：出现条件、成功链、三条失败路径、in-flight 守卫）。 - 仍未验证：真实浏览器里 ZIP 的落盘行为（jsdom spy 证据）、拖拽/多选手感、Windows 宿主差异。 + +### 9.5 第三轮（verifier 二轮报告后的修复，全部完成） + +| 编号 | 问题 | 修法 | 判别性验证 | +|---|---|---|---| +| F1【高】 | 中文等非 latin1 下载名 → 真实宿主 **500**（`content-disposition` 直接塞原始名，Node `writeHead` 拒绝 >U+00FF） | `archive-route.ts` 新增 `contentDispositionOf`：ASCII 回退 + `filename*=UTF-8''…`（无可用 ASCII stem 时回退 `download.zip`） | 临时回退 → 真 `node:http` 用例 500；对照断言钉住 `validateHeaderValue` 必抛 | +| F2【中】 | 多选同名不同目录 → 归档条目重名（解压互相覆盖） | `disambiguateArchiveNames`：只对冲突项逐级上溯父目录，必要时回退完整路径 | 临时回退 → 条目重名断言变红 | +| F3【中低】 | `maxBytes` 在读完源文件后才判定（与注释相反）；且 `budget===0` 时 0 字节 FIFO 仍会 `open()` 永久阻塞 | `prepareFile` 先 `stat`，`budget <= 0 || size > budget` 立即抛，再读 | FIFO 探针：回退后 5s 超时（阻塞），修复后 44ms 通过（Windows 跳过该探针） | +| F4【中低】 | SSH 模式下宿主不可用时 reveal 整条消失 | SSH 过滤保留 `kind === 'reveal'` 的目标，并注明理由 | 更新 open-with.spec 断言（`['explorer','vscode','cursor','custom:e2']`，reveal 恰 1 条） | +| F5【低】 | 图钉是 `` 嵌在 menuitem 按钮内（语义非法 + 键盘不可达） | 去掉 role/tabIndex，改成诚实的鼠标热区；注释写明键盘入口是设置面板 | 临时加回 role → 用例变红 | +| F6【低】 | 变更树行无 memo，空闲 tick 全量重渲染 | ① 行 `memo` + 恒定 props（图标解析搬进行内当探针）② 内容键 `entriesKey` 派生 tree ③ （Lead）共享 store 的 `sameStatus` 保住快照身份 | 三处各自临时回退 → 探针变红 | +| F7【低】 | 排序依赖运行时 locale collation | `localeCompare(other, 'en', { sensitivity: 'base' })` + 码点兜底 | 反向输入顺序不变 + 瑞典 collation 对照断言 | +| F8【低】 | POSIX 文件名里的 `\` 被静默转成层级 | `archiveName` 不再替换 `\` | 实探条目 `a\b.txt` 保持单条成员 | +| F9【低】 | 打包中重复点击静默吞掉、大选择无反馈 | `archiveBusy` state + 树顶 `Notice kind="loading" role="status"` | deferred fetch 断言忙态行出现/消失、重复点选 fetch 仍 1 次 | +| — | 新增交互：目录行支持「暂存/取消暂存该目录」 | `DirRow` hover IconButton（`gitStage`/`gitUnstage` 均接受目录 pathspec） | 断言 `gitStage(scope,'src/deep',MAIN)` 且点击不触发折叠 | + +最终门禁（修复后重跑）：`pnpm typecheck` / `pnpm lint` / `pnpm test`（**134 文件 / 1441 通过 / 9 跳过**）/ `pnpm build` 全绿； +`DSH_CMD='npx -y --package @deepseek-ai/dsh@0.1.7-rc.1 dsh' pnpm test:mount` → **7/7 通过**（mount=372ms，p95 帧间隔 16ms，无宽度泄漏）。 diff --git a/src/archive-route.ts b/src/archive-route.ts index d31df3d..5edd5c0 100644 --- a/src/archive-route.ts +++ b/src/archive-route.ts @@ -11,7 +11,7 @@ * so it can neither escape the workspace nor cycle. */ import { lstat, readdir } from 'node:fs/promises' -import { join } from 'node:path' +import { basename, dirname, join } from 'node:path' import { SidebarError } from './wire.ts' import { ZIP_MAX_ENTRIES, type ZipEntry } from './zip.ts' @@ -44,6 +44,91 @@ export function archiveNameOf(raw: string | null): string { return cleaned.toLowerCase().endsWith('.zip') ? cleaned : `${cleaned}.zip` } +/** + * The `content-disposition` value for one archive download. + * + * A non-latin1 name (a Chinese folder → `报告.zip`) CANNOT go into the header + * verbatim: Node's `writeHead` validates header values and rejects anything + * above U+00FF with `Invalid character in header content` — the route turned + * that into a 500. So the name is sent twice, exactly like `/sidebar/file` + * does for a download: an ASCII-only `filename="…"` fallback for old clients + * and the RFC 5987 `filename*=UTF-8''…` form (percent-encoded, therefore pure + * ASCII) for everyone else, which wins in every current browser. + */ +export function contentDispositionOf(name: string): string { + return `attachment; filename="${asciiFallbackOf(name)}"; filename*=UTF-8''${encodeURIComponent(name)}` +} + +/** + * The ASCII-only fallback of a download name: characters a quoted header + * string cannot carry (or that no latin1 client could render) drop out, and a + * stray quote or backslash does too. A result with no stem left (`''` or a + * bare extension, as a fully non-latin1 name reduces to) becomes + * `download.zip`, so the header always names a usable file. + */ +function asciiFallbackOf(name: string): string { + const ascii = [...name] + .filter(char => { + const code = char.codePointAt(0)! + return code >= 0x20 && code <= 0x7E && char !== '"' && char !== '\\' + }) + .join('') + const stem = ascii.replace(/\.zip$/i, '') + if (stem === '' || /^\.+$/.test(stem)) return 'download.zip' + return ascii +} + +/** + * The in-archive name of every selection, disambiguated. + * + * The common case keeps the intuition "one selection is named by its own + * basename" — archive `/ws/src` and the entries live under `src/`. Two + * selections that would collide (multi-select `/ws/a/index.ts` and + * `/ws/b/index.ts` both wanting `index.ts`) are extended with parent + * segments until every name is unique, so an extractor cannot silently + * overwrite one member with another. A path that runs out of ancestors + * (selecting the filesystem root, or `/a` twice) falls back to the full + * '/'-joined path, which is unique by construction. + * + * @param selections - absolute (already fenced) paths, in selection order. + * @returns one '/'-separated archive name per selection, same order. + */ +export function disambiguateArchiveNames(selections: readonly string[]): string[] { + const segmentsOf = (absolute: string, depth: number): string => { + const parts: string[] = [] + let current = absolute + for (let level = 0; level < depth; level += 1) { + const part = basename(current) + const parent = dirname(current) + if (part === '' || parent === current) break + parts.unshift(part) + current = parent + } + return parts.join('/') + } + const deepEnough = (absolute: string, name: string): string => { + const full = segmentsOf(absolute, Number.MAX_SAFE_INTEGER) + // Still colliding after every segment is in play (two identical paths, or + // a root-level duplicate): the full path is the best available identity. + return full === name ? absolute.replace(/^\/+/, '') : name + } + const depth = new Map(selections.map(path => [path, 1])) + for (let round = 0; round < 64; round += 1) { + const names = selections.map(path => segmentsOf(path, depth.get(path)!)) + const counts = new Map() + for (const name of names) counts.set(name, (counts.get(name) ?? 0) + 1) + if ([...counts.values()].every(count => count === 1)) return names + const grown = new Set() + for (const [index, path] of selections.entries()) { + if ((counts.get(names[index]!) ?? 0) <= 1) continue + depth.set(path, depth.get(path)! + 1) + grown.add(path) + } + if (grown.size === 0) break + } + return selections.map(path => deepEnough(path, segmentsOf(path, depth.get(path)!))) +} + /** * Collect one selected path into the archive: a file becomes one entry, a * directory is walked depth-first and each level contributes its own entry diff --git a/src/client/FileTree.tsx b/src/client/FileTree.tsx index 99978bc..c32331e 100644 --- a/src/client/FileTree.tsx +++ b/src/client/FileTree.tsx @@ -1032,16 +1032,20 @@ export function FileTree(props: { label: ( {openWithLabelOf(target)} - {/* The pushpin: a span (never a button — the Menu row itself is - a button, so a nested interactive element would be invalid). - Clicking it pins/unpins the target at the menu's top level - WITHOUT selecting the row: the pin stops propagation, so the - menu stays open and the icon flips on the next render. */} + {/* The pushpin: a PLAIN span, deliberately not a control. The + Menu renders the row itself as ` @@ -159,34 +190,62 @@ function FileRow(props: { disabled={busy} label={action} icon={staged ? : } - onClick={props.onToggleStage} + onClick={() => { props.onToggleStage(node.path, staged) }} /> ) -} +}) /** One directory row: the disclosure chevron + folder artwork + the label - * (a compressed chain reads as one 'a/b/c' line) + how much changed below. */ -function DirRow(props: { node: ChangeDir; icon: ReactNode; open: boolean; onToggle(): void }): ReactNode { - const { node, open } = props + * (a compressed chain reads as one 'a/b/c' line) + how much changed below, + * plus the SAME trailing action a file row carries — applied to the whole + * subtree (`git add -A -- ` / `git reset -q -- `, both of which + * take a directory pathspec). The action is a sibling of the disclosure + * button, so a click can never fold the row; it stops propagation anyway so + * the intent survives a future nesting change. Memoized like {@link FileRow}. */ +const DirRow = memo(function DirRow(props: { + row: ChangeDir + folderGlyph: (path: string, open: boolean) => ReactNode + staged: boolean + open: boolean + busy: boolean + onToggle: (key: string) => void + onToggleStage: (path: string, staged: boolean) => void +}): ReactNode { + const { row: node, staged, open, busy } = props + const key = `${staged ? 's' : 'u'}:${node.path}` + const action = staged ? t('unstageAll') : t('stageAll') return ( - +
+ + : } + onClick={(event) => { + event.stopPropagation() + props.onToggleStage(node.path, staged) + }} + /> +
) -} +}) export interface GitLensProps { scope: SessionScope @@ -575,28 +634,12 @@ export function GitLens(props: GitLensProps) { void writeClipboard(text) } - const openFileMenu = (event: MouseEvent, path: string, staged: boolean, untracked: boolean): void => { - event.preventDefault() - event.stopPropagation() - setFileMenu({ path, staged, untracked, x: event.clientX, y: event.clientY }) - } - const openHistoryMenu = (event: MouseEvent, entry: GitLogEntry): void => { event.preventDefault() event.stopPropagation() setHistoryMenu({ entry, x: event.clientX, y: event.clientY }) } - /** The preview ref for one changed file (one ref per path+side). */ - const worktreeRefOf = (path: string, untracked: boolean, staged: boolean): SidebarDiffRef => ({ - kind: 'worktree', - path, - staged, - untracked, - worktree: selectedWorktree, - repoRoot, - }) - /** The preview ref for one commit. */ const commitRefOf = (entry: GitLogEntry): SidebarDiffRef => ({ kind: 'commit', @@ -615,8 +658,11 @@ export function GitLens(props: GitLensProps) { } const entries = snapshot?.entries ?? NO_ENTRIES - // One fold per group, memoized on the snapshot's own entry array: the tree - // (compression + sorting + counts) is rebuilt only when git answers anew. + // The CONTENT of the answer decides whether the tree must be rebuilt; the + // array identity only decides whether the (cheap) key is recomputed. + const contentKey = useMemo(() => entriesKey(entries), [entries]) + // One fold per group, rebuilt only when the change list's CONTENT changes — + // never on a poll that answered the same files, however fresh its arrays are. const { stagedEntries, unstagedEntries, stagedTree, unstagedTree } = useMemo(() => { const staged = entries.filter(isStagedEntry) const unstaged = entries.filter(isUnstagedEntry) @@ -626,34 +672,67 @@ export function GitLens(props: GitLensProps) { stagedTree: buildChangeTree(staged), unstagedTree: buildChangeTree(unstaged), } - }, [entries]) + // The key stands in for `entries`: same content ⇒ same tree objects ⇒ the + // memoized rows below keep their props and never re-render. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [contentKey]) const isRepo = snapshot?.isRepo === true const branch = snapshot?.branch ?? '' const branchOptions = branch === '' ? branchNames : [branch, ...branchNames.filter(name => name !== branch)] const clean = isRepo && stagedEntries.length === 0 && unstagedEntries.length === 0 - /** Fold or unfold one directory row (keyed per group, see `collapsed`). */ - const toggleDir = (key: string): void => { + /** Fold or unfold one directory row (keyed per group, see `collapsed`). + * Stable: it is a memoized row's prop. */ + const toggleDir = useCallback((key: string): void => { setCollapsed((current) => { const next = new Set(current) if (next.has(key)) next.delete(key) else next.add(key) return next }) - } + }, []) + + /** The latest-value holder for the actions a memoized row calls: a row's + * props must keep their identity across polls, so these callbacks read the + * current closures through the ref instead of closing over them. */ + const rowActions = useRef({ + preview: (_node: ChangeFile, _staged: boolean): void => { /* replaced below */ }, + stage: (_path: string, _staged: boolean): void => { /* replaced below */ }, + }) + useEffect(() => { + rowActions.current = { + preview: (node, staged) => { + onPreview(worktreeRefOf(node.path, isUntrackedStatus(node.status), staged, chosenPathRef.current, repoRootRef.current)) + }, + stage: (path, staged) => { stageEntry(path, staged) }, + } + }) + + /** Stable row callbacks (see rowActions). */ + const previewFile = useCallback((node: ChangeFile, staged: boolean): void => { + rowActions.current.preview(node, staged) + }, []) + const stagePath = useCallback((path: string, staged: boolean): void => { + rowActions.current.stage(path, staged) + }, []) + const openRowMenu = useCallback((event: MouseEvent, node: ChangeFile, staged: boolean): void => { + event.preventDefault() + event.stopPropagation() + setFileMenu({ path: node.path, staged, untracked: isUntrackedStatus(node.status), x: event.clientX, y: event.clientY }) + }, []) /** One file row of a group's tree (the single file-row renderer). */ const renderFile = (node: ChangeFile, staged: boolean): ReactNode => ( { onPreview(worktreeRefOf(node.path, isUntrackedStatus(node.status), staged)) }} - onContextMenu={(event) => { openFileMenu(event, node.path, staged, isUntrackedStatus(node.status)) }} - onToggleStage={() => { stageEntry(node.path, staged) }} + onPreview={previewFile} + onContextMenu={openRowMenu} + onToggleStage={stagePath} /> ) @@ -662,8 +741,16 @@ export function GitLens(props: GitLensProps) { const key = `${staged ? 's' : 'u'}:${node.path}` const open = !collapsed.has(key) return ( -
- { toggleDir(key) }} /> +
+ {open && (
{node.children.map(child => (child.kind === 'dir' ? renderDir(child, staged) : renderFile(child, staged)))} diff --git a/src/client/changes/change-tree.ts b/src/client/changes/change-tree.ts index eead591..20f6ec8 100644 --- a/src/client/changes/change-tree.ts +++ b/src/client/changes/change-tree.ts @@ -47,12 +47,26 @@ interface DirDraft { files: ChangeFile[] } -/** Directories before files, then by name — case-insensitively, with the raw - * name breaking ties so the order never depends on the locale's collation. */ +/** + * Directories before files, then by name — case-insensitively. + * + * The collation is PINNED (`'en'`, `sensitivity: 'base'`) rather than left to + * the runtime's default locale: `localeCompare(other)` alone follows whatever + * locale the browser/Node happens to run under, so the same change list could + * order differently on two machines. Base sensitivity makes case and accent + * variants compare equal, and the code-point tiebreak below then decides — + * a total order that does not move when the input order changes. + */ +function compareNames(left: string, right: string): number { + const collated = left.localeCompare(right, 'en', { sensitivity: 'base' }) + if (collated !== 0) return collated + if (left === right) return 0 + return left < right ? -1 : 1 +} + function compareNodes(left: ChangeNode, right: ChangeNode): number { if (left.kind !== right.kind) return left.kind === 'dir' ? -1 : 1 - const folded = left.name.toLowerCase().localeCompare(right.name.toLowerCase()) - return folded !== 0 ? folded : left.name.localeCompare(right.name) + return compareNames(left.name, right.name) } /** The number of changed files under a finished child list. */ diff --git a/src/client/changes/changes.module.css b/src/client/changes/changes.module.css index 97f20d2..a98c11c 100644 --- a/src/client/changes/changes.module.css +++ b/src/client/changes/changes.module.css @@ -103,23 +103,23 @@ border-left: 1px solid var(--dsw-alias-border-l1); } -/* One directory row: disclosure chevron + folder artwork + label + count. */ +/* A directory row's disclosure button. It shares the `.row` band with a file + row (the band owns the hover/selected fill and the trailing action), so this + button is only the row's main hit area. */ .dirRow { + flex: 1; + min-width: 0; display: flex; align-items: center; gap: 6px; - width: 100%; - min-height: 28px; - padding: 0 6px 0 2px; border: none; - border-radius: 4px; background: transparent; + padding: 0; color: var(--dsw-alias-label-secondary); font: inherit; cursor: pointer; text-align: left; } -.dirRow:hover { background: var(--dsw-alias-interactive-bg-hover); } .dirRow:focus-visible { outline: 2px solid var(--dsw-alias-interactive-bg-hover-accent); outline-offset: -1px; } .chevron { flex: none; display: inline-flex; color: var(--dsw-alias-label-tertiary); transition: transform 140ms var(--ds-ease-in-out); } diff --git a/src/client/open-with.ts b/src/client/open-with.ts index b255630..f0b4e52 100644 --- a/src/client/open-with.ts +++ b/src/client/open-with.ts @@ -137,9 +137,15 @@ function customIdOf(id: string): string { /** * The menu-visible open targets, in order (built-ins then custom editors). - * In SSH mode the local-only targets (the OS file manager, Zed, custom - * editors without the VSCode dialect) are dropped — they cannot reach a - * remote path. Unknown pinned ids are pruned here too. + * In SSH mode the local-only URL openers (Zed, custom editors without the + * VSCode dialect) are dropped — they cannot reach a remote path. The + * `explorer` target is NOT dropped: it reveals a path in the file manager of + * the machine that owns the workspace, which is exactly where the host half + * runs, and it is the only reveal left when the DSH host's own open-in-app + * capability is unavailable (older host, no desktop session). A headless + * remote cannot raise a file manager anyway — that is the same silent + * no-op the other external launches already have. Unknown pinned ids are + * pruned here too. */ export function resolveOpenWithTargets(config: OpenWithConfig): OpenWithTarget[] { const ssh = config.sshHost.trim() !== '' @@ -156,7 +162,7 @@ export function resolveOpenWithTargets(config: OpenWithConfig): OpenWithTarget[] localOnly: !editor.isVscodeFamily, })), ] - return targets.filter(target => !(ssh && target.localOnly)) + return targets.filter(target => !(ssh && target.localOnly && target.kind !== 'reveal')) } /** The SSH hint appended to a target's label in remote mode. */ diff --git a/src/client/ui/git-status.ts b/src/client/ui/git-status.ts index 5870c88..437fe47 100644 --- a/src/client/ui/git-status.ts +++ b/src/client/ui/git-status.ts @@ -90,6 +90,24 @@ function joinRoot(root: string, rel: string): string { return `${base}/${rel.replace(/^[\\/]+/, '')}` } +/** + * Whether two status snapshots carry the same information. Compared field by + * field (never by serializing the whole object): the entries are the payload + * and their order is stable, so a length + pairwise compare is enough. + */ +function sameStatus(a: GitStatusResult, b: GitStatusResult): boolean { + if (a === b) return true + if (a.isRepo !== b.isRepo || a.branch !== b.branch || a.root !== b.root + || a.truncated !== b.truncated || a.entries.length !== b.entries.length) return false + if (a.repositories?.length !== b.repositories?.length) return false + if (a.repositories !== undefined && b.repositories !== undefined + && a.repositories.some((root, index) => root !== b.repositories![index])) return false + return a.entries.every((entry, index) => { + const other = b.entries[index]! + return entry.path === other.path && entry.xy === other.xy + }) +} + interface Index { /** Normalized absolute path → status (files only). */ files: Map @@ -177,6 +195,15 @@ function fetchSlot(slot: Slot): void { api.gitStatus(scope, slot.worktree) .then((snapshot) => { if (generation !== slot.generation) return + // Idle polls return the same answer as a NEW object every 2.5s. Keeping + // the previous identity when nothing changed is what lets every + // downstream memo (the changes tree, the file tree's row model, the + // icon index) hold across ticks — without it the whole list re-rendered + // and re-resolved icons on every poll. + if (slot.snapshot !== null && sameStatus(slot.snapshot, snapshot)) { + slot.error = false + return + } slot.snapshot = snapshot slot.error = false }) diff --git a/src/index.ts b/src/index.ts index 6548e9c..d37a96c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -38,7 +38,7 @@ import { isTrustedApiRequest } from './trust-fence.ts' import { registerBundleRoute } from './bundle-route.ts' import { createDirectoryWatchers, type DirectoryWatchers } from './fs-watch.ts' import { launchExternal } from './open-external.ts' -import { archiveNameOf, collectZipEntries } from './archive-route.ts' +import { archiveNameOf, collectZipEntries, contentDispositionOf, disambiguateArchiveNames } from './archive-route.ts' import { buildZip, type ZipEntry } from './zip.ts' import * as git from './git.ts' import { SettingsConflictError } from '@deepseek-ai/dsh-settings' @@ -68,10 +68,10 @@ export type { FileViewerProps, FileFetchStrategy, } from './client/service.ts' -// The archive walk + name sanitizer are exercised directly by tests/zip.spec.ts: +// The archive walk + name helpers are exercised directly by tests/zip.spec.ts: // their bounds live inside the collector, so the suite needs them observable // without creating 10 000 rows first. -export { archiveNameOf, collectZipEntries } from './archive-route.ts' +export { archiveNameOf, collectZipEntries, contentDispositionOf, disambiguateArchiveNames } from './archive-route.ts' /** Plugin identity for cordis.yml rows. */ export const name = 'dsh-better-sidebar' @@ -945,15 +945,22 @@ export function apply(ctx: Context, config?: SidebarConfig): void { const name = archiveNameOf(url.searchParams.get('name')) const cwd = await sessionCwdOf(ctx, sessionId, url.searchParams.get('cwd') ?? undefined) const fenceOn = fenceEnabledOf(() => settingsFace) + const selected: string[] = [] + for (const raw of paths) selected.push(await ensureWorkspacePath(cwd, raw, fenceOn)) + // Same-basename selections (a/index.ts + b/index.ts) get parent + // segments prepended, so no two archive members collide and an + // extractor cannot overwrite one with the other. const entries: ZipEntry[] = [] - for (const raw of paths) { - const absolute = await ensureWorkspacePath(cwd, raw, fenceOn) - await collectZipEntries(absolute, basename(absolute), entries) + const names = disambiguateArchiveNames(selected) + for (const [index, absolute] of selected.entries()) { + await collectZipEntries(absolute, names[index]!, entries) } const body = await buildZip(entries) res.writeHead(200, { 'content-type': 'application/zip', - 'content-disposition': `attachment; filename="${name}"`, + // Never the raw name: a non-latin1 value (中文目录 → 报告.zip) makes + // Node's writeHead throw and turns the download into a 500. + 'content-disposition': contentDispositionOf(name), 'content-length': String(body.byteLength), 'cache-control': 'no-cache', }) diff --git a/src/zip.ts b/src/zip.ts index fcbc522..9dd20ed 100644 --- a/src/zip.ts +++ b/src/zip.ts @@ -8,11 +8,12 @@ * for the whole archive. No third-party dependency, no filesystem walking — * the caller supplies the entries, this module only reads their bytes. * - * The entries are bounded by `maxEntries` / `maxBytes` before anything is - * read, so a runaway selection fails with a clear error instead of filling - * memory with an archive nobody asked for. + * The entries are bounded by `maxEntries` / `maxBytes`: the count is checked + * up front, and every source is stat'ed against the remaining byte budget + * BEFORE it is read, so a runaway selection fails with a clear error instead + * of filling memory with an archive nobody asked for. */ -import { readFile } from 'node:fs/promises' +import { readFile, stat } from 'node:fs/promises' import { deflateRaw } from 'node:zlib' import { promisify } from 'node:util' import { SidebarError } from './wire.ts' @@ -95,11 +96,20 @@ function dosDateTime(date: Date): { time: number; date: number } { } } -/** Normalize an entry name to a '/'-separated relative archive path. */ +/** + * Normalize an entry name to a '/'-separated relative archive path. + * + * A backslash is NOT a separator here: on POSIX (and on the host half, which + * builds the names) `a\b.txt` is one legal file name, so converting it would + * silently split one file into a directory. Windows separators never reach + * this point — the walk composes names from `basename`/`join` results. + * Leading slashes are dropped (names are archive-relative by definition) and + * `..` segments are refused, so a name can never traverse. + */ function archiveName(raw: string, isDir: boolean): string { - const normalized = raw.replace(/\\/g, '/').replace(/^\/+/, '') + const normalized = raw.replace(/^\/+/, '') const segments = normalized.split('/').filter(segment => segment !== '' && segment !== '.') - if (segments.length === 0 || segments.includes('..')) { + if (raw === '..' || segments.length === 0 || segments.includes('..')) { throw new SidebarError('bad-request', `invalid archive entry name "${raw}"`) } const name = segments.join('/') @@ -114,8 +124,26 @@ function requireSourcePath(raw: string): string { return raw } -/** Read + compress one member (deflate only when it actually shrinks). */ -async function prepareFile(name: string, path: string): Promise { +/** + * Read + compress one member (deflate only when it actually shrinks). + * @param limit - the archive's total uncompressed ceiling (for the message). + * @param budget - uncompressed bytes still available; the source is stat'ed + * FIRST and refused when it cannot fit, so an oversized archive never pulls + * the payload into memory (let alone deflates it) before failing. + */ +async function prepareFile(name: string, path: string, limit: number, budget: number): Promise { + let size: number + try { + size = (await stat(path)).size + } catch (error) { + throw new SidebarError('fs-error', `cannot read "${path}": ${error instanceof Error ? error.message : String(error)}`, 400) + } + // `budget <= 0` refuses even a zero-size source. That is not pedantry: a + // FIFO stats as 0 bytes and reading one BLOCKS until a writer shows up, so + // an exhausted budget must never reach open(). + if (budget <= 0 || size > budget) { + throw new SidebarError('fs-error', `archive exceeds the ${limit} byte limit`, 400) + } let data: Buffer try { data = await readFile(path) @@ -183,14 +211,16 @@ export async function buildZip(entries: readonly ZipEntry[], opts: ZipOptions = continue } const path = requireSourcePath(entry.path) - const member = await prepareFile(archiveName(entry.name, false), path) // The bound is the archive's total UNCOMPRESSED payload: that is what the // reader materializes, and it makes the limit independent of how well the // input happens to compress. - totalBytes += member.size - if (totalBytes > maxBytes) { + const member = await prepareFile(archiveName(entry.name, false), path, maxBytes, maxBytes - totalBytes) + // A source can grow AFTER its stat (sparse files, concurrent writers), so + // the ceiling is re-checked against what was actually read. + if (totalBytes + member.size > maxBytes) { throw new SidebarError('fs-error', `archive exceeds the ${maxBytes} byte limit`, 400) } + totalBytes += member.size members.push(member) } diff --git a/tests/changes-ops.spec.ts b/tests/changes-ops.spec.ts index b8e43bf..1723cde 100644 --- a/tests/changes-ops.spec.ts +++ b/tests/changes-ops.spec.ts @@ -250,6 +250,31 @@ describe('change tree (the Git lens reads its groups through it)', () => { ]) }) + it('pins the collation, so the order cannot move with the runtime locale', () => { + // Mixed case + accents + non-letters: the comparator runs an explicit + // 'en' base collation and breaks ties by code point. A bare + // `localeCompare(other)` would follow the machine's ICU default and could + // reorder the same change list on another machine. + const names = ['b.ts', 'A.ts', 'ä.ts', 'Z.ts', 'a.ts', 'Ä.ts', '_x.ts', '1.ts'] + const order = (list: readonly string[]): string[] => + buildChangeTree(list.map(path => entry(path))).map(node => node.name) + expect(order(names)).toEqual(['_x.ts', '1.ts', 'A.ts', 'a.ts', 'Ä.ts', 'ä.ts', 'b.ts', 'Z.ts']) + // …and it is a TOTAL order: reversing the input cannot move a row. + expect(order([...names].reverse())).toEqual(order(names)) + + const dirs = ['Zdir/f.ts', 'adir/f.ts', 'Ädir/f.ts', '_dir/f.ts'] + const dirOrder = (list: readonly string[]): string[] => + buildChangeTree(list.map(path => entry(path))).map(node => node.name) + expect(dirOrder(dirs)).toEqual(['_dir', 'adir', 'Ädir', 'Zdir']) + expect(dirOrder([...dirs].reverse())).toEqual(dirOrder(dirs)) + + // Why the pin is load-bearing: under Swedish collation 'ä' sorts AFTER + // 'z', so an unpinned comparator would order this very list differently on + // a machine whose runtime locale happens to be Swedish. + expect(['z.ts', 'ä.ts'].sort((left, right) => left.localeCompare(right, 'sv', { sensitivity: 'base' }))) + .toEqual(['z.ts', 'ä.ts']) + }) + it('returns no nodes for an empty or all-clean list', () => { expect(buildChangeTree([])).toEqual([]) expect(buildChangeTree([entry('a.ts', ' '), entry('b.ts', '!!')])).toEqual([]) diff --git a/tests/file-tree-archive.spec.tsx b/tests/file-tree-archive.spec.tsx index 1620293..21b6065 100644 --- a/tests/file-tree-archive.spec.tsx +++ b/tests/file-tree-archive.spec.tsx @@ -314,4 +314,44 @@ describe('FileTree zip and download', () => { expect(fetchMock).toHaveBeenCalledTimes(2) await settleDownload() }) + + it('shows a loading line while the archive is being built, and hides it after', async () => { + let release: (response: Response) => void = () => {} + fetchMock.mockImplementation(async () => await new Promise(resolve => { release = resolve })) + harness = await mountTree() + const busyLine = (): HTMLElement | null => harness.container.querySelector('[data-kind="loading"]') + // Nothing is shown before the request starts… + expect(busyLine()).toBeNull() + rightClick(rowByName(harness.container, 'sub')) + clickMenuitem('Zip and download') + // …the in-flight request keeps one visible, announced line… + const line = busyLine() + expect(line).not.toBeNull() + expect(line?.textContent).toBe('Loading…') + expect(line?.getAttribute('role')).toBe('status') + // …a repeated pick is still guarded (no second fetch) AND still not silent. + rightClick(rowByName(harness.container, 'sub')) + clickMenuitem('Zip and download') + expect(fetchMock).toHaveBeenCalledTimes(1) + expect(busyLine()).not.toBeNull() + await act(async () => { release(okResponse()) }) + await settleDownload() + expect(busyLine()).toBeNull() + expect(downloads).toEqual(['blob:mock-1|sub.zip']) + }) + + it('hides the loading line when the archive fails too', async () => { + let release: (response: Response) => void = () => {} + fetchMock.mockImplementation(async () => await new Promise(resolve => { release = resolve })) + harness = await mountTree() + rightClick(rowByName(harness.container, 'sub')) + clickMenuitem('Zip and download') + expect(harness.container.querySelector('[data-kind="loading"]')).not.toBeNull() + await act(async () => { release(errorResponse(413, { ok: false, error: { message: 'too big' } })) }) + await settleDownload() + expect(harness.container.querySelector('[data-kind="loading"]')).toBeNull() + // The busy line yields to the error strip: both never race for the slot. + expect(harness.container.querySelector('[role="alert"]')?.textContent) + .toContain('Archive failed: too big') + }) }) diff --git a/tests/file-tree-open-with.spec.tsx b/tests/file-tree-open-with.spec.tsx index ad95e86..52eaf33 100644 --- a/tests/file-tree-open-with.spec.tsx +++ b/tests/file-tree-open-with.spec.tsx @@ -162,11 +162,31 @@ describe('FileTree plugin open-with menu', () => { 'File Manager', 'VS Code', 'Cursor', 'Zed', 'Windsurf', ]) expect(submenuRows().every(item => item.querySelector('[class*="openWithPin"]') !== null)).toBe(true) - // Pinned state is announced per row (and swaps the pushpin glyph). + // Pinned state is carried per row by the pin's TITLE (it swaps the glyph + // and the hover hint). const vscodeRow = submenuRows().find(item => item.textContent?.trim() === 'VS Code') const cursorRow = submenuRows().find(item => item.textContent?.trim() === 'Cursor') - expect(vscodeRow?.querySelector('[aria-label="Unpin"]')).not.toBeNull() - expect(cursorRow?.querySelector('[aria-label="Pin to menu"]')).not.toBeNull() + expect(vscodeRow?.querySelector('[title="Unpin"]')).not.toBeNull() + expect(cursorRow?.querySelector('[title="Pin to menu"]')).not.toBeNull() + }) + + it('keeps the pin a mouse hot zone, never a fake nested control', async () => { + // The Menu renders each row as `