Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical and moderate issues remain unresolved across Docker packaging, HTML safety, request ordering, and search rendering.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a browser-based KBBI UI at /ui, integrated with Fx and packaged through Docker.
Changes:
- Registers and serves the UI handler.
- Adds search, suggestions, random word, and word-of-the-day features.
- Minifies and copies the UI into the Docker image.
File summaries
| File | Changes | Findings |
|---|---|---|
internal/ui/uifx/fx.go |
Registers the UI module with Fx. | None reported. |
internal/ui/controller.go |
Serves static UI assets at /ui. |
None reported. |
Dockerfile |
Minifies and copies the UI into the image. | Line 5 — critical (2 votes): .dockerignore excludes /assets, causing the COPY to fail. |
cmd/kbbi/main.go |
Loads the UI module. | None reported. |
assets/view/index.html |
Implements the dictionary frontend. | Lines 457 — critical (3 votes): unescaped output is inserted into innerHTML.Lines 527/596 — moderate (2 votes): stale in-flight requests can overwrite newer results. Line 421 — nit (2 votes): search input lacks an accessible name. Line 596 — moderate (1 vote): forcing raw=true can render blank meanings with the sample asset.Line 599 — moderate (1 vote): all failures are reported as “word not found.” Line 654 — nit (1 vote): non-keyboard-accessible anchor controls are used. |
Review details
Suppressed comments (4)
assets/view/index.html:598
- The result area is shared by all of the asynchronous actions, but completions are not versioned or cancelled. A search followed quickly by another search, Random, or Word of the Day can therefore have its older response replace the newer result and push the wrong
kataURL. Track a shared action/request token and ignore stale completions.
const data = await fetchJson(`${API_BASE}/entry/${encodeURIComponent(entry)}?raw=true`);
renderLemma(data);
if (updateHistory) updateUrl(data.lemma || entry);
assets/view/index.html:596
- This forces
raw=true, but the checked-in sample asset used by the documented local setup containsdefinition/usageExamplesand norawDefinition/rawUsageExamplesfields.ToKBBI(true)therefore selects empty strings, so normal searches render blank meanings; make the API fall back when raw fields are absent or avoid requesting raw data for those assets.
const data = await fetchJson(`${API_BASE}/entry/${encodeURIComponent(entry)}?raw=true`);
assets/view/index.html:601
- Every rejection, including network failures and 5xx responses, is reported as “word not found.” That makes an unavailable API look like a missing dictionary entry; only a 404 should use this message, while transport and server errors need a retry/service-unavailable message.
} catch (e) {
showState(`Kata “${escapeHtml(entry)}” tidak ditemukan dalam KBBI.`, true);
if (updateHistory) updateUrl(entry);
assets/view/index.html:654
- This interactive element is an
<a>withouthrefand is only handled through a mouseclick, so it cannot be focused or activated from the keyboard; the related-word chips below use the same pattern. Render these controls as buttons or real links and preserve keyboard activation.
baseWordNote = `<div class="base-word-note">Kata dasar: <a class="chip-link" data-word="${escapeHtml(entry.baseWord)}">${escapeHtml(entry.baseWord)}</a></div>`;
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Five unresolved review findings remain in view/index.html.
Review details
Suppressed comments (5)
Previously missed (2) — in code that hasn't changed since the last review.
view/index.html:602
- These awaited requests are not sequenced against one another. A slow initial WOTD, random-word, or earlier search response can complete after a newer action and overwrite both the displayed lemma and the URL/history. Use a shared request generation or AbortController and check it before every render and history update.
view/index.html:605 - This catch treats every failure as a missing word, including network errors, 500 responses, and malformed responses. During an API outage the UI therefore reports that a valid word is absent, while the status/message produced by
fetchJsonis discarded; preserve the response status and show a generic load error for non-404 failures.
view/index.html:598
hideSuggestions()only removes the current DOM; it does not invalidate arunSuggestrequest already in flight. If the user submits the same query while that request is pending, the equality check inrunSuggeststill passes andrenderSuggestionsreopens the dropdown after the result is rendered. Track a suggestion request generation or abort the request when a search starts (and when suggestions are hidden).
hideSuggestions();
searchInput.value = entry;
showLoading();
view/index.html:492
- When the query changes, the old suggestion list and
activeSuggestIndex/currentSuggestionsremain until the new request returns. If a suggestion was active, typing another character and pressing Enter during that window selects the old suggestion instead of the current input. Clear the suggestions (or reset the active selection) on every input event before scheduling the new request.
if (q.length === 0) {
hideSuggestions();
return;
}
debounceTimer = setTimeout(() => runSuggest(q), 250);
view/index.html:550
- The suggestion options are generic
<div>elements with mouse handlers and no listbox/option semantics, focusability, or ARIA active-descendant relationship. Although the input handles arrow keys, screen readers cannot discover or announce these choices. Expose a combobox/listbox relationship with option roles and an accessible active state, or use semantic interactive elements.
suggestList.innerHTML = lemmas.map(l => `<div>${escapeHtml(l)}</div>`).join('');
Array.from(suggestList.children).forEach((el, i) => {
el.addEventListener('mousedown', (e) => {
e.preventDefault();
doSearch(lemmas[i]);
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Moderate UI accessibility and stale-response issues remain unresolved.
Review details
Suppressed comments (4)
Previously missed (1) — in code that hasn't changed since the last review.
view/index.html:790
- These generated related-word targets are spans (and the base-word target is an
<a>withouthref), but this handler only adds a mouseclicklistener. They are therefore not keyboard-focusable or activatable, so users who navigate by keyboard cannot follow base/derived/variant links; render them as real buttons/links or add focusability plus Enter/Space handling.
view/index.html:500
- This only invalidates suggestion requests. If the initial WOTD (or another view request) is still pending when the user edits the input,
viewGenstays unchanged and that response can later overwrite the text the user just typed. BumpviewGenon input changes so stale view responses are ignored.
hideSuggestions(); // drop stale list/selection immediately, and invalidate any in-flight request
view/index.html:650
- Unlike
doSearch, this action never hides or invalidates the suggestion list. A pending_searchresponse can therefore repopulate stale options after the random result loads, and pressing Enter can submit one of those old suggestions. Clear suggestions before starting this request.
document.getElementById('btnRandom').addEventListener('click', async () => {
showLoading();
view/index.html:667
loadWotdis called both by the button and bypopstate, but it does not clear or invalidate suggestions. A visible or in-flight list for the previous query can remain over the WOTD result and still be selected; clear suggestions before the request starts.
async function loadWotd(updateHistory = true) {
showLoading();
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
lgtm |
https://kbbi.raf555.dev/ui/