Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
@@ -1,2 +1,7 @@
target/
/.idea/
.idea/
.vscode/
*.class
*.log
.env

56 changes: 33 additions & 23 deletions pom.xml
Original file line number Diff line number Diff line change
@@ -1,22 +1,37 @@
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0
http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

<groupId>com.example</groupId>
<artifactId>javafx</artifactId>
<version>1.0-SNAPSHOT</version>

<properties>
<maven.compiler.release>25</maven.compiler.release>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<maven.compiler.release>25</maven.compiler.release>
<javafx.version>25</javafx.version>
<javafx.platform>win</javafx.platform>
Comment on lines +14 to +16
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

<junit.jupiter.version>6.0.0</junit.jupiter.version>
<assertj.core.version>3.27.6</assertj.core.version>
<mockito.version>5.20.0</mockito.version>
<javafx.version>25</javafx.version>
</properties>

<dependencies>
<dependency>
<groupId>org.openjfx</groupId>
<artifactId>javafx-controls</artifactId>
<version>${javafx.version}</version>
<classifier>${javafx.platform}</classifier>
</dependency>
<dependency>
<groupId>org.openjfx</groupId>
<artifactId>javafx-fxml</artifactId>
<version>${javafx.version}</version>
<classifier>${javafx.platform}</classifier>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter</artifactId>
Expand All @@ -35,34 +50,29 @@
<version>${mockito.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.openjfx</groupId>
<artifactId>javafx-controls</artifactId>
<version>${javafx.version}</version>
</dependency>
<dependency>
<groupId>org.openjfx</groupId>
<artifactId>javafx-fxml</artifactId>
<version>${javafx.version}</version>
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
<groupId>org.openjfx</groupId>
<groupId>org.openjfx</groupId>
<artifactId>javafx-maven-plugin</artifactId>
<version>0.0.8</version>
<version>0.0.8</version>
<configuration>
<mainClass>com.example.chat.App</mainClass>
</configuration>
</plugin>


<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
<version>3.13.0</version>
<configuration>
<mainClass>com.example.HelloFX</mainClass>
<options>
<option>--enable-native-access=javafx.graphics</option>
</options>
<launcher>javafx</launcher>
<stripDebug>true</stripDebug>
<noHeaderFiles>true</noHeaderFiles>
<noManPages>true</noManPages>
<release>${maven.compiler.release}</release>
</configuration>
</plugin>
</plugins>
</build>
</project>

24 changes: 24 additions & 0 deletions src/main/java/com/example/chat/App.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package com.example.chat;

import javafx.application.Application;
import javafx.fxml.FXMLLoader;
import javafx.scene.Scene;
import javafx.stage.Stage;

public class App extends Application {

@Override
public void start(Stage stage) throws Exception {
FXMLLoader loader = new FXMLLoader(getClass().getResource("/com/example/chat/ChatView.fxml"));
Scene scene = new Scene(loader.load());
stage.setTitle("JavaFX Chat Client");
stage.setScene(scene);
stage.show();
}

public static void main(String[] args) {
launch();
}
}


43 changes: 43 additions & 0 deletions src/main/java/com/example/chat/ChatController.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
package com.example.chat;

import javafx.fxml.FXML;
import javafx.scene.control.Button;
import javafx.scene.control.ListView;
import javafx.scene.control.TextField;

public class ChatController {

@FXML
private TextField inputField;

@FXML
private Button sendButton;

@FXML
private ListView<String> messageList;

private ChatModel model;

public void initialize() {

model = new ChatModel();


messageList.setItems(model.getMessages());

//Sätt upp knapp och enter-tangent
sendButton.setOnAction(e -> sendMessage());
inputField.setOnAction(e -> sendMessage());
}

private void sendMessage() {
String text = inputField.getText();
if (text != null && !text.isBlank()) {
model.addMessage(text);
inputField.clear();
}
}
}



86 changes: 86 additions & 0 deletions src/main/java/com/example/chat/ChatModel.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package com.example.chat;

import javafx.application.Platform;
import javafx.collections.FXCollections;
import javafx.collections.ObservableList;

import java.net.URI;
import java.net.http.HttpClient;
import java.net.http.HttpRequest;
import java.net.http.HttpResponse;
import java.nio.charset.StandardCharsets;

public class ChatModel {

private final ObservableList<String> messages = FXCollections.observableArrayList();
private final HttpClient client = HttpClient.newHttpClient();
private final String ntfyUrl;

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();
}
Comment on lines +19 to +84
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Thread lifecycle and constructor side effects in ChatModel

Right now:

  • The constructor immediately calls startListening(), which spawns a background thread with an infinite while (true) loop; and
  • Both sendToBackend and startListening use plain new 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 JavaFX Platform.runLater calls, 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 construct ChatModel without 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 -C3

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Check for shutdown hooks or Application lifecycle methods
rg -n 'setOnCloseRequest|shutdown|stop\(\)' --type=java -C2

Length 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 -C3

Length of output: 1718


🏁 Script executed:

#!/bin/bash
# Broader search: any file that references ChatModel
rg -n 'ChatModel' --type=java -B2 -A2

Length of output: 2880


🏁 Script executed:

#!/bin/bash
# Check if any shutdown/interrupt/close logic exists
rg -n 'Thread\.|interrupt|daemon' --type=java -C2

Length 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:

  1. Keep the JVM alive after the JavaFX window closes (non-daemon threads prevent normal shutdown)
  2. Cause test failuresChatModelTest (at src/test/resources/java/com/example/chat/model/ChatModelTest.java, line 16) instantiates ChatModel in setUp(), immediately spawning the non-daemon background thread with Platform.runLater() calls, which will block or fail in a headless test environment
  3. Make ChatModel untestable without modifications due to immediate constructor side effects

Required fixes:

  1. Mark both threads as daemon threads (as shown in the provided diff)
  2. 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.

}

8 changes: 5 additions & 3 deletions src/main/java/module-info.java
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
module hellofx {
requires javafx.controls;
requires javafx.fxml;
requires java.net.http; // tillagd

opens com.example.chat to javafx.fxml;
exports com.example.chat;
}

opens com.example to javafx.fxml;
exports com.example;
}
21 changes: 21 additions & 0 deletions src/main/resources/com/example/chat/ChatView.fxml
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?xml version="1.0" encoding="UTF-8"?>

<?import javafx.scene.control.*?>
<?import javafx.scene.layout.*?>
<?import javafx.geometry.Insets?>

<VBox xmlns="http://javafx.com/javafx/25" xmlns:fx="http://javafx.com/fxml/1"
fx:controller="com.example.chat.ChatController"
spacing="10">

<padding>
<Insets top="10" right="10" bottom="10" left="10"/>
</padding>

<ListView fx:id="messageList" prefHeight="400"/>

<HBox spacing="10">
<TextField fx:id="inputField" HBox.hgrow="ALWAYS" promptText="Type your message..."/>
<Button fx:id="sendButton" text="Send"/>
</HBox>
</VBox>
37 changes: 37 additions & 0 deletions src/test/resources/java/com/example/chat/model/ChatModelTest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
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());
}
}
Comment on lines +1 to +36
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

Test class is tightly coupled to ChatModel’s networking/JavaFX behavior and may be mislocated

  • setUp() creates a real ChatModel, which immediately starts the startListening() thread and uses HttpClient + 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 than src/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 ChatModel so 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 -20

Length 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"
fi

Length 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"
fi

Length 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/null

Length 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:

  1. 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 from src/test/java, so this test is never discovered or run by the build system.

  2. 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 uses Platform.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.


Loading