fix(exporters): paginate sectionless PPTX screenshots#307
fix(exporters): paginate sectionless PPTX screenshots#307Sun-sunshine06 merged 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Review mode: initial
Findings
-
[Minor]
normalizePageCountreturnsMath.max(1, Math.floor(pageCount))after alreadyMath.ceilin evaluate — thepageCountinside the evaluate function is already an integer viaMath.ceil, soMath.floordoes nothing; this is harmless but unnecessary. Consider usingMath.roundor keeping thepageCountas-is from evaluate.
Suggested fix:return typeof pageCount === 'number' && Number.isFinite(pageCount) ? Math.max(1, pageCount) : 1;
-
[Nit] Extract
PRIMARY_SLIDE_SELECTORandDEFAULT_FALLBACK_SLIDE_SELECTORas typed const strings is good, butPRIMARY_SLIDE_SELECTORis only used once; could be inlined. Not a real issue. -
[Nit] Test
mockPaginationPageCountoverridesevaluateMockglobally, which might affect other tests that rely onevaluateMockreturningundefinedfor non-function calls — butbeforeEachresets mocks, and the existing tests do not test the pagination path, so no risk.
Linked Issue Validation
Closes #284. Fetched issue #284 title: "PPTX export produces a single slide when artifact has no <section> elements" with acceptance criteria:
- Fallback to common slide-like containers when no
<section>exists - Paginate viewport-sized screenshots for sectionless artifacts
- Allow caller override of fallback selector
The PR satisfies all acceptance criteria: (1) adds fallback selectors [data-slide], [data-pptx-slide], [data-slide-container], .slide; (2) paginates by viewport height; (3) exposes slideSelector option. 5 new focused tests cover fallback, custom selector, multi-page, short page, and empty artifact. The claim to close #284 is correct.
Summary
Well-structured, well-tested PR. The diff is clean and follows project conventions (lazy-loading of puppeteer, no silent fallback, proper error handling, changeset added). The only minor observation is the redundant Math.floor in normalizePageCount. Merge-ready after optional cleanup.
Open-CoDesign Bot
5802673 to
a8c4866
Compare
There was a problem hiding this comment.
Review mode: follow-up after new commits
Findings
No new issues. The previous [Minor] finding about redundant Math.floor in normalizePageCount has been resolved — the function now uses Math.max(1, pageCount) directly, matching the suggested fix from the previous review.
Linked Issue Validation
PR title/body claims Closes #284. The diff satisfies all acceptance criteria (fallback slide selectors, pagination, custom selector override, tests). The claim is correct.
Summary
All prior findings addressed. The code is clean, well-tested, follows project conventions (lazy imports, changeset added, no silent fallbacks, proper error handling). Ready to merge.
Open-CoDesign Bot
Summary
data-slide,data-pptx-slide,data-slide-container,.slide) when no<section>existsCloses #284
Validation
pnpm --filter @open-codesign/exporters testpnpm typecheckpnpm exec biome check packages/exporters/src/pptx.ts packages/exporters/src/pptx.test.ts .changeset/polite-pptx-pages.md