Inline thread comments render newest-first, putting the newest reply farthest from the reply textbox #597

New issue

Closed

opened 2026-09-15 21:14:43 +00:00 by crueber · 1 comment

crueber commented

2026-09-15 21:14:43 +00:00

Owner

What's requested

Thread comments in inline thread cards must render chronologically (oldest first, newest last), consistent with the chronological thread rendering the issue page already standardizes (#225).

Evidence (static code reading; no local reproduction per standing rule)

Wire contract: GET …/pulls/{num}/threads/{tid} deliberately returns comments newest-first — internal/review/threads.go:200-250 (GetThread walks scanThreadComments' seq-ascending list in reverse; header comment at line 201 says "comments newest-first, after = exclusive upper bound"). This is the #225/#146 wire design and must not change.
The render site ignores this: ThreadComments in web/src/pages/Pull.jsx:915-932 renders getView()?.comments in raw wire order inside <For each={getView()?.comments ?? []}>. A thread card therefore shows reply #1 at the bottom and the newest reply at the top, directly above nothing useful — the reply <form> (Pull.jsx:816-821) sits below the list, so the newest response is maximally far from where the user types.
The repo already solved exactly this class for the issue timeline: web/src/lib/thread-order.js (chronological(), stable ascending-by-seq sort, headless-tested, issue #225) with the documented rule "wire stays newest-first … the render sorts by seq" (docs/features/02_issues.md:518-519, docs/features/08_ui_sdk.md:60). ThreadComments never consumes it.

Architecture notes

Server change: none. The newest-first wire order is a spec decision (02 §7 Decisions); the fix is render-side only, mirroring #225's pattern.
Reuse the existing shared helper: import chronological from web/src/lib/thread-order.js in Pull.jsx and render chronological(getView()?.comments) in ThreadComments' <For>. Do not re-implement a sort inline — the helper exists precisely so every surface agrees (also see the skill's note that a stale cache window or SSE refetch can never show wire order).
Seq is present on every ThreadComment (internal/review/model.go:128-129, kind review_thread_comment), so the sort key is always available.
Windowing caveat for the implementer to note in the fix: with more: true (server caps n at 200, default 50), the returned window is the newest n comments; reversing that window yields correct chronological order within the window. If inline cards ever grow an "older comments" pager, it must follow the appendOlderWindow + chronological assembly pattern from thread-order.js rather than naive concatenation. Current UI loads a single window, so reversing is sufficient today.
Render surfaces today: ThreadComments is the ONLY consumer of pulls.threads.get().comments in the SPA (verified by grep over web/src); PullFiles.jsx only creates threads. If a second surface for thread comments exists by the time this lands, it must consume the same chronological() helper — that is the consistency requirement.

Acceptance criteria

Inline thread cards render comments oldest-first; the newest comment is the last row, immediately above the reply input.
The fix reuses chronological() from web/src/lib/thread-order.js; no duplicated sort logic.
Wire responses remain byte-identical newest-first (no API/SDK change; SDK typedefs untouched).
Headless unit test (Node, per the thread-order.js convention) pins the rendered order for a newest-first input window, including the more: true window case and missing-seq fallback behavior.
No other surface regresses: the issue timeline (#225 behavior) and the PR conversation timeline are untouched.
Render verification before close: headless DOM assertion (or screenshot review) that a multi-comment thread card shows the newest reply adjacent above the reply textbox — code reading alone is not sufficient to close (standing #533-style mandate).

Inline review-thread comments render in wire order (newest-first), so the newest reply appears at the TOP of the thread card — farthest from the reply textbox at the bottom. Chat-style reading (and the app's own established convention) requires oldest at top, newest last, so the newest response sits immediately above the reply box. ## What's requested Thread comments in inline thread cards must render chronologically (oldest first, newest last), consistent with the chronological thread rendering the issue page already standardizes (#225). ## Evidence (static code reading; no local reproduction per standing rule) - Wire contract: `GET …/pulls/{num}/threads/{tid}` deliberately returns comments **newest-first** — `internal/review/threads.go:200-250` (`GetThread` walks `scanThreadComments`' seq-ascending list in reverse; header comment at line 201 says "comments newest-first, after = exclusive upper bound"). This is the #225/#146 wire design and must not change. - The render site ignores this: `ThreadComments` in `web/src/pages/Pull.jsx:915-932` renders `getView()?.comments` in raw wire order inside `<For each={getView()?.comments ?? []}>`. A thread card therefore shows reply #1 at the bottom and the newest reply at the top, directly above nothing useful — the reply `<form>` (Pull.jsx:816-821) sits below the list, so the newest response is maximally far from where the user types. - The repo already solved exactly this class for the issue timeline: `web/src/lib/thread-order.js` (`chronological()`, stable ascending-by-seq sort, headless-tested, issue #225) with the documented rule "wire stays newest-first … the render sorts by seq" (`docs/features/02_issues.md:518-519`, `docs/features/08_ui_sdk.md:60`). `ThreadComments` never consumes it. ## Architecture notes - Server change: none. The newest-first wire order is a spec decision (02 §7 Decisions); the fix is render-side only, mirroring #225's pattern. - Reuse the existing shared helper: import `chronological` from `web/src/lib/thread-order.js` in Pull.jsx and render `chronological(getView()?.comments)` in `ThreadComments`' `<For>`. Do not re-implement a sort inline — the helper exists precisely so every surface agrees (also see the skill's note that a stale cache window or SSE refetch can never show wire order). - `Seq` is present on every `ThreadComment` (`internal/review/model.go:128-129`, kind `review_thread_comment`), so the sort key is always available. - Windowing caveat for the implementer to note in the fix: with `more: true` (server caps n at 200, default 50), the returned window is the newest n comments; reversing that window yields correct chronological order within the window. If inline cards ever grow an "older comments" pager, it must follow the `appendOlderWindow` + `chronological` assembly pattern from thread-order.js rather than naive concatenation. Current UI loads a single window, so reversing is sufficient today. - Render surfaces today: `ThreadComments` is the ONLY consumer of `pulls.threads.get().comments` in the SPA (verified by grep over `web/src`); `PullFiles.jsx` only creates threads. If a second surface for thread comments exists by the time this lands, it must consume the same `chronological()` helper — that is the consistency requirement. ## Acceptance criteria - [ ] Inline thread cards render comments oldest-first; the newest comment is the last row, immediately above the reply input. - [ ] The fix reuses `chronological()` from `web/src/lib/thread-order.js`; no duplicated sort logic. - [ ] Wire responses remain byte-identical newest-first (no API/SDK change; SDK typedefs untouched). - [ ] Headless unit test (Node, per the thread-order.js convention) pins the rendered order for a newest-first input window, including the `more: true` window case and missing-`seq` fallback behavior. - [ ] No other surface regresses: the issue timeline (#225 behavior) and the PR conversation timeline are untouched. - [ ] Render verification before close: headless DOM assertion (or screenshot review) that a multi-comment thread card shows the newest reply adjacent above the reply textbox — code reading alone is not sufficient to close (standing #533-style mandate).

crueber added the

bug

issues

labels

2026-09-15 21:14:49 +00:00

crueber added this to the v1 milestone

2026-09-15 21:14:49 +00:00

crueber added the

pull requests

label

2026-09-15 21:15:23 +00:00

crueber referenced this issue from a commit

2026-09-15 21:35:23 +00:00

Render inline thread comments oldest-first via shared chronological() (Forgejo #597)

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

2026-09-15 21:35:24 +00:00

Render inline thread comments oldest-first (Fix #597) #604

crueber referenced this issue

2026-09-15 21:37:34 +00:00

Render inline thread comments oldest-first (Fix #597) #604

crueber referenced this issue from a commit

2026-09-15 21:37:48 +00:00

Merge pull request 'Render inline thread comments oldest-first (Fix #597)' (#604) from fix/issue-597 into main

crueber closed this issue

2026-09-15 21:37:48 +00:00

crueber commented

2026-09-15 21:37:49 +00:00

Author

Owner

Fixed by #604 (merged): ThreadComments renders chronological(comments) via the shared #225 helper (non-mutating, wire untouched, single-consumer verified); newest reply now sits adjacent above the reply input. Verified: 15/15 new + 1510 full-minus-smoke green, vite/esbuild green, independent review APPROVE.