Repository navigation
Fix Retina scaling in design mode annotation crops - #8831
Conversation
|
@codex review |
|
✅ Action performedReview finished.
|
|
To use Codex here, create a Codex account and connect to github. |
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 8 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
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:
📝 WalkthroughWalkthroughChangesScreenshot cropping now uses explicit bitmap rendering and includes deterministic pixel-dimension coverage. ABI layout assertions and mobile activation logger capture are also updated. Screenshot Crop Rendering
Compatibility and Runtime Updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ 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 |
|
To use Codex here, create a Codex account and connect to github. |
Greptile SummaryThis PR fixes a Retina-display scaling bug in design-mode annotation crops.
Confidence Score: 5/5Safe to merge — the crop now writes into an explicit 1:1 pixel bitmap, the @mainactor annotation is in place, and all three previously flagged issues (actor isolation, color space, duplicate interpolation hint) have been addressed. The production change is narrow and well-understood: one function replaced a display-scale-dependent lockFocus path with an explicit NSBitmapImageRep whose pixel dimensions are directly derived from the selection coordinates. The @mainactor annotation now matches the function's actual thread requirements. The JS side consistently drops the imageURL field across runtime, call site, and test. The regression test is deterministic across display scales. No new actor isolation gaps, blocking primitives, global state, or test seams in production sources were introduced. No files require special attention. cmuxTests/BrowserScreenshotCropTests.swift performs a process-wide NSImage method swizzle; the @suite(.serialized) + thread-dictionary forwarding makes this safe, but reviewers should be aware of the mechanism if other tests begin using NSImage.lockFocus. Important Files Changed
Sequence DiagramsequenceDiagram
participant UA as User (Annotation Draw)
participant BDM as BrowserDesignModeController
participant Pipeline as BrowserScreenshotCrop
participant Store as screenshotStore
participant JS as BrowserDesignModeRuntime.js
UA->>BDM: receiveAnnotationCaptureRequestData
BDM->>BDM: "captureAnnotation (Task @MainActor)"
BDM->>BDM: captureStableAnnotation → NSImage
BDM->>Pipeline: "croppedImage(@MainActor) NSBitmapImageRep(pixelsWide, pixelsHigh) → 1:1 pixel crop"
Pipeline-->>BDM: NSImage (correct pixel dimensions)
BDM->>Store: save(pngData) → screenshotURL
BDM->>JS: completeAnnotationCapture(id, x, y, w, h, scrollX, scrollY, vpW, vpH)
Note over JS: No imageURL — renders transparent dashed outline only
JS-->>BDM: snapshot
BDM->>BDM: "apply(snapshot), phase = .captured"
Reviews (12): Last reviewed commit: "test(browser): verify cropped annotation..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a Retina display bug in
Confidence Score: 4/5The Retina scaling fix is sound — pinning the bitmap to logical pixel dimensions is the correct approach — but The core logic is correct: the explicit Sources/Panels/BrowserScreenshotPipeline.swift — the missing Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["captureAndWrite(@MainActor)"] --> B["snapshot() → NSImage"]
B --> C["BrowserScreenshotCrop.croppedImage\n(nonisolated static)"]
C --> D["imageRect(forSelectionInView:)\n→ cropRect (integral)"]
D --> E["NSBitmapImageRep\npixelsWide: cropRect.width\npixelsHigh: cropRect.height"]
E --> F["NSGraphicsContext(bitmapImageRep:)"]
F --> G["saveGraphicsState()\nNSGraphicsContext.current = context"]
G --> H["image.draw(in: 0,0,w,h from: cropRect)"]
H --> I["restoreGraphicsState()"]
I --> J["NSImage(size: cropRect.size)\n.addRepresentation(bitmap)"]
J --> K["BrowserScreenshotPasteboardWriter\n.pngData / .write"]
K --> L["tiffRepresentation → PNG"]
L --> M["NSPasteboard / Data output"]
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/BrowserScreenshotPipeline.swift`:
- Around line 130-133: The crop pipeline must use the backing bitmap’s pixel
dimensions rather than NSImage.size for both imageRect(...) crop calculations
and NSBitmapImageRep allocation. Update the relevant imageRect and bitmap
creation flow to derive width and height from the selected representation or
CGImage, preserving consistent pixel-to-coordinate mapping, and add a regression
test where logical image size differs from backing pixel dimensions.
🪄 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 Plus
Run ID: d46e31e2-bf1b-4ee9-a35c-920e6ac56cdd
📒 Files selected for processing (3)
Sources/Panels/BrowserScreenshotPipeline.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserScreenshotCropTests.swift
Greptile SummaryThis PR fixes a Retina-display bug in
Confidence Score: 4/5The production change is a targeted, well-contained fix to the AppKit drawing path; the only outstanding concern is that the new regression test does not distinguish the old and new code paths when CI runs on a 1× display. The core change correctly replaces a display-scale-dependent drawing path with an explicit bitmap of known pixel dimensions, and the graphics-state save/restore is properly balanced. The test assertions check physical pixel counts, but the test would pass against both old and new code on a non-Retina host because the source image is created at 1:1 pixel density. BrowserScreenshotCropTests.swift — the test design should be reviewed to ensure it catches the Retina scaling regression on 1× CI machines. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as BrowserScreenshotPipeline
participant Crop as BrowserScreenshotCrop
participant Bitmap as NSBitmapImageRep
participant Ctx as NSGraphicsContext
participant Writer as BrowserScreenshotPasteboardWriter
Caller->>Crop: croppedImage(from:selectionInView:viewBounds:)
Crop->>Crop: imageRect() → cropRect (display-scale-independent)
Crop->>Bitmap: init(pixelsWide: cropRect.width, pixelsHigh: cropRect.height)
Crop->>Ctx: NSGraphicsContext(bitmapImageRep:)
Crop->>Ctx: saveGraphicsState()
Crop->>Ctx: "current = context"
Crop->>Bitmap: image.draw(in:from:cropRect operation:.copy)
Crop->>Ctx: restoreGraphicsState()
Crop->>Crop: NSImage(size:).addRepresentation(bitmap)
Crop-->>Caller: NSImage (explicit pixel-locked bitmap)
Caller->>Writer: write(image, to: pasteboard)
Writer->>Writer: tiffRepresentation → pngData
Writer-->>Caller: done
Reviews (3): Last reviewed commit: "fix(browser): crop screenshots at native..." | Re-trigger Greptile |
d6c89ed to
6f89dfb
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
@codex review |
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReview finished.
|
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 8 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
To use Codex here, create a Codex account and connect to github. |
1 similar comment
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/Panels/BrowserScreenshotPipeline.swift (1)
121-143: 🎯 Functional Correctness | 🟠 MajorCrop math still keyed off
image.size, not backing pixel dimensions — same root cause flagged previously.
croppedImagecomputescropRectviaimageRect(..., imageSize: image.size)and then allocates the new bitmap withpixelsWide: Int(cropRect.width)/pixelsHigh: Int(cropRect.height).NSImage.sizeis a logical point size, independent of the backing representation'spixelsWide/pixelsHigh; if the sourceimage's.sizedoesn't equal its native pixel dimensions (e.g. any capture path that sets a logical size different from the bitmap's pixel count), the pixel math inimageRect— and therefore the exact size of the bitmap this diff allocates — is wrong, even though the drawing itself is now Retina-independent. This is the same concern raised on the prior commit: crop box derivation should use the source representation's pixel dimensions, notNSImage.size, as the single source of truth.The new regression test in
BrowserScreenshotCropTests.swiftdoesn't cover this:makeBitmapImageexplicitly setsbitmap.sizeequal topixelsWide/pixelsHigh, soimage.size == pixel sizein the test and the mismatch case is never exercised.Suggested direction (outside this hunk, in
imageRect/croppedImage): derive pixel dimensions from the source bitmap representation instead ofimage.size.private static func pixelSize(of image: NSImage) -> NSSize? { guard let rep = image.representations.compactMap({ $0 as? NSBitmapImageRep }).first else { return nil } return NSSize(width: rep.pixelsWide, height: rep.pixelsHigh) }Then fail closed (throw
invalidImageRepresentation) when no bitmap representation is available, rather than falling back to the logical.size.Based on path instructions ("If any PR logic derives crop/scale correctness from view/string heuristics, refactor to use explicit structured inputs ... and fail/disable when the authoritative signal is missing rather than guessing.") from
.github/review-bot-rules/reliability-single-source-of-truth.md, and on the prior review comment on this same code path (lines 130-133 of the previous commit) raising the identicalimage.sizevs. pixel-dimension concern.🤖 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 121 - 143, Update croppedImage and imageRect to derive crop dimensions from the source NSBitmapImageRep pixel dimensions rather than image.size. Reuse a pixel-size helper or equivalent representation lookup, and throw BrowserScreenshotError.invalidImageRepresentation when no bitmap representation exists; preserve the existing invalidSelection validation and bitmap allocation flow.Source: Path instructions
🤖 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/BrowserScreenshotCropTests.swift`:
- Around line 32-103: Update the test using withImageFocusBackingScale so it
exercises a code path that actually invokes NSImage.lockFocus and unlockFocus,
or remove the helper and add a focused regression scenario that reproduces the
pixel/point mismatch. Ensure encodedCropUsesOnePixelPerSnapshotCoordinate
validates the intended Retina backing-scale behavior rather than only explicit
NSBitmapImageRep crop dimensions.
---
Duplicate comments:
In `@Sources/Panels/BrowserScreenshotPipeline.swift`:
- Around line 121-143: Update croppedImage and imageRect to derive crop
dimensions from the source NSBitmapImageRep pixel dimensions rather than
image.size. Reuse a pixel-size helper or equivalent representation lookup, and
throw BrowserScreenshotError.invalidImageRepresentation when no bitmap
representation exists; preserve the existing invalidSelection validation and
bitmap allocation flow.
🪄 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 Plus
Run ID: 319799cd-3df0-449c-9b7d-3d5bcfd7de84
📒 Files selected for processing (3)
Sources/Panels/BrowserScreenshotPipeline.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserScreenshotCropTests.swift
6f89dfb to
b788370
Compare
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/BrowserScreenshotPipeline.swift`:
- Around line 131-161: Strengthen the regression test for the crop flow around
NSBitmapImageRep and PNG encoding by using a fixture with distinct row and
column colors, then decode the encoded PNG and assert representative pixel
colors from the expected crop region. Keep the existing dimension checks, and
ensure the assertions detect blank, vertically flipped, or incorrectly cropped
output.
🪄 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 Plus
Run ID: 0f3e5151-5b35-47a3-a663-24b807afa8f3
📒 Files selected for processing (3)
Sources/Panels/BrowserScreenshotPipeline.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserScreenshotCropTests.swift
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
@coderabbitai review |
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 8 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
✅ Action performedReview finished.
|
1 participant
Summary
NSImage.sizeRoot cause
BrowserScreenshotCrop.croppedImageusedNSImage.lockFocus(), which creates its backing representation at the current display scale. On Retina, a 200×100 crop therefore encoded as 400×200 pixels and looked magnified at 1:1 inspection. The viewport-to-view and view-to-image rectangle transforms were already correct.Testing
swiftc -typecheck Sources/Panels/BrowserScreenshotPipeline.swift./scripts/check-pbxproj.sh./scripts/lint-pbxproj-test-wiring.shgit diff --checkxcodebuildor UI tests, per issue instructions; CI is the test-target gateThe requested Swift file-length budget command was attempted, but current
mainno longer containsscripts/swift_file_length_budget.pyor.github/swift-file-length-budget.tsvafter the budget was removed in #8125. Both touched Swift files remain below 500 lines, and no warning-budget file changed.Localization
No user-facing strings or localized surfaces changed.
Demo Video
Deferred until required CI is green and explicit approval is given for the requested cloud reload/build command.
Closes #8827
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fix Retina scaling in design‑mode annotation crops by rendering selections into a 1:1
NSBitmapImageRep, so exported PNGs match the selected pixels. During capture, annotations stay over the live page with a transparent outline only; fixes #8827.@MainActor.imageURLfromcompleteAnnotationCapture, switch runtime to transparent outlines and hover styling, and update webview tests.GhosttySurfaceConfigABITestsfor new fields and remove a redundant await in mobile diagnostic logging.Written for commit bd964ce. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests