Skip to content

Commit b56b4ef

Browse files
knocking4thcylinderknocking4thcylinder
committed
fix(jj): address review findings across workspaces and clients
1 parent 294b906 commit b56b4ef

17 files changed

Lines changed: 267 additions & 64 deletions

‎apps/mobile/src/features/threads/NewTaskContextPickerScreens.tsx‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -371,7 +371,11 @@ export function NewTaskBranchPickerRouteScreen() {
371371
const renderBranch = useCallback(
372372
({ item, index }: { readonly item: VcsRef; readonly index: number }) => (
373373
<BranchSelectionRow
374-
badge={branchBadgeLabel({ branch: item, project: flow.selectedProject })}
374+
badge={branchBadgeLabel({
375+
branch: item,
376+
project: flow.selectedProject,
377+
terminology: flow.vcsTerminology,
378+
})}
375379
branch={item}
376380
disabled={switchingBranchName !== null}
377381
isFirst={index === 0}
@@ -383,6 +387,7 @@ export function NewTaskBranchPickerRouteScreen() {
383387
[
384388
flow.filteredBranches.length,
385389
flow.selectedProject,
390+
flow.vcsTerminology,
386391
selectBranch,
387392
selectedBranchName,
388393
switchingBranchName,

‎apps/mobile/src/features/threads/git/GitBranchesSheet.tsx‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,10 +176,16 @@ export function GitBranchesSheet(_props: GitBranchesSheetProps) {
176176
<Text className="text-foreground-secondary text-sm">{currentWorktreePath}</Text>
177177
<SheetActionButton
178178
icon="trash"
179-
label={`Remove ${vcsTerminology.workspaceNoun}`}
179+
label={
180+
gitActions.pendingWorkspaceMetadataCleanup
181+
? "Retry clearing thread workspace"
182+
: `Remove ${vcsTerminology.workspaceNoun}`
183+
}
180184
disabled={busy}
181185
onPress={() => {
182-
void gitActions.onRemoveSelectedThreadWorkspace().then(() => navigation.goBack());
186+
void gitActions.onRemoveSelectedThreadWorkspace().then((removed) => {
187+
if (removed) navigation.goBack();
188+
});
183189
}}
184190
/>
185191
</View>

‎apps/mobile/src/state/use-selected-thread-git-actions.ts‎

Lines changed: 67 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useCallback, useEffect, useMemo } from "react";
1+
import { useCallback, useEffect, useMemo, useState } from "react";
22
import { Alert } from "react-native";
33

44
import { EnvironmentProject, EnvironmentThreadShell } from "@t3tools/client-runtime/state/shell";
@@ -39,6 +39,9 @@ export function useSelectedThreadGitActions() {
3939
const pull = useAtomCommand(vcsEnvironment.pull, { reportFailure: false });
4040
const { selectedThread, selectedThreadProject } = useThreadSelection();
4141
const { selectedThreadCwd, selectedThreadWorktreePath } = useSelectedThreadWorktree();
42+
const [pendingWorkspaceMetadataCleanup, setPendingWorkspaceMetadataCleanup] = useState<
43+
string | null
44+
>(null);
4245
const runStackedAction = useAtomCommand(
4346
vcsActionManager.runStackedAction({
4447
environmentId: selectedThread?.environmentId ?? null,
@@ -311,44 +314,80 @@ export function useSelectedThreadGitActions() {
311314
const project = selectedThreadProject;
312315
const worktreePath = selectedThreadWorktreePath;
313316
if (!thread || !project || !worktreePath) {
314-
return;
317+
return false;
315318
}
316-
const confirmed = await new Promise<boolean>((resolve) => {
317-
Alert.alert(
318-
`Remove this ${vcsTerminology.workspaceNoun}?`,
319-
`${worktreePath}\n\nThe thread stays; only the ${vcsTerminology.workspaceNoun} is deleted.`,
320-
[
321-
{ text: "Cancel", style: "cancel", onPress: () => resolve(false) },
322-
{ text: "Remove", style: "destructive", onPress: () => resolve(true) },
323-
],
324-
);
325-
});
326-
if (!confirmed) {
327-
return;
328-
}
329-
const result = await removeWorktree({
330-
environmentId: thread.environmentId,
331-
input: { cwd: project.workspaceRoot, path: worktreePath, force: true },
332-
});
333-
if (AsyncResult.isFailure(result)) {
334-
const error = Cause.squash(result.cause);
335-
showGitActionResult({
336-
type: "error",
337-
title: `Failed to remove ${vcsTerminology.workspaceNoun}`,
338-
description: error instanceof Error ? error.message : "An error occurred.",
319+
if (pendingWorkspaceMetadataCleanup !== worktreePath) {
320+
const confirmed = await new Promise<boolean>((resolve) => {
321+
Alert.alert(
322+
`Remove this ${vcsTerminology.workspaceNoun}?`,
323+
`${worktreePath}\n\nThe thread stays; only the ${vcsTerminology.workspaceNoun} is deleted.`,
324+
[
325+
{ text: "Cancel", style: "cancel", onPress: () => resolve(false) },
326+
{ text: "Remove", style: "destructive", onPress: () => resolve(true) },
327+
],
328+
);
339329
});
340-
return;
330+
if (!confirmed) return false;
331+
332+
let result = await removeWorktree({
333+
environmentId: thread.environmentId,
334+
input: { cwd: project.workspaceRoot, path: worktreePath, force: false },
335+
});
336+
if (AsyncResult.isFailure(result)) {
337+
const error = Cause.squash(result.cause);
338+
const message = error instanceof Error ? error.message : "An error occurred.";
339+
if (message.includes("uncommitted or unbookmarked changes")) {
340+
const discard = await new Promise<boolean>((resolve) => {
341+
Alert.alert(
342+
`Discard changes and remove ${vcsTerminology.workspaceNoun}?`,
343+
`Uncommitted or unbookmarked changes in ${worktreePath} will be lost.`,
344+
[
345+
{ text: "Cancel", style: "cancel", onPress: () => resolve(false) },
346+
{ text: "Discard and remove", style: "destructive", onPress: () => resolve(true) },
347+
],
348+
);
349+
});
350+
if (!discard) return false;
351+
result = await removeWorktree({
352+
environmentId: thread.environmentId,
353+
input: { cwd: project.workspaceRoot, path: worktreePath, force: true },
354+
});
355+
}
356+
}
357+
if (AsyncResult.isFailure(result)) {
358+
const error = Cause.squash(result.cause);
359+
showGitActionResult({
360+
type: "error",
361+
title: `Failed to remove ${vcsTerminology.workspaceNoun}`,
362+
description: error instanceof Error ? error.message : "An error occurred.",
363+
});
364+
return false;
365+
}
341366
}
342-
await syncSelectedThreadBranchState({
367+
const syncResult = await syncSelectedThreadBranchState({
343368
thread,
344369
cwd: project.workspaceRoot,
345370
nextThreadState: { worktreePath: null },
346371
});
372+
if (AsyncResult.isFailure(syncResult)) {
373+
setPendingWorkspaceMetadataCleanup(worktreePath);
374+
const error = Cause.squash(syncResult.cause);
375+
showGitActionResult({
376+
type: "error",
377+
title: `${vcsTerminology.workspaceNounTitle} removed, but thread update failed`,
378+
description:
379+
error instanceof Error ? error.message : "Try clearing the thread workspace again.",
380+
});
381+
return false;
382+
}
383+
setPendingWorkspaceMetadataCleanup(null);
347384
showGitActionResult({
348385
type: "success",
349386
title: `${vcsTerminology.workspaceNounTitle} removed`,
350387
});
388+
return true;
351389
}, [
390+
pendingWorkspaceMetadataCleanup,
352391
removeWorktree,
353392
selectedThread,
354393
selectedThreadProject,
@@ -446,6 +485,7 @@ export function useSelectedThreadGitActions() {
446485
onCreateSelectedThreadBranch,
447486
onCreateSelectedThreadWorktree,
448487
onRemoveSelectedThreadWorkspace,
488+
pendingWorkspaceMetadataCleanup: pendingWorkspaceMetadataCleanup === selectedThreadWorktreePath,
449489
onPullSelectedThreadBranch,
450490
onRunSelectedThreadGitAction,
451491
};

‎apps/server/src/git/GitWorkflowService.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -119,7 +119,7 @@ type WorkflowKind = "git" | "jj";
119119
export class GitWorkflowService extends Context.Service<
120120
GitWorkflowService,
121121
VcsWorkflowOps & {
122-
/** Git specifically, not "a repository this service can serve": a jj workspace answers false. */
122+
/** Whether a Git or Jujutsu driver owns this directory. */
123123
readonly isRepository: (cwd: string) => Effect.Effect<boolean, GitManagerServiceError>;
124124
}
125125
>()("t3/git/GitWorkflowService") {}
@@ -290,7 +290,7 @@ export const make = Effect.gen(function* () {
290290
return GitWorkflowService.of({
291291
isRepository: (cwd) =>
292292
registry.detect({ cwd }).pipe(
293-
Effect.map((handle) => handle !== null),
293+
Effect.map((handle) => handle !== null && handle.kind !== "unknown"),
294294
Effect.mapError(
295295
(cause) =>
296296
new GitManagerError({

‎apps/server/src/jj/JjWorkspaces.test.ts‎

Lines changed: 41 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,8 @@ describeJj("JjWorkspaces.createWorktree", () => {
222222
yield* fileSystem.remove(destination);
223223
const retried = yield* ops.createWorktree({
224224
cwd: root,
225-
refName: "feat/blocked",
225+
refName: "main",
226+
newRefName: "feat/blocked",
226227
path: destination,
227228
});
228229
assert.equal(retried.worktree.refName, "feat/blocked");
@@ -291,7 +292,7 @@ describeJj("JjWorkspaces.removeWorktree", () => {
291292
const created = yield* ops.createWorktree({
292293
cwd: root,
293294
refName: "main",
294-
newRefName: "feat/committed",
295+
newRefName: "topic/committed",
295296
path: null,
296297
});
297298
yield* fileSystem.writeFileString(`${created.worktree.path}/agent.txt`, "work\n");
@@ -306,12 +307,12 @@ describeJj("JjWorkspaces.removeWorktree", () => {
306307

307308
assert.isFalse(yield* fileSystem.exists(created.worktree.path));
308309
const listed = yield* refs.listRefs({ cwd: root, refresh: true });
309-
const bookmark = listed.refs.find((ref) => ref.name === "feat/committed");
310+
const bookmark = listed.refs.find((ref) => ref.name === "topic/committed");
310311
assert.isDefined(bookmark);
311312
const described = yield* runJj(root, [
312313
"log",
313314
"-r",
314-
'bookmarks(exact:"feat/committed")',
315+
'bookmarks(exact:"topic/committed")',
315316
"--no-graph",
316317
"-T",
317318
"description",
@@ -321,6 +322,42 @@ describeJj("JjWorkspaces.removeWorktree", () => {
321322
),
322323
);
323324

325+
it.effect("never advances main when the workspace bookmark was removed", () =>
326+
withRepo(({ fileSystem, ops, root }) =>
327+
Effect.gen(function* () {
328+
const created = yield* ops.createWorktree({
329+
cwd: root,
330+
refName: "main",
331+
newRefName: "topic/removed",
332+
path: null,
333+
});
334+
const originalMain = yield* runJj(root, [
335+
"log",
336+
"-r",
337+
'bookmarks(exact:"main")',
338+
"--no-graph",
339+
"-T",
340+
"commit_id",
341+
]);
342+
yield* runJj(root, ["bookmark", "delete", "topic/removed"]);
343+
yield* fileSystem.writeFileString(`${created.worktree.path}/agent.txt`, "work\n");
344+
yield* runJj(created.worktree.path, ["commit", "-m", "agent work"]);
345+
346+
yield* ops.removeWorktree({ cwd: root, path: created.worktree.path, force: true });
347+
348+
const currentMain = yield* runJj(root, [
349+
"log",
350+
"-r",
351+
'bookmarks(exact:"main")',
352+
"--no-graph",
353+
"-T",
354+
"commit_id",
355+
]);
356+
assert.equal(currentMain, originalMain);
357+
}),
358+
),
359+
);
360+
324361
it.effect("forgets and deletes a clean workspace with force", () =>
325362
withRepo(({ driver, fileSystem, ops, root }) =>
326363
Effect.gen(function* () {

‎apps/server/src/jj/JjWorkspaces.ts‎

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import {
1919
writeBookmarkUpstreamConfig,
2020
type JjRemoteOps,
2121
} from "./JjRemotes.ts";
22-
import { strandedSegmentRows } from "./JjStatus.ts";
22+
import { resolveWorkspaceRefName, strandedSegmentRows } from "./JjStatus.ts";
2323
import { bookmarkDigest, isT3WorkspaceName, workspaceNameForRef } from "./JjWorkspaceNaming.ts";
2424

2525
export { isT3WorkspaceName, workspaceNameForRef } from "./JjWorkspaceNaming.ts";
@@ -172,7 +172,14 @@ export const makeJjWorkspaces = (deps: JjWorkspaceOpsDeps): JjWorkspaceOps => {
172172
}
173173

174174
yield* assertBookmarkUsable(driver, operation, input.cwd, targetRef);
175-
if (input.newRefName !== undefined) {
175+
const localBookmarkExists =
176+
input.newRefName !== undefined &&
177+
(yield* driver
178+
.listBookmarks(input.cwd)
179+
.pipe(mapJjFailure(operation, input.cwd, "Could not list Jujutsu bookmarks."))).some(
180+
(bookmark) => bookmark.remote === null && bookmark.name === input.newRefName,
181+
);
182+
if (input.newRefName !== undefined && !localBookmarkExists) {
176183
yield* run(
177184
operation,
178185
input.cwd,
@@ -306,8 +313,12 @@ export const makeJjWorkspaces = (deps: JjWorkspaceOpsDeps): JjWorkspaceOps => {
306313
);
307314
}
308315

309-
const segmentBookmark = segment.at(-1)?.localBookmarks.toSorted()[0];
310-
if (stranded && segmentBookmark !== undefined) {
316+
const segmentBookmark = yield* resolveWorkspaceRefName(driver, input.path, segment);
317+
if (
318+
stranded &&
319+
segmentBookmark !== null &&
320+
workspaceNameForRef(segmentBookmark) === matched.name
321+
) {
311322
// `jj workspace add -r <bookmark>` leaves the bookmark on the base, so the agent's own
312323
// commits sit on no ref. Moving the bookmark to `@-` first is the jj spelling of what
313324
// git's worktree branch does for free; a failure here must not block the removal.

‎apps/server/src/project/RepositoryIdentityResolver.ts‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -104,11 +104,18 @@ function buildRepositoryIdentity(input: {
104104
};
105105
}
106106

107-
function isDirectorySync(candidate: string): boolean {
107+
function statMarker(candidate: string): NodeFS.Stats | null {
108108
try {
109-
return NodeFS.statSync(candidate).isDirectory();
110-
} catch {
111-
return false;
109+
return NodeFS.statSync(candidate);
110+
} catch (error) {
111+
if (
112+
error instanceof Error &&
113+
"code" in error &&
114+
(error.code === "ENOENT" || error.code === "ENOTDIR")
115+
) {
116+
return null;
117+
}
118+
throw error;
112119
}
113120
}
114121

@@ -121,18 +128,19 @@ function isDirectorySync(candidate: string): boolean {
121128
export function resolveRepositoryRootFromMarkers(cwd: string): string | null {
122129
let current = cwd;
123130
for (;;) {
124-
if (isDirectorySync(NodePath.join(current, ".jj"))) {
131+
if (statMarker(NodePath.join(current, ".jj"))?.isDirectory()) {
125132
const repoPointer = NodePath.join(current, ".jj", "repo");
126-
if (isDirectorySync(repoPointer)) {
133+
if (statMarker(repoPointer)?.isDirectory()) {
127134
return current;
128135
}
136+
const pointerContents = NodeFS.readFileSync(repoPointer, "utf8");
129137
try {
130-
return mainWorkspaceRootFromRepoPointer(current, NodeFS.readFileSync(repoPointer, "utf8"));
138+
return mainWorkspaceRootFromRepoPointer(current, pointerContents);
131139
} catch {
132140
return current;
133141
}
134142
}
135-
if (NodeFS.existsSync(NodePath.join(current, ".git"))) {
143+
if (statMarker(NodePath.join(current, ".git")) !== null) {
136144
return null;
137145
}
138146
const parent = NodePath.dirname(current);
@@ -143,11 +151,20 @@ export function resolveRepositoryRootFromMarkers(cwd: string): string | null {
143151
}
144152
}
145153

154+
function resolveRepositoryRootFromReadableMarkers(cwd: string): string | null {
155+
try {
156+
return resolveRepositoryRootFromMarkers(cwd);
157+
} catch {
158+
// A marker is unreadable; let git resolve the repository from cwd.
159+
return null;
160+
}
161+
}
162+
146163
const resolveRepositoryIdentityCacheKey = Effect.fn("RepositoryIdentityResolver.resolveCacheKey")(
147164
function* (cwd: string) {
148165
const processRunner = yield* ProcessRunner.ProcessRunner;
149166

150-
const jjRoot = resolveRepositoryRootFromMarkers(cwd);
167+
const jjRoot = resolveRepositoryRootFromReadableMarkers(cwd);
151168
if (jjRoot !== null) {
152169
return jjRoot;
153170
}

0 commit comments

Comments
 (0)