From f67f8c1c333a21bea264ed8580adb32a61d1c18b Mon Sep 17 00:00:00 2001 From: ajnart Date: Thu, 20 Aug 2026 15:12:04 +0200 Subject: [PATCH] fix: address assistant review feedback --- .../assistant/assistant-auto-approval.spec.ts | 40 ++++ .../assistant/assistant-auto-approval.tsx | 17 +- .../assistant/assistant-panel.module.css | 37 +--- .../components/assistant/assistant-panel.tsx | 174 ++++++++++-------- .../assistant/assistant-provider.tsx | 5 - .../assistant-runtime-provider.spec.ts | 23 ++- .../assistant/assistant-runtime-provider.tsx | 26 +-- packages/translation/src/lang/en.json | 16 +- 8 files changed, 178 insertions(+), 160 deletions(-) diff --git a/apps/nextjs/src/components/assistant/assistant-auto-approval.spec.ts b/apps/nextjs/src/components/assistant/assistant-auto-approval.spec.ts index 485e708e6..0aae58d36 100644 --- a/apps/nextjs/src/components/assistant/assistant-auto-approval.spec.ts +++ b/apps/nextjs/src/components/assistant/assistant-auto-approval.spec.ts @@ -104,6 +104,46 @@ describe("assistant automatic approval tracker", () => { await act(async () => root.unmount()); }); + test("does not approve a ready action while switching conversations", async () => { + const container = document.createElement("div"); + containers.push(container); + document.body.append(container); + const root = createRoot(container); + let confirmationCount = 0; + const renderConversation = async (conversationId: string, ready: boolean) => { + await act(async () => { + root.render( + createElement( + AssistantAutoApprovalProvider, + { conversationId }, + createElement( + Fragment, + null, + createElement(AutoApprovalProbe), + createElement(AutomaticActionProbe, { + ready, + completed: false, + confirm: () => { + confirmationCount += 1; + }, + }), + ), + ), + ); + }); + }; + + await renderConversation("local-thread-1", false); + act(() => currentAutoApproval?.setEnabled(true)); + await renderConversation("local-thread-2", true); + + expect(confirmationCount).toBe(0); + expect(currentAutoApproval?.enabled).toBe(false); + expect(automaticActionInProgress).toBe(false); + + await act(async () => root.unmount()); + }); + test("clears claimed calls when automatic approvals are turned off", async () => { const container = document.createElement("div"); containers.push(container); diff --git a/apps/nextjs/src/components/assistant/assistant-auto-approval.tsx b/apps/nextjs/src/components/assistant/assistant-auto-approval.tsx index 7e37600f1..c4eddb62a 100644 --- a/apps/nextjs/src/components/assistant/assistant-auto-approval.tsx +++ b/apps/nextjs/src/components/assistant/assistant-auto-approval.tsx @@ -59,6 +59,9 @@ export const AssistantAutoApprovalProvider = ({ const trackerRef = useRef(createAssistantAutoApprovalTracker(AUTO_APPROVAL_MAX_ATTEMPTS)); const retryTimersRef = useRef(new Map()); const previousConversationIdRef = useRef(conversationId); + const conversationMatches = previousConversationIdRef.current === conversationId; + const effectivePreparationEnabled = conversationMatches && preparationEnabled; + const effectiveApprovalEnabled = conversationMatches && approvalEnabled; const clearRetryTimer = useCallback((toolCallId: string) => { const retry = retryTimersRef.current.get(toolCallId); @@ -115,7 +118,7 @@ export const AssistantAutoApprovalProvider = ({ const requestAction = useCallback( (toolCallId: string, confirm: () => void, mode: AssistantAutomaticActionMode = "preparation") => { - const modeEnabled = mode === "approval" ? approvalEnabled : preparationEnabled; + const modeEnabled = mode === "approval" ? effectiveApprovalEnabled : effectivePreparationEnabled; if (!modeEnabled || !trackerRef.current.claim(toolCallId)) return false; try { @@ -135,7 +138,7 @@ export const AssistantAutoApprovalProvider = ({ return false; } }, - [approvalEnabled, clearRetryTimer, preparationEnabled], + [clearRetryTimer, effectiveApprovalEnabled, effectivePreparationEnabled], ); const completeAction = useCallback( @@ -147,12 +150,12 @@ export const AssistantAutoApprovalProvider = ({ ); const value = useMemo(() => { - const enabled = preparationEnabled || approvalEnabled; + const enabled = effectivePreparationEnabled || effectiveApprovalEnabled; return { enabled, setEnabled, - preparationEnabled, - approvalEnabled, + preparationEnabled: effectivePreparationEnabled, + approvalEnabled: effectiveApprovalEnabled, setPreparationEnabled, setApprovalEnabled, requestAction, @@ -160,9 +163,9 @@ export const AssistantAutoApprovalProvider = ({ retryRevision, }; }, [ - approvalEnabled, completeAction, - preparationEnabled, + effectiveApprovalEnabled, + effectivePreparationEnabled, requestAction, retryRevision, setApprovalEnabled, diff --git a/apps/nextjs/src/components/assistant/assistant-panel.module.css b/apps/nextjs/src/components/assistant/assistant-panel.module.css index 664dd41ea..1abba2121 100644 --- a/apps/nextjs/src/components/assistant/assistant-panel.module.css +++ b/apps/nextjs/src/components/assistant/assistant-panel.module.css @@ -181,23 +181,6 @@ flex: 0 0 auto; } -.windowControls { - flex: 0 0 auto; - margin: 0; - padding: 0; - border: 0; -} - -.windowControl { - width: 2.25rem; - height: 2.25rem; -} - -.windowControl:focus-visible { - outline: 2px solid var(--mantine-primary-color-filled); - outline-offset: 2px; -} - .panelAction { width: 2.625rem; height: 2.625rem; @@ -209,10 +192,6 @@ padding: 0.45rem 0.5rem 0.45rem 0.65rem; } - .composerHints { - display: none; - } - .historyButton { width: 2.625rem; max-width: 2.625rem; @@ -447,6 +426,9 @@ .message { margin-block: 0.9rem; +} + +.message[data-static] { contain-intrinsic-size: auto 12rem; content-visibility: auto; } @@ -1224,6 +1206,11 @@ background: var(--mantine-color-default-hover); } +.messageMoreItem:focus-visible { + outline: 2px solid var(--mantine-primary-color-filled); + outline-offset: -2px; +} + .messageMoreSeparator { height: 1px; margin-block: 0.2rem; @@ -1554,10 +1541,6 @@ pointer-events: none; } -.composerHints { - padding-inline: 0.45rem; -} - .composerFooter { min-width: 0; max-width: 100%; @@ -1826,10 +1809,6 @@ padding-inline: 0.1rem; } - .composerHints { - display: none; - } - .runtimeSelectorTrigger { max-width: 100%; } diff --git a/apps/nextjs/src/components/assistant/assistant-panel.tsx b/apps/nextjs/src/components/assistant/assistant-panel.tsx index cbf4400ca..e9602fbb5 100644 --- a/apps/nextjs/src/components/assistant/assistant-panel.tsx +++ b/apps/nextjs/src/components/assistant/assistant-panel.tsx @@ -99,7 +99,6 @@ import { IconFileExport, IconLink, IconMessage, - IconMicrophone, IconMinus, IconPaperclip, IconPalette, @@ -1259,7 +1258,7 @@ const UserMessage = () => { const isEditing = useAuiState((state) => state.composer.isEditing); if (isEditing) return ; return ( - + {(quote) => ( @@ -1941,51 +1940,58 @@ const WebSearchActivity = () => { ); }; -const AssistantMessage = () => ( - - - {({ part, children }) => { - switch (part.type) { - case "group-agent-trace": { - const startIndex = part.indices[0]; - const endIndex = part.indices.at(-1); - if (startIndex === undefined || endIndex === undefined) return null; - return ( - - {children} - - ); +const AssistantMessage = () => { + const isComplete = useAuiState((state) => state.message.status?.type === "complete"); + + return ( + + + {({ part, children }) => { + switch (part.type) { + case "group-agent-trace": { + const startIndex = part.indices[0]; + const endIndex = part.indices.at(-1); + if (startIndex === undefined || endIndex === undefined) return null; + return ( + + {children} + + ); + } + case "group-reasoning": + return <>{children}; + case "group-tool": + return {children}; + case "indicator": + return ; + case "text": + return ; + case "reasoning": + return ; + case "tool-call": + return part.toolUI ?? ; + case "source": + return ; + case "file": + return ; + case "image": + return ; + case "data": + return part.dataRendererUI; + default: + return null; } - case "group-reasoning": - return <>{children}; - case "group-tool": - return {children}; - case "indicator": - return ; - case "text": - return ; - case "reasoning": - return ; - case "tool-call": - return part.toolUI ?? ; - case "source": - return ; - case "file": - return ; - case "image": - return ; - case "data": - return part.dataRendererUI; - default: - return null; - } - }} - - - - - -); + }} + + + + + + ); +}; const assistantThreadMessageComponents = { UserMessage, AssistantMessage }; @@ -2001,7 +2007,13 @@ const ThreadListItem = () => { const [exporting, setExporting] = useState(false); const [actionsOpened, setActionsOpened] = useState(false); const [renameOpened, setRenameOpened] = useState(false); + const [renaming, setRenaming] = useState(false); const [nextTitle, setNextTitle] = useState(title ?? ""); + const renameInputRef = useRef(null); + + useEffect(() => { + if (renameOpened) renameInputRef.current?.focus(); + }, [renameOpened]); const exportConversation = async () => { if (!remoteId || exporting) return; @@ -2023,12 +2035,22 @@ const ThreadListItem = () => { } }; - const renameConversation = () => { + const renameConversation = async () => { const trimmedTitle = nextTitle.trim(); - if (!trimmedTitle) return; - aui.threadListItem().rename(trimmedTitle); - setRenameOpened(false); - setActionsOpened(false); + if (!trimmedTitle || renaming) return; + setRenaming(true); + try { + await aui.threadListItem().rename(trimmedTitle); + setRenameOpened(false); + setActionsOpened(false); + } catch { + showErrorNotification({ + title: t("renameConversation.failedTitle"), + message: t("renameConversation.failedDescription"), + }); + } finally { + setRenaming(false); + } }; const deleteConversation = () => { @@ -2037,7 +2059,16 @@ const ThreadListItem = () => { children: t("deleteConversation.description", { title: title ?? t("newConversation") }), confirmProps: { color: "red" }, labels: { confirm: t("delete"), cancel: t("cancel") }, - onConfirm: () => aui.threadListItem().delete(), + onConfirm: async () => { + try { + await aui.threadListItem().delete(); + } catch { + showErrorNotification({ + title: t("deleteConversation.failedTitle"), + message: t("deleteConversation.failedDescription"), + }); + } + }, }); }; @@ -2045,16 +2076,16 @@ const ThreadListItem = () => { return ( setNextTitle(event.currentTarget.value)} aria-label={t("renameConversation.label")} maxLength={72} size="xs" - data-autofocus onKeyDown={(event) => { - if (event.key === "Enter") renameConversation(); - if (event.key === "Escape") setRenameOpened(false); + if (event.key === "Enter") void renameConversation(); + if (event.key === "Escape" && !renaming) setRenameOpened(false); }} /> { size="sm" aria-label={t("renameConversation.save")} disabled={!nextTitle.trim()} - onClick={renameConversation} + loading={renaming} + loaderProps={{ type: "bars" }} + onClick={() => void renameConversation()} > @@ -2072,6 +2105,7 @@ const ThreadListItem = () => { color="gray" size="sm" aria-label={t("cancel")} + disabled={renaming} onClick={() => setRenameOpened(false)} > @@ -2902,26 +2936,6 @@ const Composer = (props: ComposerProps) => { - state.thread.capabilities.dictation && state.composer.dictation === undefined} - > - - - - - - - - - state.composer.dictation !== undefined}> - - - - - - - - (null); const panelRef = useRef(null); - const stopVoiceActivityWithoutSurface = () => { + const stopSpeechWithoutSurface = () => { if (hasVisibleWidget) return; - const composer = aui.composer(); - if (composer.getState().dictation !== undefined) composer.stopDictation(); const thread = aui.thread(); if (thread.getState().speech !== undefined) thread.stopSpeaking(); }; const minimize = () => { - stopVoiceActivityWithoutSurface(); + stopSpeechWithoutSurface(); onClose(); }; @@ -3408,7 +3420,7 @@ export const AssistantPanel = ({ autoFocusComposer onMinimize={minimize} onDismiss={() => { - stopVoiceActivityWithoutSurface(); + stopSpeechWithoutSurface(); onDismissActivity(); onClose(); }} diff --git a/apps/nextjs/src/components/assistant/assistant-provider.tsx b/apps/nextjs/src/components/assistant/assistant-provider.tsx index dd61b214a..0f46d7a0b 100644 --- a/apps/nextjs/src/components/assistant/assistant-provider.tsx +++ b/apps/nextjs/src/components/assistant/assistant-provider.tsx @@ -5,7 +5,6 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { useRouter } from "next/navigation"; import type { AttachmentAdapter, - DictationAdapter, MessageFormatAdapter, RemoteThreadListAdapter, SpeechSynthesisAdapter, @@ -19,7 +18,6 @@ import { useAuiEvent, useAuiState, useRemoteThreadListRuntime, - WebSpeechDictationAdapter, WebSpeechSynthesisAdapter, } from "@assistant-ui/react"; import { useChat } from "@ai-sdk/react"; @@ -486,10 +484,8 @@ const AssistantThreadRuntime = () => { ), [selectedModel], ); - const [dictation, setDictation] = useState(); const [speech, setSpeech] = useState(); useEffect(() => { - if (WebSpeechDictationAdapter.isSupported()) setDictation(new WebSpeechDictationAdapter()); if ("speechSynthesis" in window && typeof SpeechSynthesisUtterance !== "undefined") { setSpeech(new WebSpeechSynthesisAdapter()); } @@ -530,7 +526,6 @@ const AssistantThreadRuntime = () => { adapters: { history, attachments, - dictation, feedback, speech, }, diff --git a/apps/nextjs/src/components/assistant/assistant-runtime-provider.spec.ts b/apps/nextjs/src/components/assistant/assistant-runtime-provider.spec.ts index 44cd03775..24f1aa940 100644 --- a/apps/nextjs/src/components/assistant/assistant-runtime-provider.spec.ts +++ b/apps/nextjs/src/components/assistant/assistant-runtime-provider.spec.ts @@ -26,7 +26,8 @@ import { z } from "zod/v4"; import { showErrorNotification } from "@homarr/notifications"; import { - AssistantComposerSurfaceProvider, + AssistantComposerRuntimeProvider, + AssistantComposerSurfaceBoundary, AssistantRunFocusPreserver, AssistantRuntimeProviderWithTools, assistantSurfaceComposerCacheLimit, @@ -148,14 +149,22 @@ const SurfaceTestAssistant = ({ { runtime, toolkit }, createElement(AssistantRunFocusPreserver), createElement( - AssistantComposerSurfaceProvider, - { surfaceId: "surface-a" }, - createElement(SurfaceProbe, { id: "a", onReady }), + AssistantComposerRuntimeProvider, + null, + createElement( + AssistantComposerSurfaceBoundary, + { surfaceId: "surface-a" }, + createElement(SurfaceProbe, { id: "a", onReady }), + ), ), createElement( - AssistantComposerSurfaceProvider, - { surfaceId: "surface-b" }, - createElement(SurfaceProbe, { id: "b", onReady }), + AssistantComposerRuntimeProvider, + null, + createElement( + AssistantComposerSurfaceBoundary, + { surfaceId: "surface-b" }, + createElement(SurfaceProbe, { id: "b", onReady }), + ), ), simulateRunFocus ? createElement(LexicalRunFocusProbe) : null, ); diff --git a/apps/nextjs/src/components/assistant/assistant-runtime-provider.tsx b/apps/nextjs/src/components/assistant/assistant-runtime-provider.tsx index e108e395e..85edd94ed 100644 --- a/apps/nextjs/src/components/assistant/assistant-runtime-provider.tsx +++ b/apps/nextjs/src/components/assistant/assistant-runtime-provider.tsx @@ -42,7 +42,6 @@ class AssistantSurfaceComposerRuntimeCore extends INTERNAL.DefaultThreadComposer } public dispose() { - if (this.dictation !== undefined) this.stopDictation(); composerConnectionDisposers.get(this)?.(); composerConnectionDisposers.delete(this); } @@ -158,9 +157,6 @@ const createSurfaceComposerHost = ( get attachments() { return getThreadAdapters(threadCore)?.attachments ? attachments : undefined; }, - get dictation() { - return getThreadAdapters(threadCore)?.dictation; - }, }; return new Proxy(threadCore as ThreadCoreWithAdapters, { @@ -302,7 +298,6 @@ export const createAssistantSurfaceRuntime = (runtime: AssistantRuntime): Assist const nextCore = sharedBinding.getState(); const nextConversationId = getConversationId(); if (nextCore !== activeCore || nextConversationId !== activeConversationId) { - if (activeComposer.dictation !== undefined) activeComposer.stopDictation(); if (activeCore.speech !== undefined) activeCore.stopSpeaking(); unsubscribeComposer(); releaseConversation(activeConversationId); @@ -365,7 +360,10 @@ export const createAssistantSurfaceRuntime = (runtime: AssistantRuntime): Assist registerModelContextProvider: () => () => {}, dispose() { activeConversationCounts.clear(); - const threadCores = new Set(Array.from(surfaceCoreByConversation.values(), (entry) => entry.source)); + const threadCores = new Set([ + ...Array.from(surfaceCoreByConversation.values(), (entry) => entry.source), + ...Array.from(composerByConversation.values(), (entry) => entry.source), + ]); for (const threadCore of threadCores) { if (threadCore.speech !== undefined) threadCore.stopSpeaking(); } @@ -382,7 +380,7 @@ const AssistantComposerSurfaceContext = createContext { const value = useContext(AssistantComposerSurfaceContext); - if (!value) throw new Error("useAssistantComposerSurface must be used within AssistantComposerSurfaceProvider"); + if (!value) throw new Error("useAssistantComposerSurface must be used within AssistantComposerSurfaceBoundary"); return value; }; @@ -576,10 +574,6 @@ export const AssistantRunFocusPreserver = () => { return null; }; -interface AssistantComposerSurfaceProviderProps extends PropsWithChildren { - surfaceId: string; -} - /** * Gives every rendered assistant surface one shared local composer while preserving the shared * conversation. Individual surfaces should add AssistantComposerSurfaceBoundary around their UI. @@ -601,13 +595,3 @@ export const AssistantComposerRuntimeProvider = ({ children }: PropsWithChildren ); }; - -/** - * Convenience boundary for a standalone surface. Use AssistantComposerRuntimeProvider with - * localized boundaries when multiple surfaces need to share one draft and attachment queue. - */ -export const AssistantComposerSurfaceProvider = ({ children, surfaceId }: AssistantComposerSurfaceProviderProps) => ( - - {children} - -); diff --git a/packages/translation/src/lang/en.json b/packages/translation/src/lang/en.json index f665574b8..36c806e1c 100644 --- a/packages/translation/src/lang/en.json +++ b/packages/translation/src/lang/en.json @@ -2309,7 +2309,6 @@ "open": "Open Homarr Assistant", "close": "Close assistant", "minimize": "Minimize assistant", - "windowControls": "Conversation window controls", "spotlight": "Ask Homarr Assistant", "spotlightDescription": "Type and send a prompt without leaving the command menu.", "spotlightPrompt": { @@ -2334,13 +2333,16 @@ "moreActions": "More response actions", "renameConversation": { "action": "Rename", - "title": "Rename conversation", "label": "Conversation name", - "save": "Save name" + "save": "Save name", + "failedTitle": "Conversation could not be renamed", + "failedDescription": "Homarr could not rename this conversation. Try again." }, "deleteConversation": { "title": "Delete conversation?", - "description": "“{title}” and its messages will be permanently deleted." + "description": "“{title}” and its messages will be permanently deleted.", + "failedTitle": "Conversation could not be deleted", + "failedDescription": "Homarr could not delete this conversation. Try again." }, "closeHistory": "Close conversation history", "openHistory": "Open conversation history", @@ -2416,13 +2418,10 @@ "notHelpful": "Not helpful", "readAloud": "Read response aloud", "stopReading": "Stop reading response", - "startVoiceInput": "Start voice input", - "stopVoiceInput": "Stop voice input", "feedbackError": "Your feedback could not be saved. Try again.", "exportMarkdown": "Export as Markdown", "exportConversation": { "action": "Export conversation as Markdown", - "working": "Exporting conversation…", "failedTitle": "Conversation could not be exported", "failedDescription": "Homarr could not load this conversation. Try again." }, @@ -2447,7 +2446,6 @@ }, "mentions": { "menu": "Add Homarr context", - "hint": "Type @ to add context", "context": "Context", "app": "Apps", "integration": "Integrations", @@ -2462,7 +2460,6 @@ }, "commands": { "menu": "Assistant commands", - "hint": "Type / for shortcuts", "health": "Prepare a service-health check", "explore": "Prepare an overview of this Homarr instance", "media": "Prepare a media search", @@ -2575,7 +2572,6 @@ "otherPlaceholder": "Type another answer…", "submit": "Send answer", "cancelOther": "Cancel other answer", - "automaticAffirmative": "Choosing the affirmative answer automatically", "optionKind": { "affirmative": "Confirm", "negative": "Decline",