From 3f354e227a11345218b8ca01180176be35e61dde Mon Sep 17 00:00:00 2001 From: Heiko Braun Date: Tue, 19 May 2026 16:32:09 +0200 Subject: [PATCH] Show outdated comments instead of silently hiding them When a document is edited after comments are placed, character offsets shift and excerpt text may vanish. Previously these threads silently disappeared from the document view while the sidebar still showed the correct count. Now the server computes outdated status (hash mismatch + excerpt not found) and the frontend renders unresolvable threads in a visible "Outdated Comments" section at the bottom of the document with their original context preserved. Also removes the dead sync/publish code (ReviewSyncer interface, handlers, UI buttons) which were no-op stubs. Co-Authored-By: Claude Opus 4.6 --- internal/cli/review.go | 13 +--- internal/review/api.go | 32 ++-------- internal/review/client.go | 20 +----- internal/review/server.go | 78 +++++++++-------------- internal/review/types.go | 12 ++++ internal/review/ui.go | 130 ++++++++++++++++++++++++-------------- 6 files changed, 133 insertions(+), 152 deletions(-) diff --git a/internal/cli/review.go b/internal/cli/review.go index aa29669..aefd9c0 100644 --- a/internal/cli/review.go +++ b/internal/cli/review.go @@ -96,7 +96,7 @@ func runReview(port int, branchOverride, serverURL string, statusOnly, debug boo // 5. --status mode. if statusOnly { - return printReviewStatus(client, client) + return printReviewStatus(client) } // 6. Index documents from local filesystem. @@ -110,7 +110,7 @@ func runReview(port int, branchOverride, serverURL string, statusOnly, debug boo // 8. Create local server backed by remote client. srv := review.NewServer( - client, client, docIndex, cfg, + client, docIndex, cfg, repo.Root, repo.Root, sourceBranch, filepath.Base(repo.Root), userEmail, debug, @@ -136,7 +136,7 @@ func runReview(port int, branchOverride, serverURL string, statusOnly, debug boo return nil } -func printReviewStatus(store review.ReviewStore, syncer review.ReviewSyncer) error { +func printReviewStatus(store review.ReviewStore) error { openReviews, err := store.ListOpenReviews() if err != nil { return fmt.Errorf("listing open reviews: %w", err) @@ -154,16 +154,9 @@ func printReviewStatus(store review.ReviewStore, syncer review.ReviewSyncer) err } } - pending, _ := syncer.HasPendingChanges() - fmt.Printf("Open reviews: %d\n", len(openReviews)) fmt.Printf("Open threads: %d\n", openThreads) fmt.Printf("Total threads: %d\n", len(allThreads)) - if pending { - fmt.Printf("Pending changes: yes\n") - } else { - fmt.Printf("Pending changes: no\n") - } return nil } diff --git a/internal/review/api.go b/internal/review/api.go index d5e80d8..cbca3dd 100644 --- a/internal/review/api.go +++ b/internal/review/api.go @@ -17,13 +17,6 @@ type ReviewStore interface { CreateReview(title string, documents []string, sourceRef string) (*Review, error) } -// ReviewSyncer is the interface for sync operations. -type ReviewSyncer interface { - SyncAll() error - Publish() error - HasPendingChanges() (bool, error) -} - // --- Request types --- // CreateReviewRequest is the JSON body for POST /api/reviews. @@ -71,24 +64,9 @@ type DocumentDetail struct { // StatusResponse is returned by GET /api/status. type StatusResponse struct { - RepoName string `json:"repo_name"` - Branch string `json:"branch"` - PendingChanges bool `json:"pending_changes"` - OpenReviews int `json:"open_reviews"` - OpenThreads int `json:"open_threads"` - TotalThreads int `json:"total_threads"` -} - -// SyncResponse is returned by POST /api/sync. -type SyncResponse struct { - OK bool `json:"ok"` - Message string `json:"message,omitempty"` - Error string `json:"error,omitempty"` -} - -// PublishResponse is returned by POST /api/publish. -type PublishResponse struct { - OK bool `json:"ok"` - Message string `json:"message,omitempty"` - Error string `json:"error,omitempty"` + RepoName string `json:"repo_name"` + Branch string `json:"branch"` + OpenReviews int `json:"open_reviews"` + OpenThreads int `json:"open_threads"` + TotalThreads int `json:"total_threads"` } diff --git a/internal/review/client.go b/internal/review/client.go index ff546a8..c40ac0a 100644 --- a/internal/review/client.go +++ b/internal/review/client.go @@ -10,13 +10,10 @@ import ( "time" ) -// Compile-time interface checks. -var ( - _ ReviewStore = (*Client)(nil) - _ ReviewSyncer = (*Client)(nil) -) +// Compile-time interface check. +var _ ReviewStore = (*Client)(nil) -// Client is an HTTP client that implements ReviewStore and ReviewSyncer +// Client is an HTTP client that implements ReviewStore // by delegating to the reviewd API. type Client struct { baseURL string // e.g. "http://localhost:5100" @@ -248,17 +245,6 @@ func (c *Client) CreateReview(title string, documents []string, sourceRef string return &rev, nil } -// --- ReviewSyncer implementation --- - -// SyncAll is a no-op — data lives on the server. -func (c *Client) SyncAll() error { return nil } - -// Publish is a no-op — mutations are sent immediately. -func (c *Client) Publish() error { return nil } - -// HasPendingChanges always returns false — no local queue. -func (c *Client) HasPendingChanges() (bool, error) { return false, nil } - // --- Helpers --- func newUUIDMust() string { diff --git a/internal/review/server.go b/internal/review/server.go index ea3ca32..1f67656 100644 --- a/internal/review/server.go +++ b/internal/review/server.go @@ -22,7 +22,6 @@ import ( // Server serves the review UI and JSON API. type Server struct { store ReviewStore - syncer ReviewSyncer docIndex *DocIndex config ReviewConfig docsRoot string @@ -35,10 +34,9 @@ type Server struct { logger *log.Logger } -// NewServer creates a Server wired to the given store, syncer, and document index. +// NewServer creates a Server wired to the given store and document index. func NewServer( store ReviewStore, - syncer ReviewSyncer, docIndex *DocIndex, config ReviewConfig, docsRoot, repoRoot, sourceBranch, repoName, userEmail string, @@ -46,7 +44,6 @@ func NewServer( ) *Server { s := &Server{ store: store, - syncer: syncer, docIndex: docIndex, config: config, docsRoot: docsRoot, @@ -92,8 +89,6 @@ func (s *Server) routes() { s.mux.HandleFunc("/api/reviews", s.handleReviews) s.mux.HandleFunc("/api/threads", s.handleThreads) s.mux.HandleFunc("/api/threads/", s.handleThreadAction) - s.mux.HandleFunc("/api/sync", s.handleSync) - s.mux.HandleFunc("/api/publish", s.handlePublish) s.mux.HandleFunc("/api/status", s.handleStatus) } @@ -180,6 +175,19 @@ func (s *Server) handleDocumentDetail(w http.ResponseWriter, r *http.Request) { threads, _ := s.store.ListThreadsByDocument(docPath) + // Compute outdated status for each thread. + textContent := stripHTMLToText(htmlContent) + for i := range threads { + t := &threads[i] + if t.Anchor.FileHash == fileHash { + continue // hash matches, offsets are valid + } + if t.Anchor.Excerpt != "" && strings.Contains(textContent, t.Anchor.Excerpt) { + continue // excerpt still found via fallback + } + t.Outdated = true + } + detail := DocumentDetail{ Path: docPath, Title: doc.Title, @@ -338,42 +346,6 @@ func (s *Server) handleThreadAction(w http.ResponseWriter, r *http.Request) { } } -func (s *Server) handleSync(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost { - http.Error(w, "method not allowed", http.StatusMethodNotAllowed) - return - } - - err := s.syncer.SyncAll() - if err != nil { - writeJSONResponse(w, SyncResponse{OK: false, Error: err.Error()}) - return - } - - // Re-index documents after sync. - newIndex, indexErr := IndexDocuments(s.docsRoot, s.config.DocumentPaths) - if indexErr == nil { - s.docIndex = newIndex - } - - writeJSONResponse(w, SyncResponse{OK: true, Message: "sync complete"}) -} - -func (s *Server) handlePublish(w http.ResponseWriter, r *http.Request) { - if r.Method != http.MethodPost { - http.Error(w, "method not allowed", http.StatusMethodNotAllowed) - return - } - - err := s.syncer.Publish() - if err != nil { - writeJSONResponse(w, PublishResponse{OK: false, Error: err.Error()}) - return - } - - writeJSONResponse(w, PublishResponse{OK: true, Message: "published successfully"}) -} - func (s *Server) handleStatus(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { http.Error(w, "method not allowed", http.StatusMethodNotAllowed) @@ -382,7 +354,6 @@ func (s *Server) handleStatus(w http.ResponseWriter, r *http.Request) { openReviews, _ := s.store.ListOpenReviews() allThreads, _ := s.store.ListAllThreads() - pending, _ := s.syncer.HasPendingChanges() openThreads := 0 for _, t := range allThreads { @@ -392,12 +363,11 @@ func (s *Server) handleStatus(w http.ResponseWriter, r *http.Request) { } writeJSONResponse(w, StatusResponse{ - RepoName: s.repoName, - Branch: s.sourceBranch, - PendingChanges: pending, - OpenReviews: len(openReviews), - OpenThreads: openThreads, - TotalThreads: len(allThreads), + RepoName: s.repoName, + Branch: s.sourceBranch, + OpenReviews: len(openReviews), + OpenThreads: openThreads, + TotalThreads: len(allThreads), }) } @@ -414,6 +384,16 @@ func httpError(w http.ResponseWriter, msg string, err error) { http.Error(w, fmt.Sprintf("%s: %v", msg, err), http.StatusInternalServerError) } +// stripHTMLToText removes HTML tags and returns the text content, +// approximating the browser's element.textContent for anchor matching. +func stripHTMLToText(html string) string { + // Remove all HTML tags. + stripped := regexp.MustCompile(`<[^>]*>`).ReplaceAllString(html, "") + // Collapse whitespace runs the way textContent does. + stripped = strings.Join(strings.Fields(stripped), " ") + return stripped +} + // renderReviewMarkdown renders markdown to HTML using goldmark, adding // data-paragraph-index attributes to

tags for anchor positioning. func renderReviewMarkdown(source []byte) (string, error) { diff --git a/internal/review/types.go b/internal/review/types.go index a8bf212..8f47160 100644 --- a/internal/review/types.go +++ b/internal/review/types.go @@ -92,6 +92,14 @@ type Anchor struct { // Excerpt is the selected text. Used for display and as a fallback // to re-locate the annotation when the file has changed. Excerpt string `json:"excerpt"` + + // ContextBefore is ~300 chars of rendered text preceding the selection, + // captured at creation time for display when the anchor becomes outdated. + ContextBefore string `json:"context_before,omitempty"` + + // ContextAfter is ~300 chars of rendered text following the selection, + // captured at creation time for display when the anchor becomes outdated. + ContextAfter string `json:"context_after,omitempty"` } // ThreadStatus represents the resolution status of a review thread. @@ -148,6 +156,10 @@ type Thread struct { // UpdatedAt is the RFC 3339 timestamp when the thread was last modified. UpdatedAt string `json:"updated_at,omitempty"` + + // Outdated is a computed field (not persisted) indicating the anchor + // can no longer be resolved in the current document content. + Outdated bool `json:"outdated,omitempty"` } // Comment is a single entry in a thread's discussion. diff --git a/internal/review/ui.go b/internal/review/ui.go index d93836a..ae5bac5 100644 --- a/internal/review/ui.go +++ b/internal/review/ui.go @@ -252,6 +252,48 @@ body { opacity: 0.6; } +/* Outdated threads section */ +.outdated-section { + margin-top: 3rem; + padding-top: 1.5rem; + border-top: 2px dashed var(--warning, #d97706); +} +.outdated-header { + font-size: 0.8rem; + font-weight: 600; + color: var(--warning, #d97706); + margin-bottom: 0.75rem; + text-transform: uppercase; + letter-spacing: 0.05em; +} +.outdated-thread { + border-left: 3px solid var(--warning, #d97706); + padding: 0.75rem; + margin-bottom: 0.75rem; + background: rgba(217, 119, 6, 0.04); + border-radius: 0 var(--radius, 6px) var(--radius, 6px) 0; + cursor: pointer; + transition: background 0.15s; +} +.outdated-thread:hover { background: rgba(217, 119, 6, 0.1); } +.outdated-context { + font-size: 0.8rem; + line-height: 1.6; + white-space: pre-wrap; + word-break: break-word; +} +.outdated-ctx-fade { color: var(--text-muted, #6b7280); opacity: 0.5; } +.outdated-excerpt { + background: rgba(217, 119, 6, 0.15); + border-bottom: 2px solid var(--warning, #d97706); + border-radius: 2px; +} +.outdated-meta { + font-size: 0.7rem; + color: var(--text-muted, #6b7280); + margin-top: 0.4rem; +} + /* Right panel */ .panel { border-left: 1px solid var(--border); @@ -550,9 +592,6 @@ body { -- -- - - -

@@ -666,6 +705,7 @@ function renderDocContent() { '' + '
' + currentDoc.html + '
'; applyHighlights(); + renderOutdatedThreads(); setupTextSelection(); renderMermaid(); } @@ -726,8 +766,9 @@ function applyHighlights() { const fileHashMatch = currentDoc.file_hash; // Sort threads by start offset descending so wrapping doesn't shift later offsets. + // Skip outdated threads — they render in a separate section. const sorted = [...currentDoc.threads] - .filter(t => t.anchor && t.anchor.excerpt) + .filter(t => t.anchor && t.anchor.excerpt && !t.outdated) .sort((a, b) => (b.anchor.start || 0) - (a.anchor.start || 0)); sorted.forEach(t => { @@ -768,6 +809,34 @@ function applyHighlights() { }); } +function renderOutdatedThreads() { + if (!currentDoc || !currentDoc.threads) return; + const outdated = currentDoc.threads.filter(t => t.outdated); + if (outdated.length === 0) return; + const wrapper = document.getElementById('doc-content-wrapper'); + let html = '
Outdated comments (' + outdated.length + ')
'; + outdated.forEach(t => { + const commentCount = (t.comments || []).length; + html += '
'; + html += '
'; + if (t.anchor.context_before) html += '' + escHtml(t.anchor.context_before) + ''; + html += '' + escHtml(t.anchor.excerpt) + ''; + if (t.anchor.context_after) html += '' + escHtml(t.anchor.context_after) + ''; + html += '
'; + html += '
' + commentCount + ' comment' + (commentCount !== 1 ? 's' : '') + + ' · ' + (t.status || 'open') + '
'; + html += '
'; + }); + html += '
'; + wrapper.insertAdjacentHTML('beforeend', html); + wrapper.querySelectorAll('.outdated-thread').forEach(el => { + el.onclick = () => { + const t = outdated.find(th => th.id === el.dataset.threadId); + if (t) showThread(t); + }; + }); +} + function setupTextSelection() { const wrapper = document.getElementById('doc-content-wrapper'); wrapper.addEventListener('mouseup', () => { @@ -802,6 +871,11 @@ async function submitNewComment() { const input = document.getElementById('modal-comment-input'); const body = input.value.trim(); if (!body || !selectionAnchor || !currentDoc) return; + // Capture surrounding context for outdated-comment display. + const fullText = document.getElementById('doc-content-wrapper').textContent; + const ctxRadius = 300; + const ctxBefore = fullText.substring(Math.max(0, selectionAnchor.start - ctxRadius), selectionAnchor.start); + const ctxAfter = fullText.substring(selectionAnchor.end, Math.min(fullText.length, selectionAnchor.end + ctxRadius)); const req = { review_id: '', document: currentDoc.path, @@ -809,7 +883,9 @@ async function submitNewComment() { file_hash: currentDoc.file_hash || '', start: selectionAnchor.start, end: selectionAnchor.end, - excerpt: selectionAnchor.excerpt + excerpt: selectionAnchor.excerpt, + context_before: ctxBefore, + context_after: ctxAfter }, body: body, author: '' @@ -911,54 +987,10 @@ function showToast(msg, type) { setTimeout(() => { el.remove(); }, 3000); } -async function doSync() { - const btn = document.getElementById('btn-sync'); - btn.disabled = true; - btn.textContent = 'Syncing...'; - try { - await api('/api/sync', { method: 'POST' }); - showToast('Sync complete', 'success'); - await loadDocuments(); - await loadStatus(); - if (currentDoc) await selectDoc(currentDoc.path); - } catch (e) { - showToast('Sync failed: ' + e.message, 'error'); - } finally { - btn.disabled = false; - btn.textContent = 'Sync'; - } -} - -async function doPublish() { - const btn = document.getElementById('btn-publish'); - btn.disabled = true; - btn.textContent = 'Publishing...'; - try { - const res = await api('/api/publish', { method: 'POST' }); - if (res.error) { - showToast('Publish failed: ' + res.error, 'error'); - } else { - showToast('Published successfully', 'success'); - } - await loadStatus(); - } catch (e) { - showToast('Publish failed: ' + e.message, 'error'); - } finally { - btn.disabled = false; - btn.textContent = 'Publish'; - } -} - async function loadStatus() { const s = await api('/api/status'); document.getElementById('status-repo').textContent = s.repo_name || '--'; document.getElementById('status-branch').textContent = s.branch || '--'; - const pending = document.getElementById('status-pending'); - if (s.pending_changes) { - pending.textContent = 'Pending changes'; - } else { - pending.textContent = ''; - } } function formatTime(ts) {