Fix forwarded media from private channels showing broken placeholder#126
Fix forwarded media from private channels showing broken placeholder#126
Conversation
When a message is forwarded from a private channel and contains a
document, media.document is None. The _get_media_type() method
incorrectly returned "document" for this case because the fallback
return was outside the `if media.document` block, causing
telegram_file_id to become the string "None" and downloads to fail
permanently.
Move `return "document"` inside the truthy branch and return None
when the document reference is inaccessible. Add a safety guard in
_process_media to catch any remaining str("None") file IDs.
Fixes #125
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 32 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughModified media handling in listener and backup modules to return Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
🐳 Dev images published!
The dev/test instance will pick up these changes automatically (Portainer GitOps). To test locally: docker pull drumsergio/telegram-archive:dev
docker pull drumsergio/telegram-archive-viewer:dev |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/telegram_backup.py (1)
1316-1319: Guard is correct but silent.The
"None"string guard works, but since this only fires for inaccessible media (forwarded from private sources), alogger.debughere would help diagnose why certain forwarded media get skipped without producing DB records. Purely optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/telegram_backup.py` around lines 1316 - 1319, The guard that converts the literal string "None" to None for telegram_file_id is fine but lacks visibility; update the block where telegram_file_id is checked (the variable telegram_file_id in src/telegram_backup.py) to emit a logger.debug showing that an inaccessible/forwarded media produced the "None" string and any contextual identifiers available (e.g., message_id, chat_id, or user info) before setting telegram_file_id = None so it's easier to trace skipped media in logs.src/listener.py (1)
519-520: Mirrored fix — LGTM.Both the
_get_media_typereturn-Nonebranch and the"None"string guard in_download_mediamirror the backup module's fix, keeping behavior consistent across the real-time and scheduled paths. Good belt-and-suspenders approach.Side note: in
on_new_message(around line 879-887), when aMessageMediaDocumentarrives without an accessible document,media_typeis nowNoneand the media block is skipped entirely — which is the desired behavior, but means forwarded-from-private media on the listener path will silently produce a message row with no media record. Worth a singlelogger.debugso users grepping logs can correlate missing media to the private-forward case.Also applies to: 576-579
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/listener.py` around lines 519 - 520, When handling incoming messages in on_new_message, add a logger.debug when a MessageMediaDocument arrives but _get_media_type returns None and the media block is skipped; log enough context (e.g., message id, sender id/username and a short note like "skipping media: forwarded-from-private, document reference unavailable") so users can correlate missing media rows to private-forward cases; place this debug immediately before the branch that skips media (the same spot that checks for media_type is None and calls/relies on _download_media).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/listener.py`:
- Around line 519-520: When handling incoming messages in on_new_message, add a
logger.debug when a MessageMediaDocument arrives but _get_media_type returns
None and the media block is skipped; log enough context (e.g., message id,
sender id/username and a short note like "skipping media:
forwarded-from-private, document reference unavailable") so users can correlate
missing media rows to private-forward cases; place this debug immediately before
the branch that skips media (the same spot that checks for media_type is None
and calls/relies on _download_media).
In `@src/telegram_backup.py`:
- Around line 1316-1319: The guard that converts the literal string "None" to
None for telegram_file_id is fine but lacks visibility; update the block where
telegram_file_id is checked (the variable telegram_file_id in
src/telegram_backup.py) to emit a logger.debug showing that an
inaccessible/forwarded media produced the "None" string and any contextual
identifiers available (e.g., message_id, chat_id, or user info) before setting
telegram_file_id = None so it's easier to trace skipped media in logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1ac07b93-f118-41e0-bf04-c4a7e0ce9a49
📒 Files selected for processing (2)
src/listener.pysrc/telegram_backup.py
Tests now assert None instead of "document" when media.document is None, matching the fix for forwarded media from private channels.
|
🐳 Dev images published!
The dev/test instance will pick up these changes automatically (Portainer GitOps). To test locally: docker pull drumsergio/telegram-archive:dev
docker pull drumsergio/telegram-archive-viewer:dev |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #126 +/- ##
==========================================
- Coverage 85.93% 85.93% -0.01%
==========================================
Files 21 21
Lines 5865 5871 +6
==========================================
+ Hits 5040 5045 +5
- Misses 825 826 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary
_get_media_type()returning"document"whenmedia.documentisNone(forwarded from private channels), causing broken media records withtelegram_file_id = "None"and permanent download failures_process_mediato catch any remainingstr(None)file IDsFixes #125
Type of Change
Database Changes
scripts/Data Consistency Checklist
chat_idvalues use marked format (via_get_marked_id())_strip_tz()before DB operationsTesting
python -m pytest tests/ -v)ruff check .)ruff format --check .)Security Checklist
Deployment Notes
telegram_file_id = "None"in the database will still show as broken; a future cleanup script or re-backup will fix thoseSummary by CodeRabbit