Conversation
…Also had to fix big issue with running JavaFX on old JDK version.
…aFX on old JDK version
WalkthroughProject introduces JavaFX chat client with backend integration. Changes include updating pom.xml with platform-specific JavaFX dependencies and build plugins, creating application entry point, UI controller and model classes for message handling, configuring module exports, adding FXML layout, and initial unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as ChatView (FXML)
participant Ctrl as ChatController
participant Model as ChatModel
participant Backend as Backend Server
User->>UI: Click send / Press enter
UI->>Ctrl: Event trigger
Ctrl->>Ctrl: Validate input
Ctrl->>Model: addMessage(text)
rect rgb(200, 220, 255)
note right of Model: Send Message Path
Model->>Model: Prefix "Me: "
Model->>UI: Update messages list
Model->>Backend: HTTP POST message
Backend-->>Model: Response (2xx or error)
end
rect rgb(220, 200, 255)
note right of Model: Receive Message Path (Background)
Backend-->>Model: Poll /events endpoint
Model->>Model: Parse "Other: " + content
Model->>UI: Update messages list
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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: 3
🧹 Nitpick comments (4)
src/main/java/com/example/chat/ChatController.java (1)
19-39: Consider injectingChatModelinstead of constructing it directlyCreating the model with
model = new ChatModel();ininitialize()tightly couples the controller to a concrete implementation and makes it harder to test the controller or share a model between views.A lighter‑weight design is to:
- Add a
setModel(ChatModel model)method on the controller, and- After loading the FXML in
App, retrieve the controller and inject aChatModel(or a mock in tests).This keeps the current behavior but opens the door for cleaner MVC separation and testing.
Please check how
Apploads the FXML and confirm whether injecting the model there would fit your intended architecture.src/main/resources/com/example/chat/ChatView.fxml (1)
7-20: Bindings look correct; optional layout tweak for better resizingController class and
fx:ids align withChatController, so wiring is fine. If you want the message list to grow with the window instead of relying on a fixedprefHeight, you can addVBox.vgrow="ALWAYS"to theListView.Please verify in the running app whether the current resizing behavior matches your UX expectations.
src/main/java/com/example/chat/App.java (1)
11-21: Optionally guard against missing FXML resource
getClass().getResource("/com/example/chat/ChatView.fxml")will returnnullif the resource is moved or not on the classpath, leading to a somewhat opaque failure duringloader.load().Not mandatory, but you could defensively check for
nulland throw a clearer exception (or log a helpful message) to make debugging easier if the FXML path ever changes.After packaging, please verify that the FXML is actually present in the built artifact under
/com/example/chat/ChatView.fxml.pom.xml (1)
58-64: Verify javafx-maven-plugin version; fix XML indentation.Two issues here:
- Outdated plugin version: Version 0.0.8 is significantly outdated (from ~2019). Verify this is intentional and compatible with Java 25 and JavaFX 25. Modern versions are typically 0.0.15+.
- XML indentation: The tags on lines 58-62 have inconsistent indentation, making the POM harder to maintain.
Fix the indentation to align with the rest of the POM:
- <plugin> - <groupId>org.openjfx</groupId> - <artifactId>javafx-maven-plugin</artifactId> - <version>0.0.8</version> - <configuration> - <mainClass>com.example.chat.App</mainClass> - </configuration> - </plugin> + <plugin> + <groupId>org.openjfx</groupId> + <artifactId>javafx-maven-plugin</artifactId> + <version>0.0.8</version> + <configuration> + <mainClass>com.example.chat.App</mainClass> + </configuration> + </plugin>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignore(1 hunks)pom.xml(2 hunks)src/main/java/com/example/chat/App.java(1 hunks)src/main/java/com/example/chat/ChatController.java(1 hunks)src/main/java/com/example/chat/ChatModel.java(1 hunks)src/main/java/module-info.java(1 hunks)src/main/resources/com/example/chat/ChatView.fxml(1 hunks)src/test/resources/java/com/example/chat/model/ChatModelTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/resources/java/com/example/chat/model/ChatModelTest.java (1)
src/main/java/com/example/chat/ChatModel.java (1)
ChatModel(13-85)
🔇 Additional comments (4)
.gitignore (1)
1-7: Sensible .gitignore additions for JavaFX/Maven project.The updates appropriately ignore build artifacts (
target/,*.class), IDE configurations (.idea/,.vscode/), logs (*.log), and environment files (.env). Note that changing from/.idea/to.idea/broadens the scope from root-only to any directory level—this is a safe, common practice for IDE folders.src/main/java/module-info.java (1)
4-7: Module directives look consistent with the new chat package
requires java.net.httpand theopens/exports com.example.chatdirectives align with the newChatModel,ChatController, andAppclasses, so module wiring looks correct.Please run your normal build on the module path to confirm there are no module‑system surprises (e.g. missing
requiresfor other JavaFX modules).pom.xml (2)
23-34: JavaFX dependencies structurally sound but depend on platform resolution.The use of classifiers for platform-specific JavaFX modules follows best practices and allows for correct native library loading. However, resolve the hardcoded
javafx.platformissue first (see previous comment).Once that's fixed, this dependency structure will work correctly across platforms.
68-72: maven-compiler-plugin looks good.The configuration correctly uses the modern
<release>element and references the Java 25 property. Version 3.13.0 is current and should support Java 25 without issues.Verify that Java 25 is an intentional choice for this project. While it's the latest version, classroom projects sometimes target more stable LTS versions (currently Java 21). If Java 25 is required, no changes needed; otherwise, consider targeting Java 21 LTS for better stability and broader tool support.
| <maven.compiler.release>25</maven.compiler.release> | ||
| <javafx.version>25</javafx.version> | ||
| <javafx.platform>win</javafx.platform> |
There was a problem hiding this comment.
Hardcoded Windows platform breaks cross-platform builds.
The javafx.platform property is hardcoded to win, which means developers using Linux or macOS systems will be unable to build or run the project. This is a critical blocker for a classroom project where students likely use different operating systems.
Consider using platform detection via Maven profiles or a build plugin to automatically determine the platform. Here's a common approach using profiles:
<properties>
<javafx.platform>win</javafx.platform>
</properties>
<profiles>
<profile>
<id>mac</id>
<activation>
<os>
<family>mac</family>
</os>
</activation>
<properties>
<javafx.platform>mac</javafx.platform>
</properties>
</profile>
<profile>
<id>linux</id>
<activation>
<os>
<family>unix</family>
<name>Linux</name>
</os>
</activation>
<properties>
<javafx.platform>linux</javafx.platform>
</properties>
</profile>
</profiles>Alternatively, use the os-maven-plugin to automatically detect the platform at build time.
🤖 Prompt for AI Agents
In pom.xml around lines 14-16, the javafx.platform property is hardcoded to
"win" which breaks builds on macOS/Linux; remove the hardcoded value and replace
it with a platform-detection approach — either add Maven profiles (mac, linux,
win) with OS activation to set javafx.platform appropriately, or integrate an
os-detection plugin (e.g., os-maven-plugin) to populate javafx.platform at build
time; ensure a sensible default or profile is defined so CI and other OSes
resolve the property instead of failing.
| public ChatModel() { | ||
| String url = System.getenv("NTFY_URL"); | ||
| if (url == null || url.isBlank()) { | ||
| url = "http://localhost:8080/topic/test"; // fallback | ||
| } | ||
| ntfyUrl = url; | ||
| startListening(); | ||
| } | ||
|
|
||
| public ObservableList<String> getMessages() { | ||
| return messages; | ||
| } | ||
|
|
||
| public void addMessage(String message) { | ||
| if (message != null && !message.isBlank()) { | ||
| messages.add("Me: " + message); | ||
| sendToBackend(message); | ||
| } | ||
| } | ||
|
|
||
| private void sendToBackend(String message) { | ||
| new Thread(() -> { | ||
| try { | ||
| HttpRequest request = HttpRequest.newBuilder() | ||
| .uri(URI.create(ntfyUrl)) | ||
| .POST(HttpRequest.BodyPublishers.ofString(message, StandardCharsets.UTF_8)) | ||
| .header("Content-Type", "text/plain") | ||
| .build(); | ||
|
|
||
| HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString()); | ||
| if (response.statusCode() >= 400) { | ||
| Platform.runLater(() -> messages.add("Error sending message: " + response.statusCode())); | ||
| } | ||
| } catch (Exception e) { | ||
| Platform.runLater(() -> messages.add("Error sending message: " + e.getMessage())); | ||
| } | ||
| }).start(); | ||
| } | ||
|
|
||
| private void startListening() { | ||
| new Thread(() -> { | ||
| try { | ||
| while (true) { | ||
| HttpRequest request = HttpRequest.newBuilder() | ||
| .uri(URI.create(ntfyUrl + "/events")) | ||
| .build(); | ||
|
|
||
| client.send(request, HttpResponse.BodyHandlers.ofLines()) | ||
| .body() | ||
| .forEach(line -> { | ||
| if (line != null && !line.isBlank()) { | ||
| // Ta bort "data:" om det finns | ||
| String content = line.contains("data:") ? line.replaceFirst("data:", "").trim() : line; | ||
| if (!content.isEmpty()) { | ||
| Platform.runLater(() -> messages.add("Other: " + content)); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| Thread.sleep(1000); // poll varje sekund | ||
| } | ||
| } catch (Exception e) { | ||
| Platform.runLater(() -> messages.add("Error receiving messages: " + e.getMessage())); | ||
| } | ||
| }).start(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Thread lifecycle and constructor side effects in ChatModel
Right now:
- The constructor immediately calls
startListening(), which spawns a background thread with an infinitewhile (true)loop; and - Both
sendToBackendandstartListeninguse plainnew Thread(...)without marking them daemon or providing any way to stop them.
Implications:
- These non‑daemon threads can keep the JVM alive even after the JavaFX window is closed.
- Instantiating
ChatModel(e.g. in tests) always kicks off networking and JavaFXPlatform.runLatercalls, making the class hard to use in headless or unit‑test contexts.
At minimum, I’d recommend:
- Marking these threads as daemon, and
- Considering an explicit lifecycle (e.g.
start()/stop()or a flag) so tests or future callers can constructChatModelwithout immediately starting long‑lived network loops.
For example, to mark threads as daemon:
private void sendToBackend(String message) {
- new Thread(() -> {
+ Thread thread = new Thread(() -> {
try {
HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(ntfyUrl))
.POST(HttpRequest.BodyPublishers.ofString(message, StandardCharsets.UTF_8))
.header("Content-Type", "text/plain")
.build();
HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString());
if (response.statusCode() >= 400) {
Platform.runLater(() -> messages.add("Error sending message: " + response.statusCode()));
}
} catch (Exception e) {
Platform.runLater(() -> messages.add("Error sending message: " + e.getMessage()));
}
- }).start();
+ });
+ thread.setDaemon(true);
+ thread.start();
}
private void startListening() {
- new Thread(() -> {
+ Thread thread = new Thread(() -> {
try {
while (true) {
HttpRequest request = HttpRequest.newBuilder()
.uri(URI.create(ntfyUrl + "/events"))
.build();
client.send(request, HttpResponse.BodyHandlers.ofLines())
.body()
.forEach(line -> {
if (line != null && !line.isBlank()) {
// Ta bort "data:" om det finns
String content = line.contains("data:") ? line.replaceFirst("data:", "").trim() : line;
if (!content.isEmpty()) {
Platform.runLater(() -> messages.add("Other: " + content));
}
}
});
Thread.sleep(1000); // poll varje sekund
}
} catch (Exception e) {
Platform.runLater(() -> messages.add("Error receiving messages: " + e.getMessage()));
}
- }).start();
+ });
+ thread.setDaemon(true);
+ thread.start();
}You can then later extend this with a proper shutdown/interrupt strategy if needed.
Please verify the app actually terminates cleanly after closing the window, and confirm tests don’t hang or produce background thread errors when ChatModel is instantiated.
---
🏁 Script executed:
```shell
#!/bin/bash
# Inspect full ChatModel.java structure
cat src/main/java/com/example/chat/ChatModel.java
Length of output: 3157
🏁 Script executed:
#!/bin/bash
# Search for test files referencing ChatModel
fd -e java -path '*/test/*' -exec grep -l 'ChatModel' {} \;Length of output: 168
🏁 Script executed:
#!/bin/bash
# Search for ChatModel instantiation throughout codebase
rg -n 'new ChatModel\|ChatModel\s*\(\s*\)' --type=java -C3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for shutdown hooks or Application lifecycle methods
rg -n 'setOnCloseRequest|shutdown|stop\(\)' --type=java -C2Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for test files (corrected fd command)
fd -type f -name "*.java" | grep -i test
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for Application.java, main, or JavaFX app entry points
rg -n 'class.*extends Application|public static void main' --type=java -C3Length of output: 1718
🏁 Script executed:
#!/bin/bash
# Broader search: any file that references ChatModel
rg -n 'ChatModel' --type=java -B2 -A2Length of output: 2880
🏁 Script executed:
#!/bin/bash
# Check if any shutdown/interrupt/close logic exists
rg -n 'Thread\.|interrupt|daemon' --type=java -C2Length of output: 392
Non-daemon threads and constructor side effects prevent clean shutdown and break tests
The concerns are confirmed. ChatModel instantiation spawns non-daemon threads with an infinite loop that will:
- Keep the JVM alive after the JavaFX window closes (non-daemon threads prevent normal shutdown)
- Cause test failures —
ChatModelTest(atsrc/test/resources/java/com/example/chat/model/ChatModelTest.java, line 16) instantiatesChatModelinsetUp(), immediately spawning the non-daemon background thread withPlatform.runLater()calls, which will block or fail in a headless test environment - Make
ChatModeluntestable without modifications due to immediate constructor side effects
Required fixes:
- Mark both threads as daemon threads (as shown in the provided diff)
- Consider adding explicit
start()/stop()lifecycle methods so tests can instantiate without triggering networking
Apply the suggested diff to both sendToBackend() and startListening() to call thread.setDaemon(true) before thread.start().
🤖 Prompt for AI Agents
In src/main/java/com/example/chat/ChatModel.java around lines 19 to 84, the
constructor starts background threads that are non-daemon and perform networking
immediately, preventing JVM shutdown and breaking tests; change both threads
created in sendToBackend() and startListening() to be daemon threads by calling
thread.setDaemon(true) before thread.start(), and refactor the constructor to
avoid side effects by providing explicit start() and stop() lifecycle methods
(start() creates/starts the daemon threads and stop() interrupts/joins them and
closes resources) so tests can instantiate ChatModel without launching
networking.
| package com.example.chat.model; | ||
|
|
||
| import com.example.chat.ChatModel; | ||
| import javafx.collections.ObservableList; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| class ChatModelTest { | ||
|
|
||
| private ChatModel model; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| model = new ChatModel(); | ||
| } | ||
|
|
||
| @Test | ||
| void testAddMessageAddsToList() { | ||
| ObservableList<String> messages = model.getMessages(); | ||
| assertEquals(0, messages.size()); | ||
|
|
||
| model.addMessage("Hello"); | ||
| assertEquals(1, messages.size()); | ||
| assertEquals("Me: Hello", messages.get(0)); | ||
| } | ||
|
|
||
| @Test | ||
| void testAddMessageIgnoresEmpty() { | ||
| ObservableList<String> messages = model.getMessages(); | ||
| model.addMessage(""); | ||
| model.addMessage(" "); | ||
| assertEquals(0, messages.size()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Test class is tightly coupled to ChatModel’s networking/JavaFX behavior and may be mislocated
setUp()creates a realChatModel, which immediately starts thestartListening()thread and usesHttpClient+Platform.runLater. In a plain JUnit environment (no JavaFX toolkit, no backend service), this can cause background errors or flaky behavior that’s unrelated to the assertions.- The file path in the PR (
src/test/resources/java/...) suggests it might be under resources rather thansrc/test/java, in which case Maven/Gradle won’t compile or execute the test at all.
Consider:
- Moving the test to
src/test/java/com/example/chat/model/ChatModelTest.java, and - Introducing a test‑friendly constructor or a way to disable networking/listening in
ChatModelso unit tests can exercise message list behavior without spinning up HTTP and JavaFX runtime dependencies.
Please verify the actual on‑disk path of this file and run the tests once ChatModel’s background behavior is accounted for.
🏁 Script executed:
#!/bin/bash
# Locate ChatModelTest.java and ChatModel.java in the repository
echo "=== Finding ChatModelTest.java ==="
fd -n "ChatModelTest\.java"
echo ""
echo "=== Finding ChatModel.java ==="
fd -n "ChatModel\.java"
echo ""
echo "=== Checking for build files ==="
fd -n "(pom\.xml|build\.gradle)"Length of output: 793
🏁 Script executed:
#!/bin/bash
# Check directory structure for test locations
echo "=== Directory structure for test sources ==="
find . -type d -name "test" 2>/dev/null | head -20
echo ""
echo "=== Directory structure for resources ==="
find . -type d -name "resources" 2>/dev/null | head -20Length of output: 140
🏁 Script executed:
#!/bin/bash
# If ChatModel.java exists, inspect its content for networking and JavaFX behavior
CHATMODEL=$(fd -n "ChatModel\.java" | grep -v Test | head -1)
if [ -n "$CHATMODEL" ]; then
echo "=== ChatModel.java implementation ==="
echo "File: $CHATMODEL"
echo ""
cat "$CHATMODEL"
else
echo "ChatModel.java not found"
fiLength of output: 255
🏁 Script executed:
#!/bin/bash
# Locate ChatModelTest.java and ChatModel.java with correct fd syntax
echo "=== Finding ChatModelTest.java ==="
fd "ChatModelTest\.java"
echo ""
echo "=== Finding ChatModel.java ==="
fd "ChatModel\.java"
echo ""
echo "=== Checking for build files ==="
fd "(pom\.xml|build\.gradle)"Length of output: 220
🏁 Script executed:
#!/bin/bash
# Inspect ChatModel.java implementation
echo "=== ChatModel.java implementation ==="
CHATMODEL=$(find . -name "ChatModel.java" -not -name "*Test*" | head -1)
if [ -n "$CHATMODEL" ]; then
echo "File: $CHATMODEL"
echo ""
cat "$CHATMODEL"
else
echo "ChatModel.java not found"
fiLength of output: 3250
🏁 Script executed:
#!/bin/bash
# Check the exact path of ChatModelTest.java in the test directory
echo "=== Listing files in src/test ==="
find src/test -type f -name "*.java" 2>/dev/null
echo ""
echo "=== Full path of ChatModelTest.java ==="
find . -path "*/src/test*" -name "ChatModelTest.java" 2>/dev/nullLength of output: 208
Move test to src/test/java and refactor ChatModel to support unit testing without network dependencies
The review comment is accurate. Two critical issues require fixing:
-
File location prevents test discovery: ChatModelTest.java is at
src/test/resources/java/com/example/chat/model/ChatModelTest.java. Maven/Gradle only compile and execute tests fromsrc/test/java, so this test is never discovered or run by the build system. -
ChatModel constructor creates unavoidable side effects: The implementation immediately calls
startListening()in the constructor, spawning a daemon thread that runs an infinite loop with HTTP polling to a backend service. It also usesPlatform.runLater()which requires JavaFX runtime initialization. Unit tests cannot isolate message-list behavior from networking and JavaFX dependencies.
Fixes required:
- Move file to
src/test/java/com/example/chat/model/ChatModelTest.java - Add a test-friendly constructor to ChatModel (e.g., with a flag to skip
startListening()) or refactor to inject a message listener, allowing tests to verify message-list logic without spinning up HTTP and JavaFX threads.
🤖 Prompt for AI Agents
In src/test/resources/java/com/example/chat/model/ChatModelTest.java lines 1-36:
the test is in the resources tree (so Maven/Gradle won't discover it) and
ChatModel's constructor creates network/JavaFX side-effects that prevent unit
testing; move this test file to
src/test/java/com/example/chat/model/ChatModelTest.java and refactor ChatModel
to support testability by adding a test-friendly constructor or dependency
injection that skips startListening() (or accepts a message-source/listener
interface) so the test can construct a ChatModel without spawning HTTP threads
or calling Platform.runLater(), then use that no-network instance in the moved
test to assert message list behavior.
2 participants
Summary by CodeRabbit
New Features
Tests