-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix image drop from main editor #7910
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
base: main
Are you sure you want to change the base?
Fix image drop from main editor #7910
Conversation
…-image-drop-from-main-editor
…-image-drop-from-main-editor
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.
10 issues found across 9 files
Prompt for AI agents (all 10 issues)
Understand the root cause of the following 10 issues and fix them.
<file name="extensions/vscode/src/extension/VsCodeMessenger.ts">
<violation number="1" location="extensions/vscode/src/extension/VsCodeMessenger.ts:295">
Non-PNG images are all forced to image/jpeg; gif/webp/svg (and others) will be mislabeled, causing incorrect data URLs.</violation>
<violation number="2" location="extensions/vscode/src/extension/VsCodeMessenger.ts:295">
Extension comparison is case-sensitive; uppercase extensions (e.g., .PNG) are misclassified, producing the wrong MIME type.</violation>
</file>
<file name="gui/src/components/mainInput/TipTapEditor/TipTapEditor.tsx">
<violation number="1" location="gui/src/components/mainInput/TipTapEditor/TipTapEditor.tsx:226">
Unconditional hide overrides delayed/conditional logic in onDragLeave, making the delay and Shift-key behavior ineffective.</violation>
<violation number="2" location="gui/src/components/mainInput/TipTapEditor/TipTapEditor.tsx:241">
onDrop does not prevent default, which can cause the browser to navigate/open the dropped file when dropping outside the editor area. Add event.preventDefault() to avoid data loss risk.</violation>
</file>
<file name="gui/src/components/mainInput/TipTapEditor/utils/editorConfig.ts">
<violation number="1" location="gui/src/components/mainInput/TipTapEditor/utils/editorConfig.ts:151">
Drop handler prevents default and returns true even when nothing is handled, likely blocking normal text/URL drops. Consider only preventing default/returning true when an image is actually processed, otherwise fall through.</violation>
<violation number="2" location="gui/src/components/mainInput/TipTapEditor/utils/editorConfig.ts:189">
Image from HTML drop is inserted at position 0 instead of the intended drop/caret position.</violation>
</file>
<file name="gui/src/components/mainInput/TipTapEditor/utils/imageUtils.ts">
<violation number="1" location="gui/src/components/mainInput/TipTapEditor/utils/imageUtils.ts:44">
Avoid logging absolute local file paths to prevent leaking sensitive user information.</violation>
<violation number="2" location="gui/src/components/mainInput/TipTapEditor/utils/imageUtils.ts:66">
Avoid logging the full response object; it may contain large base64 data and sensitive content. Log only metadata (e.g., type or status) instead.</violation>
<violation number="3" location="gui/src/components/mainInput/TipTapEditor/utils/imageUtils.ts:146">
Logging the entire HTML content can expose sensitive data and add overhead. Prefer logging a short, non-sensitive summary.</violation>
<violation number="4" location="gui/src/components/mainInput/TipTapEditor/utils/imageUtils.ts:154">
Do not log full resource URLs that may include local file paths; log a generic message instead.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai
to give feedback, ask questions, or re-run the review.
const fileUri = vscode.Uri.file(filepath); | ||
const fileContents = await vscode.workspace.fs.readFile(fileUri); | ||
const fileType = | ||
filepath.split(".").pop() === "png" ? "image/png" : "image/jpeg"; |
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.
Non-PNG images are all forced to image/jpeg; gif/webp/svg (and others) will be mislabeled, causing incorrect data URLs.
Prompt for AI agents
Address the following comment on extensions/vscode/src/extension/VsCodeMessenger.ts at line 295:
<comment>Non-PNG images are all forced to image/jpeg; gif/webp/svg (and others) will be mislabeled, causing incorrect data URLs.</comment>
<file context>
@@ -287,6 +287,18 @@ export class VsCodeMessenger {
+ const fileUri = vscode.Uri.file(filepath);
+ const fileContents = await vscode.workspace.fs.readFile(fileUri);
+ const fileType =
+ filepath.split(".").pop() === "png" ? "image/png" : "image/jpeg";
+ const dataUrl = `data:${fileType};base64,${Buffer.from(
+ fileContents,
</file context>
const fileUri = vscode.Uri.file(filepath); | ||
const fileContents = await vscode.workspace.fs.readFile(fileUri); | ||
const fileType = | ||
filepath.split(".").pop() === "png" ? "image/png" : "image/jpeg"; |
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.
Extension comparison is case-sensitive; uppercase extensions (e.g., .PNG) are misclassified, producing the wrong MIME type.
Prompt for AI agents
Address the following comment on extensions/vscode/src/extension/VsCodeMessenger.ts at line 295:
<comment>Extension comparison is case-sensitive; uppercase extensions (e.g., .PNG) are misclassified, producing the wrong MIME type.</comment>
<file context>
@@ -287,6 +287,18 @@ export class VsCodeMessenger {
+ const fileUri = vscode.Uri.file(filepath);
+ const fileContents = await vscode.workspace.fs.readFile(fileUri);
+ const fileType =
+ filepath.split(".").pop() === "png" ? "image/png" : "image/jpeg";
+ const dataUrl = `data:${fileType};base64,${Buffer.from(
+ fileContents,
</file context>
filepath.split(".").pop() === "png" ? "image/png" : "image/jpeg"; | |
(filepath.split(".").pop() || "").toLowerCase() === "png" ? "image/png" : "image/jpeg"; |
} | ||
}); | ||
event.preventDefault(); | ||
// Let the event bubble to ProseMirror by not preventing default |
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.
onDrop does not prevent default, which can cause the browser to navigate/open the dropped file when dropping outside the editor area. Add event.preventDefault() to avoid data loss risk.
Prompt for AI agents
Address the following comment on gui/src/components/mainInput/TipTapEditor/TipTapEditor.tsx at line 241:
<comment>onDrop does not prevent default, which can cause the browser to navigate/open the dropped file when dropping outside the editor area. Add event.preventDefault() to avoid data loss risk.</comment>
<file context>
@@ -221,40 +222,23 @@ function TipTapEditorInner(props: TipTapEditorProps) {
- }
- });
- event.preventDefault();
+ // Let the event bubble to ProseMirror by not preventing default
}}
>
</file context>
// Let the event bubble to ProseMirror by not preventing default | |
event.preventDefault(); |
} else { | ||
setTimeout(() => setShowDragOverMsg(false), 2000); | ||
setTimeout(() => { | ||
setShowDragOverMsg(false); |
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.
Unconditional hide overrides delayed/conditional logic in onDragLeave, making the delay and Shift-key behavior ineffective.
Prompt for AI agents
Address the following comment on gui/src/components/mainInput/TipTapEditor/TipTapEditor.tsx at line 226:
<comment>Unconditional hide overrides delayed/conditional logic in onDragLeave, making the delay and Shift-key behavior ineffective.</comment>
<file context>
@@ -221,40 +222,23 @@ function TipTapEditorInner(props: TipTapEditorProps) {
} else {
- setTimeout(() => setShowDragOverMsg(false), 2000);
+ setTimeout(() => {
+ setShowDragOverMsg(false);
+ }, 2000);
}
</file context>
const plugin = new Plugin({ | ||
props: { | ||
handleDOMEvents: { | ||
drop(view, event) { |
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.
Drop handler prevents default and returns true even when nothing is handled, likely blocking normal text/URL drops. Consider only preventing default/returning true when an image is actually processed, otherwise fall through.
Prompt for AI agents
Address the following comment on gui/src/components/mainInput/TipTapEditor/utils/editorConfig.ts at line 151:
<comment>Drop handler prevents default and returns true even when nothing is handled, likely blocking normal text/URL drops. Consider only preventing default/returning true when an image is actually processed, otherwise fall through.</comment>
<file context>
@@ -147,6 +148,80 @@ export function createEditorConfig(options: {
const plugin = new Plugin({
props: {
handleDOMEvents: {
+ drop(view, event) {
+ // Hide drag overlay immediately when drop is handled
+ setShowDragOverMsg(false);
</file context>
const node = schema.nodes.image.create({ | ||
src: dataUrl, | ||
}); | ||
const tr = view.state.tr.insert(0, node); |
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.
Image from HTML drop is inserted at position 0 instead of the intended drop/caret position.
Prompt for AI agents
Address the following comment on gui/src/components/mainInput/TipTapEditor/utils/editorConfig.ts at line 189:
<comment>Image from HTML drop is inserted at position 0 instead of the intended drop/caret position.</comment>
<file context>
@@ -147,6 +148,80 @@ export function createEditorConfig(options: {
+ const node = schema.nodes.image.create({
+ src: dataUrl,
+ });
+ const tr = view.state.tr.insert(0, node);
+ view.dispatch(tr);
+ }
</file context>
return undefined; | ||
} | ||
|
||
console.log("Extracted filepath:", filepath); |
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 logging absolute local file paths to prevent leaking sensitive user information.
Prompt for AI agents
Address the following comment on gui/src/components/mainInput/TipTapEditor/utils/imageUtils.ts at line 44:
<comment>Avoid logging absolute local file paths to prevent leaking sensitive user information.</comment>
<file context>
@@ -2,6 +2,159 @@ import { IIdeMessenger } from "../../../../context/IdeMessenger";
+ return undefined;
+ }
+
+ console.log("Extracted filepath:", filepath);
+
+ try {
</file context>
console.log("Extracted filepath:", filepath); | |
console.log("Extracted filepath"); |
return undefined; | ||
} | ||
|
||
console.log("Found VS Code resource URL:", vscodeResourceUrl); |
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.
Do not log full resource URLs that may include local file paths; log a generic message instead.
Prompt for AI agents
Address the following comment on gui/src/components/mainInput/TipTapEditor/utils/imageUtils.ts at line 154:
<comment>Do not log full resource URLs that may include local file paths; log a generic message instead.</comment>
<file context>
@@ -2,6 +2,159 @@ import { IIdeMessenger } from "../../../../context/IdeMessenger";
+ return undefined;
+ }
+
+ console.log("Found VS Code resource URL:", vscodeResourceUrl);
+ return await handleVSCodeResourceUrl(ideMessenger, vscodeResourceUrl);
+}
</file context>
console.log("Found VS Code resource URL:", vscodeResourceUrl); | |
console.log("Found VS Code resource URL"); |
ideMessenger: IIdeMessenger, | ||
html: string, | ||
): Promise<string | undefined> { | ||
console.log("Processing HTML for VS Code resource URL:", html); |
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.
Logging the entire HTML content can expose sensitive data and add overhead. Prefer logging a short, non-sensitive summary.
Prompt for AI agents
Address the following comment on gui/src/components/mainInput/TipTapEditor/utils/imageUtils.ts at line 146:
<comment>Logging the entire HTML content can expose sensitive data and add overhead. Prefer logging a short, non-sensitive summary.</comment>
<file context>
@@ -2,6 +2,159 @@ import { IIdeMessenger } from "../../../../context/IdeMessenger";
+ ideMessenger: IIdeMessenger,
+ html: string,
+): Promise<string | undefined> {
+ console.log("Processing HTML for VS Code resource URL:", html);
+
+ const vscodeResourceUrl = extractVSCodeResourceUrlFromHtml(html);
</file context>
console.log("Processing HTML for VS Code resource URL:", html); | |
console.log("Processing HTML for VS Code resource URL"); |
|
||
const response = await Promise.race([requestPromise, timeoutPromise]); | ||
|
||
console.log("Got response from ideMessenger.request:", response); |
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 logging the full response object; it may contain large base64 data and sensitive content. Log only metadata (e.g., type or status) instead.
Prompt for AI agents
Address the following comment on gui/src/components/mainInput/TipTapEditor/utils/imageUtils.ts at line 66:
<comment>Avoid logging the full response object; it may contain large base64 data and sensitive content. Log only metadata (e.g., type or status) instead.</comment>
<file context>
@@ -2,6 +2,159 @@ import { IIdeMessenger } from "../../../../context/IdeMessenger";
+
+ const response = await Promise.race([requestPromise, timeoutPromise]);
+
+ console.log("Got response from ideMessenger.request:", response);
+ console.log("Response type:", typeof response);
+
</file context>
console.log("Got response from ideMessenger.request:", response); | |
console.log("Got response from ideMessenger.request"); |
@aadarshkt it doesn't appear that I have access to push to this PR, could you either change this or run the following and add a commit? git checkout main -- .vscode/launch.json
git checkout main -- .vscode/tasks.json |
Description
#7908
Reopening this with requested changes: