From 3607551c9d2e1c4d5b04e7e14a6ed2da6d3f857f Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 19 May 2026 12:08:33 +1000 Subject: [PATCH 1/7] chore(staged): log diff load timing on frontend and backend Add info-level logs along the diff-open path so slow clicks can be attributed to the right stage. Frontend logs cover modal open, loadFiles, loadFileDiff (including cache hits), and switchContext with elapsed millis. Backend fills in the previously-silent remote cache hit paths and the remote cache-miss collection step in get_diff_files and get_file_diff. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/diff_commands.rs | 36 ++++++++++++++++++ .../features/diff/diffViewerState.svelte.ts | 37 +++++++++++++++++-- 2 files changed, 70 insertions(+), 3 deletions(-) diff --git a/apps/staged/src-tauri/src/diff_commands.rs b/apps/staged/src-tauri/src/diff_commands.rs index a0ee7b088..72948868d 100644 --- a/apps/staged/src-tauri/src/diff_commands.rs +++ b/apps/staged/src-tauri/src/diff_commands.rs @@ -548,6 +548,12 @@ pub(crate) fn get_diff_files_impl( &branch_id, sha, ) { + log::info!( + "get_diff_files: remote branch cache hit in {:?}: {} files (sha={})", + start.elapsed(), + cached.files.len(), + &sha[..7.min(sha.len())] + ); return Ok(DiffFilesResponse { commit_sha: sha.clone(), files: cached.files, @@ -564,6 +570,12 @@ pub(crate) fn get_diff_files_impl( &branch_id, sha, ) { + log::info!( + "get_diff_files: remote commit cache hit in {:?}: {} files (sha={})", + start.elapsed(), + cached.files.len(), + &sha[..7.min(sha.len())] + ); return Ok(DiffFilesResponse { commit_sha: sha.clone(), files: cached.files, @@ -572,8 +584,17 @@ pub(crate) fn get_diff_files_impl( } } + log::info!( + "get_diff_files: remote cache miss, populating cache (scope={scope}, branch_id={branch_id})" + ); + let t_populate = std::time::Instant::now(); let (index, _, commit_results) = ensure_cache_populated(&ctx, &store, &branch_id, &scope, commit_sha.as_deref())?; + log::info!( + "get_diff_files: ensure_cache_populated done in {:?} (total {:?})", + t_populate.elapsed(), + start.elapsed() + ); if scope == "commit" { let sha = commit_sha.ok_or("commit_sha required for commit scope")?; @@ -662,6 +683,10 @@ pub(crate) fn get_file_diff_impl( &commit_sha, &path, ) { + log::info!( + "get_file_diff: remote branch cache hit in {:?}: path={path}", + start.elapsed() + ); return Ok(file_diff); } } @@ -675,12 +700,23 @@ pub(crate) fn get_file_diff_impl( &commit_sha, &path, ) { + log::info!( + "get_file_diff: remote commit cache hit in {:?}: path={path}", + start.elapsed() + ); return Ok(file_diff); } } + log::info!("get_file_diff: remote cache miss, populating cache (scope={scope}, path={path})"); + let t_populate = std::time::Instant::now(); let (_, branch_file_diffs, commit_results) = ensure_cache_populated(&ctx, &store, &branch_id, &scope, Some(&commit_sha))?; + log::info!( + "get_file_diff: ensure_cache_populated done in {:?} (total {:?})", + t_populate.elapsed(), + start.elapsed() + ); if scope == "commit" { if let Some(diff) = commit_results diff --git a/apps/staged/src/lib/features/diff/diffViewerState.svelte.ts b/apps/staged/src/lib/features/diff/diffViewerState.svelte.ts index 16df8640f..e4e39686e 100644 --- a/apps/staged/src/lib/features/diff/diffViewerState.svelte.ts +++ b/apps/staged/src/lib/features/diff/diffViewerState.svelte.ts @@ -38,20 +38,36 @@ export function createDiffViewerState(branchId: string, scope: DiffScope, commit let selectionGeneration = 0; let contextGeneration = 0; + console.info( + `[diff] open: branchId=${branchId} scope=${scope} commitSha=${commitSha ?? '(unresolved)'}` + ); + async function loadFiles(generation: number): Promise { state.loading = true; state.error = null; + const t0 = performance.now(); + console.info( + `[diff] loadFiles start: branchId=${state.branchId} scope=${state.scope} commitSha=${state.commitSha ?? '(unresolved)'}` + ); try { const response = await commands.getDiffFiles( state.branchId, state.commitSha ?? undefined, state.scope ); - if (generation !== contextGeneration) return; + if (generation !== contextGeneration) { + console.info( + `[diff] loadFiles stale (took ${Math.round(performance.now() - t0)}ms) — ignoring` + ); + return; + } state.commitSha = response.commitSha; state.files = response.files; + console.info( + `[diff] loadFiles done in ${Math.round(performance.now() - t0)}ms: files=${response.files.length} commitSha=${response.commitSha}` + ); if (state.files.length > 0) { await selectFile(sharedFileSummaryPath(state.files[0])); @@ -60,6 +76,9 @@ export function createDiffViewerState(branchId: string, scope: DiffScope, commit if (generation !== contextGeneration) return; state.error = e instanceof Error ? e.message : String(e); state.files = []; + console.warn( + `[diff] loadFiles failed in ${Math.round(performance.now() - t0)}ms: ${state.error}` + ); } finally { if (generation === contextGeneration) { state.loading = false; @@ -81,18 +100,29 @@ export function createDiffViewerState(branchId: string, scope: DiffScope, commit if (!state.commitSha) return null; const cached = state.diffCache.get(path); - if (cached) return cached; + if (cached) { + console.info(`[diff] loadFileDiff cache hit: path=${path}`); + return cached; + } state.loadingFile = path; + const t0 = performance.now(); + console.info(`[diff] loadFileDiff start: path=${path} scope=${state.scope}`); try { const diff = await commands.getFileDiff(state.branchId, state.commitSha, state.scope, path); const newCache = new Map(state.diffCache); newCache.set(path, diff); state.diffCache = newCache; + console.info( + `[diff] loadFileDiff done in ${Math.round(performance.now() - t0)}ms: path=${path}` + ); return diff; } catch (e) { - console.error(`Failed to load diff for ${path}:`, e); + console.error( + `[diff] loadFileDiff failed in ${Math.round(performance.now() - t0)}ms: path=${path}:`, + e + ); return null; } finally { state.loadingFile = null; @@ -109,6 +139,7 @@ export function createDiffViewerState(branchId: string, scope: DiffScope, commit newCommitSha?: string ): Promise { const generation = ++contextGeneration; + console.info(`[diff] switchContext: scope=${newScope} commitSha=${newCommitSha ?? '(none)'}`); state.scope = newScope; state.commitSha = newCommitSha ?? null; state.diffCache = new Map(); From 5b2b4a3308222ad70a01d1eef7168b44bd238da3 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 5 Jun 2026 11:41:01 +1000 Subject: [PATCH 2/7] =?UTF-8?q?chore(staged):=20log=20click=E2=86=92open?= =?UTF-8?q?=20and=20post-render=20diff=20timing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing diff-open instrumentation brackets the wrong window: it starts at "[diff] open" (DiffModal init) and stops at computeLineDiff, leaving two uninstrumented gaps where the perceived latency actually lives. Close both. Click→open: markDiffOpenClick() stamps performance.now() at every diff-open trigger in BranchCard (Diff button, review/commit/worktree opens). createDiffViewerState logs the delta to "[diff] open" as clickToOpen, capturing the DiffModal + DiffViewer mount cost. Render tail: a guarded $effect in DiffModal times from a file's diff becoming available through to pixels on screen via a double-rAF, so syntax highlighting, DOM build, and layout/paint after computeLineDiff are no longer invisible. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- .../lib/features/branches/BranchCard.svelte | 12 +++++++++- .../src/lib/features/diff/DiffModal.svelte | 22 +++++++++++++++++++ .../features/diff/diffViewerState.svelte.ts | 20 ++++++++++++++++- 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/apps/staged/src/lib/features/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index 98fcb5651..ba2bc3d9d 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -39,6 +39,7 @@ import ImageViewerModal from '../timeline/ImageViewerModal.svelte'; import { countUserComments, shouldWarnBeforeDeletingReview } from '../timeline/reviewState'; import DiffModal from '../diff/DiffModal.svelte'; + import { markDiffOpenClick } from '../diff/diffViewerState.svelte'; import SessionModal from '../sessions/SessionModal.svelte'; import NewSessionModal from '../sessions/NewSessionModal.svelte'; import NoteModal from '../notes/NoteModal.svelte'; @@ -845,6 +846,7 @@ } function handleCommitClick(sha: string) { + markDiffOpenClick(); commitDiffSha = sha; } @@ -863,6 +865,7 @@ const cached = timelineReviewDetailsById[reviewId]; if (cached) { reviewDiffTarget = { commitSha: cached.commitSha, scope: cached.scope, reviewId }; + markDiffOpenClick(); showBranchDiff = true; return; } @@ -875,6 +878,7 @@ return; } reviewDiffTarget = { commitSha: review.commitSha, scope: review.scope, reviewId }; + markDiffOpenClick(); showBranchDiff = true; } catch (e) { console.error('Failed to open review:', e); @@ -1458,7 +1462,12 @@ : undefined} {forcePushingOrigin} rebaseBranchDisabledReason={branchCommandDisabledReason} - onViewWorktreeDiff={isLocal ? () => (showWorktreeDiff = true) : undefined} + onViewWorktreeDiff={isLocal + ? () => { + markDiffOpenClick(); + showWorktreeDiff = true; + } + : undefined} onCommitWorktreeChanges={() => sessionMgr.startOrQueueSession('commit', 'Commit uncommitted changes')} onDiscardWorktreeChanges={handleDiscardWorktreeChanges} @@ -1490,6 +1499,7 @@ size="sm" onclick={() => { reviewDiffTarget = null; + markDiffOpenClick(); showBranchDiff = true; }} class="text-xs" diff --git a/apps/staged/src/lib/features/diff/DiffModal.svelte b/apps/staged/src/lib/features/diff/DiffModal.svelte index 78406c643..7ab11b387 100644 --- a/apps/staged/src/lib/features/diff/DiffModal.svelte +++ b/apps/staged/src/lib/features/diff/DiffModal.svelte @@ -645,6 +645,28 @@ // ========================================================================== let currentDiff = $derived(diffViewer.getCurrentDiff()); + + // Render-tail timing: `computeLineDiff` is sub-millisecond, but the syntax + // highlighting, DOM construction, and layout/paint the DiffViewer performs + // afterwards are never measured. Once a file's diff is in the cache and + // selected, time how long it takes to actually hit the screen via a + // double-rAF (the second frame fires after the browser has painted). Guarded + // by path so unrelated reactive updates (comments, search) don't re-log. + let renderTimedPath: string | null = null; + $effect(() => { + const path = diffViewer.state.selectedFile; + const ready = currentDiff !== null && diffViewer.state.loadingFile === null; + if (!path || !ready || renderTimedPath === path) return; + renderTimedPath = path; + const t0 = performance.now(); + requestAnimationFrame(() => { + requestAnimationFrame(() => { + console.info( + `[diff] render painted in ${Math.round(performance.now() - t0)}ms: path=${path}` + ); + }); + }); + }); let diffViewerEmptyMessage = $derived( diffViewer.state.loading || diffViewer.state.loadingFile !== null ? 'Loading changes...' diff --git a/apps/staged/src/lib/features/diff/diffViewerState.svelte.ts b/apps/staged/src/lib/features/diff/diffViewerState.svelte.ts index e4e39686e..17a71a288 100644 --- a/apps/staged/src/lib/features/diff/diffViewerState.svelte.ts +++ b/apps/staged/src/lib/features/diff/diffViewerState.svelte.ts @@ -19,6 +19,19 @@ export interface DiffViewerState { error: string | null; } +/** + * Timestamp of the most recent diff-open trigger (e.g. a "Diff" button click), + * recorded by `markDiffOpenClick`. Lets `createDiffViewerState` log the + * click→open gap — the otherwise-invisible window spent mounting DiffModal and + * its statically-imported DiffViewer before any diff work begins. + */ +let lastOpenClickAt: number | null = null; + +/** Stamp the moment a diff-open was triggered, just before showing DiffModal. */ +export function markDiffOpenClick() { + lastOpenClickAt = performance.now(); +} + /** * Create a reactive diff viewer state instance, pre-bound to Staged's Tauri commands. */ @@ -38,8 +51,13 @@ export function createDiffViewerState(branchId: string, scope: DiffScope, commit let selectionGeneration = 0; let contextGeneration = 0; + const clickToOpen = + lastOpenClickAt !== null + ? ` clickToOpen=${Math.round(performance.now() - lastOpenClickAt)}ms` + : ''; + lastOpenClickAt = null; console.info( - `[diff] open: branchId=${branchId} scope=${scope} commitSha=${commitSha ?? '(unresolved)'}` + `[diff] open: branchId=${branchId} scope=${scope} commitSha=${commitSha ?? '(unresolved)'}${clickToOpen}` ); async function loadFiles(generation: number): Promise { From b45c465688a379173b50fb9b56d462cc5b611584 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 5 Jun 2026 12:08:59 +1000 Subject: [PATCH 3/7] refactor(diff-viewer): index diff lines by identity for windowing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of virtualizing the diff body: decouple anchor positioning from the assumption that every line is in the DOM, so it stays correct once the body renders only a window of lines. Stamp every rendered .line with data-line-index={i} (absolute index) in all four pane {#each} blocks, and add a lineAt(pane, n) helper that resolves a line by that attribute. Convert the positional NodeList lookups (querySelectorAll('.line')[n]) in updateToolbarPosition, decideCommentPosition, updateCommentEditorPosition, updateLineSelectionToolbar, decideLineCommentPosition, updateLineCommentEditorPosition, and the range-copy loop to identity lookups via lineAt. Replace handleSelectionDragMove's rect hit-test loop with arithmetic (floor((clientY - paneTop + scrollY) / lineHeight), clamped) so the drag focus line is derived from scroll math instead of iterating rendered rows — correct and faster even when rows are unrendered. No windowing yet: this is a correctness-preserving change against the current full render. Comment open/jump, range-selection toolbar, and drag-select behave identically. Typecheck passes. Signed-off-by: Matt Toohey --- .../src/lib/components/DiffViewer.svelte | 71 +++++++++++-------- 1 file changed, 42 insertions(+), 29 deletions(-) diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index 6284f3925..7ae40f25d 100644 --- a/packages/diff-viewer/src/lib/components/DiffViewer.svelte +++ b/packages/diff-viewer/src/lib/components/DiffViewer.svelte @@ -1185,6 +1185,14 @@ // Range hover handling // ========================================================================== + // Resolve a line element by its absolute index rather than by NodeList + // position. Positional lookups (`querySelectorAll('.line')[n]`) only equal the + // absolute index while every line is in the DOM; identity lookups stay correct + // once the body is windowed and only a slice of lines is rendered. + function lineAt(pane: HTMLElement, index: number): HTMLElement | null { + return pane.querySelector(`.line[data-line-index="${index}"]`); + } + function updateToolbarPosition() { if (hoveredRangeIndex === null || !afterPane || !diffViewerEl) { rangeToolbarStyle = null; @@ -1198,7 +1206,7 @@ } const lineIndex = alignmentData.alignment.after.start; - const lineEl = afterPane.querySelectorAll('.line')[lineIndex] as HTMLElement | null; + const lineEl = lineAt(afterPane, lineIndex); if (!lineEl) { rangeToolbarStyle = null; @@ -1347,15 +1355,13 @@ const editorHeight = 120; const lastLineIndex = Math.max(alignment.after.start, alignment.after.end - 1); - const lastLineEl = afterPane.querySelectorAll('.line')[lastLineIndex] as HTMLElement | null; + const lastLineEl = lineAt(afterPane, lastLineIndex); if (!lastLineEl) return 'below'; const lastLineRect = lastLineEl.getBoundingClientRect(); const spaceBelow = paneRect.bottom - lastLineRect.bottom; - const firstLineEl = afterPane.querySelectorAll('.line')[ - alignment.after.start - ] as HTMLElement | null; + const firstLineEl = lineAt(afterPane, alignment.after.start); if (!firstLineEl) return 'below'; const firstLineRect = firstLineEl.getBoundingClientRect(); @@ -1384,15 +1390,13 @@ if (commentPositionPreference === 'below') { const lastLineIndex = Math.max(alignment.after.start, alignment.after.end - 1); - anchorLineEl = afterPane.querySelectorAll('.line')[lastLineIndex] as HTMLElement | null; + anchorLineEl = lineAt(afterPane, lastLineIndex); if (!anchorLineEl) { commentEditorStyle = null; return; } } else { - anchorLineEl = afterPane.querySelectorAll('.line')[ - alignment.after.start - ] as HTMLElement | null; + anchorLineEl = lineAt(afterPane, alignment.after.start); if (!anchorLineEl) { commentEditorStyle = null; return; @@ -1458,18 +1462,27 @@ function handleSelectionDragMove(event: MouseEvent) { if (!isSelecting || !lineSelection) return; - const pane = lineSelection.pane === 'before' ? beforePane : afterPane; + const side = lineSelection.pane; + const pane = side === 'before' ? beforePane : afterPane; if (!pane) return; - const lineElements = pane.querySelectorAll('.line'); - for (let i = 0; i < lineElements.length; i++) { - const rect = lineElements[i].getBoundingClientRect(); - if (event.clientY >= rect.top && event.clientY < rect.bottom) { - if (lineSelection.focusLine !== i) { - lineSelection = { ...lineSelection, focusLine: i }; - } - break; - } + // Map the cursor to an absolute line index arithmetically rather than + // hit-testing rendered `.line` rects: the wrapper is transform-translated by + // -scrollY over a uniform lineHeight, and once the body is windowed the rows + // under the cursor may not be mounted. + const lineHeight = scrollController.getDimensions(side).lineHeight || 20; + const scrollY = + side === 'before' ? scrollController.beforeScrollY : scrollController.afterScrollY; + const total = (side === 'before' ? beforeLines.length : afterLines.length) - 1; + if (total < 0) return; + + const paneTop = pane.getBoundingClientRect().top; + const focusLine = Math.min( + Math.max(Math.floor((event.clientY - paneTop + scrollY) / lineHeight), 0), + total + ); + if (lineSelection.focusLine !== focusLine) { + lineSelection = { ...lineSelection, focusLine }; } } @@ -1521,8 +1534,7 @@ return; } - const lines = pane.querySelectorAll('.line'); - const lineEl = lines[selectedLineRange.start] as HTMLElement | null; + const lineEl = lineAt(pane, selectedLineRange.start); if (!lineEl) { lineSelectionToolbarStyle = null; lineSelectionToolbarLeft = null; @@ -1565,9 +1577,8 @@ const pane = commentingOnLines.pane === 'before' ? beforePane : afterPane; if (!pane) return 'below'; - const lines = pane.querySelectorAll('.line'); - const firstLineEl = lines[commentingOnLines.start] as HTMLElement | null; - const lastLineEl = lines[commentingOnLines.end] as HTMLElement | null; + const firstLineEl = lineAt(pane, commentingOnLines.start); + const lastLineEl = lineAt(pane, commentingOnLines.end); if (!firstLineEl || !lastLineEl) return 'below'; const paneRect = pane.getBoundingClientRect(); @@ -1599,13 +1610,13 @@ let anchorLineEl: HTMLElement | null; if (lineCommentPositionPreference === 'below') { - anchorLineEl = pane.querySelectorAll('.line')[commentingOnLines.end] as HTMLElement | null; + anchorLineEl = lineAt(pane, commentingOnLines.end); if (!anchorLineEl) { lineCommentEditorStyle = null; return; } } else { - anchorLineEl = pane.querySelectorAll('.line')[commentingOnLines.start] as HTMLElement | null; + anchorLineEl = lineAt(pane, commentingOnLines.start); if (!anchorLineEl) { lineCommentEditorStyle = null; return; @@ -1737,10 +1748,9 @@ if (!pane) return; const lines: string[] = []; - const lineElements = pane.querySelectorAll('.line'); for (let i = selectedLineRange.start; i <= selectedLineRange.end; i++) { - const lineEl = lineElements[i]; + const lineEl = lineAt(pane, i); if (lineEl) { const contentEl = lineEl.querySelector('.line-content'); if (contentEl) { @@ -1957,6 +1967,7 @@
{#each beforeLines as line, i} -
+
{#each getHighlightedTokens(i, 'before') as segment}
handleLineMouseDown('after', i, e)} > From 3e2f7950a67e666e52cd577386da51ec42c68820 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Fri, 5 Jun 2026 12:14:35 +1000 Subject: [PATCH 4/7] perf(diff-viewer): virtualize the diff body by rendering a window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 of virtualizing the diff body: render only the lines near the viewport instead of the entire file, building on Phase 1's index-by- identity refactor. Add per-pane window deriveds (beforeWindow/afterWindow) computed from the scroll offset, line height, and viewport height the scroll controller already owns: firstVisible = floor(scrollY / lineHeight), then a slice [firstVisible - OVERSCAN, firstVisible + visibleCount + OVERSCAN) clamped to the line count. computeWindow returns absolute indices, so iterating beforeWindow.indices keeps every i-consumer (highlighting, boundaries, class lookups, mouse handlers) unchanged — i is still the absolute line index. A keyed {#each ... (i)} lets Svelte reuse rows across scrolls. Position the slice under the existing -scrollY transform with a single top spacer (.line-spacer) sized windowStart * lineHeight; no bottom spacer is needed because the scrollbar height comes from the separate contentHeight derived, not the node count. To keep the window reactive on resize and font changes (not just scroll), track lineHeight and viewportHeight per pane as $state, updated wherever dimensions are already measured. OVERSCAN is 40 rows — generous enough to cover the comment editor's vertical extent near a viewport edge. Applied to all four line {#each} blocks: two-pane before/after, deleted- file before, and new-file after. scrollToRow, markers, and connector rendering are untouched (they read lengths/offsets, not nodes). Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- .../src/lib/components/DiffViewer.svelte | 95 ++++++++++++++++++- 1 file changed, 91 insertions(+), 4 deletions(-) diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index 7ae40f25d..a25361f13 100644 --- a/packages/diff-viewer/src/lib/components/DiffViewer.svelte +++ b/packages/diff-viewer/src/lib/components/DiffViewer.svelte @@ -431,6 +431,8 @@ if (beforePane && beforeLines.length > 0) { const lineHeight = measureLineHeight(beforePane); const contentWidth = measureContentWidth(beforePane); + beforeLineHeight = lineHeight || 20; + beforeViewportHeight = beforePane.clientHeight; scrollController.setDimensions('before', { viewportHeight: beforePane.clientHeight, contentHeight: beforeLines.length * lineHeight, @@ -445,6 +447,8 @@ if (afterPane && afterLines.length > 0) { const lineHeight = measureLineHeight(afterPane); const contentWidth = measureContentWidth(afterPane); + afterLineHeight = lineHeight || 20; + afterViewportHeight = afterPane.clientHeight; scrollController.setDimensions('after', { viewportHeight: afterPane.clientHeight, contentHeight: afterLines.length * lineHeight, @@ -473,14 +477,74 @@ let beforeContentHeight = $state(0); let afterContentHeight = $state(0); + // Reactive line/viewport metrics that drive the rendered window (Phase 2 + // virtualization). Kept in sync wherever dimensions are measured below so the + // window deriveds recompute on resize and font changes, not just on scroll. + let beforeLineHeight = $state(20); + let afterLineHeight = $state(20); + let beforeViewportHeight = $state(0); + let afterViewportHeight = $state(0); + // Content width needs to be measured after DOM renders, using state + effect let beforeContentWidth = $state(0); let afterContentWidth = $state(0); + // ========================================================================== + // Rendered window (Phase 2 virtualization) + // ========================================================================== + + /** + * Rows rendered above/below the viewport. Generous enough to cover the + * comment editor's vertical extent (it can render above a multi-line range), + * so anchor lookups near a viewport edge still resolve to a mounted row. + */ + const OVERSCAN = 40; + + /** + * Compute the absolute index range [start, end) to render for a pane given + * its scroll offset and metrics. Returns absolute indices so every existing + * `i`-consumer (highlighting, boundaries, class lookups) stays unchanged. + */ + function computeWindow( + total: number, + lineHeight: number, + scrollY: number, + viewportHeight: number + ): { start: number; indices: number[] } { + const lh = lineHeight || 20; + const firstVisible = Math.max(0, Math.floor(scrollY / lh)); + const visibleCount = Math.ceil((viewportHeight || 0) / lh); + const start = Math.max(0, firstVisible - OVERSCAN); + const end = Math.min(total, firstVisible + visibleCount + OVERSCAN); + const indices: number[] = []; + for (let i = start; i < end; i++) indices.push(i); + return { start, indices }; + } + + let beforeWindow = $derived( + computeWindow( + beforeLines.length, + beforeLineHeight, + scrollController.beforeScrollY, + beforeViewportHeight + ) + ); + + let afterWindow = $derived( + computeWindow( + afterLines.length, + afterLineHeight, + scrollController.afterScrollY, + afterViewportHeight + ) + ); + function updateContentWidths() { requestAnimationFrame(() => { if (beforePane) { const lh = measureLineHeight(beforePane) || 20; + beforeLineHeight = lh; + beforeViewportHeight = beforePane.clientHeight; beforeContentHeight = beforeLines.length * lh; beforeContentWidth = measureContentWidth(beforePane); scrollController.setDimensions('before', { @@ -493,6 +557,8 @@ } if (afterPane) { const lh = measureLineHeight(afterPane) || 20; + afterLineHeight = lh; + afterViewportHeight = afterPane.clientHeight; afterContentHeight = afterLines.length * lh; afterContentWidth = measureContentWidth(afterPane); scrollController.setDimensions('after', { @@ -1956,7 +2022,11 @@ class="lines-wrapper" style="transform: translate(-{scrollController.beforeScrollX}px, -{scrollController.beforeScrollY}px)" > - {#each beforeLines as line, i} +
+ {#each beforeWindow.indices as i (i)} {@const boundary = showRangeMarkers ? getLineBoundary(activeAlignments, 'before', i) : { isStart: false, isEnd: false }} @@ -2049,7 +2119,11 @@ class="lines-wrapper" style="transform: translate(-{scrollController.beforeScrollX}px, -{scrollController.beforeScrollY}px)" > - {#each beforeLines as line, i} +
+ {#each beforeWindow.indices as i (i)}
{#each getHighlightedTokens(i, 'before') as segment} @@ -2132,7 +2206,11 @@ class="lines-wrapper" style="transform: translate(-{scrollController.afterScrollX}px, -{scrollController.afterScrollY}px)" > - {#each afterLines as line, i} +
+ {#each afterWindow.indices as i (i)} {@const boundary = showRangeMarkers ? getLineBoundary(activeAlignments, 'after', i) : { isStart: false, isEnd: false }} @@ -2231,7 +2309,11 @@ class="lines-wrapper" style="transform: translate(-{scrollController.afterScrollX}px, -{scrollController.afterScrollY}px)" > - {#each afterLines as line, i} +
+ {#each afterWindow.indices as i (i)} {@const isSelected = isLineSelected('after', i)}
Date: Fri, 5 Jun 2026 12:18:16 +1000 Subject: [PATCH 5/7] =?UTF-8?q?perf(diff-viewer):=20sequence=20scroll=20?= =?UTF-8?q?=E2=86=92=20window-render=20=E2=86=92=20measure=20for=20comment?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 of virtualizing the diff body: make the comment editor and line- selection toolbar wait for the windowed body to render their anchor rows before measuring, instead of assuming the rows exist on the next frame. After Phase 2, scrolling to a comment sets a pane's scroll offset, which recomputes the window and renders the target rows — but only on a later frame. focusCommentInViewer's single requestAnimationFrame could fire before the anchor mounted, so lineAt() returned null and the editor silently never appeared. The same one-shot defer backed handleCommentHighlightClick's fallback and handleLineMouseUp's toolbar. Replace those one-shot rAFs with a bounded mount-then-measure retry: measureAfterWindowRender ticks a measure callback across up to WINDOW_RENDER_MAX_FRAMES (5) frames, stopping as soon as it reports success (anchor resolved and positioned) or there is nothing to position. A trackRaf callback lets a newer request cancel an in-flight wait, and both pending frame ids are cancelled on unmount. positionLineCommentEditor requires both the first and last range rows to be mounted before running, since decideLineCommentPosition measures space-above/space-below from both and the editor can render above the range. This also moves the position decision off the synchronous path in focusCommentInViewer and handleCommentHighlightClick, where it previously measured against unmounted rows and fell back to 'below'. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- .../src/lib/components/DiffViewer.svelte | 92 +++++++++++++++++-- 1 file changed, 82 insertions(+), 10 deletions(-) diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index a25361f13..0a2e24654 100644 --- a/packages/diff-viewer/src/lib/components/DiffViewer.svelte +++ b/packages/diff-viewer/src/lib/components/DiffViewer.svelte @@ -251,6 +251,7 @@ let lastHandledJumpLineToken = $state(null); let lastAutoScrolledFile: string | null = null; let lineCommentEditorRaf: number | null = null; + let lineSelectionToolbarRaf: number | null = null; // Markdown preview mode let markdownPreview = $state(false); @@ -1259,6 +1260,33 @@ return pane.querySelector(`.line[data-line-index="${index}"]`); } + // Cap on the number of frames a deferred measure waits for the windowed body + // to render the rows it needs. A scroll typically renders within one frame; + // the budget absorbs a slow render pass without spinning indefinitely. + const WINDOW_RENDER_MAX_FRAMES = 5; + + // Phase 3: scroll → window-render → measure sequencing. Setting a pane's scroll + // offset recomputes its window and Svelte renders the new rows, but only on a + // later frame — so an imperative measure on the *next* frame can still find the + // anchor row unmounted, leaving the editor/toolbar silently unpositioned. Retry + // `measure` across a bounded number of frames until it reports success: it + // returns true to stop (positioned, or nothing to do) and false to retry. + // `trackRaf` lets the caller hold the pending frame id so a newer request can + // cancel an in-flight wait. + function measureAfterWindowRender( + measure: () => boolean, + trackRaf: (raf: number | null) => void + ) { + let frames = 0; + const tick = () => { + trackRaf(null); + if (measure() || frames >= WINDOW_RENDER_MAX_FRAMES) return; + frames++; + trackRaf(requestAnimationFrame(tick)); + }; + trackRaf(requestAnimationFrame(tick)); + } + function updateToolbarPosition() { if (hoveredRangeIndex === null || !afterPane || !diffViewerEl) { rangeToolbarStyle = null; @@ -1332,18 +1360,43 @@ lineSelection = { pane: 'after', anchorLine: start, focusLine: end }; commentingOnLines = { pane: 'after', start, end }; - lineCommentPositionPreference = decideLineCommentPosition(); editingCommentId = comment.id; activeLineComment = comment; lineCommentReadOnly = comment.author === 'agent'; - // scrollToRow updates pane transforms; defer editor positioning until next frame + // scrollToRow updates pane transforms, but the windowed body only renders the + // target rows on a later frame — wait for the anchor to mount before deciding + // the editor's side and positioning it. + scheduleLineCommentEditorPositioning(); + } + + // Resolve the comment editor's side and screen position once the anchor rows are + // mounted. Returns true to stop the mount-then-measure retry (positioned, or + // nothing to position), false to retry on a later frame. + function positionLineCommentEditor(): boolean { + if (!commentingOnLines) return true; + + const pane = commentingOnLines.pane === 'before' ? beforePane : afterPane; + if (!pane) return true; + + // decideLineCommentPosition measures both the first and last range rows, and + // the editor anchors to one of them — require both mounted before measuring. + if (!lineAt(pane, commentingOnLines.start) || !lineAt(pane, commentingOnLines.end)) { + return false; + } + + lineCommentPositionPreference = decideLineCommentPosition(); + updateLineCommentEditorPosition(); + return true; + } + + function scheduleLineCommentEditorPositioning() { if (lineCommentEditorRaf !== null) { cancelAnimationFrame(lineCommentEditorRaf); - } - lineCommentEditorRaf = requestAnimationFrame(() => { lineCommentEditorRaf = null; - updateLineCommentEditorPosition(); + } + measureAfterWindowRender(positionLineCommentEditor, (raf) => { + lineCommentEditorRaf = raf; }); } @@ -1372,11 +1425,10 @@ lineSelection = { pane: 'after', anchorLine: start, focusLine: end }; commentingOnLines = { pane: 'after', start, end }; - lineCommentPositionPreference = decideLineCommentPosition(); editingCommentId = commentId; activeLineComment = comment; lineCommentReadOnly = false; - updateLineCommentEditorPosition(); + scheduleLineCommentEditorPositioning(); } // Jump to a comment requested by the sidebar comments list. @@ -1560,12 +1612,28 @@ document.removeEventListener('mousemove', handleSelectionDragMove); if (lineSelection) { - requestAnimationFrame(() => { - updateLineSelectionToolbar(true); - }); + scheduleLineSelectionToolbar(true); } } + // Position the line-selection toolbar once the selection's anchor row is + // mounted. A drag that ends after auto-scrolling can leave the anchor outside + // the freshly rendered window for a frame, so wait for it like the editor. + function scheduleLineSelectionToolbar(recalculateLeft = false) { + measureAfterWindowRender( + () => { + if (!selectedLineRange) return true; + const pane = selectedLineRange.pane === 'before' ? beforePane : afterPane; + if (pane && !lineAt(pane, selectedLineRange.start)) return false; + updateLineSelectionToolbar(recalculateLeft); + return true; + }, + (raf) => { + lineSelectionToolbarRaf = raf; + } + ); + } + function clearLineSelection() { lineSelection = null; isSelecting = false; @@ -1905,6 +1973,10 @@ cancelAnimationFrame(lineCommentEditorRaf); lineCommentEditorRaf = null; } + if (lineSelectionToolbarRaf !== null) { + cancelAnimationFrame(lineSelectionToolbarRaf); + lineSelectionToolbarRaf = null; + } if (connectorRenderer) { connectorRenderer.destroy(); connectorRenderer = null; From 26e62870f291d22e71aec88ddf716d49a53930c0 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Tue, 9 Jun 2026 17:24:16 +1000 Subject: [PATCH 6/7] fix(diff-viewer): correct three windowing edge cases for tall ranges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diff-body windowing (b45c465 → 673271b) renders only the rows near the viewport, but three spots still assumed every row was in the DOM. Two were real bugs for ranges taller than the window (~OVERSCAN + viewport); one was dead CSS. Range copy: handleCopy's selectedLineRange branch read line text from the DOM via lineAt, so any selected line outside the rendered slice resolved to null and was silently dropped — a drag-selection spanning more than the window copied only a partial range. Reconstruct the copied text from the beforeLines/afterLines source arrays by index instead (absolute line index maps 1:1 to the source-line array). The native-selection branch stays DOM-bound, as a browser selection can't extend to unrendered rows anyway. Comment editor: positionLineCommentEditor required both the first and last range rows mounted before measuring, because decideLineCommentPosition reads space-above from the first row and space-below from the last. For a range taller than the window the two ends can never co-mount, so it spun out WINDOW_RENDER_MAX_FRAMES and the editor silently never appeared. Now both-ends-mounted keeps the full space-based decision (short ranges), and the tall-range case anchors to whichever end is rendered — callers scroll to start first, so it lands in the window — passing the resolved row to updateLineCommentEditorPosition so the anchor isn't re-derived from the preference against an unmounted row. CSS: drop the inert flex-shrink: 0 on .line-spacer; its wrapper is display: block so it never applied, and the inline height already reserves the unrendered-rows space. Typecheck passes. Signed-off-by: Matt Toohey --- .../src/lib/components/DiffViewer.svelte | 61 ++++++++++++------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index 0a2e24654..5d60b2da4 100644 --- a/packages/diff-viewer/src/lib/components/DiffViewer.svelte +++ b/packages/diff-viewer/src/lib/components/DiffViewer.svelte @@ -1379,15 +1379,35 @@ const pane = commentingOnLines.pane === 'before' ? beforePane : afterPane; if (!pane) return true; - // decideLineCommentPosition measures both the first and last range rows, and - // the editor anchors to one of them — require both mounted before measuring. - if (!lineAt(pane, commentingOnLines.start) || !lineAt(pane, commentingOnLines.end)) { - return false; + const firstEl = lineAt(pane, commentingOnLines.start); + const lastEl = lineAt(pane, commentingOnLines.end); + + // Both ends mounted: full space-based decision (short ranges). + if (firstEl && lastEl) { + lineCommentPositionPreference = decideLineCommentPosition(); + updateLineCommentEditorPosition(); + return true; } - lineCommentPositionPreference = decideLineCommentPosition(); - updateLineCommentEditorPosition(); - return true; + // Range taller than the window — both ends can't co-mount in one frame, so + // decideLineCommentPosition (which measures both) can never run. Anchor to + // whichever end is rendered. Callers scroll to `start` first, so it lands in + // the window; position the editor below it (there's space below after the + // scroll). Pass the resolved row explicitly so the anchor doesn't have to be + // re-derived from the preference against an unmounted row. + if (firstEl) { + lineCommentPositionPreference = 'below'; + updateLineCommentEditorPosition(firstEl); + return true; + } + if (lastEl) { + lineCommentPositionPreference = 'above'; + updateLineCommentEditorPosition(lastEl); + return true; + } + + // Neither end mounted yet: keep retrying for the window render. + return false; } function scheduleLineCommentEditorPositioning() { @@ -1726,7 +1746,7 @@ return decideCommentPositionBySpace(spaceBelow, spaceAbove, editorHeight); } - function updateLineCommentEditorPosition() { + function updateLineCommentEditorPosition(anchorOverride?: HTMLElement) { if (!commentingOnLines || !diffViewerEl) { lineCommentEditorStyle = null; return; @@ -1743,7 +1763,11 @@ const editorHeight = 120; let anchorLineEl: HTMLElement | null; - if (lineCommentPositionPreference === 'below') { + if (anchorOverride) { + // Tall-range path: the caller resolved the only mounted end; use it directly + // rather than re-deriving from the preference against an unmounted row. + anchorLineEl = anchorOverride; + } else if (lineCommentPositionPreference === 'below') { anchorLineEl = lineAt(pane, commentingOnLines.end); if (!anchorLineEl) { lineCommentEditorStyle = null; @@ -1878,18 +1902,15 @@ if (selectedLineRange) { event.preventDefault(); - const pane = selectedLineRange.pane === 'before' ? beforePane : afterPane; - if (!pane) return; - + // Reconstruct from the source-line array by index rather than the DOM: + // with the body windowed, rows outside the rendered slice aren't mounted, + // so a DOM-based read would silently drop lines from a tall selection. + const sourceLines = selectedLineRange.pane === 'before' ? beforeLines : afterLines; const lines: string[] = []; for (let i = selectedLineRange.start; i <= selectedLineRange.end; i++) { - const lineEl = lineAt(pane, i); - if (lineEl) { - const contentEl = lineEl.querySelector('.line-content'); - if (contentEl) { - lines.push(contentEl.textContent || ''); - } + if (i >= 0 && i < sourceLines.length) { + lines.push(sourceLines[i]); } } @@ -2928,10 +2949,6 @@ } /* Top spacer reserving the height of unrendered rows above the window. */ - .line-spacer { - flex-shrink: 0; - } - .line-content { flex: 1; padding: 0 12px; From 00c59310944048e6743fc139bc45c8b98dd967a2 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 10 Jun 2026 11:49:18 +1000 Subject: [PATCH 7/7] fix(diff-viewer): measure content width from source, not the rendered window MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diff-body virtualization (b45c465 → 26e6287) renders only the rows near the viewport, but measureContentWidth still read the widest line from the DOM via .lines-wrapper's scrollWidth. With most rows unmounted, that saw only the widest *rendered* line, so a file whose longest line sat outside the window clamped horizontal scrolling too narrow — long offscreen lines couldn't be panned into view, and the horizontal scrollbar thumb was mis-sized. Because updateContentWidths runs on lines change / resize / style mutation but not on scroll, scrolling the long line into the window didn't fix it either; the under-clamp persisted until an unrelated resize or font change. Measure from the source text instead. measureContentWidth now takes the pane's source lines, picks the widest by display-column count (tabs expanded to 8-column stops, iterated by code point), and measures just that one line in a hidden offscreen probe span appended to the pane so it inherits the live computed font and white-space: pre. Add the 24px .line-content horizontal padding and floor at pane.clientWidth to keep the .lines-wrapper min-width: 100% behavior. One probe node keeps the virtualization intact while the result reflects the whole file at any scroll position. Applied to all four measurement sites (before/after dimension effects and updateContentWidths) by passing beforeLines/afterLines. Empty panes still return 0. Typecheck passes. Co-Authored-By: Claude Opus 4.8 (1M context) Signed-off-by: Matt Toohey --- .../src/lib/components/DiffViewer.svelte | 8 +-- .../src/lib/utils/diffViewerHelpers.ts | 64 ++++++++++++++++++- 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/packages/diff-viewer/src/lib/components/DiffViewer.svelte b/packages/diff-viewer/src/lib/components/DiffViewer.svelte index 5d60b2da4..f895ae780 100644 --- a/packages/diff-viewer/src/lib/components/DiffViewer.svelte +++ b/packages/diff-viewer/src/lib/components/DiffViewer.svelte @@ -431,7 +431,7 @@ $effect(() => { if (beforePane && beforeLines.length > 0) { const lineHeight = measureLineHeight(beforePane); - const contentWidth = measureContentWidth(beforePane); + const contentWidth = measureContentWidth(beforeLines, beforePane); beforeLineHeight = lineHeight || 20; beforeViewportHeight = beforePane.clientHeight; scrollController.setDimensions('before', { @@ -447,7 +447,7 @@ $effect(() => { if (afterPane && afterLines.length > 0) { const lineHeight = measureLineHeight(afterPane); - const contentWidth = measureContentWidth(afterPane); + const contentWidth = measureContentWidth(afterLines, afterPane); afterLineHeight = lineHeight || 20; afterViewportHeight = afterPane.clientHeight; scrollController.setDimensions('after', { @@ -547,7 +547,7 @@ beforeLineHeight = lh; beforeViewportHeight = beforePane.clientHeight; beforeContentHeight = beforeLines.length * lh; - beforeContentWidth = measureContentWidth(beforePane); + beforeContentWidth = measureContentWidth(beforeLines, beforePane); scrollController.setDimensions('before', { viewportHeight: beforePane.clientHeight, contentHeight: beforeContentHeight, @@ -561,7 +561,7 @@ afterLineHeight = lh; afterViewportHeight = afterPane.clientHeight; afterContentHeight = afterLines.length * lh; - afterContentWidth = measureContentWidth(afterPane); + afterContentWidth = measureContentWidth(afterLines, afterPane); scrollController.setDimensions('after', { viewportHeight: afterPane.clientHeight, contentHeight: afterContentHeight, diff --git a/packages/diff-viewer/src/lib/utils/diffViewerHelpers.ts b/packages/diff-viewer/src/lib/utils/diffViewerHelpers.ts index 05f50cf8a..17ea262ad 100644 --- a/packages/diff-viewer/src/lib/utils/diffViewerHelpers.ts +++ b/packages/diff-viewer/src/lib/utils/diffViewerHelpers.ts @@ -205,10 +205,68 @@ export function measureLineHeight(pane: HTMLElement | null): number { return firstLine ? firstLine.getBoundingClientRect().height : 20; } -export function measureContentWidth(pane: HTMLElement | null): number { +/** Horizontal padding on `.line-content` (`padding: 0 12px`), in pixels. */ +const LINE_CONTENT_PADDING_X = 24; + +/** + * Display-column count of a line, expanding tabs to the next 8-column tab stop + * (the CSS sets no `tab-size`, so the default 8 applies). Iterates by code + * point so astral characters count as one. Under a monospace font this is a + * faithful proxy for which line is widest in pixels. + */ +function displayColumns(line: string): number { + let columns = 0; + for (const ch of line) { + if (ch === '\t') columns += 8 - (columns % 8); + else columns += 1; + } + return columns; +} + +/** + * Width of the widest line in pixels, measured from the source text rather than + * the rendered DOM. + * + * The diff body is virtualized — only a window of lines is mounted — so reading + * `.lines-wrapper`'s `scrollWidth` only sees the widest *rendered* line and + * under-clamps horizontal scrolling when the file's longest line is offscreen. + * Instead, pick the widest line by display-column count and measure just that + * one line in an offscreen probe span that inherits `.line-content`'s font and + * `white-space: pre`. One node, so virtualization is preserved, and the result + * reflects the whole file regardless of scroll position. + * + * Returns at least `pane.clientWidth` to keep the `.lines-wrapper` + * `min-width: 100%` floor. + */ +export function measureContentWidth(lines: string[], pane: HTMLElement | null): number { if (!pane) return 0; - const linesWrapper = pane.querySelector('.lines-wrapper') as HTMLElement | null; - return linesWrapper ? linesWrapper.scrollWidth : 0; + if (lines.length === 0) return 0; + + let widest = ''; + let maxColumns = -1; + for (const line of lines) { + const columns = displayColumns(line); + if (columns > maxColumns) { + maxColumns = columns; + widest = line; + } + } + + // Probe inside the pane so it inherits the live computed font (font-family, + // size, and any --code-font-size override), matching the rendered lines. + const probe = document.createElement('span'); + probe.textContent = widest; + probe.style.position = 'absolute'; + probe.style.top = '0'; + probe.style.left = '0'; + probe.style.visibility = 'hidden'; + probe.style.whiteSpace = 'pre'; + probe.style.pointerEvents = 'none'; + pane.appendChild(probe); + const textWidth = probe.getBoundingClientRect().width; + pane.removeChild(probe); + + return Math.max(Math.ceil(textWidth) + LINE_CONTENT_PADDING_X, pane.clientWidth); } export function getTokensForLine(tokens: Token[][], index: number): Token[] {