Issue/69 remove html concat - #73
Merged
Merged
Conversation
Closed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a webRoot field and a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client (Socket)
participant Conn as ConnectionHandler
participant SFH as StaticFileHandler
participant FS as FileSystem
Client->>Conn: send HTTP request (e.g., "GET /" or "GET /image.jpg")
Conn->>Conn: resolveTargetFile(uri) -> target ("/" -> "index.html", strip leading '/')
Conn->>SFH: initialize/request file (pass webRoot if provided)
SFH->>FS: read target file from filesystem
FS-->>SFH: file bytes or not found
SFH-->>Conn: response bytes (200 or 404)
Conn-->>Client: write HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/example/ConnectionHandler.java (2)
26-41: Consider simplifying the conditionalStaticFileHandlerinstantiation.Minor readability improvement — a ternary or a single constructor with a default could reduce the branching here.
Suggested simplification
public void runConnectionHandler() throws IOException { - StaticFileHandler sfh; - - if (webRoot != null) { - sfh = new StaticFileHandler(webRoot); - } else { - sfh = new StaticFileHandler(); - } + StaticFileHandler sfh = (webRoot != null) + ? new StaticFileHandler(webRoot) + : 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/ConnectionHandler.java` around lines 26 - 41, The runConnectionHandler method has a redundant if/else when creating StaticFileHandler; replace the branching with a single instantiation using a conditional expression or by calling a constructor that accepts webRoot (e.g., assign sfh = (webRoot != null) ? new StaticFileHandler(webRoot) : new StaticFileHandler()) or update StaticFileHandler to treat a null webRoot as the default so you can always call new StaticFileHandler(webRoot); update references in runConnectionHandler to use the new single-assignment for sfh.
14-24: Constructor comments in Swedish; minor style nit.Consider using English for code comments to keep the codebase consistent and accessible to all contributors (e.g., "Original constructor" and "Constructor for testing").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/ConnectionHandler.java` around lines 14 - 24, Change the Swedish comments above the two constructors in class ConnectionHandler to English for consistency: replace "// Original konstruktor" with "// Original constructor" and "// Ny konstruktor (för testning)" with "// Constructor for testing" so the comments sit directly above the corresponding constructors ConnectionHandler(Socket) and ConnectionHandler(Socket, String).src/test/java/org/example/ConnectionHandlerTest.java (1)
28-52: Well-structured test covering the core fix of this PR.The mock setup and temp-file approach cleanly validate that non-HTML static files are served with a 200 rather than a 404. A couple of optional suggestions:
outputStream.toString()(Line 48) uses the platform default charset. ConsideroutputStream.toString(StandardCharsets.UTF_8)for explicitness.- Additional coverage: A test for
GET / HTTP/1.1resolving toindex.htmlwould lock down the remaining branch inresolveTargetFile.,
Minor charset fix
- String response = outputStream.toString(); + String response = outputStream.toString(StandardCharsets.UTF_8);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/ConnectionHandlerTest.java` around lines 28 - 52, Change the test to use an explicit charset when reading the response by replacing outputStream.toString() with outputStream.toString(StandardCharsets.UTF_8) in ConnectionHandlerTest to avoid platform-dependent decoding, and add a second test that sends "GET / HTTP/1.1" through the same mock socket setup and asserts the response contains "HTTP/1.1 200 OK" (verifying ConnectionHandler.resolveTargetFile resolves to index.html) so the branch that maps "/" to "index.html" is covered.
🤖 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/ConnectionHandler.java`:
- Around line 43-49: In resolveTargetFile, replace the regex check
uri.matches("/$") with the simpler "/".equals(uri) and set this.uri to
"index.html" for that case; for the non-root branch ensure you strip any leading
'/' (e.g., if uri.startsWith("/") then use uri.substring(1)) before assigning to
this.uri so StaticFileHandler.handleGetRequest() will resolve files relative to
the configured root rather than treating them as absolute paths.
---
Nitpick comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 26-41: The runConnectionHandler method has a redundant if/else
when creating StaticFileHandler; replace the branching with a single
instantiation using a conditional expression or by calling a constructor that
accepts webRoot (e.g., assign sfh = (webRoot != null) ? new
StaticFileHandler(webRoot) : new StaticFileHandler()) or update
StaticFileHandler to treat a null webRoot as the default so you can always call
new StaticFileHandler(webRoot); update references in runConnectionHandler to use
the new single-assignment for sfh.
- Around line 14-24: Change the Swedish comments above the two constructors in
class ConnectionHandler to English for consistency: replace "// Original
konstruktor" with "// Original constructor" and "// Ny konstruktor (för
testning)" with "// Constructor for testing" so the comments sit directly above
the corresponding constructors ConnectionHandler(Socket) and
ConnectionHandler(Socket, String).
In `@src/test/java/org/example/ConnectionHandlerTest.java`:
- Around line 28-52: Change the test to use an explicit charset when reading the
response by replacing outputStream.toString() with
outputStream.toString(StandardCharsets.UTF_8) in ConnectionHandlerTest to avoid
platform-dependent decoding, and add a second test that sends "GET / HTTP/1.1"
through the same mock socket setup and asserts the response contains "HTTP/1.1
200 OK" (verifying ConnectionHandler.resolveTargetFile resolves to index.html)
so the branch that maps "/" to "index.html" is covered.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/example/ConnectionHandler.java (2)
10-12: Consider making fieldsprivate(andfinalwhere possible).
client,uri, andwebRootare all package-private.clientandwebRootare set only in constructors and never reassigned, soprivate finalfits naturally.uriis reassigned viaresolveTargetFile, so justprivateis appropriate.♻️ Proposed refactor
- Socket client; - String uri; - String webRoot; + private final Socket client; + private String uri; + private final String webRoot;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/ConnectionHandler.java` around lines 10 - 12, The fields in ConnectionHandler are package-private but should be encapsulated: change the declarations of client, uri, and webRoot to private; make client and webRoot immutable by marking them private final (since they are only set in constructors) and keep uri as private (non-final) because it is updated by resolveTargetFile; update any direct accesses from within the class (no change) and adjust external accessors or tests to use new getters if needed.
14-14: Replace Swedish inline comments with English.
// Original konstruktorand// Ny konstruktor (för testning)are in Swedish. The rest of the codebase appears to use English; keeping comments in one language improves consistency for all contributors.♻️ Proposed change
- // Original konstruktor + // Default constructor public ConnectionHandler(Socket client) {- // Ny konstruktor (för testning) + // Constructor for testing with a configurable web root public ConnectionHandler(Socket client, String webRoot) {Also applies to: 20-21
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/ConnectionHandler.java` at line 14, Replace the Swedish inline comments in ConnectionHandler with English equivalents: change "// Original konstruktor" to "// Original constructor" and "// Ny konstruktor (för testning)" to "// New constructor (for testing)"; update any other occurrences on the same block (lines referenced 20-21) so the constructor comments inside the ConnectionHandler class are consistent with the rest of the codebase.
🤖 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/ConnectionHandler.java`:
- Around line 43-49: resolveTargetFile can NPE if the incoming uri is null; add
a null guard at the start of resolveTargetFile to handle null inputs (e.g.,
treat null the same as "/" and set this.uri to "index.html" or another sane
default) and ensure you only call uri.startsWith("/") after confirming uri is
non-null; update resolveTargetFile accordingly so it safely sets this.uri
without throwing on null.
---
Nitpick comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 10-12: The fields in ConnectionHandler are package-private but
should be encapsulated: change the declarations of client, uri, and webRoot to
private; make client and webRoot immutable by marking them private final (since
they are only set in constructors) and keep uri as private (non-final) because
it is updated by resolveTargetFile; update any direct accesses from within the
class (no change) and adjust external accessors or tests to use new getters if
needed.
- Line 14: Replace the Swedish inline comments in ConnectionHandler with English
equivalents: change "// Original konstruktor" to "// Original constructor" and
"// Ny konstruktor (för testning)" to "// New constructor (for testing)"; update
any other occurrences on the same block (lines referenced 20-21) so the
constructor comments inside the ConnectionHandler class are consistent with the
rest of the codebase.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/org/example/ConnectionHandler.java (2)
14-24: LGTM — constructors are clean; consider English for inline comments.The two-constructor design correctly isolates the test-injectable
webRootpath. The Swedish comments (// Original konstruktor,// Ny konstruktor (för testning)) work fine locally but can confuse contributors unfamiliar with the language.✏️ Optional: translate comments to English
- // Original konstruktor + // Default constructor — uses ClassLoader resource root public ConnectionHandler(Socket client) { this.client = client; this.webRoot = null; } - // Ny konstruktor (för testning) + // Constructor for testing — resolves static files from the given webRoot public ConnectionHandler(Socket client, String webRoot) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/ConnectionHandler.java` around lines 14 - 24, Replace the Swedish inline comments above the two constructors in ConnectionHandler with English equivalents to improve clarity for all contributors; specifically change the comment for the single-arg constructor and the two-arg test constructor (the methods named ConnectionHandler(Socket) and ConnectionHandler(Socket, String)) to something like "// Original constructor" and "// New constructor (for testing)" or similar clear English descriptions.
27-33: Consider collapsing to a ternary for conciseness.Both constructors are properly defined in
StaticFileHandler.java(no-arg at line 16 and String-accepting at line 21), so the code will compile correctly.The if-else block at lines 27-33 can be simplified using a ternary operator:
Suggested ternary refactoring
- StaticFileHandler sfh; - - if (webRoot != null) { - sfh = new StaticFileHandler(webRoot); - } else { - sfh = new StaticFileHandler(); - } + StaticFileHandler sfh = (webRoot != null) + ? new StaticFileHandler(webRoot) + : 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/ConnectionHandler.java` around lines 27 - 33, Replace the if-else assignment of sfh with a concise ternary expression: assign sfh using the webRoot ? new StaticFileHandler(webRoot) : new StaticFileHandler() pattern so the Conditional creates the correct StaticFileHandler instance; update the code in ConnectionHandler where sfh is declared and initialized to use this ternary form referencing the sfh variable, webRoot, and StaticFileHandler constructors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Line 44: The null-check in ConnectionHandler for the uri variable (if (uri ==
null || "/".equals(uri))) properly prevents a NullPointerException, so leave
this condition as-is; no code changes required to the ConnectionHandler class or
the uri handling logic.
---
Nitpick comments:
In `@src/main/java/org/example/ConnectionHandler.java`:
- Around line 14-24: Replace the Swedish inline comments above the two
constructors in ConnectionHandler with English equivalents to improve clarity
for all contributors; specifically change the comment for the single-arg
constructor and the two-arg test constructor (the methods named
ConnectionHandler(Socket) and ConnectionHandler(Socket, String)) to something
like "// Original constructor" and "// New constructor (for testing)" or similar
clear English descriptions.
- Around line 27-33: Replace the if-else assignment of sfh with a concise
ternary expression: assign sfh using the webRoot ? new
StaticFileHandler(webRoot) : new StaticFileHandler() pattern so the Conditional
creates the correct StaticFileHandler instance; update the code in
ConnectionHandler where sfh is declared and initialized to use this ternary form
referencing the sfh variable, webRoot, and StaticFileHandler constructors.
MartinStenhagen
self-requested a review
February 20, 2026 11:36
MartinStenhagen
previously approved these changes
Feb 20, 2026
MartinStenhagen
left a comment
MartinStenhagen
left a comment
There was a problem hiding this comment.
Ser ut som att det löser problemet i issuen med relativt lite ändring av koden, bra jobbat!
eeebbaandersson
self-requested a review
February 20, 2026 15:35
eeebbaandersson
previously approved these changes
Feb 20, 2026
eeebbaandersson
left a comment
eeebbaandersson
left a comment
There was a problem hiding this comment.
Snygg uppdatering av koden och bra jobbat med testet som bekräftar den nya logiken!:)
Rickank
dismissed stale reviews from eeebbaandersson and MartinStenhagen
via
February 25, 2026 16:13
6389a09
Rickank
force-pushed
the
issue/69-remove-html-concat
branch
from
February 25, 2026 16:13
119afd6 to
6389a09
Compare
eeebbaandersson
self-requested a review
February 25, 2026 16:15
eeebbaandersson
approved these changes
Feb 25, 2026
This was referenced Feb 26, 2026
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
4 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.
Summary by CodeRabbit
New Features
Changes
Tests