Skip to content

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

Open
octo-patch wants to merge 1 commit 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 1 commit 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.

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