Skip to content

fix(extractor): clip sparse off-page text - #437

Open
yzxcj797 wants to merge 1 commit into
firecrawl:mainfrom
yzxcj797:fix/318-visible-page-clipping
Open

fix(extractor): clip sparse off-page text#437
yzxcj797 wants to merge 1 commit into
firecrawl:mainfrom
yzxcj797:fix/318-visible-page-clipping

Conversation

@yzxcj797

@yzxcj797 yzxcj797 commented Aug 20, 2026

Copy link
Copy Markdown

Summary

  • Fixes Off-page text survives CropBox clipping on sparse and rotated pages #318.
  • Removes the ten-item minimum from visible-page clipping so a sparse, coherent off-page phrase no longer survives a nonzero CropBox.
  • Keeps clipping in PDF user-space coordinates for /Rotate 0/90/180/270.
  • Transforms the clipping box alongside already-normalized embedded rotated coordinates.
  • Retains short off-page glyph fragments and text that continues a visible line.

Testing

  • The new sparse CropBox regression failed on pristine main (HIDDEN-OFF-PAGE survived) and passes after the change.
  • Added coverage for /Rotate 0/90/180/270, normalized embedded rotated text, line continuations, and short fragments.
  • cargo test — 987 lib, 3 CLI, 166 integration, and 2 doc tests passed.
  • cargo clippy -- -D warnings passed.
  • cargo fmt --all -- --check passed.
  • git diff --check passed.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 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:343">
P2: When a text run continues into the visible page from the left edge, `straddles` does not recognize it because it only checks right-side adjacency. Check adjacency in both directions before clipping, or this drops left-edge continuations despite the retention rule.</violation>

<violation number="2" location="src/extractor/mod.rs:346">
P2: Removing the `off.len() >= 10` floor and the `!coords_rotated` guard makes clipping much more aggressive for rotated pages while the surrounding comment still claims rotated display text "must stay." The `wordy_chars * 2 >= total_chars.max(1)` ratio treats a single off-box item of 4+ characters as coherent, so a lone rotated or curved display label/word lying outside the CropBox (e.g. a vertical side label) is now silently dropped, where previously rotated pages were never clipped and needed at least 10 off-page items even for non-rotated pages. The `straddles` guard only protects items that continue a visible on-page line, so it does not cover a standalone wordy fragment. If a sparse coherent phrase is the goal, a minimal item-length floor (2+) would still clip the regression cases while preserving single legitimate fragments.</violation>
</file>

<file name="tests/integration_tests.rs">

<violation number="1" location="tests/integration_tests.rs:600">
P3: The `a` and `b` assertions don't actually verify the short fragments survive, because the retained words already contain those letters: "Continuation" has an `a` and "Visible" has a `b`. The joined string would still match these substrings if the `a`/`b` fragments were dropped, silently masking a partial regression. Only the `c` assertion is meaningful. Assert against the item list (e.g. check each fragment exists as its own item) or a more specific substring so each fragment is genuinely validated.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.

Fix all with cubic | Re-trigger cubic

Comment thread src/extractor/mod.rs
// artifacts of transforms we mis-model — don't clip those.
let straddles = off.iter().any(|o| {
items.iter().any(|i| {
!outside(i) && (i.y - o.y).abs() <= 2.0 && (o.x - (i.x + i.width)).abs() <= 10.0

@cubic-dev-ai cubic-dev-ai Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: When a text run continues into the visible page from the left edge, straddles does not recognize it because it only checks right-side adjacency. Check adjacency in both directions before clipping, or this drops left-edge continuations despite the retention rule.

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 343:

<comment>When a text run continues into the visible page from the left edge, `straddles` does not recognize it because it only checks right-side adjacency. Check adjacency in both directions before clipping, or this drops left-edge continuations despite the retention rule.</comment>

<file context>
@@ -303,74 +303,80 @@ fn extract_positioned_text_impl(
+            // artifacts of transforms we mis-model — don't clip those.
+            let straddles = off.iter().any(|o| {
+                items.iter().any(|i| {
+                    !outside(i) && (i.y - o.y).abs() <= 2.0 && (o.x - (i.x + i.width)).abs() <= 10.0
+                })
+            });
</file context>
Suggested change
!outside(i) && (i.y - o.y).abs() <= 2.0 && (o.x - (i.x + i.width)).abs() <= 10.0
!outside(i)
&& (i.y - o.y).abs() <= 2.0
&& ((o.x - (i.x + i.width)).abs() <= 10.0
|| (i.x - (o.x + o.width)).abs() <= 10.0)
Fix with cubic
Comment thread src/extractor/mod.rs
!outside(i) && (i.y - o.y).abs() <= 2.0 && (o.x - (i.x + i.width)).abs() <= 10.0
})
});
let coherent = wordy_chars * 2 >= total_chars.max(1) && !straddles;

@cubic-dev-ai cubic-dev-ai Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Removing the off.len() >= 10 floor and the !coords_rotated guard makes clipping much more aggressive for rotated pages while the surrounding comment still claims rotated display text "must stay." The wordy_chars * 2 >= total_chars.max(1) ratio treats a single off-box item of 4+ characters as coherent, so a lone rotated or curved display label/word lying outside the CropBox (e.g. a vertical side label) is now silently dropped, where previously rotated pages were never clipped and needed at least 10 off-page items even for non-rotated pages. The straddles guard only protects items that continue a visible on-page line, so it does not cover a standalone wordy fragment. If a sparse coherent phrase is the goal, a minimal item-length floor (2+) would still clip the regression cases while preserving single legitimate fragments.

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 346:

<comment>Removing the `off.len() >= 10` floor and the `!coords_rotated` guard makes clipping much more aggressive for rotated pages while the surrounding comment still claims rotated display text "must stay." The `wordy_chars * 2 >= total_chars.max(1)` ratio treats a single off-box item of 4+ characters as coherent, so a lone rotated or curved display label/word lying outside the CropBox (e.g. a vertical side label) is now silently dropped, where previously rotated pages were never clipped and needed at least 10 off-page items even for non-rotated pages. The `straddles` guard only protects items that continue a visible on-page line, so it does not cover a standalone wordy fragment. If a sparse coherent phrase is the goal, a minimal item-length floor (2+) would still clip the regression cases while preserving single legitimate fragments.</comment>

<file context>
@@ -303,74 +303,80 @@ fn extract_positioned_text_impl(
+                    !outside(i) && (i.y - o.y).abs() <= 2.0 && (o.x - (i.x + i.width)).abs() <= 10.0
+                })
+            });
+            let coherent = wordy_chars * 2 >= total_chars.max(1) && !straddles;
+            if bx1 - bx0 >= 72.0 && by1 - by0 >= 72.0 && coherent {
+                let before = items.len();
</file context>
Fix with cubic

assert!(text.contains("Visible"));
assert!(text.contains("Continuation"));
assert!(text.contains("a"));

@cubic-dev-ai cubic-dev-ai Bot Aug 20, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: The a and b assertions don't actually verify the short fragments survive, because the retained words already contain those letters: "Continuation" has an a and "Visible" has a b. The joined string would still match these substrings if the a/b fragments were dropped, silently masking a partial regression. Only the c assertion is meaningful. Assert against the item list (e.g. check each fragment exists as its own item) or a more specific substring so each fragment is genuinely validated.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/integration_tests.rs, line 600:

<comment>The `a` and `b` assertions don't actually verify the short fragments survive, because the retained words already contain those letters: "Continuation" has an `a` and "Visible" has a `b`. The joined string would still match these substrings if the `a`/`b` fragments were dropped, silently masking a partial regression. Only the `c` assertion is meaningful. Assert against the item list (e.g. check each fragment exists as its own item) or a more specific substring so each fragment is genuinely validated.</comment>

<file context>
@@ -527,6 +529,79 @@ fn test_digit_only_text_runs_are_preserved_in_markdown() {
+
+    assert!(text.contains("Visible"));
+    assert!(text.contains("Continuation"));
+    assert!(text.contains("a"));
+    assert!(text.contains("b"));
+    assert!(text.contains("c"));
</file context>
Fix with cubic
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

1 participant