feat(crop): content-trim crop stage (smart-crop phase 1) - #2
Conversation
Trim each served page to its content bounds before framing. A new internal/crop package provides a normalized Box + Detector seam and the Phase 1 content-trim detector: a bounding-box-of-ink trim that also steps over top-bleed printer's marks (registration / CMYK bars, plate-ident codes) under strict guards, so it can never cut real content. Wire it into the engine serve path: resolve a crop plan (a stored crop_overrides box wins, else the auto detector), apply it to the decoded master before compose, and fold the crop identity into the ETag so a re-crop invalidates caches. On by default; BROADSHEET_CROP=off serves full pages. The applied box is echoed in the X-Broadsheet-Crop header. The crop's top edge is the pluggable axis for a future masthead/skybox detector; content-trim owns the sides, bottom, and a safe fallback top. It does not remove ad/promo skyboxes yet. Verified end to end on a real NYT edition (registration marks removed) and against the eval-corpus A/B harness (12 fixes, 0 regressions). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013G3Wpmfq9mxWMc5Lsfjdvo
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds default-on content-aware cropping before page framing, with detector heuristics, stored overrides, cache invalidation, configuration controls, response headers, tests, and documentation. ChangesContent-aware page cropping
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant broadsheet-server
participant broadsheet.Engine
participant Store
participant crop.ContentTrim
Client->>broadsheet-server: request page
broadsheet-server->>broadsheet.Engine: render page
broadsheet.Engine->>Store: load crop override
broadsheet.Engine->>crop.ContentTrim: detect crop if needed
broadsheet.Engine->>broadsheet.Engine: apply crop before framing and compute ETag
broadsheet.Engine-->>broadsheet-server: rendered result and crop box
broadsheet-server-->>Client: image with ETag and X-Broadsheet-Crop
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Pull request overview
Adds a new “smart crop” stage (phase 1: content-trim) that trims served pages to content bounds (including conservative skipping of top-bleed printer’s marks), wires it into the serve/render path with cache-aware ETag identity, and exposes configuration + headers/docs for operators.
Changes:
- Introduce
internal/cropwith normalizedBox/Detectorseam andContentTrimdetector (bounding-box-of-ink with top-bleed skip). - Apply crop (override-first, else auto) to decoded masters before framing; fold crop identity into ETag and expose applied crop via
X-Broadsheet-Crop. - Add
BROADSHEET_CROP/DisableCropconfiguration plus tests and documentation updates.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents BROADSHEET_CROP configuration. |
| pkg/broadsheet/broadsheet.go | Wires crop planning/detection into serve path, adds DisableCrop, includes crop token in ETag, returns applied crop in results. |
| pkg/broadsheet/broadsheet_test.go | Adds an end-to-end serve-path test verifying crop on/off behavior and ETag differences. |
| internal/store/store.go | Adds CropOverride model + GetCropOverride lookup used by the engine. |
| internal/crop/trim.go | Implements the ContentTrim detector. |
| internal/crop/trim_test.go | Adds unit tests for trimming and top-bleed strip skipping/guard conditions. |
| internal/crop/crop.go | Adds core crop types (Box, Detector, Hints) and application logic. |
| docs/architecture.md | Documents cropping architecture, behavior, headers, and package layout. |
| cmd/broadsheet-server/main.go | Adds BROADSHEET_CROP env handling and emits X-Broadsheet-Crop header. |
| CHANGELOG.md | Notes new default cropping behavior and operator controls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/broadsheet/broadsheet.go (1)
746-797: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winCrop resolution (DB lookup + auto-detect) is opts-independent but recomputed per opts-specific variant, undercutting the variant cache's purpose.
resolveCropruns before the variant-cache lookup on every request, sop.store.GetCropOverrideis hit on every serve() call whenever crop is enabled (the default) — even for what would otherwise be a fully-cached thumbnail. The comment at Lines 752-754 explicitly frames the variant cache as avoiding "re-decoding and re-cropping the master per image" for thumbnail-heavy pages, but the DB round-trip (and, on cache misses, the O(w·h)cropper.Detectscan at Lines 787-791) is keyed only by the full opts-specific ETag, not by (source, edition, render) — so a grid page requesting many distinct thumbnail sizes of the same edition pays the DB query and, on first load of each size, a full redundant detection scan, for content that's identical across all those requests.Consider resolving/caching the crop plan (DB lookup result and/or detected box) once per (sourceID, render mtime) — independent of
opts— e.g. a small TTL/invalidated cache alongsidep.variants, so repeated requests for the same source/edition don't re-hit the store or re-scan the master.🤖 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 `@pkg/broadsheet/broadsheet.go` around lines 746 - 797, Decouple crop-plan resolution from opts-specific variant caching in the serve flow around resolveCrop and the cropper.Detect call. Cache the stored override and auto-detected crop box by source identity and render version (such as SourceID plus render mtime), with appropriate TTL or invalidation, so repeated thumbnail variants reuse the same plan without repeated store lookups or detection scans while preserving override precedence and no-op behavior.
🧹 Nitpick comments (1)
internal/crop/crop.go (1)
54-83: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider a minimum-size floor for sanity, not just positivity.
A box like
{X:0.999, Y:0, W:0.001, H:1}passesW<=0||H<=0and clamps to itself unchanged — it's a "valid" rectangle but crops away virtually the whole page, which is presumably never an intended outcome for any real override.Clampcurrently only guards against degenerate/out-of-bounds boxes, not against nonsensical-but-valid tiny ones.
[optional_nitpick]🤖 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 `@internal/crop/crop.go` around lines 54 - 83, Update Box.Clamp to enforce a defined minimum width and height after all boundary adjustments, rejecting boxes smaller than that floor by returning Full(). Apply the floor consistently to the initial validation and final clamped dimensions, while preserving existing handling for out-of-bounds and non-positive boxes.
🤖 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 `@README.md`:
- Line 153: Update the README’s “Not done yet” section to remove the stale claim
that smart cropping is unwritten and pages are served whole, keeping the
documented BROADSHEET_CROP behavior consistent with the current implementation.
---
Outside diff comments:
In `@pkg/broadsheet/broadsheet.go`:
- Around line 746-797: Decouple crop-plan resolution from opts-specific variant
caching in the serve flow around resolveCrop and the cropper.Detect call. Cache
the stored override and auto-detected crop box by source identity and render
version (such as SourceID plus render mtime), with appropriate TTL or
invalidation, so repeated thumbnail variants reuse the same plan without
repeated store lookups or detection scans while preserving override precedence
and no-op behavior.
---
Nitpick comments:
In `@internal/crop/crop.go`:
- Around line 54-83: Update Box.Clamp to enforce a defined minimum width and
height after all boundary adjustments, rejecting boxes smaller than that floor
by returning Full(). Apply the floor consistently to the initial validation and
final clamped dimensions, while preserving existing handling for out-of-bounds
and non-positive boxes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9dc40149-c4f5-4daf-b21b-84dee63c4179
📒 Files selected for processing (10)
CHANGELOG.mdREADME.mdcmd/broadsheet-server/main.godocs/architecture.mdinternal/crop/crop.gointernal/crop/trim.gointernal/crop/trim_test.gointernal/store/store.gopkg/broadsheet/broadsheet.gopkg/broadsheet/broadsheet_test.go
- crop.Box.Clamp: reject non-finite (NaN/Inf) coordinates up front, so a corrupted override can't slip past the bounds checks (Copilot). - ContentTrim.Detect doc: it does error on ctx cancellation; say so (Copilot). - trim tests: correct the "four guards" wording (three here, one separate) and tighten the tall-page scale assertion to near(185) instead of a loose bound (Copilot). - README "Not done yet": pages are now content-trimmed; the open item is skybox removal, not cropping wholesale (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013G3Wpmfq9mxWMc5Lsfjdvo
A tiny-but-positive override (e.g. W=0.001) slipped past Clamp's <=0 checks and Apply would emit a 1px sliver crop. Enforce a minSpan (2%) floor on both the initial and final dimension checks, collapsing anything smaller to Full() — same fail-safe-to-uncropped philosophy as the non-finite guard. Adds a Clamp guard test (NaN/Inf/below-floor/negative/legit). Addresses CodeRabbit nitpick on internal/crop/crop.go. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013G3Wpmfq9mxWMc5Lsfjdvo
Smart crop, phase 1: content-trim
Trims each served page to its content bounds before framing — the safe first rung of the smart-crop track. Established as the shippable floor by a field evaluation of 7 crop approaches against a hand-verified corpus (content-trim was the only approach with 0% over-crops — it can never cut real content).
What it does
internal/croppackage: a normalizedBox+Detectorseam and the content-trim detector — bounding-box-of-ink trim.C M Y K … Nxxx,…,Bs-4C,E1) — but only a band that's provably junk: thin, faint, in the extreme-top bleed, and separated from the body by a clear whitespace gap. Every guard is conservative enough that real content is never cut.crop_overridesbox wins, else auto) → apply to the decoded master beforecompose→ fold the crop identity into the ETag so a re-crop invalidates caches.BROADSHEET_CROP=offserves full pages. Applied box echoed inX-Broadsheet-Crop.What it deliberately does NOT do
Remove ad/promo skyboxes above the masthead — that's a semantic call needing a text-layer or learned detector (a later phase). The seam is built for it: the crop's top edge is the pluggable axis; content-trim owns the sides, bottom, and a safe fallback top.
Behavior change
Existing deployments start serving cropped pages on upgrade. Safe (whitespace and printer's-marks only);
BROADSHEET_CROP=offrestores full pages.Verification
pa-pndateline near-miss before shipping).ny-nyt/20260702through the server —X-Broadsheet-Crop: 0.02,0.04,0.96,0.92, registration marks gone; crop-off unchanged.go build/vet/test ./...andgolangci-lintall clean.Docs
Architecture (new Cropping section + config/headers/package-layout), README config table, and CHANGELOG (with the behavior-change note).
🤖 Generated with Claude Code