Conversation
|
@BugBot run |
Summary of ChangesHello, 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 introduces a significant new 'Media Library' feature, enhancing user interaction with application assets. It integrates AI-powered image generation directly into the workflow and refines the overall library experience with improved navigation, search, and display for all content types. The changes provide a more cohesive and powerful environment for managing and utilizing creative assets within the application. Highlights
Changelog
Activity
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
|
|
@BugBot run |
![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 comprehensive Media Library feature, including UI components for browsing, managing (rename, move, delete), and generating images, as well as backend handlers and e2e tests. The implementation is robust, with thoughtful additions like file operation locking to prevent race conditions and a unified library view. I've identified a couple of high-severity bugs in the new UI components related to incorrect CSS class definitions that will cause styling issues. Once these are addressed, this will be an excellent addition to the application.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (5 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
github-actions
bot
added
the
needs-human:review-issue
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All review comments have been addressed with code changes, but full E2E testing is recommended before merge given the breadth of changes across rendering, IPC, and accessibility. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions — principles were clear enough for all decisions. 🤖 Generated by Claude Code |
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (8 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: 🤔 NOT SURE - Potential issues Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Note: 21 issues were already flagged by previous reviews. This summary covers only NEW findings. Issues Summary
🟢 Low Priority Notes (6 items)
🚫 Dropped False Positives (5 items)
Generated by Dyadbot multi-agent code review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 3/5Nine review comments addressed with code changes; six threads flagged for human review due to architectural decisions needed (mention format redesign, Pro user gating, performance optimization). Unresolved Threads
Resolved Threads
Product Principle SuggestionsThe following suggestions could improve rules/product-principles.md to help resolve ambiguous cases in the future:
🤖 Generated by Claude Code |
🎭 Playwright Test Results❌ Some tests failed
Summary: 248 passed, 2 failed, 11 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/media_library.spec.ts \
e2e-tests/undo.spec.ts
|
- Add data: URI blocking to SVG sanitizer (defense-in-depth) - Add Content-Security-Policy header for SVG responses - Add MAX_IMAGE_SIZE check for base64-decoded images - Fix e2e test selector (remove non-existent data attribute) - Move INVALID_FILE_NAME_CHARS import to top of DyadAppMediaFolder - Remove redundant .catch() on move operation - Convert sync fs calls to async (rename, unlink, copyFile) - Use buildDyadMediaUrl instead of inline URL construction - Add reduced-motion support for library landing animation Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All actionable review comments addressed; one thread flagged for human review on a product decision (slash command hint in LibraryCard). Unresolved Threads
Resolved Threads
Product Principle SuggestionsThe following suggestions could improve
🤖 Generated by Claude Code |
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
| chat.app.path, | ||
| chat.app.name, | ||
| ); | ||
| const resolvedMediaRefs = resolvedMedia.map((media) => |
There was a problem hiding this comment.
🟡 MEDIUM | data-integrity
buildDyadMediaUrl receives chat.app.path (relative) but media URL builder expects an absolute app path
The buildDyadMediaUrl function at src/lib/dyadMediaUrl.ts encodes appPath directly into the dyad-media:// URL. Here it receives chat.app.path (the DB-stored relative path), but resolveMediaMentions above already calls getDyadAppPath(appPath) internally to resolve the absolute path. This means the display URL and the resolved file path may use different path formats, causing the thumbnail in the chat UI to fail to load.
💡 Suggestion: Use the absolute media.filePath's parent directory or call getDyadAppPath(chat.app.path) before passing to buildDyadMediaUrl, consistent with how ImagePreview and DyadAppMediaFolder use absolute paths.
|
|
||
| function MediaFileThumbnail({ | ||
| file, | ||
| appPath, |
There was a problem hiding this comment.
🟡 MEDIUM | correctness
handleStartNewChatWithImage encodes filename but display text in chat shows encoded %20 etc.
When starting a new chat with an image, prefillInput is set to @media:${encodeURIComponent(file.fileName)}. The ExternalValueSyncPlugin in LexicalChatInput.tsx then strips @media: and decodes it for display. However, the e2e test at media_library.spec.ts:162 asserts the chat input toContainText('@chat-image.png') — this works for simple names but filenames with spaces (e.g., my photo.png) would show as @my photo.png in the editor while the internal value has @media:my%20photo.png. If the user edits the text, the round-trip encoding could break because the onChange handler looks for exact filename matches to re-encode, but the displayed text has already been decoded.
💡 Suggestion: Consider adding an e2e test case with a filename containing spaces to verify the full round-trip works correctly.
| // Remove <foreignObject> elements (can embed arbitrary HTML) | ||
| .replace(/<foreignObject[\s>][\s\S]*?<\/foreignObject>/gi, "") | ||
| // Remove on* event handler attributes | ||
| .replace(/\s+on\w+\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]+)/gi, "") |
There was a problem hiding this comment.
🟡 MEDIUM | security
SVG sanitizer regex can be bypassed with HTML entity encoding or newlines in attribute values
The on\w+ event handler removal regex uses (?:"[^"]*"|'[^']*'|[^\s>]+) to match attribute values, but this doesn't handle cases where the attribute value contains newlines or HTML entities like javascript:. Also, <script tags can be obfuscated with unusual whitespace (e.g., <script\n>) — while the [\s>] pattern handles this, SVG payloads could use <script/src="..."> (self-closing with attributes) which is not covered.
Note: This is distinct from the existing comments about data: URIs. The existing comments flagged data: URI bypass and general regex fragility. This specifically calls out the entity-encoding bypass vector in event handlers.
💡 Suggestion: The CSP script-src 'none' header added in main.ts is a good defense-in-depth layer. Consider documenting that the regex sanitizer is a best-effort layer and the CSP header is the primary security control. For higher assurance, consider using a proper HTML/SVG parser like DOMPurify.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge (1 HIGH issue) Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard.
Issues Summary
🟢 Low Priority Notes (2 items)
🚫 Dropped / Already Covered (many items)The following categories of issues were identified but already have existing comments on this PR:
These are not re-flagged since they are already tracked. Generated by Dyadbot multi-agent code review |
|
|
||
| if (imageData.b64_json) { | ||
| const buffer = Buffer.from(imageData.b64_json, "base64"); | ||
| const MAX_IMAGE_SIZE = 50 * 1024 * 1024; // 50 MB |
There was a problem hiding this comment.
🟡 MEDIUM | duplication
MAX_IMAGE_SIZE constant declared twice in same function
const MAX_IMAGE_SIZE = 50 * 1024 * 1024 appears identically on both the b64_json branch (line 128) and the URL branch (line 145). If the limit needs to change, both must be updated.
💡 Suggestion: Hoist the constant above the if/else block so it's declared once.
| setActiveFilter("prompts"); | ||
| setPromptDialogOpen(true); | ||
| clearLastDeepLink(); | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM | correctness
useEffect dependency on lastDeepLink?.timestamp is fragile
The effect body reads lastDeepLink and casts it as unknown as AddPromptDeepLinkData, but the dependency array only lists lastDeepLink?.timestamp and clearLastDeepLink. If lastDeepLink changes without its timestamp changing, the effect won't re-run. The double cast (as unknown as) also bypasses type safety.
💡 Suggestion: Include lastDeepLink in the dependency array and use a proper type guard instead of the double cast.
| * Sanitize SVG content by stripping elements and attributes that could | ||
| * execute scripts in Electron (e.g., <script>, on* handlers, javascript: URIs). | ||
| */ | ||
| export function sanitizeSvgContent(raw: string): string { |
There was a problem hiding this comment.
🔴 HIGH | security
SVG sanitization bypass via <animate> / <set> elements
The regex-based sanitizer can be bypassed using SVG animation elements like:
<a><animate attributeName="href" values="javascript:alert(1)" /></a>This dynamically sets the href attribute to a javascript: URI without matching any of the existing regex patterns (which only check static href attributes). While the Content-Security-Policy: script-src 'none' header in main.ts is a defense-in-depth layer, CSP script-src doesn't reliably block javascript: URI navigation in all Electron contexts.
This is distinct from the existing comments about data: URI bypass and general regex fragility — this is a specific, exploitable vector.
💡 Suggestion: Strip <animate> and <set> elements that target href/xlink:href/on* attributes. Better yet, use DOMPurify for proper SVG sanitization.
| } | ||
|
|
||
| // Resolve @media: mentions to image attachments | ||
| const mediaRefs = parseMediaMentions(userPrompt); |
There was a problem hiding this comment.
🟡 MEDIUM | correctness
parseMediaMentions runs on already-transformed userPrompt
By this point, userPrompt has already been through prompt reference replacement (replacePromptReference) and slash skill expansion (replaceSlashSkillReference). If any of those expansions introduce text containing @media: tokens, they would be falsely detected as media mentions — potentially attaching unexpected files or stripping text from the prompt.
💡 Suggestion: Parse media mentions from req.prompt (original input) before any transformations, or move media mention parsing to run first in the pipeline.
🤖 Claude Code Review SummaryPR Confidence: 4/5All review threads have been addressed and resolved; no outstanding code issues remain. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions (principles were clear enough for all decisions) 🤖 Generated by Claude Code |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@BugBot run |
| .replace(/<script[\s>][\s\S]*?<\/script>/gi, "") | ||
| // Remove self-closing <script/> tags |
There was a problem hiding this comment.
HIGH | security
SVG sanitizer fails to remove <script> tags without a closing </script>
The regex /<script[\s>][\s\S]*?<\/script>/gi requires a matching </script> tag. An SVG file containing an unclosed <script> tag (e.g., <script>alert(document.cookie) at the end of the file, or <script>/*) will not be matched and the tag will survive sanitization.
While the Content-Security-Policy: script-src 'none' header on the protocol response provides defense-in-depth, CSP enforcement depends on the rendering context. If the sanitized SVG content is ever used in a context where CSP isn't applied (e.g., stored and later served differently), this becomes exploitable.
Additionally, the regex can be bypassed with HTML comments inside the script: <script>alert(1)</script> — the non-greedy *? would match <script>alert(1)</script> in the output.
Suggestion: Add a catch-all regex for unclosed script tags after the existing ones:
| .replace(/<script[\s>][\s\S]*?<\/script>/gi, "") | |
| // Remove self-closing <script/> tags | |
| .replace(/<script[\s>][\s\S]*?<\/script>/gi, "") | |
| // Remove self-closing <script/> tags | |
| .replace(/<script\s*\/>/gi, "") | |
| // Remove unclosed <script> tags (no matching </script>) | |
| .replace(/<script[\s>][\s\S]*/gi, "") |
Or better, use a proper DOM parser (like DOMParser or linkedom) for SVG sanitization instead of regex.
| // Convert media mentions : @filename -> @media:filename | ||
| const currentAppMediaFiles = mediaApps.find( | ||
| (app) => app.appId === selectedAppId, | ||
| ); | ||
| if (currentAppMediaFiles) { | ||
| for (const file of currentAppMediaFiles.files) { | ||
| const escaped = file.fileName.replace( | ||
| /[.*+?^${}()|[\]\\]/g, | ||
| "\\$&", | ||
| ); | ||
| const mediaRegex = new RegExp(`@(${escaped})(?![\\w-])`, "g"); | ||
| textContent = textContent.replace( | ||
| mediaRegex, | ||
| `@media:${encodeURIComponent(file.fileName)}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
MEDIUM | correctness
Media mention conversion can be corrupted by subsequent app name conversion
Media mentions are converted first (@filename -> @media:encodedFilename), then app name conversion runs. If an app happens to be named media, the app regex @(media)(?![a-zA-Z0-9_/\\-]) would match the @media prefix in the already-converted @media:filename.png (since : is NOT in the negative lookahead character class), turning it into @app:media:filename.png and breaking the media mention.
Suggestion: After converting media mentions, the app name conversion regex should exclude matches that are followed by : (i.e., already-prefixed mentions). Change the app name regex negative lookahead to also include ::
const mentionRegex = new RegExp(
`@(${escapedAppName})(?![a-zA-Z0-9_/:\\-])`,
"g",
);Or more robustly, skip app name conversion for any @app:, @media:, @prompt:, @file: prefixed tokens.
| ); | ||
| throw new Error( | ||
| `Image generation failed (HTTP ${response.status}). Please try again.`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
MEDIUM | data-integrity
Unvalidated JSON response from external API can cause runtime crash
response.json() is called without a try/catch, and then data.data?.[0] is accessed. If the response body is not valid JSON (e.g., HTML error page, truncated response), response.json() will throw an unhandled error that propagates as a generic "object is not valid" error rather than a user-friendly message.
Suggestion: Wrap in try/catch:
let data: any;
try {
data = await response.json();
} catch {
throw new Error("Invalid response from image generation service. Please try again.");
}
| throw new Error("Unsupported media file extension"); | ||
| } | ||
|
|
||
| return extension; | ||
| } | ||
|
|
||
| function getMediaFilePath(appPath: string, fileName: string): string { | ||
| assertSafeFileName(fileName); | ||
| assertSupportedMediaExtension(fileName); | ||
| return safeJoin(appPath, DYAD_MEDIA_DIR_NAME, fileName); | ||
| } | ||
|
|
||
| function getMediaDirectoryPath(appPath: string): string { | ||
| return path.join(appPath, DYAD_MEDIA_DIR_NAME); | ||
| } | ||
|
|
||
| async function getAppOrThrow(appId: number) { | ||
| const app = await db.query.apps.findFirst({ | ||
| where: eq(apps.id, appId), | ||
| }); | ||
|
|
||
| if (!app) { | ||
| throw new Error("App not found"); | ||
| } |
There was a problem hiding this comment.
MEDIUM | race-condition
TOCTOU race between existsSync check and rename operation
The rename handler checks fs.existsSync(sourcePath) and later fs.existsSync(destinationPath) before calling fs.promises.rename(). Between the existence check and the rename, another process (or a concurrent IPC call to a different handler that doesn't acquire the same lock) could create a file at destinationPath or delete the source file.
While withMediaLock serializes operations for the same appId, the lock is app-scoped. External processes (the user's file manager, git operations, the app's dev server) are not coordinated and could modify files concurrently.
Suggestion: Use fs.promises.rename() directly and handle the resulting ENOENT/EEXIST errors, or use fs.promises.link() + fs.promises.unlink() with exclusive flags for atomicity. At minimum, document that the lock only protects against concurrent IPC calls, not external modifications.
| if (files.length > 0) { | ||
| result.push({ | ||
| appId: app.id, | ||
| appName: app.name, | ||
| appPath, | ||
| files, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return { apps: result }; | ||
| }); | ||
|
|
||
| createTypedHandler(mediaContracts.renameMediaFile, async (_, params) => { | ||
| await withMediaLock([params.appId], async () => { | ||
| const app = await getAppOrThrow(params.appId); | ||
| const appPath = getDyadAppPath(app.path); | ||
|
|
There was a problem hiding this comment.
MEDIUM | data-integrity
Cross-device move can silently leave source file behind on copyFile failure
The move operation uses copyFile + unlink for cross-device support, which is correct. However, there's a subtle issue: if copyFile partially writes the destination (e.g., disk full during copy) and then throws, the partially-written destination file is NOT cleaned up. The source file is intact, but the destination has a corrupted partial copy that could confuse the user if they look at the target app's media folder.
Suggestion: Wrap the copyFile call similarly to how the unlink failure is handled — add a cleanup of destinationPath in a catch block around copyFile:
try {
await fs.promises.copyFile(sourcePath, destinationPath);
} catch (copyErr) {
// Clean up partial destination on copy failure
try { await fs.promises.unlink(destinationPath); } catch {}
throw copyErr;
}
| // Resolve @media: mentions to image attachments | ||
| const mediaRefs = parseMediaMentions(userPrompt); | ||
| if (mediaRefs.length > 0) { | ||
| try { | ||
| const resolvedMedia = await resolveMediaMentions( | ||
| mediaRefs, | ||
| chat.app.path, | ||
| chat.app.name, | ||
| ); | ||
| const resolvedMediaRefs = resolvedMedia.map((media) => | ||
| encodeURIComponent(media.fileName), | ||
| ); | ||
| let mediaDisplayInfo = ""; | ||
| for (const media of resolvedMedia) { | ||
| attachmentPaths.push(media.filePath); | ||
| const mediaUrl = buildDyadMediaUrl(chat.app.path, media.fileName); | ||
| mediaDisplayInfo += `\n<dyad-attachment name="${escapeXmlAttr(media.fileName)}" type="${escapeXmlAttr(media.mimeType)}" url="${escapeXmlAttr(mediaUrl)}" path="${escapeXmlAttr(media.filePath)}" attachment-type="chat-context"></dyad-attachment>\n`; | ||
| } | ||
| // Strip only resolved @media: tags from the prompt text. | ||
| // This preserves adjacent user text when mentions are directly followed | ||
| // by text without a whitespace separator. | ||
| userPrompt = stripResolvedMediaMentions( | ||
| userPrompt, | ||
| resolvedMediaRefs, | ||
| ); | ||
| // Build display prompt with attachment tags for inline rendering. | ||
| if (mediaDisplayInfo) { | ||
| const strippedPrompt = stripResolvedMediaMentions( | ||
| displayUserPrompt ?? req.prompt, | ||
| resolvedMediaRefs, | ||
| ); | ||
| displayUserPrompt = strippedPrompt + mediaDisplayInfo; | ||
| } | ||
| } catch (e) { | ||
| logger.error("Failed to resolve media mentions:", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
MEDIUM | correctness
Media resolution silently succeeds with zero resolved files, leaving raw @media: tags in prompt
When parseMediaMentions finds media refs but resolveMediaMentions fails to resolve ANY of them (e.g., files were deleted between mention creation and message send), resolvedMedia is an empty array. In this case:
resolvedMediaRefsis emptystripResolvedMediaMentionswith an empty array returns the prompt unchanged (just trimmed)- The raw
@media:filename.pngtext is sent to the AI model as-is
This means the AI receives garbled @media: prefixed text in the prompt instead of either the image attachment or a user-friendly error message.
Suggestion: When resolvedMedia.length === 0 but mediaRefs.length > 0, either warn the user that referenced media files were not found, or strip the unresolved @media: tokens from the prompt to avoid confusing the AI model.
| export function stripResolvedMediaMentions( | ||
| prompt: string, | ||
| resolvedMediaRefs: string[], | ||
| ): string { | ||
| if (resolvedMediaRefs.length === 0) { | ||
| return prompt.trim(); | ||
| } | ||
|
|
||
| let stripped = prompt; | ||
| for (const mediaRef of resolvedMediaRefs) { | ||
| const token = `@media:${mediaRef}`; | ||
| stripped = stripped.split(token).join(""); | ||
| } | ||
|
|
||
| return stripped.replace(/\s{2,}/g, " ").trim(); |
There was a problem hiding this comment.
LOW | correctness
stripResolvedMediaMentions uses string splitting which is vulnerable to substring collisions
stripped.split(token).join("") performs a literal string replacement of ALL occurrences. If a user's message text happens to contain the literal string @media:cat.png as part of a code block, documentation, or discussion about the media mention syntax, it would be incorrectly stripped.
This is unlikely in normal usage but could occur in edge cases where users are discussing the mention system.
Suggestion: Only strip the first occurrence per resolved ref, or use a regex with word boundary/whitespace anchoring to ensure only standalone mention tokens are removed.
| if (!preserveTabOrder) { | ||
| pushRecentViewedChatId(chatId); | ||
| } | ||
| navigate({ | ||
| const navigationResult = navigate({ | ||
| to: "/chat", | ||
| search: { id: chatId }, | ||
| }); | ||
|
|
||
| if (prefillInput !== undefined) { | ||
| Promise.resolve(navigationResult) |
There was a problem hiding this comment.
MEDIUM | error-handling
Promise.resolve(navigationResult) wrapping obscures the actual navigation promise type
navigate() from TanStack Router may not return a promise at all (it can return void or a Promise<void> depending on the router version). Wrapping it in Promise.resolve() means that if navigate is synchronous and returns undefined, the .then() fires immediately in the microtask queue -- before the route component has mounted. This makes the prefillInput timing issue (already noted in existing reviews) even more likely to fail.
Additionally, the .catch(() => {}) swallows ALL errors, including potential bugs in setChatInputValue itself, making debugging difficult.
Suggestion: At minimum, log the caught error instead of silently discarding it:
.catch((err) => {
console.warn("Failed to prefill chat input after navigation:", err);
});
| ? `${systemPrompt}\n\n${params.prompt}` | ||
| : params.prompt; | ||
|
|
||
| const requestId = `image-gen-${uuidv4()}`; |
There was a problem hiding this comment.
LOW | correctness
Abort timeout clearTimeout runs in finally but the timeout callback calls controller.abort() which triggers an AbortError -- the catch block re-wraps it, losing the original error if the fetch was already in-flight
The flow is: timeout fires -> controller.abort() -> fetch rejects with AbortError -> caught -> re-thrown as "Image generation timed out". This is correct.
However, if the user's network drops and fetch throws a TypeError (e.g., DNS resolution failure), the catch block re-wraps it as "Failed to connect to image generation service." -- losing the original error details. This makes debugging connectivity issues harder.
Suggestion: Include the original error message in the re-thrown error for diagnostics:
throw new Error(`Failed to connect to image generation service: ${error instanceof Error ? error.message : "Unknown error"}`);
| ) { | ||
| const mediaDir = path.join(appPath, DYAD_MEDIA_DIR_NAME); | ||
| try { | ||
| await fs.promises.access(mediaDir); | ||
| } catch { | ||
| return []; | ||
| } | ||
|
|
||
| const entries = await fs.promises.readdir(mediaDir, { withFileTypes: true }); |
There was a problem hiding this comment.
MEDIUM | performance
Sequential stat() calls for every file in media directory
getMediaFilesForApp calls fs.promises.stat(fullPath) individually for each file in a serial for loop. For a media directory with many files, this is O(n) sequential I/O operations. Since each stat call is independent, they could be parallelized.
Suggestion: Use Promise.all with map to parallelize the stat calls:
const files = await Promise.all(
entries
.filter(entry => entry.isFile() && SUPPORTED_MEDIA_EXTENSIONS.includes(path.extname(entry.name).toLowerCase()))
.map(async (entry) => {
const ext = path.extname(entry.name).toLowerCase();
const fullPath = path.join(mediaDir, entry.name);
const stat = await fs.promises.stat(fullPath);
return { fileName: entry.name, filePath: fullPath, appId, appName, sizeBytes: stat.size, mimeType: getMimeType(ext) };
})
);
| // Remove data: URIs in href and xlink:href attributes (defense-in-depth) | ||
| .replace(/((?:xlink:)?href\s*=\s*(?:"|'))data:[^"']*("|')/gi, "$1#$2") | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔴 HIGH | security
Regex-based SVG sanitization is bypassable
The regex patterns can be bypassed by unclosed <script> tags (e.g., <script>alert(1) at EOF without a closing tag), nested HTML comments, encoding tricks, and CDATA sections. Regex-based HTML/XML sanitization is fundamentally unreliable.
The Content-Security-Policy: script-src 'none' header set in main.ts provides defense-in-depth, but the sanitizer itself should be robust since it's the primary protection layer.
💡 Suggestion: Use a proper DOM parser (e.g., DOMPurify on the Node side) to parse and allowlist safe SVG elements/attributes. As a quick fix, also add a catch-all for unclosed script tags: .replace(/<script[\s>][\s\S]*/gi, '').
Found by: Correctness Expert, Code Health Expert
| // Skip animation entirely for users who prefer reduced motion | ||
| if (prefersReducedMotion) { | ||
| setTimeout(onComplete, 0); | ||
| return null; |
There was a problem hiding this comment.
🔴 HIGH | correctness
Calling setTimeout during render is a React anti-pattern
setTimeout(onComplete, 0) is called directly in the render body when prefersReducedMotion is true. This is a side effect during render that causes double-invocations in StrictMode and potential state-update-during-render warnings.
💡 Suggestion:
useEffect(() => {
if (prefersReducedMotion) onComplete();
}, [prefersReducedMotion, onComplete]);
if (prefersReducedMotion) return null;Found by: Code Health Expert, UX Wizard
| mediaRegex, | ||
| `@media:${encodeURIComponent(file.fileName)}`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM | correctness
Media mention conversion corrupted if an app is named 'media'
Media mentions are converted first (@filename -> @media:encodedFilename), then app names are converted. If an app is named media, the app regex @(media)(?![a-zA-Z0-9_/\\-]) matches the @media prefix in @media:filename.png because : is not in the negative lookahead, corrupting it to @app:media:filename.png.
💡 Suggestion: Add : to the app name regex negative lookahead: (?![a-zA-Z0-9_/:\\-]).
Found by: Correctness Expert
| } catch (e) { | ||
| logger.error("Failed to resolve media mentions:", e); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM | correctness
Unresolved media mentions sent as raw @media: text to AI model
When parseMediaMentions finds refs but resolveMediaMentions resolves none (e.g., files deleted after mention), resolvedMediaRefs is empty and stripResolvedMediaMentions returns the prompt unchanged. The raw @media:filename.png text is sent to the AI, which won't understand it.
💡 Suggestion: When resolvedMedia is empty but mediaRefs is non-empty, either warn the user about missing files or strip all unresolved @media: tokens.
Found by: Correctness Expert
| }); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 MEDIUM | data-integrity
Partial copy not cleaned up on copyFile failure during move
If copyFile fails partway (e.g., disk full), a corrupted partial destination file remains. The cleanup logic only handles unlink(source) failure, not copyFile failure.
💡 Suggestion: Wrap copyFile in a try/catch that cleans up the destination file on failure:
try {
await fs.promises.copyFile(sourcePath, destinationPath);
} catch (e) {
try { await fs.promises.unlink(destinationPath); } catch {}
throw e;
}Found by: Correctness Expert
|
|
||
| if (imageData.b64_json) { | ||
| const buffer = Buffer.from(imageData.b64_json, "base64"); | ||
| const MAX_IMAGE_SIZE = 50 * 1024 * 1024; // 50 MB |
There was a problem hiding this comment.
🟡 MEDIUM | duplication
MAX_IMAGE_SIZE constant declared twice in the same function
MAX_IMAGE_SIZE = 50 * 1024 * 1024 is declared identically in both the b64_json branch (line 128) and the url branch (line 145). If the limit is updated in one place but not the other, they'll drift.
💡 Suggestion: Hoist to module level alongside IMAGE_GENERATION_TIMEOUT_MS.
Found by: Code Health Expert
| "hover:border-primary/30 transition-colors", | ||
| )} | ||
| role="button" | ||
| tabIndex={0} |
There was a problem hiding this comment.
🟡 MEDIUM | interaction
Keyboard events on dropdown trigger bubble to thumbnail, causing accidental preview opens
The thumbnail area is a clickable button that opens the preview, and the DropdownMenuTrigger is absolutely positioned inside it. While stopPropagation() is on the trigger's onClick, the parent's onKeyDown handler can still fire when a keyboard user presses Enter while the dropdown trigger is focused.
💡 Suggestion: Add onKeyDown={(e) => e.stopPropagation()} to the DropdownMenuTrigger.
Found by: UX Wizard
| <p className="text-xs text-muted-foreground/60"> | ||
| This may take up to a minute | ||
| </p> | ||
| </motion.div> |
There was a problem hiding this comment.
🟡 MEDIUM | performance-feel
No way to cancel image generation (up to 2-minute wait)
The 120s timeout means users can be stuck waiting with no cancel option. The Cancel button in the footer is disabled during generation, and the copy says 'up to a minute' which understates the actual timeout.
💡 Suggestion: Allow closing the dialog during generation, or add a Cancel button that aborts the request. Update the copy to match the actual timeout.
Found by: UX Wizard
| appFiles, | ||
| messageHistory, | ||
| mediaApps, | ||
| ]); |
There was a problem hiding this comment.
🟡 MEDIUM | interaction
Media mentions prepended first in autocomplete, pushing frequent items down
Media items are added before app mentions, prompts, and files. If an app has many media files, users must scroll past all of them to find frequently-used app/prompt mentions.
💡 Suggestion: Place media items after app and prompt mentions, or add section headers to the dropdown for easier scanning.
Found by: UX Wizard
| > | ||
| <span className={selectedApp ? "" : "text-muted-foreground"}> | ||
| {selectedApp?.name ?? "Select an app..."} | ||
| </span> |
There was a problem hiding this comment.
🟡 MEDIUM | accessibility
App selector popover lacks proper ARIA listbox patterns
The PopoverTrigger declares aria-haspopup='listbox' but the content doesn't use role='listbox'/role='option'. Arrow key navigation is not implemented, forcing keyboard users to Tab through every option.
💡 Suggestion: Add role='listbox' to the options container and role='option' to each item, or use an existing accessible combobox component.
Found by: UX Wizard
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (8 items)
🚫 Dropped False Positives (4 items)
Generated by Dyadbot multi-agent code review |
🎭 Playwright Test Results❌ Some tests failed
Summary: 249 passed, 1 failed, 11 flaky, 6 skipped Failed Tests🍎 macOS
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/template-create-nextjs.spec.ts
|
|
I'm closing this and opening a cleaner PR |
2 participants
I am gonna add the following follow-up PRs :