feat: adding attachments to snippets. - #3421
Conversation
This adds the ability to add attachments to snippets
|
I have began a review which will result in a "changes requested" verdict. I will need additional time to complete a full review. I hope to have it completed by sometime around 12PM tomorrow, Eastern. |
There was a problem hiding this comment.
Pull request overview
This PR adds support for storing, managing, and sending file attachments associated with snippets, including persisting snippet attachments in MongoDB GridFS and relaying them through the existing thread send pipeline.
Changes:
- Persist snippet attachments in MongoDB GridFS with upload/download/delete APIs and a configurable max attachment size.
- Extend snippet commands (
snippet add/edit/remove) to accept/manage an optional attachment and display attachment presence in snippet views. - Attach downloaded snippet files to outgoing thread messages (including embedding snippet images via
attachment://).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| core/thread.py | Adds files_to_upload pipeline and special handling for embedded snippet images when sending thread messages. |
| core/config.py | Introduces snippet_attachment_max_size config key and conversion handling for MB-based values. |
| core/config_help.json | Documents the new snippet_attachment_max_size config option. |
| core/clients.py | Adds GridFS bucket + upload/download/delete methods for snippet attachments. |
| cogs/modmail.py | Updates snippet CRUD commands to support attachments (validation, confirmation, GridFS persistence). |
| bot.py | Downloads snippet attachments at invocation time and wraps them to flow through existing attachment sending logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (4)
core/thread.py:2020
extincludes forwarded attachments (added just above), but the loop only iterates overmessage.attachments, so forwarded attachments appended toextare never classified intoimages/attachmentsand won't be included in the outgoing embed/log.
for i, a in enumerate(message.attachments):
attachment = ext[i]
if getattr(a, "is_snippet_attachment", False):
bot.py:1413
- This replaces the original message attachments with only the snippet attachment. If a user invokes a snippet while also attaching files, those files will be silently dropped.
if attachment is not None:
snippet_message = copy.copy(message)
snippet_message.attachments = [attachment]
ctx.message = snippet_message
bot.py:1386
- This overwrites any attachments present on the original alias-invoking message when a snippet attachment exists, so user-provided attachments would be dropped instead of being sent along with the snippet attachment.
This issue also appears on line 1410 of the same file.
attachment = await self._download_snippet_attachment(snippet_data)
if attachment is not None:
context_message.attachments = [attachment]
else:
cogs/modmail.py:363
- The help text hard-codes a 10 MB limit, but the actual limit is configurable via
snippet_attachment_max_size(default 10). This can mislead users if the config is changed.
You can also attach a file (max 10 MB) to include with the snippet.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (2)
cogs/modmail.py:399
confirm_msgis only defined inside theif ctx.message.attachments:block, but it’s referenced later unconditionally (if confirm_msg:). When adding a text-only snippet (no attachment), this will raise an UnboundLocalError.
# Handle optional attachment
file_id = None
attachment_info = None
if ctx.message.attachments:
cogs/modmail.py:363
- The docstring hard-codes “max 10 MB”, but the actual limit is configurable via
snippet_attachment_max_size(default 10). This will become inaccurate if the config is changed.
You can also attach a file (max 10 MB) to include with the snippet.
Pull request was closed
StephenDaDev
left a comment
There was a problem hiding this comment.
tested working, no serious issues found
sebkuip
left a comment
There was a problem hiding this comment.
Review for now. Did not check all, and is purely a code review. Have to do some testing later myself and look further into the code.
StephenDaDev
left a comment
There was a problem hiding this comment.
please see my mostly formatting consistency comments
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (5)
cogs/modmail.py:846
- The attachment upload error is logged without a traceback, which makes diagnosing intermittent Discord/Mongo/GridFS failures difficult. Log with
exc_info=Trueand avoid stringifying the exception separately.
except Exception as e:
logger.error("Failed to upload snippet attachment: %s", e)
core/clients.py:860
download_snippet_attachmentwill currently raiseInvalidIdorNoFileif the storedfile_idis malformed or missing. Callers handle this as a generic exception, but it would be better to translate these expected failure modes into a clear, consistent error (mirroringdelete_snippet_attachment, which treats them as benign).
grid_out = await self.fs.open_download_stream(ObjectId(file_id))
cogs/modmail.py:564
- The attachment upload error is logged without a traceback, which makes diagnosing intermittent Discord/Mongo/GridFS failures difficult. Log with
exc_info=Trueand avoid stringifying the exception separately.
This issue also appears on line 845 of the same file.
except Exception as e:
logger.error("Failed to upload snippet attachment: %s", e)
core/thread.py:2273
- This warning doesn’t include which snippet attachment failed to convert, which makes it hard to debug if multiple attachments are ever supported or filenames collide. Include the filename (best-effort) in the log message.
discord_files.append(await att.to_file())
except Exception:
logger.warning("Failed to convert snippet attachment to file.", exc_info=True)
core/thread.py:2120
- Snippet images are embedded via
attachment://{filename}before the code attempts to builddiscord_files. Ifto_file()fails for any reason, the embed will reference an attachment that wasn’t actually sent, resulting in a broken image. Consider converting todiscord.Filefirst and only callingembed.set_image(url="attachment://…")when you know the corresponding file will be included in the send call.
# Handle snippet images first (embedded directly)
for a in snippet_images_to_upload:
if not embedded_image:
embed.set_image(url=f"attachment://{a.filename}")
embed.add_field(name="Image", value=a.filename)
embedded_image = True
# Always add to files_to_upload so the attachment is physically present
files_to_upload.append(a)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
cogs/modmail.py:918
- If you keep an existing attachment when no new file is uploaded, this block should only delete
old_file_idwhen it is being replaced. As written, it will delete the old attachment even for text-only edits.
if old_file_id:
old_attachment_deleted = await self._delete_snippet_attachment(old_file_id, name)
cogs/modmail.py:893
snippet_editcurrently dropsold_file_idwhen no new attachment is provided, so a text-only edit will implicitly remove any existing attachment. That makes it impossible to edit snippet text while keeping the stored attachment, and can cause accidental data loss.
This issue also appears on line 917 of the same file.
updated_snippet = {"text": new_text or ""}
if new_file_id:
updated_snippet["file_id"] = new_file_id
bot.py:1394
- In alias expansion,
command_invocation_textalready includesinvoked_prefix, but you also prependinvoked_prefixagain when creating theStringView. This results in a duplicated prefix (e.g.,??fpreply ...), which can prevent alias->snippet invocations from being parsed correctly.
command = self._get_snippet_command()
command_invocation_text = f"{invoked_prefix}{command} {snippet_text}"
view = StringView(invoked_prefix + command_invocation_text)
This solves that if conversion later fails, the embed is sent without its referenced file.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (5)
cogs/modmail.py:865
- This exception handler logs only the exception message, dropping the traceback. Using logger.exception (or exc_info=True) will preserve stack traces for attachment upload failures during snippet edits.
except Exception as e:
logger.error("Failed to upload snippet attachment: %s", e)
core/thread.py:2283
- When converting snippet attachments to Discord files fails, the warning log doesn’t include which attachment failed, making it hard to diagnose problematic files in production logs. Include the filename (when available) in the warning.
for att in files_to_upload:
try:
discord_files.append(await att.to_file())
except Exception:
logger.warning("Failed to convert snippet attachment to file.", exc_info=True)
cogs/modmail.py:577
- This exception handler logs only the exception message, dropping the traceback. Using logger.exception (or exc_info=True) will preserve stack traces, which is important for diagnosing GridFS upload failures.
This issue also appears on line 864 of the same file.
except Exception as e:
logger.error("Failed to upload snippet attachment: %s", e)
bot.py:1322
- The download failure log drops the traceback and doesn’t include which file_id failed, which makes it hard to debug GridFS/ObjectId issues. Log the file_id and include exc_info=True.
try:
file_data, metadata = await self.api.download_snippet_attachment(snippet_data["file_id"])
except Exception as e:
logger.warning("Failed to download snippet attachment: %s", e)
return None
core/clients.py:897
- On unexpected GridFS deletion failures, the warning log only includes the exception message. Include exc_info=True so the traceback is preserved for debugging.
except Exception as e:
logger.warning("Failed to delete snippet attachment %s: %s", file_id, e)
return False
StephenDaDev
left a comment
There was a problem hiding this comment.
Tested working, no further comments from me


This adds the ability to add attachments to snippets.