From 03580ce50b1077e331ede5739b629d45b63210d0 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Thu, 5 Nov 2026 19:31:39 +0100 Subject: [PATCH] fix(plugins): a New workspace item's commands stop working once its project or host is left Every item received the same command functions, which act on the current screen state. A lookup that finished after the user switched projects could still call setBranch or setMetadata and overwrite the new project's choices; the reset on a switch only clears what was already written. The setup model now hands out commands per target with commandsFor({ serverId, projectId }). They reject with a "moved to another project or host" error when the screen is on another target, like the lock rejects, so the promise only resolves when the change took effect. --- docs/plugins.md | 12 ++- .../src/plugins/new-workspace-items/index.tsx | 12 ++- .../plugins/new-workspace-items/model.test.ts | 2 +- .../screens/new-workspace/setup-model.test.ts | 101 +++++++++++++----- .../src/screens/new-workspace/setup-model.ts | 75 ++++++++----- public-docs/plugins/reference.md | 2 +- public-docs/plugins/reference.zh-CN.md | 2 +- 7 files changed, 148 insertions(+), 58 deletions(-) diff --git a/docs/plugins.md b/docs/plugins.md index 6945af01b3..3d3ce76d83 100644 --- a/docs/plugins.md +++ b/docs/plugins.md @@ -724,9 +724,15 @@ now means another project, because its identity changes every render. The store and `metadata` on the same change in `syncInputs`, and a remounted item's mount effect sets them again. -Known limit: an item that issues a command from `useLayoutEffect` on mount runs before the screen's -`syncInputs` layout effect, so it is validated against the previous target's inputs and the reset -can clear what it set. This applies to a project switch as well as a host switch, since items +An item's commands come from `setup.commandsFor({ serverId, projectId })`, bound to the target the +item was mounted for. Once `syncInputs` reports another target they reject with a "moved" error, so +a lookup that finishes after a project switch cannot write into the new project's choices. The +reset only clears what was already written; this check covers what is written later. Rejecting, not +ignoring, matches the lock: a command's promise resolves only when the change took effect. + +Known limit: an item that issues a command from `useLayoutEffect` on mount after a project or host +switch runs before the screen's `syncInputs` layout effect, so its commands (bound to the new +target) still see the previous target and reject as moved. This applies to a project switch as well as a host switch, since items remount per `(serverId, project id)`. Passive effects are not affected. `ensureWorkspace` sends the store's `branch` as `source.branchName` whenever the worktree cuts a diff --git a/packages/app/src/plugins/new-workspace-items/index.tsx b/packages/app/src/plugins/new-workspace-items/index.tsx index 41f27d2182..642cc9360e 100644 --- a/packages/app/src/plugins/new-workspace-items/index.tsx +++ b/packages/app/src/plugins/new-workspace-items/index.tsx @@ -60,6 +60,7 @@ export function PluginNewWorkspaceItems({ setup.commandsFor({ serverId, projectId }), + [setup, serverId, projectId], + ); const newWorkspace = useMemo( () => toPluginNewWorkspace({ host: props.host, view: { ...screen, branch, metadata }, - commands: setup.commands, + commands, }), - [branch, metadata, props.host, screen, setup.commands], + [branch, metadata, props.host, screen, commands], ); return ( 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 4806b9dcd4..ce274e85bd 100644 --- a/packages/app/src/plugins/new-workspace-items/model.test.ts +++ b/packages/app/src/plugins/new-workspace-items/model.test.ts @@ -50,7 +50,7 @@ describe("the base ref a plugin reads", () => { const read = toPluginNewWorkspace({ host: HOST, view: viewOf(origin), - commands: setup.commands, + commands: setup.commandsFor({ serverId: "host-a", projectId: "p" }), }); expect(read.baseRef).toEqual({ type: "branch", name: "refs/remotes/origin/release" }); 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 2f2ee2612d..2e395cd99f 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,12 @@ import { describe, expect, it } from "vitest"; import type { PickerItem } from "../new-workspace-picker-item"; -import { createNewWorkspaceSetup, type SetupEffects, type SetupInputs } from "./setup-model"; +import { + createNewWorkspaceSetup, + setupTargetOf, + type NewWorkspaceSetup, + type SetupEffects, + type SetupInputs, +} from "./setup-model"; const READY: SetupInputs = { serverId: "host-a", @@ -10,6 +16,11 @@ const READY: SetupInputs = { supportsMetadata: true, }; +/** The commands an item mounted for `inputs`' project and host receives. */ +function commandsOf(setup: NewWorkspaceSetup, inputs: SetupInputs = READY) { + return setup.commandsFor(setupTargetOf(inputs)); +} + function recordingEffects() { const chosen: PickerItem[] = []; const isolations: string[] = []; @@ -24,7 +35,7 @@ describe("New workspace setup commands", () => { it("accepts commands from the first moment, before the screen has committed", async () => { const setup = createNewWorkspaceSetup(READY, recordingEffects().effects); - await setup.commands.setBranch("pas-101"); + await commandsOf(setup).setBranch("pas-101"); expect(setup.getSnapshot().branch).toBe("pas-101"); }); @@ -32,9 +43,9 @@ describe("New workspace setup commands", () => { it("applies commands that run back to back without waiting for a render", async () => { const setup = createNewWorkspaceSetup(READY, recordingEffects().effects); - void setup.commands.setBranch("pas-101"); - void setup.commands.setMetadata({ issue: "PAS-101", keep: true }); - await setup.commands.setMetadata({ keep: null }); + void commandsOf(setup).setBranch("pas-101"); + void commandsOf(setup).setMetadata({ issue: "PAS-101", keep: true }); + await commandsOf(setup).setMetadata({ keep: null }); expect(setup.getSnapshot().branch).toBe("pas-101"); expect(setup.getSnapshot().metadata).toEqual({ issue: "PAS-101" }); @@ -44,8 +55,8 @@ describe("New workspace setup commands", () => { const { effects, chosen, isolations } = recordingEffects(); const setup = createNewWorkspaceSetup(READY, effects); - await setup.commands.setBaseRef({ type: "branch", name: " release " }); - await setup.commands.setIsolation("worktree"); + await commandsOf(setup).setBaseRef({ type: "branch", name: " release " }); + await commandsOf(setup).setIsolation("worktree"); expect(chosen).toMatchObject([{ kind: "branch", name: "release", refName: "release" }]); expect(setup.getSnapshot().pickerSelection.selectedItem).toEqual(chosen[0]); @@ -56,7 +67,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, recordingEffects().effects); - await setup.commands.setBaseRef({ type: "branch", name: "refs/remotes/origin/release" }); + await commandsOf(setup).setBaseRef({ type: "branch", name: "refs/remotes/origin/release" }); expect(setup.getSnapshot().pickerSelection.selectedItem).toMatchObject({ name: "release", @@ -65,12 +76,13 @@ describe("New workspace setup commands", () => { }); it("drops the branch and metadata chosen for another project, but not while the project resolves", async () => { - const setup = createNewWorkspaceSetup({ ...READY, project: null }, recordingEffects().effects); - await setup.commands.setBranch("pas-101"); + const resolving = { ...READY, project: null }; + const setup = createNewWorkspaceSetup(resolving, recordingEffects().effects); + await commandsOf(setup, resolving).setBranch("pas-101"); setup.syncInputs(READY); expect(setup.getSnapshot().branch).toBe("pas-101"); - await setup.commands.setMetadata({ issue: "PAS-101" }); + await commandsOf(setup).setMetadata({ issue: "PAS-101" }); setup.syncInputs({ ...READY, project: { id: "p2", name: "other" } }); expect(setup.getSnapshot().branch).toBeNull(); @@ -79,8 +91,8 @@ describe("New workspace setup commands", () => { it("drops the branch and metadata when the host changes under the same project id", async () => { const setup = createNewWorkspaceSetup(READY, recordingEffects().effects); - await setup.commands.setBranch("pas-101"); - await setup.commands.setMetadata({ issue: "PAS-101" }); + await commandsOf(setup).setBranch("pas-101"); + await commandsOf(setup).setMetadata({ issue: "PAS-101" }); setup.syncInputs({ ...READY, serverId: "host-b" }); @@ -88,14 +100,52 @@ describe("New workspace setup commands", () => { expect(setup.getSnapshot().metadata).toEqual({}); }); + it("rejects a late command from the project or host the screen has left, and leaves the new choices alone", async () => { + const setup = createNewWorkspaceSetup(READY, recordingEffects().effects); + const old = commandsOf(setup); + const otherProject = { ...READY, project: { id: "p2", name: "other" } }; + const otherHost = { ...READY, serverId: "host-b" }; + + setup.syncInputs(otherProject); + await commandsOf(setup, otherProject).setBranch("chosen-for-p2"); + + const results = await Promise.allSettled([ + old.setBranch("late"), + old.setMetadata({ issue: "late" }), + old.setIsolation("local"), + old.setBaseRef({ type: "branch", name: "late" }), + ]); + expect(results.map((result) => result.status)).toEqual(Array(4).fill("rejected")); + await expect(old.setBranch("late")).rejects.toThrow("moved to another project or host"); + expect(setup.getSnapshot().branch).toBe("chosen-for-p2"); + expect(setup.getSnapshot().metadata).toEqual({}); + expect(setup.getSnapshot().manualIsolation).toBeNull(); + + setup.syncInputs(otherHost); + await expect(commandsOf(setup, otherProject).setBranch("late")).rejects.toThrow( + "moved to another project or host", + ); + }); + + it("accepts commands again when the screen returns to the project they were made for", async () => { + const setup = createNewWorkspaceSetup(READY, recordingEffects().effects); + const first = commandsOf(setup); + + setup.syncInputs({ ...READY, project: { id: "p2", name: "other" } }); + setup.syncInputs(READY); + await first.setBranch("back"); + + expect(setup.getSnapshot().branch).toBe("back"); + }); + it("rejects every command while the screen is locked, as the pickers do", async () => { const setup = createNewWorkspaceSetup({ ...READY, locked: true }, recordingEffects().effects); const results = await Promise.allSettled([ - setup.commands.setBranch("x"), - setup.commands.setMetadata({ a: 1 }), - setup.commands.setIsolation("local"), - setup.commands.setBaseRef({ type: "branch", name: "main" }), + commandsOf(setup).setBranch("x"), + commandsOf(setup).setMetadata({ a: 1 }), + commandsOf(setup).setIsolation("local"), + commandsOf(setup).setBaseRef({ type: "branch", name: "main" }), ]); expect(results.map((result) => result.status)).toEqual(Array(4).fill("rejected")); @@ -105,11 +155,12 @@ 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, recordingEffects().effects); - setup.syncInputs({ ...READY, project: null }); + const resolving = { ...READY, project: null }; + setup.syncInputs(resolving); - await expect(setup.commands.setBaseRef({ type: "branch", name: "main" })).rejects.toThrow( - "Choose a project first.", - ); + await expect( + commandsOf(setup, resolving).setBaseRef({ type: "branch", name: "main" }), + ).rejects.toThrow("Choose a project first."); }); it("rejects what the project or host cannot do", async () => { @@ -118,12 +169,12 @@ describe("New workspace setup commands", () => { recordingEffects().effects, ); - await expect(setup.commands.setIsolation("worktree")).rejects.toThrow( + await expect(commandsOf(setup).setIsolation("worktree")).rejects.toThrow( "cannot create worktrees", ); - await expect(setup.commands.setMetadata({ a: 1 })).rejects.toThrow("Update the host"); - await expect(setup.commands.setBranch(" ")).rejects.toThrow("needs a name"); - await setup.commands.setBranch(null); + await expect(commandsOf(setup).setMetadata({ a: 1 })).rejects.toThrow("Update the host"); + await expect(commandsOf(setup).setBranch(" ")).rejects.toThrow("needs a name"); + await commandsOf(setup).setBranch(null); expect(setup.getSnapshot().branch).toBeNull(); }); }); diff --git a/packages/app/src/screens/new-workspace/setup-model.ts b/packages/app/src/screens/new-workspace/setup-model.ts index acf90ab2f7..2a810d24c1 100644 --- a/packages/app/src/screens/new-workspace/setup-model.ts +++ b/packages/app/src/screens/new-workspace/setup-model.ts @@ -34,6 +34,17 @@ export interface SetupInputs { supportsMetadata: boolean; } +/** The project on the host a screen creates in; what an item is mounted for. */ +export interface SetupTarget { + serverId: string; + /** Null while the screen is still resolving its project. */ + projectId: string | null; +} + +export function setupTargetOf(input: Pick): SetupTarget { + return { serverId: input.serverId, projectId: input.project?.id ?? null }; +} + export interface SetupCommands { setIsolation(isolation: Isolation): Promise; setBaseRef(ref: { type: "branch"; name: string }): Promise; @@ -51,7 +62,11 @@ export interface NewWorkspaceSetup { /** The isolation picker's entry, and the one `setIsolation` takes. */ chooseIsolation(isolation: Isolation): void; syncInputs(inputs: SetupInputs): void; - commands: SetupCommands; + /** + * Commands for the items mounted for `target`. Once the screen is on another project or host + * they reject, so a lookup that finishes after the switch cannot write into the new choices. + */ + commandsFor(target: SetupTarget): SetupCommands; } /** What a pick does beyond changing the model, the same for the picker and for a command. */ @@ -62,6 +77,8 @@ export interface SetupEffects { syncBaseAttachment(item: PickerItem): void; } +const MOVED_MESSAGE = + "The New workspace screen moved to another project or host; this item belongs to the previous one."; const LOCKED_MESSAGE = "The New workspace screen is locked while the workspace is being created."; /** @@ -82,15 +99,15 @@ export function createNewWorkspaceSetup( metadata: {}, }; let inputs = initial; - let target = initial.project ? `${initial.serverId}\n${initial.project.id}` : null; + let screenTarget = initial.project ? `${initial.serverId}\n${initial.project.id}` : null; const listeners = new Set<() => void>(); const update = (patch: Partial) => { state = { ...state, ...patch }; for (const listener of listeners) listener(); }; - const unlocked = (): SetupInputs => { - if (inputs.locked) throw new Error(LOCKED_MESSAGE); - return inputs; + const sameTarget = (target: SetupTarget) => { + const current = setupTargetOf(inputs); + return current.serverId === target.serverId && current.projectId === target.projectId; }; const dispatchPicker = (event: PickerSelectionEvent) => update({ pickerSelection: reducePickerSelection(state.pickerSelection, event) }); @@ -102,26 +119,13 @@ export function createNewWorkspaceSetup( update({ manualIsolation: isolation }); effects.persistIsolation(isolation); }; - return { - subscribe(listener) { - listeners.add(listener); - return () => listeners.delete(listener); - }, - getSnapshot: () => state, - dispatchPicker, - chooseBase, - chooseIsolation, - syncInputs: (next) => { - inputs = next; - // A branch and metadata are chosen for one project on one host. A project id can repeat - // across hosts (a remote repository keeps its key everywhere), so the host is part of the - // target. An absent project is a screen that is still resolving, not a switch. - if (!next.project) return; - const nextTarget = `${next.serverId}\n${next.project.id}`; - if (target && target !== nextTarget) update({ branch: null, metadata: {} }); - target = nextTarget; - }, - commands: { + const commandsFor = (target: SetupTarget): SetupCommands => { + const unlocked = (): SetupInputs => { + if (!sameTarget(target)) throw new Error(MOVED_MESSAGE); + if (inputs.locked) throw new Error(LOCKED_MESSAGE); + return inputs; + }; + return { async setIsolation(isolation) { const current = unlocked(); if (isolation === "worktree" && !current.canCreateWorktree) { @@ -152,7 +156,28 @@ export function createNewWorkspaceSetup( if (!current.supportsMetadata) throw new Error("Update the host to use metadata."); update({ metadata: applyMetadataPatch(state.metadata, patch) }); }, + }; + }; + return { + subscribe(listener) { + listeners.add(listener); + return () => listeners.delete(listener); }, + getSnapshot: () => state, + dispatchPicker, + chooseBase, + chooseIsolation, + syncInputs: (next) => { + inputs = next; + // A branch and metadata are chosen for one project on one host. A project id can repeat + // across hosts (a remote repository keeps its key everywhere), so the host is part of the + // target. An absent project is a screen that is still resolving, not a switch. + if (!next.project) return; + const nextTarget = `${next.serverId}\n${next.project.id}`; + if (screenTarget && screenTarget !== nextTarget) update({ branch: null, metadata: {} }); + screenTarget = nextTarget; + }, + commandsFor, }; } diff --git a/public-docs/plugins/reference.md b/public-docs/plugins/reference.md index 5e51831b3b..f51613098a 100644 --- a/public-docs/plugins/reference.md +++ b/public-docs/plugins/reference.md @@ -2033,7 +2033,7 @@ export default function contribute(client: PluginClientContext) { Commands follow the [composer item](#composer-items) rules: each one acts on the screen's state when it runs, so `await newWorkspace.setBranch(name)` followed by `newWorkspace.setMetadata(patch)` changes both, and its promise resolves once the change has taken effect. A command works from your component's first effect, including a mount effect. Payloads are JSON values. -Your items are mounted for one project on one host. When the user changes either, Paseo remounts them and clears `branch` and `metadata`, so state and `Combobox` results for the previous project do not carry over; an item that sets up in a mount effect sets up again for the new project. +Your items are mounted for one project on one host. When the user changes either, Paseo remounts them and clears `branch` and `metadata`, so state and `Combobox` results for the previous project do not carry over; an item that sets up in a mount effect sets up again for the new project. A command called after its project or host has been left, such as the end of a lookup the user abandoned by switching, rejects with "The New workspace screen moved to another project or host; this item belongs to the previous one." and changes nothing; the items mounted for the new project are not affected. `host`, `project`, `baseRef` and `metadata` are frozen. While the screen creates the workspace it is locked: your fields render disabled and every command rejects with "The New workspace screen is locked while the workspace is being created." A callback that returns a promise keeps the control disabled until it settles, and a diff --git a/public-docs/plugins/reference.zh-CN.md b/public-docs/plugins/reference.zh-CN.md index 941ba67cb5..5a32945b46 100644 --- a/public-docs/plugins/reference.zh-CN.md +++ b/public-docs/plugins/reference.zh-CN.md @@ -1708,7 +1708,7 @@ export default function contribute(client: PluginClientContext) { | `branch`、`setBranch` | 新 worktree 所创建的分支名称。`null` 表示由 Paseo 选择名称。隔离为本地或起点为变更请求时会保留该值，它为任何新建分支的 worktree 命名，起点可以是默认基线或所选分支；检出已有分支或变更请求的 worktree 不适用。 | | `metadata`、`setMetadata` | 所创建工作区的[元数据](/docs/plugins/hooks#metadata)，而不是第一个 Agent 的元数据。顶层键替换，`null` 删除该键。主机不支持元数据时以 “Update the host to use metadata.” 拒绝。 | -命令遵循 [Composer 项](#composer-items)的规则：每个命令都按界面运行时的状态执行，所以 `await newWorkspace.setBranch(name)` 之后再调用 `newWorkspace.setMetadata(patch)` 会同时生效，命令的 Promise 在更改生效后完成。命令从组件的第一个 effect 起即可使用，包括挂载时的 effect。载荷是 JSON 值。你的项针对某一台主机上的某一个项目挂载。用户更改其中任何一个时，Paseo 会重新挂载你的项并清除 `branch` 和 `metadata`，因此上一个项目的状态和 `Combobox` 结果不会保留；在挂载 effect 中做初始化的项会针对新项目再次初始化。`host`、`project`、`baseRef` 和 `metadata` 是冻结的。界面创建工作区期间会被锁定：你的字段显示为禁用，所有命令都会以 "The New workspace screen is locked while the workspace is being created." 拒绝。返回 Promise 的回调在完成之前会让控件保持禁用，被拒绝时会以错误提示向用户显示。 +命令遵循 [Composer 项](#composer-items)的规则：每个命令都按界面运行时的状态执行，所以 `await newWorkspace.setBranch(name)` 之后再调用 `newWorkspace.setMetadata(patch)` 会同时生效，命令的 Promise 在更改生效后完成。命令从组件的第一个 effect 起即可使用，包括挂载时的 effect。载荷是 JSON 值。你的项针对某一台主机上的某一个项目挂载。用户更改其中任何一个时，Paseo 会重新挂载你的项并清除 `branch` 和 `metadata`，因此上一个项目的状态和 `Combobox` 结果不会保留；在挂载 effect 中做初始化的项会针对新项目再次初始化。在其项目或主机已被离开之后才调用的命令（例如用户切换项目时被放弃的查询最终返回）会以 "The New workspace screen moved to another project or host; this item belongs to the previous one." 拒绝，并且不做任何更改；为新项目挂载的项不受影响。`host`、`project`、`baseRef` 和 `metadata` 是冻结的。界面创建工作区期间会被锁定：你的字段显示为禁用，所有命令都会以 "The New workspace screen is locked while the workspace is being created." 拒绝。返回 Promise 的回调在完成之前会让控件保持禁用，被拒绝时会以错误提示向用户显示。 `Combobox` 是带可搜索列表的字段，结果由你的代码提供：