fix: wire portable presentation renderer into Telegram delivery#2483
Conversation
…#2481) Bind the orphaned MessagePresentation renderers into the live delivery path. The Telegram adapter now implements SupportsPresentation (render_presentation / presentation_limits / truncate_presentation), delegating to TelegramPresentationRenderer and sending a native inline keyboard. The inbound handler (streaming + non-streaming) pops the agent-captured presentation and renders it after the text reply, which serves as the plain-text fallback. Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Action performedReview finished.
|
MervinPraison
added
pipeline/blocked:ci
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
Telegram Presentation Wiring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
Greptile SummaryThis PR wires portable presentation output into Telegram delivery. The main changes are:
Confidence Score: 4/5This is close, but the remaining callback and delivery issues should be fixed before merging.
src/praisonai/praisonai/bots/_presentation_renderer.py; src/praisonai/praisonai/bots/telegram.py Important Files Changed
Reviews (2): Last reviewed commit: "fix(telegram): harden presentation rende..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/praisonai/tests/unit/test_presentation_wiring.py (1)
25-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the encoded action payloads, not just markup presence.
This only proves that some inline keyboard was sent. It would still pass if command/URL/callback actions were encoded incorrectly, and there is no callback-action case at all even though that is a required objective for this PR.
Suggested test expansion
def _buttons_presentation(): @@ PresentationButton( label="Docs", action=PresentationAction( type=ActionType.URL, url="https://example.com" ), ), + PresentationButton( + label="Later", + action=PresentationAction( + type=ActionType.CALLBACK, value="later" + ), + ), ]), ]) @@ assert message_id == "42" assert sent["chat_id"] == 123 - # The portable buttons must reach the channel as a native inline keyboard. - assert "reply_markup" in sent + markup = sent["reply_markup"] + rows = markup.inline_keyboard + assert rows[0][0].callback_data == "cmd:/yes" + assert rows[0][1].url == "https://example.com" + assert rows[0][2].callback_data == "later"Also applies to: 72-96
🤖 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/praisonai/tests/unit/test_presentation_wiring.py` around lines 25 - 48, The presentation wiring test only checks that markup exists, so it can miss broken action encoding in MessagePresentation and PresentationButton. Update the relevant tests around _buttons_presentation and the other presentation case to assert the encoded payloads for each action type, including COMMAND, URL, and the missing CALLBACK path, using the existing PresentationAction, ActionType, and PresentationBlock helpers to verify the actual serialized button data rather than just presence of an inline keyboard.src/praisonai/praisonai/bots/telegram.py (1)
1689-1725: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse the outbound retry wrapper for presentation delivery too.
This new send path calls
self._application.bot.send_message(...)directly, while the rest of the adapter routes outbound text throughdeliver_with_retry. A transient Telegram failure will silently drop the inline keyboard even though the adapter already has backoff/DLQ handling for normal sends.Suggested fix
- message = await self._application.bot.send_message(**send_kwargs) + message = await deliver_with_retry( + lambda: self._application.bot.send_message(**dict(send_kwargs)), + policy=self._outbound_backoff, + platform="telegram", + parked_store=self._outbound_dlq, + reply_data={ + "channel_id": str(target), + "reply_text": send_kwargs["text"], + "reply_to": "", + }, + )🤖 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/praisonai/praisonai/bots/telegram.py` around lines 1689 - 1725, The new `render_presentation` send path bypasses the adapter’s outbound retry/DLQ flow by calling `self._application.bot.send_message` directly. Update `TelegramBotAdapter.render_presentation` to send the rendered text and inline keyboard through `deliver_with_retry` (or the same outbound wrapper used by normal messages) so transient Telegram failures are retried consistently. Keep the `TelegramPresentationRenderer.render` and inline keyboard construction logic, but move the final delivery step into the existing retry-backed helper and preserve the returned message id handling.
🤖 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/praisonai/praisonai/bots/telegram.py`:
- Around line 541-551: Move the session cleanup earlier in the telegram flow: in
the branches around self._session.chat(...) and the later presentation rendering
block, call self._session.pop_last_presentation(user_id) immediately after a
successful chat return and store it in a local presentation variable. Then
render that stored presentation later if present, so hook cancels or send errors
don’t leave stale UI behind and the session contract of
_session.pop_last_presentation(...) is preserved.
---
Nitpick comments:
In `@src/praisonai/praisonai/bots/telegram.py`:
- Around line 1689-1725: The new `render_presentation` send path bypasses the
adapter’s outbound retry/DLQ flow by calling
`self._application.bot.send_message` directly. Update
`TelegramBotAdapter.render_presentation` to send the rendered text and inline
keyboard through `deliver_with_retry` (or the same outbound wrapper used by
normal messages) so transient Telegram failures are retried consistently. Keep
the `TelegramPresentationRenderer.render` and inline keyboard construction
logic, but move the final delivery step into the existing retry-backed helper
and preserve the returned message id handling.
In `@src/praisonai/tests/unit/test_presentation_wiring.py`:
- Around line 25-48: The presentation wiring test only checks that markup
exists, so it can miss broken action encoding in MessagePresentation and
PresentationButton. Update the relevant tests around _buttons_presentation and
the other presentation case to assert the encoded payloads for each action type,
including COMMAND, URL, and the missing CALLBACK path, using the existing
PresentationAction, ActionType, and PresentationBlock helpers to verify the
actual serialized button data rather than just presence of an inline keyboard.
🪄 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: 7fbc6660-fe9c-4987-9b77-3503c094eaac
📒 Files selected for processing (2)
src/praisonai/praisonai/bots/telegram.pysrc/praisonai/tests/unit/test_presentation_wiring.py
| # Render any agent-attached portable presentation | ||
| # (buttons/menus) natively after the streamed text. | ||
| try: | ||
| presentation = self._session.pop_last_presentation(user_id) | ||
| if presentation is not None: | ||
| await self.render_presentation( | ||
| str(update.message.chat_id), presentation | ||
| ) | ||
| except Exception as e: # pragma: no cover — defensive | ||
| logger.debug("presentation render skipped: %s", e) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Pop the captured presentation before any cancel/send path.
pop_last_presentation() is the clearing step for this turn’s session UI, but here it runs only after the primary reply succeeds. A hook cancel on Line 503 or Line 603, or any earlier send error, skips these blocks and leaves stale buttons behind for the next user turn. That breaks the session contract from _session.pop_last_presentation(...).
Suggested fix
- try:
- presentation = self._session.pop_last_presentation(user_id)
+ try:
if presentation is not None:
await self.render_presentation(
str(update.message.chat_id), presentation
)Move the pop_last_presentation(user_id) call to immediately after each successful self._session.chat(...) return in both branches, store it in a local presentation, and only render it later.
Also applies to: 610-622
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 549-549: Do not catch blind exception: Exception
(BLE001)
🤖 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/praisonai/praisonai/bots/telegram.py` around lines 541 - 551, Move the
session cleanup earlier in the telegram flow: in the branches around
self._session.chat(...) and the later presentation rendering block, call
self._session.pop_last_presentation(user_id) immediately after a successful chat
return and store it in a local presentation variable. Then render that stored
presentation later if present, so hook cancels or send errors don’t leave stale
UI behind and the session contract of _session.pop_last_presentation(...) is
preserved.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2483 ✅Read all prior reviews (Greptile, CodeRabbit, Qodo, Copilot). Implemented the valid fixes and pushed to this branch ( Phase 1 — Architecture (per AGENTS.md)
Phase 2 — Reviewer findings triaged & fixed
Files modified
Phase 4 — Tests
Phase 3 — VerdictApprove. All blocking/valid issues from the reviewers are resolved; the change is focused, protocol-correct, and backward-compatible. The one skipped nitpick (retry wrapper) is intentional and non-blocking. |
- Truncate callback_data on UTF-8 byte boundary (Telegram 64-byte limit), preventing non-ASCII payloads from silently dropping the inline keyboard - Convert renderer web_app dicts to WebAppInfo objects (python-telegram-bot >=20) - Pop the captured presentation immediately after chat() in both streaming and non-streaming paths so cancel/return doesn't leave stale buttons next turn - Strengthen tests: assert per-button encoded payloads + callback case, add a 64-byte callback_data truncation test Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #2483 ✅
I re-read all prior reviews (Greptile, CodeRabbit, Qodo, Copilot) and verified the current branch state ( Phase 1 — Architecture (per AGENTS.md)
Phase 2 — Reviewer findings verified as resolved
Phase 4 — Tests
Phase 3 — VerdictApprove. Every blocking/valid issue from the reviewers is resolved, the change is focused, protocol-correct, and backward-compatible. The one skipped nitpick (retry wrapper) is intentional and non-blocking. No further changes pushed — branch is already in a mergeable state. |
| encoded = value.encode("utf-8") | ||
| if len(encoded) <= TelegramPresentationRenderer._CALLBACK_MAX_BYTES: | ||
| return value | ||
| return encoded[: TelegramPresentationRenderer._CALLBACK_MAX_BYTES].decode( |
There was a problem hiding this comment.
| # Consume the agent-attached portable presentation now | ||
| # (session-contract clearing step) so a cancel/return | ||
| # below doesn't leave stale buttons for the next turn. | ||
| presentation = self._session.pop_last_presentation(user_id) |
There was a problem hiding this comment.
pop_last_presentation() call, _send_response_with_media() or render_presentation() can fail, and render_presentation() returns None after catching delivery errors. In that case the text reply can be sent without controls, while the session no longer has the presentation to retry or render later. Keep the presentation until the keyboard send succeeds, or restore it when rendering fails.
MervinPraison
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #2481
Summary
The typed, portable presentation model (
MessagePresentation/PresentationBlock/PresentationButton) and its per-channel renderers existed but had no call site — agent-emitted buttons/menus were captured into message metadata and dropped. This wires the renderer into the live Telegram delivery path.Changes
telegram.py—TelegramBotnow implements theSupportsPresentationcontract:presentation_limits→PresentationLimits.telegram()truncate_presentation()→adapt_presentation(...)render_presentation()→ delegates toTelegramPresentationRenderer, builds a nativeInlineKeyboardMarkup, and sends it._session.pop_last_presentation()and renders it natively after the text reply, so the text serves as the plain-text fallback and a render failure degrades gracefully.Layer placement
Per the issue's routing analysis, this is wrapper-only: the model +
adapt_presentationalready live in core, and the renderers already live in the wrapper. No new dependencies; backward-compatible (plain-text turns are unaffected).Test plan
tests/unit/test_presentation_wiring.pyverifies the adapter exposes the protocol methods, thatrender_presentationsends an inline keyboard, and that the inbound handler consumes/renders the captured presentation (importorskip("telegram")for optional dep). — 5 passed.test_presentation_adapt.py,test_agent_presentation_reply.py) still pass — 28 passed.Generated with Claude Code
Summary by CodeRabbit