Thông báo Discord: Hỗ trợ dạng embed - #72
Conversation
WalkthroughReplaces simple per-webhook text with payload-based Discord webhooks: introduces an external Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Player as Player Action
participant DotMan as DotMan.updateLeaderBoard
participant DiscordCfg as Discord.config (webhooks)
participant Webhook as Webhook (net.minevn.dotman.discord)
participant Formatter as FormatUtils.applyReplacements
participant DiscordAPI as Discord HTTP
Player->>DotMan: trigger leaderboard update
DotMan->>DotMan: build replacements map (includes %TIME%)
DotMan->>DiscordCfg: read configured webhooks
loop for each webhook
DotMan->>Webhook: send(replacements)
Webhook->>Formatter: applyReplacements(payload, replacements)
Formatter-->>Webhook: resolved payload (maps/lists/strings processed)
Webhook->>DiscordAPI: POST resolved JSON payload
alt success (2xx)
DiscordAPI-->>Webhook: 2xx
else failure
DiscordAPI-->>Webhook: error / exception (logged)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dotman-plugin/src/main/java/net/minevn/dotman/DotMan.kt(2 hunks)dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt(1 hunks)dotman-plugin/src/main/resources/discord.yml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt (1)
dotman-plugin/src/main/java/net/minevn/dotman/utils/Utils.kt (1)
info(27-27)
amadeusmz
left a comment
amadeusmz
left a comment
There was a problem hiding this comment.
Yêu cầu: Giải quyết các comment này bằng AI (ChatGPT Codex hoặc Junie), chụp lại bằng chứng và commit dưới tên AI
|
f2300a9 to
19cf963
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt (2)
56-62: Consider usingfoldfor more idiomatic Kotlin.The current implementation works correctly but could be more idiomatic:
-private fun replaceAllPlaceholders(text: String, replacements: Map<String, String>): String { - var result: String = text - for ((placeholder, replaceValue) in replacements) { - result = result.replace(placeholder, replaceValue, ignoreCase = false) - } - return result -} +private fun replaceAllPlaceholders(text: String, replacements: Map<String, String>): String { + return replacements.entries.fold(text) { acc, (placeholder, replaceValue) -> + acc.replace(placeholder, replaceValue, ignoreCase = false) + } +}
68-76: Consider extracting the magic string "color" as a constant.The hardcoded string
"color"at line 70 could be extracted for better maintainability:companion object { private const val COLOR_FIELD = "color" }Then update line 70:
if (key == COLOR_FIELD && value is String) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
dotman-plugin/build.gradle.kts(1 hunks)dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt(1 hunks)dotman-plugin/src/main/resources/discord.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dotman-plugin/src/main/resources/discord.yml
🔇 Additional comments (2)
dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt (1)
83-93: The review comment is based on incorrect assumptions about the code design.The Vietnamese documentation at lines 78-81 explicitly states the intentional behavior: "Nếu list chứa toàn String -> join thành 1 chuỗi với separator \n" (If list contains all strings → join into 1 string with \n separator). An empty list of strings logically becomes an empty string, consistent with this design. This is not a type mismatch bug; it's the documented conversion of string-only lists into multiline text content. The code is working as designed.
Likely an incorrect or invalid review comment.
dotman-plugin/build.gradle.kts (1)
20-20: Dependency version bump requires manual verification—repository server unreachable.The minevnlib-plugin version upgrade from 1.2.0-beta3 to 1.2.1-beta3 is syntactically correct, but the remote repository (https://repo.minevn.net/releases/) is currently returning HTTP 522 errors, preventing verification of artifact availability. Confirm that your local build succeeds and that this version is accessible from your configured repositories before merging.
|
@codex review |
- chuyển các hàm format vào utils - bổ sung config mẫu: sử dụng nhiều webhook cùng lúc + EOF
8afba5c to
c90e03f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dotman-plugin/src/main/java/net/minevn/dotman/DotMan.kt(2 hunks)dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt(1 hunks)dotman-plugin/src/main/java/net/minevn/dotman/discord/Webhook.kt(1 hunks)dotman-plugin/src/main/java/net/minevn/dotman/utils/FormatUtils.kt(2 hunks)dotman-plugin/src/main/resources/discord.yml(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-03T09:28:38.800Z
Learnt from: amadeusmz
Repo: minevn/dotman PR: 68
File: dotman-plugin/src/main/java/net/minevn/dotman/config/PlannedExtras.kt:10-10
Timestamp: 2025-08-03T09:28:38.800Z
Learning: In the dotman codebase, SimpleDateFormat is widely used throughout the project. The team prefers to defer thread safety improvements to DateTimeFormatter until they can address it comprehensively across the entire codebase rather than making piecemeal changes.
Applied to files:
dotman-plugin/src/main/java/net/minevn/dotman/DotMan.ktdotman-plugin/src/main/java/net/minevn/dotman/utils/FormatUtils.kt
🧬 Code graph analysis (2)
dotman-plugin/src/main/java/net/minevn/dotman/discord/Webhook.kt (1)
dotman-plugin/src/main/java/net/minevn/dotman/utils/FormatUtils.kt (1)
applyReplacements(40-58)
dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt (1)
dotman-plugin/src/main/java/net/minevn/dotman/utils/Utils.kt (1)
info(27-27)
⏰ 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). (1)
- GitHub Check: build
| if (map["enabled"] as? Boolean != true) { | ||
| return@mapNotNull null | ||
| } |
There was a problem hiding this comment.
Entries without enabled: true are silently dropped
The loader currently requires enabled == true; when the field is missing (per the YAML comment “mặc định là true”), map["enabled"] is null, so the webhook never registers. Anyone trusting the documented default will lose every notification. Please treat null as enabled and only skip when the flag is explicitly false.
- if (map["enabled"] as? Boolean != true) {
- return@mapNotNull null
- }
+ val enabled = map["enabled"] as? Boolean ?: true
+ if (!enabled) {
+ return@mapNotNull null
+ }🤖 Prompt for AI Agents
In dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt around lines
11 to 13, the current check drops entries when map["enabled"] is null; change
the logic to only skip when the flag is explicitly false. Replace the condition
so it returns null only if (map["enabled"] as? Boolean == false), thereby
treating missing/null as enabled by default and preserving documented behavior.
There was a problem hiding this comment.
nope, expected behaviour
There was a problem hiding this comment.
@minhh2792, understood! I've noted that the explicit enabled: true requirement is intentional for webhook registration.
/)_/)
(o.o)
(> <) *noted*
✏️ Learnings added
Learnt from: minhh2792
Repo: minevn/dotman PR: 72
File: dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt:0-0
Timestamp: 2025-11-02T09:53:20.038Z
Learning: In the Discord webhook configuration (dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt), webhooks must have `enabled: true` explicitly set to be registered. Missing or null `enabled` fields result in the webhook being silently dropped, and this is the expected behavior (explicit opt-in).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
7c7f5cc to
c90e03f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt (1)
11-13: Theenabledfield default behavior is still incorrect.This issue was flagged in a previous review but remains unaddressed. When
enabledis missing ornull, the current conditionmap["enabled"] as? Boolean != trueevaluates totrue, causing the webhook to be skipped. Per the documented default behavior (and the previous review), webhooks should be enabled by default when the field is omitted.Users who don't explicitly set
enabled: truewill have their webhooks silently ignored, breaking expected functionality.Apply this diff to fix the default behavior:
- if (map["enabled"] as? Boolean != true) { - return@mapNotNull null - } + val enabled = map["enabled"] as? Boolean ?: true + if (!enabled) { + return@mapNotNull null + }
🧹 Nitpick comments (1)
dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt (1)
17-19: Consider enhancing error messages with entry context.The current error handling logs only the exception message. Including the entry index or partial content (e.g., URL if available) would make debugging configuration issues much easier for users.
Consider this enhancement:
- } catch (e: TypeCastException) { - warning("Có lỗi xảy ra khi nạp webhook: ${e.message}") - null + } catch (e: TypeCastException) { + val urlHint = (raw as? Map<*, *>)?.get("url")?.let { " (url: $it)" } ?: "" + warning("Có lỗi xảy ra khi nạp webhook${urlHint}: ${e.message}") + null
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt (1)
dotman-plugin/src/main/java/net/minevn/dotman/utils/Utils.kt (1)
info(27-27)
🔇 Additional comments (1)
dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt (1)
23-25: Initialization logging looks good.The init block appropriately logs the number of successfully loaded webhooks, providing useful feedback for configuration validation.
|
@amadeusmz em đã sửa lại tí |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dotman-plugin/src/main/java/net/minevn/dotman/utils/FormatUtils.kt (1)
86-96: Empty lists are collapsed to an empty String – confirm this is desiredBecause
inputList.all { it is String }is alsotruefor an empty list,[]becomes""instead of staying as an empty list. If you prefer to preserve[](e.g. for JSON shape correctness on fields that are arrays), you could gate the join onisNotEmpty():fun applyReplacements(inputList: List<*>, replacements: Map<String, String>): Any { - val isAllStrings = inputList.all { it is String } - if (isAllStrings) { + val isAllStrings = inputList.all { it is String } + if (isAllStrings && inputList.isNotEmpty()) { val joinedText = inputList.filterIsInstance<String>().joinToString("\n") return replaceAllPlaceholders(joinedText, replacements) } else { return inputList.map { element -> applyReplacements(element, replacements) } } }If your payloads never use empty lists for “joined-text” fields, current behavior may be fine, but it’s worth deciding explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dotman-plugin/src/main/java/net/minevn/dotman/utils/FormatUtils.kt(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: minhh2792
Repo: minevn/dotman PR: 72
File: dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt:0-0
Timestamp: 2025-11-02T09:53:20.064Z
Learning: In the Discord webhook configuration (dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt), webhooks must have `enabled: true` explicitly set to be registered. Missing or null `enabled` fields result in the webhook being silently dropped, and this is the expected behavior (explicit opt-in).
📚 Learning: 2025-11-02T09:53:48.847Z
Learnt from: minhh2792
Repo: minevn/dotman PR: 72
File: dotman-plugin/src/main/java/net/minevn/dotman/utils/FormatUtils.kt:83-90
Timestamp: 2025-11-02T09:53:48.847Z
Learning: In dotman-plugin/src/main/java/net/minevn/dotman/utils/FormatUtils.kt, the processMap function intentionally does NOT replace placeholders in "color" field values before parsing them with parseHexColorToInt. Color values should remain static/literal hex strings rather than supporting dynamic placeholder replacement.
Applied to files:
dotman-plugin/src/main/java/net/minevn/dotman/utils/FormatUtils.kt
📚 Learning: 2025-08-03T09:28:38.800Z
Learnt from: amadeusmz
Repo: minevn/dotman PR: 68
File: dotman-plugin/src/main/java/net/minevn/dotman/config/PlannedExtras.kt:10-10
Timestamp: 2025-08-03T09:28:38.800Z
Learning: In the dotman codebase, SimpleDateFormat is widely used throughout the project. The team prefers to defer thread safety improvements to DateTimeFormatter until they can address it comprehensively across the entire codebase rather than making piecemeal changes.
Applied to files:
dotman-plugin/src/main/java/net/minevn/dotman/utils/FormatUtils.kt
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (1)
dotman-plugin/src/main/java/net/minevn/dotman/utils/FormatUtils.kt (1)
40-76: Recursive replacement + color handling look consistent with prior intentThe overload-based
applyReplacementsdispatcher forString/Map/Listis clean, and theMapoverload’s"color"branch correctly keeps color as a literal hex string and only parses it viaparseHexColorToIntinstead of running placeholder replacement, matching the documented “static color only” requirement. Based on learnings, this behavior is expected and avoids reintroducing dynamic colors.If
parseHexColorToInt’s behavior changes in future versions ofminevnlib-plugin(e.g. error handling or accepted formats), please re-check that invalid hex values here fail gracefully for Discord embeds.
* added %server% placeholder to discord.yml
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
dotman-plugin/src/main/resources/discord.yml (1)
10-89: Consider consolidating or clearly documenting the duplicate webhook examples.The configuration includes two nearly identical webhook entries (lines 10–48 and 51–89) for demonstration purposes. While this may be intentional to show multiple channels, the duplication is extensive. Consider:
- Adding a comment explaining that this is a template/example for multiple channels, or
- Consolidating into a single example with a note about replication for additional channels.
This will improve clarity and reduce confusion for users copying the configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dotman-plugin/src/main/java/net/minevn/dotman/DotMan.kt(2 hunks)dotman-plugin/src/main/resources/discord.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dotman-plugin/src/main/java/net/minevn/dotman/DotMan.kt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-02T09:53:20.064Z
Learnt from: minhh2792
Repo: minevn/dotman PR: 72
File: dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt:0-0
Timestamp: 2025-11-02T09:53:20.064Z
Learning: In the Discord webhook configuration (dotman-plugin/src/main/java/net/minevn/dotman/config/Discord.kt), webhooks must have `enabled: true` explicitly set to be registered. Missing or null `enabled` fields result in the webhook being silently dropped, and this is the expected behavior (explicit opt-in).
Applied to files:
dotman-plugin/src/main/resources/discord.yml
|
@coderabbitai hãy đọc các comment của tôi trong pull request này, và ghi nhớ những điểm tôi comment để áp dụng cho các pull request tương lai |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3986 @amadeusmz Tôi đã đọc tất cả các comment của bạn trong pull request này. Đây là những điểm tôi sẽ ghi nhớ cho các pull request tương lai: Về cấu trúc code:
Về cách viết code:
Về error handling:
Về technical:
Tôi đã ghi nhớ các guideline này và sẽ áp dụng cho các review trong tương lai. ✏️ Learnings added
|
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.