Conversation
Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
📝 WalkthroughWalkthroughReplaces the minimal static frontend with a full client UI (search, add form, stats, cards, loaders, error/empty states), rewrites JavaScript for card-based rendering and HTTP helpers (GET/POST/DELETE), introduces CSS variables/theme, adds a DELETE /food/{id} endpoint, and includes frontend test harness and a JUnit delete test. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant DOM as HTML/DOM
participant JS as JavaScript
participant API as Backend API
rect rgba(100, 150, 200, 0.5)
Note over User,API: Initial Page Load
User->>DOM: Load page
DOM->>JS: DOMContentLoaded
JS->>JS: loadAllFood()
JS->>JS: showLoading()
JS->>API: GET /food (httpGetAsync)
API-->>JS: JSON array
JS->>JS: hideLoading()
JS->>JS: displayFoodItems(response)
JS->>JS: updateStats(response)
JS->>DOM: Render food cards
DOM-->>User: Display grid & stats
end
rect rgba(150, 200, 100, 0.5)
Note over User,API: Add Item
User->>DOM: Submit add form
DOM->>JS: addItem(event)
JS->>JS: validate & prepare body
JS->>API: POST /food (httpPostAsync)
API-->>JS: Success (201/200)
JS->>JS: clearForm()
JS->>JS: loadAllFood()
JS->>JS: showSuccess()
JS->>DOM: Update grid
end
rect rgba(200, 150, 100, 0.5)
Note over User,API: Delete Item
User->>DOM: Click delete
DOM->>JS: deleteItem(id)
JS->>JS: confirm()
JS->>API: DELETE /food/{id} (httpDeleteAsync)
API-->>JS: Success (204)
JS->>JS: loadAllFood()
JS->>JS: showSuccess()
JS->>DOM: Update grid
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/resources/META-INF/resources/styles.css (1)
99-152: Add explicit keyboard focus styling for buttons.Buttons have hover styling, but an explicit
:focus-visiblestate will improve consistent keyboard accessibility across browsers/themes.♿ Suggested CSS tweak
.btn { padding: 0.75rem 1.5rem; border: none; border-radius: var(--border-radius); font-size: 1rem; font-weight: 600; cursor: pointer; transition: all 0.3s ease; white-space: nowrap; } + +.btn:focus-visible { + outline: 3px solid rgba(79, 70, 229, 0.35); + outline-offset: 2px; +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/META-INF/resources/styles.css` around lines 99 - 152, Buttons currently only have hover styles; add explicit :focus-visible rules to preserve keyboard accessibility by providing a visible focus ring and matching interaction state. For the base .btn add a .btn:focus-visible rule (and per-variant .btn-primary:focus-visible, .btn-secondary:focus-visible, .btn-success:focus-visible, .btn-danger:focus-visible) that applies a clear high-contrast outline or focus ring (e.g., 3px solid or box-shadow using a color variable), include the same transform/translateY(-2px) and box-shadow used on :hover for visual consistency, and ensure outline: none is not suppressing keyboard focus; use :focus-visible specifically so mouse users aren’t affected.
🤖 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/resources/META-INF/resources/script.js`:
- Around line 149-156: The frontend calls httpDeleteAsync(foodEndpoint + '/' +
id) to hit DELETE /food/{id} but the backend class FoodResource (no `@DELETE`
method exists) so implement a matching delete endpoint: add a method annotated
with `@DELETE` and `@Path`("{id}") in FoodResource (e.g., deleteFood with
`@PathParam`("id") String id) that performs the removal and returns an appropriate
Response (200/204 on success, 404 if not found, 500 on error), or alternatively
disable/remove the UI delete action in script.js (httpDeleteAsync call) until
the backend route is added; ensure method name and annotations in FoodResource
match the route used by the frontend.
- Around line 257-266: The escapeHtml function (escapeHtml) is not null-safe and
will throw when text is null/undefined; update it to handle null/undefined
inputs (e.g., coerce with String(text) or return '' for nullish values) before
calling replace so it never calls text.replace on a non-string, and keep the
existing replacement map and regex unchanged.
- Around line 50-52: The call to JSON.parse in displayFoodItems can throw and
crash the UI; wrap the parse in a try/catch inside the displayFoodItems function
(which assigns to allFoodItems) and on parse failure handle it gracefully —
e.g., log the error (console.error or existing logger), set allFoodItems to a
safe default (empty array or previous value), and update the UI to indicate a
load/error state rather than letting an exception propagate.
---
Nitpick comments:
In `@src/main/resources/META-INF/resources/styles.css`:
- Around line 99-152: Buttons currently only have hover styles; add explicit
:focus-visible rules to preserve keyboard accessibility by providing a visible
focus ring and matching interaction state. For the base .btn add a
.btn:focus-visible rule (and per-variant .btn-primary:focus-visible,
.btn-secondary:focus-visible, .btn-success:focus-visible,
.btn-danger:focus-visible) that applies a clear high-contrast outline or focus
ring (e.g., 3px solid or box-shadow using a color variable), include the same
transform/translateY(-2px) and box-shadow used on :hover for visual consistency,
and ensure outline: none is not suppressing keyboard focus; use :focus-visible
specifically so mouse users aren’t affected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aae831e9-94f1-4c1e-bc72-3dc64781296c
📒 Files selected for processing (3)
src/main/resources/META-INF/resources/index.htmlsrc/main/resources/META-INF/resources/script.jssrc/main/resources/META-INF/resources/styles.css
Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/resources/META-INF/resources/script.js`:
- Around line 42-44: The error callback currently always calls showError('No
items found for restaurant: ' + input) which misleads users for network/server
errors; update the anonymous error handler (the (error) => { ... } callback that
calls hideLoading and showError) to branch on the error status (e.g.,
error.status or error.response?.status) and show "No items found for restaurant:
<input>" only for 404, otherwise show a different message like "Server error" or
include the error.message for non-404/network failures so users see accurate
feedback.
In `@src/main/resources/META-INF/resources/test.html`:
- Around line 9-15: The test HTML is missing DOM fixtures referenced by
script.js during app initialization (loadAllFood() on DOMContentLoaded) —
specifically add an input element with id "myInput" and a loading container with
id "loading" (and ensure they appear before the script runs) so that calls in
loadAllFood() and any loading helpers don't throw; update the test.html markup
to include <input id="myInput"> and a visible/hidden element with id "loading"
alongside the existing `#error`, `#foodGrid`, `#emptyState`, `#totalItems`,
`#totalRestaurants`, and `#avgPrice` so tests initialize reliably.
- Around line 58-63: The test for escapeHtml (Test case 4) asserts the wrong
expected value: it currently expects the original unescaped string while the
assertion message says it should be escaped; update the assertion to expect the
escaped HTML output from escapeHtml('<script>alert("xss")</script>') (i.e., the
HTML-escaped form) so the test correctly verifies escaping behavior in the
escapeHtml function and prevents masking regressions.
In `@src/test/java/org/eclipse/che/incubator/food/FoodEndpointTest.java`:
- Around line 74-97: The testDelete() in FoodEndpointTest mutates seeded
resource /food/1 and asserts a hardcoded list size; change it to be
self-contained by creating its own record, deleting that record, and asserting
deletion and list-size change relative to the pre-delete state. Specifically, in
testDelete() call POST /food to create a Food, extract the created id (or
Location), optionally get the list size before deletion, then DELETE the created
resource using the extracted id, assert the DELETE returns 204 and that a
subsequent GET for that id returns 404/204 as your API defines, and finally
verify the list length decreased by one by comparing the pre-delete and
post-delete arrays rather than using a hardcoded value; do not touch /food/1 or
any seeded ids.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c018cd30-d26f-48bf-81dd-12bd4d0a45b4
📒 Files selected for processing (4)
src/main/java/org/eclipse/che/incubator/food/FoodResource.javasrc/main/resources/META-INF/resources/script.jssrc/main/resources/META-INF/resources/test.htmlsrc/test/java/org/eclipse/che/incubator/food/FoodEndpointTest.java
| (error) => { | ||
| hideLoading(); | ||
| showError('No items found for restaurant: ' + input); |
There was a problem hiding this comment.
Search error message is misleading for non-404 failures.
The callback always shows “No items found,” even for server/network errors. Consider branching on status code to keep user feedback accurate.
🎯 Suggested status-aware messaging
- }, (error) => {
+ }, (error) => {
hideLoading();
- showError('No items found for restaurant: ' + input);
+ if (error === 404) {
+ showError('No items found for restaurant: ' + input);
+ } else {
+ showError('Search failed. Please try again.');
+ }
}📝 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.
| (error) => { | |
| hideLoading(); | |
| showError('No items found for restaurant: ' + input); | |
| }, (error) => { | |
| hideLoading(); | |
| if (error && error.status === 404) { | |
| showError('No items found for restaurant: ' + input); | |
| } else { | |
| showError('Search failed. Please try again.'); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/META-INF/resources/script.js` around lines 42 - 44, The
error callback currently always calls showError('No items found for restaurant:
' + input) which misleads users for network/server errors; update the anonymous
error handler (the (error) => { ... } callback that calls hideLoading and
showError) to branch on the error status (e.g., error.status or
error.response?.status) and show "No items found for restaurant: <input>" only
for 404, otherwise show a different message like "Server error" or include the
error.message for non-404/network failures so users see accurate feedback.
| <div id="error" class="hidden"></div> | ||
| <div id="foodGrid"></div> | ||
| <div id="emptyState" class="hidden"></div> | ||
| <div id="totalItems"></div> | ||
| <div id="totalRestaurants"></div> | ||
| <div id="avgPrice"></div> | ||
|
|
There was a problem hiding this comment.
Harness is missing required DOM nodes used during app initialization.
script.js auto-runs loadAllFood() on DOMContentLoaded; Line 13 in script.js accesses #myInput, and loading helpers reference #loading. Their absence can throw before tests run.
🧩 Add missing test DOM fixtures
<div id="error" class="hidden"></div>
+ <input id="myInput" value="" />
+ <div id="loading" class="hidden"></div>
<div id="foodGrid"></div>
<div id="emptyState" class="hidden"></div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/META-INF/resources/test.html` around lines 9 - 15, The
test HTML is missing DOM fixtures referenced by script.js during app
initialization (loadAllFood() on DOMContentLoaded) — specifically add an input
element with id "myInput" and a loading container with id "loading" (and ensure
they appear before the script runs) so that calls in loadAllFood() and any
loading helpers don't throw; update the test.html markup to include <input
id="myInput"> and a visible/hidden element with id "loading" alongside the
existing `#error`, `#foodGrid`, `#emptyState`, `#totalItems`, `#totalRestaurants`, and
`#avgPrice` so tests initialize reliably.
| // Test case 4: escapeHtml with a valid string | ||
| try { | ||
| console.log("Test 4: escapeHtml with a valid string"); | ||
| const result = escapeHtml('<script>alert("xss")<\\/script>'); | ||
| console.assert(result === '<script>alert("xss")<\\/script>', "Test 4 Failed: HTML should be escaped"); | ||
| } catch (e) { |
There was a problem hiding this comment.
XSS test assertion is inverted.
Line 62 currently expects unescaped HTML, but the assertion message says it should be escaped. This can hide a real escaping regression.
✅ Correct expected escaped value
- const result = escapeHtml('<script>alert("xss")<\\/script>');
- console.assert(result === '<script>alert("xss")<\\/script>', "Test 4 Failed: HTML should be escaped");
+ const result = escapeHtml('<script>alert("xss")<\\/script>');
+ console.assert(
+ result === '<script>alert("xss")<\\/script>',
+ "Test 4 Failed: HTML should be escaped"
+ );📝 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.
| // Test case 4: escapeHtml with a valid string | |
| try { | |
| console.log("Test 4: escapeHtml with a valid string"); | |
| const result = escapeHtml('<script>alert("xss")<\\/script>'); | |
| console.assert(result === '<script>alert("xss")<\\/script>', "Test 4 Failed: HTML should be escaped"); | |
| } catch (e) { | |
| // Test case 4: escapeHtml with a valid string | |
| try { | |
| console.log("Test 4: escapeHtml with a valid string"); | |
| const result = escapeHtml('<script>alert("xss")<\\/script>'); | |
| console.assert( | |
| result === '<script>alert("xss")<\\/script>', | |
| "Test 4 Failed: HTML should be escaped" | |
| ); | |
| } catch (e) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/META-INF/resources/test.html` around lines 58 - 63, The
test for escapeHtml (Test case 4) asserts the wrong expected value: it currently
expects the original unescaped string while the assertion message says it should
be escaped; update the assertion to expect the escaped HTML output from
escapeHtml('<script>alert("xss")</script>') (i.e., the HTML-escaped form) so the
test correctly verifies escaping behavior in the escapeHtml function and
prevents masking regressions.
| @Test | ||
| public void testDelete() { | ||
| // Given a food exists | ||
| given() | ||
| .when().get("/food/1") | ||
| .then().statusCode(200); | ||
|
|
||
| // When we delete it | ||
| given() | ||
| .when().delete("/food/1") | ||
| .then().statusCode(204); | ||
|
|
||
| // Then it is gone | ||
| given() | ||
| .when().get("/food/1") | ||
| .then().statusCode(204); // No content because it was deleted | ||
|
|
||
| // And the list is smaller | ||
| Food[] foods = given() | ||
| .when().get("/food") | ||
| .then().statusCode(200) | ||
| .extract().as(Food[].class); | ||
| assertEquals(4, foods.length); | ||
| } |
There was a problem hiding this comment.
testDelete() is stateful and can make the suite flaky.
This test permanently mutates seeded data (/food/1) and asserts a hardcoded post-delete size. That can break other tests depending on execution order. Prefer creating and deleting a test-owned record within the same test.
💡 Suggested isolated test flow
`@Test`
public void testDelete() {
- // Given a food exists
- given()
- .when().get("/food/1")
- .then().statusCode(200);
-
- // When we delete it
- given()
- .when().delete("/food/1")
- .then().statusCode(204);
-
- // Then it is gone
- given()
- .when().get("/food/1")
- .then().statusCode(204); // No content because it was deleted
-
- // And the list is smaller
- Food[] foods = given()
- .when().get("/food")
- .then().statusCode(200)
- .extract().as(Food[].class);
- assertEquals(4, foods.length);
+ Food[] before = given()
+ .when().get("/food")
+ .then().statusCode(200)
+ .extract().as(Food[].class);
+
+ String location = given()
+ .contentType("application/json")
+ .body("{\"name\":\"Temp\",\"restaurantName\":\"Test\",\"price\":1.23}")
+ .when().post("/food")
+ .then().statusCode(201)
+ .extract().header("Location");
+
+ String id = location.substring(location.lastIndexOf('/') + 1);
+
+ given()
+ .when().delete("/food/" + id)
+ .then().statusCode(204);
+
+ given()
+ .when().get("/food/" + id)
+ .then().statusCode(204);
+
+ Food[] after = given()
+ .when().get("/food")
+ .then().statusCode(200)
+ .extract().as(Food[].class);
+ assertEquals(before.length, after.length);
}📝 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.
| @Test | |
| public void testDelete() { | |
| // Given a food exists | |
| given() | |
| .when().get("/food/1") | |
| .then().statusCode(200); | |
| // When we delete it | |
| given() | |
| .when().delete("/food/1") | |
| .then().statusCode(204); | |
| // Then it is gone | |
| given() | |
| .when().get("/food/1") | |
| .then().statusCode(204); // No content because it was deleted | |
| // And the list is smaller | |
| Food[] foods = given() | |
| .when().get("/food") | |
| .then().statusCode(200) | |
| .extract().as(Food[].class); | |
| assertEquals(4, foods.length); | |
| } | |
| `@Test` | |
| public void testDelete() { | |
| Food[] before = given() | |
| .when().get("/food") | |
| .then().statusCode(200) | |
| .extract().as(Food[].class); | |
| String location = given() | |
| .contentType("application/json") | |
| .body("{\"name\":\"Temp\",\"restaurantName\":\"Test\",\"price\":1.23}") | |
| .when().post("/food") | |
| .then().statusCode(201) | |
| .extract().header("Location"); | |
| String id = location.substring(location.lastIndexOf('/') + 1); | |
| given() | |
| .when().delete("/food/" + id) | |
| .then().statusCode(204); | |
| given() | |
| .when().get("/food/" + id) | |
| .then().statusCode(204); | |
| Food[] after = given() | |
| .when().get("/food") | |
| .then().statusCode(200) | |
| .extract().as(Food[].class); | |
| assertEquals(before.length, after.length); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/org/eclipse/che/incubator/food/FoodEndpointTest.java` around
lines 74 - 97, The testDelete() in FoodEndpointTest mutates seeded resource
/food/1 and asserts a hardcoded list size; change it to be self-contained by
creating its own record, deleting that record, and asserting deletion and
list-size change relative to the pre-delete state. Specifically, in testDelete()
call POST /food to create a Food, extract the created id (or Location),
optionally get the list size before deletion, then DELETE the created resource
using the extracted id, assert the DELETE returns 204 and that a subsequent GET
for that id returns 404/204 as your API defines, and finally verify the list
length decreased by one by comparing the pre-delete and post-delete arrays
rather than using a hardcoded value; do not touch /food/1 or any seeded ids.
Summary by CodeRabbit
New Features
Style
Tests