More design updates for the issue view page.

crueber commented

2026-09-05 03:38:59 +00:00

Owner

Each comment has its own container. It should not even be in a container, each comment should just have a separator.
Re-imagine the label added to be a single line, that makes it clear that it isn't a comment but a system message.
When clicking "close" it says it is closed as complete. That may not be the case. It should just be closed. If a status is necessary, the options should be given before it's closed.

![image](/attachments/f0ded12d-4725-4cce-9373-1154b4b968f6) 1. Each comment has its own container. It should not even be in a container, each comment should just have a separator. 2. Re-imagine the label added to be a single line, that makes it clear that it isn't a comment but a system message. 3. When clicking "close" it says it is closed as complete. That may not be the case. It should just be closed. If a status is necessary, the options should be given before it's closed.

image.png

186 KiB

crueber referenced this issue from a commit

2026-09-05 04:08:03 +00:00

Fix #109: timeline system rows + honest close

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

2026-09-05 04:08:09 +00:00

Fix #109: timeline system rows + honest close #111

crueber commented

2026-09-05 04:08:17 +00:00

Author

Owner

Fix is up: PR #111 (#111) — timeline renders comments as divider-separated entries with system events as single-line muted rows, and Close / Comment-and-Close are explicit reason choosers (the API defaults an omitted reason to completed, so the UI always sends one). Browser-verified dark + light with zero console errors.

Fix is up: PR #111 (https://git.packden.us/crueber/walhub/pulls/111) — timeline renders comments as divider-separated entries with system events as single-line muted rows, and Close / Comment-and-Close are explicit reason choosers (the API defaults an omitted reason to completed, so the UI always sends one). Browser-verified dark + light with zero console errors.

crueber commented

2026-09-05 04:10:48 +00:00

Author

Owner

Review: PR #111 (fix/issue-109) — timeline system rows + honest close

Reviewed diff main...origin/fix/issue-109 (6 files) in scratch worktrees; main worktree untouched. BEFORE screenshot confirms the bug (label events as boxed 'labels +approved' comment cards). No browser used — node tests + code reasoning only.

Checklist (all pass)

Divider-separated comments: ThreadTimeline.jsx:50-64 comment entries are border-t dividers with author/date header, no card/box classes; composer keeps card (CommentComposer.jsx:160) — still distinct. ✅
All non-comment kinds are system lines: labels/assignees/milestone/state/references/title/closed_by_pr all return fragments via lib/issue-events.js; ThreadTimeline.jsx:35-47 renders them as single centered muted lines; reaction_changed still filtered in Issue.jsx:68 (folds into chips). None render as comment boxes. ✅
Honest text: 'added the “…“ label' (issue-events.js:26-33), reason-less close → plain 'closed' (issue-events.js:66), never asserts completion. ✅
Close chooser: completed/not-planned chosen pre-close, passed as onClose(reason)/onCommentAndClose(body,reason); Esc closes + refocuses toggle, ArrowUp/Down cycle, Tab dismisses (CommentComposer.jsx:32-111); Reopen is a plain button (closeChooser false when closed, Issue.jsx:303-304); reopen sends {state:'open'} only and the server clears the reason (internal/issues/service.go:520 t.StateReason = nil). ✅
state_reason always explicit: close() and commentAndClose() both go through closePatch(), which throws on anything but completed|not_planned — the UI can never send a reason-less close, so the API's omitted→completed default (service.go:426) is never relied upon. ✅
Closed header: closedStateLabel(t().state_reason) (Issue.jsx:237); thread view carries state_reason (model.go:106); 'Closed' alone when none recorded. ✅
PR conversations: Pull.jsx untouched, still uses shared ThreadTimeline — opened/commented → divider entries, merged/head_force_pushed/state fragments read sanely with actor prefix ('alice merged as abc…'). No reaction_changed emission in pulls service, so the unfiltered PR path can't hit the raw-type fallback in practice. ✅
Laws: no dep changes (no package.json diff; only solid-js + local imports) — Law 1 ✅; no new long work/silent waiting — Law 7 ✅; web-only + docs, no core-package imports — Law 8 ✅; 08_ui_sdk.md updated in same change with accurate entries (verified DOM ids, chooser contract, default semantics) — Law 12 ✅.
Dark+light: all new rows/menu carry zinc-500/dark:zinc-400, border-zinc-200/dark:border-zinc-800, hover + dark:hover variants. ✅

Pre-existing-failure claim — corrected, not a blocker

Author reported data-guard + reaction-cache fail identically on main. Verified: with web/node_modules resolvable, those two files PASS 11/11 on unmodified origin/main AND the full suite passes 252/252 on the PR branch. The observed failures are the missing-node_modules scratch-worktree artifact (node_modules is gitignored, present only in the main checkout), identical on any branch — environmental, not code. No action needed.

Verification results (scratch worktree @`3e95ab2`, node_modules symlinked, removed afterward)

node --test web/test/unit/*.test.js: 252 pass / 0 fail (incl. 7 new issue-events tests)
vite build: success (109 modules, 1.53s)

Minor nits (not blocking, no fix pushed)

<ol class="timeline"> has no .timeline CSS rule — harmless hook class, layout comes from row classes.
Chooser menu has no outside-click dismiss (Esc/Tab/choose all dismiss — fine for v1).
issue-events.js default branch renders unknown kinds as raw type strings — deliberate honest fallback.

MERGE RECOMMENDATION: ready to merge.

## Review: PR #111 (fix/issue-109) — timeline system rows + honest close Reviewed diff main...origin/fix/issue-109 (6 files) in scratch worktrees; main worktree untouched. BEFORE screenshot confirms the bug (label events as boxed 'labels +approved' comment cards). No browser used — node tests + code reasoning only. ### Checklist (all pass) - **Divider-separated comments**: ThreadTimeline.jsx:50-64 comment entries are border-t dividers with author/date header, no card/box classes; composer keeps `card` (CommentComposer.jsx:160) — still distinct. ✅ - **All non-comment kinds are system lines**: labels/assignees/milestone/state/references/title/closed_by_pr all return fragments via lib/issue-events.js; ThreadTimeline.jsx:35-47 renders them as single centered muted lines; reaction_changed still filtered in Issue.jsx:68 (folds into chips). None render as comment boxes. ✅ - **Honest text**: 'added the “…“ label' (issue-events.js:26-33), reason-less close → plain 'closed' (issue-events.js:66), never asserts completion. ✅ - **Close chooser**: completed/not-planned chosen pre-close, passed as onClose(reason)/onCommentAndClose(body,reason); Esc closes + refocuses toggle, ArrowUp/Down cycle, Tab dismisses (CommentComposer.jsx:32-111); Reopen is a plain button (closeChooser false when closed, Issue.jsx:303-304); reopen sends {state:'open'} only and the server clears the reason (internal/issues/service.go:520 `t.StateReason = nil`). ✅ - **state_reason always explicit**: close() and commentAndClose() both go through closePatch(), which throws on anything but completed|not_planned — the UI can never send a reason-less close, so the API's omitted→completed default (service.go:426) is never relied upon. ✅ - **Closed header**: closedStateLabel(t().state_reason) (Issue.jsx:237); thread view carries state_reason (model.go:106); 'Closed' alone when none recorded. ✅ - **PR conversations**: Pull.jsx untouched, still uses shared ThreadTimeline — opened/commented → divider entries, merged/head_force_pushed/state fragments read sanely with actor prefix ('alice merged as abc…'). No reaction_changed emission in pulls service, so the unfiltered PR path can't hit the raw-type fallback in practice. ✅ - **Laws**: no dep changes (no package.json diff; only solid-js + local imports) — Law 1 ✅; no new long work/silent waiting — Law 7 ✅; web-only + docs, no core-package imports — Law 8 ✅; 08_ui_sdk.md updated in same change with accurate entries (verified DOM ids, chooser contract, default semantics) — Law 12 ✅. - **Dark+light**: all new rows/menu carry zinc-500/dark:zinc-400, border-zinc-200/dark:border-zinc-800, hover + dark:hover variants. ✅ ### Pre-existing-failure claim — corrected, not a blocker Author reported data-guard + reaction-cache fail identically on main. Verified: with web/node_modules resolvable, those two files PASS 11/11 on unmodified origin/main AND the full suite passes 252/252 on the PR branch. The observed failures are the missing-node_modules scratch-worktree artifact (node_modules is gitignored, present only in the main checkout), identical on any branch — environmental, not code. No action needed. ### Verification results (scratch worktree @3e95ab2, node_modules symlinked, removed afterward) - `node --test web/test/unit/*.test.js`: **252 pass / 0 fail** (incl. 7 new issue-events tests) - `vite build`: **success** (109 modules, 1.53s) ### Minor nits (not blocking, no fix pushed) - `<ol class="timeline">` has no .timeline CSS rule — harmless hook class, layout comes from row classes. - Chooser menu has no outside-click dismiss (Esc/Tab/choose all dismiss — fine for v1). - issue-events.js default branch renders unknown kinds as raw type strings — deliberate honest fallback. **MERGE RECOMMENDATION: ready to merge.**

crueber closed this issue

2026-09-05 04:11:03 +00:00

crueber commented

2026-09-05 04:11:04 +00:00

Author

Owner

Fixed by PR #111 (review clean; pre-existing-failure claim corrected to env artifact; 252/252 node tests), merged. Closing.

crueber referenced this issue from a commit

2026-09-05 04:11:04 +00:00

Merge pull request 'Fix #109: timeline system rows + honest close' (#111) from fix/issue-109 into main

crueber referenced this issue from a commit

2026-09-05 15:50:21 +00:00