fix(plugins): New workspace commands have the same effects as a pick, even from a mount effect

The screen attached the pick side effects (remembering the isolation, syncing the picker's PR
attachment) as listeners in its own useEffect, which runs after a child item's mount effect. A
mount-time setIsolation("worktree") switched the screen but was not saved, and setBaseRef skipped
the attachment sync. The effects are now injected when the setup model is built, so a command and
a pick run the same code from the first render. Closing the picker stays in the picker's handler.
This commit is contained in:
Mohamed Boudra
2026-11-04 14:05:15 +01:00
parent e5bc1fa823
commit cb6cc73683
7 changed files with 113 additions and 69 deletions
@@ -36,14 +36,16 @@ describe("the base ref a plugin reads", () => {
const origin = rows.find((row) => row.kind === "branch" && row.refName.includes("remotes"));
if (!origin) throw new Error("expected an origin row");
const setup = createNewWorkspaceSetup({
locked: false,
project: { id: "p", name: "repo" },
canCreateWorktree: true,
supportsMetadata: true,
});
const chosen: PickerItem[] = [];
setup.onBaseChosen((item) => chosen.push(item));
const setup = createNewWorkspaceSetup(
{
locked: false,
project: { id: "p", name: "repo" },
canCreateWorktree: true,
supportsMetadata: true,
},
{ persistIsolation: () => {}, syncBaseAttachment: (item) => chosen.push(item) },
);
const read = toPluginNewWorkspace({
host: HOST,
view: viewOf(origin),
@@ -93,6 +93,7 @@ import {
startsNewBranch,
type BranchPickerDetail,
type PickerCheckoutRequest,
type PickerItem,
type PickerOptionData,
} from "./new-workspace-picker-item";
import {
@@ -207,19 +208,11 @@ function useWorkspaceIsolation(input: {
// The last isolation choice is remembered alongside the other New Workspace
// form preferences (provider, model, mode). A manual in-screen pick overrides
// the remembered default until the screen remounts.
const { preferences, updatePreferences } = useFormPreferences();
const { preferences } = useFormPreferences();
const { manualIsolation } = useNewWorkspaceSetupState(setup);
const isolation = manualIsolation ?? preferences.isolation ?? "local";
const isWorktree = isolation === "worktree" && canCreateWorktree;
useEffect(
() =>
setup.onIsolationChosen((value) => {
void updatePreferences({ isolation: value });
}),
[setup, updatePreferences],
);
return {
isolation,
setIsolation: setup.chooseIsolation,
@@ -1022,6 +1015,8 @@ export function NewWorkspaceScreen({
supportsMultiplicity: supportsWorkspaceMultiplicity,
pendingAction,
isDraftHandoffActive,
updatePreferences: updateFormPreferences,
setAttachments: chatDraft.setAttachments,
});
const dispatchPickerSelection = setup.dispatchPicker;
const selectedItem = useNewWorkspaceSetupState(setup).pickerSelection.selectedItem;
@@ -1123,15 +1118,12 @@ export function NewWorkspaceScreen({
const displayItem = itemById.get(selectedOptionId);
return displayItem ? pickerItemLabel(displayItem) : "main";
}, [itemById, selectedOptionId]);
const selectPickerItem = setup.chooseBase;
const setDraftAttachments = chatDraft.setAttachments;
useEffect(
() =>
setup.onBaseChosen((item) => {
setDraftAttachments((attachments) => syncPickerPrAttachment({ attachments, item }));
setPickerOpen(false);
}),
[setDraftAttachments, setup],
const selectPickerItem = useCallback(
(item: PickerItem) => {
setup.chooseBase(item);
setPickerOpen(false);
},
[setup],
);
const pluginScreen = useMemo<NewWorkspaceItemsScreen>(
@@ -1635,8 +1627,18 @@ function usePluginSetup(input: {
supportsMultiplicity: boolean;
pendingAction: Parameters<typeof isNewWorkspacePending>[0]["pendingAction"];
isDraftHandoffActive: boolean;
updatePreferences: ReturnType<typeof useFormPreferences>["updatePreferences"];
setAttachments: ReturnType<typeof useAgentInputDraft>["setAttachments"];
}) {
const { serverId, project, supportsMultiplicity, pendingAction, isDraftHandoffActive } = input;
const {
serverId,
project,
supportsMultiplicity,
pendingAction,
isDraftHandoffActive,
updatePreferences,
setAttachments,
} = input;
const worktreeSupport = project
? getWorktreeSupportForHostProject({ project, serverId })
: "unsupported";
@@ -1650,7 +1652,15 @@ function usePluginSetup(input: {
[projectId, projectName],
);
const inputs = { locked, project: pluginProject, canCreateWorktree, supportsMetadata };
const [setup] = useState(() => createNewWorkspaceSetup(inputs));
// Both are stable for the life of the screen: preferences write through the query client, and
// the draft is keyed per mount. A command and a pick run the same effects.
const [setup] = useState(() =>
createNewWorkspaceSetup(inputs, {
persistIsolation: (isolation) => void updatePreferences({ isolation }),
syncBaseAttachment: (item) =>
setAttachments((attachments) => syncPickerPrAttachment({ attachments, item })),
}),
);
useLayoutEffect(() => setup.syncInputs(inputs));
return { setup, pluginProject, canCreateWorktree, isPending: locked };
}
@@ -1,6 +1,6 @@
import { describe, expect, it } from "vitest";
import type { PickerItem } from "../new-workspace-picker-item";
import { createNewWorkspaceSetup, type SetupInputs } from "./setup-model";
import { createNewWorkspaceSetup, type SetupEffects, type SetupInputs } from "./setup-model";
const READY: SetupInputs = {
locked: false,
@@ -9,9 +9,19 @@ const READY: SetupInputs = {
supportsMetadata: true,
};
function recordingEffects() {
const chosen: PickerItem[] = [];
const isolations: string[] = [];
const effects: SetupEffects = {
persistIsolation: (isolation) => isolations.push(isolation),
syncBaseAttachment: (item) => chosen.push(item),
};
return { effects, chosen, isolations };
}
describe("New workspace setup commands", () => {
it("accepts commands from the first moment, before the screen has committed", async () => {
const setup = createNewWorkspaceSetup(READY);
const setup = createNewWorkspaceSetup(READY, recordingEffects().effects);
await setup.commands.setBranch("pas-101");
@@ -19,7 +29,7 @@ describe("New workspace setup commands", () => {
});
it("applies commands that run back to back without waiting for a render", async () => {
const setup = createNewWorkspaceSetup(READY);
const setup = createNewWorkspaceSetup(READY, recordingEffects().effects);
void setup.commands.setBranch("pas-101");
void setup.commands.setMetadata({ issue: "PAS-101", keep: true });
@@ -29,12 +39,9 @@ describe("New workspace setup commands", () => {
expect(setup.getSnapshot().metadata).toEqual({ issue: "PAS-101" });
});
it("changes the model and tells the screen, for a base branch and an isolation choice", async () => {
const setup = createNewWorkspaceSetup(READY);
const chosen: PickerItem[] = [];
const isolations: string[] = [];
setup.onBaseChosen((item) => chosen.push(item));
setup.onIsolationChosen((isolation) => isolations.push(isolation));
it("changes the model and runs the effects of a pick, for a base branch and an isolation choice", async () => {
const { effects, chosen, isolations } = recordingEffects();
const setup = createNewWorkspaceSetup(READY, effects);
await setup.commands.setBaseRef({ type: "branch", name: " release " });
await setup.commands.setIsolation("worktree");
@@ -46,7 +53,7 @@ describe("New workspace setup commands", () => {
});
it("shows a base ref's short name and keeps the exact ref for creation", async () => {
const setup = createNewWorkspaceSetup(READY);
const setup = createNewWorkspaceSetup(READY, recordingEffects().effects);
await setup.commands.setBaseRef({ type: "branch", name: "refs/remotes/origin/release" });
@@ -57,7 +64,7 @@ describe("New workspace setup commands", () => {
});
it("rejects every command while the screen is locked, as the pickers do", async () => {
const setup = createNewWorkspaceSetup({ ...READY, locked: true });
const setup = createNewWorkspaceSetup({ ...READY, locked: true }, recordingEffects().effects);
const results = await Promise.allSettled([
setup.commands.setBranch("x"),
@@ -72,7 +79,7 @@ describe("New workspace setup commands", () => {
});
it("reads the inputs when a command runs, so a project change after render is seen", async () => {
const setup = createNewWorkspaceSetup(READY);
const setup = createNewWorkspaceSetup(READY, recordingEffects().effects);
setup.syncInputs({ ...READY, project: null });
await expect(setup.commands.setBaseRef({ type: "branch", name: "main" })).rejects.toThrow(
@@ -81,11 +88,10 @@ describe("New workspace setup commands", () => {
});
it("rejects what the project or host cannot do", async () => {
const setup = createNewWorkspaceSetup({
...READY,
canCreateWorktree: false,
supportsMetadata: false,
});
const setup = createNewWorkspaceSetup(
{ ...READY, canCreateWorktree: false, supportsMetadata: false },
recordingEffects().effects,
);
await expect(setup.commands.setIsolation("worktree")).rejects.toThrow(
"cannot create worktrees",
@@ -48,22 +48,31 @@ export interface NewWorkspaceSetup {
chooseBase(item: PickerItem): void;
/** The isolation picker's entry, and the one `setIsolation` takes. */
chooseIsolation(isolation: Isolation): void;
/** What the screen does beyond changing the model when a pick lands; returns the remover. */
onBaseChosen(listener: (item: PickerItem) => void): () => void;
onIsolationChosen(listener: (isolation: Isolation) => void): () => void;
syncInputs(inputs: SetupInputs): void;
commands: SetupCommands;
}
/** What a pick does beyond changing the model, the same for the picker and for a command. */
export interface SetupEffects {
/** Remembers the isolation as the default for the next New workspace screen. */
persistIsolation(isolation: Isolation): void;
/** Keeps the picker's change-request attachment in step with the chosen base. */
syncBaseAttachment(item: PickerItem): void;
}
const LOCKED_MESSAGE = "The New workspace screen is locked while the workspace is being created.";
/**
* The New workspace screen's choices as one plain model. The pickers and the plugin commands
* write to it through the same entries, so a command sees what an earlier command or pick left,
* has the effects of a pick, and applies the same rules as the picker it stands in for. It is
* built with its inputs, so a command is valid from the first render.
* built with its inputs and effects, so a command does the same thing from a child's mount effect
* as from a button later.
*/
export function createNewWorkspaceSetup(initial: SetupInputs): NewWorkspaceSetup {
export function createNewWorkspaceSetup(
initial: SetupInputs,
effects: SetupEffects,
): NewWorkspaceSetup {
let state: SetupState = {
pickerSelection: initialPickerSelectionState,
manualIsolation: null,
@@ -71,8 +80,6 @@ export function createNewWorkspaceSetup(initial: SetupInputs): NewWorkspaceSetup
metadata: {},
};
let inputs = initial;
let baseChosen: (item: PickerItem) => void = () => {};
let isolationChosen: (isolation: Isolation) => void = () => {};
const listeners = new Set<() => void>();
const update = (patch: Partial<SetupState>) => {
state = { ...state, ...patch };
@@ -86,11 +93,11 @@ export function createNewWorkspaceSetup(initial: SetupInputs): NewWorkspaceSetup
update({ pickerSelection: reducePickerSelection(state.pickerSelection, event) });
const chooseBase = (item: PickerItem) => {
dispatchPicker({ type: "picker-selected", item });
baseChosen(item);
effects.syncBaseAttachment(item);
};
const chooseIsolation = (isolation: Isolation) => {
update({ manualIsolation: isolation });
isolationChosen(isolation);
effects.persistIsolation(isolation);
};
return {
subscribe(listener) {
@@ -101,18 +108,6 @@ export function createNewWorkspaceSetup(initial: SetupInputs): NewWorkspaceSetup
dispatchPicker,
chooseBase,
chooseIsolation,
onBaseChosen(listener) {
baseChosen = listener;
return () => {
baseChosen = () => {};
};
},
onIsolationChosen(listener) {
isolationChosen = listener;
return () => {
isolationChosen = () => {};
};
},
syncInputs: (next) => {
inputs = next;
},