Repository navigation
feat(ppt): Implement Orchestration and Assembly layer For PPT Gen - #799
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
WalkthroughAdds end-to-end PPT generation: context extraction, AI-driven content planning, slide generation (user/AI images), rich rendering (standard and composite slides), PPTX assembly and file output, and propagation of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BaseProvider
participant PPTUtils as PPT Utils
participant Orchestrator
participant Planner as Content Planner
participant Generator as Slide Generator
participant Renderer as Slide Renderers
participant AI as AI Provider
participant FS as FileSystem
User->>BaseProvider: generate(options with PPT output)
BaseProvider->>PPTUtils: extractPPTContext(options)
PPTUtils-->>BaseProvider: PPTGenerationContext
BaseProvider->>Orchestrator: generatePresentation(context)
Orchestrator->>Planner: generateContentPlan(context)
Planner->>AI: request content plan
AI-->>Planner: content plan
Planner-->>Orchestrator: validated plan
Orchestrator->>Generator: generate slides(plan)
loop per slide
Generator->>Generator: check userImages
alt user image available
Generator->>Generator: load user image
else
Generator->>AI: generate image (if enabled)
AI-->>Generator: image (base64)
end
Generator->>Renderer: render slide(content, image)
Renderer-->>Generator: rendered slide
end
Orchestrator->>Orchestrator: assemble PPTX, set metadata
Orchestrator->>FS: write PPTX file
FS-->>Orchestrator: file saved
Orchestrator-->>BaseProvider: PPTGenerationResult
BaseProvider-->>User: GenerateResult (includes ppt)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
635b458 to
fbee608
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@src/lib/core/baseProvider.ts`:
- Around line 1416-1425: Wrap the long-running call to generatePresentation in
baseProvider (the ppt orchestration block using generatePresentation) with the
withTimeout utility to enforce a maximum wait time; replace the direct await
generatePresentation(...) with await withTimeout(generatePresentation({...}),
<timeoutMs>) (use the existing config/constant or this.options.pptTimeout and a
sensible default), and ensure you catch/propagate the timeout error consistently
(same error handling path as other orchestration failures) so hung requests are
prevented.
In `@src/lib/features/ppt/presentationOrchestrator.ts`:
- Around line 185-188: The slide generation and file write calls must be wrapped
with the withTimeout utility to ensure they fail fast; update the code where
SlideGenerator is instantiated and where
slideGenerator.generateSlides(state.contentPlan.slides) is awaited, and wrap any
subsequent writeFile(...) calls (also at the later block around the other
occurrence) with withTimeout, passing the configured timeout and
throwing/returning a typed timeout error (e.g., SlideGenerationTimeoutError or
FileWriteTimeoutError) so callers can distinguish timeout vs other failures;
ensure you keep the original calls as the inner promise argument to withTimeout
and propagate the typed error on timeout.
In `@src/lib/features/ppt/slideGenerator.ts`:
- Around line 166-189: The loadUserImage method uses synchronous fs.readFileSync
and an unbounded fetch; change it to fully async by using fs.promises.readFile
instead of readFileSync and wrap both the fetch call and the file read in the
withTimeout utility (e.g., await withTimeout(fetch(...), ms) and await
withTimeout(fs.promises.readFile(path), ms)). Ensure you handle non-OK fetch
responses as before, preserve the Buffer return type, catch timeout errors in
the existing try/catch, and add an import for withTimeout if not already
present; update references in loadUserImage accordingly.
In `@src/lib/features/ppt/slideRenderers.ts`:
- Around line 223-248: The while loop currently uses an assignment inside its
condition (while ((match = pattern.exec(text)) !== null) ) which Biome flags;
refactor by performing the assignment on its own line before the loop or at the
end of the loop and use a plain condition (e.g., assign match =
pattern.exec(text) before entering a while (match !== null) loop or use a
for(;;) with a break) so that the RegExpExecArray | null variable match
(declared above) is set via match = pattern.exec(text) and then tested with
match !== null; update references to pattern, match, lastIndex and
pattern.lastIndex in the loop body accordingly.
- Around line 2856-2868: The code calls String.fromCodePoint(parseInt(icon, 16))
without validating the parsed value which can throw for invalid or out-of-range
inputs; update the logic around the iconChar computation (the parseInt(icon, 16)
call and the String.fromCodePoint usage used before slide.addText) to validate
and guard: parse the hex safely, check for NaN and ensure the code point is
within 0..0x10FFFF, and wrap the fromCodePoint call in a try/catch or
conditional so that on invalid input you use a safe fallback (e.g., a
placeholder character or skip adding the icon) before calling slide.addText (the
parameters x/y/w/h/fontSize/color/align/valign/fit and DEFAULT_TEXT_FIT should
remain unchanged).
- Around line 1003-1047: The code unsafely casts the rich-text runs returned by
createFormattedTextProps to string using "as unknown as string"; fix by
introducing a proper rich-text union type and updating signatures instead of
casting: define a RichTextRun/FormattedTextRun type that matches pptxgenjs
rich-text run objects (or import the appropriate type from pptxgenjs), change
PptxTextProps.text from string to string | RichTextRun[], and update
createFormattedTextProps to return RichTextRun[] (and any callers like the
textLines pushes in the bullet-handling block and the other occurrence around
line ~1078) so you can remove the "as unknown as string" casts and preserve type
safety while keeping existing logic in hasMarkdownFormatting, parseMarkdownText,
and getBulletOptions intact.
In `@test/unit/ppt-generation.test.ts`:
- Around line 1476-1488: The test's expected defaults don't match the current
implementation: update either the test or the implementation. If you want to
keep the current behavior, change the assertions in the test to expect
extractPPTContext's actual defaults (theme "AI will decide", audience "AI will
decide", tone "AI will decide", generateAIImages false) while keeping topic and
pages assertions; otherwise, if the new defaults are intended, change the
default values inside the extractPPTContext function (and any related defaulting
logic for PPT in GenerateOptions) to set theme="modern", audience="general",
tone="professional", and generateAIImages=true so the test passes. Ensure you
only modify extractPPTContext (or its default provider) or the test assertions
accordingly.
🧹 Nitpick comments (4)
src/lib/features/ppt/utils.ts (1)
140-215: Consider adding timeout handling for provider creation.The
getEffectivePPTProviderfunction performs async operations including dynamic imports and provider creation viaAIProviderFactory.createProvider, but doesn't wrap these with timeout handling.As per coding guidelines: "Wrap async operations with withTimeout utility for timeout handling". Consider adding timeout protection to prevent hanging if provider creation takes too long.
src/lib/features/ppt/constants.ts (1)
444-459: Remove redundant null check.Lines 449-451 contain a redundant null check for
modelInfo.name. The check at lines 445-447 already handles the case wheremodelInfo?.nameis falsy, making the second check unreachable when the first passes.🧹 Proposed fix to remove redundancy
export function getPromptTier(modelInfo?: ModelInfo): PromptTier { if (!modelInfo?.name) { return "basic"; } - if (!modelInfo.name) { - return "basic"; - } - const modelLower = modelInfo.name.toLowerCase();test/unit/ppt-generation.test.ts (1)
1-214: Align test placement with suite structure and add CLI multimodal coverage.This unit test lives under
test/unit/, but the suite layout requires feature-specific tests undertest/suites/, and CLI multimodal coverage (with--image,--csv) should live in integration tests. Please relocate and add the missing CLI coverage in the appropriate integration suite.
As per coding guidelines: Test suite organization must follow: feature-specific in test/suites/, integration tests in test/integration/; Test multimodal content handling with --image, --pdf, and --csv flags in CLI tests.src/lib/features/ppt/presentationOrchestrator.ts (1)
15-19: Avoidas unknown asfor pptxgenjs typing.Casting through
unknownsidesteps strict typing. Consider using the library’s official type export or a local.d.tsshim so the constructor type is enforced without unsafe casts.
As per coding guidelines: Type safety must be maintained across all TypeScript modules using strict TypeScript compiler settings.
ceb0419 to
89c295e
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/features/ppt/constants.ts (1)
468-485:⚠️ Potential issue | 🟠 MajorAlign the “last slide” rule with post‑processing.
The system prompt now instructs the AI that the last slide must be
"closing", butpostProcessPlan/ensureThankYouSlidealways coerces the final slide to"thank-you". That mismatch can cause the AI to generate closing-specific content (bullets/summary) that won’t render correctly on a thank‑you slide. Either update the prompt rule to"thank-you"or adjust post‑processing to preserve"closing".🛠️ Suggested prompt alignment
-6. First slide is always type "title", last slide is always type "closing" +6. First slide is always type "title", last slide is always type "thank-you"
🤖 Fix all issues with AI agents
In `@src/lib/features/ppt/presentationOrchestrator.ts`:
- Around line 247-262: Wrap the calls to ensureOutputDirectory(state.outputPath)
and fs.stat(state.outputPath) in withTimeout to prevent hangs: replace the
direct await ensureOutputDirectory(...) with await
withTimeout(ensureOutputDirectory(state.outputPath), PPT_GENERATION_TIMEOUT_MS /
4, ErrorFactory.toolTimeout("ensureOutputDir", PPT_GENERATION_TIMEOUT_MS / 4))
and similarly wrap fs.stat(state.outputPath) with await
withTimeout(fs.stat(state.outputPath), PPT_GENERATION_TIMEOUT_MS / 4,
ErrorFactory.toolTimeout("statOutputPath", PPT_GENERATION_TIMEOUT_MS / 4)); keep
using the existing withTimeout helper and ErrorFactory.toolTimeout and preserve
assignment to fileSize from the resolved stats.
In `@src/lib/features/ppt/slideTypeInference.ts`:
- Around line 350-397: The function applyBulletStyleToContent returns early when
content.bullets is falsy, which skips applying inferred bulletStyle to
column-only slides; remove that early return and always construct and return the
content object that maps bullets for the top-level bullets (if present) and for
leftColumn, rightColumn, and centerColumn, ensuring you only set bulletStyle
when b.bulletStyle is falsy (preserve the existing conditional b.bulletStyle ||
bulletStyle) and keep all other fields via spreading (refer to content.bullets,
leftColumn, rightColumn, centerColumn and the applyBulletStyleToContent function
to locate and update the logic).
In `@src/lib/features/ppt/utils.ts`:
- Around line 82-92: The code sets generateAIImages to false which conflicts
with validatePPTGenerationInput that treats undefined as enabled; update the
context extraction so generateAIImages is left undefined when not explicitly set
(e.g. change generateAIImages: pptOptions.generateAIImages ?? false to
generateAIImages: pptOptions.generateAIImages ?? undefined or simply omit the
default) so validatePPTGenerationInput can correctly enable AI images, and
ensure the surrounding comment/documentation aligns with generateAIImages and
validatePPTGenerationInput behavior.
In `@test/unit/ppt-generation.test.ts`:
- Around line 1225-1542: The Presentation Orchestrator feature-level tests
(those exercising orchestratorValidateInput, extractPPTContext, PPTError and
GenerateOptions) need to be relocated from test/unit to the feature test suites
directory: move this entire test block (the "Presentation Orchestrator" describe
suite and all its nested tests) into a new or existing file under test/suites/
so it lives with other feature-specific tests; ensure any import paths remain
correct after the move and update test runner config if necessary.
🧹 Nitpick comments (3)
src/lib/types/pptTypes.ts (1)
618-742: Tighten dashboard zone typing to preserve strict TS guarantees.
data?: unknownforces downstream casting; consider a discriminated union keyed bytypeto enforce payload shapes at compile time.As per coding guidelines: Type safety must be maintained across all TypeScript modules using strict TypeScript compiler settings.
src/lib/features/ppt/utils.ts (2)
34-39: PreferErrorFactoryfor PPT errors to keep SDK error creation consistent.
This file constructsPPTErrordirectly; consider adding/using factory helpers for these cases.As per coding guidelines: Use ErrorFactory for creating typed errors across the SDK.
Also applies to: 255-260
140-194: Wrap long-running async ops withwithTimeout.
Provider creation and filesystem directory creation can hang; wrapping them helps enforce bounded latency.As per coding guidelines: Wrap async operations with withTimeout utility for timeout handling.
Also applies to: 248-252
89c295e to
718fc6a
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/features/ppt/contentPlanner.ts (1)
548-588: 🛠️ Refactor suggestion | 🟠 MajorWrap the AI planning call with
withTimeout.
provider.generateis an external async call; per guideline it should be wrapped withwithTimeoutso timeouts are enforced consistently even if provider-level timeouts are ignored.As per coding guidelines: Wrap async operations with withTimeout utility for timeout handling.
src/lib/features/ppt/constants.ts (1)
468-485:⚠️ Potential issue | 🟡 MinorPrompt rule conflicts with enforced final slide type.
The system prompt mandates last slide type
"closing", but post-processing enforces"thank-you"(viaensureThankYouSlide). Align the prompt or the post-process rule to avoid the AI generating content that gets overwritten.💡 Possible prompt fix
-6. First slide is always type "title", last slide is always type "closing" +6. First slide is always type "title", last slide is always type "thank-you"
🤖 Fix all issues with AI agents
In `@src/lib/features/ppt/contentPlanner.ts`:
- Around line 319-351: The current passThrough loop uses a string[] and casts to
Record<string, unknown>, which bypasses SlideContent typing; update by making
passThrough typed as (keyof SlideContent)[] or by filtering the existing
string[] against keys of SlideContent before assignment, and remove the unsafe
cast to Record<string, unknown> so assignments target normalized typed as
SlideContent; if fields like "formatting" or "showHeadingLine" are legitimate,
add them to the SlideContent interface instead of leaving them as untyped
strings.
In `@src/lib/features/ppt/slideRenderers.ts`:
- Around line 2689-2699: The fill color currently concatenates an 8‑digit hex
(theme.colors.primary.replace("#", "") + "15") which pptxgenjs doesn't accept;
update the fill object in the slide renderer so fill.color is the 6‑digit hex
(theme.colors.primary.replace("#","")) and set fill.transparency to the
corresponding percent (alpha 0x15 ≈ 21/255 → ~92% transparency) when isPrimary;
leave non‑primary as "F8F9FA" (or set transparency 0). Adjust the code around
the fill object in slideRenderers.ts (the block that builds fill for the shape
using isPrimary and theme.colors.primary) to use fill.color + fill.transparency
instead of concatenating an alpha nibble.
In `@src/lib/features/ppt/utils.ts`:
- Around line 140-214: In getEffectivePPTProvider wrap the async call to
AIProviderFactory.createProvider with the withTimeout helper (use the project
standard timeout constant or pass a sensible timeout) so the provider creation
can't hang; similarly, locate the async fs.mkdir usage (around the referenced
other block) and wrap that call with withTimeout as well; ensure you import or
reference withTimeout where used and propagate/handle the timeout error path
consistently (keep the returned types and ErrorFactory.invalidParameters
behavior unchanged).
In `@src/lib/types/pptTypes.ts`:
- Around line 1393-1419: The PptxTextProps.bullet option type is using
characterCode while BulletOptions and the pptxgenjs API expect code; update the
union type under PptxTextProps.bullet to rename characterCode to code (string,
representing the hex Unicode codepoint) so it matches BulletOptions and the
external API, ensuring all references to characterCode in that type are replaced
with code.
718fc6a to
54756eb
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
54756eb to
37caa49
Compare
| * - basic: 10 essential slide types for simpler/faster models | ||
| * - advanced: All slide types for powerful models | ||
| */ | ||
| export type PromptTier = "basic" | "advanced"; |
There was a problem hiding this comment.
no types outside of types fodler
| /** | ||
| * Model info for prompt tier detection | ||
| */ | ||
| export type ModelInfo = { |
There was a problem hiding this comment.
check this for all the files
b3dae72 to
7f676fd
Compare
|
🎉 This PR is included in version 9.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Pull Request
Description
What does this PR do?
This PR implements Step 4: Orchestrator + Assembly from the PPT Generation implementation plan. The orchestration layer coordinates the entire PPT generation pipeline - from content planning through slide generation to final PPTX assembly.
Related Issues
Does this PR close any issues?
Fixes #(issue number)
Closes #(issue number)
Relates to #(issue number)
Type of Change
Please select the type of change:
Motivation and Context
Why is this change needed? What problem does it solve?
Provide context for reviewers:
Changes Made
What specific changes were made?
Provide a bullet-point list of the key changes:
Breaking Changes
Does this PR introduce breaking changes?
If yes, describe:
Testing
How has this been tested?
Please describe the tests you ran and their results:
Test Coverage
Manual Testing Steps
Provide steps for manual testing:
Code Quality
Have you followed code quality standards?
Documentation
Have you updated documentation?
Commit Message Format
Does your commit follow semantic commit conventions?
type(scope): descriptionExample:
feat(providers): add support for LiteLLM proxyDependencies
Does this PR add, update, or remove dependencies?
If yes, list dependencies and justification:
Performance Impact
Does this change affect performance?
If applicable, provide benchmark results:
Security Considerations
Are there any security implications?
If applicable, describe:
Deployment Notes
Special deployment instructions?
Screenshots / Videos
If applicable, add screenshots or videos to demonstrate changes:
[Add screenshots or videos here]
Reviewer Checklist
For reviewers:
Additional Notes
Any additional information for reviewers:
[Add any extra context, concerns, or questions here]
Pre-submission Checklist
Before submitting, ensure you have:
pnpm testpnpm buildpnpm run validate:alland all checks passThank you for contributing to NeuroLink!
Summary by CodeRabbit
New Features
Configuration Updates