Repository navigation
feat(web): improve non-image attachment UX and persistence - #3531
Closed
italic-jinxin wants to merge 9 commits into
Closed
italic-jinxin wants to merge 9 commits into
italic-jinxin wants to merge 9 commits into
Conversation
- Bump per-file cap 5 MiB → 7 MiB - Reject video/* at chat-input boundary with i18n alert - Lucide SVG icons (audio = note, others = generic file) - Show type + size in card meta line - Fix × button styling clobbered by .chat-input button accent-pill - Add click-to-zoom image lightbox (ESC / backdrop closes) - Remove dead stagedImages legacy paths - 9 new regression tests for attachment validation
Address review feedback on the non-image attachment UI:
- Show filename in the video-rejection alert (i18n string was missing
the `{name}` placeholder while the call site was passing one).
- i18n the lightbox `aria-label` via the new `chat.imagePreview` key
in en/ko/zh-CN.
- Replace fragile `.replace('-name', '-icon')` class derivation in
`appendAttachmentFileCard` with explicit per-element class names
passed via a `classes` object.
- Don't hijack clicks on `<a><img></a>` markup — let the link navigate.
- Make the lightbox click listener idempotent so re-injection (e.g.
hot-reload) doesn't stack handlers.
- Drop the unreachable empty-string fallback in `displayContent` and
the dead `.image-preview-container` rule the new CSS shadowed.
) Before this change, user images degraded to file cards after a page refresh because the persisted XML carried metadata only — no pixels. Now they're written to disk on ingest, served via a new authenticated route, and re-rendered inline. - Backend: extract `sanitize_attachment_segment` / `persist_attachment_at` shared by v1 and v2. v1 (default `ENGINE_V2=false`) now calls a new `persist_legacy_image_attachments` from `thread_ops::process_user_input`, matching v2's existing project-aware persist; both land under `~/.ironclaw/attachments/<owner>/...`. - New route `GET /api/attachments/{owner}/{*path}`, Bearer-auth'd. Defense-in-depth: cross-user → 404 (no info leak), `..`/`\` rejected at URL boundary, canonical-path containment, streamed via `tokio::fs::File::open` + `ReaderStream`. - `HistoryResponse.turns[].user_attachments` surfaces URLs out-of-band from the LLM-facing XML. Frontend fetches with Bearer and renders each image as a blob URL — token stays out of the URL. - Frontend canvas resize: JPEGs over 1600 px are re-encoded to JPEG q=0.88 before upload, capping body size and LLM context. PNG / GIF / WebP pass through to preserve transparency / animation.
- `turn_info_from_in_memory_turn` and `in_progress_from_thread` were returning `Vec::new()` for user_attachments, leaving every refresh during a still-in-memory session falling back to file-card render even though the persisted XML already carried `project_path`. Both builders now call `extract_user_attachments(&t.user_input)`, same as the DB-persisted path. Adds 2 caller-level regression tests. - Adds `user_attachments` to `InProgressInfo` so the frontend's in-progress fallback path renders correctly. - Generalizes the canvas resize from JPEG-only to all bitmap MIMEs. Opaque PNGs (the common screenshot case) now re-encode as JPEG, saving ~80% of bytes; PNGs with real alpha keep PNG output. GIF / SVG continue to pass through to preserve animation / vector.
- Persist each attachment under a per-index subdirectory so filenames that sanitize to the same string (e.g. "a b.png" and "a_b.png") no longer overwrite each other on disk. - Switch serve_user_attachment to tokio::fs::canonicalize so the async handler doesn't stall a runtime worker on blocking syscalls. - Set Cache-Control: private, max-age=300 + Vary: Authorization on attachment responses so shared proxies don't cache per-user bytes while the browser still gets a short-lived cache. - Revoke blob URLs once <img> finishes decoding so long chat sessions don't leak megabytes of object URLs across history prunes.
Aligns the attachment trigger with the iMessage / Slack / WhatsApp pattern: a 32px circular button with a thin border sitting to the left of the textarea, replacing the paperclip emoji on the right. - HTML: `.chat-input` becomes a column container; new `.chat-input-row` holds `[+][textarea][Send]`. The preview strip sits above without relying on `flex-wrap: wrap`. - CSS: `.attach-btn` is 32×32 with a 1.5px border, transparent background, 18px Lucide plus SVG centered. Padding lifts the touch target to 44px (WCAG). Hover lightens border/text and adds a subtle background; `:focus-visible` adds an accent outline. - The Lucide SVG replaces the paperclip emoji for cross-platform visual consistency with the rest of the icon set. - No JS or wiring changes — element IDs preserved.
github-actions
Bot
added
scope: agent
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a robust attachment handling system that ensures user-uploaded images persist correctly across page refreshes. Key changes include client-side image compression (downscaling and opaque PNG-to-JPEG conversion), a new authenticated API endpoint for serving attachments, and a lightbox for image previews. Additionally, the per-file attachment limit was increased to 7MB. Feedback identifies a functional issue where immediate blob URL revocation breaks the lightbox and suggests implementing a time-based cache for attachment fetches to reduce redundant network requests.
italic-jinxin
changed the title
Add inline `// safety:` comments to the three regex compile sites in `extract_user_attachments` so the pre-commit panic-style check recognizes them as infallible static patterns rather than production .expect() violations.
a007a2c revoked the blob URL inside the inline `<img>`'s `load` handler. That works for the inline render in Chrome's happy path, but two scenarios produced broken images: 1. Lightbox click: `showImageLightbox(target.src, ...)` feeds the same blob URL to a fresh `<img>` in an overlay. That second load reused the revoked URL and silently rendered the broken-image placeholder (Chrome reports `complete === true` but `naturalWidth === 0`). 2. Repaint after bitmap eviction: a backgrounded tab or memory pressure can drop the decoded bitmap; the next paint needs to re-fetch the URL, which is now dead. Tie the revoke to DOM removal instead. The blob URL is stamped on `image.dataset.blobUrl`; a one-shot MutationObserver wired on `#chat-messages` revokes it when the bubble (or any ancestor carrying it) is removed. That keeps memory tracking the visible history (pruneOldMessages, thread switch, ad-hoc remove) without breaking either the inline render or click-to-preview. Adds an e2e regression that uploads a PNG, refreshes the page to exercise the persist-then-fetch path, clicks the inline image, and asserts the lightbox `<img>` decodes (`naturalWidth > 0`). The old revoke-on-load code fails this assertion because the lightbox `<img>` gets a dead blob URL.
a007a2c revoked the blob URL inside the inline `<img>`'s `load` handler. That works for the inline render in Chrome's happy path, but two scenarios produced broken images: 1. Lightbox click: `showImageLightbox(target.src, ...)` feeds the same blob URL to a fresh `<img>` in an overlay. That second load reused the revoked URL and silently rendered the broken-image placeholder (Chrome reports `complete === true` but `naturalWidth === 0`). 2. Repaint after bitmap eviction: a backgrounded tab or memory pressure can drop the decoded bitmap; the next paint needs to re-fetch the URL, which is now dead. Tie the revoke to DOM removal instead. The blob URL is stamped on `image.dataset.blobUrl`; a one-shot MutationObserver wired on `#chat-messages` revokes it when the bubble (or any ancestor carrying it) is removed. That keeps memory tracking the visible history (pruneOldMessages, thread switch, ad-hoc remove) without breaking either the inline render or click-to-preview. Adds an e2e regression that uploads a PNG, refreshes the page to exercise the persist-then-fetch path, clicks the inline image, and asserts the lightbox `<img>` decodes (`naturalWidth > 0`). The old revoke-on-load code fails this assertion because the lightbox `<img>` gets a dead blob URL. Also bumps the oversized-file threshold in `test_gateway_attachment_limits_block_batched_uploads` from 5 MiB to 7 MiB + 1 byte. `MAX_ATTACHMENT_SIZE_BYTES` was raised to 7 MiB in 7e368b8 (#1341 polish) but this test still allocated a 5 MiB + 1 byte file, which now sits below the limit and never fires the alert the test waits for.
Closed
2 of 14 tasks
Contributor
Author
|
feature implemented for v1, closed as conflict with reborn |
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
Closes the user-visible polish for Support non-image file attachments in web gateway (PDF, audio, documents) #1341 ("Support non-image file
attachments in web gateway"). Backend MIME allowlist + magic-byte
sniffing already shipped in [codex] Support web document uploads #2332 / feat(gateway): add attachment flows, v2 skill install coverage, and e2e stabilization #2385 / [codex] Fix gateway slash autocomplete and attachment rendering #2763; this PR finishes
the per-file budget bump and the staging-strip UI.
User-uploaded image attachments now survive a page refresh — instead of
degrading into a generic file card, the chat surface re-fetches the
original bytes and renders them inline.
The persisted
<attachments>XML carries metadata only (filename,mime, size). Without disk persistence + a URL the browser can fetch,
images visibly "disappear" on every refresh. This PR closes that gap.
a properly styled 32×32 circular button with a Lucide-style
+icon, in thevisual tradition of iMessage / Slack / WhatsApp. Closes Replace paperclip emoji with styled + button for attachment trigger #1342.
Changes
Backend
MAX_INLINE_ATTACHMENT_BYTES: 5 MiB → 7 MiB. Total budget (10 MiB),file count (5), and 14 MiB request body cap unchanged.
web/CLAUDE.mdto match.Frontend
"DOCX"-text icon with Lucide-style SVGs (audio = music note,others = generic file). Show
type • sizein card meta.video/*at the chat-input boundary with an i18n alert (backendallowlist excludes video).
.image-preview,.message-attachment-image,.generated-image) to open a fullscreen lightbox; ESC / backdrop closes..chat-input button(Send accent-pill) wasoutranking the bare selector and rendering it as a stretched green pill.
stagedImages/handleImageFileslegacy paths.Backend
persist_legacy_image_attachmentsruns on user-message ingest(
thread_ops::process_user_input), writing image bytes to disk andstamping
local_pathon the attachment soformat_attachmentcanemit
project_path="..."in the persisted XML.channels/attachments.rs:sanitize_attachment_segment,persist_attachment_at,legacy_attachment_relative_path. Engine v2's existingpersist_project_attachmentswas refactored to call them — purerefactor, no behavior change.
~/.ironclaw/attachments/):<owner>/.legacy/<msg>/<file><owner>/<project>/<date>/<file>(unchanged)HTTP route
GET /api/attachments/{owner}/{*path}, behind the regulargateway Bearer-auth middleware. Authorization is layered:
<owner>segment must match the authenticated user.Mismatch → 404 (deliberately not 403 — we don't confirm
files exist for a different user).
..and\rejected at the URL boundary.tokio::fs::File::open+ReaderStreamper.claude/rules/safety-and-sandbox.md"Bounded Resources".History surface
HistoryResponse.turns[].user_attachmentsfield shape:{ filename, mime_type, size_label, url, kind }. Deliberatelyout-of-band from the LLM-facing
user_inputtext — the<attachments>XML stays clean and the frontend gets the URLs itneeds without us polluting the agent's context.
extract_user_attachmentsparses each persisted<attachment project_path="...">element and maps it to a/api/attachments/{owner}/...URL.Frontend
renderMessageAttachmentsnow fetches images withAuthorization: Bearer ...and swaps a blob URL onto the<img>.Token never enters a URL — no leakage via logs / Referer / browser
history. Fetch failure (404 / auth / network) falls back to the
existing file-card render so the bubble never shows a broken-image
icon.
JPEG q=0.88 before upload, capping request body size and LLM
context. PNG / GIF / WebP pass through untouched to preserve
transparency and animation.
HTML* (crates/ironclaw_gateway/static/index.html)
[file-input][attach-btn][textarea][send-btn]in a new.chat-input-rowso the preview strip sits cleanly above the input row(no more flex-wrap reflow hack).
#attach-btnto the left of the textarea (was: right).📎emoji for an inline LucideplusSVG witharia-hidden.CSS (crates/ironclaw_gateway/static/styles/surfaces/chat.css)
.chat-input→flex-direction: column(wasrow + flex-wrap)..chat-input-rowfor the actual input row..chat-input .attach-btn:width/height: 32px,border-radius: 50%)var(--border), transparent backgroundtransform: scale(0.95):focus-visible)display: block(0,2,0)wins over the broader.chat-input buttonaccent-pill rule(0,1,1)from both base.cssmobile @media block and the Send button styles — explicit padding /
border-radius / background resets are no longer needed.
JS (crates/ironclaw_gateway/static/js/core/widgets.js)
composer". No logic change.
No JS behavior changes. All wiring (
#attach-btn/#image-file-inputevent listeners) preserved untouched.
Screenshots
Change Type
Linked Issue
Closes #1341 and #3272 and #1342
Validation
cargo fmt --all -- --checkcargo clippy --all --benches --tests --examples --all-features -- -D warningscargo buildcargo test --features integrationif database-backed or integration behavior changedreview-prorpr-shepherd --fixwas run before requesting reviewSecurity Impact
Database Impact
Blast Radius
Rollback Plan
Review Follow-Through
Review track: