Fix browser pane screenshots showing placeholder instead of actual page content#247
Open
danshapiro wants to merge 58 commits intomainfrom
Open
Fix browser pane screenshots showing placeholder instead of actual page content#247danshapiro wants to merge 58 commits intomainfrom
danshapiro wants to merge 58 commits intomainfrom
Conversation
…hints When called with unrecognized params (e.g. new-tab with url), the MCP tool now returns an error listing valid parameters. A specific hint suggests open-browser when url is passed to new-tab. Help text updated to clarify the distinction between new-tab and open-browser, with a new playbook for opening URLs.
- Remove localStorage persistence for tool strip expanded state - ToolStrip now uses local useState initialized from showTools prop - ToolBlocks inherit initial expanded state from showTools - Remove autoExpandAbove/completedToolOffset props (no longer needed) - All toggle state is session-only, resets on page refresh
Add implementation plan for making terminal URLs clickable (opening in browser panes) with right-click context menu support for open in pane, open in tab, open in browser, and copy URL. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reviewed all source files referenced by the plan against the actual codebase and corrected several issues: - Fix ILinkHandler hover/leave signatures (need range param for OSC 8) - Add explicit wrapperRef JSX attachment instructions for outer div - Fix test file reference to canonical path (components/context-menu/) - Add useMemo dependency array update note for ContextMenuProvider - Clarify link provider registration order for priority - Note cleanup needs for both hoveredUrl map and data attribute - Add context-menu-utils.test.ts to new files list - Document picker vs direct browser pane design decision Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unnecessary empty hover/leave stubs on file path links - Note pre-existing mock staleness in menu-defs.test.ts (6 missing actions) - Add docs/index.html update step per repo rules - Add timing safety note for wrapperRef in hover callbacks - Clarify ContextMenuProvider action wiring locations and imports - Add balanced parenthesis edge case for URL detection tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Corrects three factual errors in the implementation plan: 1. xterm.js link provider priority is first-registered=highest (not last) 2. menu-defs.test.ts does not exist yet (was incorrectly labeled "update existing") 3. Removes context-menu-constants.ts from modified files list (no changes needed) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
58 new tests across 8 files plus updates to 2 existing test files, aligned to the implementation plan's TDD phases. Covers hover state tracking, URL detection, left-click behavior, context menu integration, multi-pane integration, and browser-use E2E smoke testing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Phase 1 of clickable terminal URLs: module-level map for tracking hovered URLs per pane, and findUrls utility for detecting http/https URLs in terminal output text. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…link provider - Left-click on OSC 8 links and detected URLs now opens a browser pane (split right) instead of window.open - Warning modal confirm also opens browser pane instead of window.open - Add hover/leave callbacks to OSC 8 linkHandler for tracking hovered URL - Register URL link provider (after file path provider) to detect plain http/https URLs in terminal output - Track hovered URL in module-level map and data-hovered-url DOM attribute - Clear hover state on terminal dispose and tab hide - Update existing link warning and keyboard tests for new behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add hoveredUrl optional field to terminal ContextTarget - Parse hoveredUrl from dataset in parseContextTarget - Add URL-specific menu items (Open in pane/tab/browser, Copy URL) to terminal context menu when hovering a URL - Add openUrlInPane, openUrlInTab, openUrlInBrowser, copyUrl actions to ContextMenuProvider - Add tests for context-menu-utils and menu-defs URL behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- terminal-url-link-click: verifies URL click in nested pane opens browser pane on the correct branch (both plain URL and OSC 8 paths) - terminal-url-context-menu: verifies URL context menu items appear when hovering a URL, are absent without hover, and "Open URL in pane" creates a browser pane split Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verifies that when a terminal tab becomes hidden, the hovered URL module state and DOM attribute are cleared. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
With the new URL link provider registered after the file path provider, the existing test mock was capturing the URL provider instead of the file path provider. Fixed by only storing the first registered provider. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unnecessary escape in url-utils regex character class - Capture wrapperRef.current in local variable before cleanup to satisfy react-hooks/exhaustive-deps rule Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ks, docs update - URL detection now preserves balanced parentheses (e.g. Wikipedia URLs) while still stripping unbalanced trailing parens - OSC 8 linkHandler.activate validates http/https scheme before opening browser panes; non-http schemes fall back to window.open - Added clickable URLs feature to docs/index.html feature list Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Prevents non-left-click from triggering link activation on OSC 8 links, file path links, and URL links. Adds tests for right-click and middle-click. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Align .opencode/.gitignore with upstream best practices: add plans/, package-lock.json, and .freshell-mcp-state.json. Remove the transient MCP state file from tracking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace flat list layout with a compact card grid organized by device. Key changes: - Device-centric grouping: local tabs first, then remote by device - Compact card grid (auto-fill 220px min) with colored left borders - Right-click context menus with Jump/Pull/Open pane/Copy actions - Segmented controls for status and scope filters - Hover-to-reveal action labels on cards - Collapsible "Recently closed" section - Pane type icons with distinct colors for visual scanning - Click-to-act: click card to jump (local) or pull (remote/closed) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update all 4 test files to match the new device-centric card grid layout: - Tests now click tab cards directly instead of finding "Open copy" buttons - Section headings updated for device-centric grouping - New tests for: device grouping, context menus, segmented filters, pane kind icons, multi-pane context menu items - Explicit cleanup between tests to prevent DOM leakage Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Strip X-Frame-Options and Content-Security-Policy headers from proxied responses in proxy-router.ts so browser pane iframes can render localhost content and the MCP screenshot tool captures actual content instead of a placeholder. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Concrete test plan covering proxy header stripping, iframe screenshot capture, Playwright E2E verification, graceful fallback preservation, and MCP instructions accuracy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Dev servers commonly send X-Frame-Options and Content-Security-Policy headers that prevent iframe embedding. Since the proxy exists to make localhost content embeddable in browser panes, strip these headers so the iframe renders content and the MCP screenshot tool can capture it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Documents that browser pane iframes using /api/proxy/http/PORT/ URLs are captured as image content (not placeholders) when the proxy strips iframe-blocking headers, making the content same-origin accessible. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Proxied localhost URLs now render actual iframe content instead of placeholders. Update both the key gotchas and screenshot guidance sections to clarify that only truly cross-origin URLs show placeholders. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
R4: Fix variable shadowing of 'headers' in proxy-router.ts where the inner const in the proxy callback shadowed the outer request headers. Renamed to 'strippedHeaders' for clarity. R3: Add MCP instructions content verification test ensuring the tool instructions correctly state that proxied localhost URLs render actual content (not placeholders) in browser pane screenshots. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
R1: Test that proxied localhost URLs with X-Frame-Options: DENY and CSP frame-ancestors 'none' headers render actual content in the iframe (not a placeholder), and that the MCP screenshot API succeeds. R2: Test that truly cross-origin URLs (https://example.com) correctly fall back to placeholder behavior since they bypass the proxy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…browsers When the browser accesses Freshell remotely, http: localhost URLs were using TCP port forwarding which creates cross-origin iframe URLs, preventing screenshot capture. Now buildHttpProxyUrl handles http: localhost URLs regardless of browser location, routing through the same-origin /api/proxy/http/:port/ path. HTTPS URLs still use TCP forwarding since the HTTP proxy can't do TLS passthrough. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b01dfce5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+21
to
+22
| 'content-security-policy', | ||
| 'content-security-policy-report-only', |
There was a problem hiding this comment.
Removing content-security-policy and content-security-policy-report-only for every proxied response drops much more than frame-ancestors, including script and sandbox protections that upstream services rely on. In the browser pane this content is served from Freshell’s own origin (/api/proxy/http/:port/...), so stripping full CSP can let a compromised or untrusted localhost app run with authenticated same-origin access to Freshell APIs. Please only relax the embedding directives (e.g., rewrite/remove frame-ancestors) instead of deleting the whole CSP policy.
Useful? React with 👍 / 👎.
| ) | ||
| const expanded = showTools && expandedPref | ||
| function ToolStrip({ pairs, isStreaming, showTools = true }: ToolStripProps) { | ||
| const [stripExpanded, setStripExpanded] = useState(showTools) |
There was a problem hiding this comment.
showTools changes
stripExpanded is initialized from showTools only on mount, so runtime toggles of the “Show tools” setting are ignored for existing strips. A common case is turning tools off in settings while a strip is expanded: it stays expanded because rendering is driven by stale local state, not the updated prop. This regresses the toggle behavior and should be fixed by syncing state to prop changes (or deriving expansion directly from the prop).
Useful? React with 👍 / 👎.
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
1 participant
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.
Summary
/api/proxy/http/:port/forwardedX-Frame-OptionsandContent-Security-Policyheaders from upstream servers verbatim, which blocked iframecontentDocumentaccess needed forhtml2canvasscreenshot capturehttp:localhost URLs route through the same-origin HTTP proxy regardless of browser location.https:URLs still use TCP forwarding since the HTTP proxy can't do TLS passthrough.Changes
server/proxy-router.ts—stripIframeBlockingHeaders()removesx-frame-options,content-security-policy, andcontent-security-policy-report-onlyfrom proxied responsessrc/components/panes/BrowserPane.tsx—buildHttpProxyUrl()now handles http: localhost URLs for both local and remote browsersserver/mcp/freshell-tool.ts— Updated MCP tool instructions to reflect new behaviorTest plan
npm test— all 300+ client test files, server tests, electron tests passX-Frame-Options: DENY+CSP: frame-ancestors 'none'renders actual content in screenshot🤖 Generated with Claude Code