Ben Stull
0e1805b8ce
PRView.jsx declared its threadsByKind useMemo after the early-return guard for the loading state. First mount: pr is null, early return fires, 14 hooks called. Second render: pr populated, execution reaches the useMemo, 15 hooks — React #310, page goes blank. Move the useMemo above the early returns and null-safe the pr?.threads access so the hook count stays stable across renders. Pre-existing bug in the post-Contribute-rewrite PR view; surfaced in production by 0.2.0's graduation merge race fix enabling the workflow that opens this code path. Patch per §20.2 — no operator action required beyond rebuilding the frontend and restarting.
549 lines
21 KiB
React
549 lines
21 KiB
React
// PRView.jsx — the §10.3 PR review page.
|
||
//
|
||
// Three-column shape per §8.1: the catalog on the left is the App
|
||
// chrome (so it sits outside this component, same as RFCView). This
|
||
// component owns the center (diff toggleable between unified and
|
||
// split) and the right (compressed conversation + inline review
|
||
// surface). A header strip carries title, editable description,
|
||
// status, the merge button, and aggregate counts.
|
||
//
|
||
// The per-user seen-cursor (§10.3) accents new hunks and new
|
||
// conversation messages on the next visit. The cursor advances on
|
||
// view; reviewers do not mark anything as read.
|
||
|
||
import { useCallback, useEffect, useMemo, useState } from 'react'
|
||
import { useNavigate, useParams } from 'react-router-dom'
|
||
import {
|
||
advancePRSeen,
|
||
editPRDescription,
|
||
getPR,
|
||
mergePR,
|
||
postPRReview,
|
||
startResolutionBranch,
|
||
withdrawPR,
|
||
} from '../api'
|
||
|
||
export default function PRView({ viewer }) {
|
||
const { slug, prNumber: prNumberParam } = useParams()
|
||
const navigate = useNavigate()
|
||
const prNumber = Number(prNumberParam)
|
||
|
||
const [pr, setPR] = useState(null)
|
||
const [error, setError] = useState(null)
|
||
const [diffMode, setDiffMode] = useState('unified') // 'unified' | 'split'
|
||
const [editingHeader, setEditingHeader] = useState(false)
|
||
const [draftTitle, setDraftTitle] = useState('')
|
||
const [draftDescription, setDraftDescription] = useState('')
|
||
const [acting, setActing] = useState(null) // 'merge' | 'withdraw' | 'resolution'
|
||
|
||
// Review-comment composer state.
|
||
const [reviewDraft, setReviewDraft] = useState(null) // { quote, anchorPayload }
|
||
const [reviewText, setReviewText] = useState('')
|
||
|
||
const refresh = useCallback(() => {
|
||
return getPR(slug, prNumber)
|
||
.then(setPR)
|
||
.catch(err => setError(err.message))
|
||
}, [slug, prNumber])
|
||
|
||
useEffect(() => { refresh() }, [refresh])
|
||
|
||
// §10.3: advance the seen cursor on view. We snapshot the most
|
||
// recent commit sha and the highest message id we render, then
|
||
// POST. Fire-and-forget; the next read returns the advanced cursor.
|
||
useEffect(() => {
|
||
if (!pr) return
|
||
const headSha = pr.merge_commit_sha || pr.head_sha
|
||
const allMessages = Object.values(pr.messages_by_thread || {}).flat()
|
||
const lastMessageId = allMessages.reduce((m, x) => Math.max(m, x.id || 0), 0)
|
||
if (!headSha && !lastMessageId) return
|
||
if (viewer == null) return
|
||
advancePRSeen(slug, prNumber, {
|
||
lastSeenCommitSha: headSha,
|
||
lastSeenMessageId: lastMessageId || null,
|
||
}).catch(() => {})
|
||
}, [pr?.head_sha, pr?.merge_commit_sha, prNumber, slug, viewer])
|
||
|
||
// ── header actions ─────────────────────────────────────────────
|
||
const onMerge = useCallback(async () => {
|
||
if (!confirm(`Merge PR #${prNumber}? §10.5 produces a no-fast-forward merge commit on main.`)) return
|
||
setActing('merge')
|
||
setError(null)
|
||
try {
|
||
await mergePR(slug, prNumber)
|
||
await refresh()
|
||
} catch (e) {
|
||
setError(e.message)
|
||
} finally {
|
||
setActing(null)
|
||
}
|
||
}, [slug, prNumber, refresh])
|
||
|
||
const onWithdraw = useCallback(async () => {
|
||
if (!confirm(`Withdraw PR #${prNumber}? The branch is not deleted. §10.8.`)) return
|
||
setActing('withdraw')
|
||
setError(null)
|
||
try {
|
||
await withdrawPR(slug, prNumber)
|
||
await refresh()
|
||
} catch (e) {
|
||
setError(e.message)
|
||
} finally {
|
||
setActing(null)
|
||
}
|
||
}, [slug, prNumber, refresh])
|
||
|
||
const onResolutionBranch = useCallback(async () => {
|
||
if (!confirm('Start a resolution branch off main? §10.9 cuts a fresh branch, replays the diff, and opens a new PR.')) return
|
||
setActing('resolution')
|
||
setError(null)
|
||
try {
|
||
const { resolution_branch } = await startResolutionBranch(slug, prNumber)
|
||
navigate(`/rfc/${slug}?branch=${encodeURIComponent(resolution_branch)}`)
|
||
} catch (e) {
|
||
setError(e.message)
|
||
} finally {
|
||
setActing(null)
|
||
}
|
||
}, [slug, prNumber, navigate])
|
||
|
||
const startHeaderEdit = useCallback(() => {
|
||
setDraftTitle(pr?.title || '')
|
||
setDraftDescription(pr?.description || '')
|
||
setEditingHeader(true)
|
||
}, [pr])
|
||
|
||
const saveHeader = useCallback(async () => {
|
||
try {
|
||
await editPRDescription(slug, prNumber, {
|
||
title: draftTitle.trim(),
|
||
description: draftDescription.trim(),
|
||
})
|
||
setEditingHeader(false)
|
||
await refresh()
|
||
} catch (e) {
|
||
setError(e.message)
|
||
}
|
||
}, [slug, prNumber, draftTitle, draftDescription, refresh])
|
||
|
||
// ── review composer ────────────────────────────────────────────
|
||
const onSubmitReview = useCallback(async () => {
|
||
if (!reviewText.trim()) return
|
||
try {
|
||
await postPRReview(slug, prNumber, {
|
||
text: reviewText.trim(),
|
||
anchorPayload: reviewDraft?.anchorPayload || {},
|
||
quote: reviewDraft?.quote || null,
|
||
})
|
||
setReviewText('')
|
||
setReviewDraft(null)
|
||
await refresh()
|
||
} catch (e) {
|
||
setError(e.message)
|
||
}
|
||
}, [slug, prNumber, reviewText, reviewDraft, refresh])
|
||
|
||
// §10.6: split messages by thread kind for the visual distinction
|
||
// §10.4 requires. Within each kind, sort by id for chronological
|
||
// order. Declared above the early returns below so the hook count
|
||
// stays stable between the loading render (pr == null) and the
|
||
// loaded render (Rules of Hooks — React #310 otherwise).
|
||
const threadsByKind = useMemo(() => {
|
||
const out = { chat: [], review: [], flag: [] }
|
||
for (const t of pr?.threads || []) {
|
||
out[t.thread_kind]?.push(t)
|
||
}
|
||
return out
|
||
}, [pr?.threads])
|
||
|
||
// ── render ─────────────────────────────────────────────────────
|
||
if (error) return <article className="entry-view"><p>Error: {error}</p></article>
|
||
if (!pr) return <article className="entry-view">Loading PR…</article>
|
||
|
||
const merged = pr.state === 'merged'
|
||
const withdrawn = pr.state === 'withdrawn'
|
||
const closed = pr.state === 'closed'
|
||
const open = pr.state === 'open'
|
||
const supersededBy = pr.superseded_by_pr_number
|
||
const supersedes = pr.supersedes_pr_number
|
||
|
||
const seenSha = pr.seen?.last_seen_commit_sha
|
||
const seenMsgId = pr.seen?.last_seen_message_id || 0
|
||
|
||
// Compute new-since-cursor accents. A commit is "new" when the PR
|
||
// moved past the seen sha; for v1 we accent the diff whole-cloth
|
||
// when seenSha differs from current head/merge sha (a future slice
|
||
// can render per-hunk accents — the cursor is in place).
|
||
const diffHasNewCommits = seenSha && seenSha !== (pr.merge_commit_sha || pr.head_sha)
|
||
|
||
return (
|
||
<div className="rfc-view pr-view">
|
||
{/* Header strip */}
|
||
<div className="pr-header">
|
||
<div className="pr-header-left">
|
||
<div className="pr-breadcrumb">
|
||
<a href={`/rfc/${slug}`}>{pr.rfc_id || pr.slug}</a>
|
||
<span className="breadcrumb-sep">›</span>
|
||
<a href={`/rfc/${slug}?branch=${encodeURIComponent(pr.head_branch)}`}>{pr.head_branch}</a>
|
||
<span className="breadcrumb-sep">›</span>
|
||
<strong>PR #{pr.pr_number}</strong>
|
||
</div>
|
||
{editingHeader ? (
|
||
<div className="pr-header-edit">
|
||
<input
|
||
type="text"
|
||
className="modal-input"
|
||
value={draftTitle}
|
||
onChange={e => setDraftTitle(e.target.value)}
|
||
maxLength={240}
|
||
/>
|
||
<textarea
|
||
className="modal-textarea"
|
||
value={draftDescription}
|
||
onChange={e => setDraftDescription(e.target.value)}
|
||
rows={4}
|
||
maxLength={8000}
|
||
/>
|
||
<div className="modal-actions">
|
||
<button className="btn-secondary" onClick={() => setEditingHeader(false)}>Cancel</button>
|
||
<button className="btn-primary" onClick={saveHeader}>Save</button>
|
||
</div>
|
||
</div>
|
||
) : (
|
||
<>
|
||
<h1 className="pr-title">{pr.title}</h1>
|
||
{pr.description && (
|
||
<p className="pr-description">{pr.description}</p>
|
||
)}
|
||
{pr.capabilities?.can_edit_text && (
|
||
<button className="btn-link" onClick={startHeaderEdit}>Edit title & description</button>
|
||
)}
|
||
</>
|
||
)}
|
||
</div>
|
||
<div className="pr-header-right">
|
||
<StateBanner state={pr.state} mergedAt={pr.merged_at} closedAt={pr.closed_at} />
|
||
{(supersedes || supersededBy) && (
|
||
<div className="pr-supersedes">
|
||
{supersedes && <>Supersedes <a href={`/rfc/${slug}/pr/${supersedes}`}>#{supersedes}</a></>}
|
||
{supersededBy && <>Closed by <a href={`/rfc/${slug}/pr/${supersededBy}`}>#{supersededBy}</a></>}
|
||
</div>
|
||
)}
|
||
<div className="pr-counts">
|
||
<span className="pr-count-flags" title="Open flags on the source branch (§8.13)">
|
||
{pr.counts.open_flags} flag{pr.counts.open_flags === 1 ? '' : 's'}
|
||
</span>
|
||
<span className="pr-count-reviews">
|
||
{pr.counts.open_review_threads} review thread{pr.counts.open_review_threads === 1 ? '' : 's'}
|
||
</span>
|
||
<span className="pr-count-chats">
|
||
{pr.counts.open_chat_threads} open chat{pr.counts.open_chat_threads === 1 ? '' : 's'}
|
||
</span>
|
||
</div>
|
||
{open && pr.mergeable === false && (
|
||
<div className="pr-conflict-banner">
|
||
<p>
|
||
<strong>Merge conflict with main.</strong> Per §10.9, start a
|
||
resolution branch off main's tip to replay your work.
|
||
</p>
|
||
<button
|
||
className="btn-primary"
|
||
onClick={onResolutionBranch}
|
||
disabled={acting === 'resolution'}
|
||
>
|
||
{acting === 'resolution' ? 'Starting…' : 'Start resolution branch'}
|
||
</button>
|
||
</div>
|
||
)}
|
||
<div className="pr-actions">
|
||
{pr.capabilities?.can_merge && pr.mergeable !== false && (
|
||
<button
|
||
className="btn-primary"
|
||
onClick={onMerge}
|
||
disabled={acting === 'merge'}
|
||
>
|
||
{acting === 'merge' ? 'Merging…' : 'Merge'}
|
||
</button>
|
||
)}
|
||
{pr.capabilities?.can_withdraw && (
|
||
<button
|
||
className="btn-secondary"
|
||
onClick={onWithdraw}
|
||
disabled={acting === 'withdraw'}
|
||
>
|
||
{acting === 'withdraw' ? 'Withdrawing…' : 'Withdraw'}
|
||
</button>
|
||
)}
|
||
</div>
|
||
{!viewer && (
|
||
<a className="btn-link" href="/auth/login">Sign in to review</a>
|
||
)}
|
||
</div>
|
||
</div>
|
||
|
||
{/* Two columns under the header */}
|
||
<div className="rfc-body pr-body">
|
||
<div className="editor-area">
|
||
<div className="diff-mode-toolbar">
|
||
<button
|
||
className={`btn-link ${diffMode === 'unified' ? 'active' : ''}`}
|
||
onClick={() => setDiffMode('unified')}
|
||
>Unified</button>
|
||
<button
|
||
className={`btn-link ${diffMode === 'split' ? 'active' : ''}`}
|
||
onClick={() => setDiffMode('split')}
|
||
>Split</button>
|
||
{diffHasNewCommits && (
|
||
<span className="pr-diff-accent" title="New commits since your last visit">
|
||
New since your last visit
|
||
</span>
|
||
)}
|
||
</div>
|
||
<DiffPane
|
||
mainBody={pr.main_body || ''}
|
||
branchBody={pr.branch_body || ''}
|
||
mode={diffMode}
|
||
onReviewRange={(quote, anchorPayload) => {
|
||
if (!pr.capabilities?.can_post_review) return
|
||
setReviewDraft({ quote, anchorPayload })
|
||
setReviewText('')
|
||
}}
|
||
/>
|
||
</div>
|
||
|
||
<div className="right-panel">
|
||
<PRConversation
|
||
threads={pr.threads || []}
|
||
messagesByThread={pr.messages_by_thread || {}}
|
||
threadsByKind={threadsByKind}
|
||
seenMsgId={seenMsgId}
|
||
/>
|
||
{reviewDraft && pr.capabilities?.can_post_review && (
|
||
<div className="pr-review-composer">
|
||
<div className="pr-review-quote">
|
||
<strong>Reviewing:</strong>
|
||
<pre>{reviewDraft.quote || '(no selection)'}</pre>
|
||
</div>
|
||
<textarea
|
||
className="modal-textarea"
|
||
value={reviewText}
|
||
onChange={e => setReviewText(e.target.value)}
|
||
placeholder="Leave a review comment on this range — §10.4."
|
||
rows={4}
|
||
/>
|
||
<div className="modal-actions">
|
||
<button className="btn-secondary" onClick={() => { setReviewDraft(null); setReviewText('') }}>Cancel</button>
|
||
<button className="btn-primary" onClick={onSubmitReview} disabled={!reviewText.trim()}>Post review</button>
|
||
</div>
|
||
</div>
|
||
)}
|
||
</div>
|
||
</div>
|
||
</div>
|
||
)
|
||
}
|
||
|
||
function StateBanner({ state, mergedAt, closedAt }) {
|
||
if (state === 'merged') return <div className="pr-state-banner merged">Merged {fmtDate(mergedAt)}</div>
|
||
if (state === 'withdrawn') return <div className="pr-state-banner withdrawn">Withdrawn {fmtDate(closedAt)}</div>
|
||
if (state === 'closed') return <div className="pr-state-banner closed">Closed {fmtDate(closedAt)}</div>
|
||
return <div className="pr-state-banner open">Open</div>
|
||
}
|
||
|
||
function fmtDate(iso) {
|
||
if (!iso) return ''
|
||
try { return new Date(iso).toLocaleDateString() } catch { return iso }
|
||
}
|
||
|
||
// ─────────────────────────────────────────────────────────────────
|
||
// Conversation surface — chat + review interleaved by chronology.
|
||
// §8.5: compressed by default. We expand all for v1; the toggle and
|
||
// the "Show full conversation" affordance are tracked as a §19.2
|
||
// follow-on if usage shows it matters.
|
||
// ─────────────────────────────────────────────────────────────────
|
||
|
||
function PRConversation({ threads, messagesByThread, threadsByKind, seenMsgId }) {
|
||
// Flatten every message with its thread context, sort by id.
|
||
const items = []
|
||
for (const t of threads) {
|
||
const msgs = messagesByThread[t.id] || []
|
||
for (const m of msgs) {
|
||
items.push({ ...m, _thread: t })
|
||
}
|
||
if (t.thread_kind === 'flag' && msgs.length === 0) {
|
||
items.push({
|
||
id: `flag-${t.id}`,
|
||
role: 'system',
|
||
text: t.label || '',
|
||
_thread: t,
|
||
created_at: t.created_at,
|
||
})
|
||
}
|
||
}
|
||
items.sort((a, b) => {
|
||
const ai = typeof a.id === 'number' ? a.id : 0
|
||
const bi = typeof b.id === 'number' ? b.id : 0
|
||
return ai - bi
|
||
})
|
||
|
||
return (
|
||
<div className="chat-panel pr-conversation">
|
||
<div className="pr-conv-disclosure">
|
||
{threadsByKind.review.length > 0 && (
|
||
<div className="thread-disclosure">
|
||
<strong>{threadsByKind.review.length}</strong> review thread{threadsByKind.review.length === 1 ? '' : 's'}
|
||
</div>
|
||
)}
|
||
{threadsByKind.flag.length > 0 && (
|
||
<div className="thread-disclosure">
|
||
<strong>{threadsByKind.flag.length}</strong> flag{threadsByKind.flag.length === 1 ? '' : 's'}
|
||
</div>
|
||
)}
|
||
</div>
|
||
<ul className="chat-feed">
|
||
{items.map(m => {
|
||
const t = m._thread
|
||
const isReview = t?.thread_kind === 'review'
|
||
const isFlag = t?.thread_kind === 'flag'
|
||
const isNew = typeof m.id === 'number' && m.id > seenMsgId
|
||
const cls = [
|
||
'chat-msg',
|
||
`chat-msg-${m.role}`,
|
||
isReview ? 'chat-msg-review' : '',
|
||
isFlag ? 'chat-msg-flag' : '',
|
||
isNew ? 'chat-msg-new' : '',
|
||
].filter(Boolean).join(' ')
|
||
return (
|
||
<li key={m.id} className={cls} data-message-id={m.id}>
|
||
<div className="chat-msg-header">
|
||
{isReview && <span className="chat-msg-badge review">Review</span>}
|
||
{isFlag && <span className="chat-msg-badge flag">Flag</span>}
|
||
<span className="chat-msg-author">
|
||
{m.role === 'assistant' ? `Claude (${m.model_id || 'ai'})` : (m.author_display || m.author_login || (m.role === 'system' ? 'system' : ''))}
|
||
</span>
|
||
{isNew && <span className="chat-msg-new-pip" title="New since your last visit">●</span>}
|
||
</div>
|
||
{m.quote && <pre className="chat-msg-quote">{m.quote}</pre>}
|
||
<div className="chat-msg-body">{m.text}</div>
|
||
</li>
|
||
)
|
||
})}
|
||
</ul>
|
||
</div>
|
||
)
|
||
}
|
||
|
||
// ─────────────────────────────────────────────────────────────────
|
||
// Diff pane — minimal line-by-line unified/split renderer over the
|
||
// branch's RFC.md vs main's RFC.md. The §19.2 candidate "DiffView
|
||
// reconstruction from changes history" is the long-form follow-on;
|
||
// for Slice 3 we serve the file-level diff readers expect.
|
||
// ─────────────────────────────────────────────────────────────────
|
||
|
||
function DiffPane({ mainBody, branchBody, mode, onReviewRange }) {
|
||
const lines = useMemo(() => computeLineDiff(mainBody, branchBody), [mainBody, branchBody])
|
||
|
||
if (mode === 'split') {
|
||
return (
|
||
<div className="diff-pane diff-split">
|
||
<div className="diff-col">
|
||
<div className="diff-col-header">main</div>
|
||
{lines.map((l, i) => (
|
||
<DiffLine key={`m${i}`} side="left" type={l.type} text={l.left} onReviewRange={onReviewRange} />
|
||
))}
|
||
</div>
|
||
<div className="diff-col">
|
||
<div className="diff-col-header">{`branch`}</div>
|
||
{lines.map((l, i) => (
|
||
<DiffLine key={`b${i}`} side="right" type={l.type} text={l.right} onReviewRange={onReviewRange} />
|
||
))}
|
||
</div>
|
||
</div>
|
||
)
|
||
}
|
||
|
||
return (
|
||
<div className="diff-pane diff-unified">
|
||
{lines.map((l, i) => (
|
||
<UnifiedRow key={i} line={l} onReviewRange={onReviewRange} />
|
||
))}
|
||
</div>
|
||
)
|
||
}
|
||
|
||
function UnifiedRow({ line, onReviewRange }) {
|
||
if (line.type === 'equal') {
|
||
return <div className="diff-row diff-equal" onMouseUp={makeReviewHandler(line.left || line.right, onReviewRange)}>
|
||
<span className="diff-marker"> </span><span className="diff-text">{line.left}</span>
|
||
</div>
|
||
}
|
||
return (
|
||
<>
|
||
{line.left != null && (
|
||
<div className="diff-row diff-del" onMouseUp={makeReviewHandler(line.left, onReviewRange)}>
|
||
<span className="diff-marker">−</span><span className="diff-text">{line.left}</span>
|
||
</div>
|
||
)}
|
||
{line.right != null && (
|
||
<div className="diff-row diff-add" onMouseUp={makeReviewHandler(line.right, onReviewRange)}>
|
||
<span className="diff-marker">+</span><span className="diff-text">{line.right}</span>
|
||
</div>
|
||
)}
|
||
</>
|
||
)
|
||
}
|
||
|
||
function DiffLine({ type, text, onReviewRange }) {
|
||
const cls = `diff-row diff-${type === 'equal' ? 'equal' : (text == null ? 'empty' : type)}`
|
||
return (
|
||
<div className={cls} onMouseUp={makeReviewHandler(text || '', onReviewRange)}>
|
||
<span className="diff-text">{text || ''}</span>
|
||
</div>
|
||
)
|
||
}
|
||
|
||
function makeReviewHandler(rowText, onReviewRange) {
|
||
return () => {
|
||
const sel = window.getSelection?.()
|
||
if (!sel || sel.isCollapsed) return
|
||
const quote = sel.toString()
|
||
if (!quote || !quote.trim()) return
|
||
onReviewRange?.(quote, { row_text: rowText, quote })
|
||
}
|
||
}
|
||
|
||
// Crude longest-common-subsequence-ish line diff. Sufficient for the
|
||
// single-file diff we render; switching to a proper diff library is a
|
||
// §19.2 candidate ("markdown round-trip fidelity" earned attention
|
||
// first). The output is a list of {type, left, right} rows where
|
||
// type is one of 'equal' | 'del' | 'add'.
|
||
function computeLineDiff(a, b) {
|
||
const aLines = (a || '').split('\n')
|
||
const bLines = (b || '').split('\n')
|
||
const n = aLines.length, m = bLines.length
|
||
const dp = Array(n + 1).fill(null).map(() => Array(m + 1).fill(0))
|
||
for (let i = n - 1; i >= 0; i--) {
|
||
for (let j = m - 1; j >= 0; j--) {
|
||
if (aLines[i] === bLines[j]) dp[i][j] = dp[i + 1][j + 1] + 1
|
||
else dp[i][j] = Math.max(dp[i + 1][j], dp[i][j + 1])
|
||
}
|
||
}
|
||
const out = []
|
||
let i = 0, j = 0
|
||
while (i < n && j < m) {
|
||
if (aLines[i] === bLines[j]) {
|
||
out.push({ type: 'equal', left: aLines[i], right: bLines[j] })
|
||
i++; j++
|
||
} else if (dp[i + 1][j] >= dp[i][j + 1]) {
|
||
out.push({ type: 'del', left: aLines[i], right: null })
|
||
i++
|
||
} else {
|
||
out.push({ type: 'add', left: null, right: bLines[j] })
|
||
j++
|
||
}
|
||
}
|
||
while (i < n) { out.push({ type: 'del', left: aLines[i++], right: null }) }
|
||
while (j < m) { out.push({ type: 'add', left: null, right: bLines[j++] }) }
|
||
return out
|
||
}
|