feat(attachments): inline PDF preview for application/pdf (lazy-inline)#296
Open
Stvad wants to merge 3 commits into
Open
feat(attachments): inline PDF preview for application/pdf (lazy-inline)#296Stvad wants to merge 3 commits into
Stvad wants to merge 3 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Stvad
force-pushed
the
claude/brave-thompson-phw0ey
branch
from
a43aadd to
4629f2d
Compare
Stvad
force-pushed
the
claude/brave-thompson-phw0ey
branch
from
9153acc to
a808a5b
Compare
Stvad
changed the title
|
Stvad
force-pushed
the
claude/brave-thompson-phw0ey
branch
from
a808a5b to
292114a
Compare
Rebased onto master and re-implemented on the facilities the inline-audio increment (#300) introduced, rather than the earlier eager approach. - application/pdf is now a LAZY-INLINE viewer (eager: false), mirroring audio: a metadata poster ("Preview" + download) that resolves NOTHING on mount and arms the same fail-closed object-URL resolve via the renderer's content-scoped requestResolve latch only on the first preview intent. Once ready it renders a bounded-height <object type="application/pdf"> of the verified url; loading → spinner; a fail-closed resolve → a broken indicator. - Why lazy over eager: the down-lane already replicates every media block to the local plaintext byte store (§8), so an eager resolve is usually a local HIT — not a re-download or re-decrypt — but it still reads the full (possibly large) bytes into a decrypted object-URL Blob and pins it in memory for the block's lifetime (a note of large PDFs holds them all, even un-viewed), and on iOS Safari (which can't inline-render a PDF) that Blob is pure waste. Lazy also closes the earlier eager version's downloadable-floor gap. - UX: the poster carries an explicit "Preview" affordance (eye pill) so it's clear the row opens the PDF; the expanded preview is collapsible back to the poster (resolve stays armed → instant re-open, no refetch); and the object URL carries #navpanes=0 to hide the native viewer's page sidebar by default (Chrome PDFium open-param, best-effort per browser). - Reuses the audio increment's shared facilities: useMediaDownload + DownloadIconButton (generalized with a `testid` prop). PDF holds the "every attachment is at least downloadable" floor in every state (poster/preview/broken), since application/pdf no longer hits the file download fallback. - mediaBlock: PDF_MIME + isPdfMime (exact, case-insensitive). index: register pdfMediaViewer. No renderer change — a pure facet contribution. Security / XSS (verified empirically in Chromium): the object URL's Blob is pinned to application/pdf by the isPdfMime match, so the browser routes it to its native, out-of-process PDF viewer and never HTML-sniffs hash-verified-but-non-PDF bytes into a same-origin document; PDF-level JS is sandboxed by the viewer itself. A sandboxed iframe is NOT an option for a blob: source: Chromium blocks a sandboxed frame from loading a parent-origin blob: URL — even WITH allow-same-origin (which is honored for an http doc; a separate blob-navigation block still fires, ERR_BLOCKED_BY_CLIENT). The strongest form of "sandbox untrusted HTML" is applied elsewhere: text/html (and any unclaimed mime) is never inline-rendered — it goes to the octet-stream download. design.html §11/§15/scope/banner updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S2J741JwXMwT2wGYNM1j5u
Stvad
force-pushed
the
claude/brave-thompson-phw0ey
branch
from
292114a to
353ddc4
Compare
…pand jump) The collapsed poster and the expanded player were two different top-level elements (an inline-flex chip vs a block flex-col), so toggling repositioned the widget — the header visibly shifted down from the top of the block when the preview/player opened (reported for both PDF and audio). Both viewers now render ONE stable container whose header row (filename + controls + download) is always the first child, with the <object>/<audio> body toggling in BELOW it. The header is the same element in the same position in every state, so it can't move vertically on expand/collapse. - PDF: a single toggle (Preview ⇄ collapse) on the stable header; the <object> mounts below only when expanded. Download stays in the header (the §11 downloadable floor) in every state. - Audio: same shape — the play affordance lives in the header while un-armed; once armed the native <audio controls> mounts below and the header shows just filename + download. Testids and behavior unchanged (tests pass as-is). No behavior change beyond layout. yarn run check green (compile, lint, full test suite, sync-config, rpc-projections, no-service-role). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S2J741JwXMwT2wGYNM1j5u
…eference A `((id))` reference renders the block's raw content inside the inline <a> citation link (referenceLayout.tsx). The PDF/audio viewer containers were block-level (`flex`), so they sat on that text line's baseline and dropped a full line-height (~24px, measured) below the block's top — the "offset from the top of the block" seen for both viewers. Make each container `inline-flex` + `align-top` (vertical-align: top) so it hugs the line top: flush in a reference AND unchanged when the media block renders standalone (both measured 0px against the production CSS in the real reference-link structure). Scoped to the two media viewers — no change to the shared ReferenceLink, which must stay inline for text references. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S2J741JwXMwT2wGYNM1j5u
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
2 participants
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.
What & why
Adds an inline PDF preview for
application/pdfmedia blocks. Rebased ontomasterand re-implemented on the facilities the inline-audio increment (#300) introduced — the earlier version (stacked on #290) was an eager viewer; this adopts the lazy-inline pattern instead.application/pdfis a LAZY-INLINE viewer (eager: false), mirroring audio: a metadata poster (preview + download) that resolves nothing on mount and arms the same fail-closed object-URL resolve via the renderer's content-scopedrequestResolvelatch only on the first preview intent. Oncereadyit renders a bounded-height<object type="application/pdf">of the verified url;loading→ spinner; a fail-closed resolve → a broken indicator.useMediaDownload+DownloadIconButton(generalized with atestidprop). PDF holds the "every attachment is at least downloadable" floor in every state (poster / preview / broken), sinceapplication/pdfno longer falls through to the file download fallback.mediaBlock:PDF_MIME+isPdfMime(exact, case-insensitive).index: registerpdfMediaViewer. No renderer change — a pure facet contribution.Security (XSS) — and the sandbox question, answered empirically
The object URL's Blob is pinned to
application/pdfby theisPdfMimematch that selects the viewer (theBlobconstructor also lowercases the type), so the browser routes it to its native PDF viewer and never HTML-sniffs hash-verified-but-non-PDF bytes into a same-origin document; PDF-level JS is sandboxed by the viewer itself (out-of-process).A
sandboxed iframe (noallow-same-origin) is deliberately not used. Verified in Chromium: a sandboxed opaque-origin frame cannot load a parent-originblob:URL (net::ERR_BLOCKED_BY_CLIENT— blank), so it would break the preview while adding nothing the type-pin doesn't already give. The strongest form of "sandbox untrusted HTML" is applied elsewhere:text/html(and any unclaimed mime) is never inline-rendered — it goes to the transient octet-stream download.<object type="application/pdf">(shipped)<iframe>(no sandbox)<iframe sandbox>(noallow-same-origin)Docs & verification
docs/media-attachments/design.html§11 / §15 (new "phase 8 — inline PDF" row) / scope / status banner updated, including the sandbox decision.<object>typedapplication/pdf, loading, fail-closed broken (no<object>), download-from-poster (savable without arming), downloadable-floor when preview fails, disarm-on-content-change.isPdfMime+pickMediaViewerPDF dispatch.yarn run checkgreen (compile, lint, full test suite, sync-config, rpc-projections, no-service-role). CI now runs since this targetsmaster.🤖 Generated with Claude Code
https://claude.ai/code/session_01S2J741JwXMwT2wGYNM1j5u