fix(FUX-015): address slash-command review feedback
Greptile/CodeRabbit review of the chat slash-command framework: - Composer-wipe race: ChatView and TaskPlannerChatTab cleared the composer inside the command's success callback, silently wiping any text the user typed while the command was in flight. Clear on submit (before the network round-trip) instead — consistent with normal chat send, which also clears immediately and does not restore on failure. - Attachments were silently dropped when dispatching a slash command in ChatView (clearing the composer revokes staged attachment URLs). Block dispatch with a warning toast when attachments are staged. - CHAT_COMMANDS is now a readonly array; helper signatures accept readonly ChatCommand[]. - The planner command menu (commands-only) used skill-menu aria-label/empty copy; use command-specific copy instead. Add regression tests: in-flight text survives command success, composer clears on submit even on failure, attachment dispatch is blocked, and the planner command menu uses command-specific accessible copy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -1526,6 +1526,23 @@ export function ChatView({ projectId, addToast, floating = false, compactLayout
|
||||
return;
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:ChatSlashCommands 2026-07-10-11:40:
|
||||
Slash commands carry no attachments. Block dispatch (rather than silently dropping) when files are staged, since clearing the composer below revokes their object URLs before they could ever be sent.
|
||||
*/
|
||||
if (files.length > 0) {
|
||||
addToast(
|
||||
t("chat.commandNoAttachments", "Attachments aren't supported with commands — remove them before sending"),
|
||||
"warning",
|
||||
);
|
||||
return;
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:ChatSlashCommands 2026-07-10-11:40:
|
||||
Clear the composer immediately on submit — BEFORE the network round-trip — not inside the success callback. Clearing late wipes any text the user typed while the command was in flight (composer-wipe race, FUX-015).
|
||||
*/
|
||||
clearComposerState();
|
||||
void commandMatch.command
|
||||
.run({
|
||||
taskId: chatCommandContext.taskId,
|
||||
@@ -1533,7 +1550,6 @@ export function ChatView({ projectId, addToast, floating = false, compactLayout
|
||||
remainder: commandMatch.remainder,
|
||||
})
|
||||
.then(() => {
|
||||
clearComposerState();
|
||||
addToast(t("chat.commandSteerSuccess", "Sent to the running agent"), "success");
|
||||
})
|
||||
.catch((error: unknown) => {
|
||||
@@ -2534,6 +2550,7 @@ export function ChatView({ projectId, addToast, floating = false, compactLayout
|
||||
<input
|
||||
ref={fileInputRef}
|
||||
type="file"
|
||||
data-testid="chat-file-input"
|
||||
accept="image/*,.txt,.json,.yaml,.yml,.log,.csv,.xml,.md"
|
||||
multiple
|
||||
style={{ display: "none" }}
|
||||
|
||||
@@ -701,9 +701,13 @@ export function TaskPlannerChatTab({ task, projectId, active, expanded = false,
|
||||
return;
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:ChatSlashCommands 2026-07-10-11:40:
|
||||
Clear the draft immediately on submit — BEFORE awaiting command.run — not after it resolves. Clearing in the success path wipes any text the user typed while the command was in flight (composer-wipe race, FUX-015).
|
||||
*/
|
||||
setDraft("");
|
||||
try {
|
||||
await command.run({ taskId: task.id, projectId, remainder });
|
||||
setDraft("");
|
||||
// Reuse the existing steering-refresh path (same toast + task refresh already
|
||||
// used by the tool-call-driven steering flow above) instead of a second,
|
||||
// divergent success toast for the same underlying action.
|
||||
@@ -1060,10 +1064,10 @@ export function TaskPlannerChatTab({ task, projectId, active, expanded = false,
|
||||
className="chat-skill-menu task-planner-chat-command-menu"
|
||||
data-testid="task-planner-chat-command-menu"
|
||||
role="listbox"
|
||||
aria-label={t("chat.skillSuggestions", "Skill suggestions")}
|
||||
aria-label={t("chat.commandSuggestions", "Command suggestions")}
|
||||
>
|
||||
{filteredCommands.length === 0 ? (
|
||||
<div className="chat-skill-menu-empty">{t("chat.noSkillsFound", "No skills found")}</div>
|
||||
<div className="chat-skill-menu-empty">{t("chat.noCommandsFound", "No commands found")}</div>
|
||||
) : (
|
||||
filteredCommands.map((command, index) => (
|
||||
<button
|
||||
|
||||
@@ -225,7 +225,12 @@ describe("ChatView slash-command dispatch (/steer)", () => {
|
||||
expect(screen.getByText(/no running agent/i)).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("leaves the composer text intact and shows an error toast when run() fails", async () => {
|
||||
// FUX-015 composer-wipe race: the composer is cleared on submit — BEFORE the
|
||||
// network round-trip — so text the user types while the command is in flight
|
||||
// is never wiped by a late callback. This matches normal chat send (which also
|
||||
// clears immediately and does not restore on failure), so the composer stays
|
||||
// empty after run() rejects; the error is surfaced via a toast.
|
||||
it("clears the composer on submit and shows an error toast when run() fails", async () => {
|
||||
const sendMessage = vi.fn();
|
||||
setupMockChat({ activeSession: activeSessionFixture, messages: [], sendMessage });
|
||||
mockAddSteeringComment.mockRejectedValueOnce(new Error("network down"));
|
||||
@@ -240,6 +245,60 @@ describe("ChatView slash-command dispatch (/steer)", () => {
|
||||
fireEvent.keyDown(textarea, { key: "Enter" });
|
||||
|
||||
await waitFor(() => expect(addToast).toHaveBeenCalledWith("network down", "error"));
|
||||
expect(textarea).toHaveValue("");
|
||||
});
|
||||
|
||||
// FUX-015 composer-wipe race: text typed AFTER submit (while the command is
|
||||
// in flight) must survive the success callback — success no longer clears.
|
||||
it("preserves text typed while the command is in flight (no late wipe on success)", async () => {
|
||||
const sendMessage = vi.fn();
|
||||
setupMockChat({ activeSession: activeSessionFixture, messages: [], sendMessage });
|
||||
let resolveRun: () => void = () => {};
|
||||
const runPromise = new Promise<void>((resolve) => { resolveRun = resolve; });
|
||||
mockAddSteeringComment.mockReturnValueOnce(runPromise as unknown as ReturnType<typeof addSteeringComment>);
|
||||
const addToast = vi.fn();
|
||||
|
||||
await renderWithAct(
|
||||
<ChatView projectId="proj-123" addToast={addToast} chatCommandContext={commandContext} />,
|
||||
);
|
||||
|
||||
const textarea = screen.getByTestId("chat-input");
|
||||
fireEvent.change(textarea, { target: { value: "/steer do X" } });
|
||||
fireEvent.keyDown(textarea, { key: "Enter" });
|
||||
|
||||
// Composer cleared immediately on submit, before the command resolves.
|
||||
await waitFor(() => expect(textarea).toHaveValue(""));
|
||||
// User starts typing a new message while the command is still in flight.
|
||||
fireEvent.change(textarea, { target: { value: "next message" } });
|
||||
|
||||
resolveRun();
|
||||
await waitFor(() => expect(addToast).toHaveBeenCalledWith(expect.any(String), "success"));
|
||||
// The in-flight text must not be wiped by the success callback.
|
||||
expect(textarea).toHaveValue("next message");
|
||||
});
|
||||
|
||||
it("blocks command dispatch and warns when attachments are staged", async () => {
|
||||
const sendMessage = vi.fn();
|
||||
setupMockChat({ activeSession: activeSessionFixture, messages: [], sendMessage });
|
||||
const addToast = vi.fn();
|
||||
|
||||
await renderWithAct(
|
||||
<ChatView projectId="proj-123" addToast={addToast} chatCommandContext={commandContext} />,
|
||||
);
|
||||
|
||||
const textarea = screen.getByTestId("chat-input");
|
||||
const file = new File(["hi"], "note.txt", { type: "text/plain" });
|
||||
const fileInput = screen.getByTestId("chat-file-input");
|
||||
await userEvent.upload(fileInput, file);
|
||||
|
||||
fireEvent.change(textarea, { target: { value: "/steer do X" } });
|
||||
fireEvent.keyDown(textarea, { key: "Enter" });
|
||||
|
||||
await waitFor(() =>
|
||||
expect(addToast).toHaveBeenCalledWith(expect.stringMatching(/attachment/i), "warning"),
|
||||
);
|
||||
expect(mockAddSteeringComment).not.toHaveBeenCalled();
|
||||
// Command was blocked, not sent — the composed text is preserved for the user to fix.
|
||||
expect(textarea).toHaveValue("/steer do X");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -1687,5 +1687,43 @@ describe("TaskPlannerChatTab", () => {
|
||||
await screen.findByText("please /steer this");
|
||||
expect(mockAddSteeringComment).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
// FUX-015 composer-wipe race: the draft is cleared on submit (before awaiting
|
||||
// command.run), not in the success path, so text typed while the command is
|
||||
// in flight is not wiped when run() resolves.
|
||||
it("preserves text typed while the command is in flight (no late wipe on success)", async () => {
|
||||
let resolveRun: () => void = () => {};
|
||||
const runPromise = new Promise<void>((resolve) => { resolveRun = resolve; });
|
||||
mockAddSteeringComment.mockReturnValueOnce(runPromise as unknown as ReturnType<typeof mockAddSteeringComment>);
|
||||
|
||||
renderPlannerChat({ task: makeTask("FN-7310", { column: "in-progress" }), projectId: "proj-1" });
|
||||
const textarea = await screen.findByLabelText("Message planner chat");
|
||||
|
||||
fireEvent.change(textarea, { target: { value: "/steer do X" } });
|
||||
fireEvent.keyDown(textarea, { key: "Enter" });
|
||||
|
||||
// Draft cleared immediately on submit, before the command resolves.
|
||||
await waitFor(() => expect(textarea).toHaveValue(""));
|
||||
// User begins a new message while the command is still in flight.
|
||||
fireEvent.change(textarea, { target: { value: "next message" } });
|
||||
|
||||
resolveRun();
|
||||
await waitFor(() => expect(mockAddSteeringComment).toHaveBeenCalledWith("FN-7310", "do X", "proj-1"));
|
||||
// The in-flight text must not be wiped by the success path.
|
||||
expect(textarea).toHaveValue("next message");
|
||||
});
|
||||
|
||||
// The planner command menu renders only commands (not skills), so its
|
||||
// accessible copy must say "command", not the reused skill-menu copy.
|
||||
it("labels the command menu with command-specific copy, not skill copy", async () => {
|
||||
renderPlannerChat({ task: makeTask("FN-7310", { column: "in-progress" }) });
|
||||
const textarea = await screen.findByLabelText("Message planner chat");
|
||||
|
||||
fireEvent.change(textarea, { target: { value: "/" } });
|
||||
|
||||
const menu = await screen.findByRole("listbox", { name: /command suggestions/i });
|
||||
expect(menu).toBeInTheDocument();
|
||||
expect(screen.queryByRole("listbox", { name: /skill suggestions/i })).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -49,7 +49,7 @@ export interface ChatCommand {
|
||||
* entry point into that same, already-shipped mechanism, not a new backend
|
||||
* behavior.
|
||||
*/
|
||||
export const CHAT_COMMANDS: ChatCommand[] = [
|
||||
export const CHAT_COMMANDS: readonly ChatCommand[] = [
|
||||
{
|
||||
trigger: "/steer",
|
||||
name: "steer",
|
||||
@@ -76,7 +76,7 @@ export interface ChatCommandMatch {
|
||||
* remainder character (e.g. "/steer" alone with nothing after it is not
|
||||
* a dispatchable match and falls through to normal send behavior).
|
||||
*/
|
||||
export function matchChatCommand(text: string, commands: ChatCommand[] = CHAT_COMMANDS): ChatCommandMatch | null {
|
||||
export function matchChatCommand(text: string, commands: readonly ChatCommand[] = CHAT_COMMANDS): ChatCommandMatch | null {
|
||||
for (const command of commands) {
|
||||
const prefix = `${command.trigger} `;
|
||||
if (!text.startsWith(prefix)) {
|
||||
@@ -97,10 +97,10 @@ export function matchChatCommand(text: string, commands: ChatCommand[] = CHAT_CO
|
||||
* slash), so the menu can show commands and skills side by side using one
|
||||
* shared filter value.
|
||||
*/
|
||||
export function filterChatCommands(filter: string, commands: ChatCommand[] = CHAT_COMMANDS): ChatCommand[] {
|
||||
export function filterChatCommands(filter: string, commands: readonly ChatCommand[] = CHAT_COMMANDS): ChatCommand[] {
|
||||
const normalized = filter.trim().toLowerCase();
|
||||
if (!normalized) {
|
||||
return commands;
|
||||
return [...commands];
|
||||
}
|
||||
return commands.filter((command) =>
|
||||
command.name.toLowerCase().includes(normalized)
|
||||
|
||||
Reference in New Issue
Block a user