refactor: info command #1170
refactor: info command #1170
Conversation
…g, and moderation improvements - Added new items to the TODO list, including translation command, invite tracking, and a data expiration policy for guilds. - Included enhancements for the help command with slash support and a redesign of the atl rolecount plugin for global use. - Introduced an audit logging system and a function to parse all times, along with a moderation command refactor to improve target argument handling.
…ntity lookups - Reorganized the `/info` command group to provide specific subcommands for different Discord entity types, enhancing clarity and usability. - Updated documentation to reflect the new subcommand structure, including examples for each entity type such as server, user, emoji, role, channel, and invite. - Improved command syntax and examples for better user guidance when retrieving information about Discord entities.
…g methods - Removed `_check_tables_exist` and `_stamp_database_if_needed` methods from `DatabaseSetupService`, streamlining the database setup process. - Updated error handling in the `_upgrade_head_if_needed` method to provide clearer guidance for migration failures, particularly for inconsistent database states. - Ensured Alembic is the single source of truth for database schema management, enhancing overall reliability and maintainability.
- Introduced new builders for creating detailed views of Discord entities, including guilds, members, channels, roles, emojis, and invites, utilizing Components V2 for improved user interaction. - Added utility functions for sending responses and error messages, streamlining the response handling process in info commands. - Refactored the info command structure to leverage the new builders, enhancing the clarity and organization of information displayed to users. - Implemented helper functions for formatting and processing Discord objects, improving the overall functionality and maintainability of the info module.
…ory modules - Added functionality to check for a setup function in the parent directory's module file, improving the extension loading process. - Introduced logging for found modules to aid in debugging and traceability of loaded extensions.
Reviewer's GuideRefactors the info command system from a monolithic embed/reactionmenu implementation into modular Components V2 views with dedicated helpers/builders, adds pagination via LayoutView, adjusts docs accordingly, simplifies database setup by removing auto-stamping logic and improving migration error messaging, enhances hot-reload extension resolution, and adds unit tests for new helper utilities. Sequence diagram for info_server subcommand with ID and invite handlingsequenceDiagram
actor User
participant DiscordClient
participant TuxBot as TuxBot
participant InfoCog as Info
participant Builders as build_guild_view
participant UtilsSendView as send_view
participant Helpers as extract_invite_code
participant InviteConverter as InviteConverter
User->>DiscordClient: /info server guild_input
DiscordClient->>TuxBot: ApplicationCommandInvoke
TuxBot->>InfoCog: info_server(ctx, guild_input)
alt interaction_based
InfoCog->>TuxBot: ctx.interaction.defer(ephemeral=True)
end
alt guild_input_is_none
InfoCog->>InfoCog: guild = ctx.guild
InfoCog->>Builders: build_guild_view(guild)
Builders-->>InfoCog: view
InfoCog->>UtilsSendView: send_view(ctx, view)
UtilsSendView-->>DiscordClient: respond with LayoutView
else guild_input_is_guild_id
InfoCog->>TuxBot: bot.get_guild(int(guild_input))
alt guild_found
InfoCog->>Builders: build_guild_view(guild)
Builders-->>InfoCog: view
InfoCog->>UtilsSendView: send_view(ctx, view)
UtilsSendView-->>DiscordClient: respond with LayoutView
else guild_not_found_try_invite
InfoCog->>Helpers: extract_invite_code(guild_input)
Helpers-->>InfoCog: extracted_code
InfoCog->>InviteConverter: convert(ctx, extracted_code)
alt invite_valid_and_guild_present
InviteConverter-->>InfoCog: invite
InfoCog->>TuxBot: bot.get_guild(invite.guild.id)
alt guild_joined
InfoCog->>Builders: build_guild_view(guild)
Builders-->>InfoCog: view
InfoCog->>UtilsSendView: send_view(ctx, view)
else guild_not_joined
InfoCog->>UtilsSendView: send_error(ctx, "not in server from invite")
end
else invite_without_guild
InfoCog->>UtilsSendView: send_error(ctx, "invite does not point to a server")
end
end
else invalid_input
InfoCog->>UtilsSendView: send_error(ctx, "could not find server by ID or invite")
end
Class diagram for refactored info command moduleclassDiagram
class Tux {
}
class BaseCog {
}
class Info {
+Info(bot:Tux)
+info(ctx:commands.Context)
+info_server(ctx:commands.Context, guild_input:str|None)
+info_user(ctx:commands.Context, entity:str)
+info_emoji(ctx:commands.Context, emoji:discord.Emoji)
+info_role(ctx:commands.Context, role:discord.Role)
+info_channel(ctx:commands.Context, channel:discord.abc.GuildChannel|discord.Thread)
+info_invite(ctx:commands.Context, invite_code:str)
}
class builders {
<<module>>
+build_guild_view(guild:discord.Guild) discord.ui.LayoutView
+build_member_view(member:discord.Member, bot:Tux) discord.ui.LayoutView
+build_user_view(user:discord.User) discord.ui.LayoutView
+build_channel_view(channel:discord.abc.GuildChannel) discord.ui.LayoutView
+build_role_view(role:discord.Role) discord.ui.LayoutView
+build_emoji_view(emoji:discord.Emoji) discord.ui.LayoutView
+build_invite_view(invite:discord.Invite) discord.ui.LayoutView
+build_thread_view(thread:discord.Thread) discord.ui.LayoutView
}
class helpers {
<<module>>
+format_bool(value:bool) str
+format_datetime(dt:datetime|None, style:TimestampStyle) str
+format_date_long(dt:datetime|None) str
+format_permissions(permissions:discord.Permissions) str
+format_guild_verification_level(level:discord.VerificationLevel) str
+format_guild_nsfw_level(guild:discord.Guild) str
+format_guild_content_filter(level:discord.ContentFilter) str
+format_guild_notifications(level:discord.NotificationLevel) str
+format_guild_premium_tier(tier:object) str
+format_invite_uses(invite:discord.Invite) str
+format_invite_max_age(max_age:int|None) str
+extract_invite_code(invite_input:str) str
+count_guild_members(guild:discord.Guild) tuple[int,int]
+count_guild_bans(guild:discord.Guild) int
+build_guild_channel_counts(guild:discord.Guild) str
+build_guild_member_stats(guild:discord.Guild, humans:int, bots:int, ban_count:int) str
+build_guild_special_channels(guild:discord.Guild) str
+add_guild_title_section(container:discord.ui.Container, guild:discord.Guild) void
+add_guild_basic_info_section(container:discord.ui.Container, guild:discord.Guild, tier_text:str) void
+add_guild_security_section(container:discord.ui.Container, verification_text:str, mfa_text:str, nsfw_text:str, content_filter_text:str, notification_text:str) void
+add_guild_channels_section(container:discord.ui.Container, channel_counts:str, special_channels_text:str) void
+add_guild_resources_section(container:discord.ui.Container, guild:discord.Guild) void
+add_guild_members_section(container:discord.ui.Container, member_stats:str) void
+add_guild_footer_section(container:discord.ui.Container, guild:discord.Guild) void
+add_guild_media(container:discord.ui.Container, guild:discord.Guild) void
+add_invite_statistics(container:discord.ui.Container, invite:discord.Invite) void
+add_invite_target_info(container:discord.ui.Container, invite:discord.Invite) void
+add_invite_scheduled_event(container:discord.ui.Container, invite:discord.Invite) void
+add_text_channel_info(container:discord.ui.Container, channel:discord.TextChannel) void
+add_voice_channel_info(container:discord.ui.Container, channel:discord.VoiceChannel) void
+add_stage_channel_info(container:discord.ui.Container, channel:discord.StageChannel) void
+add_forum_channel_info(container:discord.ui.Container, channel:discord.ForumChannel) void
+add_category_channel_info(container:discord.ui.Container, channel:discord.CategoryChannel) void
+get_role_type_info(role:discord.Role) list[tuple[str,bool]]
+get_role_tags_info(role:discord.Role) list[str]|None
+get_role_flags_info(role:discord.Role) list[str]|None
+get_user_banner(user:discord.User) str|None
+get_member_banner(member:discord.Member, bot:Tux) str|None
+chunks(it:Iterator[str], size:int) Generator[list[str]]
}
class utils_module {
<<module>>
+send_view(ctx:commands.Context, view:discord.ui.LayoutView, ephemeral:bool) None
+send_error(ctx:commands.Context, error_msg:str, ephemeral:bool) None
}
class InfoPaginatorView {
+CONTENT_ID:int
+PAGE_INFO_ID:int
+items:list[str]
+chunks_list:list[list[str]]
+current_page:int
+title:str
+list_type:str
+guild_name:str
+__init__(items:Iterable[str], title:str, list_type:str, guild_name:str, chunk_size:int, timeout:float)
+_build_layout() None
+_get_page_info() str
+_build_navigation() discord.ui.ActionRow
+_handle_first(interaction:discord.Interaction) None
+_handle_prev(interaction:discord.Interaction) None
+_handle_next(interaction:discord.Interaction) None
+_handle_last(interaction:discord.Interaction) None
+_handle_close(interaction:discord.Interaction) None
+_update_page(interaction:discord.Interaction) None
+_update_navigation_buttons() None
}
class LayoutView {
}
class file_utils_module {
<<module>>
+get_extension_from_path(file_path:Path, base_dir:Path) str|None
}
class InfoPackageInit {
<<module>>
+Info
+InfoPaginatorView
+send_error
+send_view
}
Info --|> BaseCog
Info ..> builders : uses
Info ..> utils_module : uses
Info ..> helpers : uses_extract_invite_code
InfoPaginatorView --|> LayoutView
InfoPaginatorView ..> helpers : uses_chunks
builders ..> helpers : uses
builders ..> Tux : uses
utils_module ..> Tux : uses
file_utils_module ..> Info : may_load_cog
InfoPackageInit ..> Info
InfoPackageInit ..> InfoPaginatorView
InfoPackageInit ..> utils_module
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRefactors the info command into explicit hybrid subcommands with Components V2 views/builders/helpers, replaces reaction pagination with a paginator view, centralizes view/error sending utilities, adds tests and docs updates, and removes manual DB stamping in favor of Alembic-only migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Cmd as Info Subcommand
participant Builder as View Builder
participant View as InfoPaginator / LayoutView
participant Util as send_view / send_error
participant Discord as Discord API
User->>Cmd: /info server (or subcommand)
activate Cmd
Cmd->>Builder: build_*_view(target)
activate Builder
Builder->>Builder: gather & format data (helpers)
Builder-->>Cmd: discord.ui.LayoutView
deactivate Builder
Cmd->>Util: send_view(ctx, layout_view)
activate Util
alt interaction (slash)
Util->>Discord: response.send_message / followup.send (ephemeral if requested)
else prefix
Util->>Discord: ctx.send (channel message)
end
deactivate Util
deactivate Cmd
Note over User,View: User interacts with components (pagination, buttons)
User->>View: press navigation button
View->>Discord: acknowledge interaction / update component
Discord->>User: updated layout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (10)
✏️ Tip: You can disable this entire section by setting 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 |
Summary of ChangesHello @kzndotsh, 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 delivers a significant overhaul of the bot's 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
|
📚 Documentation Preview
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 7 issues, and left some high level feedback:
- The new helpers module has grown quite large (1000+ lines) and mixes guild, role, channel, invite, and user helpers; consider splitting it into smaller, domain-focused modules (e.g. guild_helpers, role_helpers, channel_helpers) to keep each file easier to navigate and maintain.
- In
build_member_viewthebanner_urlvariable is computed but only referenced in a commented-out block; either wire it into the MediaGallery as planned or remove the assignment for now to avoid confusion about unused data.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new helpers module has grown quite large (1000+ lines) and mixes guild, role, channel, invite, and user helpers; consider splitting it into smaller, domain-focused modules (e.g. guild_helpers, role_helpers, channel_helpers) to keep each file easier to navigate and maintain.
- In `build_member_view` the `banner_url` variable is computed but only referenced in a commented-out block; either wire it into the MediaGallery as planned or remove the assignment for now to avoid confusion about unused data.
## Individual Comments
### Comment 1
<location> `src/tux/modules/info/builders.py:129-138` </location>
<code_context>
+ return view
+
+
+async def build_member_view(member: discord.Member, bot: Tux) -> discord.ui.LayoutView:
+ """Build a Components V2 view for member information.
+
+ Parameters
+ ----------
+ member : discord.Member
+ The member to display information about.
+ bot : Tux
+ The bot instance for fetching user data.
+
+ Returns
+ -------
+ discord.ui.LayoutView
+ The built view.
+ """
+ banner_url = await get_member_banner(member, bot) # noqa: F841 # pyright: ignore[reportUnusedVariable]
+
+ # Build username display
</code_context>
<issue_to_address>
**issue (performance):** Avoid fetching the member banner if it is not actually used to prevent unnecessary API calls.
`build_member_view` calls `get_member_banner` on every invocation, but `banner_url` is unused and suppressed with noqa/pyright. This adds an extra REST call per member view with no user benefit and increases rate-limit risk.
Either connect the banner to the view now (e.g. via a `MediaGallery`/`Thumbnail` accessory), or remove this fetch and instead resolve the banner lazily where it is actually rendered.
</issue_to_address>
### Comment 2
<location> `tests/modules/test_info.py:40-43` </location>
<code_context>
+ assert "<t:" in result or ":" in result
+
+
+def test_format_datetime_naive_uses_utc() -> None:
+ dt = datetime(2025, 1, 25, 12, 0, 0, tzinfo=UTC)
+ result = format_datetime(dt)
+ assert result != "Unknown"
+
+
</code_context>
<issue_to_address>
**issue (testing):** Test name suggests naive datetime handling but uses an aware datetime instead
This test uses an aware `datetime` (`tzinfo=UTC`), so it never exercises the naive-to-UTC conversion path. The name is misleading, and the assertion (`!= 'Unknown'`) doesn’t verify UTC handling. Please either rename the test to indicate it covers aware datetimes, or better, make `dt` naive and assert that `format_datetime` ultimately calls `discord.utils.format_dt` with a UTC-aware value (e.g., via timestamp inspection or mocking).
</issue_to_address>
### Comment 3
<location> `tests/modules/test_info.py:10-15` </location>
<code_context>
+
+import pytest
+
+from tux.modules.info.helpers import (
+ chunks,
+ extract_invite_code,
+ format_bool,
+ format_date_long,
+ format_datetime,
+)
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding tests for `format_permissions`, especially truncation and empty-permission cases
`format_permissions` has non-trivial behavior (building a comma-separated list and truncating to fit Discord’s 1024-character limit) but currently lacks tests. Please add unit tests for at least: (1) no enabled permissions (should return `"None"`), and (2) enough enabled permissions to exceed 1024 characters to validate truncation and ellipsis handling.
Suggested implementation:
```python
from tux.modules.info.helpers import (
chunks,
extract_invite_code,
format_bool,
format_date_long,
format_datetime,
format_permissions,
)
def test_format_permissions_no_enabled_permissions() -> None:
"""format_permissions should return 'None' when there are no enabled permissions."""
assert format_permissions([]) == "None"
def test_format_permissions_truncates_long_permission_list() -> None:
"""format_permissions should truncate long permission lists and add an ellipsis."""
# Build a list of permission names long enough that their comma-separated
# representation will exceed Discord's 1024-character field limit.
many_permissions = [f"very_long_permission_name_{i:03d}" for i in range(300)]
result = format_permissions(many_permissions)
# Ensure we actually hit the truncation logic.
assert len(result) <= 1024
# The string should end with some form of ellipsis to indicate truncation.
assert result.endswith("…") or result.endswith("...")
# The truncated string should still start with the first permission name.
assert result.startswith("very_long_permission_name_000")
```
If `format_permissions` does not accept a `list[str]` but instead takes a different structure (e.g. a `discord.Permissions` object or a mapping of permission names to booleans), adjust the test inputs accordingly:
1. For `test_format_permissions_no_enabled_permissions`, pass whatever represents "no enabled permissions" in your implementation (e.g. `Permissions.none()`, `{}`, or similar) instead of `[]`.
2. For `test_format_permissions_truncates_long_permission_list`, construct an input value that results in a comma-separated permission string longer than 1024 characters when processed by `format_permissions` (e.g. many enabled flags on a `Permissions` instance, or a larger list/dict of permission names).
3. If your implementation uses a specific ellipsis character (only `"…"` or only `"..."`), you can simplify the `endswith` assertion to match exactly what your helper produces.
</issue_to_address>
### Comment 4
<location> `TODO.md:45` </location>
<code_context>
+Invite tracking
+Define data expiration policy for guilds
+Help command slash support
+Redesign atl rolecount plugin for global use
+Audit logging system
+Use one function to parse all times #420
</code_context>
<issue_to_address>
**issue (typo):** “atl” looks like it might be a typo for “alt” in this TODO entry.
In `Redesign atl rolecount plugin for global use`, change “atl” to “alt” if you mean “alternative rolecount plugin.” If “atl” is an internal acronym, consider documenting it for clarity.
```suggestion
Redesign alt rolecount plugin for global use
```
</issue_to_address>
### Comment 5
<location> `src/tux/modules/info/helpers.py:404` </location>
<code_context>
+ return ", ".join(special_channels) if special_channels else "None"
+
+
+def add_guild_title_section(
+ container: discord.ui.Container[discord.ui.LayoutView],
+ guild: discord.Guild,
</code_context>
<issue_to_address>
**issue (complexity):** Consider merging several tiny guild, role, and banner helpers into a few cohesive builders to make the control flow easier to follow and reduce indirection.
You can keep the centralization benefit while reducing indirection by merging some of the ultra‑granular helpers into a few cohesive units.
### 1. Collapse guild section helpers into a single composer
All the `add_guild_*_section` functions are thin wrappers around `TextDisplay` + `Separator`, used only in the guild view. They can be collapsed into a single helper that composes the full guild body, so `build_guild_view` only calls one function and readers don’t have to hop around.
For example:
```py
def build_guild_sections(
container: discord.ui.Container[discord.ui.LayoutView],
guild: discord.Guild,
tier_text: str,
verification_text: str,
mfa_text: str,
nsfw_text: str,
content_filter_text: str,
notification_text: str,
channel_counts: str,
special_channels_text: str,
member_stats: str,
) -> None:
description = guild.description or "No description available."
title_content = f"# {guild.name}\n\n{description}"
# title + thumbnail
if guild.icon:
container.add_item(
discord.ui.Section(
discord.ui.TextDisplay(title_content),
accessory=discord.ui.Thumbnail(media=guild.icon.url),
),
)
else:
container.add_item(discord.ui.TextDisplay(title_content))
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# basic info
owner_text = guild.owner.mention if guild.owner else "Unknown"
container.add_item(
discord.ui.TextDisplay(
f"### Basic Information\n"
f"👑 **Owner:** {owner_text} • "
f"🔗 **Vanity URL:** {guild.vanity_url_code or 'None'} • "
f"💎 **Premium Tier:** {tier_text}\n"
f"⭐ **Boosts:** {guild.premium_subscription_count}"
),
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# security
container.add_item(
discord.ui.TextDisplay(
f"### Security & Settings\n"
f"🔒 **Verification:** {verification_text} • "
f"🛡️ **MFA Level:** {mfa_text} • "
f"🔞 **NSFW Level:** {nsfw_text}\n"
f"🚫 **Content Filter:** {content_filter_text} • "
f"🔔 **Notifications:** {notification_text}"
),
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# channels
container.add_item(
discord.ui.TextDisplay(
f"### Channels\n"
f"📝 **Channels:** {channel_counts}\n"
f"📍 **Special Channels:** {special_channels_text}",
),
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# resources
container.add_item(
discord.ui.TextDisplay(
f"### Resources\n"
f"😀 **Emojis:** {len(guild.emojis)}/{2 * guild.emoji_limit} • "
f"🎨 **Stickers:** {len(guild.stickers)}/{guild.sticker_limit} • "
f"🎭 **Roles:** {len(guild.roles)}",
),
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# members
container.add_item(discord.ui.TextDisplay(f"### Members\n{member_stats}"))
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# footer
footer_text = (
f"🆔 **ID:** `{guild.id}` • "
f"📅 **Created:** {format_date_long(guild.created_at)}"
)
container.add_item(discord.ui.TextDisplay(footer_text))
```
Then `build_guild_view` can call just `build_guild_sections(...)` (and optionally a separate `add_guild_media` if you want media clearly separated), instead of 6–7 small helpers.
This keeps all functionality, but shrinks the public helper surface and makes the view flow visible in one place.
---
### 2. Merge role helpers into a single metadata builder
`get_role_type_info`, `get_role_tags_info`, and `get_role_flags_info` are only used by the role view and each returns tiny bits of related data. Collapsing them into a single “role metadata” helper removes three jumps:
```py
def get_role_metadata(role: discord.Role) -> list[str]:
info: list[str] = []
# type info
if hasattr(role, "is_default") and role.is_default():
info.append("Default Role")
if hasattr(role, "is_bot_managed") and role.is_bot_managed():
info.append("Bot Managed")
if hasattr(role, "is_integration") and role.is_integration():
info.append("Integration Managed")
if hasattr(role, "is_premium_subscriber") and role.is_premium_subscriber():
info.append("Premium Subscriber")
if hasattr(role, "is_assignable") and not role.is_assignable():
info.append("Not Assignable")
# tags
tags = getattr(role, "tags", None)
if tags:
if getattr(tags, "bot_id", None):
info.append(f"Bot: <@{tags.bot_id}>")
if getattr(tags, "integration_id", None):
info.append(f"Integration: {tags.integration_id}")
if getattr(tags, "subscription_listing_id", None):
info.append("Premium Subscriber Role")
# flags
flags = getattr(role, "flags", None)
if flags:
if getattr(flags, "inverted", False):
info.append("Inverted")
if getattr(flags, "mentionable_by_everyone", False):
info.append("Mentionable by Everyone")
return info
```
`build_role_view` can then consume a single list and join it however it currently does, with no behavior change.
---
### 3. Simplify `get_member_banner` control flow
The current implementation uses `suppress`, nested checks, and exception handling intertwined. You can keep the fetch‑then‑cache semantics with clearer linear logic and early returns:
```py
async def get_member_banner(member: discord.Member, bot: Tux) -> str | None:
async def extract_banner(user: discord.abc.User) -> str | None:
banner = getattr(user, "banner", None)
url = getattr(banner, "url", None)
return url if isinstance(url, str) and url else None
# try fetch first
try:
user = await bot.fetch_user(member.id)
if banner_url := extract_banner(user):
return banner_url
except discord.NotFound:
pass
except Exception:
# fall back to cache on fetch failure
pass
if cached_user := bot.get_user(member.id):
return extract_banner(cached_user)
return None
```
This keeps all existing behavior (fetch first, cache fallback, ignore failures) but reduces branching and removes the `suppress` context, making the function easier to reason about.
</issue_to_address>
### Comment 6
<location> `src/tux/modules/info/builders.py:59` </location>
<code_context>
+ return view, container
+
+
+def _add_section(
+ container: ContainerT,
+ heading: str,
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying these builders by inlining the small layout helpers and co-locating one-off helper functions so each `build_*_view` fully describes its own UI structure.
You can keep the builder separation while reducing ceremony by (a) co-locating one‑off helpers with their builders and (b) inlining the “micro” helpers that only wrap `TextDisplay`/`Separator`.
### 1. Inline `_add_section` / `_add_footer` where they’re just formatting sugar
These two are adding extra indirection for a very small gain. Inlining them makes each builder’s layout self‑describing.
For example, in `build_user_view`:
```py
# current
_add_section(
container,
"Basic Information",
f"👤 **Username:** `{username_display}` • "
f"🆔 **ID:** `{user.id}` • "
f"🤖 **Bot:** {format_bool(user.bot)}",
)
_add_section(
container,
"Dates",
f"📅 **Registered:** {format_datetime(user.created_at)}",
add_sep=False,
)
```
You can inline the layout and remove the helper:
```py
container.add_item(
discord.ui.TextDisplay(
"### Basic Information\n"
f"👤 **Username:** `{username_display}` • "
f"🆔 **ID:** `{user.id}` • "
f"🤖 **Bot:** {format_bool(user.bot)}"
)
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
container.add_item(
discord.ui.TextDisplay(
"### Dates\n"
f"📅 **Registered:** {format_datetime(user.created_at)}"
)
)
# no separator here
```
And similarly in `build_channel_view`, `build_emoji_view`, etc. Once inlined everywhere, you can drop `_add_section` and `_add_footer` entirely and keep only `_create_info_view`, which actually removes duplication.
For the footer, e.g. in `build_thread_view`:
```py
# current
_add_footer(container, f"`{thread.id}`", format_date_long(thread.created_at))
```
Inline:
```py
container.add_item(
discord.ui.TextDisplay(
f"🆔 **ID:** `{thread.id}` • "
f"📅 **Created:** {format_date_long(thread.created_at)}"
)
)
```
This keeps functionality while making the structure of each view visible where it’s used.
### 2. Co-locate one‑off helpers with their builders
Helpers like `get_role_type_info`, `get_role_tags_info`, `get_role_flags_info`, and even some `add_*_section` helpers look role‑specific and are only used by `build_role_view`. Moving them into this module (or even as small inner functions) removes cross‑file jumping without reverting the builder pattern.
For example, instead of importing:
```py
from .helpers import (
get_role_flags_info,
get_role_tags_info,
get_role_type_info,
)
```
you can put minimal helpers next to `build_role_view`:
```py
def _get_role_type_info(role: discord.Role) -> list[tuple[str, bool]]:
# same logic as helpers.get_role_type_info
...
def _get_role_tags_info(role: discord.Role) -> list[str] | None:
# same logic as helpers.get_role_tags_info
...
def _get_role_flags_info(role: discord.Role) -> list[str] | None:
# same logic as helpers.get_role_flags_info
...
```
and then:
```py
if role_type_info := _get_role_type_info(role):
type_parts = [f"**{name}:** {format_bool(value)}" for name, value in role_type_info]
container.add_item(
discord.ui.TextDisplay(
"### Role Type\n" + " • ".join(type_parts)
)
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
```
Apply the same idea to guild/channel/emoji helpers that are only used by a single `build_*_view`. Shared/complex pieces (like `count_guild_members`, `format_*` utilities) can stay in `helpers.py`, but layout‑specific text/sections become local, making each builder much easier to read in isolation.
### 3. Keep most layout declaration inline, use helpers only for data/formatting
You already have a nice separation where helpers compute/format values:
```py
channel_counts = build_guild_channel_counts(guild)
member_stats = build_guild_member_stats(guild, humans, bots, ban_count)
special_channels_text = build_guild_special_channels(guild)
```
You can keep these, but inline the actual section structure in `build_guild_view` instead of `add_guild_*_section` helpers.
For example, instead of:
```py
add_guild_channels_section(container, channel_counts, special_channels_text)
```
inline something like:
```py
container.add_item(
discord.ui.TextDisplay(
"### Channels\n"
f"{channel_counts}\n\n"
f"{special_channels_text}"
)
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
```
This preserves the builder layer and shared data helpers while making the “shape” of the view obvious in a single function, which is where maintainers will look first.
</issue_to_address>
### Comment 7
<location> `src/tux/modules/info/views.py:10` </location>
<code_context>
+from tux.modules.info.helpers import chunks
+
+
+class InfoPaginatorView(discord.ui.LayoutView):
+ """Components V2 pagination view for info command lists.
+
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the paginator by storing direct references to UI elements, centralizing page-change logic, and avoiding ID-based lookups and tree walking.
You can simplify this without changing behavior by keeping direct references and centralizing page-change logic.
### 1. Avoid integer IDs + `find_item` for TextDisplays
Instead of `CONTENT_ID`, `PAGE_INFO_ID`, and `find_item`, keep attribute references when building the layout:
```python
class InfoPaginatorView(discord.ui.LayoutView):
def __init__(...):
super().__init__(timeout=timeout)
...
self._content_display: discord.ui.TextDisplay | None = None
self._page_info_display: discord.ui.TextDisplay | None = None
self._first_btn = self._prev_btn = None
self._next_btn = self._last_btn = None
self._build_layout()
```
```python
def _build_layout(self) -> None:
self.clear_items()
container = discord.ui.Container[discord.ui.LayoutView](accent_color=0x5865F2)
...
self._content_display = discord.ui.TextDisplay(f"# {self.title}\n\n{content_text}")
container.add_item(self._content_display)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
self._page_info_display = discord.ui.TextDisplay(self._get_page_info())
container.add_item(self._page_info_display)
self.add_item(container)
if len(self.chunks_list) > 1:
nav_row = self._build_navigation()
self.add_item(nav_row)
```
Then `_update_page` can be simplified to direct attribute access:
```python
async def _update_page(self, interaction: discord.Interaction) -> None:
if self._content_display is not None:
if not self.chunks_list:
content_text = "No items available."
else:
current_chunk = self.chunks_list[self.current_page]
content_text = (
f"### {self.list_type.capitalize()} list for {self.guild_name}\n\n"
f"{' '.join(current_chunk)}"
)
self._content_display.content = f"# {self.title}\n\n{content_text}"
if self._page_info_display is not None:
self._page_info_display.content = self._get_page_info()
self._update_navigation_buttons()
await interaction.response.edit_message(view=self)
```
### 2. Avoid `walk_children` and `custom_id` checks for navigation buttons
Store button references when building the navigation row and update them directly:
```python
def _build_navigation(self) -> discord.ui.ActionRow[discord.ui.LayoutView]:
nav_row = discord.ui.ActionRow[discord.ui.LayoutView]()
total_pages = len(self.chunks_list)
self._first_btn = discord.ui.Button[discord.ui.LayoutView](
label="⏮️ First",
style=discord.ButtonStyle.secondary,
disabled=self.current_page == 0,
)
self._first_btn.callback = self._handle_first
nav_row.add_item(self._first_btn)
self._prev_btn = discord.ui.Button[discord.ui.LayoutView](
label="⬅️ Previous",
style=discord.ButtonStyle.secondary,
disabled=self.current_page == 0,
)
self._prev_btn.callback = self._handle_prev
nav_row.add_item(self._prev_btn)
self._next_btn = discord.ui.Button[discord.ui.LayoutView](
label="➡️ Next",
style=discord.ButtonStyle.secondary,
disabled=self.current_page >= total_pages - 1,
)
self._next_btn.callback = self._handle_next
nav_row.add_item(self._next_btn)
self._last_btn = discord.ui.Button[discord.ui.LayoutView](
label="⏭️ Last",
style=discord.ButtonStyle.secondary,
disabled=self.current_page >= total_pages - 1,
)
self._last_btn.callback = self._handle_last
nav_row.add_item(self._last_btn)
close_btn = discord.ui.Button[discord.ui.LayoutView](
label="❌ Close",
style=discord.ButtonStyle.danger,
)
close_btn.callback = self._handle_close
nav_row.add_item(close_btn)
return nav_row
```
```python
def _update_navigation_buttons(self) -> None:
total_pages = len(self.chunks_list)
at_first = self.current_page == 0
at_last = self.current_page >= total_pages - 1
if self._first_btn:
self._first_btn.disabled = at_first
if self._prev_btn:
self._prev_btn.disabled = at_first
if self._next_btn:
self._next_btn.disabled = at_last
if self._last_btn:
self._last_btn.disabled = at_last
```
This removes `walk_children`, `custom_id` strings, and makes the navigation row explicit.
### 3. Consolidate page-change logic to reduce handler boilerplate
All four nav handlers share the same pattern. A small helper keeps behavior identical but cuts branching duplication:
```python
async def _change_page(self, interaction: discord.Interaction, target: int) -> None:
# Clamp to valid range
target = max(0, min(target, max(len(self.chunks_list) - 1, 0)))
if target == self.current_page:
await interaction.response.defer()
return
self.current_page = target
await self._update_page(interaction)
```
Then the handlers become:
```python
async def _handle_first(self, interaction: discord.Interaction) -> None:
await self._change_page(interaction, 0)
async def _handle_prev(self, interaction: discord.Interaction) -> None:
await self._change_page(interaction, self.current_page - 1)
async def _handle_next(self, interaction: discord.Interaction) -> None:
await self._change_page(interaction, self.current_page + 1)
async def _handle_last(self, interaction: discord.Interaction) -> None:
await self._change_page(interaction, len(self.chunks_list) - 1)
```
### 4. Minor: `_build_layout` only needs to run once
Since `_build_layout` is only called from `__init__` and the structure is fixed, you can safely drop `self.clear_items()` unless you anticipate reuse:
```python
def _build_layout(self) -> None:
# self.clear_items() # not needed if layout is built once
...
```
These changes should keep all existing behavior while making the paginator more direct and easier to follow.
</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 test_format_datetime_naive_uses_utc() -> None: | ||
| dt = datetime(2025, 1, 25, 12, 0, 0, tzinfo=UTC) | ||
| result = format_datetime(dt) | ||
| assert result != "Unknown" |
There was a problem hiding this comment.
issue (testing): Test name suggests naive datetime handling but uses an aware datetime instead
This test uses an aware datetime (tzinfo=UTC), so it never exercises the naive-to-UTC conversion path. The name is misleading, and the assertion (!= 'Unknown') doesn’t verify UTC handling. Please either rename the test to indicate it covers aware datetimes, or better, make dt naive and assert that format_datetime ultimately calls discord.utils.format_dt with a UTC-aware value (e.g., via timestamp inspection or mocking).
| from tux.modules.info.helpers import ( | ||
| chunks, | ||
| extract_invite_code, | ||
| format_bool, | ||
| format_date_long, | ||
| format_datetime, |
There was a problem hiding this comment.
suggestion (testing): Consider adding tests for format_permissions, especially truncation and empty-permission cases
format_permissions has non-trivial behavior (building a comma-separated list and truncating to fit Discord’s 1024-character limit) but currently lacks tests. Please add unit tests for at least: (1) no enabled permissions (should return "None"), and (2) enough enabled permissions to exceed 1024 characters to validate truncation and ellipsis handling.
Suggested implementation:
from tux.modules.info.helpers import (
chunks,
extract_invite_code,
format_bool,
format_date_long,
format_datetime,
format_permissions,
)
def test_format_permissions_no_enabled_permissions() -> None:
"""format_permissions should return 'None' when there are no enabled permissions."""
assert format_permissions([]) == "None"
def test_format_permissions_truncates_long_permission_list() -> None:
"""format_permissions should truncate long permission lists and add an ellipsis."""
# Build a list of permission names long enough that their comma-separated
# representation will exceed Discord's 1024-character field limit.
many_permissions = [f"very_long_permission_name_{i:03d}" for i in range(300)]
result = format_permissions(many_permissions)
# Ensure we actually hit the truncation logic.
assert len(result) <= 1024
# The string should end with some form of ellipsis to indicate truncation.
assert result.endswith("…") or result.endswith("...")
# The truncated string should still start with the first permission name.
assert result.startswith("very_long_permission_name_000")If format_permissions does not accept a list[str] but instead takes a different structure (e.g. a discord.Permissions object or a mapping of permission names to booleans), adjust the test inputs accordingly:
- For
test_format_permissions_no_enabled_permissions, pass whatever represents "no enabled permissions" in your implementation (e.g.Permissions.none(),{}, or similar) instead of[]. - For
test_format_permissions_truncates_long_permission_list, construct an input value that results in a comma-separated permission string longer than 1024 characters when processed byformat_permissions(e.g. many enabled flags on aPermissionsinstance, or a larger list/dict of permission names). - If your implementation uses a specific ellipsis character (only
"…"or only"..."), you can simplify theendswithassertion to match exactly what your helper produces.
| return ", ".join(special_channels) if special_channels else "None" | ||
|
|
||
|
|
||
| def add_guild_title_section( |
There was a problem hiding this comment.
issue (complexity): Consider merging several tiny guild, role, and banner helpers into a few cohesive builders to make the control flow easier to follow and reduce indirection.
You can keep the centralization benefit while reducing indirection by merging some of the ultra‑granular helpers into a few cohesive units.
1. Collapse guild section helpers into a single composer
All the add_guild_*_section functions are thin wrappers around TextDisplay + Separator, used only in the guild view. They can be collapsed into a single helper that composes the full guild body, so build_guild_view only calls one function and readers don’t have to hop around.
For example:
def build_guild_sections(
container: discord.ui.Container[discord.ui.LayoutView],
guild: discord.Guild,
tier_text: str,
verification_text: str,
mfa_text: str,
nsfw_text: str,
content_filter_text: str,
notification_text: str,
channel_counts: str,
special_channels_text: str,
member_stats: str,
) -> None:
description = guild.description or "No description available."
title_content = f"# {guild.name}\n\n{description}"
# title + thumbnail
if guild.icon:
container.add_item(
discord.ui.Section(
discord.ui.TextDisplay(title_content),
accessory=discord.ui.Thumbnail(media=guild.icon.url),
),
)
else:
container.add_item(discord.ui.TextDisplay(title_content))
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# basic info
owner_text = guild.owner.mention if guild.owner else "Unknown"
container.add_item(
discord.ui.TextDisplay(
f"### Basic Information\n"
f"👑 **Owner:** {owner_text} • "
f"🔗 **Vanity URL:** {guild.vanity_url_code or 'None'} • "
f"💎 **Premium Tier:** {tier_text}\n"
f"⭐ **Boosts:** {guild.premium_subscription_count}"
),
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# security
container.add_item(
discord.ui.TextDisplay(
f"### Security & Settings\n"
f"🔒 **Verification:** {verification_text} • "
f"🛡️ **MFA Level:** {mfa_text} • "
f"🔞 **NSFW Level:** {nsfw_text}\n"
f"🚫 **Content Filter:** {content_filter_text} • "
f"🔔 **Notifications:** {notification_text}"
),
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# channels
container.add_item(
discord.ui.TextDisplay(
f"### Channels\n"
f"📝 **Channels:** {channel_counts}\n"
f"📍 **Special Channels:** {special_channels_text}",
),
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# resources
container.add_item(
discord.ui.TextDisplay(
f"### Resources\n"
f"😀 **Emojis:** {len(guild.emojis)}/{2 * guild.emoji_limit} • "
f"🎨 **Stickers:** {len(guild.stickers)}/{guild.sticker_limit} • "
f"🎭 **Roles:** {len(guild.roles)}",
),
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# members
container.add_item(discord.ui.TextDisplay(f"### Members\n{member_stats}"))
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
# footer
footer_text = (
f"🆔 **ID:** `{guild.id}` • "
f"📅 **Created:** {format_date_long(guild.created_at)}"
)
container.add_item(discord.ui.TextDisplay(footer_text))Then build_guild_view can call just build_guild_sections(...) (and optionally a separate add_guild_media if you want media clearly separated), instead of 6–7 small helpers.
This keeps all functionality, but shrinks the public helper surface and makes the view flow visible in one place.
2. Merge role helpers into a single metadata builder
get_role_type_info, get_role_tags_info, and get_role_flags_info are only used by the role view and each returns tiny bits of related data. Collapsing them into a single “role metadata” helper removes three jumps:
def get_role_metadata(role: discord.Role) -> list[str]:
info: list[str] = []
# type info
if hasattr(role, "is_default") and role.is_default():
info.append("Default Role")
if hasattr(role, "is_bot_managed") and role.is_bot_managed():
info.append("Bot Managed")
if hasattr(role, "is_integration") and role.is_integration():
info.append("Integration Managed")
if hasattr(role, "is_premium_subscriber") and role.is_premium_subscriber():
info.append("Premium Subscriber")
if hasattr(role, "is_assignable") and not role.is_assignable():
info.append("Not Assignable")
# tags
tags = getattr(role, "tags", None)
if tags:
if getattr(tags, "bot_id", None):
info.append(f"Bot: <@{tags.bot_id}>")
if getattr(tags, "integration_id", None):
info.append(f"Integration: {tags.integration_id}")
if getattr(tags, "subscription_listing_id", None):
info.append("Premium Subscriber Role")
# flags
flags = getattr(role, "flags", None)
if flags:
if getattr(flags, "inverted", False):
info.append("Inverted")
if getattr(flags, "mentionable_by_everyone", False):
info.append("Mentionable by Everyone")
return infobuild_role_view can then consume a single list and join it however it currently does, with no behavior change.
3. Simplify get_member_banner control flow
The current implementation uses suppress, nested checks, and exception handling intertwined. You can keep the fetch‑then‑cache semantics with clearer linear logic and early returns:
async def get_member_banner(member: discord.Member, bot: Tux) -> str | None:
async def extract_banner(user: discord.abc.User) -> str | None:
banner = getattr(user, "banner", None)
url = getattr(banner, "url", None)
return url if isinstance(url, str) and url else None
# try fetch first
try:
user = await bot.fetch_user(member.id)
if banner_url := extract_banner(user):
return banner_url
except discord.NotFound:
pass
except Exception:
# fall back to cache on fetch failure
pass
if cached_user := bot.get_user(member.id):
return extract_banner(cached_user)
return NoneThis keeps all existing behavior (fetch first, cache fallback, ignore failures) but reduces branching and removes the suppress context, making the function easier to reason about.
| return view, container | ||
|
|
||
|
|
||
| def _add_section( |
There was a problem hiding this comment.
issue (complexity): Consider simplifying these builders by inlining the small layout helpers and co-locating one-off helper functions so each build_*_view fully describes its own UI structure.
You can keep the builder separation while reducing ceremony by (a) co-locating one‑off helpers with their builders and (b) inlining the “micro” helpers that only wrap TextDisplay/Separator.
1. Inline _add_section / _add_footer where they’re just formatting sugar
These two are adding extra indirection for a very small gain. Inlining them makes each builder’s layout self‑describing.
For example, in build_user_view:
# current
_add_section(
container,
"Basic Information",
f"👤 **Username:** `{username_display}` • "
f"🆔 **ID:** `{user.id}` • "
f"🤖 **Bot:** {format_bool(user.bot)}",
)
_add_section(
container,
"Dates",
f"📅 **Registered:** {format_datetime(user.created_at)}",
add_sep=False,
)You can inline the layout and remove the helper:
container.add_item(
discord.ui.TextDisplay(
"### Basic Information\n"
f"👤 **Username:** `{username_display}` • "
f"🆔 **ID:** `{user.id}` • "
f"🤖 **Bot:** {format_bool(user.bot)}"
)
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
container.add_item(
discord.ui.TextDisplay(
"### Dates\n"
f"📅 **Registered:** {format_datetime(user.created_at)}"
)
)
# no separator hereAnd similarly in build_channel_view, build_emoji_view, etc. Once inlined everywhere, you can drop _add_section and _add_footer entirely and keep only _create_info_view, which actually removes duplication.
For the footer, e.g. in build_thread_view:
# current
_add_footer(container, f"`{thread.id}`", format_date_long(thread.created_at))Inline:
container.add_item(
discord.ui.TextDisplay(
f"🆔 **ID:** `{thread.id}` • "
f"📅 **Created:** {format_date_long(thread.created_at)}"
)
)This keeps functionality while making the structure of each view visible where it’s used.
2. Co-locate one‑off helpers with their builders
Helpers like get_role_type_info, get_role_tags_info, get_role_flags_info, and even some add_*_section helpers look role‑specific and are only used by build_role_view. Moving them into this module (or even as small inner functions) removes cross‑file jumping without reverting the builder pattern.
For example, instead of importing:
from .helpers import (
get_role_flags_info,
get_role_tags_info,
get_role_type_info,
)you can put minimal helpers next to build_role_view:
def _get_role_type_info(role: discord.Role) -> list[tuple[str, bool]]:
# same logic as helpers.get_role_type_info
...
def _get_role_tags_info(role: discord.Role) -> list[str] | None:
# same logic as helpers.get_role_tags_info
...
def _get_role_flags_info(role: discord.Role) -> list[str] | None:
# same logic as helpers.get_role_flags_info
...and then:
if role_type_info := _get_role_type_info(role):
type_parts = [f"**{name}:** {format_bool(value)}" for name, value in role_type_info]
container.add_item(
discord.ui.TextDisplay(
"### Role Type\n" + " • ".join(type_parts)
)
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))Apply the same idea to guild/channel/emoji helpers that are only used by a single build_*_view. Shared/complex pieces (like count_guild_members, format_* utilities) can stay in helpers.py, but layout‑specific text/sections become local, making each builder much easier to read in isolation.
3. Keep most layout declaration inline, use helpers only for data/formatting
You already have a nice separation where helpers compute/format values:
channel_counts = build_guild_channel_counts(guild)
member_stats = build_guild_member_stats(guild, humans, bots, ban_count)
special_channels_text = build_guild_special_channels(guild)You can keep these, but inline the actual section structure in build_guild_view instead of add_guild_*_section helpers.
For example, instead of:
add_guild_channels_section(container, channel_counts, special_channels_text)inline something like:
container.add_item(
discord.ui.TextDisplay(
"### Channels\n"
f"{channel_counts}\n\n"
f"{special_channels_text}"
)
)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))This preserves the builder layer and shared data helpers while making the “shape” of the view obvious in a single function, which is where maintainers will look first.
| from tux.modules.info.helpers import chunks | ||
|
|
||
|
|
||
| class InfoPaginatorView(discord.ui.LayoutView): |
There was a problem hiding this comment.
issue (complexity): Consider simplifying the paginator by storing direct references to UI elements, centralizing page-change logic, and avoiding ID-based lookups and tree walking.
You can simplify this without changing behavior by keeping direct references and centralizing page-change logic.
1. Avoid integer IDs + find_item for TextDisplays
Instead of CONTENT_ID, PAGE_INFO_ID, and find_item, keep attribute references when building the layout:
class InfoPaginatorView(discord.ui.LayoutView):
def __init__(...):
super().__init__(timeout=timeout)
...
self._content_display: discord.ui.TextDisplay | None = None
self._page_info_display: discord.ui.TextDisplay | None = None
self._first_btn = self._prev_btn = None
self._next_btn = self._last_btn = None
self._build_layout()def _build_layout(self) -> None:
self.clear_items()
container = discord.ui.Container[discord.ui.LayoutView](accent_color=0x5865F2)
...
self._content_display = discord.ui.TextDisplay(f"# {self.title}\n\n{content_text}")
container.add_item(self._content_display)
container.add_item(discord.ui.Separator(spacing=discord.SeparatorSpacing.small))
self._page_info_display = discord.ui.TextDisplay(self._get_page_info())
container.add_item(self._page_info_display)
self.add_item(container)
if len(self.chunks_list) > 1:
nav_row = self._build_navigation()
self.add_item(nav_row)Then _update_page can be simplified to direct attribute access:
async def _update_page(self, interaction: discord.Interaction) -> None:
if self._content_display is not None:
if not self.chunks_list:
content_text = "No items available."
else:
current_chunk = self.chunks_list[self.current_page]
content_text = (
f"### {self.list_type.capitalize()} list for {self.guild_name}\n\n"
f"{' '.join(current_chunk)}"
)
self._content_display.content = f"# {self.title}\n\n{content_text}"
if self._page_info_display is not None:
self._page_info_display.content = self._get_page_info()
self._update_navigation_buttons()
await interaction.response.edit_message(view=self)2. Avoid walk_children and custom_id checks for navigation buttons
Store button references when building the navigation row and update them directly:
def _build_navigation(self) -> discord.ui.ActionRow[discord.ui.LayoutView]:
nav_row = discord.ui.ActionRow[discord.ui.LayoutView]()
total_pages = len(self.chunks_list)
self._first_btn = discord.ui.Button[discord.ui.LayoutView](
label="⏮️ First",
style=discord.ButtonStyle.secondary,
disabled=self.current_page == 0,
)
self._first_btn.callback = self._handle_first
nav_row.add_item(self._first_btn)
self._prev_btn = discord.ui.Button[discord.ui.LayoutView](
label="⬅️ Previous",
style=discord.ButtonStyle.secondary,
disabled=self.current_page == 0,
)
self._prev_btn.callback = self._handle_prev
nav_row.add_item(self._prev_btn)
self._next_btn = discord.ui.Button[discord.ui.LayoutView](
label="➡️ Next",
style=discord.ButtonStyle.secondary,
disabled=self.current_page >= total_pages - 1,
)
self._next_btn.callback = self._handle_next
nav_row.add_item(self._next_btn)
self._last_btn = discord.ui.Button[discord.ui.LayoutView](
label="⏭️ Last",
style=discord.ButtonStyle.secondary,
disabled=self.current_page >= total_pages - 1,
)
self._last_btn.callback = self._handle_last
nav_row.add_item(self._last_btn)
close_btn = discord.ui.Button[discord.ui.LayoutView](
label="❌ Close",
style=discord.ButtonStyle.danger,
)
close_btn.callback = self._handle_close
nav_row.add_item(close_btn)
return nav_rowdef _update_navigation_buttons(self) -> None:
total_pages = len(self.chunks_list)
at_first = self.current_page == 0
at_last = self.current_page >= total_pages - 1
if self._first_btn:
self._first_btn.disabled = at_first
if self._prev_btn:
self._prev_btn.disabled = at_first
if self._next_btn:
self._next_btn.disabled = at_last
if self._last_btn:
self._last_btn.disabled = at_lastThis removes walk_children, custom_id strings, and makes the navigation row explicit.
3. Consolidate page-change logic to reduce handler boilerplate
All four nav handlers share the same pattern. A small helper keeps behavior identical but cuts branching duplication:
async def _change_page(self, interaction: discord.Interaction, target: int) -> None:
# Clamp to valid range
target = max(0, min(target, max(len(self.chunks_list) - 1, 0)))
if target == self.current_page:
await interaction.response.defer()
return
self.current_page = target
await self._update_page(interaction)Then the handlers become:
async def _handle_first(self, interaction: discord.Interaction) -> None:
await self._change_page(interaction, 0)
async def _handle_prev(self, interaction: discord.Interaction) -> None:
await self._change_page(interaction, self.current_page - 1)
async def _handle_next(self, interaction: discord.Interaction) -> None:
await self._change_page(interaction, self.current_page + 1)
async def _handle_last(self, interaction: discord.Interaction) -> None:
await self._change_page(interaction, len(self.chunks_list) - 1)4. Minor: _build_layout only needs to run once
Since _build_layout is only called from __init__ and the structure is fixed, you can safely drop self.clear_items() unless you anticipate reuse:
def _build_layout(self) -> None:
# self.clear_items() # not needed if layout is built once
...These changes should keep all existing behavior while making the paginator more direct and easier to follow.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Excellent Refactoring! 🎉
This PR represents a significant improvement to the info command system with several key benefits:
✅ Major Improvements
-
Modern UI Architecture: Successfully migrated from embeds to Discord's Components V2, providing a more interactive and visually appealing user experience.
-
Clean Command Structure: Transformed the complex single command with type detection into clear, explicit subcommands (
/info server,/info user, etc.), eliminating ambiguity and improving discoverability. -
Better Code Organization:
- Separated concerns with dedicated modules (
builders.py,helpers.py,utils.py) - Removed complex type handler system in favor of straightforward subcommand implementations
- Improved maintainability and testability
- Separated concerns with dedicated modules (
-
Database Migration Simplification: Removed complex database stamping logic, making Alembic the single source of truth for schema management.
-
Enhanced Hot Reload Support: Added proper detection for the new module structure.
-
Comprehensive Testing: Added unit tests for helper functions, improving code reliability.
🔧 Technical Excellence
- Error Handling: Improved error messages with helpful guidance for users
- Performance: Better handling of Discord's character limits with smart truncation
- Documentation: Updated docs to reflect the new subcommand structure
- Type Safety: Maintained strong typing throughout the refactor
📊 Impact
- Lines of Code: Reduced from ~1000 to ~300 lines in the main module while adding more functionality
- User Experience: More intuitive command structure with better visual presentation
- Maintainability: Significantly easier to extend and modify individual entity types
This refactor successfully modernizes the info command system while maintaining all existing functionality and adding new capabilities. The code is cleaner, more maintainable, and provides a better user experience. Great work! 🚀
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| async def _check_tables_exist(self) -> bool: | ||
| """Check if any application tables exist in the database. | ||
|
|
||
| Returns | ||
| ------- | ||
| bool | ||
| True if tables exist, False otherwise. | ||
| """ | ||
| try: | ||
| async with self.db_service.session() as session: | ||
| result = await session.execute( | ||
| text( | ||
| """ | ||
| SELECT COUNT(*) FROM information_schema.tables | ||
| WHERE table_schema = 'public' | ||
| AND table_type = 'BASE TABLE' | ||
| AND table_name != 'alembic_version' | ||
| """, | ||
| ), | ||
| ) | ||
| count = result.scalar() or 0 | ||
| return count > 0 | ||
| except Exception: | ||
| # If query fails, assume no tables exist | ||
| return False | ||
|
|
||
| async def _stamp_database_if_needed(self) -> None: | ||
| """Stamp database with head revision if tables exist but no Alembic version. | ||
|
|
||
| This handles the case where tables were created manually (e.g., via | ||
| SQLModel.metadata.create_all) but Alembic version tracking is missing. | ||
| """ | ||
| current_rev = await self._get_current_revision() | ||
| tables_exist = await self._check_tables_exist() | ||
|
|
||
| # If tables exist but no Alembic version, we need to stamp the database | ||
| if tables_exist and current_rev is None: | ||
| logger.warning( | ||
| "Database has tables but no Alembic version. " | ||
| "Stamping database with head revision...", | ||
| ) | ||
|
|
||
| cfg = self._build_alembic_config() | ||
| script_dir = ScriptDirectory.from_config(cfg) | ||
| head_revs = [rev.revision for rev in script_dir.get_revisions("head")] | ||
|
|
||
| if not head_revs: | ||
| logger.error("No head revisions found. Cannot stamp database.") | ||
| msg = "Cannot stamp database: no migration head found" | ||
| raise TuxDatabaseMigrationError(msg) | ||
|
|
||
| # Use the first head revision (or comma-separated if multiple) | ||
| head_rev = ",".join(head_revs) if len(head_revs) > 1 else head_revs[0] | ||
|
|
||
| loop = asyncio.get_event_loop() | ||
|
|
||
| def _stamp_sync(): | ||
| try: | ||
| command.stamp(cfg, head_rev) | ||
| logger.success(f"Database stamped with revision: {head_rev}") | ||
| except Exception as e: | ||
| error_msg = f"Failed to stamp database: {e}" | ||
| logger.exception(error_msg) | ||
| raise TuxDatabaseMigrationError(error_msg) from e | ||
|
|
||
| try: | ||
| await asyncio.wait_for( | ||
| loop.run_in_executor(None, _stamp_sync), | ||
| timeout=10.0, | ||
| ) | ||
| except TimeoutError as e: | ||
| error_msg = "Database stamping timed out after 10 seconds" | ||
| logger.exception(error_msg) | ||
| raise TuxDatabaseMigrationError(error_msg) from e | ||
| except TuxDatabaseMigrationError: | ||
| raise | ||
| except Exception as e: | ||
| error_msg = f"Unexpected error during database stamping: {e}" | ||
| logger.exception(error_msg) | ||
| raise TuxDatabaseMigrationError(error_msg) from e | ||
|
|
There was a problem hiding this comment.
Good refactoring! Removing the complex database stamping logic simplifies the migration process and makes Alembic the single source of truth for database schema management. The removal of _check_tables_exist and _stamp_database_if_needed methods eliminates potential edge cases and race conditions that could occur when trying to detect and handle manually created tables.
| # Try {parent_dir_name}.py in parent directory (e.g., info.py for info module) | ||
| with suppress(ImportError, AttributeError): | ||
| module_file = importlib.import_module( | ||
| f"{parent_module_name}.{parent_dir_name}", | ||
| ) | ||
| if hasattr(module_file, "setup") and callable(module_file.setup): | ||
| logger.trace( | ||
| f"Found {parent_dir_name}.py: {parent_module_name}.{parent_dir_name}", | ||
| ) | ||
| return f"{parent_module_name}.{parent_dir_name}" |
There was a problem hiding this comment.
Good addition! This enhancement to the hot reload system properly handles the new module structure where the main cog file has the same name as its parent directory (e.g., info.py in the info module). This ensures that changes to the main info command file will trigger proper hot reloading.
| # Truncate if too long (Discord limit is 4000 chars, reserve ~100 for heading/formatting) | ||
| max_roles_length = 3900 | ||
| if len(roles_display) > max_roles_length: | ||
| # Find the last complete role mention that fits | ||
| truncated = roles_display[:max_roles_length] | ||
| last_comma = truncated.rfind(", ") | ||
| if last_comma > 0: | ||
| truncated = truncated[:last_comma] | ||
| # Count how many roles are in the truncated string | ||
| roles_included = truncated.count(", ") + 1 | ||
| remaining = len(roles_list) - roles_included | ||
| roles_display = f"{truncated} (+{remaining} more roles)" | ||
| else: | ||
| roles_display = f"{truncated}..." |
There was a problem hiding this comment.
Excellent refactoring! The new Components V2 architecture is well-structured with clear separation of concerns. The builders module provides clean, reusable functions for creating Discord UI views, and the comprehensive helper functions make the code maintainable and testable. The role truncation logic is particularly well-implemented to handle Discord's character limits gracefully.
| self._type_handlers: dict[ | ||
| type, | ||
| Callable[[commands.Context[Tux], Any], Awaitable[None]], | ||
| ] = { | ||
| discord.Member: self._show_member_info, | ||
| discord.User: self._show_user_info, | ||
| discord.Message: self._show_message_info, | ||
| discord.abc.GuildChannel: self._show_channel_info, | ||
| discord.Guild: self._show_guild_info, | ||
| discord.Role: self._show_role_info, | ||
| discord.Emoji: self._show_emoji_info, | ||
| discord.GuildSticker: self._show_sticker_info, | ||
| discord.Invite: self._show_invite_info, | ||
| discord.Thread: self._show_thread_info, | ||
| discord.ScheduledEvent: self._show_event_info, | ||
| } |
There was a problem hiding this comment.
Excellent transformation! The refactored info command structure is much cleaner and more maintainable. The shift from a single monolithic command with complex type detection to explicit subcommands provides better user experience and clearer API design. The removal of the complex type handler system in favor of dedicated subcommands eliminates the ambiguity and edge cases that existed in the original implementation.
| def test_chunks_splits_into_size() -> None: | ||
| it = iter(["a", "b", "c", "d", "e"]) | ||
| result = list(chunks(it, 2)) | ||
| assert result == [["a", "b"], ["c", "d"], ["e"]] | ||
|
|
||
|
|
||
| def test_chunks_exact_multiple() -> None: | ||
| it = iter(["x", "y", "z"]) | ||
| result = list(chunks(it, 1)) | ||
| assert result == [["x"], ["y"], ["z"]] | ||
|
|
||
|
|
||
| def test_chunks_empty_iterator() -> None: | ||
| it = iter([]) | ||
| result = list(chunks(it, 5)) | ||
| assert result == [] | ||
|
|
||
|
|
||
| def test_chunks_single_large_chunk() -> None: | ||
| it = iter(["a", "b", "c"]) | ||
| result = list(chunks(it, 10)) | ||
| assert result == [["a", "b", "c"]] |
There was a problem hiding this comment.
Great addition of unit tests! The test coverage for the helper functions is comprehensive and well-structured. The tests cover edge cases like empty iterators, different date formats, and various invite URL patterns. This will help ensure the refactored code maintains reliability and makes future changes safer.
❌ 4 Tests Failed:
View the full list of 4 ❄️ flaky test(s)
To view more test analytics, go to the [Prevent Tests Dashboard](https://All Things Linux.sentry.io/prevent/tests/?preventPeriod=30d&integratedOrgName=allthingslinux&repository=tux&branch=refactor%2Finfo-command) |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
src/tux/modules/info/builders.py
Outdated
| discord.ui.LayoutView | ||
| The built view. | ||
| """ | ||
| banner_url = await get_member_banner(member, bot) # noqa: F841 # pyright: ignore[reportUnusedVariable] |
| with suppress(AttributeError, TypeError): | ||
| if url := banner.url: | ||
| return url | ||
| except discord.NotFound: |
![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 is an excellent and comprehensive refactoring of the info command. The move to explicit subcommands and Components V2 views is a significant improvement in terms of code clarity, maintainability, and user experience. The codebase is well-structured with new helper modules for builders, formatters, and utilities. The addition of unit tests for the new helpers is also a great practice. I've found a couple of minor areas for improvement related to error handling and logging, but overall, this is a very high-quality pull request.
| if guild_input.isdigit() and 15 <= len(guild_input) <= 20: | ||
| guild = self.bot.get_guild(int(guild_input)) | ||
| if guild is not None: | ||
| view = await build_guild_view(guild) | ||
| await send_view(ctx, view) | ||
| return | ||
|
|
There was a problem hiding this comment.
The current logic for handling a guild_input that is a guild ID for a server the bot is not in can be improved. It currently falls through to the invite check, which will fail and result in a generic error message. It would be better to handle this case explicitly to provide a more accurate error to the user, improving the command's feedback.
if guild_input.isdigit() and 15 <= len(guild_input) <= 20:
guild_id = int(guild_input)
if guild := self.bot.get_guild(guild_id):
view = await build_guild_view(guild)
await send_view(ctx, view)
else:
error_msg = (
f"❌ I'm not in a server with ID `{guild_id}`. "
"I can only show information for servers I'm a member of."
)
await send_error(ctx, error_msg)
return
src/tux/modules/info/helpers.py
Outdated
| except Exception: | ||
| # Fallback to cached user if fetch fails |
There was a problem hiding this comment.
The broad except Exception: at this line silently ignores any errors that occur while fetching a user's banner, other than discord.NotFound. This can make it difficult to debug issues if the fetch fails for other reasons (e.g., network problems, Discord API errors). It would be beneficial to log the exception to improve visibility into potential problems. You will need to import logger from loguru at the top of the file.
except Exception as e:
logger.warning(f"Failed to fetch user {member.id} for banner, falling back to cache: {e!r}")
# Fallback to cached user if fetch fails![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@src/tux/modules/info/info.py`:
- Around line 85-90: Replace the assert with an explicit runtime check: in the
branch where guild_input is None, read ctx.guild into guild, and if guild is
falsy, handle it gracefully (e.g., log the unexpected state and send an error
message or raise a controlled exception) instead of using assert; then continue
to call build_guild_view(guild) and send_view(ctx, view) as before. Ensure you
reference ctx.guild, guild_input, build_guild_view, and send_view when making
the change so the defensive check is applied in the correct code path.
In `@src/tux/modules/info/utils.py`:
- Around line 3-59: The send_error function currently echoes error_msg without
controlling mentions; update send_error to pass
allowed_mentions=discord.AllowedMentions.none() to every Discord send call (the
three paths: ctx.interaction.followup.send,
ctx.interaction.response.send_message, and ctx.send) so mention markup in
error_msg cannot trigger pings; locate and modify the send_error function to
construct a single allowed_mentions value (e.g., allowed_mentions =
discord.AllowedMentions.none()) and include it as an argument on each send
invocation.
In `@tests/modules/test_info.py`:
- Around line 40-43: The test test_format_datetime_naive_uses_utc is creating an
aware datetime (tzinfo=UTC) instead of a naive one, so update the test input to
use a naive datetime (remove tzinfo from the datetime constructor) so it
exercises the naive branch of format_datetime; keep the same assertion that
result != "Unknown" and adjust only the dt assignment in the test.
In `@TODO.md`:
- Around line 41-49: The new TODO entries in TODO.md (e.g., "Translate command",
"Invite tracking", "Define data expiration policy for guilds", "Help command
slash support", "Redesign atl rolecount plugin for global use", "Audit logging
system", "Use one function to parse all times `#420`", "Block emoji reactions
being used if emoji name contains blacklisted word", "Refactor moderation
commands for \"greedy conversion\" of target arg") must be converted to the
repository's checklist format; update each line to the checklist item pattern "-
[ ] <task description>" so they match existing TODO style and restore
consistency in documentation formatting.
🧹 Nitpick comments (9)
src/tux/services/hot_reload/file_utils.py (1)
87-96: Minor: Consider renamingmodule_filefor consistency.The variable
module_fileholds a module object, not a file. Other similar variables in this function use the*_modulesuffix (parent_module,cog_module). Considerdir_named_moduleor similar for clarity.♻️ Suggested rename
# Try {parent_dir_name}.py in parent directory (e.g., info.py for info module) with suppress(ImportError, AttributeError): - module_file = importlib.import_module( + dir_named_module = importlib.import_module( f"{parent_module_name}.{parent_dir_name}", ) - if hasattr(module_file, "setup") and callable(module_file.setup): + if hasattr(dir_named_module, "setup") and callable(dir_named_module.setup): logger.trace( f"Found {parent_dir_name}.py: {parent_module_name}.{parent_dir_name}", ) return f"{parent_module_name}.{parent_dir_name}"src/tux/core/setup/database_setup.py (1)
142-159: Consider a safer first recovery step before suggesting a reset.Recommending
uv run db resetis destructive. Add a non-destructive first step and a data-loss caution to reduce risk.Suggested wording tweak
if ( "already exists" in error_str.lower() or "duplicate" in error_str.lower() ): error_msg = ( f"Migration failed: {e}\n\n" "The database appears to be in an inconsistent state. " "This can happen if a previous migration attempt partially succeeded.\n" - "To fix this, reset the database with: uv run db reset\n" + "First, try rerunning migrations with: uv run db push\n" + "If this persists and data loss is acceptable, reset with: uv run db reset\n" "Or manually clean up the conflicting objects and try again." )src/tux/modules/info/views.py (1)
20-53: Validatechunk_sizeand add instance attribute annotations.Guard against zero/negative chunk sizes and annotate instance fields to match the project’s strict typing rules.
As per coding guidelines, ensure boundary validation and complete type hints.♻️ Proposed change
class InfoPaginatorView(discord.ui.LayoutView): """Components V2 pagination view for info command lists. @@ # Component IDs for dynamic updates CONTENT_ID = 1000 PAGE_INFO_ID = 1001 + + items: list[str] + chunks_list: list[list[str]] + current_page: int + title: str + list_type: str + guild_name: str def __init__( self, items: Iterable[str], @@ timeout: float = 300.0, ) -> None: @@ - super().__init__(timeout=timeout) + if chunk_size < 1: + raise ValueError("chunk_size must be >= 1") + super().__init__(timeout=timeout)docs/content/user/modules/info/info.md (1)
80-84: Parameter type documentation could be clarified.The
entityparameter is documented as typeUSER/MEMBER, but in the implementation (info.pyline 147), it's actually astrthat gets converted viaMemberConverter/UserConverter. Consider documenting it asSTRINGwith a description mentioning it accepts mentions or IDs, for consistency with howguild_idandinvite_codeare documented.src/tux/modules/info/info.py (1)
102-131: Overly broad condition may cause unnecessary invite lookups.The condition
len(guild_input) >= 6(line 107) will trigger invite extraction for any string of 6+ characters, even if it doesn't look like an invite code. This could cause unnecessary Discord API calls and confusing error messages when users provide invalid input like "randomtext".Consider making the fallback to invite lookup more restrictive or moving it to an explicit else branch after the guild ID check fails.
Proposed fix - only try invite extraction for URL-like inputs
# If not a guild ID, try to extract from invite URL invite_input_lower = guild_input.lower() if ( "discord.gg/" in invite_input_lower or "discord.com/invite/" in invite_input_lower - or len(guild_input) >= 6 # Minimum invite code length ): with suppress(commands.BadArgument): extracted_code = extract_invite_code(guild_input) invite_converter = commands.InviteConverter() invite = await invite_converter.convert(ctx, extracted_code) # ... rest of logicAlternatively, if you want to support bare invite codes (without the URL prefix), consider validating that the input looks like a valid invite code format (alphanumeric only) before attempting the lookup.
src/tux/modules/info/builders.py (1)
144-144: Unused variablebanner_urladds unnecessary API call.The
banner_urlis fetched viaawait get_member_banner(member, bot)(which makes an API call tobot.fetch_user) but is never used due to the commented-out MediaGallery code. This wastes an API call per invocation.Consider removing the call until the MediaGallery implementation is ready, or implementing the TODO.
Proposed fix - remove unused API call
- banner_url = await get_member_banner(member, bot) # noqa: F841 # pyright: ignore[reportUnusedVariable] - + # TODO: Fetch banner when MediaGallery implementation is ready + # banner_url = await get_member_banner(member, bot)src/tux/modules/info/helpers.py (3)
274-294: Functioncount_guild_membersdoesn't need to beasync.This function contains no
awaitcalls - it only iterates overguild.memberssynchronously. Making it async adds unnecessary overhead and can be misleading.Proposed fix
-async def count_guild_members(guild: discord.Guild) -> tuple[int, int]: +def count_guild_members(guild: discord.Guild) -> tuple[int, int]: """Count humans and bots in guild. ParametersNote: This will also require updating the call site in
builders.pyline 92 to remove theawait.
1003-1023: Broadexcept Exceptionsilently swallows errors.The generic
except Exception(line 1014) catches all errors including programming bugs, network issues, rate limits, etc. This makes debugging difficult. Consider catching specific exceptions or at minimum logging the error.Also, the fallback at line 1016 uses
bot.get_user()which returns cached data. If the user isn't in cache (likely since we're fetching), this will returnNoneanyway, making the fallback less useful.Proposed fix - log errors and simplify
+from loguru import logger + async def get_member_banner(member: discord.Member, bot: Tux) -> str | None: ... try: user = await bot.fetch_user(member.id) banner = getattr(user, "banner", None) if banner is not None and hasattr(banner, "url"): with suppress(AttributeError, TypeError): if url := banner.url: return url - except discord.NotFound: - pass - except Exception: - # Fallback to cached user if fetch fails - if user := bot.get_user(member.id): - banner = getattr(user, "banner", None) - if banner is not None and hasattr(banner, "url"): - with suppress(AttributeError, TypeError): - if url := banner.url: - return url + except discord.NotFound: + logger.debug(f"User {member.id} not found when fetching banner") + except discord.HTTPException as e: + logger.warning(f"Failed to fetch user {member.id} for banner: {e}") return NoneAs per coding guidelines, errors should be logged with context information.
1029-1053: Consider makingchunksgeneric for reusability.The function is typed for
Iterator[str]but the logic works for any type. Using aTypeVarwould make it reusable.Proposed generic version
+from typing import TypeVar + +T = TypeVar("T") + -def chunks(it: Iterator[str], size: int) -> Generator[list[str]]: +def chunks(it: Iterator[T], size: int) -> Generator[list[T], None, None]: """Split an iterator into chunks of a specified size. ... """ - chunk: list[str] = [] + chunk: list[T] = [] for item in it: chunk.append(item) if len(chunk) == size: yield chunk chunk = [] if chunk: yield chunk
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
TODO.mddocs/content/user/modules/info/index.mddocs/content/user/modules/info/info.mdsrc/tux/core/setup/database_setup.pysrc/tux/modules/info/__init__.pysrc/tux/modules/info/builders.pysrc/tux/modules/info/helpers.pysrc/tux/modules/info/info.pysrc/tux/modules/info/utils.pysrc/tux/modules/info/views.pysrc/tux/services/hot_reload/file_utils.pytests/modules/test_info.py
🧰 Additional context used
📓 Path-based instructions (11)
**/*.md
📄 CodeRabbit inference engine (.cursor/rules/rules.mdc)
**/*.md: Follow documentation rules and master guide as defined indocs/docs.mdc
Follow documentation patterns and practical examples as defined indocs/patterns.mdc
Follow documentation organization structure as defined indocs/structure.mdc
Follow documentation writing standards as defined indocs/style.mdc
Files:
docs/content/user/modules/info/index.mdTODO.mddocs/content/user/modules/info/info.md
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/rules.mdc)
**/*.py: Follow security patterns as defined insecurity/patterns.mdc
Follow input validation patterns as defined insecurity/validation.mdc
Follow error handling patterns as defined inerror-handling/patterns.mdc
Follow logging patterns using loguru as defined inerror-handling/logging.mdc
Follow Sentry integration patterns as defined inerror-handling/sentry.mdc
**/*.py: Use strict type hints (Type | NonenotOptional[Type])
Use NumPy docstrings for documenting functions and classes
Use absolute imports preferred, relative imports allowed within the same module
Group imports in order: stdlib → third-party → local
Enforce 88 character line length
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Add imports to the top of the file unless absolutely necessary
Log errors with context information
Keep files to maximum 1600 lines - split larger files into logical modules
Use one class or function per file when possible
Provide complete type hints in all functions and variables
Files:
src/tux/services/hot_reload/file_utils.pysrc/tux/modules/info/views.pysrc/tux/modules/info/utils.pysrc/tux/core/setup/database_setup.pysrc/tux/modules/info/builders.pytests/modules/test_info.pysrc/tux/modules/info/__init__.pysrc/tux/modules/info/info.pysrc/tux/modules/info/helpers.py
src/tux/{services,database}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use async/await for all I/O operations (database and HTTP)
Files:
src/tux/services/hot_reload/file_utils.py
src/tux/{services,modules}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use custom exceptions for business logic errors
Files:
src/tux/services/hot_reload/file_utils.pysrc/tux/modules/info/views.pysrc/tux/modules/info/utils.pysrc/tux/modules/info/builders.pysrc/tux/modules/info/__init__.pysrc/tux/modules/info/info.pysrc/tux/modules/info/helpers.py
src/tux/services/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use aiocache for caching frequently accessed data
Files:
src/tux/services/hot_reload/file_utils.py
src/tux/{services,modules,database/controllers}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Write docstrings for all public APIs
Files:
src/tux/services/hot_reload/file_utils.pysrc/tux/modules/info/views.pysrc/tux/modules/info/utils.pysrc/tux/modules/info/builders.pysrc/tux/modules/info/__init__.pysrc/tux/modules/info/info.pysrc/tux/modules/info/helpers.py
src/tux/modules/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
src/tux/modules/**/*.py: Use hybrid commands (slash + traditional) in Discord cogs
Implement role-based permission checks in commands
Implement cooldowns and rate limiting in commands
Files:
src/tux/modules/info/views.pysrc/tux/modules/info/utils.pysrc/tux/modules/info/builders.pysrc/tux/modules/info/__init__.pysrc/tux/modules/info/info.pysrc/tux/modules/info/helpers.py
src/tux/{modules,services/handlers}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Validate all user inputs at boundaries
Files:
src/tux/modules/info/views.pysrc/tux/modules/info/utils.pysrc/tux/modules/info/builders.pysrc/tux/modules/info/__init__.pysrc/tux/modules/info/info.pysrc/tux/modules/info/helpers.py
src/tux/{modules,plugins}/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Load modules and plugins on demand for lazy loading
Files:
src/tux/modules/info/views.pysrc/tux/modules/info/utils.pysrc/tux/modules/info/builders.pysrc/tux/modules/info/__init__.pysrc/tux/modules/info/info.pysrc/tux/modules/info/helpers.py
**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/rules.mdc)
**/tests/**/*.py: Follow pytest configuration and patterns as defined intesting/pytest.mdc
Follow test marker conventions as defined intesting/markers.mdc
Maintain coverage requirements as defined intesting/coverage.mdc
Follow async testing patterns as defined intesting/async.mdc
Files:
tests/modules/test_info.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
pytestwith markersunit,integration,slow,database,asyncfor test organization
Files:
tests/modules/test_info.py
🧠 Learnings (5)
📚 Learning: 2026-01-23T10:16:45.474Z
Learnt from: CR
Repo: allthingslinux/tux PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-23T10:16:45.474Z
Learning: Applies to src/tux/{modules,plugins}/**/*.py : Load modules and plugins on demand for lazy loading
Applied to files:
src/tux/services/hot_reload/file_utils.py
📚 Learning: 2026-01-23T10:16:24.342Z
Learnt from: CR
Repo: allthingslinux/tux PR: 0
File: .cursor/rules/rules.mdc:0-0
Timestamp: 2026-01-23T10:16:24.342Z
Learning: Applies to **/commands/**/*.py : Follow user-facing error message patterns as defined in `error-handling/user-feedback.mdc`
Applied to files:
TODO.md
📚 Learning: 2026-01-23T10:16:45.474Z
Learnt from: CR
Repo: allthingslinux/tux PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-23T10:16:45.474Z
Learning: Applies to src/tux/ui/embeds.py : Use rich embeds for Discord message formatting
Applied to files:
docs/content/user/modules/info/info.md
📚 Learning: 2026-01-23T10:16:45.474Z
Learnt from: CR
Repo: allthingslinux/tux PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-23T10:16:45.474Z
Learning: Applies to src/tux/database/migrations/**/*.py : Use Alembic for database schema migrations
Applied to files:
src/tux/core/setup/database_setup.py
📚 Learning: 2026-01-23T10:16:45.474Z
Learnt from: CR
Repo: allthingslinux/tux PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-23T10:16:45.474Z
Learning: Applies to src/tux/database/{controllers,service.py} : Use async database operations and transactions for multi-step operations
Applied to files:
src/tux/core/setup/database_setup.py
🧬 Code graph analysis (6)
src/tux/services/hot_reload/file_utils.py (4)
src/tux/core/cog_loader.py (1)
setup(605-662)src/tux/modules/utility/remindme.py (1)
setup(259-267)src/tux/services/hot_reload/cog.py (1)
setup(9-18)src/tux/core/setup/orchestrator.py (1)
setup(46-82)
src/tux/modules/info/views.py (3)
src/tux/modules/info/info.py (1)
info(49-61)src/tux/modules/info/helpers.py (1)
chunks(1029-1053)src/tux/ui/views/tldr.py (1)
next(105-119)
src/tux/modules/info/utils.py (1)
src/tux/modules/utility/encode_decode.py (1)
send_message(74-100)
tests/modules/test_info.py (1)
src/tux/modules/info/helpers.py (5)
chunks(1029-1053)extract_invite_code(250-268)format_bool(16-29)format_date_long(58-76)format_datetime(32-55)
src/tux/modules/info/__init__.py (3)
src/tux/modules/info/info.py (2)
info(49-61)Info(31-288)src/tux/modules/info/utils.py (2)
send_error(34-59)send_view(9-31)src/tux/modules/info/views.py (1)
InfoPaginatorView(10-269)
src/tux/modules/info/info.py (3)
src/tux/modules/info/builders.py (8)
build_channel_view(314-378)build_emoji_view(476-543)build_guild_view(78-126)build_invite_view(546-629)build_member_view(129-245)build_role_view(381-473)build_thread_view(632-674)build_user_view(248-311)src/tux/modules/info/helpers.py (1)
extract_invite_code(250-268)src/tux/modules/info/utils.py (2)
send_error(34-59)send_view(9-31)
🪛 LanguageTool
docs/content/user/modules/info/info.md
[style] ~193-~193: The noun “invitation” is usually used instead of ‘invite’ in formal writing.
Context: ...info invite Get information about an invite. Aliases: inv Parameters: ...
(AN_INVITE)
⏰ 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). (3)
- GitHub Check: Seer Code Review
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Amazon Q Developer
🔇 Additional comments (13)
src/tux/services/hot_reload/file_utils.py (2)
32-32: LGTM!The
noqa: PLR0911suppression is appropriate here. The cascading lookup pattern with early returns is clearer than alternative control flow structures for this use case.
71-71: LGTM!Safe extraction guarded by the
len(parts) > 1check on line 69.src/tux/core/setup/database_setup.py (1)
212-214: Nice clarification on Alembic being the source of truth.The note cleanly communicates the intended ownership of schema management.
src/tux/modules/info/__init__.py (1)
3-12: Clean public API export.Nicely exposes the new info components and utilities in one place.
docs/content/user/modules/info/index.md (1)
23-32: Docs align well with the new subcommand workflow.Clearer subcommand listing and examples make the new flow easy to follow.
Also applies to: 50-58
docs/content/user/modules/info/info.md (1)
1-252: Documentation is comprehensive and well-structured.The documentation accurately reflects the new subcommand-based architecture and provides clear examples for each subcommand. The information displayed sections are helpful for users understanding what data they'll receive.
src/tux/modules/info/info.py (2)
139-178: Good fallback pattern for user/member lookup.The approach of trying
MemberConverterfirst, then falling back toUserConverter, handles both guild members and external users gracefully. The use ofsuppress(commands.BadArgument)keeps the code clean.
44-61: Well-structured hybrid command group.The refactored command group with explicit subcommands is cleaner than the previous dispatcher pattern. The
guild_only()decorator and help text fallback are appropriate.src/tux/modules/info/builders.py (3)
49-56: LGTM -timeout=Noneis appropriate for static info views.Since these views only contain read-only components (TextDisplay, Separator, MediaGallery) without interactive elements requiring callbacks, having no timeout is acceptable and avoids unnecessary cleanup.
314-378: Well-structured channel view builder with good type handling.The builder correctly handles different channel types with
isinstancechecks and delegates to specialized helper functions. The pattern of checking fornsfwattribute presence before access is defensive and appropriate.
1-674: Overall well-designed modular architecture.The builder pattern cleanly separates view construction from command logic. Consistent use of helper functions (
_add_section,_add_footer) reduces duplication. Docstrings follow NumPy format as required. Type hints are complete.src/tux/modules/info/helpers.py (2)
16-98: Well-implemented formatting utilities with proper error handling.The formatting functions (
format_bool,format_datetime,format_date_long,format_permissions) have appropriate fallbacks for edge cases and proper type hints. The UTC-aware datetime handling informat_datetimeis particularly well done.
250-268:extract_invite_codecorrectly handles multiple URL formats.The function properly handles both
discord.gg/anddiscord.com/invite/formats, strips query parameters, and falls back to returning the input as-is. Case-insensitive matching on the URL while preserving the original case in the extracted code is a nice touch.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 12 files
Confidence score: 3/5
- Some user-facing correctness risks exist, so merge confidence is moderate rather than high.
src/tux/modules/info/helpers.pycomputes auto-archive hours by dividing minutes by 3600, which will report 0h for many valid durations and can mislead users.src/tux/modules/info/views.pyuses inconsistent deletion logic (interaction.messagevsdelete_original_response()), which may leave messages undeleted or delete the wrong response.- Pay close attention to
src/tux/modules/info/helpers.py,src/tux/modules/info/views.py,src/tux/modules/info/builders.py,tests/modules/test_info.py,docs/content/user/modules/info/info.md- time conversion accuracy, message deletion targeting, unused API call/topic attribute misuse, test coverage of naive datetimes, and docs syntax mismatch.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="docs/content/user/modules/info/info.md">
<violation number="1" location="docs/content/user/modules/info/info.md:22">
P3: The general syntax shows `<entity>` as required, but `info server` explicitly allows omitting the entity. This makes the syntax line inaccurate for optional-entity subcommands. Use optional notation to avoid misleading users.</violation>
</file>
<file name="tests/modules/test_info.py">
<violation number="1" location="tests/modules/test_info.py:34">
P2: This test is meant to cover the naive-datetime path, but it constructs a timezone-aware datetime. Use a naive `datetime` (no tzinfo) so the test actually verifies UTC fallback behavior.</violation>
</file>
<file name="src/tux/modules/info/helpers.py">
<violation number="1" location="src/tux/modules/info/helpers.py:739">
P2: Incorrect time unit conversion: `default_auto_archive_duration` is in minutes, not seconds. Dividing by 3600 (`// 60 // 60`) will always return 0h for durations under 2.5 days. Should divide by 60 only to convert minutes to hours.</violation>
</file>
<file name="src/tux/modules/info/builders.py">
<violation number="1" location="src/tux/modules/info/builders.py:144">
P2: Wasteful API call: `get_member_banner` fetches data that is never used. Either remove the call until the MediaGallery code is implemented, or remove the suppression comments and use the result.</violation>
<violation number="2" location="src/tux/modules/info/builders.py:647">
P2: `discord.Thread` does not have a `topic` attribute (that's a `TextChannel` property). This will always show "No topic available." Consider removing this misleading line or using a meaningful thread attribute.</violation>
</file>
<file name="src/tux/modules/info/views.py">
<violation number="1" location="src/tux/modules/info/views.py:210">
P2: Inconsistent message deletion logic: checks `interaction.message` but calls `delete_original_response()`. These refer to different things - use `interaction.message.delete()` to delete the actual message containing the button.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| """Handle close button click.""" | ||
| await interaction.response.defer() | ||
| if interaction.message: | ||
| await interaction.delete_original_response() |
There was a problem hiding this comment.
P2: Inconsistent message deletion logic: checks interaction.message but calls delete_original_response(). These refer to different things - use interaction.message.delete() to delete the actual message containing the button.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/tux/modules/info/views.py, line 210:
<comment>Inconsistent message deletion logic: checks `interaction.message` but calls `delete_original_response()`. These refer to different things - use `interaction.message.delete()` to delete the actual message containing the button.</comment>
<file context>
@@ -0,0 +1,269 @@
+ """Handle close button click."""
+ await interaction.response.defer()
+ if interaction.message:
+ await interaction.delete_original_response()
+
+ async def _update_page(self, interaction: discord.Interaction) -> None:
</file context>
|
|
||
| ```text | ||
| /info entity:STRING | ||
| /info <subcommand> <entity> |
There was a problem hiding this comment.
P3: The general syntax shows <entity> as required, but info server explicitly allows omitting the entity. This makes the syntax line inaccurate for optional-entity subcommands. Use optional notation to avoid misleading users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At docs/content/user/modules/info/info.md, line 22:
<comment>The general syntax shows `<entity>` as required, but `info server` explicitly allows omitting the entity. This makes the syntax line inaccurate for optional-entity subcommands. Use optional notation to avoid misleading users.</comment>
<file context>
@@ -10,23 +10,23 @@ tags:
```text
-/info entity:STRING
+/info <subcommand> <entity>
</file context>
</details>
```suggestion
/info <subcommand> [entity]
Summary by Sourcery
Refactor the info command module to use Components V2 views and dedicated builders/helpers, introduce paginated views for large info lists, improve database migration error messaging, and update documentation and hot-reload utilities to match the new command structure.
Enhancements:
Documentation:
Tests:
Chores: