Conversation
… allow us to get information on what lines need Voice acting
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThese changes refactor dialogue logging behavior by removing NPC-line detection logic from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/wynnvp/wynncraftvp/sound/line/LineReporter.java (1)
29-40:⚠️ Potential issue | 🔴 CriticalFix concurrency race and logging violations in the async reporting path.
Line 30 now lets more misses reach
CompletableFuture.runAsync(...), butreportedLinesis a non-thread-safeLinkedList. Thecontains/add/removesequence races under concurrent access, allowing duplicate reports and inconsistent queue eviction.Additionally, lines 43–48 and 79 violate the logging guideline—replace all
System.out.printlnande.printStackTrace()calls withModCore.LOGGER.Concurrency fix
CompletableFuture.runAsync(() -> { - if (reportedLines.contains(lineData.getRealLine())) { - return; - } - reportedLines.add(lineData.getRealLine()); - - if (reportedLines.size() > 20) { - reportedLines.remove(); + synchronized (reportedLines) { + if (reportedLines.contains(lineData.getRealLine())) { + return; + } + reportedLines.add(lineData.getRealLine()); + + if (reportedLines.size() > 20) { + reportedLines.remove(); + } } try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/wynnvp/wynncraftvp/sound/line/LineReporter.java` around lines 29 - 40, The MissingLine method is racing on the non-thread-safe reportedLines LinkedList (contains/add/remove sequence) and also uses System.out.println / e.printStackTrace for logging; replace the LinkedList usage with a thread-safe construct (e.g., a ConcurrentLinkedQueue or LinkedBlockingDeque) or wrap the contains/add/remove sequence in a synchronized block around reportedLines so duplicates cannot slip through and the eviction (size > 20 -> remove) is atomic, and replace all System.out.println and e.printStackTrace calls in this class with ModCore.LOGGER calls to comply with logging guidelines (refer to MissingLine, reportedLines, and any exception handling sites).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/com/wynnvp/wynncraftvp/sound/line/LineReporter.java`:
- Around line 29-30: The reporter methods (starting with MissingLine(LineData))
currently print success/failure via System.out and call printStackTrace; replace
all uses of System.out.println/System.err.println and Throwable.printStackTrace
in this class (including the branches around lines 30, 43-48, and 79) to use
ModCore.LOGGER instead—use ModCore.LOGGER.info/debug for normal status messages
and ModCore.LOGGER.error(...) with the exception parameter for failures so the
exception stacktrace is logged through the mod logger and respects
configuration.
- Around line 29-30: The report is sending sensitive data over plain HTTP;
update the endpoint URL used in reportUnvoicedLine() inside LineReporter (the
method that POSTs player name, coordinates, and config.getWord()) to use HTTPS
(https://voicesofwynn.com/api/unvoiced-line-report/new) so the API key and
telemetry are transmitted securely; locate the HTTP string in
reportUnvoicedLine() and replace the protocol to https and ensure any HTTP
client code honors TLS (no other logic changes).
In `@src/main/java/com/wynnvp/wynncraftvp/text/OverlayHandler.java`:
- Around line 145-160: Remove the early VowLogger.logLine call for the
wrongKeyPlayed branch so logging is not emitted before the final lookup/retry;
inside the wrongKeyPlayed block only stop the audio via
ModCore.instance.soundPlayer.stopCurrentAudio() (keep any existing config checks
if needed) and let
ModCore.instance.soundPlayer.playSound(LineFormatter.formatToLineData(formattedPlaybackLine))
be solely responsible for logging/reporting the retry outcome (avoid writing
formattedPlaybackLine in wrongKeyPlayed to prevent duplicate/misleading logs).
---
Outside diff comments:
In `@src/main/java/com/wynnvp/wynncraftvp/sound/line/LineReporter.java`:
- Around line 29-40: The MissingLine method is racing on the non-thread-safe
reportedLines LinkedList (contains/add/remove sequence) and also uses
System.out.println / e.printStackTrace for logging; replace the LinkedList usage
with a thread-safe construct (e.g., a ConcurrentLinkedQueue or
LinkedBlockingDeque) or wrap the contains/add/remove sequence in a synchronized
block around reportedLines so duplicates cannot slip through and the eviction
(size > 20 -> remove) is atomic, and replace all System.out.println and
e.printStackTrace calls in this class with ModCore.LOGGER calls to comply with
logging guidelines (refer to MissingLine, reportedLines, and any exception
handling sites).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 885b68c8-6728-41e9-beb8-95e22a33061c
📒 Files selected for processing (4)
src/main/java/com/wynnvp/wynncraftvp/sound/SoundPlayer.javasrc/main/java/com/wynnvp/wynncraftvp/sound/line/LineData.javasrc/main/java/com/wynnvp/wynncraftvp/sound/line/LineReporter.javasrc/main/java/com/wynnvp/wynncraftvp/text/OverlayHandler.java
💤 Files with no reviewable changes (1)
- src/main/java/com/wynnvp/wynncraftvp/sound/line/LineData.java
kmaxii
force-pushed
the
fix/line-collector
branch
from
42d6e28 to
7c3bafd
Compare
1 participant
Summary by CodeRabbit