Conversation
WalkthroughAdds a JavaFX chat application integrated with an NTFY backend: new chat model, controller, network HTTP client, FXML UI, environment loader, config (ntfy-server.yml, .gitignore) and build updates for Java 21, plus unit tests and module descriptor changes. No public API removals; several new public classes and exports added. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ChatController
participant ChatModel
participant NtfyHttpClient
participant NtfyServer
Note right of ChatController `#f9f3e6`: Send message (async)
User->>ChatController: click Send / enter
ChatController->>ChatModel: sendMessage(text)
ChatModel->>NtfyHttpClient: send(baseUrl, NtfyMessage)
NtfyHttpClient->>NtfyServer: POST /{topic} JSON
NtfyServer-->>NtfyHttpClient: 200 OK
NtfyHttpClient-->>ChatModel: success
ChatModel-->>ChatController: Platform.runLater -> update ObservableList
Note right of ChatModel `#eef9f2`: Subscribe (stream)
ChatModel->>NtfyHttpClient: subscribe(baseUrl, topic, onMessage, onError)
NtfyHttpClient->>NtfyServer: GET /{topic}/json (stream)
NtfyServer-->>NtfyHttpClient: stream JSON lines
loop for each line
NtfyHttpClient->>ChatModel: onMessage(NtfyMessage)
ChatModel->>ChatController: Platform.runLater -> append message
end
Note right of ChatController `#fff5f0`: Send file (async)
User->>ChatController: attach file
ChatController->>ChatModel: sendFile(file)
ChatModel->>NtfyHttpClient: sendFile(baseUrl, topic, file)
NtfyHttpClient->>NtfyServer: PUT /{topic} with Filename header
NtfyServer-->>NtfyHttpClient: 200 OK
ChatModel->>ChatController: Platform.runLater -> append file info message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Areas to inspect closely:
Possibly related PRs
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 |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @gitnes94. * #12 (comment) The following files were modified: * `src/main/java/com/example/HelloFX.java` * `src/main/java/com/example/HelloModel.java` * `src/main/java/com/example/controller/ChatController.java` * `src/main/java/com/example/model/ChatModel.java` * `src/main/java/com/example/model/NtfyMessage.java` * `src/main/java/com/example/network/ChatNetworkClient.java` * `src/main/java/com/example/network/NtfyHttpClient.java` * `src/main/java/com/example/util/EnvLoader.java`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Nitpick comments (12)
ntfy-server.yml (1)
2-2: Note: Ensure production configuration is separate.This is a hardcoded development configuration using
http://localhost:8080. When deploying to production, you'll need a separate configuration file with proper domain, HTTPS, and security headers configured.pom.xml (1)
32-48: Remove redundantjunit-jupiteraggregator dependency.The
junit-jupiterdependency (lines 32–35) is a POM aggregator that transitively includes bothjunit-jupiter-apiandjunit-jupiter-engine. Declaring all three explicitly is redundant and complicates dependency management. Keep only the explicitapiandenginedeclarations or use only the aggregator.Apply this diff to remove the redundant aggregator:
<dependency> - <groupId>org.junit.jupiter</groupId> - <artifactId>junit-jupiter</artifactId> - <version>5.10.2</version> - <scope>test</scope> - </dependency> - <dependency> <groupId>org.junit.jupiter</groupId> <artifactId>junit-jupiter-api</artifactId> <version>5.10.2</version> <scope>test</scope> </dependency>src/main/java/com/example/util/EnvLoader.java (2)
20-20: PreferFiles.newBufferedReaderoverFileReader.Using
Files.newBufferedReader(envPath)is the modern approach and handles charset encoding more explicitly.Apply this diff:
- try (BufferedReader reader = new BufferedReader(new FileReader(envPath.toFile()))) { + try (BufferedReader reader = Files.newBufferedReader(envPath)) {
29-35: Consider validating key format and handling quoted values.Environment files often contain quoted values (e.g.,
KEY="value with spaces"). Consider trimming quotes from values. Also, validate that keys contain only valid characters to prevent issues.Example enhancement:
String[] parts = line.split("=", 2); if (parts.length == 2) { String key = parts[0].trim(); String value = parts[1].trim(); // Remove surrounding quotes if present if (value.startsWith("\"") && value.endsWith("\"") && value.length() > 1) { value = value.substring(1, value.length() - 1); } System.setProperty(key, value); System.out.println("Loaded env: " + key); }src/test/java/com/example/model/ChatModelTest.java (2)
10-18: Remove redundantassertNotNullcheck.Line 17's
assertNotNull(message)is redundant since lines 15-16 already dereferencemessage. If it were null, you'd get an NPE before reaching line 17.Apply this diff:
void testMessageCreation() { NtfyMessage message = new NtfyMessage("testTopic", "Hello"); assertEquals("testTopic", message.topic()); assertEquals("Hello", message.message()); - assertNotNull(message); }
8-36: Consider testing edge cases.The tests cover happy paths but don't validate behavior with null or empty inputs. Consider adding tests for edge cases like
new NtfyMessage(null, "message"),new NtfyMessage("", ""), etc., to document expected behavior.src/main/java/com/example/controller/ChatController.java (3)
30-33: Consider error handling for ChatModel initialization.If the NTFY base URL is invalid or the connection fails during
connect(), there's no feedback to the user. Consider catching exceptions and displaying an error dialog or updating the status label.
35-57: Extract style string to constant to avoid recreation.The style string is recreated for every cell update. Extract it to a static final constant to improve performance.
private static final String CELL_STYLE = "-fx-background-color: rgba(255, 250, 240, 0.6); " + "-fx-text-fill: #3d3d3d; " + "-fx-padding: 14 18 14 18; " + // ... rest of style ;Then use:
setStyle(CELL_STYLE);
89-91: No user feedback for send failures.When sending fails, only console output is produced. Users won't see error messages. Consider showing an Alert dialog or updating a status message in the UI.
Example:
task.setOnFailed(e -> { updateStatusOffline(); Alert alert = new Alert(Alert.AlertType.ERROR); alert.setTitle("Send Failed"); alert.setContentText("Failed to send message: " + task.getException().getMessage()); alert.showAndWait(); });src/main/resources/com/example/ChatView.fxml (2)
8-217: Consider extracting inline styles to external CSS.The FXML contains extensive inline styling throughout. For better maintainability and reusability, consider extracting common styles to an external CSS file. This also makes theming easier.
For example, create
styles.css:.chat-container { -fx-background-color: linear-gradient(to bottom, #f5f0e8 0%, #e8dfd0 100%); } .message-cell { -fx-background-color: rgba(255, 250, 240, 0.6); -fx-text-fill: #3d3d3d; /* ... more styles */ }Then reference with
styleClass="chat-container"instead of inlinestyle="".
111-116: Consider accessibility improvements.The ListView doesn't have accessibility attributes. Consider adding
aria-labelor similar accessibility properties to improve screen reader support.src/main/java/com/example/network/NtfyHttpClient.java (1)
25-28: Consider configuring request timeouts.The HttpClient is created with default settings, which means no timeout. Network operations could hang indefinitely if the server becomes unresponsive.
Apply this diff to configure reasonable timeouts:
public NtfyHttpClient() { - this.http = HttpClient.newHttpClient(); + this.http = HttpClient.newBuilder() + .connectTimeout(java.time.Duration.ofSeconds(10)) + .build(); this.objectMapper = new ObjectMapper(); }Additionally, consider setting request-level timeouts on individual requests using
.timeout(Duration)on the HttpRequest.Builder.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.gitignore(1 hunks)ntfy-server.yml(1 hunks)pom.xml(1 hunks)src/main/java/com/example/HelloController.java(0 hunks)src/main/java/com/example/HelloFX.java(1 hunks)src/main/java/com/example/HelloModel.java(1 hunks)src/main/java/com/example/controller/ChatController.java(1 hunks)src/main/java/com/example/model/ChatModel.java(1 hunks)src/main/java/com/example/model/NtfyMessage.java(1 hunks)src/main/java/com/example/network/ChatNetworkClient.java(1 hunks)src/main/java/com/example/network/NtfyHttpClient.java(1 hunks)src/main/java/com/example/util/EnvLoader.java(1 hunks)src/main/java/module-info.java(1 hunks)src/main/resources/com/example/ChatView.fxml(1 hunks)src/test/java/com/example/model/ChatModelTest.java(1 hunks)src/test/java/com/example/model/NtfyMesageTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/com/example/HelloController.java
🧰 Additional context used
🧬 Code graph analysis (3)
src/main/java/com/example/controller/ChatController.java (1)
src/main/java/com/example/model/ChatModel.java (1)
ChatModel(11-72)
src/main/java/com/example/HelloFX.java (1)
src/main/java/com/example/util/EnvLoader.java (1)
EnvLoader(10-41)
src/main/java/com/example/model/ChatModel.java (1)
src/main/java/com/example/network/NtfyHttpClient.java (1)
NtfyHttpClient(20-108)
🔇 Additional comments (14)
src/main/java/com/example/HelloModel.java (1)
1-10: LGTM!The model class is clean and functionally correct. The removal of Javadoc comments is a documentation-only change that doesn't affect behavior.
One minor note: if this class is not actively used in the chat application (given that this PR introduces new chat-focused models), consider flagging it as dead code to keep the codebase lean.
ntfy-server.yml (1)
1-11: Configuration looks good for development.The ntfy server configuration is well-aligned with your Docker setup and chat application requirements. The attachment limits (5G total, 50M per file) and 3-hour expiry are sensible defaults for file uploads in a development environment.
pom.xml (3)
88-118: Module system configuration looks correct for Java 21 + JavaFX 21.The
--add-exportsflag forjavafx.graphics/com.sun.glass.utilsand the--add-opensflags in surefire forjava.basemodules are appropriate for modern Java/JavaFX integration. These allow Mockito bytecode generation and JavaFX internals access during tests.Confirm that if additional test frameworks or reflection-heavy libraries are added, corresponding module openings are documented or added here.
1-121: Overall dependency and build configuration is modern and well-structured.The move to Java 21, JavaFX 21.0.3, and updated test tooling (Mockito 5.11.0, AssertJ 3.25.3, WireMock 3.9.2) provides a solid foundation. Explicit version pinning and test scope segregation are best practices.
Minor note: Ensure that CI/CD pipelines and developer environments have Java 21 installed; this is a breaking upgrade from earlier Java versions.
26-30: All Jackson 2.17.0 compatibility checks pass for Java 21.Jackson 2.17.0 is compatible with Java 21, and your build configuration properly targets Java 21 with aligned dependencies (JavaFX 21.0.3 matches the Java 21 target). The maven-surefire
--add-opensflags are appropriately configured for the module system—no version mismatches or breaking changes detected.src/main/java/com/example/HelloFX.java (2)
14-16: Good exception narrowing and FXML update.The change from
throws Exceptiontothrows IOExceptionis more specific and accurate. The FXML resource update toChatView.fxmlaligns with the new chat interface.
24-24: Consider error handling for environment loading failures.If
EnvLoader.load()encounters critical errors (beyond missing .env), the application continues anyway. Consider whether certain environment variables are required and if their absence should prevent startup.For a more robust approach, you could:
- Make
EnvLoader.load()throw exceptions for critical errors- Validate required environment variables after loading
- Fail fast if critical configuration is missing
Is the current behavior (continue on any error) intentional for your use case?
src/main/java/com/example/model/NtfyMessage.java (1)
14-20: LGTM - Convenience constructors provide good defaults.The convenience constructors appropriately set
eventto "message" and allow optional attachment handling. This aligns well with the NTFY message structure.src/test/java/com/example/model/NtfyMesageTest.java (1)
13-91: Comprehensive test coverage for JSON serialization.The test suite thoroughly covers JSON (de)serialization, unknown field handling, and null values. This provides good confidence in the Jackson integration with
NtfyMessage.src/main/java/module-info.java (1)
1-16: LGTM! Module configuration is well-structured.The module exports and opens directives are appropriately configured for JavaFX FXML reflection and Jackson JSON serialization/deserialization.
src/main/java/com/example/network/ChatNetworkClient.java (1)
8-18: LGTM! Clean interface design.The interface provides a clear contract for network operations. The subscribe method appropriately uses callbacks for async error handling rather than checked exceptions.
src/main/java/com/example/network/NtfyHttpClient.java (1)
72-103: Subscription lifecycle and error handling look good.The async streaming implementation correctly uses
CompletableFuturewith proper error propagation through theonErrorcallback. The cancellation mechanism viafuture.cancel(true)is appropriate.src/main/java/com/example/model/ChatModel.java (2)
19-35: Good configuration hierarchy with fallback chain.The constructor properly checks System properties, then environment variables, then defaults to localhost. The diagnostic print statement at line 33 is helpful for debugging.
Note that
connect()is called in the constructor (line 34). If the network operation fails, the constructor will fail. Verify this is the intended behavior, as it prevents graceful degradation if the server is temporarily unavailable.
66-71: LGTM! File size formatting is correct.The binary unit calculations (1024-based) are accurate and the formatting provides good UX.
| public void initialize() { | ||
| this.model = new ChatModel(); | ||
|
|
||
| updateStatusOnline(); | ||
|
|
||
| messageListView.setCellFactory(listView -> new ListCell<String>() { | ||
| @Override | ||
| protected void updateItem(String item, boolean empty) { | ||
| super.updateItem(item, empty); | ||
| if (empty || item == null) { | ||
| setText(null); | ||
| setGraphic(null); | ||
| setStyle(""); | ||
| } else { | ||
| setText(item); | ||
| setAlignment(Pos.CENTER_LEFT); | ||
| setStyle("-fx-background-color: rgba(255, 250, 240, 0.6); " + | ||
| "-fx-text-fill: #3d3d3d; " + | ||
| "-fx-padding: 14 18 14 18; " + | ||
| "-fx-border-color: rgba(214, 69, 69, 0.15); " + | ||
| "-fx-border-width: 0 0 0 3; " + | ||
| "-fx-font-size: 13px; " + | ||
| "-fx-background-radius: 0; " + | ||
| "-fx-border-radius: 0; " + | ||
| "-fx-effect: dropshadow(gaussian, rgba(0,0,0,0.05), 3, 0, 0, 2);"); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| model.getMessages().addListener((javafx.collections.ListChangeListener.Change<? extends NtfyMessage> change) -> { | ||
| while (change.next()) { | ||
| if (change.wasAdded()) { | ||
| for (NtfyMessage msg : change.getAddedSubList()) { | ||
| String formatted = "🌸 " + msg.message(); | ||
| messageListView.getItems().add(formatted); | ||
| } | ||
| messageListView.scrollTo(messageListView.getItems().size() - 1); | ||
| } | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Resource leak: ChatModel subscription is never closed.
The ChatModel.connect() method creates a subscription, but there's no cleanup when the controller is destroyed. If the view is closed and recreated, this could lead to resource leaks.
Consider adding cleanup:
public void shutdown() {
// Add a close/disconnect method to ChatModel and call it here
if (model != null) {
model.disconnect(); // You'll need to implement this
}
}Then ensure this is called when the stage closes (add a listener in start() method of HelloFX).
🤖 Prompt for AI Agents
In src/main/java/com/example/controller/ChatController.java around lines 30 to
70, the controller initializes a ChatModel subscription but never closes it;
implement a cleanup path by adding a public disconnect()/close() method on
ChatModel that unsubscribes/closes the underlying subscription and releases
resources, add a shutdown() (or @PreDestroy) method on ChatController that calls
model.disconnect() if model != null, and ensure this shutdown is invoked when
the view/stage closes (e.g., register a stage close listener in your application
start method to call controller.shutdown() or rely on lifecycle cleanup).
| updateStatusOffline(); | ||
| }); | ||
|
|
||
| new Thread(task).start(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use ExecutorService instead of manual thread creation.
Creating threads manually with new Thread(task).start() is not ideal for resource management. Consider using a cached or fixed thread pool via ExecutorService to properly manage thread lifecycle.
Example using JavaFX utilities:
- new Thread(task).start();
+ javafx.concurrent.Service<Void> service = new javafx.concurrent.Service<>() {
+ @Override
+ protected javafx.concurrent.Task<Void> createTask() {
+ return task;
+ }
+ };
+ service.start();Or more simply, consider Platform.runLater for the background work with proper thread pool management at the model level.
📝 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.
| new Thread(task).start(); | |
| javafx.concurrent.Service<Void> service = new javafx.concurrent.Service<>() { | |
| @Override | |
| protected javafx.concurrent.Task<Void> createTask() { | |
| return task; | |
| } | |
| }; | |
| service.start(); |
🤖 Prompt for AI Agents
In src/main/java/com/example/controller/ChatController.java around line 94,
replace the manual thread creation ("new Thread(task).start()") with submission
to a shared ExecutorService (e.g., a cached or fixed thread pool) to manage
lifecycle and resource usage; create a private final ExecutorService field in
the controller or a shared model/service, call executor.submit(task) instead of
new Thread(...).start(), ensure long-lived Executors are shutdown appropriately
(e.g., in controller/application stop or @PreDestroy), and if task updates UI
elements, wrap UI updates with Platform.runLater to marshal back to the JavaFX
thread.
| System.err.println("❌ File send failed: " + task.getException().getMessage()); | ||
| }); | ||
|
|
||
| new Thread(task).start(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Same thread management issue as line 94.
Apply the same ExecutorService approach here for consistency.
🤖 Prompt for AI Agents
In src/main/java/com/example/controller/ChatController.java around line 121,
replace the direct new Thread(task).start() usage with the same ExecutorService
approach used at line 94: submit the Runnable/Callable to a shared
ExecutorService (reuse the existing executor field or factory method), handle
returned Future if needed, and avoid creating raw threads; ensure proper error
handling around task submission and do not call shutdown here (manage lifecycle
where the executor is created).
| public void connect() { | ||
| this.subscription = networkClient.subscribe( | ||
| baseUrl, | ||
| topic, | ||
| msg -> Platform.runLater(() -> messages.add(msg)), | ||
| error -> System.err.println("Error: " + error.getMessage()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Resource leak: Subscription is never closed.
The subscription field is assigned but never closed. If connect() is called multiple times, previous subscriptions leak. When the application shuts down, the subscription should be closed to release resources.
Add a method to properly close the subscription:
+ public void disconnect() {
+ if (subscription != null) {
+ subscription.close();
+ subscription = null;
+ }
+ }
+
public void connect() {
+ disconnect(); // Close any existing subscription first
this.subscription = networkClient.subscribe(
baseUrl,
topic,Additionally, consider calling disconnect() when the application is closing (e.g., in a JavaFX stop() method or similar lifecycle hook).
🤖 Prompt for AI Agents
In src/main/java/com/example/model/ChatModel.java around lines 41 to 48, the
connect() method assigns subscription but never closes prior subscriptions or
exposes a way to close it, causing resource leaks if connect() is called
repeatedly or on shutdown; add a public void disconnect() method that checks if
subscription is non-null, calls subscription.close() or subscription.cancel()
(whichever the subscription API provides), catches/logs any exception, and sets
subscription to null, and modify connect() to close any existing subscription
before creating a new one; ensure the application lifecycle calls disconnect()
on shutdown (e.g., from JavaFX stop()).
| <Label text="🌸" style="-fx-font-size: 10px; -fx-text-fill: rgba(214, 69, 69, 0.4);"/> | ||
| </HBox> | ||
|
|
||
| <!-- input område --> |
There was a problem hiding this comment.
Use English for comments.
"input område" should be "input area".
🤖 Prompt for AI Agents
In src/main/resources/com/example/ChatView.fxml at line 134, the comment text is
in Danish ("input område"); change it to English by replacing it with "input
area" so all comments are consistent and use English.
| <HBox spacing="15" alignment="CENTER_LEFT"> | ||
|
|
||
| <TextField fx:id="messageInput" | ||
| promptText="Skriv något fint..." |
There was a problem hiding this comment.
Swedish prompt text should be internationalized or in English.
The prompt text "Skriv något fint..." is in Swedish. For a project with English code/comments, consider using English ("Write something nice...") or implementing proper i18n with resource bundles.
🤖 Prompt for AI Agents
In src/main/resources/com/example/ChatView.fxml around line 153, the TextField
promptText contains a Swedish literal "Skriv något fint..."; replace it with an
English string or wire it to the JavaFX resource bundle for i18n. Either change
the attribute to an English literal such as "Write something nice..." or set
promptText to a resource key (e.g. "%chat.input.prompt") and add that key to the
appropriate .properties resource bundle with value "Write something nice..." so
the FXML uses the bundle-managed text.
|
|
||
| <HBox alignment="CENTER" spacing="5" style="-fx-padding: 8 0 0 0;"> | ||
| <Label text="🌸" style="-fx-font-size: 8px; -fx-text-fill: rgba(214, 69, 69, 0.5);"/> | ||
| <Label text="Skriv något fint..." |
There was a problem hiding this comment.
Swedish label text should be internationalized or in English.
The label text "Skriv något fint..." is in Swedish. Match the language with the rest of the UI or use resource bundles for i18n.
🤖 Prompt for AI Agents
In src/main/resources/com/example/ChatView.fxml around line 203, the Label text
"Skriv något fint..." is in Swedish while the rest of the UI is not; replace the
hard-coded Swedish string with a reference to a resource bundle key (e.g. fx:id
or text="%chat.placeholder") and add the corresponding key/value to the
appropriate properties file(s) (e.g. messages.properties and localized variants)
so the UI uses i18n or English consistently.
| </HBox> | ||
| </VBox> | ||
|
|
||
| <!-- längst ner blad --> |
There was a problem hiding this comment.
Use English for comments.
"längst ner blad" should be "bottom leaves".
🤖 Prompt for AI Agents
In src/main/resources/com/example/ChatView.fxml at line 211, the XML comment
text is in Swedish ("längst ner blad"); replace it with English by changing the
comment to "bottom leaves" so all source comments remain in English and
consistent with project conventions.
| package com.example.model; | ||
|
|
||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.DisplayName; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| class NtfyMessageTest { |
There was a problem hiding this comment.
Fix filename typo: NtfyMesageTest.java → NtfyMessageTest.java.
The filename has a typo ("Mesage" instead of "Message"). While the class name is correct, the filename should match for clarity and to avoid confusion. Rename the file to NtfyMessageTest.java.
🤖 Prompt for AI Agents
In src/test/java/com/example/model/NtfyMesageTest.java lines 1-9, the filename
contains a typo (NtfyMesageTest.java) that does not match the class name
NtfyMessageTest; rename the file to NtfyMessageTest.java (use git mv to preserve
history) and update any references or test-runner configurations if present so
the test class filename and path match the class declaration.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/com/example/model/ChatModel.java (2)
41-48: Resource leak: subscription is never closed.The subscription created at line 42 is never closed. If
connect()is called multiple times (e.g., after reconnection), previous subscriptions will leak. Additionally, the subscription should be closed when the application shuts down.Add a
disconnect()method to properly release resources:public void disconnect() { if (subscription != null) { subscription.close(); subscription = null; } }Then call
disconnect()before creating a new subscription inconnect(), and ensure it's called during application shutdown (e.g., in JavaFXstop()method).
59-68: Non-atomic file upload and notification.If
networkClient.sendFile()at line 64 succeeds butsendMessage()at line 67 fails, the file will be uploaded without a notification message appearing in chat. This inconsistency could confuse users.Consider wrapping both operations to ensure they succeed or fail together:
public void sendFile(File file) throws Exception { if (file == null || !file.exists()) { throw new IllegalArgumentException("File not found"); } - networkClient.sendFile(baseUrl, topic, file); - String fileMessage = "📎 " + file.getName() + " (" + formatFileSize(file.length()) + ")"; + + try { + networkClient.sendFile(baseUrl, topic, file); + sendMessage(fileMessage); + } catch (Exception e) { + throw new Exception("Failed to upload file: " + e.getMessage(), e); + } - sendMessage(fileMessage); }
🧹 Nitpick comments (5)
src/test/java/com/example/model/ChatModelTest.java (2)
25-30: Duplicate test withtestInitialState.Both
testGetMessagesandtestInitialStateverify the same behavior: thatgetMessages()returns a non-null, empty list. Consider removing one or differentiating their purpose.
9-57: Consider expanding test coverage for network operations.The current tests only validate the message list structure. Consider adding tests for:
connect()behaviorsendMessage()success and error casessendFile()validation and error handling- Invalid inputs (null/empty messages, missing files)
You could use mocking frameworks like Mockito to test the network client interactions without actual network calls.
src/main/java/com/example/model/ChatModel.java (3)
19-35: Constructor creates background subscription automatically.The constructor calls
connect()at line 34, which immediately starts a background subscription. This approach has drawbacks:
- Makes unit testing more difficult (subscription starts before test setup completes)
- No control over connection lifecycle
- Could cause failures during object construction
- Violates the principle of doing minimal work in constructors
Consider making
connect()an explicit method that callers invoke after construction, or provide a constructor parameter to control auto-connect behavior.
37-39: Consider immutability of exposed message list.
getMessages()returns the internalObservableListdirectly, allowing external code to mutate it. While this is a common JavaFX pattern for UI binding, it bypasses any future validation or control logic you might add.If direct mutation is necessary for UI binding, the current approach is acceptable. However, if mutation should be controlled, consider returning an unmodifiable view:
public ObservableList<NtfyMessage> getMessages() { return FXCollections.unmodifiableObservableList(messages); }Note: This would break the current tests that directly add messages to the list.
41-48: Limited error handling in subscription.The error callback at line 46 only prints to
System.err, which makes it difficult to:
- Debug subscription failures
- Notify the UI of connection issues
- Implement reconnection logic
Consider enhancing error handling:
public void connect() { this.subscription = networkClient.subscribe( baseUrl, topic, msg -> Platform.runLater(() -> messages.add(msg)), error -> { System.err.println("Subscription error: " + error.getMessage()); error.printStackTrace(); // Consider: notify UI, attempt reconnection, etc. } ); }You might also want to expose connection status to the UI layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/com/example/model/ChatModel.java(1 hunks)src/main/java/com/example/model/NtfyMessage.java(1 hunks)src/test/java/com/example/model/ChatModelTest.java(1 hunks)src/test/java/com/example/model/NtfyMesageTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/example/model/NtfyMesageTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/example/model/ChatModel.java (1)
src/main/java/com/example/network/NtfyHttpClient.java (1)
NtfyHttpClient(20-108)
🔇 Additional comments (1)
src/main/java/com/example/model/NtfyMessage.java (1)
5-21: Clean record design.The record structure is well-designed:
@JsonIgnorePropertiesproperly handles unknown fields from server responses- Convenience constructors reduce boilerplate for common cases
- Hardcoded
"message"event aligns with the ntfy protocol
2 participants
JavaFX Chat Application
Features implemented:
How to run:
docker run -d --name ntfy-server -p 8080:80 -v ${PWD}/ntfy-server.yml:/etc/ntfy/server.yml -v ntfy-cache:/var/cache/ntfy binwiederhier/ntfy serve.envfile with:NTFY_BASE_URL=http://localhost:8080./mvnw clean javafx:runTests:
Run with:
./mvnw testSummary by CodeRabbit
New Features
Chores
Tests
Documentation