crueber/walhub

Watch

Fork 0

Redesign PR conversation page to match the issue page's design language (header chip, markdown bodies, merged state) #521

New issue

Closed

opened 2026-09-14 15:47:03 +00:00 by crueber · 3 comments

crueber commented

2026-09-14 15:47:03 +00:00

Owner

What's requested

Redesign the PR conversation page (web/src/pages/Pull.jsx) to match the design language the issue page already establishes (web/src/pages/Issue.jsx), so the two conversation surfaces read as siblings. Three areas: page structure/state display, PR-body rendering, and open/merged state consistency.

Reference implementation: the issue page (web/src/pages/Issue.jsx), which is the canonical pattern for this app's conversation surfaces: an issue-page grid (Issue.jsx:422), a bordered header block with #num title heading and a state chip (Issue.jsx:434–442), a muted byline, and a right sidebar of grouped metadata cards (Labels / Assignees / Milestone, Issue.jsx:549+).

Evidence (static read of the current tree)

1. Header/state display diverges from the issue page

Pull.jsx:654–662: bare <h1>#{num} {title}</h1> followed by a plain text line {thread()?.state} · {author} · {date} — the state renders as lowercase prose, not the app's state chip. Issue.jsx:436–442 pairs the same heading with chip chip-open / chip chip-closed and a border-b header block.
web/src/ui.css (lines ~108–116) defines chip-open, chip-closed, chip-draft, chip-prerelease — but no chip-merged. The PR list (Pulls.jsx:124) already emits chip chip-${pr.state}, so a merged PR renders a chip with an unstyled class (the known unstyled-class failure mode). The conversation page needs a merged chip and so does the list.

2. PR-body rendering is inconsistent

Comment bodies in the timeline go through renderBody (markdown + DOMPurify) via ThreadTimeline.jsx:83 — correct.
But Pull.jsx:136 (review bodies) and Pull.jsx:470 (thread comments under ThreadComments) render raw whitespace-pre-wrap text — no markdown, unlike the issue thread. Staged line comments in the finish-review modal (Pull.jsx:514) are plain text, acceptable as a draft preview but the submitted review body is not.
The PR description itself surfaces only as the opened event row inside the timeline; there is no dedicated, styled PR-body block like the issue page's first-comment treatment (Issue.jsx:430–433 describes the intended pattern: number + title header with byline, body as first comment).

3. Page structure

Pull.jsx:655 uses a generic grid gap-6 lg:grid-cols-[1fr_320px] rather than the issue page's issue-page idiom; the Review summary / Reviews / Reviewers / Mergeability / Checks cards carry ad-hoc h2 text-sm headings at mixed hierarchy with no unified card-header treatment, and the ReviewSummaryBar floats between header and timeline rather than composing into the header/sidebar structure.

Architecture notes

State chip vocabulary lives in one place (ui.css); adding chip-merged there covers the list page for free. chip-${state} interpolation in Pulls.jsx:124 needs the CSS rule to exist or a mapping for merged.
ThreadTimeline is the ONE P3 renderer for issue threads and PR conversations — do not fork it; the raw-text call sites (Pull.jsx:136, 470) should route through renderBody from web/src/lib/render-md.js like the timeline does. Review bodies carry the repo context for the autolinker the same way Pull.jsx:668 already passes mdCtx.
No wire/API change required: all rendering inputs (thread state, pr.merged, review bodies) are already in the existing payloads.

Acceptance criteria

PR conversation header matches the issue page idiom: bordered header block, #num title heading, state chip (open/closed/merged) beside the title, muted byline below.
chip-merged defined in ui.css consistent with the other chips (purple or the app's chosen merged hue, light + dark), and merged PRs render it on both the conversation page and the pulls list (chip-${state} path).
Closed PRs show the closed chip (not lowercase prose) on the conversation page.
Review bodies (Pull.jsx:136) and thread comments (Pull.jsx:470) render through renderBody with the same mdCtx the timeline gets; PR description renders as a styled first-comment body, not just a timeline row.
Page uses the issue-page grid idiom; sidebar cards (Mergeability, Reviewers, Checks, Merge) share one card-header treatment with consistent heading hierarchy.
No change to wire payloads, pinned hash twins (anchorContextSha untouched), or the ThreadTimeline contract; existing markdown/DOMPurify behavior on comment bodies unchanged.

## What's requested Redesign the PR conversation page (`web/src/pages/Pull.jsx`) to match the design language the issue page already establishes (`web/src/pages/Issue.jsx`), so the two conversation surfaces read as siblings. Three areas: page structure/state display, PR-body rendering, and open/merged state consistency. **Reference implementation:** the issue page (`web/src/pages/Issue.jsx`), which is the canonical pattern for this app's conversation surfaces: an `issue-page` grid (Issue.jsx:422), a bordered header block with `#num title` heading and a state chip (Issue.jsx:434–442), a muted byline, and a right sidebar of grouped metadata cards (Labels / Assignees / Milestone, Issue.jsx:549+). ## Evidence (static read of the current tree) **1. Header/state display diverges from the issue page** - Pull.jsx:654–662: bare `<h1>#{num} {title}</h1>` followed by a plain text line `{thread()?.state} · {author} · {date}` — the state renders as lowercase prose, not the app's state chip. Issue.jsx:436–442 pairs the same heading with `chip chip-open` / `chip chip-closed` and a `border-b` header block. - `web/src/ui.css` (lines ~108–116) defines `chip-open`, `chip-closed`, `chip-draft`, `chip-prerelease` — but **no `chip-merged`**. The PR list (`Pulls.jsx:124`) already emits `chip chip-${pr.state}`, so a merged PR renders a chip with an unstyled class (the known unstyled-class failure mode). The conversation page needs a merged chip and so does the list. **2. PR-body rendering is inconsistent** - Comment bodies in the timeline go through `renderBody` (markdown + DOMPurify) via ThreadTimeline.jsx:83 — correct. - But Pull.jsx:136 (review bodies) and Pull.jsx:470 (thread comments under `ThreadComments`) render raw `whitespace-pre-wrap` text — no markdown, unlike the issue thread. Staged line comments in the finish-review modal (Pull.jsx:514) are plain text, acceptable as a draft preview but the submitted review body is not. - The PR description itself surfaces only as the `opened` event row inside the timeline; there is no dedicated, styled PR-body block like the issue page's first-comment treatment (Issue.jsx:430–433 describes the intended pattern: number + title header with byline, body as first comment). **3. Page structure** - Pull.jsx:655 uses a generic `grid gap-6 lg:grid-cols-[1fr_320px]` rather than the issue page's `issue-page` idiom; the Review summary / Reviews / Reviewers / Mergeability / Checks cards carry ad-hoc `h2 text-sm` headings at mixed hierarchy with no unified card-header treatment, and the ReviewSummaryBar floats between header and timeline rather than composing into the header/sidebar structure. ## Architecture notes - State chip vocabulary lives in one place (`ui.css`); adding `chip-merged` there covers the list page for free. `chip-${state}` interpolation in Pulls.jsx:124 needs the CSS rule to exist or a mapping for `merged`. - ThreadTimeline is the ONE P3 renderer for issue threads and PR conversations — do not fork it; the raw-text call sites (Pull.jsx:136, 470) should route through `renderBody` from `web/src/lib/render-md.js` like the timeline does. Review bodies carry the repo context for the autolinker the same way Pull.jsx:668 already passes `mdCtx`. - No wire/API change required: all rendering inputs (thread state, pr.merged, review bodies) are already in the existing payloads. ## Acceptance criteria - [ ] PR conversation header matches the issue page idiom: bordered header block, `#num title` heading, state chip (open/closed/merged) beside the title, muted byline below. - [ ] `chip-merged` defined in `ui.css` consistent with the other chips (purple or the app's chosen merged hue, light + dark), and merged PRs render it on both the conversation page and the pulls list (`chip-${state}` path). - [ ] Closed PRs show the closed chip (not lowercase prose) on the conversation page. - [ ] Review bodies (Pull.jsx:136) and thread comments (Pull.jsx:470) render through `renderBody` with the same mdCtx the timeline gets; PR description renders as a styled first-comment body, not just a timeline row. - [ ] Page uses the issue-page grid idiom; sidebar cards (Mergeability, Reviewers, Checks, Merge) share one card-header treatment with consistent heading hierarchy. - [ ] No change to wire payloads, pinned hash twins (`anchorContextSha` untouched), or the ThreadTimeline contract; existing markdown/DOMPurify behavior on comment bodies unchanged.

crueber added the

pull requests

label

2026-09-14 15:47:41 +00:00

crueber added this to the v1 milestone

2026-09-14 15:47:41 +00:00

crueber referenced this issue from a commit

2026-09-14 18:18:09 +00:00

Redesign PR conversation page to issue-page idiom (Forgejo #521)

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

2026-09-14 18:18:11 +00:00

Fix #521: PR page redesign #528

crueber commented

2026-09-14 18:18:23 +00:00

Author

Owner

Fix ready for review: #528 (branch fix/issue-521). Client-only, no wire change, ThreadTimeline untouched; full node --test 1191/1190 (1 pre-existing live-server smoke failure, verified head-to-head); vite + esbuild green; no new deps.

Fix ready for review: https://git.packden.us/crueber/walhub/pulls/528 (branch fix/issue-521). Client-only, no wire change, ThreadTimeline untouched; full node --test 1191/1190 (1 pre-existing live-server smoke failure, verified head-to-head); vite + esbuild green; no new deps.

crueber commented

2026-09-14 18:25:35 +00:00

Author

Owner

Review of PR #528 (fix/issue-521) — verified in scratch worktree @ `f1caa05`

No browser used (per review instructions: node tests + reasoning; browser proof explicitly open). No .go touched, no live instance touched.

(1) Header idiom — PASS

Pull.jsx:754-766: bordered header block (mb-4 border-b ... pb-3), h1 with muted #num span + title, single badge right (badge().cls, exactly 1 surface — no stacked second badge), muted byline author · date below. Matches Issue.jsx:434-449 idiom (h1-vs-h2 level difference is correct: page title vs card titles). #517 badge unified via pullBadgeView (open/closed/merged) — no double badge.

(2) chip-merged — PASS (pre-existing from #517, not this PR)

chip-merged already ships in main's ui.css:114 (purple light+dark). This PR only uses it via the #517 badge path. Nothing to add.

(3) LIST merged chip — DECLINE VERIFIED CORRECT, contradiction in the issue itself

Claim checked against source: PROut (internal/pulls/service.go:731-741) carries Num/Title/State/Author/BaseRef/HeadRef/HeadSHA/Draft/UpdatedAt only — no merged signal. ListPRs (service.go:801-805) loads each sidecar (pr.Merged in hand) but drops it; State comes from the index Card (open|closed). So merged vs plain-closed are indistinguishable on the list. Wire-clean alternatives: per-row GET pulls/{num} = N+1 round trips (violates laws 4+6); inferring merged from state==closed = wrong. A list chip genuinely needs a wire change (add merged to PROut), which criterion 6 forbids. Ruling: declining is the right call — criteria "list merged chip" and "no wire change" are mutually exclusive given the PROut shape, and the issue's "all inputs already in the payloads" premise is wrong for the list payload (true only for the conversation payload). The chip-merged CSS stands ready. Recommend: close #521 on the conversation-page scope (all achievable criteria met) + open a follow-up ticket for PROut.merged wire addition + list chip; or keep #521 open solely for the list part — maintainer's call.

(4) Markdown bodies — PASS

Review bodies (Pull.jsx:146) + thread comments (Pull.jsx:482) go through renderBody with one shared repo mdCtx (Pull.jsx:728, the #340 contract); exactly one whitespace-pre-wrap site remains — the finish-review modal staged-line draft preview (Pull.jsx:526), plain by design. Timeline bodies unchanged.

(5) PR description first-comment — PASS, reasoning sound

PRDescription (Pull.jsx:30-49) renders live pr.body, hidden on empty (never an empty box), byline from the opened event's actor/at with thread fallback. Stale-row claim verified: opened event carries ORIGINAL body (service.go:406 Body: strPtr(in.Body), seq 0); UpdatePR (§8 PUT) writes pr.json only, appends events solely for title/state — so the old timeline body row would go stale AND duplicate. pullEventText opened→system row keeps the event-0 anchor (ThreadTimeline.jsx:62,74 id=event-{seq} untouched) — deep links hold.

(6) Structure — PASS

issue-page grid gap-4 md:grid-cols-[1fr_16rem] + min-w-0 main (Pull.jsx:747, byte-identical to Issue.jsx:422); sidebar grid content-start gap-3 aria-label "Details" with ReviewSummaryBar composed first (Pull.jsx:868-869); one .card-header (ui.css:128) on all 8 titles incl. MergeBox.jsx:159; Checks composes flex items-center gap-2 alongside. No stray mb-2 text-sm font-semibold h2 remains.

(7) Untouched contracts — PASS

ThreadTimeline.jsx: zero diff. anchorContextSha: zero diff hits. pullEventText moved verbatim from Pull.jsx (sole delta: opened→system row, as designed); no local eventText fork remains.

(8) #517 no-regress — PASS

pullBadgeView/pullCloseVisibility untouched (diff only adds pullEventText); Close/Reopen/comment-and-close wiring intact (Pull.jsx:706-737,789-794); pull-state-517 tests pass.

(9) Laws/docs — PASS

Law 1: no new deps (no package.json/pnpm change). Law 7: no task/async change (pure render). Law 8: no new seams (no registry additions). Law 12: decision appended to docs/go/12_web_ui.md; doc claims spot-checked true (chip-merged pre-existence, PROut shape, UpdatePR behavior, opened-event body). No wire change (no .go files in diff).

Tests (scratch worktree, node_modules symlinked from main)

Targeted: pull-event-text-521 + pull-state-517 + refs-autolink — 40/40 pass.
Full suite minus smoke: 1188/1188 pass. smoke.test.js fails on the live-server subtest (/setup 403, no live Go server in this env) — matches the PR's stated pre-existing failure.
vite build: green (2.58s).

MERGE RECOMMENDATION: ready to merge

Only open question is the process call on criterion (3): accept the decline (recommend close #521 + follow-up ticket for PROut.merged) or keep #521 open for the list part. Either way, #528 itself is correct and complete — do not hold it for the wire change.

## Review of PR #528 (fix/issue-521) — verified in scratch worktree @ f1caa05 No browser used (per review instructions: node tests + reasoning; browser proof explicitly open). No .go touched, no live instance touched. ### (1) Header idiom — PASS Pull.jsx:754-766: bordered header block (`mb-4 border-b ... pb-3`), h1 with muted `#num` span + title, single badge right (`badge().cls`, exactly 1 surface — no stacked second badge), muted byline author · date below. Matches Issue.jsx:434-449 idiom (h1-vs-h2 level difference is correct: page title vs card titles). #517 badge unified via `pullBadgeView` (open/closed/merged) — no double badge. ### (2) chip-merged — PASS (pre-existing from #517, not this PR) `chip-merged` already ships in main's ui.css:114 (purple light+dark). This PR only *uses* it via the #517 badge path. Nothing to add. ### (3) LIST merged chip — DECLINE VERIFIED CORRECT, contradiction in the issue itself Claim checked against source: `PROut` (internal/pulls/service.go:731-741) carries `Num/Title/State/Author/BaseRef/HeadRef/HeadSHA/Draft/UpdatedAt` only — **no merged signal**. `ListPRs` (service.go:801-805) loads each sidecar (`pr.Merged` in hand) but drops it; `State` comes from the index Card (open|closed). So merged vs plain-closed are indistinguishable on the list. Wire-clean alternatives: per-row `GET pulls/{num}` = N+1 round trips (violates laws 4+6); inferring merged from `state==closed` = wrong. A list chip genuinely needs a wire change (add `merged` to PROut), which criterion 6 forbids. **Ruling: declining is the right call — criteria "list merged chip" and "no wire change" are mutually exclusive given the PROut shape, and the issue's "all inputs already in the payloads" premise is wrong for the *list* payload (true only for the conversation payload).** The `chip-merged` CSS stands ready. Recommend: close #521 on the conversation-page scope (all achievable criteria met) + open a follow-up ticket for `PROut.merged` wire addition + list chip; or keep #521 open solely for the list part — maintainer's call. ### (4) Markdown bodies — PASS Review bodies (Pull.jsx:146) + thread comments (Pull.jsx:482) go through `renderBody` with one shared repo `mdCtx` (Pull.jsx:728, the #340 contract); exactly one `whitespace-pre-wrap` site remains — the finish-review modal staged-line draft preview (Pull.jsx:526), plain by design. Timeline bodies unchanged. ### (5) PR description first-comment — PASS, reasoning sound `PRDescription` (Pull.jsx:30-49) renders live `pr.body`, hidden on empty (never an empty box), byline from the opened event's actor/at with thread fallback. Stale-row claim verified: opened event carries ORIGINAL body (service.go:406 `Body: strPtr(in.Body)`, seq 0); `UpdatePR` (§8 PUT) writes pr.json only, appends events solely for title/state — so the old timeline body row would go stale AND duplicate. `pullEventText` opened→system row keeps the event-0 anchor (ThreadTimeline.jsx:62,74 `id=event-{seq}` untouched) — deep links hold. ### (6) Structure — PASS `issue-page grid gap-4 md:grid-cols-[1fr_16rem]` + `min-w-0` main (Pull.jsx:747, byte-identical to Issue.jsx:422); sidebar `grid content-start gap-3` aria-label "Details" with ReviewSummaryBar composed first (Pull.jsx:868-869); one `.card-header` (ui.css:128) on all 8 titles incl. MergeBox.jsx:159; Checks composes `flex items-center gap-2` alongside. No stray `mb-2 text-sm font-semibold` h2 remains. ### (7) Untouched contracts — PASS ThreadTimeline.jsx: zero diff. `anchorContextSha`: zero diff hits. `pullEventText` moved verbatim from Pull.jsx (sole delta: opened→system row, as designed); no local `eventText` fork remains. ### (8) #517 no-regress — PASS `pullBadgeView`/`pullCloseVisibility` untouched (diff only *adds* `pullEventText`); Close/Reopen/comment-and-close wiring intact (Pull.jsx:706-737,789-794); `pull-state-517` tests pass. ### (9) Laws/docs — PASS Law 1: no new deps (no package.json/pnpm change). Law 7: no task/async change (pure render). Law 8: no new seams (no registry additions). Law 12: decision appended to docs/go/12_web_ui.md; doc claims spot-checked true (chip-merged pre-existence, PROut shape, UpdatePR behavior, opened-event body). No wire change (no .go files in diff). ### Tests (scratch worktree, node_modules symlinked from main) - Targeted: pull-event-text-521 + pull-state-517 + refs-autolink — 40/40 pass. - Full suite minus smoke: 1188/1188 pass. smoke.test.js fails on the live-server subtest (/setup 403, no live Go server in this env) — matches the PR's stated pre-existing failure. - `vite build`: green (2.58s). ### MERGE RECOMMENDATION: ready to merge Only open question is the process call on criterion (3): accept the decline (recommend close #521 + follow-up ticket for PROut.merged) or keep #521 open for the list part. Either way, #528 itself is correct and complete — do not hold it for the wire change.

crueber referenced this issue

2026-09-14 18:25:56 +00:00

PROut needs merged flag so the pulls list can render the merged chip #530

crueber closed this issue

2026-09-14 18:26:09 +00:00

crueber commented

2026-09-14 18:26:10 +00:00

Author

Owner

Fixed by PR #528 (review clean — all 9 checks pass; list-chip scoped to follow-up #530 since it needs a wire change), merged. Closing.

crueber referenced this issue from a commit

2026-09-14 18:26:10 +00:00