From 2755596b9fa81629b80fa6cab30ec94dfd261ce7 Mon Sep 17 00:00:00 2001 From: Christoph Leiter Date: Thu, 27 Aug 2026 10:00:25 +0200 Subject: [PATCH] fix(app): align the scrolling diff order with the Changes tree (#3913) The Changes view listed the same files in two different orders. The tree rail put directories before files at each level of the path. The scrolling diff compared the full path byte by byte. A root file such as .gitlab-ci.yml therefore opened the diff, but sat below every directory in the tree. Scrolling the diff did not walk the tree from top to bottom. Two sorts caused this. sortTree in git/diff-tree.ts sorted the tree. compareCheckoutDiffPaths in git/diff-order.ts sorted the flat list at ingest. The tree is now the only ordering authority. orderCheckoutDiffFiles builds the tree and reads the file sequence back out of it. compareCheckoutDiffPaths is deleted, so the two surfaces can no longer disagree. The daemon keeps its own whole-path sort, so daemons that are not updated also get this fix. Commit diffs keep the order that git gives them, because that panel shows no tree rail. --- packages/app/e2e/browser/changes-pane.spec.ts | 32 +++++++++++ packages/app/src/git/diff-order.test.ts | 54 ++++++++++++++----- packages/app/src/git/diff-order.ts | 25 +++++---- packages/app/src/git/diff-tree.ts | 15 ++++-- 4 files changed, 96 insertions(+), 30 deletions(-) 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; });