Skip to content

[codex] fix(ui): proxy inbound media previews#90172

Closed
sweetcornna wants to merge 1 commit into
openclaw:mainfrom
sweetcornna:fix/89591-inbound-media-previews
Closed

[codex] fix(ui): proxy inbound media previews#90172
sweetcornna wants to merge 1 commit into
openclaw:mainfrom
sweetcornna:fix/89591-inbound-media-previews

Conversation

@sweetcornna

@sweetcornna sweetcornna commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Route canonical inbound image refs like media://inbound/photo.png through the existing authenticated Control UI __openclaw__/assistant-media route.
  • Keep proxying limited to single-file media://inbound/<id> refs so other media:// locations do not bypass the existing gateway validation.
  • Add a renderer regression test covering transcript MediaPath inbound image refs with empty local preview roots.

Fixes #89591

Verification

  • node scripts/run-vitest.mjs ui/src/ui/chat/grouped-render.test.ts -t "renders canonical inbound media refs" --reporter=dot
  • node scripts/run-vitest.mjs ui/src/ui/chat/grouped-render.test.ts src/gateway/control-ui.http.test.ts --reporter=dot
  • npx oxfmt --check ui/src/ui/chat/grouped-render.ts ui/src/ui/chat/grouped-render.test.ts
  • node scripts/run-oxlint.mjs ui/src/ui/chat/grouped-render.ts ui/src/ui/chat/grouped-render.test.ts
  • git diff --check
  • node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json

Real behavior proof

Behavior addressed: WebUI transcript images stored as media://inbound/... were detected as image MediaPath entries, but the renderer left the browser-facing <img src> as media://... instead of using the existing authenticated __openclaw__/assistant-media route. Browsers cannot load that scheme, so inbound Telegram/iMessage images showed as attachments without an inline preview.

Real environment tested: Local OpenClaw worktree at commit 98f8ec1fafcf2e32320e0a488a21462d84a3067f on macOS with repo dependencies installed via pnpm install --frozen-lockfile. The proof used the actual Control UI grouped renderer in jsdom, the actual async media-ticket path, and the existing Gateway HTTP media route tests.

Exact steps or command run after this patch:

node scripts/run-vitest.mjs ui/src/ui/chat/grouped-render.test.ts src/gateway/control-ui.http.test.ts --reporter=dot
node --import tsx - <<'NODE'
# jsdom render probe: render a user message with MediaPath="media://inbound/proof-photo.png",
# stub the authenticated meta response, flush the renderer update, and print fetchCalls + imageSrc.
NODE
node scripts/run-oxlint.mjs ui/src/ui/chat/grouped-render.ts ui/src/ui/chat/grouped-render.test.ts
npx oxfmt --check ui/src/ui/chat/grouped-render.ts ui/src/ui/chat/grouped-render.test.ts
git diff --check
node scripts/run-tsgo.mjs -p test/tsconfig/tsconfig.core.test.json

Evidence after fix:

RED before fix:
FAIL grouped-render.test.ts > renders canonical inbound media refs through the Control UI media route
AssertionError: expected "vi.fn()" to be called 1 times, but got 0 times

After fix targeted suite:
Test Files  4 passed (4)
Tests  200 passed (200)
Test Files  1 passed (1)
Tests  57 passed (57)

Actual jsdom renderer output:
{"fetchCalls":[{"url":"/openclaw/__openclaw__/assistant-media?source=media%3A%2F%2Finbound%2Fproof-photo.png&meta=1","auth":"Bearer session-token"}],"imageSrc":"/openclaw/__openclaw__/assistant-media?source=media%3A%2F%2Finbound%2Fproof-photo.png&mediaTicket=proof-ticket"}

Observed result after fix: The renderer now treats media://inbound/proof-photo.png as a managed inbound media ref, requests authenticated metadata from /openclaw/__openclaw__/assistant-media?...&meta=1, receives a media ticket, and renders the final <img> with /openclaw/__openclaw__/assistant-media?source=media%3A%2F%2Finbound%2Fproof-photo.png&mediaTicket=proof-ticket.

What was not tested: I did not run a live Telegram or iMessage account through the full Gateway UI in a browser screenshot pass. The channel receive path was not changed; existing Gateway tests cover serving canonical inbound refs, and the new proof exercises the shared WebUI renderer path that was leaving media://inbound unloadable.

@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui size: S proof: supplied External PR includes structured after-fix real behavior proof. labels Jun 4, 2026
@clawsweeper

clawsweeper Bot commented Jun 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 8:00 AM ET / 12:00 UTC.

Summary
The PR routes single-file media://inbound/<id> image transcript refs through the existing authenticated Control UI __openclaw__/assistant-media route and adds a grouped-render regression test.

PR surface: Source +24, Tests +51. Total +75 across 2 files.

Reproducibility: yes. at source level: current main extracts image MediaPath values into image URLs but does not classify media://inbound/<id> as assistant-media proxyable. I did not run a live browser reproduction in this read-only review.

Review metrics: none identified.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #89591
Summary: The linked issue is the canonical WebUI inbound-media preview report, and this PR is the open draft candidate fix for that issue.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🐚 platinum hermit
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add real browser or live Gateway Control UI proof showing a media://inbound/<id> image rendered inline through __openclaw__/assistant-media, with private details redacted.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body now includes terminal test output and a jsdom renderer probe, but the media metadata response is stubbed and no real browser or live Gateway Control UI proof shows the inline image after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Proof path suggestion
A visible Control UI proof would materially reduce the remaining browser-rendering uncertainty. Mantis is currently scoped to Telegram, Discord, and web UI chat proof, so it is not the right proof path for this surface.
Use maintainer screenshot/manual proof, browser or Playwright proof, Crabbox where appropriate, or normal local artifact proof instead.

Risk before merge

  • [P1] Merging without real browser or live Gateway Control UI proof could leave the visual inline-preview path unproven; the supplied jsdom probe stubs the metadata response instead of exercising a real served image.

Maintainer options:

  1. Require real Control UI proof (recommended)
    Have the contributor or a maintainer add a screenshot, recording, redacted logs, or linked artifact from a real browser or live Control UI setup showing the inbound image rendered through __openclaw__/assistant-media before merge.
  2. Accept the synthetic proof
    Maintainers could decide the jsdom renderer probe plus gateway route tests are enough for this narrow path, but that would explicitly accept the remaining browser-runtime uncertainty.

Next step before merge

  • [P1] The open draft PR is already the implementation candidate; the remaining action is real behavior proof plus ordinary maintainer review, not a narrow automated repair.

Security
Cleared: No concrete security or supply-chain concern found; the diff adds no dependencies, workflows, execution hooks, or new unauthenticated media serving path.

Review details

Best possible solution:

Land this or an equivalent narrow renderer fix after real browser or live Control UI proof shows a canonical inbound image rendering inline through the authenticated, ticketed assistant-media route.

Do we have a high-confidence way to reproduce the issue?

Yes, at source level: current main extracts image MediaPath values into image URLs but does not classify media://inbound/<id> as assistant-media proxyable. I did not run a live browser reproduction in this read-only review.

Is this the best way to solve the issue?

Yes for the implementation shape: reusing the existing authenticated, ticketed assistant-media route is narrower and safer than adding a new public inbound media serving path. Merge readiness still needs real browser or live Control UI proof.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 6cb82eaab865.

Label changes

Label changes:

  • add merge-risk: 🚨 other: The remaining risk is visual browser/runtime proof for the Control UI media preview path, which normal unit tests and CI do not fully settle.

Label justifications:

  • P2: This is a normal-priority WebUI media preview bugfix with limited blast radius and no evidence of data loss, security bypass, crash loop, or unusable core runtime.
  • merge-risk: 🚨 other: The remaining risk is visual browser/runtime proof for the Control UI media preview path, which normal unit tests and CI do not fully settle.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body now includes terminal test output and a jsdom renderer probe, but the media metadata response is stubbed and no real browser or live Gateway Control UI proof shows the inline image after the fix. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +24, Tests +51. Total +75 across 2 files.

View PR surface stats
Area Files Added Removed Net
Source 1 24 0 +24
Tests 1 51 0 +51
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 2 75 0 +75

What I checked:

  • Repository policy read: Root and UI-scoped AGENTS.md were read; their PR review guidance requires full-path source review and real behavior proof scrutiny for user-visible UI changes. (AGENTS.md:28, 6cb82eaab865)
  • Current-main renderer gap: Current main extracts transcript MediaPath image entries and appends the raw path as an image URL, so media://inbound/<id> enters rendering as a browser-unloadable custom scheme. (ui/src/ui/chat/grouped-render.ts:333, 6cb82eaab865)
  • Current-main proxy predicate: Current main only treats file, home, absolute, and Windows-drive paths as assistant-media proxyable; it has no media://inbound branch. (ui/src/ui/chat/grouped-render.ts:1015, 6cb82eaab865)
  • Existing serving boundary: The Control UI assistant-media handler already authorizes reads, supports metadata and ticketed media serving, and resolves sources through the media-reference path. (src/gateway/control-ui.ts:548, 6cb82eaab865)
  • Inbound reference contract: The shared parser accepts canonical media://inbound/<id> refs and rejects nested, cross-bucket, backslash, empty, and NUL ids before resolving store paths. (src/media/media-reference.ts:116, 6cb82eaab865)
  • PR implementation: The PR adds a renderer-local canonical inbound media predicate, excludes it from local path normalization, and allows it through the assistant-media availability flow. (ui/src/ui/chat/grouped-render.ts:1010, 98f8ec1fafcf)

Likely related people:

  • steipete: Authored or committed inbound media reference resolution and assistant-media serving fixes reused by this PR, plus history-backed chat image rendering work. (role: assistant-media and media-reference area contributor; confidence: high; commits: 6290ed52ff9f, 45730f61176d, 6d40de45c7b7; files: src/media/media-reference.ts, src/gateway/control-ui.ts, ui/src/ui/chat/grouped-render.ts)
  • vincentkoc: Recent grouped-render history shows repeated work in the same renderer surface and adjacent transcript rendering path. (role: recent Control UI renderer contributor; confidence: medium; commits: 45f935887769, b9b057b6d58f, 402e2bb81a73; files: ui/src/ui/chat/grouped-render.ts)
  • Syysean: Authored the gateway attachment offload work that stores received attachments as canonical media://inbound/<id> refs consumed by this WebUI path. (role: inbound media offload contributor; confidence: medium; commits: c6f2db150687; files: src/gateway/chat-attachments.ts, src/media/store.ts)
  • Takhoffman: The large Control UI assistant directive/embed change touched assistant media routing, media contracts, and grouped renderer behavior adjacent to this fix. (role: adjacent UI media feature contributor; confidence: medium; commits: cc5c691f0069; files: ui/src/ui/chat/grouped-render.ts, src/gateway/control-ui.ts, src/gateway/control-ui.http.test.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

How this review workflow works
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-06-21T12:21:39.050Z sha 98f8ec1 :: needs real behavior proof before merge. :: none

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal backlog priority with limited blast radius. labels Jun 4, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 15, 2026
Closed
@clawsweeper

clawsweeper Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

@sweetcornna thanks for the PR. ClawSweeper is still waiting on real behavior proof before this can move forward.

Useful proof can be a screenshot, short video, terminal output, copied live output, linked artifact, or redacted logs that show the changed behavior after the fix. Please redact private tokens, phone numbers, private endpoints, customer data, and anything else sensitive.

Once proof is added to the PR body or a comment, ClawSweeper or a maintainer can re-check it.

@clawsweeper clawsweeper Bot mentioned this pull request Jul 3, 2026
Open
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. label Jul 6, 2026
@steipete steipete self-assigned this Jul 6, 2026
@steipete steipete mentioned this pull request Jul 6, 2026
Merged
@steipete

steipete commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Superseded by #100725, now landed as 3170bfa489c8.

The replacement rebuilt this fix on current main, narrowed acceptance to canonical one-segment media://inbound/<id> references, kept authenticated metadata/ticket delivery, and added focused Gateway/HTTP plus full Chromium chat-flow proof. Thank you @sweetcornna for identifying the reload failure and contributing the original fix direction; contributor credit is preserved in #100740.

@steipete steipete closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

app: web-ui App: web-ui merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal backlog priority with limited blast radius. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. size: S status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Feature Request: Display inbound media attachments as image previews in WebUI

2 participants

@sweetcornna @steipete