Repository navigation
Add browser screenshot clipboard actions - #4479
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds right‑click "Screenshot Page" and "Screenshot Section" actions plus an address‑bar screenshot button; captures full-page snapshots (with tiled stitch fallback) or user-selected regions, crops images, writes PNG/TIFF to the clipboard, shows a transient flash/copy indicator, and includes tests, localization, and project wiring. ChangesBrowser Screenshot Capture Feature
Sequence Diagram(s)sequenceDiagram
participant PanelView as BrowserPanelView
participant Panel as BrowserPanel
participant WebView as CmuxWebView
participant Pipeline as BrowserScreenshotPipeline
participant Pasteboard as NSPasteboard
PanelView->>Panel: trigger captureScreenshotPageToClipboard()
Panel->>WebView: captureScreenshotPageToClipboard()
WebView->>Pipeline: captureAndWrite(mode:snapshotProvider:pasteboard:)
Pipeline->>Pasteboard: write PNG + TIFF
Pipeline-->>WebView: return BrowserScreenshotResult
WebView-->>PanelView: success -> show copied indicator / flash
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…reenshot-context-menu
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 11004-11071: Add full locale blocks for the four new localization
keys (browser.screenshotPage.copy.help, browser.screenshotPage.copied,
browser.screenshotPage.copied.help, browser.screenshotSection.instructions) in
Resources/Localizable.xcstrings so they include entries for every locale already
present in that catalog (not just en/ja); for each locale add the same
"stringUnit" structure with "extractionState": "manual" and a "stringUnit"
containing "state" and "value" (use the translated text if available, otherwise
set the locale's "state" to an appropriate placeholder like "needs-translation"
and a fallback value) to match the existing locale blocks format used throughout
the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a846992b-e2b6-4ce0-91cb-c5725c659d8e
📒 Files selected for processing (6)
Resources/Localizable.xcstringsSources/Panels/BrowserPanelView.swiftSources/Panels/BrowserScreenshot.swiftSources/Panels/CmuxWebView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CmuxWebViewDragRoutingTests.swift
Greptile SummaryImplements browser screenshot-to-clipboard via a new toolbar camera button (full-page capture with a 1.4 s "Copied" badge) and two context-menu items ("Screenshot Page" / "Screenshot Section"), backed by a three-layer architecture split across
Confidence Score: 5/5Safe to merge. All previously flagged issues are resolved and no new defects were found. The capture pipeline, tile stitching, crop math, capture gate, pasteboard writer, AppKit overlay, and toolbar badge all look correct. The timer-based Copied indicator is properly invalidated on disappear. The tiled fallback restores scroll position before re-throwing any error. Every new string key carries full translations across all 20 supported locales. Behavior tests cover the critical paths including gate re-entrancy, crop coordinate math, tile-placement clamping, and the 100 MP rejection guard. No actor isolation mistakes, no blocking primitives, and all three new files are well within the per-file line budget. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant BPV as BrowserPanelView
participant CMW as CmuxWebView
participant Gate as CaptureGate
participant Pipeline as ScreenshotPipeline
participant Snapper as WebViewSnapshotter
participant PB as NSPasteboard
Note over U,PB: Full-page toolbar button flow
U->>BPV: Click camera button
BPV->>BPV: Set captureInProgress true
BPV->>CMW: captureScreenshotPageToClipboard
CMW->>Gate: run operation
Gate->>Pipeline: captureAndWrite fullPage
Pipeline->>Snapper: captureFullPage
Snapper->>Snapper: webContentMetrics via JS
Snapper->>Snapper: validateFullPageSize max 100MP
alt single snapshot covers 95pct of content
Snapper-->>Pipeline: NSImage
else tiled fallback
loop each tile position
Snapper->>CMW: scrollTo via JS with rAF wait
Snapper->>CMW: takeSnapshot viewport
CMW-->>Snapper: tile NSImage
Snapper->>Snapper: drawTile into output bitmap
end
Snapper->>CMW: restoreScrollOffset
Snapper-->>Pipeline: stitched NSImage
end
Pipeline->>PB: clearContents then write PNG and TIFF
Pipeline-->>CMW: BrowserScreenshotResult
CMW->>CMW: BrowserScreenshotFlash show
CMW-->>BPV: didCopy true
BPV->>BPV: showCopiedIndicator Timer 1.4s
BPV->>BPV: reset captureInProgress on timer fire
Note over U,PB: Section context-menu flow
U->>CMW: contextMenuScreenshotSection
CMW->>CMW: beginScreenshotSectionSelection
CMW->>CMW: add SelectionOverlayView crosshair
U->>CMW: drag to select region then mouseUp
CMW->>Gate: run captureAndWrite section
Gate->>Pipeline: captureAndWrite mode section
Pipeline->>Snapper: captureVisibleViewport
Snapper-->>Pipeline: viewport NSImage
Pipeline->>Pipeline: BrowserScreenshotCrop scale and clamp
Pipeline->>PB: clearContents then write cropped PNG and TIFF
CMW->>CMW: BrowserScreenshotFlash show
Reviews (12): Last reviewed commit: "fix: address screenshot review feedback" | Re-trigger Greptile |
There was a problem hiding this comment.
1 issue found across 6 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserScreenshotPipeline.swift`:
- Around line 11-24: Replace the hard-coded English messages in the
errorDescription computed property with localized lookups using
String(localized:defaultValue:) (or your app's localization helper) for each
case (.emptySnapshot, .invalidSelection, .invalidImageRepresentation,
.pasteboardWriteFailed, .webContentMetricsUnavailable); update the
.emptySnapshot text to a generic message that does not mention "WebKit" (e.g.,
"No screenshot was returned") and add corresponding keys and default values to
the app's string catalog so the localized lookups resolve at runtime, keeping
the same enum and property names (errorDescription) intact.
- Around line 111-115: The code in BrowserScreenshotPipeline.write clears the
system pasteboard before attempting to write the new NSPasteboardItem, which can
erase the user's clipboard if pasteboard.writeObjects([item]) fails; instead, do
not call pasteboard.clearContents() up-front — create the item via
pasteboardItem(for:), attempt pasteboard.writeObjects([item]) first, and only
modify/clear the pasteboard if the write succeeds (or simply rely on
writeObjects replacing contents on success); update the logic in static func
write(_ image: NSImage, to pasteboard: NSPasteboard = .general) to try writing
the item and throw BrowserScreenshotError.pasteboardWriteFailed on failure
without clearing existing clipboard contents, referencing pasteboardItem(for:)
and write(_:to:) to locate the change.
In `@Sources/Panels/BrowserScreenshotSnapshotter.swift`:
- Around line 58-88: Instead of buffering every captured tile into tiles,
allocate the destination bitmap once (sized to contentSize) and composite each
tile into it immediately after capture to avoid OOM: create a destination
NSImage/CGContext/NSBitmapImageRep before the loops, remove the tiles array and
in the nested loops call scroll(webView,to:), await
captureVisibleViewport(from:), draw the returned tile into the destination at
origin NSPoint(x:y:) (and release the tile afterwards), and track whether any
tile was drawn; preserve the existing error handling pattern (captureError
variable and restoring scroll via try? await scroll(webView, to:
metrics.scrollOffset)), then if captureError is set throw it, and if no tiles
were drawn throw BrowserScreenshotError.emptySnapshot, finally return the
composited destination image instead of calling stitchedImage(...). Ensure you
still reference tileOrigins(...), scroll(webView,to:),
captureVisibleViewport(from:), metrics.scrollOffset, and
BrowserScreenshotError.emptySnapshot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26a3840c-5591-4bda-9b6c-54c4542519fe
📒 Files selected for processing (5)
Sources/Panels/BrowserPanelView.swiftSources/Panels/BrowserScreenshot.swiftSources/Panels/BrowserScreenshotPipeline.swiftSources/Panels/BrowserScreenshotSnapshotter.swiftcmux.xcodeproj/project.pbxproj
💤 Files with no reviewable changes (1)
- Sources/Panels/BrowserScreenshot.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/BrowserScreenshotSnapshotter.swift`:
- Around line 150-152: Drawing uses the full viewport size as the source rect
which can exceed the actual tile.size and cause scaling; update the tile.draw
call in BrowserScreenshotSnapshotter (where tile.draw(in: destination, from:
NSRect(origin: .zero, size: tile.size)) is used) to compute a bounded source
rect that uses the minimum of tile.size and the reported viewportSize (for both
width and height) so the source rect never exceeds tile.size, preventing
unintended scaling and preserving pixel alignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c26f2bfc-fb34-4759-a501-2c3f374b0fba
📒 Files selected for processing (1)
Sources/Panels/BrowserScreenshotSnapshotter.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 11641-11765: The five new localization keys
(browser.screenshot.error.emptySnapshot,
browser.screenshot.error.invalidImageRepresentation,
browser.screenshot.error.invalidSelection,
browser.screenshot.error.pasteboardWriteFailed,
browser.screenshot.error.webContentMetricsUnavailable) contain copied English
marked as "translated" for mature locales (de, es, fr, it, ko, nb, pt-BR, ru,
uk, zh-Hans, zh-Hant); update each of those locale entries to either supply
proper native translations for the respective key or switch their stringUnit
state to the project’s non-final workflow (e.g., clear the value and mark as
untranslated/placeholder per repo convention) so that no copied English is
marked as translated for these keys. Ensure you update the exact string keys
listed above in the Localizable.xcstrings entries and keep other locales
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 782d11e9-54d8-4cbe-93fc-12993917e966
📒 Files selected for processing (2)
Resources/Localizable.xcstringsSources/Panels/BrowserScreenshotPipeline.swift
|
Addressed the remaining Greptile timer actor-isolation note in |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Panels/BrowserScreenshotSnapshotter.swift (1)
159-172:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlways append the last tile origin when any remainder is still uncovered.
The
> 0.5check drops the final origin when the page exceeds the viewport by less than half a point, which clips the right/bottom edge instead of capturing the remaining strip.🩹 Proposed fix
- if origins.last.map({ abs($0 - last) > 0.5 }) ?? true { + if origins.last.map({ abs($0 - last) > CGFloat.ulpOfOne }) ?? true { origins.append(last) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/BrowserScreenshotSnapshotter.swift` around lines 159 - 172, tileOrigins currently skips appending the final origin when the remaining uncovered strip is <= 0.5 points, which can clip the edge; in the tileOrigins(contentLength:viewportLength:) function replace the tolerance check (abs($0 - last) > 0.5) with a definitive check that the last origin differs from the computed last (e.g. append last when origins.isEmpty || origins.last! < last or origins.last != last) so the final tile origin is always added whenever any remainder remains uncovered.cmuxTests/CmuxWebViewDragRoutingTests.swift (1)
259-259: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider extracting
BrowserScreenshotPipelineTeststo its own file.This file is named
CmuxWebViewDragRoutingTests.swiftbut now hosts an unrelatedBrowserScreenshotPipelineTestsclass covering the screenshot pipeline. Splitting it intocmuxTests/BrowserScreenshotPipelineTests.swiftkeeps each test file aligned with a single feature/responsibility and improves discoverability when navigating from the production sources (Sources/Panels/BrowserScreenshotPipeline.swift,BrowserScreenshotSnapshotter.swift).As per coding guidelines: "Do not mix UI rendering, state ownership, persistence, networking, parsing, subprocess/socket protocol, and platform bridge code in one Swift file" — the same single-responsibility principle applies here to keep drag-routing vs. screenshot pipeline test surfaces separated.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/CmuxWebViewDragRoutingTests.swift` at line 259, The BrowserScreenshotPipelineTests class is unrelated to CmuxWebViewDragRoutingTests and should be moved to its own test file: create a new cmuxTests/BrowserScreenshotPipelineTests.swift, paste the final class BrowserScreenshotPipelineTests { ... } into it, add the necessary imports (import XCTest and any test helpers used by BrowserScreenshotPipelineTests), adjust any file-scoped helpers or test utilities referenced by the class (move or make them internal/public as needed), and remove the BrowserScreenshotPipelineTests declaration from CmuxWebViewDragRoutingTests.swift so each file only contains its single responsibility test suite.
♻️ Duplicate comments (1)
Sources/Panels/BrowserScreenshotPipeline.swift (1)
129-133:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't erase the current clipboard before the new screenshot is accepted.
clearContents()runs before the pasteboard write is known to succeed, so a failedwriteObjects([item])still wipes the user's existing clipboard contents.🩹 Proposed fix
static func write(_ image: NSImage, to pasteboard: NSPasteboard = .general) throws { let item = try pasteboardItem(for: image) - pasteboard.clearContents() guard pasteboard.writeObjects([item]) else { throw BrowserScreenshotError.pasteboardWriteFailed } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/BrowserScreenshotPipeline.swift` around lines 129 - 133, The pasteboard.clearContents() call in static func write(_:to:) wipes the clipboard before we know pasteboard.writeObjects([item]) succeeded; remove the pre-emptive clearContents() (or move it after a successful write) so we only modify the user's clipboard when writeObjects returns true, keeping the existing clipboard intact when writeObjects fails; locate the write function and the pasteboardItem(for:) call and ensure failure still throws BrowserScreenshotError.pasteboardWriteFailed without having cleared the pasteboard first.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/CmuxWebViewDragRoutingTests.swift`:
- Around line 386-403: The test only asserts an oversized width case; add a
symmetric negative-case that calls
BrowserScreenshotCaptureBounds.validateFullPageSize with an oversized height
(e.g., NSSize(width: 10_000, height: 10_001)) and assert it throws
BrowserScreenshotError.captureAreaTooLarge in the same pattern as the existing
XCTAssertThrowsError block so both axes are validated; locate the test method
testFullPageCaptureBoundsRejectsHugePageBeforeBitmapAllocation and add the
second XCTAssertThrowsError that mirrors the width check but swaps the oversized
dimension to height.
---
Outside diff comments:
In `@cmuxTests/CmuxWebViewDragRoutingTests.swift`:
- Line 259: The BrowserScreenshotPipelineTests class is unrelated to
CmuxWebViewDragRoutingTests and should be moved to its own test file: create a
new cmuxTests/BrowserScreenshotPipelineTests.swift, paste the final class
BrowserScreenshotPipelineTests { ... } into it, add the necessary imports
(import XCTest and any test helpers used by BrowserScreenshotPipelineTests),
adjust any file-scoped helpers or test utilities referenced by the class (move
or make them internal/public as needed), and remove the
BrowserScreenshotPipelineTests declaration from
CmuxWebViewDragRoutingTests.swift so each file only contains its single
responsibility test suite.
In `@Sources/Panels/BrowserScreenshotSnapshotter.swift`:
- Around line 159-172: tileOrigins currently skips appending the final origin
when the remaining uncovered strip is <= 0.5 points, which can clip the edge; in
the tileOrigins(contentLength:viewportLength:) function replace the tolerance
check (abs($0 - last) > 0.5) with a definitive check that the last origin
differs from the computed last (e.g. append last when origins.isEmpty ||
origins.last! < last or origins.last != last) so the final tile origin is always
added whenever any remainder remains uncovered.
---
Duplicate comments:
In `@Sources/Panels/BrowserScreenshotPipeline.swift`:
- Around line 129-133: The pasteboard.clearContents() call in static func
write(_:to:) wipes the clipboard before we know pasteboard.writeObjects([item])
succeeded; remove the pre-emptive clearContents() (or move it after a successful
write) so we only modify the user's clipboard when writeObjects returns true,
keeping the existing clipboard intact when writeObjects fails; locate the write
function and the pasteboardItem(for:) call and ensure failure still throws
BrowserScreenshotError.pasteboardWriteFailed without having cleared the
pasteboard first.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3725e54f-f0ed-4ba9-8729-71971e0b2733
📒 Files selected for processing (5)
Resources/Localizable.xcstringsSources/Panels/BrowserPanelView.swiftSources/Panels/BrowserScreenshotPipeline.swiftSources/Panels/BrowserScreenshotSnapshotter.swiftcmuxTests/CmuxWebViewDragRoutingTests.swift
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5923b10. Configure here.

Summary
Screenshot PageandScreenshot Section, preserving WebKit's default menu items.Visual verification
Captured locally from the tagged Debug build:
/tmp/cmux-4472-pr-artifacts/context-menu-after-badge-build.png/tmp/cmux-4472-pr-artifacts/section-instructions.png/tmp/cmux-4472-pr-artifacts/section-overlay-selection.pngManual checks covered embedded browser panes, a normal page, a tall page, and a wide page. Clipboard inspection confirmed PNG/TIFF output for full-page and cropped section captures.
Test approach
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-4472-browser-screenshot-context-menu --launch.Closes #4472
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Adds new WebKit/AppKit screenshot capture flows (full-page tiling, selection overlay, clipboard writes) that could impact browser performance/memory and user clipboard behavior if edge cases aren’t handled.
Overview
Adds browser screenshot-to-clipboard support via a new toolbar camera button (full page) and new context-menu items for Screenshot Page and Screenshot Section.
Introduces a shared screenshot pipeline that snapshots full pages (single-shot with stitched-tiles fallback), crops section selections, writes PNG/TIFF to the pasteboard, and gates concurrent captures; includes an AppKit selection overlay (dimming, marching-ants, dimension tooltip, cancel shortcuts) plus localized strings and new unit tests covering pasteboard output, crop math, tiling, and size limits.
Reviewed by Cursor Bugbot for commit 0703b6d. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds browser screenshot actions: a toolbar camera for full‑page capture and context‑menu options for full page or a selected section. Copies PNG/TIFF to the clipboard with a flash and a brief “Copied” badge; clears the pasteboard first. Implements #4472.
New Features
Bug Fixes
Written for commit 0703b6d. Summary will update on new commits. Review in cubic
Summary by CodeRabbit