Make a nicer homescreen with improved design and health page#121
Open
alicewersen-rgb wants to merge 1 commit intomainfrom
Open
Make a nicer homescreen with improved design and health page#121alicewersen-rgb wants to merge 1 commit intomainfrom
alicewersen-rgb wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughThe PR introduces HTTP request handling infrastructure by modifying Changes
Sequence DiagramsequenceDiagram
participant Client
participant HttpServer as HttpServer<br/>(Context Handler)
participant Handler as StaticFileHttpHandler
participant FileHandler as StaticFileHandler
participant FileSystem as FileSystem
Client->>HttpServer: GET /health.html
HttpServer->>Handler: handle(exchange)
Handler->>Handler: Normalize path<br/>("/" → "index.html")
Handler->>Handler: Check method == GET
Handler->>FileHandler: handleGetRequest(uri)
FileHandler->>FileSystem: Read file bytes
FileSystem-->>FileHandler: byte[] or IOException
FileHandler->>FileHandler: Set status & bytes
Handler->>Handler: Retrieve getFileBytes()<br/>& getStatusCode()
Handler->>HttpServer: sendResponseHeaders(status)
HttpServer->>Client: HTTP Response<br/>(status + bytes)
Handler->>HttpServer: close exchange
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
alicewersen-rgb
requested review from
AnnaZiafar,
AntonAhlqvist,
DennSel,
EdvinSandgren,
Ericthilen,
FeFFe1996,
eafalkens and
eraiicphu
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/main/java/org/example/StaticFileHttpHandler.java (1)
37-44: Response body output stream should be closed explicitly.The
getResponseBody()output stream should be closed before callingexchange.close(). Whileexchange.close()typically handles this, explicitly closing the stream ensures the response is fully flushed and follows best practices for resource management.♻️ Suggested fix
- exchange.sendResponseHeaders(statusCode, fileBytes.length); - exchange.getResponseBody().write(fileBytes); + exchange.sendResponseHeaders(statusCode, fileBytes.length); + try (var os = exchange.getResponseBody()) { + os.write(fileBytes); + } } else { fileBytes = "405 Method Not Allowed".getBytes(); statusCode = 405; - exchange.sendResponseHeaders(statusCode, fileBytes.length); - exchange.getResponseBody().write(fileBytes); + exchange.sendResponseHeaders(statusCode, fileBytes.length); + try (var os = exchange.getResponseBody()) { + os.write(fileBytes); + } } - - exchange.close();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/StaticFileHttpHandler.java` around lines 37 - 44, In StaticFileHttpHandler (the method handling the HttpExchange), after writing fileBytes to exchange.getResponseBody() in both the success and 405 branches, explicitly close the OutputStream returned by exchange.getResponseBody() (e.g., call close() on it) before calling exchange.close(); ensure this is done in all code paths (including error branches) so the response is flushed and the stream is properly released.www/health.html (1)
1-1: Consider using a complete HTML document structure.This health page is just an HTML snippet while
index.htmlis a full HTML5 document. For consistency and proper browser rendering, consider adding the standard document structure (<!DOCTYPE html>,<html>,<head>,<body>tags). This also helps with accessibility and consistent character encoding.💡 Suggested structure
-<h1>Server is healthy! 😊 </h1> +<!DOCTYPE html> +<html lang="en"> +<head> + <meta charset="UTF-8"> + <title>Health Check</title> +</head> +<body> + <h1>Server is healthy! 😊</h1> +</body> +</html>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/health.html` at line 1, The health page currently contains only an H1 snippet; update the file to be a complete HTML5 document by adding <!DOCTYPE html>, wrapping content in <html> and <body> tags, and include a <head> with a <meta charset="utf-8"> and a <title> (e.g., "Health") so the existing <h1>Server is healthy! 😊</h1> is placed inside the body; ensure the structure mirrors index.html for consistency and proper rendering/accessibility.src/main/java/org/example/StaticFileHandler.java (1)
27-31: Duplicate normalization logic exists in StaticFileHttpHandler.The path normalization for empty/root paths to
index.htmlis implemented here (lines 29-31) and also duplicated inStaticFileHttpHandler(lines 21-25). This redundancy could lead to maintenance issues if the logic diverges.Consider keeping the normalization in one place only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/StaticFileHandler.java` around lines 27 - 31, The root/empty-path normalization to "index.html" is duplicated between StaticFileHandler.handleGetRequest and StaticFileHttpHandler; refactor to a single implementation and call it instead of duplicating logic. Extract the normalization into a shared method (e.g., StaticFileHandler.normalizePath or a new util like StaticFileUtils.normalizeIndexPath) or move the logic into StaticFileHttpHandler and have StaticFileHandler.handleGetRequest call that shared method; then remove the duplicate conditional in handleGetRequest so only the single referenced function performs the "/" -> "index.html" mapping. Ensure both classes reference the exact shared method name so future changes remain centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 13-14: The instance fields fileBytes and statusCode in
StaticFileHandler are not thread-safe; refactor handleGetRequest to return an
immutable result object (e.g., FileResponse with byte[] bytes and int status)
instead of writing to instance fields, remove or deprecate
getFileBytes()/getStatusCode() so no shared mutable state is used, and update
StaticFileHttpHandler to call StaticFileHandler.handleGetRequest(...) and use
the returned FileResponse to form the HTTP response. Ensure FileResponse is
immutable (final fields) and contains all data previously held in the instance
fields.
In `@src/main/java/org/example/StaticFileHttpHandler.java`:
- Around line 11-13: The current StaticFileHttpHandler stores a single
StaticFileHandler instance (created in the StaticFileHttpHandler() constructor)
which leads to race conditions because StaticFileHandler mutates
fileBytes/statusCode across requests; fix by instantiating a new
StaticFileHandler for each incoming request instead of reusing the shared
handler—update the code paths that call handler.handleGetRequest(...) to
construct new StaticFileHandler() locally (or alternatively change
handleGetRequest to return an immutable result object containing fileBytes and
statusCode and remove mutable fields), referencing the StaticFileHttpHandler
constructor, the handler field, StaticFileHandler, and handleGetRequest when
making the change.
---
Nitpick comments:
In `@src/main/java/org/example/StaticFileHandler.java`:
- Around line 27-31: The root/empty-path normalization to "index.html" is
duplicated between StaticFileHandler.handleGetRequest and StaticFileHttpHandler;
refactor to a single implementation and call it instead of duplicating logic.
Extract the normalization into a shared method (e.g.,
StaticFileHandler.normalizePath or a new util like
StaticFileUtils.normalizeIndexPath) or move the logic into StaticFileHttpHandler
and have StaticFileHandler.handleGetRequest call that shared method; then remove
the duplicate conditional in handleGetRequest so only the single referenced
function performs the "/" -> "index.html" mapping. Ensure both classes reference
the exact shared method name so future changes remain centralized.
In `@src/main/java/org/example/StaticFileHttpHandler.java`:
- Around line 37-44: In StaticFileHttpHandler (the method handling the
HttpExchange), after writing fileBytes to exchange.getResponseBody() in both the
success and 405 branches, explicitly close the OutputStream returned by
exchange.getResponseBody() (e.g., call close() on it) before calling
exchange.close(); ensure this is done in all code paths (including error
branches) so the response is flushed and the stream is properly released.
In `@www/health.html`:
- Line 1: The health page currently contains only an H1 snippet; update the file
to be a complete HTML5 document by adding <!DOCTYPE html>, wrapping content in
<html> and <body> tags, and include a <head> with a <meta charset="utf-8"> and a
<title> (e.g., "Health") so the existing <h1>Server is healthy! 😊</h1> is
placed inside the body; ensure the structure mirrors index.html for consistency
and proper rendering/accessibility.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/org/example/StaticFileHandler.javasrc/main/java/org/example/StaticFileHttpHandler.javasrc/test/java/org/example/TestServer.javawww/health.htmlwww/index.html
Comment on lines
13
to
14
| private byte[] fileBytes; | ||
| private int statusCode; |
There was a problem hiding this comment.
Thread-safety issue: instance state is not safe for concurrent requests.
The fileBytes and statusCode fields are instance variables that are written in handleGetRequest and read via getters. If a single StaticFileHandler instance is shared across concurrent requests (which happens in StaticFileHttpHandler), a race condition occurs:
- Thread A calls
handleGetRequest, setsfileBytes/statusCode - Thread B calls
handleGetRequest, overwrites the fields - Thread A calls
getFileBytes()/getStatusCode()— reads Thread B's values
Since com.sun.net.httpserver.HttpServer handles requests concurrently, this will cause incorrect responses.
🔒 Recommended fix: return a result object instead of using instance state
+record FileResponse(byte[] fileBytes, int statusCode) {}
- private byte[] fileBytes;
- private int statusCode;
...
- void handleGetRequest(String uri) throws IOException {
+ FileResponse handleGetRequest(String uri) throws IOException {
+ byte[] fileBytes;
+ int statusCode;
...
- return;
+ return new FileResponse(fileBytes, statusCode);
}
...
- }
+ return new FileResponse(fileBytes, statusCode);
}
- public byte[] getFileBytes() { return fileBytes; }
- public int getStatusCode() { return statusCode; }Then update StaticFileHttpHandler to use the returned FileResponse.
Based on learnings: thread-safe patterns are required for shared state accessed by multiple request threads.
Also applies to: 75-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/StaticFileHandler.java` around lines 13 - 14, The
instance fields fileBytes and statusCode in StaticFileHandler are not
thread-safe; refactor handleGetRequest to return an immutable result object
(e.g., FileResponse with byte[] bytes and int status) instead of writing to
instance fields, remove or deprecate getFileBytes()/getStatusCode() so no shared
mutable state is used, and update StaticFileHttpHandler to call
StaticFileHandler.handleGetRequest(...) and use the returned FileResponse to
form the HTTP response. Ensure FileResponse is immutable (final fields) and
contains all data previously held in the instance fields.
Comment on lines
+11
to
+13
| public StaticFileHttpHandler() { | ||
| this.handler = new StaticFileHandler(); | ||
| } |
There was a problem hiding this comment.
Shared handler instance causes thread-safety issues.
The StaticFileHandler instance is created once and reused for all requests. As noted in the StaticFileHandler review, this causes race conditions when concurrent requests access the shared fileBytes/statusCode state.
Either create a new StaticFileHandler per request, or refactor handleGetRequest to return a result object.
🔒 Quick fix: create handler per request
- private final StaticFileHandler handler;
-
- public StaticFileHttpHandler() {
- this.handler = new StaticFileHandler();
- }
+ public StaticFileHttpHandler() {
+ }
`@Override`
public void handle(HttpExchange exchange) throws IOException {
+ StaticFileHandler handler = new StaticFileHandler();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/StaticFileHttpHandler.java` around lines 11 - 13,
The current StaticFileHttpHandler stores a single StaticFileHandler instance
(created in the StaticFileHttpHandler() constructor) which leads to race
conditions because StaticFileHandler mutates fileBytes/statusCode across
requests; fix by instantiating a new StaticFileHandler for each incoming request
instead of reusing the shared handler—update the code paths that call
handler.handleGetRequest(...) to construct new StaticFileHandler() locally (or
alternatively change handleGetRequest to return an immutable result object
containing fileBytes and statusCode and remove mutable fields), referencing the
StaticFileHttpHandler constructor, the handler field, StaticFileHandler, and
handleGetRequest when making the change.
Kathify
reviewed
Mar 8, 2026
Kathify
left a comment
Kathify
left a comment
There was a problem hiding this comment.
Snyggt jobbat!
En tanke: just nu gör både StaticFileHandler och StaticFileHttpHandler samma (t.ex. index.html och /). Det vore kanske snyggare att flytta allt till StaticFileHandler så vi slipper dubbelkod
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improved the default homepage UI. Added modern styling with centered layout, card design, gradient background and a styled action button. Also ensured the health endpoint works correctly from the homepage.
Summary by CodeRabbit
Release Notes
New Features
Style