From abc8c8bcb3dca29a976c5305d0f608c02953ba34 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 30 Jul 2026 07:24:58 -0700 Subject: [PATCH] fix(file-explorer): natural sort on SSH funnels, relay, and pickers Adversarial-review round 1 rework: - Both readDir funnels short-circuited to the SSH filesystem provider before the patched sort, so SSH workspaces kept lexicographic order; re-sort locally after the provider returns (the remote relay may be an older build), and fix the relay's own comparator for relay-native consumers. - sortDirEntries (shared, unit-tested) owns the directories-first + natural-order listing contract used by every funnel. - compareFileNames breaks numeric-collation ties ('2' vs '02') by code units so sibling order stays total instead of readdir order, and pins the collator locale to 'en' so every host produces one order. - The SSH folder browser and runtime server dir picker now match the Explorer they browse into. - Ordering pinned by tests at the relay, source-control tree, and shared helper. --- src/main/ipc/filesystem.ts | 13 ++++------ src/main/ipc/ssh-browse.ts | 10 +++---- src/main/runtime/orca-runtime-files.ts | 13 ++++------ src/main/runtime/orca-runtime.ts | 8 ++---- src/relay/fs-handler.test.ts | 18 ++++++++----- src/relay/fs-handler.ts | 8 ++---- .../right-sidebar/source-control-tree.test.ts | 18 +++++++++++++ src/shared/file-name-sort.test.ts | 26 ++++++++++++++++++- src/shared/file-name-sort.ts | 25 ++++++++++++++++-- 9 files changed, 94 insertions(+), 45 deletions(-) diff --git a/src/main/ipc/filesystem.ts b/src/main/ipc/filesystem.ts index 06a34e6eb5a..eb08aa0bc65 100644 --- a/src/main/ipc/filesystem.ts +++ b/src/main/ipc/filesystem.ts @@ -30,7 +30,7 @@ import type { } from '../../shared/types' import type { GitHistoryOptions, GitHistoryResult } from '../../shared/git-history' import type { SshMutationExpectation } from '../../shared/ssh-types' -import { compareFileNames } from '../../shared/file-name-sort' +import { sortDirEntries } from '../../shared/file-name-sort' import { assertSshMutationExpectation } from '../ssh/ssh-connection-generation' import { buildRgArgs, @@ -513,7 +513,9 @@ export function registerFilesystemHandlers( if (args.connectionId) { throwSite = 'ssh-provider' const provider = requireSshFilesystemProvider(args.connectionId) - return await provider.readDir(args.dirPath) + // Why: re-sort locally — the remote relay may be an older build with + // lexicographic ordering. + return sortDirEntries(await provider.readDir(args.dirPath)) } throwSite = 'authorize' const dirPath = await resolveAuthorizedPath(args.dirPath, store) @@ -528,12 +530,7 @@ export function registerFilesystemHandlers( isSymlink: entry.isSymbolicLink() })) ) - return mapped.sort((a, b) => { - if (a.isDirectory !== b.isDirectory) { - return a.isDirectory ? -1 : 1 - } - return compareFileNames(a.name, b.name) - }) + return sortDirEntries(mapped) } catch (error: unknown) { recordCrashBreadcrumb( 'fs_readdir_error', diff --git a/src/main/ipc/ssh-browse.ts b/src/main/ipc/ssh-browse.ts index 2356de614b0..f2a779ee412 100644 --- a/src/main/ipc/ssh-browse.ts +++ b/src/main/ipc/ssh-browse.ts @@ -3,6 +3,7 @@ import type { SshConnectionManager } from '../ssh/ssh-connection-manager' import type { SshExecOptions } from '../ssh/ssh-connection-utils' import { powerShellCommand, powerShellLiteral } from '../ssh/ssh-remote-powershell' import type { FilesystemPathFlavor } from '../../shared/types' +import { sortDirEntries } from '../../shared/file-name-sort' export type RemoteDirEntry = { name: string @@ -220,13 +221,8 @@ async function runBrowseCommand( } } - // Sort: directories first, then alphabetical - entries.sort((a, b) => { - if (a.isDirectory !== b.isDirectory) { - return a.isDirectory ? -1 : 1 - } - return a.name.localeCompare(b.name) - }) + // Sort: directories first, then natural name order (matches the Explorer) + sortDirEntries(entries) resolveOnce({ entries, resolvedPath, pathFlavor }) } diff --git a/src/main/runtime/orca-runtime-files.ts b/src/main/runtime/orca-runtime-files.ts index 2ab0e0b9cc8..ea5510a44c5 100644 --- a/src/main/runtime/orca-runtime-files.ts +++ b/src/main/runtime/orca-runtime-files.ts @@ -38,7 +38,7 @@ import { resolveRuntimePath } from '../../shared/cross-platform-path' import { PhysicalExitTracker } from '../../shared/physical-exit-tracker' -import { compareFileNames } from '../../shared/file-name-sort' +import { sortDirEntries } from '../../shared/file-name-sort' import type { RuntimeFileListResult, RuntimeFileOpenResult, @@ -1259,7 +1259,9 @@ export class RuntimeFileCommands { if (!provider) { throw new Error(SSH_FILESYSTEM_PROVIDER_UNAVAILABLE_MESSAGE) } - return provider.readDir(target.path) + // Why: re-sort locally — the remote relay may be an older build with + // lexicographic ordering. + return sortDirEntries(await provider.readDir(target.path)) } const dirPath = await resolveAuthorizedPath(target.path, this.host.requireStore()) @@ -1274,12 +1276,7 @@ export class RuntimeFileCommands { } }) ) - return mapped.sort((a, b) => { - if (a.isDirectory !== b.isDirectory) { - return a.isDirectory ? -1 : 1 - } - return compareFileNames(a.name, b.name) - }) + return sortDirEntries(mapped) } async watchFileExplorer( diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 4770f89e234..f016974e245 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -10,6 +10,7 @@ import { normalizeTerminalTitle } from '../../shared/agent-detection' import { extractOscTitleScanTail } from '../../shared/osc-title-scan-tail' +import { sortDirEntries } from '../../shared/file-name-sort' import { isServerDriveListRequest, listWindowsDrives } from './windows-drive-listing' import { extractLastOsc7Uri, extractOscScanTail } from '../daemon/osc7-uri-extraction' import { parseFileUriPathParts } from '../daemon/osc7-file-uri' @@ -17133,12 +17134,7 @@ export class OrcaRuntimeService { isDirectory: entry.isDirectory(), isSymlink: entry.isSymbolicLink() })) - mapped.sort((a, b) => { - if (a.isDirectory !== b.isDirectory) { - return a.isDirectory ? -1 : 1 - } - return a.name.localeCompare(b.name) - }) + sortDirEntries(mapped) return { resolvedPath: dirPath, entries: mapped, diff --git a/src/relay/fs-handler.test.ts b/src/relay/fs-handler.test.ts index 23dd28c35cb..9dc4812ef98 100644 --- a/src/relay/fs-handler.test.ts +++ b/src/relay/fs-handler.test.ts @@ -164,19 +164,23 @@ describe('FsHandler', () => { await expect(dispatcher.callRequest('fs.tempDir')).resolves.toBe(tmpdir()) }) - it('readDir returns sorted entries with directories first', async () => { + it('readDir returns entries directories-first in natural name order', async () => { mkdirSync(path.join(tmpDir, 'subdir')) - writeFileSync(path.join(tmpDir, 'file.txt'), 'hello') - writeFileSync(path.join(tmpDir, 'aaa.txt'), 'world') + for (const name of ['file.txt', '100 - b.txt', '99 - a.txt', '9 - c.txt']) { + writeFileSync(path.join(tmpDir, name), 'x') + } const result = (await dispatcher.callRequest('fs.readDir', { dirPath: tmpDir })) as { name: string isDirectory: boolean }[] - expect(result[0].name).toBe('subdir') - expect(result[0].isDirectory).toBe(true) - expect(result.find((e) => e.name === 'file.txt')).toBeDefined() - expect(result.find((e) => e.name === 'aaa.txt')).toBeDefined() + expect(result[0]).toMatchObject({ name: 'subdir', isDirectory: true }) + expect(result.slice(1).map((e) => e.name)).toEqual([ + '9 - c.txt', + '99 - a.txt', + '100 - b.txt', + 'file.txt' + ]) }) it('readDir reports symlinked directories as directories', async () => { diff --git a/src/relay/fs-handler.ts b/src/relay/fs-handler.ts index 9c146af1450..d07e2bf397f 100644 --- a/src/relay/fs-handler.ts +++ b/src/relay/fs-handler.ts @@ -5,6 +5,7 @@ import { execFile } from 'node:child_process' import { tmpdir } from 'node:os' import { join } from 'node:path' import type { RelayDispatcher, RequestContext } from './dispatcher' +import { sortDirEntries } from '../shared/file-name-sort' import type { RelayContext } from './context' // Why: RelayContext is accepted in the constructor for protocol back-compat // (see docs/relay-fs-allowlist-removal.md), but no longer consulted on FS ops. @@ -148,12 +149,7 @@ export class FsHandler { isSymlink: entry.isSymbolicLink() })) ) - return mapped.sort((a, b) => { - if (a.isDirectory !== b.isDirectory) { - return a.isDirectory ? -1 : 1 - } - return a.name.localeCompare(b.name) - }) + return sortDirEntries(mapped) } private async readFile(params: Record) { diff --git a/src/renderer/src/components/right-sidebar/source-control-tree.test.ts b/src/renderer/src/components/right-sidebar/source-control-tree.test.ts index f585c1c35ef..421b5f3cd06 100644 --- a/src/renderer/src/components/right-sidebar/source-control-tree.test.ts +++ b/src/renderer/src/components/right-sidebar/source-control-tree.test.ts @@ -39,6 +39,24 @@ describe('buildSourceControlTree', () => { ]) }) + it('orders numbered sibling directories naturally, matching the file rows', () => { + const tree = buildSourceControlTree('unstaged', [ + entry({ path: 'migrations/100 - b/a.sql' }), + entry({ path: 'migrations/9 - a/a.sql' }), + entry({ path: 'migrations/99 - c/a.sql' }) + ]) + + expect(labels(flattenSourceControlTree(tree, new Set()))).toEqual([ + 'directory:migrations', + 'directory:migrations/9 - a', + 'file:migrations/9 - a/a.sql', + 'directory:migrations/99 - c', + 'file:migrations/99 - c/a.sql', + 'directory:migrations/100 - b', + 'file:migrations/100 - b/a.sql' + ]) + }) + it('keeps conflict rows ahead of normal rows within a directory', () => { const tree = buildGitStatusSourceControlTree('unstaged', [ entry({ path: 'src/a.ts' }), diff --git a/src/shared/file-name-sort.test.ts b/src/shared/file-name-sort.test.ts index 85046075635..b9bd1c52473 100644 --- a/src/shared/file-name-sort.test.ts +++ b/src/shared/file-name-sort.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest' -import { compareFileNames } from './file-name-sort' +import { compareFileNames, sortDirEntries } from './file-name-sort' describe('compareFileNames', () => { it('sorts numeric name segments naturally instead of lexicographically', () => { @@ -35,4 +35,28 @@ describe('compareFileNames', () => { 'b1.md' ]) }) + + it('breaks numeric-collation ties deterministically instead of readdir order', () => { + // numeric collation ties "2" with "02"; the code-unit fallback keeps a total order. + expect(['2.txt', '02.txt'].sort(compareFileNames)).toEqual(['02.txt', '2.txt']) + expect(['02.txt', '2.txt'].sort(compareFileNames)).toEqual(['02.txt', '2.txt']) + expect(compareFileNames('a.txt', 'a.txt')).toBe(0) + }) +}) + +describe('sortDirEntries', () => { + it('keeps directories first, each group in natural order', () => { + const entries = [ + { name: '10 - notes', isDirectory: false }, + { name: '2 - src', isDirectory: true }, + { name: '9 - docs.txt', isDirectory: false }, + { name: '10 - assets', isDirectory: true } + ] + expect(sortDirEntries(entries).map((e) => e.name)).toEqual([ + '2 - src', + '10 - assets', + '9 - docs.txt', + '10 - notes' + ]) + }) }) diff --git a/src/shared/file-name-sort.ts b/src/shared/file-name-sort.ts index 3c1553440d7..8f55ed17400 100644 --- a/src/shared/file-name-sort.ts +++ b/src/shared/file-name-sort.ts @@ -1,8 +1,29 @@ // Why hoisted: localeCompare with an options object resolves a fresh ICU collator // on every comparison, so a directory sort paid for one per O(n log n) step. // numeric: true orders "99 - a" before "100 - b", matching Finder/Explorer. -export const fileNameCollator = new Intl.Collator(undefined, { numeric: true }) +// Locale pinned to 'en' so local main, renderer, relay, and remote runtime hosts +// all produce one order regardless of each process's LANG (precedent: +// skill-freshness pins 'en' for canonical ordering). +export const fileNameCollator = new Intl.Collator('en', { numeric: true }) export function compareFileNames(a: string, b: string): number { - return fileNameCollator.compare(a, b) + const primary = fileNameCollator.compare(a, b) + if (primary !== 0) { + return primary + } + // Why: numeric collation ties distinct names ("2" vs "02"); fall back to code + // units so sibling order stays a total order instead of readdir order. + return a < b ? -1 : a > b ? 1 : 0 +} + +/** Directories-first, then natural name order — the File Explorer listing contract. */ +export function sortDirEntries( + entries: T[] +): T[] { + return entries.sort((a, b) => { + if (a.isDirectory !== b.isDirectory) { + return a.isDirectory ? -1 : 1 + } + return compareFileNames(a.name, b.name) + }) }