Add wafflebase slides import CLI + backend writer#245
Merged
Conversation
Implements Task 6 of docs/tasks/active/20260515-pptx-import-todo.md — the deferred follow-up to PR #243. Users can now import a .pptx via the CLI in addition to the deck list UI: wafflebase slides import deck.pptx --title "My Deck" Why a parallel writer instead of reusing the frontend's pending-import flow: the CLI never attaches a Yorkie client, so the parsed deck has to be persisted through the backend. The existing docs content endpoint already had this shape (REST in, Yorkie write), so the slides side mirrors it. Backend - `packages/backend/src/yorkie/slides-tree.ts` — `writeSlidesRoot` and `readSlidesRoot` for the slides Yorkie root. Plain JSON bulk-assign (slides body is not a Tree CRDT — that experiment was reverted in Phase 5a), matching `slides-view.tsx`'s `doc.update` pattern. - `packages/backend/src/api/v1/docs-content.controller.ts` — `PUT/GET /documents/:id/content` now dispatches on the persisted document type. Slides bodies route to `writeSlidesRoot`, docs to the existing tree writer. A `sniffBodyShape` helper picks the validator arm via `Array.isArray(body.slides|blocks)`; mismatched shape/type returns 400 before any Yorkie attach. - Validation includes per-slide and per-element shape checks (`id`, `type`, `frame`) so corrupted decks fail with a useful 400 instead of persisting and crashing the renderer later. CLI - `packages/cli/src/slides/pptx-import.ts` — thin wrapper around `@wafflebase/slides/node`'s `importPptx`. Defaults to a base64 inline uploader (matches `docs import` — keeps the import round-trippable without a separate `/images` upload pass). - `packages/cli/src/slides/import.ts` — orchestrator mirroring `runDocsImport`, with a `parser` injection point so unit tests avoid building real .pptx bytes per case. - `packages/cli/src/commands/slides.ts` + bin wiring. - HttpClient gets `putSlidesContent` and a widened `createDocument` type union. Testing - Unit specs cover the writer round-trip, the controller's dispatch + validation arms (docs + slides), and the orchestrator's success / failure / dry-run / replace flows. - `packages/backend/test/slides-pptx-import.e2e-spec.ts` exercises the full CLI → API → Yorkie path. Gated by the existing `RUN_DB_INTEGRATION_TESTS` + `RUN_YORKIE_INTEGRATION_TESTS` env switches; the synthetic .pptx is generated inline via the slides fixture builder so no binary needs to be committed. `pnpm verify:fast` green. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds end-to-end PPTX slide-deck import: Yorkie slides serialization, dual-type content API with defensive body-shape validation, CLI PPTX parsing and create/replace orchestration, CLI wiring/HTTP client updates, integration tests, and documentation/type export updates. ChangesSlides PPTX Import Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/test/slides-pptx-import.e2e-spec.ts`:
- Around line 243-249: The test parses importResult.stdout into JSON without
asserting the CLI succeeded; change the mismatch test to first assert the runCli
result indicates success (e.g., check importResult.exitCode === 0 or use the
test framework's expect assertion) before calling JSON.parse on
importResult.stdout so failures produce a clear CLI-failure assertion instead of
a noisy JSON parse error; update the block that uses runCli and the importResult
variable (and the subsequent JSON.parse and { id } extraction) accordingly.
- Around line 82-88: The test currently resolves the child process on the 'exit'
event which can fire before stdio is flushed; update the handler on the
ChildProcess instance (child) to listen for the 'close' event instead of 'exit'
and call resolveResult({ exitCode: signal ? 1 : (code ?? 1), stdout, stderr })
there so stdout/stderr are fully captured for later
JSON.parse(importResult.stdout); leave the same parameters and resolution shape
but move it to 'close'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a922cac2-6b1d-41ca-9eb6-e060cddd8473
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
docs/tasks/active/20260515-pptx-import-lessons.mdpackages/backend/src/api/v1/docs-content.controller.spec.tspackages/backend/src/api/v1/docs-content.controller.tspackages/backend/src/yorkie/slides-tree.spec.tspackages/backend/src/yorkie/slides-tree.tspackages/backend/test/slides-pptx-import.e2e-spec.tspackages/cli/package.jsonpackages/cli/src/bin.tspackages/cli/src/client/http-client.tspackages/cli/src/commands/slides.tspackages/cli/src/slides/import.tspackages/cli/src/slides/pptx-import.tspackages/cli/test/slides-import.test.tspackages/cli/test/slides-pptx-import.test.tspackages/slides/src/node.ts
Contributor
Verification: verify:selfResult: ✅ PASS in 190.2s
Verification: verify:integrationResult: ✅ PASS |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
verify-integration on PR #245 failed with `jszip_1.default is not a constructor` — the slides fixture builder relies on `new JSZip()`, but ts-jest compiles `import JSZip from 'jszip'` to `jszip_1.default()` under its strict CommonJS interop. That's the same trap the docs e2e hit back when it tried to import `@wafflebase/docs` directly. The docs fix was to pre-generate the `.docx` binary; do the same here. - `packages/cli/scripts/gen-sample-pptx.mjs` — tsx script that calls the slides fixture builder and writes the bytes to the backend fixtures dir. Mirrors `gen-sample-docx.mjs`. - `packages/backend/test/fixtures/slides-cli-sample.pptx` — committed binary (8.7 KB). - `slides-pptx-import.e2e-spec.ts` — drops the relative import of the slides fixture and `readFileSync`s the committed binary instead. While here, address CodeRabbit's two inline comments: - Resolve the spawned CLI on `'close'` instead of `'exit'` so stdout/stderr are guaranteed flushed before JSON.parse. Add a `settled` flag so a stray `error` after `close` cannot double- resolve. - Assert `exitCode === 0` before parsing JSON in the body-shape- mismatch test, surfacing a clear CLI failure rather than a noisy JSON.parse stack trace. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
verify-integration runs on a fresh runner separate from verify-self, so it starts without any `dist/` from the prior build job. The CLI e2e specs spawn `tsx packages/cli/src/bin.ts`, which eagerly imports `@wafflebase/docs` and `@wafflebase/slides/node` — both resolve via each package's `exports` field to files under `dist/`. Without the build artifacts the CLI subprocess exits 1 immediately on startup, making every spawn-based e2e (including the previously-green docs-cli-roundtrip) look as if its first assertion failed. This used to work on `main` only because the CLI never reached the slides import path; the docs `dist/` apparently lived in some cached state on the integration runner. Once this PR's CLI started touching `@wafflebase/slides/node`, the missing-dist failure became deterministic. Make the dependency explicit by building both packages in the integration lane before invoking the e2e suite. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/backend/test/slides-pptx-import.e2e-spec.ts`:
- Around line 12-15: Update the stale header comment to reflect that the test
now reads a committed .pptx fixture via SAMPLE_PPTX_PATH instead of generating
it in beforeAll; locate the header in slides-pptx-import.e2e-spec.ts and replace
the description that says the fixture is generated in beforeAll with a brief
"why" note stating the test uses a committed SAMPLE_PPTX_PATH fixture (to keep
tests deterministic and avoid runtime generation), and remove or update any
mention of the prior builder/unit-test generation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59df4cc0-8f3b-49f3-85fb-068d63a9d69b
⛔ Files ignored due to path filters (1)
packages/backend/test/fixtures/slides-cli-sample.pptxis excluded by!**/*.pptx
📒 Files selected for processing (3)
.github/workflows/ci.ymlpackages/backend/test/slides-pptx-import.e2e-spec.tspackages/cli/scripts/gen-sample-pptx.mjs
Comment on lines
+12
to
+15
| * The .pptx fixture is generated in `beforeAll` using the same | ||
| * builder the slides package's unit tests use — a tiny synthetic | ||
| * deck with 1 blank slide. Avoids committing a binary fixture and | ||
| * keeps the test runnable in CI without any external file. |
There was a problem hiding this comment.
Update stale fixture-generation comment to match current behavior.
Lines 12-15 still claim the .pptx is generated in beforeAll and not committed, but this test now reads a committed fixture (SAMPLE_PPTX_PATH). Please align the header comment with the implemented approach to avoid future confusion.
As per coding guidelines, "Write clear comments explaining the 'why' behind complex logic, not just the 'what'".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/backend/test/slides-pptx-import.e2e-spec.ts` around lines 12 - 15,
Update the stale header comment to reflect that the test now reads a committed
.pptx fixture via SAMPLE_PPTX_PATH instead of generating it in beforeAll; locate
the header in slides-pptx-import.e2e-spec.ts and replace the description that
says the fixture is generated in beforeAll with a brief "why" note stating the
test uses a committed SAMPLE_PPTX_PATH fixture (to keep tests deterministic and
avoid runtime generation), and remove or update any mention of the prior
builder/unit-test generation behavior.
The slides e2e expected `meta.title === 'IT Slides'` after the CLI's `--title 'IT Slides'`, but the PPTX parser fills `meta.title` with whatever string the source `.pptx` carries (the synthetic fixture uses "Imported deck"). That value was making it into the Yorkie root and overriding the user's intent. `createDocument` already records the title on the Document row from the same `inferredTitle`, so the deck list reflected the right name — the divergence only surfaced through `GET /content`. Mirror the row title onto `deck.meta.title` before the PUT so the deck list, the editor header, and the GET response all agree. Only the default (POST + PUT) flow is touched. `--replace` keeps the parser's title so re-importing into an existing deck doesn't silently rename its meta when the caller omits `--title`. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
hackerwins
added a commit
that referenced
this pull request
May 16, 2026
PR2 (parser + UI, #243) and PR3 / Task 6 (CLI + backend writer, #245) both shipped. The CLI orchestrator and the backend slides-content dispatch were the last open items; check the remaining boxes and let `tasks:archive` move the pair to `docs/tasks/archive/2026/05/`. Image upload (base64 → real `/images` POST) and orphan-deck cleanup on PUT failure are tracked as follow-ups in the merge commit body, not as still-open items in this todo. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
This was referenced May 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Task 6 of
docs/tasks/active/20260515-pptx-import-todo.md— the deferred follow-up to #243. Users can now import a.pptxvia the CLI in addition to the deck list UI:wafflebase slides import deck.pptx --title "My Deck"packages/backend/src/yorkie/slides-tree.ts—writeSlidesRoot/readSlidesRootfor the slides Yorkie root (plain JSON bulk-assign, matchesslides-view.tsx's pending-importdoc.update).packages/backend/src/api/v1/docs-content.controller.ts—PUT/GET /documents/:id/contentnow dispatches by persisted document type. Slides →writeSlidesRoot; docs → existing Tree CRDT path. AsniffBodyShapehelper picks the validator arm; mismatched shape/type returns 400 before any Yorkie attach.packages/cli/src/slides/{pptx-import,import}.ts— thinimportPptxwrapper (base64 inline uploader, matchesdocs import) and orchestrator mirroringrunDocsImport. Aparserinjection point keeps unit tests deterministic without real.pptxbytes.putSlidesContentand a widenedcreateDocumenttype union.id,type,frame) so corrupted decks fail with a useful 400 instead of crashing the renderer later.Test plan
pnpm verify:fastgreenpnpm verify:integration:docker— e2eslides-pptx-import.e2e-spec.tsgated byRUN_DB_INTEGRATION_TESTS+RUN_YORKIE_INTEGRATION_TESTS; synthesises a minimal.pptxinline so no binary is committedwafflebase slides import deck.pptxagainst a dev backend; confirm GET/documents/:id/contentreturns the parsed deckKnown limitations
data:URLs). Real/api/v1/.../imagesupload is deferred to a follow-up.runSlidesImport(and the docs CLI it mirrors) does not delete the placeholder document if the PUT fails, leaving an empty deck in the workspace. Pre-existing pattern; track as a follow-up that fixes both flows.🤖 Generated with Claude Code
Summary by CodeRabbit