-
Notifications
You must be signed in to change notification settings - Fork 193
upload more file types #616
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
🦋 Changeset detectedLatest commit: c5094ab The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
c0d3598
to
7eaeedd
Compare
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.
Looks pretty good to me! I left a few comments / questions mostly around conflicts and possibly trying to share more of the image/file code (for size limits). I think my main comment overall is @kevinvandijk likes us to add //kilocode_change start/end
markers in any roo code, so make sure to go though and add those in
case "selectImages": | ||
const images = await selectImages() | ||
await provider.postMessageToWebview({ type: "selectedImages", images }) | ||
const { images, files } = await selectFiles() | ||
await provider.postMessageToWebview({ type: "selectedImages", images, filePaths: files }) |
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.
Hmm, should we have a separate message type for selectFiles
? Just noticing the names matched before and now there's a disconnect, case selectImages
calling selectFiles
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.
This is how Cline does it but I'm down to change it!
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.
Would be possible to keep Roo's selectImages, but then also add our own selectFiles below this that only deals with files?
@@ -1061,7 +1062,10 @@ export const webviewMessageHandler = async (provider: ClineProvider, message: We | |||
supportPrompt.create("ENHANCE", { userInput: message.text }, customSupportPrompts), | |||
) | |||
|
|||
await provider.postMessageToWebview({ type: "enhancedPrompt", text: enhancedPrompt }) | |||
await provider.postMessageToWebview({ |
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.
revert if you can to avoid conflicts
7eaeedd
to
0739a52
Compare
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.
submitting our pair review comments
this.emit("taskAskResponded") | ||
return result | ||
} | ||
|
||
async handleWebviewAskResponse(askResponse: ClineAskResponse, text?: string, images?: string[]) { | ||
async handleWebviewAskResponse(askResponse: ClineAskResponse, text?: string, images?: string[], files?: string[]) { |
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.
filePaths maybe?
@@ -430,26 +425,6 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |||
|
|||
useEffect(() => () => everVisibleMessagesTsRef.current.clear(), []) | |||
|
|||
useEffect(() => { |
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.
double check this– make sure it wasn't a merge conflict
color: "var(--vscode-foreground)", | ||
}}></span> | ||
<span | ||
style={{ |
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.
maybe tailwind-ify :D
Co-authored-by: Christiaan Arnoldus <[email protected]>
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.
Still a bunch of spots where we could be better with the kilocode_change comments. I'm sorry– I know they are annoying!
I think my main thought is this is going to be a slight-nightmare to merge with Roo... I wonder if we should open this PR against them to save ourselves difficulty down the line. @kevinvandijk might have thoughts, but I don't want to slow you down more
@@ -88,6 +88,7 @@ import { parseMentions } from "../mentions" // kilocode_change | |||
import { parseKiloSlashCommands } from "../slash-commands/kilo" // kilocode_change | |||
import { GlobalFileNames } from "../../shared/globalFileNames" // kilocode_change | |||
import { ensureLocalKilorulesDirExists } from "../context/instructions/kilo-rules" // kilocode_change | |||
import { processFilesIntoText } from "../../integrations/misc/process-files" |
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.
this file needs a bunch of kilocode_change
case "selectImages": | ||
const images = await selectImages() | ||
await provider.postMessageToWebview({ type: "selectedImages", images }) | ||
const { images, files } = await selectFiles() | ||
await provider.postMessageToWebview({ type: "selectedImages", images, filePaths: files }) |
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.
Would be possible to keep Roo's selectImages, but then also add our own selectFiles below this that only deals with files?
If we have to have both, I would rename "Add file" to "Reference file" and "Import" to "Upload". But tbh I don't understand why we need both, "Add file" would work with external files too if the UI allowed selecting them (or if you mention their paths manually). |
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.
Im still seeing missing //kilocode_changes
and there still seems to be some code that was unintentionally changed in webview-ui/src/components/chat/ChatView.tsx
. I'd recommend going into the full diff view of the PR and be very thorough with the // kilocode_changes
. I didn't want to leave a bunch of comments, but there's still quite a few missing spots. Edit sorry, some of those changes were definitely intended! but maybe wrap them in useCallback
(the code in question)
const handleSuggestionClickInRow = useCallback(
(answer: string, event?: React.MouseEvent) => {
if (event?.shiftKey) {
// Always append to existing text, don't overwrite
setInputValue((currentValue) => {
return currentValue !== "" ? `${currentValue} \n${answer}` : answer
})
} else {
handleSendMessage(answer, [])
}
},
[handleSendMessage, setInputValue], // setInputValue is stable, handleSendMessage depends on clineAsk
)
const handleBatchFileResponse = useCallback((response: { [key: string]: boolean }) => {
// Handle batch file response, e.g., for file uploads
vscode.postMessage({ type: "askResponse", askResponse: "objectResponse", text: JSON.stringify(response) })
}, [])
text, | ||
images, | ||
files, | ||
}) | ||
break | ||
// kilocode_change start |
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.
maybe just move this up to include your changes? // kilocode_change start
console.error("Error selecting files and images:", error) | ||
} | ||
}, [selectedImages, selectedFiles]) | ||
// kilocode_change end |
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.
combine
onToggleExpand={() => toggleRowExpansion(messageOrGroup.ts)} | ||
lastModifiedMessage={modifiedMessages.at(-1)} | ||
isLast={index === groupedMessages.length - 1} | ||
onHeightChange={handleRowHeightChange} |
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.
Unintentional changes here?
if (event?.shiftKey) { | ||
// Always append to existing text, don't overwrite | ||
setInputValue((currentValue) => { | ||
return currentValue !== "" ? `${currentValue} \n${answer}` : answer | ||
}) | ||
} else { | ||
handleSendMessage(answer, [], []) | ||
} | ||
}} |
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.
should this be a useCallback like the previous version?
Context
Adding the ability to upload more file types specifically "xml", "json", "txt", "log", "md", "docx", "ipynb", "pdf".
Fixes #577
Screenshots
Screen.Recording.2025-06-05.at.9.22.48.PM.mov
How to Test
Import a file type that's been recently added and see if it is able to read and parse through the file!