fix(lanes,git): harden the lane git surface and fix client races in the Changes panel
Server (git-diff.js, git-ops.js, worktree.js, routes/lanes.js): - Every lane git call runs with GIT_LITERAL_PATHSPECS=1 and -z porcelain, so a file name is a file name, never a glob, and odd names parse correctly. - A lane that is a subdirectory of its repo sees and acts on ONLY its own paths (reads, stage/unstage/discard/commit, and gitFacts.files). - Any file named settings.local.json (the lane's router API key) is refused as 403 ESECRET on disk (symlinks resolved) and at every committed rev. - commit refuses with 400 ESTAGEDOUTSIDE while anything outside the lane is staged (checked with --no-renames, so a move into the lane is caught). - push resolves its remote (upstream → only remote → origin), else 400 ENOREMOTE; unstaging a rename unstages both halves. - Git writes serialize on the repo's git dir, not the lane id, so two lanes on one index cannot collide on index.lock. - git refusing an action (a numeric exit status) answers 409 EGIT with git's stderr instead of a 500 carrying a numeric code; one shared parseNumstat. Client: - Changes panel: no stale answer from a previous lane or an older quick-open query repaints the panel; discard disarms on lane switch; Enter on a file path opens it instead of folding; bounded tree expand-all. - Large diffs (> 500 rows) render only the visible rows, with stable width and in-sync split columns; mirrored split scroll is clamped. - Git bar: no "in sync" without counts, a clear no-remote message, branch list re-read after a switch, and no previous-lane facts after a lane switch. - Tooltips no longer flicker or close on unrelated scrolls.
This commit is contained in:
@@ -59,6 +59,8 @@ const OVERVIEW_REFRESH_MS = 15_000;
|
||||
const CONTEXT_STEPS: readonly [number, ...number[]] = [3, 12, 40, 5000];
|
||||
|
||||
const LAST_CONTEXT = CONTEXT_STEPS[CONTEXT_STEPS.length - 1]!;
|
||||
/** Tree reads "expand all" keeps in flight at once. */
|
||||
const TREE_CONCURRENCY = 4;
|
||||
|
||||
type Mode = "overview" | "review" | "file" | "history" | "browse";
|
||||
|
||||
@@ -191,6 +193,94 @@ function IconButton({
|
||||
);
|
||||
}
|
||||
|
||||
/** One overview row. At module scope on purpose: declared inside the panel it
|
||||
* was a new component type every render, so each poll and each keystroke in
|
||||
* the commit box remounted every row (focus dropped to <body>). */
|
||||
function ChangeRow({
|
||||
f,
|
||||
range,
|
||||
busy,
|
||||
armed,
|
||||
onOpen,
|
||||
onAct,
|
||||
onArm,
|
||||
}: {
|
||||
f: LaneChangeFile;
|
||||
range: LaneChangeRange;
|
||||
busy: boolean;
|
||||
/** This row's discard has been clicked once and awaits the second click. */
|
||||
armed: boolean;
|
||||
onOpen: (path: string) => void;
|
||||
onAct: (action: "stage" | "unstage" | "discard", path: string, confirm?: boolean) => void;
|
||||
onArm: (path: string) => void;
|
||||
}) {
|
||||
const { t } = useTranslation(["lanes"]);
|
||||
return (
|
||||
<li className="group flex items-baseline gap-1">
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => onOpen(f.path)}
|
||||
className="flex min-w-0 flex-1 items-baseline gap-1.5 rounded px-1 py-0.5 text-left hover:bg-surface-2"
|
||||
>
|
||||
<span
|
||||
className={`w-3 shrink-0 font-mono text-[11px] font-semibold ${statusClass(
|
||||
f.status,
|
||||
f.untracked
|
||||
)}`}
|
||||
>
|
||||
{f.untracked ? "A" : f.status.trim()[0] || "M"}
|
||||
</span>
|
||||
<span
|
||||
className="min-w-0 flex-1 truncate font-mono text-[11px] text-fg-primary"
|
||||
title={f.path}
|
||||
>
|
||||
{f.path}
|
||||
</span>
|
||||
{f.conflicted && (
|
||||
<span className="shrink-0 text-[10px] text-status-danger">{t("changes.conflicted")}</span>
|
||||
)}
|
||||
<Delta added={f.added} deleted={f.deleted} binary={f.binary} />
|
||||
</button>
|
||||
{range === "work" && (
|
||||
<span className="flex shrink-0 items-center gap-0.5">
|
||||
{f.staged ? (
|
||||
<IconButton
|
||||
label={t("changes.unstage")}
|
||||
disabled={busy}
|
||||
onClick={() => onAct("unstage", f.path)}
|
||||
>
|
||||
<Minus className="h-3 w-3" />
|
||||
</IconButton>
|
||||
) : null}
|
||||
{f.unstaged || f.untracked ? (
|
||||
<IconButton
|
||||
label={t("changes.stage")}
|
||||
disabled={busy}
|
||||
onClick={() => onAct("stage", f.path)}
|
||||
>
|
||||
<Plus className="h-3 w-3" />
|
||||
</IconButton>
|
||||
) : null}
|
||||
<IconButton
|
||||
label={armed ? t("changes.discardConfirm") : t("changes.discard")}
|
||||
disabled={busy}
|
||||
danger
|
||||
onClick={() => {
|
||||
// Two clicks, always: the first arms the row, the second is the
|
||||
// only one that reaches the server (which itself refuses the
|
||||
// call without `confirm: true`).
|
||||
if (armed) onAct("discard", f.path, true);
|
||||
else onArm(f.path);
|
||||
}}
|
||||
>
|
||||
<Trash2 className={`h-3 w-3 ${armed ? "text-status-danger" : ""}`} />
|
||||
</IconButton>
|
||||
</span>
|
||||
)}
|
||||
</li>
|
||||
);
|
||||
}
|
||||
|
||||
export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
const { t } = useTranslation(["lanes"]);
|
||||
const [range, setRange] = useState<LaneChangeRange>("work");
|
||||
@@ -227,7 +317,21 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
const [message, setMessage] = useState("");
|
||||
const [error, setError] = useState<string | null>(null);
|
||||
|
||||
// Every diff read gets a ticket; only the newest may land. A lane switch (or
|
||||
// a mode/range change) re-issues the read, and without this a slow reply for
|
||||
// the previous lane painted its files — and its discard buttons — over this one.
|
||||
const loadSeq = useRef(0);
|
||||
// The same guard for the side reads (history, tree, whole file), which only
|
||||
// race across a lane switch: a reply for a lane no longer shown is dropped.
|
||||
const laneNow = useRef(laneId);
|
||||
laneNow.current = laneId;
|
||||
const forLane =
|
||||
<T,>(id: number, fn: (v: T) => void) =>
|
||||
(v: T) => {
|
||||
if (laneNow.current === id) fn(v);
|
||||
};
|
||||
const load = useCallback(async () => {
|
||||
const seq = ++loadSeq.current;
|
||||
setLoading(true);
|
||||
try {
|
||||
const result = await api.lanes.changes(laneId, {
|
||||
@@ -236,12 +340,30 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
...(mode === "file" && openFile ? { path: openFile, context } : {}),
|
||||
...(mode === "review" ? { full: true } : {}),
|
||||
});
|
||||
if (seq !== loadSeq.current) return;
|
||||
setData(result);
|
||||
setError(null);
|
||||
} catch (err) {
|
||||
if (seq !== loadSeq.current) return;
|
||||
// A tracked file opened from Files that has nothing to diff: a normal
|
||||
// state, not a fault. The whole-file view is the answer; no banner.
|
||||
if (mode === "file" && (err as { code?: string })?.code === "ENOFILECHANGE") {
|
||||
setData({
|
||||
available: true,
|
||||
range,
|
||||
base: null,
|
||||
baseRef: null,
|
||||
sha,
|
||||
context,
|
||||
files: [],
|
||||
truncated: false,
|
||||
});
|
||||
setError(null);
|
||||
return;
|
||||
}
|
||||
setError(err instanceof Error ? err.message : String(err));
|
||||
} finally {
|
||||
setLoading(false);
|
||||
if (seq === loadSeq.current) setLoading(false);
|
||||
}
|
||||
}, [laneId, range, sha, mode, openFile, context]);
|
||||
|
||||
@@ -260,7 +382,7 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
if (mode !== "history") return;
|
||||
void api.lanes
|
||||
.gitLog(laneId, { limit: 50 })
|
||||
.then((r) => setCommits(r.commits))
|
||||
.then(forLane(laneId, (r) => setCommits(r.commits)))
|
||||
.catch((err) => setError(err instanceof Error ? err.message : String(err)));
|
||||
}, [mode, laneId]);
|
||||
|
||||
@@ -268,7 +390,7 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
if (mode !== "browse" || query.trim() || "" in treeChildren) return;
|
||||
void api.lanes
|
||||
.gitTree(laneId, "")
|
||||
.then((r) => setTreeChildren((prev) => ({ ...prev, "": r.entries })))
|
||||
.then(forLane(laneId, (r) => setTreeChildren((prev) => ({ ...prev, "": r.entries }))))
|
||||
.catch((err) => setError(err instanceof Error ? err.message : String(err)));
|
||||
// `treeChildren` is the cache this fills — reading it as a dep would loop.
|
||||
// eslint-disable-next-line react-hooks/exhaustive-deps
|
||||
@@ -284,36 +406,56 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
if (path in treeChildren) return;
|
||||
void api.lanes
|
||||
.gitTree(laneId, path)
|
||||
.then((r) => setTreeChildren((prev) => ({ ...prev, [path]: r.entries })))
|
||||
.then(forLane(laneId, (r) => setTreeChildren((prev) => ({ ...prev, [path]: r.entries }))))
|
||||
.catch((err) => setError(err instanceof Error ? err.message : String(err)));
|
||||
};
|
||||
|
||||
const [treeBusy, setTreeBusy] = useState(false);
|
||||
// Which lane `treeChildren` was filled for; the reset effect moves it.
|
||||
const treeLane = useRef(laneId);
|
||||
|
||||
// Fetches whatever level isn't cached yet, depth-first, so "expand all"
|
||||
// works even on a tree nothing has been opened in.
|
||||
const expandAllDirs = async () => {
|
||||
// A walk is many reads long; a lane switch mid-walk must not land lane A's
|
||||
// tree in lane B, nor start from a cache the switch has not cleared yet.
|
||||
const lane = laneId;
|
||||
setTreeBusy(true);
|
||||
const cache = { ...treeChildren };
|
||||
const cache = treeLane.current === lane ? { ...treeChildren } : {};
|
||||
const toExpand = new Set(expandedDirs);
|
||||
const walk = async (path: string) => {
|
||||
// Level by level, at most TREE_CONCURRENCY reads in flight: one request
|
||||
// per folder all at once is thousands on a monorepo.
|
||||
const visit = async (path: string, next: string[]) => {
|
||||
let entries = cache[path];
|
||||
if (!entries) {
|
||||
entries = (await api.lanes.gitTree(laneId, path)).entries;
|
||||
entries = (await api.lanes.gitTree(lane, path)).entries;
|
||||
cache[path] = entries;
|
||||
}
|
||||
const dirs = entries.filter((e) => e.dir);
|
||||
for (const d of dirs) toExpand.add(d.path);
|
||||
await Promise.all(dirs.map((d) => walk(d.path)));
|
||||
for (const e of entries) {
|
||||
if (!e.dir) continue;
|
||||
toExpand.add(e.path);
|
||||
next.push(e.path);
|
||||
}
|
||||
};
|
||||
try {
|
||||
await walk("");
|
||||
for (let level = [""]; level.length && laneNow.current === lane; ) {
|
||||
const next: string[] = [];
|
||||
// Workers share one iterator, so each path is taken exactly once.
|
||||
const it = level.values();
|
||||
await Promise.all(
|
||||
Array.from({ length: TREE_CONCURRENCY }, async () => {
|
||||
for (const path of it) await visit(path, next);
|
||||
})
|
||||
);
|
||||
level = next;
|
||||
}
|
||||
if (laneNow.current !== lane) return;
|
||||
setTreeChildren(cache);
|
||||
setExpandedDirs(toExpand);
|
||||
} catch (err) {
|
||||
setError(err instanceof Error ? err.message : String(err));
|
||||
if (laneNow.current === lane) setError(err instanceof Error ? err.message : String(err));
|
||||
} finally {
|
||||
setTreeBusy(false);
|
||||
if (laneNow.current === lane) setTreeBusy(false);
|
||||
}
|
||||
};
|
||||
|
||||
@@ -323,13 +465,19 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
setHits(null);
|
||||
return;
|
||||
}
|
||||
// `live` drops an answer that a newer query, a mode change or a lane
|
||||
// switch has overtaken — a slow "main" must not overwrite "mainx".
|
||||
let live = true;
|
||||
const timer = window.setTimeout(() => {
|
||||
void api.lanes
|
||||
.gitLs(laneId, q)
|
||||
.then((r) => setHits(r.paths))
|
||||
.catch(() => setHits([]));
|
||||
.then((r) => live && setHits(r.paths))
|
||||
.catch(() => live && setHits([]));
|
||||
}, 150);
|
||||
return () => window.clearTimeout(timer);
|
||||
return () => {
|
||||
live = false;
|
||||
window.clearTimeout(timer);
|
||||
};
|
||||
}, [mode, laneId, query]);
|
||||
|
||||
// The whole-file read is a different endpoint from the diff, so it is its
|
||||
@@ -338,8 +486,10 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
if (mode !== "file" || !whole || !openFile) return;
|
||||
void api.lanes
|
||||
.gitFile(laneId, openFile, range === "commit" && sha ? sha : "work")
|
||||
.then((r) =>
|
||||
setText(r.available ? { path: r.path, lines: r.lines, truncated: r.truncated } : null)
|
||||
.then(
|
||||
forLane(laneId, (r) =>
|
||||
setText(r.available ? { path: r.path, lines: r.lines, truncated: r.truncated } : null)
|
||||
)
|
||||
)
|
||||
.catch((err) => setError(err instanceof Error ? err.message : String(err)));
|
||||
}, [mode, whole, openFile, laneId, range, sha]);
|
||||
@@ -352,9 +502,19 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
setSha(null);
|
||||
setRange("work");
|
||||
setTreeChildren({});
|
||||
treeLane.current = laneId;
|
||||
setExpandedDirs(new Set());
|
||||
setTreeBusy(false);
|
||||
setBusy(false);
|
||||
setError(null);
|
||||
setCommits(null);
|
||||
setData(null);
|
||||
setCollapsed(new Set());
|
||||
// An armed discard names a PATH, not a file in a lane — carried over, one
|
||||
// click would discard the same path in the lane just switched to.
|
||||
setConfirmDiscard(null);
|
||||
// Same for a half-typed commit message: it describes the OTHER lane's work.
|
||||
setMessage("");
|
||||
returnTo.current = null;
|
||||
pendingScroll.current = null;
|
||||
}, [laneId]);
|
||||
@@ -384,34 +544,41 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
setMode("file");
|
||||
};
|
||||
|
||||
// An action that resolves after a lane switch belongs to the lane left: its
|
||||
// `load` is the pre-switch one and would take a fresh ticket and paint the
|
||||
// old lane's files here, so everything after the await is dropped.
|
||||
const act = async (action: "stage" | "unstage" | "discard", path: string, confirm?: boolean) => {
|
||||
const lane = laneId;
|
||||
setBusy(true);
|
||||
setError(null);
|
||||
try {
|
||||
await api.lanes.gitAction(laneId, action, { paths: [path], ...(confirm ? { confirm } : {}) });
|
||||
await api.lanes.gitAction(lane, action, { paths: [path], ...(confirm ? { confirm } : {}) });
|
||||
if (laneNow.current !== lane) return;
|
||||
setConfirmDiscard(null);
|
||||
await load();
|
||||
} catch (err) {
|
||||
setError(err instanceof Error ? err.message : String(err));
|
||||
if (laneNow.current === lane) setError(err instanceof Error ? err.message : String(err));
|
||||
} finally {
|
||||
setBusy(false);
|
||||
if (laneNow.current === lane) setBusy(false);
|
||||
}
|
||||
};
|
||||
|
||||
const commitStaged = async () => {
|
||||
const lane = laneId;
|
||||
setBusy(true);
|
||||
setError(null);
|
||||
try {
|
||||
// `only: "staged"` — the whole point of staging file by file is that the
|
||||
// commit takes the index as it stands, with no `add -A` sweeping the
|
||||
// rest of the tree in behind it.
|
||||
await api.lanes.gitAction(laneId, "commit", { message: message.trim(), only: "staged" });
|
||||
await api.lanes.gitAction(lane, "commit", { message: message.trim(), only: "staged" });
|
||||
if (laneNow.current !== lane) return;
|
||||
setMessage("");
|
||||
await load();
|
||||
} catch (err) {
|
||||
setError(err instanceof Error ? err.message : String(err));
|
||||
if (laneNow.current === lane) setError(err instanceof Error ? err.message : String(err));
|
||||
} finally {
|
||||
setBusy(false);
|
||||
if (laneNow.current === lane) setBusy(false);
|
||||
}
|
||||
};
|
||||
|
||||
@@ -427,72 +594,13 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
const stagedCount = range === "work" ? files.filter((f) => f.staged).length : 0;
|
||||
const current = mode === "file" ? files.find((f) => f.path === openFile) : undefined;
|
||||
|
||||
const Row = ({ f }: { f: LaneChangeFile }) => (
|
||||
<li className="group flex items-baseline gap-1">
|
||||
<button
|
||||
type="button"
|
||||
onClick={() => show(f.path)}
|
||||
className="flex min-w-0 flex-1 items-baseline gap-1.5 rounded px-1 py-0.5 text-left hover:bg-surface-2"
|
||||
>
|
||||
<span
|
||||
className={`w-3 shrink-0 font-mono text-[11px] font-semibold ${statusClass(
|
||||
f.status,
|
||||
f.untracked
|
||||
)}`}
|
||||
>
|
||||
{f.untracked ? "A" : f.status.trim()[0] || "M"}
|
||||
</span>
|
||||
<span
|
||||
className="min-w-0 flex-1 truncate font-mono text-[11px] text-fg-primary"
|
||||
title={f.path}
|
||||
>
|
||||
{f.path}
|
||||
</span>
|
||||
{f.conflicted && (
|
||||
<span className="shrink-0 text-[10px] text-status-danger">{t("changes.conflicted")}</span>
|
||||
)}
|
||||
<Delta added={f.added} deleted={f.deleted} binary={f.binary} />
|
||||
</button>
|
||||
{range === "work" && (
|
||||
<span className="flex shrink-0 items-center gap-0.5">
|
||||
{f.staged ? (
|
||||
<IconButton
|
||||
label={t("changes.unstage")}
|
||||
disabled={busy}
|
||||
onClick={() => void act("unstage", f.path)}
|
||||
>
|
||||
<Minus className="h-3 w-3" />
|
||||
</IconButton>
|
||||
) : null}
|
||||
{f.unstaged || f.untracked ? (
|
||||
<IconButton
|
||||
label={t("changes.stage")}
|
||||
disabled={busy}
|
||||
onClick={() => void act("stage", f.path)}
|
||||
>
|
||||
<Plus className="h-3 w-3" />
|
||||
</IconButton>
|
||||
) : null}
|
||||
<IconButton
|
||||
label={confirmDiscard === f.path ? t("changes.discardConfirm") : t("changes.discard")}
|
||||
disabled={busy}
|
||||
danger
|
||||
onClick={() => {
|
||||
// Two clicks, always: the first arms the row, the second is the
|
||||
// only one that reaches the server (which itself refuses the
|
||||
// call without `confirm: true`).
|
||||
if (confirmDiscard === f.path) void act("discard", f.path, true);
|
||||
else setConfirmDiscard(f.path);
|
||||
}}
|
||||
>
|
||||
<Trash2
|
||||
className={`h-3 w-3 ${confirmDiscard === f.path ? "text-status-danger" : ""}`}
|
||||
/>
|
||||
</IconButton>
|
||||
</span>
|
||||
)}
|
||||
</li>
|
||||
);
|
||||
// The armed path left the list (committed, reverted, lane switched): disarm,
|
||||
// so a later reappearance starts from the first click again.
|
||||
useEffect(() => {
|
||||
if (confirmDiscard && data && !files.some((f) => f.path === confirmDiscard)) {
|
||||
setConfirmDiscard(null);
|
||||
}
|
||||
}, [data, files, confirmDiscard]);
|
||||
|
||||
return (
|
||||
<div data-testid="lane-changes" className="flex min-h-0 flex-1 flex-col">
|
||||
@@ -770,13 +878,22 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
<p className="text-[11px] text-fg-muted">{t("changes.loading")}</p>
|
||||
) : !data.available ? (
|
||||
<p className="text-[11px] text-fg-muted">{t("changes.unavailable")}</p>
|
||||
) : files.length === 0 ? (
|
||||
) : files.length === 0 && mode !== "file" ? (
|
||||
<p className="text-[11px] text-fg-muted">{t(`changes.empty.${range}`)}</p>
|
||||
) : mode === "overview" ? (
|
||||
<>
|
||||
<ul data-testid="lane-changes-overview" className="space-y-0.5">
|
||||
{files.map((f) => (
|
||||
<Row key={f.path} f={f} />
|
||||
<ChangeRow
|
||||
key={f.path}
|
||||
f={f}
|
||||
range={range}
|
||||
busy={busy}
|
||||
armed={confirmDiscard === f.path}
|
||||
onOpen={show}
|
||||
onAct={(...a) => void act(...a)}
|
||||
onArm={setConfirmDiscard}
|
||||
/>
|
||||
))}
|
||||
</ul>
|
||||
{stagedCount > 0 && (
|
||||
@@ -849,6 +966,9 @@ export function LaneChangesPanel({ laneId }: { laneId: number }) {
|
||||
tabIndex={0}
|
||||
onClick={() => toggleCollapsed(f.path)}
|
||||
onKeyDown={(e) => {
|
||||
// Only the header's own keys: Enter on the path button
|
||||
// inside bubbles here and must open, not fold.
|
||||
if (e.target !== e.currentTarget) return;
|
||||
if (e.key === "Enter" || e.key === " ") {
|
||||
e.preventDefault();
|
||||
toggleCollapsed(f.path);
|
||||
|
||||
Reference in New Issue
Block a user