Skip to content

fix(sheet): preserve merged-cell spans that extend past the used range - #16

Open
mathurshubham wants to merge 2 commits into
firecrawl:mainfrom
mathurshubham:fix/preserve-merge-spans-beyond-used-range
Open

fix(sheet): preserve merged-cell spans that extend past the used range#16
mathurshubham wants to merge 2 commits into
firecrawl:mainfrom
mathurshubham:fix/preserve-merge-spans-beyond-used-range

Conversation

@mathurshubham

@mathurshubham mathurshubham commented Aug 5, 2026

Copy link
Copy Markdown

Fixes #8.

What

A merge anchored on populated cells but reaching into empty rows or columns
was clipped to the used range, so a valid source merge (F1:O3 with only
F1 populated) silently collapsed to an ordinary 1×1 cell in
to_document().

The collapse happened in two layers, so both change:

  1. Sheet frontend (src/formats/sheet/mod.rs) intersected each merged
    region with calamine's used range before building the grid — the clip the
    issue reports.
  2. GridBuilder::finish (src/model/table.rs) dropped trailing rows
    holding only covered positions ("all-empty" treated covered slots as
    empty) and then re-clamped the surviving spans — so even an unclipped
    span would have shrunk back.

The fix

  • The sheet frontend keeps a region's full extent. The two existing
    defenses are preserved unchanged: the clamped top-left still decides the
    origin, and regions that don't intersect the used range at all are still
    ignored (the merge_outside_the_used_columns_is_ignored test is
    untouched). Rows below the used range are appended so the overhang has
    positions to materialize into.
  • finish()'s trailing trim now exempts span-claimed rows: covered
    positions are structure a producer declared, and dropping them silently
    shrinks a valid span. Rows of plain empty cells still trim (new test pins
    that).

Runaway regions stay bounded — a region whose full extent would blow the
expansion budget (a crafted A1:XFD1048576 merge, or a real whole-column
merge) degrades to today's clipped form instead of materializing: the file
still converts, it just loses the overhang (new test proves it).

Behavior

The reporter's exact openpyxl repro, before → after:

-1 1
-origin 1 1
+3 10
+origin 3 10

The rendered Markdown is unchanged — the renderer already trims the
trailing empty area visually, so to_markdown still emits
| Merged heading |. The fix is observable through to_document(), which
is the surface the issue is about. Consequently no existing snapshot
changed
.

Tests

  • Sheet: full-extent span over a 2×2 used range (5×5 merge), the issue's
    1×1-anchor shape (3×10 span, 29 covered), the existing out-of-range-merge
    defense unchanged, and the whole-sheet crafted merge degrading to the
    clipped form instead of erroring
    (oversized_merge_degrades_to_the_clipped_form).
  • Model: trailing span-claimed rows survive finish() (replaces the test
    that asserted the old clamping behavior); trailing empty-origin rows still
    trim.
  • Integration: new handmade-overhang.xlsx fixture + snapshot, and a
    model-retention test asserting the 3×10 grid and spans via
    to_document().
  • cargo fmt --check, clippy --workspace --all-targets --all-features -D warnings, and cargo test --locked all pass; only the new snapshot
    was added, nothing else moved.

Note

This branch is rebased on current main and composes with the new
resolve_header_rows detection (fe97550) — header resolution runs on the
finished grid, including the preserved overhang rows.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@mathurshubham

Copy link
Copy Markdown
Author

Closing in favor of #13, which was opened first and takes the same two-layer approach (sheet-side extent + the finish() trailing-trim) — I hadn't seen it when I opened this.

One deliberate difference worth carrying over from this PR if useful: an integration fixture (handmade-overhang.xlsx) plus a to_document() retention test asserting the 3×10 grid and spans — happy to contribute those to #13 as a follow-up if wanted. Verified #13 against the issue's openpyxl repro locally; it resolves it.

@mathurshubham

Copy link
Copy Markdown
Author

Reopening: #13, which this was closed in favor of, was closed when its author's account was deleted, so #8 is unaddressed again.

Since then I've folded in the one behavior where #13 had the better call — an oversized region (a crafted whole-sheet merge, or a real whole-column merge) now degrades to today's clipped form instead of failing the conversion with ResourceLimit (c19fbb0).

Branch is rebased on current main and the full suite passes.

A merge anchored on populated cells but reaching into empty rows or
columns was intersected with the used range, so a valid source merge
(F1:O3 with only F1 populated) silently collapsed to an ordinary 1x1
cell. Two layers clipped it:

- the sheet frontend clamped the region's end to the used range;
- GridBuilder::finish dropped trailing rows holding only covered
  positions and re-clamped the surviving spans.

Keep the region's full extent (the clamped top-left still decides the
origin, and regions that do not intersect the used range stay ignored),
give the overhanging rows a place to materialize, and exempt span-
claimed rows from the trailing trim - covered positions are structure a
producer declared. Runaway regions stay bounded: place() charges the
whole span against MAX_EXPANSION before any expansion, so a crafted
whole-sheet merge fails as a resource limit.

The rendered Markdown is unchanged (the renderer trims the empty tail
visually); the fix is observable through to_document().

Fixes firecrawl#8
A region whose full extent would blow the expansion budget (a crafted
whole-sheet merge, or a real whole-column merge) previously failed the
conversion with ResourceLimit. Files that converted before this branch
must keep converting: such a region now degrades to the clipped form -
the populated rectangle - and only loses the overhang.
@mathurshubham
mathurshubham force-pushed the fix/preserve-merge-spans-beyond-used-range branch from c19fbb0 to 067b07d Compare August 6, 2026 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XLSX merged-cell spans are clipped to the populated range

2 participants