-
-
Notifications
You must be signed in to change notification settings - Fork 7
I've made a few changes to improve the chat experience. #317
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
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.
|
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThe 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
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
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)
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 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||
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.
- 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
FormDatakey fromfiletofilesrisks breaking the backend; append multiple entries underfileinstead. - 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 Reactkeycan 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 toselectedFiles(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 underjules-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 globalwindow.setIsMapLoadedhook 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/imagefor each selected file with per-item removal and a "clear all" path. - Updated submission to build
contententries for each image and append multiple files toFormData. - Changed file input to accept multiple images only and disabled text attachments.
- Added verification assets/scripts and a
dev.logfile.
| <Image | ||
| src={URL.createObjectURL(file)} | ||
| alt={file.name} | ||
| width={80} | ||
| height={80} | ||
| className="w-full h-full object-cover rounded-md" | ||
| /> |
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.
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
unoptimizedtonext/imagefor 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 -
Revoke the specific preview in
removeAttachmentsetSelectedFiles(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.
| if (selectedFiles.length > 0) { | ||
| selectedFiles.forEach(file => { | ||
| formData.append('files', file) | ||
| }) |
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.
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.
| accept="image/png,image/jpeg,image/webp" | ||
| multiple |
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.
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.
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (3)
dev.logis excluded by!**/*.logjules-scratch/verification/test_image_1.pngis excluded by!**/*.pngjules-scratch/verification/test_image_2.pngis 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)
| <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" | ||
| /> |
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.
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.
| <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.
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.
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 inapp/actions.tsxhas 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., incopilot.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/plainsupport. 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
⛔ Files ignored due to path filters (1)
jules-scratch/verification/debug_previews.pngis 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
imagePartsfrom the content array and mapping each to its ownImagecomponent 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 verifiedThe files
jules-scratch/verification/test_image_1.pngandtest_image_2.pngare 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
removeAttachmentandclearAllAttachmentscorrectly 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
| if (file.type.startsWith('image/')) { | ||
| const dataUrl = `data:${file.type};base64,${Buffer.from( | ||
| buffer | ||
| ).toString('base64')}` | ||
| messageParts.push({ | ||
| type: 'image', | ||
| image: dataUrl | ||
| }) |
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.
🧹 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.
| } 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 }) | ||
| } | ||
| } |
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.
🧹 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.
| } 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.
| 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]) |
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.
🧹 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:
- Add a small delay (e.g., 300ms) before auto-submitting to allow for rapid successive selections
- Only auto-submit when the file input's
onChangehas completed, not on every state update - 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.
| 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]) |
| selectedFiles.forEach(file => { | ||
| if (file.type.startsWith('image/')) { | ||
| content.push({ | ||
| type: 'image', | ||
| image: URL.createObjectURL(file) | ||
| }) | ||
| } | ||
| }) |
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.
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.
| # 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}") |
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.
🧹 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.
| # 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.
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
File Walkthrough
chat-panel.tsx
Multiple file attachments with mobile improvementscomponents/chat-panel.tsx
verify_multi_attachment.py
Mobile multi-attachment verification testjules-scratch/verification/verify_multi_attachment.py
Summary by CodeRabbit