-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: handle paste image on linux #6299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to b915f1f in 2 minutes and 3 seconds. Click for details.
- Reviewed
77
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/ChatInput.tsx:377
- Draft comment:
Good use of async/await for the paste handler. Consider adding a brief comment explaining why the handler is async (i.e., to support fallback for Linux where clipboardData.items is missing). - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
2. web-app/src/containers/ChatInput.tsx:386
- Draft comment:
The condition checks if clipboardItems is falsy. Note that if clipboardData.items exists but is an empty list, the fallback won't trigger. Verify this behavior is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is technically correct - an empty list would trigger the fallback. However, this is likely intentional since an empty clipboard items list means there's nothing to paste, so trying the fallback path makes sense. The comment is asking for verification of intended behavior without providing evidence that this is actually problematic. The comment identifies a real edge case in the code's behavior. Maybe there's a reason we should handle empty lists differently from null/undefined. While technically accurate, the behavior makes logical sense - if there are no clipboard items through the standard API, trying the fallback API is reasonable. There's no clear reason why empty lists should be treated differently. Delete the comment. While it identifies a real edge case, the current behavior is logical and there's no evidence provided that it needs to be changed.
3. web-app/src/containers/ChatInput.tsx:406
- Draft comment:
Converting the blob to a File uses a filename based on the MIME type. For types like 'image/svg+xml', the resulting filename may include unexpected characters. Consider sanitizing or handling such cases if needed. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
4. web-app/src/containers/ChatInput.tsx:418
- Draft comment:
The synthetic event casting to React.ChangeEvent (using 'as unknown as ...') bypasses type-checking. Although common, consider a more explicit approach or add a clarifying comment to explain this pattern. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
Workflow ID: wflow_QyicY20foaH7JrET
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 36.09%Your code coverage diff: -0.08% ▾ Uncovered files and lines
|
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.
Important
Looks good to me! 👍
Reviewed b93f77a in 2 minutes and 35 seconds. Click for details.
- Reviewed
131
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/containers/ChatInput.tsx:444
- Draft comment:
Potential filename collision: using Date.now() alone may not ensure unique names when multiple images are pasted quickly. Consider appending an extra unique identifier. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% 1. The code handles pasted images and generates filenames using Date.now() 2. Date.now() returns milliseconds, so collisions are possible but unlikely 3. These are temporary files for a paste operation, not permanent storage 4. Even if a collision occurred, it likely wouldn't cause any real issues since the files are just being temporarily held for upload 5. The suggested solution adds complexity without clear benefit The comment identifies a real theoretical edge case. Multiple pastes in the same millisecond could collide. While technically correct, this is an extremely unlikely edge case for a temporary file operation. The complexity cost outweighs the minimal risk. The comment should be deleted. The current implementation using Date.now() is sufficient for this temporary file handling use case.
2. web-app/src/containers/ChatInput.tsx:392
- Draft comment:
Refactor the processedCount and hasProcessedImage logic for clarity. The current flow in the traditional branch is a bit convoluted. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. web-app/src/containers/ChatInput.tsx:436
- Draft comment:
Consider using Promise.all in the modern Clipboard API fallback to process items concurrently for improved readability and potential performance benefits. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. web-app/src/containers/ChatInput.tsx:474
- Draft comment:
Consider reducing the verbosity of logging (or using a debug log) when no image data is found or clipboard access fails, to avoid noise in production logs. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_ObqfksWHW7cu2G4Q
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
LGTM in my testing
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.
LGTM!
Describe Your Changes
This pull request updates the paste handling logic in the
ChatInput
component to improve compatibility with Linux systems and browsers that do not support the legacyclipboardData.items
API. The main change is the addition of a fallback that uses the modern Clipboard API to read pasted images when the legacy API is unavailable.Paste handling improvements:
navigator.clipboard.read
) whenclipboardData.items
is unavailable, improving image paste support on Linux and newer browsers.File
objects and handled consistently by synthesizing a change event for the existing file handling logic.Fixes Issues
Self Checklist
Important
Improves image paste handling in
ChatInput.tsx
for Linux and newer browsers by using modern Clipboard API as a fallback and adding error handling.handlePaste
inChatInput.tsx
to use modern Clipboard API (navigator.clipboard.read
) as a fallback whenclipboardData.items
is unavailable.File
objects and processed by synthesizing a change event.This description was created by
for b93f77a. You can customize this summary. It will automatically update as commits are pushed.