feat(ui,api): Add filesystem chat tools and UI components #123
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughOverhauls chat orchestration to a unified agent with SSE streaming, introduces client-executed tools over Socket.IO (with server gateway/service), adds visual analysis API, updates model/provider cost tokens, implements multiple LangChain middlewares, and expands UI: new tool UIs, planning/reasoning displays, image previews, usage metrics, KCL LSP resilience, and geometry RGBA/exports. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Client UI
participant Chat as Chat Agent (Server)
participant WS as ChatToolsGateway (Socket.IO)
participant CTS as ChatToolsService
rect rgba(0, 128, 255, 0.5)
User->>Chat: Send messages (SSE stream)
Chat-->>User: Stream parts (incl. tool-call)
Chat->>WS: emit tool_call_request(chatId, toolName, args)
WS->>CTS: register pending request
WS-->>User: tool request via socket
User->>User: Execute client tool (handlers)
User-->>WS: tool_call_result(result/error)
WS->>CTS: resolve pending, validate schemas
CTS-->>Chat: return tool output or ToolError
Chat-->>User: continue stream with tool-output
end
sequenceDiagram
autonumber
actor UI as Client (Screenshots)
participant Tool as testModelTool (Server)
participant An as AnalysisService
participant LLM as Model Provider
rect rgba(0, 200, 120, 0.5)
Tool->>UI: capture_observations request
UI-->>Tool: observations (6 views)
Tool->>An: runVisualTests(observations, requirements)
An->>LLM: single multi-view prompt + images
LLM-->>An: JSON results (pass/fail with reasons)
An-->>Tool: TestModelOutput
Tool-->>UI: tool output (trimmed via middleware)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
API Preview 🐙Status: ✅ Deployed |
UI Preview 🎈Status: ❌ Deployment Failed |
✅ Deploy Preview for taucad ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@claude review |
This comment was marked as outdated.
This comment was marked as outdated.
|
@cursor review |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected -t lint test build typecheck |
❌ Failed | 7m 32s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-01-21 13:10:52 UTC
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Fix all issues with AI Agents 🤖
In @apps/api/app/api/chat/chat.controller.spec.ts:
- Around line 11-12: Replace the type-only import of streamText with a direct
import of the StreamTextResult type from the ai package and use StreamTextResult
wherever ReturnType<typeof streamText> or the local alias is used (remove the
alias declaration). Update function signatures that reference the alias (e.g.,
the chat controller test helpers and the function using the alias) to use
StreamTextResult directly so the types come from the ai package's exported
StreamTextResult type.
In @apps/api/app/api/chat/chat.controller.ts:
- Around line 161-189: The current resume path uses an ordered array
(resumeValues) derived from tryExtractAllToolResults which can misalign results
for parallel tool calls; change it to construct a mapping from toolCallId →
result by iterating the array returned by tryExtractAllToolResults (preserving
each toolResult.toolCallId and toolResult.result) and pass new Command({ resume:
resumeMap }) to graph.streamEvents instead of the ordered list; update the block
that builds resumeValues and the graph.streamEvents call to use resumeMap and
keep the existing logging and abortController usage.
In @apps/api/app/api/tools/tools/tool-create-file.ts:
- Around line 1-28: The create-file tool currently allows unvalidated targetFile
values; update the createFileInputSchema to reject directory-traversal and
absolute paths by adding a Zod refinement on the targetFile field that (1)
disallows segments like "..", (2) rejects absolute paths (e.g., leading "/" or
Windows drive letters), and (3) rejects suspicious characters (e.g., null
bytes); implement the check by normalizing the path (use path.posix.normalize or
equivalent) and ensuring the normalized path does not start with ".." or "/" and
does not contain ".." segments, then regenerate createFileJsonSchema from the
updated createFileInputSchema so createFileToolDefinition and createFileTool
enforce the new validation.
In @apps/api/app/api/tools/tools/tool-glob-search.ts:
- Around line 26-29: The exported const globSearchTool is missing an explicit
return type; update its declaration to include a concrete return type (for
example annotate globSearchTool with an explicit type like ReturnType<typeof
tool> or the appropriate Tool type your codebase uses) so the exported value of
tool((args) => { const result = interrupt<unknown, GlobSearchOutput>(args);
return result; }, globSearchToolDefinition) has an explicit return annotation
rather than relying on inference.
In @apps/api/app/api/tools/tools/tool-reasoning.ts:
- Around line 30-33: The exported const reasoningTool is missing an explicit
return type; add a type annotation to the exported identifier (reasoningTool)
that matches the type returned by tool(...) — for example use the library's Tool
type (or ReturnType<typeof tool>) parameterized with ReasoningOutput so the
signature becomes explicit and still accepts the interrupt call and
reasoningToolDefinition.
In @apps/ui/app/components/chat/chat-context-actions.tsx:
- Line 76: The inline comment next to the quality setting for screenshots is
misleading: it says "Slightly higher quality for single screenshots" while the
value is quality: 0.3 (which is low); update either the comment or the value
used by the quality property in chat-context-actions.tsx (the quality: 0.3
assignment) so they match — either raise the numeric value to reflect "slightly
higher quality" (e.g., ~0.8) or change the comment to accurately describe that
this is a low/compressed quality setting.
In @apps/ui/app/components/ui/button.tsx:
- Line 8: The mobile active-state scale class in the button component is
inverted; replace the current max-md:active:scale-120 token in the class string
inside the Button component (apps/ui/app/components/ui/button.tsx) with a
standard pressed-down value such as max-md:active:scale-98 (or remove the
scaling altogether by using max-md:active:scale-100) to prevent the button from
growing on press and avoid layout shifts.
In @apps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsx:
- Line 4: The import of toolName is incorrect: it's a runtime constant but is
imported with "import type"; change the import to a value import (replace
"import type { toolName }" with a plain "import { toolName }" from
'@taucad/chat/constants') so toolName is available at runtime where used (e.g.,
in the module that references toolName).
In @apps/ui/app/routes/builds_.$id/chat-message-tool-delete-file.tsx:
- Around line 9-13: The component ChatMessageToolDeleteFile currently declares
its return type as ReactNode which is too broad; change its explicit return type
to React.JSX.Element (i.e., export function ChatMessageToolDeleteFile({ part }:
{ readonly part: UIToolInvocation<MyTools[typeof toolName.deleteFile]>; }):
React.JSX.Element) so the function signature reflects that it always returns JSX
and satisfies the coding guideline requiring explicit return types for exported
components.
- Line 5: The import uses "import type { toolName }" but toolName is a runtime
constant, not a type; change the import to a value import ("import { toolName }
from '...';") wherever it's used (e.g., in this module's top-level import) so
the constant is available at runtime and remove the incorrect type-only
modifier.
In @apps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsx:
- Line 5: The import incorrectly uses a type-only import for the runtime
constant toolName; change the import from "import type { toolName } from
'@taucad/chat/constants';" to a value import "import { toolName } from
'@taucad/chat/constants';" so the constant is available at runtime wherever
toolName is referenced in this module (e.g., in
chat-message-tool-get-kernel-result handlers).
In @apps/ui/app/routes/builds_.$id/chat-message-tool-read-file.tsx:
- Around line 72-76: In the 'output-available' case block remove the redundant
redeclarations that shadow the outer-scope variables: do not re-declare input,
targetFile, lineRange, or startLine; instead reference the already-declared
outer variables (input, targetFile, lineRange from formatLineRange, and
startLine) so the case uses the existing values and avoids shadowing.
In @apps/ui/app/utils/filesystem-snapshot.utils.ts:
- Around line 5-10: The local FileEntry type in filesystem-snapshot.utils.ts is
missing the isLoaded property and duplicates the canonical type; replace the
local definition by importing the FileEntry type from
apps/ui/app/hooks/use-file-manager.tsx (where the canonical FileEntry is
defined) and update any uses such as generateFilesystemSnapshotFromMap to rely
on the imported FileEntry so the shape (including isLoaded) stays consistent
across modules.
In @libs/chat/src/schemas/tools/delete-file.tool.schema.ts:
- Around line 3-5: The deleteFileInputSchema currently allows arbitrary
targetFile strings; add validation to prevent path traversal by refining
targetFile: either reject any segment containing '..' (e.g., fail if targetFile
includes '/../' or equals '..') or implement a custom validation that resolves
the path with Node's path.resolve against the project root and asserts the
resolved path starts with the project root; update the schema
(deleteFileInputSchema) to use z.string().refine(...) with this logic and ensure
any code that uses joinPath/normalizePath also performs the same resolved-path
containment check.
In @package.json:
- Line 264: The project depends on "vitest-mock-extended" which pulls in Vitest;
ensure Vitest is pinned to a patched release to mitigate CVE-2025-24964 by
adding/adjusting your dependency pin or package manager override so Vitest
resolves to >=1.6.1, >=2.1.9, or >=3.0.5 (for example update package.json or use
npm/yarn/pnpm "overrides" or "resolutions" to force vitest@^3.0.5), or upgrade
vitest-mock-extended to a version that itself depends on a patched Vitest;
verify with lockfile changes and run install + audit to confirm the patched
Vitest is used.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ui/app/machines/cad.machine.ts (1)
13-33: Adderrorfield to context per XState patterns.The context is missing an
error: Error | undefinedfield for tracking general machine-level errors. WhilekernelErrorsandcodeErrorshandle specific error types, a general error field is needed for unexpected failures.Based on learnings, XState machines should include this field for proper error handling.
🔎 Proposed fix
export type CadContext = { file: GeometryFile | undefined; screenshot: string | undefined; parameters: Record<string, unknown>; units: { length: LengthSymbol; }; defaultParameters: Record<string, unknown>; geometries: Geometry[]; kernelErrors: Map<string, KernelError[]>; codeErrors: CodeError[]; + error: Error | undefined; kernelRef: ActorRefFrom<typeof kernelMachine>; exportedBlob: Blob | undefined; shouldInitializeKernelOnStart: boolean; isKernelInitializing: boolean; isKernelInitialized: boolean; graphicsRef?: ActorRefFrom<typeof graphicsMachine>; logActorRef?: ActorRefFrom<typeof logMachine>; fileManagerRef?: ActorRefFrom<typeof fileManagerMachine>; jsonSchema?: JSONSchema7; };Then initialize it in the context factory (line 294):
context: ({ input, spawn }) => ({ file: undefined, screenshot: undefined, + error: undefined, units: {
🧹 Nitpick comments (29)
apps/api/app/api/models/model.constants.ts (1)
39-39: Consider using numeric separator for consistency.Other numeric literals in this file use underscore separators (e.g.,
32_000,200_000). For consistency, consider using5_000here.🔎 Suggested fix
thinking: { type: 'enabled', // eslint-disable-next-line @typescript-eslint/naming-convention -- some models use snake_case - budget_tokens: 5000, + budget_tokens: 5_000, },apps/ui/app/routes/import.$/repository-card.tsx (1)
93-101: Well-implemented external link with good security practices!The GitHub link implementation correctly uses
rel="noopener noreferrer"to prevent security vulnerabilities withtarget="_blank", and the icon animation provides a nice visual feedback on hover.Optional: Enhance accessibility with aria-label
Consider adding an
aria-labelto explicitly indicate the link opens GitHub in a new tab:<a href={`https://github.com/${owner}/${repo}`} target="_blank" rel="noopener noreferrer" + aria-label={`View ${owner}/${repo} on GitHub (opens in new tab)`} className="group/link inline-flex items-center gap-1 font-mono text-lg font-semibold underline decoration-muted-foreground/50 underline-offset-2 transition-colors hover:decoration-foreground" >This improves the experience for screen reader users by explicitly stating the link destination and behavior.
apps/ui/app/machines/cad.machine.ts (1)
126-131: Optimize Map creation to avoid unnecessary allocation.The current implementation always creates a new
Mapand attempts deletion, even when the key doesn't exist. Consider following the pattern used insetGeometries(lines 147-156) for consistency and efficiency.🔎 Proposed optimization
kernelErrors({ context, event }) { assertEvent(event, 'setFile'); + const filename = event.file.filename; + if (!filename || !context.kernelErrors.has(filename)) { + return context.kernelErrors; + } const newErrorsMap = new Map(context.kernelErrors); - newErrorsMap.delete(event.file.filename); + newErrorsMap.delete(filename); return newErrorsMap; },apps/ui/app/components/geometry/kernel/openscad/openscad.worker.test.ts (1)
73-86: Consider typing theencodingparameter more specifically.The
encodingparameter is typed asstring | undefined, but based on usage, it should only accept'utf8'orundefined. This could be made more explicit.🔎 Suggested improvement
- readFile: vi.fn(async (filepath: string, encoding?: string) => { + readFile: vi.fn(async (filepath: string, encoding?: 'utf8') => {apps/ui/app/components/geometry/kernel/openscad/openscad.worker.ts (2)
117-127: Consider extracting magic strings to constants.The group names
'Global'and'Parameters'appear here and are also referenced inprocessOpenScadParameters(from the relevant code snippet). Consider extracting these to shared constants for consistency and maintainability.🔎 Suggested approach
// In a shared constants file or at the top of parse-parameters.ts const DEFAULT_GROUP_NAMES = ['Global', 'Parameters', ''] as const; // Then use: const needsFileGroup = !isMainFile && (!parameter.group || DEFAULT_GROUP_NAMES.includes(parameter.group));
318-329: Consider using filter for cleaner code.The current implementation works correctly, but could be more concise using functional style.
🔎 Alternative implementation
private async getAllScadFiles(): Promise<string[]> { const files = await this.fileManager.getDirectoryContents(this.basePath); - const scadFiles: string[] = []; - - for (const [relativePath] of Object.entries(files)) { - if (relativePath.endsWith('.scad')) { - scadFiles.push(relativePath); - } - } - - return scadFiles; + return Object.keys(files).filter((path) => path.endsWith('.scad')); }apps/ui/app/lib/github-api.ts (1)
73-89: Add explicit return type for consistency.While not exported, adding an explicit
: booleanreturn type would improve consistency with the coding guidelines that prefer explicit types for functions.🔎 Suggested refactor
-function isUnauthorizedError(error: unknown): boolean { +function isUnauthorizedError(error: unknown): boolean {Note: TypeScript already infers the return type correctly, but explicit annotation improves readability.
apps/ui/app/routes/builds_.$id/chat-message-tool-transfer.tsx (2)
94-94: Consider adding runtime validation for type safety.The type assertion
destination as AgentTypeassumes the extracted destination string matches one of the known agent types, but there's no runtime check. While the helper functions handle unexpected values with default cases, adding explicit validation would make the code more robust and self-documenting.🔎 Proposed refactor with runtime validation
- const agentType = destination as AgentType; + const knownAgents = ['cad_expert', 'research_expert', 'supervisor'] as const; + const agentType: AgentType = knownAgents.includes(destination as typeof knownAgents[number]) + ? (destination as AgentType) + : 'unknown';Alternatively, create a helper function:
function toAgentType(destination: string): AgentType { const knownAgents: AgentType[] = ['cad_expert', 'research_expert', 'supervisor']; return knownAgents.includes(destination as AgentType) ? (destination as AgentType) : 'unknown'; }
111-119: Consider extracting icon selection to a helper function.The nested ternary operator for icon selection is readable but could be extracted into a helper function for better maintainability and testability, especially if this logic needs to evolve.
🔎 Proposed refactor
+function getDirectionIcon(isStreaming: boolean, isComplete: boolean, isTransferBack: boolean): React.JSX.Element { + if (isStreaming) { + return <LoaderCircle className="size-3 animate-spin" />; + } + if (isComplete) { + return <Check className="size-3 text-success" />; + } + if (isTransferBack) { + return <CornerDownLeft className="size-3" />; + } + return <ArrowRight className="size-3" />; +} + export function ChatMessageToolTransfer({ part }: { readonly part: ToolUIPart }): React.JSX.Element { // ... existing code ... return ( <div ...> <div className={cn('flex items-center gap-1.5', isStreaming ? 'text-primary' : 'text-muted-foreground')}> - {isStreaming ? ( - <LoaderCircle className="size-3 animate-spin" /> - ) : isComplete ? ( - <Check className="size-3 text-success" /> - ) : isTransferBack ? ( - <CornerDownLeft className="size-3" /> - ) : ( - <ArrowRight className="size-3" /> - )} + {getDirectionIcon(isStreaming, isComplete, isTransferBack)} </div>apps/ui/app/routes/builds_.$id/chat-error-service-unavailable.tsx (1)
26-35: Consider simplifying the onClick handler.The arrow function wrapper is unnecessary since
regeneratematches the expected signature.🔎 Proposed simplification
<Button variant="outline" size="sm" - onClick={() => { - regenerate(); - }} + onClick={regenerate} > <RefreshCcw className="size-3.5" /> Retry </Button>apps/ui/app/lib/kcl-language/lsp/kcl-navigation-service.ts (1)
83-99: Potential timing fragility with hardcoded delay.The
setTimeout(() => {...}, 100)relies on a hardcoded delay for the editor to mount and model to be ready. This could be fragile on slower machines or under load.Consider using a more robust approach such as polling for the model/editor readiness or using Monaco's event system if available.
apps/ui/app/components/geometry/parameters/parameters.tsx (1)
54-71: Consider combining related effects.These two
useEffecthooks both respond toenableSearchchanges. Combining them would reduce re-render cycles and make the relationship between these behaviors clearer.🔎 Proposed refactor to combine effects
- // Focus the search input when search changes from disabled to enabled (not on initial render) - React.useEffect(() => { - const wasDisabled = !previousEnableSearchRef.current; - const isNowEnabled = enableSearch; - - // Only focus if transitioning from disabled to enabled - if (wasDisabled && isNowEnabled && searchInputReference.current) { - searchInputReference.current.focus(); - } - - previousEnableSearchRef.current = enableSearch; - }, [enableSearch]); - - // Clear search term when search is hidden - React.useEffect(() => { - if (!enableSearch) { - setSearchTerm(''); - } - }, [enableSearch]); + // Handle enableSearch transitions: focus on enable, clear on disable + React.useEffect(() => { + const wasDisabled = !previousEnableSearchRef.current; + const isNowEnabled = enableSearch; + + if (wasDisabled && isNowEnabled && searchInputReference.current) { + // Focus when transitioning from disabled to enabled + searchInputReference.current.focus(); + } else if (!enableSearch) { + // Clear search term when search is hidden + setSearchTerm(''); + } + + previousEnableSearchRef.current = enableSearch; + }, [enableSearch]);apps/ui/app/routes/builds_.$id/chat-history-selector.tsx (1)
155-157: Simplify redundant type check.The type guard on line 157 is redundant since
find()on line 156 already filters forpart.type === 'text'. You can simplify this to:-const draftTextPart = chat.draft?.parts.find((part) => part.type === 'text'); -const draftText = draftTextPart?.type === 'text' ? draftTextPart.text : undefined; +const draftText = chat.draft?.parts.find((part) => part.type === 'text')?.text;apps/ui/app/components/geometry/kernel/utils/import-off.test.ts (1)
65-68: Consider consistent array access pattern.The test file inconsistently uses optional chaining for array element access:
- Lines 19-20, 36-37, 51, 82, 100: Direct access without optional chaining
- Lines 65-68, 102-105: Optional chaining
result.colors[0]?.[...]After asserting the array length, the elements should exist, so optional chaining is unnecessary. Consider using consistent direct access throughout for better readability.
🔎 Proposed refactor for consistency
expect(result.colors).toHaveLength(1); // Semi-transparent blue: RGBA(0, 0, 255, 128) -> [0, 0, 1, ~0.5] - expect(result.colors[0]?.[0]).toBeCloseTo(0, 5); - expect(result.colors[0]?.[1]).toBeCloseTo(0, 5); - expect(result.colors[0]?.[2]).toBeCloseTo(1, 5); - expect(result.colors[0]?.[3]).toBeCloseTo(128 / 255, 5); + expect(result.colors[0]![0]).toBeCloseTo(0, 5); + expect(result.colors[0]![1]).toBeCloseTo(0, 5); + expect(result.colors[0]![2]).toBeCloseTo(1, 5); + expect(result.colors[0]![3]).toBeCloseTo(128 / 255, 5);And similarly for lines 102-105:
expect(result.colors).toHaveLength(2); // First face: opaque red expect(result.colors[0]).toEqual([1, 0, 0, 1]); // Second face: semi-transparent blue - expect(result.colors[1]?.[0]).toBeCloseTo(0, 5); - expect(result.colors[1]?.[1]).toBeCloseTo(0, 5); - expect(result.colors[1]?.[2]).toBeCloseTo(1, 5); - expect(result.colors[1]?.[3]).toBeCloseTo(127 / 255, 5); + expect(result.colors[1]![0]).toBeCloseTo(0, 5); + expect(result.colors[1]![1]).toBeCloseTo(0, 5); + expect(result.colors[1]![2]).toBeCloseTo(1, 5); + expect(result.colors[1]![3]).toBeCloseTo(127 / 255, 5);Also applies to: 102-105
apps/api/app/api/chat/utils/extract-tool-result.ts (1)
78-79: Useundefinedinstead ofnullfor consistency.Line 79 converts
undefinedtonullfor JSON compatibility, but the coding guidelines specify usingundefinedinstead ofnullin TypeScript. Since the result is immediately stringified on line 85, the distinction is lost in the JSON serialization anyway.🔎 Proposed fix
- // Ensure we don't push undefined - convert to null for JSON compatibility - const finalResult = unwrappedResult === undefined ? null : unwrappedResult; + // Use the unwrapped result directly; JSON.stringify handles undefined + const finalResult = unwrappedResult;As per coding guidelines, prefer
undefinedovernullin TypeScript.apps/ui/app/routes/builds_.$id/chat-message-tool-reasoning.tsx (2)
44-54: Consider extractingformatDurationas a module-level constant.The
formatDurationfunction is a pure utility that doesn't depend on component state or props. Extracting it outside the component would prevent recreation on every render and improve testability.🔎 Proposed refactor
+/** + * Formats a duration in seconds to a human-readable string + */ +function formatDuration(seconds: number): string { + if (seconds < 1) { + return '<1 second'; + } + + if (seconds === 1) { + return '1 second'; + } + + return `${seconds} seconds`; +} + export function ChatMessageToolReasoning({ part, }: { readonly part: UIToolInvocation<MyTools[typeof toolName.reasoning]>; }): React.JSX.Element { const [isOpen, setIsOpen] = useState(false); // Capture start time when component mounts (tool call begins) const startTimeRef = useRef<number>(Date.now()); const [elapsedSeconds, setElapsedSeconds] = useState(0); // ... rest of component - // Format duration display - const formatDuration = (seconds: number): string => { - if (seconds < 1) { - return '<1 second'; - } - - if (seconds === 1) { - return '1 second'; - } - - return `${seconds} seconds`; - };
61-67: Consider computing the label directly instead of wrapping in a function.The
getLabelfunction is called only once and could be simplified to a computed constant value, reducing unnecessary function call overhead.🔎 Proposed refactor
- // Get the label text based on state - const getLabel = (): string => { - if (isThinking) { - return 'Thinking...'; - } - - return `Thought for ${formatDuration(finalDurationSeconds)}`; - }; + // Get the label text based on state + const label = isThinking ? 'Thinking...' : `Thought for ${formatDuration(finalDurationSeconds)}`; // ... later in JSX - {getLabel()} + {label}apps/ui/app/utils/filesystem-snapshot.utils.ts (2)
36-38: Remove redundant check: filter(Boolean) already removes empty strings.The
filter(Boolean)call on Line 31 already removes empty strings, making this additional check for emptypartredundant.🔎 Proposed fix to remove redundant check
for (let index = 0; index < parts.length; index++) { const part = parts[index]; - if (!part) { - continue; - } const isLast = index === parts.length - 1;
51-54: Simplify: child cannot be undefined after Map.set().The child retrieved with
get(part)immediately afterset(part, ...)will always be defined. The conditional check is unnecessary and adds cognitive load.🔎 Proposed fix to simplify the logic
if (!current.children.has(part)) { current.children.set(part, { name: part, type: isLast ? entry.type : 'dir', size: isLast ? entry.size : 0, children: new Map(), }); } - const child = current.children.get(part); - if (child) { - current = child; - } + current = current.children.get(part)!;apps/ui/app/routes/builds_.$id/chat-message-tool-image-analysis.tsx (1)
179-204: Remove commented-out code or document why it's preserved.There's a large block of commented-out JSX (26 lines) for the Analysis section. Commented-out code should generally be removed to reduce clutter, or if it's intentionally preserved for reference, add a comment explaining why.
If this code is no longer needed, remove it:
- {/* Analysis Section */} - {/* <div className="border-t"> - <div className="mb-1 flex items-center gap-1 px-2 py-1 pt-2 text-xs text-muted-foreground"> - <Eye className="size-3" /> - <span>Analysis</span> - </div> - <div className={cn('relative', isImageExpanded ? '' : 'max-h-96 overflow-y-auto')}> - <div className="prose prose-sm max-w-none px-3 pb-3 text-xs dark:prose-invert"> - <ReactMarkdown>{result.analysis}</ReactMarkdown> - </div> - <Tooltip> - <TooltipTrigger asChild> - <Button - size="xs" - variant="ghost" - className="sticky bottom-0 h-6 w-full rounded-none bg-neutral/10 text-center text-foreground/50 hover:bg-neutral/40" - onClick={() => { - setIsImageExpanded((previous) => !previous); - }} - > - <ChevronDown className={cn('transition-transform', isImageExpanded ? 'rotate-x-180' : '')} /> - </Button> - </TooltipTrigger> - <TooltipContent>{isImageExpanded ? 'Collapse' : 'Expand'} analysis</TooltipContent> - </Tooltip> - </div> - </div> */}If it's intentionally preserved, add a comment explaining the context:
{/* Analysis Section */} + {/* TODO: Re-enable inline markdown rendering once performance issues are resolved + Temporarily disabled due to rendering lag with large analysis outputs */} {/* <div className="border-t">apps/ui/app/routes/builds_.$id/chat-message-tool-glob-search.tsx (1)
65-72: Consider adding a default case for exhaustiveness checking.While TypeScript's type narrowing typically ensures all switch cases are covered, adding a default case that throws or returns a fallback can provide runtime safety.
🔎 Proposed addition for exhaustiveness checking
case 'output-error': { return ( <Badge variant="destructive"> <span>File search failed</span> </Badge> ); } + + default: { + // Exhaustiveness check - TypeScript will error if a case is missing + const _exhaustiveCheck: never = part; + return _exhaustiveCheck; + } } }apps/api/app/api/chat/chat.controller.ts (1)
91-128: Consider simplifying the filesystem snapshot injection logic.The nested conditionals can be flattened using early returns. Also, the check on line 101 is redundant since
findLastIndexreturning a valid index guarantees the element exists.🔎 Proposed simplification
// Extract filesystem snapshot from metadata and inject into last message content const filesystemSnapshot = lastHumanMessage.metadata?.filesystemSnapshot; let messagesWithContext = sanitizedMessages; - if (filesystemSnapshot) { - this.logger.debug('Injecting filesystem snapshot into last message'); - // Find the last user message and prepend the project layout - const lastUserMessageIndex = sanitizedMessages.findLastIndex((message) => message.role === 'user'); - if (lastUserMessageIndex !== -1) { - const lastUserMessage = sanitizedMessages[lastUserMessageIndex]; - if (lastUserMessage) { - const projectLayoutContext = `<project_layout> + if (filesystemSnapshot) { + this.logger.debug('Injecting filesystem snapshot into last message'); + const lastUserMessageIndex = sanitizedMessages.findLastIndex((message) => message.role === 'user'); + + if (lastUserMessageIndex !== -1) { + const lastUserMessage = sanitizedMessages[lastUserMessageIndex]!; + const projectLayoutContext = `<project_layout> Below is a snapshot of the current project's file structure: ${filesystemSnapshot} </project_layout> `; - // Create updated message with project layout prepended to text content - const updatedParts = lastUserMessage.parts.map((part) => { - if (part.type === 'text') { - return { ...part, text: projectLayoutContext + part.text }; - } - - return part; - }); - - messagesWithContext = [ - ...sanitizedMessages.slice(0, lastUserMessageIndex), - { ...lastUserMessage, parts: updatedParts }, - ...sanitizedMessages.slice(lastUserMessageIndex + 1), - ]; - } + const updatedParts = lastUserMessage.parts.map((part) => { + if (part.type === 'text') { + return { ...part, text: projectLayoutContext + part.text }; + } + return part; + }); + + messagesWithContext = [ + ...sanitizedMessages.slice(0, lastUserMessageIndex), + { ...lastUserMessage, parts: updatedParts }, + ...sanitizedMessages.slice(lastUserMessageIndex + 1), + ]; } }apps/ui/app/routes/builds_.$id/chat-editor.tsx (1)
226-260: Consider extracting the event type to improve readability.The inline type annotation on line 234 works but could be extracted for clarity and reuse.
🔎 Proposed type extraction
+type FileOpenedEvent = { + path: string; + lineNumber?: number; + column?: number; +}; + // Subscribe to fileOpened events to jump to specific line numbers useEffect(() => { if (!monaco) { return; } - const subscription = fileExplorerActor.on( - 'fileOpened', - (event: { path: string; lineNumber?: number; column?: number }) => { + const subscription = fileExplorerActor.on('fileOpened', (event: FileOpenedEvent) => {apps/api/app/api/chat/chat.service.ts (1)
55-73: Consider usingtoolNameconstants for tool property access.The tool keys are hardcoded as strings (e.g.,
tools.edit_file). Using thetoolNameconstants would ensure consistency and catch typos at compile time.🔎 Proposed refactor using constants
const allTools = [ // CAD and filesystem tools - tools.edit_file, - tools.analyze_image, - tools.read_file, - tools.list_directory, - tools.create_file, - tools.delete_file, - tools.grep, - tools.glob_search, - tools.get_kernel_result, - tools.reasoning, + tools[toolName.fileEdit], + tools[toolName.imageAnalysis], + tools[toolName.readFile], + tools[toolName.listDirectory], + tools[toolName.createFile], + tools[toolName.deleteFile], + tools[toolName.grep], + tools[toolName.globSearch], + tools[toolName.getKernelResult], + tools[toolName.reasoning], // Research tools - tools.web_search, - tools.web_browser, + tools[toolName.webSearch], + tools[toolName.webBrowser], ].filter((tool) => tool !== undefined);apps/api/app/api/chat/utils/langgraph-adapter.ts (1)
175-185: Remove or usetoolCallsByIdto avoid dead state
toolCallStatenow tracks bothtoolCallsByIdandtoolCallQueue, but only the queue andcurrentToolCallId/currentToolNameare actually read. ThetoolCallsByIdmap is written inhandleChatModelStreambut never consumed.Either:
- Remove
toolCallsByIdentirely, or- Use it to resolve
tool_call_chunks(e.g., by chunkid) instead of relying solely on queue position.This will simplify the adapter and avoid maintaining unused per-call state.
Also applies to: 339-346, 617-624, 695-701, 812-818
apps/ui/app/components/geometry/parameters/parameters.test.tsx (1)
743-905: Expansion and feature-flag tests give good behavioral coverageThe new suites around
isAllExpanded,isInitialExpanded, andenableSearch(including focus transitions) exercise the key UI states and aria attributes in a realistic way. This should catch regressions in group expansion behavior and the controls bar without overcoupling to implementation details.Optional test cleanup (no behavior change)
You might consider extracting small helpers for repeated
defaultParameters/schemasetups (e.g., config/array schemas) to reduce duplication across the expansion-related tests, but it’s not required for clarity.Also applies to: 2682-2861
apps/ui/app/hooks/use-chat-tools.tsx (2)
75-77: Guard against uninitializedfileTreewhen tools run early
handleListDirectory,handleGrep, andhandleGlobSearchall iteratefileTree.entries(). If a tool call fires before the file-manager machine initializes itsfileTree(e.g., initial context isundefinedornull), these handlers will throw before reaching their try/catch blocks.If
fileTreeis guaranteed to be aMapfrom initial state onward, this is fine. Otherwise, consider a small guard:if (!fileTree) { return { entries: [], path: resolvedPath || '/' }; // or equivalent empty result }and similarly for grep/glob to return empty results instead of failing the tool call.
Also applies to: 213-233, 273-350
69-77: Remove redundantfileEditRef.start()and clarifyresolvePathimplementationTwo confirmed issues:
resolvePathis a no-op — Both code branches returntargetFileunchanged (lines 81-89). The comment says "resolve paths relative to build root," but the implementation does nothing. Either remove the function and use paths directly, or implement proper build-root-relative normalization if required by the file manager.
fileEditRef.start()is redundant — In all@xstate/reactversions (v4+),useActorRefautomatically creates and starts the actor for the component's lifetime. The explicit.start()call on line 107 is unnecessary and could cause issues if the state machine expects only one start cycle. Remove the call and rely onsend+waitForto drive the actor.Also applies to: 81-89, 92-137
apps/ui/app/components/chat/chat-tool-file-operation.tsx (1)
13-41: Shared file-operation UI components are well-factoredThe new components (
ErrorSection,StatusIcon,Filename,ApplyButton,CodePreview,DiffView,FileOperationHeader,CollapsibleFileOperation*) form a coherent, reusable surface for file tools with clear props and good accessibility cues (aria labels, line/column display, streaming vs. static states).
computeDiffis a deliberately simple line-by-line diff, which is reasonable for short code previews in chat; if you ever need higher-fidelity diffs, you can swap in a more sophisticated algorithm behind the sameDiffLinecontract without changing callers.Also applies to: 50-188, 190-233, 235-283, 292-335, 337-377, 378-438, 440-507
apps/ui/app/routes/builds_.$id/chat-message-tool-delete-file.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Fix all issues with AI Agents 🤖
In @apps/ui/app/routes/builds_.$id/chat-message-tool-file-edit.tsx:
- Around line 25-26: The file calls useBuild twice (const build = useBuild({
enableNoContext: true }) and const { getMainFilename } = useBuild()), which is
redundant and unsafe; replace both calls with a single call to useBuild that
includes enableNoContext: true and destructure both the build object and
getMainFilename from that single call (e.g., const { getMainFilename, ...build }
= useBuild({ enableNoContext: true }) or const build = useBuild({
enableNoContext: true }); const { getMainFilename } = build) so you only invoke
useBuild once and preserve the optional/no-context behavior.
In @apps/ui/app/utils/currency.utils.ts:
- Around line 17-24: The current formatting builds a currency string then strips
the symbol using numericValue.toLocaleString(...).replace('$', ''), which is
fragile and locale-dependent; update the formatting to use a number/decimal
formatter instead of style: 'currency' (e.g., Intl.NumberFormat or
toLocaleString with style: 'decimal') and keep the existing
minimumFractionDigits and maximumFractionDigits options so the function returns
a properly localized numeric string without manually removing any currency
symbol; change the return in the function that uses
numericValue.toLocaleString(...) to use a decimal/number format with the same
fraction settings.
- Around line 27-33: The current return uses toLocaleString with style:
'currency' then strips the '$' via .replace('$', ''), which is fragile; change
this path to format the number as a decimal instead (e.g., use
toLocaleString('en-US', { style: 'decimal', minimumFractionDigits: 6,
maximumFractionDigits: 6 })) and remove the .replace call so the formatted
string is produced reliably across locales; update the return that begins with
value.toLocaleString(...) accordingly.
- Around line 10-11: The zero-value branch currently returns "0.00 USD" which
mismatches the non-zero branches; change the value === 0 branch to return the
same format as the non-zero paths (i.e., no " USD" suffix) — e.g., return "0.00"
or reuse the same formatting helper used by the other branches so the output for
zero and non-zero values is consistent with lines 24 and 33.
♻️ Duplicate comments (3)
apps/ui/app/utils/filesystem-snapshot.utils.ts (1)
5-10: Import the canonical FileEntry type instead of duplicating it.The local
FileEntrytype is missing theisLoadedproperty from the canonical definition inuse-file-manager.tsx. Import the type to maintain consistency.🔎 Proposed fix
+import type { FileEntry } from '#hooks/use-file-manager.js'; + /** * Utility functions for generating token-efficient filesystem snapshots for LLM consumption. */ -type FileEntry = { - path: string; - name: string; - type: 'file' | 'dir'; - size: number; -}; - type TreeNode = {As per coding guidelines: Use
import typefor type-only imports and absolute imports with#prefix.apps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsx (1)
4-4: Use value import fortoolNameconstant.The
toolNameis a constant object, not a type. It should be imported as a value, not as a type.As per coding guidelines, use
import typeonly for type-only imports.🔎 Proposed fix
-import type { toolName } from '@taucad/chat/constants'; +import { toolName } from '@taucad/chat/constants';apps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsx (1)
5-5: Use value import fortoolNameconstant.This was flagged in a previous review.
toolNameis a runtime constant, not a type, so it should be a value import.🔎 Proposed fix
-import type { toolName } from '@taucad/chat/constants'; +import { toolName } from '@taucad/chat/constants';
🧹 Nitpick comments (17)
apps/ui/app/utils/filesystem-snapshot.utils.ts (1)
34-38: Remove redundant check for empty part.Line 31 already filters out falsy values with
filter(Boolean), so the check on line 36 is unreachable and can be removed.🔎 Proposed fix
- for (let index = 0; index < parts.length; index++) { + for (let index = 0; index < parts.length; index++) { const part = parts[index]; - if (!part) { - continue; - } - const isLast = index === parts.length - 1;apps/api/app/api/chat/prompts/chat-prompt-cad.ts (1)
451-548: LGTM! Helper functions provide clear, mode-specific guidance.Both
getFileOrganizationStrategyandgetMultiFileWorkflowExamplecorrectly branch onfileLayoutModeto provide appropriate documentation. The full-nesting mode encourages lib/ subdirectories, while assembly-only mode emphasizes flat structure with the KCL import constraint clearly documented.Minor: Consider more descriptive variable name
The variable
extcould be renamed tofileExtensionfor improved clarity:- const ext = config.fileExtension; + const fileExtension = config.fileExtension;This is purely a readability enhancement and not required.
Also applies to: 550-575
libs/chat/src/schemas/tools/error.schema.ts (1)
18-24: Consider adding a meta ID for consistency.While
errorLocationSchemaworks correctly without a meta ID (since it's composed into other schemas), adding.meta({ id: 'ErrorLocation' })would maintain consistency withcodeErrorSchemaandkernelErrorSchema.apps/ui/app/utils/currency.utils.ts (1)
8-8: Consider extracting options to a named type.The inline type definition for options could be extracted to a separate
typedefinition for better reusability and consistency with the coding guideline to prefer named types.🔎 Proposed refactor
+type FormatCurrencyOptions = { + significantFigures?: number; +}; + -export const formatCurrency = (value: number, options?: { significantFigures?: number }): string => { +export const formatCurrency = (value: number, options?: FormatCurrencyOptions): string => {As per coding guidelines: "Use
typeinstead ofinterfacefor defining types in TypeScript".apps/ui/app/routes/builds_.$id/chat-message-tool-transfer.tsx (1)
14-14: Unsafe type assertion may cause runtime issues with unknown agent types.The
destinationvalue extracted from the tool name is cast directly toAgentTypewithout validation. If the tool name contains an unexpected agent (e.g.,transfer_to_new_agent), the helper functions will fall through to thedefaultcase, which works, but the type assertion obscures this possibility.Consider using a type guard or validation:
🔎 Proposed fix
-type AgentType = 'cad_expert' | 'research_expert' | 'supervisor' | 'unknown'; +const KNOWN_AGENTS = ['cad_expert', 'research_expert', 'supervisor'] as const; +type KnownAgentType = (typeof KNOWN_AGENTS)[number]; +type AgentType = KnownAgentType | 'unknown'; + +function toAgentType(destination: string): AgentType { + return KNOWN_AGENTS.includes(destination as KnownAgentType) ? (destination as KnownAgentType) : 'unknown'; +}Then use:
- const agentType = destination as AgentType; + const agentType = toAgentType(destination);Also applies to: 94-96
apps/ui/app/components/chat/chat-tool-inline.tsx (1)
104-106: Potential duplicate animation classes.When
isLoadingis true, bothanimate-shiny-textclass is applied to the outer<span>andAnimatedShinyTextcomponent is rendered inside, which also appliesanimate-shiny-text. This might cause unintended visual effects or be redundant.🔎 Proposed fix
return ( - <span className={cn('text-sm text-muted-foreground', isLoading && 'animate-shiny-text', className)}> + <span className={cn('text-sm text-muted-foreground', className)}> {isLoading ? ( <AnimatedShinyText>{children}</AnimatedShinyText> ) : (apps/ui/app/components/chat/chat-tool-file-operation.tsx (3)
91-91: UnnecessaryNumber()conversion.
errors.lengthis already anumber, so wrapping it inNumber()is redundant.🔎 Proposed fix
<span className="text-left text-xs font-normal"> - {Number(errors.length)} {type} error{errors.length > 1 ? 's' : ''} + {errors.length} {type} error{errors.length > 1 ? 's' : ''} </span>
97-101: Key collision possible with identical errors.If multiple errors have the same line number and message, the generated key will be duplicated. Consider including the index as a fallback.
🔎 Proposed fix
- {errors.map((error) => { + {errors.map((error, index) => { const lineNumber = getErrorLineNumber(error); const column = getErrorColumn(error); - const key = `${lineNumber ?? 'unknown'}-${error.message}`; + const key = `${lineNumber ?? 'unknown'}-${column ?? 'unknown'}-${index}`;
531-532: Unused variables in non-streaming path.
filenameandhasPathare declared but only used within the streaming branch (lines 534-566). They're not used in the non-streaming Collapsible path sinceCollapsibleFileOperationTriggerhandles its own filename extraction.🔎 Proposed fix
Move the declarations inside the streaming block:
const [isOpen, setIsOpen] = useState(isDefaultOpen); const isStreaming = ['input-streaming', 'input-available'].includes(toolStatus); - const filename = getFilename(targetFile); - const hasPath = targetFile !== filename; // For streaming, show last 4 lines without collapsible if (isStreaming && content) { + const filename = getFilename(targetFile); + const hasPath = targetFile !== filename; const lines = content.split('\n');apps/ui/app/routes/builds_.$id/chat-message-tool-file-edit.tsx (2)
85-99: Consider extractinggetApplyStateor using direct mapping.The function is recreated on each render. Since it only maps state values, a simple object lookup would be cleaner:
🔎 Proposed fix
- const getApplyState = (): 'idle' | 'applying' | 'success' | 'error' => { - if (fileEditState === 'applying') { - return 'applying'; - } - - if (fileEditState === 'success') { - return 'success'; - } - - if (fileEditState === 'error') { - return 'error'; - } - - return 'idle'; - }; + const applyStateMap: Record<string, 'idle' | 'applying' | 'success' | 'error'> = { + applying: 'applying', + success: 'success', + error: 'error', + }; + const applyState = applyStateMap[fileEditState] ?? 'idle';Then use
applyStatedirectly instead ofgetApplyState().
165-167: Minimal error state rendering.The
output-errorcase renders a plain<div>while other tool components use styled error banners (e.g.,ChatMessageToolCreateFileusesChatToolInlinewithstatus="error"). Consider using a consistent error UI.🔎 Proposed fix
case 'output-error': { - return <div>File edit failed</div>; + return ( + <div className="flex items-center gap-2 rounded-md border border-destructive bg-destructive/10 px-3 py-2 text-sm text-destructive"> + <X className="size-4" /> + <span>File edit failed</span> + </div> + ); }Note: You'll need to import
Xfromlucide-react.apps/ui/app/routes/builds_.$id/chat-history-settings.tsx (1)
31-71: Simplify callback handlers by using setters directly.These
useCallbackwrappers add no value since they just forward to the cookie setters. The setters fromuseCookieare already stable references.🔎 Proposed simplification
- const handleShowModelCostToggle = useCallback( - (checked: boolean) => { - setShowModelCost(checked); - }, - [setShowModelCost], - ); - - const handleShowKernelErrorsToggle = useCallback( - (checked: boolean) => { - setShowKernelErrors(checked); - }, - [setShowKernelErrors], - ); - - const handleShowCodeErrorsToggle = useCallback( - (checked: boolean) => { - setShowCodeErrors(checked); - }, - [setShowCodeErrors], - ); - - const handleShowCodePreviewToggle = useCallback( - (checked: boolean) => { - setShowCodePreview(checked); - }, - [setShowCodePreview], - ); - - const handleShowImageScreenshotToggle = useCallback( - (checked: boolean) => { - setShowImageScreenshot(checked); - }, - [setShowImageScreenshot], - ); - - const handleShowImageRequirementsToggle = useCallback( - (checked: boolean) => { - setShowImageRequirements(checked); - }, - [setShowImageRequirements], - );Then use the setters directly in the JSX:
- onIsCheckedChange={handleShowModelCostToggle} + onIsCheckedChange={setShowModelCost}Apply similarly for all other
onIsCheckedChangeprops.apps/ui/app/routes/builds_.$id/chat-message-tool-list-directory.tsx (1)
21-90: Add exhaustive switch handling.The switch statement handles the known states but lacks a default case. While TypeScript's exhaustiveness checking should catch missing cases at compile time, adding an explicit exhaustive check improves runtime safety.
🔎 Proposed fix
case 'output-error': { return ( <ChatToolCard variant="minimal" status="error" isDefaultOpen={false}> <ChatToolCardHeader> <ChatToolCardIcon isError icon={FolderOpen} /> <ChatToolCardTitle>Failed to list directory</ChatToolCardTitle> </ChatToolCardHeader> </ChatToolCard> ); } + + default: { + // Exhaustiveness check - TypeScript will error if a case is missing + const _exhaustiveCheck: never = part; + return _exhaustiveCheck; + } } }apps/ui/app/routes/builds_.$id/chat-message-tool-image-analysis.tsx (1)
27-131: Add exhaustive switch handling for type safety.The switch covers the known states but lacks a default case for exhaustiveness checking.
🔎 Proposed fix
case 'output-error': { return ( <ChatToolCard variant="card" status="error" isDefaultOpen={false}> <ChatToolCardHeader> <ChatToolCardIcon isError icon={Eye} /> <ChatToolCardTitle>Image analysis failed</ChatToolCardTitle> </ChatToolCardHeader> </ChatToolCard> ); } + + default: { + const _exhaustiveCheck: never = part; + return _exhaustiveCheck; + } } }apps/ui/app/components/chat/chat-tool-card.tsx (3)
1-2: Consider consolidating React imports.Line 1 imports
* as Reactand line 2 imports specific hooks. This is redundant since you could useReact.createContext,React.useContext, etc.🔎 Proposed consolidation
-import * as React from 'react'; -import { createContext, useContext, useState, useCallback } from 'react'; +import React, { createContext, useContext, useState, useCallback, useMemo } from 'react';Note:
useMemois also used (line 105) but not in the current import list.
80-80: The'__unused'cookie name pattern is a type workaround.Using
'__unused' as CookieNamebypasses type safety. Consider using a more explicit pattern:🔎 Proposed alternative
Create a sentinel constant or use conditional hook calling pattern:
+// At the top of the file, define a dummy cookie name for when no cookie is needed +const UNUSED_COOKIE_NAME = '__unused' as CookieName; - const [cookieValue] = useCookie(cookieName ?? ('__unused' as CookieName), cookieDefault); + const [cookieValue] = useCookie(cookieName ?? UNUSED_COOKIE_NAME, cookieDefault);This centralizes the cast and makes the intent clearer. Alternatively, consider a custom hook that handles the conditional cookie reading.
335-339: Consider makingmaxHeighttype-safe.The
maxHeightprop accepts any string but is intended for Tailwind classes. Consider using a union type for common values or documenting the expected format.🔎 Suggested improvement
type ChatToolCardListProps = { readonly children: React.ReactNode; readonly className?: string; - readonly maxHeight?: string; + /** Tailwind max-height class (e.g., 'max-h-40', 'max-h-60') */ + readonly maxHeight?: `max-h-${string}`; };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/api/app/api/chat/prompts/chat-prompt-cad.tsapps/ui/app/components/chat/chat-tool-card.tsxapps/ui/app/components/chat/chat-tool-file-operation.tsxapps/ui/app/components/chat/chat-tool-inline.tsxapps/ui/app/constants/cookie.constants.tsapps/ui/app/routes/builds_.$id/chat-history-selector.tsxapps/ui/app/routes/builds_.$id/chat-history-settings.tsxapps/ui/app/routes/builds_.$id/chat-message-metadata-usage.tsxapps/ui/app/routes/builds_.$id/chat-message-reasoning.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-delete-file.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-file-edit.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-glob-search.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-grep.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-image-analysis.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-list-directory.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-read-file.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-reasoning.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-transfer.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-web-browser.tsxapps/ui/app/utils/currency.utils.tsapps/ui/app/utils/filesystem-snapshot.utils.tslibs/chat/src/schemas/tools/error.schema.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/ui/app/routes/builds_.$id/chat-message-tool-reasoning.tsx
- apps/ui/app/routes/builds_.$id/chat-message-tool-read-file.tsx
- apps/ui/app/routes/builds_.$id/chat-message-tool-glob-search.tsx
- apps/ui/app/routes/builds_.$id/chat-message-tool-delete-file.tsx
- apps/ui/app/routes/builds_.$id/chat-history-selector.tsx
- apps/ui/app/routes/builds_.$id/chat-message-tool-grep.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Useimport typefor type-only imports in TypeScript
All exported/public functions must have explicit return types (including React components). Private/internal functions don't require explicit return types
Usetypeinstead ofinterfacefor defining types in TypeScript
Useundefinedinstead ofnullin TypeScript
Noanytype - useunknowninstead in TypeScript
Always include.jsextensions for local imports with#prefix
Always use curly braces for control flow statements
Use explicit class member accessibility (public,private) in TypeScript
Use absolute imports with#prefix (e.g.,#utils/helper.js) instead of relative imports
Use descriptive variable names (e.g.,propertiesnotprops,eventnote)
**/*.{ts,tsx}: Useimport typefor type-only imports in TypeScript files
Always provide explicit return types for exported functions in TypeScript
Useunknowninstead ofanytype in TypeScript
Useundefinedinstead ofnulltype in TypeScript
Usetypeinstead ofinterfacefor defining object shapes in TypeScript
Always use curly braces for control flow statements in TypeScript/JavaScript
Explicitly declare class member accessibility (public/private) in TypeScript classes
Order class members in TypeScript: static, public, then private
Use absolute imports with#prefix instead of relative imports
Avoid optional chaining (?.) when a value cannot be nullish based on its type
Boolean props in React components should useisor similar prefix (e.g.,isDisabledinstead ofdisabled)
Always include a description comment for ESLint disable directives
Always include.jsextensions for local imports in TypeScript files
Keep type and value imports separate in TypeScript files
Files:
apps/ui/app/routes/builds_.$id/chat-history-settings.tsxapps/ui/app/constants/cookie.constants.tsapps/ui/app/routes/builds_.$id/chat-message-reasoning.tsxapps/ui/app/components/chat/chat-tool-inline.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-file-edit.tsxapps/ui/app/routes/builds_.$id/chat-message-metadata-usage.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-transfer.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-list-directory.tsxapps/ui/app/utils/currency.utils.tsapps/ui/app/routes/builds_.$id/chat-message-tool-web-browser.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-image-analysis.tsxapps/ui/app/components/chat/chat-tool-file-operation.tsxapps/ui/app/utils/filesystem-snapshot.utils.tsapps/ui/app/components/chat/chat-tool-card.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsxlibs/chat/src/schemas/tools/error.schema.tsapps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsxapps/api/app/api/chat/prompts/chat-prompt-cad.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/senior-eng.mdc)
**/*.{ts,tsx,js,jsx}: Use early returns in functions
Use conditional classes over ternary operators
Use descriptive, full names for variables and parameters (e.g.,propertiesinstead ofprops,eventinstead ofe,errorinstead ofe)
Prefer constants over functions when appropriate
Write functional and immutable code
Write pure functions that avoid side effects
Use composition over inheritance
Avoid mutation in functional programming; use Map, Filter, Reduce instead
Use currying and partial application techniques
Maintain immutability in code
Never usenull, onlyundefinedunless absolutely necessary
Comment function purpose and use JSDoc for JavaScript/TypeScript functions
Order functions with higher-order functionality first and group related functions together
Use appropriate error handling techniques; prefer returning errors over exceptions
Files:
apps/ui/app/routes/builds_.$id/chat-history-settings.tsxapps/ui/app/constants/cookie.constants.tsapps/ui/app/routes/builds_.$id/chat-message-reasoning.tsxapps/ui/app/components/chat/chat-tool-inline.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-file-edit.tsxapps/ui/app/routes/builds_.$id/chat-message-metadata-usage.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-transfer.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-list-directory.tsxapps/ui/app/utils/currency.utils.tsapps/ui/app/routes/builds_.$id/chat-message-tool-web-browser.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-image-analysis.tsxapps/ui/app/components/chat/chat-tool-file-operation.tsxapps/ui/app/utils/filesystem-snapshot.utils.tsapps/ui/app/components/chat/chat-tool-card.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsxlibs/chat/src/schemas/tools/error.schema.tsapps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsxapps/api/app/api/chat/prompts/chat-prompt-cad.ts
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/tailwind.mdc)
**/*.tsx: Use colors from global.css (see app/styles/global.css) when applying styles in TSX components
Apply opacity modifiers using slash suffix notation (e.g., bg-neutral/30) with valid values in range [5, 10, 20, 30, 40, 50, 60, 70, 80, 90]
Files:
apps/ui/app/routes/builds_.$id/chat-history-settings.tsxapps/ui/app/routes/builds_.$id/chat-message-reasoning.tsxapps/ui/app/components/chat/chat-tool-inline.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-file-edit.tsxapps/ui/app/routes/builds_.$id/chat-message-metadata-usage.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-transfer.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-list-directory.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-web-browser.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-image-analysis.tsxapps/ui/app/components/chat/chat-tool-file-operation.tsxapps/ui/app/components/chat/chat-tool-card.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsx
🧠 Learnings (9)
📚 Learning: 2025-12-22T05:18:52.859Z
Learnt from: rifont
Repo: taucad/tau PR: 105
File: apps/ui/app/hooks/use-analytics.tsx:36-36
Timestamp: 2025-12-22T05:18:52.859Z
Learning: In any TSX files using PostHog analytics, check analytics._isIdentified() before calling identify() to avoid emitting unnecessary identified events. Even though it has an underscore, it is part of the public API per PostHog's cost optimization guidance. Use the check to gate identify() calls (e.g., if (!analytics._isIdentified()) { analytics.identify(...); }). This improves performance and reduces costs by ensuring events are only identified when needed.
Applied to files:
apps/ui/app/routes/builds_.$id/chat-history-settings.tsxapps/ui/app/routes/builds_.$id/chat-message-reasoning.tsxapps/ui/app/components/chat/chat-tool-inline.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-file-edit.tsxapps/ui/app/routes/builds_.$id/chat-message-metadata-usage.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-transfer.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-list-directory.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-web-browser.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-image-analysis.tsxapps/ui/app/components/chat/chat-tool-file-operation.tsxapps/ui/app/components/chat/chat-tool-card.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsx
📚 Learning: 2025-12-04T11:06:46.474Z
Learnt from: CR
Repo: taucad/tau PR: 0
File: .cursor/rules/eslint.mdc:0-0
Timestamp: 2025-12-04T11:06:46.474Z
Learning: Applies to **/*.{ts,tsx} : Use `import type` for type-only imports in TypeScript files
Applied to files:
apps/ui/app/utils/filesystem-snapshot.utils.tsapps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsx
📚 Learning: 2025-12-04T11:06:36.186Z
Learnt from: CR
Repo: taucad/tau PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T11:06:36.186Z
Learning: Applies to **/*.{ts,tsx} : Use `import type` for type-only imports in TypeScript
Applied to files:
apps/ui/app/utils/filesystem-snapshot.utils.tsapps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsx
📚 Learning: 2025-12-04T11:06:36.186Z
Learnt from: CR
Repo: taucad/tau PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T11:06:36.186Z
Learning: Applies to **/*.{ts,tsx} : Use absolute imports with `#` prefix (e.g., `#utils/helper.js`) instead of relative imports
Applied to files:
apps/ui/app/utils/filesystem-snapshot.utils.tsapps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsx
📚 Learning: 2025-12-04T11:06:36.186Z
Learnt from: CR
Repo: taucad/tau PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T11:06:36.186Z
Learning: Applies to **/*.{ts,tsx} : Use `type` instead of `interface` for defining types in TypeScript
Applied to files:
apps/ui/app/utils/filesystem-snapshot.utils.tsapps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsx
📚 Learning: 2025-12-04T11:07:12.025Z
Learnt from: CR
Repo: taucad/tau PR: 0
File: .cursor/rules/senior-eng.mdc:0-0
Timestamp: 2025-12-04T11:07:12.025Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Prefer constants over functions when appropriate
Applied to files:
apps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsx
📚 Learning: 2025-12-04T11:06:36.186Z
Learnt from: CR
Repo: taucad/tau PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-04T11:06:36.186Z
Learning: Applies to **/*.{ts,tsx} : Use descriptive variable names (e.g., `properties` not `props`, `event` not `e`)
Applied to files:
apps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsxapps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsx
📚 Learning: 2025-12-04T11:07:12.025Z
Learnt from: CR
Repo: taucad/tau PR: 0
File: .cursor/rules/senior-eng.mdc:0-0
Timestamp: 2025-12-04T11:07:12.025Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : Use descriptive, full names for variables and parameters (e.g., `properties` instead of `props`, `event` instead of `e`, `error` instead of `e`)
Applied to files:
apps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsx
📚 Learning: 2025-12-04T11:06:46.474Z
Learnt from: CR
Repo: taucad/tau PR: 0
File: .cursor/rules/eslint.mdc:0-0
Timestamp: 2025-12-04T11:06:46.474Z
Learning: Applies to **/*.{ts,tsx} : Use absolute imports with `#` prefix instead of relative imports
Applied to files:
apps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsx
🧬 Code graph analysis (13)
apps/ui/app/routes/builds_.$id/chat-history-settings.tsx (2)
apps/ui/app/hooks/use-cookie.ts (1)
useCookie(82-127)apps/ui/app/constants/cookie.constants.ts (1)
cookieName(12-112)
apps/ui/app/routes/builds_.$id/chat-message-reasoning.tsx (3)
apps/ui/app/hooks/use-chat.tsx (1)
useChatSelector(277-307)apps/ui/app/components/chat/chat-tool-card.tsx (5)
ChatToolCard(419-419)ChatToolCardHeader(420-420)ChatToolCardTitle(422-422)ChatToolCardIcon(421-421)ChatToolCardContent(424-424)apps/ui/app/components/markdown/markdown-viewer.tsx (2)
MarkdownViewer(28-61)defaultMarkdownControls(22-26)
apps/ui/app/components/chat/chat-tool-inline.tsx (2)
apps/ui/app/utils/ui.utils.ts (1)
cn(10-12)apps/ui/app/components/magicui/animated-shiny-text.tsx (1)
AnimatedShinyText(8-27)
apps/ui/app/routes/builds_.$id/chat-message-tool-file-edit.tsx (4)
apps/ui/app/hooks/use-build.tsx (1)
useBuild(236-245)apps/ui/app/hooks/use-file-manager.tsx (1)
useFileManager(244-251)apps/ui/app/components/chat/chat-tool-file-operation.tsx (4)
CollapsibleFileOperation(517-599)ApplyButton(154-196)ErrorSection(58-119)CodePreview(206-241)apps/ui/app/components/copy-button.tsx (1)
CopyButton(20-68)
apps/ui/app/routes/builds_.$id/chat-message-metadata-usage.tsx (4)
apps/ui/app/hooks/use-cookie.ts (1)
useCookie(82-127)apps/ui/app/constants/cookie.constants.ts (1)
cookieName(12-112)apps/ui/app/components/ui/badge.tsx (1)
Badge(37-37)apps/ui/app/utils/currency.utils.ts (1)
formatCurrency(8-34)
apps/ui/app/routes/builds_.$id/chat-message-tool-transfer.tsx (4)
libs/chat/src/constants/tool.constants.ts (1)
toolName(1-17)apps/ui/app/hooks/use-chat.tsx (1)
useChatSelector(277-307)apps/ui/app/utils/ui.utils.ts (1)
cn(10-12)apps/ui/app/components/magicui/animated-shiny-text.tsx (1)
AnimatedShinyText(8-27)
apps/ui/app/routes/builds_.$id/chat-message-tool-web-browser.tsx (1)
apps/ui/app/components/chat/chat-tool-inline.tsx (1)
ChatToolInline(42-74)
apps/ui/app/routes/builds_.$id/chat-message-tool-image-analysis.tsx (4)
apps/ui/app/hooks/use-chat.tsx (1)
useChatSelector(277-307)apps/ui/app/components/chat/chat-tool-card.tsx (7)
ChatToolCard(419-419)ChatToolCardHeader(420-420)ChatToolCardIcon(421-421)ChatToolCardTitle(422-422)ChatToolCardContent(424-424)ChatToolCardActions(423-423)ChatToolCardSection(427-427)apps/ui/app/constants/cookie.constants.ts (1)
cookieName(12-112)apps/ui/app/components/ui/hover-card.tsx (3)
HoverCard(41-41)HoverCardTrigger(41-41)HoverCardContent(41-41)
apps/ui/app/components/chat/chat-tool-file-operation.tsx (6)
libs/types/src/types/cad.types.ts (1)
CodeError(5-11)libs/types/src/types/kernel.types.ts (1)
KernelError(23-29)apps/ui/app/utils/ui.utils.ts (1)
cn(10-12)apps/ui/app/components/magicui/animated-shiny-text.tsx (1)
AnimatedShinyText(8-27)apps/ui/app/components/code/code-viewer.tsx (1)
CodeViewer(14-29)apps/ui/app/components/copy-button.tsx (1)
CopyButton(20-68)
apps/ui/app/utils/filesystem-snapshot.utils.ts (2)
apps/ui/app/hooks/use-file-manager.tsx (1)
FileEntry(31-37)apps/ui/vitest.setup.ts (1)
root(44-46)
apps/ui/app/components/chat/chat-tool-card.tsx (6)
apps/ui/app/constants/cookie.constants.ts (2)
CookieName(117-117)cookieName(12-112)apps/ui/app/hooks/use-cookie.ts (1)
useCookie(82-127)apps/ui/app/components/ui/collapsible.tsx (3)
Collapsible(19-19)CollapsibleTrigger(19-19)CollapsibleContent(19-19)apps/ui/app/utils/ui.utils.ts (1)
cn(10-12)apps/ui/app/components/ui/button.tsx (1)
Button(54-54)apps/ui/app/components/magicui/animated-shiny-text.tsx (1)
AnimatedShinyText(8-27)
apps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsx (4)
libs/chat/src/types/tool.types.ts (1)
MyTools(44-60)libs/chat/src/constants/tool.constants.ts (1)
toolName(1-17)apps/ui/app/hooks/use-build.tsx (1)
useBuild(236-245)apps/ui/app/components/chat/chat-tool-file-operation.tsx (2)
CollapsibleFileOperation(517-599)CodePreview(206-241)
libs/chat/src/schemas/tools/error.schema.ts (2)
libs/types/src/types/cad.types.ts (1)
CodeError(5-11)libs/types/src/types/kernel.types.ts (1)
KernelError(23-29)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Redirect rules - taucad
- GitHub Check: Header rules - taucad
- GitHub Check: Pages changed - taucad
- GitHub Check: Main
- GitHub Check: Deploy API Review App
- GitHub Check: Deploy UI Review App
🔇 Additional comments (30)
apps/api/app/api/chat/prompts/chat-prompt-cad.ts (3)
14-19: LGTM! Clear type definitions with helpful documentation.The
FileLayoutModetype is well-documented with JSDoc explaining the distinction between 'full-nesting' and 'assembly-only' modes. The addition toKernelConfigis consistent with the existing structure.Also applies to: 36-36
127-127: LGTM! Kernel configurations correctly reflect their import capabilities.The
fileLayoutModevalues are appropriately assigned:
- OpenSCAD, Replicad, and JSCAD use 'full-nesting' (supporting arbitrary subdirectory imports)
- Zoo/KCL uses 'assembly-only' (reflecting its constraint that subdirectory imports must reference main entry points)
Also applies to: 195-195, 294-294, 407-407
577-722: LGTM! System prompt successfully integrates filesystem tool documentation.The
getCadSystemPromptfunction properly integrates the new file organization strategy and workflow examples at appropriate points in the prompt. The filesystem tools section provides comprehensive guidance covering read operations, modifications, file organization strategy, reasoning tool, kernel result tool, and visual validation.libs/chat/src/schemas/tools/error.schema.ts (2)
1-44: Well-structured error schema centralization.The implementation follows Zod and TypeScript best practices, properly uses type-only imports, and leverages
z.ZodType<T>to ensure schema-type alignment. The centralization of these schemas will reduce duplication across tool definitions.
31-42: No action required. The inline schema definitions are correctly aligned with their corresponding TypeScript types:stackFramesmatches theKernelStackFramestructure, and thetypeenum matches theKernelErrorTypeunion. Schema integrity is maintained.apps/ui/app/routes/builds_.$id/chat-message-tool-get-kernel-result.tsx (1)
16-103: LGTM! Component implementation follows guidelines.The component correctly handles all tool invocation states with proper TypeScript types, descriptive variable names, and functional patterns. The state-specific rendering logic is clear and appropriate.
apps/ui/app/routes/builds_.$id/chat-message-tool-transfer.tsx (2)
98-99: LGTM!The streaming and completion state detection logic is clear and correctly derives UI state from the combined chat status and part state.
103-136: LGTM!The component renders a well-structured, status-aware transfer UI with appropriate visual feedback for streaming, complete, and transfer-back states. The use of
AnimatedShinyTextduring streaming and contextual icons enhances the UX.apps/ui/app/components/chat/chat-tool-inline.tsx (1)
42-74: LGTM!
ChatToolInlineprovides a clean, reusable badge component with proper status-driven rendering, supporting loading states with animation, error variants, and optional image/icon display.apps/ui/app/routes/builds_.$id/chat-message-tool-web-browser.tsx (1)
9-48: LGTM!The component is cleanly refactored to use
ChatToolInlinefor all states, providing consistent status-driven rendering with appropriate icons and loading/error feedback.apps/ui/app/components/chat/chat-tool-file-operation.tsx (2)
252-290: Simple diff algorithm is acceptable for display purposes.The O(n×m) complexity from
.includes()calls is fine for typical file sizes in chat tool displays. For larger files, consider using a proper diff library (likedifffrom npm), but the current implementation with the "not optimal" comment is reasonable.
154-196: LGTM!
ApplyButtonprovides clear visual feedback for all states (idle, applying, success, error) with appropriate icons and tooltip messages.apps/ui/app/routes/builds_.$id/chat-message-tool-create-file.tsx (1)
9-72: LGTM!The component follows the established pattern for tool UI components with proper state handling, file navigation support, and consistent error styling.
apps/ui/app/routes/builds_.$id/chat-message-metadata-usage.tsx (2)
21-21: LGTM!The cookie-based toggle for model cost visibility is cleanly implemented. The
DollarSignicon provides a clear visual indicator, and the conditional rendering withundefinedaligns with the coding guidelines.Also applies to: 37-43
54-54: LGTM!The changes from
nulltoundefinedin conditional rendering align with the coding guidelines.Also applies to: 109-109
apps/ui/app/routes/builds_.$id/chat-history-settings.tsx (2)
73-184: Component structure looks good.The dropdown menu structure with nested submenus for File Operations and Image Analysis is well-organized. The use of tooltip, proper icon imports, and consistent styling follows the established patterns.
22-22: Add explicit return type for exported function.Per coding guidelines, all exported functions must have explicit return types.
-export function ChatHistorySettings(): React.ReactNode { +export function ChatHistorySettings(): React.JSX.Element {⛔ Skipped due to learnings
Learnt from: CR Repo: taucad/tau PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-04T11:06:36.186Z Learning: Applies to **/*.{ts,tsx} : All exported/public functions must have explicit return types (including React components). Private/internal functions don't require explicit return typesLearnt from: CR Repo: taucad/tau PR: 0 File: .cursor/rules/eslint.mdc:0-0 Timestamp: 2025-12-04T11:06:46.474Z Learning: Applies to **/*.{ts,tsx} : Always provide explicit return types for exported functions in TypeScriptLearnt from: CR Repo: taucad/tau PR: 0 File: .cursor/rules/senior-eng.mdc:0-0 Timestamp: 2025-12-04T11:07:12.025Z Learning: Applies to **/*.{ts,tsx,js,jsx} : Prefer constants over functions when appropriateapps/ui/app/routes/builds_.$id/chat-message-reasoning.tsx (2)
3-12: LGTM!Imports are well-organized, using
import typewhere appropriate for type-only imports. The separation of type and value imports follows the coding guidelines.
24-57: Well-structured component with proper streaming state handling.The component correctly:
- Detects streaming state via
useChatSelector- Shows a loading state when reasoning text is empty
- Uses
shouldBeOpento auto-expand during generation- Renders markdown content with proper streaming mode
The logic for
shouldBeOpen(line 42) is clear: force open while content is generating, then let user control it.apps/ui/app/constants/cookie.constants.ts (1)
64-77: LGTM!The new cookie entries for chat tool sections follow the established naming conventions:
- CamelCase TypeScript property names
- Hyphen-separated cookie string values
- Descriptive comments explaining each entry's purpose
apps/ui/app/routes/builds_.$id/chat-message-tool-list-directory.tsx (2)
40-47: Sorting logic is clean and correct.The sorting implementation correctly prioritizes directories over files and uses
localeComparefor proper alphabetical ordering within each type.
16-20: Add explicit return type for exported function.Per coding guidelines, all exported functions must have explicit return types. While the inline
ReactNodeis inferred, it should be explicit.export function ChatMessageToolListDirectory({ part, }: { readonly part: UIToolInvocation<MyTools[typeof toolName.listDirectory]>; -}): ReactNode { +}): React.JSX.Element {Note: Since this function always returns JSX elements (not null/undefined),
React.JSX.Elementis more precise thanReactNode.⛔ Skipped due to learnings
Learnt from: CR Repo: taucad/tau PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-12-04T11:06:36.186Z Learning: Applies to **/*.{ts,tsx} : All exported/public functions must have explicit return types (including React components). Private/internal functions don't require explicit return typesLearnt from: CR Repo: taucad/tau PR: 0 File: .cursor/rules/eslint.mdc:0-0 Timestamp: 2025-12-04T11:06:46.474Z Learning: Applies to **/*.{ts,tsx} : Always provide explicit return types for exported functions in TypeScriptLearnt from: CR Repo: taucad/tau PR: 0 File: .cursor/rules/senior-eng.mdc:0-0 Timestamp: 2025-12-04T11:07:12.025Z Learning: Applies to **/*.{ts,tsx,js,jsx} : Use appropriate error handling techniques; prefer returning errors over exceptionsLearnt from: CR Repo: taucad/tau PR: 0 File: .cursor/rules/senior-eng.mdc:0-0 Timestamp: 2025-12-04T11:07:12.025Z Learning: Applies to **/*.{ts,tsx,js,jsx} : Use early returns in functionsapps/ui/app/routes/builds_.$id/chat-message-tool-image-analysis.tsx (2)
24-25: TheisLoadingcheck may be dead code in the output-available case.
isLoadingis computed aschatStatus === 'streaming' && ['input-streaming', 'input-available'].includes(part.state). In theoutput-availablecase (line 55),part.stateis'output-available', so the.includes()check will always befalse, makingisLoadingalwaysfalsewhen used.If the intent is to show loading during streaming even after output is available, consider:
- const isLoading = chatStatus === 'streaming' && ['input-streaming', 'input-available'].includes(part.state); + const isLoading = chatStatus === 'streaming';Or if this loading state is only meant for input cases, remove it from the
output-availablecase entirely.
50-119: Clean refactor to ChatToolCard-based UI.The use of
ChatToolCardSectionwith cookie-backed persistence for screenshot and requirements sections is well-implemented. The conditional rendering withundefinedinstead ofnullfollows the coding guidelines.apps/ui/app/components/chat/chat-tool-card.tsx (6)
29-36: Add explicit return type for the exported hook.Per coding guidelines, all exported functions require explicit return types.
-function useChatToolCard(): ChatToolCardContextValue { +export function useChatToolCard(): ChatToolCardContextValue {Wait, looking at line 428,
useChatToolCardis already exported. The return type is already specified, so this is correct.
67-133: Well-designed root component with controlled/uncontrolled pattern.The ChatToolCard properly implements:
- Controlled vs uncontrolled open state
- Cookie-backed initial state (read-only, not persisted)
- Context provider for child components
- Proper memoization of context value
The architecture is clean and follows React best practices.
144-209: Header implementation handles both variants appropriately.The logic correctly differentiates between minimal (inline button/trigger) and card (full-width trigger) variants. The conditional
isCollapsiblehandling prevents the trigger when not needed.
280-297: Correct use ofstopPropagationfor action buttons.Using
event.stopPropagation()on the actions container prevents clicks from triggering the collapsible, which is the correct behavior. The parameter nameeventfollows the coding guidelines.
382-416: ChatToolCardSection correctly mirrors root component patterns.The section component properly implements cookie-backed initial state with the same pattern as the root component, maintaining consistency across the API.
418-431: Complete and well-organized exports.All components and the hook are properly exported, along with the type exports for variant and status. The export structure enables both named imports and type imports.
| throw new Error( | ||
| 'CLIENT_DISCONNECTED: No WebSocket connection to the browser. The user has likely closed or navigated away from the page. ' + | ||
| 'DO NOT RETRY this or any other tool - inform the user that you cannot proceed because they are no longer connected.', | ||
| ); |
There was a problem hiding this comment.
Test expects return values but implementation throws
High Severity
The sendToolCallRequest method throws an Error when no socket connection exists, but the tests expect it to return structured error objects with errorCode: 'NO_CLIENT_CONNECTION' and attempt tracking messages like '(attempt 1/3)'. The tests also expect failure counts to be tracked per-chatId and reset when connections are registered, but the implementation has none of this retry tracking logic.
Additional Locations (1)
| if (!modificationMap.has(file)) { | ||
| modificationMap.set(file, i); | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale detection uses earliest modification, misses later edits
Medium Severity
The buildFileModificationMap function records only the earliest modification for each file (if (!modificationMap.has(file))). When a file is modified, then read, then modified again, the read won't be marked stale because the check modificationIndex > index uses the earliest modification index. The logic needs to track the latest modification instead.
Additional Locations (1)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/api/app/api/chat/chat-exception.filter.ts`:
- Around line 63-70: The current logic in the ChatExceptionFilter catch block
sets message only when responseObject['message'] is a string, but NestJS
ValidationPipe returns message as string[]; update the handling in the block
that processes exceptionResponse and responseObject so that if
responseObject['message'] is an array (e.g., string[]), you join it into a
single readable string (e.g., responseObject['message'].join(', ')) and assign
that to message; keep falling back to exception.message and leave code
resolution using this.getErrorCode(exception) unchanged.
♻️ Duplicate comments (5)
apps/api/app/api/tools/tools/tool-read-file.ts (1)
23-26: JSDoc comment incorrectly describes padding direction.The comment says "right-padded to 6 chars" but
String.padStart(6)left-pads the number. The output format" 1|content"shows left-padding, so the code is correct but the documentation is inaccurate.📝 Suggested fix
/** * Add line numbers to raw content for LLM display. - * Format: " 1|content" where the number is right-padded to 6 chars. + * Format: " 1|content" where the number is left-padded to 6 chars. */apps/api/app/api/tools/tools/tool-edit-tests.ts (1)
63-67: Avoid overwriting when read fails for reasons other than "missing file."Any read error currently falls back to
defaultTestFile, which can overwrite a valid file if the read failed due to permissions or transient I/O. The conditionreadResult.content.startsWith('Error reading file:')doesn't distinguish between "file not found" and other errors.💡 Safer read-error handling
- // If file doesn't exist, use default content - const originalContent = - readResult.content.startsWith('Error reading file:') || readResult.content === '' - ? defaultTestFile - : readResult.content; + // Only use default content for missing file, not other errors + const isReadError = readResult.content.startsWith('Error reading file:'); + const isMissingFile = isReadError && readResult.content.includes('ENOENT'); + + if (isReadError && !isMissingFile) { + const result: EditTestsOutput = { + success: false, + diffStats: { + linesAdded: 0, + linesRemoved: 0, + originalContent: '', + modifiedContent: '', + }, + }; + return result; + } + + const originalContent = isMissingFile || readResult.content === '' ? defaultTestFile : readResult.content;apps/ui/app/components/chat/chat-tool-error.tsx (1)
138-142: Potential key collision in validation errors list.Using
error_.pathas the React key could cause issues if multiple validation errors share the same path. Use the array index or combine path with index for uniqueness.🔧 Suggested fix
- {validationError.validationErrors.map((error_) => ( - <li key={error_.path}> + {validationError.validationErrors.map((error_, index) => ( + <li key={`${error_.path}-${index}`}> <code className="text-destructive">{error_.path || 'root'}</code>: {error_.message} </li> ))}apps/api/app/api/analysis/analysis.service.ts (2)
30-38: Fail fast (or preserve placeholders) when any view is missing.Filtering out missing views shifts image order while the prompt still asserts 6 views, which can mislabel sides and skew results. Prefer rejecting the request (or inserting placeholders) so the model always receives the expected order and count.
🔧 Proposed guard (fail fast when views are missing)
- const sortedObservations = viewOrder - .map((side) => observations.find((obs) => obs.side === side)) - .filter((obs): obs is Observation => obs !== undefined); - - if (sortedObservations.length !== 6) { - this.logger.warn(`Expected 6 views, got ${sortedObservations.length}`); - } + const orderedObservations = viewOrder.map((side) => observations.find((obs) => obs.side === side)); + const missingSides = viewOrder.filter((side, index) => orderedObservations[index] === undefined); + if (missingSides.length > 0) { + this.logger.warn(`Missing views: ${missingSides.join(', ')}`); + return { + failures: requirements.map((requirement) => ({ + id: requirement.id, + requirement: requirement.description, + reason: `Missing views: ${missingSides.join(', ')}`, + suggestion: 'Provide all six orthographic views and retry.', + })), + passes: [], + passed: 0, + total: requirements.length, + }; + } + const sortedObservations = orderedObservations.filter((obs): obs is Observation => obs !== undefined);Also applies to: 145-149
56-103: Validate structured output before use (presence + completeness).
Output.objectcan yieldoutputmissing or incomplete results; current code treats missing requirement results as absent, which can silently undercount failures/passes. Add a guard for missingoutputand validate result coverage before mapping.✅ Suggested validation guard
const { output } = await generateText({ model: openai('gpt-4o'), output: Output.object({ schema: responseSchema, }), system: systemPrompt, messages: [ { role: 'user', content: [ { type: 'text', text: userPrompt }, // Include all views in order ...sortedObservations.map((obs) => ({ type: 'image' as const, image: obs.src, })), ], }, ], }); + + if (output === undefined) { + throw new Error('Structured output missing from model response'); + } + if (output.results.length !== requirements.length) { + this.logger.warn( + `Expected ${requirements.length} results, got ${output.results.length}`, + ); + }
🧹 Nitpick comments (3)
apps/ui/app/components/chat/chat-tool-error.tsx (1)
35-40: Consider removing this no-op function.The function returns its input unchanged. If formatting is planned for the future, consider adding a TODO comment; otherwise, remove the function and use
error.toolNamedirectly at the call site (line 88).apps/api/app/api/analysis/analysis.service.ts (1)
45-50: Avoidnullin the LLM result schema; make status-driven fields optional.
nullable()conflicts with the no-nullguidance and doesn’t enforce reason/suggestion only whenstatus === 'failed'. Prefer a discriminated union (or preprocessnull→undefined) so the schema matches intended output.Based on learnings, avoid `null` in TypeScript.♻️ Proposed schema refactor
- const llmResultSchema = z.object({ - id: z.string(), - status: z.enum(['passed', 'failed']), - reason: z.string().nullable().describe('Required when status is "failed", null otherwise'), - suggestion: z.string().nullable().describe('Required when status is "failed", null otherwise'), - }); + const llmResultSchema = z.discriminatedUnion('status', [ + z.object({ + id: z.string(), + status: z.literal('passed'), + }), + z.object({ + id: z.string(), + status: z.literal('failed'), + reason: z.string(), + suggestion: z.string(), + }), + ]);apps/api/app/api/chat/chat-exception.filter.ts (1)
20-23: Reorder class members to keep public before private.Line 21 defines a private field before the public
catchmethod. The guideline prefers static → public → private ordering; consider movingloggerbelow the public method. As per coding guidelines, please keep member order consistent.
| if (typeof exceptionResponse === 'string') { | ||
| message = exceptionResponse; | ||
| code = this.getErrorCode(exception); | ||
| } else if (typeof exceptionResponse === 'object') { | ||
| const responseObject = exceptionResponse as Record<string, unknown>; | ||
| message = typeof responseObject['message'] === 'string' ? responseObject['message'] : exception.message; | ||
| code = typeof responseObject['code'] === 'string' ? responseObject['code'] : this.getErrorCode(exception); | ||
| } else { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the actual file to understand the context
cat -n apps/api/app/api/chat/chat-exception.filter.ts | head -80Repository: taucad/tau
Length of output: 164
🏁 Script executed:
# Search for how HttpException is used in this file
rg -A 5 -B 5 "HttpException" apps/api/app/api/chat/chat-exception.filter.tsRepository: taucad/tau
Length of output: 164
🏁 Script executed:
# Look for other error handling patterns in the codebase
rg "message.*Array\|message.*\[\]" --type ts --type tsx | head -20Repository: taucad/tau
Length of output: 164
🏁 Script executed:
# Check if there are other exception filters that handle validation errors
fd -e ts -e tsx -path "*/exception*" -o -path "*/filter*"Repository: taucad/tau
Length of output: 164
🌐 Web query:
NestJS ValidationError HttpException message array format
💡 Result:
Short answer: use ValidationPipe.exceptionFactory to convert class-validator ValidationError[] into an array of messages and throw an HttpException/BadRequestException with that array as the message. Nest's ValidationPipe supports exceptionFactory for exactly this purpose. [1]
Example (global pipe):
- Key idea: map ValidationError -> string(s) and throw new BadRequestException(messagesArray)
TypeScript example:
import { ValidationPipe, BadRequestException, ValidationError } from '@nestjs/common';
function flattenErrors(errors: ValidationError[]): string[] {
const result: string[] = [];
const walk = (errs: ValidationError[]) => {
for (const err of errs) {
if (err.constraints) {
result.push(...Object.values(err.constraints));
}
if (err.children && err.children.length) {
walk(err.children);
}
}
};
walk(errors);
return result;
}
app.useGlobalPipes(
new ValidationPipe({
exceptionFactory: (errors: ValidationError[]) => {
const messages = flattenErrors(errors);
return new BadRequestException(messages); // messages will be the response "message" array
},
}),
);
Notes:
- You can return any HttpException (or custom structure) from exceptionFactory. [1]
- Useful examples / patterns shown on community threads. [2]
Sources:
- NestJS ValidationPipe options (exceptionFactory). [1]
- Community example using exceptionFactory to build a custom response. [2]
🌐 Web query:
NestJS class-validator validation pipe error response format
💡 Result:
By default ValidationPipe returns Nest's standard Bad Request body:
{
"statusCode": 400,
"error": "Bad Request",
"message": [ "field must be ..." ]
}
You can change that shape by passing options to ValidationPipe (notably errorHttpStatusCode, disableErrorMessages, and exceptionFactory) or by using an exception filter. Example — global pipe with a custom response shape via exceptionFactory:
app.useGlobalPipes(
new ValidationPipe({
whitelist: true,
transform: true,
exceptionFactory: (validationErrors = []) => {
const errors = validationErrors.map(err => ({
field: err.property,
messages: err.constraints ? Object.values(err.constraints) : [],
}));
return new BadRequestException({ status: 'error', errors });
},
}),
);
Notes
- exceptionFactory receives class-validator ValidationError[] and must return/throw an Exception instance. (docs.nestjs.com)
- Alternatively implement a global ExceptionFilter to normalize all exceptions (including validation) into one API error format. (ayoubkhial.com)
References
- NestJS Validation docs (ValidationPipe options & example). (docs.nestjs.com)
- StackOverflow example showing exceptionFactory usage. (stackoverflow.com)
- Blog / best-practice on consistent API error shapes (optional ExceptionFilter). (ayoubkhial.com)
Citations:
- 1: https://docs.nestjs.com/techniques/validation?utm_source=openai
- 2: https://www.ayoubkhial.com/blog/api-error-handling-done-right-nestjs?utm_source=openai
- 3: https://docs.nestjs.com/techniques/validation?utm_source=openai
- 4: https://stackoverflow.com/questions/60270468/throw-same-error-format-as-class-validator-in-nestjs?utm_source=openai
- 5: https://www.ayoubkhial.com/blog/api-error-handling-done-right-nestjs?utm_source=openai
Handle array messages from validation errors in HttpException responses.
Line 68 only checks if message is a string, but NestJS ValidationPipe by default responds with message: string[] for validation errors, causing error details to be lost. Join array messages into a readable format.
Suggested fix
- message = typeof responseObject['message'] === 'string' ? responseObject['message'] : exception.message;
+ const responseMessage = responseObject['message'];
+ if (typeof responseMessage === 'string') {
+ message = responseMessage;
+ } else if (Array.isArray(responseMessage)) {
+ message = responseMessage.join('; ');
+ } else {
+ message = exception.message;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (typeof exceptionResponse === 'string') { | |
| message = exceptionResponse; | |
| code = this.getErrorCode(exception); | |
| } else if (typeof exceptionResponse === 'object') { | |
| const responseObject = exceptionResponse as Record<string, unknown>; | |
| message = typeof responseObject['message'] === 'string' ? responseObject['message'] : exception.message; | |
| code = typeof responseObject['code'] === 'string' ? responseObject['code'] : this.getErrorCode(exception); | |
| } else { | |
| if (typeof exceptionResponse === 'string') { | |
| message = exceptionResponse; | |
| code = this.getErrorCode(exception); | |
| } else if (typeof exceptionResponse === 'object') { | |
| const responseObject = exceptionResponse as Record<string, unknown>; | |
| const responseMessage = responseObject['message']; | |
| if (typeof responseMessage === 'string') { | |
| message = responseMessage; | |
| } else if (Array.isArray(responseMessage)) { | |
| message = responseMessage.join('; '); | |
| } else { | |
| message = exception.message; | |
| } | |
| code = typeof responseObject['code'] === 'string' ? responseObject['code'] : this.getErrorCode(exception); | |
| } else { |
🤖 Prompt for AI Agents
In `@apps/api/app/api/chat/chat-exception.filter.ts` around lines 63 - 70, The
current logic in the ChatExceptionFilter catch block sets message only when
responseObject['message'] is a string, but NestJS ValidationPipe returns message
as string[]; update the handling in the block that processes exceptionResponse
and responseObject so that if responseObject['message'] is an array (e.g.,
string[]), you join it into a single readable string (e.g.,
responseObject['message'].join(', ')) and assign that to message; keep falling
back to exception.message and leave code resolution using
this.getErrorCode(exception) unchanged.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| 'CLIENT_DISCONNECTED: No WebSocket connection to the browser. The user has likely closed or navigated away from the page. ' + | ||
| 'DO NOT RETRY this or any other tool - inform the user that you cannot proceed because they are no longer connected.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Test expects error return but implementation throws
High Severity
The sendToolCallRequest method throws an Error when no WebSocket connection exists, but the accompanying tests expect it to return a structured error object with errorCode: 'NO_CLIENT_CONNECTION' and retry attempt tracking (attempt 1/3, attempt 2/3, etc.). The tests document specific retry behavior including resetting failure counts on reconnection, but none of this logic exists in the implementation. All "connection failure tracking" tests will fail.
Additional Locations (1)
| if (!modificationMap.has(file)) { | ||
| modificationMap.set(file, i); | ||
| } | ||
| } |
There was a problem hiding this comment.
Stale file detection stores first modification instead of last
Medium Severity
The buildFileModificationMap function only stores the earliest modification index for each file, but the stale detection logic requires the latest modification index. When a file is modified multiple times, a read_file result between two modifications won't be correctly detected as stale. For example: edit at index 0, read at index 2, edit at index 4 — the read at index 2 should be marked stale (modified at index 4) but won't be because only index 0 is stored.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| message: expect.stringContaining('(attempt 1/3)'), | ||
| }), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Tests expect behavior that doesn't exist in implementation
High Severity
The ChatToolsService tests expect sendToolCallRequest to return error objects with errorCode: 'NO_CLIENT_CONNECTION' and retry attempt tracking like '(attempt 1/3)', but the actual implementation throws an Error with 'CLIENT_DISCONNECTED' message instead. The tests also expect per-chat failure counting and counter reset on registerConnection, none of which exist in the implementation. All "connection failure tracking" tests will fail.
Additional Locations (1)
| // Only record the earliest modification | ||
| if (!modificationMap.has(file)) { | ||
| modificationMap.set(file, i); | ||
| } |
There was a problem hiding this comment.
Stale detection records earliest modification instead of latest
Medium Severity
The buildFileModificationMap function records only the earliest modification of each file (via if (!modificationMap.has(file))), but the stale detection logic at line 609 checks modificationIndex > index assuming it has the latest modification. This causes read_file results between multiple edits to not be marked as stale when a later edit exists after them.
Additional Locations (1)
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| 'CLIENT_DISCONNECTED: No WebSocket connection to the browser. The user has likely closed or navigated away from the page. ' + | ||
| 'DO NOT RETRY this or any other tool - inform the user that you cannot proceed because they are no longer connected.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Tests expect retry tracking that implementation lacks
High Severity
The sendToolCallRequest method throws an Error when no WebSocket connection exists, but the tests expect it to return an error object with retry tracking. The tests expect errorCode: 'NO_CLIENT_CONNECTION' with messages containing (attempt 1/3), (attempt 2/3), (attempt 3/3), and failure count reset on reconnection. The implementation instead throws with 'CLIENT_DISCONNECTED' and has no retry counting mechanism. All tests in the "connection failure tracking" describe block will fail.
Additional Locations (1)
| // Only record the earliest modification | ||
| if (!modificationMap.has(file)) { | ||
| modificationMap.set(file, i); | ||
| } |
There was a problem hiding this comment.
Stale detection misses reads between multiple file modifications
Medium Severity
The buildFileModificationMap function records only the EARLIEST modification index for each file, but the stale detection check (modificationIndex > index) needs the LATEST modification to work correctly. When a file has multiple edits, a read_file occurring between two edits won't be marked stale. For example: read at index 3, edit at index 2 (recorded), edit at index 6 (ignored). The read at index 3 incorrectly passes the check (2 > 3 is false) even though edit at index 6 happened after it.
Additional Locations (1)
| const researchTools = [tools.web_search, tools.web_browser].filter((tool) => tool !== undefined); | ||
| const { model: supervisorModel } = this.modelService.buildModel(modelId); | ||
| const { model: cadModel, support: cadSupport } = this.modelService.buildModel(modelId); | ||
| const { model: researchModel, support: researchSupport } = this.modelService.buildModel(modelId); |
There was a problem hiding this comment.
Database connection leak from per-request PostgresSaver creation
High Severity
The createAgent method creates a new PostgresSaver.fromConnString() instance and calls setup() on every invocation. Since createAgent is called for each chat request from the controller, this creates a new database connection (or connection pool) per request that's never closed. Over time, this will exhaust the database's connection limit, causing "too many connections" errors and service degradation. The checkpointer should be created once as a singleton and reused across requests.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| 'CLIENT_DISCONNECTED: No WebSocket connection to the browser. The user has likely closed or navigated away from the page. ' + | ||
| 'DO NOT RETRY this or any other tool - inform the user that you cannot proceed because they are no longer connected.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Method throws instead of returning error object
High Severity
The sendToolCallRequest method throws an Error when there's no WebSocket connection, but its return type is Promise<ClientToolOutput<T> | ToolExecutionError | ToolValidationError>, indicating it should return an error object instead. The tests expect a returned object with errorCode: 'NO_CLIENT_CONNECTION' and failure tracking messages like "(attempt 1/3)", but the implementation throws. Other error cases in the same file (timeout at line 141-148, client error at lines 205-211) correctly return ToolExecutionError objects, making this inconsistent. The tests will fail when run.
Additional Locations (1)
| // Only record the earliest modification | ||
| if (!modificationMap.has(file)) { | ||
| modificationMap.set(file, i); | ||
| } |
There was a problem hiding this comment.
Stale file detection fails with multiple edits
Medium Severity
The buildFileModificationMap function stores the EARLIEST modification index for each file (due to the if (!modificationMap.has(file)) check), but the stale detection logic at line 607-610 checks modificationIndex > index to determine if a file was modified AFTER a read. This fails when a file is edited, then read, then edited again - the read won't be marked stale even though the later edit invalidated it. The function should store the LATEST modification index instead, so later edits are detected.
Note
Modernizes the chat/runtime stack and adds visual analysis.
ChatToolsGateway(Socket.IO),ChatToolsServicewith input/output validation, timeouts, per-chat connection tracking, and tests; dev namespace viaDevWebSocketService; integrated inChatModuleandApiModule.POST /v1/analysis/visual-testswithAnalysisServiceusing a single multi-view LLM call; schemas/DTOs and prompt for robust pass/fail reasoning.createAgent,toBaseMessages/toUIMessageStream, andcreateUIMessageStreamResponse; newChatExceptionFilterfor consistent errors; snapshot context injection.cad-agent.prompt.ts,cad-name.prompt.ts,git-commit.prompt.ts; kernel-config driven guidance.REDIS_URL, infra scripts in docs, env examples; minor path/tsconfig fixes and Cursor rules/docs for tools/context engineering.Written by Cursor Bugbot for commit 6ee63c6. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.