fix(layout): detect dense justified two-column layouts with narrow gutters - #423
fix(layout): detect dense justified two-column layouts with narrow gutters#423yzxcj797 wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/extractor/mod.rs">
<violation number="1" location="src/extractor/mod.rs:984">
P2: Every qualifying merge now emits raw layout diagnostics to stderr through `eprintln!`, including during library extraction. Remove these prints or replace them with the existing `debug!` logging so callers do not receive unsolicited output.</violation>
<violation number="2" location="src/extractor/mod.rs:984">
P2: `narrow_prose_column_boundaries` re-runs the full `detect_columns` pass (with per-page item cloning) for every page inside `merge_text_items`, duplicating column detection that `layout.rs` already performs for the same pages. This adds avoidable work to the hot merge path, including pages where a narrow gutter is impossible. Reuse the existing column-detection result or early-skip pages whose detected gutter is not in the 4–8pt range.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: instead of fixing issues one by one fix them all with cubic
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
| return items; | ||
| } | ||
|
|
||
| let narrow_column_boundaries = layout::narrow_prose_column_boundaries(&items); |
There was a problem hiding this comment.
P2: Every qualifying merge now emits raw layout diagnostics to stderr through eprintln!, including during library extraction. Remove these prints or replace them with the existing debug! logging so callers do not receive unsolicited output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/extractor/mod.rs, line 984:
<comment>Every qualifying merge now emits raw layout diagnostics to stderr through `eprintln!`, including during library extraction. Remove these prints or replace them with the existing `debug!` logging so callers do not receive unsolicited output.</comment>
<file context>
@@ -981,6 +981,8 @@ pub(crate) fn merge_text_items(items: Vec<TextItem>) -> Vec<TextItem> {
return items;
}
+ let narrow_column_boundaries = layout::narrow_prose_column_boundaries(&items);
+
// Group items by (page, Y position) with 5pt tolerance
</file context>
| return items; | ||
| } | ||
|
|
||
| let narrow_column_boundaries = layout::narrow_prose_column_boundaries(&items); |
There was a problem hiding this comment.
P2: narrow_prose_column_boundaries re-runs the full detect_columns pass (with per-page item cloning) for every page inside merge_text_items, duplicating column detection that layout.rs already performs for the same pages. This adds avoidable work to the hot merge path, including pages where a narrow gutter is impossible. Reuse the existing column-detection result or early-skip pages whose detected gutter is not in the 4–8pt range.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/extractor/mod.rs, line 984:
<comment>`narrow_prose_column_boundaries` re-runs the full `detect_columns` pass (with per-page item cloning) for every page inside `merge_text_items`, duplicating column detection that `layout.rs` already performs for the same pages. This adds avoidable work to the hot merge path, including pages where a narrow gutter is impossible. Reuse the existing column-detection result or early-skip pages whose detected gutter is not in the 4–8pt range.</comment>
<file context>
@@ -981,6 +981,8 @@ pub(crate) fn merge_text_items(items: Vec<TextItem>) -> Vec<TextItem> {
return items;
}
+ let narrow_column_boundaries = layout::narrow_prose_column_boundaries(&items);
+
// Group items by (page, Y position) with 5pt tolerance
</file context>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Fix all with cubic | Re-trigger cubic
…tters Dense justified prose can leave a gutter of 4-8pt - below the general 8pt minimum - and justified column edges leave histogram bins non-empty, so these pages fell through to single-column layout and downstream run-merging joined text across the gutter (firecrawl#395). The reduced 4pt valley threshold is only accepted with strong corroboration: at least 100 text items, no detected table, prose-shaped columns on both sides (validated for both center assignments), and a boundary that stays observable across most of the body's rows. Table pages and sparse layouts keep the conservative 8pt behavior. merge_text_items additionally refuses to join adjacent runs whose gap crosses an accepted narrow boundary, with an edge tolerance so justified columns cannot stitch back together. Regression coverage runs both directions: a 50-row justified two-column body with a 4.7pt gutter stays fully separated, while the same gap between short scattered table fragments is not promoted to a boundary.
Propagate table-page classification into merge-time boundary detection, skip impossible narrow-gutter candidates before replaying column detection, avoid cloning page items, and replace raw stderr diagnostics with redacted debug logs. The regression fixture now exercises the real narrow merge gap.
392cb9e to
397c74e
Compare
Summary
Dense justified two-column prose can leave a gutter of only 4–8pt — below the general 8pt minimum — and justified column edges leave the histogram bins non-empty, so these pages fell through to single-column layout, and downstream run-merging then joined text across the gutter (#395).
Two coordinated changes:
detect_columns(src/extractor/layout.rs): the valley width floor drops to 4pt only with strong corroboration — ≥100 text items, no detected table on the page, prose-shaped columns on both sides (validated under both center-assignment interpretations), and a boundary that remains observable across most of the body's rows (narrow_boundary_has_body_support: ≥20 items per side, ≥60% vertical overlap, wide-title exclusion). Table pages and sparse layouts keep the pre-existing conservative 8pt behavior unchanged.merge_text_items(src/extractor/mod.rs): adjacent runs whose gap crosses an accepted narrow boundary (±0.25pt edge tolerance) are no longer joined, so justified columns cannot stitch back together after detection.Testing
merge_does_not_join_dense_columns_across_a_narrow_gutter: a 50-row justified two-column body at 9.5pt with a 4.7pt gutter (below the old 4.75pt merge cutoff) stays fully separated — 100 items in, 100 items out.merge_keeps_narrow_table_fragments_together: the same 4.7pt gap between short, scattered table fragments is not promoted to a boundary — the conservative path is preserved.cargo test --lib— 958/958 pass (includes both new regressions)cargo test --test integration_tests— 162/162 passcargo fmt --checkclean;cargo clippy --lib --tests— no new diagnostics (the onefont_changeswarning predates this branch)Fixes #395
Summary by cubic
Detects dense justified two‑column pages with 4–8pt gutters and prevents merge‑time stitching across that gutter. Previously we required an ≥8pt empty valley, so dense pages were treated as single column and runs merged across the gutter.
page_has_data_tableuses structural rects/lines to flag a page only when a data table covers ≥50% of items; mixed pages with small tables keep narrow‑prose boundaries. No body‑font heuristic runs at this stage.narrow_prose_column_boundariesblock joins across accepted narrow gutters (±0.25pt). We propagate table‑page state viamerge_text_items_with_table_pagesand skip replaying column detection on non‑candidate pages.src/extractor/layout.rs: narrow‑gutter validation (narrow_valleys_have_body_support,narrow_boundary_has_body_support), candidate/lookup helpers (has_narrow_prose_candidate,narrow_prose_column_boundaries).src/extractor/mod.rs: merge guard viagap_crosses_narrow_column_boundary; newmerge_text_items_with_table_pages.src/extractor/content_stream.rs: structural table preflight (page_has_data_table) and pass‑through tomerge_text_items_with_table_pages.Written for commit 397c74e. Summary will update on new commits.