fix(discord): ignore invalid poll payloads#388
fix(discord): ignore invalid poll payloads#388deanfluencebot wants to merge 2 commits intospacedriveapp:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughIntroduces poll validation and normalization in Discord messaging: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/messaging/discord.rs`:
- Around line 1179-1197: The validation currently counts non-empty answers
across the whole vector but build_poll only serializes the first 10 raw answers,
so edge cases like ["Yes", "", "No"] or a second non-empty answer past index 9
slip through; update validation to use the same normalized/trimming/filtering
logic as build_poll by adding or using a helper normalized_poll_answers(poll:
&crate::Poll) -> Vec<String> that trims and filters out empty answers and then
validate against that normalized list (and only consider the first 10 answers
exactly as build_poll will), change is_valid_poll to call that helper, update
build_poll to also call the same helper, and add a regression test asserting
that polls with empty strings and answers beyond the 10th are handled
consistently (rich reply accepted only when normalized answers >= 2 and
serialization uses the first 10 normalized answers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d7bbd5d-8db8-4894-ac12-bd8ff4dce17a
📒 Files selected for processing (1)
src/messaging/discord.rs
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
src/messaging/discord.rs
Outdated
|
|
||
| fn is_valid_poll(poll: &crate::Poll) -> bool { | ||
| let question = poll.question.trim(); | ||
| let answer_count = poll |
There was a problem hiding this comment.
Nice. One edge case: build_poll only sends the first 10 answers, but is_valid_poll counts across all of them; that can accept a poll that still gets truncated to <2 usable answers.
| let answer_count = poll | |
| let answer_count = poll | |
| .answers | |
| .iter() | |
| .take(10) | |
| .filter(|answer| !answer.trim().is_empty()) | |
| .count(); |
There was a problem hiding this comment.
Addressed in c42dae2. The validation now uses the same normalized answer set as serialization via normalized_poll_answers, so we no longer count answers that would be dropped or skipped before sending to Discord.
1 participant
Summary
Why
Some reply payloads can include an empty poll stub even when the intent is just a normal text reply. Discord rejects those malformed polls, which causes the whole rich message send to fail.
This change validates poll payloads before attaching them to Discord messages. Invalid polls are skipped instead of breaking delivery.
Testing
Note
This PR adds poll validation to the Discord adapter. A new
is_valid_poll()function checks that polls have a non-empty question and at least 2 answers, filtering them at the point of attachment in two locations. Invalid polls are silently skipped with a debug log. The unit test covers empty, single-answer, and valid poll payloads.Written by Tembo for commit badfb61. This will update automatically on new commits.