[Feature] - new modal wrappers, profile #49
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideAdds a new profile feature with Pydantic-backed modals and shared UI primitives, enhances command sync tooling and bot startup, and introduces supporting guild event handling and docs for scalable patterns. Sequence diagram for profile create/update with modal, validation, and retrysequenceDiagram
actor User
participant DiscordClient
participant Bot
participant ProfileCog as Profile
participant ModelModal
participant Pydantic as UserProfileSchema
participant RetryView
User->>DiscordClient: /profile action:create
DiscordClient->>Bot: InteractionCreate
Bot->>ProfileCog: profile(interaction, action="create")
ProfileCog->>ProfileCog: handle_edit_action(interaction, "create")
ProfileCog->>ProfileCog: lookup profiles[user_id]
alt no existing profile
ProfileCog->>ModelModal: create(model_cls=UserProfileSchema, callback=_handle_profile_submit)
ProfileCog->>DiscordClient: interaction.response.send_modal(ModelModal)
else profile exists
ProfileCog->>DiscordClient: send ephemeral "Use update instead"
end
User->>DiscordClient: submit modal fields
DiscordClient->>ModelModal: on_submit(interaction)
ModelModal->>ModelModal: collect raw_data from TextInputs
ModelModal->>UserProfileSchema: validate(**raw_data)
alt validation ok
UserProfileSchema-->>ModelModal: validated_instance
ModelModal->>ProfileCog: callback(interaction, validated_instance)
ProfileCog->>ProfileCog: store profile in profiles[user_id]
ProfileCog->>DiscordClient: interaction.response.send_message(success + embed)
else validation error
UserProfileSchema-->>ModelModal: raise ValidationError
ModelModal->>ModelModal: build error_text
ModelModal->>RetryView: create(model_cls, callback, title, initial_data=raw_data)
ModelModal->>DiscordClient: send ephemeral "Validation Failed" with RetryView
User->>DiscordClient: click Fix Errors
DiscordClient->>RetryView: retry(interaction, button)
RetryView->>ModelModal: create(model_cls, callback, title, initial_data)
RetryView->>DiscordClient: interaction.response.send_modal(ModelModal)
end
Class diagram for new UI modal and view primitivesclassDiagram
direction LR
class ui_View {
}
class BaseModal {
- log : Logger
+ BaseModal(title: str, timeout: float)
}
class CallbackModal {
- submission_callback : Callable
+ CallbackModal(callback, title: str, timeout: float)
+ on_submit(interaction)
}
class ModelModal {
- model_cls : type
- callback : Callable
- log : Logger
- _inputs : dict~str, TextInput~
+ ModelModal(model_cls: type, callback, title: str, initial_data: dict, timeout: float)
- _generate_fields(initial_data: dict)
+ on_submit(interaction)
}
class RetryView {
- model_cls : type
- callback : Callable
- title : str
- initial_data : dict
+ RetryView(model_cls: type, callback, title: str, initial_data: dict)
+ retry(interaction, button)
}
class BaseView {
- message : InteractionMessage
- log : Logger
+ BaseView(timeout: float)
+ on_error(interaction, error, item)
+ on_timeout()
+ disable_all_items()
+ reply(interaction, content, embed, embeds, file, files, ephemeral, allowed_mentions)
}
class ConfirmDeleteView {
- value : bool|None
+ ConfirmDeleteView()
+ confirm(interaction, button)
+ cancel(interaction, button)
}
class commands_Cog {
}
class Profile {
- bot : Bot
- log : Logger
- profiles : dict~int, UserProfileSchema~
+ Profile(bot: Bot)
+ profile(interaction, action: str)
+ handle_edit_action(interaction, action: str)
+ handle_show_action(interaction)
+ handle_delete_action(interaction)
+ _handle_profile_submit(interaction, profile: UserProfileSchema)
- _create_profile_embed(user, profile: UserProfileSchema) discord.Embed
}
class UserProfileSchema {
+ preferred_name : str
+ student_id : str
+ school_email : str
+ graduation_year : int
+ major : str
}
ui_View <|-- BaseView
BaseView <|-- ConfirmDeleteView
BaseModal <|-- CallbackModal
BaseModal <|-- ModelModal
commands_Cog <|-- Profile
Profile o-- ConfirmDeleteView
Profile o-- ModelModal
Profile ..> UserProfileSchema
ModelModal ..> UserProfileSchema
RetryView ..> ModelModal
RetryView ..> UserProfileSchema
BaseModal ..> logging_Logger
BaseView ..> logging_Logger
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The
synctext command assumesctx.guildis always present when handlingspec(e.g.copy_global_to(guild=ctx.guild)andtree.sync(guild=ctx.guild)); consider guarding against DM usage wherectx.guildcan beNoneto avoid runtime issues. - In
BaseView.on_timeout, appending"**[Timed Out]**"toself.message.contentwill stringifyNoneif the original message had no content; you may want to handle theNonecase explicitly to avoid showing"None"to users. - The slash
/synchandler reimplements the instance check andtree.sync()logic that_sync_commandsalready encapsulates; consider refactoring so both the prefix and slash commands share the same sync helper for consistent behavior and reduced duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `sync` text command assumes `ctx.guild` is always present when handling `spec` (e.g. `copy_global_to(guild=ctx.guild)` and `tree.sync(guild=ctx.guild)`); consider guarding against DM usage where `ctx.guild` can be `None` to avoid runtime issues.
- In `BaseView.on_timeout`, appending `"**[Timed Out]**"` to `self.message.content` will stringify `None` if the original message had no content; you may want to handle the `None` case explicitly to avoid showing `"None"` to users.
- The slash `/sync` handler reimplements the instance check and `tree.sync()` logic that `_sync_commands` already encapsulates; consider refactoring so both the prefix and slash commands share the same sync helper for consistent behavior and reduced duplication.
## Individual Comments
### Comment 1
<location> `capy_discord/exts/tools/sync.py:40-42` </location>
<code_context>
+ """Sync commands manually with "!" prefix (owner only)."""
try:
- synced = await self._sync_commands()
+ if spec in [".", "guild"]:
+ # Instant sync to current guild
+ ctx.bot.tree.copy_global_to(guild=ctx.guild)
+ synced = await ctx.bot.tree.sync(guild=ctx.guild)
+ description = f"Synced {len(synced)} commands to **current guild**."
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `ctx.guild` being `None` when using guild-specific sync options.
When the command runs in a DM (or any context where `ctx.guild` is `None`), using the `"."`/`"guild"` or `"clear"` specs will pass `None` into `copy_global_to`/`clear_commands`/`sync(guild=...)`, causing an exception. Please either block guild-specific specs when `ctx.guild is None` (e.g., early return with a message) or fall back to a global sync in that scenario.
</issue_to_address>
### Comment 2
<location> `capy_discord/exts/profile/profile.py:128-136` </location>
<code_context>
+ ephemeral=True,
+ )
+
+ await view.wait()
+
+ if view.value is True:
+ # [DB CALL]: Delete profile
+ del self.profiles[interaction.user.id]
</code_context>
<issue_to_address>
**suggestion:** Distinguish between an explicit cancel action and a timeout in the delete flow.
Right now, when the view times out without user input, `view.value` remains `None` and falls into the same path as an explicit cancel, sending “Profile deletion cancelled.” This is misleading and sends a followup after a timeout. Please handle `view.value is None` separately (e.g., no followup or a timeout-specific message) and only send the cancel message when `view.value is False`.
```suggestion
await view.wait()
if view.value is True:
# [DB CALL]: Delete profile
del self.profiles[interaction.user.id]
self.log.info("Deleted profile for user %s", interaction.user)
await interaction.followup.send("✅ Your profile has been deleted.", ephemeral=True)
elif view.value is False:
await interaction.followup.send("❌ Profile deletion cancelled.", ephemeral=True)
else:
# view.value is None: timed out, do not send a followup
return
```
</issue_to_address>
### Comment 3
<location> `capy_discord/ui/views.py:39-41` </location>
<code_context>
+ self.log.info("View timed out: %s", self)
+ self.disable_all_items()
+
+ if self.message:
+ try:
+ await self.message.edit(view=self, content=f"{self.message.content}\n\n**[Timed Out]**")
+ except discord.NotFound:
+ # Message might have been deleted
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle the case where `self.message.content` is `None` when appending the timeout marker.
If `self.message.content` is `None` (e.g., embed-only messages), this will produce the literal string `"None"` in the edited message. Consider defaulting to an empty string (e.g. `content = (self.message.content or "") + "\n\n**[Timed Out]**"`) or only appending the timeout marker when there is existing content.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+40
to
+42
| if spec in [".", "guild"]: | ||
| # Instant sync to current guild | ||
| ctx.bot.tree.copy_global_to(guild=ctx.guild) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Guard against ctx.guild being None when using guild-specific sync options.
When the command runs in a DM (or any context where ctx.guild is None), using the "."/"guild" or "clear" specs will pass None into copy_global_to/clear_commands/sync(guild=...), causing an exception. Please either block guild-specific specs when ctx.guild is None (e.g., early return with a message) or fall back to a global sync in that scenario.
Comment on lines
+128
to
+136
| await view.wait() | ||
|
|
||
| if view.value is True: | ||
| # [DB CALL]: Delete profile | ||
| del self.profiles[interaction.user.id] | ||
| self.log.info("Deleted profile for user %s", interaction.user) | ||
| await interaction.followup.send("✅ Your profile has been deleted.", ephemeral=True) | ||
| else: | ||
| await interaction.followup.send("❌ Profile deletion cancelled.", ephemeral=True) |
Contributor
There was a problem hiding this comment.
suggestion: Distinguish between an explicit cancel action and a timeout in the delete flow.
Right now, when the view times out without user input, view.value remains None and falls into the same path as an explicit cancel, sending “Profile deletion cancelled.” This is misleading and sends a followup after a timeout. Please handle view.value is None separately (e.g., no followup or a timeout-specific message) and only send the cancel message when view.value is False.
Suggested change
| await view.wait() | |
| if view.value is True: | |
| # [DB CALL]: Delete profile | |
| del self.profiles[interaction.user.id] | |
| self.log.info("Deleted profile for user %s", interaction.user) | |
| await interaction.followup.send("✅ Your profile has been deleted.", ephemeral=True) | |
| else: | |
| await interaction.followup.send("❌ Profile deletion cancelled.", ephemeral=True) | |
| await view.wait() | |
| if view.value is True: | |
| # [DB CALL]: Delete profile | |
| del self.profiles[interaction.user.id] | |
| self.log.info("Deleted profile for user %s", interaction.user) | |
| await interaction.followup.send("✅ Your profile has been deleted.", ephemeral=True) | |
| elif view.value is False: | |
| await interaction.followup.send("❌ Profile deletion cancelled.", ephemeral=True) | |
| else: | |
| # view.value is None: timed out, do not send a followup | |
| return |
Comment on lines
+39
to
+41
| if self.message: | ||
| try: | ||
| await self.message.edit(view=self, content=f"{self.message.content}\n\n**[Timed Out]**") |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Handle the case where self.message.content is None when appending the timeout marker.
If self.message.content is None (e.g., embed-only messages), this will produce the literal string "None" in the edited message. Consider defaulting to an empty string (e.g. content = (self.message.content or "") + "\n\n**[Timed Out]**") or only appending the timeout marker when there is existing content.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce profile management features, shared UI modal/view abstractions, and improve command syncing and logging behavior.
New Features:
Enhancements:
Build:
Documentation: