fix: error handler to properly handle None updates and add exception … #157
Conversation
Walkthroughcentralizes and strengthens error handling: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BotHandler as main.py Handler
participant ErrorHandler as general_error_handler.py
participant Logger
participant Admins
User->>BotHandler: Send message/update
BotHandler->>BotHandler: Validate message/text (inside try)
alt URL or content processing
BotHandler->>BotHandler: Normalize/clean URL
BotHandler->>BotHandler: Check supported sites / length
BotHandler-->>User: Reply or continue processing
end
alt Exception thrown
BotHandler->>ErrorHandler: Raise/Catch exception
ErrorHandler->>Logger: Log exception + full traceback
ErrorHandler->>ErrorHandler: Extract user/chat/snippet
ErrorHandler->>Admins: Build Markdown notification (structured payload)
ErrorHandler->>Admins: Try send -> on failure log send error
BotHandler-->>User: Send localized error reply (if applicable)
ErrorHandler->>BotHandler: Re-raise (propagate)
end
BotHandler->>BotHandler: Finally: cleanup media_path/resources
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/general_error_handler.py (1)
1-74: Runblackon this file to fix CI formattingGitHub Actions reports that
blackwould reformatsrc/general_error_handler.py. Please run:black src/general_error_handler.pyso that the CI linters pass before merge.
src/main.py (1)
1-656: Runblackon this file to satisfy CI formattingCI reports that
blackwould reformatsrc/main.py. Please run:black src/main.pylocally so that the formatter check passes before merging.
🧹 Nitpick comments (5)
src/general_error_handler.py (2)
51-61: Escape user-controlled text inside Markdown admin notifications
error_msgand especiallymessage_textcan contain backticks or other Markdown control characters. Putting them directly inside`...`(Line 56 and Line 61) can lead to “can't parse entities” errors from Telegram and cause admin notifications to fail.Consider escaping these fields or switching to HTML parse mode. For example:
-from telegram.ext import CallbackContext +from telegram.ext import CallbackContext +from telegram.helpers import escape_markdown @@ - error_msg = str(exception)[:500] + error_msg = str(exception)[:500] @@ - f"*Error:* `{error_msg}`\n" + f"*Error:* `{escape_markdown(error_msg, version=2)}`\n" @@ - notification += f"*Message:* `{message_text}`\n" + notification += f"*Message:* `{escape_markdown(message_text, version=2)}`\n" @@ - parse_mode='Markdown', + parse_mode='MarkdownV2',This keeps formatting while making the notification robust to arbitrary user content.
Please double‑check the exact import path and signature of
escape_markdownfor yourpython-telegram-botversion in the official docs before applying this change.
63-72: BroadExceptioncatch when sending admin notificationsCatching a broad
Exceptionhere is defensible to avoid cascading failures if admin notifications break, but Ruff (BLE001) flags it and it can hide unexpected bugs in the send path.If you want to keep it broad, you can at least make that intent explicit to static analysis, or narrow it to the concrete error types you expect (e.g., network/Telegram errors) while still logging:
- except Exception as send_error: # pylint: disable=broad-except - error("Failed to send error notification to admin %s: %s", - admin_chat_id, send_error) + except Exception as send_error: # pylint: disable=broad-except # noqa: BLE001 + error( + "Failed to send error notification to admin %s: %s", + admin_chat_id, + send_error, + )This keeps behavior but documents the deliberate choice.
src/main.py (3)
115-135:clean_urlassumes URL is the first token; may mis-handle “text + URL” messagesOn Line 129 you do
url = url.split()[0], so a message like"Check this https://example.com"will pass the"http"check (Line 210) but end up callingdownload_media("Check"), which will likely fail.Consider extracting the first token that actually looks like a URL instead of always taking the first token, e.g.:
- # Split by space and take the first part (the URL) - url = url.split()[0] + # Split by space and take the first part that looks like a URL + for part in url.split(): + if part.startswith(("http://", "https://")): + url = part + breakThis keeps existing behavior for pure-URL messages while being safer for messages that contain extra text.
188-192: Guard access control againsteffective_userbeingNoneLine 190 assumes
update.effective_useris always present and accesses.usernamedirectly. For some update types (e.g., channel posts),effective_usercan beNone, which would raise an exception before you even reach the error handler.You can make this robust with a small guard:
- if is_user_or_chat_not_allowed(update.effective_user.username, update.effective_chat.id): + username = update.effective_user.username if update.effective_user else None + if is_user_or_chat_not_allowed(username, update.effective_chat.id):
is_user_or_chat_not_allowedalready acceptsOptional[str], so this integrates cleanly.
314-327: Avoid bareexcept/passwhen sending the user-facing error messageThe outer
except Exception as e(Line 314) is reasonable for a last-resort handler, but the innerexcept: pass(Lines 325–326) both hides any bug in the reply path and triggers Ruff’s E722/S110 warnings.You can still prevent cascaded failures while logging what went wrong:
- try: - if update and update.message: - await update.message.reply_text( - "Вибачте, сталася помилка під час обробки повідомлення." - if language == "uk" - else "Sorry, an error occurred while processing your message.", - reply_to_message_id=update.message.message_id, - ) - except: # pylint: disable=bare-except - pass + try: + if update and update.message: + await update.message.reply_text( + "Вибачте, сталася помилка під час обробки повідомлення." + if language == "uk" + else "Sorry, an error occurred while processing your message.", + reply_to_message_id=update.message.message_id, + ) + except Exception as reply_error: # noqa: E722 + debug("Failed to send error reply to user: %s", reply_error)This addresses the Ruff hints while keeping the handler resilient.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/general_error_handler.py(2 hunks)src/main.py(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-02-02T16:48:04.210Z
Learnt from: yurnov
Repo: ovchynnikov/load-bot-linux PR: 37
File: src/main.py:0-0
Timestamp: 2025-02-02T16:48:04.210Z
Learning: When sending multiple media files through Telegram Bot API, implement throttling with `asyncio.sleep` between sends to prevent "Flood control exceeded" errors. A delay of 15 seconds between iterations has been proven effective.
Applied to files:
src/main.py
📚 Learning: 2025-02-02T16:48:05.922Z
Learnt from: yurnov
Repo: ovchynnikov/load-bot-linux PR: 37
File: src/main.py:0-0
Timestamp: 2025-02-02T16:48:05.922Z
Learning: When sending multiple media files through Telegram API, add delays (15 seconds) between consecutive sends using `asyncio.sleep` to prevent "Flood control exceeded" errors from the API.
Applied to files:
src/main.py
🧬 Code graph analysis (1)
src/main.py (2)
src/permissions.py (2)
is_user_or_chat_not_allowed(16-35)inform_user_not_allowed(39-58)src/video_utils.py (1)
is_video_too_long_to_download(83-101)
🪛 GitHub Actions: 📝 Linters
src/general_error_handler.py
[error] black check failed: 2 files would be reformatted (src/general_error_handler.py). Run 'black --write' to fix code style issues.
src/main.py
[error] black check failed: 2 files would be reformatted (src/main.py). Run 'black --write' to fix code style issues.
🪛 Ruff (0.14.5)
src/general_error_handler.py
70-70: Do not catch blind exception: Exception
(BLE001)
src/main.py
325-325: Do not use bare except
(E722)
325-326: try-except-pass detected, consider logging the exception
(S110)
🔇 Additional comments (2)
src/general_error_handler.py (1)
30-45: Exception logging and contextual info extraction look solidThe handler now logs the exception, full traceback, and safely derives user/chat/message only when
updateis present, so it will not blow up onNoneupdates and gives useful context for debugging.src/main.py (1)
571-571: LLM safety-fallback message change is behavior-only and looks fineThe updated prefix to
"Якщо коротко, ..."for the safety-fallback path (Line 571) is a pure text change and still produces a concise, user-friendly summary; no functional issues here.
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 (2)
src/main.py (2)
1-659: Fix Black formatting to unblock the pipelineCI reports that
black --checkwould reformat this file. To keep style consistent and get the pipeline green, re-run Black with the project settings:black --write --skip-string-normalization --line-length 120 src/main.py
367-388: Remove redundantfinally: file.close()blocks in bothsend_videoandsend_picfunctionsVerification confirms both issues are present in the current code:
- Lines 367-388 (
send_video):with open(video, 'rb') as video_file:followed byfinally: video_file.close()at lines 387-388- Lines 428-446 (
send_pic):with open(pic, 'rb') as pic_file:followed byfinally: pic_file.close()at lines 445-446The
finallyblocks are indeed redundant and dangerous. Ifopen()fails (e.g., missing file, permission denied), the variable is never bound, butfinallystill executes and raisesUnboundLocalError, masking the real I/O error. Thewithstatement already guarantees file closure. Remove bothfinallyblocks as shown in the review comment's diffs.
🧹 Nitpick comments (2)
src/main.py (2)
181-242: Error-handling refactor andmedia_pathinitialization now look robustInitializing
media_path = []before thetryand keeping the cleanup infinallyfixes the previousUnboundLocalErrorrisk and ensures temporary files are cleaned up consistently across all early returns and exceptions. Thetry/except/finallystructure with traceback logging and a user-facing fallback message is a solid improvement and aligns with the project’s pattern of doing cleanup at the parent function level. Minor nit: the comment# media_path can be string or list of stringsno longer reflects reality—return_pathis string-or-list, whilemedia_pathis always a list—so updating the comment would avoid future confusion. Based on learnings, this keeps cleanup consistent with howdownload_media-created files are managed elsewhere.Also applies to: 328-330
314-327: Narrow the bareexceptclause to handle only expected exceptionsThe bare
except:at line 325 will swallow all exceptions including system exceptions likeasyncio.CancelledError, which is what Ruff flags with E722 and S110. Since the original error is already logged above, it's safe to ignore secondary failures from the reply attempt. However, explicitly narrow the exception type and optionally add a debug log for clarity:try: if update and update.message: await update.message.reply_text( "Вибачте, сталася помилка під час обробки повідомлення." if language == "uk" else "Sorry, an error occurred while processing your message.", reply_to_message_id=update.message.message_id, ) - except: # pylint: disable=bare-except + except Exception as reply_err: # noqa: E722 # pylint: disable=broad-except + debug("Failed to send error reply to user: %s", reply_err) pass
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main.py(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-02-02T10:30:20.245Z
Learnt from: yurnov
Repo: ovchynnikov/load-bot-linux PR: 37
File: src/video_utils.py:169-226
Timestamp: 2025-02-02T10:30:20.245Z
Learning: Temporary directory management (creation and cleanup) in video_utils.py is consistently handled at the parent function level (download_media) rather than in individual helper functions.
Applied to files:
src/main.py
📚 Learning: 2025-02-02T16:48:04.210Z
Learnt from: yurnov
Repo: ovchynnikov/load-bot-linux PR: 37
File: src/main.py:0-0
Timestamp: 2025-02-02T16:48:04.210Z
Learning: When sending multiple media files through Telegram Bot API, implement throttling with `asyncio.sleep` between sends to prevent "Flood control exceeded" errors. A delay of 15 seconds between iterations has been proven effective.
Applied to files:
src/main.py
📚 Learning: 2025-02-02T16:48:05.922Z
Learnt from: yurnov
Repo: ovchynnikov/load-bot-linux PR: 37
File: src/main.py:0-0
Timestamp: 2025-02-02T16:48:05.922Z
Learning: When sending multiple media files through Telegram API, add delays (15 seconds) between consecutive sends using `asyncio.sleep` to prevent "Flood control exceeded" errors from the API.
Applied to files:
src/main.py
🧬 Code graph analysis (1)
src/main.py (2)
src/permissions.py (2)
is_user_or_chat_not_allowed(16-35)inform_user_not_allowed(39-58)src/video_utils.py (1)
is_video_too_long_to_download(83-101)
🪛 GitHub Actions: 📝 Linters
src/main.py
[error] 1-1: Black formatting would reformat this file. Run 'black --write' to fix formatting. Command: black --check --diff --skip-string-normalization --line-length 120 src/
🪛 Ruff (0.14.5)
src/main.py
325-325: Do not use bare except
(E722)
325-326: try-except-pass detected, consider logging the exception
(S110)
🔇 Additional comments (1)
src/main.py (1)
571-577: LLM fallback prefix change is safeThe change to the Ukrainian fallback prefix (
"Якщо коротко, ...") only affects user-facing text when safety filters trigger and does not alter control flow or error handling. No issues from a correctness standpoint.
1 participant
Fix error handler to properly handle None updates and add exception handling
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.