Conversation
FossilOrigin-Name: a2c4ae47171cdd4e4039b43c474d15bdfb0d694a9ad18bc2adc2efaa4bfa642a
FossilOrigin-Name: 3b65f27fe629c7181994e74b10febda597b9367f3930c160602ab506b2cfb9e7
FossilOrigin-Name: 8f4dcf5e9d2476313c3602a66578c8be839ab93fdea32de1e14b2297bc1b0995
FossilOrigin-Name: 9c4f09193a56f530d70a707d1e3dd33060fc43841b0c42bd9524fe19a64881ee
FossilOrigin-Name: fabcca1eafc9428eb800012313eadbdfd08533a35ee12e9ed6313585569fb74c
|
Warning Review limit reached
Next review available in: 54 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR introduces a unified Telegram bot abstraction, TTL-backed per-owner feed caching, HTML message handling, callback query support, updated feed delivery, and new project metadata and dependencies. Command, database, Core, entry-point, and CI integrations are updated for the new architecture. ChangesTelegram and feed interaction refactor
Estimated code review effort: 4 (Complex) | ~50 minutes Sequence Diagram(s)sequenceDiagram
participant User as Telegram user
participant Handler as answer_cb
participant Core as Core
participant Database as Database
participant Tg as Tg
participant Telegram as Telegram API
User->>Telegram: Click inline callback
Telegram->>Handler: Deliver CallbackQuery
Handler->>Core: Resolve owner and get_feeds
Core->>Database: Load feeds on cache miss
Database-->>Core: Return feed rows
Core-->>Handler: Return feed list
Handler->>Tg: update_message
Tg->>Telegram: Edit message with feed keyboard
Telegram-->>User: Display updated menu
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @kworr. * #8 (comment) The following files were modified: * `src/command.rs` * `src/core.rs` * `src/main.rs` * `src/sql.rs` * `src/tg_bot.rs`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/sql.rs (1)
212-212: Fix Clippy error: too many arguments (8/7).The pipeline is failing because this function exceeds the default argument limit. Consider grouping related parameters into a struct to improve maintainability and satisfy the linter.
Suggested fix options
Option 1: Add an allow attribute to suppress the warning:
+ #[allow(clippy::too_many_arguments)] pub async fn update <I> (&mut self, update: Option<i32>, channel: &str, channel_id: i64, url: &str, iv_hash: Option<&str>, url_re: Option<&str>, owner: I) -> Result<&str>Option 2 (recommended): Introduce a parameter struct:
pub struct SourceUpdate<'a> { pub source_id: Option<i32>, pub channel: &'a str, pub channel_id: i64, pub url: &'a str, pub iv_hash: Option<&'a str>, pub url_re: Option<&'a str>, }src/core.rs (1)
357-386: Typo in documentation: "erro" should be "error".Line 361 has a truncated word in the doc comment.
Suggested fix
- /// which is also reported to the chat. + /// which is also reported to the chat. Unknown commands produce an errorsrc/command.rs (1)
45-79: Typo: "exacly" should be "exactly".Line 75 contains a spelling mistake in the user-facing error message.
Suggested fix
- "This command needs exacly one number.".into() + "This command needs exactly one number.".into()
🧹 Nitpick comments (1)
src/main.rs (1)
18-24: Consider propagating errors instead of usingunwrap().Using
unwrap()on line 20 will cause a panic without useful context ifasync_main()fails. The error information fromstacked_errorswould be lost.Suggested fix
fn main () -> Result<()> { smol::block_on(Compat::new(async { - async_main().await.unwrap(); + async_main().await })); - - Ok(()) }Alternatively, if you need to keep the current structure:
fn main () -> Result<()> { smol::block_on(Compat::new(async { - async_main().await.unwrap(); - })); - - Ok(()) + async_main().await + })) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Cargo.tomlsrc/command.rssrc/core.rssrc/main.rssrc/sql.rssrc/tg_bot.rs
💤 Files with no reviewable changes (1)
- Cargo.toml
🧰 Additional context used
🧬 Code graph analysis (1)
src/command.rs (1)
src/sql.rs (1)
update(212-252)
🪛 GitHub Actions: rust-ci
src/sql.rs
[error] 212-212: Clippy error: this function has too many arguments (8/7) in update method. Consider reducing arguments or add #[allow(clippy::too_many_arguments)]. Command: cargo clippy --all-targets --all-features -- -D warnings.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run rust-clippy analyzing and tests
- GitHub Check: Analyze (rust)
🔇 Additional comments (10)
src/sql.rs (1)
153-169: LGTM! Clean simplification of the exists query.The consolidation of error handling using
try_get(...).stack_err(...)is more idiomatic and reduces boilerplate compared to manual extraction with separate error paths.src/tg_bot.rs (3)
25-47: LGTM! Well-documented constructor with clear configuration requirements.The documentation clearly specifies the required configuration keys, and the implementation correctly initialises all components.
49-53: LGTM!Documentation accurately describes the method behaviour and return value.
65-78: LGTM! Good use of generics for flexibility.Making
with_ownergeneric overO: Into<i64>allows acceptingUserPeerIdand other integer-like types directly, reducing boilerplate at call sites.src/main.rs (1)
26-41: LGTM! Clean async initialisation.The configuration setup with a sensible default for
api_gatewayand the core/LongPoll initialisation are well-structured.src/core.rs (2)
128-128: LGTM!Clarified documentation for the
ownerfield.
301-343: LGTM! Background autofetch implementation looks correct.The logic correctly spawns checks for overdue sources and computes the minimum delay until the next fetch. Cloning the
Corewith the appropriate owner for each spawned task ensures proper isolation.src/command.rs (3)
26-31: LGTM!Simple and clear informational message handler.
33-43: LGTM!Correctly retrieves sender ID and fetches their subscription list.
81-188: LGTM! Comprehensive validation and permission checks.The update function properly validates:
- Channel username format
- URL scheme (http/https only)
- IV hash format (14 hex digits)
- Sed-style regex syntax
- Admin permissions for both bot and user
The flow storing the result before sending (lines 186-187) is cleaner than inline execution.
FossilOrigin-Name: 13265e769714ff186697e13ac212a530fd2a8ec12b9d9c0d537e194e9e807a86
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/core.rs (1)
377-384:⚠️ Potential issue | 🟡 MinorError message not escaped for MarkdownV2.
The
{err}content is not passed through theencodefunction, unlike the similar error handling inautofetch(line 331) which does useencode(&err.to_string()). If the error message contains MarkdownV2 special characters (e.g.,[,],_,*), the message may fail to send or render incorrectly.🔧 Proposed fix
if let Err(err) = res - && let Err(err2) = self.tg.send(format!("\\#error\n```\n{err}\n```"), + && let Err(err2) = self.tg.send(format!("\\#error\n```\n{}\n```", encode(&err.to_string())), Some(msg.chat.get_id()), Some(ParseMode::MarkdownV2) ).await
🧹 Nitpick comments (2)
src/core.rs (1)
385-385: TODO comment acknowledged.Consider creating a tracking issue if debug logging for skipped updates is intended to be implemented.
Would you like me to open an issue to track the debug logging enhancement?
src/tg_bot.rs (1)
29-35: Unused parameter and hardcoded value inget_kb.The
cbparameter is never used; the function always serialises a hardcodedCallback::List(0,"xxx".to_owned())instead. This appears to be incomplete scaffold code. Additionally, the function is private and currently unused, which will trigger compiler warnings.If this is intentional placeholder code, consider either:
- Prefixing the parameter with
_to suppress the warning:_cb: &Callback- Actually using the parameter, or
- Removing the function until it's needed
🔧 Proposed fix if the parameter should be used
fn get_kb (cb: &Callback) -> Result<InlineKeyboardMarkup> { let mark = InlineKeyboardMarkup::from(vec![vec![ InlineKeyboardButton::for_callback_data("1", - toml::to_string(&Callback::List(0,"xxx".to_owned())).stack()?), + toml::to_string(cb).stack()?), ]]); Ok(mark) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a06f62b-06f7-4db0-a7c2-31c13da3c86d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
Cargo.tomlsrc/command.rssrc/core.rssrc/tg_bot.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/command.rs
FossilOrigin-Name: 9adc69d5142d7250899a8481d5a8a8a50a115d11568b90973d0063f71c83a920
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/command.rs (1)
53-60: Consider using a user-friendly display format instead of Debug formatting.Line 58 uses
{:?}(Debug format) formsg.sender, which may produce internal representation details that aren't user-friendly. Consider implementing or using a display-friendly format for the sender.💡 Suggested improvement
- core.tg.send(MyMessage::html_to_kb(format!("List of feeds owned by {:?}:", msg.sender), msg.chat.get_id(), kb)).await.stack()?; + core.tg.send(MyMessage::html_to_kb(format!("List of feeds owned by you:"), msg.chat.get_id(), kb)).await.stack()?;Alternatively, extract a meaningful identifier (e.g., username or user ID) from
msg.senderfor display.src/core.rs (2)
124-129: Consider removing or using the underscore-prefixed fields.The fields
_title,_authors, and_summaryare populated but never read. If these are intended for future use, consider adding a TODO comment. Otherwise, removing them would reduce unnecessary allocations during feed parsing.
365-380: Database connection is acquired unnecessarily on cache hit.The DB connection at line 366 is acquired before checking the cache. When there's a cache hit (line 379), the connection is never used. Consider restructuring to acquire the connection only on cache miss.
♻️ Proposed fix
pub async fn get_feeds (&self, owner: i64) -> Result<Arc<Mutex<HashMap<i32, String>>>> { - let mut conn = self.db.begin().await.stack()?; let mut feeds = self.feeds.lock_arc().await; Ok(match feeds.get(&owner) { None => { + let mut conn = self.db.begin().await.stack()?; let feed_list = conn.get_feeds(owner).await.stack()?; let mut map = HashMap::new(); for feed in feed_list { map.insert(feed.source_id, feed.channel); }; let res = Arc::new(Mutex::new(map)); feeds.insert(owner, res.clone(), Duration::from_secs(60 * 60 * 3)); res }, Some(res) => res.clone(), }) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f9c27edd-1b74-4b76-b1da-c5bffe4bef6f
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
Cargo.tomlsrc/command.rssrc/core.rssrc/main.rssrc/sql.rssrc/tg_bot.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- Cargo.toml
- src/tg_bot.rs
- src/main.rs
| /// Adds feed to cached list | ||
| pub async fn add_feed (&self, owner: i64, source_id: i32, channel: String) -> Result<()> { | ||
| let mut inserted = true; | ||
| { | ||
| let mut feeds = self.feeds.lock_arc().await; | ||
| if let Some(feed) = feeds.get_mut(&owner) { | ||
| let mut feed = feed.lock_arc().await; | ||
| feed.insert(source_id, channel); | ||
| } else { | ||
| inserted = false; | ||
| } | ||
| } | ||
| if !inserted { | ||
| self.get_feeds(owner).await.stack()?; | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
Cache miss in add_feed doesn't actually add the new feed.
When the cache doesn't contain an entry for the owner (line 391-392), the code sets inserted = false and then calls get_feeds(owner) at line 396. However, get_feeds only loads existing feeds from the database — it doesn't insert the new (source_id, channel) pair that was passed to add_feed. The newly added feed is lost on cache miss.
🐛 Proposed fix
pub async fn add_feed (&self, owner: i64, source_id: i32, channel: String) -> Result<()> {
- let mut inserted = true;
- {
- let mut feeds = self.feeds.lock_arc().await;
- if let Some(feed) = feeds.get_mut(&owner) {
- let mut feed = feed.lock_arc().await;
- feed.insert(source_id, channel);
- } else {
- inserted = false;
- }
- }
- if !inserted {
- self.get_feeds(owner).await.stack()?;
- }
+ let feeds_map = self.get_feeds(owner).await.stack()?;
+ let mut feed = feeds_map.lock_arc().await;
+ feed.insert(source_id, channel);
Ok(())
}📝 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.
| /// Adds feed to cached list | |
| pub async fn add_feed (&self, owner: i64, source_id: i32, channel: String) -> Result<()> { | |
| let mut inserted = true; | |
| { | |
| let mut feeds = self.feeds.lock_arc().await; | |
| if let Some(feed) = feeds.get_mut(&owner) { | |
| let mut feed = feed.lock_arc().await; | |
| feed.insert(source_id, channel); | |
| } else { | |
| inserted = false; | |
| } | |
| } | |
| if !inserted { | |
| self.get_feeds(owner).await.stack()?; | |
| } | |
| Ok(()) | |
| /// Adds feed to cached list | |
| pub async fn add_feed (&self, owner: i64, source_id: i32, channel: String) -> Result<()> { | |
| let feeds_map = self.get_feeds(owner).await.stack()?; | |
| let mut feed = feeds_map.lock_arc().await; | |
| feed.insert(source_id, channel); | |
| Ok(()) | |
| } |
FossilOrigin-Name: 374eadef45f782806bea6e38e9949b1a7cd3277cc31e04555d6079a550a744ea
There was a problem hiding this comment.
Actionable comments posted: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4239a00d-1cfd-494e-abc1-70f8fbddd90d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
src/command.rssrc/core.rssrc/sql.rssrc/tg_bot.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/sql.rs
- src/command.rs
- src/core.rs
…callbacks FossilOrigin-Name: 3fd8c40aa8d86f47c2d227490ecc7fb44b69ebba88edf35b79aa640ac79496a4
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/tg_bot.rs (1)
92-108:⚠️ Potential issue | 🟠 MajorPagination logic has off-by-one errors.
The current pagination has issues:
- For a "long" list,
start = page * 5 + 1andend = 5 + page * 5, but the break conditioni > endmeans 6 items are shown (indices start to end+1 inclusive).- For page 0: start=1, end=5, items where i ∈ {1,2,3,4,5,6} are shown before
i > 5breaks.- The non-long path uses
(0, 6)which shows 7 items (0-6 inclusive).Consider using
skip/takeon a collectedVecfor clearer pagination:🐛 Proposed fix using skip/take
Callback::List(_, name, page) => { - let mut kb = vec![]; let feeds = feeds.lock_arc().await; + let items: Vec<_> = feeds.iter().collect(); let long = feeds.len() > 6; - let (start, end) = if long { - (page * 5 + 1, 5 + page * 5) - } else { - (0, 6) - }; - let mut i = 0; + let page_size = 5; + let mut kb = vec![]; if name.is_empty() { - for (id, name) in feeds.iter() { - i += 1; - if i < start { continue } + for (id, name) in items.iter().skip((*page as usize) * page_size).take(page_size) { kb.push(vec![ InlineKeyboardButton::for_callback_data( format!("{}. {}", id, name), Callback::edit(name).to_string()), ]); - if i > end { break } }
🧹 Nitpick comments (3)
src/tg_bot.rs (2)
84-87: XXX comments indicate incomplete features.Lines 84 and 156 contain
XXXcomments indicating unimplemented functionality for "edit" and "Add new channel". These should be tracked or addressed before merging.Would you like me to open issues to track these incomplete features?
Also applies to: 154-157
232-234: Zero retries may cause transient failures.
with_max_retries(0)disables automatic retries on the Telegram client. Transient network issues or rate limits could cause failures that would otherwise succeed with a small retry count. Consider allowing at least 1-2 retries.src/core.rs (1)
118-123: Unused struct fields prefixed with underscore.The
Poststruct fields_title,_authors, and_summaryare prefixed with underscores to suppress unused warnings. If these are intended for future use, consider tracking via a TODO or removing them until needed to reduce maintenance burden.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5731a8d5-474b-4e6e-a81f-298ca46822e5
📒 Files selected for processing (5)
.github/workflows/rust-clippy.ymlsrc/command.rssrc/core.rssrc/sql.rssrc/tg_bot.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sql.rs
| pub async fn cb (&self, query: &CallbackQuery, cb: &str) -> Result<()> { | ||
| let cb: Callback = toml::from_str(cb).stack()?; | ||
| todo!(); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
cb() method contains todo!() that will panic at runtime.
The callback handler at line 417 contains todo!() which will cause a panic when a user presses an inline keyboard button. This will crash the handler and leave callbacks unanswered. Either implement the callback handling or return an appropriate error.
🐛 Proposed fix to return not-implemented error
pub async fn cb (&self, query: &CallbackQuery, cb: &str) -> Result<()> {
let cb: Callback = toml::from_str(cb).stack()?;
- todo!();
- Ok(())
+ bail!("Callback handling not yet implemented for {:?}", cb);
}📝 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.
| pub async fn cb (&self, query: &CallbackQuery, cb: &str) -> Result<()> { | |
| let cb: Callback = toml::from_str(cb).stack()?; | |
| todo!(); | |
| Ok(()) | |
| } | |
| pub async fn cb (&self, query: &CallbackQuery, cb: &str) -> Result<()> { | |
| let cb: Callback = toml::from_str(cb).stack()?; | |
| bail!("Callback handling not yet implemented for {:?}", cb); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/tg_bot.rs (1)
145-146:⚠️ Potential issue | 🟡 MinorPotential
u8overflow on page increment.
page + 1overflows ifpageis 255. Usepage.saturating_add(1)or clamp to a maximum page.🛡️ Proposed fix
InlineKeyboardButton::for_callback_data(">>", - Callback::list("", page + 1).to_string()), + Callback::list("", page.saturating_add(1)).to_string()),
🧹 Nitpick comments (4)
src/tg_bot.rs (4)
84-87: MultipleXXXcomments indicate incomplete implementation.Lines 84, 120, and 156 contain
XXXmarkers for unfinished functionality (edit handling, callback payload, and "Add new channel"). Ensure these are tracked as follow-up work items before merging or consider gating the incomplete paths.Would you like me to open issues to track these incomplete implementations?
Also applies to: 120-120, 156-156
195-208:req()never returnsErr; consider simplifying the return type.The method always returns
Ok(...). If no error paths are anticipated, returningSendMessagedirectly would be clearer. Alternatively, if future variants might fail, the current signature is fine—just note it for now.♻️ Optional simplification
- fn req (&self, tg: &Tg) -> Result<SendMessage> { - Ok(match self { + fn req (&self, tg: &Tg) -> SendMessage { + match self { MyMessage::Html { text } => SendMessage::new(tg.owner, text.as_ref()) .with_parse_mode(ParseMode::Html), // ... remaining arms - }) + } }
232-234:with_max_retries(0)disables automatic retries.Transient network errors or rate-limit responses will not be retried. If intentional (e.g., to fail fast and handle retries at a higher level), this is fine; otherwise, a small retry count (e.g., 2–3) may improve resilience.
251-256:answer_cbtakes ownership ofidandtextunnecessarily.The method could accept
&strreferences and let the API call handle conversion, avoiding allocation when the caller already has ownedStrings that could be moved. However, this is minor given the low frequency of callback answers.♻️ Alternative signature
- pub async fn answer_cb (&self, id: String, text: String) -> Result<bool> { + pub async fn answer_cb (&self, id: impl Into<String>, text: impl Into<String>) -> Result<bool> { self.client.execute( - AnswerCallbackQuery::new(id) - .with_text(text) + AnswerCallbackQuery::new(id.into()) + .with_text(text.into()) ).await.stack() }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c5e2974-9004-4b02-a426-3dd12b70ba3d
📒 Files selected for processing (5)
.github/workflows/rust-clippy.ymlsrc/command.rssrc/core.rssrc/sql.rssrc/tg_bot.rs
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/rust-clippy.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/sql.rs
- src/command.rs
- src/core.rs
FossilOrigin-Name: be0b8602d10321789e21c663db166e0ade9df9115863d55683af9096d08233a1
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
src/core.rs (1)
428-467:handlestill advertises MarkdownV2 in its docs.Minor doc-drift nit: the rustdoc on Line 426 says errors are sent back "using MarkdownV2 formatting", but the implementation now uses
MyMessage::html_to(...)(HTML parse mode). Please update the doc comment to match.src/tg_bot.rs (1)
283-304:update_messagecan loop indefinitely if Telegram requests retries withretry_after≤ 60.The loop only exits when
delay > 60or a non-Responseerror occurs. If Telegram repeatedly returnsretry_after≤ 60, the function will retry indefinitely whilst blocking the caller and its handler task. Consider capping either the number of retries or the total elapsed time.Regarding the type/signedness concern:
retry_after()returnsOption<u32>orOption<i32>, both of which are safely and implicitly converted byDuration::from_secs(delay). The comparison and duration construction are consistent and correct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: af1a2fa5-88c7-424b-a0d9-05bfc42f7125
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/rust-clippy.ymlCargo.tomlsrc/command.rssrc/core.rssrc/tg_bot.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/rust-clippy.yml
- Cargo.toml
| "/delete" => { | ||
| let res = conn.delete(number, sender).await.stack()?; | ||
| core.rm_feed(sender.into(), &number).await.stack()?; | ||
| res | ||
| } |
There was a problem hiding this comment.
/delete updates the cache even when no row was deleted.
conn.delete(...) (see src/sql.rs:128-137) can return "No data found found." with rows_affected() == 0, yet core.rm_feed(sender.into(), &number) is still called unconditionally and will evict the entry from the per-owner cache (or trigger a full reload). Consider only invoking rm_feed when the delete actually removed a row, e.g. by returning the row count from Db::delete and branching on it.
There was a problem hiding this comment.
I'm not sure about this one. Database and cache are loosely coupled, whatever happens in database still has to be reflected in cache. The code as it is ensures entry will be deleted from both database and cache regardless of their state.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check rm_feed implementation
ast-grep --pattern 'pub async fn rm_feed($$$) {
$$$
}'
# Check conn.delete implementation
rg -n -A 15 'pub async fn delete' src/sql.rs
Length of output: 714
🏁 Script executed:
#!/bin/bash
# Check rm_feed implementation in core.rs
rg -n -A 20 'pub async fn rm_feed' src/core.rs
Length of output: 695
@kworr, that's a fair point and the implementation backs it up. Looking at rm_feed (core.rs Lines 397–412):
- If the owner's entry is found in the cache, it removes
source_idfrom theFeedList— correct for both "row was deleted" and the stale-cache case where the DB already didn't have it. - If the owner's entry is not found in the cache at all (
!dropped), it does a full reload from the DB — which would also reflect the deletion if it happened.
So the "loosely coupled" design is intentional and the cache will always end up consistent regardless of what the DB reported.
The one minor inefficiency worth being aware of: when source_id doesn't exist in either the DB or the cache (e.g. the user passes a wrong ID), rm_feed falls into the !dropped branch and triggers a full DB reload unnecessarily. This is harmless but slightly wasteful. If that's acceptable in your workload, the current code is fine.
I'll withdraw the original concern given this is a deliberate design choice.
✏️ Learnings added
Learnt from: kworr
Repo: kworr/rsstg PR: 8
File: src/command.rs:89-93
Timestamp: 2026-04-18T19:20:11.585Z
Learning: In `src/command.rs` (Rust), the `/delete` command calls `core.rm_feed()` unconditionally after `conn.delete()`, even when no DB row was removed. This is intentional: the database and cache are loosely coupled, and `rm_feed` ensures cache consistency regardless of DB state. `rm_feed` falls back to a full DB reload only when the owner entry is absent from the cache entirely. This is an accepted design trade-off in this codebase.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| let mut conn = core.db.begin().await.stack()?; | ||
| core.send(conn.update(source_id, channel, channel_id, url, iv_hash, url_re, sender).await.stack()?, Some(msg.chat.get_id()), None).await.stack()?; | ||
| let update = conn.update(source_id, channel, channel_id, url, iv_hash, url_re, sender).await.stack()?; | ||
| core.tg.send(MyMessage::html_to(update, msg.chat.get_id())).await.stack()?; | ||
| if command == "/add" { | ||
| if let Some(new_record) = conn.get_one_name(sender, channel).await.stack()? { | ||
| core.add_feed(sender.into(), new_record.source_id, new_record.channel).await.stack()?; | ||
| } else { | ||
| bail!("Failed to read data on freshly inserted source."); | ||
| } | ||
| }; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Success is reported to the user before the follow-up read/cache-update can fail.
For /add, core.tg.send(...) on Line 201 delivers the "Channel added." reply to the user. Only afterwards do we call conn.get_one_name(...) and core.add_feed(...). If get_one_name returns None (or errors), bail! fires and the caller in Core::handle emits a second message #error<pre>Failed to read data on freshly inserted source.</pre>. That produces contradictory UX (success + error) and, because conn is a Db::begin() transaction that is never explicitly committed here, the bail! path drops conn before commit and the insert is rolled back — yet we already told the user it was added.
Additionally, the update reply string from sql.rs may be "Duplicate key." or "Database error." (i.e. the row was not inserted), and the code still proceeds to get_one_name + add_feed. In the duplicate-key case this will silently refresh/overwrite cache entries that weren't actually inserted in this transaction.
Please commit the transaction, then run get_one_name/add_feed, and only then send the success reply — and gate the post-insert work on update actually reporting success.
| pub async fn answer_cb (core: &Core, query: &CallbackQuery, cb: &str) -> Result<()> { | ||
| let cb: Callback = toml::from_str(cb).stack()?; | ||
| let sender = &query.from; | ||
| //let mut conn = core.db.begin().await.stack()?; | ||
| let text = "Sample".to_owned(); | ||
| if let Some(msg) = &query.message { | ||
| match msg { | ||
| MaybeInaccessibleMessage::Message(message) => { | ||
| if let Some(owner) = message.sender.get_user() | ||
| && sender == owner | ||
| { | ||
| let feeds = core.get_feeds(owner.id.into()).await.stack()?; | ||
| core.tg.update_message(message.chat.get_id().into(), message.id, text, &feeds, cb).await?; | ||
| } else { | ||
| core.tg.send(MyMessage::html(format!("Can't identify request sender:<br><pre>{:?}</pre>", message))).await.stack()?; | ||
| } | ||
| }, | ||
| MaybeInaccessibleMessage::InaccessibleMessage(message) => { | ||
| let sender: i64 = sender.id.into(); | ||
| if let Chat::Private(priv_chat) = &message.chat | ||
| && priv_chat.id == sender | ||
| { | ||
| let feeds = core.get_feeds(priv_chat.id.into()).await.stack()?; | ||
| core.tg.update_message(message.chat.get_id().into(), message.message_id, text, &feeds, cb).await?; | ||
| } else { | ||
| core.tg.send(MyMessage::html(format!("Can't identify request sender:<br><pre>{:?}</pre>", message))).await.stack()?; | ||
| } | ||
| }, | ||
| }; | ||
| }; | ||
| Ok(()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether command::answer_cb has any caller.
rg -nP '\banswer_cb\s*\(' --type=rust -C2Repository: kworr/rsstg
Length of output: 817
🏁 Script executed:
# Check Core::cb implementation to confirm it's the stub mentioned
rg -A 15 'fn cb\s*\(' src/core.rs | head -40Repository: kworr/rsstg
Length of output: 787
🏁 Script executed:
# Search for any indirect references to answer_cb or macro-based dispatch
rg 'answer_cb' src/ --type=rustRepository: kworr/rsstg
Length of output: 331
🌐 Web query:
Telegram Bot API HTML parse mode supported tags
💡 Result:
The Telegram Bot API HTML parse mode supports the following tags: - bold, bold - italic, italic - underline, underline - strikethrough, strikethrough, strikethrough - spoiler, spoiler - inline URL - inline mention of a user - 👍 - inline fixed-width code -
pre-formatted fixed-width code block-
pre-formatted fixed-width code block written in the Python programming languageBlock quotationNotes: - Nested formatting is supported with restrictions (e.g., bold/italic can nest inside others except pre/code). - All <, >, & not part of tags/entities must be escaped as <, >, &. - Only named entities &, <, >, " are supported; numerical entities work. - Programming language syntax highlighting in pre/code (see supported languages). - No changes to HTML tags in recent updates (up to Bot API 9.6 as of April 2026); recent additions like date_time are entities, not new tags. - Only listed tags supported; others ignored or error.
Citations:
- 1: https://tgrm.oss.hagever.com/types/Telegram.ParseMode.html
- 2: https://core.telegram.org/bots/update56kabdkb12ibuisabdubodbasbdaosd
- 3: https://pytba.readthedocs.io/en/4.29.0/formatting.html
command::answer_cb is dead code and contains multiple unresolved issues.
This handler appears unreachable. Core::handle dispatches UpdateType::CallbackQuery to self.cb(&query, cb) (which is a stub containing only todo!()), not to command::answer_cb. No callers to this function exist anywhere in the codebase. Either wire it up in Core::handle to replace the stub, or delete it to prevent bit-rot.
If this function is retained, several bugs must be fixed:
let text = "Sample".to_owned();on line 216 is a placeholder. It will be shown to end users if the handler runs.- Lines 226 and 237 use
<br>inside HTML, but the Telegram Bot API does not support this tag. Supported tags are only:<b>,<i>,<u>,<s>,<a>,<code>,<pre>,<blockquote>, and a few others. Use\ninside<pre>…</pre>instead. - The Debug-formatted
messagein<pre>{:?}</pre>is not HTML-escaped. If any field contains<or&, the request fails. Escape these as<and&respectively. - The callback string is parsed twice via
toml::from_str— here and again inCore::cb. Remove the duplicate.
| UpdateType::CallbackQuery(query) => { | ||
| if let Some(ref cb) = query.data | ||
| && let Err(err) = self.cb(&query, cb).await | ||
| && let Err(err) = self.tg.answer_cb(query.id, err.to_string()).await | ||
| { | ||
| println!("{err:?}"); | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
tgbot 0.44.0 AnswerCallbackQuery required for every callback query acknowledgment
💡 Result:
No, AnswerCallbackQuery is not newly required in tgbot 0.44.0 specifically for every callback query acknowledgment. This is a general Telegram Bot API requirement that has existed since callback queries were introduced. In the Telegram Bot API, when a user presses an inline keyboard button, Telegram sends a callback_query update to your bot. To acknowledge receipt and stop the loading spinner on the client side, you must call answerCallbackQuery with the callback_query.id within 10 seconds. Failure to do so results in errors like "query is too old" and leaves the user seeing an indefinite spinner. tgbot 0.44.0 (released 2026-04-08) supports Bot API 9.6 but its changelog shows no changes related to making AnswerCallbackQuery mandatory or altering callback query handling. The library provides the AnswerCallbackQuery type and method as part of full Bot API support. Examples in the repo (in /examples directory) demonstrate handling updates including callback queries via UpdateHandler trait, where you manually call api.answer_callback_query if needed. General best practice in tgbot: - Implement UpdateHandler for your bot. - In the handler, match on update.callback_query. - Call bot.api.answer_callback_query(callback_query.id, None) immediately (silently acknowledges), then process data. This behavior is consistent across versions; no version-specific mandate in 0.44.0. Official Telegram docs confirm: "Always call this method, even silently. If you don't answer a callback query, the user sees a loading spinner indefinitely."
Citations:
- 1: https://gramio.dev/telegram/methods/answerCallbackQuery
- 2: https://github.com/tg-rs/tgbot/blob/master/CHANGELOG.md
- 3: https://stackoverflow.com/questions/77366040/why-do-i-have-to-call-to-the-answercallbackquery-endpoint-to-know-if-a-user-p
- 4: https://docs.rs/tg_bot_models/latest/tg_bot_models/struct.CallbackQuery.html
- 5: https://docs.rs/telegram-bot/latest/telegram_bot/types/requests/struct.AnswerCallbackQuery.html
- 6: https://github.com/tg-rs/tgbot
🏁 Script executed:
# Check the file exists and examine the specific lines
head -n 470 src/core.rs | tail -n 20Repository: kworr/rsstg
Length of output: 446
🏁 Script executed:
# Search for the answer_cb method definition to understand its signature
rg "fn answer_cb|answer_cb\(" src/core.rs -A 3 -B 1Repository: kworr/rsstg
Length of output: 215
🏁 Script executed:
# Look for the cb method definition
rg "fn cb\(|async fn cb" src/core.rs -A 3 -B 1Repository: kworr/rsstg
Length of output: 199
🏁 Script executed:
# Check if there are any examples or tests handling callback queries
find . -type f -name "*.rs" | xargs rg -l "CallbackQuery" 2>/dev/null | head -5Repository: kworr/rsstg
Length of output: 101
🏁 Script executed:
# Search for answer_cb method more broadly
rg "answer_cb" src/ -B 2 -A 5Repository: kworr/rsstg
Length of output: 1064
🏁 Script executed:
# Check the struct definition to see what tg is
rg "struct.*{" src/core.rs -A 10 | head -30Repository: kworr/rsstg
Length of output: 136
🏁 Script executed:
# Look at tg_bot.rs to find answer_cb
cat src/tg_bot.rs | head -100Repository: kworr/rsstg
Length of output: 2026
Every callback query must be acknowledged, regardless of success or failure.
The current logic only calls answer_cb() when cb() errors. On the success path, AnswerCallbackQuery is never sent, leaving the Telegram client's button in a loading state until it times out (~15s). Per Telegram Bot API requirements, this method must be called exactly once for every received callback query to clear the loading state immediately.
🛠️ Suggested fix
UpdateType::CallbackQuery(query) => {
- if let Some(ref cb) = query.data
- && let Err(err) = self.cb(&query, cb).await
- && let Err(err) = self.tg.answer_cb(query.id, err.to_string()).await
- {
- println!("{err:?}");
- }
+ let ack_text = match &query.data {
+ Some(cb) => match self.cb(&query, cb).await {
+ Ok(()) => String::new(),
+ Err(err) => err.to_string(),
+ },
+ None => "No callback data.".to_owned(),
+ };
+ if let Err(err) = self.tg.answer_cb(query.id, ack_text).await {
+ println!("{err:?}");
+ }
},📝 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.
| UpdateType::CallbackQuery(query) => { | |
| if let Some(ref cb) = query.data | |
| && let Err(err) = self.cb(&query, cb).await | |
| && let Err(err) = self.tg.answer_cb(query.id, err.to_string()).await | |
| { | |
| println!("{err:?}"); | |
| } | |
| }, | |
| UpdateType::CallbackQuery(query) => { | |
| let ack_text = match &query.data { | |
| Some(cb) => match self.cb(&query, cb).await { | |
| Ok(()) => String::new(), | |
| Err(err) => err.to_string(), | |
| }, | |
| None => "No callback data.".to_owned(), | |
| }; | |
| if let Err(err) = self.tg.answer_cb(query.id, ack_text).await { | |
| println!("{err:?}"); | |
| } | |
| }, |
| impl fmt::Display for Callback { | ||
| fn fmt (&self, f: &mut fmt::Formatter) -> fmt::Result { | ||
| f.write_str(&toml::to_string(self).map_err(|_| fmt::Error)?) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Telegram Bot API callback_data 64 bytes limit InlineKeyboardButton
💡 Result:
Yes, the Telegram Bot API enforces a 64-byte limit on the callback_data field in InlineKeyboardButton. The data must be 1-64 bytes in length, measured as UTF-8 encoded bytes. Exceeding this limit results in errors like BUTTON_DATA_INVALID or silent truncation. This limit remains unchanged as of Bot API 9.6 (April 2026), per recent changelogs and documentation.
Citations:
- 1: callback_data is allowed to exceed 64 characters nmlorg/metabot#1
- 2: https://docs.python-telegram-bot.org/en/v22.4/telegram.inlinekeyboardbutton.html
- 3: https://python-telegram-bot.readthedocs.io/telegram.inlinekeyboardbutton.html
- 4: https://docs.python-telegram-bot.org/en/v22.5/telegram.inlinekeyboardbutton.html
- 5: https://core.telegram.org/bots/api
- 6: https://pytba.readthedocs.io/en/4.24.0/types.html
- 7: https://hfeu-telegram.com/news/step-by-step-deploy-telegram-inline-keyboards-1320930732
- 8: https://docs.python-telegram-bot.org/en/stable/telegram.inlinekeyboardbutton.html?highlight=inlinekeyboard
- 9: https://core.telegram.org/bots/api-changelog
🏁 Script executed:
# First, let's explore the structure of the repository
git ls-files src/ | head -20Repository: kworr/rsstg
Length of output: 118
🏁 Script executed:
# Read the relevant file section
cat -n src/tg_bot.rs | sed -n '1,100p'Repository: kworr/rsstg
Length of output: 2726
🏁 Script executed:
# Search for Callback struct definition
rg 'enum Callback|struct Callback' src/ -A 10Repository: kworr/rsstg
Length of output: 460
🏁 Script executed:
# Search for where Callback Display is used (callback_data usage)
rg 'Callback' src/tg_bot.rs -B 2 -A 2 | head -80Repository: kworr/rsstg
Length of output: 2049
🏁 Script executed:
# Look for any validation or length checks
rg 'len\(\)|\.to_string\(\)|callback_data' src/tg_bot.rs -B 2 -A 2Repository: kworr/rsstg
Length of output: 1197
🏁 Script executed:
# Check Cargo.toml for serde/toml versions to understand serialisation behaviour
cat Cargo.tomlRepository: kworr/rsstg
Length of output: 896
🏁 Script executed:
# Get more of the tg_bot.rs file to see actual usage
cat -n src/tg_bot.rs | sed -n '100,250p'Repository: kworr/rsstg
Length of output: 5237
toml::to_string for callback payloads may exceed Telegram's 64-byte limit.
Telegram enforces a 1-64 byte UTF-8 limit on callback_data in InlineKeyboardButton. TOML serialisation of Callback::List(0, "some-long-channel-name", 0) produces List = [0, "some-long-channel-name", 0], which easily exceeds 64 bytes with realistic feed names and will be rejected by the Bot API with BUTTON_DATA_INVALID. The code passes unrestricted feed names directly to the callback without validation (e.g., line 111: Callback::edit(name).to_string()).
Use a compact encoding (binary/base64 payload), store the feed name server-side and use only the ID, and/or validate length in Display before returning.
| Callback::Edit(_, _name) => { // XXX edit missing | ||
| let kb: Vec<Vec<InlineKeyboardButton>> = vec![]; | ||
| InlineKeyboardMarkup::from(kb) | ||
| }, |
There was a problem hiding this comment.
Callback::Edit is a no-op stub.
get_kb for Callback::Edit returns an empty keyboard with an // XXX edit missing marker. Any caller emitting Callback::edit(...) (e.g. the list-view buttons on Line 111) will land on a message with no actionable controls. At minimum, wire up a "back to menu" button so the user isn't stranded, and track the missing implementation in an issue.
Want me to open a tracking issue for the missing Callback::Edit keyboard?
| } else { | ||
| let mut found = false; | ||
| let mut first_page = None; | ||
| for (id, feed_name) in feeds.iter() { | ||
| if name == feed_name { | ||
| found = true; | ||
| } | ||
| i += 1; | ||
| kb.push(vec![ | ||
| InlineKeyboardButton::for_callback_data( | ||
| format!("{}. {}", id, feed_name), | ||
| Callback::list("xxx", *page).to_string()), // XXX edit | ||
| ]); | ||
| if i == end { | ||
| // page complete, if found we got the right page, if not - reset and | ||
| // continue. | ||
| if found { | ||
| break | ||
| } else { | ||
| if first_page.is_none() { | ||
| first_page = Some(kb); | ||
| } | ||
| kb = vec![]; | ||
| i = 0 | ||
| } | ||
| } | ||
| } | ||
| if !found { | ||
| // name not found, showing first page | ||
| kb = first_page.unwrap_or_default(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Name-search branch can drop a partial first page.
When Callback::List is invoked with a non-empty name and the total number of feeds is less than end (e.g. long == true but only 4 items, or name is never matched), the inner loop ends without ever hitting i == end, so first_page stays None even though kb has been populated with a usable partial page. Line 145 then does kb = first_page.unwrap_or_default();, discarding the partial kb and replying with an empty keyboard.
Consider saving the current kb as first_page on loop exit when it's still None, or restructuring to collect/sort into a Vec once and paginate with chunks/skip/take.
| Callback::Menu(_) => { | ||
| let kb = vec![ | ||
| vec![ | ||
| InlineKeyboardButton::for_callback_data( | ||
| "Add new channel", | ||
| Callback::menu().to_string()), // new XXX | ||
| ], | ||
| vec![ | ||
| InlineKeyboardButton::for_callback_data( | ||
| "List channels", | ||
| Callback::list("", 0).to_string()), | ||
| ], | ||
| ]; | ||
| InlineKeyboardMarkup::from(kb) | ||
| }, |
There was a problem hiding this comment.
"Add new channel" button points back to the menu.
Line 163 wires the "Add new channel" button's callback data to Callback::menu() (same as the current screen) with an // new XXX marker. Pressing the button will just re-render the menu. Either plug in the real "add" callback variant or hide the button until it's implemented, so callers aren't misled.
FossilOrigin-Name: d8c1d259a292ba4d06a715abf834bd0f399d9c232103c0ec1ea2f5fa8f44402d
There was a problem hiding this comment.
♻️ Duplicate comments (11)
src/tg_bot.rs (5)
96-100:⚠️ Potential issue | 🟠 Major
toml::to_stringcallback payloads can still exceed Telegram's 64-byte limit.Feed names are still passed unvalidated into
Callback::edit(name).to_string()at line 128 and any caller ofCallback::list(name, _). TOML output likeList = [0, "some-long-channel-name", 0]trivially exceeds 64 bytes with realistic channel names, and Telegram will reject the inline keyboard withBUTTON_DATA_INVALID. Please encode the numericsource_idinstead of the name in callback data and/or validate the payload length inDisplay/get_kbbefore returning the markup.
141-164:⚠️ Potential issue | 🟡 MinorName-search branch still drops a partial first page.
When the total number of matching feeds is less than
end(e.g.long == truebut thenamenever matches and the list has fewer thanendentries), the inner loop ends without ever hittingi == end, sofirst_pageremainsNoneand the fallback at line 162 renders an empty keyboard. Either capturekbintofirst_pageon loop exit when it's stillNone, or collect/sort into aVeconce and paginate withchunks/skip/take.
107-111:⚠️ Potential issue | 🟡 Minor
Callback::Editstill returns an empty keyboard.
get_kbforCallback::EditreturnsInlineKeyboardMarkup::from(vec![])with an// XXX edit missingmarker. Any caller emittingCallback::edit(...)— including the list view at line 128 — lands on a message with no actionable controls. At minimum, wire up a "back to menu" button so the user isn't stranded until the real edit flow is implemented.
175-189:⚠️ Potential issue | 🟡 Minor"Add new channel" still points back to the root menu.
Line 180 wires the "Add new channel" button's callback to
Callback::menu()— the same screen the user is already on. Pressing it just re-renders the menu. Either introduce a dedicatedCallback::Add(...)variant or temporarily hide the button until the add flow is wired up, so callers aren't misled.
121-132:⚠️ Potential issue | 🟠 MajorPagination bound incorrect for
page > 0in the empty-name branch.
istarts at0and only counts pushed items, butendis computed as5 + page * 5. Afterfeeds.iter().skip(start), the loop pushes items and breaks wheni == end. Onpage == 1that givesend = 10whileican only reach5by pushing five items — the break never fires and the loop continues, potentially rendering up to 10 items on a single page (or however many remain). Either computeendrelative to page size (end = 5) or count absolute position withstart + i.🐛 Proposed fix
- let (start, end) = if long { - (page * 5, 5 + page * 5) - } else { - (0, 6) - }; - let mut i = 0; + let (start, page_size) = if long { (page * 5, 5) } else { (0, 6) }; + let mut i = 0; if name.is_empty() { let feed_iter = feeds.iter().skip(start); for (id, name) in feed_iter { kb.push(vec![ InlineKeyboardButton::for_callback_data( format!("{}. {}", id, name), Callback::edit(name).to_string()), ]); i += 1; - if i == end { break } + if i == page_size { break } } } else {src/core.rs (4)
417-421:⚠️ Potential issue | 🔴 Critical
cb()still panics viatodo!()at runtime.When a user presses an inline keyboard button,
todo!()will panic and crash the handler. Either delegate tocommand::answer_cb(which already contains the callback handling logic) or return an explicit error so the outer handler can answer the callback query with a descriptive message. Additionally,Ok(())on line 420 is unreachable aftertodo!().🐛 Proposed fix
pub async fn cb (&self, query: &CallbackQuery, cb: &str) -> Result<()> { - let cb: Callback = toml::from_str(cb).stack()?; - todo!(); - Ok(()) + command::answer_cb(self, query, cb).await }
464-471:⚠️ Potential issue | 🟠 MajorCallback queries are only acknowledged on the error path.
On success,
AnswerCallbackQueryis never sent, so the Telegram client keeps the button's loading spinner until it times out (~15s). Per the Bot API requirement,answerCallbackQuerymust be called exactly once for every callback query received. Always acknowledge — with an empty string on success and the error text on failure.🛠️ Suggested fix
UpdateType::CallbackQuery(query) => { - if let Some(ref cb) = query.data - && let Err(err) = self.cb(&query, cb).await - && let Err(err) = self.tg.answer_cb(query.id, err.to_string()).await - { - println!("{err:?}"); - } + let ack_text = match &query.data { + Some(cb) => match self.cb(&query, cb).await { + Ok(()) => String::new(), + Err(err) => err.to_string(), + }, + None => "No callback data.".to_owned(), + }; + if let Err(err) = self.tg.answer_cb(query.id, ack_text).await { + println!("{err:?}"); + } },
380-397:⚠️ Potential issue | 🟠 Major
add_feedcache-miss path does not insert the new feed.On cache miss,
inserted = falseand the fallback at line 394 callsget_feeds(owner)which only loads existing feeds from the database — it does not insert the(source_id, channel)pair that was passed in. In the/addflow,conn.update(...)commits the new row first (viaDb::begin's implicit commit) only if the transaction is committed; if the cache was empty at this moment but the DB row is already visible, the reload will pick it up, otherwise the new feed is silently lost from the cache. Prefer populating the cache, then inserting:🐛 Proposed fix
pub async fn add_feed (&self, owner: i64, source_id: i32, channel: String) -> Result<()> { - let mut inserted = true; - { - let mut feeds = self.feeds.lock_arc().await; - if let Some(feed) = feeds.get_mut(&owner) { - let mut feed = feed.lock_arc().await; - feed.insert(source_id, channel); - } else { - inserted = false; - } - } - // in case insert failed - we miss the entry we needed to expand, reload everything from - // database - if !inserted { - self.get_feeds(owner).await.stack()?; - } + let feeds_map = self.get_feeds(owner).await.stack()?; + let mut feed = feeds_map.lock_arc().await; + feed.insert(source_id, channel); Ok(()) }
445-459:⚠️ Potential issue | 🟠 Major
validate()is not sufficient to make arbitrary error text HTML-safe.
validate()(seesrc/tg_bot.rs:54-60) only rejects closing</pre>/</code>tags — it does not escape<,>, or&, which Telegram requires to be escaped inside HTML parse-mode text. Error messages stacked viastacked_errorsfrequently includeVec<_>,Debugformatted types, or URLs with&in query strings, all of which will causeSendMessageto fail with a parse error and the#errorreply will be silently dropped. The same applies at lines 160 (🛑 {err}) and 335 (<pre>{}</pre>). Add an HTML-escape helper used everywhere user-visible or error-derived text is interpolated into HTML.src/command.rs (2)
199-210:⚠️ Potential issue | 🟠 MajorSuccess reply is still sent before the post-insert steps can fail.
core.tg.send(...)on line 201 tells the user "Channel added." beforeget_one_name/add_feedrun. Ifget_one_namereturnsNoneor the transaction was not actually committed (e.g.updatereturned"Duplicate key."), thebail!at line 206 produces a second#errorreply and the user sees contradictory UX. Gate the reply on successful commit + cache update, and skipget_one_name/add_feedwhenupdatedidn't actually insert a row.
212-242:⚠️ Potential issue | 🟠 Major
command::answer_cbis still unreachable and carries several bugs.
Core::handledispatchesUpdateType::CallbackQuerytoCore::cb, which is stilltodo!(); nothing callscommand::answer_cb. Either wire this handler up fromCore::cb(see the suggestion in thesrc/core.rsreview) or delete it. If it is retained, please also fix:
- Line 216:
let text = "Sample".to_owned();is a placeholder that will reach end users.- Lines 226 and 237:
<br>is not part of Telegram's HTML parse mode (only<b>,<i>,<u>,<s>,<a>,<code>,<pre>,<blockquote>, etc. are supported). Use\ninside<pre>…</pre>instead.- Lines 226 / 237: the
{:?}interpolation ofmessageis not HTML-escaped — any<or&in the debug output will break thesendMessagecall.- Line 213 re-parses
cbfrom TOML even thoughCore::cbis expected to parse it as well — once this is reachable, deduplicate the parse.- Line 221:
sender == ownercompares the clicker againstmessage.sender.get_user(), which for callback-query messages is the bot (since the bot sent the message carrying the inline keyboard), so this branch will virtually never hit theupdate_messagepath.
🧹 Nitpick comments (1)
src/tg_bot.rs (1)
300-321: Theretry_after()return type is confirmed to be seconds, but consider bounding the retry loop.The code correctly assumes
retry_after()returns an unsigned integer representing seconds. The numeric comparisondelay > 60andDuration::from_secs(delay)confirm this — the code compiles successfully with these operations. However, the pre-existing// XXX Can loop indefinitely…comment remains valid: even with the 60-second threshold, the function will loop indefinitely if Telegram repeatedly returnsretry_aftervalues under 60 seconds. Adding a bounded retry count (e.g. 3 attempts) would improve robustness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 07f4ad4b-b22e-472c-9348-e3ebd405c4d2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
src/command.rssrc/core.rssrc/sql.rssrc/tg_bot.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sql.rs
FossilOrigin-Name: d3dbaf11e1a58a8cf61a62a1a7437b2095647236781b69e7d14eb53026276619
FossilOrigin-Name: fdae2d969b532e8f508527afea9967a5a43a1ab761ac92e41511c35ef52bb5fd
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/rust-clippy.yml (1)
21-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the action to a full commit SHA.
actions/checkout@v7is a mutable tag. Pin this dependency to the verified full commit SHA for the v7 release to prevent future tag movement from changing the CI code unexpectedly. (docs.github.com)Source: MCP tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ff2f5d6-836c-44d0-a3f9-4aeec6b55214
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.github/workflows/rust-clippy.ymlCargo.tomldeny.tomlsrc/command.rssrc/core.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/command.rs
- src/core.rs
| steps: | ||
| # SETUP | ||
| - uses: actions/checkout@v6 | ||
| - uses: actions/checkout@v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Disable checkout credential persistence.
The checkout action persists GITHUB_TOKEN by default, while this job executes repository-controlled build and test code. Add persist-credentials: false so a malicious build script or procedural macro cannot read the token from Git configuration. (github.com)
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 21-21: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
Sources: MCP tools, Linters/SAST tools
1 participant
Summary by CodeRabbit
Release Notes – Version 0.6.0
New Features
/testcommand to quickly show the feed management menu.Bug Fixes
Chores