fix(pptx): do not emit a Notes heading for a slide with no notes - #2427
Conversation
Diogo Damasceno (Diogo-Damasceno)
left a comment
There was a problem hiding this comment.
Verified locally (Arch Linux, Python 3.14.7, markitdown from source at b6e8bbd, python-pptx installed).
Reproduced before the fix. Applying only this PR's test file to main without the source change fails 2 tests, with the empty heading visible in the output:
assert '### Notes:' not in '<!-- Slide ...\n### Notes:'
So the heading really is emitted for a slide whose notes slide carries no text, matching the report.
After the fix: the PR's 4 tests pass. I also ran the wider pptx selection (-k pptx, ignoring test_outlook_msg_ansi.py which fails to collect here for an unrelated missing module): 14 passed with the change vs 12 passed / 2 failed on main. No regressions.
The change reads right to me: gating on notes_text.strip() instead of on has_notes_slide is the actual distinction, and it keeps the heading and its content emitted together rather than stripping afterwards.
There was a problem hiding this comment.
🟡 Changes recommended
The OCR-enhanced PPTX converter retains the same empty-heading bug.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents empty PPTX speaker-note headings by checking note content before rendering.
Changes:
- Filters empty and whitespace-only notes.
- Adds regression coverage for empty, absent, and populated notes.
File summaries
| File | Description |
|---|---|
_pptx_converter.py |
Conditionally emits notes sections. |
test_pptx_notes.py |
Adds notes-rendering tests. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if notes_text.strip(): | ||
| md_content += "\n\n### Notes:\n" + notes_text |
What this changes
A slide gets a
### Notes:heading with nothing under it whenever the deck has a notes slidethat carries no text:
The heading announces a section that does not exist. Downstream that reads as a slide whose
speaker notes were dropped, rather than a slide that never had any.
Why
The converter treats "has a notes slide" as "has notes":
Those are different things. PowerPoint writes a
notesSlidepart for a slide whose notes panehas merely been opened, and a deck saved from a template can carry one for every slide, so
has_notes_slideis true for a great many slides with empty notes. The heading is emittedbefore the text is read, so an empty (or whitespace-only) notes frame still gets one. The
trailing
.strip()removes the newline after the heading but not the heading itself.Fix
Read the notes first and only emit the heading when there is something to head. This is the
same shape as the fix in #1990, where
WikipediaConverterstopped rendering a# Noneheading for a page with no title.
Tests
New file
packages/markitdown/tests/test_pptx_notes.py, over one-slide decks built in memory:""produces no headingAgainst unmodified
main:With the fix:
The
test.pptxfixture emits no### Notes:before or after this change, so the existingvectors are untouched.
How I tested
Windows 11, Python 3.12, editable install of
packages/markitdown[all].Those 18 are unchanged by this PR:
maingives18 failed, 426 passed, 4 skippedon thismachine before any edit, and the 4 added tests account for the difference. They are the CLI
stdout-encoding, Windows file-URI and speech-transcription tests that need a UTF-8 console, a
case-sensitive path and network/ffmpeg.
black --checkclean on both files.