Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 8 additions & 1 deletion src/main/java/com/example/HelloModel.java
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,14 @@ public void sendMessage() {
}

public void receiveMessage() {
connection.receive(m->Platform.runLater(()->messages.add(m)));
connection.receive(m-> {
try {
Platform.runLater(()->messages.add(m));
} catch (IllegalStateException e) {
messages.add(m);
}

});
Comment on lines +55 to +62
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fallback bypasses JavaFX thread safety and can cause concurrency issues.

The catch block that directly adds messages to the ObservableList violates JavaFX threading requirements. ObservableList modifications must occur on the JavaFX Application Thread, but this fallback executes on whatever thread invokes the handler (likely a background HTTP thread from NtfyConnectionImpl.receive).

If Platform.runLater throws IllegalStateException (JavaFX toolkit not initialized), the correct approach is to either:

  1. Ensure JavaFX is initialized before creating HelloModel, or
  2. Queue the message for later processing once JavaFX is ready

The current fallback can cause ConcurrentModificationException or corrupt the observable list if UI components are bound to it.

Consider removing the catch block entirely and ensuring JavaFX is properly initialized:

-        connection.receive(m-> {
-            try {
-                Platform.runLater(()->messages.add(m));
-            } catch (IllegalStateException e) {
-                messages.add(m);
-            }
-
-        });
+        connection.receive(m -> Platform.runLater(() -> messages.add(m)));
🤖 Prompt for AI Agents
In src/main/java/com/example/HelloModel.java around lines 55 to 62, the current
catch block directly adds to the ObservableList off the JavaFX thread which
breaks thread-safety; remove the fallback that calls messages.add(m) inside the
catch and instead either (A) ensure the JavaFX toolkit is initialized before
constructing HelloModel so Platform.runLater never throws, or (B) enqueue
incoming messages into a thread-safe queue when Platform.runLater fails and
drain that queue on the JavaFX Application Thread once initialized (e.g.,
schedule a Platform.runLater to flush queued messages). Ensure all
ObservableList modifications happen only via Platform.runLater on the FX thread.

}

public void sendFile(File file) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/com/example/NtfyConnectionImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public boolean sendFile(File file){
.PUT(HttpRequest.BodyPublishers.ofFile(file.toPath()))
.build();

http.sendAsync(request, HttpResponse.BodyHandlers.discarding());
http.send(request, HttpResponse.BodyHandlers.discarding());
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

Blocking HTTP call can freeze the JavaFX UI thread.

Changing from sendAsync to send makes this a synchronous blocking operation. If sendFile is invoked from the JavaFX Application Thread (e.g., triggered by a UI button click), the UI will freeze until the HTTP request completes. File uploads can take seconds or longer, resulting in poor user experience.

The TODO comment at lines 36-38 acknowledges the same issue for the send() method. Consider applying the same fix here—either use sendAsync or execute on a background thread.

Revert to the asynchronous approach:

-            http.send(request, HttpResponse.BodyHandlers.discarding());
+            http.sendAsync(request, HttpResponse.BodyHandlers.discarding());

Committable suggestion skipped: line range outside the PR's diff.

return true;

} catch (Exception e) {
Expand Down
90 changes: 88 additions & 2 deletions src/test/java/com/example/HelloModelTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import java.io.File;
import java.io.IOException;

import static com.github.tomakehurst.wiremock.client.WireMock.*;
import static org.assertj.core.api.Assertions.assertThat;

Expand Down Expand Up @@ -33,13 +36,96 @@ void sendMessageToFakeServer(WireMockRuntimeInfo wmRuntimeInfo) {
var con = new NtfyConnectionImpl("http://localhost:" + wmRuntimeInfo.getHttpPort());
var model = new HelloModel(con);
model.setMessageToSend("Hello World");
stubFor(post("/mytopic").willReturn(ok()));
stubFor(post("/adam").willReturn(ok()));

model.sendMessage();

// Verify call made to server
verify(postRequestedFor(urlEqualTo("/mytopic"))
verify(postRequestedFor(urlEqualTo("/adam"))
.withRequestBody(matching("Hello World")));
}

@Test
@DisplayName("Given file when calling sendFile then connection.sendFile should be called")
void sendFileCallConnectionSendFile() {
var spy = new NtfyConnectionSpy();
var model = new HelloModel(spy);

File file = new File("dummy.txt");
model.sendFile(file);

assertThat(spy.sentFile).isEqualTo(file);
}

@Test
@DisplayName("sendFile should upload file to the server using PUT")
void sendFileToFakeServer(WireMockRuntimeInfo wmRuntimeInfo) throws Exception {
var con = new NtfyConnectionImpl("http://localhost:" + wmRuntimeInfo.getHttpPort());
var model = new HelloModel(con);

File temp = File.createTempFile("upload_test", ".txt");

stubFor(put("/adam").willReturn(ok()));

model.sendFile(temp);

verify(putRequestedFor(urlEqualTo("/adam")));

}

@Test
@DisplayName("Model should receive messages when connection invokes handler")
void receiveMessageShouldAddToModelViewHander(){
var spy = new NtfyConnectionSpy();
var model = new HelloModel(spy);

NtfyMessageDto incoming = new NtfyMessageDto(
"123",
1000,
0,
"message",
"adam",
"this is a test hehehe",
null
);

spy.simulateIncoming(incoming);

assertThat(model.getMessages()).containsExactly(incoming);
}
Comment on lines +76 to +95
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 may not properly verify JavaFX threading behavior.

The test calls simulateIncoming which triggers Platform.runLater in the model, but the test immediately asserts without waiting for the JavaFX thread to execute. This test likely only passes because of the IllegalStateException catch block in HelloModel.receiveMessage that bypasses JavaFX threading (which itself is problematic).

In a proper JavaFX test environment, you should either:

  1. Use WaitForAsyncUtils.waitForFxEvents() from TestFX library to ensure JavaFX thread processes the event, or
  2. Initialize the JavaFX toolkit properly for testing

Consider verifying whether JavaFX is properly initialized in the test environment:

Additionally, search for TestFX or similar JavaFX testing utilities:


🏁 Script executed:

#!/bin/bash
# Check if JavaFX test dependencies or initialization are present in the test setup
rg -n "Platform\.startup|FxToolkit|ApplicationTest|@BeforeAll.*Platform" --type=java

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Look for JavaFX testing frameworks in dependencies
rg -n "testfx|WaitForAsyncUtils" 

Length of output: 0


I need to gather more information about the actual implementation to verify the review concern. Let me search for the relevant source code:


🏁 Script executed:

#!/bin/bash
# Find HelloModel class and check receiveMessage implementation
rg -n "class HelloModel" --type=java -A 20

Length of output: 3266


🏁 Script executed:

#!/bin/bash
# Find the receiveMessage method in HelloModel
rg -n "receiveMessage" --type=java -B 2 -A 10

Length of output: 2816


🏁 Script executed:

#!/bin/bash
# Find NtfyConnectionSpy and simulateIncoming implementation
rg -n "class NtfyConnectionSpy|simulateIncoming" --type=java -B 2 -A 10

Length of output: 2522


🏁 Script executed:

#!/bin/bash
# Check for pom.xml to see dependencies
fd -n "pom.xml|build.gradle" -x cat {}

Length of output: 230


🏁 Script executed:

#!/bin/bash
# Look for test setup or configuration methods in the test file
rg -n "@BeforeEach|@Before|@BeforeAll|static.*{|setUp" src/test/java/com/example/HelloModelTest.java -B 1 -A 5

Length of output: 168


🏁 Script executed:

#!/bin/bash
# Check imports and class declaration at top of HelloModelTest
head -20 src/test/java/com/example/HelloModelTest.java

Length of output: 688


🏁 Script executed:

#!/bin/bash
# Check HelloModel imports for JavaFX
head -15 src/main/java/com/example/HelloModel.java

Length of output: 360


🏁 Script executed:

#!/bin/bash
# Search for any Platform or javafx usage in the codebase
rg -n "import.*Platform|import.*javafx" --type=java

Length of output: 1882


🏁 Script executed:

#!/bin/bash
# Fix the fd command to list pom.xml and build.gradle files
fd "pom.xml|build.gradle"

Length of output: 7


🏁 Script executed:

#!/bin/bash
# Check pom.xml for dependencies
cat pom.xml

Length of output: 3194


Test relies on IllegalStateException fallback instead of verifying actual JavaFX threading.

The test passes because:

  1. JavaFX toolkit is never initialized (no imports, no @beforeeach setup, no TestFX)
  2. Platform.runLater() in HelloModel.receiveMessage() throws IllegalStateException
  3. The catch block at line 58-60 executes messages.add(m) directly, bypassing threading entirely
  4. Immediate assertion succeeds on this fallback path, not on proper JavaFX thread execution

The production code's try-catch (lines 56-60 in src/main/java/com/example/HelloModel.java) is a code smell—it couples production logic to test environment state.

Fix options:

  1. Add TestFX dependency and use WaitForAsyncUtils.waitForFxEvents() to properly wait for JavaFX thread execution
  2. Initialize JavaFX toolkit with @BeforeEach using Platform.startup() or com.sun.javafx.application.PlatformImpl.startup()
  3. Remove the try-catch workaround once proper JavaFX testing is in place
🤖 Prompt for AI Agents
In src/test/java/com/example/HelloModelTest.java around lines 76-95 the test is
passing due to HelloModel.receiveMessage() falling back to catching
IllegalStateException from Platform.runLater(), so update the test to run
against a real JavaFX toolkit rather than relying on that fallback: initialize
the JavaFX platform before tests (e.g., with a @BeforeEach that calls
Platform.startup(...) or use TestFX/TestApplication setup), then use a proper
sync/wait mechanism (e.g., WaitForAsyncUtils.waitForFxEvents() or an explicit
wait) after spy.simulateIncoming(...) before asserting model.getMessages(); once
tests are updated, remove the try-catch fallback in
src/main/java/com/example/HelloModel.java (lines ~56-60) so production code no
longer depends on IllegalStateException from missing JavaFX initialization.


@Test
@DisplayName("Model constructor should register receive handler on connection")
void constructorShouldRegisterHandler() {

var spy = new NtfyConnectionSpy();
var model = new HelloModel(spy);

assertThat(spy.handler).isNotNull();
}

@Test
@DisplayName("messageToSendProperty should update when setting message")
void messagePropertyShouldUpdate() {

var spy = new NtfyConnectionSpy();
var model = new HelloModel(spy);

model.setMessageToSend("Hello!");

assertThat(model.getMessageToSend()).isEqualTo("Hello!");
}

@Test
@DisplayName("getGreeting should return the expected greeting text")
void greetingShouldBeCorrect() {

var model = new HelloModel(new NtfyConnectionSpy());

assertThat(model.getGreeting()).isEqualTo("Chat Client by Adam");
}




}
15 changes: 15 additions & 0 deletions src/test/java/com/example/NtfyConnectionSpy.java
Original file line number Diff line number Diff line change
@@ -1,18 +1,33 @@
package com.example;

import java.io.File;
import java.util.function.Consumer;

public class NtfyConnectionSpy implements NtfyConnection {

String message;
File sentFile;
Consumer<NtfyMessageDto> handler;

@Override
public boolean send(String message) {
this.message = message;
return true;
}

public boolean sendFile(File file){
this.sentFile = file;
return true;
}

@Override
public void receive(Consumer<NtfyMessageDto> messageHandler) {
this.handler = messageHandler;
}

public void simulateIncoming(NtfyMessageDto message){
if (handler !=null){
handler.accept(message);
}
}
}
Loading