Skip to content

fix(bookmarks): Better handle attachments to prevent errors #1131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
malcontent5 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(bookmarks): Better handle attachments to prevent errors #1131

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 51 additions & 18 deletions src/tux/modules/features/bookmarks.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@
from __future__ import annotations

import io
from collections.abc import Sequence

import aiohttp
import discord
from discord import StickerFormatType, StickerItem
from discord.abc import Messageable
from discord.ext import commands
from loguru import logger
Expand Down Expand Up @@ -181,7 +183,7 @@ async def _get_files_from_attachments(
The list to append extracted files to.
"""
for attachment in message.attachments:
if len(files) >= 10:
if len(files) > 10:
Copy link
Contributor

@sourcery-ai sourcery-ai bot Dec 30, 2025

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.

This comment was marked as outdated.

Sign in to view

Copy link

@coderabbitai coderabbitai bot Dec 30, 2025

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.

break

if attachment.content_type and "image" in attachment.content_type:
Expand Down Expand Up @@ -240,11 +242,11 @@ async def _get_files_from_embeds(
files : list[discord.File]
The list to append extracted files to.
"""
if len(files) >= 10:
if len(files) > 10:
return

for embed in message.embeds:
if len(files) >= 10:
if len(files) > 10:
break
Comment on lines +245 to 250
Copy link

@coderabbitai coderabbitai bot Dec 31, 2025

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.


if embed.image and embed.image.url:
Expand Down Expand Up @@ -311,13 +313,56 @@ def _create_bookmark_embed(self, message: discord.Message) -> discord.Embed:
if len(content) > EMBED_MAX_DESC_LENGTH:
content = f"{content[: EMBED_MAX_DESC_LENGTH - 4]}..."

# Prepare attachment bullets
attachment_list = ""
if message.attachments:
attachment_list = "\n".join(
f"• [{att.filename}]({att.url})" for att in message.attachments
)

sticker_list: str = ""

stickers: Sequence[StickerItem] = message.stickers
Copy link

@sentry sentry bot Jan 27, 2026

Choose a reason for hiding this comment

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

Bug: The code calls .read() on StickerItem objects from message.stickers, but this method does not exist, which will cause an AttributeError when bookmarking messages with stickers.
Severity: HIGH

Suggested Fix

To fix this, you must first fetch the full Sticker object, which contains the asset information. Replace await sticker.read() with a call to fetch the sticker data. For example: full_sticker = await sticker.fetch() followed by sticker_bytes = await full_sticker.read().

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/tux/modules/features/bookmarks.py#L325

Potential issue: When a user bookmarks a message containing stickers, the
`_get_files_from_stickers` function is called. This function iterates over
`message.stickers`, which is a sequence of `StickerItem` objects. Inside the loop, the
code attempts to call `await sticker.read()`. However, the `StickerItem` class in the
discord.py library does not have a `read()` method. This will result in an
`AttributeError: 'StickerItem' object has no attribute 'read'` at runtime, causing the
bookmarking operation to fail for any message that includes stickers.

Did we get this right? 👍 / 👎 to inform future reviews.


if stickers:
bullets: list[str] = [
f"• [{sticker.name}](https://cdn.discordapp.com/stickers/{sticker.id}.png)"
Copy link

@sentry sentry bot Jan 27, 2026

Choose a reason for hiding this comment

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

Bug: The code hardcodes the cdn.discordapp.com domain for sticker URLs, which is incorrect for animated (APNG) stickers, causing them to break or not animate.
Severity: MEDIUM

Suggested Fix

Instead of manually constructing the URL, revert to using the sticker.url attribute from the StickerItem object. This property automatically provides the correct CDN domain for all sticker formats. The implementation would look like f"• [{sticker.name}]({sticker.url})".

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/tux/modules/features/bookmarks.py#L329

Potential issue: The code at line 329 constructs a URL for stickers using the format
`https://cdn.discordapp.com/stickers/{sticker.id}.png`. This URL is used for both
`StickerFormatType.png` and `StickerFormatType.apng` stickers. However, Discord serves
animated stickers (APNG) from a different CDN domain, `media.discordapp.net`. As a
result, any bookmarked message containing an animated sticker will have a link that
either fails to load or displays a static, non-animated version of the sticker. This is
a functional regression, as the previous implementation correctly used the `sticker.url`
attribute, which provides the correct URL for each sticker type.

Did we get this right? 👍 / 👎 to inform future reviews.

if sticker.format in {StickerFormatType.png, StickerFormatType.apng}
and sticker.id != 0
else f"• {sticker.name}"
Comment on lines +329 to +332
Copy link

@coderabbitai coderabbitai bot Dec 30, 2025

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.

for sticker in stickers
]
sticker_list = "\n".join(bullets)

# 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)

limit_warning = "\n\n*(Message cut off due to character limit.)*"
# Ensure we never exceed Discord's embed description limit
if len(description) > EMBED_MAX_DESC_LENGTH:
# reserve space for the notice
cutoff = EMBED_MAX_DESC_LENGTH - len(limit_warning)

# avoid negative slice sizes in extreme cases
cutoff = max(0, cutoff)

description = description[:cutoff].rstrip() + limit_warning

embed = EmbedCreator.create_embed(
bot=self.bot,
embed_type=EmbedCreator.INFO,
title="Message Bookmarked",
description=f"{content}"
if content
else "> No content available to display",
description=description,
)

# Add author to the embed
Expand All @@ -341,18 +386,6 @@ def _create_bookmark_embed(self, message: discord.Message) -> discord.Embed:
value=f"[Click Here]({message.jump_url})",
)

# Add attachments to the embed
if message.attachments:
Comment on lines 342 to -345

This comment was marked as outdated.

Sign in to view

attachments = "\n".join(
f"[{a.filename}]({a.url})" for a in message.attachments
)
embed.add_field(name="Attachments", value=attachments, inline=False)

# Add stickers to the embed
if message.stickers:
stickers = "\n".join(f"[{s.name}]({s.url})" for s in message.stickers)
embed.add_field(name="Stickers", value=stickers, inline=False)

# Handle embeds
if message.embeds:
embed.add_field(
Expand Down
Loading