mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 08:03:28 +00:00
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.
This commit is contained in:
@@ -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',
|
||||
|
||||
@@ -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 })
|
||||
}
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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<string, unknown>) {
|
||||
|
||||
@@ -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' }),
|
||||
|
||||
@@ -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'
|
||||
])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<T extends { name: string; isDirectory: boolean }>(
|
||||
entries: T[]
|
||||
): T[] {
|
||||
return entries.sort((a, b) => {
|
||||
if (a.isDirectory !== b.isDirectory) {
|
||||
return a.isDirectory ? -1 : 1
|
||||
}
|
||||
return compareFileNames(a.name, b.name)
|
||||
})
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user