✨(back) add pptx generation tool#624
Conversation
|
Warning Review limit reached
Next review available in: 7 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (25)
WalkthroughAdds an AI-powered slide deck tool that generates structured presentations, renders them into PPTX files using a bundled template, stores downloadable files, gates tool registration behind a feature flag, and updates chat progress labels. ChangesPresentation generation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChatAgent
participant PresentationAgent
participant build_presentation
participant ObjectStorage
ChatAgent->>PresentationAgent: generate structured Presentation
PresentationAgent-->>ChatAgent: Presentation with slides
ChatAgent->>build_presentation: build PPTX from template
build_presentation-->>ChatAgent: PPTX bytes
ChatAgent->>ObjectStorage: store presentation and request expiring URL
ObjectStorage-->>ChatAgent: signed download URL
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 4
🧹 Nitpick comments (4)
src/backend/chat/file_generation/renderer.py (2)
166-215: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider restricting hyperlink schemes.
run.hyperlink.address = state.linkwrites the Markdownhrefstraight through with no scheme allowlist. Since slide content is model-generated, a manipulated brief could embed a non-http(s) link (e.g.javascript:/data:) that ends up clickable in the delivered deck. Worth restricting tohttp(s)(and maybemailto:) before setting the address.🤖 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 `@src/backend/chat/file_generation/renderer.py` around lines 166 - 215, Restrict hyperlink assignment in _apply_run_style to an allowlist of safe URI schemes, permitting http and https (and mailto only if supported by the existing contract) while rejecting javascript, data, and other schemes. Set run.hyperlink.address only after validation, preserving the current behavior for valid links.
141-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce
_render_list's cognitive complexity.SonarCloud reports this function at 18 vs. the 15 allowed. Extracting the per-item non-list-child handling (paragraph creation + inline rendering) into a small helper would bring it under the threshold without behavior change.
🤖 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 `@src/backend/chat/file_generation/renderer.py` around lines 141 - 164, Reduce cognitive complexity in _render_list by extracting handling of each non-nested-list child into a small helper responsible for paragraph creation, ordered/bullet formatting, and _add_inline. Keep _render_list focused on iterating items, recursing into LIST_TAGS, and skipping whitespace, preserving the current rendering behavior and ordering.Source: Linters/SAST tools
src/backend/chat/tools/generate_presentation.py (2)
63-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMinor: align with
logging.exception()convention flagged by SonarCloud.
exc_info=Trueis already passed at lines 91 and 99, so the traceback isn't actually lost — but SonarCloud's rule (also flagged inbuilder.py) preferslogging.exception()for this pattern. Purely cosmetic since behavior is already equivalent.🩹 Proposed fix
- logger.error("Failed to build the presentation: %s", exc, exc_info=True) + logger.exception("Failed to build the presentation: %s", exc)- logger.error("Failed to store the generated presentation: %s", exc, exc_info=True) + logger.exception("Failed to store the generated presentation: %s", exc)🤖 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 `@src/backend/chat/tools/generate_presentation.py` around lines 63 - 115, Update the exception handlers in generate_presentation, specifically the PresentationAgent.run and store_presentation failure paths, to use logger.exception(...) instead of logger.warning/error(..., exc_info=True). Preserve the existing log messages, exception chaining, and ModelRetry/ModelCannotRetry behavior; leave the PresentationBuildError handler unchanged unless the same convention is required there.Source: Linters/SAST tools
46-60: 🚀 Performance & Scalability | 🔵 TrivialNo cleanup path for the stored blob.
No attachment row is created by design, but the underlying object still gets written to
default_storagepermanently — only the presigned URL expires after an hour, not the object itself. Without an S3 lifecycle rule (or equivalent) scoped to these keys, every generated deck accumulates in storage indefinitely.Please confirm whether a lifecycle policy already prunes objects under
{conversation.pk}/{AttachmentMixin.ATTACHMENTS_FOLDER}/that aren't backed by an attachment row, or whether one should be added for this new artifact type.🤖 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 `@src/backend/chat/tools/generate_presentation.py` around lines 46 - 60, The store_presentation flow currently writes permanent objects without an attachment row or cleanup mechanism. Verify whether default_storage already applies a lifecycle policy to keys under the conversation and AttachmentMixin.ATTACHMENTS_FOLDER prefix; if not, add an equivalent policy for these generated presentation artifacts so unreferenced blobs expire, while preserving the existing presigned URL behavior.
🤖 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 `@src/backend/chat/agents/presentation.py`:
- Around line 26-35: Update the REQUIREMENTS instructions to explicitly allow
the slide_notes field on every slide, alongside each layout’s declared fields.
Preserve the existing guidance to add presenter notes where context helps, while
clarifying that slide_notes is the permitted field name for those notes.
In `@src/backend/chat/file_generation/builder.py`:
- Around line 20-26: Update the exception handler around pptx.Presentation in
the template-opening flow to preserve the traceback by using logger.exception or
passing exc_info=True to logger.error, while retaining the existing contextual
message and PresentationBuildError chaining.
In `@src/backend/chat/file_generation/entities.py`:
- Around line 43-44: Update the Presentation model validation for slides so the
collection must be non-empty and its first Slide has type “cover”; reject any
other first-slide type at schema validation time before rendering.
In
`@src/frontend/apps/conversations/src/features/chat/components/MessageItem.tsx`:
- Line 423: Update the value used by getStreamingToolLabel in MessageItem so it
selects the currently pending/current tool invocation rather than an earlier
completed result from find(). Use the newest eligible invocation from
message.parts when parts are chronological, ensuring a following presentation
tool displays its own label such as “Generating the slides...” instead of the
previous tool’s label.
---
Nitpick comments:
In `@src/backend/chat/file_generation/renderer.py`:
- Around line 166-215: Restrict hyperlink assignment in _apply_run_style to an
allowlist of safe URI schemes, permitting http and https (and mailto only if
supported by the existing contract) while rejecting javascript, data, and other
schemes. Set run.hyperlink.address only after validation, preserving the current
behavior for valid links.
- Around line 141-164: Reduce cognitive complexity in _render_list by extracting
handling of each non-nested-list child into a small helper responsible for
paragraph creation, ordered/bullet formatting, and _add_inline. Keep
_render_list focused on iterating items, recursing into LIST_TAGS, and skipping
whitespace, preserving the current rendering behavior and ordering.
In `@src/backend/chat/tools/generate_presentation.py`:
- Around line 63-115: Update the exception handlers in generate_presentation,
specifically the PresentationAgent.run and store_presentation failure paths, to
use logger.exception(...) instead of logger.warning/error(..., exc_info=True).
Preserve the existing log messages, exception chaining, and
ModelRetry/ModelCannotRetry behavior; leave the PresentationBuildError handler
unchanged unless the same convention is required there.
- Around line 46-60: The store_presentation flow currently writes permanent
objects without an attachment row or cleanup mechanism. Verify whether
default_storage already applies a lifecycle policy to keys under the
conversation and AttachmentMixin.ATTACHMENTS_FOLDER prefix; if not, add an
equivalent policy for these generated presentation artifacts so unreferenced
blobs expire, while preserving the existing presigned URL 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 280763e4-6f0c-408a-bb8e-67fdc6c78e10
⛔ Files ignored due to path filters (2)
src/backend/chat/file_generation/templates/generic.pptxis excluded by!**/*.pptxsrc/backend/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
CHANGELOG.mdsrc/backend/chat/agents/presentation.pysrc/backend/chat/clients/pydantic_ai.pysrc/backend/chat/file_generation/__init__.pysrc/backend/chat/file_generation/builder.pysrc/backend/chat/file_generation/entities.pysrc/backend/chat/file_generation/renderer.pysrc/backend/chat/file_generation/templates/__init__.pysrc/backend/chat/file_generation/templates/generic.pysrc/backend/chat/tests/file_generation/__init__.pysrc/backend/chat/tests/file_generation/test_builder.pysrc/backend/chat/tests/file_generation/test_renderer.pysrc/backend/chat/tests/tools/test_generate_presentation.pysrc/backend/chat/tools/descriptions.pysrc/backend/chat/tools/generate_presentation.pysrc/backend/core/feature_flags/flags.pysrc/backend/core/file_upload/utils.pysrc/backend/core/tests/file_upload/test_utils.pysrc/backend/pyproject.tomlsrc/frontend/apps/conversations/src/features/chat/components/MessageItem.tsxsrc/frontend/apps/conversations/src/i18n/translations.json
| REQUIREMENTS: | ||
| - Only use the slide types listed above, and only fill the fields each one declares. | ||
| - Open with a `cover` slide, and use `section` slides to separate the main parts. | ||
| - Vary the layouts to keep the deck readable; avoid repeating one type throughout. | ||
| - Aim for several slides, each carrying one idea rather than a wall of text. | ||
| - Write field content in Markdown. Inline styles, links, nested lists (indent | ||
| with 4 spaces) and tables are supported. A table takes over its whole field, | ||
| so do not put other content alongside it. | ||
| - Add presenter notes on slides where context helps the speaker. | ||
| - Write in the language of the brief. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Explicitly permit slide_notes on every slide.
Line 27 limits output to fields declared by a layout, but no layout declares slide_notes; Line 34 simultaneously requests notes. Clarify that slide_notes is allowed for every slide, otherwise the model is steered away from producing them.
🤖 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 `@src/backend/chat/agents/presentation.py` around lines 26 - 35, Update the
REQUIREMENTS instructions to explicitly allow the slide_notes field on every
slide, alongside each layout’s declared fields. Preserve the existing guidance
to add presenter notes where context helps, while clarifying that slide_notes is
the permitted field name for those notes.
| title: str | ||
| slides: list[Slide] = Field(min_length=1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg 'src/backend/chat/file_generation/entities.py|file_generation|presentation|generation' || true
echo "== target outline =="
ast-grep outline src/backend/chat/file_generation/entities.py --view expanded || true
echo "== target lines =="
cat -n src/backend/chat/file_generation/entities.py
echo "== searches for Presentation/SlideType/constraints =="
rg -n "class Presentation|class Slide|SlideType|cover|cover_first|first slide|slides\\[0\\]|model_validator|hypothesis|validator" src/backend/chat src/backend -S || trueRepository: suitenumerique/conversations
Length of output: 38944
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== presentation agent lines =="
cat -n src/backend/chat/agents/presentation.py | sed -n '1,110p'
echo "== generate presentation lines =="
cat -n src/backend/chat/tools/generate_presentation.py | sed -n '40,90p'
echo "== builder lines =="
cat -n src/backend/chat/file_generation/builder.py | sed -n '1,70p'
echo "== tests around validation/use =="
cat -n src/backend/chat/tests/file_generation/test_builder.py | sed -n '140,160p'
cat -n src/backend/chat/tests/tools/test_generate_presentation.py | sed -n '55,85p'
echo "== pydantic availability =="
python3 - <<'PY'
try:
import pydantic, pydantic.version
print("pydantic", pydantic.__version__, pydantic.version.VERSION)
except Exception as e:
print("pydantic unavailable:", type(e).__name__, e)
else:
from pydantic import BaseModel, Field, model_validator
from enum import StrEnum
class SlideType(StrEnum):
COVER = "cover"
OTHER = "other"
class Slide(BaseModel):
type: SlideType
class WeakPresentation(BaseModel):
title: str
slides: list[str] = Field(min_length=1)
class StrongPresentation(BaseModel):
title: str
slides: list[Slide] = Field(min_length=1)
`@model_validator`(mode="after")
def validate_structure(self):
if self.slides[0].type != "cover":
raise ValueError("The first slide must be a cover slide")
print("weak valid non-cover", WeakPresentation(title="x", slides=["other"]).slides)
try:
StrongPresentation(title="x", slides=[{"type": "cover"}])
print("strong valid cover allowed")
except Exception as e:
print("strong cover error", e)
try:
StrongPresentation(title="x", slides=[{"type": "other"}])
except Exception as e:
print("strong non-cover error", type(e).__name__)
PYRepository: suitenumerique/conversations
Length of output: 10604
Enforce the cover-first deck contract in Presentation.
The agent contract requires the first slide to be cover, but Presentation only rejects empty decks. A non-cover first slide is accepted via structured output and passed to deck rendering; add a Pydantic validator so this violates the schema boundary instead.
🤖 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 `@src/backend/chat/file_generation/entities.py` around lines 43 - 44, Update
the Presentation model validation for slides so the collection must be non-empty
and its first Slide has type “cover”; reject any other first-slide type at
schema validation time before rendering.
| {activeToolInvocation?.toolName === 'summarize' | ||
| ? t('Summarizing...') | ||
| : t('Search...')} | ||
| {getStreamingToolLabel(activeToolInvocation?.toolName, t)} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Select the currently streaming tool before displaying its label.
Line [423] uses activeToolInvocation, but that value is derived with find() and can refer to an earlier completed invocation. When a presentation tool follows another tool in the same message, the loader may still show “Search...” instead of “Generating the slides...”. Select the pending/current invocation (or the newest eligible invocation if message.parts is chronological).
🤖 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 `@src/frontend/apps/conversations/src/features/chat/components/MessageItem.tsx`
at line 423, Update the value used by getStreamingToolLabel in MessageItem so it
selects the currently pending/current tool invocation rather than an earlier
completed result from find(). Use the newest eligible invocation from
message.parts when parts are chronological, ensuring a following presentation
tool displays its own label such as “Generating the slides...” instead of the
previous tool’s label.
Add a pptx generation tool
3d1cd48 to
b4f749e
Compare
There was a code spell deteciton on acount
Was missing a new config
|
Purpose
This PR adds a pptx generation tool. The code is largely (and voluntarily) derived from PIAG's implementation, in light of the convergence efforts. The same template is used, for now, to keep a feature parity.
Summary by CodeRabbit
New Features
Bug Fixes