mirror of
https://github.com/nexu-io/open-design.git
synced 2026-10-27 09:51:12 +01:00
fix(web): keep Add to My plugins success affordance after panel remount (#2897)
The Add to My plugins button looked like nothing happened: the action succeeded (the plugin showed up on the Plugins page) but the originating panel only ever showed a transient "Sending..." state. Two root causes: 1. ProjectView toggles `hiddenPluginActionPaths` during the install action, which unmounts `PluginActionPanel` while the API call is in flight. The panel's own `noticeByFolder` state went with the unmount, so the `setNoticeByFolder(...)` call after `await onRequestPluginFolderAgentAction` landed on a dead fiber and the success notice never rendered. 2. The `PluginInstallOutcome` contract leaves `message` optional. When it is absent the surface had no fallback affordance — the button silently reverted to "Add to My plugins" with nothing else changed. Lift `busyKey` / `noticeByFolder` / `runPluginAction` from `PluginActionPanel` up into `AssistantMessage` so the state survives the unmount cycle. Render notices inside the panel for visible folders and add an orphan-notice slot that covers folders the parent has hidden in the meantime. Default to an "Added to My plugins." notice when the install resolves without a message. Publish / contribute paths still rely on outcome messages they always emit, so they're unaffected. Fixes #2876
This commit is contained in:
@@ -204,6 +204,64 @@ export function AssistantMessage({
|
||||
: [],
|
||||
[displayedProduced, fileOps, hiddenPluginActionPaths, isLast, message.content, projectFiles, projectId, streaming],
|
||||
);
|
||||
// Plugin action state lives at the AssistantMessage level (not inside
|
||||
// PluginActionPanel) so the success notice survives the unmount/remount
|
||||
// cycle ProjectView triggers via `hiddenPluginActionPaths` during install
|
||||
// (issue #2876). If state lived inside the panel the setNoticeByFolder
|
||||
// call after `await onRequestPluginFolderAgentAction(...)` would land on
|
||||
// a dead fiber and the user would see nothing change after "Sending...".
|
||||
const [pluginBusyKey, setPluginBusyKey] = useState<string | null>(null);
|
||||
const [pluginNoticeByFolder, setPluginNoticeByFolder] = useState<Record<string, ActionNotice>>({});
|
||||
const runPluginAction = useCallback(
|
||||
async (folder: PluginFolderCandidate, action: PluginFolderAgentAction) => {
|
||||
if (pluginBusyKey || !onRequestPluginFolderAgentAction) return;
|
||||
const key = `${action}:${folder.path}`;
|
||||
setPluginBusyKey(key);
|
||||
setPluginNoticeByFolder((prev) => {
|
||||
if (!(folder.path in prev)) return prev;
|
||||
const next = { ...prev };
|
||||
delete next[folder.path];
|
||||
return next;
|
||||
});
|
||||
try {
|
||||
const outcome = await onRequestPluginFolderAgentAction(folder.path, action);
|
||||
const url =
|
||||
outcome && typeof outcome === "object" && typeof outcome.url === "string"
|
||||
? outcome.url
|
||||
: "";
|
||||
const message =
|
||||
outcome && typeof outcome === "object" && typeof outcome.message === "string"
|
||||
? outcome.message
|
||||
: "";
|
||||
// The install endpoint's PluginInstallOutcome contract leaves
|
||||
// `message` optional. When both message and url are absent we still
|
||||
// need to confirm success — the bug report explicitly describes
|
||||
// "the plugin was in fact added successfully, but the original
|
||||
// screen did not communicate that outcome." Default to a short
|
||||
// success label keyed off the action.
|
||||
const notice: ActionNotice | null =
|
||||
message || url
|
||||
? buildActionNotice(message || url, url)
|
||||
: action === "install"
|
||||
? { message: "Added to My plugins." }
|
||||
: null;
|
||||
if (notice) {
|
||||
setPluginNoticeByFolder((prev) => ({
|
||||
...prev,
|
||||
[folder.path]: notice,
|
||||
}));
|
||||
}
|
||||
} catch (err) {
|
||||
setPluginNoticeByFolder((prev) => ({
|
||||
...prev,
|
||||
[folder.path]: { message: err instanceof Error ? err.message : String(err) },
|
||||
}));
|
||||
} finally {
|
||||
setPluginBusyKey(null);
|
||||
}
|
||||
},
|
||||
[pluginBusyKey, onRequestPluginFolderAgentAction],
|
||||
);
|
||||
const usage = events.find((e) => e.kind === "usage") as
|
||||
| Extract<AgentEvent, { kind: "usage" }>
|
||||
| undefined;
|
||||
@@ -332,10 +390,36 @@ export function AssistantMessage({
|
||||
{!streaming && projectId && pluginActionFolders.length > 0 ? (
|
||||
<PluginActionPanel
|
||||
folders={pluginActionFolders}
|
||||
notices={pluginNoticeByFolder}
|
||||
busyKey={pluginBusyKey}
|
||||
onRunAction={runPluginAction}
|
||||
onRequestOpenFile={onRequestOpenFile}
|
||||
onRequestPluginFolderAgentAction={onRequestPluginFolderAgentAction}
|
||||
activePluginActionPaths={activePluginActionPaths}
|
||||
/>
|
||||
) : null}
|
||||
{/*
|
||||
Notices for folders that completed an action while the panel was
|
||||
unmounted (the parent toggled `hiddenPluginActionPaths` during the
|
||||
install) need a place to render once the panel goes away. Without
|
||||
this fallback, a successful "Add to My plugins" that hides the
|
||||
folder afterwards would silently swallow the confirmation
|
||||
(issue #2876).
|
||||
*/}
|
||||
{!streaming && projectId
|
||||
? Object.entries(pluginNoticeByFolder)
|
||||
.filter(([path]) => !pluginActionFolders.some((folder) => folder.path === path))
|
||||
.map(([path, notice]) => (
|
||||
<div
|
||||
key={`plugin-orphan-notice-${path}`}
|
||||
className="plugin-action-orphan-notice"
|
||||
role="status"
|
||||
data-testid={`plugin-folder-notice-${path}`}
|
||||
>
|
||||
<ActionNoticeView notice={notice} />
|
||||
</div>
|
||||
))
|
||||
: null}
|
||||
{!streaming && unfinishedTodos.length > 0 ? (
|
||||
<UnfinishedTodosPanel
|
||||
todos={unfinishedTodos}
|
||||
@@ -1072,13 +1156,25 @@ function ProducedFiles({
|
||||
);
|
||||
}
|
||||
|
||||
// Pure renderer. State (busyKey, notices) and the action runner live in the
|
||||
// AssistantMessage parent so they survive the panel's unmount/remount cycle
|
||||
// during install (issue #2876).
|
||||
function PluginActionPanel({
|
||||
folders,
|
||||
notices,
|
||||
busyKey,
|
||||
onRunAction,
|
||||
onRequestOpenFile,
|
||||
onRequestPluginFolderAgentAction,
|
||||
activePluginActionPaths = new Set(),
|
||||
}: {
|
||||
folders: PluginFolderCandidate[];
|
||||
notices: Record<string, ActionNotice>;
|
||||
busyKey: string | null;
|
||||
onRunAction: (
|
||||
folder: PluginFolderCandidate,
|
||||
action: PluginFolderAgentAction,
|
||||
) => Promise<void> | void;
|
||||
onRequestOpenFile?: (name: string) => void;
|
||||
onRequestPluginFolderAgentAction?: (
|
||||
relativePath: string,
|
||||
@@ -1086,46 +1182,8 @@ function PluginActionPanel({
|
||||
) => Promise<{ message?: string; url?: string } | void> | { message?: string; url?: string } | void;
|
||||
activePluginActionPaths?: Set<string>;
|
||||
}) {
|
||||
const [busyKey, setBusyKey] = useState<string | null>(null);
|
||||
const [noticeByFolder, setNoticeByFolder] = useState<Record<string, ActionNotice>>(
|
||||
{},
|
||||
);
|
||||
|
||||
async function runAction(
|
||||
folder: PluginFolderCandidate,
|
||||
action: PluginFolderAgentAction,
|
||||
) {
|
||||
if (busyKey || !onRequestPluginFolderAgentAction) return;
|
||||
const key = `${action}:${folder.path}`;
|
||||
setBusyKey(key);
|
||||
setNoticeByFolder((prev) => {
|
||||
const next = { ...prev };
|
||||
delete next[folder.path];
|
||||
return next;
|
||||
});
|
||||
try {
|
||||
const outcome = await onRequestPluginFolderAgentAction(folder.path, action);
|
||||
const url = outcome && typeof outcome === 'object' && typeof outcome.url === 'string'
|
||||
? outcome.url
|
||||
: '';
|
||||
const message = outcome && typeof outcome === 'object' && typeof outcome.message === 'string'
|
||||
? outcome.message
|
||||
: '';
|
||||
if (message || url) {
|
||||
setNoticeByFolder((prev) => ({
|
||||
...prev,
|
||||
[folder.path]: buildActionNotice(message || url, url),
|
||||
}));
|
||||
}
|
||||
} catch (err) {
|
||||
setNoticeByFolder((prev) => ({
|
||||
...prev,
|
||||
[folder.path]: { message: err instanceof Error ? err.message : String(err) },
|
||||
}));
|
||||
} finally {
|
||||
setBusyKey(null);
|
||||
}
|
||||
}
|
||||
const noticeByFolder = notices;
|
||||
const runAction = onRunAction;
|
||||
|
||||
return (
|
||||
<div className="plugin-action-panel" aria-label="Plugin next actions">
|
||||
|
||||
@@ -0,0 +1,186 @@
|
||||
// @vitest-environment jsdom
|
||||
|
||||
/**
|
||||
* Issue #2876 coverage: when a generated plugin folder is offered as
|
||||
* "Add to My plugins" in the assistant message panel and the install
|
||||
* succeeds, the originating surface must leave behind a clear success
|
||||
* affordance. Two failure modes are covered:
|
||||
*
|
||||
* 1. The panel's internal notice state is lost across an unmount/remount
|
||||
* cycle (ProjectView toggles `hiddenPluginActionPaths` during the
|
||||
* action). The notice has to live above the panel so it survives.
|
||||
* 2. The install endpoint may legitimately resolve without a `message`
|
||||
* string; the UI still has to confirm success instead of silently
|
||||
* reverting the button.
|
||||
*/
|
||||
|
||||
import { useState } from 'react';
|
||||
import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react';
|
||||
import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest';
|
||||
|
||||
import { AssistantMessage } from '../../src/components/AssistantMessage';
|
||||
import type { AgentEvent, ChatMessage, ProjectFile } from '../../src/types';
|
||||
|
||||
beforeAll(() => {
|
||||
if (window.localStorage) return;
|
||||
const store = new Map<string, string>();
|
||||
Object.defineProperty(window, 'localStorage', {
|
||||
configurable: true,
|
||||
value: {
|
||||
clear: () => store.clear(),
|
||||
getItem: (key: string) => store.get(key) ?? null,
|
||||
removeItem: (key: string) => store.delete(key),
|
||||
setItem: (key: string, value: string) => store.set(key, value),
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
cleanup();
|
||||
});
|
||||
|
||||
function pluginFolderFiles(folderPath: string): ProjectFile[] {
|
||||
return [
|
||||
{
|
||||
name: `${folderPath}/open-design.json`,
|
||||
path: `${folderPath}/open-design.json`,
|
||||
size: 100,
|
||||
mtime: 1700000005,
|
||||
kind: 'code',
|
||||
mime: 'application/json',
|
||||
} as ProjectFile,
|
||||
{
|
||||
name: `${folderPath}/SKILL.md`,
|
||||
path: `${folderPath}/SKILL.md`,
|
||||
size: 100,
|
||||
mtime: 1700000005,
|
||||
kind: 'text',
|
||||
mime: 'text/markdown',
|
||||
} as ProjectFile,
|
||||
];
|
||||
}
|
||||
|
||||
function pluginMessage(folderPath: string): ChatMessage {
|
||||
return {
|
||||
id: 'msg-plugin-1',
|
||||
role: 'assistant',
|
||||
content: 'Plugin is ready to add to My plugins.',
|
||||
runStatus: 'succeeded',
|
||||
startedAt: 1700000000,
|
||||
endedAt: 1700000005,
|
||||
events: [{ kind: 'text', text: 'Plugin is ready to add to My plugins.' } as AgentEvent],
|
||||
producedFiles: pluginFolderFiles(folderPath),
|
||||
} as ChatMessage;
|
||||
}
|
||||
|
||||
/**
|
||||
* Wrapper that mimics ProjectView's hide-during-install toggle: when the
|
||||
* `install` action fires, the parent adds the folder to
|
||||
* `hiddenPluginActionPaths`, which unmounts the panel for that folder
|
||||
* during the await. The wrapper deletes the entry from the hidden set
|
||||
* after the action resolves, so the panel remounts. This is the exact
|
||||
* shape of the production code path the bug lives on.
|
||||
*/
|
||||
function ToggleHostWrapper({
|
||||
folderPath,
|
||||
outcome,
|
||||
resolveSignal,
|
||||
}: {
|
||||
folderPath: string;
|
||||
outcome: { message?: string; url?: string } | void;
|
||||
resolveSignal: { resolve: () => void; promise: Promise<void> };
|
||||
}) {
|
||||
const [hidden, setHidden] = useState<Set<string>>(() => new Set());
|
||||
return (
|
||||
<AssistantMessage
|
||||
message={pluginMessage(folderPath)}
|
||||
streaming={false}
|
||||
isLast
|
||||
projectId="proj-1"
|
||||
projectFiles={pluginFolderFiles(folderPath)}
|
||||
hiddenPluginActionPaths={hidden}
|
||||
onRequestPluginFolderAgentAction={async (path, action) => {
|
||||
if (action !== 'install') return outcome;
|
||||
setHidden((prev) => new Set(prev).add(path));
|
||||
await resolveSignal.promise;
|
||||
setHidden((prev) => {
|
||||
const next = new Set(prev);
|
||||
next.delete(path);
|
||||
return next;
|
||||
});
|
||||
return outcome;
|
||||
}}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
describe('AssistantMessage plugin install success feedback (#2876)', () => {
|
||||
it('leaves a visible success affordance after install resolves, even though the panel unmounts mid-install', async () => {
|
||||
const folderPath = 'my-skill';
|
||||
let resolveAction!: () => void;
|
||||
const signal = {
|
||||
promise: new Promise<void>((res) => {
|
||||
resolveAction = res;
|
||||
}),
|
||||
resolve: () => resolveAction(),
|
||||
};
|
||||
|
||||
render(
|
||||
<ToggleHostWrapper
|
||||
folderPath={folderPath}
|
||||
outcome={{ message: 'Installed My Skill.' }}
|
||||
resolveSignal={signal}
|
||||
/>,
|
||||
);
|
||||
|
||||
const addButton = screen.getByTestId(`assistant-plugin-install-${folderPath}`);
|
||||
fireEvent.click(addButton);
|
||||
|
||||
await act(async () => {
|
||||
signal.resolve();
|
||||
await signal.promise;
|
||||
});
|
||||
|
||||
// The success message must persist even though the panel unmounted and
|
||||
// remounted while the install was in flight. The notice has to outlive
|
||||
// the panel's internal state for the user to see confirmation.
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText('Installed My Skill.')).toBeTruthy();
|
||||
});
|
||||
});
|
||||
|
||||
it('shows a default success affordance when the install resolves without a message', async () => {
|
||||
const folderPath = 'sparse-skill';
|
||||
let resolveAction!: () => void;
|
||||
const signal = {
|
||||
promise: new Promise<void>((res) => {
|
||||
resolveAction = res;
|
||||
}),
|
||||
resolve: () => resolveAction(),
|
||||
};
|
||||
|
||||
render(
|
||||
<ToggleHostWrapper
|
||||
folderPath={folderPath}
|
||||
outcome={undefined}
|
||||
resolveSignal={signal}
|
||||
/>,
|
||||
);
|
||||
|
||||
fireEvent.click(screen.getByTestId(`assistant-plugin-install-${folderPath}`));
|
||||
|
||||
await act(async () => {
|
||||
signal.resolve();
|
||||
await signal.promise;
|
||||
});
|
||||
|
||||
// The contract for the install outcome leaves `message` optional. When
|
||||
// it is absent, the UI still needs to affirm success — the bug report
|
||||
// explicitly describes this case ("the plugin was in fact added
|
||||
// successfully, but the original screen did not communicate that
|
||||
// outcome"). A default success label fills that gap.
|
||||
await waitFor(() => {
|
||||
expect(screen.getByText(/added to my plugins/i)).toBeTruthy();
|
||||
});
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user