Skip to content

fix(ppt-engine): deliver the deck alone, with no pdf copy beside it - #575

Merged
0xKT merged 2 commits into
mainfrom
fix/ppt_engine_no_pdf_beside_deck
Sep 21, 2026
Merged

0xKT merged 2 commits into
mainfrom
fix/ppt_engine_no_pdf_beside_deck

Conversation

@Tchen-data

Copy link
Copy Markdown
Contributor

Summary

The ppt-engine plugin's outbound hook still wrote a PDF rendering next to every published deck and announced it on a second MEDIA: line. A delivery therefore carried two files where the user asked for one, and the transcript showed a PDF tile beside the deck. The earlier change that stopped the build stage from producing the preview (D64) did not reach this hook.

The hook now publishes the deck and names it once. The web UI renders its own preview from the deck on demand, so nothing is lost; the two helpers that placed and named the copy are removed.

Type

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

  • uv run --frozen pytest tests/test_ppt_engine_plugin.py -q -> 52 passed (after rebase onto main). Four tests that asserted the PDF copy and the second MEDIA line are flipped to assert their absence.

  • ruff check / ruff format --check on the two files -> clean.

  • Observed on a full gateway before the fix: a template-engine delivery arrived as deck.pptx plus deck.pdf and two transcript tiles.

  • Relevant tests pass locally

  • Relevant lint / type checks pass locally

  • User-facing docs or screenshots are updated when needed

Risk

  • User-visible: a deck delivery has one file, not two. Anyone who relied on the PDF beside the deck now opens the preview in the web UI or converts the deck themselves.

  • Rollback: revert the squash commit.

  • Security impact considered

  • Backward compatibility considered

  • Rollback path is clear for risky changes

Related Issues

N/A

The plugin's outbound hook still wrote a PDF rendering next to every
published deck and announced it on a second MEDIA line, so a delivery
carried two files where the user asked for one, and the transcript showed
a PDF tile beside the deck. The earlier change that stopped the build
stage from producing the preview did not reach this hook. The hook now
publishes the deck and names it once; the web UI renders its own preview
from the deck on demand, so nothing is lost.
@Tchen-data
Tchen-data requested a review from 0xKT September 21, 2026 03:22

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

I reviewed the full diff and the relevant surrounding paths. Coverage included the repository rules in AGENTS.md and CONTEXT-MAP.md, the hook's iteration and outbound flow, deck verification and publish/delivery helpers, the D64 history that stopped copying previews to named destinations, the browser's on-demand PPTX-to-PDF viewer path, backward compatibility, and the changed tests for weakened coverage. The patch removes the remaining hook-side copy/announcement without removing the browser preview route, and the updated tests retain coverage for standard, renamed, stale-preview, and explicitly delivered decks.

Verification:

  • uv run --frozen --extra dev pytest tests/test_ppt_engine_plugin.py -q -> 52 passed.
  • uv run --frozen --extra dev pytest tests/test_rpc_files.py -q -k 'test_a_deck_is_rendered_and_served_as_a_pdf or test_a_second_click_reads_the_cache or test_a_rebuilt_deck_renders_again' -> 3 passed.
  • git diff --check github/main...HEAD -> clean.
  • Ruff check and format check on both changed files -> clean.

An initial run without the repository's dev extra skipped collection because python-pptx was absent; the reported plugin result above is the rerun in the declared development environment, where all 52 tests executed.

@0xKT

0xKT commented Sep 21, 2026

Copy link
Copy Markdown
Member

Not a blocker. Reviewed by hand (fast tier, 2 files, +24/-61). The removal is
clean and it is pinned: putting the second MEDIA line back turns three tests red,
including test_the_delivered_path_is_announced_without_a_preview, which is the
one that states the new rule.

I checked the claim the commit message rests on rather than taking it: the
repo's own services/publish/deliver.py already says it, in the deliver
docstring -- "the build stage hands none since the deliverable became the deck
alone, and the web surface renders a deck it is shown by itself". So this hook
really was the last place still writing a PDF beside a delivered deck, and the
two helpers it used (_preview_beside_copy, _preview_of) have no remaining
callers anywhere in the tree.

One thing left behind:

published_original in services/publish/deliver.py:348 is now an orphan. At
2bc761b6 it had three references -- two in this hook and its own definition;
at this head only the definition is left, and nothing in tests/ names it. The
hook was its last caller and this PR removed it.

Either delete it in the same change, or, if it is being kept deliberately for a
caller that is coming, a line saying so would stop the next reader from deleting
it for you. Not a blocker either way -- dead code costs reading, not behaviour.

…o call

published_original found the deck a renamed copy was made from so that the
copy's preview could be looked up under the original's stem. With no
preview placed beside a delivered deck any more, nothing asks that
question; the hook was its only caller and the previous commit removed it.
@Tchen-data

Copy link
Copy Markdown
Contributor Author

Deleted in cdebb0f. published_original had no caller left once the hook stopped placing a preview, and nothing was coming for it, so it goes in this change rather than with a note. tests/test_ppt_engine_plugin.py and tests/test_ppt_engine_publish.py pass (86), ruff clean.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

I reviewed the new revision and re-read the full PR diff. The only change since my prior review deletes published_original; it has no remaining repository callers, was not re-exported by raven_ppt.services.publish, and existed solely to support the hook-side preview-copy path this PR removes. The deletion is therefore complete and does not alter a supported compatibility surface. I also rechecked the applicable repository rules, surrounding publish imports and history, and the existing test changes; there is no new weakening or architecture issue.

Verification on this head:

  • uv run --frozen --extra dev pytest tests/test_ppt_engine_plugin.py tests/test_ppt_engine_publish.py -q -> 86 passed.
  • git diff --check github/main...HEAD -> clean.
  • Ruff check and format check on all three changed files -> clean.

@0xKT
0xKT merged commit 96be992 into main Sep 21, 2026
21 checks passed
@0xKT
0xKT deleted the fix/ppt_engine_no_pdf_beside_deck branch September 21, 2026 06:45
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.

3 participants