From c82cbff3ca9ca82127e12ab95f902824bb90ab5b Mon Sep 17 00:00:00 2001 From: Sophia <44297511+SweetSophia@users.noreply.github.com> Date: Wed, 1 Apr 2026 19:44:26 +0200 Subject: [PATCH 01/13] Addressing PR comments (#1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor: extract ChatPanel into focused modules Decompose the 1472-line ChatPanel god component into four focused files: - toolDefinitions.ts (133 lines): tool defs, system prompt builder, config guard - ChatSubComponents.tsx (178 lines): CharacterAvatar, StageIndicator, ActionsTaken - SettingsModal.tsx (270 lines): LLM + image generation configuration UI - useConversationEngine.ts (341 lines): conversation loop + tool dispatch hook ChatPanel/index.tsx reduced from 1472 → 632 lines (-57%), now a thin shell that wires state to UI. No behavior changes — pure structural refactor. Benefits: - Conversation engine is testable in isolation - Settings modal can be reasoned about independently - Each file has a single clear responsibility - Future PRs can target specific modules without touching the whole * fix: consolidate duplicate ModManager imports in useConversationEngine.ts Agent-Logs-Url: https://github.com/SweetSophia/OpenRoom/sessions/f4ebc43a-24c2-4853-b135-f234fd25d12b Co-authored-by: SweetSophia <44297511+SweetSophia@users.noreply.github.com> * fix: stabilize runConversation identity to prevent listener churn runConversation was recreated on every render, causing the onUserAction subscription effect to unsubscribe/resubscribe on each render. App actions emitted during the cleanup-rebind window could be silently dropped. Since runConversation only reads from refs (no reactive state), wrapping it in useCallback with an empty dependency array makes its identity stable across renders, eliminating the churn. * fix: address code review findings across ChatPanel modules useConversationEngine: - Wrap callbacks in ref to prevent stale closure in stabilized runConversation - Break outer loop after respond_to_user to avoid extra model round-trip - Add console.warn for unparseable tool call arguments - Handle loadMemories rejection with fallback to empty array ChatSubComponents: - Reactivate existing inactive layer instead of skipping it - Use ref for cleanup timeout to prevent stale timeout interference index.tsx: - Move setSessionPath call from render body to useEffect - Reduce reload effect deps to sessionPath only, add cancellation guard SettingsModal: - Sync local state from parent props via useEffect toolDefinitions: - Replace hardcoded emotion examples with generic reference to character keys * fix: address review feedback on types, timeout cleanup, and config guard Agent-Logs-Url: https://github.com/SweetSophia/OpenRoom/sessions/6cd6bd9b-6a80-474a-b067-8f2ff9f8f32a Co-authored-by: SweetSophia <44297511+SweetSophia@users.noreply.github.com> * fix: address Copilot review — tool loop, save snapshot, layer cleanup useConversationEngine: - Remove inner break on respond_to_user so sibling tool calls (e.g. finish_target) still execute before outer loop stops index.tsx: - Guard loadMemories .then/.catch with cancelled flag - Snapshot session/data at debounce schedule time instead of reading refs at fire-time, preventing cross-session data mixing - Remove now-unused sessionPathRef2 ChatSubComponents: - On layer reactivation, cancel pending cleanup timeout and explicitly deactivate all other layers to prevent dual-active state * fix: Prettier import style, stale memory guard, remove redundant ref useConversationEngine: - Break React import to multiline per Prettier config - Guard loadMemories handlers against session change during async gap index.tsx: - Remove unused sessionPathRef_forSet ref — setSessionPath in useEffect alone is sufficient for module-level sync * fix: flush debounced save on cleanup, remove stale dep in handleResetSession index.tsx: - Flush pending saveChatHistory in cleanup instead of discarding, preventing data loss on rapid session switches - Remove modCollection from handleResetSession dependency array (uses refs and functional state updates, no direct read needed) * fix(ChatPanel): useLayoutEffect for setSessionPath; fix debounced save write amplification Agent-Logs-Url: https://github.com/SweetSophia/OpenRoom/sessions/4ddaf906-91f2-4ac3-ac97-7ce3fd331545 Co-authored-by: SweetSophia <44297511+SweetSophia@users.noreply.github.com> * refactor: extract PendingSaveSnapshot type from inline ref type Agent-Logs-Url: https://github.com/SweetSophia/OpenRoom/sessions/4ddaf906-91f2-4ac3-ac97-7ce3fd331545 Co-authored-by: SweetSophia <44297511+SweetSophia@users.noreply.github.com> * fix: ModManager value import, sessionPath out of save deps, redact sensitive args from warn log Agent-Logs-Url: https://github.com/SweetSophia/OpenRoom/sessions/1954e0a2-8dec-4da9-85f4-92df5e2ddf00 Co-authored-by: SweetSophia <44297511+SweetSophia@users.noreply.github.com> * fix: only break loop when respond_to_user is sole tool call Previously the loop broke after respond_to_user even when other tools ran in the same batch. If the model planned to emit follow-up tool calls (e.g. finish_target) in a subsequent round-trip, those were silently skipped. Now the loop only terminates when respond_to_user was the only tool call — batched tool calls get their follow-up round-trip as intended. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> --- .../ChatPanel/ChatSubComponents.tsx | 198 +++ .../components/ChatPanel/SettingsModal.tsx | 292 +++++ .../src/components/ChatPanel/index.tsx | 1103 +++-------------- .../components/ChatPanel/toolDefinitions.ts | 136 ++ .../ChatPanel/useConversationEngine.ts | 373 ++++++ 5 files changed, 1162 insertions(+), 940 deletions(-) create mode 100644 apps/webuiapps/src/components/ChatPanel/ChatSubComponents.tsx create mode 100644 apps/webuiapps/src/components/ChatPanel/SettingsModal.tsx create mode 100644 apps/webuiapps/src/components/ChatPanel/toolDefinitions.ts create mode 100644 apps/webuiapps/src/components/ChatPanel/useConversationEngine.ts diff --git a/apps/webuiapps/src/components/ChatPanel/ChatSubComponents.tsx b/apps/webuiapps/src/components/ChatPanel/ChatSubComponents.tsx new file mode 100644 index 0000000..57798a4 --- /dev/null +++ b/apps/webuiapps/src/components/ChatPanel/ChatSubComponents.tsx @@ -0,0 +1,198 @@ +/** + * Helper sub-components extracted from ChatPanel + * + * StageIndicator, ActionsTaken, CharacterAvatar, renderMessageContent + */ + +import React, { useState, useEffect, useCallback, memo, useRef } from 'react'; +import { ChevronDown, ChevronRight } from 'lucide-react'; +import type { CharacterConfig } from '@/lib/characterManager'; +import { resolveEmotionMedia } from '@/lib/characterManager'; +import type { ModManager } from '@/lib/modManager'; +import styles from './index.module.scss'; + +// --------------------------------------------------------------------------- +// Render message content — formats (action text) as styled spans +// --------------------------------------------------------------------------- + +export function renderMessageContent(content: string): React.ReactNode { + const parts = content.split(/(\([^)]+\))/g); + return parts.map((part, i) => { + if (/^\([^)]+\)$/.test(part)) { + return ( + + {part} + + ); + } + return part; + }); +} + +// --------------------------------------------------------------------------- +// Stage Indicator +// --------------------------------------------------------------------------- + +export const StageIndicator: React.FC<{ modManager: ModManager | null }> = ({ modManager }) => { + if (!modManager) return null; + + const total = modManager.stageCount; + const current = modManager.currentStageIndex; + const finished = modManager.isFinished; + + return ( +
+ + Stage {finished ? total : current + 1}/{total} + +
+ {Array.from({ length: total }, (_, i) => ( +
+ ))} +
+
+ ); +}; + +// --------------------------------------------------------------------------- +// Actions Taken (collapsible) +// --------------------------------------------------------------------------- + +export const ActionsTaken: React.FC<{ calls: string[] }> = ({ calls }) => { + const [open, setOpen] = useState(false); + if (calls.length === 0) return null; + + return ( +
+ + {open && ( +
+ {calls.map((c, i) => ( +
{c}
+ ))} +
+ )} +
+ ); +}; + +// --------------------------------------------------------------------------- +// CharacterAvatar – crossfade between emotion media without flashing +// --------------------------------------------------------------------------- + +interface AvatarLayer { + url: string; + type: 'video' | 'image'; + active: boolean; +} + +export const CharacterAvatar: React.FC<{ + character: CharacterConfig; + emotion?: string; + onEmotionEnd: () => void; +}> = memo(({ character, emotion, onEmotionEnd }) => { + const isIdle = !emotion; + const media = resolveEmotionMedia(character, emotion || 'default'); + + const [layers, setLayers] = useState(() => + media ? [{ url: media.url, type: media.type, active: true }] : [], + ); + const activeUrl = layers.find((l) => l.active)?.url; + const cleanupRef = useRef | null>(null); + + useEffect(() => { + return () => { + if (cleanupRef.current) clearTimeout(cleanupRef.current); + }; + }, []); + + useEffect(() => { + if (!media) { + setLayers([]); + return; + } + if (media.url === activeUrl) return; + setLayers((prev) => { + // If the URL already exists (possibly inactive), reactivate it + const existing = prev.find((l) => l.url === media.url); + if (existing) { + // Cancel any pending cleanup that might remove this layer + if (cleanupRef.current) { + clearTimeout(cleanupRef.current); + cleanupRef.current = null; + } + return prev.map((l) => ({ + ...l, + active: l.url === media.url, + })); + } + return [...prev, { url: media.url, type: media.type, active: false }]; + }); + }, [media?.url, activeUrl]); + + const handleMediaReady = useCallback((readyUrl: string) => { + setLayers((prev) => { + const staleUrls = prev.filter((l) => l.url !== readyUrl).map((l) => l.url); + if (cleanupRef.current) clearTimeout(cleanupRef.current); + cleanupRef.current = setTimeout(() => { + setLayers((curr) => curr.filter((l) => !staleUrls.includes(l.url))); + }, 300); + return prev.map((l) => ({ ...l, active: l.url === readyUrl })); + }); + }, []); + + if (layers.length === 0) { + return
{character.character_name.charAt(0)}
; + } + + return ( + <> + {layers.map((layer) => { + const layerStyle: React.CSSProperties = { + position: 'absolute', + inset: 0, + opacity: layer.active ? 1 : 0, + transition: 'opacity 0.25s ease-out', + }; + if (layer.type === 'video') { + return ( +