diff --git a/SPEC.md b/SPEC.md index 5512260..fc3d3c5 100644 --- a/SPEC.md +++ b/SPEC.md @@ -842,29 +842,33 @@ click-to-card binding from §8.8. The margin marker is scannable precise ("what changed here?"). The two answer different questions and both are kept. -The inline markup is session-local. Each accepted change is already -a clean commit on Gitea per §8.6, so the branch's canonical state at -any reload is the integrated text without markup. Regenerating -markup on load by diffing against earlier commits is technically -possible but adds a mechanism — a per-user seen-cursor for accepted -changes, an explicit dismiss UI — to solve a problem that DiffView -already solves durably and at higher fidelity. The editor is for -writing; layering permanent diff overlay on top of writable text -degrades the writing surface, so the markup clears on reload and -DiffView is the durable artifact for inspecting accepted changes in -context. +The inline markup lives in the rendered preview surface, not on the +writable editor. With the Contribute-mode split (§8.3) the raw markdown +source is a CodeMirror buffer that can't host HTML decorations, and +even when a writable surface could host them — as Tiptap did in earlier +revisions — layering permanent diff overlay on top of writable text +degraded the writing surface. The preview pane is the natural home: on +the right side of the Contribute split it shows the editor's own work +in-context (default-on, since the pane exists for that editorial +review), and in Discuss mode the single preview pane gates the overlay +behind a toolbar toggle so the default reading experience stays clean +prose. The overlay regenerates on every render from the branch's +`changes` table — no per-user seen-cursor, no dismiss UI; every +accepted change on the branch is included, drift is tolerated by +skipping spans whose `proposed` text no longer matches verbatim, and +mermaid intersections are skipped as a known limitation. Hovering any +marked span surfaces a tooltip with the change's type badge (`ai` or +`manual`), the model identifier where applicable, the +`was_edited_before_accept` flag where set, the user prompt and +selection-quote that drove the change, and the AI's `reason`. -DiffView is the read-only render surface invoked via a toolbar -toggle (§8.15). It reads from the -`changes` table for the branch, reconstructs the markup for every -accepted change in branch history, and renders the result in-place -where the editor was. Hovering any marked span surfaces a tooltip -with the change's type badge (`ai` or `manual`), the model -identifier where applicable, the `was_edited_before_accept` flag -where set, the user prompt and selection-quote that drove the -change, and the AI's `reason`. The toggle is reversible; returning -to the editor restores the live writing surface and reattaches the -session-local baseline. +DiffView is the legacy read-only render surface invoked via the +Contribute-mode toolbar toggle (§8.15) — a full-editor swap-in that +reads the same `changes` table. It is retained as an interim path while +the preview-pane overlay matures and is slated for retirement once +gutter markers (the §8.10 paragraph-margin layer) land in the preview +itself; at that point both visual layers live in the same surface and +the toolbar toggle collapses. ### 8.11 Manual edits and collisions with AI proposals diff --git a/frontend/src/App.css b/frontend/src/App.css index b5e02aa..f40a9a8 100644 --- a/frontend/src/App.css +++ b/frontend/src/App.css @@ -540,6 +540,21 @@ background: rgba(99, 102, 241, 0.22); } +/* Tracked-change overlay (Phase 3, §8.10) — preview-pane equivalents of + the .editor-content .tiptap rules above. The Contribute pane shows + these by default; the Discuss pane gates them behind a toolbar + toggle. Hovering surfaces a ChangeTooltip with the source message + and reason. */ +.markdown-preview .tracked-delete { + background: #fee2e2; color: #991b1b; text-decoration: line-through; + border-radius: 2px; padding: 1px 2px; cursor: pointer; + margin-right: 2px; +} +.markdown-preview .tracked-insert { + background: #dcfce7; color: #166534; + border-radius: 2px; padding: 1px 2px; cursor: pointer; +} + /* Narrow-viewport banner — Phase 2 punts the chat-drawer collapse to Phase 6, so the split layout asks for ≥1280px. */ .narrow-viewport-banner { diff --git a/frontend/src/components/ChangeTooltip.jsx b/frontend/src/components/ChangeTooltip.jsx new file mode 100644 index 0000000..f07b648 --- /dev/null +++ b/frontend/src/components/ChangeTooltip.jsx @@ -0,0 +1,71 @@ +// ChangeTooltip.jsx — the §8.10 hover affordance for a tracked-change +// span. Extracted from DiffView in Phase 3 so the MarkdownPreview +// tracked-changes overlay can reuse it. Same shape: badge row, +// user-prompt + selection-quote, and the AI's reason. + +import { MODEL_STYLES } from '../modelStyles' + +function tooltipStyle(x, y) { + const vw = window.innerWidth, vh = window.innerHeight + const style = {} + if (x > vw * 0.55) style.right = vw - x + 10 + else style.left = x + 14 + if (y > vh * 0.55) style.bottom = vh - y + 10 + else style.top = y + 14 + return style +} + +export function changeContext(change, messages) { + if (!change?.source_message_id || !messages) return {} + const idx = messages.findIndex(m => m.id === change.source_message_id) + if (idx < 0) return {} + const assistant = messages[idx] + const userMsg = [...messages].slice(0, idx).reverse().find(m => m.role === 'user') + return { assistant, userMsg } +} + +export default function ChangeTooltip({ change, messages, position }) { + const { assistant, userMsg } = changeContext(change, messages) + const style = { ...tooltipStyle(position.x, position.y), position: 'fixed' } + const modelStyle = MODEL_STYLES[assistant?.model_id] || MODEL_STYLES.default + + return ( +
+
+ {change.kind === 'ai' ? ( + + {modelStyle.label} + + ) : ( + Manual edit + )} + {change.was_edited_before_accept && ( + Edited before accept + )} +
+ {userMsg && ( +
+ {userMsg.quote && ( +
+ "{userMsg.quote.length > 120 ? userMsg.quote.slice(0, 120) + '…' : userMsg.quote}" +
+ )} +
+ {userMsg.text.length > 240 ? userMsg.text.slice(0, 240) + '…' : userMsg.text} +
+
+ )} + {change.reason && ( +
+ Reason + {change.reason} +
+ )} + {!userMsg && change.kind === 'ai' && ( +
+ No linked conversation message in this session. +
+ )} +
+ ) +} diff --git a/frontend/src/components/DiffView.jsx b/frontend/src/components/DiffView.jsx index 1240f62..b4f2c5c 100644 --- a/frontend/src/components/DiffView.jsx +++ b/frontend/src/components/DiffView.jsx @@ -7,72 +7,7 @@ // tooltip with the change's type/model/prompt/reason context. import { useState, useCallback } from 'react' -import { MODEL_STYLES } from '../modelStyles' - -function tooltipStyle(x, y) { - const vw = window.innerWidth, vh = window.innerHeight - const style = {} - if (x > vw * 0.55) style.right = vw - x + 10 - else style.left = x + 14 - if (y > vh * 0.55) style.bottom = vh - y + 10 - else style.top = y + 14 - return style -} - -function changeContext(change, messages) { - if (!change?.source_message_id) return {} - const idx = messages.findIndex(m => m.id === change.source_message_id) - if (idx < 0) return {} - const assistant = messages[idx] - const userMsg = [...messages].slice(0, idx).reverse().find(m => m.role === 'user') - return { assistant, userMsg } -} - -function ChangeTooltip({ change, messages, position }) { - const { assistant, userMsg } = changeContext(change, messages) - const style = { ...tooltipStyle(position.x, position.y), position: 'fixed' } - const modelStyle = MODEL_STYLES[assistant?.model_id] || MODEL_STYLES.default - - return ( -
-
- {change.kind === 'ai' ? ( - - {modelStyle.label} - - ) : ( - Manual edit - )} - {change.was_edited_before_accept && ( - Edited before accept - )} -
- {userMsg && ( -
- {userMsg.quote && ( -
- "{userMsg.quote.length > 120 ? userMsg.quote.slice(0, 120) + '…' : userMsg.quote}" -
- )} -
- {userMsg.text.length > 240 ? userMsg.text.slice(0, 240) + '…' : userMsg.text} -
-
- )} - {change.reason && ( -
- Reason - {change.reason} -
- )} - {!userMsg && change.kind === 'ai' && ( -
- No linked conversation message in this session. -
- )} -
- ) -} +import ChangeTooltip from './ChangeTooltip.jsx' export default function DiffView({ html, changes, messages }) { const [tooltip, setTooltip] = useState(null) diff --git a/frontend/src/components/MarkdownPreview.jsx b/frontend/src/components/MarkdownPreview.jsx index c7d8ea3..33f5a62 100644 --- a/frontend/src/components/MarkdownPreview.jsx +++ b/frontend/src/components/MarkdownPreview.jsx @@ -19,8 +19,10 @@ // attribute, and the renderer takes (source) → SVG. A future // authoring layer can intercept before mount without restructuring. -import { useEffect, useRef } from 'react' +import { useEffect, useRef, useState, useCallback } from 'react' import { Marked } from 'marked' +import { decorateAcceptedChanges } from './trackedOverlay.js' +import ChangeTooltip from './ChangeTooltip.jsx' // Module-level mermaid loader. Holds the Promise across consumers so // the chunk fetches exactly once across the app lifetime. @@ -71,6 +73,15 @@ export default function MarkdownPreview({ content, onSelectionChange, className, + // Phase 3 — tracked-change overlay (§8.10). When `showTrackedChanges` + // is true, accepted changes from the branch's changes list are + // decorated inline as / . Hovering a decorated span surfaces a + // ChangeTooltip with the source message + reason. `messages` is + // optional; without it the tooltip falls back to badge + reason. + changes, + messages, + showTrackedChanges = false, }) { const hostRef = useRef(null) // Per-block source memo — lets us skip mermaid re-renders for blocks @@ -79,8 +90,12 @@ export default function MarkdownPreview({ // Monotonic counter the async mermaid pass uses to bail if a newer // render has already replaced the DOM beneath it. const renderTokenRef = useRef(0) + // Tracked-change hover tooltip state. Shape: { change, position }. + const [tooltip, setTooltip] = useState(null) - // Render markdown → HTML on every content change. + // Render markdown → HTML on every content change. The tracked-change + // overlay runs in the same effect so accepted-change spans appear + // synchronously with the body itself — no flash of un-decorated text. useEffect(() => { if (!hostRef.current) return const html = previewMarked.parse(content || '') @@ -88,8 +103,11 @@ export default function MarkdownPreview({ const token = ++renderTokenRef.current // Reset memo so the new block set re-renders from scratch. lastMermaidSourcesRef.current = [] + if (showTrackedChanges && changes && changes.length > 0) { + decorateAcceptedChanges(hostRef.current, changes) + } renderMermaidBlocks(hostRef.current, lastMermaidSourcesRef, token, renderTokenRef) - }, [content]) + }, [content, showTrackedChanges, changes]) // Window-selection bridge for §8.12. Listen on document mouseup so // releases outside the preview still clear the prior selection. @@ -123,11 +141,42 @@ export default function MarkdownPreview({ return () => document.removeEventListener('mouseup', handleMouseUp) }, [onSelectionChange]) + // Hover handler for tracked-change spans (§8.10). The decorated spans + // carry data-change-id; we look the change row up out of `changes` + // and surface a ChangeTooltip anchored to the cursor. + const handleMouseMove = useCallback((e) => { + if (!showTrackedChanges || !changes) return + const span = e.target.closest?.('[data-change-id]') + if (!span) { + if (tooltip) setTooltip(null) + return + } + const id = span.getAttribute('data-change-id') + const change = changes.find(c => String(c.id) === String(id)) + if (!change) return + setTooltip({ change, position: { x: e.clientX, y: e.clientY } }) + }, [showTrackedChanges, changes, tooltip]) + + const handleMouseLeave = useCallback(() => { + setTooltip(null) + }, []) + return ( -
+ <> +
+ {tooltip && ( + + )} + ) } diff --git a/frontend/src/components/RFCView.jsx b/frontend/src/components/RFCView.jsx index 05bfe9e..52ab255 100644 --- a/frontend/src/components/RFCView.jsx +++ b/frontend/src/components/RFCView.jsx @@ -94,6 +94,11 @@ export default function RFCView({ viewer }) { const [selection, setSelection] = useState(null) const [reviewMode, setReviewMode] = useState(false) const [reviewHTML, setReviewHTML] = useState('') + // Phase 3 — Discuss-mode opt-in for the §8.10 tracked-change overlay + // in the single-pane preview. Contribute mode is default-on (the + // pane exists for editorial review of your own work); Discuss mode + // keeps clean prose by default and exposes a toggle. + const [discussShowChanges, setDiscussShowChanges] = useState(false) // Mode: discuss vs contribute (§8.3). Always discuss on main. const [mode, setMode] = useState('discuss') @@ -142,6 +147,7 @@ export default function RFCView({ viewer }) { setPendingDiscussChanges([]) setManualPending(null) setReviewMode(false) + setDiscussShowChanges(false) setSelection(null) setMode('discuss') @@ -649,6 +655,22 @@ export default function RFCView({ viewer }) {
)} + {inDiscuss && branchParam !== 'main' + && changes.some(c => c.state === 'accepted') && ( +
+ + + {changes.filter(c => c.state === 'accepted').length} accepted on this branch + +
+ )} {reviewMode ? ( ) : ( @@ -666,6 +688,9 @@ export default function RFCView({ viewer }) {
@@ -674,6 +699,9 @@ export default function RFCView({ viewer }) { content={editorContent} onSelectionChange={handleSelectionChange} className="markdown-preview-solo" + changes={changes} + messages={messages} + showTrackedChanges={inDiscuss && discussShowChanges} /> )} ` and +// `` markup for each accepted change on +// the branch, restoring the §8.10 inline tracked-change layer that +// Phase 1 dropped when Tiptap left. +// +// The pipeline is DOM-side (post-marked.parse) rather than markdown-side +// so the overlay sees the exact text the reader sees — header / +// emphasis / inline code shape doesn't matter for matching, and we +// don't try to re-anchor proposed text that crosses inline tags. +// Single-text-node match is the pre-fancy stance; cross-tag spans skip +// cleanly per the Phase 3 instructions ("if proposed doesn't match, +// skip the decoration rather than mis-anchor"). +// +// Subtleties handled: +// • acted_at ordering — later spans win on overlap because we always +// search for un-wrapped occurrences; an earlier wrap removes that +// substring from the search space, so a later change that hits the +// same passage just lands its mark on whatever is left. +// • mermaid intersections — text nodes inside `.mermaid-block` are +// skipped before matching, so a tracked span whose `proposed` +// only matches inside diagram source gets dropped silently. +// • drift — if the proposed text was further edited since acceptance +// it won't match verbatim; we skip rather than mis-anchor. +// +// Out of scope for Phase 3 (per the prompt): retro-fitting DiffView +// (which uses dangerouslySetInnerHTML — the per-session injection it +// expects is dying in Phase 7); rendering changes that have been +// declined or are still pending; cursor-aware "what's new since last +// visit" filtering. + +const DECORATABLE_PARENT_SKIP = new Set([ + 'SCRIPT', + 'STYLE', + 'CODE', + 'PRE', +]) + +function isInsideMermaid(node) { + let p = node.parentNode + while (p && p.nodeType === Node.ELEMENT_NODE) { + if (p.classList?.contains('mermaid-block')) return true + p = p.parentNode + } + return false +} + +function isInsideTrackedSpan(node) { + let p = node.parentNode + while (p && p.nodeType === Node.ELEMENT_NODE) { + if (p.classList?.contains('tracked-insert')) return true + if (p.classList?.contains('tracked-delete')) return true + p = p.parentNode + } + return false +} + +// Yield every text node inside `root` whose nearest non-text parent is +// neither inside a mermaid block nor inside an already-applied tracked +// span. Code/pre nodes are excluded — accepted-change text rarely +// targets verbatim code blocks and matching inside them produces noisy +// false positives. +function* eligibleTextNodes(root) { + const walker = document.createTreeWalker(root, NodeFilter.SHOW_TEXT, { + acceptNode(node) { + if (!node.nodeValue) return NodeFilter.FILTER_REJECT + const parent = node.parentElement + if (!parent) return NodeFilter.FILTER_REJECT + if (DECORATABLE_PARENT_SKIP.has(parent.tagName)) return NodeFilter.FILTER_REJECT + if (isInsideMermaid(node)) return NodeFilter.FILTER_REJECT + if (isInsideTrackedSpan(node)) return NodeFilter.FILTER_REJECT + return NodeFilter.FILTER_ACCEPT + }, + }) + let n = walker.nextNode() + while (n) { + yield n + n = walker.nextNode() + } +} + +// Collapse the kind of whitespace differences marked produces (newlines +// become spaces, indentation folds) so a proposed string authored +// against raw markdown can still match against rendered text. +function normalizeForMatch(s) { + return String(s || '').replace(/\s+/g, ' ').trim() +} + +// Find the first text node containing `needle` (after whitespace +// normalization) and return the (node, offsetInNodeValue, +// matchedLength) for splitting. Returns null when no eligible node +// contains the needle. +function findFirstMatch(root, needle) { + const target = normalizeForMatch(needle) + if (!target) return null + for (const node of eligibleTextNodes(root)) { + const haystack = node.nodeValue + const normalized = haystack.replace(/\s+/g, ' ') + const idx = normalized.indexOf(target) + if (idx < 0) continue + // Translate the normalized index back to the raw nodeValue offset. + // We walk the raw string counting non-whitespace characters until + // we hit the normalized index, then count out the matched length. + const start = mapNormalizedOffsetToRaw(haystack, idx) + const end = mapNormalizedOffsetToRaw(haystack, idx + target.length) + if (start == null || end == null || end <= start) continue + return { node, start, end } + } + return null +} + +function mapNormalizedOffsetToRaw(raw, normalizedOffset) { + // Re-derive the position in `raw` that corresponds to position + // `normalizedOffset` in `raw.replace(/\s+/g, ' ')`. We walk raw, + // tracking how many normalized characters we've emitted so far. + let emitted = 0 + let i = 0 + let inWhitespaceRun = false + while (i <= raw.length) { + if (emitted === normalizedOffset) return i + if (i === raw.length) break + const c = raw[i] + if (/\s/.test(c)) { + if (!inWhitespaceRun) { + emitted += 1 // collapsed whitespace counts as one space + inWhitespaceRun = true + } + } else { + emitted += 1 + inWhitespaceRun = false + } + i += 1 + } + return emitted === normalizedOffset ? raw.length : null +} + +function makeSpan(className, text, change) { + const span = document.createElement('span') + span.className = className + span.setAttribute('data-change-id', String(change.id)) + if (change.source_message_id != null) { + span.setAttribute('data-source-message-id', String(change.source_message_id)) + } + span.textContent = text + return span +} + +// Replace the matched range inside `node` with: optional tracked-delete +// span (rendering `original` struck-through), followed by tracked-insert +// span wrapping the matched proposed text. +function decorateMatch(match, change) { + const { node, start, end } = match + const before = node.nodeValue.slice(0, start) + const matched = node.nodeValue.slice(start, end) + const after = node.nodeValue.slice(end) + + const parent = node.parentNode + if (!parent) return + + const insertSpan = makeSpan('tracked-insert', matched, change) + let deleteSpan = null + const originalText = (change.original || '').trim() + // Skip the strikethrough when original is empty (pure insertion) or + // when it would duplicate the inserted text exactly — common for + // light AI edits where the proposal essentially repeats the source. + if (originalText && normalizeForMatch(originalText) !== normalizeForMatch(matched)) { + deleteSpan = makeSpan('tracked-delete', change.original, change) + } + + const beforeNode = before ? document.createTextNode(before) : null + const afterNode = after ? document.createTextNode(after) : null + + // Build the replacement sequence in order. + const frag = document.createDocumentFragment() + if (beforeNode) frag.appendChild(beforeNode) + if (deleteSpan) frag.appendChild(deleteSpan) + frag.appendChild(insertSpan) + if (afterNode) frag.appendChild(afterNode) + + parent.replaceChild(frag, node) +} + +// Sort accepted changes by acted_at ascending so later spans win on +// overlap. Falls back to id ordering when acted_at is null/equal. +function orderForDecoration(changes) { + return changes + .filter(c => c?.state === 'accepted' && c.proposed) + .slice() + .sort((a, b) => { + const aa = a.acted_at || '' + const bb = b.acted_at || '' + if (aa === bb) return (a.id || 0) - (b.id || 0) + return aa.localeCompare(bb) + }) +} + +export function decorateAcceptedChanges(root, changes) { + if (!root || !changes || changes.length === 0) return + const ordered = orderForDecoration(changes) + for (const change of ordered) { + const match = findFirstMatch(root, change.proposed) + if (!match) continue + try { + decorateMatch(match, change) + } catch { + // Tolerate per-change failures so one bad span doesn't kill the + // overlay for everything that came after it. + } + } +}