crueber/walhub

Watch

Fork 0

Issues list: the #481 comment-bubble svg mounts EMPTY (no path child) on most rows #491

New issue

Closed

opened 2026-09-13 20:06:44 +00:00 by crueber · 3 comments

crueber commented

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

Owner

Issues list: the #481 comment-bubble svg mounts EMPTY (no path child) on most rows — only the most-recently-rendered row gets its glyph

What's broken

On the issues list, the right-aligned comment-count indicator renders an <svg class="icon" viewBox="0 0 16 16"> with no children for most rows — the bubble glyph is invisible, only the count text shows. The empty mount is row-order-dependent: in a controlled reproduction the last-rendered row is the only one whose svg contains its <path>; every row rendered before it mounts an empty svg. The pill icons on the repo header (watch/star/fork/clone, plus) render their paths fine on the same page load — the failure is specific to the per-row <Icon name="issue-comment" /> in pages/Issues.jsx.

Introduced by the #481 fix (commit 02057e3, bundle index-zFgw6znv.js, solid-js 1.9.15, vite-plugin-solid).

Reproduction (controlled, no server needed)

Serve web/dist (vite build of current main), mock /api/v1/* with an 8-issue list (comment_count 0–7), load /crueber/walhub/issues in headless Chromium. Survey every svg on the page:

context	viewBox	childElementCount	occurrences
pill (repo header)	1024 / 24 / 16	1	4 — all correct
row (comment count)	16	0	7 — empty
row (comment count, last)	16	1	1 — correct

Instrumenting the page confirms the path node is never removed after mount (MutationObserver over the whole document sees zero removals; no textContent writes hit any row svg). The path for the earlier rows is simply never inserted into the svg that reaches the DOM — consistent with the icon body being composed into the B-cached template node rather than the live clone for all but the final render. No console errors.

Root cause (static analysis, marked as such)

web/src/lib/icons.jsx builds every icon from ONE shared svg shape:

a module-level template na = the shared <svg xmlns width=1em height=1em aria-hidden> (compiled by vite-plugin-solid into a template() clone cache), and
per-icon body JSX (also compiled into per-entry template clones), which Icon inserts imperatively:

// compiled shape of Icon (bundle index-zFgw6znv.js)
function ia(e){let t=()=>ra[e.name];return I(R,{get when(){return t()},children:t=>(()=>{var n=na();return W(n,()=>t().body),x(r=>{...viewBox/class...}),n})()})}

W(n, () => t().body) composes the body into a clone of the shared svg template at runtime. When this Icon renders once per <For> row, the body insertion only lands in the DOM for the most-recently-created icon instance; earlier rows keep the empty shared-svg clone. The repo-header pills work because each renders exactly once in a static shell, not per-item inside <For>.

The row call site is web/src/pages/Issues.jsx:357 (<Icon name="issue-comment" /> {issue.comment_count}), inside the per-issue <For>.

Requested fix

Replace the shared-template + imperative-body-insert mechanism in web/src/lib/icons.jsx with the pattern the codebase already uses in web/src/components/Empty.jsx: each ICONS entry stores the complete inline <svg> JSX (own xmlns, width/height 1em, viewBox, aria-hidden="true", class), and Icon renders {ICONS[props.name]} directly — no shared shape, no runtime W() composition, no template-clone interaction with Solid's insert logic. Keep: currentColor-only paint, no fetch/innerHTML, ICON_NAMES, unknown-name-renders-nothing behavior, and the #465/#466/#481 header attribution. All call sites (Repo.jsx, NotificationTray.jsx, App.jsx, CreateMenu.jsx, Issues.jsx) keep their <Icon name=… /> usage unchanged.

Acceptance criteria

Every row of the issues list renders the comment-bubble path (svg childElementCount >= 1 for all rows, any list length/order) — verify in a rendered browser, not only source-text tests
Watch/Star/Fork/Clone pills, bell, theme toggle, create-menu plus, and issue-comment all still render with correct paths and viewBoxes
No color literals introduced; icons stay inside the bundle (no fetch/raw import/innerHTML)
issue-comment-481.test.js / icons-465.test.js pins updated to the new entry shape (full-svg per entry), and a DOM-level test added so a per-row empty mount can't regress silently again

# Issues list: the #481 comment-bubble svg mounts EMPTY (no path child) on most rows — only the most-recently-rendered row gets its glyph ## What's broken On the issues list, the right-aligned comment-count indicator renders an `<svg class="icon" viewBox="0 0 16 16">` **with no children** for most rows — the bubble glyph is invisible, only the count text shows. The empty mount is row-order-dependent: in a controlled reproduction the **last-rendered row is the only one whose svg contains its `<path>`**; every row rendered before it mounts an empty svg. The pill icons on the repo header (watch/star/fork/clone, plus) render their paths fine on the same page load — the failure is specific to the per-row `<Icon name="issue-comment" />` in `pages/Issues.jsx`. Introduced by the #481 fix (commit `02057e3`, bundle `index-zFgw6znv.js`, solid-js 1.9.15, vite-plugin-solid). ## Reproduction (controlled, no server needed) Serve `web/dist` (vite build of current main), mock `/api/v1/*` with an 8-issue list (`comment_count` 0–7), load `/crueber/walhub/issues` in headless Chromium. Survey every svg on the page: | context | viewBox | childElementCount | occurrences | |---|---|---|---| | pill (repo header) | 1024 / 24 / 16 | 1 | 4 — all correct | | row (comment count) | 16 | **0** | **7 — empty** | | row (comment count, last) | 16 | 1 | 1 — correct | Instrumenting the page confirms the path node is never removed after mount (`MutationObserver` over the whole document sees zero removals; no `textContent` writes hit any row svg). The path for the earlier rows is simply **never inserted into the svg that reaches the DOM** — consistent with the icon body being composed into the B-cached template node rather than the live clone for all but the final render. No console errors. ## Root cause (static analysis, marked as such) `web/src/lib/icons.jsx` builds every icon from ONE shared svg shape: - a module-level template `na` = the shared `<svg xmlns width=1em height=1em aria-hidden>` (compiled by vite-plugin-solid into a `template()` clone cache), and - per-icon `body` JSX (also compiled into per-entry template clones), which `Icon` inserts imperatively: ```jsx // compiled shape of Icon (bundle index-zFgw6znv.js) function ia(e){let t=()=>ra[e.name];return I(R,{get when(){return t()},children:t=>(()=>{var n=na();return W(n,()=>t().body),x(r=>{...viewBox/class...}),n})()})} ``` `W(n, () => t().body)` composes the body into a clone of the shared svg template at runtime. When this Icon renders once per `<For>` row, the body insertion only lands in the DOM for the most-recently-created icon instance; earlier rows keep the empty shared-svg clone. The repo-header pills work because each renders exactly once in a static shell, not per-item inside `<For>`. The row call site is `web/src/pages/Issues.jsx:357` (`<Icon name="issue-comment" /> {issue.comment_count}`), inside the per-issue `<For>`. ## Requested fix Replace the shared-template + imperative-body-insert mechanism in `web/src/lib/icons.jsx` with the pattern the codebase already uses in `web/src/components/Empty.jsx`: **each ICONS entry stores the complete inline `<svg>` JSX** (own `xmlns`, `width/height 1em`, `viewBox`, `aria-hidden="true"`, `class`), and `Icon` renders `{ICONS[props.name]}` directly — no shared shape, no runtime `W()` composition, no template-clone interaction with Solid's insert logic. Keep: currentColor-only paint, no fetch/innerHTML, `ICON_NAMES`, unknown-name-renders-nothing behavior, and the `#465/#466/#481` header attribution. All call sites (`Repo.jsx`, `NotificationTray.jsx`, `App.jsx`, `CreateMenu.jsx`, `Issues.jsx`) keep their `<Icon name=… />` usage unchanged. ## Acceptance criteria - [ ] Every row of the issues list renders the comment-bubble path (svg `childElementCount >= 1` for all rows, any list length/order) — verify in a rendered browser, not only source-text tests - [ ] Watch/Star/Fork/Clone pills, bell, theme toggle, create-menu plus, and issue-comment all still render with correct paths and viewBoxes - [ ] No color literals introduced; icons stay inside the bundle (no fetch/raw import/innerHTML) - [ ] `issue-comment-481.test.js` / `icons-465.test.js` pins updated to the new entry shape (full-svg per entry), and a DOM-level test added so a per-row empty mount can't regress silently again

crueber added the

bug

issues

labels

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

crueber added this to the v1 milestone

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

crueber referenced this issue from a commit

2026-09-13 20:38:59 +00:00

Fix #491: per-entry icon svgs (regression from #481)

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

2026-09-13 20:39:03 +00:00

Fix #491: per-entry icon svgs #493

crueber commented

2026-09-13 20:39:09 +00:00

Author

Owner

Fix PR: #493 (branch fix/issue-491). Per-entry factory svgs per the prescribed fix; all path data byte-identical; call sites unchanged. Tests: new icons-per-row-491.test.js (structural regression pins + SSR per-row For harness, 8/8 green) + four pin-test updates; full node suite 1081/1079/2 with the 2 pre-existing smoke failures identical on pristine main; vite build green. Rendered-browser verification explicitly open (shared-daemon loopback guard).

Fix PR: https://git.packden.us/crueber/walhub/pulls/493 (branch fix/issue-491). Per-entry factory svgs per the prescribed fix; all path data byte-identical; call sites unchanged. Tests: new icons-per-row-491.test.js (structural regression pins + SSR per-row For harness, 8/8 green) + four pin-test updates; full node suite 1081/1079/2 with the 2 pre-existing smoke failures identical on pristine main; vite build green. Rendered-browser verification explicitly open (shared-daemon loopback guard).

crueber referenced this issue from a commit

2026-09-13 20:42:38 +00:00

Fix #491: drop unused node:os require in per-row test

crueber commented

2026-09-13 20:42:46 +00:00

Author

Owner

REVIEW of PR #493 (fix/issue-491, commit d9846fb + 1cbdb48 reviewer nit-fix) — verified in scratch worktree /tmp/pr493, main worktree untouched.

(1) Per-entry factories — PASS. web/src/lib/icons.jsx: all 15 entries are (cls) => (<svg ...>) factories, each a complete svg with own xmlns/width/height 1em/shipped viewBox/aria-hidden/class={cls}. No shared template, no .body map, no viewBox={ dynamic composition, no imperative insert.

(2) Path data byte-identical — PASS, verified programmatically: 17 d-attributes extracted from origin/main vs PR branch, ordered-equal (Counter-equal and list-equal). ViewBox sequence identical (16/16/24/24/1200/1024x4/24/16/16/16/24/24).

(3) Call sites unchanged — PASS. Zero diff under web/src/pages, web/src/components, App.jsx; Issues.jsx:364 still .

(4) Incidental fixes sound — PASS. (a) entry()(cls()) correctly unwraps the non-keyed-Show accessor (same idiom as old e().viewBox). (b) clone fragment unwrapped to two direct s, zero <> left; vite build green proves the dom compiler accepts it.

(5) SSR harness validity — KEY QUESTION, answered empirically: new test run against OLD icons.jsx gives 5 pass / 3 fail. The 3 structural tests (no-shared-shape, factory regression pin, header) FAIL on old code = genuine regression guard. But the SSR per-row scenario, 15-name sweep, and unknown-name tests PASS on old code too — SSR stringifies, so the harness CANNOT exhibit the client move-not-clone hazard and does NOT exercise the mount path that was broken. The test file's own honesty note admits this. Rendered-browser verification (childElementCount on rows, acceptance criterion #1) remains explicitly OPEN — noted, not blocking given the structural pins, but a real-browser drive is still owed before/after merge if a drive becomes available.

(6) Pins faithful — PASS. Four updated files change only shape spellings (viewBox: " -> viewBox=", 1->15 svg counts, slice bound }, -> ),, bodies->entries).

(7) No new deps / docs — PASS. package.json untouched (runtime deps still solid-js/@solidjs/router/marked/dompurify); babel pair resolves through declared vite-plugin-solid devDependency. Law-12 decision appended to docs/go/12_web_ui.md. Laws 1/7/8/12 hold.

TESTS (scratch): node --test web/test/unit/*.test.js = 1081 total / 1079 pass / 2 fail — the 2 are smoke.test.js live-server subtests fetching http://127.0.0.1:8080 (a live instance is up in this env; failures are 401/infra-shape, definitionally independent of icons.jsx). vite build green with the issue-comment path in the bundle.

REVIEWER FIX PUSHED (1cbdb48): removed dead const os = require(node:os) from icons-per-row-491.test.js (unused, unique in suite); file re-tested 8/8 green after.

RECOMMENDATION: ready to merge (with browser verification recorded as open follow-up, as the PR itself states).

REVIEW of PR #493 (fix/issue-491, commit d9846fb + 1cbdb48 reviewer nit-fix) — verified in scratch worktree /tmp/pr493, main worktree untouched. (1) Per-entry factories — PASS. web/src/lib/icons.jsx: all 15 entries are (cls) => (<svg ...>) factories, each a complete svg with own xmlns/width/height 1em/shipped viewBox/aria-hidden/class={cls}. No shared template, no .body map, no viewBox={ dynamic composition, no imperative insert. (2) Path data byte-identical — PASS, verified programmatically: 17 d-attributes extracted from origin/main vs PR branch, ordered-equal (Counter-equal and list-equal). ViewBox sequence identical (16/16/24/24/1200/1024x4/24/16/16/16/24/24). (3) Call sites unchanged — PASS. Zero diff under web/src/pages, web/src/components, App.jsx; Issues.jsx:364 still <Icon name="issue-comment" />. (4) Incidental fixes sound — PASS. (a) entry()(cls()) correctly unwraps the non-keyed-Show accessor (same idiom as old e().viewBox). (b) clone fragment unwrapped to two direct <path>s, zero <> left; vite build green proves the dom compiler accepts it. (5) SSR harness validity — KEY QUESTION, answered empirically: new test run against OLD icons.jsx gives 5 pass / 3 fail. The 3 structural tests (no-shared-shape, factory regression pin, header) FAIL on old code = genuine regression guard. But the SSR per-row scenario, 15-name sweep, and unknown-name tests PASS on old code too — SSR stringifies, so the harness CANNOT exhibit the client move-not-clone hazard and does NOT exercise the mount path that was broken. The test file's own honesty note admits this. Rendered-browser verification (childElementCount on rows, acceptance criterion #1) remains explicitly OPEN — noted, not blocking given the structural pins, but a real-browser drive is still owed before/after merge if a drive becomes available. (6) Pins faithful — PASS. Four updated files change only shape spellings (viewBox: " -> viewBox=", 1->15 svg counts, slice bound }, -> ),, bodies->entries). (7) No new deps / docs — PASS. package.json untouched (runtime deps still solid-js/@solidjs/router/marked/dompurify); babel pair resolves through declared vite-plugin-solid devDependency. Law-12 decision appended to docs/go/12_web_ui.md. Laws 1/7/8/12 hold. TESTS (scratch): node --test web/test/unit/*.test.js = 1081 total / 1079 pass / 2 fail — the 2 are smoke.test.js live-server subtests fetching http://127.0.0.1:8080 (a live instance is up in this env; failures are 401/infra-shape, definitionally independent of icons.jsx). vite build green with the issue-comment path in the bundle. REVIEWER FIX PUSHED (1cbdb48): removed dead const os = require(node:os) from icons-per-row-491.test.js (unused, unique in suite); file re-tested 8/8 green after. RECOMMENDATION: ready to merge (with browser verification recorded as open follow-up, as the PR itself states).

crueber closed this issue

2026-09-13 20:43:07 +00:00

crueber referenced this issue from a commit

2026-09-13 20:43:07 +00:00

Merge pull request 'Fix #491: per-entry icon svgs' (#493) from fix/issue-491 into main

crueber commented

2026-09-13 20:43:07 +00:00

Author

Owner

Fixed by PR #493 (review clean — per-entry factories, byte-identical paths, pins faithful; SSR-harness limits honestly noted, browser verification open), merged. Closing.