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

New issue

Closed

opened 2026-09-15 13:07:45 +00:00 by crueber · 1 comment

crueber commented

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

Owner

What's requested

Staging an inline review comment on the PR conversation diff currently makes the comment disappear: the composer closes, nothing renders in its place, and the only feedback is the staged-count increment on the "Finish review (N staged)" button plus the list inside the FinishReview form. A staged comment must remain visible at its anchor line — in-thread, in place — until the review is submitted.

Evidence (static diagnosis — no local repro, per standing rule)

The inline composers are the keyed-draft Map in web/src/pages/Pull.jsx DiffFile (getDrafts, line 355; keys ${hunkIdx}:${rowIdx} — Forgejo #560). On submit the composer hands the draft to the page and immediately closes itself: props.onStage({ anchor: d().anchor, body }) followed by closeDraft(draftKey(hi(), ri())) (Pull.jsx:504-507). Deleting the Map entry unmounts the only thing rendered under that row — the composer card is <Show when={... draftAt(hi(), ri())}> (Pull.jsx:494-515), so after staging the row shows nothing.
The staged draft lands in the page-level signal getPending (Pull.jsx:822; stage at 885, unstage at 886). The only surfaces that render it are:
- the "Finish review" button label counter — Finish review (N staged) (Pull.jsx:1029);
- the FinishReview form's pending list / "no staged line comments" fallback (Pull.jsx:723-738), which posts the list as threads on submit.
The diff itself never renders staged drafts: DiffFile's inline rendering under each row covers only open composers (draftAt) and posted threads (threadsAt → ThreadCard, Pull.jsx:516-518). There is no staged-draft card, so the comment is invisible at its line from stage until review submit.

Proposed fix

On stage, keep the composer's closing behavior, but render a staged-comment card immediately below the same anchor line, in the same threaded position the composer occupied (the existing ml-14 mt-1 rounded border inline-card slot that ThreadCard and the composer already use). Do this inside the keyed structure so each staged draft renders at its own line (multiple staged comments per file must all render simultaneously, per #560's keyed-collection model — the staged drafts need their own keyed store parallel to getDrafts, or an equivalent keyed Map entry state).
The staged card follows the posted-comment idiom (see ThreadCard): author, body, plus a visually distinct "staged" chip that distinguishes it from posted comments (a pill like the existing resolved/outdated pills, in both themes). Plain-text body is fine at first (FinishReview already renders p.body plain — keep the two consistent).
The card carries edit/unstage affordances: click the card (or an edit control) to re-open the composer in place pre-filled with the staged body (edit round-trip updates the pending list entry); an explicit unstage/remove drops it from the pending list. Unstage from the inline card must agree with FinishReview's per-item remove (same pending-list mutation).
The FinishReview form list stays as-is (unchanged), and the staged count on the button still increments — both surfaces must agree with what renders inline at all times (one pending-list source of truth feeding all three).
Interaction with the #546 jump-to-comments index (ThreadIndex, Pull.jsx:618): index entries for staged comments link to the inline staged card (scroll + flash, the thread-<tid>/flashTid idiom), so a reviewer can jump to their own staged comments. Staged comments must be listed in the index alongside posted threads, marked staged.

Consistency requirements

Works in unified AND split diff views (whatever the Files-tab/commit diff renderers share with DiffFile — the fix should live in the shared seam, not be PR-page-only, or be applied to each renderer).
Multiple staged comments per file each render at their own line simultaneously (#560 keyed-collection semantics; text preserved across other staging actions).
Both themes (light/dark) and 390px mobile width — the inline card must not widen the diff scroll area beyond what the composer already does.

Acceptance criteria

On "Stage comment", the composer closes and a staged-comment card renders immediately below the anchored line — no moment where the comment is invisible at its line.
Staged card is visually distinct from posted comments via a "staged" chip; author + body render; both themes.
Edit works from the inline card: re-open composer in place pre-filled, round-trip updates the pending list entry; unstage/remove from the inline card removes it everywhere.
FinishReview pending list and staged-count button unchanged and consistent — counts and bodies agree with what renders inline.
Works in unified AND split diff views.
#546 ThreadIndex lists staged comments and links to the inline card (scroll + flash works).
Multiple staged comments per file render at their own lines simultaneously.
Headless tests (node --test web/test/unit/, per the #560 inline-composer-560.test.js convention): stage → inline render, edit round-trip, multi-instance.
Themes + 390px render verified per the render-verification mandate (#533/#545) — headless DOM assertions and/or screenshot review before closing.

#546 — jump-to-comments index (ThreadIndex)
#555 — per-line tap affordance in DiffTable
#560 — inline keyed composers in DiffFile (this builds directly on its draft Map)
#561 — cancel affordance on the PR page (sibling composer-interaction fix)
#533 / #545 — render-verification mandate

## What's requested Staging an inline review comment on the PR conversation diff currently makes the comment disappear: the composer closes, nothing renders in its place, and the only feedback is the staged-count increment on the "Finish review (N staged)" button plus the list inside the FinishReview form. A staged comment must remain visible at its anchor line — in-thread, in place — until the review is submitted. ## Evidence (static diagnosis — no local repro, per standing rule) - The inline composers are the keyed-draft Map in `web/src/pages/Pull.jsx` `DiffFile` (`getDrafts`, line 355; keys `${hunkIdx}:${rowIdx}` — Forgejo #560). On submit the composer hands the draft to the page and immediately closes itself: `props.onStage({ anchor: d().anchor, body })` followed by `closeDraft(draftKey(hi(), ri()))` (Pull.jsx:504-507). Deleting the Map entry unmounts the only thing rendered under that row — the composer card is `<Show when={... draftAt(hi(), ri())}>` (Pull.jsx:494-515), so after staging the row shows nothing. - The staged draft lands in the page-level signal `getPending` (Pull.jsx:822; `stage` at 885, `unstage` at 886). The only surfaces that render it are: - the "Finish review" button label counter — `Finish review (N staged)` (Pull.jsx:1029); - the FinishReview form's pending list / "no staged line comments" fallback (Pull.jsx:723-738), which posts the list as `threads` on submit. - The diff itself never renders staged drafts: `DiffFile`'s inline rendering under each row covers only open composers (`draftAt`) and posted threads (`threadsAt` → `ThreadCard`, Pull.jsx:516-518). There is no staged-draft card, so the comment is invisible at its line from stage until review submit. ## Proposed fix - On stage, keep the composer's closing behavior, but render a staged-comment card immediately below the same anchor line, in the same threaded position the composer occupied (the existing `ml-14 mt-1 rounded border` inline-card slot that `ThreadCard` and the composer already use). Do this inside the keyed structure so each staged draft renders at its own line (multiple staged comments per file must all render simultaneously, per #560's keyed-collection model — the staged drafts need their own keyed store parallel to `getDrafts`, or an equivalent keyed Map entry state). - The staged card follows the posted-comment idiom (see `ThreadCard`): author, body, plus a visually distinct "staged" chip that distinguishes it from posted comments (a `pill` like the existing `resolved`/`outdated` pills, in both themes). Plain-text body is fine at first (FinishReview already renders `p.body` plain — keep the two consistent). - The card carries edit/unstage affordances: click the card (or an edit control) to re-open the composer in place pre-filled with the staged body (edit round-trip updates the pending list entry); an explicit unstage/remove drops it from the pending list. Unstage from the inline card must agree with FinishReview's per-item `remove` (same pending-list mutation). - The FinishReview form list stays as-is (unchanged), and the staged count on the button still increments — both surfaces must agree with what renders inline at all times (one pending-list source of truth feeding all three). - Interaction with the #546 jump-to-comments index (`ThreadIndex`, Pull.jsx:618): index entries for staged comments link to the inline staged card (scroll + flash, the `thread-<tid>`/`flashTid` idiom), so a reviewer can jump to their own staged comments. Staged comments must be listed in the index alongside posted threads, marked staged. ## Consistency requirements - Works in unified AND split diff views (whatever the Files-tab/commit diff renderers share with `DiffFile` — the fix should live in the shared seam, not be PR-page-only, or be applied to each renderer). - Multiple staged comments per file each render at their own line simultaneously (#560 keyed-collection semantics; text preserved across other staging actions). - Both themes (light/dark) and 390px mobile width — the inline card must not widen the diff scroll area beyond what the composer already does. ## Acceptance criteria - [ ] On "Stage comment", the composer closes and a staged-comment card renders immediately below the anchored line — no moment where the comment is invisible at its line. - [ ] Staged card is visually distinct from posted comments via a "staged" chip; author + body render; both themes. - [ ] Edit works from the inline card: re-open composer in place pre-filled, round-trip updates the pending list entry; unstage/remove from the inline card removes it everywhere. - [ ] FinishReview pending list and staged-count button unchanged and consistent — counts and bodies agree with what renders inline. - [ ] Works in unified AND split diff views. - [ ] #546 ThreadIndex lists staged comments and links to the inline card (scroll + flash works). - [ ] Multiple staged comments per file render at their own lines simultaneously. - [ ] Headless tests (`node --test web/test/unit/`, per the #560 `inline-composer-560.test.js` convention): stage → inline render, edit round-trip, multi-instance. - [ ] Themes + 390px render verified per the render-verification mandate (#533/#545) — headless DOM assertions and/or screenshot review before closing. ## Related - #546 — jump-to-comments index (ThreadIndex) - #555 — per-line tap affordance in DiffTable - #560 — inline keyed composers in DiffFile (this builds directly on its draft Map) - #561 — cancel affordance on the PR page (sibling composer-interaction fix) - #533 / #545 — render-verification mandate

crueber added the

bug

issues

pull requests

labels

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

crueber added this to the v1 milestone

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

crueber referenced this issue from a commit

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

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

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

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

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

crueber referenced this issue from a 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

crueber referenced this issue

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

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

crueber closed this issue

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

crueber referenced this issue 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 commented

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

Author

Owner

Fixed by #570 (merged): staged inline comments now render as in-thread StagedCards at their anchor line (ThreadCard idiom, staged pill) until submit, with edit round-trip + unstage sharing the one pending-list source of truth, and ThreadIndex listing/linking staged entries. Review also fixed two defects pre-merge: invisible .link controls (the #566 pattern) and same-line re-edit staleness. Verified: 1374 unit tests green (smoke excluded, pre-existing), vite/esbuild green, independent review APPROVED.