Temp Fix SystemChat #6
Conversation
WalkthroughThe updates add checks to avoid processing empty message components in various packet handlers, restrict packet handling to Changes
Estimated code review effort
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This PR is not a proper fix, it is a temporary fix, but it works fine. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/main/java/net/skullian/listeners/PEOutMessagePacket.java (1)
331-331: Debug statement should be removed instead of commented outFor a production-ready fix (even temporary), commented-out debug statements should be removed entirely.
- //System.out.println(event.getPacketType());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/net/skullian/listeners/PEOutMessagePacket.java(10 hunks)
🔇 Additional comments (1)
src/main/java/net/skullian/listeners/PEOutMessagePacket.java (1)
359-359: Verify per-packet sender mapping and fallback
I wasn’t able to find anynew PacketHandler(…)calls that provide a non-null sender function (the 2nd constructor parameter); it looks like every handler falls back to the defaultUNDETERMINED_SENDER(which returnsnull) and then your existingICPlayerFactory.getICPlayer(…)logic. Please double-check that:
- Every
new PacketHandler(…)instantiation inPEOutMessagePacket.javaexplicitly passes the intendedFunction<PacketSendEvent, ICPlayer>as its second argument.- The default
UNDETERMINED_SENDERand any subsequent fallback toICPlayerFactory.getICPlayer(event.getSender())produce the correctICPlayerfor all packet types (chat, title, etc.).
| @@ -62,7 +62,7 @@ private void initializeModernPacketHandlers() { | |||
| packet -> { | |||
| net.kyori.adventure.text.Component nativeComponment = ((WrapperPlayServerDisguisedChat) packet).getMessage(); | |||
| return new PacketAccessorResult( | |||
| nativeComponment != null ? NativeAdventureConverter.componentFromNative(nativeComponment) : Component.empty(), | |||
| (nativeComponment != null && nativeComponment.children().size() > 0) ? NativeAdventureConverter.componentFromNative(nativeComponment) : Component.empty(), | |||
There was a problem hiding this comment.
Potential NPE when nativeComponment is null
The check nativeComponment != null && nativeComponment.children().size() > 0 could throw NPE if children() returns null. Consider using a more defensive approach.
Apply this safer null check pattern:
- (nativeComponment != null && nativeComponment.children().size() > 0) ? NativeAdventureConverter.componentFromNative(nativeComponment) : Component.empty(),
+ (nativeComponment != null && nativeComponment.children() != null && !nativeComponment.children().isEmpty()) ? NativeAdventureConverter.componentFromNative(nativeComponment) : Component.empty(),📝 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.
| (nativeComponment != null && nativeComponment.children().size() > 0) ? NativeAdventureConverter.componentFromNative(nativeComponment) : Component.empty(), | |
| (nativeComponment != null | |
| && nativeComponment.children() != null | |
| && !nativeComponment.children().isEmpty()) | |
| ? NativeAdventureConverter.componentFromNative(nativeComponment) | |
| : Component.empty(), |
🤖 Prompt for AI Agents
In src/main/java/net/skullian/listeners/PEOutMessagePacket.java at line 65, the
current null check on nativeComponment does not account for the possibility that
nativeComponment.children() might return null, which could cause a
NullPointerException. Update the condition to first check if nativeComponment is
not null, then verify that nativeComponment.children() is not null, and finally
check that nativeComponment.children().size() is greater than 0 before calling
NativeAdventureConverter.componentFromNative. This defensive null check will
prevent potential NPEs.
| if (component.children().size() > 0) { | ||
| chatPacket.setMessage((net.kyori.adventure.text.Component) type.convertTo(component, legacyRGB)); | ||
| } |
There was a problem hiding this comment.
Inconsistent empty component handling
The code checks if component.children().size() > 0 but doesn't handle the case when the component itself has content but no children. This might skip processing valid text components.
Consider checking if the component has any content instead:
- if (component.children().size() > 0) {
- chatPacket.setMessage((net.kyori.adventure.text.Component) type.convertTo(component, legacyRGB));
- }
+ if (!component.equals(Component.empty())) {
+ chatPacket.setMessage((net.kyori.adventure.text.Component) type.convertTo(component, legacyRGB));
+ }🤖 Prompt for AI Agents
In src/main/java/net/skullian/listeners/PEOutMessagePacket.java around lines 84
to 86, the current check only processes the component if it has children,
ignoring components with content but no children. Modify the condition to check
if the component itself has any content or text instead of just children,
ensuring all valid text components are processed by chatPacket.setMessage.
| return new PacketAccessorResult( | ||
| (nativeComponent != null && nativeComponent.children().size() > 0) ? NativeAdventureConverter.componentFromNative(nativeComponent) : Component.empty(), | ||
| ChatComponentType.NativeAdventureComponent, | ||
| 0, | ||
| false | ||
| ); |
There was a problem hiding this comment.
Same NPE risk in chat handler
Similar to line 65, this has the same potential NPE issue with the children() check.
Apply the same defensive null check:
- return new PacketAccessorResult(
- (nativeComponent != null && nativeComponent.children().size() > 0) ? NativeAdventureConverter.componentFromNative(nativeComponent) : Component.empty(),
- ChatComponentType.NativeAdventureComponent,
- 0,
- false
- );
+ return new PacketAccessorResult(
+ (nativeComponent != null && nativeComponent.children() != null && !nativeComponent.children().isEmpty()) ? NativeAdventureConverter.componentFromNative(nativeComponent) : Component.empty(),
+ ChatComponentType.NativeAdventureComponent,
+ 0,
+ false
+ );📝 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.
| return new PacketAccessorResult( | |
| (nativeComponent != null && nativeComponent.children().size() > 0) ? NativeAdventureConverter.componentFromNative(nativeComponent) : Component.empty(), | |
| ChatComponentType.NativeAdventureComponent, | |
| 0, | |
| false | |
| ); | |
| return new PacketAccessorResult( | |
| (nativeComponent != null | |
| && nativeComponent.children() != null | |
| && !nativeComponent.children().isEmpty()) | |
| ? NativeAdventureConverter.componentFromNative(nativeComponent) | |
| : Component.empty(), | |
| ChatComponentType.NativeAdventureComponent, | |
| 0, | |
| false | |
| ); |
🤖 Prompt for AI Agents
In src/main/java/net/skullian/listeners/PEOutMessagePacket.java around lines 140
to 145, the code checks nativeComponent.children() without ensuring
nativeComponent is not null, risking a NullPointerException. Add a defensive
null check for nativeComponent before calling children(), similar to the fix
applied at line 65, to prevent NPE by verifying nativeComponent is not null
before accessing its children.
| if (component.children().size() > 0) { | ||
| if (InteractiveChat.version.isNewerOrEqualTo(MCVersion.V1_19_3)) { | ||
| ((ChatMessage_v1_19_3) chatMessage.getMessage()).setUnsignedChatContent((net.kyori.adventure.text.Component) type.convertTo(component, legacyRGB)); | ||
| } else { | ||
| chatMessage.getMessage().setChatContent((net.kyori.adventure.text.Component) type.convertTo(component, legacyRGB)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Repeated pattern of checking children().size() > 0
The same pattern of checking component.children().size() > 0 is repeated multiple times throughout the code. This:
- Has the same issue of potentially missing components with content but no children
- Creates code duplication
- Makes maintenance harder
Extract this logic into a helper method:
private static boolean hasContent(Component component) {
return component != null && !component.equals(Component.empty());
}
private static boolean hasContent(net.kyori.adventure.text.Component component) {
return component != null &&
(component.children() != null && !component.children().isEmpty() ||
!net.kyori.adventure.text.Component.empty().equals(component));
}Then use it consistently throughout the code.
Also applies to: 182-184, 194-203, 253-255, 298-307
🤖 Prompt for AI Agents
In src/main/java/net/skullian/listeners/PEOutMessagePacket.java around lines
164-170 and also at 182-184, 194-203, 253-255, and 298-307, the code repeatedly
checks if component.children().size() > 0 to determine if a component has
content, which can miss components with content but no children and causes
duplication. To fix this, create a helper method named hasContent that checks if
the component is not null and not empty, considering both children and content
presence as shown in the suggested method signatures. Replace all repeated
children().size() > 0 checks with calls to this new hasContent method to reduce
duplication and improve correctness.
| // Allow only SystemChat - start | ||
| if (event.getPacketType() != PacketType.Play.Server.SYSTEM_CHAT_MESSAGE) { | ||
| return; | ||
| } | ||
| // Allow only SystemChat - end | ||
|
|
||
| Player receiver = event.getPlayer(); | ||
|
|
||
| if (!(event.getLastUsedWrapper() instanceof WrapperPlayServerSystemChatMessage)) { | ||
| event.setCancelled(true); | ||
| } | ||
| event.setCancelled(true); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Restrictive packet filtering may cause issues
This change restricts processing to only SYSTEM_CHAT_MESSAGE packets and unconditionally cancels them. This is a significant behavioral change that:
- Ignores all other chat message types (CHAT_MESSAGE, DISGUISED_CHAT, etc.)
- Always cancels the original event regardless of processing outcome
This aligns with the "temp fix" nature but may break other chat functionality.
Given this is described as a temporary fix for ChatControl compatibility, verify if this restrictive filtering affects other chat features:
🏁 Script executed:
#!/bin/bash
# Search for other usages of the ignored packet types
echo "=== Searching for CHAT_MESSAGE packet usage ==="
rg -A 5 "PacketType\.Play\.Server\.CHAT_MESSAGE" --type java
echo -e "\n=== Searching for DISGUISED_CHAT packet usage ==="
rg -A 5 "PacketType\.Play\.Server\.DISGUISED_CHAT" --type java
echo -e "\n=== Searching for ChatControl plugin integration ==="
fd -e java -e yml | xargs rg -i "chatcontrol" -A 3Length of output: 5036
Refine chat packet filtering to preserve other message handlers
The current guard in src/main/java/net/skullian/listeners/PEOutMessagePacket.java (lines 347–355)
// Allow only SystemChat
if (event.getPacketType() != PacketType.Play.Server.SYSTEM_CHAT_MESSAGE) {
return;
}
Player receiver = event.getPlayer();
event.setCancelled(true);unconditionally skips all non-system chat packets—bypassing existing handlers for:
- CHAT_MESSAGE (PERedispatchSignedPacket.java & PACKET_HANDLERS in PEOutMessagePacket.java)
- DISGUISED_CHAT, DELETE_CHAT, CUSTOM_CHAT_COMPLETIONS, DISPLAY_CHAT_PREVIEW, PLAYER_CHAT_HEADER, etc.
To avoid breaking other chat features, narrow the early return and cancellation to only the SystemChat case. For example:
--- a/src/main/java/net/skullian/listeners/PEOutMessagePacket.java
+++ b/src/main/java/net/skullian/listeners/PEOutMessagePacket.java
@@ -346,9 +346,12 @@
- // Allow only SystemChat
- if (event.getPacketType() != PacketType.Play.Server.SYSTEM_CHAT_MESSAGE) {
- return;
- }
- Player receiver = event.getPlayer();
- event.setCancelled(true);
+ switch (event.getPacketType()) {
+ case SYSTEM_CHAT_MESSAGE -> {
+ Player receiver = event.getPlayer();
+ event.setCancelled(true);
+ // existing SystemChat handling…
+ }
+ default -> {
+ // continue so PACKET_HANDLERS for CHAT_MESSAGE, DISGUISED_CHAT, etc. run
+ }
+ }This ensures other chat packet handlers remain in effect while still isolating the temporary SystemChat fix.
🤖 Prompt for AI Agents
In src/main/java/net/skullian/listeners/PEOutMessagePacket.java around lines 347
to 355, the current code returns early for all non-SystemChat packets, which
prevents other chat packet handlers from running. Modify the logic so that only
SystemChat packets are cancelled and returned early, while other packet types
continue processing normally. This means removing the unconditional return for
non-SystemChat packets and applying cancellation only within the SystemChat
packet check block to preserve handling of other chat message types.
I tested it with ChatControl plugin and it works fine.
Summary by CodeRabbit
Bug Fixes
Chores