Conversation
WalkthroughThe PR transforms a JavaFX application from displaying static greetings to a chat-based messaging system. HelloController's initialization is reworked to manage user input and message listening, HelloModel is redesigned to integrate with an ntfy service for sending and receiving messages, the FXML view is restructured with chat-like controls, and unit tests are added. Changes
Sequence DiagramsequenceDiagram
participant User
participant Controller as HelloController
participant Model as HelloModel
participant ntfy as ntfy Service
participant UI as JavaFX UI
User->>Controller: Enters message & clicks Send
activate Controller
Controller->>Controller: sendMessage()
Controller->>Model: sendMessage(message)
deactivate Controller
activate Model
Model->>ntfy: POST /baseUrl/topic
ntfy-->>Model: Response
Model-->>Controller: (async)
deactivate Model
Controller->>UI: Append message to ListView
rect rgba(100, 200, 150, 0.2)
Note over Controller,ntfy: Background Listening Loop
Controller->>Controller: startListeningToMessages()
Controller->>Model: startListening(onMessage, onError)
activate Model
Model->>Model: Spawn background thread
loop Poll messages
Model->>ntfy: GET /baseUrl/topic/json
ntfy-->>Model: Message stream
Model->>Controller: onMessage callback (UI thread)
end
deactivate Model
end
Controller->>UI: Update ListView with received messages
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 2
🧹 Nitpick comments (4)
src/test/java/com/example/HelloModelTest.java (2)
9-13: Hard‑coded URL in test tightly couples to HelloModel internals
info_returnsCorrectUrl(Line 9–Line 13) asserts against a literal"https://ntfy.sh/java25-erika-chat", so any change tobaseUrlortopicinHelloModelwill require test updates. Consider deriving the expected value from a shared constant or configuration to reduce duplication.
15-19:sendMessage_doesNotThrowhas very limited value with current implementation
HelloModel.sendMessagecatchesExceptioninternally, so this test will almost never fail even if networking is misconfigured. If you want a stronger test, consider refactoringHelloModelto inject an HTTP client or base URL so you can:
- Verify that a POST is issued to the right endpoint, or
- Simulate failures and assert on error handling behavior.
src/main/java/com/example/HelloModel.java (2)
13-29:sendMessageblocks caller thread and lacks timeouts/charset handling
sendMessage(Line 13–Line 29) runs the HTTP POST synchronously on the calling thread (in this app, the JavaFX event thread), with no connect/read timeouts and using the platform default encoding viagetBytes(). This can freeze the UI on slow networks and makes behavior depend on platform encoding.Consider:
- Moving the HTTP call off the UI thread (e.g. using a background
Thread/ExecutorService), and- Adding timeouts and explicit UTF‑8 encoding, plus try‑with‑resources for the output stream.
Example of the core change inside
sendMessage:- var url = new URL(baseUrl + "/" + topic); - var conn = (HttpURLConnection) url.openConnection(); - - conn.setRequestMethod("POST"); - conn.setDoOutput(true); - - conn.getOutputStream().write(message.getBytes()); - - System.out.println("Skickat till " + url + ": " + message); - conn.disconnect(); + var url = new URL(baseUrl + "/" + topic); + var conn = (HttpURLConnection) url.openConnection(); + conn.setRequestMethod("POST"); + conn.setDoOutput(true); + conn.setConnectTimeout(5000); + conn.setReadTimeout(5000); + + try (var out = conn.getOutputStream()) { + out.write(message.getBytes(java.nio.charset.StandardCharsets.UTF_8)); + } + + System.out.println("Skickat till " + url + ": " + message); + conn.disconnect();(Plus moving this whole block off the JavaFX thread when called from the controller.)
31-59: Listener thread never closed, no timeouts, and may keep JVM aliveIn
startListening(Line 31–Line 59):
- The new
Threadis non‑daemon and blocks onreadLine()against the ntfy JSON endpoint, with no timeouts.BufferedReaderand theHttpURLConnectionare never explicitly closed.- There is no way to stop the listener from outside.
This can lead to:
- Resource leaks (open connections/streams), and
- The JVM staying alive because of the non‑daemon listener thread after the UI is closed or tests finish.
Consider:
- Marking the thread as daemon.
- Using try‑with‑resources for the
InputStream/BufferedReader, and callingconn.disconnect()in a finally block.- Optionally exposing a stop mechanism if you expect to start/stop listening during app lifetime.
Example skeleton:
- public void startListening(Consumer<String> onMessage, Consumer<Throwable> onError) { - new Thread(() -> { + public void startListening(Consumer<String> onMessage, Consumer<Throwable> onError) { + Thread t = new Thread(() -> { try { var url = new URL(baseUrl + "/" + topic + "/json"); var conn = (HttpURLConnection) url.openConnection(); conn.setRequestMethod("GET"); - - var reader = new BufferedReader(new InputStreamReader(conn.getInputStream())); - String line; - - while ((line = reader.readLine()) != null) { + try (var in = conn.getInputStream(); + var reader = new BufferedReader(new InputStreamReader(in))) { + String line; + while ((line = reader.readLine()) != null) { // parsing logic... - } - - } catch (Throwable ex) { - if (onError != null) onError.accept(ex); - } - }).start(); + } + } finally { + conn.disconnect(); + } + } catch (Throwable ex) { + if (onError != null) onError.accept(ex); + } + }, "ntfy-listener"); + t.setDaemon(true); + t.start(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/example/HelloController.java(1 hunks)src/main/java/com/example/HelloModel.java(1 hunks)src/main/resources/com/example/hello-view.fxml(1 hunks)src/test/java/com/example/HelloModelTest.java(1 hunks)
🔇 Additional comments (2)
src/main/resources/com/example/hello-view.fxml (1)
9-28: FXML IDs and layout align well with HelloControllerThe BorderPane + HBox structure is clear, and
messages,inputField, andsendBtnIDs match the controller fields. Looks good for the chat use case.src/main/java/com/example/HelloController.java (1)
51-61: Good use ofPlatform.runLaterto update UI from listener callbacks
startListeningToMessagescorrectly wrapsmessages.getItems().add(...)calls inPlatform.runLater, so UI updates happen on the JavaFX Application Thread despite the background listener.
| // När man trycker på Skicka-knappen | ||
| sendBtn.setOnAction(_ -> sendMessage()); | ||
| } |
There was a problem hiding this comment.
❓ Verification inconclusive
Lambda parameter _ is not valid in modern Java versions
On Line 30, sendBtn.setOnAction(_ -> sendMessage()); uses _ as the lambda parameter name. In Java 9+ _ is a reserved identifier and this will fail to compile. Use a conventional name instead, e.g.:
- sendBtn.setOnAction(_ -> sendMessage());
+ sendBtn.setOnAction(event -> sendMessage());Lambda parameter _ is not valid in modern Java versions
On Line 30, sendBtn.setOnAction(_ -> sendMessage()); uses _ as the lambda parameter name. In Java 9+, it is a compile-time error if a lambda parameter has the name _. This restriction was introduced in Java 9. Use a conventional name instead, e.g.:
- sendBtn.setOnAction(_ -> sendMessage());
+ sendBtn.setOnAction(event -> sendMessage());🤖 Prompt for AI Agents
In src/main/java/com/example/HelloController.java around lines 29 to 31, the
lambda uses `_` as the parameter name which is a compile-time error in Java 9+;
replace the underscore with a valid identifier (for example `e`, `event`, or
`actionEvent`) or use a no-arg lambda form if appropriate, e.g. change
`sendBtn.setOnAction(_ -> sendMessage());` to `sendBtn.setOnAction(e ->
sendMessage());` so the code compiles on modern Java versions.
| @Test | ||
| void startListening_doesNotThrow() { | ||
| HelloModel model = new HelloModel(); | ||
| assertDoesNotThrow(() -> model.startListening(msg -> {}, err -> {})); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
startListening_doesNotThrow can leak a long‑running background thread
startListening starts a new non‑daemon thread that blocks on reader.readLine() against the live ntfy endpoint. The test only asserts the call does not throw; it does not wait for or stop the listener. This can:
- Leave a background thread running after the test finishes, and
- Make tests depend on external network availability.
Consider either:
- Refactoring
HelloModelso the listener thread can be stopped (and doing that in the test), or - Abstracting the network layer so the test can run against a stub instead of the real ntfy service.
I'll verify the review comment by examining the codebase to understand the actual implementation of HelloModel and the test setup.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find HelloModel class
fd -type f -name "HelloModel.java"
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for startListening method definition and usage
rg -n "startListening" --type=java -C 5Length of output: 3648
🏁 Script executed:
#!/bin/bash
# Check for test lifecycle methods in HelloModelTest.java
rg -n "@Before|@After|@BeforeEach|@AfterEach" src/test/java/com/example/HelloModelTest.java -C 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for thread creation and daemon status
rg -n "new Thread|setDaemon|Thread\(" --type=java -C 4Length of output: 786
🏁 Script executed:
#!/bin/bash
# Check HelloModelTest.java structure for any listeners or cleanup
cat src/test/java/com/example/HelloModelTest.javaLength of output: 730
I need to see the complete startListening() implementation to confirm the thread behavior:
---
🏁 Script executed:
```shell
#!/bin/bash
# View full HelloModel.java file
cat src/main/java/com/example/HelloModel.java
Length of output: 2241
Fix the thread leak in startListening_doesNotThrow by implementing listener cleanup
The issue is confirmed: HelloModel.startListening() creates a non-daemon thread that blocks indefinitely on reader.readLine() against the live ntfy endpoint (line 42 in src/main/java/com/example/HelloModel.java). The test starts this listener but never stops it, leaving a background thread running after the test completes.
Recommended fixes:
- Refactor
HelloModel.startListening()to return aCloseableorAutoCloseablereference so tests can stop the listener, then use try-with-resources in the test. - Mock or stub the HTTP layer in tests to avoid external network dependencies and eliminate the blocking I/O.
1 participant
Lagt till Unit tests för metoderna info(), sendMessage() och startListening() i HelloModel.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.