Skip to content

feat(ui): render session attachment images in Markdown - #4137

Merged
Astro-Han merged 6 commits into
mainfrom
feat/4093-markdown-attachment-images
Aug 29, 2026
Merged

feat(ui): render session attachment images in Markdown#4137
Astro-Han merged 6 commits into
mainfrom
feat/4093-markdown-attachment-images

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Render canonical maka://runtime/attachments/<artifactId> image refs in assistant Markdown without widening the Markdown URL policy or adding another attachment transport.

Astryx 0.5.0 already routes inline and standalone images through components.image (facebook/astryx#4100), so this change makes Maka's MarkdownImage the single policy chokepoint and deletes the obsolete pre-parse Markdown scanner. Canonical session refs resolve through the existing attachments:readBytes capability; HTTP(S) images retain their current presentation, and every other scheme falls back to [alt].

The session reader now enters @maka/ui once at ChatView. A session-scoped provider and shared loading hook serve both assistant Markdown and user attachment thumbnails, replacing the previous ChatView -> TurnView -> UserMessageBody reader prop chain. The provider only deduplicates concurrent reads, so transient failures remain retryable. One core raster-preview policy now governs every transcript image: the Desktop host rejects ineligible MIME metadata and payloads over 2 MiB before streaming, rechecks the byte cap while streaming, and the renderer validates the returned MIME/base64 again before committing it to state. Quote Companion receives the same capability through its existing Workbar attachment service.

Conversation copies rewrite canonical attachment refs in both stored assistant messages and Runtime ledger text through the existing Artifact id map. Canonical user attachment storage refs also follow that map, so branches, revisions, and companion forks resolve copied Artifacts rather than source-Session ids. The model-facing attachment block identifies image refs as valid Markdown image sources.

flowchart LR
  Host["Desktop host<br/>attachments:readBytes"]
  Provider["ChatView<br/>SessionAttachmentProvider"]
  Hook["useAttachmentImageSource"]
  User["User attachment thumbnail"]
  Astryx["Astryx Markdown<br/>inline / standalone / reference"]
  Gate["MarkdownImage<br/>single URL policy"]
  Http["HTTP(S) img"]
  Placeholder["[alt] fallback"]
  Session["Current-session attachment img"]

  Host --> Provider
  Provider --> Hook
  User --> Hook
  Astryx --> Gate
  Gate --> Http
  Gate --> Placeholder
  Gate --> Hook
  Hook --> Session
Loading

Before / after

Assistant attachment Markdown before and after

Fixes #4093

Verification

  • npm run build --workspace @maka/ui && node --test packages/ui/dist/__tests__/attachment-image.test.js packages/ui/dist/__tests__/markdown-body.test.js — 35/35 passed
  • npm run build --workspace @maka/runtime && node --test packages/runtime/dist/__tests__/conversation-copy.test.js packages/runtime/dist/__tests__/runtime-event-read-model.test.js — 79/79 passed
  • npm run build:main --workspace @maka/desktop && node --test apps/desktop/dist/main/__tests__/runtime-host-artifacts-ipc-main.test.js apps/desktop/dist/main/__tests__/artifact-preview-registry.test.js — 6/6 passed
  • npm run build:workspace-deps --workspace @maka/desktop && npm run typecheck --workspace @maka/desktop — passed, including preload, main, renderer, and Storybook
  • npm run format:check — passed
  • Storybook Product/Attachments/AssistantMarkdownImage through the real ChatView composition — resolved a data:image/png source, computed display: block and max-width: 100%, rendered 256px wide inside the 800px assistant content column, with no console warnings or errors

Repository-wide tests were not run.

Review focus

The Markdown ref never carries a session id. parseAttachmentResourceRef admits only the canonical Runtime Host attachment form, and SessionAttachmentProvider supplies the session that owns the rendered transcript. Missing readers, unknown/deleted artifacts, malformed refs, disallowed MIME types, oversized transcript previews, data:, file:, and other maka:// hosts remain inert placeholders. Conversation copy owns ref rewriting when Artifact ids change.

AI use

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Codex diagnosed the Astryx seam, implemented the UI/runtime changes and tests, performed the adversarial review, and verified the focused suites and Storybook composition. Commits carry Generated-by: Codex trailers.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Aug 29, 2026
@Astro-Han
Astro-Han force-pushed the feat/4093-markdown-attachment-images branch from 8c8193d to e3061e7 Compare August 29, 2026 07:12
@Astro-Han
Astro-Han marked this pull request as ready for review August 29, 2026 07:24
@Astro-Han
Astro-Han force-pushed the feat/4093-markdown-attachment-images branch 2 times, most recently from e848547 to 2083cc0 Compare August 29, 2026 09:53

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I found no P0–P3 issues on exact head 2083cc0ed9fe8e09929af7fc5b295982d7ab8a9f.

The Markdown image URL is the trust boundary, and this change keeps it closed. MarkdownImage is the only chokepoint: canonical maka://runtime/attachments/<artifactId> refs resolve through the existing attachments:readBytes channel against the session that owns the transcript; HTTP(S) keeps the current inline presentation; file:, data:, other maka:// hosts, query strings, and non-canonical ids stay [alt]. The ref never carries a session id, so model text cannot name another session. Assistant previews go through decideImageReadOutcome (raster MIME allowlist, 2MB cap, no SVG). User thumbnails keep their existing payload contract.

Conversation copy rewrites those refs through parseAttachmentResourceRef and the Artifact id map, including Runtime ledger text, and leaves non-canonical spellings untouched.

I did not treat issue #4093 as evidence. The tests can show the claim false: an attachment ref becomes an <img> with a data URL on the current session; unreadable, oversized, and disallowed sources do not; a missing reader stays a placeholder; copy maps source Artifact ids and does not rewrite ?session=.

I am not merging. This is a feature; merge is a human call. Hosted test was still in progress when I posted. This review does not claim CI is green.


Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.

简体中文

精确 head 2083cc0ed9fe8e09929af7fc5b295982d7ab8a9f 上我没有发现 P0–P3。

Markdown 图片 URL 是信任边界,这次改动把它关着。唯一关口是 MarkdownImage:规范的 maka://runtime/attachments/<artifactId> 走现有的 attachments:readBytes,对着当前转录所属的 session 解析;HTTP(S) 保持原来的行内展示;file:data:、其它 maka:// host、query、非规范 id 都落成 [alt]。ref 不带 session id,模型文本不能点名另一个 session。助手预览走 decideImageReadOutcome(光栅 MIME 白名单、2MB 上限、没有 SVG)。用户缩略图仍用原来的 payload 契约。

会话拷贝用 parseAttachmentResourceRef 和 Artifact id 映射改写这些 ref,包括 Runtime ledger 文本,非规范写法不动。

我没有把 issue #4093 当证据。测试能证伪核心主张:附件 ref 会变成当前 session 上的 data URL 图片;读不到、过大、不允许的源不会;没有 reader 时是占位;拷贝会映射源 Artifact id,且不改写 ?session=

我不合入。这是功能,合入由人类决定。发这条时 hosted test 还在跑,这次审查不表示 CI 已绿。

本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I found one P1 and one P2 on exact head 2083cc0ed9fe8e09929af7fc5b295982d7ab8a9f. The P1 blocks approval: the 2 MiB assistant-preview guard runs only after the Desktop host has materialized and transferred payloads of up to 50 MiB, while distinct Markdown refs have no load budget. The P2 is a recoverable branch/revision failure: the new ref rewrite is not used by the production copy mode.

The closed URL policy, stale-completion cancellation, identical-ref deduplication, transient-failure retryability, and Quote Companion wiring otherwise held up. I ran 113 focused UI/runtime tests, all workspace dependency builds, all four Desktop TypeScript checks, the Desktop artifact IPC test, and the production renderer build. windows_recovery is green; hosted test was still running when I posted.


Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.

简体中文

本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。

Comment thread packages/ui/src/attachment-image.tsx
Comment thread packages/runtime/src/conversation-copy.ts

@zhiiw zhiiw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed at exact head 2083cc0e (20 files, +586/−168).

No P0–P3 findings. The claims I checked hardest, verified rather than trusted:

  • The deleted pre-parse scanner stays deleted safely: the installed Astryx 0.5.0 routes both inline and standalone images through components.image (confirmed in node_modules/@astryxdesign/core/dist/Markdown/Markdown.js — both case 'image' branches honor ImageComp, with sanitizeUrl rejecting javascript:/data:/vbscript: ahead of it), so Maka's MarkdownImage is genuinely the single policy point. maka: refs pass the sanitizer and resolve through the session attachment reader; everything else still falls back to [alt].
  • Attachment loading: concurrent reads deduplicated per session+artifact, the pending entry clears on settle so transient failures stay retryable, and assistant Markdown keeps the raster MIME guard via safePreview.
  • Conversation copy: assistant text, ledger text, and user session_file refs all rewrite through the artifact id map, so copies resolve their own attachments.

Executed on a real Windows machine at this head: clean rebuild, @maka/ui attachment-image + markdown-body 34/34, runtime conversation-copy/read-model attachment tests green (the 7 other conversation-copy failures on this machine are the pre-existing Windows flake set, identical on the base commit).

简体中文

无 P0–P3。删掉预扫描器是安全的:已在本地安装的 Astryx 0.5.0 里核实内联与独立图片都走 components.image,且 sanitizeUrl 先拦危险 scheme;maka:// 引用经 session 附件读取器解析,其余照旧回落 [alt]。附件加载去重、失败可重试、raster 守卫保留;会话复制的引用改写三处都覆盖了。本机真 Windows 干净重建 + 靶向测试全绿。

@xxhZs xxhZs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head 2083cc0ed9fe8e09929af7fc5b295982d7ab8a9f. NO-GO: one P1 and one P2, confirmed in the existing inline threads.

The P1 is reachable through ordinary use: up to eight admitted 50 MiB attachments can be named by one assistant answer, and every distinct Markdown ref starts its host read concurrently before the renderer applies the 2 MiB preview cap. The P2 is also on the production path: branch/revision copies use preserve_external, while Markdown refs have no source Session identity, so copied assistant images resolve against the target Session and become placeholders.

The closed URL policy, current-session authority, post-load MIME/size check, pending-read deduplication, retry-on-remount behavior, and Quote Companion wiring otherwise hold. I found no additional P0–P3 issues in the changed surface.

Hosted test and windows_recovery are terminal green, and GitHub reports the exact head mergeable. Those gates do not close the reachable P1.

@Astro-Han
Astro-Han force-pushed the feat/4093-markdown-attachment-images branch from 2083cc0 to bea6c57 Compare August 29, 2026 10:42

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I re-reviewed exact head bea6c5715d9c05541fbb8a34a52d6066ab673555 and found no remaining P0–P3 issues.

The previous P1 is fixed. The Desktop host now applies the shared raster MIME and 2 MiB policy to artifact metadata before starting a stream, enforces the same cap while streaming, and returns a typed failure if the stored size drifts. The demonstrated eight-by-50-MiB path therefore starts no byte streams; admitted previews are bounded to 2 MiB each. The renderer still validates the returned MIME and payload before committing it to state, and user thumbnails now use the same policy instead of a separate exception.

I am also retracting my previous P2. I correctly observed that the direct SessionManager copy helper uses preserve_external, but I incorrectly treated that helper as the shipped Desktop branch/revision path. Repository-wide call tracing shows that Desktop goes through sessions:branchFromTurn / sessions:reviseBeforeTurn, DesktopRuntimeHostClient.copySession, and HostSessionRevisionCoordinator. That coordinator copies the artifacts and invokes the existing rewrite with mode: 'exact' plus the copied artifact-id map. The direct helper I probed has no production caller.

I rebuilt Core, Runtime, UI, and Desktop main; ran all four Desktop TypeScript checks; ran 85 focused tests covering the shared policy, Host IPC, Markdown images, materialization, and conversation copying; and completed the production renderer build, formatting check, and diff check. windows_recovery is green on this head. Hosted test and Windows package were still running when I posted, so this approval does not claim those checks are complete.

This remains a feature, so I am not merging it; the product decision remains with a human maintainer.


Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.

简体中文

本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。

@Astro-Han
Astro-Han force-pushed the feat/4093-markdown-attachment-images branch from bea6c57 to eea2d68 Compare August 29, 2026 10:56

@xxhZs xxhZs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head eea2d68e3163ddc7ef572598b9e5124acb476e8c. Approved — no P0–P3 findings remain.

The previous P1 is closed at the owning boundary. attachments:readBytes now applies the shared raster policy to metadata before streaming, admits at most 2 MiB, enforces the same cap while consuming chunks, verifies the stored size, and returns typed failures; the renderer independently revalidates MIME and base64 before state. The former eight-by-50-MiB path now starts zero streams. Pending reads remain keyed by Session and Artifact, and the existing Runtime Host request cap bounds transport concurrency.

I also retract my previous P2 after tracing the shipped Desktop path end to end. Desktop branch/revision requests go through DesktopRuntimeHostClient.copySession to HostSessionRevisionCoordinator, which copies conversation Artifacts and calls the rewriter with mode: "exact" plus the Artifact id map. The SessionManager preserve_external helper I cited is not the production Desktop route.

The URL-policy chokepoint, copy rewriting, transient retry behavior, and Quote Companion wiring still hold after the rebase. Fresh merge-tree is clean and git diff --check passes. Hosted checks were still in progress when I submitted this code review.

@zhiiw zhiiw left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-reviewed at the current head eea2d68e (previous approval was on 2083cc0e).

The branch was rebased and gained one commit. git range-diff shows the five original commits are patch-identical between the two heads; the only new content is eea2d68e "fix: enforce one attachment image preview policy", which this approval covers:

  • Single policy authority: the raster-MIME allowlist, extension fallback, and 2 MiB cap moved from packages/ui/src/artifact-preview-registry.ts into @maka/core/artifacts (resolveArtifactImagePreview / normalizeArtifactImagePreviewMime). The UI registry now delegates; semantics are unchanged (MIME authoritative when present, extension fallback otherwise, SVG still excluded).
  • IPC admission tightened to match: attachments:readBytes in runtime-host-artifacts-ipc-main.ts now rejects preview-ineligible metadata before streaming (oversize → too_large, SVG/unknown → unsupported_mime), caps the stream itself at the same 2 MiB (mid-stream drift aborts cleanly via a dedicated sentinel, not a generic error), and returns only the normalized allowlisted MIME instead of passing artifact.mimeType through.
  • The two-policy gap is closed: useAttachmentImageSource no longer takes safePreview — the user-thumbnail path (previously unvalidated passthrough of result.mimeType into a data URL) and the Markdown path now both go through decideImageReadOutcome, and AttachmentImage pre-gates on metadata so oversized/unsupported attachments never trigger a read. I confirmed readBytes has no non-image consumer: it flows only through SessionAttachmentProvider to the two image call sites.

Executed on a real Windows machine at this head: clean forced rebuild, desktop artifact-preview-registry + runtime-host-artifacts-ipc-main 6/6 (including the new admission tests: no stream on ineligible metadata, mid-stream oversize abort), @maka/ui attachment-image + markdown-body 35/35.

简体中文

分支 rebase 后多了一个提交。range-diff 确认原有 5 个提交逐字节等价,新内容只有 eea2d68「统一附件图片预览策略」。本 approve 覆盖该增量:图片预览的 MIME 白名单/扩展名回落/2MiB 上限收进 @maka/core 单一权威,UI 侧改为委托;attachments:readBytes 在流式读取前先做准入(超限/非法 MIME 直接拒),流中漂移也能干净中止,且只回规范化后的白名单 MIME;用户缩略图与 Markdown 两条路径合一,都过 decideImageReadOutcome,元数据不合格时不发起读取。已确认 readBytes 没有非图片消费方。本机真 Windows 干净重建:desktop 6/6(含新准入测试)、ui 35/35。

@Astro-Han
Astro-Han force-pushed the feat/4093-markdown-attachment-images branch from eea2d68 to c7c1234 Compare August 29, 2026 11:09
Route every Astryx Markdown image through Maka's component policy, resolve canonical session attachment refs through the existing Runtime Host byte reader, and keep all other schemes inert. Share the session-scoped loader with user attachment thumbnails, remove the old reader prop chain and pre-parse image scanner, and tell the model which attachment ref is a valid Markdown image source. Add streaming, settled, missing-attachment, scheme-policy, model-history, and Storybook coverage.

Generated-by: Codex
@Astro-Han
Astro-Han force-pushed the feat/4093-markdown-attachment-images branch from c7c1234 to dbc1b60 Compare August 29, 2026 11:35
Rewrite canonical attachment resource references when conversations are copied so branched and companion transcripts keep rendering the copied Artifact. Route Quote Companion through the existing Workbar attachment service, reuse the renderer image allowlist and payload cap, and share repeated reads per Session provider.

Generated-by: Codex
@Astro-Han
Astro-Han force-pushed the feat/4093-markdown-attachment-images branch from dbc1b60 to ed4eb7a Compare August 29, 2026 11:37
@Astro-Han
Astro-Han merged commit 2fa1f78 into main Aug 29, 2026
3 checks passed
@Astro-Han
Astro-Han deleted the feat/4093-markdown-attachment-images branch August 29, 2026 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/L Under 1000 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(ui): render session attachment images in assistant Markdown

5 participants