Beta #224
Beta #224
Conversation
Signed-off-by: 5hojib <yesiamshojib@gmail.com>
Signed-off-by: 5hojib <yesiamshojib@gmail.com>
Signed-off-by: 5hojib <yesiamshojib@gmail.com>
Reviewer's GuideThis PR introduces a unified NAME_PREFIX feature to replace the old LEECH_FILENAME_PREFIX, implements logic to apply that prefix to downloaded and leeched files, cleans up legacy prefix code in the uploader, and integrates an Instagram resolver into the TrueLink framework. Sequence diagram for applying NAME_PREFIX during file processingsequenceDiagram
participant Listener
participant HelperCommon
participant TaskListener
participant FileSystem
Listener->>HelperCommon: Set name_prefix from user_dict or Config
TaskListener->>HelperCommon: Call proceed_name_prefix(dl_path)
HelperCommon->>FileSystem: Rename file(s) with name_prefix if needed
FileSystem-->>HelperCommon: Return new path
HelperCommon-->>TaskListener: Return updated path
Class diagram for NAME_PREFIX unification and InstagramResolver additionclassDiagram
class Config {
JD_PASS: str
IS_TEAM_DRIVE: bool
LEECH_DUMP_CHAT: list[str]
LEECH_SPLIT_SIZE: int
MEDIA_GROUP: bool
HYBRID_LEECH: bool
INSTADL_API: str
HEROKU_APP_NAME: str
HEROKU_API_KEY: str
NAME_PREFIX: str
_convert(key, value)
}
class InstagramResolver {
DOMAINS: list[str]
resolve(url: str): LinkResult | FolderResult
}
Config <|-- InstagramResolver: uses
Class diagram for removal of LEECH_FILENAME_PREFIX and related uploader changesclassDiagram
class TelegramUploader {
_media_dict: dict
_last_msg_in_group: bool
_up_path: str
_user_dump: str
_lcaption: str
_media_group: bool
_nprefix: str
_user_settings()
_prepare_file(file_, dirpath)
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @5hojib, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new capability for the bot to download content directly from Instagram by leveraging an external InstaDL API. Alongside this new feature, it undertakes a significant refactoring of the file naming prefix functionality, transitioning from Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consolidate the NAME_PREFIX assignment and application code into a shared utility to reduce duplication in
common.py,task_listener.py, andtelegram_uploader.py. - Review the
or-chaining fallback for NAME_PREFIX to ensure empty strings or explicitly set prefixes aren’t being skipped unintentionally. - In InstagramResolver.resolve, catch more specific exceptions instead of a broad
except Exceptionso you don’t mask other errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consolidate the NAME_PREFIX assignment and application code into a shared utility to reduce duplication in `common.py`, `task_listener.py`, and `telegram_uploader.py`.
- Review the `or`-chaining fallback for NAME_PREFIX to ensure empty strings or explicitly set prefixes aren’t being skipped unintentionally.
- In InstagramResolver.resolve, catch more specific exceptions instead of a broad `except Exception` so you don’t mask other errors.
## Individual Comments
### Comment 1
<location> `bot/helper/common.py:1041-1044` </location>
<code_context>
return dl_path
+ async def proceed_name_prefix(self, dl_path):
+ def add_prefix(name):
+ if name.startswith(self.name_prefix):
+ return name
+ return f"{self.name_prefix} {name}"
+
+ if self.is_file:
+ up_dir, name = dl_path.rsplit("/", 1)
+ new_name = add_prefix(name)
+ if new_name == name:
+ return dl_path
+ new_path = ospath.join(up_dir, new_name)
+ with contextlib.suppress(Exception):
</code_context>
<issue_to_address>
**suggestion:** Consider handling cases where name_prefix is empty or whitespace.
If self.name_prefix is empty or whitespace, the function may produce filenames with unintended leading spaces. Consider skipping prefixing in these cases.
```suggestion
def add_prefix(name):
if not self.name_prefix or self.name_prefix.strip() == "":
return name
if name.startswith(self.name_prefix):
return name
return f"{self.name_prefix} {name}"
```
</issue_to_address>
### Comment 2
<location> `bot/helper/common.py:1051-1054` </location>
<code_context>
+ if new_name == name:
+ return dl_path
+ new_path = ospath.join(up_dir, new_name)
+ with contextlib.suppress(Exception):
+ await move(dl_path, new_path)
+ return new_path
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Suppressing all exceptions during file move may hide critical errors.
Logging suppressed exceptions will help identify and address issues like permission errors or failed moves, reducing the risk of silent failures.
```suggestion
new_path = ospath.join(up_dir, new_name)
try:
await move(dl_path, new_path)
except Exception as e:
import logging
logging.error(f"Failed to move file from {dl_path} to {new_path}: {e}", exc_info=True)
return new_path
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| def add_prefix(name): | ||
| if name.startswith(self.name_prefix): | ||
| return name | ||
| return f"{self.name_prefix} {name}" |
There was a problem hiding this comment.
suggestion: Consider handling cases where name_prefix is empty or whitespace.
If self.name_prefix is empty or whitespace, the function may produce filenames with unintended leading spaces. Consider skipping prefixing in these cases.
| def add_prefix(name): | |
| if name.startswith(self.name_prefix): | |
| return name | |
| return f"{self.name_prefix} {name}" | |
| def add_prefix(name): | |
| if not self.name_prefix or self.name_prefix.strip() == "": | |
| return name | |
| if name.startswith(self.name_prefix): | |
| return name | |
| return f"{self.name_prefix} {name}" |
| new_path = ospath.join(up_dir, new_name) | ||
| with contextlib.suppress(Exception): | ||
| await move(dl_path, new_path) | ||
| return new_path |
There was a problem hiding this comment.
suggestion (bug_risk): Suppressing all exceptions during file move may hide critical errors.
Logging suppressed exceptions will help identify and address issues like permission errors or failed moves, reducing the risk of silent failures.
| new_path = ospath.join(up_dir, new_name) | |
| with contextlib.suppress(Exception): | |
| await move(dl_path, new_path) | |
| return new_path | |
| new_path = ospath.join(up_dir, new_name) | |
| try: | |
| await move(dl_path, new_path) | |
| except Exception as e: | |
| import logging | |
| logging.error(f"Failed to move file from {dl_path} to {new_path}: {e}", exc_info=True) | |
| return new_path |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors LEECH_FILENAME_PREFIX to a more generic NAME_PREFIX and adds an Instagram resolver. The refactoring is a good improvement, making the prefix feature available for all downloads. However, I've identified a critical issue in the new proceed_name_prefix function related to unsafe file renaming and error suppression. There's also a high-severity bug in the user settings display logic. Additionally, I've provided some suggestions to improve code maintainability and documentation.
| async def proceed_name_prefix(self, dl_path): | ||
| def add_prefix(name): | ||
| if name.startswith(self.name_prefix): | ||
| return name | ||
| return f"{self.name_prefix} {name}" | ||
|
|
||
| if self.is_file: | ||
| up_dir, name = dl_path.rsplit("/", 1) | ||
| new_name = add_prefix(name) | ||
| if new_name == name: | ||
| return dl_path | ||
| new_path = ospath.join(up_dir, new_name) | ||
| with contextlib.suppress(Exception): | ||
| await move(dl_path, new_path) | ||
| return new_path | ||
|
|
||
| for dirpath, _, files in await sync_to_async(walk, dl_path, topdown=False): | ||
| for file_ in files: | ||
| f_path = ospath.join(dirpath, file_) | ||
| new_name = add_prefix(file_) | ||
| if new_name == file_: | ||
| continue | ||
| with contextlib.suppress(Exception): | ||
| await move(f_path, ospath.join(dirpath, new_name)) | ||
|
|
||
| return dl_path |
There was a problem hiding this comment.
This new function has two critical issues:
- Broad Exception Suppression: Using
contextlib.suppress(Exception)hides all errors during file renaming (move). If the operation fails (e.g., due to invalid characters in the prefix, permissions issues, or a full disk), the error is silently ignored. This can lead to unexpected behavior. It's better to catch specific exceptions (likeOSError) and log them. - Unsanitized Prefix in Filename: The
self.name_prefixcan contain HTML tags (a feature inherited from the oldLEECH_FILENAME_PREFIX), but these are not stripped before being used in a filename. Characters like<and>are invalid in filenames on most operating systems. This will cause themoveoperation to fail, and the failure will be hidden bysuppress(Exception).
I suggest refactoring to handle exceptions gracefully and to sanitize the prefix.
async def proceed_name_prefix(self, dl_path):
from re import sub as re_sub
# Sanitize the prefix by removing HTML tags, as they are invalid in filenames.
sanitized_prefix = re_sub("<.*?>", "", self.name_prefix).strip()
if not sanitized_prefix:
return dl_path
def add_prefix(name):
if name.startswith(sanitized_prefix):
return name
return f"{sanitized_prefix} {name}"
if self.is_file:
up_dir, name = dl_path.rsplit("/", 1)
new_name = add_prefix(name)
if new_name == name:
return dl_path
new_path = ospath.join(up_dir, new_name)
try:
await move(dl_path, new_path)
return new_path
except Exception as e:
LOGGER.error(f"Failed to apply prefix and rename '{dl_path}' to '{new_path}': {e}")
return dl_path
for dirpath, _, files in await sync_to_async(walk, dl_path, topdown=False):
for file_ in files:
f_path = ospath.join(dirpath, file_)
new_name = add_prefix(file_)
if new_name == file_:
continue
try:
await move(f_path, ospath.join(dirpath, new_name))
except Exception as e:
LOGGER.error(f"Failed to apply prefix and rename file '{f_path}': {e}")
return dl_path| if user_dict.get("NAME_PREFIX", False): | ||
| np_msg = user_dict["NAME_PREFIX"] | ||
| elif "NAME_PREFIX" not in user_dict: | ||
| np_msg = excluded_extensions | ||
| else: | ||
| np_msg = "None" | ||
| buttons.data_button("Name Prefix", f"userset {user_id} menu NAME_PREFIX") |
There was a problem hiding this comment.
There seems to be a copy-paste error in the logic for setting np_msg. When NAME_PREFIX is not defined in user_dict, np_msg is incorrectly assigned the value of excluded_extensions, which is a list of file extensions. This will result in a confusing value being displayed to the user for the "Name Prefix" setting. The logic should instead fall back to Config.NAME_PREFIX or "None".
| if user_dict.get("NAME_PREFIX", False): | |
| np_msg = user_dict["NAME_PREFIX"] | |
| elif "NAME_PREFIX" not in user_dict: | |
| np_msg = excluded_extensions | |
| else: | |
| np_msg = "None" | |
| buttons.data_button("Name Prefix", f"userset {user_id} menu NAME_PREFIX") | |
| if user_dict.get("NAME_PREFIX"): | |
| np_msg = user_dict["NAME_PREFIX"] | |
| elif "NAME_PREFIX" not in user_dict and Config.NAME_PREFIX: | |
| np_msg = Config.NAME_PREFIX | |
| else: | |
| np_msg = "None" | |
| buttons.data_button("Name Prefix", f"userset {user_id} menu NAME_PREFIX") |
| self.name_prefix = ( | ||
| self.name_prefix | ||
| or self.user_dict.get("NAME_PREFIX", False) | ||
| or (Config.NAME_PREFIX if "NAME_PREFIX" not in self.user_dict else "") | ||
| ) |
There was a problem hiding this comment.
The logic for setting self.name_prefix is a bit complex and hard to follow due to chained or operators and a nested ternary expression. For better readability and maintainability, consider refactoring this into a more explicit if/elif/else structure.
if self.name_prefix:
pass # Already set by command-line arg
elif "NAME_PREFIX" in self.user_dict:
self.name_prefix = self.user_dict["NAME_PREFIX"]
else:
self.name_prefix = Config.NAME_PREFIX| if self.name_prefix: | ||
| up_path = await self.proceed_name_prefix(up_path) | ||
| if self.is_cancelled: | ||
| return | ||
| self.is_file = await aiopath.isfile(up_path) | ||
| self.name = up_path.replace(f"{up_dir}/", "").split("/", 1)[0] | ||
|
|
There was a problem hiding this comment.
The lines to update self.is_file and self.name are repeated three times in this function (before this block, inside this block, and after). This code duplication can make maintenance more difficult. Consider refactoring this logic into a private helper method to avoid repetition. For example:
async def _update_path_details(self, up_path, up_dir):
self.is_file = await aiopath.isfile(up_path)
self.name = up_path.replace(f"{up_dir}/", "").split("/", 1)[0]Then you can call await self._update_path_details(up_path, up_dir) in the respective places.
| | `HEROKU_APP_NAME` | `str` | Name of your Heroku app, used to get `BASE_URL` automatically. | | ||
| | `HEROKU_API_KEY` | `str` | API key for accessing and controlling your Heroku app. | No newline at end of file | ||
| | `HEROKU_API_KEY` | `str` | API key for accessing and controlling your Heroku app. | | ||
| | `NAME_PREFIX` | `str` | Name prefix. | No newline at end of file |
There was a problem hiding this comment.
The description for NAME_PREFIX is very brief. To help users understand its purpose, consider expanding it to explain what it does.
| | `NAME_PREFIX` | `str` | Name prefix. | | |
| | `NAME_PREFIX` | `str` | Prefix to add to all downloaded file/folder names. | |
1 participant
Summary by Sourcery
Introduce a new NAME_PREFIX feature to allow custom filename prefixes throughout the download and upload pipeline, remove the deprecated LEECH_FILENAME_PREFIX setting, implement prefix application logic, and integrate an InstagramResolver for video downloads via InstaDL API.
New Features:
Enhancements:
Documentation: