crueber/walhub

Watch

Fork 0

Replace the leftover speech-bubble emoji on the issue list with the provided comment-bubble icon via the #465 icons.jsx mechanism #481

New issue

Closed

opened 2026-09-13 19:20:36 +00:00 by crueber · 3 comments

crueber commented

2026-09-13 19:20:36 +00:00

Owner

What's requested

Replace the leftover emoji glyph (the speech-bubble emoji) on the issues list comment-count indicator with the user-provided comment-bubble SVG, embedded through the shared #465 icon mechanism (web/src/lib/icons.jsx), and sweep other occurrences of the same glyph across web/src.

Evidence (current tree, SHA `7cfecee`)

web/src/pages/Issues.jsx:356 — the comment-count indicator on each issue-list row renders a literal emoji speech-bubble span via <span aria-hidden="true"> </span> followed by {issue.comment_count}. This row survived the #465 icon sweep (all six header controls were converted; the issue-list glyph was not).
Full-tree grep of web/src for the same glyph: Issues.jsx:356 is the only remaining occurrence of the speech-bubble emoji. Adjacent count renderings worth checking while in the file:
- web/src/pages/Issue.jsx:406 — renders {t().comment_count} comments as plain text with no glyph; if the icon lands on the list rows, applying the same icon here (or explicitly deciding not to) should be noted so the two surfaces agree.
The check/checklist glyphs still rendered as text on Issues.jsx (the ✓ marks at lines 131/151) are NOT in scope — they are styled text glyphs in picker rows, not the emoji in question; leave them unless the implementer wants a follow-up.

Provided asset

The user-provided comment-bubble SVG (/tmp/svg-icons/issue-comment.svg on the filing machine — transcribe verbatim, matching the #465 convention):

<svg xmlns="http://www.w3.org/2000/svg" width="1em" height="1em" viewBox="0 0 16 16">
	<path fill="currentColor" d="M3.5 2A2.5 2.5 0 0 0 1 4.5v5A2.5 2.5 0 0 0 3.5 12H4v1.942a.98.98 0 0 0 1.625.738L8.688 12H12.5A2.5 2.5 0 0 0 15 9.5v-5A2.5 2.5 0 0 0 12.5 2zM2 4.5A1.5 1.5 0 0 1 3.5 3h9A1.5 1.5 0 0 1 14 4.5v5a1.5 1.5 0 0 1-1.5 1.5H8.312L5 13.898V11H3.5A1.5 1.5 0 0 1 2 9.5zM7.5 8h5a.5.5 0 0 0 0-1h-5a.5.5 0 0 0 0 1m-2-1h-2a.5.5 0 0 0 0 1h2a.5.5 0 0 0 0-1m-2 2a.5.5 0 0 0 0 1h5a.5.5 0 0 0 0-1zm7 1a.5.5 0 0 1 0-1h2a.5.5 0 0 1 0 1z" />
</svg>

Architecture notes

Add an issue-comment entry to the ICONS map in web/src/lib/icons.jsx (the shared mechanism from #465, closed via PR #475): keep the shipped viewBox="0 0 16 16" as-is, transcribe the d path verbatim, paint via fill="currentColor" — no color literals, no fetch/innerHTML (law-12 pattern already documented in docs/go/12_web_ui.md).
Consume it at Issues.jsx:356 as <Icon name="issue-comment" /> replacing the emoji span; the shared .icon utility in web/src/ui.css (1em box, baseline-aligned) handles sizing — the surrounding text-xs text-zinc-500 span already sets color and gap, so no per-call-site CSS should be needed.
The mechanism's header comment lists its consumers; update that comment's count/naming when the map grows to 12 entries.
Sweep note: a full-string grep for the emoji over web/src currently returns exactly one hit (Issues.jsx:356), so the "other occurrences" sweep should be a verification step, not expected new work — unless the implementer extends the icon to Issue.jsx:406's bare comment count, which is a judgment call (pick one and note it).

Acceptance criteria

issue-comment exists in the ICONS map in web/src/lib/icons.jsx, path transcribed verbatim from the provided SVG, viewBox 0 0 16 16 preserved, currentColor paint, no color literals
Issues.jsx:356 renders <Icon name="issue-comment" /> instead of the emoji span; count renders after the icon as before
Grep for the speech-bubble emoji over web/src returns zero hits after the change
Issue.jsx:406 comment-count treatment reconciled with the list (icon added there too, or an explicit note why not)
icons.jsx header comment updated to reflect the new entry/consumer count
No new dependencies; no runtime fetches/innerHTML; existing unit tests stay green

## What's requested Replace the leftover emoji glyph (the speech-bubble emoji) on the issues list comment-count indicator with the user-provided comment-bubble SVG, embedded through the shared #465 icon mechanism (`web/src/lib/icons.jsx`), and sweep other occurrences of the same glyph across `web/src`. ## Evidence (current tree, SHA 7cfecee) - `web/src/pages/Issues.jsx:356` — the comment-count indicator on each issue-list row renders a literal emoji speech-bubble span via `<span aria-hidden="true"> </span>` followed by `{issue.comment_count}`. This row survived the #465 icon sweep (all six header controls were converted; the issue-list glyph was not). - Full-tree grep of `web/src` for the same glyph: **Issues.jsx:356 is the only remaining occurrence** of the speech-bubble emoji. Adjacent count renderings worth checking while in the file: - `web/src/pages/Issue.jsx:406` — renders `{t().comment_count} comments` as plain text with no glyph; if the icon lands on the list rows, applying the same icon here (or explicitly deciding not to) should be noted so the two surfaces agree. - The check/checklist glyphs still rendered as text on `Issues.jsx` (the `✓` marks at lines 131/151) are NOT in scope — they are styled text glyphs in picker rows, not the emoji in question; leave them unless the implementer wants a follow-up. ## Provided asset The user-provided comment-bubble SVG (`/tmp/svg-icons/issue-comment.svg` on the filing machine — transcribe verbatim, matching the #465 convention): ```svg <svg xmlns="http://www.w3.org/2000/svg" width="1em" height="1em" viewBox="0 0 16 16"> <path fill="currentColor" d="M3.5 2A2.5 2.5 0 0 0 1 4.5v5A2.5 2.5 0 0 0 3.5 12H4v1.942a.98.98 0 0 0 1.625.738L8.688 12H12.5A2.5 2.5 0 0 0 15 9.5v-5A2.5 2.5 0 0 0 12.5 2zM2 4.5A1.5 1.5 0 0 1 3.5 3h9A1.5 1.5 0 0 1 14 4.5v5a1.5 1.5 0 0 1-1.5 1.5H8.312L5 13.898V11H3.5A1.5 1.5 0 0 1 2 9.5zM7.5 8h5a.5.5 0 0 0 0-1h-5a.5.5 0 0 0 0 1m-2-1h-2a.5.5 0 0 0 0 1h2a.5.5 0 0 0 0-1m-2 2a.5.5 0 0 0 0 1h5a.5.5 0 0 0 0-1zm7 1a.5.5 0 0 1 0-1h2a.5.5 0 0 1 0 1z" /> </svg> ``` ## Architecture notes - Add an `issue-comment` entry to the `ICONS` map in `web/src/lib/icons.jsx` (the shared mechanism from #465, closed via PR #475): keep the shipped `viewBox="0 0 16 16"` as-is, transcribe the `d` path verbatim, paint via `fill="currentColor"` — no color literals, no fetch/innerHTML (law-12 pattern already documented in `docs/go/12_web_ui.md`). - Consume it at `Issues.jsx:356` as `<Icon name="issue-comment" />` replacing the emoji span; the shared `.icon` utility in `web/src/ui.css` (1em box, baseline-aligned) handles sizing — the surrounding `text-xs text-zinc-500` span already sets color and gap, so no per-call-site CSS should be needed. - The mechanism's header comment lists its consumers; update that comment's count/naming when the map grows to 12 entries. - Sweep note: a full-string grep for the emoji over `web/src` currently returns exactly one hit (Issues.jsx:356), so the "other occurrences" sweep should be a verification step, not expected new work — unless the implementer extends the icon to `Issue.jsx:406`'s bare comment count, which is a judgment call (pick one and note it). ## Acceptance criteria - [ ] `issue-comment` exists in the `ICONS` map in `web/src/lib/icons.jsx`, path transcribed verbatim from the provided SVG, viewBox `0 0 16 16` preserved, `currentColor` paint, no color literals - [ ] `Issues.jsx:356` renders `<Icon name="issue-comment" />` instead of the emoji span; count renders after the icon as before - [ ] Grep for the speech-bubble emoji over `web/src` returns zero hits after the change - [ ] `Issue.jsx:406` comment-count treatment reconciled with the list (icon added there too, or an explicit note why not) - [ ] icons.jsx header comment updated to reflect the new entry/consumer count - [ ] No new dependencies; no runtime fetches/innerHTML; existing unit tests stay green

crueber added the

enhancement

issues

labels

2026-09-13 19:20:45 +00:00

crueber added this to the v1 milestone

2026-09-13 19:20:52 +00:00

crueber referenced this issue from a commit

2026-09-13 19:25:39 +00:00

Fix #481: comment-bubble icon via shared icons.jsx mechanism

crueber referenced this issue from a pull request that will close it,

2026-09-13 19:25:45 +00:00

Fix #481: comment-bubble icon #483

crueber commented

2026-09-13 19:25:48 +00:00

Author

Owner

Fix ready for review: PR #483 (#483) — issue-comment icon via the shared icons.jsx mechanism, emoji sweep clean, Issue.jsx:406 kept as bare byline text with rationale noted. node --test 916 total / 901 pass / 15 fail (15 pre-existing, verified identical on pristine origin/main); vite build green.

Fix ready for review: PR #483 (https://git.packden.us/crueber/walhub/pulls/483) — issue-comment icon via the shared icons.jsx mechanism, emoji sweep clean, Issue.jsx:406 kept as bare byline text with rationale noted. node --test 916 total / 901 pass / 15 fail (15 pre-existing, verified identical on pristine origin/main); vite build green.

crueber referenced this issue

2026-09-13 19:26:17 +00:00

Add milestone-open/milestone-done icons to the #465 icons mechanism; apply state-mapped on milestones page, issues list, and the repo tab strip #484

crueber commented

2026-09-13 19:28:17 +00:00

Author

Owner

REVIEW of PR #483 (fix/issue-481) — verified in scratch worktrees only; main worktree untouched (still 2b42843, clean).

VERBATIM TRANSCRIPTION — PASS. Fetched the issue body via the Forgejo API and diffed the provided SVG's d attribute byte-for-byte against web/src/lib/icons.jsx "issue-comment" entry: IDENTICAL (412 chars). viewBox 0 0 16 16 preserved, fill=currentColor, no hex/rgb literals (codeOf-stripped scan in the new test).
Issues.jsx SWAP — PASS (web/src/pages/Issues.jsx:357). Emoji span gone, replaced with name-only {issue.comment_count} — icon-first/count-after order kept, no per-icon class (sizing from the shared .icon utility, web/src/ui.css:84: inline-block 1em, shrink-0, baseline-aligned), title + aria-label count strings byte-identical.
EMOJI GREP-ZERO — PASS. grep for the speech-bubble emoji over web/src on the PR branch: zero hits. Same grep on pristine main: exactly one hit (Issues.jsx:356, the fixed line).
Issue.jsx:406 DECISION — SOUND. Byline keeps bare "{n} comments" mid-sentence text with an explicit #481 why-not comment (web/src/pages/Issue.jsx:405-409): the shared badge icon would break reading flow; icon lives on the list rows. Both surfaces agree, rationale recorded at the call site.
HEADER COMMENT — PASS. icons.jsx header now reads eight surfaces / 12 bodies / ICON_NAMES twelve, with the twelfth body attributed to #481 and Issues.jsx named as consumer.
PINS — FAITHFUL. No package.json/pnpm-lock change (nothing to faithfully pin — deps correctly unchanged). The three stale source pins were updated minimally and correctly: icons-465.test.js (11→12, +issue-comment box/name), create-menu-466.test.js (plus-entry slice now bounded at its own "}," close instead of a fixed 300-char width so the new #481 attribution can't leak into the scan — good defensive fix), issues-row-milestone.test.js (emoji-span assertion → shared-Icon assertion).
NO NEW DEPS / LAW-12 — PASS / REASONABLE. Runtime deps still exactly solid-js + @solidjs/router + marked + dompurify; no fetch/?raw/innerHTML (asserted in tests). No docs/go/12_web_ui.md amendment needed: the mechanism contract is unchanged (one more map entry), consistent with the #466 plus-icon precedent which also shipped without a doc entry; attribution lives in the code header + tests.

HEAD-TO-HEAD FULL SUITE (node --test web/test/unit/*.test.js, symlinked node_modules, both scratch branches): PR branch 1036 tests / 1034 pass / 2 fail; pristine main 1029 / 1027 / 2 (+7 net new tests, all passing). Failing sets IDENTICAL file-by-file: both are the two live-server smoke.test.js subtests ("built SPA shell…" 401 vs 200, "hashed assets…") which need a running Go server — environment-dependent, zero PR-caused. NOTE: the reported "15 full-suite failures" do not reproduce — I see 2 pre-existing failures on both branches; no Go files are touched by this diff so the Go tiers cannot be affected.

VERIFY: vite build green (2.19s); the transcribed path (M3.5 2A2.5…) is present in web/dist/assets/*.js. Targeted files (issue-comment-481, icons-465, create-menu-466, issues-row-milestone): 39/39 pass. Out-of-scope ✓ picker glyphs (Issues.jsx:132/152) untouched. No browser drive per review instructions (node tests + reasoning; 1em icon swap inside the existing span carries no layout risk). No fixes pushed — nothing to fix.

MERGE RECOMMENDATION: ready to merge.

REVIEW of PR #483 (fix/issue-481) — verified in scratch worktrees only; main worktree untouched (still 2b42843, clean). 1) VERBATIM TRANSCRIPTION — PASS. Fetched the issue body via the Forgejo API and diffed the provided SVG's d attribute byte-for-byte against web/src/lib/icons.jsx "issue-comment" entry: IDENTICAL (412 chars). viewBox 0 0 16 16 preserved, fill=currentColor, no hex/rgb literals (codeOf-stripped scan in the new test). 2) Issues.jsx SWAP — PASS (web/src/pages/Issues.jsx:357). Emoji span gone, replaced with name-only <Icon name="issue-comment" /> {issue.comment_count} — icon-first/count-after order kept, no per-icon class (sizing from the shared .icon utility, web/src/ui.css:84: inline-block 1em, shrink-0, baseline-aligned), title + aria-label count strings byte-identical. 3) EMOJI GREP-ZERO — PASS. grep for the speech-bubble emoji over web/src on the PR branch: zero hits. Same grep on pristine main: exactly one hit (Issues.jsx:356, the fixed line). 4) Issue.jsx:406 DECISION — SOUND. Byline keeps bare "{n} comments" mid-sentence text with an explicit #481 why-not comment (web/src/pages/Issue.jsx:405-409): the shared badge icon would break reading flow; icon lives on the list rows. Both surfaces agree, rationale recorded at the call site. 5) HEADER COMMENT — PASS. icons.jsx header now reads eight surfaces / 12 bodies / ICON_NAMES twelve, with the twelfth body attributed to #481 and Issues.jsx named as consumer. 6) PINS — FAITHFUL. No package.json/pnpm-lock change (nothing to faithfully pin — deps correctly unchanged). The three stale source pins were updated minimally and correctly: icons-465.test.js (11→12, +issue-comment box/name), create-menu-466.test.js (plus-entry slice now bounded at its own "}," close instead of a fixed 300-char width so the new #481 attribution can't leak into the scan — good defensive fix), issues-row-milestone.test.js (emoji-span assertion → shared-Icon assertion). 7) NO NEW DEPS / LAW-12 — PASS / REASONABLE. Runtime deps still exactly solid-js + @solidjs/router + marked + dompurify; no fetch/?raw/innerHTML (asserted in tests). No docs/go/12_web_ui.md amendment needed: the mechanism contract is unchanged (one more map entry), consistent with the #466 plus-icon precedent which also shipped without a doc entry; attribution lives in the code header + tests. HEAD-TO-HEAD FULL SUITE (node --test web/test/unit/*.test.js, symlinked node_modules, both scratch branches): PR branch 1036 tests / 1034 pass / 2 fail; pristine main 1029 / 1027 / 2 (+7 net new tests, all passing). Failing sets IDENTICAL file-by-file: both are the two live-server smoke.test.js subtests ("built SPA shell…" 401 vs 200, "hashed assets…") which need a running Go server — environment-dependent, zero PR-caused. NOTE: the reported "15 full-suite failures" do not reproduce — I see 2 pre-existing failures on both branches; no Go files are touched by this diff so the Go tiers cannot be affected. VERIFY: vite build green (2.19s); the transcribed path (M3.5 2A2.5…) is present in web/dist/assets/*.js. Targeted files (issue-comment-481, icons-465, create-menu-466, issues-row-milestone): 39/39 pass. Out-of-scope ✓ picker glyphs (Issues.jsx:132/152) untouched. No browser drive per review instructions (node tests + reasoning; 1em icon swap inside the existing span carries no layout risk). No fixes pushed — nothing to fix. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-13 19:28:33 +00:00

crueber commented

2026-09-13 19:28:34 +00:00

Author

Owner

Fixed by PR #483 (review clean — byte-identical transcription verified programmatically, all 6 criteria pass), merged. Closing.

crueber referenced this issue from a commit

2026-09-13 19:28:34 +00:00

Merge pull request 'Fix #481: comment-bubble icon' (#483) from fix/issue-481 into main

crueber referenced this issue

2026-09-13 20:06:44 +00:00