feat: fix ios aspect ratio#86
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
More reviews will be available in 46 minutes and 35 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces comprehensive video metadata extraction and persistence across the FastFetchBot pipeline. It adds utilities for positive integer validation, extends the MediaFile model with dimensions and duration, enhances bot-side capture to forward video metadata into Redis, implements async ffprobe-based probing in message composition, and updates the worker to persist video metadata to MongoDB. ChangesVideo Metadata Capture and Persistence
Sequence Diagram(s)sequenceDiagram
participant MessageSender
participant VideoMetadata
participant Ffprobe
participant InputMediaVideo
MessageSender->>VideoMetadata: ensure_video_metadata(media_item, io_object)
VideoMetadata->>VideoMetadata: check ffprobe available
VideoMetadata->>Ffprobe: async subprocess probe
Ffprobe-->>VideoMetadata: width, height, duration, rotation
VideoMetadata->>VideoMetadata: swap dims if rotation 90°
VideoMetadata-->>MessageSender: populated media_item
MessageSender->>InputMediaVideo: build with video_input_kwargs(media_item)
InputMediaVideo-->>MessageSender: InputMediaVideo with supports_streaming=True
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #86 +/- ##
=======================================
Coverage ? 81.13%
=======================================
Files ? 68
Lines ? 3785
Branches ? 0
=======================================
Hits ? 3071
Misses ? 714
Partials ? 0
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/async-worker/async_worker/services/file_id_consumer.py`:
- Around line 49-51: In the shutdown requeue exception path in
file_id_consumer.py (the block that currently calls logger.warning with
raw_payload), replace the warning call with logger.exception so the traceback is
captured; keep the existing message context (e.g. "Failed to requeue payload on
shutdown") and include raw_payload in the log call (use logger.exception("Failed
to requeue payload on shutdown: %s", raw_payload)) so errors during the requeue
in that function/path are logged with the full stack trace.
In `@apps/telegram-bot/core/services/video_metadata.py`:
- Around line 146-149: The current early-return checks only for width and height
using video_metadata_from_mapping(media_item), which skips probing when other
fields (e.g., duration) are missing; change the guard so we only return early
when all required fields (width, height, duration) are present — otherwise call
probe_video_metadata(io_object) to fill missing fields; update the conditional
around metadata/probed_metadata in the function that uses
video_metadata_from_mapping and probe_video_metadata so it checks all required
keys instead of just width and height.
In `@packages/shared/fastfetchbot_shared/utils/number.py`:
- Around line 4-8: The current positive_int() uses round(float(value)) which
silently converts fractional inputs (e.g. "720.6") into integers; change it to
strictly accept only whole integers: if value is an int accept it, if it's a
string accept only when it matches an integer pattern (no decimal point, no
exponent, optional +/-), then cast with int(), and return the int only when > 0;
otherwise return None. Update the logic in positive_int (in
packages/shared/fastfetchbot_shared/utils/number.py) to replace the float-round
path with the strict integer validation described.
In `@tests/unit/telegram_bot/test_message_sender.py`:
- Around line 300-311: Remove the redundant create=True from the AsyncMock patch
of "core.services.video_metadata.probe_video_metadata" in the test; specifically
update the patch call in tests/unit/telegram_bot/test_message_sender.py (the
patch that returns {"width": 720, "height": 1280, "duration": 14"} and is bound
to mock_probe) so it does not pass create=True, ensuring the test strictly mocks
the existing probe_video_metadata used by ensure_video_metadata and will fail if
that symbol is renamed or removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5fc019cf-6ae0-4e9d-81d9-289d0b03b9c0
📒 Files selected for processing (14)
.gitignoreAGENTS.mdapps/async-worker/async_worker/services/file_id_consumer.pyapps/telegram-bot/Dockerfileapps/telegram-bot/core/services/file_id_capture.pyapps/telegram-bot/core/services/message_sender.pyapps/telegram-bot/core/services/video_metadata.pypackages/shared/fastfetchbot_shared/models/metadata_item.pypackages/shared/fastfetchbot_shared/utils/number.pytests/unit/async_worker/test_file_id_consumer.pytests/unit/database/mongodb/test_file_id_fields.pytests/unit/telegram_bot/test_file_id_capture.pytests/unit/telegram_bot/test_message_sender.pytests/unit/test_number.py
Comment on lines
+49
to
+51
| logger.warning( | ||
| f"Failed to requeue payload on shutdown: {raw_payload}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Use logger.exception() in this exception path.
This branch is already handling a real exception during shutdown requeue, so logger.warning(...) drops the traceback exactly where you need it for queue-loss debugging.
Proposed fix
except Exception:
- logger.warning(
+ logger.exception(
f"Failed to requeue payload on shutdown: {raw_payload}"
)As per coding guidelines, **/*.py: Always use logger.exception() for logging errors with tracebacks and logger.error() for message-only logging; never use print() or traceback.print_exc().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/async-worker/async_worker/services/file_id_consumer.py` around lines 49
- 51, In the shutdown requeue exception path in file_id_consumer.py (the block
that currently calls logger.warning with raw_payload), replace the warning call
with logger.exception so the traceback is captured; keep the existing message
context (e.g. "Failed to requeue payload on shutdown") and include raw_payload
in the log call (use logger.exception("Failed to requeue payload on shutdown:
%s", raw_payload)) so errors during the requeue in that function/path are logged
with the full stack trace.
Comment on lines
+4
to
+8
| try: | ||
| number = int(round(float(value))) | ||
| except (TypeError, ValueError): | ||
| return None | ||
| return number if number > 0 else None |
Contributor
There was a problem hiding this comment.
Reject fractional values instead of rounding them.
positive_int() is being used to validate exact metadata fields, but round(float(value)) silently invents values. Inputs like "720.6" become 721 and "0.6" becomes 1, which can corrupt stored width/height/duration instead of rejecting invalid input.
Proposed fix
def positive_int(value) -> int | None:
if value is None or isinstance(value, bool):
return None
try:
- number = int(round(float(value)))
+ number = float(value)
except (TypeError, ValueError):
return None
- return number if number > 0 else None
+ if not number.is_integer():
+ return None
+ number = int(number)
+ return number if number > 0 else None🧰 Tools
🪛 Ruff (0.15.14)
[warning] 5-5: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/shared/fastfetchbot_shared/utils/number.py` around lines 4 - 8, The
current positive_int() uses round(float(value)) which silently converts
fractional inputs (e.g. "720.6") into integers; change it to strictly accept
only whole integers: if value is an int accept it, if it's a string accept only
when it matches an integer pattern (no decimal point, no exponent, optional
+/-), then cast with int(), and return the int only when > 0; otherwise return
None. Update the logic in positive_int (in
packages/shared/fastfetchbot_shared/utils/number.py) to replace the float-round
path with the strict integer validation described.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores