Conversation
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>
WalkthroughThe pull request introduces a notification messaging system to the JavaFX application. A new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant HelloController
participant HelloModel
participant NtfyConnection
participant NtfyServer
User->>HelloController: initialize()
HelloController->>HelloModel: model.receiveMessage()
HelloModel->>NtfyConnection: recieve(messageHandler)
NtfyConnection->>NtfyServer: GET /topic/json?since=all
NtfyServer-->>NtfyConnection: stream NtfyMessage lines
NtfyConnection->>HelloModel: messageHandler.accept(NtfyMessage)
HelloModel->>HelloModel: logMessage(message)
HelloModel-->>HelloController: formatedMessages updated
HelloController-->>User: display in messageView
User->>HelloController: sendMessage(ActionEvent)
HelloController->>HelloModel: model.sendMessage(text)
HelloModel->>NtfyConnection: send(message)
NtfyConnection->>NtfyServer: POST /topic with message
NtfyServer-->>NtfyConnection: 200 OK
NtfyConnection-->>HelloModel: return true
sequenceDiagram
participant User
participant HelloController
participant HelloModel
participant Dialog
participant NtfyConnection
User->>HelloController: changeTopic(ActionEvent)
HelloController->>HelloModel: model.changeTopic()
HelloModel->>Dialog: TextInputDialog.show()
User->>Dialog: enter new topic
Dialog-->>HelloModel: newTopic
HelloModel->>NtfyConnection: setTopic(newTopic)
HelloModel->>HelloModel: receiveMessage()
Note over HelloModel,NtfyConnection: Fetch messages<br/>from new topic
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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: 7
🧹 Nitpick comments (7)
src/main/java/com/example/NtfyMessage.java (1)
9-9: Consider using an immutable collection for the attachment field.Records are designed to be immutable, but
HashMap<String, String>is mutable. Consider usingMap<String, String>as the type and creating an immutable copy in a compact constructor, or usejava.util.Collections.unmodifiableMap()to enforce immutability.-public record NtfyMessage(String id, long time, String event, String topic, String message, HashMap<String, String> attachment){} +public record NtfyMessage(String id, long time, String event, String topic, String message, Map<String, String> attachment){ + public NtfyMessage { + attachment = attachment == null ? Map.of() : Map.copyOf(attachment); + } +}src/test/java/com/example/NtfyConnectionSpy.java (2)
10-10: Consider initializing the topic property with a default value.The
SimpleStringPropertyis initialized without a value, resulting innull. If any code callsgetTopic()beforesetTopic(), this could lead to unexpected null values. Consider initializing with an empty string or default topic.- private SimpleStringProperty topic = new SimpleStringProperty(); + private SimpleStringProperty topic = new SimpleStringProperty("");
35-38: Note: "recieve" is misspelled.The method name
recieveshould bereceive. However, since this implements theNtfyConnectioninterface (not provided in the review), the typo likely exists in the interface definition. Consider fixing the spelling in the interface if possible.src/main/java/com/example/NtfyConnection.java (1)
8-22: Statictopicon the interface is misleading; fix API before it spreadsDefining
SimpleStringProperty topicas a static field on the interface means all implementations would share one mutable topic, whileNtfyConnectionImplalso keeps its own instance property. That’s confusing API and easy to misuse (e.g., someone binding toNtfyConnection.topicinstead of an instance).Also,
recieveis misspelled; this typo is now part of the public surface.I’d strongly suggest:
- Remove the static
topicfield from the interface and rely solely ontopicProperty()/getTopic()/setTopic()implemented per instance.- Rename
recieve(...)toreceive(...)across the interface and implementations while this is still a fresh API.public interface NtfyConnection { - SimpleStringProperty topic = new SimpleStringProperty(); - - public SimpleStringProperty topicProperty(); - - public String getTopic(); - - public void setTopic(String topic); - - public boolean send(String message); - - public void recieve(Consumer<NtfyMessage> messageHandler); - - public boolean sendFile(File attachment); + SimpleStringProperty topicProperty(); + + String getTopic(); + + void setTopic(String topic); + + boolean send(String message); + + void receive(Consumer<NtfyMessage> messageHandler); + + boolean sendFile(File attachment); }src/main/java/com/example/HelloController.java (1)
18-36: Decouple controller from concreteNtfyConnectionImplfor testability/config
HelloControlleris directly new-ingNtfyConnectionImplandHelloModel, which makes it harder to:
- Swap connection implementations (e.g. test double, mock server, different base URL).
- Unit test the controller without hitting real network code.
Consider inverting this dependency, for example:
- Inject
NtfyConnection(and possiblyHelloModel) via constructor or setter, wiring them inHelloFXor a small DI helper, or- Use a factory that can be overridden in tests.
That keeps the controller focused on UI concerns and makes the messaging backend replaceable.
src/main/java/com/example/HelloModel.java (1)
53-56: Potentially unbounded number of active receive streams on topic change
receiveMessage()clears the list and starts a new asyncrecieve(...)each time:formatedMessages.clear(); connection.recieve(m -> Platform.runLater(() -> logMessage(m)));When
changeTopic()callsreceiveMessage()after updating the topic, the previous async receive (for the old topic) is never cancelled. If the underlying HTTP call streams updates, you may:
- Keep receiving messages from old topics, and
- Accumulate multiple active network streams over time.
Consider storing the
CompletableFuture<?>returned fromsendAsync(...)inNtfyConnectionImpl.recieve(...)and exposing a way to cancel/shutdown the previous stream before starting a new one.src/main/java/com/example/NtfyConnectionImpl.java (1)
25-35: Make HOSTNAME loading and topic handling more robustA couple of points around configuration and state:
HOSTNAMEis loaded withObjects.requireNonNull(dotenv.get("HOSTNAME"));. IfHOSTNAMEis missing in.envor the environment, you’ll get aNullPointerExceptionduring construction with no clear message about what went wrong.- The alternative constructor allows passing a HOSTNAME but doesn’t validate it (e.g., blank string).
Consider:
- Providing a clearer error when HOSTNAME is missing (e.g.
IllegalStateException("HOSTNAME not configured")).- Optionally validating that
HOSTNAMEis non-null/non-blank in the explicit constructor too.public NtfyConnectionImpl() { Dotenv dotenv = Dotenv.load(); - HOSTNAME = Objects.requireNonNull(dotenv.get("HOSTNAME")); + String host = dotenv.get("HOSTNAME"); + if (host == null || host.isBlank()) { + throw new IllegalStateException("HOSTNAME not configured in environment/.env"); + } + HOSTNAME = host; } public NtfyConnectionImpl(String HOSTNAME) { - this.HOSTNAME = HOSTNAME; + if (HOSTNAME == null || HOSTNAME.isBlank()) { + throw new IllegalArgumentException("HOSTNAME must be non-empty"); + } + this.HOSTNAME = HOSTNAME; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.gitignore(1 hunks)pom.xml(1 hunks)src/main/java/com/example/HelloController.java(1 hunks)src/main/java/com/example/HelloFX.java(1 hunks)src/main/java/com/example/HelloModel.java(1 hunks)src/main/java/com/example/NtfyConnection.java(1 hunks)src/main/java/com/example/NtfyConnectionImpl.java(1 hunks)src/main/java/com/example/NtfyMessage.java(1 hunks)src/main/java/module-info.java(1 hunks)src/main/resources/com/example/hello-view.fxml(1 hunks)src/test/java/com/example/HelloModelTest.java(1 hunks)src/test/java/com/example/NtfyConnectionSpy.java(1 hunks)
🔇 Additional comments (6)
.gitignore (1)
3-3: LGTM!Adding
.envto.gitignoreis the correct approach when using dotenv-java to prevent committing sensitive configuration like API keys or hostnames.src/main/java/com/example/HelloFX.java (1)
13-13: Whitespace-only change.This blank line has no functional impact.
src/main/resources/com/example/hello-view.fxml (1)
6-30: LGTM! UI layout looks well-structured.The new BorderPane-based layout with message list, topic label, and input controls is appropriately wired to the controller actions (
sendMessage,changeTopic,sendFile).src/main/java/module-info.java (1)
6-6: Verify the Jackson module name.The module name
tools.jackson.databindshould match the actual Jackson library being used. Standard Jackson uses module namecom.fasterxml.jackson.databind. This relates to the groupId issue flagged inpom.xml.src/test/java/com/example/HelloModelTest.java (1)
14-26: LGTM! Test correctly verifies message delegation.The test appropriately uses a spy to verify that
HelloModel.sendMessagedelegates to the connection'ssendmethod.pom.xml (1)
53-57: ---The groupId
tools.jackson.coreis correct for Jackson 3.0.1. The original review comment was incorrect. For Jackson 3.x the groupId changed to tools.jackson.core, whilecom.fasterxml.jackson.coreis the groupId for Jackson 2.x. The dependency specification in your code is accurate and uses the correct groupId for the specified version.Likely an incorrect or invalid review comment.
| /** | ||
| * Send a message, file if one has been attached else a string from the input field | ||
| * @param actionEvent | ||
| */ | ||
| public void sendMessage(ActionEvent actionEvent) { | ||
| if(attachment != null) { | ||
| connection.sendFile(attachment); | ||
| attachment = null; | ||
| } | ||
| else{ | ||
| model.sendMessage(input.getText().trim()); | ||
| } | ||
| input.clear(); | ||
| } | ||
|
|
||
| /** | ||
| * Opens a filechooser and adds selected file as attachement to be sent | ||
| * @param actionEvent | ||
| */ | ||
| public void sendFile(ActionEvent actionEvent){ | ||
| FileChooser fileChooser = new FileChooser(); | ||
| fileChooser.setTitle("Attach a file"); | ||
|
|
||
| Stage stage = (Stage) topic.getScene().getWindow(); | ||
|
|
||
|
|
||
| attachment = fileChooser.showOpenDialog(stage); | ||
| if(attachment != null) { | ||
| input.setText(attachment.getAbsolutePath()); | ||
| } | ||
| } | ||
|
|
||
| public void changeTopic(ActionEvent actionEvent) { | ||
| model.changeTopic(); |
There was a problem hiding this comment.
Surface send failures and avoid sending empty messages/attachments
In sendMessage and sendFile:
- The boolean results from
connection.send(...)andconnection.sendFile(...)are ignored, so the UI can’t distinguish between success and failure (e.g. connection down, server error). sendMessagewill happily callmodel.sendMessage(input.getText().trim())even if the trimmed text is empty; the connection then rejects it, but you still hit the network and the user gets no feedback.- For attachments, you clear
attachmentand the input regardless of whethersendFileactually worked.
Consider:
- Checking for blank input before delegating to the model, and simply returning (or showing an error) if empty.
- Using the boolean result to provide minimal user feedback (status label, alert, or log) and not clearing attachment on failure.
public void sendMessage(ActionEvent actionEvent) {
if (attachment != null) {
- connection.sendFile(attachment);
- attachment = null;
+ boolean ok = connection.sendFile(attachment);
+ if (ok) {
+ attachment = null;
+ } else {
+ // e.g. show error or log
+ return;
+ }
} else {
- model.sendMessage(input.getText().trim());
+ String text = input.getText().trim();
+ if (text.isEmpty()) {
+ return;
+ }
+ model.sendMessage(text);
}
input.clear();
}🤖 Prompt for AI Agents
In src/main/java/com/example/HelloController.java around lines 38 to 71,
sendMessage and sendFile ignore the boolean result from connection.send(...) and
connection.sendFile(...) and allow empty messages/clear attachments even on
failure; change sendMessage to first trim and if the text is blank return or set
a status/error without calling model.sendMessage, call connection.send or
model.sendMessage and capture its boolean result, only clear input (and
attachment) when the call returns true, and on false show minimal feedback (set
a status label, log or show an alert) so the UI reflects send failures;
similarly, when sending attachments call connection.sendFile and only null out
attachment and clear the input on true, otherwise preserve attachment and show
feedback.
| public void logMessage(NtfyMessage message) { | ||
| messageHistory.add(message); | ||
|
|
||
| if(message.attachment() != null) { | ||
| try { | ||
| URL url = new URL(message.attachment().get("url")); | ||
|
|
||
| if(Pattern.matches("^image\\/\\w+",message.attachment().get("type"))) {//check if the attachment is an image | ||
| ImageView image = new ImageView(new Image(url.toExternalForm())); | ||
| image.setPreserveRatio(true); | ||
| image.setFitHeight ( 250 ); | ||
| image.setFitWidth ( 250 ); | ||
|
|
||
| formatedMessages.addFirst(image); | ||
| } | ||
|
|
||
| else{ | ||
| Hyperlink hyperlink = new Hyperlink(url.toExternalForm()); | ||
| formatedMessages.addFirst(hyperlink); | ||
| } | ||
|
|
||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Date timeStamp = new Date(message.time()*1000); | ||
| DateFormat dateFormat = new SimpleDateFormat("HH:mm:ss"); | ||
| String stringMessage = dateFormat.format(timeStamp) + " : " + message.message(); | ||
| formatedMessages.addFirst(stringMessage); | ||
| } |
There was a problem hiding this comment.
formatedMessages.addFirst(...) will not compile and attachment handling is brittle
In logMessage:
formatedMessages.addFirst(image);
...
formatedMessages.addFirst(hyperlink);
...
formatedMessages.addFirst(stringMessage);ObservableList does not define addFirst; this will not compile. To prepend items, use index-based adds:
- formatedMessages.addFirst(image);
+ formatedMessages.add(0, image);
...
- formatedMessages.addFirst(hyperlink);
+ formatedMessages.add(0, hyperlink);
...
- formatedMessages.addFirst(stringMessage);
+ formatedMessages.add(0, stringMessage);Also, the attachment handling assumes the map always contains non-null "url" and "type" values:
URL url = new URL(message.attachment().get("url"));
Pattern.matches("^image\\/\\w+", message.attachment().get("type"));If either key is missing or null, you’ll get NullPointerException or MalformedURLException that aren’t caught. To make this more robust:
- Guard against null or missing keys before constructing the URL or matching the pattern.
- Optionally fall back to treating such cases as plain text or ignoring the attachment rather than throwing.
Map<String, String> att = message.attachment();
if (att == null) {
// no attachment
} else {
String urlStr = att.get("url");
String type = att.get("type");
if (urlStr != null && type != null) {
// existing logic
}
}🤖 Prompt for AI Agents
In src/main/java/com/example/HelloModel.java around lines 64 to 95, replace
non-compiling formatedMessages.addFirst(...) calls with prepending via
index-based insertion (e.g., add(0, item)) and make the attachment handling
robust by null-checking message.attachment() and verifying the "url" and "type"
values before using them; validate/try-create the URL inside a narrower
try/catch that handles MalformedURLException/IOException and on failure fall
back to treating the attachment as plain text or skip it rather than throwing a
RuntimeException, and avoid NPEs by branching when keys are missing.
| public boolean send(String message) { | ||
| if (message == null || message.isBlank()) return false; //only send messages that isn't null and has text | ||
|
|
||
|
|
||
| HttpRequest request = HttpRequest.newBuilder() | ||
| .uri(URI.create(HOSTNAME + "/" + topic.getValue())) | ||
| .POST(HttpRequest.BodyPublishers.ofString(message)) | ||
| .build(); | ||
|
|
||
| try { | ||
| HttpResponse response = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); | ||
|
|
||
| if (response.statusCode() >= 200 && response.statusCode() < 300) { | ||
| return true; | ||
| } else { | ||
| System.out.println("Error sending message " + response.statusCode()); | ||
| return false; | ||
| } | ||
| } catch (IOException e) { | ||
| System.err.println("Error sending message"); | ||
| } catch (InterruptedException e) { | ||
| System.err.println("Interrupted sending message"); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Synchronous HttpClient.send will block the JavaFX UI thread
send(String message) uses HttpClient.send(...), which is a blocking call:
HttpResponse response = httpClient.send(request, HttpResponse.BodyHandlers.ofString());Since HelloModel.sendMessage is invoked from the controller’s event handler, this will run on the JavaFX Application Thread. Any slow network or server issues will freeze the UI until the request completes or times out.
Additionally:
- On
InterruptedException, you log but don’t restore the thread’s interrupted status.
Consider moving this to an async style or a background thread, for example:
- Use
sendAsync(...)and complete aCompletableFuture<Boolean>, or - Wrap the blocking call in a
Taskor another background executor and only touch the UI viaPlatform.runLater.
And, if you keep the blocking path, restore the interrupt flag:
} catch (InterruptedException e) {
System.err.println("Interrupted sending message");
+ Thread.currentThread().interrupt();
}
---
🏁 Script executed:
```shell
#!/bin/bash
# Find HelloModel and examine sendMessage method
fd -e java | xargs rg -l "HelloModel" | head -5
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Search for sendMessage method definition
rg -nP "sendMessage|send\(" --type=java -B2 -A5 | head -40Length of output: 3073
🏁 Script executed:
#!/bin/bash
# Find controller files and event handlers
fd -e java | xargs rg -l "Controller|controller|@FXML|EventHandler" | head -10Length of output: 46
🏁 Script executed:
#!/bin/bash
# Check for existing async patterns in codebase
rg -nP "CompletableFuture|Platform\.runLater|Task|Thread|Executor" --type=java | head -20Length of output: 221
🏁 Script executed:
#!/bin/bash
# Examine the full NtfyConnectionImpl class structure
fd "NtfyConnectionImpl.java" --type f -x catLength of output: 4498
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine HelloModel.java to find sendMessage method
cat src/main/java/com/example/HelloModel.java
Length of output: 4149
🏁 Script executed:
#!/bin/bash
# Examine HelloController.java for event handlers and calls to HelloModel
cat src/main/java/com/example/HelloController.javaLength of output: 1863
Fix blocking HttpClient.send() calls and restore interrupt flag in both send() and sendFile() methods
The review comment correctly identifies that send() uses a blocking call on the JavaFX UI thread. However, the same issue exists in sendFile() (line 109), which also calls httpClient.send() directly from the UI thread via HelloController.sendFile().
Both methods also fail to restore the interrupt flag on InterruptedException:
send()method: lines 82–84sendFile()method: lines 121–123
Since the codebase already uses async patterns (e.g., sendAsync() in recieve()), consider applying the same approach to both send() and sendFile():
} catch (InterruptedException e) {
System.err.println("Interrupted sending message");
+ Thread.currentThread().interrupt();
}And wrap the blocking calls in background execution with UI updates via Platform.runLater() or Task.
Committable suggestion skipped: line range outside the PR's diff.
| public boolean sendFile(File attachment) { | ||
| if (attachment == null || !attachment.exists()) return false; | ||
|
|
||
| try { | ||
| byte[] attachmentAsBytes = Files.readAllBytes(Paths.get(attachment.getAbsolutePath())); | ||
|
|
||
| String contentType = Files.probeContentType(attachment.toPath()); | ||
|
|
||
| HttpRequest request = HttpRequest.newBuilder() | ||
| .uri(URI.create(HOSTNAME + "/" + topic.getValue())) | ||
| .header("Content-Type", contentType) | ||
| .header("Filename", attachment.getName()) | ||
| .POST(HttpRequest.BodyPublishers.ofByteArray(attachmentAsBytes)) | ||
| .build(); | ||
|
|
||
|
|
||
| HttpResponse response = httpClient.send(request, HttpResponse.BodyHandlers.ofString()); | ||
|
|
||
| if (response.statusCode() >= 200 && response.statusCode() < 300) { | ||
| return true; | ||
| } else { | ||
| System.out.println("Error sending attachment " + response.statusCode()); | ||
| return false; | ||
| } | ||
| } catch (IOException e) { | ||
| System.err.println("Error sending attachment"); | ||
| } catch (InterruptedException e) { | ||
| System.err.println("Interrupted sending attachment"); | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Guard against null content type and very large attachments in sendFile
In sendFile(File attachment):
String contentType = Files.probeContentType(attachment.toPath());
HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(HOSTNAME + "/" + topic.getValue()))
.header("Content-Type", contentType)Files.probeContentType(...) can legitimately return null (unknown type). Passing null into .header(...) will throw a NullPointerException. This makes sending some files fail unexpectedly.
Also, Files.readAllBytes(...) reads the entire file into memory, which may be problematic for large attachments.
Recommended adjustments:
- Handle
nullcontent type (e.g. default to"application/octet-stream"). - (Optional) For large files, consider using a streaming body instead of loading everything into memory.
try {
- byte[] attachmentAsBytes = Files.readAllBytes(Paths.get(attachment.getAbsolutePath()));
-
- String contentType = Files.probeContentType(attachment.toPath());
+ byte[] attachmentAsBytes = Files.readAllBytes(attachment.toPath());
+
+ String contentType = Files.probeContentType(attachment.toPath());
+ if (contentType == null) {
+ contentType = "application/octet-stream";
+ }I'll verify the key claims in this review comment by checking Java API documentation and the actual codebase.
Let me search more specifically for Java's HttpRequest API behavior with null headers:
Now let me check the actual code in the repository to verify the current state:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Find and inspect the NtfyConnectionImpl.java file
find . -name "NtfyConnectionImpl.java" -type f 2>/dev/null | head -5Length of output: 51
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Inspect lines 93-123 in the NtfyConnectionImpl.java file
sed -n '93,123p' ./src/main/java/com/example/NtfyConnectionImpl.javaLength of output: 1282
Guard against null content type in sendFile
Files.probeContentType() can legitimately return null for unknown file types. The current code passes this directly to .header("Content-Type", contentType) without a null check, causing an immediate NullPointerException and preventing those files from being sent.
Add a null guard:
String contentType = Files.probeContentType(attachment.toPath());
if (contentType == null) {
contentType = "application/octet-stream";
}Additionally, simplify the file path logic by using attachment.toPath() directly instead of Paths.get(attachment.getAbsolutePath()).
🤖 Prompt for AI Agents
In src/main/java/com/example/NtfyConnectionImpl.java around lines 93 to 123,
Files.probeContentType(...) may return null causing a NullPointerException when
passed to .header("Content-Type", ...); fix by using attachment.toPath() for
file I/O (readAllBytes and probeContentType) and guard the contentType: if
probeContentType returns null set contentType to "application/octet-stream",
then use that variable in the header; keep the existing exception handling and
return behavior.
| public void recieve(Consumer<NtfyMessage> messageHandler) { | ||
| HttpRequest httpRequest = HttpRequest.newBuilder() | ||
| .GET() | ||
| .uri(URI.create(HOSTNAME + "/" + topic.getValue() + "/json?since=all")) | ||
| .build(); | ||
|
|
||
| httpClient.sendAsync(httpRequest, HttpResponse.BodyHandlers.ofLines()) | ||
| .thenAccept(response -> response.body() | ||
| .map(s -> objectMapper.readValue(s, NtfyMessage.class)) | ||
| .filter(m -> m.event().equals("message")) | ||
| .forEach(messageHandler)); |
There was a problem hiding this comment.
🧩 Analysis chain
recieve lacks error handling and JSON parsing is unchecked
The async receive path:
httpClient.sendAsync(httpRequest, HttpResponse.BodyHandlers.ofLines())
.thenAccept(response -> response.body()
.map(s -> objectMapper.readValue(s, NtfyMessage.class))
.filter(m -> m.event().equals("message"))
.forEach(messageHandler));Issues to watch:
- No check on
response.statusCode(); non-2xx responses are treated as if they contained valid JSON messages. objectMapper.readValue(...)can throw (e.g. malformed JSON). Any such exception inside the stream pipeline will cause theCompletableFutureto complete exceptionally and stop delivering messages, without logging.- There’s no logging or retry behavior if the HTTP request itself fails.
Consider:
- Checking the status code and logging or aborting for non-2xx responses.
- Wrapping
readValuein a try/catch that logs and skips bad lines rather than killing the entire stream. - Attaching an
.exceptionally(...)or.whenComplete(...)to thesendAsyncfuture to log failures and possibly re-establish the connection.
Example sketch:
httpClient.sendAsync(httpRequest, HttpResponse.BodyHandlers.ofLines())
.thenAccept(response -> {
if (response.statusCode() / 100 != 2) {
System.err.println("Receive failed: " + response.statusCode());
return;
}
response.body().forEach(line -> {
try {
NtfyMessage m = objectMapper.readValue(line, NtfyMessage.class);
if ("message".equals(m.event())) {
messageHandler.accept(m);
}
} catch (Exception e) {
System.err.println("Failed to parse message: " + e.getMessage());
}
});
})
.exceptionally(ex -> {
System.err.println("Receive error: " + ex.getMessage());
return null;
});I'll verify the review comment by examining the actual codebase. Let me generate shell scripts to inspect the code.
---
🏁 Script executed:
```shell
#!/bin/bash
# Locate and display the complete recieve method in NtfyConnectionImpl.java
find . -type f -name "NtfyConnectionImpl.java" | head -5
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Search for the recieve method and surrounding code with context
rg -n -A 20 "public void recieve" --type java -C 3Length of output: 1588
🏁 Script executed:
#!/bin/bash
# Check for exception handling patterns in the file (try-catch, exceptionally, handle, etc.)
rg -n "catch|exceptionally|handle|\.when" --type java -C 2 | head -50Length of output: 1605
🏁 Script executed:
#!/bin/bash
# Look for NtfyConnectionImpl.java and display its full content to understand class structure
find . -type f -name "NtfyConnectionImpl.java" -exec cat {} \;Length of output: 4498
recieve lacks error handling and JSON parsing is unchecked
The async receive path has no defensive error handling:
- No check on
response.statusCode(); non-2xx responses attempt JSON parsing on invalid data. objectMapper.readValue(...)can throw (e.g., malformed JSON). Exceptions inside the stream pipeline cause theCompletableFutureto complete exceptionally and silently stop delivering messages, with no logging.- No
.exceptionally(...)or.whenComplete(...)handler to log or recover from HTTP request failures.
This contrasts with send() and sendFile() methods in the same class, which both check status codes and wrap calls in try-catch. Apply the same defensive pattern here:
- Check status code and log/abort for non-2xx responses.
- Wrap
readValuein try-catch to log and skip malformed lines instead of breaking the stream. - Attach
.exceptionally(...)to log and handle async failures.
🤖 Prompt for AI Agents
In src/main/java/com/example/NtfyConnectionImpl.java around lines 127 to 137,
the async recieve method does not handle non-2xx HTTP responses, unchecked JSON
parsing, or async failures; update the method to inspect response.statusCode()
and log/abort if it is not a 2xx before processing body lines, wrap the
objectMapper.readValue(...) call in a try-catch that logs the malformed line and
skips it instead of throwing, and attach an .exceptionally(...) or
.whenComplete(...) handler on the CompletableFuture to log and handle HTTP
request failures so the stream doesn’t silently stop delivering messages.
| package com.example; | ||
|
|
||
|
|
||
| import com.fasterxml.jackson.annotation.JsonIgnoreProperties; |
There was a problem hiding this comment.
Inconsistency: Jackson import vs pom.xml groupId.
This import uses com.fasterxml.jackson.annotation, but pom.xml declares tools.jackson.core as the groupId. Verify that the correct Jackson dependency is specified in pom.xml (typically com.fasterxml.jackson.core:jackson-databind).
🤖 Prompt for AI Agents
In src/main/java/com/example/NtfyMessage.java around line 4, the file imports
com.fasterxml.jackson.annotation.JsonIgnoreProperties but pom.xml currently
declares tools.jackson.core as the groupId; update the project dependencies so
the Jackson package referenced by imports is provided: open pom.xml and either
replace the incorrect groupId/artifact with the standard
com.fasterxml.jackson.core:jackson-databind (and companion jackson-annotations
if needed) with a proper version, or if you intentionally use a different
Jackson distribution change the import to match that distribution; ensure the
chosen dependency is added to the dependencies section and the project is
reimported so the JsonIgnoreProperties class resolves.
| @Test | ||
| void sendMessageToFakeServer(WireMockRuntimeInfo wireMockRuntimeInfo) { | ||
| var con = new NtfyConnectionImpl("http://localhost:" + wireMockRuntimeInfo.getHttpPort()); | ||
| var model = new HelloModel(con); | ||
|
|
||
| stubFor(post("JUV25D").willReturn(ok())); | ||
|
|
||
| model.sendMessage("Hello World"); | ||
|
|
||
| //verify | ||
| verify(postRequestedFor(urlEqualTo("/JUV25D")).withRequestBody(containing("Hello World"))); | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix path inconsistency in WireMock stub.
Line 33 stubs post("JUV25D") without a leading slash, but line 38 verifies urlEqualTo("/JUV25D") with a leading slash. This inconsistency will cause the stub not to match and the test to fail.
Apply this diff to fix the path:
- stubFor(post("JUV25D").willReturn(ok()));
+ stubFor(post("/JUV25D").willReturn(ok()));🤖 Prompt for AI Agents
In src/test/java/com/example/HelloModelTest.java around lines 28 to 40, the
WireMock stub uses post("JUV25D") while the verification expects
urlEqualTo("/JUV25D"), causing a path mismatch; update the stub to use the same
path format as the verification (e.g. post(urlEqualTo("/JUV25D")) or change the
stub string to "/JUV25D") so the stub and verify calls match exactly.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Tests