diff --git a/docs/plugins.md b/docs/plugins.md index 5ab180fbae..964a095722 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -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 diff --git a/packages/app/e2e/browser/plugin-new-workspace-items.spec.ts b/packages/app/e2e/browser/plugin-new-workspace-items.spec.ts index ec7158164c..e0fa0ce97d 100644 --- a/packages/app/e2e/browser/plugin-new-workspace-items.spec.ts +++ b/packages/app/e2e/browser/plugin-new-workspace-items.spec.ts @@ -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" }); diff --git a/packages/app/e2e/support/helpers/plugin-new-workspace-items.ts b/packages/app/e2e/support/helpers/plugin-new-workspace-items.ts index b90ad5a8ec..8d7962d936 100644 --- a/packages/app/e2e/support/helpers/plugin-new-workspace-items.ts +++ b/packages/app/e2e/support/helpers/plugin-new-workspace-items.ts @@ -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; chooseIssue(label: string): Promise; createWorkspace(options?: { prompt?: string }): Promise<{ branch: string; metadata: unknown }>; + expectIsolation(isolation: "local" | "worktree"): Promise; + /** Removes the plugin, leaves the screen, and opens New workspace for the project again. */ + reopenWithoutPlugin(): Promise; }) => Promise, ) { 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); diff --git a/packages/app/src/plugins/new-workspace-items/model.test.ts b/packages/app/src/plugins/new-workspace-items/model.test.ts index 8cd450d5d8..827c54280d 100644 --- a/packages/app/src/plugins/new-workspace-items/model.test.ts +++ b/packages/app/src/plugins/new-workspace-items/model.test.ts @@ -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), diff --git a/packages/app/src/screens/new-workspace-screen.tsx b/packages/app/src/screens/new-workspace-screen.tsx index 0b4d3d101d..67a9d33511 100644 --- a/packages/app/src/screens/new-workspace-screen.tsx +++ b/packages/app/src/screens/new-workspace-screen.tsx @@ -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( @@ -1638,8 +1630,18 @@ function usePluginSetup(input: { supportsMultiplicity: boolean; pendingAction: Parameters[0]["pendingAction"]; isDraftHandoffActive: boolean; + updatePreferences: ReturnType["updatePreferences"]; + setAttachments: ReturnType["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 }; } diff --git a/packages/app/src/screens/new-workspace/setup-model.test.ts b/packages/app/src/screens/new-workspace/setup-model.test.ts index 83ba9cb8ac..d14e9949c5 100644 --- a/packages/app/src/screens/new-workspace/setup-model.test.ts +++ b/packages/app/src/screens/new-workspace/setup-model.test.ts @@ -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", diff --git a/packages/app/src/screens/new-workspace/setup-model.ts b/packages/app/src/screens/new-workspace/setup-model.ts index 2beedfaeca..9902fd814f 100644 --- a/packages/app/src/screens/new-workspace/setup-model.ts +++ b/packages/app/src/screens/new-workspace/setup-model.ts @@ -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) => { 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; },