From 120fd69eccee70fc5713662821b3ae23a4314643 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Wed, 10 Jun 2026 16:30:46 +1000 Subject: [PATCH] refactor(diff-viewer): remove debug/timing logs added in #773 The virtualization perf work in #773 also introduced a layer of debug/timing instrumentation to measure it. Remove only that logging (and the scaffolding that existed solely to feed it), restoring the affected code paths to their pre-#773 behavior while keeping the virtualization feature fully intact. - diff_commands.rs: drop the 8 cache-hit / cache-miss / populate-timing log::info! calls and the two t_populate Instant bindings; the pre-existing `start` timer and its surrounding logs are untouched. - BranchCard.svelte: remove the markDiffOpenClick import and all call sites; restore onViewWorktreeDiff to its single-line form. - DiffModal.svelte: remove the render-tail timing $effect. - diffViewerState.svelte.ts: remove lastOpenClickAt / markDiffOpenClick, the click-to-open and per-call timing console logs, and restore the original "Failed to load diff" error log. cargo check passes with no warnings; the four touched files now match their pre-#773 state exactly and markDiffOpenClick has no remaining importers repo-wide. Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/diff_commands.rs | 36 ------------ .../lib/features/branches/BranchCard.svelte | 12 +--- .../src/lib/features/diff/DiffModal.svelte | 22 -------- .../features/diff/diffViewerState.svelte.ts | 55 +------------------ 4 files changed, 4 insertions(+), 121 deletions(-) diff --git a/apps/staged/src-tauri/src/diff_commands.rs b/apps/staged/src-tauri/src/diff_commands.rs index 72948868d..a0ee7b088 100644 --- a/apps/staged/src-tauri/src/diff_commands.rs +++ b/apps/staged/src-tauri/src/diff_commands.rs @@ -548,12 +548,6 @@ 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, @@ -570,12 +564,6 @@ 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, @@ -584,17 +572,8 @@ 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")?; @@ -683,10 +662,6 @@ 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); } } @@ -700,23 +675,12 @@ 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/branches/BranchCard.svelte b/apps/staged/src/lib/features/branches/BranchCard.svelte index ba2bc3d9d..98fcb5651 100644 --- a/apps/staged/src/lib/features/branches/BranchCard.svelte +++ b/apps/staged/src/lib/features/branches/BranchCard.svelte @@ -39,7 +39,6 @@ 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'; @@ -846,7 +845,6 @@ } function handleCommitClick(sha: string) { - markDiffOpenClick(); commitDiffSha = sha; } @@ -865,7 +863,6 @@ const cached = timelineReviewDetailsById[reviewId]; if (cached) { reviewDiffTarget = { commitSha: cached.commitSha, scope: cached.scope, reviewId }; - markDiffOpenClick(); showBranchDiff = true; return; } @@ -878,7 +875,6 @@ return; } reviewDiffTarget = { commitSha: review.commitSha, scope: review.scope, reviewId }; - markDiffOpenClick(); showBranchDiff = true; } catch (e) { console.error('Failed to open review:', e); @@ -1462,12 +1458,7 @@ : undefined} {forcePushingOrigin} rebaseBranchDisabledReason={branchCommandDisabledReason} - onViewWorktreeDiff={isLocal - ? () => { - markDiffOpenClick(); - showWorktreeDiff = true; - } - : undefined} + onViewWorktreeDiff={isLocal ? () => (showWorktreeDiff = true) : undefined} onCommitWorktreeChanges={() => sessionMgr.startOrQueueSession('commit', 'Commit uncommitted changes')} onDiscardWorktreeChanges={handleDiscardWorktreeChanges} @@ -1499,7 +1490,6 @@ 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 7ab11b387..78406c643 100644 --- a/apps/staged/src/lib/features/diff/DiffModal.svelte +++ b/apps/staged/src/lib/features/diff/DiffModal.svelte @@ -645,28 +645,6 @@ // ========================================================================== 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 17a71a288..16df8640f 100644 --- a/apps/staged/src/lib/features/diff/diffViewerState.svelte.ts +++ b/apps/staged/src/lib/features/diff/diffViewerState.svelte.ts @@ -19,19 +19,6 @@ 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. */ @@ -51,41 +38,20 @@ 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)'}${clickToOpen}` - ); - 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) { - console.info( - `[diff] loadFiles stale (took ${Math.round(performance.now() - t0)}ms) — ignoring` - ); - return; - } + if (generation !== contextGeneration) 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])); @@ -94,9 +60,6 @@ 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; @@ -118,29 +81,18 @@ export function createDiffViewerState(branchId: string, scope: DiffScope, commit if (!state.commitSha) return null; const cached = state.diffCache.get(path); - if (cached) { - console.info(`[diff] loadFileDiff cache hit: path=${path}`); - return cached; - } + if (cached) 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( - `[diff] loadFileDiff failed in ${Math.round(performance.now() - t0)}ms: path=${path}:`, - e - ); + console.error(`Failed to load diff for ${path}:`, e); return null; } finally { state.loadingFile = null; @@ -157,7 +109,6 @@ 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();