Conversation
…a trycka enter för att skicka text samt shift+enter för att skapa ny rad.
WalkthroughA JavaFX greeting application is refactored into a chat application with HTTP-based networking. The changes introduce message models, a connection interface, HTTP implementation, updated UI components, and corresponding test infrastructure spanning dependencies, modules, and core logic layers. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as HelloController
participant Model as HelloModel
participant Conn as NtfyConnectionImpl
participant Server as HTTP Server<br/>(Ntfy)
rect rgb(200, 220, 255)
Note over User,Server: Send Message Flow
User->>UI: Enter message & click sendButton
UI->>UI: Read inputField, trim text
UI->>Model: setMessageToSend(message)
UI->>UI: Append outgoing message to chatArea
UI->>Model: sendMessage()
Model->>Conn: send(message)
Conn->>Server: POST /mytopic (message body)
Server-->>Conn: 200 OK
Conn-->>Model: return true
end
rect rgb(200, 255, 220)
Note over User,Server: Receive Message Flow (Async Polling)
activate Conn
Conn->>Server: GET /mytopic/json (subscribe)
loop Poll for incoming messages
Server-->>Conn: NtfyMessageDto line (JSON)
Conn->>Conn: Parse JSON, filter event=="message"
Conn->>Model: handler.accept(messageDto)
Model->>Model: Platform.runLater(append to messages)
UI->>UI: Observe messages list, render to chatArea
end
deactivate Conn
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
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: 6
🧹 Nitpick comments (13)
pom.xml (2)
82-90: Missing version formaven-compiler-plugin.Specifying an explicit version for the compiler plugin ensures reproducible builds.
<plugin> <groupId>org.apache.maven.plugins</groupId> <artifactId>maven-compiler-plugin</artifactId> + <version>3.13.0</version> <configuration>
58-63: Use a stable WireMock release instead of beta version.
wiremock:4.0.0-beta.15is a pre-release beta version. WireMock 3.x is the current stable release line. While beta test dependencies are often acceptable, prefer a stable 3.x release for more predictable behavior and long-term support.src/main/java/com/example/NtfyConnection.java (1)
6-12: Interface design looks good.The interface provides a clean abstraction for messaging operations. Minor formatting note: there are extra spaces before the parentheses in method declarations (
send (String message)vssend(String message)).public interface NtfyConnection { - boolean send (String message); + boolean send(String message); - void receive (Consumer<NtfyMessageDto> messageHandler); + void receive(Consumer<NtfyMessageDto> messageHandler); boolean sendFile(Path filePath) throws FileNotFoundException; }src/main/java/com/example/HelloController.java (2)
23-23: Consider making the model field private.The
modelfield ispublic final, which exposes internal state. If this is for testing purposes, consider package-private visibility or injecting dependencies through a setter/constructor instead.- public final HelloModel model = new HelloModel(new NtfyConnectionImpl()); + private final HelloModel model = new HelloModel(new NtfyConnectionImpl());
42-47: I was unable to access the repository to verify the exact code, but I have verified the underlying technical concern through web search. The evidence confirms that the original review comment identifies a real and valid issue with how JavaFX TextArea handles key events.Key Finding: Consuming a
KEY_PRESSEDevent does NOT prevent TextArea from inserting a newline character. The insertion happens during theKEY_TYPEDevent phase, which occurs afterKEY_PRESSED. Therefore, simply callingevent.consume()on theKEY_PRESSEDhandler will not stop the newline insertion.The correct solutions are:
- Consume the
KEY_TYPEDevent instead (or filter it)- Use
deletePreviousChar()after consuming (as the comment suggests)The review comment's suggestion is technically sound but could be more specific about using a
KEY_TYPEDfilter as the preferred approach.
Prevent newline insertion on ENTER by consuming the
KEY_TYPEDevent.Consuming
KEY_PRESSEDalone does not prevent TextArea from inserting a newline, as character insertion occurs during theKEY_TYPEDphase. Use an event filter onKEY_TYPEDto check for newline characters and consume before insertion, or alternatively callinputField.deletePreviousChar()after consumingKEY_PRESSED.Example approach:
inputField.addEventFilter(KeyEvent.KEY_TYPED, event -> { if ("\r".equals(event.getCharacter()) || "\n".equals(event.getCharacter())) { event.consume(); sendMessage(); } });src/test/java/com/example/HelloModelTest.java (1)
26-35: Add @DisplayName for consistency and verify stub timing.For consistency, add a
@DisplayNameannotation. Also, the stub is set up after model construction - sinceHelloModelconstructor callsreceiveMessage()which makes an async HTTP call, this might cause timing issues in some scenarios.@Test +@DisplayName("Given a model with messageToSend when calling sendMessage then HTTP POST should be made to /mytopic") void sendMessageToFakeServer(WireMockRuntimeInfo wmRuntimeInfo) { + stubFor(post("/mytopic").willReturn(ok())); var con = new NtfyConnectionImpl("http://localhost:" + wmRuntimeInfo.getHttpPort()); var model = new HelloModel(con); model.setMessageToSend("Hello World!"); - stubFor(post("/mytopic").willReturn(ok())); model.sendMessage(); verify(postRequestedFor(urlEqualTo("/mytopic"))); }src/main/java/com/example/HelloModel.java (5)
8-12: Remove unused imports.
URI,HttpRequest, andHttpResponseare imported but not used in this class.import javafx.collections.FXCollections; import javafx.collections.ObservableList; import java.io.*; -import java.net.URI; -import java.net.http.HttpRequest; -import java.net.http.HttpResponse; import java.util.UUID;
25-25:clientIdis generated but never used.The
clientIdfield is initialized with a UUID but not referenced anywhere. Either remove it or implement its intended purpose (e.g., prefixing messages, filtering own messages).
47-54: Consider propagating send failures.The return value of
connection.send()is ignored. If sending fails, the UI has no way to inform the user.- public void sendMessage() { + public boolean sendMessage() { String text = messageToSend.get().trim(); - if (text.isEmpty()) return; + if (text.isEmpty()) return false; - connection.send(text); + boolean success = connection.send(text); messageToSend.set(""); + return success; }
56-62: Improve error handling for file sending.Using
e.printStackTrace()is not user-friendly. Consider returning a boolean or throwing a runtime exception to allow the UI to display an error.- public void sendFile(File file) { + public boolean sendFile(File file) { try { - connection.sendFile(file.toPath()); + return connection.sendFile(file.toPath()); } catch (IOException e) { - e.printStackTrace(); + System.err.println("Failed to send file: " + e.getMessage()); + return false; } }
70-78: Remove trailing blank lines.Multiple trailing blank lines at end of file should be cleaned up.
src/main/java/com/example/NtfyConnectionImpl.java (2)
7-7: Remove unused imports.
FileandUUIDare imported but not used.-import java.io.File; import java.io.FileNotFoundException;-import java.util.UUID; import java.util.function.Consumer;Also applies to: 16-16
40-43: Consider adding request timeouts.HTTP requests without timeouts can hang indefinitely if the server is unresponsive. Consider adding a timeout to the
HttpClientor individual requests.- private final HttpClient http = HttpClient.newHttpClient(); + private final HttpClient http = HttpClient.newBuilder() + .connectTimeout(java.time.Duration.ofSeconds(10)) + .build();Also applies to: 61-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitignore(1 hunks)pom.xml(2 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/NtfyMessageDto.java(1 hunks)src/main/java/com/example/Singelton.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 (10)
.gitignore (1)
3-3: LGTM!Good practice to exclude
.envfrom version control to prevent accidental commit of sensitive configuration likeHOST_NAME.src/main/resources/com/example/hello-view.fxml (1)
7-19: Chat UI layout looks functional.The VBox layout with a read-only chat area and input controls is appropriate for a basic chat application. Minor suggestions:
- Consider adding
accessibleTextoraccessibleHelpattributes for screen reader support.- The
promptTextuses Swedish ("Skriv meddelande...") – ensure this aligns with your localization strategy.src/main/java/module-info.java (1)
4-8: LGTM!The module dependencies are correctly configured:
java.net.httpfor HTTP client functionalitycom.fasterxml.jackson.databindfor JSON serializationjava.dotenvfor environment variable loading- Opening
com.exampleto Jackson enables reflection for DTO deserializationsrc/main/java/com/example/NtfyMessageDto.java (1)
5-9: NestedAttachmentrecord appears unused.The
Attachmentrecord is defined but not referenced as a field inNtfyMessageDto. Is this intended for future use, or should it be a field in the DTO?If attachments are part of the message payload, consider adding a field:
@JsonIgnoreProperties(ignoreUnknown = true) -public record NtfyMessageDto(String id, long time, String event, String topic, String message) { +public record NtfyMessageDto(String id, long time, String event, String topic, String message, Attachment attachment) { public record Attachment(String name, String type, long size, String url) { } }src/main/java/com/example/Singelton.java (1)
1-11: Fix the class name typo from "Singelton" to "Singleton".The class name contains a spelling error. "Singelton" should be corrected to "Singleton" throughout the class definition and file name (if applicable).
Regarding whether this class is unused: the review suggests verifying its usage in the codebase, but this cannot be confirmed without access to search across all files for imports and references to this class. If it is indeed unreferenced, consider removing it to avoid dead code.
src/main/java/com/example/HelloFX.java (1)
22-24: Themainmethod signature is standard and supported in Java 25.Java 25 (via JEP 512) explicitly supports
static void main()without aString[] argsparameter. The Java Language Specification confirms this as a valid candidate main method signature, and the launcher accepts it without issues. This is not a non-standard pattern in modern Java—it's a supported feature. No launcher compatibility concerns apply.src/main/java/com/example/HelloController.java (1)
29-37: LGTM on the message listener setup.The
ListChangeListenercorrectly iterates through added messages and appends them to the chat area. SincereceiveMessage()inHelloModelusesPlatform.runLater(), UI updates are thread-safe.src/test/java/com/example/HelloModelTest.java (1)
13-24: Well-structured unit test with spy.Good use of a test spy to isolate unit behavior. The
endsWithassertion suggests the message may include a prefix (e.g., clientId) - this is correctly handled.src/test/java/com/example/NtfyConnectionSpy.java (1)
7-23: Test spy is functional but limited.The spy correctly captures the last sent message for assertions. Consider documenting that only the last message is captured, or using a
List<String>if multiple sends need verification in future tests.src/main/java/com/example/NtfyConnectionImpl.java (1)
76-94: Good use of async HTTP for receiving.Using
sendAsyncwith stream processing is appropriate for long-polling/SSE-style message reception. Silently ignoring parse errors is acceptable for robustness against malformed events.
| private void sendMessage() { | ||
| String msg = inputField.getText().trim(); | ||
| if (msg.isEmpty()) return; | ||
|
|
||
| chatArea.appendText("Du: " + msg + "\n"); | ||
| model.setMessageToSend(msg); | ||
| model.sendMessage(); | ||
| inputField.clear(); | ||
| } |
There was a problem hiding this comment.
Sent messages may appear twice in the chat.
Outgoing messages are appended locally with "Du:" prefix (line 54), but if the server echoes messages back, they will also appear via the ListChangeListener. Consider filtering out own messages in the listener or not appending locally.
🤖 Prompt for AI Agents
In src/main/java/com/example/HelloController.java around lines 50 to 58,
outgoing messages are appended locally which causes duplicates when the server
echoes them back; fix by either (A) stop appending the message locally and rely
solely on the ListChangeListener to display messages (remove
chatArea.appendText(...) and clear input after send), or (B) mark outgoing
messages with a senderId/flag on the message model before sending and update the
ListChangeListener to ignore messages whose senderId equals the current client’s
id so echoed messages are filtered out; implement one of these two approaches
and ensure inputField is still cleared and model.sendMessage() is called.
| private void attachFile() { | ||
| FileChooser chooser = new FileChooser(); | ||
| chooser.setTitle("Välj fil att bifoga"); | ||
|
|
||
| File file = chooser.showOpenDialog(sendFileButton.getScene().getWindow()); | ||
| if (file != null) { | ||
| model.sendFile(file); | ||
| chatArea.appendText("Du skickade fil: " + file.getName() + "\n"); | ||
| } |
There was a problem hiding this comment.
Missing user feedback on file send failure.
model.sendFile() may fail, but the success message is always displayed. Consider checking the result or handling errors.
File file = chooser.showOpenDialog(sendFileButton.getScene().getWindow());
if (file != null) {
- model.sendFile(file);
- chatArea.appendText("Du skickade fil: " + file.getName() + "\n");
+ boolean success = model.sendFile(file);
+ if (success) {
+ chatArea.appendText("Du skickade fil: " + file.getName() + "\n");
+ } else {
+ chatArea.appendText("Kunde inte skicka fil: " + file.getName() + "\n");
+ }
}Note: This requires changing sendFile in HelloModel to return a boolean.
Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| @Override | ||
| public void start(Stage stage) throws Exception { | ||
| System.getenv("HOST_NAME"); |
There was a problem hiding this comment.
Dead code: System.getenv() result is discarded.
The call System.getenv("HOST_NAME") retrieves the environment variable but doesn't assign the result to anything. Either remove this line or assign it to a variable if it's needed.
- System.getenv("HOST_NAME");📝 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.
| System.getenv("HOST_NAME"); |
🤖 Prompt for AI Agents
In src/main/java/com/example/HelloFX.java around line 13, the call to
System.getenv("HOST_NAME") is dead code because its return value is discarded;
either remove the line if the environment variable is not needed, or assign the
result to a variable (or use it directly) where needed (e.g., String hostName =
System.getenv("HOST_NAME"); or pass the value to whatever consumer). Ensure any
subsequent logic uses that variable or perform a null check/default if required.
| @Override | ||
| public boolean send (String message) { | ||
| HttpRequest request = HttpRequest.newBuilder() | ||
| .uri(URI.create(hostName + "/mytopic")) | ||
| .POST(HttpRequest.BodyPublishers.ofString(message)) | ||
| .build(); | ||
|
|
||
| try { | ||
| http.send(request, HttpResponse.BodyHandlers.discarding()); | ||
| return true; | ||
|
|
||
| } catch (IOException | InterruptedException e) { | ||
| System.out.println("Error sending message!"); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 1. First, check the full context of the NtfyConnectionImpl class
cat -n src/main/java/com/example/NtfyConnectionImpl.java 2>/dev/null | head -100Repository: ithsjava25/ithsjava25-java-java-fx-and-network-javafx-classrom
Length of output: 268
🌐 Web query:
Java HttpClient send vs sendAsync blocking behavior
💡 Result:
Short answer
- client.send(request, bodyHandler) is synchronous: it blocks the calling thread until the request/response cycle is finished (including whatever the BodyHandler does). [1]
- client.sendAsync(request, bodyHandler) is asynchronous: it returns a CompletableFuture immediately and the request/response work runs on the HttpClient's executor; the Future completes when the response (as handled by the BodyHandler) is ready. Calling get()/join() on that Future will block the caller. [1][2]
Extra notes (brief)
- The default HttpClient executor in OpenJDK is a cached thread pool (newCachedThreadPool) unless you supply your own, so async tasks run on those threads. [2]
- “Finished” means whatever your BodyHandler requires (e.g., BodyHandlers.ofString reads the whole body before completion). [1]
Sources
- Oracle Java HttpClient API (send / sendAsync behavior and executor note). [1]
- OpenJDK / community notes on default HttpClient thread-pool (newCachedThreadPool). [2]
Blocking HTTP call may freeze the UI if invoked from the JavaFX Application Thread.
http.send() is a synchronous (blocking) call confirmed by Java API documentation. When invoked from the JavaFX Application Thread, it will freeze the UI until the request completes. sendAsync() is available as an alternative that returns a CompletableFuture and executes on a background thread pool, or explicitly run send() on a background thread (e.g., using Task or Platform.runLater()).
🤖 Prompt for AI Agents
In src/main/java/com/example/NtfyConnectionImpl.java around lines 38–53, the
method currently calls the blocking http.send() on the calling thread which can
freeze the JavaFX Application Thread; change the method to perform the HTTP call
asynchronously: update the signature to return a CompletableFuture<Boolean> (or
provide a callback) and replace http.send(...) with http.sendAsync(request,
HttpResponse.BodyHandlers.discarding()).thenApply(response ->
true).exceptionally(e -> { /* log e */ return false; }); ensure any UI updates
resulting from the response are marshalled back to the JavaFX thread via
Platform.runLater().
| try { | ||
| http.send(request, HttpResponse.BodyHandlers.discarding()); | ||
| return true; | ||
|
|
||
| } catch (IOException | InterruptedException e) { | ||
| System.out.println("Error sending message!"); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Restore interrupt status when catching InterruptedException.
When catching InterruptedException, the thread's interrupt status should be restored to allow proper interrupt handling by calling code.
} catch (IOException | InterruptedException e) {
+ if (e instanceof InterruptedException) {
+ Thread.currentThread().interrupt();
+ }
System.out.println("Error sending message!");
return false;
}🤖 Prompt for AI Agents
In src/main/java/com/example/NtfyConnectionImpl.java around lines 45 to 52, the
catch block lumps IOException and InterruptedException together and does not
restore the thread's interrupt status; split the exceptions into separate
catches or detect InterruptedException and call
Thread.currentThread().interrupt() before returning, and include the exception
in the log message so the interrupt is preserved for callers while still
handling IO errors appropriately.
| try { | ||
| http.send(httpRequest, HttpResponse.BodyHandlers.discarding()); | ||
| return true; | ||
| } catch (IOException | InterruptedException e) { | ||
| System.out.println("Error sending file: " + e.getMessage()); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Restore interrupt status in sendFile as well.
} catch (IOException | InterruptedException e) {
+ if (e instanceof InterruptedException) {
+ Thread.currentThread().interrupt();
+ }
System.out.println("Error sending file: " + e.getMessage());
return 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.
| try { | |
| http.send(httpRequest, HttpResponse.BodyHandlers.discarding()); | |
| return true; | |
| } catch (IOException | InterruptedException e) { | |
| System.out.println("Error sending file: " + e.getMessage()); | |
| return false; | |
| } | |
| try { | |
| http.send(httpRequest, HttpResponse.BodyHandlers.discarding()); | |
| return true; | |
| } catch (IOException | InterruptedException e) { | |
| if (e instanceof InterruptedException) { | |
| Thread.currentThread().interrupt(); | |
| } | |
| System.out.println("Error sending file: " + e.getMessage()); | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/NtfyConnectionImpl.java around lines 67 to 73, the
catch block currently swallows InterruptedException by catching it in a
multi-catch and not restoring the thread's interrupt status; split the catches
so InterruptedException is handled separately, call
Thread.currentThread().interrupt() inside the InterruptedException catch, then
log the error and return false, and keep a separate catch for IOException that
logs and returns false.
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.