diff --git a/packages/app/e2e/browser/changes-pane.spec.ts b/packages/app/e2e/browser/changes-pane.spec.ts index 9efaf379e1..d1feb1699e 100644 --- a/packages/app/e2e/browser/changes-pane.spec.ts +++ b/packages/app/e2e/browser/changes-pane.spec.ts @@ -19,6 +19,10 @@ interface WorkspaceFixtureOptions { includeDeletedFile?: boolean; includeNestedFolders?: boolean; includeRenamedFile?: boolean; + // A root-level file whose name sorts BEFORE the root-level "src" directory. + // zz-untracked.txt cannot stand in for it: "src" < "zz", so that file lands + // last whether paths are compared whole or segment by segment. + includeRootFileSortingFirst?: boolean; includeUntrackedFile?: boolean; } @@ -461,6 +465,31 @@ test("changes tree aligns every file status after its diff stat", async ({ page expect(statusBounds.x - (statBounds.x + statBounds.width)).toBeGreaterThanOrEqual(8); }); +test("the scrolling diff lists files in changes tree order", async ({ page }) => { + const workspace = await createWorkspaceWithMountedTabDiff({ + includeNestedFolders: true, + includeRootFileSortingFirst: true, + }); + await useUnwrappedDiffLines(page); + await openWorkspaceChanges(page, workspace); + + const tree = changesTree(page); + const treeNames = tree.locator('[data-testid^="diff-tree-file-"][data-testid$="-name"]'); + const diffNames = page.locator('[data-testid^="diff-file-"][data-testid$="-name"]'); + + // Directories sort before files at every level, so the root note lands last + // even though "a-root-note.txt" compares below "src/..." as a whole string. + const expected = ["changed.ts", "root.ts", "use-mounted-tab-set.ts", "a-root-note.txt"]; + + // Equal counts also prove no folder is collapsed: flattenDiffTree drops the + // descendants of a collapsed folder, which would make the tree a subsequence + // of the diff rather than a match, and mask a real ordering difference. + await expect(treeNames).toHaveCount(expected.length); + await expect(diffNames).toHaveCount(expected.length); + await expect(treeNames).toHaveText(expected); + await expect(diffNames).toHaveText(expected); +}); + test("changes context menu recursively collapses descendant folders", async ({ page }) => { const workspace = await createWorkspaceWithMountedTabDiff({ includeNestedFolders: true }); await useUnwrappedDiffLines(page); @@ -1285,6 +1314,9 @@ async function createWorkspaceWithMountedTabDiff( if (options.includeUntrackedFile) { await writeFile(path.join(repo.path, "zz-untracked.txt"), "remove me\n"); } + if (options.includeRootFileSortingFirst) { + await writeFile(path.join(repo.path, "a-root-note.txt"), "root note\n"); + } if (options.includeDeletedFile) { await unlink(path.join(repo.path, "src/zz-deleted.ts")); } diff --git a/packages/app/src/git/diff-order.test.ts b/packages/app/src/git/diff-order.test.ts index 921e343b53..3d38534a33 100644 --- a/packages/app/src/git/diff-order.test.ts +++ b/packages/app/src/git/diff-order.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { compareCheckoutDiffPaths, orderCheckoutDiffFiles } from "./diff-order"; +import { orderCheckoutDiffFiles } from "./diff-order"; function createFile(path: string, additions = 0) { return { @@ -12,21 +12,51 @@ function createFile(path: string, additions = 0) { }; } +function order(paths: string[]): string[] { + return orderCheckoutDiffFiles(paths.map((path) => createFile(path))).map((file) => file.path); +} + describe("checkout diff ordering", () => { - it("compares paths deterministically", () => { - expect(compareCheckoutDiffPaths("a.ts", "b.ts")).toBeLessThan(0); - expect(compareCheckoutDiffPaths("b.ts", "a.ts")).toBeGreaterThan(0); - expect(compareCheckoutDiffPaths("same.ts", "same.ts")).toBe(0); + it("sorts sibling files by name", () => { + expect(order(["zeta.ts", "alpha.ts", "beta.ts"])).toEqual(["alpha.ts", "beta.ts", "zeta.ts"]); }); - it("sorts files by path", () => { - const ordered = orderCheckoutDiffFiles([ - createFile("zeta.ts"), - createFile("alpha.ts"), - createFile("beta.ts"), - ]); + it("puts a directory before a file that sits beside it", () => { + expect(order(["z.ts", "a/b.ts"])).toEqual(["a/b.ts", "z.ts"]); + }); - expect(ordered.map((file) => file.path)).toEqual(["alpha.ts", "beta.ts", "zeta.ts"]); + it("keeps loose root files below root directories", () => { + // The reported bug: comparing whole paths byte-for-byte put ".gitlab-ci.yml" + // first (because "." < "b"), while the tree rail listed it last. + expect( + order([ + ".gitlab-ci.yml", + "process-compose.yaml", + "bin/lib/node-stamp.sh", + "samera-one/package.json", + ]), + ).toEqual([ + "bin/lib/node-stamp.sh", + "samera-one/package.json", + ".gitlab-ci.yml", + "process-compose.yaml", + ]); + }); + + it("ranks a nested directory above a shallower file with the same prefix", () => { + // "src/a.ts" < "src/a/z.ts" as whole strings, but "a" is a directory here. + expect(order(["src/a.ts", "src/a/z.ts"])).toEqual(["src/a/z.ts", "src/a.ts"]); + }); + + it("orders a directory ahead of a file sharing its name", () => { + // Converting an extensionless file into a directory puts both in one diff. + expect(order(["bin/stack", "bin/stack/run"])).toEqual(["bin/stack/run", "bin/stack"]); + }); + + it("returns lists too short to reorder untouched", () => { + const single = [createFile("only.ts")]; + expect(orderCheckoutDiffFiles([])).toEqual([]); + expect(orderCheckoutDiffFiles(single)).toBe(single); }); it("preserves relative order for equal paths", () => { diff --git a/packages/app/src/git/diff-order.ts b/packages/app/src/git/diff-order.ts index 127121ed36..d57bb0ef29 100644 --- a/packages/app/src/git/diff-order.ts +++ b/packages/app/src/git/diff-order.ts @@ -1,19 +1,18 @@ -import type { SubscribeCheckoutDiffResponse } from "@getpaseo/protocol/messages"; - -type ParsedDiffFile = SubscribeCheckoutDiffResponse["payload"]["files"][number]; - -export function compareCheckoutDiffPaths(left: string, right: string): number { - if (left === right) { - return 0; - } - return left < right ? -1 : 1; -} +import type { ParsedDiffFile } from "@getpaseo/protocol/messages"; +import { buildDiffTree, flattenDiffTree } from "@/git/diff-tree"; +// The Changes tree is the single ordering authority: `sortTree` (diff-tree.ts) +// ranks directories before files within a level, then compares names. Surfaces +// that render the flat file list — the scrolling diff — derive their order from +// the tree here instead of reimplementing the rule, so the two cannot disagree. export function orderCheckoutDiffFiles(files: ParsedDiffFile[]): ParsedDiffFile[] { if (files.length < 2) { return files; } - const ordered = [...files]; - ordered.sort((a, b) => compareCheckoutDiffPaths(a.path, b.path)); - return ordered; + // Path compression only merges display rows for single-child directory + // chains, so it is skipped; the collapsed set is empty because the flat list + // always carries every file, whatever the rail has collapsed. + return flattenDiffTree(buildDiffTree(files), new Set()).flatMap((row) => + row.kind === "file" ? [row.file] : [], + ); } diff --git a/packages/app/src/git/diff-tree.ts b/packages/app/src/git/diff-tree.ts index 9069b608dc..49709cd5e2 100644 --- a/packages/app/src/git/diff-tree.ts +++ b/packages/app/src/git/diff-tree.ts @@ -1,8 +1,13 @@ import type { ParsedDiffFile } from "@/git/use-diff-query"; -// Builds a directory hierarchy from the flat, path-sorted `ParsedDiffFile[]` the -// Changes view renders. The tree renders on every form factor, consistent with -// the Files explorer. +// Builds a directory hierarchy from the `ParsedDiffFile[]` the Changes view +// renders. The tree renders on every form factor, consistent with the Files +// explorer. +// +// `sortTree` below is the single ordering authority for the Changes view. The +// flat scrolling diff does not sort for itself — `orderCheckoutDiffFiles` +// (diff-order.ts) builds this tree and reads its file sequence back out — so +// changing `sortTree` reorders both surfaces together, by construction. // // Directory nodes are keyed by their FULL uncompressed path (e.g. "packages/app/src"). // That path is the stable identity used to persist folder-collapse state, so the @@ -55,8 +60,8 @@ function sortTree(node: DiffTreeDirNode): void { // directories before files within a level return a.kind === "dir" ? -1 : 1; } - // Plain ASCII comparison, matching compareCheckoutDiffPaths (diff-order.ts) - // so the tree order is consistent with the rest of the Changes view. + // Plain ASCII, not localeCompare: the order must not shift with the + // device locale, and every surface derives from this comparison. if (a.name === b.name) return 0; return a.name < b.name ? -1 : 1; });