Skip to content

feat: update UI#56

Merged
dkwon17 merged 2 commits intomainfrom
new-ui
Apr 7, 2026
Merged

feat: update UI#56
dkwon17 merged 2 commits intomainfrom
new-ui

Conversation

@ibuziuk
Copy link
Copy Markdown
Collaborator

@ibuziuk ibuziuk commented Mar 13, 2026

Contribute

Screenshot 2026-03-13 at 11 50 42

Summary by CodeRabbit

  • New Features

    • Redesigned interface with header, search/filter, add-item form, item cards, empty-state and footer.
    • Item creation and deletion from the UI with loading, success and error feedback.
    • Live stats panel (total items, unique restaurants, average price).
  • Style

    • New theme and component-based styling with responsive layout and UI tokens.
  • Tests

    • Added frontend test harness and backend test validating item deletion.

Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
@ibuziuk ibuziuk requested a review from dkwon17 as a code owner March 13, 2026 10:39
@github-actions
Copy link
Copy Markdown

Click here to review and test in web IDE: Contribute

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 13, 2026

📝 Walkthrough

Walkthrough

Replaces 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

Cohort / File(s) Summary
HTML: main UI
src/main/resources/META-INF/resources/index.html
Replaced simple scaffold with full UI: header, search/filter, add-item form, loading indicator, error area, stats grid, food items grid, empty state, and footer.
JavaScript: client logic
src/main/resources/META-INF/resources/script.js
Rewrote from table-based to card-based rendering; added global allFoodItems, page init (DOMContentLoadedloadAllFood), displayFoodItems, createFoodCard, updateStats, addItem(event), deleteItem(id), form utilities, loading/error/success UI helpers, escapeHtml, and expanded HTTP helpers (httpGetAsync, httpPostAsync, httpDeleteAsync). Removed legacy table rendering and input helper functions.
CSS: theming & layout
src/main/resources/META-INF/resources/styles.css
Replaced stylesheet with CSS variable–based design system: tokens for colors, radii, shadows; styled header, main container, search/add/stats/items sections, responsive rules, buttons/inputs/cards, loader, error/empty states, and footer.
Backend: delete endpoint
src/main/java/org/eclipse/che/incubator/food/FoodResource.java
Added @DELETE @path("/{id}") handler public Response delete(@PathParam("id") Long id) — transactional, returns 404 if not found, deletes entity and returns 204 on success.
Tests & harness
src/main/resources/META-INF/resources/test.html, src/test/java/org/eclipse/che/incubator/food/FoodEndpointTest.java
Added test.html frontend harness with placeholder helpers and four small display/escapeHtml tests. Added JUnit testDelete() verifying GET/DELETE/GET sequence for /food/1 and overall collection size decrement.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hopped through HTML, CSS, and script,
Cards replaced tables with a jaunty skip,
Fetches and deletes now dance in a row,
Stats glow bright and errors show,
A rabbit cheers — the UI's aglow! 🎉

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.27% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'feat: update UI' is vague and overly generic, using non-descriptive language that does not convey meaningful information about the specific changes made. Consider a more specific title that highlights the primary change, such as 'feat: redesign food manager UI with card-based layout and improved search functionality' or similar.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch new-ui

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-visible state 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

📥 Commits

Reviewing files that changed from the base of the PR and between 0f030bf and 8b6a5c6.

📒 Files selected for processing (3)
  • src/main/resources/META-INF/resources/index.html
  • src/main/resources/META-INF/resources/script.js
  • src/main/resources/META-INF/resources/styles.css

Comment thread src/main/resources/META-INF/resources/script.js
Comment thread src/main/resources/META-INF/resources/script.js
Comment thread src/main/resources/META-INF/resources/script.js
@ibuziuk ibuziuk changed the title feat: udpate UI feat: update UI Mar 13, 2026
Signed-off-by: Ilya Buziuk <ibuziuk@redhat.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 8b6a5c6 and 877dc2d.

📒 Files selected for processing (4)
  • src/main/java/org/eclipse/che/incubator/food/FoodResource.java
  • src/main/resources/META-INF/resources/script.js
  • src/main/resources/META-INF/resources/test.html
  • src/test/java/org/eclipse/che/incubator/food/FoodEndpointTest.java

Comment on lines +42 to +44
(error) => {
hideLoading();
showError('No items found for restaurant: ' + input);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
(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.

Comment on lines +9 to +15
<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>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Comment on lines +58 to +63
// 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) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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 === '&lt;script&gt;alert(&quot;xss&quot;)&lt;\\/script&gt;',
+                    "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.

Suggested change
// 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 === '&lt;script&gt;alert(&quot;xss&quot;)&lt;\\/script&gt;',
"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.

Comment on lines +74 to +97
@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);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Suggested change
@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.

@dkwon17 dkwon17 merged commit fcaf54d into main Apr 7, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants