Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes error handling and logging in the LocationWebSocket mod by introducing a new file-based logging system, improving error recovery logic, and adding a configuration option to enable/disable the mod. The PR title contains a typo: "reseting" should be "resetting".
Key Changes
- Added file-based error logging system with
logErrors(),logInfo(), andlogDebug()methods in Config.java - Implemented error recovery logic that removes errors from the error list when connections are re-established
- Added
enableModconfiguration option to allow disabling the mod without removal
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/main/java/com/nyxz/fabric/locationwebsocket/mixin/TickEventsMixin.java |
Added null/enabled checks, dual logging (file + logger), and error recovery for UNKNOWN_ERROR |
src/main/java/com/nyxz/fabric/locationwebsocket/handler/WebSocket.java |
Updated field references, added file-based logging, and implemented error recovery for WEBSOCKET_NOT_CONNECTED |
src/main/java/com/nyxz/fabric/locationwebsocket/handler/ERRORS.java |
Added ERROR_FILE_WRITE_FAILURE enum value for file logging errors |
src/main/java/com/nyxz/fabric/locationwebsocket/LocationWebSocket.java |
Changed config directory structure, improved error message formatting, and simplified list initialization |
src/main/java/com/nyxz/fabric/locationwebsocket/Config.java |
Added file-based logging methods, enableMod field, error file handling, and new constructor for file writer initialization |
gradle.properties |
Bumped mod version from 1.0.0 to 1.1.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return enableMod; | ||
| } | ||
|
|
||
| public Writer getErrorFile() { |
There was a problem hiding this comment.
The method name getErrorFile() suggests it returns a File object, but it actually returns a Writer. Consider renaming to getErrorWriter() or getErrorFileWriter() to better reflect what it returns.
Suggested change
| public Writer getErrorFile() { | |
| public Writer getErrorWriter() { |
Comment on lines
+46
to
+48
| try{ | ||
| Files.newByteChannel(errorFilePath.toPath(), StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING).close(); | ||
| } |
There was a problem hiding this comment.
The file truncation logic creates a new ByteChannel but doesn't write anything, which means the errorFile Writer (created on line 43) may be writing to a file that gets truncated afterwards. Move the truncation logic before creating the FileWriter, or use FileWriter with append=false to handle truncation during construction.
src/main/java/com/nyxz/fabric/locationwebsocket/mixin/TickEventsMixin.java
Outdated
Show resolved
Hide resolved
src/main/java/com/nyxz/fabric/locationwebsocket/handler/WebSocket.java
Outdated
Show resolved
Hide resolved
| public static boolean enableMod = true; | ||
|
|
||
|
|
||
| public static File errorFilePath; |
There was a problem hiding this comment.
The field name errorFilePath suggests it stores a path, but it's declared as File. Consider renaming to errorFile for consistency with how it's used, or change the type to Path if you want to emphasize it's a path.
Comment on lines
148
to
238
| public static boolean logErrors(String message) { | ||
| if (LocationWebSocket.CONFIG.getErrorFile() != null) { | ||
| try { | ||
| LocationWebSocket.CONFIG.getErrorFile().write("[ERROR]: " + message + "\n"); | ||
| LocationWebSocket.CONFIG.getErrorFile().flush(); | ||
| LocationWebSocket.errors.remove(ERRORS.ERROR_FILE_WRITE_FAILURE); | ||
| return true; | ||
| } catch (IOException e) { | ||
| if (LocationWebSocket.errors.contains(ERRORS.ERROR_FILE_WRITE_FAILURE)) { | ||
| return false; | ||
| } | ||
| LocationWebSocket.errors.add(ERRORS.ERROR_FILE_WRITE_FAILURE); | ||
| LocationWebSocket.LOGGER.error("Failed to write to error log file", e); | ||
| return false; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Log errors to the error file with throwable | ||
| * @param message The error message | ||
| * @param throwable The throwable | ||
| * @return True if the error was logged, false otherwise | ||
| */ | ||
| public static boolean logErrors(String message, Throwable throwable) { | ||
| if (LocationWebSocket.CONFIG.getErrorFile() != null) { | ||
| try { | ||
| LocationWebSocket.CONFIG.getErrorFile().write("[ERROR]: " + message + " " + throwable.toString() + "\n"); | ||
| LocationWebSocket.CONFIG.getErrorFile().flush(); | ||
| LocationWebSocket.errors.remove(ERRORS.ERROR_FILE_WRITE_FAILURE); | ||
| return true; | ||
| } catch (IOException e) { | ||
| if (LocationWebSocket.errors.contains(ERRORS.ERROR_FILE_WRITE_FAILURE)) { | ||
| return false; | ||
| } | ||
| LocationWebSocket.errors.add(ERRORS.ERROR_FILE_WRITE_FAILURE); | ||
| LocationWebSocket.LOGGER.error("Failed to write to error log file", e); | ||
| return false; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Log info messages to the error file | ||
| * @param message The info message | ||
| * @return True if the info was logged, false otherwise | ||
| */ | ||
| public static boolean logInfo(String message) { | ||
| if (LocationWebSocket.CONFIG.getErrorFile() != null) { | ||
| try{ | ||
| LocationWebSocket.CONFIG.getErrorFile().write("[INFO]: "+message + "\n"); | ||
| LocationWebSocket.CONFIG.getErrorFile().flush(); | ||
| LocationWebSocket.errors.remove(ERRORS.ERROR_FILE_WRITE_FAILURE); | ||
| return true; | ||
| } catch (IOException e) { | ||
| if (LocationWebSocket.errors.contains(ERRORS.ERROR_FILE_WRITE_FAILURE)){ | ||
| return false; | ||
| } | ||
| LocationWebSocket.errors.add(ERRORS.ERROR_FILE_WRITE_FAILURE); | ||
| LocationWebSocket.LOGGER.error("Failed to write to error log file", e); | ||
| return false; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Log debug messages to the error file | ||
| * @param message The debug message | ||
| * @return True if the debug was logged, false otherwise | ||
| */ | ||
| public static boolean logDebug(String message) { | ||
| if (LocationWebSocket.CONFIG.getErrorFile() != null) { | ||
| try { | ||
| LocationWebSocket.CONFIG.getErrorFile().write("[DEBUG]: " + message + "\n"); | ||
| LocationWebSocket.CONFIG.getErrorFile().flush(); | ||
| LocationWebSocket.errors.remove(ERRORS.ERROR_FILE_WRITE_FAILURE); | ||
| return true; | ||
| } catch (IOException e) { | ||
| if (LocationWebSocket.errors.contains(ERRORS.ERROR_FILE_WRITE_FAILURE)) { | ||
| return false; | ||
| } | ||
| LocationWebSocket.errors.add(ERRORS.ERROR_FILE_WRITE_FAILURE); | ||
| LocationWebSocket.LOGGER.error("Failed to write to error log file", e); | ||
| return false; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The logging methods logErrors, logInfo, and logDebug have significant code duplication. Consider extracting the common logic into a private helper method like log(String level, String message) to improve maintainability.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…tsMixin.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ket.java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.