Skip to content

Conversation

@ngoiyaeric
Copy link
Collaborator

@ngoiyaeric ngoiyaeric commented Oct 5, 2025

User description

First, I've addressed an issue on mobile devices where attaching a file didn't always work as expected. Now, when you select a file on your phone, it should be sent automatically.

Second, you can now attach multiple files at once. I've updated the interface to show small previews of each image you've selected, and you can remove them individually if needed. This should make adding attachments much smoother.


PR Type

Enhancement, Bug fix


Description

  • Enable multiple file attachments with image previews

  • Fix mobile auto-submit for file attachments

  • Add individual file removal functionality

  • Improve mobile attachment workflow


Diagram Walkthrough

flowchart LR
  A["File Selection"] --> B["Multiple Files Support"]
  B --> C["Image Previews"]
  C --> D["Individual Removal"]
  A --> E["Mobile Detection"]
  E --> F["Auto-submit on Mobile"]
Loading

File Walkthrough

Relevant files
Enhancement
chat-panel.tsx
Multiple file attachments with mobile improvements             

components/chat-panel.tsx

  • Replace single file state with array for multiple files
  • Add image preview thumbnails with removal buttons
  • Implement mobile auto-submit when files selected
  • Update form submission to handle multiple files
+78/-36 
Tests
verify_multi_attachment.py
Mobile multi-attachment verification test                               

jules-scratch/verification/verify_multi_attachment.py

  • Add Playwright test for mobile multi-file attachment
  • Verify image preview display functionality
  • Test auto-submit behavior on mobile devices
+42/-0   

Summary by CodeRabbit

  • New Features
    • Attach multiple images to a message with horizontal thumbnail previews and per-image remove buttons; also clear all attachments.
    • Messages render multiple attached images in the message view (grid/wrapped layout).
    • File input accepts multiple images; send is enabled only when there’s text or at least one attachment.
    • Auto-submit on mobile when attachments are added and message text is empty.
  • Tests
    • Added an end-to-end test verifying multi-file uploads, previews, mobile behavior, and screenshot capture.

First, I've addressed an issue on mobile devices where attaching a file didn't always work as expected. Now, when you select a file on your phone, it should be sent automatically.

Second, you can now attach multiple files at once. I've updated the interface to show small previews of each image you've selected, and you can remove them individually if needed. This should make adding attachments much smoother.
@charliecreates charliecreates bot requested a review from CharlieHelps October 5, 2025 10:48
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@vercel
Copy link
Contributor

vercel bot commented Oct 5, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
qcx Ready Ready Preview Comment Oct 5, 2025 11:27am

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 5, 2025

Walkthrough

The UI and server submit flow were updated to support multiple image attachments: selection, per-file validation, previews with per-file removal and clear-all, aggregated FormData submission, and mobile auto-submit when no text is present. A Playwright verification script for multi-attachment mobile flows was added.

Changes

Cohort / File(s) Summary of Changes
Chat panel multi-attachment
components/chat-panel.tsx
Replaced single-file state with selectedFiles: File[]; accumulate validated files on input; per-file size checks with user alerts; removeAttachment(index) and clearAllAttachments(); reset file input to allow re-selecting same files; render horizontal preview list with per-file remove buttons; file input now multiple and accepts images; auto-submit on mobile when no text; submit builds FormData with multiple files[index].
Submission & server-side processing
app/actions.tsx
Reworked form handling to collect all files[...] entries into an array; build messageParts for all uploaded files (image data URLs and text/plain concatenation across files); determine content/type based on presence of any uploaded file; removed single-file code paths; streaming/AI response logic retained with multi-file support.
Message rendering
components/user-message.tsx
Replaced single-image rendering with mapping over multiple image parts; images rendered in a flex-wrapped grid with individual wrappers and alt text; text rendering preserved.
E2E verification script
jules-scratch/verification/verify_multi_attachment.py
New Playwright sync script that launches a mobile-like context, removes overlay if present, uploads two images via file input, asserts two previews in the user message area, waits for bot response (up to 30s), screenshots, and exposes run_verification() plus __main__ entry.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User
  participant CP as ChatPanel (UI)
  participant FD as FormData
  participant S as Server
  participant B as Bot/AI

  rect rgb(245,250,240)
    U->>CP: Select 1..N image files
    CP->>CP: Validate & accumulate files (size checks)
    CP->>CP: Render previews (per-file remove / clear all)
    alt Mobile + empty text
      CP->>CP: Auto-submit
    else Manual send
      U->>CP: Click Send
    end
  end

  CP->>FD: Append text (if any) and all files as files[index]
  CP->>S: POST FormData
  S->>B: Forward content + files
  B-->>S: Generate response
  S-->>CP: Stream/return bot message
  CP-->>U: Display bot reply and any returned assets
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

Review effort 4/5, Possible security concern

Poem

I hopped through bytes with whiskers bright,
Packed little pictures, ready for flight.
Tap on mobile — they bounce and go,
Thumbnails parade in a neat little row.
Screenshot taken, the bot replies — carrot cheers and happy sighs. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The current title is vague and generic and does not convey the primary changes of enabling multiple attachments, previews, and improved mobile behavior, making it unclear to reviewers what the pull request actually implements. Please update the title to concisely reflect the key enhancements, for example “Enable multi-file attachments with preview thumbnails and mobile auto-submit,” so that it clearly communicates the main changes in this pull request.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-mobile-attachment-and-previews

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 Ruff (0.13.3)
jules-scratch/verification/verify_multi_attachment.py

�[1;31mruff failed�[0m
�[1mCause:�[0m Failed to load configuration /ruff.toml
�[1mCause:�[0m Failed to parse /ruff.toml
�[1mCause:�[0m TOML parse error at line 26, column 3
|
26 | "RSE100", # Use of assert detected
| ^^^^^^^^
Unknown rule selector: RSE100


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Oct 5, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Resource leak

Description: Using URL.createObjectURL(file) for image previews without revoking the object URLs can
cause memory leaks, especially during repeated selections.
chat-panel.tsx [185-191]

Referred Code
<Image
    src={URL.createObjectURL(file)}
    alt={file.name}
    width={80}
    height={80}
    className="w-full h-full object-cover rounded-md"
/>
Insufficient upload validation

Description: File input accepts only images, but there is no server-side validation shown; relying
solely on client-side accept can allow unsupported or malicious files via crafted
requests.
chat-panel.tsx [220-226]

Referred Code
  type="file"
  ref={fileInputRef}
  onChange={handleFileChange}
  className="hidden"
  accept="image/png,image/jpeg,image/webp"
  multiple
/>
Test bypass hook

Description: The test calls window.setIsMapLoaded(true) assuming a global function which, if present in
production, could be abused to bypass gating; ensure such globals are not exposed outside
test context.
verify_multi_attachment.py [14-16]

Referred Code
# Bypass the loading overlay by directly setting the isMapLoaded state to true
page.evaluate("() => { window.setIsMapLoaded(true); }")
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link
Contributor

qodo-code-review bot commented Oct 5, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent memory leaks from unrevoked URLs

To prevent memory leaks, manage object URLs in a state and use a useEffect
cleanup function to revoke them with URL.revokeObjectURL when they are no longer
needed.

components/chat-panel.tsx [102-109]

-selectedFiles.forEach(file => {
-    if (file.type.startsWith('image/')) {
-        content.push({
-            type: 'image',
-            image: URL.createObjectURL(file)
-        })
-    }
+const [imagePreviews, setImagePreviews] = useState<string[]>([]);
+
+useEffect(() => {
+  if (selectedFiles.length === 0 && imagePreviews.length === 0) {
+    return;
+  }
+  const newImagePreviews = selectedFiles.map(file => URL.createObjectURL(file));
+  setImagePreviews(newImagePreviews);
+
+  return () => {
+    newImagePreviews.forEach(url => URL.revokeObjectURL(url));
+  };
+}, [selectedFiles]);
+// ... inside handleSubmit
+imagePreviews.forEach(url => {
+    content.push({
+        type: 'image',
+        image: url
+    })
 })
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a memory leak caused by not revoking object URLs created with URL.createObjectURL. This is a critical issue that can degrade application performance and stability over time.

High
Prevent duplicate file selections and bugs

In handleFileChange, prevent duplicate file uploads by filtering new files
against the existing list. Also, reset the file input's value after each
selection to ensure the onChange event fires consistently, allowing re-selection
of the same file.

components/chat-panel.tsx [49-61]

 const handleFileChange = (e: ChangeEvent<HTMLInputElement>) => {
-  const files = e.target.files
-  if (files) {
-    const newFiles = Array.from(files).filter(file => {
+  if (e.target.files) {
+    const newFiles = Array.from(e.target.files).filter(file => {
       if (file.size > 10 * 1024 * 1024) {
-        alert(`File ${file.name} is too large (max 10MB)`)
-        return false
+        alert(`File ${file.name} is too large (max 10MB)`);
+        return false;
       }
-      return true
-    })
-    setSelectedFiles(prevFiles => [...prevFiles, ...newFiles])
+      return true;
+    });
+
+    setSelectedFiles(prevFiles => {
+      const combined = [...prevFiles, ...newFiles];
+      // Filter out duplicates based on name, size, and lastModified time
+      const uniqueFiles = combined.filter(
+        (file, index, self) =>
+          index ===
+          self.findIndex(
+            f => f.name === file.name && f.size === file.size && f.lastModified === file.lastModified
+          )
+      );
+      return uniqueFiles;
+    });
   }
-}
+  // Reset file input to allow re-selecting the same file
+  if (fileInputRef.current) {
+    fileInputRef.current.value = '';
+  }
+};
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies two user experience issues: the possibility of adding duplicate files and the inability to re-select a file after removing it. The proposed solution effectively addresses both problems, improving the feature's robustness.

Medium
  • Update

Copy link

@charliecreates charliecreates bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Blob URLs are created during render and never revoked, leading to memory leaks for both previews and message content; generate once and revoke or use data URLs.
  • Changing the FormData key from file to files risks breaking the backend; append multiple entries under file instead.
  • Using the array index as a key in preview items can cause reconciliation issues; use a stable key per file.
  • The repo includes local dev/test artifacts (dev.log, jules-scratch/verification/*) that should be removed or moved to a proper tests directory.
Additional notes (5)
  • Maintainability | components/chat-panel.tsx:182-183
    Using the array index as a React key can cause incorrect UI updates when removing items from the middle of the list. Use a stable key derived from the file (e.g., name + lastModified + size) to keep reconciliation correct.

  • Readability | components/chat-panel.tsx:193-199
    The remove button for each preview lacks an accessible label. Screen readers will announce it as a generic button, making it hard to understand its purpose.

  • Maintainability | components/chat-panel.tsx:52-59
    Selecting files multiple times can add duplicate entries to selectedFiles (same file chosen repeatedly). This can waste bandwidth and clutter the UI.

  • Maintainability | dev.log:1-1
    This log file appears to be a local dev artifact and shouldn’t be committed to the repo. Keeping it can clutter the history and confuse contributors.

  • Maintainability | jules-scratch/verification/verify_multi_attachment.py:1-42
    Scratch verification assets and scripts under jules-scratch/verification/ look like local debugging artifacts. Consider moving them to a proper e2e/tests directory and wiring them into CI, or exclude them from the repo to avoid bloat. The reliance on a global window.setIsMapLoaded hook suggests they aren’t production-reliable.

Summary of changes
  • Replaced single-file attachment state with multi-file support via selectedFiles: File[] and updated UI accordingly.
  • Added mobile auto-submit when files are selected and input is empty.
  • Introduced image previews using next/image for each selected file with per-item removal and a "clear all" path.
  • Updated submission to build content entries for each image and append multiple files to FormData.
  • Changed file input to accept multiple images only and disabled text attachments.
  • Added verification assets/scripts and a dev.log file.

Comment on lines +185 to +191
<Image
src={URL.createObjectURL(file)}
alt={file.name}
width={80}
height={80}
className="w-full h-full object-cover rounded-md"
/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URL.createObjectURL(file) is called directly during render for each preview. This creates a new blob URL on each render and none are revoked, which can cause memory leaks and increased heap usage over time, especially if users add/remove attachments repeatedly. Additionally, next/image with blob URLs may require unoptimized to avoid the Image Optimization pipeline in some Next.js setups. Consider generating stable preview URLs once per file (e.g., in state or useMemo/useEffect), reusing them in render, and revoking them on cleanup/removal.

Suggestion

Suggested approach:

  • Track preview URLs in state and revoke them on cleanup/removal.
  • Use those for both previews and to avoid re-creating URLs during render.
  • Add unoptimized to next/image for blob/data URLs if needed.

Example changes:

  • Create previews and cleanup

    const [previews, setPreviews] = useState<string[]>([])

    useEffect(() => {
    // Revoke previous URLs
    return () => {
    previews.forEach(url => URL.revokeObjectURL(url))
    }
    }, [previews])

    useEffect(() => {
    const urls = selectedFiles.map(f => URL.createObjectURL(f))
    setPreviews(urls)
    return () => {
    urls.forEach(url => URL.revokeObjectURL(url))
    }
    }, [selectedFiles])

  • Use previews in render and add unoptimized

    {file.name}

  • Revoke the specific preview in removeAttachment

    setSelectedFiles(prev => prev.filter((, i) => i !== index))
    URL.revokeObjectURL(previews[index])
    setPreviews(prev => prev.filter((
    , i) => i !== index))

Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.

Comment on lines 121 to 124
if (selectedFiles.length > 0) {
selectedFiles.forEach(file => {
formData.append('files', file)
})
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FormData field name changed from 'file' (singular) to 'files' (plural). Unless the server-side submit handler has been updated accordingly, this is a silent breaking change. A safer pattern is to append multiple entries under the same field name 'file', which most frameworks parse as an array.

Suggestion

Use the original field name and append multiple values:

selectedFiles.forEach(file => {
formData.append('file', file)
})

This maintains backward compatibility while supporting multiple files. Reply with "@CharlieHelps yes please" if you'd like me to add a commit to make this change.

Comment on lines +224 to +225
accept="image/png,image/jpeg,image/webp"
multiple
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file input now only accepts images, dropping previous text/plain support. If text attachments were intentionally supported before, this is a functional regression. Confirm product expectations; if text files should still be allowed, keep text/plain.

Suggestion

If text files remain supported, restore the accept list:

accept="text/plain,image/png,image/jpeg,image/webp"

Reply with "@CharlieHelps yes please" if you'd like me to add a commit reverting the accept filter accordingly.

@charliecreates charliecreates bot removed the request for review from CharlieHelps October 5, 2025 10:52
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 69da84b and 4360646.

⛔ Files ignored due to path filters (3)
  • dev.log is excluded by !**/*.log
  • jules-scratch/verification/test_image_1.png is excluded by !**/*.png
  • jules-scratch/verification/test_image_2.png is excluded by !**/*.png
📒 Files selected for processing (2)
  • components/chat-panel.tsx (9 hunks)
  • jules-scratch/verification/verify_multi_attachment.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jules-scratch/verification/verify_multi_attachment.py (1)
app/layout.tsx (1)
  • viewport (42-47)

Comment on lines +181 to +191
<div
key={index}
className="flex-shrink-0 relative w-20 h-20 bg-muted rounded-lg p-1"
>
<Image
src={URL.createObjectURL(file)}
alt={file.name}
width={80}
height={80}
className="w-full h-full object-cover rounded-md"
/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid creating blob URLs during render

URL.createObjectURL(file) runs on every render, leaking blob URLs and forcing <Image> to remount. Memoize the previews and revoke them during cleanup.

-import { useEffect, useState, useRef, ChangeEvent, forwardRef, useImperativeHandle } from 'react'
+import { useEffect, useState, useRef, ChangeEvent, forwardRef, useImperativeHandle, useMemo } from 'react'
@@
-  const [selectedFiles, setSelectedFiles] = useState<File[]>([])
+  const [selectedFiles, setSelectedFiles] = useState<File[]>([])
+  const previewFiles = useMemo(
+    () =>
+      selectedFiles.map(file => ({
+        file,
+        previewUrl: URL.createObjectURL(file)
+      })),
+    [selectedFiles]
+  )
+
+  useEffect(() => {
+    return () => {
+      previewFiles.forEach(({ previewUrl }) => URL.revokeObjectURL(previewUrl))
+    }
+  }, [previewFiles])
@@
-                {selectedFiles.map((file, index) => (
+                {previewFiles.map(({ file, previewUrl }, index) => (
@@
-                        <Image
-                            src={URL.createObjectURL(file)}
+                        <Image
+                            src={previewUrl}
📝 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.

Suggested change
<div
key={index}
className="flex-shrink-0 relative w-20 h-20 bg-muted rounded-lg p-1"
>
<Image
src={URL.createObjectURL(file)}
alt={file.name}
width={80}
height={80}
className="w-full h-full object-cover rounded-md"
/>
// components/chat-panel.tsx
import {
useEffect,
useState,
useRef,
ChangeEvent,
forwardRef,
useImperativeHandle,
useMemo
} from 'react'
import { useMemo } from 'react'
const [selectedFiles, setSelectedFiles] = useState<File[]>([])
const previewFiles = useMemo(
() =>
selectedFiles.map(file => ({
file,
previewUrl: URL.createObjectURL(file)
})),
[selectedFiles]
)
useEffect(() => {
return () => {
previewFiles.forEach(({ previewUrl }) => URL.revokeObjectURL(previewUrl))
}
}, [previewFiles])
return (
{selectedFiles.map((file, index) => (
{previewFiles.map(({ file, previewUrl }, index) => (
<div
key={index}
className="flex-shrink-0 relative w-20 h-20 bg-muted rounded-lg p-1"
>
<Image
src={URL.createObjectURL(file)}
src={previewUrl}
alt={file.name}
width={80}
height={80}
className="w-full h-full object-cover rounded-md"
/>
</div>
))}
)
🤖 Prompt for AI Agents
In components/chat-panel.tsx around lines 181 to 191, the code calls
URL.createObjectURL(file) directly during render which generates new blob URLs
on every render and leaks them; instead, create and memoize blob URLs (e.g., in
a useEffect that runs when the files array changes or per file using
useMemo/useState), store the generated preview URLs in state or a ref and use
those for the Image src, and revoke each blob URL in the effect cleanup (or when
a file is removed) using URL.revokeObjectURL to prevent memory leaks and
unnecessary remounts.

…files, and it works better on mobile devices. I've also fixed a few bugs to make sure everything runs smoothly.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

♻️ Duplicate comments (3)
components/chat-panel.tsx (3)

185-191: Memory leak: blob URLs created during render without revocation.

URL.createObjectURL(file) is called during render on lines 186 and 106, creating new blob URLs on every render without revoking them. This causes memory leaks, especially if users repeatedly add/remove attachments.

This issue was previously flagged in past review comments.


121-124: Potential breaking change: FormData field name changed from 'file' to 'files[index]'.

The field name changed from 'file' (singular) to 'files[${index}]' (plural with index). Unless the server-side handler in app/actions.tsx has been updated to parse keys starting with 'files[', this breaks backward compatibility.

This concern was previously raised in past review comments. Based on the review of app/actions.tsx, the server does scan for keys starting with 'files[', so the change is handled. However, verify that all other submit handlers (e.g., in copilot.tsx, followup-panel.tsx) also support this pattern if they accept file attachments.


224-225: File type restriction: text/plain support removed.

The accept attribute now only includes images, dropping text/plain support. If text file attachments were previously supported and are still needed, this is a functional regression.

This concern was previously flagged in past review comments.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4360646 and 7850fa6.

⛔ Files ignored due to path filters (1)
  • jules-scratch/verification/debug_previews.png is excluded by !**/*.png
📒 Files selected for processing (4)
  • app/actions.tsx (3 hunks)
  • components/chat-panel.tsx (9 hunks)
  • components/user-message.tsx (1 hunks)
  • jules-scratch/verification/verify_multi_attachment.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
components/chat-panel.tsx (2)
components/copilot.tsx (1)
  • e (70-85)
components/followup-panel.tsx (1)
  • event (18-37)
app/actions.tsx (3)
components/followup-panel.tsx (1)
  • event (18-37)
components/copilot.tsx (1)
  • e (70-85)
lib/actions/chat.ts (1)
  • msg (119-127)
jules-scratch/verification/verify_multi_attachment.py (1)
app/layout.tsx (1)
  • viewport (42-47)
🔇 Additional comments (6)
components/user-message.tsx (1)

27-51: LGTM! Multi-image rendering correctly implemented.

The refactor from single-image to multi-image rendering is clean and correct. Filtering imageParts from the content array and mapping each to its own Image component with incremented alt text properly supports the new multi-file attachment workflow.

jules-scratch/verification/verify_multi_attachment.py (2)

3-55: Test structure looks good.

The Playwright verification script correctly:

  • Sets up a mobile browser context
  • Handles potential loading overlays
  • Verifies multi-file attachment UI
  • Waits for bot response
  • Captures verification screenshot

33-36: Test images verified

The files jules-scratch/verification/test_image_1.png and test_image_2.png are present; no further action required.

components/chat-panel.tsx (2)

49-61: File size validation correctly implemented.

Per-file validation with user alerts for oversized files is well-implemented. The 10MB limit is enforced individually, and invalid files are filtered out before being added to state.


77-90: Attachment management functions are correct.

Both removeAttachment and clearAllAttachments correctly update state and reset the file input to allow re-selection. The input value reset prevents the browser from blocking selection of previously chosen files.

app/actions.tsx (1)

131-181: Multi-file processing correctly implemented.

The server-side multi-file handling is well-structured:

  • Extracts all files from FormData entries with keys starting with 'files['
  • Validates that at least one file or userInput exists before proceeding
  • Processes images as base64 data URLs and text files as concatenated text parts

Comment on lines +163 to +170
if (file.type.startsWith('image/')) {
const dataUrl = `data:${file.type};base64,${Buffer.from(
buffer
).toString('base64')}`
messageParts.push({
type: 'image',
image: dataUrl
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider async data URL conversion for large images.

Base64 encoding large images (lines 164-166) is synchronous and blocks the Node.js event loop. While the 10MB file size limit in chat-panel.tsx mitigates this, consider using async methods if you plan to increase the limit or support more file types.

For future scalability, consider:

const dataUrl = `data:${file.type};base64,${(await buffer).toString('base64')}`

However, since arrayBuffer() is already async and the encoding itself is fast for files under 10MB, this is low priority.

🤖 Prompt for AI Agents
In app/actions.tsx around lines 163 to 170, the current synchronous Base64
conversion (Buffer.from(buffer).toString('base64')) can block the event loop for
large images; change it to use an async conversion by awaiting the data source
before creating the Buffer (e.g., Buffer.from(await buffer).toString('base64'))
or, for very large files, switch to a non-blocking/streaming encoding approach
so encoding does not run synchronously on the main event loop.

Comment on lines +171 to 179
} else if (file.type === 'text/plain') {
const textContent = Buffer.from(buffer).toString('utf-8')
const existingTextPart = messageParts.find(p => p.type === 'text')
if (existingTextPart) {
existingTextPart.text = `${textContent}\n\n${existingTextPart.text}`
} else {
messageParts.push({ type: 'text', text: textContent })
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Text file concatenation order may be unexpected.

When processing multiple text files, new file content is prepended to existing text parts (line 175: ${textContent}\n\n${existingTextPart.text}). This means later files in the array appear first in the final text. If users expect files to be concatenated in the order they were selected, this could be confusing.

Consider appending instead of prepending:

         const existingTextPart = messageParts.find(p => p.type === 'text')
         if (existingTextPart) {
-          existingTextPart.text = `${textContent}\n\n${existingTextPart.text}`
+          existingTextPart.text = `${existingTextPart.text}\n\n${textContent}`
         } else {
           messageParts.push({ type: 'text', text: textContent })
         }

Alternatively, if prepending is intentional (e.g., to prioritize later attachments), document this behavior in a code comment.

📝 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.

Suggested change
} else if (file.type === 'text/plain') {
const textContent = Buffer.from(buffer).toString('utf-8')
const existingTextPart = messageParts.find(p => p.type === 'text')
if (existingTextPart) {
existingTextPart.text = `${textContent}\n\n${existingTextPart.text}`
} else {
messageParts.push({ type: 'text', text: textContent })
}
}
} else if (file.type === 'text/plain') {
const textContent = Buffer.from(buffer).toString('utf-8')
const existingTextPart = messageParts.find(p => p.type === 'text')
if (existingTextPart) {
existingTextPart.text = `${existingTextPart.text}\n\n${textContent}`
} else {
messageParts.push({ type: 'text', text: textContent })
}
}
🤖 Prompt for AI Agents
In app/actions.tsx around lines 171 to 179, the code prepends new text file
content to existing text parts, causing later files to appear first; change the
concatenation to append (replace `${textContent}\n\n${existingTextPart.text}`
with `${existingTextPart.text}\n\n${textContent}`) so files preserve the
original selection order, or if prepending is intentional, add a clear inline
comment explaining that later attachments are prioritized and why.

Comment on lines +64 to +71
useEffect(() => {
if (selectedFiles.length > 0 && isMobile) {
// Only auto-submit if there wasn't text before, to avoid interrupting typing
if (input.trim() === '') {
formRef.current?.requestSubmit()
}
}
}, [selectedFiles, isMobile, input])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Auto-submit may interrupt multi-file selection.

The auto-submit effect triggers on every selectedFiles change. If a user selects multiple files from a file picker that allows multi-selection, or uses the file input multiple times in quick succession, the form might auto-submit before they finish adding all desired files.

Consider one of these approaches:

  1. Add a small delay (e.g., 300ms) before auto-submitting to allow for rapid successive selections
  2. Only auto-submit when the file input's onChange has completed, not on every state update
  3. Add a flag to track whether the current selection is the first one and only auto-submit on first add

Example with delay:

   useEffect(() => {
     if (selectedFiles.length > 0 && isMobile) {
       if (input.trim() === '') {
-        formRef.current?.requestSubmit()
+        const timer = setTimeout(() => {
+          formRef.current?.requestSubmit()
+        }, 300)
+        return () => clearTimeout(timer)
       }
     }
   }, [selectedFiles, isMobile, input])
📝 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.

Suggested change
useEffect(() => {
if (selectedFiles.length > 0 && isMobile) {
// Only auto-submit if there wasn't text before, to avoid interrupting typing
if (input.trim() === '') {
formRef.current?.requestSubmit()
}
}
}, [selectedFiles, isMobile, input])
useEffect(() => {
if (selectedFiles.length > 0 && isMobile) {
// Only auto-submit if there wasn't text before, to avoid interrupting typing
if (input.trim() === '') {
const timer = setTimeout(() => {
formRef.current?.requestSubmit()
}, 300)
return () => clearTimeout(timer)
}
}
}, [selectedFiles, isMobile, input])

Comment on lines +102 to +109
selectedFiles.forEach(file => {
if (file.type.startsWith('image/')) {
content.push({
type: 'image',
image: URL.createObjectURL(file)
})
}
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Duplicate blob URL creation for image content.

URL.createObjectURL(file) is called again for each file when building content (line 106), in addition to the preview URLs created during render (line 186). These URLs are never revoked and contribute to the memory leak. Consider reusing preview URLs from state or creating them once in a useMemo hook.

This exacerbates the memory leak issue. When fixing the preview URL creation, also use the same memoized URLs here:

     selectedFiles.forEach(file => {
         if (file.type.startsWith('image/')) {
             content.push({
                 type: 'image',
-                image: URL.createObjectURL(file)
+                image: previewUrls[selectedFiles.indexOf(file)]
             })
         }
     })

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In components/chat-panel.tsx around lines 102 to 109, duplicate calls to
URL.createObjectURL(file) create new blob URLs for each image instead of reusing
the preview URLs generated during render, causing a memory leak; replace the
direct URL.createObjectURL(file) here with the memoized/managed preview URL
stored in component state or produced by a useMemo hook (create blob URLs only
once), and ensure you revoke them via URL.revokeObjectURL in the appropriate
cleanup (useEffect return or when removing files) so preview URLs are not
leaked.

Comment on lines +18 to +24
# Workaround for the persistent loading overlay in the test environment
overlay_selector = 'div[class*="z-[9999]"]'
try:
page.wait_for_selector(overlay_selector, state='visible', timeout=15000)
page.evaluate(f"document.querySelector('{overlay_selector}').remove()")
except Exception as e:
print(f"Could not find or remove overlay, perhaps it's not there: {e}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider making overlay removal more robust.

The overlay removal workaround catches exceptions but uses a hardcoded selector. If the overlay selector changes or doesn't exist, the test continues silently. Consider logging a warning if the overlay is not found rather than treating it as optional.

Apply this diff to improve the messaging:

         try:
             page.wait_for_selector(overlay_selector, state='visible', timeout=15000)
             page.evaluate(f"document.querySelector('{overlay_selector}').remove()")
+            print("✓ Removed persistent overlay")
         except Exception as e:
-            print(f"Could not find or remove overlay, perhaps it's not there: {e}")
+            print(f"ℹ No persistent overlay found (this is normal if the overlay doesn't appear): {e}")
📝 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.

Suggested change
# Workaround for the persistent loading overlay in the test environment
overlay_selector = 'div[class*="z-[9999]"]'
try:
page.wait_for_selector(overlay_selector, state='visible', timeout=15000)
page.evaluate(f"document.querySelector('{overlay_selector}').remove()")
except Exception as e:
print(f"Could not find or remove overlay, perhaps it's not there: {e}")
# Workaround for the persistent loading overlay in the test environment
overlay_selector = 'div[class*="z-[9999]"]'
try:
page.wait_for_selector(overlay_selector, state='visible', timeout=15000)
page.evaluate(f"document.querySelector('{overlay_selector}').remove()")
print("✓ Removed persistent overlay")
except Exception as e:
print(f"ℹ No persistent overlay found (this is normal if the overlay doesn't appear): {e}")
🤖 Prompt for AI Agents
In jules-scratch/verification/verify_multi_attachment.py around lines 18 to 24,
the overlay removal uses a hardcoded selector and prints exceptions silently;
replace this with a more robust check: use page.query_selector or
wait_for_selector with a short timeout to test presence, if found call
page.evaluate to remove it, and if not found log a warning via the logging
module (not print) that includes the selector and the exception or timeout info;
catch specific timeout/selector exceptions rather than blanket Exception and
ensure the log message clearly states the overlay was not present so the test
continues intentionally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants