Skip to content

feat(ask-code): send attached images to an image-capable MiniMax model - #262

Open
octo-patch wants to merge 2 commits into
johannesjo:mainfrom
octo-patch:octo/20260817-input-capability-recvs9d7W3SH0j
Open

feat(ask-code): send attached images to an image-capable MiniMax model#262
octo-patch wants to merge 2 commits into
johannesjo:mainfrom
octo-patch:octo/20260817-input-capability-recvs9d7W3SH0j

Conversation

@octo-patch

Copy link
Copy Markdown
Contributor

Reason: The inline code Q&A already resolves pasted images to temp file paths, but the MiniMax provider could only send a text prompt, so an attached image never reached a model that accepts image input.

What changed

  • electron/ipc/ask-code-minimax.ts — a request can now carry imagePaths. When images are attached, the user message is sent as content parts (a text part plus one image_url data URL per image) instead of a plain string, and the request is routed to a model whose catalog input modalities include images (MiniMax-M3, which accepts text, image and video input). MiniMax-M2.7 is text-only and stays the default for text questions, so text-only requests send exactly the same payload as before. Image input is validated and capped separately from the 50,000-character prompt limit, since the bytes never count against it: at most 4 images per question, 10 MB each, .png / .jpg / .jpeg / .webp / .gif. Unsupported or excessive input is rejected before a request slot is taken, and an unreadable file is reported on the response channel.
  • electron/ipc/ask-code.ts, electron/ipc/register.ts — forward the new imagePaths argument and validate it, running every entry through the existing absolute-path check. The other Q&A backend is unchanged and still receives a text prompt only.
  • src/components/InlineInput.tsx — pasting an image into the inline Ask input attaches it, reusing the existing resolve_clipboard_paste handler that the terminal already uses, so no new IPC channel is added. The affordance is only active in Ask mode while the MiniMax provider is selected, and a small chip shows the attachment and clears it.
  • src/components/ReviewProvider.tsx, src/components/ScrollingDiffView.tsx, src/components/PlanViewerDialog.tsx, src/components/AskCodeCard.tsx — thread the attached paths from the inline input through the question to the request. Both Ask surfaces share the same input, so both gain the capability.

New tests in electron/ipc/ask-code-minimax.test.ts cover the model modality lookup, the unchanged text-only payload, image parts sent as data URLs to the image-capable model, a rejected image type, the per-question image cap, and an unreadable image surfacing as an error without a request being sent.

Checks

  • npx vitest run — 115 files passed, 1873 tests passed, 22 skipped
  • npx vitest run --config vitest.client.config.ts — 2 files passed, 9 tests passed
  • npx tsc --noEmit and npx tsc -p electron/tsconfig.json — clean
  • npx eslint . --max-warnings 0 — clean
  • npx prettier --check . — clean
  • npm run lint:arch — no dependency violations
  • npm run lint:dead — clean

The inline code Q&A resolved pasted images to temp file paths, but the
MiniMax backend only ever sent a text prompt, so the image was dropped.

Requests now carry image paths, the user message becomes text plus
image_url content parts when images are attached, and such requests go to
a model whose catalog input modalities include images. Text-only requests
keep the previous payload and model. Image input is validated and capped
separately from the prompt length limit.

@johannesjo johannesjo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Review

Applied locally and verified npx tsc --noEmit (both configs) and npx eslint are clean. I couldn't run vitest in my environment, so I'm taking the suite results from the description.

The backend half is in good shape. The async refactor of the fetch chain preserves the session lifecycle correctly — a failed image read lands in the existing .catch, which calls session.cleanup() and session.complete(), so the registry slot is released and exactly one done is sent; cancel and timeout during the read both abort the controller before fetch is reached. Text-only requests serialize to byte-identical JSON. Every existing test awaits waitForDone before asserting on mockFetch, so deferring the call by a microtask doesn't disturb them.

I also checked the provider constants against MiniMax's docs rather than assuming: MiniMax-M3 and MiniMax-M2.7 are both real model IDs, M3 takes text/image/video, image_url carrying a base64 data URL is the documented content-part shape for the OpenAI-compatible endpoint, and the 10 MB cap plus the PNG/JPEG/WEBP/GIF list match the published limits. Those are all correct.

The problems are on the renderer side, where the attachment is produced.

1. Blocker — every clipboard image resolves to the same fixed temp path

electron/ipc/register.ts:943 defines clipboardImagePath as a single fixed file, os.tmpdir()/parallel-code-clipboard.png, overwritten on every ResolveClipboardPaste and never removed. Two consequences:

Multi-image is unreachable. InlineInput.handlePaste dedupes with prev.includes(attached). Since every paste returns that same path, a second paste never adds a second entry — it silently overwrites the first image's bytes on disk while the chip still reads "1 image". Two images cannot be attached through the only producer this PR wires up. That makes MAX_IMAGES_PER_REQUEST = 4, the parts loop, the ${n} images × plural label, and the "rejects more images than a single question allows" test all cover a path nothing can reach.

The wrong image can be sent. Bytes are read at submit time (AskCodeCard onMountimageDataUrl), not at paste time. Between pasting into the Ask box and pressing Enter — while the user is still typing the question — any other paste that hits this handler rewrites the file. TerminalView.tsx:621 calls exactly this handler on the paste keybinding. Paste a screenshot into Ask, paste a different one into a terminal, submit: the question ships the terminal's image.

The codebase already solves this for the other image path — sanitizeDroppedName (register.ts:358) appends a timestamp and random suffix precisely "so two same-name drops landing in the same millisecond don't overwrite each other". Giving the clipboard resolver the same treatment would fix both symptoms.

2. Bug — pasting an image file never attaches anything

ResolveClipboardPaste checks file references first and returns { kind: 'file' } for them (register.ts:958-962); only a raster clipboard image reaches the kind: 'image' branch. handlePaste accepts kind === 'image' only. So copying an image file in Finder or Nautilus and pasting it into the Ask input attaches nothing — and because preventDefault() already fired on the image/* clipboard item, the paste is swallowed silently with no attachment and no message. Where the item isn't image-typed, the fallback is to paste the file path as text into the question box, which is also not what was asked for.

Worth noting the comment right above that handler calls the Finder-copy case out as the reason the resolver exists at all. Accepting kind: 'file' when the extension is supported would cover it.

3. Settings still advertises M2.7 while images silently switch models

SettingsDialog.tsx:609 reads MiniMax (M2.7) and :644 says "Uses MiniMax M2.7 (204K context)". Attaching an image routes to M3 — different model, different context window, different pricing — with no signal in the picker, the input chip, or the answer card. This file isn't touched by the PR, so the copy is now stale.

4. MIME type is inferred from the extension alone

imageDataUrl builds data:image/png;base64,… from path.extname. A .png holding JPEG bytes goes out mislabeled and returns an opaque API error. MiniMax's own multimodal guide explicitly recommends validating the file signature rather than trusting a supplied extension or MIME type. A four-byte magic-number check is cheap and also tightens item 7.

5. Size check runs after the whole file is buffered

imageDataUrl does fs.promises.readFile and only then tests bytes.byteLength > MAX_IMAGE_BYTES, so an oversized .png is fully resident in the main process before it's rejected. A stat first is one line. Relatedly, 4 × 10 MB is roughly 53 MB of base64 against the documented 64 MB request-body ceiling — it fits today, but there's no aggregate guard, so raising either constant later fails at the provider rather than locally.

6. The modality catalog is dead weight

MINIMAX_INPUT_MODALITIES, MinimaxInputModality (including an unused 'video' member), and the exported minimaxModelAcceptsImages exist to answer one static question. resolveModel's guard minimaxModelAcceptsImages(MINIMAX_MODEL) is a compile-time-constant false — a branch that can never be taken. And assertImagesSupported and imageDataUrl throw the identical Unsupported image type error for the same condition. This collapses to const model = imagePaths.length ? MINIMAX_IMAGE_INPUT_MODEL : MINIMAX_MODEL plus one extension→MIME lookup, taking the exported helper and one test with it.

7. Test gaps

expect(body.model).toBe(MINIMAX_IMAGE_INPUT_MODEL) is self-referential — it passes for any string, so a wrong model ID would ship green. The constants happen to be right, but the test doesn't establish that. Also uncovered: MAX_IMAGE_BYTES rejection, and that the claude provider drops imagePaths rather than choking on them.

8. Note on path validation

validatePath enforces only absolute-and-no-.., so any absolute path with an image extension gets read by the main process and base64'd to a third-party API. That matches how the other handlers in register.ts treat renderer input, so it isn't a regression — but it's the first one that sends file contents off-machine, which seems worth being deliberate about.


Summary: items 1 and 2 mean the feature doesn't reliably do what it says on the box — between them, the common ways to attach an image either silently do nothing or can send the wrong file. Both are worth fixing before merge; 3, 4 and 6 are cheap in the same pass. The rest are fine as follow-ups.

@octo-patch

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I updated clipboard image handling to use a unique temporary path for each paste and now accept supported pasted image files. I added focused coverage and ran both TypeScript configurations plus ESLint on the changed files.

@johannesjo johannesjo left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Review (round 2 — head 06edd6c)

Re-applied locally: npx vitest run on the three touched test files → 35 passed, and npx tsc --noEmit is clean on both configs.

Items 1 and 2 are genuinely fixed. createClipboardImagePath() (electron/ipc/register.ts:371) routes through sanitizeDroppedName, so each paste gets parallel-code-drop-<ms>-<6 hex>-clipboard.png. Both symptoms are gone: two pastes now produce two distinct entries, and a concurrent terminal paste can no longer rewrite the bytes the Ask card reads at submit time. handlePaste accepting kind === 'file' behind isSupportedAskCodeImagePath (InlineInput.tsx:68-72) covers the Finder/Nautilus copy. Reusing sanitizeDroppedName rather than inventing a second naming scheme was the right call, and exporting it for a uniqueness test is a nice touch.

Items 3–8 were not addressed and not replied to. I've re-checked each against this head: SettingsDialog.tsx:609 still reads MiniMax (M2.7) and :644 still says "Uses MiniMax M2.7 (204K context)"; resolveModel's guard at ask-code-minimax.ts:69 is still a compile-time-constant false; the size check still runs after the full buffer; expect(body.model).toBe(MINIMAX_IMAGE_INPUT_MODEL) is still self-referential. If any of those were deliberate deferrals that's fine — but they need a "won't do, because…" rather than silence, since I can't tell which were considered.

Below is only what's new since the last round.

1. The fix for item 1 makes PRIVACY.md factually wrong

PRIVACY.md:78 currently states, as a deliberate distinction:

clipboard pastes overwrite a single file at $TMPDIR/parallel-code-clipboard.png, and drops are written to $TMPDIR/parallel-code-drop-<timestamp>-<random>[-<original-filename>] … These temp files are not deleted by Parallel Code; they remain in your temp directory until your OS cleans it

After this PR, clipboard pastes no longer overwrite a single file — they join the never-deleted parallel-code-drop-* set. PRIVACY.md:104 lists both names and is stale for the same reason.

That's a behavioural change worth being deliberate about rather than an accident of the fix. TerminalView.tsx:621 calls this resolver on every terminal paste keybinding, so pasting screenshots into a terminal now accretes multi-MB PNGs in $TMPDIR where it previously held at exactly one file. Uniqueness is still the correct fix for item 1 — the question is whether you want to (a) accept the growth and update both PRIVACY.md lines, or (b) add a startup sweep of parallel-code-drop-* older than N hours, which would also let the existing drop-path wording get better. Either is fine; leaving the doc asserting the old guarantee is not.

2. Attaching requires switching to Ask before pasting

mode() defaults to 'review' (InlineInput.tsx:24) and handlePaste early-returns on !imageInputEnabled() (:59). So the natural order — select code, inline input opens in Comment mode, paste the screenshot, then click Ask — drops the image with no feedback: no preventDefault, and an image-only clipboard puts nothing in the text field, so nothing visibly happens. The user has to know to re-paste.

Attaching regardless of mode and leaving the existing imageInputEnabled() gate in submit() (:51) to decide what actually gets sent removes the ordering trap at no cost.

3. Item 2's residue: image-typed clipboard content that can't attach is swallowed

e.preventDefault() fires at :65 as soon as any clipboard item is image/*, but the attach at :68-72 only lands for kind === 'image' or a kind === 'file' with a supported extension. Copy a .bmp, .avif, or .svg file in the file manager and paste it into Ask: the resolver returns kind: 'file', the extension check rejects it, and the paste vanishes — no attachment, no text, no message, only a logWarn the user never sees. Narrower than the original item 2, but the same silent-no-op shape, and ask-code-image.test.ts already asserts .svg is rejected, so the path is known-reachable. A one-line inline hint on the non-attaching branches would close it.

4. Minor — two independent lists of supported extensions

src/components/ask-code-image.ts:1 and IMAGE_MIME_TYPES at ask-code-minimax.ts:50 agree today. If they drift, the renderer attaches a file the backend rejects at submit time with Unsupported image type. InlineInput.tsx:5 already imports from electron/ipc/channels, so a shared constants module is the established pattern here — ask-code-minimax.ts itself can't be imported into the renderer (it pulls in fs), but the extension→MIME map could live in its own file consumed by both sides.


Summary: the two blockers are properly fixed and the feature now works for its main flows — nice work on the sanitizeDroppedName reuse. What's left: PRIVACY.md now describes behaviour this PR changed, two attach flows still fail silently, and six items from the last round need either a fix or an answer. Item 3 in particular ships user-visible copy that is wrong about which model runs, which seems like the cheapest thing on the list.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants