refactor: feedback from agent-skills for frontend#22300
Merged
Conversation
DanielleMaywood
changed the title
johnstcn
added a commit
that referenced
this pull request
Feb 25, 2026
DanielleMaywood
force-pushed
the
agents-review-fixes
branch
2 times, most recently
from
3dc7333 to
cf2a2f0
Compare
Split the 1258-line ModelForm.tsx into three focused files: - modelConfigSchemas.ts (166 lines): Provider schema reference data and getModelConfigSchemaReference helper. - modelConfigFormLogic.ts (604 lines): Form state types, empty state constant, parse helpers, extractModelConfigFormState, and buildModelConfigFromForm. Deduplicates the openrouter/vercel switch cases via a shared buildReasoningProviderOptions helper. - ModelForm.tsx (503 lines): React component, props type, and JSX only. Also updates ModelConfigFields.tsx to import types from the new modelConfigFormLogic module.
Split the monolithic 1834-line tool.tsx into focused per-tool module files under a tool/ directory. This improves navigability and maintainability. New structure: - tool/ToolCollapsible.tsx: Shared expand/collapse pattern extracted from repeated usage across ~8 components - tool/utils.ts: All utility functions, types, and constants - tool/ToolIcon.tsx: Icon resolver component - tool/ToolLabel.tsx: Label resolver component - tool/ExecuteTool.tsx: Execute, ExecuteAuthRequired, WaitForExternalAuth - tool/ReadFileTool.tsx: Read file viewer (now uses ToolCollapsible) - tool/WriteFileTool.tsx: Write file diff viewer (now uses ToolCollapsible) - tool/EditFilesTool.tsx: Edit files diff viewer (now uses ToolCollapsible) - tool/CreateWorkspaceTool.tsx: Workspace creation with build logs - tool/SubagentTool.tsx: Sub-agent status + SubagentStatusIcon - tool/AgentReportTool.tsx: Collapsible markdown report - tool/ChatSummarizedTool.tsx: Summary viewer (now uses ToolCollapsible) - tool/Tool.tsx: Main dispatcher (memo-wrapped) - tool/index.ts: Barrel re-exporting Tool, ToolIcon, ToolLabel Existing imports from './tool' resolve to './tool/index.ts' automatically. No behavioral changes.
…SS vars
Three CSS regressions fixed:
1. Restore `html { scrollbar-gutter: stable; }` that was accidentally
removed. Without it, the MUI workaround (padding-right override)
has nothing to work against, causing layout shift when modals open.
2. Replace Tailwind v4 `@source` directive with entries in the
tailwind.config.js `content` array. The project uses Tailwind
v3.4.18 which does not support `@source`; streamdown's Tailwind
classes would not be included in the build.
3. Improve comments on the generic CSS custom properties
(`--background`, `--foreground`, `--muted`, `--primary`, etc.)
to clearly document they are shadcn-compatible aliases consumed
by both streamdown and internal components (e.g. shimmer), and
should not be renamed without updating every consumer.
The delete button in ModelsSection.tsx fired onDeleteModel immediately with no confirmation. Add a DeleteDialog that requires the user to type the model name before deletion proceeds. - Track the model pending deletion via modelToDelete state - Open DeleteDialog on trash button click instead of deleting directly - Close the dialog on cancel or after delete completes (via .finally)
Two issues fixed:
1. Error handling: onCreateModel/onUpdateModel rejections were silently
swallowed because handleSubmit was called via `void handleSubmit(event)`.
Wrap mutations in try/catch and show a displayError toast on failure.
On failure the form stays open, preserving user input.
2. Double navigation: On success, ModelForm called onCancel() to navigate
back to list view, but the parent ModelsSection already wraps
onCreateModel/onUpdateModel to call setView({ mode: 'list' }) after
the promise resolves. Remove the redundant onCancel() call from
handleSubmit — let the parent control navigation.
Wrap onCreateProvider/onUpdateProvider calls in try/catch. On failure,
show a displayError toast and preserve form state (notably the API key
field). Previously, setApiKey('') ran unconditionally after the mutation,
clearing the user's input even on error.
Every collapsible toggle with role="button" + tabIndex={0} now includes
aria-expanded so screen readers announce expand/collapse state.
Changes:
- ToolCollapsible.tsx: add aria-expanded, conditionally omit role/tabIndex/
handlers when hasContent is false
- SubagentTool.tsx: add aria-expanded={expanded}
- AgentReportTool.tsx: add aria-expanded={expanded}
- ExecuteTool.tsx: add aria-expanded={expanded}
- CreateWorkspaceTool.tsx: add aria-expanded={expanded}
- ConversationTimeline.tsx (ReasoningDisclosure): conditionally omit
role/tabIndex/aria-expanded/aria-controls/handlers when !hasText
Extract ConfigureAgentsDialog to its own file, removing ~210 lines from AgentsEmptyState (429 → 224 lines). The dialog now manages its own tab navigation state (configureSectionOptions, activeConfigureSection) and icon imports internally. Key changes: - Remove dead canUseLocalWorkspaceMode code paths (hardcoded false): eliminated selectedWorkspaceMode state, selectedWorkspaceModeRef, localWorkspaceValue, the local-workspace Select option, and workspace_mode from CreateChatOptions. - Replace 3 state-sync useEffects with derived state: 1. selectedModel validated against modelOptions every render 2. activeConfigureSection validated inside ConfigureAgentsDialog 3. canUseLocalWorkspaceMode guard removed (dead code) - Extract ConfigureAgentsDialog to ConfigureAgentsDialog.tsx (213 lines) with a clean props interface for system prompt management. - Clean up 10+ imports from AgentsPage.tsx (Dialog components, ScrollArea, TextareaAutosize, icon components, ChatModelAdminPanel). Net result: 3 useEffects eliminated, 2 useStates removed, 2 useRefs removed, 1 prop removed, AgentsEmptyState well under 250 lines.
Convert hardcoded dark theme constants (DIFF_VIEWER_OPTIONS, FILE_VIEWER_OPTIONS, FILE_VIEWER_OPTIONS_NO_HEADER, FILE_VIEWER_OPTIONS_MINIMAL) to functions that accept an isDark boolean parameter. Each consumer component now uses useTheme() from @emotion/react to derive isDark and passes it to the option factory functions. Light mode uses the "github-light" theme while dark mode continues to use "github-dark-high-contrast".
- Add ChatTreeContext with createContext/useContext to share ambient props (chatTree, chatById, visibleChatIDs, expandedById, modelOptions, chatErrorReasons, isArchiving, archivingChatId, toggleExpanded, onArchiveAgent, normalizedSearch) across the tree. - Simplify ChatTreeNodeProps from 13 props down to 2 (chat, isChildNode). - Memoize context value with useMemo for stable identity. - Remove redundant ChatWithExtendedMetadata type alias (Chat already has diff_status, parent_chat_id, root_chat_id fields). - Remove unnecessary cast in getModelDisplayName (model_config is already Record<string, string>).
…nd streamState Extract shared functions into new blockUtils.ts: - mergeThinkingTitles: was duplicated verbatim in both files - appendTextBlock: consolidates appendParsedTextBlock and appendStreamTextBlock using a joinText parameter to preserve the different text concatenation strategies (newline-joining for complete blocks, direct concatenation for streaming) - asNonEmptyString: was duplicated in messageParsing.ts and chatHelpers.ts Also adds a module-level doc comment to streamingJson.ts describing its purpose and known limitations.
…ard visibility for model admin buttons Item 19: Link form error messages to their inputs via aria-describedby and mark errored inputs with aria-invalid in ModelForm.tsx (contextLimit, compressionThreshold) and ModelConfigFields.tsx (InputField, SelectField, JSONField generic renderers). Item 31: Add group-focus-within:opacity-100 to the edit/delete button container in ModelsSection.tsx so buttons are visible when keyboard focus moves to them.
- Wrap shimmer animation with MotionConfig reducedMotion="user" so motion/react respects prefers-reduced-motion media query. - Add motion-reduce:animate-none to all animate-spin LoaderIcon instances in ai-elements/tool/ components (9 instances across 8 files). - Replace template literal className with cn() in ToolIcon.tsx and ConversationTimeline.tsx for consistency with codebase conventions. - Replace hardcoded hsl(240_5%_26%) scrollbar color with hsl(var(--surface-quaternary)) CSS variable for theme awareness.
Move all WebSocket/SSE streaming state management from AgentDetail.tsx into a dedicated useChatStream hook: - 6 useState calls (messagesById, streamState, streamError, queuedMessages, chatStatus, subagentStatusOverrides) - streamResetFrameRef and related scheduling callbacks - updateSidebarChat callback (only used by SSE handler) - 3 sync effects (chatMessages, chatRecord, chatQueuedMessages) - SSE event handler useEffect (172 lines) AgentDetail.tsx reduced from 693 to 434 lines. The hook exposes a clean interface via UseChatStreamOptions/UseChatStreamResult, keeping the component focused on queries, mutations, and rendering.
Tool.tsx: - Extract each tool branch into a dedicated renderer function (ExecuteRenderer, SubagentRenderer, GenericToolRenderer, etc.) - Build a lookup map (toolRenderers) to dispatch by tool name - Simplify the outer Tool component to a single wrapper div + map lookup, eliminating 10x duplicated wrapper divs - Defer expensive computations (useTheme, parseArgs, formatResultOutput, getFileContentForViewer, etc.) to only the renderers that need them ConversationTimeline.tsx: - Extract renderBlockList() — a shared plain function that encapsulates the response/thinking/tool rendering switch used by both ChatMessageItem (historical) and StreamingOutput (live stream) - Use a plain function (not a component) so renderedToolIDs mutation occurs during the caller's render body, preserving correct evaluation order with sibling JSX that reads the Set
Extract readOptionalString and normalizeProvider into a shared helpers.ts module. Remove duplicate definitions from: - ChatModelAdminPanel.tsx (readOptionalString + normalizeProvider) - ProviderForm.tsx (readOptionalString) - ModelConfigFields.tsx (inline .trim().toLowerCase()) - modelConfigFormLogic.ts (inline .trim().toLowerCase()) - ProviderIcon.tsx (inline .trim().toLowerCase())
Define DiffPanelState and WorkspaceActions interfaces to group 8 individual props into 2 logical objects. Update the component body and the caller in AgentDetail.tsx accordingly.
…ng in AgentsSidebar Add a getTimeGroup helper that classifies chat timestamps into Today, Yesterday, This Week, and Older buckets. The sidebar now dynamically groups visible root chats by their updated_at field and only renders section headers for non-empty groups. The group header styling matches the previous 'This Week' header.
… UI fixes Apply two sets of conflicting patches to the restructured AgentDetail codebase: Conflict 1 — scattered fixes: - Create ErrorBoundary component at components/ErrorBoundary/ErrorBoundary.tsx - Wrap conversation content (ConversationTimeline + QueuedMessagesList) in ErrorBoundary with fallback message in AgentDetail.tsx - Replace raw <button> with <Button> in ExecuteTool.tsx - Replace raw <button> with <Button> + add aria-label in QueuedMessagesList.tsx - Remove unused hasQueuedMessages prop from AgentChatInput interface + destructuring and from AgentDetail.tsx caller - Add aria-label='Chat message' to textarea in AgentChatInput.tsx - Remove no-op 'Mark as unread' DropdownMenuItem from AgentsSidebar.tsx - Remove unused user prop from AgentsNavItem in NavbarView.tsx - Add ScrollBar.displayName in ScrollArea.tsx Conflict 2 — query keys + SSE type guards: - Make query keys hierarchical: chatKey now uses ['chats', chatId] instead of ['chat', chatId] so invalidating chatsKey cascades to children - Simplify redundant chatKey+chatsKey invalidation pairs in chats.ts and useChatStream.ts to single chatsKey invalidations - Add isChatStreamEvent() type guard replacing unsafe 'as' cast in useChatStream.ts SSE handler - Add isValidChatStatus() type guard replacing unsafe 'as ChatStatus' cast - Add SSE type guard in AgentsPage.tsx for chat list events - Update test query key matchers in AgentDetail.test.tsx to match hierarchical keys and simplified invalidation expectations
Replace ChatModelAdminPanel.test.tsx (vitest + MSW) with ChatModelAdminPanel.stories.tsx (Storybook + spyOn API mocks). 8 stories covering all original test cases: - ProviderAccordionCards: renders accordion cards from backend data - EnvPresetProviders: env presets block API key editing - CreateAndUpdateProvider: create + update provider with base URL - ModelsListAndCreate: model listing, creation, deletion, disabled providers - ProviderSpecificModelConfigSchema: schema switches per provider - NoModelConfigByDefault: model_config omitted when not filled - SubmitModelConfigExplicitly: model_config included when explicitly set - ValidatesModelConfigFields: validation blocks invalid input Uses spyOn(API, ...) pattern matching repo conventions (see TasksSidebar.stories.tsx). Mutable state object in setupChatSpies lets mutations update subsequent query responses.
…functions Migrate all 13 test blocks (17 individual scenarios when expanding it.each) from tool.test.tsx into tool.stories.tsx as play functions. Changes to existing stories (5): - ExecuteAuthRequired: play verifies auth button, link href, window.open - WaitForExternalAuthRunning: play verifies waiting text - WaitForExternalAuthAuthenticated: play verifies authenticated text - SubagentRunning: play verifies View agent link href - SubagentReport: play verifies title text renders New stories with play functions (13): - SubagentAwaitLinkCard, SubagentMessageLinkCard: link card variants - SubagentCompletedDelegatedPending: completed tool, pending delegate - SubagentStreamOverrideStatus: stream override rendering - SubagentNoErrorWhenCompleted: no error icon despite parser noise - SubagentAwaitPreferredTitle: result title takes precedence - SubagentRequestMetadata, SubagentAwaitRequestMetadata, SubagentMessageRequestMetadata: duration metadata for each variant - SubagentReportSimple: simple report content rendering - ChatSummarized: collapsed state + expand interaction - SubagentTerminate: terminate label rendering - TaskNameGenericRendering: generic fallback, no subagent link Also adds reactRouter parameters at meta level for Link resolution and deletes the now-redundant tool.test.tsx.
Replace the 1074-line vitest file with a Storybook story file containing 7 stories that cover the same visual behaviors: - Loading: skeleton placeholder when queries are pending - CompletedWithDiffPanel: full layout with actions menu and diff panel - NoDiffUrl: right panel stays closed without a diff URL - WithSubagentCards: subagent tool-call/result messages render cards - WithReasoningCollapsed: reasoning toggle expands/collapses - StreamedSubagentTitle: WebSocket-streamed subagent title - StreamedReasoningCollapsed: WebSocket-streamed reasoning toggle Three tests are intentionally not converted because they assert internal hook behavior (invalidateQueries calls, message preservation across re-renders, sequential intermediate streaming states) that cannot be meaningfully tested through Storybook's visual/interaction model.
Replace FilesChangedPanel.test.tsx with FilesChangedPanel.stories.tsx. The two test cases are preserved as story variants: - EmptyDiff: empty state when there is no diff content - ParseError: empty state fallback when diff parsing fails Both use spyOn(API, ...) to mock getChatDiffStatus and getChatDiffContents rather than vi.mock of useQuery.
Add stories covering: - SystemPromptOnly: dialog with only the system prompt section - ModelConfigOnly: dialog with only model config management (providers/models), with pre-seeded react-query data - BothEnabled: dialog with both sections available - EditAndSaveSystemPrompt: play function that types into the textarea and clicks Save, asserting callbacks fire
Add comprehensive test coverage for the largest untested utility file in the ai-elements component directory (86 tests): - toProviderLabel: fallback chain (displayName → ID → type → default) - shortDurationMs: undefined, negative, seconds/minutes/hours formatting - normalizeStatus: trim + lowercase - isSubagentSuccessStatus: completed/reported recognition, case-insensitivity - isSubagentRunningStatus: pending/running/awaiting recognition - mapSubagentStatusToToolStatus: all status mappings + fallback logic - parseArgs: null/undefined, JSON strings, invalid JSON, objects, arrays - formatResultOutput: null/undefined, strings, output/content extraction - getDiffViewerOptions / getFileViewerOptions: dark/light theme configs - getFileViewerOptionsNoHeader / getFileViewerOptionsMinimal: extended opts - getFileContentForViewer: execute + read_file tool paths, edge cases - buildWriteFileDiff: new-file diff generation, empty/trailing-newline - getWriteFileDiff: tool name gating, arg parsing, valid diffs - parseEditFilesArgs: invalid entry filtering, JSON string parsing - buildEditDiff: single/multi edits, leading-slash strip, empty search - Constants: COLLAPSED_OUTPUT_HEIGHT, COLLAPSED_REPORT_HEIGHT, CSS strings
Add vitest unit tests for two AgentDetail utility modules: - chatHelpers.test.ts (38 tests): covers extractContextUsageFromMessage, getLatestContextUsage, getParentChatID, resolveModelFromChatConfig, and getWorkspaceAgent with edge cases for undefined inputs, negative tokens, non-numeric values, and multi-resource workspaces. - blockUtils.test.ts (20 tests): covers asNonEmptyString, mergeThinkingTitles, and appendTextBlock with edge cases for whitespace handling, block merging, title compatibility, custom joinText functions, and immutability.
Add comprehensive stories covering: - Default state (single provider, no selection) - Selected value display - Custom placeholder text - Disabled state - Multiple providers with grouped headings - Custom provider label formatting - Empty options state - Play function testing model selection interaction
Add comprehensive unit tests for all exported functions in modelConfigFormLogic.ts: - parsePositiveInteger: empty, whitespace, valid, zero, negative, non-numeric, Infinity, float truncation - parseThresholdInteger: empty, whitespace, boundary values (0, 100), in-range, out-of-range, non-numeric - extractModelConfigFormState: empty config, top-level fields, all provider options (OpenAI, Anthropic, Google, OpenAI-compat, OpenRouter, Vercel), missing provider_options - buildModelConfigFromForm: empty form, top-level numeric fields, all provider-specific options for OpenAI/Azure, Anthropic/Bedrock, Google, OpenAI-compat, OpenRouter, Vercel; validation errors for invalid numbers/booleans/selects/JSON; provider normalization (case, whitespace, null, undefined, unknown); multiple errors; whitespace trimming 82 tests covering happy paths, edge cases, and error conditions.
Add stories covering: - Empty list (renders nothing) - Single message - Several messages queued at once - Mixed content types (text field, non-text object, empty object) - Long queue (10 items) - Long message text (truncation)
Replace hardcoded query key arrays with exported helpers in AgentDetail.stories.tsx and ConfigureAgentsDialog.stories.tsx: - ["chats", id] → chatKey(id) - ["chats"] → chatsKey - ["chats", id, "diff-status"] → chatDiffStatusKey(id) - ["chat-models"] → chatModelsKey - ["chat-provider-configs"] → chatProviderConfigsKey - ["chat-model-configs"] → chatModelConfigsKey Add workspaceByIdKey/workspaceById helpers to api/queries/workspaces.ts for fetching a workspace by ID, replacing the ad-hoc inline key in AgentDetail.tsx. The story now uses workspaceByIdKey(id) as well.
Remove two test cases that cast invalid types with `as any` to test impossible runtime scenarios — the functions are typed and will never receive a string where an object or number is expected. Remove unused `body` variable in AgentsPage.stories.tsx.
ChatModelAdminPanel:ModelsListAndCreate — The 'Add model' form replaces the model list entirely (early return in ModelsSection). Move model-list assertions before the 'Add model' click. ConfigureAgentsDialog:EditAndSaveSystemPrompt — Dialog content portals to document.body, so within(canvasElement) can't find it. Use screen queries instead. AgentsEmptyState:UsesSavedBehaviorPromptOnSend — After clicking the dialog Close button, the exit animation keeps the dialog overlay present momentarily, blocking interaction with the page. Wait for the dialog to fully unmount before typing in the chat input.
Reset index.css to match main exactly. Our branch had drifted: it was missing the baseline CSS rules moved from MUI (html/body height, button/input font, autofill, placeholder, fieldset) and the shadcn-compatible CSS variable aliases added in #22238.
This reverts commit fb04602.
Replace React.forwardRef with plain React.FC using ComponentPropsWithRef, which supports ref via props spread. Remove the now-unnecessary displayName assignment and the explicit React import.
- modelConfigFormLogic: remove dead fieldErrors param from buildReasoningProviderOptions (closures already capture it from outer scope). - ToolCollapsible: split into interactive/non-interactive branches so aria-expanded is only present with role="button". - ConversationTimeline: same split for conditional role/aria-expanded. - AgentChatInput: add role="button" to context-usage span so aria-label and tabIndex are valid. - ModelForm: remove useEffect that reset form on provider change; key <ModelForm> on provider in ModelsSection instead so React remounts cleanly. - AgentsPage: add missing useMemo dependencies.
DanielleMaywood
force-pushed
the
agents-review-fixes
branch
from
493463c to
fea9182
Compare
- Remove unused exports (chatGitChanges, deleteChatProviderConfig, etc.) - Remove dead code (MessageAvatar, MessageHeader components) - Remove unused @streamdown/code dependency - Fix merge conflict leftover (localWorkspaceMode reference) - Clean up stray semicolons from knip auto-removal - Un-export ChatGitChangeResponse (only used internally)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.