fixing file uploads on context page #212
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. WalkthroughAdds server /api/context routes for image and file description backed by streaming Claude; introduces shared context-file loader and UI image/text utilities; extends UI, hooks, store, and types to support text-file attachments and context descriptions; standardizes quoting style. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Frontend as UI
participant BrowserFS as Browser/FileSystem
participant Server
participant Claude as Claude API
User->>Frontend: Drop/select image or text file(s)
Frontend->>BrowserFS: Read file(s) (fileToBase64 / fileToText)
Frontend->>Frontend: Validate & sanitize (validateImageFile/validateTextFile, sanitizeFilename)
Frontend->>Server: POST /api/context/describe-image or /describe-file { path }
Server->>Server: Resolve path, enforce ALLOWED_ROOT_DIRECTORY, read file
Server->>Claude: Open streaming request (multipart for image or prompt for file)
Claude-->>Server: Streamed assistant chunks
Server->>Server: extractTextFromStream -> assemble description
Server-->>Frontend: { success: true, description } or { success: false, error }
Frontend->>Frontend: Persist metadata & update UI (show description)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello @webdevcody, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the application's context management capabilities. It introduces robust file upload features for both images and text, integrating AI-powered description generation to enhance the system's understanding of user-provided context. The changes span across the server-side API, UI components, and shared utility libraries, resulting in a more streamlined, intelligent, and user-friendly experience for managing contextual information. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: the Context page, which allows users to manage and upload files (both images and text) to be used as context for the AI. This includes new backend endpoints for generating file descriptions using an AI model. The frontend is updated with a new view for managing these context files, and the agent chat view is enhanced to support text file attachments. Additionally, there's a valuable refactoring that centralizes file and image handling utilities into a new image-utils.ts file, improving code organization.
I've identified a couple of areas for improvement. First, the way images are stored in the context directory (as data URLs in text files) is not ideal and could be made more robust and efficient. Second, the new "Create Markdown" functionality could accidentally overwrite existing files. My detailed comments provide suggestions for addressing these points. Overall, this is a great addition that significantly enhances the application's capabilities.
| filePath = `${contextPath}/${fileName}`; | ||
| await api.writeFile(filePath, dataUrl); |
There was a problem hiding this comment.
Storing images as data URLs inside text files in the .automaker/context directory is inefficient and can be confusing. It makes these files not actual image files, which could cause issues if other parts of the system expect to read binary image data from them.
A better approach would be to only store the image in the .automaker/images directory (as is already being done for description generation) and then reference this path. For displaying the image in the UI, you can use an API endpoint to serve the image, similar to how the board background image is handled. This would keep the context directory clean and ensure files are stored in their native format.
| const filePath = `${contextPath}/${filename}`; | ||
|
|
||
| if (newFileType === 'image' && uploadedImageData) { | ||
| // Write image data | ||
| await api.writeFile(filePath, uploadedImageData); | ||
| } else { | ||
| // Write text file with content (or empty if no content) | ||
| await api.writeFile(filePath, newFileContent); | ||
| // Write markdown file | ||
| await api.writeFile(filePath, newMarkdownContent); |
There was a problem hiding this comment.
The handleCreateMarkdown function does not check if a file with the given name already exists before writing to it. This could lead to accidental overwriting of existing context files. You should add a check using api.exists() and show an error to the user if the file already exists.
const filePath = `${contextPath}/${filename}`;
const exists = await api.exists(filePath);
if (exists) {
toast.error(`A file named "${filename}" already exists.`);
return;
}
// Write markdown file
await api.writeFile(filePath, newMarkdownContent);
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
apps/server/src/routes/context/routes/describe-file.ts (2)
42-67: Inconsistent implementation compared to describe-image.ts.The
extractTextFromStreamfunction here is simpler than the one indescribe-image.ts:
- Missing logging for debugging stream events
- Missing
requestIdparameter for log correlationConsider extracting a shared helper or aligning implementations for consistency.
129-137: Missing detailed error handling.Unlike
describe-image.ts, this handler:
- Doesn't have error mapping for common failure modes (rate limits, CLI issues)
- Doesn't include a
requestIdin error responses for debugging- Doesn't log error details comprehensively
Consider adding similar error handling for consistency and debuggability.
apps/ui/src/lib/image-utils.ts (1)
39-51: Suggest more generic fallback name.The fallback name
'image'at line 50 is used for both images and text files via this utility function. Consider using a more generic name like'file'or passing a type parameter to specify the appropriate fallback.🔎 Proposed refactor
-export function sanitizeFilename(filename: string): string { +export function sanitizeFilename(filename: string, defaultName = 'file'): string { const lastDot = filename.lastIndexOf('.'); const name = lastDot > 0 ? filename.substring(0, lastDot) : filename; const ext = lastDot > 0 ? filename.substring(lastDot) : ''; const sanitized = name .replace(/[\s\u00A0\u202F\u2009\u200A]+/g, '_') // Various space characters (regular, non-breaking, narrow no-break, thin, hair) .replace(/[^a-zA-Z0-9_-]/g, '_') // Non-alphanumeric chars .replace(/_+/g, '_') // Collapse multiple underscores .replace(/^_|_$/g, ''); // Trim leading/trailing underscores - return `${sanitized || 'image'}${ext}`; + return `${sanitized || defaultName}${ext}`; }apps/ui/src/components/ui/description-image-dropzone.tsx (1)
196-196: Inconsistent ID generation: UsegenerateFileId()for consistency.The image ID is generated inline here, but
generateFileId()fromimage-utilsis already imported and used for text files at line 163. Consider using the same utility for images to maintain consistency.🔎 Suggested fix
- const imageId = `img-${Date.now()}-${Math.random().toString(36).substring(2, 11)}`; + const imageId = generateFileId();apps/ui/src/components/views/context-view.tsx (1)
400-412: Sequential file processing could impact UX for bulk uploads.Files are processed sequentially with
await uploadFile(file)inside the loop. For multiple files, this could be slow. Consider processing in parallel withPromise.allorPromise.allSettledfor better UX.🔎 Suggested parallel processing
const handleDrop = async (e: React.DragEvent<HTMLDivElement>) => { e.preventDefault(); e.stopPropagation(); setIsDropHovering(false); const files = Array.from(e.dataTransfer.files); if (files.length === 0) return; - // Process files sequentially - for (const file of files) { - await uploadFile(file); - } + // Process files in parallel + await Promise.allSettled(files.map(uploadFile)); };Note: You may need to adjust
isUploadinganduploadingFileNamestate management for parallel uploads.apps/ui/src/hooks/use-electron-agent.ts (1)
82-90: Duplicated text file context building logic.The logic for building
messageContentwith text file context (lines 82-90) is duplicated insendMessage(lines 341-349). Consider extracting this into a helper function to maintain DRY principles.🔎 Suggested refactor
// Helper function (add near the top of the file or in a utils module) function buildMessageWithTextContext( content: string, textFiles?: TextFileAttachment[] ): string { if (!textFiles || textFiles.length === 0) { return content; } const contextParts = textFiles.map((file) => { return `<file name="${file.filename}">\n${file.content}\n</file>`; }); const contextBlock = `Here are some files for context:\n\n${contextParts.join('\n\n')}\n\n`; return contextBlock + content; } // Then use in both sendMessageDirectly and sendMessage: const messageContent = buildMessageWithTextContext(content, textFiles);Also applies to: 341-349
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
apps/server/src/index.tsapps/server/src/providers/claude-provider.tsapps/server/src/routes/context/index.tsapps/server/src/routes/context/routes/describe-file.tsapps/server/src/routes/context/routes/describe-image.tsapps/ui/src/components/dialogs/board-background-modal.tsxapps/ui/src/components/ui/description-image-dropzone.tsxapps/ui/src/components/ui/feature-image-upload.tsxapps/ui/src/components/ui/image-drop-zone.tsxapps/ui/src/components/views/agent-view.tsxapps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsxapps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsxapps/ui/src/components/views/context-view.tsxapps/ui/src/hooks/use-electron-agent.tsapps/ui/src/hooks/use-message-queue.tsapps/ui/src/lib/electron.tsapps/ui/src/lib/http-api-client.tsapps/ui/src/lib/image-utils.tsapps/ui/src/store/app-store.tslibs/types/src/feature.tslibs/types/src/index.tslibs/types/src/model.ts
🧰 Additional context used
🧬 Code graph analysis (12)
apps/ui/src/hooks/use-message-queue.ts (1)
apps/ui/src/store/app-store.ts (2)
ImageAttachment(190-196)TextFileAttachment(198-204)
libs/types/src/feature.ts (3)
apps/ui/src/components/ui/description-image-dropzone.tsx (1)
FeatureTextFilePath(27-27)apps/ui/src/store/app-store.ts (1)
FeatureTextFilePath(228-228)libs/types/src/index.ts (1)
FeatureTextFilePath(19-19)
apps/ui/src/lib/image-utils.ts (1)
.github/scripts/upload-to-r2.js (1)
filename(339-339)
apps/server/src/routes/context/routes/describe-file.ts (1)
libs/types/src/model.ts (1)
CLAUDE_MODEL_MAP(4-8)
apps/ui/src/components/ui/image-drop-zone.tsx (1)
apps/ui/src/lib/image-utils.ts (3)
DEFAULT_MAX_FILES(27-27)validateImageFile(105-127)generateImageId(134-136)
apps/ui/src/components/ui/feature-image-upload.tsx (1)
apps/ui/src/lib/image-utils.ts (3)
DEFAULT_MAX_FILES(27-27)validateImageFile(105-127)generateImageId(134-136)
apps/server/src/routes/context/routes/describe-image.ts (2)
apps/server/src/lib/sdk-options.ts (1)
createCustomOptions(306-326)libs/types/src/index.ts (1)
CLAUDE_MODEL_MAP(36-36)
apps/server/src/providers/claude-provider.ts (2)
libs/types/src/index.ts (2)
ExecuteOptions(10-10)ProviderMessage(12-12)apps/server/src/providers/types.ts (2)
ExecuteOptions(25-36)ProviderMessage(54-65)
apps/ui/src/hooks/use-electron-agent.ts (5)
apps/ui/src/store/app-store.ts (2)
ImageAttachment(190-196)TextFileAttachment(198-204)apps/ui/src/lib/electron.ts (1)
getElectronAPI(558-567)apps/ui/src/lib/image-utils.ts (1)
sanitizeFilename(39-51)apps/ui/src/hooks/use-message-queue.ts (1)
useMessageQueue(35-90)apps/ui/src/types/electron.d.ts (2)
StreamEvent(27-57)Message(13-20)
apps/ui/src/lib/electron.ts (5)
apps/ui/src/store/app-store.ts (2)
ClaudeUsageResponse(490-490)Feature(249-260)apps/ui/src/types/electron.d.ts (1)
ElectronAPI(447-576)libs/types/src/feature.ts (1)
Feature(24-59)libs/types/src/index.ts (1)
Feature(19-19)apps/server/src/services/feature-loader.ts (1)
Feature(20-20)
apps/server/src/index.ts (5)
apps/server/src/routes/health/index.ts (1)
createHealthRoutes(9-16)apps/server/src/lib/auth.ts (1)
authMiddleware(18-45)apps/server/src/routes/sessions/index.ts (1)
createSessionsRoutes(14-25)apps/server/src/routes/terminal/index.ts (1)
createTerminalRoutes(28-44)apps/server/src/routes/terminal/common.ts (2)
isTerminalEnabled(102-104)isTerminalPasswordRequired(95-97)
apps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsx (7)
apps/ui/src/components/ui/feature-image-upload.tsx (1)
FeatureImage(13-19)apps/ui/src/store/app-store.ts (4)
FeatureImage(237-243)AgentModel(229-229)ThinkingLevel(231-231)PlanningMode(230-230)libs/types/src/index.ts (3)
AgentModel(36-36)ThinkingLevel(53-53)PlanningMode(52-52)libs/types/src/model.ts (1)
AgentModel(23-23)apps/ui/src/components/ui/description-image-dropzone.tsx (1)
ImagePreviewMap(24-24)apps/ui/src/lib/utils.ts (1)
modelSupportsThinking(12-15)apps/ui/src/components/ui/button.tsx (1)
Button(116-116)
🔇 Additional comments (48)
apps/ui/src/components/ui/feature-image-upload.tsx (2)
4-11: Good refactor: Centralized image utilities.The imports from
@/lib/image-utilsconsolidate validation, ID generation, and constants into a shared module, improving consistency across components.
51-56: Validation logic is correctly delegated.The
validateImageFilecall properly handles type and size validation. The non-null assertion onvalidation.error!(line 54) is safe since it's only accessed when!validation.isValid, which guarantees the error is set per the function contract inimage-utils.ts.apps/server/src/routes/context/routes/describe-image.ts (3)
364-376: Good error details capture for debugging.The comprehensive error property extraction helps diagnose SDK failures, especially for stderr/stdout/exitCode which are common in process-based integrations.
314-321: The promptGenerator structure is compatible with the Claude Agent SDK.The query function accepts
prompt: string | AsyncIterable<SDKUserMessage>, and the yielded message structure matches the expectedSDKUserMessagetype exactly. No changes are needed.
296-306: Validation is already in place.The
createCustomOptionsfunction validatescwdviavalidateWorkingDirectory, which enforcesALLOWED_ROOT_DIRECTORYboundaries when configured. The code comment at line 299 correctly acknowledges this. Note that ifALLOWED_ROOT_DIRECTORYis not set in the environment, all paths are allowed by design for backward compatibility.apps/ui/src/lib/electron.ts (2)
468-479: New context API interface added.The
contextproperty correctly defines the client-side interface for the newdescribe-imageanddescribe-fileserver endpoints. The response types match the server implementation.
106-140: String literal style normalization.Quote style changes from double to single quotes are cosmetic and consistent with the codebase style.
apps/server/src/routes/context/index.ts (1)
17-24: Router implementation is clean and correctly wires handler factories.No authentication middleware is currently applied to these context endpoints. Before approving, verify whether
/describe-imageand/describe-fileshould be protected based on their sensitivity and intended access patterns. If these endpoints process user data or sensitive resources, consider adding authentication middleware at the router level or mount point.apps/ui/src/lib/http-api-client.ts (1)
965-983: LGTM! Clean API integration for context endpoints.The new context API methods follow the established patterns in this file and provide a consistent interface for describing images and files via the backend.
apps/server/src/providers/claude-provider.ts (1)
1-178: LGTM! Consistent quote standardization.The changes apply single-quote formatting consistently across string literals, aligning with the PR's broader formatting standardization. No functional changes introduced.
libs/types/src/feature.ts (2)
15-22: LGTM! Well-structured text file attachment type.The new
FeatureTextFilePathinterface mirrors the existingFeatureImagePathpattern and appropriately includes acontentfield to store the text data.
38-38: LGTM! Clean integration of text file support.The optional
textFilePathsfield extends the Feature interface to support text file attachments without breaking existing code.apps/ui/src/components/dialogs/board-background-modal.tsx (1)
19-24: LGTM! Good refactoring to use centralized utilities.The refactoring to use image validation and conversion utilities from
@/lib/image-utilsreduces code duplication and ensures consistent file handling across the application.Also applies to: 77-89, 93-93
apps/ui/src/hooks/use-message-queue.ts (1)
2-2: LGTM! Clean extension of message queue to support text files.The hook is properly extended to handle text file attachments while maintaining backward compatibility through optional parameters.
Also applies to: 8-8, 19-23, 40-52
apps/ui/src/lib/image-utils.ts (1)
59-72: LGTM! Comprehensive and well-implemented file utilities.The utility functions provide robust validation, conversion, and type checking for both images and text files. The implementations follow standard patterns with appropriate error handling.
Also applies to: 105-127, 134-145, 153-168, 177-201, 209-222, 230-236
apps/ui/src/components/views/agent-view.tsx (3)
161-253: LGTM! Robust file processing with proper validation.The updated
processDroppedFilesfunction properly handles both images and text files with appropriate validation, size checks, and error handling. The combined file limit enforcement across both types is well-implemented.
765-852: LGTM! Clean UI implementation for text file previews.The text file attachment previews mirror the image preview structure and provide consistent user experience with appropriate visual indicators and remove functionality.
134-150: No action required. ThesendMessagefunction in theuseElectronAgenthook properly supports thetextFilesparameter as the third argument, acceptingtextFiles?: TextFileAttachment[]. The code correctly passesmessageTextFilesto this function.libs/types/src/index.ts (1)
19-19: LGTM!The
FeatureTextFilePathexport is correctly added alongside the existing feature-related type exports. This properly exposes the new text file path type for use across the application.apps/ui/src/store/app-store.ts (2)
198-213: LGTM - Well-structured text file attachment support.The
TextFileAttachmentinterface is properly defined with all necessary fields. The integration withChatMessagevia the optionaltextFilesfield is consistent with the existingimagespattern.
249-258: LGTM - Consistent pattern with image paths.The
Featureinterface correctly extends the base type by omitting and re-addingtextFilePathswith the stricter UI-specific type, following the same pattern asimagePaths.apps/ui/src/components/views/board-view/dialogs/add-feature-dialog.tsx (3)
16-21: LGTM - Clean import aliasing for text file support.The import of
FeatureTextFilePathaliased asDescriptionTextFilePathmaintains consistency with the existingFeatureImagePathaliasing pattern.
104-117: LGTM - Proper state initialization.The
newFeaturestate correctly initializestextFilePathsas an empty array, consistent with theimagePathsinitialization pattern.
329-348: LGTM - Well-integrated text file handling in DescriptionImageDropZone.The
textFilesandonTextFilesChangeprops are properly wired to manage text file attachments alongside images, maintaining a clean separation of concerns.apps/ui/src/components/views/board-view/dialogs/edit-feature-dialog.tsx (3)
16-21: LGTM - Consistent import pattern with AddFeatureDialog.The import aliasing follows the same convention as in
add-feature-dialog.tsx, maintaining consistency across the codebase.
164-178: LGTM - Proper null-safety in updates object.The
textFilePaths: editingFeature.textFilePaths ?? []provides appropriate fallback handling for features that may not have text file paths defined.
300-306: LGTM - Consistent DescriptionImageDropZone integration.The text file handling props (
textFilesandonTextFilesChange) are properly wired with null-safety via?? [], mirroring the image handling pattern.apps/server/src/index.ts (3)
49-49: LGTM - Context routes import added.The import follows the established pattern for route modules in this server.
160-174: LGTM - WebSocket upgrade handling unchanged.The upgrade handling logic correctly routes to the appropriate WebSocket servers. The formatting updates (template literals, single quotes) don't affect functionality.
149-149: Context routes properly mounted with authentication.The
/api/contextroutes are correctly mounted after theauthMiddleware(line 129), ensuring they require authentication. This follows the same security pattern as other protected routes.apps/ui/src/components/ui/image-drop-zone.tsx (3)
5-13: Good refactor: Centralized utility imports.The imports from
@/lib/image-utilsconsolidate previously duplicated logic for file validation, ID generation, and formatting. This improves maintainability and ensures consistent behavior across components.
62-67: LGTM: Validation refactored to use centralized utility.The validation logic is now delegated to
validateImageFile, which returns a structured result. The non-null assertion onvalidation.error!is safe here since it's only accessed when!validation.isValid.
76-84: LGTM: Image attachment creation uses centralized utilities.The
generateImageId()function provides consistent ID generation, and the attachment structure properly includes all required fields.apps/ui/src/components/ui/description-image-dropzone.tsx (4)
6-21: Good extension: Text file handling utilities added.The imports support the new dual-file functionality (images and text files). The utilities from
image-utilsare properly leveraged for validation and processing.
140-142: LGTM: Combined file count calculation.Correctly calculates the total current files from both images and text files to enforce the
maxFileslimit across both types.
145-172: LGTM: Text file processing logic.The text file branch properly validates using
validateTextFile, reads content viafileToText, sanitizes filenames, and constructsFeatureTextFilePathobjects. Error handling is appropriate.
507-538: LGTM: Text file preview UI.The text file previews are well-structured with appropriate icons, remove buttons, and filename/size tooltips. The UI follows the same patterns as the image previews.
apps/ui/src/components/views/context-view.tsx (6)
59-61: LGTM: Metadata interface definition.Clean interface for persisting file descriptions in
context-metadata.json.
134-150: LGTM: Metadata loading with graceful fallback.The
loadMetadatafunction properly handles missing metadata files by returning an empty structure.
385-387: Appropriate fire-and-forget pattern for background description generation.The
generateDescriptionAsynccall withoutawaitis intentional here to allow the file to appear in the list immediately while the description is generated in the background. The function already has its own error handling and state management.
255-283: LGTM: Description generation with proper error handling.The
generateDescriptionfunction properly handles both success and error cases, using toast notifications to inform users of failures.
286-314: LGTM: Async description generation with proper state tracking.The
generateDescriptionAsyncfunction correctly tracks generating state per file using a Set, handles errors gracefully, and refreshes the UI after successful generation.
542-548: LGTM: Metadata sync during rename operation.The rename handler properly updates the metadata to preserve descriptions when files are renamed.
apps/ui/src/hooks/use-electron-agent.ts (5)
1-6: LGTM: Extended imports for text file support.The imports now include
TextFileAttachmentandsanitizeFilenameto support the new text file attachment functionality.
19-23: LGTM: API surface extended for text file attachments.The
sendMessagesignature andqueuedMessagestype are properly extended to support text file attachments alongside images.Also applies to: 28-34
82-90: Good context format for Claude.The XML-like
<file name="...">format is appropriate for providing structured context to Claude. The content is clearly delineated and the filename is preserved as metadata.
137-146: LGTM: Queue processing correctly propagates text files.The
onProcessNexthandler properly passestextFilesthrough tosendMessageDirectly, ensuring queued messages retain their text file attachments.
97-99: Good practice: Sanitizing filenames before saving.Using
sanitizeFilenameon image filenames before saving prevents issues with special characters and ensures consistent file naming.
| function findActualFilePath(requestedPath: string): string | null { | ||
| // First, try the exact path | ||
| if (fs.existsSync(requestedPath)) { | ||
| return requestedPath; | ||
| } | ||
|
|
||
| // Try with Unicode normalization | ||
| const normalizedPath = requestedPath.normalize('NFC'); | ||
| if (fs.existsSync(normalizedPath)) { | ||
| return normalizedPath; | ||
| } | ||
|
|
||
| // If not found, try to find the file in the directory by matching the basename | ||
| // This handles cases where the space character differs (U+0020 vs U+202F vs U+00A0) | ||
| const dir = path.dirname(requestedPath); | ||
| const baseName = path.basename(requestedPath); | ||
|
|
||
| if (!fs.existsSync(dir)) { | ||
| return null; | ||
| } | ||
|
|
||
| try { | ||
| const files = fs.readdirSync(dir); | ||
|
|
||
| // Normalize the requested basename for comparison | ||
| // Replace various space-like characters with regular space for comparison | ||
| const normalizeSpaces = (s: string): string => s.replace(/[\u00A0\u202F\u2009\u200A]/g, ' '); | ||
|
|
||
| const normalizedBaseName = normalizeSpaces(baseName); | ||
|
|
||
| for (const file of files) { | ||
| if (normalizeSpaces(file) === normalizedBaseName) { | ||
| logger.info(`Found matching file with different space encoding: ${file}`); | ||
| return path.join(dir, file); | ||
| } | ||
| } | ||
| } catch (err) { | ||
| logger.error(`Error reading directory ${dir}: ${err}`); | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/server/src/routes/context/routes/describe-image.ts | head -200Repository: AutoMaker-Org/automaker
Length of output: 7572
🏁 Script executed:
find apps/server/src/routes/context/routes -name "describe-image.ts" -type f 2>/dev/nullRepository: AutoMaker-Org/automaker
Length of output: 122
🏁 Script executed:
cat -n apps/server/src/routes/context/routes/describe-image.ts | tail -n +200Repository: AutoMaker-Org/automaker
Length of output: 9069
Add path traversal protection: validate imagePath is within an allowed directory before file operations.
The findActualFilePath function accepts untrusted user input from req.body.imagePath (line 214) without validating that the resolved path remains within an intended directory. An attacker can exploit this by sending paths like ../../../../etc to enumerate directory contents. Line 50's fs.readdirSync(dir) will read any accessible directory based on the supplied path.
Implement bounds checking before calling findActualFilePath:
- Define a base directory for allowed images (e.g., project root or specific uploads folder)
- Resolve the imagePath to its canonical absolute form using
path.resolve(baseDir, imagePath) - Verify the resolved path starts with baseDir using
resolvedPath.startsWith(baseDir) - Reject the request if the path escapes the allowed scope
This prevents directory enumeration and ensures file operations remain scoped to intended locations.
🤖 Prompt for AI Agents
In apps/server/src/routes/context/routes/describe-image.ts around lines 28 to 69
(and where req.body.imagePath is read around line 214), the function currently
uses an untrusted imagePath directly which allows path traversal; fix by scoping
all file operations to a configured base directory: define a baseDir (e.g.,
path.resolve(__dirname, '../../../uploads') or process.env.IMAGES_DIR), resolve
the incoming imagePath to an absolute canonical path using path.resolve(baseDir,
imagePath) (optionally fs.realpathSync on the resolved path), verify the
resolved canonical path is inside baseDir (use path.relative(baseDir,
resolvedPath).startsWith('..') or ensure resolvedPath.startsWith(baseDir +
path.sep) and reject the request if it escapes), and only call
findActualFilePath or any fs operations after this validation; return a 400/403
response when validation fails.
| <div className="absolute bottom-0 left-0 right-0 bg-black/60 px-1 py-0.5 opacity-0 group-hover:opacity-100 transition-opacity"> | ||
| <p className="text-[10px] text-white truncate">{file.filename}</p> | ||
| <p className="text-[9px] text-white/70">{formatFileSize(file.content.length)}</p> | ||
| </div> |
There was a problem hiding this comment.
Inaccurate file size display for text files.
formatFileSize(file.content.length) measures the JavaScript string length (number of UTF-16 code units), not the actual byte size. For files with multi-byte characters (e.g., UTF-8 encoded text with non-ASCII characters), this can be misleading. Consider storing the original byte size in FeatureTextFilePath or using new Blob([file.content]).size for accurate byte count.
🔎 Suggested fix
- <p className="text-[9px] text-white/70">{formatFileSize(file.content.length)}</p>
+ <p className="text-[9px] text-white/70">{formatFileSize(new Blob([file.content]).size)}</p>Alternatively, store the original file size when reading the text file in processFiles.
📝 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.
| <div className="absolute bottom-0 left-0 right-0 bg-black/60 px-1 py-0.5 opacity-0 group-hover:opacity-100 transition-opacity"> | |
| <p className="text-[10px] text-white truncate">{file.filename}</p> | |
| <p className="text-[9px] text-white/70">{formatFileSize(file.content.length)}</p> | |
| </div> | |
| <div className="absolute bottom-0 left-0 right-0 bg-black/60 px-1 py-0.5 opacity-0 group-hover:opacity-100 transition-opacity"> | |
| <p className="text-[10px] text-white truncate">{file.filename}</p> | |
| <p className="text-[9px] text-white/70">{formatFileSize(new Blob([file.content]).size)}</p> | |
| </div> |
🤖 Prompt for AI Agents
In apps/ui/src/components/ui/description-image-dropzone.tsx around lines 533 to
536, the displayed file size uses file.content.length which counts JS string
code units rather than actual bytes, causing wrong sizes for multi-byte text;
fix by using an accurate byte length (either add a byteSize property to
FeatureTextFilePath when files are read in processFiles and use that here, or
compute bytes on render via new Blob([file.content]).size) and replace
formatFileSize(file.content.length) with formatFileSize(file.byteSize) or
formatFileSize(new Blob([file.content]).size) accordingly.
| const dataUrl = await new Promise<string>((resolve) => { | ||
| const reader = new FileReader(); | ||
| reader.onload = (event) => resolve(event.target?.result as string); | ||
| reader.readAsDataURL(file); | ||
| }); |
There was a problem hiding this comment.
Missing error handling in FileReader promise.
The FileReader promise doesn't handle the onerror event. If the file read fails, the promise will never resolve or reject, potentially causing the upload to hang.
🔎 Suggested fix
const dataUrl = await new Promise<string>((resolve, reject) => {
const reader = new FileReader();
reader.onload = (event) => resolve(event.target?.result as string);
+ reader.onerror = () => reject(new Error('Failed to read file'));
reader.readAsDataURL(file);
});📝 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.
| const dataUrl = await new Promise<string>((resolve) => { | |
| const reader = new FileReader(); | |
| reader.onload = (event) => resolve(event.target?.result as string); | |
| reader.readAsDataURL(file); | |
| }); | |
| const dataUrl = await new Promise<string>((resolve, reject) => { | |
| const reader = new FileReader(); | |
| reader.onload = (event) => resolve(event.target?.result as string); | |
| reader.onerror = () => reject(new Error('Failed to read file')); | |
| reader.readAsDataURL(file); | |
| }); |
🤖 Prompt for AI Agents
In apps/ui/src/components/views/context-view.tsx around lines 337 to 341, the
Promise wrapping FileReader lacks error handling and never rejects on read
failures; add an onerror handler that rejects the promise with the reader error
(or a descriptive Error) and also clear/unset handlers in onload/onerror to
avoid leaks, so the promise resolves with the data URL on success and rejects
with an error on failure.
| const saveResult = await api.saveImageToTemp?.( | ||
| base64Data, | ||
| fileName, | ||
| mimeType, | ||
| currentProject!.path | ||
| ); |
There was a problem hiding this comment.
Non-null assertion on currentProject may cause runtime error.
While uploadFile is only called when files are dropped/imported, the currentProject!.path assertion at line 354 could cause a runtime error if currentProject becomes null between the initial check and the async operation. Consider adding a guard or using optional chaining with early return.
🔎 Suggested fix
+ if (!currentProject?.path) {
+ throw new Error('No project selected');
+ }
+
// Use saveImageToTemp to properly save as binary file in .automaker/images
const saveResult = await api.saveImageToTemp?.(
base64Data,
fileName,
mimeType,
- currentProject!.path
+ currentProject.path
);📝 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.
| const saveResult = await api.saveImageToTemp?.( | |
| base64Data, | |
| fileName, | |
| mimeType, | |
| currentProject!.path | |
| ); | |
| if (!currentProject?.path) { | |
| throw new Error('No project selected'); | |
| } | |
| const saveResult = await api.saveImageToTemp?.( | |
| base64Data, | |
| fileName, | |
| mimeType, | |
| currentProject.path | |
| ); |
🤖 Prompt for AI Agents
In apps/ui/src/components/views/context-view.tsx around lines 350-355, the code
uses a non-null assertion currentProject! when calling api.saveImageToTemp which
can race to null during the async await; capture the needed value before the
await (e.g., const projectPath = currentProject?.path) and if it's undefined
return early, or check currentProject and projectPath and throw/return before
calling api.saveImageToTemp, avoiding the non-null assertion and preventing a
runtime error.
| const content = await new Promise<string>((resolve) => { | ||
| const reader = new FileReader(); | ||
| reader.onload = (event) => resolve(event.target?.result as string); | ||
| reader.readAsText(file); | ||
| }); |
There was a problem hiding this comment.
Same FileReader error handling issue.
This FileReader promise also lacks error handling, similar to lines 337-341.
🔎 Suggested fix
const content = await new Promise<string>((resolve, reject) => {
const reader = new FileReader();
reader.onload = (event) => resolve(event.target?.result as string);
+ reader.onerror = () => reject(new Error('Failed to read file'));
reader.readAsText(file);
});📝 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.
| const content = await new Promise<string>((resolve) => { | |
| const reader = new FileReader(); | |
| reader.onload = (event) => resolve(event.target?.result as string); | |
| reader.readAsText(file); | |
| }); | |
| const content = await new Promise<string>((resolve, reject) => { | |
| const reader = new FileReader(); | |
| reader.onload = (event) => resolve(event.target?.result as string); | |
| reader.onerror = () => reject(new Error('Failed to read file')); | |
| reader.readAsText(file); | |
| }); |
🤖 Prompt for AI Agents
In apps/ui/src/components/views/context-view.tsx around lines 373 to 377, the
FileReader promise resolves on load but has no error handling; update the
Promise to attach reader.onerror (and optionally reader.onabort) to reject the
Promise with the error (or an Error with context) and ensure any event handlers
are cleaned up after load/error so callers can catch failures instead of hanging
or getting unexpected results.
- Introduced a new utility function `loadContextFiles` to load project-specific context files from the `.automaker/context/` directory, enhancing agent prompts with project rules and guidelines. - Updated `AgentService` and `AutoModeService` to utilize the new context loading functionality, combining context prompts with existing system prompts for improved agent performance. - Added comprehensive documentation on the context files system, including usage examples and metadata structure, to facilitate better understanding and implementation. - Removed redundant context loading logic from `AutoModeService`, streamlining the codebase. These changes aim to improve the agent's contextual awareness and adherence to project-specific conventions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/context-files-pattern.md (1)
11-17: Add language specifier to code block.The fenced code block should specify a language identifier for consistency with Markdown best practices.
🔎 Proposed fix
-``` +```text {projectPath}/.automaker/context/ ├── CLAUDE.md # Project rules and conventions ├── CODE_QUALITY.md # Code quality guidelines ├── context-metadata.json # File descriptions └── ... (any .md or .txt files)</details> </blockquote></details> <details> <summary>libs/utils/src/context-loader.ts (1)</summary><blockquote> `148-202`: **Solid implementation with minor type assertion consideration.** The `loadContextFiles` function is well-implemented with: - Graceful error handling (returns empty results if directory doesn't exist) - Case-insensitive file filtering for cross-platform compatibility - Per-file error handling to avoid failing the entire load - Clear logging of loaded files The type assertion at line 172 (`fsModule as typeof fs`) is needed because `loadContextMetadata` expects the full `fs` type. While functional, this could be improved by updating `loadContextMetadata` to accept the same minimal interface defined in `LoadContextFilesOptions`. <details> <summary>🔎 Optional refactor to eliminate type assertion</summary> Update `loadContextMetadata` to accept the minimal interface: ```diff async function loadContextMetadata( contextDir: string, - fsModule: typeof fs + fsModule: LoadContextFilesOptions['fsModule'] ): Promise<ContextMetadata> {Then remove the type assertion at line 172:
- const metadata = await loadContextMetadata(contextDir, fsModule as typeof fs); + const metadata = await loadContextMetadata(contextDir, fsModule);This would require updating the default parameter handling in
loadContextMetadataas well.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/server/src/services/agent-service.tsapps/server/src/services/auto-mode-service.tsdocs/context-files-pattern.mdlibs/utils/src/context-loader.tslibs/utils/src/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/server/src/services/agent-service.ts (3)
libs/utils/src/context-loader.ts (1)
loadContextFiles(148-202)libs/utils/src/index.ts (1)
loadContextFiles(50-50)apps/server/src/lib/sdk-options.ts (1)
createChatOptions(251-271)
🪛 markdownlint-cli2 (0.18.1)
docs/context-files-pattern.md
11-11: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (11)
docs/context-files-pattern.md (1)
1-170: Excellent documentation for the Context Files System!The documentation is comprehensive, well-organized, and provides clear examples that align with the actual implementation. The structure covers overview, directory layout, metadata format, shared utilities, usage patterns, and best practices.
apps/server/src/services/auto-mode-service.ts (3)
14-22: LGTM on import refactoring.The import changes correctly add
loadContextFilesfrom the centralized utility and remove the now-unusedgetContextDirimport. This aligns well with the refactoring to use shared context loading.
557-606: Context loading integration looks good.The refactored code correctly:
- Loads context files using the centralized
loadContextFilesutility- Passes the secure filesystem module for sandboxed operations
- Integrates the formatted context prompt as the system prompt
- Handles the case where no context files exist (empty string → undefined)
747-750: Consistent context loading in follow-up flow.The context loading implementation in
followUpFeaturefollows the same pattern asexecuteFeature, ensuring consistent behavior across different execution paths.Also applies to: 878-878
apps/server/src/services/agent-service.ts (2)
10-15: Import additions support new features.The added imports correctly include the new
loadContextFilesutility along with other utilities needed for enhanced message handling with images and context.
186-227: Context-aware prompt generation implemented correctly.The implementation properly:
- Determines the effective working directory from available sources
- Loads project context files using the centralized utility
- Combines context files prompt with the base system prompt (prepending context for higher priority)
- Passes the combined prompt to both SDK options and provider execution
This ensures agents have access to project-specific rules and conventions during chat sessions.
libs/utils/src/index.ts (1)
40-56: Export reorganization and context loader additions look good.The changes consolidate existing exports for better readability and add the new context loading functionality to the public API. The exposed types (
ContextMetadata,ContextFileInfo,ContextFilesResult,LoadContextFilesOptions) and functions (loadContextFiles,getContextFilesSummary) provide a clean interface for context file management.libs/utils/src/context-loader.ts (4)
19-53: Well-designed type definitions for context loading.The type definitions provide a clean interface for context file management:
ContextMetadatastructures file descriptionsContextFileInfocaptures individual file dataLoadContextFilesOptionsenables dependency injection viafsModuleThe minimal
fsModuleinterface allows secure filesystem implementations while maintaining type safety.
97-123: Excellent prompt formatting with clear instructions.The
buildContextPromptfunction creates a well-structured system prompt that:
- Clearly explains the purpose of context files
- Uses bold and emphasis to highlight critical requirements
- Provides specific examples (e.g., pnpm vs npm)
- Separates file entries clearly with markdown dividers
- Reinforces the importance with a reminder at the end
This formatting ensures agents understand the significance of following project-specific conventions.
209-238: Useful summary function for lightweight context queries.The
getContextFilesSummaryfunction provides a way to discover available context files without loading their full content. This is helpful for informing users about available context or for UI displays.The implementation follows the same patterns as
loadContextFileswith consistent error handling.
162-165: Case-insensitive filtering handles cross-platform compatibility well.The file filtering logic correctly:
- Uses lowercased filename for extension checks (
.md,.txt)- Uses original filename for exact comparison (
context-metadata.json)- Ensures consistent behavior across case-sensitive and case-insensitive filesystems
…ing improvements - Implemented path validation against ALLOWED_ROOT_DIRECTORY to prevent arbitrary file reads and prompt injection attacks. - Added error handling for file reading, including specific responses for forbidden paths and file not found scenarios. - Updated the description generation logic to truncate large files and provide structured prompts for analysis. - Enhanced logging for better traceability of file access and errors. These changes aim to improve the security and reliability of the file description functionality.
- Added a mock for the `loadContextFiles` function to return an empty context by default, improving test reliability. - Updated the agent-service test suite to ensure proper initialization of the `AgentService` with mocked dependencies. These changes aim to enhance the test coverage and stability of the agent-service functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ui/src/components/views/agent-view.tsx (2)
334-361: Extend paste handler to support text files.The
handlePastefunction only processes image files (line 346), butprocessDroppedFilesaccepts both images and text files. This creates an inconsistent user experience where text files can be drag-and-dropped but not pasted.🔎 Proposed fix to support text files in paste handler
const handlePaste = useCallback( async (e: React.ClipboardEvent) => { // Check if clipboard contains files const items = e.clipboardData?.items; if (items) { const files: File[] = []; for (let i = 0; i < items.length; i++) { const item = items[i]; if (item.kind === 'file') { const file = item.getAsFile(); - if (file && file.type.startsWith('image/')) { + if (file && (file.type.startsWith('image/') || isTextFile(file))) { e.preventDefault(); // Prevent default paste of file path files.push(file); } } } if (files.length > 0) { const dataTransfer = new DataTransfer(); files.forEach((file) => dataTransfer.items.add(file)); await processDroppedFiles(dataTransfer.files); } } }, [processDroppedFiles] );
668-702: Add text file attachments display in message history.The message rendering currently only displays image attachments (lines 668-702), but does not render text file attachments. Users who send messages with text files won't be able to see which text files were attached to previous messages in the conversation history.
🔎 Proposed fix to display text files in messages
Add text file rendering after the images section:
{/* Display attached images for user messages */} {message.role === 'user' && message.images && message.images.length > 0 && ( <div className="mt-3 space-y-2"> <div className="flex items-center gap-1.5 text-xs text-primary-foreground/80"> <ImageIcon className="w-3 h-3" /> <span> {message.images.length} image {message.images.length > 1 ? 's' : ''} attached </span> </div> <div className="flex flex-wrap gap-2"> {message.images.map((image, index) => { // Construct proper data URL from base64 data and mime type const dataUrl = image.data.startsWith('data:') ? image.data : `data:${image.mimeType || 'image/png'};base64,${image.data}`; return ( <div key={image.id || `img-${index}`} className="relative group rounded-lg overflow-hidden border border-primary-foreground/20 bg-primary-foreground/10" > <img src={dataUrl} alt={image.filename || `Attached image ${index + 1}`} className="w-20 h-20 object-cover hover:opacity-90 transition-opacity" /> <div className="absolute bottom-0 left-0 right-0 bg-black/50 px-1.5 py-0.5 text-[9px] text-white truncate"> {image.filename || `Image ${index + 1}`} </div> </div> ); })} </div> </div> )} + + {/* Display attached text files for user messages */} + {message.role === 'user' && message.textFiles && message.textFiles.length > 0 && ( + <div className="mt-3 space-y-2"> + <div className="flex items-center gap-1.5 text-xs text-primary-foreground/80"> + <FileText className="w-3 h-3" /> + <span> + {message.textFiles.length} text file + {message.textFiles.length > 1 ? 's' : ''} attached + </span> + </div> + <div className="flex flex-wrap gap-2"> + {message.textFiles.map((file, index) => ( + <div + key={file.id || `file-${index}`} + className="rounded-lg border border-primary-foreground/20 bg-primary-foreground/10 px-2 py-1.5 flex items-center gap-2" + > + <FileText className="w-3 h-3 text-primary-foreground/80" /> + <div> + <p className="text-[10px] font-medium text-primary-foreground"> + {file.filename} + </p> + <p className="text-[9px] text-primary-foreground/70"> + {formatFileSize(file.size)} + </p> + </div> + </div> + ))} + </div> + </div> + )}Note: You'll also need to ensure the Message type in the store includes a
textFilesfield.
♻️ Duplicate comments (6)
apps/ui/src/components/views/context-view.tsx (5)
372-375: Storing images as data URLs in context directory is inefficient.Saving images as data URLs (base64 text) in
.automaker/contextcreates large text files rather than actual binary images. This duplicates storage (also saved in.automaker/images) and could cause confusion if other code expects binary image data.Consider storing only a reference (e.g., relative path to the images directory) in the context entry instead of the full data URL.
345-349: FileReader promise lacks error handling.The promise will hang indefinitely if the file read fails. Add an
onerrorhandler:const dataUrl = await new Promise<string>((resolve, reject) => { const reader = new FileReader(); reader.onload = (event) => resolve(event.target?.result as string); + reader.onerror = () => reject(new Error('Failed to read file')); reader.readAsDataURL(file); });
358-363: Non-null assertion oncurrentProjectcould cause runtime error.The
currentProject!.pathassertion is unsafe ifcurrentProjectbecomes null during the async operation. Add a guard:+ if (!currentProject?.path) { + throw new Error('No project selected'); + } + const saveResult = await api.saveImageToTemp?.( base64Data, fileName, mimeType, - currentProject!.path + currentProject.path );
381-385: Same FileReader error handling issue.This
FileReaderpromise also lacks error handling, similar to lines 345-349.const content = await new Promise<string>((resolve, reject) => { const reader = new FileReader(); reader.onload = (event) => resolve(event.target?.result as string); + reader.onerror = () => reject(new Error('Failed to read file')); reader.readAsText(file); });
467-470: File overwrite risk remains unaddressed.The function writes without checking if the file already exists. A user could accidentally overwrite an existing context file.
const filePath = `${contextPath}/${filename}`; + // Check if file already exists + const exists = await api.exists(filePath); + if (exists) { + toast.error(`A file named "${filename}" already exists.`); + return; + } + // Write markdown file await api.writeFile(filePath, newMarkdownContent);apps/ui/src/components/views/agent-view.tsx (1)
26-39: Past review comment has been addressed.The
formatFileSizefunction is now imported from@/lib/image-utilsinstead of being duplicated locally. The local implementation has been removed.
🧹 Nitpick comments (2)
apps/server/src/routes/context/routes/describe-file.ts (1)
170-176: Consider removing unused sandbox configuration.Since
allowedTools: []prevents any tool usage, thesandboxconfiguration has no effect. Consider removing it for clarity:const sdkOptions = createCustomOptions({ cwd, model: CLAUDE_MODEL_MAP.haiku, maxTurns: 1, allowedTools: [], - sandbox: { enabled: true, autoAllowBashIfSandboxed: true }, });apps/ui/src/components/views/agent-view.tsx (1)
190-202: Improve error logging in catch blocks.The catch blocks for file processing (text files at lines 200-202 and images at lines 233-235) silently swallow errors without logging the actual error details. This makes debugging failures difficult.
🔎 Proposed fix
try { const content = await fileToText(file); const textFileAttachment: TextFileAttachment = { id: generateFileId(), content, mimeType: getTextFileMimeType(file.name), filename: file.name, size: file.size, }; newTextFiles.push(textFileAttachment); - } catch { + } catch (error) { + console.error(`Failed to read text file ${file.name}:`, error); errors.push(`${file.name}: Failed to read text file.`); }Apply similar fix for image processing:
try { const base64 = await fileToBase64(file); const imageAttachment: ImageAttachment = { id: generateImageId(), data: base64, mimeType: file.type, filename: file.name, size: file.size, }; newImages.push(imageAttachment); - } catch { + } catch (error) { + console.error(`Failed to process image ${file.name}:`, error); errors.push(`${file.name}: Failed to process image.`); }Also applies to: 223-235
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/server/src/routes/context/routes/describe-file.tsapps/server/src/routes/context/routes/describe-image.tsapps/server/tests/unit/services/agent-service.test.tsapps/ui/src/components/views/agent-view.tsxapps/ui/src/components/views/context-view.tsxapps/ui/tests/utils/navigation/views.tslibs/types/src/model.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/server/src/routes/context/routes/describe-image.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/server/src/routes/context/routes/describe-file.ts (3)
apps/server/src/lib/secure-fs.ts (1)
secureFs(8-23)apps/server/src/lib/sdk-options.ts (1)
createCustomOptions(306-326)libs/types/src/index.ts (1)
CLAUDE_MODEL_MAP(36-36)
apps/ui/src/components/views/agent-view.tsx (3)
apps/ui/src/store/app-store.ts (2)
TextFileAttachment(219-225)ImageAttachment(211-217)apps/server/src/services/agent-service.ts (1)
sendMessage(113-357)apps/ui/src/lib/image-utils.ts (11)
isTextFile(153-158)validateTextFile(177-201)DEFAULT_MAX_FILES(27-27)fileToText(209-222)generateFileId(143-145)getTextFileMimeType(230-236)validateImageFile(105-127)DEFAULT_MAX_FILE_SIZE(21-21)fileToBase64(59-72)generateImageId(134-136)formatFileSize(90-96)
⏰ 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). (1)
- GitHub Check: e2e
🔇 Additional comments (15)
libs/types/src/model.ts (1)
5-7: Model identifiers are current and properly formatted for production use.The model IDs follow Anthropic's recommended format with specific snapshot dates. All three models (Haiku 4.5, Sonnet 4.5, Opus 4.5) are valid and align with their official release dates. This approach is correct and no changes are needed.
apps/server/tests/unit/services/agent-service.test.ts (1)
7-7: LGTM!The new
contextLoaderimport and mock setup are consistent with the existing patterns forimageHandlerandpromptBuilder. The default empty context return value is appropriate for isolating unit tests.Also applies to: 24-29
apps/ui/tests/utils/navigation/views.ts (1)
27-37: LGTM!The loading guard pattern is consistent with
navigateToSpecand appropriately handles the optional loading state. The timeouts are reasonable, and the try/catch correctly handles cases where the loading element is absent or already hidden.apps/server/src/routes/context/routes/describe-file.ts (3)
1-20: Well-structured security approach.The endpoint correctly uses
secureFsfor path validation and reads file content directly rather than via Claude's Read tool, addressing the security concerns from the previous review. The imports are clean and the module organization is appropriate.
101-140: Good error handling.The error handling comprehensively covers path validation (403), file not found (404), and generic errors (500) with appropriate status codes and informative error messages.
64-66: The edge case concern about empty string overwriting accumulated text is not valid. In JavaScript, empty strings are falsy values, somsg.result || responseTextcorrectly falls back toresponseTextwhenmsg.resultis"",null, orundefined. The current logic is sound.apps/ui/src/components/views/context-view.tsx (5)
59-61: LGTM on metadata structure.The
ContextMetadatainterface provides a clean structure for persisting per-file descriptions.
134-167: LGTM!The metadata loading and saving functions handle errors gracefully with appropriate fallback to empty metadata when the file doesn't exist or is malformed.
286-322: Well-structured async description generation flow.The
generateDescriptionAsyncfunction properly manages the loading state set, updates metadata, refreshes the file list, and updates the selected file if applicable. The error handling and cleanup in thefinallyblock ensure the UI state stays consistent.
753-819: Clean file list implementation.The file list with dropdown menus properly handles click propagation, shows generating state, and displays descriptions. The conditional rendering for the loading indicator vs description is well structured.
1-50: Clean imports and component setup.The imports are well-organized, and the new UI components (DropdownMenu, Textarea) are properly integrated for the new features.
apps/ui/src/components/views/agent-view.tsx (4)
58-58: LGTM!The
selectedTextFilesstate is correctly initialized and typed consistently with the existingselectedImagespattern.
765-853: LGTM!The file preview section correctly handles both image and text file attachments with consistent styling and functionality. The clear all and individual remove buttons work for both file types.
869-932: LGTM!The input area and buttons correctly reflect support for both images and text files. The placeholder text, button labels, and disabled states appropriately handle the combined file attachment functionality.
134-151: sendMessage correctly accepts text file attachments.The
sendMessagefunction fromuseElectronAgenthas been updated to accept text file attachments. The function signature includes thetextFiles?: TextFileAttachment[]parameter, making the call inhandleSendcorrect.
1 participant
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.