Closes #84 – Improve homescreen design#27
Closed
alicewersen-rgb wants to merge 24 commits intomainfrom
Closed
Conversation
Add ci workflow for github actions.
* build: configure pom.xml with needed plugin/tools. Setup Java 25 environment with JUnit 5, Mockito, JaCoCo, Pitest, and Spotless * fix: add missing test scope for awaitility
* Dockerfile that builds an image in a docker container then runs in another docker container Current implementation uses Temporary App.class reference before relevant file is created to start server. * Fixed unverified commit * Set up non-root user and updated Dockerfile to use user. Fixed file path to use /app/ instead of /app/org/example to prevent unnessary nesting of packages.
* added parser class for headers * refactoring * refactoring * refactoring, added getHeadersMap * refactoring * refactoring code, adding debug boolean * added unit tests for HttpParser.java class * refactoring and all green tests for class * refactoring the code * refactoring the bufferedReader * refactoring with coderabbit feedback for header put to merge * refactoring with coderabbit feedback for header put to merge, changed headersmap * refactoring from reviewer input * refactoring for code rabbit comments
* Add HttpResponseBuilder and its tests Signed-off-by: JohanHiths <johan.hietala@iths.se> * Implement minimal HTTP response builder Signed-off-by: JohanHiths <johan.hietala@iths.se> * update coderabbit * more coderabbit fixes Signed-off-by: JohanHiths <johan.hietala@iths.se> * Bytte från httptest till http efter rekommendationer från Xeutos * code rabbit fix * fixed more code rabbit problems --------- Signed-off-by: JohanHiths <johan.hietala@iths.se>
* added parser class for headers * refactoring * refactoring * refactoring, added getHeadersMap * refactoring * adding class for parsing requestLine (method, path, version). Has getters/setters for the three. * refactoring code, private setters * added tests for throwing error * refactoring the bufferedReader * refactoring, adding check for requestLineArray.length() is less or equals to two with added unit test * refactoring, adding check for requestLineArray.length() is less or equals to two with added unit test * refactoring, adding test for null value of input stream * refactoring from coderabbit input * refactoring test class. * refactoring, adding debug boolean flag for logger lines * refactoring and testing buffered reader stream * refactoring, changed class HttpParser.java to extend HttpParseRequestLine.java and making them share the same buffered reader. Added method to set the reader and then methods to call the HttpParser * refactoring, changed class HttpParser.java to extend HttpParseRequestLine.java and making them share the same buffered reader. Added method to set the reader and then methods to call the HttpParser
* Add Bucket4j dependency in pom file * Extract Bucket4j version into a property and update version in dependency for consistency
* Add support for serving static files - Introduced `StaticFileHandler` to serve static files from the `www` directory. - Enhanced `HttpResponseBuilder` to support byte array bodies. - Made `setReader` method public in `HttpParser` to improve flexibility. - Code tested via Insomnia by coding a temporary method for multithreads in TcpServer.java, this is now removed before future implementations of ConnectionHandler Co-authored-by: Daniel Fahlén xsz300@gmail.com * Fix typo in `HttpResponseBuilder` (contentLength) and refine body handling logic Co-authored-by: Daniel Fahlén xsz300@gmail.com * Add basic HTML structure to `index.html` in `www` directory * Make `handleGetRequest` method private in `StaticFileHandler` to encapsulate functionality
* Updates pom.xml, with jackson-dependencies, for config file * Updates pom.xml, removes jackson-annotations:2.20, because it was apparently unnecessary.
* * Move HTTP handling logic from TcpServer to a dedicated ConnectionTask class * Implement virtual thread execution for better scalability * Add regex-based URI routing to support clean URLs and default to index.html * Ensure sockets are properly closed Co-authored-by: Caroline Nordbrandt <caroline_nordbradt@hotmail.com> * change thrown exception to runtime instead with appropiet message in tcpServer. --------- Co-authored-by: Caroline Nordbrandt <caroline_nordbradt@hotmail.com>
* Added basic YAML config-file. * Added class ConfigLoader with static classes for encapsulation * Added static metod loadOnce and step one of static method load * Added static method createMapperFor that checks for YAML or JSON-files before creating an ObjectMapper object. * implement ConfigLoader refs #13 * Added AppConfig.java record for config after coderabbit feedback * Updated ConfigLoader to use AppConfig record and jackson 3 * Added tests for ConfigLoader and reset cached method in ConfigLoader to ensure test isolation with static cache * Removed unused dependency. Minor readability tweaks in AppConfig. * Added check for illegal port numbers to withDefaultsApplied-method. * Added test for illegal port numbers.
* Add 404 error handling to `StaticFileHandler` with fallback page * Add test coverage for `StaticFileHandler` and improve constructor flexibility - Introduced a new constructor in `StaticFileHandler` to support custom web root paths, improving testability. - Added `StaticFileHandlerTest` to validate static file serving and error handling logic. * Add test for 404 handling in `StaticFileHandler` - Added a test to ensure nonexistent files return a 404 response. - Utilized a temporary directory and fallback file for improved test isolation. * Verify `Content-Type` header in `StaticFileHandlerTest` after running Pittest, mutations survived where setHeaders could be removed without test failure. * Remove unused `.btn` styles from `pageNotFound.html` * Improve 404 handling in `StaticFileHandler`: add fallback to plain text response if `pageNotFound.html` is missing, and fix typo in `pageNotFound.html`, after comments from CodeRabbit.
* Implements configuration loading and ensures that ConfigLoader is invoked during application startup (App.main). * Minor formating.
* initial commit, added interfaces Filter and FilterChain * Added HttpRequest class, groups together all information about a request that the server needs and easier to handle by future filters * added interfaces Filter and FilterChain with Java Servlet Filter architecture. * added FilterChainImpl * Corrected imports from JDKS HttpRequest, to projects HttpRequest class * Changed, params for FilterChain * Updated HttpRequest with attributes, * Revert "Updated HttpRequest with attributes," This reverts commit 0fd490e.
* Added MIME detector class and test class * Added logic for Mime detector class * Added Unit tests * Added logic in HttpResponseBuilder and tests to try it out * Solves duplicate header issue * Removed a file for another issue * Changed hashmap to Treemap per code rabbits suggestion * Corrected logic error that was failing tests as per P2P review * Added more reason phrases and unit testing, also applied code rabbits suggestions! * Added changes to Responsebuilder to make merging easier * Changed back to earlier commit to hande byte corruption new PR * Added StaticFileHandler from main * Added staticFileHandler with binary-safe writing * Fix: Normalize Content-Type charset to uppercase UTF-8 Changed 'charset=utf-8' to 'charset=UTF-8' in StaticFileHandlerTest to match MimeTypeDetector output and align with HTTP RFC standard. Uppercase UTF-8 is the correct format per RFC 2616/7231.
* Update Dockerfile Dockerfiles now copies www folder aswell * Added building and copying of dependency jar files * Fix dependency path in Dockerfile and update classpath in ENTRYPOINT configuration. * Fixed typo in Entrypoint configuration * Expose port 8080 in Dockerfile and changed appuser to come before ENTRYPOINT configuration. * Adjust Dockerfile paths for classes and dependencies, update `COPY` targets accordingly.
* Added comprehensive README.MD * Added formatting recommendations, clearer info
* Prevent path traversal and sanitize URI in StaticFileHandler. * Add test for path traversal protection and support 403 responses. * Add tests for URI sanitization and handling of encoded/special URLs. * Add test for null byte injection prevention in StaticFileHandler * Improve StaticFileHandler path traversal handling and test coverage * Improve URI sanitization and add test for 404 response handling in StaticFileHandler
* Resolve port: CLI > config > default * Wire port resolution to AppConfig/ConfigLoader and update docs/tests * Update PortConfigurationGuide.md * Update PortConfigurationGuide.md * Remove ServerPortResolver; use ConfigLoader for port * Update PortConfigurationGuide.md * Update PortConfigurationGuide.md * may be done
* refactor: add predefined HTTP status code constants to HttpResponseBuilder * refactor: use status code constants in StaticFileHandler * test: refactor StaticFileHandlerTest to use status code constants * test: refactor HttpResponseBuilderTest to use status code constants and explicit assertations
📝 WalkthroughWalkthroughIntroduces a fully functional HTTP web server implemented in Java with static file serving, YAML/JSON configuration, request parsing, Docker support, CI/CD workflows, and comprehensive test coverage including port resolution, file handling, and response building. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant TcpServer
participant ConnectionHandler
participant HttpParser
participant StaticFileHandler
participant HttpResponseBuilder
Client->>TcpServer: TCP connection on port 8080
TcpServer->>ConnectionHandler: create with client socket
TcpServer->>ConnectionHandler: runConnectionHandler()
ConnectionHandler->>HttpParser: create parser
HttpParser->>HttpParser: parseRequest()<br/>(reads request line + headers)
HttpParser-->>ConnectionHandler: parsed method, URI, headers
ConnectionHandler->>StaticFileHandler: resolveTargetFile(URI)
StaticFileHandler->>StaticFileHandler: sanitize URI<br/>check path traversal<br/>validate file exists
StaticFileHandler-->>ConnectionHandler: file bytes, status code
ConnectionHandler->>HttpResponseBuilder: set status, body, content-type
HttpResponseBuilder->>HttpResponseBuilder: build() response
HttpResponseBuilder-->>ConnectionHandler: HTTP response bytes
ConnectionHandler->>Client: send response
Client->>Client: receive response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
mattiasbarth,
met4lk1tty and
viktorlindell12
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (12)
www/health.html (1)
1-1: Consider adding proper HTML structure for consistency.Unlike
www/index.html, this file lacks a proper HTML document structure (doctype, html, head, body tags). While browsers will render it, having consistent structure across all HTML files improves maintainability and ensures consistent rendering.📝 Suggested minimal 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, Add a minimal, proper HTML document wrapper around the existing <h1>Server is healthy! 😊</h1> so the file uses a doctype and html/head/body structure like other pages (include a <title> and a basic <meta charset="utf-8"> in head) and move the existing <h1> into the body; ensure the resulting document mirrors the structure used in www/index.html for consistency and maintainability.src/main/java/org/example/httpparser/HttpRequest.java (1)
19-29: Consider validating required fields for null.The constructor accepts
method,path, andversionwithout null checks. If any of these arenull, it could causeNullPointerExceptiondownstream when these values are used in comparisons or string operations.🛡️ Proposed null validation
public HttpRequest(String method, String path, String version, Map<String, String> headers, String body) { + if (method == null || path == null || version == null) { + throw new IllegalArgumentException("method, path, and version must not be null"); + } this.method = method; this.path = path; this.version = version; this.headers = headers != null ? Map.copyOf(headers) : Collections.emptyMap(); this.body = body; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpRequest.java` around lines 19 - 29, The HttpRequest constructor accepts method, path, and version without validation which can lead to NPEs; update the HttpRequest(String method, String path, String version, Map<String, String> headers, String body) constructor to validate required params by calling Objects.requireNonNull (or equivalent) on method, path, and version with clear messages (e.g., "method must not be null") before assigning fields, while preserving the existing headers null-handling (Map.copyOf(headers) or Collections.emptyMap()) and assigning body as before.www/index.html (1)
19-35: Addtransitionproperty to.cardfor smooth hover animation.The
.card:hoverrule appliestransform: translateY(-5px), but the.cardclass lacks atransitionproperty. Without it, the hover effect will be abrupt rather than animated.✨ Proposed fix
.card { background: rgba(255, 255, 255, 0.1); padding: 40px; border-radius: 12px; backdrop-filter: blur(10px); + -webkit-backdrop-filter: blur(10px); box-shadow: 0 10px 30px rgba(0,0,0,0.3); max-width: 500px; - - margin-top: 50px; + transition: transform 0.3s, box-shadow 0.3s; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/index.html` around lines 19 - 35, The .card hover feels abrupt because the .card rule lacks a transition; update the .card CSS rule to include a transition for the properties changed on hover (at least transform and box-shadow), e.g., add a transition declaration to .card so the transform: translateY(-5px) and box-shadow changes in .card:hover animate smoothly (keep the selector names .card and .card:hover intact)..github/workflows/ci.yml (1)
16-26: Step name may not display the Java version dynamically.The step name
Set up JDK ${{ env.JAVA_VERSION }}references an environment variable set in a previous step'srunblock. GitHub Actions evaluates step names before execution, so the name may display literally asSet up JDKwithout the version. The workflow will still function correctly—this is purely cosmetic.💡 Alternative: use a static name
- - name: Set up JDK ${{ env.JAVA_VERSION }} + - name: Set up JDK (version from pom.xml)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 16 - 26, The step name "Set up JDK ${{ env.JAVA_VERSION }}" may not render the version because GitHub evaluates step names before the "Get Java Version" run sets the env; change the step name to a static or literal string (e.g., "Set up JDK") or move the version resolution into a step output and reference that output in the name so it is available at evaluation time; update the step with the existing "Get Java Version" and "Set up JDK ${{ env.JAVA_VERSION }}" identifiers accordingly.src/main/java/org/example/httpparser/HttpParseRequestLine.java (1)
26-31: Avoid partial object mutation before validation.Lines 26-31 set fields before all tokens are validated. Prefer validating into locals, then assigning once the request line is confirmed valid.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java` around lines 26 - 31, HttpParseRequestLine currently calls setMethod/setUri/setVersion before validating tokens which can leave the object partially mutated; change parse logic to first validate requestLineArray length and token formats into local variables (e.g., localMethod, localUri, localVersion), verify localMethod.matches("^[A-Z]+$") (and any other token checks), and only after all validations succeed call setMethod(localMethod), setUri(localUri), and setVersion(localVersion) so the object is assigned atomically and not left in a partially updated state.src/test/java/org/example/TestServer.java (1)
8-11: Consider making test-server port configurable.Hardcoding
8080can cause local conflicts. Optional improvement: accept a CLI arg/env var with fallback to 8080.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/TestServer.java` around lines 8 - 11, Replace the hardcoded 8080 in TestServer.java by reading a configurable port (from a CLI arg or an environment variable) with a safe integer parse and fallback to 8080; in the main/test entry that creates the HttpServer (the HttpServer server = HttpServer.create(new InetSocketAddress(8080), 0) line), compute an int port from e.g. args[0] if present or System.getenv("TEST_SERVER_PORT"), catch NumberFormatException and default to 8080, then pass new InetSocketAddress(port) into HttpServer.create and log the selected port (and keep using StaticFileHttpHandler as before).src/test/java/org/example/AppPortResolutionTest.java (1)
10-21: Add negative-path coverage for port parsing/validation.Current tests cover happy paths only. Consider adding cases for missing
--portvalue, non-numeric values, and out-of-range ports (0/65536) to lock down error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/AppPortResolutionTest.java` around lines 10 - 21, Add negative-path unit tests to AppPortResolutionTest targeting App.resolvePort: add test methods that assert an exception is thrown for (1) a missing --port value (e.g., args {"--port"}), (2) a non-numeric port value (e.g., {"--port","abc"}), and (3) out-of-range ports (e.g., {"--port","0"} and {"--port","65536"}). Use assertThrows to verify the method throws an appropriate exception (e.g., IllegalArgumentException or NumberFormatException) and optionally assert the exception message contains relevant text about invalid port input to make failures clear.www/pageNotFound.html (1)
48-53: Add a clear way back to the homepage.The 404 page is visually solid, but there’s no recovery action. Add a “Back to home” link/button (
href="/") to reduce dead-end UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@www/pageNotFound.html` around lines 48 - 53, Add a clear recovery action to the 404 page by inserting a "Back to home" link/button (anchor with href="/") inside the existing container div (e.g., alongside the <h2> and <p> elements) so users can easily return to the homepage; style it with a meaningful class name like "back-home" or "btn-home" to allow existing CSS to apply or to add new rules. Ensure the element is accessible (use clear text "Back to home", role/button if styled as a button, and include aria-label if needed) and place it logically beneath the message so it is visible and usable on all viewports.README.md (1)
2-2: Address markdownlint style warnings for heading progression and fenced code languages.These are low-risk, but cleaning them up (
MD001,MD019,MD040) will keep docs CI/lint clean and improve maintainability.Also applies to: 37-37, 39-44, 80-82, 87-89, 134-157, 234-239, 283-290, 340-342
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 2, README.md contains markdownlint warnings (MD001/MD019/MD040) around the heading "### **Class JUV25G**" and several other heading/code blocks; fix by ensuring proper heading progression (add or adjust a top-level H1 and make headings nest consistently so "### **Class JUV25G**" is preceded by appropriate H1/H2), ensure blank lines exist before and after headings, and add language identifiers to all fenced code blocks (and close any unclosed fences) referenced in sections around lines indicated (e.g., the "Class JUV25G" heading and blocks at 37, 39–44, 80–82, 87–89, 134–157, 234–239, 283–290, 340–342) so markdownlint rules MD001, MD019 and MD040 are satisfied.src/main/java/org/example/TcpServer.java (1)
18-18: Consider TLS at the server edge for production deployments.If this listener is exposed beyond localhost/internal networks, terminate TLS (either directly with SSL sockets or via a reverse proxy/load balancer) to avoid cleartext transport risks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/TcpServer.java` at line 18, The current TcpServer creates a plain ServerSocket (ServerSocket serverSocket = new ServerSocket(port)) which sends cleartext; for production terminate TLS at the server edge by either replacing ServerSocket with an SSLServerSocket from SSLServerSocketFactory (configure a keystore, set TLS protocols/ciphers, and use server-side authentication in the TcpServer class) or deploy an external TLS-terminating reverse proxy/load balancer in front of this listener and keep TcpServer bound to localhost/internal interface; update connection-accept logic in TcpServer to expect TLS-wrapped sockets (or document the reverse-proxy requirement) and ensure proper certificate/key management and TLS configuration.pom.xml (2)
39-56: Test dependencies look good.Adding mockito-core and awaitility is appropriate for the expanded test coverage. Minor note: awaitility version (4.3.0) is hardcoded while other versions use properties. Consider adding an
awaitility.versionproperty for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 39 - 56, The pom currently hardcodes awaitility version 4.3.0 in the dependency (artifactId awaitility); add a new property (e.g. awaitility.version) alongside the existing version properties and update the awaitility dependency to reference ${awaitility.version} so all test deps use properties consistently; ensure the new property is placed in the <properties> section near other version properties to keep pom organization consistent.
165-185: Spotless formatting plugin configured.The Spotless plugin is configured to remove unused imports and format annotations. Consider adding an explicit formatter (e.g.,
googleJavaFormat()orpalantirJavaFormat()) for more comprehensive style enforcement.💡 Example: Adding Google Java Format
<configuration> <java> + <googleJavaFormat> + <version>1.19.2</version> + </googleJavaFormat> <removeUnusedImports/> <formatAnnotations/> </java> </configuration>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` around lines 165 - 185, The Spotless plugin configuration (artifactId: spotless-maven-plugin) currently enables removeUnusedImports and formatAnnotations but lacks an explicit formatter; update the <configuration><java> block that contains removeUnusedImports and formatAnnotations to include a concrete formatter such as googleJavaFormat() or palantirJavaFormat() so Spotless enforces a complete style (e.g., add the googleJavaFormat or palantirJavaFormat entry inside the java configuration alongside removeUnusedImports/formatAnnotations).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 11-16: The runtime image copies static files to /www while WORKDIR
is /app and org.example.App expects config under ./application.yml and static
files under ./www, so align paths: update the Dockerfile so runtime WORKDIR /app
contains both the web root and the packaged resources—copy the build
resources/application.yml (or entire classes/resources from --from=build) into
/app (e.g., COPY --from=build /build/target/classes/application.yml /app/ or
COPY --from=build /build/target/classes/ /app/) and copy the static web assets
into /app/www (replace COPY /www/ /www/ with COPY /www/ /app/www/); ensure
ENTRYPOINT (["java","-classpath","/app:/app/dependencies/*","org.example.App"])
remains but now runs with the expected ./application.yml and ./www paths
available.
In `@pom.xml`:
- Line 14: The pom declares an invalid JUnit Jupiter property value; update the
<junit.jupiter.version> property to a real release (prefer 6.0.3, or use 5.14.3
if you must stay on 5.x) so Maven can resolve the dependency; change the value
of the junit.jupiter.version property and ensure any dependencies or plugins
referencing junit.jupiter.version pick up the new value, then re-run your build
to verify resolution.
In `@PortConfigurationGuide.md`:
- Around line 26-29: The YAML example has incorrect indentation: the port key
must be nested under server; update the example so that "port" is indented two
spaces under "server" (i.e., make "server" and "port" form a mapping with keys
server and port) so the YAML correctly represents server: { port: 9090 } and
parses as a nested structure.
In `@README.md`:
- Line 12: The table-of-contents link "[Features](`#features`)" is broken because
the actual heading includes an emoji and produces a different anchor; fix by
adding a stable explicit anchor before the heading (e.g., insert <a
id="features"></a> immediately above the Features heading) or remove the emoji
from the heading text so the auto-generated slug matches "#features", and then
ensure the link remains "[Features](`#features`)"; update the README heading (the
heading that currently shows the emoji) or add the explicit anchor to guarantee
the TOC link resolves.
- Around line 48-49: The Quick Start commands in README.md are wrong: remove the
duplicated word in the clone command ("git clone git clone ...") and ensure the
cd targets the actual repository folder created by the clone; replace the two
lines with a single "git clone
https://github.com/ithsjava25/project-webserver-juv25g.git" and then "cd
project-webserver-juv25g" so copy/paste users land in the correct directory.
In `@src/main/java/org/example/App.java`:
- Line 13: The code in App that constructs Path configPath =
Path.of("src/main/resources/application.yml") uses a source-tree path that won't
exist in packaged runs; update the App startup to load configuration from the
runtime classpath or a configurable external location instead: replace the
hard-coded Path creation with logic in App to first check a configurable
system/env property (e.g. "config.path") and use Paths.get(...) if present,
otherwise load "application.yml" via the classloader
(ClassLoader.getResourceAsStream or getResource) or Paths.get("application.yml")
relative to working dir so packaged jars/containers find the file; ensure
references to configPath or the config-loading code in App are updated
accordingly.
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 27-33: The resolveTargetFile method incorrectly appends ".html" to
any URI that doesn't end with ".html", which mangles assets and query URLs;
update resolveTargetFile to first parse/separate the path and query (or strip
the query string) and only append ".html" when the path has no file extension
and is not the root; keep the special-case mapping of "/" to "index.html",
preserve query parameters separately (do not include them when deciding to
append ".html"), and use a check on the path portion for an existing extension
(e.g., presence of a dot after the last slash) before calling
uri.concat(".html") so assets like "/style.css" and URLs with queries like
"/health?ok=1" are not rewritten incorrectly in resolveTargetFile.
In `@src/main/java/org/example/filter/FilterChainImpl.java`:
- Around line 17-31: FilterChainImpl currently stores mutable traversal state in
the instance field "index" and leaves the chain open when no filters remain;
change the traversal to use an immutable/local cursor and invoke an explicit
terminal action: implement doFilter(HttpRequest request, HttpResponseBuilder
response) to delegate to a private helper like doFilterAt(int pos, HttpRequest,
HttpResponseBuilder) that reads "filters" and calls
filters.get(pos).doFilter(..., this) while advancing pos (not the instance
"index"), and when pos == filters.size() call the terminal handler (e.g.,
dispatch the request or call a provided finalHandler.handle(request,response));
remove or stop using the instance "index" to ensure reentrancy and thread-safety
and wire in the terminal handler (method/field name such as finalHandler or
requestDispatcher) so requests are executed when the chain completes.
In `@src/main/java/org/example/http/HttpResponseBuilder.java`:
- Around line 78-81: The setHeaders method should explicitly reject null input:
at the start of HttpResponseBuilder.setHeaders use a null-check (e.g.,
Objects.requireNonNull(headers, "headers must not be null") or throw new
IllegalArgumentException("headers must not be null")) before creating the
TreeMap and calling putAll; keep the case-insensitive TreeMap creation and
assignment to the headers field (this.headers = new
TreeMap<>(String.CASE_INSENSITIVE_ORDER); this.headers.putAll(headers)) so
behavior is unchanged for valid inputs but fails fast with a clear error for
null.
In `@src/main/java/org/example/httpparser/HttpParser.java`:
- Around line 22-26: Add an explicit null-check for the instance field reader at
the start of parseHttp (and any other methods that call reader) and throw a
clear IllegalStateException if it hasn't been initialized by setReader;
specifically, in HttpParser.parseHttp verify reader != null and throw new
IllegalStateException("Reader not initialized - call setReader(...) before
parsing") before calling reader.readLine(), and apply the same guard where
reader is used (e.g., other methods around the existing reader usage) to fail
fast instead of allowing a NullPointerException.
In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java`:
- Around line 27-28: The current method validation in HttpParseRequestLine (the
getMethod() check) only allows A-Z and thus rejects valid token methods like
"M-SEARCH"; update the validation regex to accept valid token characters (at
minimum uppercase letters, digits and hyphen) — e.g. replace "^[A-Z]+$" with
something like "^[A-Z0-9-]+$" in the getMethod() validation so methods such as
M-SEARCH are accepted; ensure the change is applied where getMethod() is
validated and keep the same IOException behavior on mismatch.
In `@src/main/java/org/example/StaticFileHandler.java`:
- Line 29: The guard if (uri.isEmpty() || "/".equals(uri)) can NPE when uri is
null; update the check in StaticFileHandler (where uri is used) to first verify
uri != null (e.g., if (uri == null || uri.isEmpty() || "/".equals(uri))) or use
a null-safe utility before calling isEmpty(), ensuring the null check is
performed before any method call on uri.
- Around line 65-71: The sendGetRequest currently calls
response.setContentTypeFromFilename(uri) which uses the requested URI instead of
the actual file served (e.g., index.html or pageNotFound.html), causing wrong
MIME types; change the flow so handleGetRequest returns or exposes the resolved
file path or File (the actual file used to populate fileBytes and statusCode)
and call HttpResponseBuilder.setContentTypeFromFilename with that resolved
filename (or pass the resolved path into sendGetRequest) so the MIME type is
derived from the served file rather than the original uri.
In `@src/main/java/org/example/StaticFileHttpHandler.java`:
- Around line 9-13: The class currently stores a mutable StaticFileHandler in
the field "handler" (initialized in the constructor), which can leak per-request
state across requests; change to create a new StaticFileHandler per request
instead of reusing the field: remove the private final StaticFileHandler handler
field and its constructor initialization, and instantiate a fresh
StaticFileHandler inside the request-handling method (the method that currently
uses "handler" around lines referenced) so each incoming request gets a
request-local handler instance (or alternatively ensure StaticFileHandler is
immutable/stateless before keeping it as a field).
- Around line 37-43: The handler in StaticFileHttpHandler (likely in the handle
method) never sets the Content-Type header on responses; update the code to set
exchange.getResponseHeaders().set("Content-Type", <mime>) before calling
exchange.sendResponseHeaders for both the successful file branch (use
Files.probeContentType(path) or URLConnection.guessContentTypeFromName(fileName)
and fall back to "application/octet-stream" or "text/plain; charset=utf-8" for
unknown types) and the 405 branch (set "text/plain; charset=utf-8"); ensure you
compute/choose the MIME using the request URI / file name and apply it
consistently before writing fileBytes.
In `@src/main/java/org/example/TcpServer.java`:
- Around line 32-33: Wrap and rethrow the original exception as the cause
instead of concatenating it into the message so stack-cause chaining is
preserved; in TcpServer (the client handling catch block inside the method that
handles client connections) replace the current throw new
RuntimeException("Error handling client connection " + e) with constructing the
RuntimeException with the message and the original exception as the cause (i.e.,
new RuntimeException("Error handling client connection", e)) so the original
stack trace and cause are retained.
- Around line 19-23: The accept loop in TcpServer currently spawns an unbounded
virtual thread per connection which can exhaust resources; add admission control
by limiting concurrent connections (e.g., use a Semaphore or a bounded
ExecutorService) around the accept/dispatch logic: acquire a permit before
starting/dispatching to handleClient and release it when the client handler
completes, or reject/close the socket immediately when no permits are available;
update the loop that calls serverSocket.accept(), the
Thread.ofVirtual().start(...) dispatch and the handleClient method to ensure
permits are always released even on error.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 16-26: The step name "Set up JDK ${{ env.JAVA_VERSION }}" may not
render the version because GitHub evaluates step names before the "Get Java
Version" run sets the env; change the step name to a static or literal string
(e.g., "Set up JDK") or move the version resolution into a step output and
reference that output in the name so it is available at evaluation time; update
the step with the existing "Get Java Version" and "Set up JDK ${{
env.JAVA_VERSION }}" identifiers accordingly.
In `@pom.xml`:
- Around line 39-56: The pom currently hardcodes awaitility version 4.3.0 in the
dependency (artifactId awaitility); add a new property (e.g. awaitility.version)
alongside the existing version properties and update the awaitility dependency
to reference ${awaitility.version} so all test deps use properties consistently;
ensure the new property is placed in the <properties> section near other version
properties to keep pom organization consistent.
- Around line 165-185: The Spotless plugin configuration (artifactId:
spotless-maven-plugin) currently enables removeUnusedImports and
formatAnnotations but lacks an explicit formatter; update the
<configuration><java> block that contains removeUnusedImports and
formatAnnotations to include a concrete formatter such as googleJavaFormat() or
palantirJavaFormat() so Spotless enforces a complete style (e.g., add the
googleJavaFormat or palantirJavaFormat entry inside the java configuration
alongside removeUnusedImports/formatAnnotations).
In `@README.md`:
- Line 2: README.md contains markdownlint warnings (MD001/MD019/MD040) around
the heading "### **Class JUV25G**" and several other heading/code blocks; fix by
ensuring proper heading progression (add or adjust a top-level H1 and make
headings nest consistently so "### **Class JUV25G**" is preceded by appropriate
H1/H2), ensure blank lines exist before and after headings, and add language
identifiers to all fenced code blocks (and close any unclosed fences) referenced
in sections around lines indicated (e.g., the "Class JUV25G" heading and blocks
at 37, 39–44, 80–82, 87–89, 134–157, 234–239, 283–290, 340–342) so markdownlint
rules MD001, MD019 and MD040 are satisfied.
In `@src/main/java/org/example/httpparser/HttpParseRequestLine.java`:
- Around line 26-31: HttpParseRequestLine currently calls
setMethod/setUri/setVersion before validating tokens which can leave the object
partially mutated; change parse logic to first validate requestLineArray length
and token formats into local variables (e.g., localMethod, localUri,
localVersion), verify localMethod.matches("^[A-Z]+$") (and any other token
checks), and only after all validations succeed call setMethod(localMethod),
setUri(localUri), and setVersion(localVersion) so the object is assigned
atomically and not left in a partially updated state.
In `@src/main/java/org/example/httpparser/HttpRequest.java`:
- Around line 19-29: The HttpRequest constructor accepts method, path, and
version without validation which can lead to NPEs; update the HttpRequest(String
method, String path, String version, Map<String, String> headers, String body)
constructor to validate required params by calling Objects.requireNonNull (or
equivalent) on method, path, and version with clear messages (e.g., "method must
not be null") before assigning fields, while preserving the existing headers
null-handling (Map.copyOf(headers) or Collections.emptyMap()) and assigning body
as before.
In `@src/main/java/org/example/TcpServer.java`:
- Line 18: The current TcpServer creates a plain ServerSocket (ServerSocket
serverSocket = new ServerSocket(port)) which sends cleartext; for production
terminate TLS at the server edge by either replacing ServerSocket with an
SSLServerSocket from SSLServerSocketFactory (configure a keystore, set TLS
protocols/ciphers, and use server-side authentication in the TcpServer class) or
deploy an external TLS-terminating reverse proxy/load balancer in front of this
listener and keep TcpServer bound to localhost/internal interface; update
connection-accept logic in TcpServer to expect TLS-wrapped sockets (or document
the reverse-proxy requirement) and ensure proper certificate/key management and
TLS configuration.
In `@src/test/java/org/example/AppPortResolutionTest.java`:
- Around line 10-21: Add negative-path unit tests to AppPortResolutionTest
targeting App.resolvePort: add test methods that assert an exception is thrown
for (1) a missing --port value (e.g., args {"--port"}), (2) a non-numeric port
value (e.g., {"--port","abc"}), and (3) out-of-range ports (e.g., {"--port","0"}
and {"--port","65536"}). Use assertThrows to verify the method throws an
appropriate exception (e.g., IllegalArgumentException or NumberFormatException)
and optionally assert the exception message contains relevant text about invalid
port input to make failures clear.
In `@src/test/java/org/example/TestServer.java`:
- Around line 8-11: Replace the hardcoded 8080 in TestServer.java by reading a
configurable port (from a CLI arg or an environment variable) with a safe
integer parse and fallback to 8080; in the main/test entry that creates the
HttpServer (the HttpServer server = HttpServer.create(new
InetSocketAddress(8080), 0) line), compute an int port from e.g. args[0] if
present or System.getenv("TEST_SERVER_PORT"), catch NumberFormatException and
default to 8080, then pass new InetSocketAddress(port) into HttpServer.create
and log the selected port (and keep using StaticFileHttpHandler as before).
In `@www/health.html`:
- Line 1: Add a minimal, proper HTML document wrapper around the existing
<h1>Server is healthy! 😊</h1> so the file uses a doctype and
html/head/body structure like other pages (include a <title> and a basic <meta
charset="utf-8"> in head) and move the existing <h1> into the body; ensure the
resulting document mirrors the structure used in www/index.html for consistency
and maintainability.
In `@www/index.html`:
- Around line 19-35: The .card hover feels abrupt because the .card rule lacks a
transition; update the .card CSS rule to include a transition for the properties
changed on hover (at least transform and box-shadow), e.g., add a transition
declaration to .card so the transform: translateY(-5px) and box-shadow changes
in .card:hover animate smoothly (keep the selector names .card and .card:hover
intact).
In `@www/pageNotFound.html`:
- Around line 48-53: Add a clear recovery action to the 404 page by inserting a
"Back to home" link/button (anchor with href="/") inside the existing container
div (e.g., alongside the <h2> and <p> elements) so users can easily return to
the homepage; style it with a meaningful class name like "back-home" or
"btn-home" to allow existing CSS to apply or to add new rules. Ensure the
element is accessible (use clear text "Back to home", role/button if styled as a
button, and include aria-label if needed) and place it logically beneath the
message so it is visible and usable on all viewports.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
.github/workflows/ci.yml.github/workflows/release.ymlDockerfilePortConfigurationGuide.mdREADME.mdpom.xmlsrc/main/java/org/example/App.javasrc/main/java/org/example/ConnectionHandler.javasrc/main/java/org/example/StaticFileHandler.javasrc/main/java/org/example/StaticFileHttpHandler.javasrc/main/java/org/example/TcpServer.javasrc/main/java/org/example/config/AppConfig.javasrc/main/java/org/example/config/ConfigLoader.javasrc/main/java/org/example/filter/Filter.javasrc/main/java/org/example/filter/FilterChain.javasrc/main/java/org/example/filter/FilterChainImpl.javasrc/main/java/org/example/http/HttpResponseBuilder.javasrc/main/java/org/example/http/MimeTypeDetector.javasrc/main/java/org/example/httpparser/HttpParseRequestLine.javasrc/main/java/org/example/httpparser/HttpParser.javasrc/main/java/org/example/httpparser/HttpRequest.javasrc/main/resources/application.ymlsrc/test/java/org/example/AppPortResolutionTest.javasrc/test/java/org/example/StaticFileHandlerTest.javasrc/test/java/org/example/TestServer.javasrc/test/java/org/example/config/ConfigLoaderTest.javasrc/test/java/org/example/http/HttpResponseBuilderTest.javasrc/test/java/org/example/http/MimeTypeDetectorTest.javasrc/test/java/org/example/httpparser/HttpParseRequestLineTest.javasrc/test/java/org/example/httpparser/HttpParserTest.javawww/health.htmlwww/index.htmlwww/pageNotFound.html
Comment on lines
+11
to
+16
| WORKDIR /app/ | ||
| COPY --from=build /build/target/classes/ /app/ | ||
| COPY --from=build /build/target/dependency/ /app/dependencies/ | ||
| COPY /www/ /www/ | ||
| USER appuser | ||
| ENTRYPOINT ["java", "-classpath", "/app:/app/dependencies/*", "org.example.App"] |
There was a problem hiding this comment.
Runtime paths don’t match app expectations for config and web root.
From Line 11, working dir is /app, but Line 14 copies static files to /www while default config uses ./www. Also App resolves config via src/main/resources/application.yml, which is not copied into that path in the runtime image. This can break static serving/config loading in container runs.
Proposed fix
WORKDIR /app/
COPY --from=build /build/target/classes/ /app/
COPY --from=build /build/target/dependency/ /app/dependencies/
-COPY /www/ /www/
+COPY /www/ /app/www/
+RUN mkdir -p /app/src/main/resources
+COPY src/main/resources/application.yml /app/src/main/resources/application.yml
USER appuser
ENTRYPOINT ["java", "-classpath", "/app:/app/dependencies/*", "org.example.App"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Dockerfile` around lines 11 - 16, The runtime image copies static files to
/www while WORKDIR is /app and org.example.App expects config under
./application.yml and static files under ./www, so align paths: update the
Dockerfile so runtime WORKDIR /app contains both the web root and the packaged
resources—copy the build resources/application.yml (or entire classes/resources
from --from=build) into /app (e.g., COPY --from=build
/build/target/classes/application.yml /app/ or COPY --from=build
/build/target/classes/ /app/) and copy the static web assets into /app/www
(replace COPY /www/ /www/ with COPY /www/ /app/www/); ensure ENTRYPOINT
(["java","-classpath","/app:/app/dependencies/*","org.example.App"]) remains but
now runs with the expected ./application.yml and ./www paths available.
| <maven.compiler.release>23</maven.compiler.release> | ||
| <maven.compiler.release>25</maven.compiler.release> | ||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <junit.jupiter.version>6.0.2</junit.jupiter.version> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if JUnit Jupiter 6.0.2 exists on Maven Central
curl -s "https://search.maven.org/solrsearch/select?q=g:org.junit.jupiter+AND+a:junit-jupiter+AND+v:6.0.2&rows=1&wt=json" | jq '.response.numFound'Repository: ithsjava25/ithsjava25-java-project-webserver-maven-java-template
Length of output: 109
🌐 Web query:
JUnit Jupiter latest version 2025 2026
💡 Result:
As of March 2, 2026, the latest JUnit Jupiter release (GA) is 6.0.3 (released February 15, 2026). [1][2]
If you’re staying on the JUnit 5.x line, the latest JUnit Jupiter release is 5.14.3 (released February 15, 2026). [1][2]
Maven coordinates: org.junit.jupiter:junit-jupiter:6.0.3 [2]
Sources: [1] [2]
Update JUnit Jupiter to a valid version; 6.0.2 does not exist.
JUnit Jupiter 6.0.2 is not available on Maven Central. Use 6.0.3 (latest GA release, February 2026) or 5.14.3 (latest in the 5.x line) instead. The build will fail with dependency resolution error if this is not corrected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pom.xml` at line 14, The pom declares an invalid JUnit Jupiter property
value; update the <junit.jupiter.version> property to a real release (prefer
6.0.3, or use 5.14.3 if you must stay on 5.x) so Maven can resolve the
dependency; change the value of the junit.jupiter.version property and ensure
any dependencies or plugins referencing junit.jupiter.version pick up the new
value, then re-run your build to verify resolution.
Comment on lines
+26
to
+29
| ```yaml | ||
| server: | ||
| port: 9090 | ||
| ``` |
There was a problem hiding this comment.
Fix YAML indentation in the example.
The port key should be indented under server to form a valid nested structure. The current example would be parsed as two separate top-level keys.
📝 Proposed fix
```yaml
server:
-port: 9090
+ port: 9090</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @PortConfigurationGuide.md around lines 26 - 29, The YAML example has
incorrect indentation: the port key must be nested under server; update the
example so that "port" is indented two spaces under "server" (i.e., make
"server" and "port" form a mapping with keys server and port) so the YAML
correctly represents server: { port: 9090 } and parses as a nested structure.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
|
|
||
| *A modern, high-performance HTTP web server built from scratch in Java* | ||
|
|
||
| [Features](#features) • [Quick Start](#quick-start) • [Configuration](#configuration) |
There was a problem hiding this comment.
TOC fragment for Features is not resolving.
Line 12 links to #features, but the heading includes an emoji (Line 18), so the generated anchor differs. Use a stable heading id/anchor to avoid dead nav links.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 12-12: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 12, The table-of-contents link "[Features](`#features`)" is
broken because the actual heading includes an emoji and produces a different
anchor; fix by adding a stable explicit anchor before the heading (e.g., insert
<a id="features"></a> immediately above the Features heading) or remove the
emoji from the heading text so the auto-generated slug matches "#features", and
then ensure the link remains "[Features](`#features`)"; update the README heading
(the heading that currently shows the emoji) or add the explicit anchor to
guarantee the TOC link resolves.
Comment on lines
+48
to
+49
| git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git | ||
| cd project-webserver |
There was a problem hiding this comment.
Quick Start clone commands are broken.
Line 48 repeats git clone, and Line 49 likely cds into the wrong folder name. Copy/paste users will fail immediately.
Proposed fix
-git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git
-cd project-webserver
+git clone https://github.com/ithsjava25/project-webserver-juv25g.git
+cd project-webserver-juv25g📝 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.
Suggested change
| git clone git clone https://github.com/ithsjava25/project-webserver-juv25g.git | |
| cd project-webserver | |
| git clone https://github.com/ithsjava25/project-webserver-juv25g.git | |
| cd project-webserver-juv25g |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 48 - 49, The Quick Start commands in README.md are
wrong: remove the duplicated word in the clone command ("git clone git clone
...") and ensure the cd targets the actual repository folder created by the
clone; replace the two lines with a single "git clone
https://github.com/ithsjava25/project-webserver-juv25g.git" and then "cd
project-webserver-juv25g" so copy/paste users land in the correct directory.
Comment on lines
+65
to
+71
| public void sendGetRequest(OutputStream outputStream, String uri) throws IOException { | ||
| handleGetRequest(uri); | ||
| HttpResponseBuilder response = new HttpResponseBuilder(); | ||
| response.setStatusCode(statusCode); | ||
| // Use MimeTypeDetector instead of hardcoded text/html | ||
| response.setContentTypeFromFilename(uri); | ||
| response.setBody(fileBytes); |
There was a problem hiding this comment.
Set Content-Type from the served file, not the requested URI.
Line 70 uses the incoming uri, but the response body may come from another file (index.html, pageNotFound.html). This can send incorrect MIME types.
🛠️ Suggested change
public class StaticFileHandler {
@@
private byte[] fileBytes;
private int statusCode;
+ private String servedFilename = "";
@@
if (file.isFile()) {
fileBytes = Files.readAllBytes(file.toPath());
statusCode = SC_OK;
+ servedFilename = file.getName();
} else {
File errorFile = new File(WEB_ROOT, "pageNotFound.html");
if (errorFile.isFile()) {
fileBytes = Files.readAllBytes(errorFile.toPath());
+ servedFilename = errorFile.getName();
} else {
fileBytes = "404 Not Found".getBytes(java.nio.charset.StandardCharsets.UTF_8);
+ servedFilename = "not-found.txt";
}
statusCode = SC_NOT_FOUND;
}
@@
- response.setContentTypeFromFilename(uri);
+ response.setContentTypeFromFilename(servedFilename);📝 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.
Suggested change
| public void sendGetRequest(OutputStream outputStream, String uri) throws IOException { | |
| handleGetRequest(uri); | |
| HttpResponseBuilder response = new HttpResponseBuilder(); | |
| response.setStatusCode(statusCode); | |
| // Use MimeTypeDetector instead of hardcoded text/html | |
| response.setContentTypeFromFilename(uri); | |
| response.setBody(fileBytes); | |
| public void sendGetRequest(OutputStream outputStream, String uri) throws IOException { | |
| handleGetRequest(uri); | |
| HttpResponseBuilder response = new HttpResponseBuilder(); | |
| response.setStatusCode(statusCode); | |
| // Use MimeTypeDetector based on the served file, not the requested URI | |
| response.setContentTypeFromFilename(servedFilename); | |
| response.setBody(fileBytes); |
🤖 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 65 - 71, The
sendGetRequest currently calls response.setContentTypeFromFilename(uri) which
uses the requested URI instead of the actual file served (e.g., index.html or
pageNotFound.html), causing wrong MIME types; change the flow so
handleGetRequest returns or exposes the resolved file path or File (the actual
file used to populate fileBytes and statusCode) and call
HttpResponseBuilder.setContentTypeFromFilename with that resolved filename (or
pass the resolved path into sendGetRequest) so the MIME type is derived from the
served file rather than the original uri.
Comment on lines
+9
to
+13
| private final StaticFileHandler handler; | ||
|
|
||
| public StaticFileHttpHandler() { | ||
| this.handler = new StaticFileHandler(); | ||
| } |
There was a problem hiding this comment.
Avoid reusing a mutable StaticFileHandler instance across requests.
Holding one handler in a field risks cross-request state leakage if that handler stores per-request status/body internally.
🔧 Suggested fix (request-local handler)
public class StaticFileHttpHandler implements HttpHandler {
- private final StaticFileHandler handler;
-
public StaticFileHttpHandler() {
- this.handler = new StaticFileHandler();
}
`@Override`
public void handle(HttpExchange exchange) throws IOException {
+ StaticFileHandler handler = new StaticFileHandler();
byte[] fileBytes;
int statusCode;Also applies to: 27-32
🤖 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 9 - 13,
The class currently stores a mutable StaticFileHandler in the field "handler"
(initialized in the constructor), which can leak per-request state across
requests; change to create a new StaticFileHandler per request instead of
reusing the field: remove the private final StaticFileHandler handler field and
its constructor initialization, and instantiate a fresh StaticFileHandler inside
the request-handling method (the method that currently uses "handler" around
lines referenced) so each incoming request gets a request-local handler instance
(or alternatively ensure StaticFileHandler is immutable/stateless before keeping
it as a field).
Comment on lines
+37
to
+43
| exchange.sendResponseHeaders(statusCode, fileBytes.length); | ||
| exchange.getResponseBody().write(fileBytes); | ||
| } else { | ||
| fileBytes = "405 Method Not Allowed".getBytes(); | ||
| statusCode = 405; | ||
| exchange.sendResponseHeaders(statusCode, fileBytes.length); | ||
| exchange.getResponseBody().write(fileBytes); |
There was a problem hiding this comment.
Content-Type header is never set in outgoing responses.
This handler sends status/body only. Static assets should include MIME type headers, otherwise browsers may mis-handle CSS/JS/resources.
🔧 Suggested fix (set response MIME type)
package org.example;
import com.sun.net.httpserver.HttpExchange;
import com.sun.net.httpserver.HttpHandler;
+import org.example.http.MimeTypeDetector;
+import java.nio.charset.StandardCharsets;
import java.io.IOException;
...
public void handle(HttpExchange exchange) throws IOException {
byte[] fileBytes;
int statusCode;
+ String contentType = "text/plain; charset=UTF-8";
String path = exchange.getRequestURI().getPath();
...
if ("GET".equalsIgnoreCase(exchange.getRequestMethod())) {
try {
handler.handleGetRequest(path);
fileBytes = handler.getFileBytes();
statusCode = handler.getStatusCode();
+ contentType = MimeTypeDetector.detectMimeType(path);
} catch (IOException e) {
- fileBytes = "500 Internal Server Error".getBytes();
+ fileBytes = "500 Internal Server Error".getBytes(StandardCharsets.UTF_8);
statusCode = 500;
}
+ exchange.getResponseHeaders().set("Content-Type", contentType);
exchange.sendResponseHeaders(statusCode, fileBytes.length);
exchange.getResponseBody().write(fileBytes);
} else {
- fileBytes = "405 Method Not Allowed".getBytes();
+ fileBytes = "405 Method Not Allowed".getBytes(StandardCharsets.UTF_8);
statusCode = 405;
+ exchange.getResponseHeaders().set("Content-Type", "text/plain; charset=UTF-8");
exchange.sendResponseHeaders(statusCode, fileBytes.length);
exchange.getResponseBody().write(fileBytes);
}🤖 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 - 43,
The handler in StaticFileHttpHandler (likely in the handle method) never sets
the Content-Type header on responses; update the code to set
exchange.getResponseHeaders().set("Content-Type", <mime>) before calling
exchange.sendResponseHeaders for both the successful file branch (use
Files.probeContentType(path) or URLConnection.guessContentTypeFromName(fileName)
and fall back to "application/octet-stream" or "text/plain; charset=utf-8" for
unknown types) and the 405 branch (set "text/plain; charset=utf-8"); ensure you
compute/choose the MIME using the request URI / file name and apply it
consistently before writing fileBytes.
Comment on lines
+19
to
+23
| while (true) { | ||
| Socket clientSocket = serverSocket.accept(); // block | ||
| System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress()); | ||
| Thread.ofVirtual().start(() -> handleClient(clientSocket)); | ||
| } |
There was a problem hiding this comment.
Connection handling is unbounded; add backpressure/admission control.
Spawning a task for every accepted socket without limits can still exhaust resources under high connection rates.
🔧 Suggested fix (simple concurrent-connection cap)
import java.net.ServerSocket;
import java.net.Socket;
+import java.util.concurrent.Semaphore;
public class TcpServer {
private final int port;
+ private final Semaphore concurrentClients = new Semaphore(1000);
...
while (true) {
Socket clientSocket = serverSocket.accept(); // block
System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress());
- Thread.ofVirtual().start(() -> handleClient(clientSocket));
+ if (!concurrentClients.tryAcquire()) {
+ clientSocket.close();
+ continue;
+ }
+ Thread.ofVirtual().start(() -> {
+ try {
+ handleClient(clientSocket);
+ } finally {
+ concurrentClients.release();
+ }
+ });
}📝 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.
Suggested change
| while (true) { | |
| Socket clientSocket = serverSocket.accept(); // block | |
| System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress()); | |
| Thread.ofVirtual().start(() -> handleClient(clientSocket)); | |
| } | |
| import java.net.ServerSocket; | |
| import java.net.Socket; | |
| import java.util.concurrent.Semaphore; | |
| public class TcpServer { | |
| private final int port; | |
| private final Semaphore concurrentClients = new Semaphore(1000); | |
| // ... constructor and other fields ... | |
| public void start() throws IOException { | |
| ServerSocket serverSocket = new ServerSocket(port); | |
| System.out.println("Server listening on port " + port); | |
| while (true) { | |
| Socket clientSocket = serverSocket.accept(); // block | |
| System.out.println("Client connected: " + clientSocket.getRemoteSocketAddress()); | |
| if (!concurrentClients.tryAcquire()) { | |
| clientSocket.close(); | |
| continue; | |
| } | |
| Thread.ofVirtual().start(() -> { | |
| try { | |
| handleClient(clientSocket); | |
| } finally { | |
| concurrentClients.release(); | |
| } | |
| }); | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/TcpServer.java` around lines 19 - 23, The accept
loop in TcpServer currently spawns an unbounded virtual thread per connection
which can exhaust resources; add admission control by limiting concurrent
connections (e.g., use a Semaphore or a bounded ExecutorService) around the
accept/dispatch logic: acquire a permit before starting/dispatching to
handleClient and release it when the client handler completes, or reject/close
the socket immediately when no permits are available; update the loop that calls
serverSocket.accept(), the Thread.ofVirtual().start(...) dispatch and the
handleClient method to ensure permits are always released even on error.
Comment on lines
+32
to
+33
| } catch (Exception e) { | ||
| throw new RuntimeException("Error handling client connection " + e); |
There was a problem hiding this comment.
Preserve exception cause when wrapping client handling failures.
Current wrapping drops stack-cause chaining, which makes production debugging harder.
🔧 Suggested fix
- } catch (Exception e) {
- throw new RuntimeException("Error handling client connection " + e);
+ } catch (Exception e) {
+ throw new RuntimeException("Error handling client connection", e);
}📝 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.
Suggested change
| } catch (Exception e) { | |
| throw new RuntimeException("Error handling client connection " + e); | |
| } catch (Exception e) { | |
| throw new RuntimeException("Error handling client connection", e); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/TcpServer.java` around lines 32 - 33, Wrap and
rethrow the original exception as the cause instead of concatenating it into the
message so stack-cause chaining is preserved; in TcpServer (the client handling
catch block inside the method that handles client connections) replace the
current throw new RuntimeException("Error handling client connection " + e) with
constructing the RuntimeException with the message and the original exception as
the cause (i.e., new RuntimeException("Error handling client connection", e)) so
the original stack trace and cause are retained.
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
17 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 homescreen design with modern styling. Also added health page and navigation button. Improved the layout and overall user experience.
Summary by CodeRabbit
New Features
Security
Infrastructure
Documentation