Fix #567: staged inline review comments render in-thread until submit #570

Merged

crueber merged 2 commits from fix/issue-567 into main

2026-09-15 13:38:33 +00:00

crueber commented

2026-09-15 13:34:09 +00:00

Owner

Implements Forgejo issue #567 per the proposed fix.

Problem: staging a keyed draft handed {anchor, body} to the page pending list and unmounted the composer — the row showed nothing until Finish review.

Fix (web/src/pages/Pull.jsx + CommentComposer.jsx):

Page-level pending list (same signal as FinishReview list + button count, no forked state) locates per row into StagedCards below the anchor line, same ml-14 mt-1 card slot as composer/ThreadCard.
Card: stager author + plain-text body (FinishReview-consistent) + canonical amber chip-draft pill (both themes, no per-callsite bg override per #545 F2) + staged- id with emerald flash.

Edit re-opens the keyed composer pre-filled (new CommentComposer initialValue; Save vs Stage comment label); submit resolves the pending index fresh by anchor (mid-edit removals safe, vanished entries stage anew). Cancel drops only the draft; remove shares the pending-list unstage with the modal.

Unlocatable staged anchors render once at the file end (no edit control). ThreadIndex lists staged entries marked staged with staged- jump+flash; panel opens for staged-only reviews, header counts + N staged.

#502 gate holds (canComment-gated slots, anon pending empty). Unified/split: conversation DiffFile is the single unified staged surface; Files-tab DiffBody posts threads directly, nothing to mirror.

Verification: new web/test/unit/staged-inline-567.test.js (21 tests) + #567-scoped update to one stale pull-event-text-521 pin (second legitimate plain-text site); full unit suite minus smoke 1373/1373 green; related suites (560/566/555/546/554) green; vite build + esbuild SDK green; go vet clean. Browser proof open per shared-daemon guard. Docs: FIXED (Forgejo #567) amendment in docs/go/12_web_ui.md, same commit (law 12).

Implements Forgejo issue #567 per the proposed fix. **Problem:** staging a keyed draft handed {anchor, body} to the page pending list and unmounted the composer — the row showed nothing until Finish review. **Fix (web/src/pages/Pull.jsx + CommentComposer.jsx):** - Page-level pending list (same signal as FinishReview list + button count, no forked state) locates per row into StagedCards below the anchor line, same ml-14 mt-1 card slot as composer/ThreadCard. - Card: stager author + plain-text body (FinishReview-consistent) + canonical amber chip-draft pill (both themes, no per-callsite bg override per #545 F2) + staged-<i> id with emerald flash. - Edit re-opens the keyed composer pre-filled (new CommentComposer initialValue; Save vs Stage comment label); submit resolves the pending index fresh by anchor (mid-edit removals safe, vanished entries stage anew). Cancel drops only the draft; remove shares the pending-list unstage with the modal. - Unlocatable staged anchors render once at the file end (no edit control). ThreadIndex lists staged entries marked staged with staged-<i> jump+flash; panel opens for staged-only reviews, header counts + N staged. - #502 gate holds (canComment-gated slots, anon pending empty). Unified/split: conversation DiffFile is the single unified staged surface; Files-tab DiffBody posts threads directly, nothing to mirror. **Verification:** new web/test/unit/staged-inline-567.test.js (21 tests) + #567-scoped update to one stale pull-event-text-521 pin (second legitimate plain-text site); full unit suite minus smoke 1373/1373 green; related suites (560/566/555/546/554) green; vite build + esbuild SDK green; go vet clean. Browser proof open per shared-daemon guard. Docs: FIXED (Forgejo #567) amendment in docs/go/12_web_ui.md, same commit (law 12).

crueber added 1 commit

2026-09-15 13:34:10 +00:00

FIXED (Forgejo #567 ) — staged inline review comments render in-thread until submit a9bbeaa224

Conversation DiffFile locates the page-level pending list per row into
StagedCards below the anchor line (ThreadCard slot idiom, chip-draft
staged pill, plain-text body); edit re-opens the keyed composer
pre-filled via CommentComposer initialValue with anchor-verified index
resolution; remove shares the pending-list unstage with FinishReview;
ThreadIndex lists staged entries with staged-<i> jump+flash. #502 gate
holds (anon sees nothing new). Docs: 12_web_ui.md amendment.

crueber added 1 commit

2026-09-15 13:38:07 +00:00

REVIEW (Forgejo #567 ) — StagedCard controls use the #566 canonical small-btn (no new invisible .link); CommentComposer prefill via createEffect so same-line re-edits re-prefill 68cfbf2df1

crueber commented

2026-09-15 13:38:19 +00:00

Author

Owner

Independent review — APPROVED (with 2 small defects fixed inline as `68cfbf2`)

Verified against the #567 acceptance list on branch fix/issue-567 (a9bbeaa + my review commit 68cfbf2, pushed).

Acceptance — all hold:

Stage→card below the line, no invisible moment: staged For sits inside the row For, below the composer slot, above posted ThreadCards (pinned).
Card idiom: ThreadCard slot classes (ml-14 mt-1 rounded border…), author (stagedBy) + plain-text body (whitespace-pre-wrap, never markdown — FinishReview-consistent), canonical chip chip-draft with zero per-callsite bg overrides (compliant with the #545 F2 rule — the class itself is the amber token in ui.css; nothing hardcoded at the call site). staged-<i> id + emerald flash mirror the thread-<tid> idiom.
Edit round-trips pending via editStaged/resolveStagedEdit (anchor-verified fast path + anchor+body slow path; vanished entries stage anew); unstage shares the one index-based unstage with the FinishReview modal. updatePending maps over the same signal — one pending-list source of truth feeding cards + form list + button count (all three wirings pinned).
ThreadIndex lists staged in pending order marked staged, jumpToStaged scroll+flash; panel opens for staged-only reviews, header counts + N staged.
Multiple per file/line: per-line For over collected {p, i} lists; drifted anchors fall back to a file-end card with no edit control.
Unified+split: verified the claim — conversation DiffFile is a single unified renderer (no split mode) and Files-tab DiffBody posts via pulls.threads.create with no staged state, so there is nothing to mirror. Acceptable per the issue's shared-seam-or-both clause.
#502 gating: staged slots under canComment !== false; anon pending is empty by construction so the index leaks nothing.
521 pin update is justified: still asserts review/thread bodies go through renderBody, now explicitly pinning both plain-text sites instead of silently bumping a count. Not a weakening.
Law 1 (no new deps — runtime list unchanged), law 12 (12_web_ui.md amendment in the same change) hold.
Tests fail pre-fix: new suite run against origin/main source = 17/21 fail (4 negative-shape pins pass vacuously); post-fix 22/22. Unit-minus-smoke 1374/1374 green; related suites (560/566/521/546/554/555) green; go vet clean; vite build green.

Two small defects found and fixed in 68cfbf2 (3 files, +23/-6):

StagedCard edit/remove used class="link" — invisible text, since .link ships zero CSS rules (the exact bug #566 just fixed two commits ago). Switched both to the #566 canonical btn ml-2 px-2 py-0.5 text-xs, with a new test pin forbidding .link in StagedCard.
Same-line re-edit staleness: prefill ran once as if (props.initialValue) setBody(...) during setup, so editing staged entry B on a line while entry A's edit composer was still mounted kept A's text (same draft key, no remount). Prefill is now a createEffect on props.initialValue — re-prefills on target switches, never clobbers typing (typing never touches the prop), and createSignal("") is untouched so the #566 fresh-empty pin holds verbatim (first attempt broke it; corrected before push).

Note: make web fails in the /tmp/walhub-567 worktree for an environmental reason (pnpm refuses the symlinked node_modules pointing outside the worktree); vite build run directly succeeds. Smoke test failure is likewise environmental (needs a live server), identical pre-existing behavior.

Verdict: APPROVED — ready to merge.

## Independent review — APPROVED (with 2 small defects fixed inline as 68cfbf2) Verified against the #567 acceptance list on branch fix/issue-567 (a9bbeaa + my review commit 68cfbf2, pushed). **Acceptance — all hold:** - Stage→card below the line, no invisible moment: staged `For` sits inside the row `For`, below the composer slot, above posted ThreadCards (pinned). - Card idiom: ThreadCard slot classes (`ml-14 mt-1 rounded border…`), author (`stagedBy`) + plain-text body (`whitespace-pre-wrap`, never markdown — FinishReview-consistent), canonical `chip chip-draft` with zero per-callsite bg overrides (compliant with the #545 F2 rule — the class itself is the amber token in ui.css; nothing hardcoded at the call site). `staged-<i>` id + emerald flash mirror the `thread-<tid>` idiom. - Edit round-trips pending via `editStaged`/`resolveStagedEdit` (anchor-verified fast path + anchor+body slow path; vanished entries stage anew); unstage shares the one index-based `unstage` with the FinishReview modal. `updatePending` maps over the same signal — one pending-list source of truth feeding cards + form list + button count (all three wirings pinned). - ThreadIndex lists staged in pending order marked staged, `jumpToStaged` scroll+flash; panel opens for staged-only reviews, header counts `+ N staged`. - Multiple per file/line: per-line `For` over collected `{p, i}` lists; drifted anchors fall back to a file-end card with no edit control. - Unified+split: verified the claim — conversation DiffFile is a single unified renderer (no split mode) and Files-tab DiffBody posts via `pulls.threads.create` with no staged state, so there is nothing to mirror. Acceptable per the issue's shared-seam-or-both clause. - #502 gating: staged slots under `canComment !== false`; anon pending is empty by construction so the index leaks nothing. - 521 pin update is justified: still asserts review/thread bodies go through `renderBody`, now explicitly pinning both plain-text sites instead of silently bumping a count. Not a weakening. - Law 1 (no new deps — runtime list unchanged), law 12 (12_web_ui.md amendment in the same change) hold. - Tests fail pre-fix: new suite run against origin/main source = 17/21 fail (4 negative-shape pins pass vacuously); post-fix 22/22. Unit-minus-smoke 1374/1374 green; related suites (560/566/521/546/554/555) green; `go vet` clean; `vite build` green. **Two small defects found and fixed in 68cfbf2 (3 files, +23/-6):** 1. StagedCard edit/remove used `class="link"` — invisible text, since `.link` ships zero CSS rules (the exact bug #566 just fixed two commits ago). Switched both to the #566 canonical `btn ml-2 px-2 py-0.5 text-xs`, with a new test pin forbidding `.link` in StagedCard. 2. Same-line re-edit staleness: prefill ran once as `if (props.initialValue) setBody(...)` during setup, so editing staged entry B on a line while entry A's edit composer was still mounted kept A's text (same draft key, no remount). Prefill is now a `createEffect` on `props.initialValue` — re-prefills on target switches, never clobbers typing (typing never touches the prop), and `createSignal("")` is untouched so the #566 fresh-empty pin holds verbatim (first attempt broke it; corrected before push). Note: `make web` fails in the /tmp/walhub-567 worktree for an environmental reason (pnpm refuses the symlinked node_modules pointing outside the worktree); `vite build` run directly succeeds. Smoke test failure is likewise environmental (needs a live server), identical pre-existing behavior. **Verdict: APPROVED — ready to merge.**

crueber merged commit fe70265692 into main

2026-09-15 13:38:33 +00:00

crueber referenced this pull request from a commit

2026-09-15 13:38:34 +00:00

Merge pull request 'Fix #567: staged inline review comments render in-thread until submit' (#570) from fix/issue-567 into main

crueber referenced this pull request

2026-09-15 13:38:39 +00:00

Staged inline review comments vanish from the diff — only the staged count increments; they must render in-thread at their line #567