Skip to content

Conversation

@meatsnails
Copy link
Collaborator

@meatsnails meatsnails commented Dec 30, 2025

Pull Request

Description

Made changes to Better handle attachments to prevent errors from exceeding the discord character limit aswell as preventing discords built in stickers from being linked to the json

If your PR is related to an issue, please include the issue number below:

Related Issue: Closes #1051 #835

Type of Change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Performance improvement
  • Code refactoring
  • Test improvements

Guidelines

  • My code follows the style guidelines of this project (formatted with Ruff)

  • I have performed a self-review of my own code

  • I have commented my code, particularly in hard-to-understand areas

  • I have made corresponding changes to the documentation if needed

  • My changes generate no new warnings

  • I have tested this change

  • Any dependent changes have been merged and published in downstream modules

  • I have added all appropriate labels to this PR

  • I have followed all of these guidelines.

How Has This Been Tested? (if applicable)

Tested by bookmarking various messages with my development some containing stickers guild stickers images text etc i also compared with how prod handles it at the moment

Screenshots (if applicable)

image image image image

Additional Information

The way the embed was changed slightly to make this change cleaner but @kzndotsh said it's fine

Also from the action suggestions i made it cut off if the message got to long and added a message explaining such aswell as fixing some other minor things in the file

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Dec 30, 2025

Reviewer's Guide

Refactors how bookmarked message attachments and stickers are rendered by moving them into the main embed description, introducing safer sticker URL handling, and adjusting attachment limits to avoid exceeding Discord’s character limits.

Sequence diagram for updated bookmark embed creation

sequenceDiagram
    actor User
    participant DiscordClient
    participant Bot
    participant Bookmarks
    participant EmbedCreator

    User->>DiscordClient: Clicks Add Bookmark reaction
    DiscordClient->>Bot: Bookmark reaction event
    Bot->>Bookmarks: _create_bookmark_embed(message)

    activate Bookmarks
    Bookmarks->>Bookmarks: Truncate content to EMBED_MAX_DESC_LENGTH
    Bookmarks->>Bookmarks: Build attachment_list from message.attachments
    Bookmarks->>Bookmarks: Build sticker_list from message.stickers
    Bookmarks->>Bookmarks: Combine content, attachment_list, sticker_list
    Bookmarks->>EmbedCreator: create_embed(bot, INFO, title, description)
    EmbedCreator-->>Bookmarks: discord.Embed
    deactivate Bookmarks

    Bot->>DiscordClient: Send bookmark embed
    DiscordClient-->>User: Display embed with attachments and stickers
Loading

Updated class diagram for Bookmarks embed handling

classDiagram
    class Bookmarks {
        +_get_files_from_attachments(message, files) async
        +_create_bookmark_embed(message) discord.Embed
    }

    class discord_Message {
        +content
        +attachments
        +stickers
        +embeds
        +jump_url
    }

    class EmbedCreator {
        +create_embed(bot, embed_type, title, description) discord.Embed
    }

    Bookmarks --> discord_Message : uses
    Bookmarks --> EmbedCreator : uses

    class Attachment {
        +filename
        +url
        +content_type
    }

    class StickerItem {
        +id
        +name
        +format
    }

    discord_Message "*" --> "*" Attachment : has
    discord_Message "*" --> "*" StickerItem : has
Loading

File-Level Changes

Change Details Files
Refactor bookmark embed description to inline attachments and stickers while constraining content length and sticker URLs.
  • Truncate message content to EMBED_MAX_DESC_LENGTH and fall back to a default description when empty.
  • Build a bullet list of attachments using markdown links and append it under an Attachments: section in the embed description instead of a separate field.
  • Build a bullet list of stickers, generating CDN URLs only for PNG/APNG stickers with non-zero IDs and falling back to plain text names otherwise, appended under a Stickers: section.
  • Replace the previous attachment and sticker embed fields with the new combined description-based rendering to reduce risk of exceeding Discord field character limits.
src/tux/modules/features/bookmarks.py
Adjust attachment handling limits in bookmark file extraction logic.
  • Introduce FIELD_CHAR_LIMIT constant for potential character limit constraints (not yet used in the provided diff).
  • Change the attachment file loop termination condition from len(files) >= 10 to len(files) > 10 to alter how many attachments can be collected before stopping.
src/tux/modules/features/bookmarks.py

Assessment against linked issues

Issue Objective Addressed Explanation
#1051 Allow bookmarking of messages with many (>= 9) image attachments without causing Discord DM failures (likely due to embed/field character limits or invalid sticker/attachment handling).

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

@github-actions
Copy link
Contributor

github-actions bot commented Dec 30, 2025

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

@coderabbitai
Copy link

coderabbitai bot commented Dec 30, 2025

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

Changed bookmark embed construction: consolidated content, attachments, and stickers into a single description with max-length enforcement and cutoff notice; adjusted attachment/embed file-gathering threshold from >= 10 to > 10; improved sticker link/name handling; added cog_unload to close the aiohttp session; added imports for Sequence, StickerFormatType, and StickerItem.

Changes

Cohort / File(s) Summary
Bookmarks module
src/tux/modules/features/bookmarks.py
Consolidated embed description to include message content, attachments, and stickers (removed separate Attachments/Stickers fields); enforce EMBED_MAX_DESC_LENGTH and append cutoff notice when truncated; construct attachment_list and sticker_list in-description with PNG/APNG sticker links when data present; changed file-collection gating in _get_files_from_attachments and _get_files_from_embeds from len(files) >= 10 to len(files) > 10 (note: _get_files_from_stickers keeps its existing threshold logic); added imports Sequence, StickerFormatType, StickerItem; added def cog_unload(self) -> None to close aiohttp session on unload.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: improving attachment handling in bookmarks to prevent errors.
Description check ✅ Passed The description is directly related to the changeset, explaining the modifications to bookmark handling and linking related issues.
Linked Issues check ✅ Passed The PR addresses the core issue #1051 by increasing file accumulation thresholds and handling attachments/stickers safely to prevent Discord limits from causing bookmark failures.
Out of Scope Changes check ✅ Passed All changes are focused on improving bookmark attachment and sticker handling per the linked issues; the addition of cog_unload is a minor complementary change for resource cleanup.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/bookmarks

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • The change from len(files) >= 10 to len(files) > 10 in _get_files_from_attachments will now allow up to 11 files instead of 10; if the 10-attachment maximum is intentional, this condition should remain >= 10.
  • FIELD_CHAR_LIMIT is introduced but never used; either wire it into the embed description/field truncation logic or remove it to avoid dead code.
  • The new description concatenates content, attachments, and stickers after truncating only the original content by EMBED_MAX_DESC_LENGTH, so bookmark embeds can now exceed Discord’s description character limit; consider applying a final length check/truncation to the combined description string.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The change from `len(files) >= 10` to `len(files) > 10` in `_get_files_from_attachments` will now allow up to 11 files instead of 10; if the 10-attachment maximum is intentional, this condition should remain `>= 10`.
- `FIELD_CHAR_LIMIT` is introduced but never used; either wire it into the embed description/field truncation logic or remove it to avoid dead code.
- The new description concatenates content, attachments, and stickers after truncating only the original content by `EMBED_MAX_DESC_LENGTH`, so bookmark embeds can now exceed Discord’s description character limit; consider applying a final length check/truncation to the combined `description` string.

## Individual Comments

### Comment 1
<location> `src/tux/modules/features/bookmarks.py:184` </location>
<code_context>
         """
         for attachment in message.attachments:
-            if len(files) >= 10:
+            if len(files) > 10:
                 break

</code_context>

<issue_to_address>
**issue (bug_risk):** The changed comparison allows collecting 11 files instead of 10, which is likely off-by-one relative to the intended cap.

The old `>= 10` check ensured you never collected more than 10 files; with `> 10`, the loop now allows 11 before breaking. If the limit is meant to stay at 10, keep `>= 10` (or guard the body with `< 10`). If you do want a higher cap, consider introducing a named constant for the limit instead of using a magic number.
</issue_to_address>

### Comment 2
<location> `src/tux/modules/features/bookmarks.py:335-341` </location>
<code_context>
+            ]
+            sticker_list = "\n".join(bullets)
+
+        # Combine everything into the embed description
+        description = content if content else "> No content available to display"
+
+        if attachment_list:
+            description += f"\n\n**Attachments:**\n{attachment_list}"
+        if sticker_list:
+            description += f"\n\n**Stickers:**\n{sticker_list}"
+
</code_context>

<issue_to_address>
**suggestion:** New description composition may exceed Discord’s 4096-character description limit once attachments and stickers are appended.

Only `content` is enforced via `EMBED_MAX_DESC_LENGTH`; the appended attachment/sticker sections are not. With many long attachment or sticker names, the final description string can exceed Discord’s 4096-character limit and trigger API errors. Please either cap/truncate the attachment/sticker sections based on remaining space or compute the full description first and trim it to the global limit (e.g., using `FIELD_CHAR_LIMIT` for consistency).

```suggestion
        # Combine everything into the embed description and enforce the max length
        parts: list[str] = []

        if content:
            parts.append(content)
        else:
            parts.append("> No content available to display")

        if attachment_list:
            parts.append(f"**Attachments:**\n{attachment_list}")
        if sticker_list:
            parts.append(f"**Stickers:**\n{sticker_list}")

        description = "\n\n".join(parts)

        # Ensure we never exceed Discord's embed description limit
        if len(description) > EMBED_MAX_DESC_LENGTH:
            description = description[:EMBED_MAX_DESC_LENGTH]
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

"""
for attachment in message.attachments:
if len(files) >= 10:
if len(files) > 10:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): The changed comparison allows collecting 11 files instead of 10, which is likely off-by-one relative to the intended cap.

The old >= 10 check ensured you never collected more than 10 files; with > 10, the loop now allows 11 before breaking. If the limit is meant to stay at 10, keep >= 10 (or guard the body with < 10). If you do want a higher cap, consider introducing a named constant for the limit instead of using a magic number.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 30, 2025

📚 Documentation Preview

Type URL Version Message
Production https://tux.atl.dev - -
Preview https://1db0356d-tux-docs.allthingslinux.workers.dev 1db0356d-e41c-4ea7-8b02-52338a51b056 Preview: tux@107455660805ba8b10ddb352ca0a8c2b6977c4b6 on 1131/merge by meatsnails (run 250)

@sentry
Copy link

sentry bot commented Dec 30, 2025

Codecov Report

❌ Patch coverage is 33.33333% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 40.21%. Comparing base (3b70404) to head (debbcfd).
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
src/tux/modules/features/bookmarks.py 33.33% 18 Missing ⚠️

❌ Your project check has failed because the head coverage (40.21%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1131      +/-   ##
==========================================
- Coverage   40.24%   40.21%   -0.03%     
==========================================
  Files         204      204              
  Lines       14373    14391      +18     
  Branches     1686     1690       +4     
==========================================
+ Hits         5784     5788       +4     
- Misses       8589     8603      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 338 to -341
)

# Add attachments to the embed
if message.attachments:

This comment was marked as outdated.

"""
for attachment in message.attachments:
if len(files) >= 10:
if len(files) > 10:

This comment was marked as outdated.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (1)
src/tux/modules/features/bookmarks.py (1)

25-26: Remove or apply the unused constant.

FIELD_CHAR_LIMIT is defined but never referenced in the code. Since the PR changed the embed structure to use a single description field instead of separate fields, this constant may no longer be needed. Consider removing it, or if it was intended to limit the attachment/sticker list lengths, apply it accordingly.

🔎 Proposed fix to remove the unused constant
-FIELD_CHAR_LIMIT = 1024
-
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8a7f168 and 6164f38.

📒 Files selected for processing (1)
  • src/tux/modules/features/bookmarks.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/*.py: Use strict type hints with Type | None instead of Optional[Type]
Use NumPy docstrings for documenting functions and classes
Prefer absolute imports; relative imports allowed only within the same module
Organize imports in order: stdlib → third-party → local
Use 88 character line length
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants
Always add imports to the top of the file unless absolutely necessary
Use async/await for I/O operations
Use custom exceptions for business logic with context logging and meaningful user messages
Use Pydantic for data validation
Keep files to a maximum of 1600 lines
Use one class or function per file when possible
Use descriptive filenames
Add appropriate logging to services and error handlers

Files:

  • src/tux/modules/features/bookmarks.py
**/modules/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

**/modules/**/*.py: Use hybrid commands (slash + traditional) for Discord commands
Implement role-based permissions for Discord commands
Use rich embeds and implement cooldowns/rate limiting for Discord interactions
Handle Discord rate limits gracefully

Files:

  • src/tux/modules/features/bookmarks.py
🧬 Code graph analysis (1)
src/tux/modules/features/bookmarks.py (1)
src/tux/ui/embeds.py (2)
  • EmbedCreator (38-235)
  • create_embed (53-194)
⏰ 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: Run All Tests (3.13.8)
  • GitHub Check: Sourcery review
  • GitHub Check: Seer Code Review
🔇 Additional comments (3)
src/tux/modules/features/bookmarks.py (3)

11-11: LGTM!

The new imports support the enhanced sticker and attachment handling introduced in this PR.

Also applies to: 15-15


49-51: LGTM!

Proper lifecycle management for the aiohttp session. This ensures the session is closed when the cog is unloaded, preventing resource leaks.


207-228: LGTM!

The sticker extraction logic correctly enforces the 10-file limit and appropriately handles only PNG and APNG format stickers with proper error handling.

"""
for attachment in message.attachments:
if len(files) >= 10:
if len(files) > 10:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix off-by-one error in file limit check.

The condition len(files) > 10 allows up to 11 files, but Discord's limit is 10 files per message. This should be >= 10 to stop at exactly 10 files, matching the logic in _get_files_from_stickers (lines 207, 211) and the PR's stated objective of enforcing the 10-file limit.

🔎 Proposed fix
-            if len(files) > 10:
+            if len(files) >= 10:
                 break
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if len(files) > 10:
if len(files) >= 10:
🤖 Prompt for AI Agents
In src/tux/modules/features/bookmarks.py around line 184, the file-count check
uses `if len(files) > 10` which permits 11 files; change the condition to `if
len(files) >= 10` so it stops at exactly 10 files (matching the checks in
_get_files_from_stickers at lines ~207/211) and ensure any related logic that
slices or appends files respects the 10-file cap.

Comment on lines +327 to +328
f"• [{sticker.name}](https://cdn.discordapp.com/stickers/{sticker.id}.png)"
if sticker.format in {StickerFormatType.png, StickerFormatType.apng}
and sticker.id != 0
else f"• {sticker.name}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

discord.py StickerItem id attribute built-in stickers documentation

💡 Result:

Yes — discord.py exposes an id attribute on StickerItem. The docs show StickerItem.id is an int (the sticker's ID) and you can call StickerItem.fetch() to resolve the full sticker (StandardSticker/GuildSticker). See the API docs for StickerItem and StandardSticker. [1][2]

Sources:

  • discord.py / pycord StickerItem (attributes: id:int, format, url; fetch()). [1]
  • StandardSticker / Sticker docs (shows id for built-in/standard stickers). [2]

🏁 Script executed:

# Check the bookmarks.py file context around lines 327-330
fd "bookmarks.py" --type f | head -5

Repository: allthingslinux/tux

Length of output: 99


🏁 Script executed:

# Look for how stickers are sourced in this file
rg "sticker" src/tux/modules/features/bookmarks.py -B 5 -A 5 | head -60

Repository: allthingslinux/tux

Length of output: 2017


🏁 Script executed:

# Read the specific section around lines 327-330
sed -n '320,335p' src/tux/modules/features/bookmarks.py

Repository: allthingslinux/tux

Length of output: 621


🌐 Web query:

discord.py how to distinguish built-in stickers from guild stickers StandardSticker GuildSticker

💡 Result:

Use the sticker's class or its type/fields.

Examples (discord.py / pycord-style):

  • By class:
    if isinstance(sticker, discord.StandardSticker):
    # built-in (standard)
    elif isinstance(sticker, discord.GuildSticker):
    # guild (custom)

  • By type or fields:
    if getattr(sticker, "type", None) == discord.StickerType.STANDARD or getattr(sticker, "pack_id", None) is not None:
    # standard
    else:
    # guild (guild_id or user present)

Short snippet:
sticker = message.stickers[0] # example
if isinstance(sticker, discord.StandardSticker):
print("standard/built-in")
elif isinstance(sticker, discord.GuildSticker):
print("guild/custom")
elif getattr(sticker, "type", None) == discord.StickerType.STANDARD:
print("standard (by type)")

References: Discord sticker objects and types; library models for StandardSticker / GuildSticker. [1][2]

Sources
[1] Discord API — Sticker object & types.
[2] Pycord / discord.py models — StandardSticker and GuildSticker.


🏁 Script executed:

# Search for sticker-related code in the bookmarks file to understand context
rg "sticker" src/tux/modules/features/bookmarks.py -B 2 -A 2 | grep -A 20 "sticker.id"

Repository: allthingslinux/tux

Length of output: 706


🏁 Script executed:

# Check imports at the top of bookmarks.py
head -50 src/tux/modules/features/bookmarks.py | grep -E "^import|^from"

Repository: allthingslinux/tux

Length of output: 511


🏁 Script executed:

# Check for discord imports and StickerItem/StandardSticker/GuildSticker references
rg "StickerItem|StandardSticker|GuildSticker|StickerType" src/tux/modules/features/bookmarks.py

Repository: allthingslinux/tux

Length of output: 171


🏁 Script executed:

# Get full imports section
sed -n '1,100p' src/tux/modules/features/bookmarks.py | grep -A 100 "^import\|^from" | head -80

Repository: allthingslinux/tux

Length of output: 2541


Use the proper discord.py approach to identify built-in stickers instead of checking sticker.id != 0.

The condition sticker.id != 0 is not the documented way to distinguish built-in stickers in discord.py. According to the library documentation, you should use isinstance(sticker, discord.StandardSticker) or check the sticker.type attribute. The id check is unreliable and may not correctly filter Discord's built-in stickers. Import StandardSticker from discord and use isinstance, or access the type attribute if available on StickerItem.

🤖 Prompt for AI Agents
In src/tux/modules/features/bookmarks.py around lines 327 to 330, the code uses
`sticker.id != 0` to try to detect built-in stickers which is unreliable;
replace that check with the proper discord.py approach: import `StandardSticker`
from discord and use `isinstance(sticker, StandardSticker)` (or alternatively
check `sticker.type` against the StandardSticker type enum) in the conditional
so only non-built-in/appropriate-format stickers build the CDN link; update the
import and adjust the conditional to use the new check.

fix(bookmarks): Better handle attachments to prevent errors from exceeding the discord character limit
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (2)
src/tux/modules/features/bookmarks.py (2)

182-183: Off-by-one error: allows 11 files instead of 10.

The condition len(files) > 10 allows the loop to continue when there are exactly 10 files, potentially adding an 11th. Discord's API limit is 10 files per message. This is inconsistent with _get_files_from_stickers (lines 205, 209) which correctly uses >= 10.

🔎 Proposed fix
         for attachment in message.attachments:
-            if len(files) > 10:
+            if len(files) >= 10:
                 break

324-331: The sticker.id != 0 check may not reliably identify built-in stickers.

According to discord.py documentation, the proper way to distinguish built-in (standard) stickers from guild stickers is to use isinstance(sticker, discord.StandardSticker) or check sticker.type. However, since StickerItem is a partial object and doesn't expose the full type, you may need to fetch the sticker first or handle potential CDN URL failures gracefully.

Consider wrapping the URL generation in a try-except or simply using the sticker name fallback for all non-PNG/APNG formats without the id check.

🔎 Suggested simplification
         if stickers:
             bullets: list[str] = [
                 f"• [{sticker.name}](https://cdn.discordapp.com/stickers/{sticker.id}.png)"
-                if sticker.format in {StickerFormatType.png, StickerFormatType.apng}
-                and sticker.id != 0
+                if sticker.format in {StickerFormatType.png, StickerFormatType.apng}
                 else f"• {sticker.name}"
                 for sticker in stickers
             ]
             sticker_list = "\n".join(bullets)

If sticker.id being 0 is a real edge case you've observed, document it with a comment explaining why the check is needed.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6e938b1 and debbcfd.

📒 Files selected for processing (1)
  • src/tux/modules/features/bookmarks.py
🧰 Additional context used
📓 Path-based instructions (3)
**/modules/**/*.py

📄 CodeRabbit inference engine (.cursor/rules/rules.mdc)

**/modules/**/*.py: Follow Discord bot cog structure patterns as defined in modules/cogs.mdc
Follow Discord command patterns as defined in modules/commands.mdc
Follow Discord event handler patterns as defined in modules/events.mdc
Follow permission patterns as defined in modules/permissions.mdc
Follow interaction response patterns as defined in modules/interactions.mdc
Follow user-facing error messages as defined in error-handling/user-feedback.mdc
Follow Discord.py Components V2 rules as defined in ui/cv2.mdc

Files:

  • src/tux/modules/features/bookmarks.py
**/*.py

📄 CodeRabbit inference engine (.cursor/rules/rules.mdc)

**/*.py: Follow Pytest configuration and patterns as defined in testing/pytest.mdc
Follow input validation as defined in security/validation.mdc
Follow error handling patterns as defined in error-handling/patterns.mdc
Follow loguru logging patterns as defined in error-handling/logging.mdc
Follow Sentry integration as defined in error-handling/sentry.mdc

**/*.py: Use strict type hints with Type | None syntax instead of Optional[Type] in Python
Use NumPy docstring format for documenting Python functions and classes
Prefer absolute imports, with relative imports allowed only within the same module
Organize imports in order: stdlib → third-party → local, with imports at the top of the file unless absolutely necessary
Use 88 character line length for Python code
Use snake_case for functions and variables, PascalCase for classes, UPPER_CASE for constants in Python
Ensure all type hints are complete for function signatures
Add docstrings for all public APIs
Keep Python files to a maximum of 1600 lines
Use async/await for I/O operations instead of blocking calls
Use custom exceptions for business logic errors
Log with context when handling errors
Do not store secrets in code, use environment variables for configuration
Validate all user inputs in Python code
One class or function per file when possible

Files:

  • src/tux/modules/features/bookmarks.py
src/tux/modules/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

src/tux/modules/**/*.py: Use hybrid commands (slash and traditional) for Discord bot commands
Implement role-based permissions for Discord commands
Implement cooldowns and rate limiting for Discord commands

Files:

  • src/tux/modules/features/bookmarks.py
🧠 Learnings (1)
📚 Learning: 2025-12-30T22:45:12.307Z
Learnt from: CR
Repo: allthingslinux/tux PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-30T22:45:12.307Z
Learning: Applies to src/tux/ui/**/*.py : Use rich embeds for Discord message formatting

Applied to files:

  • src/tux/modules/features/bookmarks.py
🧬 Code graph analysis (1)
src/tux/modules/features/bookmarks.py (1)
src/tux/ui/embeds.py (1)
  • create_embed (53-194)
⏰ 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: Run All Tests (3.13.8)
  • GitHub Check: Sourcery review
  • GitHub Check: Seer Code Review
🔇 Additional comments (4)
src/tux/modules/features/bookmarks.py (4)

11-15: LGTM!

The new imports are correctly added and appropriately used: Sequence for type annotation of message.stickers, and StickerFormatType/StickerItem for sticker format checking and typing.


47-49: Good addition for resource cleanup.

Properly closing the aiohttp.ClientSession on cog unload prevents resource leaks. This follows the correct discord.py cog lifecycle pattern.


333-355: Description truncation logic is well-implemented.

The approach correctly:

  1. Assembles all parts into a combined description
  2. Reserves space for the limit warning when truncating
  3. Guards against negative slice indices with max(0, cutoff)
  4. Cleanly trims trailing whitespace before appending the warning

This properly addresses the concern about exceeding Discord's embed description limit.


357-362: Clean consolidation of bookmark content into embed description.

The refactored approach properly passes the combined description (content + attachments + stickers) to EmbedCreator.create_embed, simplifying the embed structure and respecting character limits.

Comment on lines +241 to 246
if len(files) > 10:
return

for embed in message.embeds:
if len(files) >= 10:
if len(files) > 10:
break
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Same off-by-one error as in _get_files_from_attachments.

Both checks at lines 241 and 245 use > 10 instead of >= 10, allowing the collection to exceed 10 files. Align with _get_files_from_stickers which correctly uses >= 10.

🔎 Proposed fix
-        if len(files) > 10:
+        if len(files) >= 10:
             return
 
         for embed in message.embeds:
-            if len(files) > 10:
+            if len(files) >= 10:
                 break
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if len(files) > 10:
return
for embed in message.embeds:
if len(files) >= 10:
if len(files) > 10:
break
if len(files) >= 10:
return
for embed in message.embeds:
if len(files) >= 10:
break
🤖 Prompt for AI Agents
In src/tux/modules/features/bookmarks.py around lines 241 to 246, the file-count
guard uses "> 10" at line 241 and again at line 245 which allows collecting an
11th file; change both checks to use ">= 10" so the function returns/breaks when
the collection has reached 10 files, matching the `_get_files_from_stickers`
behavior and preventing an off-by-one over-collection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] - Bookmarking breaks with image count

2 participants