refactor: decompose AutomationChatView into focused components (FE-18)#920
Conversation
Extract the 1,523-line monolith into 7 focused components + 1 composable, each under 400 lines: - ChatHeroHeader (119 lines) - page title, subtitle, action buttons - LlmHealthStatusBar (188 lines) - provider health status banner - ChatSessionSidebar (254 lines) - session list, create form, board selector - ChatMessageList (361 lines) - message rendering with markdown support - ChatParseHintCard (169 lines) - parse-hint instruction cards - ChatToolCallDetails (135 lines) - tool call metadata expander - ChatComposeBar (128 lines) - message input, proposal checkbox, send - useAutomationChat composable (394 lines) - orchestration state/logic Parent AutomationChatView reduced to 235 lines (template shell only). All existing tests pass without modification. Closes #859
Adversarial self-reviewChecked areasProps/events wiring: Verified all parent-to-child prop bindings and child-to-parent event emissions match. Vue's automatic camelCase-to-kebab-case normalization handles the Lost functionality check: All template sections from the original 1,523-line file are accounted for in the extracted components:
Component boundary issues: None found. The composable Test coverage: All 2,552 tests pass without modification. The tests mount Accessibility: All CSS scoping: Each component has its own No issues foundThe decomposition is functionally equivalent to the original. No fixes needed. |
There was a problem hiding this comment.
Code Review
This pull request refactors the AutomationChatView by extracting its business logic into a dedicated useAutomationChat composable and decomposing the UI into several modular components, such as ChatMessageList, ChatComposeBar, and ChatSessionSidebar. The review feedback focuses on performance optimizations, specifically identifying inefficient object instantiation and redundant JSON parsing within v-for loops and sort comparators. Additionally, there is a recommendation to refactor duplicated message-sending logic within the new composable to improve maintainability.
| return [...current.recentMessages].sort((a, b) => ( | ||
| new Date(a.createdAt).getTime() - new Date(b.createdAt).getTime() | ||
| )) |
There was a problem hiding this comment.
Creating and parsing Date objects inside a sort comparator is a performance anti-pattern. The sort method calls the comparator
| return [...current.recentMessages].sort((a, b) => ( | |
| new Date(a.createdAt).getTime() - new Date(b.createdAt).getTime() | |
| )) | |
| return [...current.recentMessages].sort((a, b) => ( | |
| Date.parse(a.createdAt) - Date.parse(b.createdAt) | |
| )) |
| async function handleSendMessage() { | ||
| if (!selectedSession.value) { | ||
| toast.error('Select a session first') | ||
| return | ||
| } | ||
| if (!messageContent.value.trim()) { | ||
| return | ||
| } | ||
|
|
||
| try { | ||
| sendingMessage.value = true | ||
| await chatApi.sendMessage(selectedSession.value.id, { | ||
| content: messageContent.value.trim(), | ||
| requestProposal: requestProposal.value, | ||
| }) | ||
| messageContent.value = '' | ||
| requestProposal.value = false | ||
| await loadSession(selectedSession.value.id) | ||
| } catch (e: unknown) { | ||
| toast.error(getErrorDisplay(e, 'Failed to send message').message) | ||
| } finally { | ||
| sendingMessage.value = false | ||
| } | ||
| } | ||
|
|
||
| async function handleSkipClarification() { | ||
| if (!selectedSession.value) return | ||
| try { | ||
| sendingMessage.value = true | ||
| await chatApi.sendMessage(selectedSession.value.id, { | ||
| content: 'Just do your best', | ||
| requestProposal: requestProposal.value, | ||
| }) | ||
| messageContent.value = '' | ||
| requestProposal.value = false | ||
| await loadSession(selectedSession.value.id) | ||
| } catch (e: unknown) { | ||
| toast.error(getErrorDisplay(e, 'Failed to send message').message) | ||
| } finally { | ||
| sendingMessage.value = false | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for sending a message in handleSendMessage and handleSkipClarification is almost identical. This duplication makes the code harder to maintain and increases the risk of bugs if the message-sending flow changes in the future. Consider refactoring the core sending logic into a private helper function.
| function isTruncatedJson(content: string): boolean { | ||
| if (!content) return false | ||
| const trimmed = content.trim() | ||
| if (!trimmed.startsWith('{') && !trimmed.startsWith('[')) return false | ||
| try { | ||
| JSON.parse(trimmed) | ||
| return false | ||
| } catch { | ||
| return true | ||
| } | ||
| } |
There was a problem hiding this comment.
The isTruncatedJson function is called within the template loop for every assistant or system message. Performing JSON.parse inside a render cycle is expensive and can lead to UI jank as the message list grows. Consider pre-calculating this state when messages are loaded or using a more efficient heuristic.
| > | ||
| <div class="td-message-header"> | ||
| <span class="td-message-role">{{ normalizeChatRole(message.role) }}</span> | ||
| <span class="td-message-time">{{ new Date(message.createdAt).toLocaleTimeString() }}</span> |
There was a problem hiding this comment.
| v-if="parseToolCallMetadata(message)" | ||
| :metadata="parseToolCallMetadata(message)!" |
There was a problem hiding this comment.
The parseToolCallMetadata function is called twice for every message that contains tool call metadata (once for v-if and once for the :metadata prop). Since this function performs JSON.parse, it should be optimized to avoid redundant parsing. Consider using a computed property to pre-parse metadata for all messages.
| @click="emit('select-session', session.id)" | ||
| > | ||
| <div class="td-session-title">{{ session.title }}</div> | ||
| <div class="td-session-meta">{{ new Date(session.updatedAt).toLocaleString() }}</div> |
- Use Date.parse() instead of new Date().getTime() in sort comparator to avoid unnecessary object allocation (Gemini comment) - Extract duplicated send logic into sendMessageToSession helper shared by handleSendMessage and handleSkipClarification (Gemini comment) - Pre-compute toolCallMetadata and truncatedJson via computed maps to eliminate redundant JSON.parse calls during render (Gemini comment)
Adversarial Review of PR #920SummaryThe decomposition is structurally sound. Props/events wiring is correct across all parent-child boundaries, no functionality was lost during extraction, and all frontend checks pass (typecheck, build, lint, 2552 tests). I pushed a follow-up commit addressing the legitimate Gemini findings plus one additional concern. Gemini Bot Comments AssessmentAll 6 Gemini comments were reviewed. Findings grouped by merit: Legitimate and fixed (3):
Valid but low-impact (3, noted but partly addressed):
My Own FindingsIssue: Massive CSS duplication across child components The
This is not a blocker for this PR since the original file also had all these styles (just in one place), but the decomposition turned a single-location issue into a multi-file one. Recommend tracking as a follow-up. No functional regressions found:
No new tests for new component boundaries: VerificationAll checks pass after the fix commit:
|
Summary
AutomationChatView.vuemonolith into 7 focused components and 1 composable, all under 400 lines eachuseAutomationChatcomposable for clean separation of state management from template renderingExtracted components
ChatHeroHeaderLlmHealthStatusBarChatSessionSidebarChatMessageListChatParseHintCardChatToolCallDetailsChatComposeBaruseAutomationChat(composable)AutomationChatView(parent)Closes #859
Test plan
npm run typecheck)npm run build)npm run lint)