Skip to content

fix(xlsx): retain embedded worksheet images - #151

Open
Fanfulla wants to merge 1 commit into
firecrawl:mainfrom
Fanfulla:fix/xlsx-embedded-assets
Open

fix(xlsx): retain embedded worksheet images#151
Fanfulla wants to merge 1 commit into
firecrawl:mainfrom
Fanfulla:fix/xlsx-embedded-assets

Conversation

@Fanfulla

@Fanfulla Fanfulla commented Aug 31, 2026

Copy link
Copy Markdown

This fixes the XLSX side of #139.

The parser now follows the worksheet -> drawing -> image relationship chain and stores embedded image bytes in Document.assets, using the existing AssetSink behavior for byte limits and deduplication. Each asset keeps its media type and original package path.

I kept Markdown output out of scope. This change only makes the shared document model complete for XLSX files. Positional Markdown references are still tracked in #63.

Tests:

  • added a regression test with a minimal XLSX containing an embedded image
  • cargo fmt --all --check
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo test --locked

Fixes #139

@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.

All reported issues were addressed across 1 file

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread src/formats/sheet/xlsx.rs Outdated
@Fanfulla
Fanfulla force-pushed the fix/xlsx-embedded-assets branch from 7d3db22 to 11c8c14 Compare August 31, 2026 14:35
@Fanfulla

Copy link
Copy Markdown
Author

Fixed in 11c8c14. rel_target_bytes and rel_image_source now accept mutable references directly, so XLSX, DOCX, and PPTX use the same relationship resolution and asset retention path.

I reran cargo fmt --all --check, Clippy with warnings denied, and the locked test suite.

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 embedded images are missing from Document.assets

1 participant