feat(media): support ContextInfo in media options - #931
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughLook, this is straightforward, and things need to work right. ChangesContext Info Support
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: We shipped a field, tight and clean, 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/media.rs (1)
21-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLook, adding fields to public structs is like changing the News Feed algorithm — people notice.
context_infois added as a plain public field across four structs. Since none of them appear to be#[non_exhaustive], any downstream consumer using an exhaustive struct literal instead of..Default::default()breaks on upgrade. Given this crate is pre-1.0 and prior precedent here tolerates breaking API changes without shims, this is likely acceptable — but worth a conscious call rather than an accident.♻️ Consider marking these Options structs non_exhaustive going forward
+#[non_exhaustive] pub struct ImageOptions { ... }Based on learnings: for this repo (pre-1.0, e.g. 0.6.0), "public API renames... are allowed without requiring a deprecated compatibility shim, per the crate's versioning policy" — the same tolerance likely applies to additive breaking changes like this one.
🤖 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/media.rs` around lines 21 - 64, The new public `context_info` fields on `ImageOptions`, `VideoOptions`, `DocumentOptions`, and `AudioOptions` are an additive breaking API change for exhaustive struct literals. If this change is intended, make that contract explicit by marking these option structs as `#[non_exhaustive]` (or otherwise document the breaking change) so downstream users know to construct them with `..Default::default()`. Keep the update aligned with the existing public option types in `src/media.rs` and the crate’s pre-1.0 API policy.Source: Learnings
🤖 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/media.rs`:
- Around line 246-279: Add parity test coverage for the new context_info wiring
in document_message and audio_message. Mirror the existing
image_maps_context_info and video_maps_context_info tests by creating
document_maps_context_info and audio_maps_context_info that build messages via
document_message and audio_message with DocumentOptions and AudioOptions
containing Some(ContextInfo), then assert the resulting document_message and
audio_message payloads preserve context_info.
---
Outside diff comments:
In `@src/media.rs`:
- Around line 21-64: The new public `context_info` fields on `ImageOptions`,
`VideoOptions`, `DocumentOptions`, and `AudioOptions` are an additive breaking
API change for exhaustive struct literals. If this change is intended, make that
contract explicit by marking these option structs as `#[non_exhaustive]` (or
otherwise document the breaking change) so downstream users know to construct
them with `..Default::default()`. Keep the update aligned with the existing
public option types in `src/media.rs` and the crate’s pre-1.0 API policy.
🪄 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 (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f628fa2e-2525-4d1f-b5e4-684608a5ba0c
📒 Files selected for processing (2)
src/features/status.rssrc/media.rs
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/media.rs">
<violation number="1" location="src/media.rs:80">
P3: Tests for `context_info` mapping were added for `image_message` and `video_message`, but `document_message` and `audio_message` received the same wiring without corresponding test coverage. Add parity tests (e.g., `document_maps_context_info` and `audio_maps_context_info`) so a future refactor can't silently drop the field on these message types.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
@arsa0x thanks <3 |
Merging this PR will not alter performance
Comparing Footnotes
|
Summary
Add an optional
context_infofield to all media option structs and forward it to the generated media message.Why
This enables creating quoted/reply media messages directly through the helper API without manually modifying the generated protobuf.
Breaking changes
None.