Conversation
WalkthroughThis pull request refactors a JavaFX application to implement a chat interface with HTTP-based backend communication. Changes include migrating the UI from a label-based design to a TextArea/TextField layout, transforming the model from static greeting logic to asynchronous HTTP messaging with message polling, updating module dependencies to include HTTP client support, and adding unit tests for model initialization and message handling. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as HelloController
participant Model as HelloModel
participant Backend
User->>UI: Types message in inputField
User->>UI: Clicks Send Button
UI->>UI: onSendButtonClick()
UI->>Model: sendMessage(text)
Model->>Backend: POST /topic with JSON message
Backend-->>Model: Response
rect rgb(240, 240, 240)
note right of Model: Async listening (background)
Model->>Backend: Poll /topic/json
Backend-->>Model: New message
Model->>UI: MessageHandler.onMessage(msg)
UI->>UI: Append to chatArea
end
UI-->>User: Display message in chat
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring focused attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 (4)
src/main/java/com/example/HelloFX.java (1)
12-13: Consider explicit scene dimensions for better UX.The scene sizing relies on the FXML content dimensions. While this works, explicitly setting scene dimensions (e.g.,
new Scene(fxmlLoader.load(), 640, 480)) provides more predictable window sizing for users.Apply this diff if you prefer explicit dimensions:
- Scene scene = new Scene(fxmlLoader.load()); + Scene scene = new Scene(fxmlLoader.load(), 640, 480);src/main/java/com/example/HelloModel.java (1)
10-10: Consider configuring HTTP client with timeouts.The default HTTP client has no explicit timeout configuration, which could cause the application to hang indefinitely on network issues.
Apply this diff to add timeout configuration:
- private final HttpClient client = HttpClient.newHttpClient(); + private final HttpClient client = HttpClient.newBuilder() + .connectTimeout(java.time.Duration.ofSeconds(10)) + .build();src/main/java/com/example/HelloController.java (2)
20-26: Consider adding user feedback for send failures.The current implementation provides no visual feedback if message sending fails due to network issues. Users won't know if their message was sent successfully.
Consider updating
HelloModel.sendMessage()to return aCompletableFuture<Boolean>and display feedback:@FXML public void onSendButtonClick() { String text = inputField.getText().trim(); if (!text.isEmpty()) { model.sendMessage(text).thenAccept(success -> { if (!success) { chatArea.appendText("[Failed to send message]\n"); } }); inputField.clear(); } }
15-15: Consider making the topic configurable.The topic name is hardcoded to
"javafx-chat". Consider making it configurable through a constructor parameter, configuration file, or user input.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
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/module-info.java(1 hunks)src/main/resources/com/example/hello-view.fxml(1 hunks)src/test/java/com/example/HelloModelTest.java(1 hunks)
🔇 Additional comments (3)
src/main/java/module-info.java (1)
4-4: LGTM! HTTP module dependency correctly added.The
java.net.httpmodule is properly added to support the HTTP client operations inHelloModel.src/main/resources/com/example/hello-view.fxml (1)
6-25: LGTM! UI layout is well-structured.The BorderPane layout with TextArea for chat display and HBox for input is appropriate for a chat application. The controls are properly wired with fx:id attributes and the button action is correctly bound.
src/main/java/com/example/HelloModel.java (1)
14-20: Verify BACKEND_URL environment variable configuration.The constructor requires
BACKEND_URLto be set but the test file (HelloModelTest.javaline 24-25) suggests a default of"https://ntfy.sh"should be used when the variable is unset. Ensure this aligns with your requirements.If a default URL is intended, apply this diff:
public HelloModel(String topic) { this.topic = topic; this.backendUrl = System.getenv("BACKEND_URL"); if (backendUrl == null || backendUrl.isBlank()) { - throw new IllegalStateException("BACKEND_URL is not set!"); + this.backendUrl = "https://ntfy.sh"; } }
| public void initialize() { | ||
| model = new HelloModel("javafx-chat"); // Du kan byta topic-namnet | ||
| model.listen(msg -> chatArea.appendText(msg + "\n")); | ||
| } |
There was a problem hiding this comment.
CRITICAL: Raw JSON displayed in chat area instead of parsed messages.
Line 16 appends the raw message from the listener to chatArea. Based on HelloModel.listen() (line 44 in HelloModel.java), this will be a raw JSON line from the HTTP response, not the parsed message text. Users will see JSON strings like {"message":"Hello"} instead of just "Hello".
Apply this diff to parse the message before displaying:
@FXML
public void initialize() {
model = new HelloModel("javafx-chat"); // Du kan byta topic-namnet
- model.listen(msg -> chatArea.appendText(msg + "\n"));
+ model.listen(msg -> {
+ // Parse JSON to extract message text
+ // Basic parsing - consider using a JSON library for production
+ int start = msg.indexOf("\"message\"");
+ if (start != -1) {
+ start = msg.indexOf("\"", start + 9) + 1;
+ int end = msg.indexOf("\"", start);
+ if (end != -1) {
+ String messageText = msg.substring(start, end);
+ chatArea.appendText(messageText + "\n");
+ }
+ }
+ });
}Better solution: Extract the message parsing logic to HelloModel so the MessageHandler.onMessage() receives the parsed message text, not raw JSON.
📝 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.
| public void initialize() { | |
| model = new HelloModel("javafx-chat"); // Du kan byta topic-namnet | |
| model.listen(msg -> chatArea.appendText(msg + "\n")); | |
| } | |
| @FXML | |
| public void initialize() { | |
| model = new HelloModel("javafx-chat"); // Du kan byta topic-namnet | |
| model.listen(msg -> { | |
| // Parse JSON to extract message text | |
| // Basic parsing - consider using a JSON library for production | |
| int start = msg.indexOf("\"message\""); | |
| if (start != -1) { | |
| start = msg.indexOf("\"", start + 9) + 1; | |
| int end = msg.indexOf("\"", start); | |
| if (end != -1) { | |
| String messageText = msg.substring(start, end); | |
| chatArea.appendText(messageText + "\n"); | |
| } | |
| } | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/HelloController.java around lines 14–17, the
listener currently appends the raw JSON string to chatArea; change this so the
handler receives/display only the parsed message text. Fix by either (A) parsing
the JSON here before calling chatArea.appendText — extract the "message" field
from the JSON payload and append that (with a safe try/catch to fallback to the
raw payload on parse errors), or (B, preferred) move the JSON parsing into
HelloModel.listen so it invokes the callback with the parsed message text
(update HelloModel.listen to parse each raw line, extract "message" and pass
that String to the registered listener), then keep this controller code as
model.listen(msg -> chatArea.appendText(msg + "\n")).
| public void sendMessage(String message) { | ||
| String json = "{\"message\": \"" + message + "\"}"; | ||
| String url = backendUrl + "/" + topic; | ||
|
|
||
| HttpRequest request = HttpRequest.newBuilder() | ||
| .uri(URI.create(url)) | ||
| .header("Content-Type", "application/json") | ||
| .POST(HttpRequest.BodyPublishers.ofString(json)) | ||
| .build(); | ||
|
|
||
| client.sendAsync(request, HttpResponse.BodyHandlers.ofString()); | ||
| } |
There was a problem hiding this comment.
CRITICAL: JSON injection vulnerability and missing error handling.
Line 23 constructs JSON by string concatenation without escaping user input. If message contains quotes, backslashes, or newlines, it will break the JSON format. More critically, this could enable injection attacks.
Additionally, line 32 ignores the HTTP response and provides no error handling for network failures.
Apply this diff to use proper JSON handling and add basic error handling:
public void sendMessage(String message) {
- String json = "{\"message\": \"" + message + "\"}";
+ // Escape quotes, backslashes, and control characters
+ String escapedMessage = message
+ .replace("\\", "\\\\")
+ .replace("\"", "\\\"")
+ .replace("\n", "\\n")
+ .replace("\r", "\\r")
+ .replace("\t", "\\t");
+ String json = "{\"message\": \"" + escapedMessage + "\"}";
String url = backendUrl + "/" + topic;
HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(url))
.header("Content-Type", "application/json")
.POST(HttpRequest.BodyPublishers.ofString(json))
.build();
- client.sendAsync(request, HttpResponse.BodyHandlers.ofString());
+ client.sendAsync(request, HttpResponse.BodyHandlers.ofString())
+ .exceptionally(e -> {
+ System.err.println("Failed to send message: " + e.getMessage());
+ return null;
+ });
}Better solution: Use a proper JSON library like Jackson or Gson instead of manual string concatenation.
🤖 Prompt for AI Agents
In src/main/java/com/example/HelloModel.java around lines 22 to 33, the method
builds JSON via string concatenation (risking JSON injection and broken
payloads) and ignores the HTTP response/errors; replace the manual concatenation
with a JSON serializer (e.g., Jackson ObjectMapper or Gson) to build a safe JSON
payload from a Map or small DTO and serialize it to a String, then send the
request and handle the result: await the response (or attach
thenApply/exceptionally on the CompletableFuture), check HTTP status codes and
log or throw on non-2xx, and catch/log network or serialization exceptions so
failures are not silently ignored.
| public CompletableFuture<Void> listen(MessageHandler handler) { | ||
| String url = backendUrl + "/" + topic + "/json"; | ||
| HttpRequest request = HttpRequest.newBuilder() | ||
| .uri(URI.create(url)) | ||
| .build(); | ||
|
|
||
| return client.sendAsync(request, HttpResponse.BodyHandlers.ofLines()) | ||
| .thenAccept(response -> response.body().forEach(line -> { | ||
| if (line.contains("\"message\"")) { | ||
| handler.onMessage(line); | ||
| } | ||
| })); | ||
| } |
There was a problem hiding this comment.
CRITICAL: Naive JSON parsing and missing error handling.
Line 43 uses line.contains("\"message\"") which will match any line containing the substring "message" in quotes, not just valid JSON message fields. This could lead to incorrect message parsing or processing of non-message data.
Additionally, there's no error handling for connection failures, timeouts, or malformed responses.
Consider using a proper JSON parser. Here's a basic improvement with error handling:
public CompletableFuture<Void> listen(MessageHandler handler) {
String url = backendUrl + "/" + topic + "/json";
HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(url))
.build();
return client.sendAsync(request, HttpResponse.BodyHandlers.ofLines())
.thenAccept(response -> response.body().forEach(line -> {
- if (line.contains("\"message\"")) {
+ // Basic JSON parsing - consider using a proper JSON library
+ if (line.trim().matches(".*\"message\"\\s*:\\s*\".*\".*")) {
handler.onMessage(line);
}
- }));
+ }))
+ .exceptionally(e -> {
+ System.err.println("Failed to listen for messages: " + e.getMessage());
+ return null;
+ });
}Recommended: Use a JSON library (Jackson/Gson) to properly parse the response and extract the message field value.
📝 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.
| public CompletableFuture<Void> listen(MessageHandler handler) { | |
| String url = backendUrl + "/" + topic + "/json"; | |
| HttpRequest request = HttpRequest.newBuilder() | |
| .uri(URI.create(url)) | |
| .build(); | |
| return client.sendAsync(request, HttpResponse.BodyHandlers.ofLines()) | |
| .thenAccept(response -> response.body().forEach(line -> { | |
| if (line.contains("\"message\"")) { | |
| handler.onMessage(line); | |
| } | |
| })); | |
| } | |
| public CompletableFuture<Void> listen(MessageHandler handler) { | |
| String url = backendUrl + "/" + topic + "/json"; | |
| HttpRequest request = HttpRequest.newBuilder() | |
| .uri(URI.create(url)) | |
| .build(); | |
| return client.sendAsync(request, HttpResponse.BodyHandlers.ofLines()) | |
| .thenAccept(response -> response.body().forEach(line -> { | |
| // Basic JSON parsing - consider using a proper JSON library | |
| if (line.trim().matches(".*\"message\"\\s*:\\s*\".*\".*")) { | |
| handler.onMessage(line); | |
| } | |
| })) | |
| .exceptionally(e -> { | |
| System.err.println("Failed to listen for messages: " + e.getMessage()); | |
| return null; | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/HelloModel.java around lines 35 to 47, the code
naively checks for the substring "\"message\"" and lacks error handling; replace
this with proper JSON parsing (e.g., Jackson ObjectMapper or Gson) to parse each
line into a JSON object and extract the "message" field explicitly, call
handler.onMessage only when the field exists and is of the expected type, and
wrap parsing/processing in try/catch to log parsing errors and avoid crashing
the stream; also add error handling for the sendAsync future (handle
exceptionally, timeouts, and connection failures) so failures are logged and the
returned CompletableFuture completes exceptionally or recovers appropriately.
| void testModelInitialization() { | ||
| // Försök skapa modellen, hantera fel ifall BACKEND_URL saknas | ||
| try { | ||
| HelloModel model = new HelloModel("test-topic"); | ||
| assertNotNull(model, "Model should be created successfully"); | ||
| } catch (IllegalStateException e) { | ||
| // Om miljövariabeln saknas ska felet ha rätt meddelande | ||
| assertTrue(e.getMessage().contains("BACKEND_URL"), "Exception should mention BACKEND_URL"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Test always passes and doesn't verify model behavior.
The try-catch structure makes this test pass in both success and failure scenarios. Additionally, creating a model with a missing BACKEND_URL should fail (as per line 17-19 in HelloModel.java), but the test accepts this as valid. This test provides false confidence.
Apply this diff to properly test model initialization:
@Test
void testModelInitialization() {
- // Försök skapa modellen, hantera fel ifall BACKEND_URL saknas
- try {
- HelloModel model = new HelloModel("test-topic");
- assertNotNull(model, "Model should be created successfully");
- } catch (IllegalStateException e) {
- // Om miljövariabeln saknas ska felet ha rätt meddelande
- assertTrue(e.getMessage().contains("BACKEND_URL"), "Exception should mention BACKEND_URL");
- }
+ // Test requires BACKEND_URL to be set in test environment
+ String originalUrl = System.getenv("BACKEND_URL");
+ if (originalUrl == null || originalUrl.isBlank()) {
+ // If BACKEND_URL is not set, verify exception is thrown
+ assertThrows(IllegalStateException.class,
+ () -> new HelloModel("test-topic"),
+ "Model should throw IllegalStateException when BACKEND_URL is not set");
+ } else {
+ // If BACKEND_URL is set, verify model is created
+ HelloModel model = new HelloModel("test-topic");
+ assertNotNull(model, "Model should be created successfully when BACKEND_URL is set");
+ }
}🤖 Prompt for AI Agents
In src/test/java/com/example/HelloModelTest.java around lines 9 to 18, the
current try-catch makes the test pass regardless of outcome and thus gives false
confidence; replace it with two explicit assertions: (1) set up a valid
BACKEND_URL env/config (or mock) and assert that new HelloModel("test-topic") is
created (assertNotNull) and behaves as expected, and (2) write a separate test
that uses assertThrows(IllegalStateException.class, () -> new
HelloModel("test-topic")) and assert the thrown exception's message contains
"BACKEND_URL" to verify the failure path when the env var is missing. Ensure you
remove the broad try-catch so both success and failure cases are independently
verified.
| @Test | ||
| void testBackendUrlDefault() { | ||
| // Testar att miljövariabeln hanteras | ||
| String backendUrl = System.getenv("BACKEND_URL"); | ||
| if (backendUrl == null || backendUrl.isBlank()) { | ||
| backendUrl = "https://ntfy.sh"; | ||
| } | ||
| assertTrue(backendUrl.startsWith("https://"), "BACKEND_URL should start with https://"); | ||
| } |
There was a problem hiding this comment.
Test doesn't verify HelloModel behavior.
This test checks environment variable logic but doesn't test how HelloModel handles the BACKEND_URL. It should verify the model's actual behavior, not replicate the model's internal logic in the test.
Consider removing this test or rewriting it to verify that HelloModel properly uses the backend URL (e.g., by mocking HTTP calls or testing against a test server).
🤖 Prompt for AI Agents
In src/test/java/com/example/HelloModelTest.java around lines 20-28, the test
only re-implements environment-variable logic instead of asserting HelloModel
behavior; replace this test with one that verifies HelloModel actually uses the
BACKEND_URL (or a provided backend URL) by injecting/configuring the URL into
the model and mocking the HTTP client or starting a lightweight test server,
then assert that HelloModel makes requests to the expected https:// URL (use
Mockito to mock the HTTP client and verify the request target or use a test HTTP
server and assert the received request host/path); alternatively remove the
redundant env-only test if no behavioral test is added.
| @Test | ||
| void testSendMessageFormatting() { | ||
| String userMessage = "Hej världen!"; | ||
| String formatted = "{\"message\": \"" + userMessage + "\"}"; | ||
| assertTrue(formatted.contains("Hej världen!")); | ||
| assertTrue(formatted.contains("{")); | ||
| assertTrue(formatted.contains("}")); | ||
| } |
There was a problem hiding this comment.
Test doesn't call HelloModel methods.
This test validates string concatenation logic within the test itself rather than testing HelloModel.sendMessage(). It doesn't verify that the model actually formats messages correctly.
The test should verify the actual HTTP request sent by sendMessage(). Consider using a mock HTTP server or intercepting HTTP requests to validate the message format. Alternatively, if you want to test JSON formatting, extract it to a separate method in HelloModel and test that method.
🤖 Prompt for AI Agents
In src/test/java/com/example/HelloModelTest.java around lines 30 to 37, the test
only constructs a string locally instead of invoking HelloModel.sendMessage(),
so it doesn't verify the model's behavior; change the test to either (A) call
HelloModel.sendMessage("Hej världen!") against a controllable HTTP endpoint (use
MockWebServer, WireMock, or an injected mock HTTP client), capture the outgoing
HTTP request body and assert it equals/contains the expected JSON (including
braces and the message), then shut down the mock server, or (B) refactor
HelloModel to extract the JSON formatting into a package-visible method (e.g.,
formatMessage) and write a unit test that calls that method and asserts the
formatted JSON; ensure mocks or extraction are used so the test validates code
in HelloModel rather than recreating the formatting logic in the test.
2 participants
Inlämning JavaFX Chat App - Dinbah18
Färdig version med fungerande GUI och tester (BUILD SUCCESS).
Summary by CodeRabbit
New Features
Tests