mirror of
https://github.com/getpaseo/paseo.git
synced 2026-11-02 22:52:58 +00:00
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:
+3
-2
@@ -691,8 +691,9 @@ metadata), so a command is valid from a child's first effect, which runs before
|
||||
effects. `syncInputs` refreshes those inputs in a layout effect on every commit, and commands read
|
||||
them when they run, never from the render that built them. The pickers and the commands pick
|
||||
through the same `chooseBase` and `chooseIsolation`, so a plugin command has the effects of a
|
||||
pick; the screen supplies those effects (syncing the picker's PR attachment, remembering the
|
||||
isolation) with `onBaseChosen` and `onIsolationChosen`. Add a capability by extending the store;
|
||||
pick; the screen injects those effects when it builds the store (syncing the picker's PR attachment,
|
||||
remembering the isolation), so a command from a mount effect does the same as a click. Closing the
|
||||
picker stays in the picker's own handler. Add a capability by extending the store;
|
||||
do not read screen state from a plugin component.
|
||||
|
||||
`ensureWorkspace` sends the store's `branch` as `source.branchName` whenever the worktree cuts a
|
||||
|
||||
@@ -48,13 +48,26 @@ test("a plugin's branch applies when the project has no default base to branch o
|
||||
);
|
||||
});
|
||||
|
||||
test("an isolation set from a mount effect is remembered like a pick", async ({ page }) => {
|
||||
test.setTimeout(180_000);
|
||||
await withNewWorkspaceIssuesExample(
|
||||
page,
|
||||
{ compact: false, setsOnMount: true },
|
||||
async (screen) => {
|
||||
await screen.expectIsolation("worktree");
|
||||
await screen.reopenWithoutPlugin();
|
||||
await screen.expectIsolation("worktree");
|
||||
},
|
||||
);
|
||||
});
|
||||
|
||||
test("an item can set the branch and metadata from its mount effect", async ({ page }) => {
|
||||
test.setTimeout(180_000);
|
||||
await withNewWorkspaceIssuesExample(
|
||||
page,
|
||||
{ compact: false, setsOnMount: true },
|
||||
async (screen) => {
|
||||
await screen.chooseWorktree();
|
||||
await screen.expectIsolation("worktree");
|
||||
const created = await screen.createWorkspace();
|
||||
expect(created.branch).toBe("set-on-mount");
|
||||
expect(created.metadata).toEqual({ "new-workspace-issues.issue": "ON-MOUNT" });
|
||||
|
||||
@@ -1,12 +1,13 @@
|
||||
import { execFileSync } from "node:child_process";
|
||||
import { writeFile } from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import { test, type Page } from "@playwright/test";
|
||||
import { expect, test, type Page } from "@playwright/test";
|
||||
import { gotoAppShell } from "./app";
|
||||
import {
|
||||
archiveWorkspaceFromDaemon,
|
||||
assertNewWorkspaceSidebarAndHeader,
|
||||
connectNewWorkspaceDaemonClient,
|
||||
expectWorkspaceIsolationSelected,
|
||||
openNewWorkspaceComposer,
|
||||
openProjectViaDaemon,
|
||||
submitNewWorkspacePrompt,
|
||||
@@ -32,6 +33,7 @@ import { Action } from "@getpaseo/plugin/client/ui";
|
||||
function OnMount() {
|
||||
const newWorkspace = useNewWorkspace();
|
||||
useEffect(() => {
|
||||
void newWorkspace.setIsolation("worktree");
|
||||
void newWorkspace.setBranch("set-on-mount");
|
||||
void newWorkspace.setMetadata({ "new-workspace-issues.issue": "ON-MOUNT" });
|
||||
}, []);
|
||||
@@ -71,6 +73,9 @@ export async function withNewWorkspaceIssuesExample(
|
||||
chooseWorktree(): Promise<void>;
|
||||
chooseIssue(label: string): Promise<void>;
|
||||
createWorkspace(options?: { prompt?: string }): Promise<{ branch: string; metadata: unknown }>;
|
||||
expectIsolation(isolation: "local" | "worktree"): Promise<void>;
|
||||
/** Removes the plugin, leaves the screen, and opens New workspace for the project again. */
|
||||
reopenWithoutPlugin(): Promise<void>;
|
||||
}) => Promise<void>,
|
||||
) {
|
||||
const client = await connectNewWorkspaceDaemonClient();
|
||||
@@ -101,6 +106,18 @@ export async function withNewWorkspaceIssuesExample(
|
||||
await page.getByRole("button", { name: "Issue", exact: true }).click();
|
||||
await page.getByText(label, { exact: true }).click();
|
||||
}),
|
||||
expectIsolation: (isolation) =>
|
||||
test.step(`the screen is on ${isolation}`, () =>
|
||||
expectWorkspaceIsolationSelected(page, isolation)),
|
||||
reopenWithoutPlugin: () =>
|
||||
test.step("reopen the screen without the plugin", async () => {
|
||||
await client.removePlugin(PLUGIN_ID);
|
||||
// Leaving the screen drops its in-memory choice, so only the saved preference is left.
|
||||
await page.goBack();
|
||||
await expect(page.getByTestId("workspace-create-isolation-trigger")).toHaveCount(0);
|
||||
await openNewWorkspaceComposer(page, project);
|
||||
await expect(page.getByRole("button", { name: "Mount", exact: true })).toHaveCount(0);
|
||||
}),
|
||||
createWorkspace: (createOptions) =>
|
||||
test.step("create the workspace and read what it carries", async () => {
|
||||
await submitCreate(page, createOptions?.prompt);
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -94,6 +94,7 @@ import {
|
||||
startsNewBranch,
|
||||
type BranchPickerDetail,
|
||||
type PickerCheckoutRequest,
|
||||
type PickerItem,
|
||||
type PickerOptionData,
|
||||
} from "./new-workspace-picker-item";
|
||||
import {
|
||||
@@ -208,19 +209,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,
|
||||
@@ -1023,6 +1016,8 @@ export function NewWorkspaceScreen({
|
||||
supportsMultiplicity: supportsWorkspaceMultiplicity,
|
||||
pendingAction,
|
||||
isDraftHandoffActive,
|
||||
updatePreferences: updateFormPreferences,
|
||||
setAttachments: chatDraft.setAttachments,
|
||||
});
|
||||
const dispatchPickerSelection = setup.dispatchPicker;
|
||||
const selectedItem = useNewWorkspaceSetupState(setup).pickerSelection.selectedItem;
|
||||
@@ -1124,15 +1119,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>(
|
||||
@@ -1638,8 +1630,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";
|
||||
@@ -1653,7 +1655,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;
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user