-
-
Notifications
You must be signed in to change notification settings - Fork 0
GH-75 Add Discord Integration #175
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.
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant new feature: Discord integration. The implementation is comprehensive, covering configuration, commands for linking/unlinking accounts, and the necessary database persistence.
My review focuses on several key areas:
- Critical Blocking Calls: There are several instances of blocking network and I/O calls (
.block()) on the main server thread. These will cause your server to freeze and must be addressed. I've marked these ascritical. - Code Readability: Some methods with asynchronous logic have deeply nested callbacks, which can be refactored for better readability and maintainability.
- Configuration Defaults: The default values in the configuration should be set to be more generic for a public plugin.
- Minor Logic Errors: I found a few minor issues, such as a duplicated condition and incorrect message targets for admin commands.
Overall, this is a great addition. Addressing the blocking calls is the highest priority before this can be safely used on a production server.
src/main/java/com/eternalcode/parcellockers/discord/DiscordClientManager.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordLinkCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/DiscordClientManager.java
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordLinkCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordLinkCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordLinkCommand.java
Outdated
Show resolved
Hide resolved
...main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java
Show resolved
Hide resolved
...main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java
Outdated
Show resolved
Hide resolved
…r's main thread is not blocked
…d verification processes
…/DiscordLinkRepositoryOrmLite.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…/DiscordLinkRepositoryOrmLite.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds Discord integration to the ParcelLockers plugin, enabling players to link their Minecraft accounts with Discord for notifications. The implementation includes account linking/unlinking commands with verification flow, repository layer for persistence, and Discord client lifecycle management.
Changes:
- Added Discord client manager with async login/logout functionality
- Implemented account linking commands with verification code flow using Paper's Dialog API
- Created repository layer with OrmLite for storing Discord-Minecraft account links
- Extended configuration with Discord settings and related messages
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| build.gradle.kts | Added discord4j-core dependency and invalid JUnit version |
| DiscordLink.java | Simple record for linking Minecraft UUID to Discord ID |
| DiscordNotificationType.java | Unused enum defining notification delivery types |
| DiscordClientManager.java | Manages Discord client lifecycle with async initialization issues |
| DiscordLinkRepository*.java | Repository interface, entity, and OrmLite implementation with query bug |
| DiscordLinkCommand.java | Account linking command with verification flow and security concerns |
| DiscordUnlinkCommand.java | Account unlinking command for self and admin operations |
| PluginConfig.java | Discord configuration settings with hardcoded test values |
| MessageConfig.java | Discord-related user messages with misleading error text |
| ParcelLockers.java | Plugin initialization with race condition and null handling issues |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordLinkCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/DiscordClientManager.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordLinkCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/configuration/implementation/PluginConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordLinkCommand.java
Outdated
Show resolved
Hide resolved
...main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/DiscordClientManager.java
Outdated
Show resolved
Hide resolved
…d DiscordSRV support
…ocking discord login
…scordLinkCommand.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…gration is enabled
…scordLinkCommand.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…scordLinkCommand.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ementation/MessageConfig.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordLinkCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordLinkCommand.java
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordSrvLinkCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordSrvUnlinkCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/command/DiscordUnlinkCommand.java
Outdated
Show resolved
Hide resolved
...va/com/eternalcode/parcellockers/discord/controller/ParcelDeliverNotificationController.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkEntity.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/DiscordClientManager.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/DiscordClientManager.java
Outdated
Show resolved
Hide resolved
...main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/DiscordClientManager.java
Outdated
Show resolved
Hide resolved
...main/java/com/eternalcode/parcellockers/discord/repository/DiscordLinkRepositoryOrmLite.java
Outdated
Show resolved
Hide resolved
...in/java/com/eternalcode/parcellockers/discord/verification/DiscordLinkValidationService.java
Outdated
Show resolved
Hide resolved
...ava/com/eternalcode/parcellockers/discord/verification/DiscordVerificationDialogFactory.java
Outdated
Show resolved
Hide resolved
...main/java/com/eternalcode/parcellockers/discord/verification/DiscordVerificationService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/eternalcode/parcellockers/discord/verification/VerificationData.java
Outdated
Show resolved
Hide resolved
…edundant javadocs
…ge templating, and add DiscordProviderPicker for improved integration
…sage delivery and update LiteCommandsBuilder type
CitralFlo
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looking good.
I have seen better solutions ex. Redis / Kafka event with minimal setup. But this is much more open. Using general solutions is more based, instead of custom discord bot code with other dependencies - like the databases.
src/main/java/com/eternalcode/parcellockers/discord/verification/VerificationCache.java
Outdated
Show resolved
Hide resolved
| import java.util.Optional; | ||
| import java.util.UUID; | ||
|
|
||
| class VerificationCache { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why extracted ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single responsibility principle
|
|
||
| public class DiscordVerificationService { | ||
|
|
||
| private final VerificationCache verificationCache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When could be init here
...ava/com/eternalcode/parcellockers/discord/verification/DiscordVerificationDialogFactory.java
Show resolved
Hide resolved
…ode expiration and improve cache management
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds a comprehensive Discord integration, supporting both a native bot and DiscordSRV. The implementation is well-structured, with clear separation of concerns for different integration providers, services, and commands. I've found a critical issue regarding the handling of the Discord bot token and a medium-severity suggestion to simplify some reactive code. Overall, this is a great addition to the plugin.
src/main/java/com/eternalcode/parcellockers/discord/DiscordProviderPicker.java
Show resolved
Hide resolved
...in/java/com/eternalcode/parcellockers/discord/notification/Discord4JNotificationService.java
Show resolved
Hide resolved
noyzys
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️
This pull request introduces comprehensive Discord integration into the ParcelLockers plugin, including support for both native Discord bots and DiscordSRV, as well as configurable messages and settings. The changes add new configuration options, message templates, and implementation classes to enable Discord account linking, notifications, and commands.
Discord Integration Features:
PluginConfig. Includes settings for enabling/disabling and specifying the Discord bot token. [1] [2]DiscordLinkService,DiscordFallbackLinkService, andDiscordLinkRepository. [1] [2] [3]DiscordClientManagerfor managing the Discord bot client lifecycle, including login and shutdown.DiscordLinkCommand,DiscordUnlinkCommand,DiscordSrvLinkCommand,DiscordSrvUnlinkCommand) and event controllers for handling parcel delivery notifications via Discord. [1] [2]Configuration and Messaging Enhancements:
MessageConfigwith a newDiscordMessagessection, allowing customization of all messages related to Discord linking, verification, and notifications, including DiscordSRV-specific messages. [1] [2]General Refactoring:
These changes collectively enable robust Discord integration, allowing users to link their Minecraft and Discord accounts, receive notifications, and configure all related messages and settings.