fix(desktop): make HTML artifacts directly openable - #5216
Conversation
liugddx
changed the title
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Result
NO-GO for exact commit da213246597f40360705099a5273f1c6033ef693: 1 P2 and 1 P3. I found no P0 or P1 in the reviewed paths.
P2 — the promised Finder fallback is not reachable for unreadable HTML previews
The PR description says it retains an explicit Finder fallback for unreadable previews. The HTML failure path does not preserve that behavior:
apps/desktop/src/renderer/features/workbar/tools/artifacts/artifact-preview.tsx:179-182,338-347renders the HTML read-failure action with theShow in Finderlabel.apps/desktop/src/renderer/features/workbar/tools/artifacts/artifact-pane.tsx:262-283,548-550,582-588routes that action throughartifacts.openPath.apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts:94-105sends every HTML open request toshell.openPath; production injects it atapps/desktop/src/main/runtime-host-boot.ts:1637-1644. Only non-HTML artifacts useshowItemInFolder.- The failure copy still directs users to Finder in
apps/desktop/src/renderer/locales/artifact-copy.ts:110-118,146-154,182-190.
Thus, when an HTML preview is too large, deleted, or otherwise unreadable, the visible Show in Finder action launches the default application instead. There is no remaining renderer path that actually reveals the materialized HTML in Finder, contrary to the stated fallback contract.
Please keep separate Open and Show in Finder actions. The HTML failure card should use a Finder/reveal IPC operation (or an explicit action mode that forces showItemInFolder), while the successful HTML action continues to use shell.openPath.
P3 — HTML opener failure paths are not regression-tested
The new test at apps/desktop/src/main/__tests__/runtime-host-artifacts-ipc-main.test.ts:253-308 covers only successful HTML materialization and opening. It does not cover shell.openPath returning an error string, rejecting, materialization failure without invoking an opener, or the HTML failure CTA's Finder behavior. Existing Save As and non-HTML Finder tests do not cover these new paths.
Please add focused tests for the opener error and throw cases, assert no opener is called when materialization fails, and add a renderer regression for the HTML failure CTA.
The HTML path still goes through the existing main-process materialization, stream/byte validation, and path boundary. The sandboxed preview remains sandbox="allow-scripts" without same-origin, popups, or top-navigation permissions. I found no new path-containment, cross-session, or Host-protocol P0/P1 issue.
At review time, hosted package and test checks were still pending. git diff --check and the merge-tree check passed. Local build/typecheck and renderer architecture checks could not run because this checkout lacks a usable tsc and @babel/parser; a direct core visibility smoke passed. No real Electron/default-application smoke was available.
Automated review notice: This review was produced by an AI agent and is not a substitute for independent human review.
|
Addressed Astro-Han's P2/P3 review findings in 86eec55.
Verification:
|
|
Follow-up: the renderer regression is now kept inside the existing |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head c64a564ef4a553d094eb767212cfd835a6463a49.
The previously identified issues are resolved. The HTML preview failure action now uses a dedicated app:showArtifactInFolder path, while app:openArtifactPath remains the default-app opener. The preload bridge, Workbar adapter, and ArtifactPane keep “Open” and “Open in Finder” as separate actions. The new regression tests cover successful reveal, default-app opener errors/rejection, materialization failure without invoking an opener, and the HTML failure CTA.
I found no new P0–P3 issue on this head. Hosted test and package are successful; merge-tree and whitespace checks are clean. Local full build/typecheck/Electron smoke were not independently run because this checkout lacks a complete toolchain.
Automated review notice: This review was produced by an AI agent and is not a substitute for independent human review.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for separating Open from Show in Finder. I independently checked c64a564ef, including the earlier review fixes. The supported flow addresses the reported problem without adding a localhost server, changing Browser file-URL admission, or introducing another Artifact authority. Both native actions continue through the existing Main materialization and validation path; the embedded preview retains sandbox="allow-scripts".
I rebuilt this exact head and passed 28 focused tests across Artifact IPC, Workbar services/renderer behavior and core visibility. I also rendered the actual ArtifactPane with production CSS in Electron in light and dark modes: tool-result HTML appears in the list, Open and Show in Finder reach separate service callbacks, and the unreadable-preview CTA invokes only reveal. The embedded iframe keeps its sandbox restriction, and no renderer errors were observed. This local UI fixture uses controlled Artifact services; it does not independently repeat the author's full Write-to-default-browser handoff.
No P0–P2 findings. There is one non-blocking P3 simplification suggestion inline: share the duplicated local error handling while keeping the two operations explicit. No general facade or additional controller is needed. Current test/package checks are green.
AI-assisted review and local verification with Codex.
中文
当前修复保留“打开”和“在 Finder 中显示”的不同语义，仍通过现有 Main Artifact 校验与物化路径，没有新增服务器、Browser 放行规则或权威。28 项测试及真实 Electron light/dark 的列表、菜单、失败 CTA 和 iframe sandbox 检查通过。UI 验证使用可控 Artifact 服务，未独立重跑作者的完整 Write→默认浏览器流程。无 P0–P2；仅建议合并两处局部重复错误处理，不增加通用门面层。
| } | ||
| } | ||
|
|
||
| async function showInFinder(artifactId: string) { |
There was a problem hiding this comment.
[P3] The separate Open and Show in Finder actions are correct, but this function duplicates the session-activity guard, result handling and catch/toast logic in openArtifact immediately above. A small local helper that accepts the selected service operation could keep that policy in one place and remove most of this duplication, while preserving the explicit menu/CTA actions and their separate IPC operations. No new service class or generic facade is needed. This is non-blocking.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for making the HTML handoff usable and for fixing the Finder fallback. I would like to pause approval and settle one product decision before merging. My earlier review checked the implementation, but did not sufficiently question where the Open action should take the user.
I would like users to have an explicit choice between opening an HTML Artifact inside Maka and opening it externally. The existing sandboxed Artifact preview is a useful starting point for the in-app experience; the embedded Browser's file-URL restriction does not by itself mean that opening HTML should take the user out of Maka.
Keeping the in-app path matters beyond previewing: I would like to build toward annotating a generated page, giving feedback on a specific version, and asking Maka to improve it in the same task. That is a product direction, not a request to implement Annotation or introduce a speculative framework in this PR.
Could we first agree on the two entry points and their behavior here?
- An explicit in-app view, reusing the existing sandboxed Artifact surface where practical and keeping the connection to the Artifact and task.
- An explicit external-open action, with a label that makes leaving Maka clear. The label should match the actual handler:
shell.openPathuses the system default application, which is not guaranteed to be a browser.
Please keep the existing sandbox and path-validation boundaries. This discussion does not require relaxing file:// admission or adding a second preview implementation. If the existing preview already provides the intended in-app flow, making that choice discoverable may be enough.
I am withdrawing my approval while this interaction decision is discussed. The previous tests and implementation findings still stand; this is a product/design question, not a newly discovered code defect. Thanks for helping clarify it before the behavior becomes established.
中文
我想先暂停批准，讨论 HTML 的打开入口。之前的审查确认了实现，但没有充分判断用户点“打开”后应该留在 Maka 还是跳到外部，这点是我考虑不够。
我希望用户明确有两种选择：在应用内查看和交互，以及交给外部默认应用打开。已有沙箱 Artifact 预览可以作为应用内入口，不需要放开 file:// 或新建第二套预览。保留应用内路径，也为以后围绕具体版本做 Annotation、反馈和修改留出自然的工作流程；不要求这次实现标注或预建框架。
请先在这里确定两个入口的呈现和行为。外部入口的标签需要说清会离开 Maka，并与系统默认应用的实际行为一致。如果现有预览已经满足应用内路径，只需让这个选择更清晰，也可以是足够的方案。当前先撤回批准，待产品取舍收敛后再继续。
Withdrawing approval pending discussion of explicit in-app and external HTML opening choices, as explained in the follow-up review. Existing implementation verification remains valid.
|
Updated the HTML external action label to explicitly say Open in Default App (localized equivalents in Chinese and Traditional Chinese). The current Artifact detail screen remains the in-Maka preview entry; the menu now clearly distinguishes in-app viewing from external default-app opening and Finder reveal. Desktop typecheck and Biome pass. |
|
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Review outcome
I reviewed the exact head d2cce145fb4e8caa1757969c81a5ce4f4171fcec. I found no reachable P0–P2 issue in the HTML artifact opening/Finder fallback change.
The default-app and Finder paths are separated end to end: apps/desktop/src/main/runtime-host-artifacts-ipc-main.ts:86-135 materializes the artifact and routes HTML through shell.openPath, while app:showArtifactInFolder uses shell.showItemInFolder. The typed preload and Workbar wiring is covered at apps/desktop/src/preload/preload.ts:3656-3678 and apps/desktop/src/renderer/platform/desktop/create-workbar-services.ts:123-136. Renderer labels and the HTML failure CTA are covered at apps/desktop/src/renderer/features/workbar/tools/artifacts/artifact-pane.tsx:263-309,519-591,622-628 and artifact-preview.tsx:73-94,179-183,338-347.
The exact-head tests cover default-app success, Finder reveal, opener failure/rejection, materialization failure, and the HTML failure-card Finder callback (runtime-host-artifacts-ipc-main.test.ts:253-430, workbar-services-adapter.test.ts:286-350). The hosted test and package checks pass, and the PR is currently mergeable.
Non-blocking P3: default-app and Finder failures still share Finder-specific error copy (artifact-pane.tsx:263-308; artifact-copy.ts:96,132,168). Splitting those messages would make default-app failures more truthful.
Automated review notice: This review was produced by an AI agent and is not a substitute for independent human review.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Review at d2cce145fb4e8caa1757969c81a5ce4f4171fcec. Opening HTML means rendering untrusted content, so I led with the trust boundary rather than the feature. It holds up well; I have one question that the diff alone cannot answer.
The two tiers are distinct, and the user is told which one they are getting. In-app preview renders through an iframe with srcdoc and sandbox="allow-scripts" and without allow-same-origin, so the document sits in an opaque origin and cannot reach application state or same-origin resources; popups are not granted either, which is why the code explains that external links are blocked rather than silently doing nothing. The external path is a separate, explicitly labelled action — "Open in Default App" against "View in Maka" — and it hands the file to the operating system's handler rather than to an Electron window, so nothing is opened in a context with privileged bindings.
The IPC contract passes identifiers, not paths. The preload surface takes a session id and an artifact id; the main process resolves the artifact first and derives the path itself. A renderer therefore cannot ask for an arbitrary file to be opened or revealed, and the path construction — which is pre-existing and merely lifted into a shared helper here — is never influenced by a caller-supplied path. That is the right shape for this kind of feature and it is worth saying so explicitly, because it is the part that would be easy to get wrong.
On the risk this feature genuinely carries. After this change, an HTML artifact is one labelled click from opening in the user's browser under a file:// origin. That was already reachable before — reveal in folder, then double-click — so the change shortens a path rather than crossing a boundary, and the label makes the destination explicit. I am comfortable with that framing.
The one thing I could not establish, and it matters. The visibility rule added here makes tool_result artifacts user-visible when their kind is html, bypassing the source policy that otherwise keeps tool results internal:
if (record.source === 'tool_result' && record.kind === 'html') return true;The stated reason is good — a directly written HTML file is an intentional deliverable and should not require a child-workspace writeback to be seen. My question is how kind comes to be html for a tool_result artifact, because that assignment happens outside this diff. If the host derives it from the written content or the file extension, this rule is exactly as narrow as its comment claims. If a tool can declare the kind, then the rule lets a tool promote arbitrary content into the Generated Files tab by labelling it html, and that same artifact then carries the one-click external open described above. I am not asserting that is the case — I am asking, because the safety of this rule rests entirely on the answer and a reader of this diff cannot see it.
A smaller note. The injected openPath is optional, and when it is absent an HTML artifact falls back to revealing in the folder. Production wires it to the shell, so the fallback is only reachable in tests or an incomplete wiring; that is a reasonable default rather than a problem, but it does mean the feature's behaviour depends on composition rather than on the handler alone.
Verification limits. I read the diff and the surrounding files and traced the path from the renderer action through preload and the main-process handler to the shell call. I ran nothing: no test, build or application session, and I did not open an artifact. The required checks on this commit are terminal and successful, which I read from the API rather than reproducing.
I have not approved. The request I received was to review, and I do not assume an approval mandate from it. If you would like a formal approval on top of this, say so and I will submit one — subject to the question above having an answer.
Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for clarifying the interaction. I rechecked d2cce145f: the existing in-Maka preview and the explicitly labelled default-app action address my earlier product concern without introducing another preview implementation.
No P0–P2 findings remain. The 22 focused tests passed, and I verified the light/dark preview, menu actions, sandbox attribute and failure CTA in an Electron harness. The harness used controlled service responses; it did not repeat the native default-app launch.
The remaining local duplication and failure-copy wording are non-blocking P3 cleanup.
AI-assisted rereview and verification with Codex.
…f0a5d) Eight upstream commits. One reaches the new renderer's contracts: apache#4878 returns expected failures as codes across IPC — the five Session setters (`setPermissionMode`, `setCollaborationMode`, `setOrchestrationMode`, `setModelConfiguration`, `setThinkingLevel`) answer `DesktopSessionUpdateResult` instead of throwing, `abandonPlanProposal` answers `PlanControlIpcResult` like its siblings, `send` / `submitMessage` gain an `attachment_blocked` refusal, and the attachment guard throws a typed `AttachmentIngestBlockedError` in place of the `attachment_ingest:<code>` message token. Also in: apache#5216 makes HTML artifacts directly openable (`app.showArtifactInFolder` beside `openArtifactPath`, which now hands an HTML artifact to the default app; `isArtifactUserVisible` admits HTML tool results), apache#5198 unifies WorkHub conversation identity and Host-owned choices (an Astryx `ChoicePanel` in packages/ui, `keyboardHint` copy, a `preserveFocus` scroll target), apache#5249's skill picker fix (`selectedSkillIds` in `chat-input-behavior`), apache#4815 admits structured-only Messages (`hasMeaningfulMessageContent` in core), apache#4862's ACP live session lifecycle in the CLI, apache#5204's workbar tab scrollbar css and apache#5180's wider locale hygiene gate. Resolution per the sync policy: conflicts under the old renderer's trees, packages/ui's deleted components, stories, e2e specs and the main tests that import them stay deleted, and upstream's new files there are dropped (`features/workhub/model/workspace-name.ts`, packages/ui's `choice-panel.tsx` and its `index.ts` export, the `styles/base.css` / `workhub.css` / `maka-tokens.css` edits, `expected-error-presentation.test.ts` and the WorkHub main tests). git's rename pairing had put upstream's `features/session-settings/ports.ts`, `features/workhub/testing.ts` and `platform/desktop/create-session-settings-services.ts` into `bridge/e2e-fixture.ts`, `components/ui/skeleton.tsx` and `lib/ported/display-frame-scheduler.ts`; all three keep ours. The renderer architecture ledger keeps ours, rewritten with `--write`. The e2e budget and `transcript-scroll-cost.spec.ts` keep ours; upstream's new `expected-failure-feedback.spec.ts` is trimmed to its second case (the IPC round trip of the setting and Plan codes), the first needing the WorkHub surface this build does not ship. Re-implemented for the new contracts: - `bridge/sessions.ts` unwraps every update result and rethrows a refusal as `ExpectedOperationError` (new `bridge/expected-operation-error.ts`, ported from upstream's `operation-diagnostics.ts`), so the turn actions store and every caller keep awaiting a summary. `localizedShellErrorMessage` renders the code through the new `updateFailures` copy on every surface and `AttachmentIngestBlockedError` by its `code`; `sessionSettingFailureCopy` is upstream's. - `ChatInput` routes an `attachment_blocked` refusal through upstream's `showSubmissionFeedback` and keeps the draft with the ingest reason; `showSkillInvocationFeedback` stays exported for the partial-success toast. - `FilesTab` reveals through `showArtifactInFolder`; an HTML row shows "View in Maka", its menu offers "Open in Default App" first, and its preview's external action opens rather than reveals. `artifact-copy` gains `viewInMaka` / `openInDefaultApp` in three locales. - `TipTapEditor` hides Skills already in the draft from the picker (apache#5249); the chips are atoms here, so the set is read from the document rather than from the `/skill:x` text upstream scans. - `composer-state.test.ts` asserts the typed preflight error. packages/ui: `use-chat-scroll.ts` merged cleanly (`preserveFocus`) on top of our `holdTurn` extensions; `conversation-copy.ts` and `chat-input-behavior.ts` take upstream's additions. apache#5217's live-turn buffer stays out as before. The compatible-change declaration is unchanged this round. The release checklist's baseline note records that the eight commits add no unlisted renderer surface. Gates: build:test + build:renderer, typecheck, biome lint and format, locale hygiene (the widened apache#5180 gate), ASF headers, renderer architecture ledger (rewritten with `--write`), e2e budget, third-party notices, knip (39 unused files, unchanged from the twelfth sync), workspace dist tests (desktop 3484 of 3498 with 14 skipped, every other workspace green), Electron smoke (44 checks, no renderer errors), core-dialogue smoke, streaming-switch smoke, and the trimmed `expected-failure-feedback` e2e case against the real preload. `packages/runtime` `model-adapter-onerror` fails on this machine before and after, as in the eleventh and twelfth syncs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3 participants
Problem
A real H5 workflow exposed misleading or unreliable local preview assumptions: an ad-hoc HTTP server could be reported as
runningeven when127.0.0.1was unreachable;file://navigation is intentionally rejected by the embedded Browser; Shell and Desktop Browser do not necessarily share a sandbox; and shellopenis not a reliable GUI handoff. These boundaries allowed an agent to claim a preview was started or opened before the user had a usable page.Issue #5213 records the full harness self-evaluation. This PR implements the supported Desktop Artifact handoff, but it does not resolve the remaining harness/platform gaps listed in that issue.
Change
Openaction for HTML Artifacts.shell.openPath). On macOS this normally opens the default browser.OpenandShow in Finderas separate operations: unreadable HTML previews and the explicit Finder action use a dedicated reveal IPC path backed byshell.showItemInFolder.file://security boundary; no localhost server or Browser admission change is introduced.This makes the supported H5 flow repeatable:
Write HTML → register HTML Artifact → Generated Files → Open → default application.Scope
Maka Desktop Artifact UI, Artifact visibility policy, and existing Artifact IPC only. The PR does not add a generic process supervisor, shared Shell/Browser networking, background-process health checks, capability preflight, or a platform-managed preview endpoint. Those remaining harness/platform gaps stay open in #5213.
Relates to #5213
Closes #5215
Related: #4267
Validation
Local checks on the latest review fix:
npm run typecheck --workspace apps/desktopnpm run check:architecture --workspace apps/desktopnpx knip --workspace apps/desktopnpm run build:test --workspace apps/desktopnode --test apps/desktop/dist/main/__tests__/runtime-host-artifacts-ipc-main.test.js apps/desktop/dist/main/__tests__/workbar-services-adapter.test.js(20/20)Maka Dev end-to-end verification of the successful HTML Artifact path:
Writeto create an interactive HTML file.生成文件 1.打开; clicking it launched Chrome Canary through the system-default-app path.Preview E2E Ready.Run interactionchanged the page toInteraction confirmedand4 of 4 confirmed.No Paseo, subagent, localhost server, or embedded Browser tool was used by the Maka task.