feat(externalchat): improve message handling and error resilience #243
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
- Add retry mechanism for failed message sends with max attempts - Implement proper state management for failed messages - Refactor message processing into separate methods - Add input validation and length checks - Improve error handling and logging - Fix incorrect variable assignment for lastOtherSize
Contributor
Reviewer's GuideIntroduces a resilient message dispatch pipeline by extracting core processing steps into dedicated methods, adding retry logic with capped attempts, input validation, and consistent error handling and logging. Sequence diagram for message sending with retry mechanismsequenceDiagram
participant ExternalChat
participant ChatGUI
participant API
participant ExtensionsAPI
ExternalChat->>ChatGUI: Ensure GUI is open
alt GUI open fails
ExternalChat->>ExtensionsAPI: Log warning (Failed to open chat GUI)
ExternalChat-->>ExternalChat: Abort message send
else GUI open succeeds
ExternalChat->>API: Send message
alt Message send succeeds
ExternalChat-->>ExternalChat: Reset retry state
else Message send fails
ExternalChat-->>ExternalChat: Update retry count
alt Retry count >= MAX_RETRY_ATTEMPTS
ExternalChat->>ExtensionsAPI: Log warning (Failed to send after max attempts)
ExternalChat-->>ExternalChat: Remove failed message
else Retry count < MAX_RETRY_ATTEMPTS
ExternalChat-->>ExternalChat: Will retry next tick
end
end
end
Class diagram for updated ExternalChat message handlingclassDiagram
class ExternalChat {
- LinkedList<String> pendingMessages
- int messageRetryCount
- static int MAX_RETRY_ATTEMPTS
- String lastFailedMessage
+ addMessageToPendingList()
+ onTickTask()
+ processPendingMessages()
+ handleMessageSending(String)
+ handleSuccessfulMessageSend()
+ handleFailedMessageSend(String)
+ updateRetryCount(String)
+ removeFailedMessageAndLog(String)
+ resetRetryState()
+ updateChatProcessors()
+ updateGlobalChatProcessor()
+ updateOtherChatProcessor()
+ sendMessage(String): boolean
}
ExternalChat --> ChatProcessor
ExternalChat --> ExtensionsAPI
ExternalChat --> PluginAPI
File-Level Changes
Possibly linked issues
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 @dm94 - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/main/java/com/deeme/tasks/externalchat/ExternalChat.java:191` </location>
<code_context>
- closeGui();
+ private void updateRetryCount(String message) {
+ if (lastFailedMessage == null || !lastFailedMessage.equals(message)) {
+ lastFailedMessage = message;
+ messageRetryCount = 1;
+ } else {
+ messageRetryCount++;
</code_context>
<issue_to_address>
Retry count logic may not handle rapid message changes correctly.
Rapid changes to the pending message could cause the retry count to reset unintentionally. Please verify if this aligns with the desired retry behavior.
</issue_to_address>
### Comment 2
<location> `src/main/java/com/deeme/tasks/externalchat/ExternalChat.java:201` </location>
<code_context>
+
+ private void removeFailedMessageAndLog(String message) {
+ pendingMessages.pollFirst();
+ extensionsAPI.getFeatureInfo(this.getClass()).addWarning(EXTERNAL_CHAT_NAME,
+ "Failed to send message after " + MAX_RETRY_ATTEMPTS + " attempts: " + message);
+ resetRetryState();
+ }
</code_context>
<issue_to_address>
Logging failed messages may expose sensitive user data.
Evaluate if logging the entire message is necessary. If sensitive data may be present, consider masking or truncating the logged content.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
private void removeFailedMessageAndLog(String message) {
pendingMessages.pollFirst();
extensionsAPI.getFeatureInfo(this.getClass()).addWarning(EXTERNAL_CHAT_NAME,
"Failed to send message after " + MAX_RETRY_ATTEMPTS + " attempts: " + message);
resetRetryState();
}
=======
private void removeFailedMessageAndLog(String message) {
pendingMessages.pollFirst();
String safeMessage;
if (message != null && message.length() > 30) {
safeMessage = message.substring(0, 30) + "...";
} else {
safeMessage = message;
}
extensionsAPI.getFeatureInfo(this.getClass()).addWarning(EXTERNAL_CHAT_NAME,
"Failed to send message after " + MAX_RETRY_ATTEMPTS + " attempts: " + safeMessage);
resetRetryState();
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/main/java/com/deeme/tasks/externalchat/ExternalChat.java:311` </location>
<code_context>
- int yPoint = (int) chatGui.getHeight() - INPUT_BOTTOM_OFFSET - (rnd.nextInt(INPUT_HEIGHT));
- chatGui.click(xPoint, yPoint);
+ private boolean sendMessage(String message) {
+ if (message == null || message.trim().isEmpty()) {
+ return false;
+ }
+
</code_context>
<issue_to_address>
Early return for empty messages is good, but consider logging or user feedback.
Returning false without any indication may hinder debugging. Logging or user feedback would help identify unexpected empty messages.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if (message == null || message.trim().isEmpty()) {
return false;
}
=======
if (message == null || message.trim().isEmpty()) {
System.out.println("[ExternalChat] Attempted to send empty or null message. Ignoring.");
// Alternatively, use a logger if available, e.g.:
// logger.warn("Attempted to send empty or null message. Ignoring.");
return false;
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `src/main/java/com/deeme/tasks/externalchat/ExternalChat.java:331` </location>
<code_context>
+ Time.sleep(200);
+
+ // Clear any existing text in input field
+ API.keyClick(1); // Ctrl+A
+ Time.sleep(50);
- Time.sleep(100);
</code_context>
<issue_to_address>
Key code for Ctrl+A may not be portable or clear.
Consider replacing the hardcoded value with a named constant or utility method to improve portability and code clarity.
Suggested implementation:
```java
// Clear any existing text in input field
API.keyClick(KEY_CTRL_A); // Ctrl+A
Time.sleep(50);
```
```java
// Press Enter to send
API.keyClick(10); // Enter key
```
```java
// Add this constant at the top of the class (with other constants)
private static final int KEY_CTRL_A = 1; // Key code for Ctrl+A (select all)
```
If your codebase has a utility method for key combinations (e.g., `API.keyCombo(int key1, int key2)`), consider using that instead of a single key code. Adjust the constant name and value if your key codes differ or are imported from a library.
</issue_to_address>
### Comment 5
<location> `src/main/java/com/deeme/tasks/externalchat/ExternalChat.java:339` </location>
<code_context>
+ Time.sleep(100);
+
+ // Press Enter to send
+ API.keyClick(10); // Enter key
+
+ // Additional delay to ensure message is sent
+ Time.sleep(300);
+
</code_context>
<issue_to_address>
Hardcoded key codes for Enter may reduce maintainability.
Consider replacing the hardcoded value with a named constant or enum for better clarity and portability.
Suggested implementation:
```java
// Press Enter to send
API.keyClick(ENTER_KEY_CODE); // Enter key
```
```java
private static final int INPUT_WIDTH_OFFSET = 20;
private static final int INPUT_BOTTOM_OFFSET = 15;
private static final int ENTER_KEY_CODE = 10;
```
</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
+191
to
+193
| if (lastFailedMessage == null || !lastFailedMessage.equals(message)) { | ||
| lastFailedMessage = message; | ||
| messageRetryCount = 1; |
Contributor
There was a problem hiding this comment.
question: Retry count logic may not handle rapid message changes correctly.
Rapid changes to the pending message could cause the retry count to reset unintentionally. Please verify if this aligns with the desired retry behavior.
Comment on lines
+199
to
+204
| private void removeFailedMessageAndLog(String message) { | ||
| pendingMessages.pollFirst(); | ||
| extensionsAPI.getFeatureInfo(this.getClass()).addWarning(EXTERNAL_CHAT_NAME, | ||
| "Failed to send message after " + MAX_RETRY_ATTEMPTS + " attempts: " + message); | ||
| resetRetryState(); | ||
| } |
Contributor
There was a problem hiding this comment.
🚨 suggestion (security): Logging failed messages may expose sensitive user data.
Evaluate if logging the entire message is necessary. If sensitive data may be present, consider masking or truncating the logged content.
Suggested change
| private void removeFailedMessageAndLog(String message) { | |
| pendingMessages.pollFirst(); | |
| extensionsAPI.getFeatureInfo(this.getClass()).addWarning(EXTERNAL_CHAT_NAME, | |
| "Failed to send message after " + MAX_RETRY_ATTEMPTS + " attempts: " + message); | |
| resetRetryState(); | |
| } | |
| private void removeFailedMessageAndLog(String message) { | |
| pendingMessages.pollFirst(); | |
| String safeMessage; | |
| if (message != null && message.length() > 30) { | |
| safeMessage = message.substring(0, 30) + "..."; | |
| } else { | |
| safeMessage = message; | |
| } | |
| extensionsAPI.getFeatureInfo(this.getClass()).addWarning(EXTERNAL_CHAT_NAME, | |
| "Failed to send message after " + MAX_RETRY_ATTEMPTS + " attempts: " + safeMessage); | |
| resetRetryState(); | |
| } |
Comment on lines
+311
to
+313
| if (message == null || message.trim().isEmpty()) { | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
suggestion: Early return for empty messages is good, but consider logging or user feedback.
Returning false without any indication may hinder debugging. Logging or user feedback would help identify unexpected empty messages.
Suggested change
| if (message == null || message.trim().isEmpty()) { | |
| return false; | |
| } | |
| if (message == null || message.trim().isEmpty()) { | |
| System.out.println("[ExternalChat] Attempted to send empty or null message. Ignoring."); | |
| // Alternatively, use a logger if available, e.g.: | |
| // logger.warn("Attempted to send empty or null message. Ignoring."); | |
| return false; | |
| } |
Comment on lines
+331
to
+332
| API.keyClick(1); // Ctrl+A | ||
| Time.sleep(50); |
Contributor
There was a problem hiding this comment.
suggestion: Key code for Ctrl+A may not be portable or clear.
Consider replacing the hardcoded value with a named constant or utility method to improve portability and code clarity.
Suggested implementation:
// Clear any existing text in input field
API.keyClick(KEY_CTRL_A); // Ctrl+A
Time.sleep(50); // Press Enter to send
API.keyClick(10); // Enter key // Add this constant at the top of the class (with other constants)
private static final int KEY_CTRL_A = 1; // Key code for Ctrl+A (select all)If your codebase has a utility method for key combinations (e.g., API.keyCombo(int key1, int key2)), consider using that instead of a single key code. Adjust the constant name and value if your key codes differ or are imported from a library.
Comment on lines
+339
to
+341
| API.keyClick(10); // Enter key | ||
|
|
||
| // Additional delay to ensure message is sent |
Contributor
There was a problem hiding this comment.
suggestion: Hardcoded key codes for Enter may reduce maintainability.
Consider replacing the hardcoded value with a named constant or enum for better clarity and portability.
Suggested implementation:
// Press Enter to send
API.keyClick(ENTER_KEY_CODE); // Enter key private static final int INPUT_WIDTH_OFFSET = 20;
private static final int INPUT_BOTTOM_OFFSET = 15;
private static final int ENTER_KEY_CODE = 10;
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
Improve external chat message handling and error resilience by adding retry logic, input validation, enhanced error handling and logging, and refactoring message and chat processing code.
New Features:
Bug Fixes:
Enhancements: