Repository navigation
feat(file): open local HTML previews in Chrome - #57734
xianshang33 wants to merge 1 commit into
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
I read the change and type-checked it but couldn't run it: the button needs the macOS desktop app with Chrome installed, and the component spec needs Playwright browsers, which this environment doesn't have. tsc -b and the existing src/file tests pass.
Public API: unchanged (one internal UI action and one new English string).
Concerns:
- Design: #57708 is still waiting for maintainer design approval, as you note. A Chrome-only, macOS-only toolbar button is a product decision. It overlaps with the more general proposals in #57477 (custom apps in "Open in") and #47765 (file type → external app), so it's worth settling the direction there first.
- Duplicated launch logic:
OpenInChromeButtonre-implements whatuseOpenInAppinopen-in-app.tsxalready does: the installed-app check,desktop.launch, the pending guard and the error toast. It also skips the sharedshared.installedcache, so every HTML view mount asks the OS about Chrome again (see inline). - Toolbar layout: the wrap change applies to every file preview, not just HTML (see inline).
- Tests: a 206-line Playwright spec plus a 223-line fixture is a lot for one button (see inline).
- Unrelated change: the formatting-only change to the
ArtifactVideoclassListcan be dropped.
| function OpenInChromeButton(props: { session: MountedSession; path: string }) { | ||
| const ctx = useExtension() | ||
| const desktop = () => (ctx.desktop?.os === "macos" ? ctx.desktop : undefined) | ||
| const installed = createLatest(desktop, (value) => value.installed("Google Chrome").catch(() => false)) |
There was a problem hiding this comment.
This duplicates what useOpenInApp already does: the installed-app check, desktop.launch with an app name, the pending guard and showRequestError. It also skips the shared shared.installed cache, so every HTML view mount asks the OS about Chrome again. Could this reuse the existing open-in-app code (or at least shared.installed) so there's a single launch path?
|
|
||
| return ( | ||
| <div data-slot="artifact-toolbar" class="flex h-10 shrink-0 items-center gap-3 px-4"> | ||
| <div data-slot="artifact-toolbar" class="flex min-h-10 shrink-0 flex-wrap items-center gap-x-3 gap-y-1 px-4 py-1"> |
There was a problem hiding this comment.
ArtifactToolbar is shared by every file preview (images, video, PDF, office files, etc.), not just HTML. Switching from a fixed h-10 to min-h-10 flex-wrap py-1 changes the toolbar layout everywhere. If it's only needed because of the extra HTML button, it would be safer to keep the change to that case, or to show screenshots of the other preview types in a narrow panel.
| @@ -0,0 +1,223 @@ | |||
| import { createStore } from "solid-js/store" | |||
There was a problem hiding this comment.
Together with artifact-chrome.spec.ts, this is about 430 lines of test code for one button. The fixture builds a whole fake desktop with lots of test-only state buttons (remote/web/Windows/Linux, active screen, unmount…). The spec mounts on the unrelated ui-line-comment--editor story and checks pixel bounds against the viewport. Could you cut this down to the core cases: the button shows only on macOS local HTML with Chrome installed, it launches the resolved absolute path, and a failed launch shows the toast and allows a retry?
7fdbdfc to
4a67729
Compare
|
Rebased on
Verified after rebasing: Still not run: real macOS Electron → Chrome launch, Windows/Linux, full backend e2e. No Desktop screenshot: I could not drive the Desktop app to a file-preview screen automatically (opening a workspace needs the native folder picker), and I did not want to present a test-harness render as the app UI. |
Issue for this PR
Closes #57708
Type of change
What does this PR do?
Adds a compact Open in Chrome action to local HTML file previews on macOS Desktop. It appears only when Chrome is detected, and launches the current file's absolute path through the existing desktop launcher. The pending guard, the
shared.installedcache and the error toast are the same ones the existing "Open in" menu uses (checkAppInstalled,showRequestError,resolveOpenInAppPath); no second launch path, SDK or IPC was added.The built-in browser action, workspace editor choice and system default browser are unchanged, and the shared artifact toolbar keeps its fixed 40px single-row layout: the new action is a 24px icon button, so no other preview type changes.
Design approval on #57708 is still pending; this is submitted for consideration, not as an approved design.
How did you verify your code works?
Isolated macOS checkout, pinned Bun 1.4.2, frozen dependencies:
bun run check: 34/34 package typechecks, lint 0 errors (the same 1,250 pre-existing warnings).bun run lint:changed origin/dev: 0 problems in the 5 changed files.bun testinpackages/gui-extensions: 174 passed, 1 platform skip, 0 failures.artifact-chrome.spec.ts: 6 passed; 2 workers × 10 with no retries: 60/60. Full cold app component suite: 127/127 with no optimizer reloads.The spec renders the real
ArtifactViewand defers only the desktop boundary. It covers the gating (macOS + local + HTML + installed), exact absolute paths for relative and already-absolute files, hidden while pending/absent/rejected, the duplicate-launch guard, and the failed-launch toast with retry. These cases fail against the base implementation.Not run: real Electron-to-Chrome launch, Windows/Linux, and full backend e2e. Runtime evidence is a simulated desktop SDK boundary; it does not establish OS integration or CI results. No Desktop screenshot is attached: the automated Desktop capture I attempted could not reach a file-preview screen (opening a workspace needs the native folder picker), so I did not substitute a test-harness render.
Checklist