Issue timeline should read oldest → newest (oldest top, newest bottom)

The issue view tab currently renders messages newest-first (newest at top). It should read chronologically: oldest at top, newest landing at the bottom (GitHub convention).

Scope

Issue thread timeline (opened event first, comments in seq order, newest comment last).
Apply to the shared ThreadTimeline so PR conversations match (same component, same convention — verify; do not leave the two surfaces inconsistent).
Pagination + live updates must compose with the direction: "load older" fetches earlier windows ABOVE, SSE appends new events AT THE BOTTOM, auto-scroll behavior sane (don't yank a reading user; standard: stick to bottom only if already near it, or don't autoscroll at all — pick and document).
System/event rows follow the same order (they're seq-ordered already — keep them in place).

Acceptance criteria

Threads read top-to-bottom chronological on issues (and PRs if shared).
Older-window loading + live SSE appends compose without duplication or jumps.
node --test green (ordering tests); browser check (long thread: top is oldest, bottom newest, load-older + live append sane) both themes, zero console errors; no new deps.

# Issue timeline should read oldest → newest (oldest top, newest bottom) The issue view tab currently renders messages newest-first (newest at top). It should read chronologically: oldest at top, newest landing at the bottom (GitHub convention). ## Scope - Issue thread timeline (opened event first, comments in seq order, newest comment last). - Apply to the shared `ThreadTimeline` so PR conversations match (same component, same convention — verify; do not leave the two surfaces inconsistent). - Pagination + live updates must compose with the direction: "load older" fetches earlier windows ABOVE, SSE appends new events AT THE BOTTOM, auto-scroll behavior sane (don't yank a reading user; standard: stick to bottom only if already near it, or don't autoscroll at all — pick and document). - System/event rows follow the same order (they're seq-ordered already — keep them in place). ## Acceptance criteria - [ ] Threads read top-to-bottom chronological on issues (and PRs if shared). - [ ] Older-window loading + live SSE appends compose without duplication or jumps. - [ ] `node --test` green (ordering tests); browser check (long thread: top is oldest, bottom newest, load-older + live append sane) both themes, zero console errors; no new deps.

crueber referenced this issue from a commit

2026-09-09 03:48:19 +00:00

Fix #225: chronological thread timelines (02 §11, 08 §2)

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

2026-09-09 03:48:26 +00:00

Fix #225: chronological timeline #226

crueber commented

2026-09-09 03:48:31 +00:00

Author

Owner

Fix is up in PR #226 (#226), branch fix/issue-225 — not merged, needs the real-Chromium pass first. Summary: shared ThreadTimeline now sorts stable-by-seq at render (oldest top, newest bottom); wire unchanged (newest-first windows, client-side adaptation — the API matches 02 §7, so no backend change). Older-events button sits above the thread, viewport pinned across prepends, SSE appends land at the bottom with no autoscroll. PR conversations inherit with zero PR-side code. node --test 438/438 green (incl. 8 new ordering tests), Go TestGetIssueHTTP green, no new deps. Open: browser check (long thread, both themes, zero console errors) — :9222 was down in this environment.

Fix is up in PR #226 (https://git.packden.us/crueber/walhub/pulls/226), branch fix/issue-225 — not merged, needs the real-Chromium pass first. Summary: shared ThreadTimeline now sorts stable-by-seq at render (oldest top, newest bottom); wire unchanged (newest-first windows, client-side adaptation — the API matches 02 §7, so no backend change). Older-events button sits above the thread, viewport pinned across prepends, SSE appends land at the bottom with no autoscroll. PR conversations inherit with zero PR-side code. node --test 438/438 green (incl. 8 new ordering tests), Go TestGetIssueHTTP green, no new deps. Open: browser check (long thread, both themes, zero console errors) — :9222 was down in this environment.

crueber commented

2026-09-09 03:58:52 +00:00

Author

Owner

Review of PR #226 (fix/issue-225, now at b9e3857 with my one review fix pushed):

VERIFIED (scratch worktree /tmp/pr226, removed afterward; main untouched):

node full unit suite 438/438 green (incl. 8 new thread-order tests). Note: suite needs web/node_modules present — scratch worktree initially lacked it (md-urls failed on missing 'marked'); symlinked/copied from main checkout, then all green. Full run ~260s.
vite build clean + esbuild SDK bundle clean. gofmt/vet clean, Go TestGetIssueHTTP green.
No dep changes: web/package.json + go.mod untouched; runtime deps exactly the allowed four (law 1).

REVIEW CHECKS:

Stable seq sort correct (web/src/lib/thread-order.js:16-24): numeric subtraction (never lexicographic), kind-agnostic so mixed comment/system rows stay seq-interleaved, index tiebreak for equal seqs. One fix pushed (b9e3857): both-missing-seq previously returned NaN from the comparator (behaviorally saved by NaN→0 coercion + stable sort, but implicit) — now falls to the index tiebreak explicitly. Re-tested 51/51 across thread-order + adjacent suites, vite rebuilt.
PR consistency: Pull.jsx untouched but renders through the same sorted path (Pull.jsx:645 passes wire-order events into ThreadTimeline, which now sorts once) — consistent by construction, confirmed.
Older-button-above + cursor-from-tail: walked it. Cursor reads the UNFILTERED newestFirst() tail, after_seq pages strictly below → no overlap, no visible dup. New 'oldest <= 0' guard also kills the refetch-newest-page case (old code only guarded empty). Button moved above thread, newest lands by composer. Correct.
Viewport pinning sane: anchorScrollTop = top + height-delta, applied only when scrollHeight changed, document-undefined guarded. SSE never autoscrolls — documented in ThreadTimeline header, Issue.jsx loadOlder comment, and 08 Decisions. Correct.
Wire unchanged: only backend touch is the http_test.go comment/assertion-text fix; assertion logic identical (it always asserted newest-first — the old 'newest-last' label was the misnomer). SDK issues.js comment fix accurate per 02 §7.
Laws: 1 (no new deps), 7 (Loading… disabled state, no silent wait), 8 (pure lib helper, no registry change), 12 (Decisions appended to 02/03/08 in same change) — all hold. Doc entries match the code (08 contract: issue passes newest-first assembly, PR passes events as-is — both true in code).

OPEN / NOT DONE BY ME:

Browser: hub CDP :9222 connection-refused (no headless browser in this env), so the long-thread top-oldest/bottom-newest + load-older + live-append check in both themes with zero console errors is still outstanding — same as the PR description states.

RECOMMENDATION: blocked — real-Chromium pass still required before merge (PR description agrees: do NOT merge before that). Code itself is ready.

Review of PR #226 (fix/issue-225, now at b9e3857 with my one review fix pushed): VERIFIED (scratch worktree /tmp/pr226, removed afterward; main untouched): - node full unit suite 438/438 green (incl. 8 new thread-order tests). Note: suite needs web/node_modules present — scratch worktree initially lacked it (md-urls failed on missing 'marked'); symlinked/copied from main checkout, then all green. Full run ~260s. - vite build clean + esbuild SDK bundle clean. gofmt/vet clean, Go TestGetIssueHTTP green. - No dep changes: web/package.json + go.mod untouched; runtime deps exactly the allowed four (law 1). REVIEW CHECKS: - Stable seq sort correct (web/src/lib/thread-order.js:16-24): numeric subtraction (never lexicographic), kind-agnostic so mixed comment/system rows stay seq-interleaved, index tiebreak for equal seqs. One fix pushed (b9e3857): both-missing-seq previously returned NaN from the comparator (behaviorally saved by NaN→0 coercion + stable sort, but implicit) — now falls to the index tiebreak explicitly. Re-tested 51/51 across thread-order + adjacent suites, vite rebuilt. - PR consistency: Pull.jsx untouched but renders through the same sorted path (Pull.jsx:645 passes wire-order events into ThreadTimeline, which now sorts once) — consistent by construction, confirmed. - Older-button-above + cursor-from-tail: walked it. Cursor reads the UNFILTERED newestFirst() tail, after_seq pages strictly below → no overlap, no visible dup. New 'oldest <= 0' guard also kills the refetch-newest-page case (old code only guarded empty). Button moved above thread, newest lands by composer. Correct. - Viewport pinning sane: anchorScrollTop = top + height-delta, applied only when scrollHeight changed, document-undefined guarded. SSE never autoscrolls — documented in ThreadTimeline header, Issue.jsx loadOlder comment, and 08 Decisions. Correct. - Wire unchanged: only backend touch is the http_test.go comment/assertion-text fix; assertion logic identical (it always asserted newest-first — the old 'newest-last' label was the misnomer). SDK issues.js comment fix accurate per 02 §7. - Laws: 1 (no new deps), 7 (Loading… disabled state, no silent wait), 8 (pure lib helper, no registry change), 12 (Decisions appended to 02/03/08 in same change) — all hold. Doc entries match the code (08 contract: issue passes newest-first assembly, PR passes events as-is — both true in code). OPEN / NOT DONE BY ME: - Browser: hub CDP :9222 connection-refused (no headless browser in this env), so the long-thread top-oldest/bottom-newest + load-older + live-append check in both themes with zero console errors is still outstanding — same as the PR description states. RECOMMENDATION: blocked — real-Chromium pass still required before merge (PR description agrees: do NOT merge before that). Code itself is ready.

crueber commented

2026-09-09 04:17:55 +00:00

Author

Owner

Browser verdict for PR #226 (fix/issue-225, commit b9e3857): PASS — real HeadlessChrome 151 over CDP against a scratch server (branch-built web + Go binary, port 18081, repo demo/thread issue #1 seeded with 60+ comments + close/reopen system events).

Per-check results:

Chronological order: PASS — initial view renders 50 rows oldest(top, event-13) → newest(bottom, event-62), strictly ascending by seq, no duplicates.
Older-events pagination: PASS — 'Older events' button sits above the timeline (btnTop 269 < olTop 311); click prepends earlier window above (full thread event-0..event-62, contiguous, zero dupes); button disappears once fully loaded.
3a. Own comment via composer: PASS — typed + submitted in-browser, new row lands at bottom (event-63, 'browser-check comment bottom').
3b. Remote SSE comment: PASS — POST via API while page open appends at bottom (event-64, 'remote SSE comment append') with older windows intact (top stays event-0), no duplicated rows.
Themes: PASS — dark default renders; toggled to light via the header button, both render cleanly.
Console/page errors: PASS — zero (no console.error, no page exceptions, no error-level log entries across the whole session).

Screenshots (800x600, on the verification host):

/tmp/wt225-shots/timeline-top-dark.png (thread top, dark)
/tmp/wt225-shots/timeline-bottom-dark.png (thread bottom, dark)
/tmp/wt225-shots/timeline-top-light.png (thread top, light)
/tmp/wt225-shots/timeline-bottom-light.png (thread bottom, light)

Observation (pre-existing, NOT introduced by #226 — report-only, no fix applied): when older windows are loaded and then a remote SSE event slides the newest-50 view, one boundary row drops from the render (this run: range event-0..event-64 rendered 64 rows with seq 14 missing; view 15..64 + pinned extras 0..13). Mechanism: Issue.jsx reload() clears extras on own mutations but the SSE path (useCollabStream -> invalidateCollab, web/src/components/collab.jsx) refetches only the view, so the slid window + pinned extras leave a one-seq hole; more() reads the (now-false) extra flag so the hole persists until full reload. Repro: load full history, POST a comment from elsewhere, diff rendered event-N ids vs server log. The new olderCursor()/appendOlderWindow() docs (web/src/lib/thread-order.js) cover cursor overlap, not this slide — may deserve a follow-up issue. Ready to merge from the browser side.

Browser verdict for PR #226 (fix/issue-225, commit b9e3857): PASS — real HeadlessChrome 151 over CDP against a scratch server (branch-built web + Go binary, port 18081, repo demo/thread issue #1 seeded with 60+ comments + close/reopen system events). Per-check results: 1. Chronological order: PASS — initial view renders 50 rows oldest(top, event-13) → newest(bottom, event-62), strictly ascending by seq, no duplicates. 2. Older-events pagination: PASS — 'Older events' button sits above the timeline (btnTop 269 < olTop 311); click prepends earlier window above (full thread event-0..event-62, contiguous, zero dupes); button disappears once fully loaded. 3a. Own comment via composer: PASS — typed + submitted in-browser, new row lands at bottom (event-63, 'browser-check comment bottom'). 3b. Remote SSE comment: PASS — POST via API while page open appends at bottom (event-64, 'remote SSE comment append') with older windows intact (top stays event-0), no duplicated rows. 4. Themes: PASS — dark default renders; toggled to light via the header button, both render cleanly. 5. Console/page errors: PASS — zero (no console.error, no page exceptions, no error-level log entries across the whole session). Screenshots (800x600, on the verification host): - /tmp/wt225-shots/timeline-top-dark.png (thread top, dark) - /tmp/wt225-shots/timeline-bottom-dark.png (thread bottom, dark) - /tmp/wt225-shots/timeline-top-light.png (thread top, light) - /tmp/wt225-shots/timeline-bottom-light.png (thread bottom, light) Observation (pre-existing, NOT introduced by #226 — report-only, no fix applied): when older windows are loaded and then a remote SSE event slides the newest-50 view, one boundary row drops from the render (this run: range event-0..event-64 rendered 64 rows with seq 14 missing; view 15..64 + pinned extras 0..13). Mechanism: Issue.jsx reload() clears extras on own mutations but the SSE path (useCollabStream -> invalidateCollab, web/src/components/collab.jsx) refetches only the view, so the slid window + pinned extras leave a one-seq hole; more() reads the (now-false) extra flag so the hole persists until full reload. Repro: load full history, POST a comment from elsewhere, diff rendered event-N ids vs server log. The new olderCursor()/appendOlderWindow() docs (web/src/lib/thread-order.js) cover cursor overlap, not this slide — may deserve a follow-up issue. Ready to merge from the browser side.

crueber closed this issue

2026-09-09 04:19:25 +00:00

crueber commented

2026-09-09 04:19:26 +00:00

Author

Owner

Fixed by PR #226 (review + real-Chromium pass: order, older-load, live-append verified; all green), merged. Closing.

crueber referenced this issue

2026-09-09 04:19:26 +00:00

SSE event with pinned older windows leaves a one-seq hole #227

crueber referenced this issue from a commit

2026-09-09 04:19:26 +00:00