-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Upload image via chat #686
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cubic analysis
2 issues found across 13 files • Review in cubic
React with 👍 or 👎 to teach cubic. You can also tag @cubic-dev-ai
to give feedback, ask questions, or re-run the review.
for (const [fileId, fileInfo] of fileUploadsMap.entries()) { | ||
if (content.includes(fileId)) { | ||
try { | ||
const fileContent = await readFile(fileInfo.filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fs.promises.readFile
is called without an encoding, returning a Buffer even for text files; this causes later logic that expects a string to be bypassed.
Prompt for AI agents
Address the following comment on src/ipc/processors/response_processor.ts at line 307:
<comment>`fs.promises.readFile` is called without an encoding, returning a Buffer even for text files; this causes later logic that expects a string to be bypassed.</comment>
<file context>
@@ -289,9 +296,33 @@ export async function processFullResponseActions(
// Process all file writes
for (const tag of dyadWriteTags) {
const filePath = tag.path;
- const content = tag.content;
+ let content: string | Buffer = tag.content;
const fullFilePath = safeJoin(appPath, filePath);
+ // Check if content contains file IDs and replace with actual file content
+ if (fileUploadsMap) {
</file context>
const fileContent = await readFile(fileInfo.filePath); | |
const fileContent = await readFile(fileInfo.filePath, "utf8"); |
src={URL.createObjectURL(attachment.file)} | ||
alt={attachment.file.name} | ||
className="max-w-[200px] max-h-[200px] object-contain bg-white p-1 rounded shadow-lg" | ||
onLoad={(e) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Object URL is not revoked when the large preview image fails to load, leading to a potential memory leak.
Prompt for AI agents
Address the following comment on src/components/chat/AttachmentsList.tsx at line 47:
<comment>Object URL is not revoked when the large preview image fails to load, leading to a potential memory leak.</comment>
<file context>
@@ -13,40 +14,47 @@ export function AttachmentsList({
return (
<div className="px-2 pt-2 flex flex-wrap gap-1">
- {attachments.map((file, index) => (
+ {attachments.map((attachment, index) => (
<div
key={index}
className="flex items-center bg-muted rounded-md px-2 py-1 text-xs gap-1"
- title={`${file.name} (${(file.size / 1024).toFixed(1)}KB)`}
</file context>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Race Condition in Singleton File Upload Tracker
The FileUploadsState
singleton has a race condition. It only tracks file uploads for a single chat ID. When multiple chat streams are processed concurrently, an initialize
call from one chat overwrites the currentChatId
and clears the fileUploadsMap
of another. This causes getFileUploadsForChat
to return an empty map for the original chat, leading to file upload failures.
src/ipc/utils/file_uploads_state.ts#L9-L66
dyad/src/ipc/utils/file_uploads_state.ts
Lines 9 to 66 in a3d7435
export class FileUploadsState { | |
private static instance: FileUploadsState; | |
private currentChatId: number | null = null; | |
private fileUploadsMap = new Map<string, FileUploadInfo>(); | |
private constructor() {} | |
public static getInstance(): FileUploadsState { | |
if (!FileUploadsState.instance) { | |
FileUploadsState.instance = new FileUploadsState(); | |
} | |
return FileUploadsState.instance; | |
} | |
/** | |
* Initialize file uploads state for a specific chat and message | |
*/ | |
public initialize({ chatId }: { chatId: number }): void { | |
this.currentChatId = chatId; | |
this.fileUploadsMap.clear(); | |
logger.debug(`Initialized file uploads state for chat ${chatId}`); | |
} | |
/** | |
* Add a file upload mapping | |
*/ | |
public addFileUpload(fileId: string, fileInfo: FileUploadInfo): void { | |
this.fileUploadsMap.set(fileId, fileInfo); | |
logger.log(`Added file upload: ${fileId} -> ${fileInfo.originalName}`); | |
} | |
/** | |
* Get the current file uploads map | |
*/ | |
public getFileUploadsForChat(chatId: number): Map<string, FileUploadInfo> { | |
if (this.currentChatId !== chatId) { | |
return new Map(); | |
} | |
return new Map(this.fileUploadsMap); | |
} | |
/** | |
* Get current chat ID | |
*/ | |
public getCurrentChatId(): number | null { | |
return this.currentChatId; | |
} | |
/** | |
* Clear the current state | |
*/ | |
public clear(): void { | |
this.currentChatId = null; | |
this.fileUploadsMap.clear(); | |
logger.debug("Cleared file uploads state"); | |
} | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
No description provided.