Skip to content

Expose slide boundary anchors for presentations - #95

Open
xianjianlf2 wants to merge 4 commits into
firecrawl:mainfrom
xianjianlf2:fix/slide-boundary-anchors
Open

Expose slide boundary anchors for presentations#95
xianjianlf2 wants to merge 4 commits into
firecrawl:mainfrom
xianjianlf2:fix/slide-boundary-anchors

Conversation

@xianjianlf2

@xianjianlf2 xianjianlf2 commented Aug 14, 2026

Copy link
Copy Markdown

Summary

  • emit slide-N anchors for every PPTX slide instead of only slides targeted by internal links
  • add slide-N anchors for legacy PPT slides in presentation order
  • cover PPTX with/without internal slide links and PPT slide ordering in document-model tests

Tests

  • cargo fmt --check
  • cargo test slide_anchor
  • cargo test
  • git diff --check

Not passing locally: cargo clippy --all-targets -- -D warnings fails on existing unrelated clippy::collapsible_match warnings in src/formats/rtf/tables.rs.

Refs #94


Summary by cubic

Expose slide boundary anchors for every PPTX and PPT slide and render them only for presentations. Previously PPTX anchors appeared only on slides targeted by internal links and unreferenced anchors rendered nothing; now each slide begins with a slide-N anchor, and Markdown emits <a id="slide-N"></a> for PPT/PPTX.

  • Details
    • PPT/PPTX: Insert Block::Paragraph with Inline::Anchor("slide-N") before each slide; numbering follows presentation order and empty slides keep anchors. Removed targeted-slide detection in PPTX; dropped SLIDE_REL and Relationships::iter.
    • Markdown: Add MarkdownOptions.render_unlinked_slide_anchors; to_markdown_bytes enables it for Format::Ppt/Format::Pptx. Other formats still suppress unlinked anchors.
    • Tests: New unit tests for anchor presence/order and updated snapshots include leading slide anchors; please sanity‑check numbering.

Written for commit cb5a21e. Summary will update on new commits.

Review in cubic

@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 4 files

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

Fix all with cubic | Re-trigger cubic

Comment thread src/formats/pptx/mod.rs
Comment thread src/formats/ppt/mod.rs

@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 13 files (changes from recent commits).

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

Fix all with cubic | Re-trigger cubic

Comment thread src/render/markdown/anchors.rs Outdated

@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 4 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic

Comment thread src/render/markdown/anchors.rs
@blueagledev

Copy link
Copy Markdown

Independent verification. We're evaluating anydoc as the extraction layer for a documentation-ingestion pipeline that consumes the document model rather than the Markdown serializer, so slide boundaries are load-bearing for us. We tested this PR and #32 against the same deck; notes on both, and the clippy question answered.

Setup: merge-base e754e1d (v0.1.9), cargo/rustc/clippy 1.95.0, checked out from refs/pull/95/head. Test deck is Pandoc-authored: 2 slides, slide 1 titled with an image and speaker notes, slide 2 has no title placeholder — the pathological case from #31.

cargo test --locked: 223 passed, 0 failed, 1 ignored.

On the clippy failure you flagged — your claim checks out, and it is not yours. cargo clippy --all-targets -- -D warnings fails identically on unmodified main:

error: this `if` can be collapsed into the outer `match`
   --> src/formats/rtf/tables.rs:293:21
    = note: `-D clippy::collapsible-match` implied by `-D warnings`
error: could not compile `anydoc` (lib) due to 3 previous errors

Three collapsible_match at src/formats/rtf/tables.rs 293/302/422, on main, on this PR, and on #91 alike. Pre-existing and unrelated. Worth noting it's a newer-clippy artifact — CI pins dtolnay/rust-toolchain@stable, so it likely hasn't surfaced there yet.

Document model on that deck. Baseline v0.1.9 gives 4 top-level blocks with no marker of any kind — Heading -> Paragraph(Image) -> BlockQuote -> Table — so slide 2's table abuts slide 1 with nothing between. With this PR, 6 blocks:

Paragraph[Anchor("slide-1")] -> Heading -> Paragraph(Image) -> BlockQuote
  -> Paragraph[Anchor("slide-2")] -> Table

The untitled slide separates correctly, and the marker carries the ordinal.

Why we'd prefer this over #32, having run both. #32 works — it emits Block::Rule and separates the same untitled slide correctly — but it gives a boundary where this PR gives an identity. Two consequences for a model consumer:

  • Ordinals can't be recovered from fix(presentations): separate slides with a thematic break #32 by counting rules. Its own (passing) test empty_slides_leave_no_dangling_separator asserts that a 3-slide deck with an empty middle slide yields exactly one rule, so the third slide would be labelled "2". This PR derives ids from slide_paths.enumerate(), so numbering survives empty and unparseable slides — empty_slides_keep_anchor_positions covers it.
  • Block::Rule is a content construct: indistinguishable from a genuine <hr> in source content.

It's also the smaller change than the diffstat suggests — v0.1.9 already builds slide-{n} ids and already pushes Block::Paragraph(vec![Inline::Anchor(..)]), gated on targeted.contains(..). This mostly deletes that gate. The slide-N convention is already upstream's own.

One gap, in fairness to #32: this PR doesn't touch the ODF frontend. On tests/fixtures/odp/pres.odp its output is byte-identical to base (10 blocks, no marker), where #32 does separate .odp (11 blocks, one rule). If .odp parity is wanted, that's a follow-up here.

The two PRs conflict — #32 applies cleanly to main, after which this one conflicts in 6 files (both edit the same statement), so whichever lands first forces the other to rebase.

Happy to re-run any of this on other fixtures if it's useful.

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.

2 participants