Repository navigation
feat(staged): accept PDFs as image attachments by rendering the first page to PNG - #975
Merged
Merged
Conversation
…achments A PDF dropped into Staged never reached the image store: the frontend extension allowlist routed it down the text-file path (a UTF-8 error toast on the branch card, or the literal path inserted into the chat textarea), paste and the file picker filtered on `image/*`, and the backend sniffer had no PDF branch. Backend: `store_sniffed_image`, the single choke point for chat and timeline attachments, now detects `%PDF-` and rasterizes page one with CoreGraphics in-process (new `note_media::pdf`, via the already-locked `objc2-core-graphics`/`objc2-core-foundation` crates) before the usual PNG sniff, renaming `<stem>.pdf` to `<stem>.png`. The page is drawn on an opaque white background at `min(2048 / long_edge_pt, 4.0)` px/pt, so the long edge never exceeds 2048 px and tiny pages are not blown up past 4x; `/Rotate` is honoured; if the PNG exceeds MAX_IMAGE_SIZE it is re-rendered once at half scale before failing with the existing size message. Malformed input is rejected by `CGPDFDocument` immediately (no subprocess, no hang). The renderer reports the page count for a later one-attachment-per-page follow-up; the PDF bytes are not kept. `create_image` and `create_image_from_data` are now async and run on `spawn_blocking` so rendering stays off the main thread; the web dispatcher keeps calling the synchronous impl. Non-macOS targets compile a stub returning "PDF attachments require macOS" and the CoreGraphics tests are cfg-gated, so the ubuntu CI build and clippy stay green. Note ingest (`validate_media`) still rejects `.pdf`. Frontend: `isImageAttachmentFile` (images + `.pdf`) drives the three native drop handlers and `isMaybeTextFile`; paste accepts `application/pdf` alongside `image/*`; both pickers list it via a shared ATTACHMENT_ACCEPT; local base64 previews are used only when the browser type matches the stored MIME, otherwise the existing `getImageData` effect fetches the rendered PNG; branch-card `createImage` failures now toast like the text path instead of failing silently. Verified: a 3150x1800 pt one-page fixture renders to a 2048x1170 opaque PNG (~206 KB) that matches the source; `cargo test` (1061 passed), `cargo clippy -D warnings`, `cargo fmt --check`, vitest (1317 passed, --maxWorkers=3), `pnpm run check` (0 errors/warnings), prettier. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…ff the runtime thread Follow-ups from review of 2760cdd (PDF attachments as PNG images). `render_first_page` chose the MediaBox and only fell back to the CropBox when the MediaBox was unusable. Preview, Quick Look and most viewers display the CropBox, so a print-ready PDF with bleed and crop marks rendered a different image from what the user sees when they open the file. The order is now CropBox first — CoreGraphics already clips it to the MediaBox and returns the MediaBox when none is set — with the MediaBox as the fallback for a degenerate CropBox. New test: a letter MediaBox with a 300x400 pt CropBox renders at 1200x1600 (4x cap) with only the in-crop content, and an empty CropBox still falls back to the full page. `tiny_pdf_with` gained a `crop_box` argument for it. The test-only `PAGE_TWO` constant moved inside the macOS test module so the Linux `cargo test` step no longer warns that it is never used. The web dispatcher's `create_image_from_data` arm now runs the impl under `tauri::async_runtime::spawn_blocking` like the other heavy arms, so PDF rasterization no longer blocks the async runtime in web mode; the `?`/`??` error shape is unchanged with a "task failed" join error. Verified: the 3150x1800 pt onboarding flow chart fixture (CropBox equal to MediaBox) still renders to 2048x1170, 205,763 bytes; `cargo test` (1062 passed), `cargo clippy -D warnings`, `cargo fmt --check`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…the attach UI Follow-ups from review of 2bdff35 (PDF attachments as PNG images). Only the rendered PNG of a PDF's first page is stored, but the 10 MB MAX_IMAGE_SIZE was applied to the PDF bytes themselves, so scans and print-ready files (routinely 10–50 MB) were refused with "media exceeds 10 MB limit" even though their first page rasterizes to a few hundred KB. `%PDF-` payloads now get MAX_PDF_INPUT_SIZE (50 MiB), chosen by a shared `attachment_limit(header)` on every route: `store_sniffed_image` for in-memory bytes, the new `read_local_attachment` for the path route (it reads the 5-byte signature first and refuses an oversized non-PDF from its metadata, so a 10 MB+ image is never buffered towards 50 MiB), and `create_image_from_data_impl`, which decodes the first two base64 quanta to pick the cap before decoding the payload. An oversized PDF now fails with "PDF exceeds 50 MB limit"; images keep "media exceeds 10 MB limit" (the base64 route previously said "Image too large", and now matches the message the same bytes get elsewhere). The rendered PNG still has to satisfy MAX_IMAGE_SIZE via the existing half-scale retry. `read_local` is split into `open_regular` + `read_capped` so both readers share the FIFO-safe open and the grew-past-cap check, and the "exceeds N MB limit" strings come from one `size_error` helper. The frontend read paths (SessionChatPane, ImageAttachment) impose no cap of their own, so nothing changes there. Tests: a well-formed PDF padded past 10 MiB with comment lines (`padded_pdf`, xref offsets skip the padding so it still renders instantly) is accepted on both routes and stored as a PNG under 10 MB; a >50 MiB PDF is refused with the new message on both routes (sparse file on the path route, a 67 MB base64 string on the data route); a >10 MiB PNG is still refused with the old message on both; and `read_local_attachment` is covered directly for the cap decision, sub-signature files, directories, and missing files. UI copy: the picker buttons now say "Attach image or PDF" / "Add image or PDF" / "Attach images or PDFs", and docs/inline-note-media.md states that a dropped, pasted, or picked PDF (up to 50 MiB) is stored as a PNG of its first page and is still refused as inline note media. BranchCard's createImage failure toast uses `attachBasename`, like the composer paths, so it reads `Could not attach "report.pdf" — …` rather than dropping the extension that explains the failure. The pdf.rs "page two was not rendered" assertion sampled a pixel that is white on either page. PAGE_TWO now draws a rule at y=400 pt that PAGE_ONE lacks; the test asserts that row is white in the rendered first page and dark when PAGE_TWO's content is rendered as page one, so the check is self-validating. Verified: `cargo test` (1064 passed), `cargo clippy -D warnings`, `cargo fmt --check`, vitest (1317 passed, --maxWorkers=3), `pnpm run check` (0 errors/warnings), prettier on the changed Svelte files (the docs file's only prettier complaint is a pre-existing one on line 18). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Follow-ups from review of 7251de3 (PDF attachment input cap). In web mode `create_image_from_data`'s MAX_PDF_INPUT_SIZE check never ran: `invoke_command` takes `Json<Value>` and the router had no body limit layer, so axum 0.8's default 2 MiB cap rejected any `/api/invoke/create_image_from_data` body larger than that with a 413 before dispatch. The desktop path route accepted the same PDF, so the "up to 50 MiB" promise in docs/inline-note-media.md was false for the browser (and images over ~1.5 MB raw had been failing there all along). `/api/invoke/{command}` — only that route, via `MethodRouter::layer`, not the whole app — now carries `DefaultBodyLimit::max( INVOKE_BODY_LIMIT)`, where the constant is the base64 size of a MAX_PDF_INPUT_SIZE payload (`div_ceil(3) * 4`) plus a 64 KiB margin for the JSON envelope, derived from the note_media::files constant so the two cannot drift. The route construction moved into `invoke_router`, generic over the handler like `media_router_for_store`, because `WebAppState` holds a Tauri `AppHandle` and cannot be built in a unit test; `app_router` calls it with `invoke_command`. The new test mounts a stand-in with the same `Path`/`Json<Value>` extractors and checks that a body holding the base64 of a cap-sized PDF (well past 2 MiB) reaches the handler with its payload intact, while one byte over INVOKE_BODY_LIMIT gets 413. The docs sentence is now true for both desktop and web, so it is unchanged. Out of scope, noted for later: axum's 413 body is plain text ("Failed to buffer the request body: length limit exceeded"), not the `{ "error": … }` JSON `invoke_command` emits, and `transport.ts` falls through its JSON.parse and surfaces that text verbatim. The test pins that shape so a future change to the transport knows what it is handling. `read_image_file` in tool_results.rs produced "Image too large (max 10 MB)" for an oversized tool-result image; it now goes through the shared `size_error("Image", MAX_IMAGE_SIZE)` and reads "Image exceeds 10 MB limit", matching the attachment routes. Only its own test's expected substring changed; nothing in the frontend matched the old string. Verified: `cargo test` (1065 passed), `cargo clippy -D warnings`, `cargo fmt --check`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… body-limit test Follow-up from review of c625fb4 (web-mode invoke body limit). The over-limit half of `invoke_route_admits_a_cap_sized_pdf_body_and_ refuses_larger` asserted `text.contains("length limit exceeded")`, which is axum's internal `LengthLimitError` wording, so a routine axum bump could fail the test with no behaviour change on our side. The property `transport.ts` actually depends on is that the body is not the `{ "error": … }` JSON `invoke_command` emits, and the following `serde_json::from_str(text).is_err()` already captures that. The substring match is gone; the 413 status and non-JSON assertions remain, with the body echoed in the failure message so a future JSON 413 is still easy to diagnose. The comment above was already about the plain-text/non-JSON shape, so it is unchanged. Verified: `cargo test --lib web_server` (12 passed), `cargo fmt --check`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e7a92f2e3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| async function handleFileDrop(paths: string[]) { | ||
| if (!activeProjectId) return; | ||
| const imagePaths = paths.filter((p) => isImageFile(p)); | ||
| const imagePaths = paths.filter((p) => isImageAttachmentFile(p)); |
There was a problem hiding this comment.
When a .pdf dropped onto the new-session modal is malformed, encrypted, over 50 MiB, or otherwise rejected by createImage, this new classification removes it from textPaths, while the catch below only writes to the console. The user therefore sees neither an attachment nor an error, whereas the same failure is surfaced by the branch-card and session-chat drop handlers; report the rejection through the modal's attachment error UI or a toast.
Useful? React with 👍 / 👎.
| * File-picker `accept` list for the image store. A PDF is accepted alongside | ||
| * images because the backend stores page one of it as a rendered PNG. | ||
| */ | ||
| export const ATTACHMENT_ACCEPT = 'image/png,image/jpeg,image/gif,image/webp,application/pdf'; |
There was a problem hiding this comment.
Advertising PDFs here makes the newly allowed 50 MiB payload size reachable through the picker and paste flows, but both addImageFile implementations synchronously build thousands of strings, join them, and call btoa on the UI thread before invoking the backend. A PDF near the supported limit consequently creates large simultaneous binary/string/base64 allocations and freezes the WebView despite PDF rendering itself being moved to spawn_blocking; encode off-thread/use a native or binary upload path, or apply a substantially lower limit to these base64-based flows.
Useful? React with 👍 / 👎.
…ils and prompt pages Reworks the PDF attachment feature (2760cdd..1e7a92f) after feedback that the stored PNG of page one loses quality when zoomed and drops the other pages. A dropped, pasted, or picked PDF is now stored byte-for-byte as `<stem>.pdf` with `mime_type = application/pdf`, opened in the webview's native PDF viewer (vector zoom, every page), and rasterized only where a bitmap is unavoidable: the thumbnail slots and the agent prompt. Storage: `sniff_format` gains a `%PDF-` branch and `mime_for_extension` a `pdf` entry, so every `mime_for_extension(sniffed).unwrap()` has an entry. `store_sniffed_image` no longer renders; on macOS it asks CoreGraphics to open the document (`pdf::check`) so `%PDF-` garbage is still refused up front, and it writes through a new `write_media` (split out of `store_media_file`) because `validate_media` must keep refusing PDFs for notes: `size_limit` now returns "PDFs cannot be embedded in notes" for the MIME, since the shared map would otherwise have made `staged-media://<id>.pdf` refs ingestable. The 50 MiB PDF cap and both read routes are unchanged. `get_image_ids_for_session` filtered `mime_type LIKE 'image/%'`, which would have dropped PDFs on a session restart; it now admits `application/pdf` too. Thumbnails: `get_attachment_thumbnail(image_id)` (Tauri command and web dispatcher arm, both on `spawn_blocking`) returns `{ dataUrl, mimeType, filename }`. Images return their own data URL, so the frontend can ask for any attachment without knowing its MIME (chat history and queued messages only have ids); a PDF gets page one rendered at a 512 px long edge via `pdf::render_first_page` and cached as `<uuid>.thumb.png` beside the file (`store::images::thumbnail_file_path`). `dataUrl` is null when rendering is unavailable (non-macOS, or a page CoreGraphics cannot draw) and the UI shows a document icon. A new `remove_image_files` removes file and thumbnail together and replaces the four hand-rolled `remove_file` sites (`delete_image` in both dispatchers, `delete_media_file`, `cleanup_pending_images`); `move_branch_image_files` moves the thumbnail with the PDF. Agent prompt: neither transport takes a PDF. claude-agent-acp's `promptToClaude` ignores blob resources outright (`// Ignore blob resources (unsupported)`), codex-acp's app-server `InputItem` is text, image, or localImage, and ACP advertises nothing that would distinguish PDF support, so an `image` block carrying `application/pdf` would reach the Claude API as an image and be rejected. The fallback path is taken: `session_runner/attachments.rs` renders pages 1..=20 (`MAX_PROMPT_PAGES`) at `MAX_EDGE_PX` as `image/png` blocks, each preceded by a text block "page N of M of <filename>" (the last one also says which pages were left out), and a PDF that cannot be rendered is reported in the prompt text ("[Attached PDF x.pdf (N bytes) could not be rendered …]") rather than silently dropped. To carry the captions, `acp-client` replaces the `(base64, mime_type)` tuple with `PromptImage { data, mime_type, label }` across `AgentDriver::run`, `SessionConnection::prompt`, the queued turn, and `build_prompt_content_blocks`; the pikchr drivers and the simple wrapper only change their signatures. Frontend: `ImageViewerModal` detects a PDF from the `get_image_data` prefix, builds a Blob URL (an `<iframe>` will not load `data:`), shows it in a full-height `w-[90vw] h-[90vh]` frame, revokes the URL in the effect cleanup, and forwards Escape from the frame window where the webview allows a listener (the close button and backdrop remain for the plugin-rendered case). No CSP change was needed: `tauri.conf.json` has `csp: null` and the web shell sends none. A shared `AttachmentThumb` component fills the composer strip, chat-history bubble, and queued message slots with the thumbnail, a document icon while loading or when unrendered, and a "PDF" badge; `attachmentPreview.ts` holds the `localPreview` rule (use the uploaded bytes only for an `image/*` whose browser label matches the stored MIME), the data-URL helpers, and the Blob URL builder. Button copy says "attachment" where it said "image"; "Attach image or PDF" is unchanged. The timeline row keeps the image icon with the `.pdf` filename. docs/inline-note-media.md describes the new behaviour. Tests: PDF storage tests assert `application/pdf`, `.pdf`, identical bytes, and the renamed `pasted.pdf`/`scan.pdf` cases on every platform (the non-macOS test now checks storage, not an error); thumbnail tests cover an image's own data URL, the macOS render/cache/reuse/delete cycle, and the absent-renderer case; `pdf.rs` gains `check` and `render_pages` coverage; `attachments.rs` tests captioned pages, the page cap, the unrenderable note, and the non-macOS note; the acp-client test pins the text/image block order; `attachmentPreview.test.ts` covers the preview rule and Blob URL; `validate_media("a.pdf", …)` pins the new refusal. Verified: `cargo clippy -D warnings` (CI config; `--tests` shows only the two pre-existing baseline lints), `cargo test` (1070 passed; the two failures, `timeout_kills_bridge_and_native_child…` and `cancelled_capture_evicts_stale_inflight`, are the known shell-spawning load flakes at load average 42 and pass in isolation), acp-client `cargo test` (183 passed) and clippy, both `cargo fmt --check`, vitest (1321 passed, --maxWorkers=3), `pnpm run check` (0 errors/warnings), prettier on the changed files. Manual end-to-end with /tmp/pdf-support-fixture/onboarding-flow-chart.pdf through `store_sniffed_image` and the thumbnail command (temporary test, not committed): stored as application/pdf with identical bytes, 512x293 thumbnail in 250 ms then 308 µs from cache, one 2048x1170 prompt page labelled "page 1 of 1 of onboarding-flow-chart.pdf", both files gone after delete. Native rendering was checked with an offscreen WKWebView loading the same Blob-URL iframe: the flow chart renders at vector quality (Tauri and Safari share the engine). Headless Chrome hands the frame to its PDF viewer but does not paint page content in a screenshot, so web-mode page rendering and interactive zoom were not exercised here; the dev app was not launched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…d view PDFs from their media URL Follow-ups from review of c4b0950 (PDF attachments stored as PDF). Prompt pages had a page cap but no byte budget, so a 20-page scan could yield up to 200 MB of PNG rendered up front and sent in one JSON-RPC prompt, which the Claude API refuses (32 MB per request, 5 MB per image). `session_runner/attachments.rs` now renders pages lazily in order through a new `pdf::Document` (`open`, `page_count`, `render_page`; the eager `render_pages` is gone) and stops once the next page would cross `MAX_PROMPT_PAGE_BYTES` (20 MB of encoded bytes), so memory holds only the accepted set plus the page being decided. Each page is encoded within `MAX_PROMPT_IMAGE_SIZE` (5 MB) by `pdf::fit_page`: PNG, then the existing half-scale PNG retry, then — new, for the prompt only (`Fit { jpeg_fallback }`) — a full-scale JPEG at quality 85 via the `image` crate (already in the lock through arboard; its `jpeg` feature adds nothing new) before the page is given up and noted in the prompt text. The caption on the last page sent now says why the rest were left out ("pages 21–25 not included: over the 20-page limit" / "over the 20 MB image budget"; a single omission is named as "page N"), and when not even the first page fits the note goes into the prompt text instead of onto an earlier attachment's label. `PageLimits` carries pages, both byte caps, and the render edge (default `MAX_EDGE_PX`), so the tests render letter pages at 256 px: the page-cap test, two budget cases, the per-page refusal, and a `fit_page` chain test on stand-in bitmaps whose PNG/half-PNG/JPEG sizes order like a photographic page's. The thumbnail cache was a plain `fs::write` whose reader only checked the PNG signature, so two slots asking for a new attachment at once could serve a truncated PNG. `cache_thumbnail` writes to a per-writer `<uuid>.thumb.<pid>-<n>.tmp` (`thumbnail_temp_file_path`) and renames it into place, removes the temp file on failure, and `remove_image_files` sweeps any stray `<uuid>.thumb.*.tmp` with the file and thumbnail. Tests cover a clean render, a rename that cannot land, and a stray. `ImageViewerModal` now points the PDF `<iframe>` at the attachment's served media URL — `resolvePdfAttachmentUrl` beside `mediaUrl.ts` resolves `<id>.pdf` by id without widening the note-embed regex — so the file is streamed with Range instead of crossing IPC as base64 and being decoded with a per-char loop; `dataUrlBytes`/`pdfBlobUrl` and the Blob lifecycle are gone. Verified in an offscreen WKWebView with a `staged-media` scheme handler: the custom scheme renders the fixture natively inside an iframe (the frame is cross-origin, so the Escape forwarding only attaches in web mode; the comment says so). The same probe showed the web route's `sandbox` CSP directive blanks WebKit's viewer (plugins are refused in sandboxed documents), so `serve_media` sends `UNTRUSTED_PDF_CSP` (without `sandbox`) for `application/pdf`; `lookup` already served the stored MIME with Range, and both routes are now covered by tests for a stored PDF. History thumbnails had become 120x120 squares with `object-fit: cover`, cropping every non-square screenshot. `AttachmentThumb` gains `fit="natural"` (width of the parent, own aspect, cropped only past the parent's `--attachment-thumb-max-height`) and the history wrapper is back to `width: 120px; max-height: 120px`; the composer strip and queue were already square slots before and are unchanged. acp-client's omitted-attachments notice counted PromptImages, so one PDF read as "20 image attachment(s) omitted"; it now says "image block(s)". Verified: `cargo clippy -D warnings`, `cargo fmt --check`, `cargo test` (1079 passed, no load flakes), acp-client clippy/fmt/`cargo test` (183 passed), vitest (1321 passed, --maxWorkers=3), `pnpm run check` (0 errors/warnings), prettier on the changed files (the docs file's only complaint is the pre-existing line 18). Fixture end-to-end through a temporary test (not committed) with /tmp/pdf-support-fixture/onboarding-flow-chart.pdf: stored as application/pdf with identical bytes, 512x293 thumbnail (30,671 B) in 167 ms then 228 µs from cache with no temp file left, one 2048x1170 PNG prompt page (205,763 B) labelled "page 1 of 1 of onboarding-flow-chart.pdf", images directory empty after delete. Headless Chrome shows its PDF viewer for the web route under every CSP variant but never paints pages, so web-mode rendering was checked in WKWebView only. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…per message Follow-ups from review of 320e593 (PDF prompt page budget). `MAX_EDGE_PX` was 2048, but the Claude API rejects any image over 2000 px on a side once a request carries more than 20 image blocks, and it counts the images resent from earlier turns and those inside tool results, so a 20-page PDF plus a single screenshot (or any later attachment in the same session) would have turned every 2048 px page in the history into an `invalid_request_error`. The constant is now 2000 with a comment saying why; the high-resolution tier's native long edge is 2576 px, so a letter page at ~182 DPI instead of ~186 loses nothing. The pdf.rs tests pass `MAX_EDGE_PX` rather than a literal and their expectations are recomputed from the box sizes: letter 612×792 pt renders 1545×2000 (rotated 2000×1545), the 3150×1800 pt fixture box 2000×1143, and the 4x-capped cases are unchanged. Thumbnails (512 px) are unaffected. The 20 MB encoded-image budget was per PDF: `used_bytes` was a local of each `add` call while `session_runner` loops over every attachment, so two PDFs could put 40 MB of pages (~54 MB as base64) into one prompt, past the 32 MB request limit the comment cites. The counter now lives on `PromptAttachments` and every image block — plain images included — reserves against it. A PDF that reaches the budget truncates with the existing "over the 20 MB image budget" caption (on its last page sent, or in the prompt text when none fit); a plain image that would cross it is noted in the prompt text as "[Attached image x.png (N bytes) not included in this prompt: over the 20 MB image budget]" rather than silently dropped, and is not counted, so a smaller one after it can still fit. `PageLimits` is renamed `PromptLimits` now that `total_bytes` spans the prompt, `MAX_PROMPT_PAGE_BYTES` becomes `MAX_PROMPT_IMAGE_BYTES`, and the tests build a `PromptAttachments::with_limits(..)` (test-only) instead of passing limits per call. The per-image comment is corrected: the direct API accepts 10 MB of base64 per image (Bedrock and Vertex less), so 5 MB of encoded bytes stays as the conservative ceiling. Tests: two small PDFs sharing one budget (the second gets the one page left and names the omission), an image before a PDF (the PDF gets one page), a PDF that uses the budget up before an image (the image is noted, the PDF's labels untouched), and a platform-independent two-images case. docs/inline-note-media.md says the budget is shared by the message's attachments. Verified: `cargo clippy -D warnings` (CI config; `--all-targets` shows only the two pre-existing baseline lints), `cargo fmt --check`, `cargo test` (1081 passed). acp-client is untouched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…s spent Follow-up from review of b5b77e9 (per-message prompt image budget). When an earlier attachment had used up MAX_PROMPT_IMAGE_BYTES, each later PDF still rendered its first page — a 2000 px CoreGraphics draw plus the PNG/half-PNG/JPEG encode chain, ~150–250 ms — only for `reserve` to refuse it. The page loop in `session_runner/attachments.rs` now checks `used_bytes >= total_bytes` before `render_page` and records the omission with the existing over-budget caption, so the first page after the budget is spent is never rendered. Pages that fit, or that fall under the per-page cap, are decided exactly as before; the check only avoids work whose outcome is already known. The shared-budget test's last case (a PDF that fills the budget, then an image noted in the prompt text) now also adds a second PDF and asserts it is noted as "[Attached PDF b.pdf: pages 1–2 not included: over the … image budget]" with no images added — the same outcome the pre-check produces without rendering. Verified: `cargo test --lib session_runner::attachments` (9 passed), `cargo clippy -D warnings`, `cargo fmt --check`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…max-w-md Opening a PDF attachment from the timeline showed a tall column about 14% of the window wide (725 px on a 5120 px window), with the landscape page drawn as a small strip scaled to that width and white space above and below. The PDF branch was reached and `w-[90vw] h-[90vh]` were applied; the width was then capped by `Dialog.Content`'s own defaults. Its `cn()` class list includes `max-w-[calc(100%-2rem)] sm:max-w-md`, and tailwind-merge only lets the viewer's `max-w-[90vw]` replace the unprefixed utility: the merged result is `sm:max-w-md max-w-[90vw] w-[90vw] h-[90vh]`, so above the 640 px breakpoint `max-width` is 28rem, which at the app's 13 px root font size is 364 CSS px — 728 physical px at 2x, matching the report. The sibling viewers (`MediaViewerModal`, `ToolResultImage`) already pass `sm:max-w-[90vw]` for the same reason (#958, #969); `ImageViewerModal` had been missed since the shadcn migration (#765), so its images were clamped to 364 px too. The fix adds `sm:max-w-[90vw]` to the dialog's classes; the arbitrary values are generated (the strings are literal in the file) and the merged list becomes `max-w-[90vw] sm:max-w-[90vw] w-[90vw] h-[90vh]`. Verified with a Vite harness mounting the HEAD and fixed components (real app.css, bits-ui dialog) in an offscreen WKWebView at 1600x1000 with the fixture PDF served at `/api/media/<id>.pdf` and `get_image_data` answered with the rendered 2048x1170 PNG; animations were disabled with a user script so snapshots show the settled layout: - PDF before: computed max-width 364px, dialog 364x900, iframe 362x855 — the narrow column from the screenshot, page fitted to 362 px. - PDF after: max-width 1440px (90vw), dialog 1440x900, iframe 1438x855; the flow chart fills the frame width at vector quality. - Image before: dialog 364 px wide, img 330x189. - Image after: dialog 800x515 (the shrink-to-fit width of the `left-1/2` fixed box, as in MediaViewerModal), img 766x438, so images display larger, not smaller, than before. Checks: prettier, `pnpm run check` (0 errors/warnings), vitest (1321 passed, --maxWorkers=3). No Rust changes. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PDFs dropped onto a branch card, pasted, or picked in the chat composer used to fall through to the text-file path, so you'd get a UTF-8 error toast or the raw path pasted into the textarea. They're now accepted as image attachments: the backend renders page one to a PNG and stores that in place of the PDF.
Backend
store_sniffed_imagedetects%PDF-and renders the first page in-process with CoreGraphics (newnote_media::pdf, usingobjc2-core-graphics/objc2-core-foundation, both already in the lock)./Rotateand draws on an opaque white background.min(2048 / long_edge_pt, 4.0). If the PNG comes out over the 10 MB image cap, it re-renders once at half scale.<stem>.pdfis stored as<stem>.png.MAX_PDF_INPUT_SIZE), shared by the in-memory, path, and base64 routes. Other images keep the 10 MB cap, and an oversized non-PDF is refused before it's buffered. Size errors now all come from onesize_errorhelper.create_imageandcreate_image_from_datarun onspawn_blocking, and so does the web dispatcher'screate_image_from_dataarm./api/invoke/{command}now has a body limit that fits a base64 50 MiB PDF. Before, axum's default 2 MiB limit returned 413 for those bodies before they reached the handler..pdfreferences in notes are still refused as inline note media.Frontend
isImageAttachmentFile(images plus.pdf) is now used by the native drop handlers. Paste acceptsapplication/pdf, and both pickers use a sharedATTACHMENT_ACCEPT.createImagefails on a branch card, the toast now names the file, e.g.Could not attach "report.pdf" — ….Docs
docs/inline-note-media.mdnow covers PDF attachments.Testing
cargo test,cargo clippy -D warnings,cargo fmt --checkpnpm run check, prettierread_local_attachmentedge cases🤖 Generated with Claude Code