fix(telegram): prevent panic when splitting messages at UTF-8 char boundaries - #379
fix(telegram): prevent panic when splitting messages at UTF-8 char boundaries#379clicksingh wants to merge 4 commits into
Conversation
…undaries The split_message function could panic when hard-cutting a message at Telegram's 4096-byte limit if the cut point landed inside a multi-byte UTF-8 character (e.g., em dashes '—'). Changed unwrap_or(max_len) to unwrap_or_else() which walks backward from max_len to find the last valid char boundary using is_char_boundary(). Also added handling for edge case where split_at could be 0.
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds native draft-based streaming to TelegramAdapter (HTTP client, sendMessageDraft, per-stream draft state and routing), plumbing for a per-instance Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ❌ 3❌ Failed checks (3 warnings)
✏️ 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/telegram.rs`:
- Around line 1019-1041: The slicing remaining[..max_len] can panic when max_len
falls inside a multi-byte UTF-8 codepoint; modify the split logic in
split_message so you first compute a safe_upper: if
remaining.is_char_boundary(max_len) use max_len, otherwise walk backwards from
max_len to the previous char boundary (same loop already used in the hard-cut
branch) and use that safe_upper for the rfind calls instead of max_len; preserve
the existing fallback hard-cut logic for when rfind yields None, and ensure you
still handle split_at == 0 as before; additionally add unit tests for
split_message that place multi-byte characters (e.g., emojis or non-Latin
scripts) at boundary positions to verify no panics and correct chunking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0df8b449-4c80-4ff9-952c-5de71d91cd76
📒 Files selected for processing (1)
src/messaging/telegram.rs
| // Find split point: prefer newline, then space, then hard-cut. | ||
| // When hard-cutting, ensure we don't split mid-character (UTF-8 safety). | ||
| let split_at = remaining[..max_len] | ||
| .rfind('\n') | ||
| .or_else(|| remaining[..max_len].rfind(' ')) | ||
| .unwrap_or(max_len); | ||
| .unwrap_or_else(|| { | ||
| // Hard-cut: find the last valid char boundary before max_len | ||
| let mut pos = max_len; | ||
| while pos > 0 && !remaining.is_char_boundary(pos) { | ||
| pos -= 1; | ||
| } | ||
| pos | ||
| }); | ||
|
|
||
| // Avoid empty chunks if split_at is 0 (e.g., first char is multi-byte) | ||
| let split_at = if split_at == 0 { | ||
| remaining.char_indices().nth(1).map(|(i, _)| i).unwrap_or(remaining.len()) | ||
| } else { | ||
| split_at | ||
| }; | ||
|
|
||
| chunks.push(remaining[..split_at].to_string()); | ||
| remaining = remaining[split_at..].trim_start(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for split_message tests
rg -n "split_message" --type rust -C 3Repository: spacedriveapp/spacebot
Length of output: 9975
🏁 Script executed:
sed -n '1005,1050p' src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 1623
🏁 Script executed:
fd -t f '\.rs$' -path '*/test*' | head -20Repository: spacedriveapp/spacebot
Length of output: 234
🏁 Script executed:
rg -n '#\[test\]|#\[tokio::test\]' src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 439
🏁 Script executed:
rg -n 'split_message' src/messaging/telegram.rs -A 5 -B 2 | grep -E 'test|#\[|mod tests'Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
sed -n '1280,1360p' src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 2029
🏁 Script executed:
sed -n '1360,1500p' src/messaging/telegram.rs | grep -A 10 -B 2 'split_message'Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
rg -n 'fn.*split_message|mod.*test' src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 149
🏁 Script executed:
grep -n 'split_message' src/messaging/telegram.rs | grep testRepository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
sed -n '1283,1500p' src/messaging/telegram.rs | tail -100Repository: spacedriveapp/spacebot
Length of output: 2963
🏁 Script executed:
wc -l src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 96
🏁 Script executed:
tail -50 src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 1577
🏁 Script executed:
rg -n "split_message" src/messaging/telegram.rs --type rust | grep "#\[test\]" -B 5Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
rg -B 5 -A 10 'fn split_message' src/messaging/*.rs | grep -A 15 '#\[test\]'Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
python3 << 'EOF'
# Check if split_message has any tests by searching the entire file content
with open('src/messaging/telegram.rs', 'r') as f:
content = f.read()
# Find the test module
test_mod_start = content.find('mod tests {')
if test_mod_start != -1:
test_mod_end = content.rfind('}')
test_module = content[test_mod_start:test_mod_end+1]
# Check if split_message is tested
if 'split_message' in test_module:
print("Found split_message test")
# Show context
for i, line in enumerate(test_module.split('\n')):
if 'split_message' in line:
print(f"Line {i}: {line}")
else:
print("NO TESTS FOR split_message FOUND")
print(f"Test module has {len(test_module.split(chr(10)))} lines")
print("Tests are for: markdown_to_telegram_html, strip_html_tags, should_retry_plain_caption")
EOFRepository: spacedriveapp/spacebot
Length of output: 210
remaining[..max_len] slicing can panic if max_len lands mid-UTF-8 character.
The boundary-aware logic only handles the hard-cut fallback. However, both rfind operations on lines 1023–1024 will panic if max_len doesn't align with a valid UTF-8 character boundary, since Rust's &str slicing requires valid boundaries.
Calculate the safe upper bound before the rfind calls:
Proposed fix
- // Find split point: prefer newline, then space, then hard-cut.
- // When hard-cutting, ensure we don't split mid-character (UTF-8 safety).
- let split_at = remaining[..max_len]
+ // Find a safe upper bound that doesn't split a UTF-8 character.
+ let safe_max = {
+ let mut pos = max_len;
+ while pos > 0 && !remaining.is_char_boundary(pos) {
+ pos -= 1;
+ }
+ pos
+ };
+
+ // Find split point: prefer newline, then space, then hard-cut at safe_max.
+ let split_at = remaining[..safe_max]
.rfind('\n')
- .or_else(|| remaining[..max_len].rfind(' '))
- .unwrap_or_else(|| {
- // Hard-cut: find the last valid char boundary before max_len
- let mut pos = max_len;
- while pos > 0 && !remaining.is_char_boundary(pos) {
- pos -= 1;
- }
- pos
- });
+ .or_else(|| remaining[..safe_max].rfind(' '))
+ .unwrap_or(safe_max);There are no tests for split_message. Add UTF-8 edge case coverage (multi-byte characters at boundary positions).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/messaging/telegram.rs` around lines 1019 - 1041, The slicing
remaining[..max_len] can panic when max_len falls inside a multi-byte UTF-8
codepoint; modify the split logic in split_message so you first compute a
safe_upper: if remaining.is_char_boundary(max_len) use max_len, otherwise walk
backwards from max_len to the previous char boundary (same loop already used in
the hard-cut branch) and use that safe_upper for the rfind calls instead of
max_len; preserve the existing fallback hard-cut logic for when rfind yields
None, and ensure you still handle split_at == 0 as before; additionally add unit
tests for split_message that place multi-byte characters (e.g., emojis or
non-Latin scripts) at boundary positions to verify no panics and correct
chunking.
- Add sendMessageDraft method for Telegram Bot API 9.5+ native streaming - Implement draft-based updates for smooth animated text in private chats - Add configuration for enabling/disabling native streaming - Fall back to edit-based streaming for groups/channels - Add draft_id and is_private tracking to ActiveStream struct
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/config/permissions.rs (1)
20-31:⚠️ Potential issue | 🔴 CriticalRestore the
from_configconstructors or update every caller in this PR.This refactor drops
DiscordPermissions::from_config,SlackPermissions::from_config, andTwitchPermissions::from_config, but the provided call sites insrc/main.rsandsrc/config/watcher.rsstill use them. Also, Line 116 is now a bareSelf::from_bindings_for_adapter(...)expression, so this impl block no longer parses.Suggested fix
impl DiscordPermissions { /// Build from the current config's discord settings and bindings. + pub fn from_config(discord: &DiscordConfig, bindings: &[Binding]) -> Self { + Self::from_bindings_for_adapter( + discord.dm_allowed_users.clone(), + discord.allow_bot_messages, + bindings, + None, + ) + } /// Build permissions for a named Discord adapter instance. pub fn from_instance_config(instance: &DiscordInstanceConfig, bindings: &[Binding]) -> Self { Self::from_bindings_for_adapter( instance.dm_allowed_users.clone(), @@ impl SlackPermissions { /// Build from the current config's slack settings and bindings. - Self::from_bindings_for_adapter(slack.dm_allowed_users.clone(), bindings, None) + pub fn from_config(slack: &SlackConfig, bindings: &[Binding]) -> Self { + Self::from_bindings_for_adapter(slack.dm_allowed_users.clone(), bindings, None) + } @@ impl TwitchPermissions { /// Build from the current config's twitch settings and bindings. + pub fn from_config(_twitch: &TwitchConfig, bindings: &[Binding]) -> Self { + Self::from_bindings_for_adapter(bindings, None) + } /// Build permissions for a named Twitch adapter instance. pub fn from_instance_config(instance: &TwitchInstanceConfig, bindings: &[Binding]) -> Self { Self::from_bindings_for_adapter(bindings, Some(instance.name.as_str())) }Also applies to: 114-125, 279-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/permissions.rs` around lines 20 - 31, The refactor removed the convenience constructors (DiscordPermissions::from_config, SlackPermissions::from_config, TwitchPermissions::from_config) but call sites still use them and one impl ends with a bare expression Self::from_bindings_for_adapter(...) which makes the block not parse; either restore each from_config constructor to call from_bindings_for_adapter (e.g., recreate DiscordPermissions::from_config that wraps from_bindings_for_adapter with the existing instance config defaults) or update every caller (in the main and watcher modules) to call from_instance_config or from_bindings_for_adapter directly; also ensure each impl method returns the value (use "return Self::from_bindings_for_adapter(...)" or simply end with that expression in a function body, not as a stray statement) so the impl block compiles.
🤖 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/telegram.rs`:
- Around line 89-90: The http_client is created with Client::new() which has no
timeout; update the construction used for the http_client field (where
Client::new() is called) to build a reqwest::Client with a request timeout
(e.g., Client::builder().timeout(Duration::from_secs(30)).build() ) and add the
necessary import for std::time::Duration (or wire the timeout from
configuration) so that requests made in the streaming path (the code awaiting
the Telegram API) will fail fast instead of hanging indefinitely.
---
Outside diff comments:
In `@src/config/permissions.rs`:
- Around line 20-31: The refactor removed the convenience constructors
(DiscordPermissions::from_config, SlackPermissions::from_config,
TwitchPermissions::from_config) but call sites still use them and one impl ends
with a bare expression Self::from_bindings_for_adapter(...) which makes the
block not parse; either restore each from_config constructor to call
from_bindings_for_adapter (e.g., recreate DiscordPermissions::from_config that
wraps from_bindings_for_adapter with the existing instance config defaults) or
update every caller (in the main and watcher modules) to call
from_instance_config or from_bindings_for_adapter directly; also ensure each
impl method returns the value (use "return Self::from_bindings_for_adapter(...)"
or simply end with that expression in a function body, not as a stray statement)
so the impl block compiles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 633e26e7-ce1e-48b5-a784-1ddcf5b664dc
📒 Files selected for processing (5)
src/config/load.rssrc/config/permissions.rssrc/config/toml_schema.rssrc/config/types.rssrc/messaging/telegram.rs
| http_client: Client::new(), | ||
| token, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && find . -name "telegram.rs" -type fRepository: spacedriveapp/spacebot
Length of output: 121
🏁 Script executed:
cd /repo && head -n 150 src/messaging/telegram.rs | tail -n 70Repository: spacedriveapp/spacebot
Length of output: 121
🏁 Script executed:
cd /repo && sed -n '1,20p' src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 121
🏁 Script executed:
cd /repo && sed -n '80,100p' src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 121
🏁 Script executed:
cd /repo && sed -n '100,160p' src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 121
🏁 Script executed:
cd /repo && sed -n '1170,1200p' src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 121
🏁 Script executed:
find . -name "telegram.rs" -type f 2>/dev/nullRepository: spacedriveapp/spacebot
Length of output: 93
🏁 Script executed:
cat -n src/messaging/telegram.rs | sed -n '80,160p'Repository: spacedriveapp/spacebot
Length of output: 3343
🏁 Script executed:
wc -l src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 96
🏁 Script executed:
grep -n "Client::new()" src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 108
🏁 Script executed:
grep -n "Client::builder()" src/messaging/telegram.rsRepository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
grep -rn "Client::builder()" src/ --include="*.rs" | head -20Repository: spacedriveapp/spacebot
Length of output: 706
🏁 Script executed:
grep -rn "timeout" src/ --include="*.rs" | grep -i "duration\|client" | head -20Repository: spacedriveapp/spacebot
Length of output: 1992
🏁 Script executed:
rg "reqwest.*Client" src/ -A 3 | head -40Repository: spacedriveapp/spacebot
Length of output: 1814
Add a timeout to the http_client. Line 89 constructs a Client::new() without timeout configuration, and lines 129–133 await it directly on the streaming path. If the Telegram API becomes unresponsive, this request will hang indefinitely instead of failing fast.
Suggested fix
- http_client: Client::new(),
+ http_client: Client::builder()
+ .timeout(std::time::Duration::from_secs(10))
+ .build()
+ .expect("hardcoded reqwest client config should be valid"),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| http_client: Client::new(), | |
| token, | |
| http_client: Client::builder() | |
| .timeout(std::time::Duration::from_secs(10)) | |
| .build() | |
| .expect("hardcoded reqwest client config should be valid"), | |
| token, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/messaging/telegram.rs` around lines 89 - 90, The http_client is created
with Client::new() which has no timeout; update the construction used for the
http_client field (where Client::new() is called) to build a reqwest::Client
with a request timeout (e.g.,
Client::builder().timeout(Duration::from_secs(30)).build() ) and add the
necessary import for std::time::Duration (or wire the timeout from
configuration) so that requests made in the streaming path (the code awaiting
the Telegram API) will fail fast instead of hanging indefinitely.
…d, Slack, Twitch Accidentally stripped in the native streaming PR (27b1084). The function signatures for DiscordPermissions::from_config, SlackPermissions::from_config, and TwitchPermissions::from_config were removed leaving orphaned function bodies that caused compile errors.
1 participant
Closing — the UTF-8 boundary fix for split_message is already in upstream main (9eb2ba1, f223009). The permissions.rs changes in this PR accidentally stripped from_config constructors and were breaking the build. No longer needed.