Add file upload support and improved message rendering #3
Conversation
-Adds open file button with styling -Adds line wrapping support for long text messages -Prevent listview row selection while still allowing buton clicks -adds static class NoSelectionModel to fix flicks when scrolling and clicking.
-Adds logic to detect mime type and fallback -Changed topic to my own
WalkthroughThis PR adds file attachment support to a JavaFX messaging application. Changes encompass UI enhancements for file selection and attachment rendering, model layer delegation, connection interface and implementation updates to send files and receive attachment metadata, and DTO schema expansion to model attachment data. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as HelloController
participant Model as HelloModel
participant Conn as NtfyConnectionImpl
participant Server as Server
User->>UI: Click attachment button
UI->>UI: Open FileChooser
User->>UI: Select file
UI->>UI: Set lastActionWasFile=true
UI->>Model: sendFile(file)
Model->>Conn: sendFile(file)
Conn->>Conn: Detect MIME type
Conn->>Server: PUT /adam<br/>(filename header,<br/>content-type header)
Server-->>Conn: Success/Failure
Conn-->>Model: true/false
Model-->>UI: Complete
UI->>UI: Clear message field,<br/>reset flag
sequenceDiagram
participant Server as Server
participant Conn as NtfyConnectionImpl
participant Parser as JSON Parser
participant Model as HelloModel
participant UI as HelloController
Server->>Conn: Poll /adam/json
Conn->>Conn: Parse JSON response
Parser->>Parser: Filter for event="message"
Note over Parser: Skip invalid lines,<br/>log failures
Parser->>Conn: NtfyMessageDto<br/>(with attachment)
Conn->>Model: Add to messages list
Model->>UI: Message observable updated
UI->>UI: Render attachment<br/>or text in cell
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
src/main/resources/com/example/hello-view.fxml (1)
19-20: Consider adding accessibility attributes for the attach button.The button uses an emoji (📎) as its label, which may not be accessible to screen reader users. Consider adding a tooltip or accessible text.
Apply this diff to improve accessibility:
- <Button text="📎" onAction="#attachFile" prefHeight="40" prefWidth="40"/> + <Button text="📎" onAction="#attachFile" prefHeight="40" prefWidth="40"> + <tooltip> + <Tooltip text="Attach file"/> + </tooltip> + </Button>src/main/java/com/example/NtfyConnection.java (1)
6-8: Consider consistent use of thepublicmodifier in interface methods.The interface methods have inconsistent use of the
publicmodifier:sendandreceiveexplicitly declarepublic, whilesendFileomits it. While both are valid (interface methods are implicitly public), consistency improves readability.Apply this diff for consistency:
public boolean send(String message); - boolean sendFile(java.io.File file); + public boolean sendFile(java.io.File file); public void receive(Consumer<NtfyMessageDto> messageHandler);src/main/java/com/example/NtfyConnectionImpl.java (3)
33-33: Consider making the topic name configurable.The topic name is hardcoded to
"/adam"in multiple places (lines 33, 54, 86). This reduces flexibility and reusability. Consider making the topic name a constructor parameter or configuration value.This would allow the connection to be used with different topics:
private final String topic; public NtfyConnectionImpl(String hostName, String topic) { this.hostName = hostName; this.topic = topic; }
58-73: Good improvements to error handling and filtering.The enhanced receive method now includes proper error handling for JSON parsing failures and filters for message events. This is a significant improvement over the previous implementation.
However, consider replacing the debug logging at line 61 with proper structured logging.
.thenAccept(response -> response.body() - .peek(s -> System.out.println(s)) .map(s -> {
95-97: Use more specific exception handling.Catching
Exceptionis overly broad. Consider catching specific exceptions likeIOExceptionor handling them separately for better error reporting.- } catch (Exception e) { + } catch (IOException e) { e.printStackTrace(); return false; }src/main/java/com/example/HelloController.java (1)
172-185: Consider clarifying the send logic.The logic works but is somewhat convoluted:
- Line 174 allows proceeding if
lastActionWasFileis true (even with blank message)- Lines 179-181 only send the text message if it's not blank
- This means file-only attachments skip the text send, which is correct but not immediately obvious
Consider making the intent clearer:
public void sendMessage(ActionEvent actionEvent) { String message = messageField.getText(); - if(!lastActionWasFile && (message == null || message.isBlank())){ + boolean hasText = message != null && !message.isBlank(); + + if (!lastActionWasFile && !hasText) { showTemporaryAlert("You must write something before sending!"); return; } - if (message != null && !message.isBlank()) { + if (hasText) { model.sendMessage(); } messageField.clear(); lastActionWasFile = false; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/com/example/HelloController.java(3 hunks)src/main/java/com/example/HelloModel.java(2 hunks)src/main/java/com/example/NtfyConnection.java(1 hunks)src/main/java/com/example/NtfyConnectionImpl.java(3 hunks)src/main/java/com/example/NtfyMessageDto.java(1 hunks)src/main/java/module-info.java(1 hunks)src/main/resources/com/example/hello-view.fxml(1 hunks)src/main/resources/com/example/style.css(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/main/resources/com/example/style.css
[error] 30-30: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🔇 Additional comments (4)
src/main/resources/com/example/style.css (1)
30-30: Good fix for the CSS syntax error.The trailing slash has been correctly removed. The static analysis warning about
-fx-cursorbeing unknown is a false positive—this is a valid JavaFX CSS property that is not part of standard CSS.src/main/java/module-info.java (1)
8-8: LGTM!The
java.desktopmodule requirement is necessary to support the new file attachment functionality, specifically forFileChooserandDesktop.getDesktop().browse()used inHelloController.src/main/java/com/example/HelloController.java (1)
202-233: Acceptable workaround for ListView selection issues.The
NoSelectionModelclass is a reasonable workaround to prevent cell re-rendering that causes flickering when cells contain images or custom content. The implementation correctly overrides all required methods to disable selection while keeping interactive elements (buttons) functional.src/main/java/com/example/NtfyMessageDto.java (1)
6-23: LGTM!The DTO structure with nested
Attachmentrecord is well-designed for JSON deserialization. The@JsonIgnorePropertiesannotations on both records will gracefully handle any additional fields from the API. The dualexpiresfields (message and attachment) make sense as they may have different expiration times.
| private void attachFile() { | ||
| FileChooser chooser = new FileChooser(); | ||
| chooser.getExtensionFilters().add( | ||
| new FileChooser.ExtensionFilter("Images", "*.png", "*.jpg", "*.jpeg", "*.gif", "*.txt") | ||
| ); | ||
|
|
||
| File file = chooser.showOpenDialog(messageField.getScene().getWindow()); | ||
| if (file != null) { | ||
| lastActionWasFile = true; | ||
| model.sendFile(file); | ||
| } | ||
| } |
There was a problem hiding this comment.
Misleading file filter label and missing error feedback.
Line 39 labels the filter as "Images" but includes *.txt files. Additionally, there's no user feedback if model.sendFile(file) fails (note that the return value issue was flagged in HelloModel.java review).
Apply this diff to fix the label:
chooser.getExtensionFilters().add(
- new FileChooser.ExtensionFilter("Images", "*.png", "*.jpg", "*.jpeg", "*.gif", "*.txt")
+ new FileChooser.ExtensionFilter("Images and Text", "*.png", "*.jpg", "*.jpeg", "*.gif", "*.txt")
);Or separate the filters:
chooser.getExtensionFilters().addAll(
- new FileChooser.ExtensionFilter("Images", "*.png", "*.jpg", "*.jpeg", "*.gif", "*.txt")
+ new FileChooser.ExtensionFilter("Images", "*.png", "*.jpg", "*.jpeg", "*.gif"),
+ new FileChooser.ExtensionFilter("Text Files", "*.txt")
);📝 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.
| private void attachFile() { | |
| FileChooser chooser = new FileChooser(); | |
| chooser.getExtensionFilters().add( | |
| new FileChooser.ExtensionFilter("Images", "*.png", "*.jpg", "*.jpeg", "*.gif", "*.txt") | |
| ); | |
| File file = chooser.showOpenDialog(messageField.getScene().getWindow()); | |
| if (file != null) { | |
| lastActionWasFile = true; | |
| model.sendFile(file); | |
| } | |
| } | |
| private void attachFile() { | |
| FileChooser chooser = new FileChooser(); | |
| chooser.getExtensionFilters().add( | |
| new FileChooser.ExtensionFilter("Images and Text", "*.png", "*.jpg", "*.jpeg", "*.gif", "*.txt") | |
| ); | |
| File file = chooser.showOpenDialog(messageField.getScene().getWindow()); | |
| if (file != null) { | |
| lastActionWasFile = true; | |
| model.sendFile(file); | |
| } | |
| } |
| private void attachFile() { | |
| FileChooser chooser = new FileChooser(); | |
| chooser.getExtensionFilters().add( | |
| new FileChooser.ExtensionFilter("Images", "*.png", "*.jpg", "*.jpeg", "*.gif", "*.txt") | |
| ); | |
| File file = chooser.showOpenDialog(messageField.getScene().getWindow()); | |
| if (file != null) { | |
| lastActionWasFile = true; | |
| model.sendFile(file); | |
| } | |
| } | |
| private void attachFile() { | |
| FileChooser chooser = new FileChooser(); | |
| chooser.getExtensionFilters().addAll( | |
| new FileChooser.ExtensionFilter("Images", "*.png", "*.jpg", "*.jpeg", "*.gif"), | |
| new FileChooser.ExtensionFilter("Text Files", "*.txt") | |
| ); | |
| File file = chooser.showOpenDialog(messageField.getScene().getWindow()); | |
| if (file != null) { | |
| lastActionWasFile = true; | |
| model.sendFile(file); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/HelloController.java around lines 36 to 47, the
FileChooser filter is mislabeled "Images" while including "*.txt" and there is
no handling if model.sendFile(file) fails; change the filter to either separate
image and text filters (e.g., an "Images" filter with only image extensions plus
a separate "Text" filter with "*.txt") or rename the combined filter to
something like "Images and Text"; also do not set lastActionWasFile before
confirming success—call model.sendFile(file), check its return/throw for
failure, and on failure show user feedback (e.g., an alert/dialog or status
message) and avoid changing lastActionWasFile, while on success set
lastActionWasFile = true.
| openBtn.setOnAction(e -> { | ||
| System.out.println("Opening: " + fileUrl); | ||
| new Thread(() -> { | ||
| try { | ||
| URI uri = new URI(fileUrl); | ||
| Desktop.getDesktop().browse(uri); | ||
| } catch (Exception ex) { | ||
| ex.printStackTrace(); | ||
| } | ||
| }).start(); |
There was a problem hiding this comment.
Validate URLs before opening in browser.
The code opens attachment URLs directly in the browser without validation. This could potentially expose users to malicious links if the server is compromised or sends malicious URLs.
Apply this diff to add basic URL validation:
Button openBtn = new Button("Open file");
openBtn.setOnAction(e -> {
System.out.println("Opening: " + fileUrl);
+ if (!isValidUrl(fileUrl)) {
+ System.err.println("Invalid or unsafe URL: " + fileUrl);
+ return;
+ }
new Thread(() -> {
try {
URI uri = new URI(fileUrl);
Desktop.getDesktop().browse(uri);
} catch (Exception ex) {
ex.printStackTrace();
}
}).start();
});Add a validation helper method:
private boolean isValidUrl(String url) {
if (url == null || url.isEmpty()) return false;
try {
URI uri = new URI(url);
String scheme = uri.getScheme();
return "http".equals(scheme) || "https".equals(scheme);
} catch (Exception e) {
return false;
}
}🤖 Prompt for AI Agents
In src/main/java/com/example/HelloController.java around lines 124 to 133, the
code directly opens fileUrl in the user's browser; add and use a URL validation
helper to ensure the URL is non-null, non-empty, parses as a URI and uses only
"http" or "https" scheme before launching Desktop.browse. Implement a private
boolean isValidUrl(String url) as described in the review and check it before
starting the new Thread — if invalid, log or show an error and return; only
construct the URI and call Desktop.getDesktop().browse(uri) when
isValidUrl(fileUrl) returns true, and avoid swallowing exceptions silently (log
the exception).
| public void sendFile(File file) { | ||
| connection.sendFile(file); | ||
| } |
There was a problem hiding this comment.
Handle the boolean return value from connection.sendFile.
The connection.sendFile(file) method returns a boolean indicating success or failure, but this return value is being ignored. This prevents callers from knowing whether the file upload succeeded and may lead to silent failures.
Consider either:
- Returning the boolean from this method so callers can handle failures
- Logging errors or notifying the user when the send fails
Apply this diff to propagate the return value:
- public void sendFile(File file) {
- connection.sendFile(file);
+ public boolean sendFile(File file) {
+ return connection.sendFile(file);
}📝 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.
| public void sendFile(File file) { | |
| connection.sendFile(file); | |
| } | |
| public boolean sendFile(File file) { | |
| return connection.sendFile(file); | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/HelloModel.java around lines 58-60, the method
ignores the boolean result of connection.sendFile(file); change the method to
propagate the result by updating the signature to return boolean and return
connection.sendFile(file), and then update all callers to handle the returned
boolean (log, notify user, or retry on false). If you prefer to keep a void API
instead, check the boolean result inside this method and handle failure by
logging an error or throwing an exception so failures are not silent; ensure
unit tests and call sites are adjusted accordingly.
|
|
||
| @Override | ||
| public void receive(Consumer<NtfyMessageDto> messageHandler) { | ||
| String startId = "8TuugOLkvDz1"; // just to make the app wont load 10000000 messages |
There was a problem hiding this comment.
Remove or use the unused startId variable.
The startId variable is declared but never used. Line 54 uses a different hardcoded ID ("3hPbr2dcIUiU") in the URL. Either use this variable in the query parameter or remove it.
Apply this diff to use the variable consistently:
- String startId = "8TuugOLkvDz1"; // just to make the app wont load 10000000 messages
+ String startId = "3hPbr2dcIUiU"; // just to make the app wont load 10000000 messages
HttpRequest httpRequest = HttpRequest.newBuilder()
.GET()
- .uri(URI.create(hostName + "/adam/json?since=3hPbr2dcIUiU"))
+ .uri(URI.create(hostName + "/adam/json?since=" + startId))
.build();Or remove it if not needed:
- String startId = "8TuugOLkvDz1"; // just to make the app wont load 10000000 messages
HttpRequest httpRequest = HttpRequest.newBuilder()
.GET()
.uri(URI.create(hostName + "/adam/json?since=3hPbr2dcIUiU"))
.build();Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/com/example/NtfyConnectionImpl.java around line 51, the local
variable startId ("8TuugOLkvDz1") is declared but never used while the code
later hardcodes a different ID ("3hPbr2dcIUiU") into the URL; either remove the
unused startId declaration or replace the hardcoded ID usage with this variable
so the same startId is used for the query parameter. Update the URL construction
to reference startId (or delete the declaration) and run a quick compile to
ensure no unused-variable warnings remain.
| public boolean sendFile(File file){ | ||
| try { | ||
| String mime = java.nio.file.Files.probeContentType(file.toPath()); | ||
|
|
||
| if(mime == null) mime = "application/octet-stream"; | ||
|
|
||
| long size = file.length(); | ||
|
|
||
| HttpRequest request = HttpRequest.newBuilder() | ||
| .uri(URI.create(hostName + "/adam")) | ||
| .header("Filename", file.getName()) | ||
| .header("Content-Type", mime) | ||
| .PUT(HttpRequest.BodyPublishers.ofFile(file.toPath())) | ||
| .build(); | ||
|
|
||
| http.sendAsync(request, HttpResponse.BodyHandlers.discarding()); | ||
| return true; | ||
|
|
||
| } catch (Exception e) { | ||
| e.printStackTrace(); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: sendFile returns before the async operation completes.
Line 92 uses sendAsync, which returns immediately without waiting for the HTTP request to complete. However, line 93 returns true, incorrectly indicating success before the actual upload finishes. This means:
- The method will always return
true(unless an exception occurs before the request starts) - The caller cannot know if the file upload actually succeeded or failed
- Network errors or server rejections will go undetected
Apply this diff to use synchronous sending:
- http.sendAsync(request, HttpResponse.BodyHandlers.discarding());
+ var response = http.send(request, HttpResponse.BodyHandlers.discarding());
+ return response.statusCode() >= 200 && response.statusCode() < 300;
- return true;
} catch (Exception e) {
e.printStackTrace();
return false;
}Or handle the CompletableFuture properly:
- public boolean sendFile(File file){
+ public CompletableFuture<Boolean> sendFile(File file){
try {
// ... mime detection code ...
- http.sendAsync(request, HttpResponse.BodyHandlers.discarding());
- return true;
+ return http.sendAsync(request, HttpResponse.BodyHandlers.discarding())
+ .thenApply(response -> response.statusCode() >= 200 && response.statusCode() < 300);
} catch (Exception e) {
e.printStackTrace();
- return false;
+ return CompletableFuture.completedFuture(false);
}
}📝 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.
| public boolean sendFile(File file){ | |
| try { | |
| String mime = java.nio.file.Files.probeContentType(file.toPath()); | |
| if(mime == null) mime = "application/octet-stream"; | |
| long size = file.length(); | |
| HttpRequest request = HttpRequest.newBuilder() | |
| .uri(URI.create(hostName + "/adam")) | |
| .header("Filename", file.getName()) | |
| .header("Content-Type", mime) | |
| .PUT(HttpRequest.BodyPublishers.ofFile(file.toPath())) | |
| .build(); | |
| http.sendAsync(request, HttpResponse.BodyHandlers.discarding()); | |
| return true; | |
| } catch (Exception e) { | |
| e.printStackTrace(); | |
| return false; | |
| } | |
| } | |
| public boolean sendFile(File file){ | |
| try { | |
| String mime = java.nio.file.Files.probeContentType(file.toPath()); | |
| if(mime == null) mime = "application/octet-stream"; | |
| long size = file.length(); | |
| HttpRequest request = HttpRequest.newBuilder() | |
| .uri(URI.create(hostName + "/adam")) | |
| .header("Filename", file.getName()) | |
| .header("Content-Type", mime) | |
| .PUT(HttpRequest.BodyPublishers.ofFile(file.toPath())) | |
| .build(); | |
| var response = http.send(request, HttpResponse.BodyHandlers.discarding()); | |
| return response.statusCode() >= 200 && response.statusCode() < 300; | |
| } catch (Exception e) { | |
| e.printStackTrace(); | |
| return false; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/example/NtfyConnectionImpl.java around lines 77 to 99,
sendFile currently calls http.sendAsync(...) and immediately returns true, which
reports success before the upload completes; change this to wait for the request
to finish and only return true on a successful HTTP response (e.g., 2xx). Either
replace sendAsync with http.send(...) to perform a synchronous request and
inspect the HttpResponse status code, or keep sendAsync but call .get() (or
otherwise handle the CompletableFuture), catch
ExecutionException/InterruptedException, and evaluate the resulting
HttpResponse.statusCode() before returning; on non-successful status codes or
exceptions return false and log/propagate the error appropriately.
|
|
||
| if(mime == null) mime = "application/octet-stream"; | ||
|
|
||
| long size = file.length(); |
There was a problem hiding this comment.
Remove unused size variable.
The size variable is calculated but never used in the method.
Apply this diff:
if(mime == null) mime = "application/octet-stream";
- long size = file.length();
-
HttpRequest request = HttpRequest.newBuilder()🤖 Prompt for AI Agents
In src/main/java/com/example/NtfyConnectionImpl.java around line 83, the local
variable `size` is assigned with `long size = file.length();` but never used;
remove this unused variable declaration so the method no longer computes file
length unnecessarily and clean up any resulting unused-import or warning (simply
delete the `long size = file.length();` line).
Summary by CodeRabbit
New Features
Bug Fixes