From a601aae1a7de41ad4b67c729deb9646c6490abb2 Mon Sep 17 00:00:00 2001 From: Pierre-Henri Toussaint Date: Tue, 12 May 2026 15:14:18 +0200 Subject: [PATCH 1/4] perf(syntax): stream syntax highlighting on a background worker The parse phase now emits highlight jobs instead of highlighting inline. A background worker drains them in parallel and streams results back to the main thread via an mpsc channel; the main loop patches spans into diff_files in place between frames. Effect: the file list and diff structure appear after the parse cost alone, instead of after parse + full synchronous highlight. Total work is unchanged (slightly more, from thread + channel overhead) but the visible-frame wait shrinks on big diffs. Notes worth recording: * git2 / hg / jj state is mostly !Send. Container-grammar full-file content is fetched on the main thread during parse and embedded into jobs as owned Strings; the worker never touches VCS state. * Workers use thread::scope + an atomic-cursor pulled from a shared Vec. Beats static chunking when one giant file sits alongside many small ones. * Per-hunk jobs and full-file (Vue/Svelte/...) jobs share a single HighlightJob enum and a single dispatch path. * HighlightSession bundles the receiver and a cancel AtomicBool. On diff swap (commit reselection, :e reload), the old session is dropped and its cancel flag set; the worker exits at its next channel send or cancel check. * Adaptive event-poll cadence: 30ms while a session is live so streamed spans land within a frame, 100ms idle to keep wakeups low. Tests use a synchronous streaming::run_blocking helper so spans are present deterministically; production code path is the worker. --- src/app.rs | 541 ++++++++++++++++-------------------- src/forge/pr_open.rs | 38 ++- src/main.rs | 21 +- src/syntax/mod.rs | 3 + src/syntax/streaming.rs | 334 ++++++++++++++++++++++ src/theme/mod.rs | 28 +- src/tuicrignore.rs | 43 +-- src/ui/diff_side_by_side.rs | 10 +- src/ui/diff_unified.rs | 12 +- src/ui/selector.rs | 12 +- src/ui/status_bar.rs | 12 +- src/ui/submit_modals.rs | 9 +- src/vcs/diff_parser.rs | 211 +++++++------- src/vcs/file.rs | 112 ++++---- src/vcs/git/cli.rs | 211 +++++++------- src/vcs/git/diff.rs | 218 +++++++-------- src/vcs/git/libgit2.rs | 41 +-- src/vcs/git/mod.rs | 46 ++- src/vcs/hg/mod.rs | 107 +++---- src/vcs/jj/mod.rs | 116 ++++---- src/vcs/mod.rs | 322 +++++---------------- src/vcs/pr_noop.rs | 11 +- src/vcs/traits.rs | 25 +- 23 files changed, 1228 insertions(+), 1255 deletions(-) create mode 100644 src/syntax/streaming.rs diff --git a/src/app.rs b/src/app.rs index 352933f0..5ba57489 100644 --- a/src/app.rs +++ b/src/app.rs @@ -1,5 +1,7 @@ use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::sync::mpsc::TryRecvError; use std::time::{Duration, Instant, SystemTime}; use chrono::Utc; @@ -17,14 +19,15 @@ use crate::model::{ }; use crate::persistence::load_latest_session_for_context; use crate::review_store::{AddCommentRequest, CommentTarget, add_comment_to_session}; -use crate::syntax::SyntaxHighlighter; +use crate::syntax::streaming::{self, HighlightJob, HighlightSession}; use crate::theme::Theme; use crate::update::UpdateInfo; use crate::vcs::git::calculate_gap; use crate::vcs::traits::VcsType; use crate::vcs::{ - ChangeKind, CommitInfo, DiffWhitespaceMode, FileBackend, GitBackendPreference, PrNoopVcs, - ResolvedRevisionRange, RevisionDiffTarget, VcsBackend, VcsChangeStatus, VcsInfo, detect_vcs, + ChangeKind, CommitInfo, DiffWhitespaceMode, DiffWithJobs, FileBackend, GitBackendPreference, + PrNoopVcs, ResolvedRevisionRange, RevisionDiffTarget, VcsBackend, VcsChangeStatus, VcsInfo, + detect_vcs, }; const VISIBLE_COMMIT_COUNT: usize = 10; @@ -91,9 +94,9 @@ fn gap_annotation_line_count( } } -fn profile_diff_result(result: &Result>) -> String { +fn profile_diff_result(result: &Result) -> String { match result { - Ok(files) => format!("files={}", files.len()), + Ok((files, _)) => format!("files={}", files.len()), Err(e) => format!("error={e}"), } } @@ -924,6 +927,18 @@ pub struct App { pub diff_source: DiffSource, pub pending_editor_target: Option, + /// Background syntax-highlight worker. `None` once the current diff is + /// fully highlighted (or had nothing to highlight). Replacing the diff + /// cancels the old session so its in-flight work is discarded. + pub(crate) highlight_session: Option, + + /// Jobs handed to `install_diff` but not yet given to a worker. Holding + /// them here lets follow-up `diff_files` reorderings (commit-message + /// insert, directory sort) settle before the worker starts emitting + /// updates keyed by `file_idx`; the first `drain_highlight_updates` call + /// resolves indices by `syntax_path` and spawns the worker. + pub(crate) pending_highlight_jobs: Vec, + pub input_mode: InputMode, pub focused_panel: FocusedPanel, pub diff_view_mode: DiffViewMode, @@ -1384,8 +1399,7 @@ impl App { if let Some(file_path) = options.file_path { let vcs = Box::new(FileBackend::new(file_path)?); let vcs_info = vcs.info().clone(); - let highlighter = theme.syntax_highlighter(); - let diff_files = vcs.get_working_tree_diff(highlighter)?; + let diff = vcs.get_working_tree_diff()?; let session = Self::load_or_create_session(&vcs_info, SessionDiffSource::WorkingTree); let mut app = Self::build( @@ -1394,7 +1408,7 @@ impl App { theme, comment_type_configs, output_to_stdout, - diff_files, + diff, session, DiffSource::WorkingTree, InputMode::Normal, @@ -1435,14 +1449,13 @@ impl App { let vcs = Box::new(FileBackend::new_pristine(paths, cwd.clone())?); let mut vcs_info = vcs.info().clone(); vcs_info.head_commit = base_commit; - let highlighter = theme.syntax_highlighter(); - let diff_files = vcs.get_working_tree_diff(highlighter)?; + let diff = vcs.get_working_tree_diff()?; // `git ls-files` already honors `.gitignore`, but `.tuicrignore` // is tuicr-specific and not known to git. Run the same post-VCS // filter every other mode uses so users can elide tracked-but- // boring files (lockfiles, generated docs) from the review surface. - let diff_files = Self::filter_ignored_diff_files(&cwd, diff_files); - if diff_files.is_empty() { + let diff = Self::filter_ignored_diff(&cwd, diff); + if diff.0.is_empty() { return Err(TuicrError::NoChanges); } let session = Self::load_or_create_session(&vcs_info, SessionDiffSource::Pristine); @@ -1453,7 +1466,7 @@ impl App { theme, comment_type_configs, output_to_stdout, - diff_files, + diff, session, DiffSource::WorkingTree, InputMode::Normal, @@ -1485,8 +1498,6 @@ impl App { detect_vcs(options.git_backend_preference, options.diff_whitespace_mode) })?; let vcs_info = vcs.info().clone(); - let highlighter = - crate::profile::time("startup.syntax_highlighter", || theme.syntax_highlighter()); // Determine the diff source, files, and session based on input. // Four paths: // 1. -r + -w: combined commit range and uncommitted changes @@ -1506,11 +1517,10 @@ impl App { if options.working_tree { // Combined: commit range + staged/unstaged changes - let diff_files = Self::get_working_tree_with_commits_diff_with_ignore( + let diff = Self::get_working_tree_with_commits_diff_with_ignore( vcs.as_ref(), &vcs_info.root_path, &commit_ids, - highlighter, options.path_filter, )?; let session = Self::load_or_create_staged_unstaged_and_commits_session( @@ -1529,7 +1539,6 @@ impl App { let change_status = Self::get_change_status_with_ignore( vcs.as_ref(), &vcs_info.root_path, - highlighter, options.path_filter, )?; let mut all_commits = Vec::new(); @@ -1547,7 +1556,7 @@ impl App { theme, comment_type_configs.clone(), output_to_stdout, - diff_files, + diff, session, DiffSource::StagedUnstagedAndCommits(commit_ids), InputMode::Normal, @@ -1579,11 +1588,10 @@ impl App { } // Resolve the revisions to commits and diff as a commit range - let diff_files = Self::get_commit_range_diff_with_ignore( + let diff = Self::get_commit_range_diff_with_ignore( vcs.as_ref(), &vcs_info.root_path, &revision_range, - highlighter, options.path_filter, )?; let session = Self::load_or_create_commit_range_session(&vcs_info, &commit_ids); @@ -1602,7 +1610,7 @@ impl App { theme, comment_type_configs.clone(), output_to_stdout, - diff_files, + diff, session, DiffSource::CommitRange(commit_ids), InputMode::Normal, @@ -1632,10 +1640,9 @@ impl App { Ok(app) } else if options.working_tree { // Skip commit selector, go straight to working tree diff - let diff_files = Self::get_working_tree_diff_with_ignore( + let diff = Self::get_working_tree_diff_with_ignore( vcs.as_ref(), &vcs_info.root_path, - highlighter, options.path_filter, )?; let session = @@ -1647,7 +1654,7 @@ impl App { theme, comment_type_configs, output_to_stdout, - diff_files, + diff, session, DiffSource::StagedAndUnstaged, InputMode::Normal, @@ -1661,7 +1668,6 @@ impl App { let change_status = Self::get_change_status_with_ignore( vcs.as_ref(), &vcs_info.root_path, - highlighter, options.path_filter, )?; let has_staged_changes = change_status.staged; @@ -1717,7 +1723,7 @@ impl App { theme, comment_type_configs, output_to_stdout, - Vec::new(), + (Vec::new(), Vec::new()), session, diff_source, InputMode::CommitSelect, @@ -1744,7 +1750,7 @@ impl App { theme: Theme, comment_type_configs: Option>, output_to_stdout: bool, - diff_files: Vec, + diff: DiffWithJobs, mut session: ReviewSession, diff_source: DiffSource, input_mode: InputMode, @@ -1757,7 +1763,7 @@ impl App { // hunk keys alive until the selected diff is loaded. let preserve_hunks = matches!(diff_source, DiffSource::PullRequest(_)) && session.commit_selection_range.is_some(); - Self::register_diff_files(&mut session, &diff_files, preserve_hunks); + Self::register_diff_files(&mut session, &diff.0, preserve_hunks); let has_more_commit = commit_list.len() >= VISIBLE_COMMIT_COUNT; let visible_commit_count = if commit_list.is_empty() { @@ -1787,9 +1793,11 @@ impl App { next_review_watch_at: Instant::now() + Duration::from_millis(DEFAULT_REVIEW_WATCH_INTERVAL_MS), ephemeral_session_paths: HashSet::new(), - diff_files, + diff_files: Vec::new(), diff_source, pending_editor_target: None, + highlight_session: None, + pending_highlight_jobs: Vec::new(), input_mode, focused_panel: FocusedPanel::Diff, diff_view_mode: DiffViewMode::Unified, @@ -1892,6 +1900,7 @@ impl App { export_legend: true, }; // Auto-hide file list when path filter matches exactly one file + app.install_diff(diff); if app.path_filter.is_some() && app.diff_files.len() == 1 { app.show_file_list = false; app.focused_panel = FocusedPanel::Diff; @@ -2555,7 +2564,7 @@ impl App { persisted.commit_selection_range, opened.commits.len(), ); - Self::register_diff_files(&mut persisted, &opened.diff_files, preserve_hunks); + Self::register_diff_files(&mut persisted, &opened.diff.0, preserve_hunks); persisted.pr_session_key = Some(key); persisted.diff_source = SessionDiffSource::PullRequest; persisted.updated_at = chrono::Utc::now(); @@ -2655,12 +2664,10 @@ impl App { }); let backend = create_forge_backend(&target_repo, local_checkout_for_target.clone()); - let highlighter = theme.syntax_highlighter(); let mut opened = open_pull_request( backend.as_ref(), parsed, local_checkout_for_target.as_deref(), - highlighter, )?; Self::load_or_apply_pr_session(&mut opened); @@ -2686,7 +2693,7 @@ impl App { theme, comment_type_configs, output_to_stdout, - opened.diff_files, + opened.diff, opened.session, diff_source, InputMode::Normal, @@ -2724,7 +2731,7 @@ impl App { ) -> Result<()> { let crate::forge::pr_open::OpenedPullRequest { details, - diff_files, + diff, session, key, commits, @@ -2746,7 +2753,7 @@ impl App { }; self.vcs = Box::new(PrNoopVcs::new(self.vcs_info.clone())); self.session = session; - self.diff_files = diff_files; + self.install_diff(diff); self.reset_persisted_session_tracking(); self.diff_source = DiffSource::PullRequest(Box::new(pr_source)); self.forge_backend = Some(backend); @@ -3135,10 +3142,9 @@ impl App { ) -> Result<()> { use crate::vcs::diff_parser::{DiffFormat, parse_unified_diff}; - let highlighter = self.theme.syntax_highlighter(); - let parsed = match parse_unified_diff(patch, DiffFormat::GitStyle, highlighter) { - Ok(files) => files, - Err(TuicrError::NoChanges) => Vec::new(), + let parsed = match parse_unified_diff(patch, DiffFormat::GitStyle) { + Ok(diff) => diff, + Err(TuicrError::NoChanges) => (Vec::new(), Vec::new()), Err(e) => return Err(e), }; @@ -3146,12 +3152,12 @@ impl App { .forge_backend .as_deref() .and_then(|b| b.local_checkout_path()); - let files = match local_checkout.as_deref() { + let diff = match local_checkout.as_deref() { Some(root) => crate::tuicrignore::filter_diff_files(root, parsed), None => parsed, }; - self.diff_files = files; + self.install_diff(diff); self.clear_expanded_gaps(); // Range diffs can hide hunks that are still reviewed in the broader // PR session, so registration must not prune them. @@ -3261,14 +3267,7 @@ impl App { .forge_backend .as_deref() .and_then(|backend| backend.local_checkout_path()); - let highlighter = self.theme.syntax_highlighter(); - let mut opened = prepare_open_pr( - details, - &patch, - commits, - local_checkout.as_deref(), - highlighter, - )?; + let mut opened = prepare_open_pr(details, &patch, commits, local_checkout.as_deref())?; let head_changed = opened.details.head_sha != request.head_sha; if head_changed { @@ -3280,7 +3279,7 @@ impl App { self.spawn_pr_threads_fetch(&details_for_threads, local_checkout); self.set_message("Reloaded PR at new head — switched to fresh session".to_string()); } else { - self.diff_files = opened.diff_files; + self.install_diff(opened.diff); self.clear_expanded_gaps(); for file in &self.diff_files { self.session.add_diff_file(file); @@ -3338,13 +3337,7 @@ impl App { current.key.number, current.key.number.to_string(), ); - let highlighter = self.theme.syntax_highlighter(); - let mut opened = open_pull_request( - backend.as_ref(), - target, - local_checkout.as_deref(), - highlighter, - )?; + let mut opened = open_pull_request(backend.as_ref(), target, local_checkout.as_deref())?; let head_changed = opened.details.head_sha != current.key.head_sha; if head_changed { @@ -3359,7 +3352,7 @@ impl App { } else { // Same head: re-parse the diff to pick up any side-channel // changes (rare), but keep the session intact. - self.diff_files = opened.diff_files; + self.install_diff(opened.diff); self.clear_expanded_gaps(); for file in &self.diff_files { self.session.add_diff_file(file); @@ -3484,136 +3477,163 @@ impl App { .saturating_sub(self.special_commit_count()) } - fn filter_ignored_diff_files(repo_root: &Path, diff_files: Vec) -> Vec { - crate::tuicrignore::filter_diff_files(repo_root, diff_files) + fn filter_ignored_diff(repo_root: &Path, diff: DiffWithJobs) -> DiffWithJobs { + crate::tuicrignore::filter_diff_files(repo_root, diff) } - fn filter_by_path(diff_files: Vec, path: &str) -> Vec { - let path = path.trim_end_matches('/'); - diff_files - .into_iter() - .filter(|f| { - let display = f.display_path().to_string_lossy(); - display == path || display.starts_with(&format!("{path}/")) - }) - .collect() + fn filter_by_path(diff: DiffWithJobs, path: &str) -> DiffWithJobs { + let path = path.trim_end_matches('/').to_string(); + crate::vcs::filter_diff_with_jobs(diff, |f| { + let display = f.display_path().to_string_lossy(); + display == path || display.starts_with(&format!("{path}/")) + }) } - fn require_non_empty_diff_files(diff_files: Vec) -> Result> { - if diff_files.is_empty() { + fn finalize_diff( + diff: DiffWithJobs, + repo_root: &Path, + path_filter: Option<&str>, + ) -> Result { + let diff = Self::filter_ignored_diff(repo_root, diff); + let diff = match path_filter { + Some(path) => Self::filter_by_path(diff, path), + None => diff, + }; + if diff.0.is_empty() { return Err(TuicrError::NoChanges); } - Ok(diff_files) + Ok(diff) + } + + /// Replace the current diff in place. Cancels any prior highlight session + /// and stages the new jobs; the worker is spawned on the first + /// `drain_highlight_updates` call so any `diff_files` mutations queued for + /// after this call settle before updates start flowing. + fn install_diff(&mut self, diff: DiffWithJobs) { + if let Some(session) = self.highlight_session.take() { + session.cancel(); + } + let (files, jobs) = diff; + self.diff_files = files; + self.pending_highlight_jobs = jobs; + } + + fn start_pending_highlight_session(&mut self) { + if self.highlight_session.is_some() || self.pending_highlight_jobs.is_empty() { + return; + } + let jobs = std::mem::take(&mut self.pending_highlight_jobs); + let highlighter_arc = Arc::clone(self.theme.syntax_highlighter_arc()); + self.highlight_session = + HighlightSession::start_resolved(jobs, &self.diff_files, highlighter_arc); + } + + /// Drain pending highlight results and patch them into `diff_files`. + /// Spawns the worker lazily on first call after `install_diff`, so all + /// synchronous `diff_files` reorderings have completed before any update + /// is produced. + pub fn drain_highlight_updates(&mut self) { + self.start_pending_highlight_session(); + let Some(session) = self.highlight_session.as_ref() else { + return; + }; + loop { + match session.try_recv() { + Ok(update) => streaming::apply_update( + &mut self.diff_files, + self.theme.syntax_highlighter(), + update, + ), + Err(TryRecvError::Empty) => break, + Err(TryRecvError::Disconnected) => { + self.highlight_session = None; + break; + } + } + } + } + + /// True while the background highlight worker still has pending results, + /// or while jobs are staged awaiting their first drain. Used by the main + /// loop to shorten its event-poll timeout so streamed results land on + /// screen with low latency. + pub fn highlight_streaming(&self) -> bool { + self.highlight_session.is_some() || !self.pending_highlight_jobs.is_empty() } fn get_working_tree_diff_with_ignore( vcs: &dyn VcsBackend, repo_root: &Path, - highlighter: &SyntaxHighlighter, path_filter: Option<&str>, - ) -> Result> { - let diff_files = crate::profile::time_with( + ) -> Result { + let diff = crate::profile::time_with( "diff.load_working_tree", - || vcs.get_working_tree_diff(highlighter), + || vcs.get_working_tree_diff(), profile_diff_result, )?; - let diff_files = Self::filter_ignored_diff_files(repo_root, diff_files); - let diff_files = if let Some(path) = path_filter { - Self::filter_by_path(diff_files, path) - } else { - diff_files - }; - Self::require_non_empty_diff_files(diff_files) + Self::finalize_diff(diff, repo_root, path_filter) } fn get_staged_diff_with_ignore( vcs: &dyn VcsBackend, repo_root: &Path, - highlighter: &SyntaxHighlighter, path_filter: Option<&str>, - ) -> Result> { - let diff_files = crate::profile::time_with( + ) -> Result { + let diff = crate::profile::time_with( "diff.load_staged", - || vcs.get_staged_diff(highlighter), + || vcs.get_staged_diff(), profile_diff_result, )?; - let diff_files = Self::filter_ignored_diff_files(repo_root, diff_files); - let diff_files = if let Some(path) = path_filter { - Self::filter_by_path(diff_files, path) - } else { - diff_files - }; - Self::require_non_empty_diff_files(diff_files) + Self::finalize_diff(diff, repo_root, path_filter) } fn get_unstaged_diff_with_ignore( vcs: &dyn VcsBackend, repo_root: &Path, - highlighter: &SyntaxHighlighter, path_filter: Option<&str>, - ) -> Result> { - let diff_files = match crate::profile::time_with( + ) -> Result { + let diff = match crate::profile::time_with( "diff.load_unstaged", - || vcs.get_unstaged_diff(highlighter), + || vcs.get_unstaged_diff(), profile_diff_result, ) { - Ok(diff_files) => diff_files, + Ok(d) => d, Err(TuicrError::UnsupportedOperation(_)) => crate::profile::time_with( "diff.load_unstaged_fallback_working_tree", - || vcs.get_working_tree_diff(highlighter), + || vcs.get_working_tree_diff(), profile_diff_result, )?, Err(e) => return Err(e), }; - let diff_files = Self::filter_ignored_diff_files(repo_root, diff_files); - let diff_files = if let Some(path) = path_filter { - Self::filter_by_path(diff_files, path) - } else { - diff_files - }; - Self::require_non_empty_diff_files(diff_files) + Self::finalize_diff(diff, repo_root, path_filter) } fn get_commit_range_diff_with_ignore( vcs: &dyn VcsBackend, repo_root: &Path, revision_range: &ResolvedRevisionRange<'_>, - highlighter: &SyntaxHighlighter, path_filter: Option<&str>, - ) -> Result> { - let diff_files = crate::profile::time_with( + ) -> Result { + let diff = crate::profile::time_with( "diff.load_commit_range", - || vcs.get_commit_range_diff(revision_range, highlighter), + || vcs.get_commit_range_diff(revision_range), profile_diff_result, )?; - let diff_files = Self::filter_ignored_diff_files(repo_root, diff_files); - let diff_files = if let Some(path) = path_filter { - Self::filter_by_path(diff_files, path) - } else { - diff_files - }; - Self::require_non_empty_diff_files(diff_files) + Self::finalize_diff(diff, repo_root, path_filter) } fn get_working_tree_with_commits_diff_with_ignore( vcs: &dyn VcsBackend, repo_root: &Path, commit_ids: &[String], - highlighter: &SyntaxHighlighter, path_filter: Option<&str>, - ) -> Result> { - let diff_files = crate::profile::time_with( + ) -> Result { + let diff = crate::profile::time_with( "diff.load_working_tree_with_commits", - || vcs.get_working_tree_with_commits_diff(commit_ids, highlighter), + || vcs.get_working_tree_with_commits_diff(commit_ids), profile_diff_result, )?; - let diff_files = Self::filter_ignored_diff_files(repo_root, diff_files); - let diff_files = if let Some(path) = path_filter { - Self::filter_by_path(diff_files, path) - } else { - diff_files - }; - Self::require_non_empty_diff_files(diff_files) + Self::finalize_diff(diff, repo_root, path_filter) } /// Resolve the staged/unstaged status the commit selector renders. @@ -3626,7 +3646,6 @@ impl App { fn get_change_status_with_ignore( vcs: &dyn VcsBackend, repo_root: &Path, - highlighter: &SyntaxHighlighter, path_filter: Option<&str>, ) -> Result { if path_filter.is_none() { @@ -3635,13 +3654,7 @@ impl App { if !crate::tuicrignore::has_ignore_rules(repo_root) { return Ok(status); } - return Self::verify_status_against_ignore( - vcs, - repo_root, - highlighter, - path_filter, - status, - ); + return Self::verify_status_against_ignore(vcs, repo_root, path_filter, status); } Err(TuicrError::UnsupportedOperation(_)) => {} Err(e) => return Err(e), @@ -3651,7 +3664,6 @@ impl App { Self::verify_status_against_ignore( vcs, repo_root, - highlighter, path_filter, VcsChangeStatus { staged: true, @@ -3666,29 +3678,16 @@ impl App { fn verify_status_against_ignore( vcs: &dyn VcsBackend, repo_root: &Path, - highlighter: &SyntaxHighlighter, path_filter: Option<&str>, assumed_status: VcsChangeStatus, ) -> Result { let staged = if assumed_status.staged { - Self::side_has_visible_changes( - vcs, - repo_root, - highlighter, - path_filter, - ChangeKind::Staged, - )? + Self::side_has_visible_changes(vcs, repo_root, path_filter, ChangeKind::Staged)? } else { false }; let unstaged = if assumed_status.unstaged { - Self::side_has_visible_changes( - vcs, - repo_root, - highlighter, - path_filter, - ChangeKind::Unstaged, - )? + Self::side_has_visible_changes(vcs, repo_root, path_filter, ChangeKind::Unstaged)? } else { false }; @@ -3698,7 +3697,6 @@ impl App { fn side_has_visible_changes( vcs: &dyn VcsBackend, repo_root: &Path, - highlighter: &SyntaxHighlighter, path_filter: Option<&str>, kind: ChangeKind, ) -> Result { @@ -3713,14 +3711,11 @@ impl App { // anything survives. This still happens for jj/hg today. let diff_result = match kind { ChangeKind::Staged => { - Self::get_staged_diff_with_ignore(vcs, repo_root, highlighter, path_filter) + Self::get_staged_diff_with_ignore(vcs, repo_root, path_filter) + } + ChangeKind::Unstaged => { + Self::get_unstaged_diff_with_ignore(vcs, repo_root, path_filter) } - ChangeKind::Unstaged => Self::get_unstaged_diff_with_ignore( - vcs, - repo_root, - highlighter, - path_filter, - ), }; match diff_result { Ok(_) => Ok(true), @@ -3759,14 +3754,12 @@ impl App { } fn load_staged_and_unstaged_selection(&mut self) -> Result<()> { - let highlighter = self.theme.syntax_highlighter(); - let diff_files = match Self::get_working_tree_diff_with_ignore( + let diff = match Self::get_working_tree_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), ) { - Ok(diff_files) => diff_files, + Ok(diff) => diff, Err(TuicrError::NoChanges) => { self.set_message("No staged or unstaged changes"); return Ok(()); @@ -3776,12 +3769,12 @@ impl App { self.session = Self::load_or_create_session(&self.vcs_info, SessionDiffSource::StagedAndUnstaged); - for file in &diff_files { + for file in &diff.0 { self.session.add_diff_file(file); } self.reset_persisted_session_tracking(); - self.diff_files = diff_files; + self.install_diff(diff); self.diff_source = DiffSource::StagedAndUnstaged; self.input_mode = InputMode::Normal; self.diff_state = DiffState::default(); @@ -3795,14 +3788,12 @@ impl App { } fn load_staged_selection(&mut self) -> Result<()> { - let highlighter = self.theme.syntax_highlighter(); - let diff_files = match Self::get_staged_diff_with_ignore( + let diff = match Self::get_staged_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), ) { - Ok(diff_files) => diff_files, + Ok(diff) => diff, Err(TuicrError::NoChanges) => { self.set_message("No staged changes"); return Ok(()); @@ -3811,12 +3802,12 @@ impl App { }; self.session = Self::load_or_create_session(&self.vcs_info, SessionDiffSource::Staged); - for file in &diff_files { + for file in &diff.0 { self.session.add_diff_file(file); } self.reset_persisted_session_tracking(); - self.diff_files = diff_files; + self.install_diff(diff); self.diff_source = DiffSource::Staged; self.input_mode = InputMode::Normal; self.diff_state = DiffState::default(); @@ -3830,14 +3821,12 @@ impl App { } fn load_unstaged_selection(&mut self) -> Result<()> { - let highlighter = self.theme.syntax_highlighter(); - let diff_files = match Self::get_unstaged_diff_with_ignore( + let diff = match Self::get_unstaged_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), ) { - Ok(diff_files) => diff_files, + Ok(diff) => diff, Err(TuicrError::NoChanges) => { self.set_message("No unstaged changes"); return Ok(()); @@ -3846,12 +3835,12 @@ impl App { }; self.session = Self::load_or_create_session(&self.vcs_info, SessionDiffSource::Unstaged); - for file in &diff_files { + for file in &diff.0 { self.session.add_diff_file(file); } self.reset_persisted_session_tracking(); - self.diff_files = diff_files; + self.install_diff(diff); self.diff_source = DiffSource::Unstaged; self.input_mode = InputMode::Normal; self.diff_state = DiffState::default(); @@ -3881,13 +3870,11 @@ impl App { prev_cursor_line.saturating_sub(start) }; - let highlighter = self.theme.syntax_highlighter(); - let diff_files = match &self.diff_source { + let diff = match &self.diff_source { DiffSource::CommitRange(commit_ids) => Self::get_commit_range_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, &ResolvedRevisionRange::from_commit_ids(commit_ids, RevisionDiffTarget::CommitList), - highlighter, self.path_filter.as_deref(), )?, DiffSource::StagedUnstagedAndCommits(commit_ids) => { @@ -3896,27 +3883,23 @@ impl App { self.vcs.as_ref(), &self.vcs_info.root_path, &ids, - highlighter, self.path_filter.as_deref(), )? } DiffSource::Staged => Self::get_staged_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), )?, DiffSource::Unstaged => Self::get_unstaged_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), )?, DiffSource::StagedAndUnstaged | DiffSource::WorkingTree => { Self::get_working_tree_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), )? } @@ -3932,13 +3915,13 @@ impl App { }; let mut invalidated = 0; - for file in &diff_files { + for file in &diff.0 { if self.session.add_diff_file(file) { invalidated += 1; } } - self.diff_files = diff_files; + self.install_diff(diff); self.clear_expanded_gaps(); self.sort_files_by_directory(false); @@ -7483,11 +7466,9 @@ impl App { self.saved_inline_selection = self.commit_selection_range; } - let highlighter = self.theme.syntax_highlighter(); let change_status = Self::get_change_status_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), )?; let has_staged_changes = change_status.staged; @@ -7556,22 +7537,17 @@ impl App { self.diff_source, DiffSource::CommitRange(_) | DiffSource::StagedUnstagedAndCommits(_) ) { - let highlighter = self.theme.syntax_highlighter(); match Self::get_working_tree_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), ) { - Ok(diff_files) => { - self.diff_files = diff_files; - self.diff_source = DiffSource::StagedAndUnstaged; - - // Update session for new files - for file in &self.diff_files { + Ok(diff) => { + for file in &diff.0 { self.session.add_diff_file(file); } - + self.install_diff(diff); + self.diff_source = DiffSource::StagedAndUnstaged; self.sort_files_by_directory(true); self.expand_all_dirs(); } @@ -7856,14 +7832,8 @@ impl App { use crate::forge::pr_open::prepare_open_pr; let local_checkout = Some(self.vcs_info.root_path.clone()); - let highlighter = self.theme.syntax_highlighter(); - let mut opened = prepare_open_pr( - details.clone(), - &patch, - commits, - local_checkout.as_deref(), - highlighter, - )?; + let mut opened = + prepare_open_pr(details.clone(), &patch, commits, local_checkout.as_deref())?; Self::load_or_apply_pr_session(&mut opened); let backend = create_forge_backend(&request.repository, local_checkout.clone()); self.enter_pr_diff_mode(backend, opened)?; @@ -8074,13 +8044,7 @@ impl App { summary.number, summary.number.to_string(), ); - let highlighter = self.theme.syntax_highlighter(); - let mut opened = open_pull_request( - backend.as_ref(), - target, - local_checkout.as_deref(), - highlighter, - )?; + let mut opened = open_pull_request(backend.as_ref(), target, local_checkout.as_deref())?; Self::load_or_apply_pr_session(&mut opened); // Sync thread + summary fetch — tests assert on // `app.forge_review_threads`/`forge_review_summaries` immediately @@ -8459,10 +8423,11 @@ impl App { }; // Collect selected entries in order from oldest to newest (end..start). + // Cloned eagerly so the immutable borrow of `self.commit_list` is + // released before any `&mut self` calls (install_diff, set_message). let selected_commits: Vec = (start..=end) .rev() - .filter_map(|i| self.commit_list.get(i)) - .cloned() + .filter_map(|i| self.commit_list.get(i).cloned()) .collect(); if selected_commits.is_empty() { @@ -8495,16 +8460,14 @@ impl App { } // Get the diff for the selected commits - let highlighter = self.theme.syntax_highlighter(); - let diff_files = Self::get_commit_range_diff_with_ignore( + let diff = Self::get_commit_range_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, &ResolvedRevisionRange::from_commit_ids(&selected_ids, RevisionDiffTarget::CommitList), - highlighter, self.path_filter.as_deref(), )?; - if diff_files.is_empty() { + if diff.0.is_empty() { self.set_message("No changes in selected commits"); return Ok(()); } @@ -8540,13 +8503,12 @@ impl App { self.session = session; // Add files to session - for file in &diff_files { + for file in &diff.0 { self.session.add_diff_file(file); } self.reset_persisted_session_tracking(); - // Update app state - self.diff_files = diff_files; + self.install_diff(diff); self.diff_source = DiffSource::CommitRange(selected_ids); self.input_mode = InputMode::Normal; @@ -8555,7 +8517,7 @@ impl App { self.file_list_state = FileListState::default(); // Set up inline commit selector for multi-commit reviews (newest-first display order) - self.review_commits = selected_commits.iter().rev().cloned().collect(); + self.review_commits = selected_commits.into_iter().rev().collect(); self.range_diff_files = Some(self.diff_files.clone()); self.commit_list = self.review_commits.clone(); self.commit_list_cursor = 0; @@ -8588,9 +8550,9 @@ impl App { // Check if all commits selected -> use cached range_diff_files if start == 0 && end == self.review_commits.len() - 1 - && let Some(ref files) = self.range_diff_files + && let Some(files) = self.range_diff_files.clone() { - self.diff_files = files.clone(); + self.install_diff((files, Vec::new())); let wrap = self.diff_state.wrap_lines; self.diff_state = DiffState::default(); self.diff_state.wrap_lines = wrap; @@ -8605,8 +8567,8 @@ impl App { } // Check cache for this subrange - if let Some(files) = self.commit_diff_cache.get(&(start, end)) { - self.diff_files = files.clone(); + if let Some(files) = self.commit_diff_cache.get(&(start, end)).cloned() { + self.install_diff((files, Vec::new())); let wrap = self.diff_state.wrap_lines; self.diff_state = DiffState::default(); self.diff_state.wrap_lines = wrap; @@ -8638,50 +8600,46 @@ impl App { .map(|c| c.id.clone()) .collect(); - let highlighter = self.theme.syntax_highlighter(); - let diff_files = if (has_staged || has_unstaged) && !selected_ids.is_empty() { + let empty: DiffWithJobs = (Vec::new(), Vec::new()); + let diff = if (has_staged || has_unstaged) && !selected_ids.is_empty() { match Self::get_working_tree_with_commits_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, &selected_ids, - highlighter, self.path_filter.as_deref(), ) { - Ok(files) => files, - Err(TuicrError::NoChanges) => Vec::new(), + Ok(d) => d, + Err(TuicrError::NoChanges) => empty, Err(e) => return Err(e), } } else if has_staged && has_unstaged { match Self::get_working_tree_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), ) { - Ok(files) => files, - Err(TuicrError::NoChanges) => Vec::new(), + Ok(d) => d, + Err(TuicrError::NoChanges) => empty, Err(e) => return Err(e), } } else if has_staged { match Self::get_staged_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), ) { - Ok(files) => files, - Err(TuicrError::NoChanges) => Vec::new(), + Ok(d) => d, + Err(TuicrError::NoChanges) => empty, Err(e) => return Err(e), } } else if has_unstaged { match Self::get_unstaged_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, - highlighter, self.path_filter.as_deref(), ) { - Ok(files) => files, - Err(TuicrError::NoChanges) => Vec::new(), + Ok(d) => d, + Err(TuicrError::NoChanges) => empty, Err(e) => return Err(e), } } else { @@ -8692,17 +8650,15 @@ impl App { &selected_ids, RevisionDiffTarget::CommitList, ), - highlighter, self.path_filter.as_deref(), ) { - Ok(files) => files, - Err(TuicrError::NoChanges) => Vec::new(), + Ok(d) => d, + Err(TuicrError::NoChanges) => empty, Err(e) => return Err(e), } }; - self.commit_diff_cache - .insert((start, end), diff_files.clone()); - self.diff_files = diff_files; + self.commit_diff_cache.insert((start, end), diff.0.clone()); + self.install_diff(diff); // Reset navigation, rebuild file tree + annotations let wrap = self.diff_state.wrap_lines; @@ -8724,15 +8680,13 @@ impl App { selected_ids: Vec, selected_commits: Vec, ) -> Result<()> { - let highlighter = self.theme.syntax_highlighter(); - let diff_files = match Self::get_working_tree_with_commits_diff_with_ignore( + let diff = match Self::get_working_tree_with_commits_diff_with_ignore( self.vcs.as_ref(), &self.vcs_info.root_path, &selected_ids, - highlighter, self.path_filter.as_deref(), ) { - Ok(diff_files) => diff_files, + Ok(d) => d, Err(TuicrError::NoChanges) => { self.set_message("No changes in selected commits + staged/unstaged"); return Ok(()); @@ -8743,12 +8697,12 @@ impl App { self.session = Self::load_or_create_staged_unstaged_and_commits_session(&self.vcs_info, &selected_ids); - for file in &diff_files { + for file in &diff.0 { self.session.add_diff_file(file); } self.reset_persisted_session_tracking(); - self.diff_files = diff_files; + self.install_diff(diff); self.diff_source = DiffSource::StagedUnstagedAndCommits(selected_ids); self.input_mode = InputMode::Normal; self.diff_state = DiffState::default(); @@ -10148,7 +10102,7 @@ mod commit_selection_tests { &self.info } - fn get_working_tree_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { Err(TuicrError::NoChanges) } @@ -10195,7 +10149,7 @@ mod commit_selection_tests { Theme::dark(), None, false, - Vec::new(), + (Vec::new(), Vec::new()), session, DiffSource::WorkingTree, InputMode::CommitSelect, @@ -10287,7 +10241,7 @@ mod target_selector_tests { &self.info } - fn get_working_tree_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { Err(TuicrError::NoChanges) } @@ -10356,7 +10310,7 @@ mod target_selector_tests { Theme::dark(), None, false, - Vec::new(), + (Vec::new(), Vec::new()), session, DiffSource::WorkingTree, InputMode::Normal, @@ -11756,7 +11710,7 @@ mod scroll_behavior_tests { &self.info } - fn get_working_tree_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { Err(TuicrError::NoChanges) } @@ -11837,7 +11791,7 @@ mod scroll_behavior_tests { Theme::dark(), None, false, - vec![file], + (vec![file], Vec::new()), session, DiffSource::WorkingTree, InputMode::Normal, @@ -12396,23 +12350,23 @@ mod change_status_tests { &self.info } - fn get_working_tree_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { Err(TuicrError::NoChanges) } - fn get_staged_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_staged_diff(&self) -> Result { if self.staged_files.is_empty() { Err(TuicrError::NoChanges) } else { - Ok(self.staged_files.clone()) + Ok((self.staged_files.clone(), Vec::new())) } } - fn get_unstaged_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_unstaged_diff(&self) -> Result { if self.unstaged_files.is_empty() { Err(TuicrError::NoChanges) } else { - Ok(self.unstaged_files.clone()) + Ok((self.unstaged_files.clone(), Vec::new())) } } @@ -12480,13 +12434,8 @@ mod change_status_tests { vcs.staged_files = vec![diff_file("ignored/generated.rs")]; vcs.unstaged_files = vec![diff_file("src/lib.rs")]; - let status = App::get_change_status_with_ignore( - &vcs, - dir.path(), - &SyntaxHighlighter::default(), - None, - ) - .expect("failed to get change status"); + let status = App::get_change_status_with_ignore(&vcs, dir.path(), None) + .expect("failed to get change status"); assert_eq!( status, @@ -12506,13 +12455,8 @@ mod change_status_tests { let dir = tempdir().expect("failed to create temp dir"); let vcs = mock_vcs(dir.path().to_path_buf()); - let status = App::get_change_status_with_ignore( - &vcs, - dir.path(), - &SyntaxHighlighter::default(), - None, - ) - .expect("failed to get change status"); + let status = App::get_change_status_with_ignore(&vcs, dir.path(), None) + .expect("failed to get change status"); assert_eq!( status, @@ -12541,20 +12485,17 @@ mod change_status_tests { fn info(&self) -> &VcsInfo { self.inner.info() } - fn get_working_tree_diff( - &self, - _highlighter: &SyntaxHighlighter, - ) -> Result> { + fn get_working_tree_diff(&self) -> Result { Err(TuicrError::UnsupportedOperation( "should not be called".into(), )) } - fn get_staged_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_staged_diff(&self) -> Result { Err(TuicrError::UnsupportedOperation( "should not be called".into(), )) } - fn get_unstaged_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_unstaged_diff(&self) -> Result { Err(TuicrError::UnsupportedOperation( "should not be called".into(), )) @@ -12600,13 +12541,8 @@ mod change_status_tests { unstaged_paths: vec![PathBuf::from("src/lib.rs")], }; - let status = App::get_change_status_with_ignore( - &vcs, - dir.path(), - &SyntaxHighlighter::default(), - None, - ) - .expect("failed to get change status"); + let status = App::get_change_status_with_ignore(&vcs, dir.path(), None) + .expect("failed to get change status"); assert_eq!( status, @@ -12637,7 +12573,7 @@ mod expand_gap_tests { &self.info } - fn get_working_tree_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { Err(TuicrError::NoChanges) } @@ -12716,7 +12652,7 @@ mod expand_gap_tests { Theme::dark(), None, false, - files, + (files, Vec::new()), session, DiffSource::WorkingTree, InputMode::Normal, @@ -13856,7 +13792,7 @@ mod submit_flow_tests { fn info(&self) -> &VcsInfo { &self.info } - fn get_working_tree_diff(&self, _h: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { Err(TuicrError::NoChanges) } fn fetch_context_lines( @@ -13953,7 +13889,7 @@ mod submit_flow_tests { Theme::dark(), None, false, - vec![diff_file], + (vec![diff_file], Vec::new()), session, DiffSource::PullRequest(Box::new(pr_source)), InputMode::Normal, @@ -14268,7 +14204,7 @@ mod submit_flow_tests { Theme::dark(), None, false, - Vec::new(), + (Vec::new(), Vec::new()), session, DiffSource::WorkingTree, InputMode::Normal, @@ -14940,11 +14876,8 @@ mod single_file_view_tests { fn info(&self) -> &VcsInfo { &self.0 } - fn get_working_tree_diff( - &self, - _hl: &crate::syntax::SyntaxHighlighter, - ) -> crate::error::Result> { - Ok(Vec::new()) + fn get_working_tree_diff(&self) -> crate::error::Result { + Ok((Vec::new(), Vec::new())) } fn fetch_context_lines( &self, @@ -15023,7 +14956,7 @@ mod single_file_view_tests { crate::theme::Theme::dark(), None, false, - files, + (files, Vec::new()), session, DiffSource::WorkingTree, InputMode::Normal, diff --git a/src/forge/pr_open.rs b/src/forge/pr_open.rs index 1a61f704..6523739d 100644 --- a/src/forge/pr_open.rs +++ b/src/forge/pr_open.rs @@ -19,15 +19,15 @@ use crate::forge::traits::{ ForgeBackend, PrSessionKey, PullRequestCommit, PullRequestDetails, PullRequestTarget, }; use crate::model::{DiffFile, ReviewSession, SessionDiffSource}; -use crate::syntax::SyntaxHighlighter; use crate::tuicrignore; +use crate::vcs::DiffWithJobs; use crate::vcs::diff_parser::{DiffFormat, parse_unified_diff}; /// Everything the App needs to enter PR review mode. #[derive(Debug)] pub struct OpenedPullRequest { pub details: PullRequestDetails, - pub diff_files: Vec, + pub diff: DiffWithJobs, pub session: ReviewSession, pub key: PrSessionKey, /// PR commits in newest-first display order. Empty when the forge @@ -45,10 +45,9 @@ pub fn open_pull_request( backend: &dyn ForgeBackend, target: PullRequestTarget, local_checkout: Option<&Path>, - highlighter: &SyntaxHighlighter, ) -> Result { let (details, patch, commits) = fetch_pr_data(backend, target)?; - prepare_open_pr(details, &patch, commits, local_checkout, highlighter) + prepare_open_pr(details, &patch, commits, local_checkout) } /// Network-only half of the PR open path: fetch PR metadata, the raw @@ -71,17 +70,16 @@ pub fn fetch_pr_data( } /// CPU-only half of the PR open path: parse the patch, apply -/// `.tuicrignore`, and build the session. Runs on the main thread because -/// `SyntaxHighlighter` is not trivially `Send`-cloneable. +/// `.tuicrignore`, and build the session. Highlighting is deferred to the +/// streaming worker via the returned `DiffWithJobs`. pub fn prepare_open_pr( details: PullRequestDetails, patch: &str, commits: Vec, local_checkout: Option<&Path>, - highlighter: &SyntaxHighlighter, ) -> Result { - let parsed = match parse_unified_diff(patch, DiffFormat::GitStyle, highlighter) { - Ok(files) => files, + let parsed = match parse_unified_diff(patch, DiffFormat::GitStyle) { + Ok(diff) => diff, Err(TuicrError::NoChanges) => { return Err(TuicrError::Forge(format!( "Pull request #{} has no file changes", @@ -91,13 +89,13 @@ pub fn prepare_open_pr( Err(e) => return Err(e), }; - let diff_files = match local_checkout { + let diff = match local_checkout { Some(root) => tuicrignore::filter_diff_files(root, parsed), None => parsed, }; let key = PrSessionKey::from_details(&details); - let session = build_session(&details, &key, &diff_files); + let session = build_session(&details, &key, &diff.0); // Forge returns commits oldest-first; the inline selector renders // newest-first so reverse here once. let mut commits = commits; @@ -105,7 +103,7 @@ pub fn prepare_open_pr( Ok(OpenedPullRequest { details, - diff_files, + diff, session, key, commits, @@ -252,11 +250,10 @@ index 1111111..2222222 100644 calls: RefCell::new(Vec::new()), }; let target = PullRequestTarget::with_repository(repo(), 125, "125"); - let highlighter = SyntaxHighlighter::default(); // when - let opened = open_pull_request(&backend, target, None, &highlighter).unwrap(); + let opened = open_pull_request(&backend, target, None).unwrap(); // then - assert_eq!(opened.diff_files.len(), 1); + assert_eq!(opened.diff.0.len(), 1); assert_eq!(opened.key.head_sha, "abcdef0123456789"); assert_eq!(opened.key.number, 125); assert_eq!(opened.session.diff_source, SessionDiffSource::PullRequest); @@ -319,13 +316,13 @@ rename to new_name.rs calls: RefCell::new(Vec::new()), }; let target = PullRequestTarget::with_repository(repo(), 125, "125"); - let highlighter = SyntaxHighlighter::default(); // when - let opened = open_pull_request(&backend, target, None, &highlighter).unwrap(); + let opened = open_pull_request(&backend, target, None).unwrap(); // then — all four files are recognized with correct statuses - assert_eq!(opened.diff_files.len(), 4); + assert_eq!(opened.diff.0.len(), 4); let statuses: Vec<(String, crate::model::FileStatus)> = opened - .diff_files + .diff + .0 .iter() .map(|f| (f.display_path().to_string_lossy().into_owned(), f.status)) .collect(); @@ -358,9 +355,8 @@ rename to new_name.rs calls: RefCell::new(Vec::new()), }; let target = PullRequestTarget::with_repository(repo(), 125, "125"); - let highlighter = SyntaxHighlighter::default(); // when - let err = open_pull_request(&backend, target, None, &highlighter).unwrap_err(); + let err = open_pull_request(&backend, target, None).unwrap_err(); // then let msg = err.to_string(); assert!( diff --git a/src/main.rs b/src/main.rs index 9997b44f..1b6711c7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -29,6 +29,13 @@ use tuicr::{config, handler, profile, ui, update}; const CTRL_C_EXIT_TIMEOUT: Duration = Duration::from_secs(2); /// Hide the file list by default on narrow terminals. const MIN_WIDTH_FOR_FILE_LIST: u16 = 100; +/// Idle event-poll cadence: long enough to keep CPU usage trivial while no +/// input or background highlight results are pending. +const IDLE_POLL_TIMEOUT: Duration = Duration::from_millis(100); +/// Poll cadence while the highlight worker is still streaming results. +/// ~30 ms keeps perceived latency between worker emit and on-screen update +/// under one frame; CPU cost of the extra wakeups is negligible. +const STREAMING_POLL_TIMEOUT: Duration = Duration::from_millis(30); fn main() -> anyhow::Result<()> { profile::init_from_env(); @@ -327,6 +334,10 @@ fn main() -> anyhow::Result<()> { app.poll_pr_submit_events(); app.poll_persisted_session_changes(); + // Drain any streaming highlight results before rendering so newly + // arrived spans land on screen this frame. + app.drain_highlight_updates(); + // Render. Bracket the frame in a synchronized-output pair // (CSI ?2026h/l) so terminals (and zellij) buffer the whole repaint // and present it atomically. Without this, scrolling over a slow link @@ -338,8 +349,16 @@ fn main() -> anyhow::Result<()> { })?; execute!(terminal.backend_mut(), EndSynchronizedUpdate)?; + // Shorter poll while highlight work is in flight keeps streaming + // latency under one frame; longer poll when idle to avoid wakeups. + let poll_timeout = if app.highlight_streaming() { + STREAMING_POLL_TIMEOUT + } else { + IDLE_POLL_TIMEOUT + }; + // Handle events - if event::poll(Duration::from_millis(100))? { + if event::poll(poll_timeout)? { let event = event::read()?; // Down/Up Release flips the `*_released_since_arm` flag so the // primed two-press file walk in single-file view requires a diff --git a/src/syntax/mod.rs b/src/syntax/mod.rs index b2a0e55b..48cda13e 100644 --- a/src/syntax/mod.rs +++ b/src/syntax/mod.rs @@ -4,6 +4,9 @@ use two_face::theme::EmbeddedThemeName; use crate::model::diff_types::LineOrigin; +pub(crate) mod streaming; +pub(crate) use streaming::{HighlightJob, HighlightJobKind}; + /// A single line of highlighted spans (style + text pairs). pub(crate) type HighlightedSpans = Vec<(Style, String)>; diff --git a/src/syntax/streaming.rs b/src/syntax/streaming.rs new file mode 100644 index 00000000..b8f533aa --- /dev/null +++ b/src/syntax/streaming.rs @@ -0,0 +1,334 @@ +//! Streaming syntax-highlight pipeline. +//! +//! The diff is parsed (and rendered) without highlighting; a background +//! worker thread then produces highlight results and streams them to the +//! main thread via an mpsc channel. The main loop drains the channel each +//! iteration and patches the model in place, then redraws. +//! +//! Why this shape: VCS access (git2, jj/hg subprocess) is often `!Send`, +//! so the parse phase has to stay on the main thread; syntect's +//! `SyntaxSet` / `Theme` are `Sync`, so the highlight phase parallelises +//! cleanly across threads as long as inputs are owned strings. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::mpsc::{self, Sender}; +use std::thread; + +use crate::model::diff_types::LineOrigin; +use crate::syntax::{HighlightedLines, HighlightedSpans, SyntaxHighlighter}; + +/// Same cost ceiling as the previous synchronous full-file pass: skip +/// highlighting for files over this size to keep runaway diffs cheap. +const MAX_HIGHLIGHT_FILE_BYTES: usize = 1024 * 1024; + +/// A single unit of highlight work, produced during the (serial) parse phase +/// and consumed on the background worker. +#[derive(Debug)] +pub struct HighlightJob { + pub file_idx: usize, + pub syntax_path: PathBuf, + pub kind: HighlightJobKind, +} + +#[derive(Debug)] +pub enum HighlightJobKind { + /// Per-hunk highlight using only the lines inside the hunk. + Hunk { + hunk_idx: usize, + old_lines: Vec, + new_lines: Vec, + old_line_indices: Vec>, + new_line_indices: Vec>, + line_origins: Vec, + }, + /// Container-grammar (Vue, Svelte, ...) full-file context. Content is + /// fetched during parse so the worker never touches VCS state. + FullFile { + old_content: Option, + new_content: Option, + }, +} + +/// A highlight result the worker streams back. Each variant patches one +/// file in place. +pub(crate) struct HighlightUpdate { + pub file_idx: usize, + pub kind: HighlightUpdateKind, +} + +pub(crate) enum HighlightUpdateKind { + Hunk { + hunk_idx: usize, + line_spans: Vec>, + }, + FullFile { + old: Option, + new: Option, + }, +} + +/// Spawn the highlight worker. Returns the join handle; the caller usually +/// doesn't need to wait on it (the worker exits when its send fails on a +/// dropped receiver, or when `cancel` is set). +pub(crate) fn spawn_highlight_worker( + jobs: Vec, + highlighter: Arc, + cancel: Arc, + tx: Sender, +) -> thread::JoinHandle<()> { + thread::spawn(move || run_worker(jobs, highlighter, cancel, tx)) +} + +/// Run the highlight pipeline synchronously on the current thread. Used by +/// tests that want spans populated deterministically without a worker thread. +#[cfg(test)] +pub(crate) fn run_blocking( + jobs: Vec, + highlighter: &SyntaxHighlighter, +) -> Vec { + let mut out = Vec::with_capacity(jobs.len()); + for job in jobs { + if let Some(update) = run_job(&job, highlighter) { + out.push(update); + } + } + out +} + +fn run_worker( + jobs: Vec, + highlighter: Arc, + cancel: Arc, + tx: Sender, +) { + if jobs.is_empty() { + return; + } + + let parallelism = thread::available_parallelism() + .map(|n| n.get()) + .unwrap_or(1) + .min(jobs.len()) + .max(1); + + if parallelism <= 1 { + for job in &jobs { + if cancel.load(Ordering::Relaxed) { + return; + } + if let Some(update) = run_job(job, &highlighter) + && tx.send(update).is_err() + { + return; + } + } + return; + } + + // Atomic-cursor work stealing: every worker pulls the next index from + // a shared counter. Beats static chunking when job sizes are skewed + // (e.g. one giant file + many small ones). + let cursor = Arc::new(AtomicUsize::new(0)); + let jobs = Arc::new(jobs); + thread::scope(|s| { + for _ in 0..parallelism { + let cursor = Arc::clone(&cursor); + let jobs = Arc::clone(&jobs); + let cancel = Arc::clone(&cancel); + let tx = tx.clone(); + let highlighter = Arc::clone(&highlighter); + s.spawn(move || { + loop { + if cancel.load(Ordering::Relaxed) { + return; + } + let idx = cursor.fetch_add(1, Ordering::Relaxed); + if idx >= jobs.len() { + return; + } + if let Some(update) = run_job(&jobs[idx], &highlighter) + && tx.send(update).is_err() + { + return; + } + } + }); + } + }); +} + +fn run_job(job: &HighlightJob, h: &SyntaxHighlighter) -> Option { + match &job.kind { + HighlightJobKind::Hunk { + hunk_idx, + old_lines, + new_lines, + old_line_indices, + new_line_indices, + line_origins, + } => { + let old_highlighted = h.highlight_file_lines(&job.syntax_path, old_lines); + let new_highlighted = h.highlight_file_lines(&job.syntax_path, new_lines); + if old_highlighted.is_none() && new_highlighted.is_none() { + return None; + } + let line_spans: Vec> = (0..line_origins.len()) + .map(|i| { + h.highlighted_line_for_diff_with_background( + old_highlighted.as_deref(), + new_highlighted.as_deref(), + old_line_indices[i], + new_line_indices[i], + line_origins[i], + ) + }) + .collect(); + Some(HighlightUpdate { + file_idx: job.file_idx, + kind: HighlightUpdateKind::Hunk { + hunk_idx: *hunk_idx, + line_spans, + }, + }) + } + HighlightJobKind::FullFile { + old_content, + new_content, + } => { + let old = highlight_full(h, &job.syntax_path, old_content.as_deref()); + let new = highlight_full(h, &job.syntax_path, new_content.as_deref()); + if old.is_none() && new.is_none() { + return None; + } + Some(HighlightUpdate { + file_idx: job.file_idx, + kind: HighlightUpdateKind::FullFile { old, new }, + }) + } + } +} + +fn highlight_full( + h: &SyntaxHighlighter, + path: &std::path::Path, + content: Option<&str>, +) -> Option { + let content = content?; + if content.len() > MAX_HIGHLIGHT_FILE_BYTES || content.as_bytes().contains(&0u8) { + return None; + } + let lines: Vec = content.lines().map(crate::vcs::tabify).collect(); + h.highlight_file_lines(path, &lines) +} + +/// A running highlight session: receiver for streamed results plus the +/// cancel token shared with the worker. The two are always twinned, so they +/// live behind a single `Option` on `App`. +pub(crate) struct HighlightSession { + rx: mpsc::Receiver, + cancel: Arc, +} + +impl HighlightSession { + /// Start a new session. Returns `None` if there is nothing to highlight, + /// in which case no worker is spawned. + pub(crate) fn start( + jobs: Vec, + highlighter: Arc, + ) -> Option { + if jobs.is_empty() { + return None; + } + let cancel = Arc::new(AtomicBool::new(false)); + let (tx, rx) = mpsc::channel(); + spawn_highlight_worker(jobs, highlighter, Arc::clone(&cancel), tx); + Some(Self { rx, cancel }) + } + + /// Re-resolve each job's `file_idx` by `syntax_path` against + /// `diff_files.display_path()`, drop jobs whose path is no longer present, + /// then start the session. Used after the parse-time `file_idx` may have + /// drifted from its file's position (commit-message insert, directory + /// sort, `.tuicrignore` re-filter). `HighlightJob::syntax_path` and + /// `DiffFile::display_path` are both `new_path or old_path`, so the keys + /// line up. + pub(crate) fn start_resolved( + mut jobs: Vec, + diff_files: &[crate::model::DiffFile], + highlighter: Arc, + ) -> Option { + let path_to_idx: HashMap<&Path, usize> = diff_files + .iter() + .enumerate() + .map(|(i, f)| (f.display_path().as_path(), i)) + .collect(); + jobs.retain_mut(|job| match path_to_idx.get(job.syntax_path.as_path()) { + Some(&idx) => { + job.file_idx = idx; + true + } + None => false, + }); + Self::start(jobs, highlighter) + } + + /// Signal the worker to stop. The receiver is also dropped, so any + /// in-flight result the worker tries to send will fail and the worker + /// exits at its next channel send. + pub(crate) fn cancel(self) { + self.cancel.store(true, Ordering::Relaxed); + // drop self → rx dropped + } + + pub(crate) fn try_recv(&self) -> Result { + self.rx.try_recv() + } +} + +/// Patch a streamed highlight result into the in-memory diff. Idempotent: +/// applying the same update twice produces the same final state. +pub(crate) fn apply_update( + files: &mut [crate::model::DiffFile], + highlighter: &SyntaxHighlighter, + update: HighlightUpdate, +) { + let Some(file) = files.get_mut(update.file_idx) else { + return; + }; + match update.kind { + HighlightUpdateKind::Hunk { + hunk_idx, + line_spans, + } => { + let Some(hunk) = file.hunks.get_mut(hunk_idx) else { + return; + }; + for (line, spans) in hunk.lines.iter_mut().zip(line_spans) { + if let Some(spans) = spans { + line.highlighted_spans = Some(spans); + } + } + } + HighlightUpdateKind::FullFile { old, new } => { + for hunk in &mut file.hunks { + for line in &mut hunk.lines { + let old_idx = line.old_lineno.map(|n| n.saturating_sub(1) as usize); + let new_idx = line.new_lineno.map(|n| n.saturating_sub(1) as usize); + let spans = highlighter.highlighted_line_for_diff_with_background( + old.as_deref(), + new.as_deref(), + old_idx, + new_idx, + line.origin, + ); + if spans.is_some() { + line.highlighted_spans = spans; + } + } + } + } + } +} diff --git a/src/theme/mod.rs b/src/theme/mod.rs index 0e0b277a..25ec3fb8 100644 --- a/src/theme/mod.rs +++ b/src/theme/mod.rs @@ -25,7 +25,7 @@ pub enum SyntaxThemeSource { /// Complete color theme for the application pub struct Theme { /// Cached syntax highlighter (lazily initialized) - highlighter: OnceLock, + highlighter: OnceLock>, // Base colors pub panel_bg: Color, @@ -2274,15 +2274,23 @@ pub fn resolve_theme_with_config( impl Theme { /// Get the syntax highlighter for this theme (lazily initialized, cached) pub fn syntax_highlighter(&self) -> &SyntaxHighlighter { - self.highlighter.get_or_init(|| match &self.syntax_theme { - SyntaxThemeSource::Embedded(theme) => { - SyntaxHighlighter::new(*theme, self.syntax_add_bg, self.syntax_del_bg) - } - SyntaxThemeSource::Custom(theme) => SyntaxHighlighter::with_theme( - *theme.clone(), - self.syntax_add_bg, - self.syntax_del_bg, - ), + self.syntax_highlighter_arc().as_ref() + } + + /// Same as `syntax_highlighter`, but yields an `Arc` clone that can be + /// moved into the highlight worker thread. + pub fn syntax_highlighter_arc(&self) -> &std::sync::Arc { + self.highlighter.get_or_init(|| { + std::sync::Arc::new(match &self.syntax_theme { + SyntaxThemeSource::Embedded(theme) => { + SyntaxHighlighter::new(*theme, self.syntax_add_bg, self.syntax_del_bg) + } + SyntaxThemeSource::Custom(theme) => SyntaxHighlighter::with_theme( + *theme.clone(), + self.syntax_add_bg, + self.syntax_del_bg, + ), + }) }) } diff --git a/src/tuicrignore.rs b/src/tuicrignore.rs index 8a87ce7e..46c00815 100644 --- a/src/tuicrignore.rs +++ b/src/tuicrignore.rs @@ -2,22 +2,19 @@ use std::path::Path; use ignore::gitignore::GitignoreBuilder; -use crate::model::DiffFile; +use crate::vcs::DiffWithJobs; -/// Apply `.tuicrignore` rules from the repository root to a diff file set. -pub fn filter_diff_files(repo_root: &Path, diff_files: Vec) -> Vec { +/// Apply `.tuicrignore` rules from the repository root to a diff file set, +/// re-indexing the accompanying highlight jobs. +pub fn filter_diff_files(repo_root: &Path, diff: DiffWithJobs) -> DiffWithJobs { let Some(matcher) = load_matcher(repo_root) else { - return diff_files; + return diff; }; - - diff_files - .into_iter() - .filter(|file| { - !matcher - .matched_path_or_any_parents(file.display_path(), false) - .is_ignore() - }) - .collect() + crate::vcs::filter_diff_with_jobs(diff, |file| { + !matcher + .matched_path_or_any_parents(file.display_path(), false) + .is_ignore() + }) } /// Apply `.tuicrignore` (and `.gitignore`, for `!`-unignore patterns) rules to a @@ -68,7 +65,7 @@ mod tests { use tempfile::tempdir; use super::*; - use crate::model::FileStatus; + use crate::model::{DiffFile, FileStatus}; fn make_diff_file(path: &str) -> DiffFile { DiffFile { @@ -83,6 +80,10 @@ mod tests { } } + fn filter_just_files(repo_root: &Path, files: Vec) -> Vec { + filter_diff_files(repo_root, (files, Vec::new())).0 + } + #[test] fn keeps_all_files_when_tuicrignore_is_missing() { let dir = tempdir().expect("failed to create temp dir"); @@ -91,7 +92,7 @@ mod tests { make_diff_file("target/debug/app"), ]; - let filtered = filter_diff_files(dir.path(), files); + let filtered = filter_just_files(dir.path(), files); assert_eq!(filtered.len(), 2); } @@ -117,7 +118,7 @@ mod tests { make_diff_file("Cargo.lock"), ]; - let filtered = filter_diff_files(dir.path(), files); + let filtered = filter_just_files(dir.path(), files); let kept_paths: Vec = filtered .iter() .map(|f| f.display_path().display().to_string()) @@ -139,7 +140,7 @@ mod tests { make_diff_file("src/main.rs"), ]; - let filtered = filter_diff_files(dir.path(), files); + let filtered = filter_just_files(dir.path(), files); let kept_paths: Vec = filtered .iter() .map(|f| f.display_path().display().to_string()) @@ -160,7 +161,7 @@ mod tests { make_diff_file("build.log"), ]; - let filtered = filter_diff_files(dir.path(), files); + let filtered = filter_just_files(dir.path(), files); let kept: Vec = filtered .iter() .map(|f| f.display_path().display().to_string()) @@ -183,7 +184,7 @@ mod tests { make_diff_file("src/lib.rs"), ]; - let filtered = filter_diff_files(dir.path(), files); + let filtered = filter_just_files(dir.path(), files); let kept: Vec = filtered .iter() .map(|f| f.display_path().display().to_string()) @@ -204,7 +205,7 @@ mod tests { make_diff_file("dist/bundle.js"), ]; - let filtered = filter_diff_files(dir.path(), files); + let filtered = filter_just_files(dir.path(), files); let kept: Vec = filtered .iter() .map(|f| f.display_path().display().to_string()) @@ -231,7 +232,7 @@ mod tests { }; let kept = make_diff_file("src/lib.rs"); - let filtered = filter_diff_files(dir.path(), vec![deleted, kept]); + let filtered = filter_just_files(dir.path(), vec![deleted, kept]); let kept_paths: Vec = filtered .iter() .map(|f| f.display_path().display().to_string()) diff --git a/src/ui/diff_side_by_side.rs b/src/ui/diff_side_by_side.rs index b5572936..df21fd24 100644 --- a/src/ui/diff_side_by_side.rs +++ b/src/ui/diff_side_by_side.rs @@ -1539,10 +1539,9 @@ mod remote_comments_side_by_side_snapshot_tests { use crate::model::{ DiffFile, DiffHunk, DiffLine, FileStatus, LineOrigin, ReviewSession, SessionDiffSource, }; - use crate::syntax::SyntaxHighlighter; use crate::theme::Theme; use crate::ui::render; - use crate::vcs::traits::{VcsBackend, VcsChangeStatus, VcsInfo, VcsType}; + use crate::vcs::traits::{DiffWithJobs, VcsBackend, VcsChangeStatus, VcsInfo, VcsType}; use ratatui::Terminal; use ratatui::backend::TestBackend; use ratatui::buffer::Buffer; @@ -1556,10 +1555,7 @@ mod remote_comments_side_by_side_snapshot_tests { fn info(&self) -> &VcsInfo { &self.info } - fn get_working_tree_diff( - &self, - _highlighter: &SyntaxHighlighter, - ) -> TuicrResult> { + fn get_working_tree_diff(&self) -> TuicrResult { Err(TuicrError::NoChanges) } fn fetch_context_lines( @@ -1683,7 +1679,7 @@ mod remote_comments_side_by_side_snapshot_tests { Theme::dark(), None, false, - vec![sample_diff_file()], + (vec![sample_diff_file()], Vec::new()), session, DiffSource::PullRequest(Box::new(pr)), InputMode::Normal, diff --git a/src/ui/diff_unified.rs b/src/ui/diff_unified.rs index bccb7175..2e6fc30c 100644 --- a/src/ui/diff_unified.rs +++ b/src/ui/diff_unified.rs @@ -1335,10 +1335,9 @@ mod remote_comments_snapshot_tests { use crate::model::{ DiffFile, DiffHunk, DiffLine, FileStatus, LineOrigin, ReviewSession, SessionDiffSource, }; - use crate::syntax::SyntaxHighlighter; use crate::theme::Theme; use crate::ui::render; - use crate::vcs::traits::{VcsBackend, VcsChangeStatus, VcsInfo, VcsType}; + use crate::vcs::traits::{DiffWithJobs, VcsBackend, VcsChangeStatus, VcsInfo, VcsType}; use ratatui::Terminal; use ratatui::backend::TestBackend; use ratatui::buffer::Buffer; @@ -1353,10 +1352,7 @@ mod remote_comments_snapshot_tests { fn info(&self) -> &VcsInfo { &self.info } - fn get_working_tree_diff( - &self, - _highlighter: &SyntaxHighlighter, - ) -> TuicrResult> { + fn get_working_tree_diff(&self) -> TuicrResult { Err(TuicrError::NoChanges) } fn fetch_context_lines( @@ -1504,7 +1500,7 @@ mod remote_comments_snapshot_tests { Theme::dark(), None, false, - vec![sample_diff_file()], + (vec![sample_diff_file()], Vec::new()), session, DiffSource::PullRequest(Box::new(pr)), InputMode::Normal, @@ -1536,7 +1532,7 @@ mod remote_comments_snapshot_tests { Theme::dark(), None, false, - diff_files, + (diff_files, Vec::new()), session, DiffSource::CommitRange(vec!["HEAD".to_string()]), InputMode::Normal, diff --git a/src/ui/selector.rs b/src/ui/selector.rs index 1545e9a0..a8f8f69f 100644 --- a/src/ui/selector.rs +++ b/src/ui/selector.rs @@ -470,12 +470,11 @@ mod selector_render_snapshot_tests { use crate::error::TuicrError; use crate::forge::selector::PullRequestsTab; use crate::forge::traits::{ForgeRepository, PullRequestSummary}; - use crate::model::{DiffFile, DiffLine, FileStatus, ReviewSession, SessionDiffSource}; - use crate::syntax::SyntaxHighlighter; + use crate::model::{DiffLine, FileStatus, ReviewSession, SessionDiffSource}; use crate::theme::Theme; use crate::ui::render; use crate::vcs::CommitInfo; - use crate::vcs::traits::{VcsBackend, VcsChangeStatus, VcsInfo, VcsType}; + use crate::vcs::traits::{DiffWithJobs, VcsBackend, VcsChangeStatus, VcsInfo, VcsType}; use chrono::{TimeZone, Utc}; use ratatui::Terminal; use ratatui::backend::TestBackend; @@ -492,10 +491,7 @@ mod selector_render_snapshot_tests { &self.info } - fn get_working_tree_diff( - &self, - _highlighter: &SyntaxHighlighter, - ) -> TuicrResult> { + fn get_working_tree_diff(&self) -> TuicrResult { Err(TuicrError::NoChanges) } @@ -560,7 +556,7 @@ mod selector_render_snapshot_tests { Theme::dark(), None, false, - Vec::new(), + (Vec::new(), Vec::new()), session, DiffSource::WorkingTree, InputMode::CommitSelect, diff --git a/src/ui/status_bar.rs b/src/ui/status_bar.rs index 2ec1a567..3f5d2f00 100644 --- a/src/ui/status_bar.rs +++ b/src/ui/status_bar.rs @@ -540,10 +540,9 @@ mod pr_header_snapshot_tests { use crate::error::Result as TuicrResult; use crate::error::TuicrError; use crate::forge::traits::{ForgeRepository, PrSessionKey}; - use crate::model::{DiffFile, DiffLine, FileStatus, ReviewSession, SessionDiffSource}; - use crate::syntax::SyntaxHighlighter; + use crate::model::{DiffLine, FileStatus, ReviewSession, SessionDiffSource}; use crate::theme::Theme; - use crate::vcs::traits::{VcsBackend, VcsInfo, VcsType}; + use crate::vcs::traits::{DiffWithJobs, VcsBackend, VcsInfo, VcsType}; use ratatui::Terminal; use ratatui::backend::TestBackend; use ratatui::buffer::Buffer; @@ -556,10 +555,7 @@ mod pr_header_snapshot_tests { fn info(&self) -> &VcsInfo { &self.info } - fn get_working_tree_diff( - &self, - _highlighter: &SyntaxHighlighter, - ) -> TuicrResult> { + fn get_working_tree_diff(&self) -> TuicrResult { Err(TuicrError::NoChanges) } fn fetch_context_lines( @@ -622,7 +618,7 @@ mod pr_header_snapshot_tests { Theme::dark(), None, false, - Vec::new(), + (Vec::new(), Vec::new()), session, DiffSource::PullRequest(Box::new(pr)), InputMode::Normal, diff --git a/src/ui/submit_modals.rs b/src/ui/submit_modals.rs index 2b78c18a..abf429cf 100644 --- a/src/ui/submit_modals.rs +++ b/src/ui/submit_modals.rs @@ -292,10 +292,9 @@ mod tests { use crate::model::ReviewSession; use crate::model::comment::{Comment, CommentType}; use crate::model::diff_types::FileStatus; - use crate::model::{DiffFile, DiffLine, SessionDiffSource}; - use crate::syntax::SyntaxHighlighter; + use crate::model::{DiffLine, SessionDiffSource}; use crate::theme::Theme; - use crate::vcs::traits::{VcsBackend, VcsChangeStatus, VcsInfo, VcsType}; + use crate::vcs::traits::{DiffWithJobs, VcsBackend, VcsChangeStatus, VcsInfo, VcsType}; use ratatui::Terminal; use ratatui::backend::TestBackend; use ratatui::buffer::Buffer; @@ -309,7 +308,7 @@ mod tests { fn info(&self) -> &VcsInfo { &self.info } - fn get_working_tree_diff(&self, _h: &SyntaxHighlighter) -> TuicrResult> { + fn get_working_tree_diff(&self) -> TuicrResult { Err(TuicrError::NoChanges) } fn fetch_context_lines( @@ -374,7 +373,7 @@ mod tests { Theme::dark(), None, false, - Vec::new(), + (Vec::new(), Vec::new()), session, DiffSource::PullRequest(Box::new(pr_source)), InputMode::Normal, diff --git a/src/vcs/diff_parser.rs b/src/vcs/diff_parser.rs index e1555892..5621778c 100644 --- a/src/vcs/diff_parser.rs +++ b/src/vcs/diff_parser.rs @@ -9,7 +9,8 @@ use std::path::PathBuf; use crate::error::{Result, TuicrError}; use crate::model::{DiffFile, DiffHunk, DiffLine, FileStatus, LineOrigin}; -use crate::syntax::{SyntaxHighlighter, needs_full_file_highlight}; +use crate::syntax::{HighlightJob, HighlightJobKind, SyntaxHighlighter, needs_full_file_highlight}; +use crate::vcs::DiffWithJobs; /// Diff format variants for different VCS tools. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -20,32 +21,26 @@ pub enum DiffFormat { GitStyle, } -/// Parse unified diff output into DiffFile structures. -pub fn parse_unified_diff( - diff_text: &str, - format: DiffFormat, - highlighter: &SyntaxHighlighter, -) -> Result> { +/// Parse unified diff output into DiffFile structures plus highlight jobs. +/// DiffLines come back with `highlighted_spans = None`; the streaming worker +/// fills them in. +pub fn parse_unified_diff(diff_text: &str, format: DiffFormat) -> Result { parse_unified_diff_lines( diff_text.lines().map(|line| Ok(Cow::Borrowed(line))), format, - highlighter, ) } -/// Parse unified diff lines into DiffFile structures. +/// Parse unified diff lines into DiffFile structures plus highlight jobs. /// /// This entry point is used by Git's sparse-checkout backend so large diffs can /// be parsed directly from command stdout instead of buffering the whole patch. -pub fn parse_unified_diff_lines<'a, I>( - diff_lines: I, - format: DiffFormat, - highlighter: &SyntaxHighlighter, -) -> Result> +pub fn parse_unified_diff_lines<'a, I>(diff_lines: I, format: DiffFormat) -> Result where I: Iterator>>, { let mut files: Vec = Vec::new(); + let mut jobs: Vec = Vec::new(); let mut lines = diff_lines.peekable(); let header_prefix = match format { @@ -94,15 +89,23 @@ where continue; } - let file_path = new_path.as_ref().or(old_path.as_ref()); - let mut hunks = Vec::new(); + let file_path = new_path.as_ref().or(old_path.as_ref()).cloned(); + let file_idx = files.len(); + let mut hunks: Vec = Vec::new(); // Parse hunks until next file or end while let Some(line) = peek_line(&mut lines)? { if line.starts_with("diff ") { break; } else if line.starts_with("@@") { - if let Some(hunk) = parse_hunk(&mut lines, file_path, highlighter)? { + let hunk_idx = hunks.len(); + if let Some(hunk) = parse_hunk( + &mut lines, + file_idx, + hunk_idx, + file_path.as_deref(), + &mut jobs, + )? { hunks.push(hunk); } } else { @@ -128,7 +131,7 @@ where return Err(TuicrError::NoChanges); } - Ok(files) + Ok((files, jobs)) } fn next_line<'a, I>(lines: &mut std::iter::Peekable) -> Result>> @@ -252,8 +255,10 @@ where fn parse_hunk<'a, I>( lines: &mut std::iter::Peekable, - file_path: Option<&PathBuf>, - highlighter: &SyntaxHighlighter, + file_idx: usize, + hunk_idx: usize, + file_path: Option<&std::path::Path>, + jobs: &mut Vec, ) -> Result> where I: Iterator>>, @@ -326,39 +331,39 @@ where line_numbers.push((old_ln, new_ln)); } - // Apply syntax highlighting by side-specific sequence to keep parser state valid. - // Container grammars skip per-hunk highlighting; the full-file post-pass - // (`enhance_with_full_file_highlight`) overwrites these spans anyway. - let highlight_sequences = - SyntaxHighlighter::split_diff_lines_for_highlighting(&line_contents, &line_origins); - let (old_highlighted_lines, new_highlighted_lines) = match file_path { - Some(path) if !needs_full_file_highlight(path) => ( - highlighter.highlight_file_lines(path, &highlight_sequences.old_lines), - highlighter.highlight_file_lines(path, &highlight_sequences.new_lines), - ), - _ => (None, None), - }; - - // Build DiffLines - let mut diff_lines: Vec = Vec::with_capacity(line_contents.len()); - for (idx, content) in line_contents.into_iter().enumerate() { - let origin = line_origins[idx]; - let (old_lineno, new_lineno) = line_numbers[idx]; - - let highlighted_spans = highlighter.highlighted_line_for_diff_with_background( - old_highlighted_lines.as_deref(), - new_highlighted_lines.as_deref(), - highlight_sequences.old_line_indices[idx], - highlight_sequences.new_line_indices[idx], - origin, - ); + let diff_lines: Vec = line_contents + .iter() + .enumerate() + .map(|(idx, content)| { + let (old_lineno, new_lineno) = line_numbers[idx]; + DiffLine { + origin: line_origins[idx], + content: content.clone(), + old_lineno, + new_lineno, + highlighted_spans: None, + } + }) + .collect(); - diff_lines.push(DiffLine { - origin, - content, - old_lineno, - new_lineno, - highlighted_spans, + // Container grammars skip per-hunk highlighting; the full-file post-pass + // emitted by the backend will fill spans for the whole file. + if let Some(path) = file_path + && !needs_full_file_highlight(path) + { + let sequences = + SyntaxHighlighter::split_diff_lines_for_highlighting(&line_contents, &line_origins); + jobs.push(HighlightJob { + file_idx, + syntax_path: path.to_path_buf(), + kind: HighlightJobKind::Hunk { + hunk_idx, + old_lines: sequences.old_lines, + new_lines: sequences.new_lines, + old_line_indices: sequences.old_line_indices, + new_line_indices: sequences.new_line_indices, + line_origins, + }, }); } @@ -465,11 +470,11 @@ mod tests { #[test] fn should_return_no_changes_for_empty_diff() { assert!(matches!( - parse_unified_diff("", DiffFormat::Hg, &SyntaxHighlighter::default()), + parse_unified_diff("", DiffFormat::Hg), Err(TuicrError::NoChanges) )); assert!(matches!( - parse_unified_diff("", DiffFormat::GitStyle, &SyntaxHighlighter::default()), + parse_unified_diff("", DiffFormat::GitStyle), Err(TuicrError::NoChanges) )); } @@ -531,8 +536,7 @@ mod tests { } "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 1); assert_eq!(result[0].status, FileStatus::Modified); assert_eq!(result[0].hunks.len(), 1); @@ -549,8 +553,7 @@ mod tests { + new "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; let lines = &result[0].hunks[0].lines; assert_eq!(lines[0].content, " old"); @@ -568,8 +571,7 @@ mod tests { +} "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 1); assert_eq!(result[0].status, FileStatus::Added); assert!(result[0].old_path.is_none()); @@ -589,8 +591,7 @@ mod tests { -} "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 1); assert_eq!(result[0].status, FileStatus::Deleted); assert_eq!( @@ -616,8 +617,7 @@ diff -r abc123 file2.rs -remove "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 2); assert_eq!( result[0].new_path.as_ref().unwrap().to_str().unwrap(), @@ -645,8 +645,7 @@ diff -r abc123 file2.rs } "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 1); assert_eq!(result[0].hunks.len(), 2); assert_eq!(result[0].hunks[0].old_start, 1); @@ -665,8 +664,7 @@ rename to new_name.rs +new content "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 1); assert_eq!(result[0].status, FileStatus::Renamed); assert_eq!( @@ -685,8 +683,7 @@ rename to new_name.rs Binary file image.png has changed "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 1); assert!(result[0].is_binary); assert!(result[0].hunks.is_empty()); @@ -700,8 +697,7 @@ rename from old_name.rs rename to new_name.rs "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 1); assert_eq!(result[0].status, FileStatus::Renamed); assert_eq!(result[0].old_path, Some(PathBuf::from("old_name.rs"))); @@ -717,8 +713,7 @@ copy from source.rs copy to dest.rs "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 1); assert_eq!(result[0].status, FileStatus::Copied); assert_eq!(result[0].old_path, Some(PathBuf::from("source.rs"))); @@ -739,8 +734,7 @@ copy to dest.rs +added line "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 1); assert_eq!(result[0].status, FileStatus::Copied); assert_eq!(result[0].old_path, Some(PathBuf::from("source.rs"))); @@ -760,8 +754,7 @@ copy to dest.rs \ No newline at end of file "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; assert_eq!(result.len(), 1); assert_eq!(result[0].hunks[0].lines.len(), 2); } @@ -779,8 +772,7 @@ copy to dest.rs context at 7->8 "#; - let result = - parse_unified_diff(diff, DiffFormat::Hg, &SyntaxHighlighter::default()).unwrap(); + let result = parse_unified_diff(diff, DiffFormat::Hg).unwrap().0; let lines = &result[0].hunks[0].lines; assert_eq!(lines[0].origin, LineOrigin::Context); @@ -817,8 +809,7 @@ copy to dest.rs line2 line3 "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert_eq!(files[0].new_path, Some(PathBuf::from("file.txt"))); assert_eq!(files[0].status, FileStatus::Modified); @@ -835,8 +826,7 @@ copy to dest.rs - old + new "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; let lines = &files[0].hunks[0].lines; assert_eq!(lines[0].content, " old"); @@ -857,8 +847,11 @@ copy to dest.rs ); "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let (mut files, jobs) = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap(); + let highlighter = SyntaxHighlighter::default(); + for update in crate::syntax::streaming::run_blocking(jobs, &highlighter) { + crate::syntax::streaming::apply_update(&mut files, &highlighter, update); + } let lines = &files[0].hunks[0].lines; assert_eq!(lines.len(), 5); @@ -880,8 +873,7 @@ new file mode 100644 +line1 +line2 "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert_eq!(files[0].status, FileStatus::Added); } @@ -896,8 +888,7 @@ deleted file mode 100644 -line1 -line2 "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert_eq!(files[0].status, FileStatus::Deleted); } @@ -909,8 +900,7 @@ deleted file mode 100644 rename from old.txt rename to new.txt "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert_eq!(files[0].status, FileStatus::Renamed); assert_eq!(files[0].old_path, Some(PathBuf::from("old.txt"))); @@ -930,8 +920,7 @@ rename to new.txt -old content +new content "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert_eq!(files[0].status, FileStatus::Renamed); assert_eq!(files[0].old_path, Some(PathBuf::from("old.txt"))); @@ -946,8 +935,7 @@ rename to new.txt copy from source.txt copy to dest.txt "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert_eq!(files[0].status, FileStatus::Copied); assert_eq!(files[0].old_path, Some(PathBuf::from("source.txt"))); @@ -967,8 +955,7 @@ copy to dest.txt original +added line "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert_eq!(files[0].status, FileStatus::Copied); assert_eq!(files[0].old_path, Some(PathBuf::from("source.txt"))); @@ -983,8 +970,7 @@ new file mode 100644 index 0000000000..abc1234567 Binary files /dev/null and b/image.png differ "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert!(files[0].is_binary); assert_eq!(files[0].status, FileStatus::Added); @@ -999,8 +985,7 @@ deleted file mode 100644 index abc1234567..0000000000 Binary files a/image.png and /dev/null differ "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert!(files[0].is_binary); assert_eq!(files[0].status, FileStatus::Deleted); @@ -1014,8 +999,7 @@ Binary files a/image.png and /dev/null differ index abc1234567..def7890123 100644 Binary files a/image.png and b/image.png differ "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert!(files[0].is_binary); assert_eq!(files[0].status, FileStatus::Modified); @@ -1038,8 +1022,7 @@ diff --git a/b.txt b/b.txt -foo +bar "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 2); assert_eq!(files[0].new_path, Some(PathBuf::from("a.txt"))); assert_eq!(files[1].new_path, Some(PathBuf::from("b.txt"))); @@ -1057,8 +1040,7 @@ diff --git a/b.txt b/b.txt +added2 more "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; let hunk = &files[0].hunks[0]; assert_eq!(hunk.lines[0].old_lineno, Some(5)); @@ -1084,8 +1066,7 @@ diff --git a/b.txt b/b.txt new file mode 100644 index 0000000000..e69de29bb2 "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert_eq!(files[0].status, FileStatus::Added); assert!(files[0].old_path.is_none()); @@ -1102,8 +1083,7 @@ index 0000000000..e69de29bb2 old mode 100644 new mode 100755 "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let files = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap().0; assert_eq!(files.len(), 1); assert_eq!(files[0].status, FileStatus::Modified); assert_eq!(files[0].old_path, Some(PathBuf::from("script.sh"))); @@ -1124,8 +1104,7 @@ literal 4 LcmeZB000M*0RR91 "#; - let files = - parse_unified_diff(diff, DiffFormat::GitStyle, &SyntaxHighlighter::default()).unwrap(); + let (files, _) = parse_unified_diff(diff, DiffFormat::GitStyle).unwrap(); assert_eq!(files.len(), 1); assert_eq!(files[0].old_path, Some(PathBuf::from("image.bin"))); @@ -1147,9 +1126,7 @@ LcmeZB000M*0RR91 .into_iter() .map(|line| Ok(Cow::Owned(line.to_string()))); - let files = - parse_unified_diff_lines(lines, DiffFormat::GitStyle, &SyntaxHighlighter::default()) - .unwrap(); + let (files, _) = parse_unified_diff_lines(lines, DiffFormat::GitStyle).unwrap(); assert_eq!(files.len(), 1); assert_eq!(files[0].new_path, Some(PathBuf::from("file.txt"))); diff --git a/src/vcs/file.rs b/src/vcs/file.rs index 76924501..3eabfda2 100644 --- a/src/vcs/file.rs +++ b/src/vcs/file.rs @@ -5,9 +5,9 @@ use ignore::WalkBuilder; use crate::error::{Result, TuicrError}; use crate::model::{DiffFile, DiffHunk, DiffLine, FileStatus, LineOrigin}; -use crate::syntax::SyntaxHighlighter; +use crate::syntax::{HighlightJob, HighlightJobKind, SyntaxHighlighter}; -use super::traits::{VcsBackend, VcsInfo, VcsType}; +use super::traits::{DiffWithJobs, VcsBackend, VcsInfo, VcsType}; /// A backend for reviewing files outside of a VCS repository (`--file`) /// and for the whole-repo `--all-files` mode. @@ -154,12 +154,15 @@ impl FileBackend { }) } + /// Build the [`DiffFile`] entry for one path, plus the data needed for a + /// background highlight job. Returns `None` to skip the file entirely + /// (binary, unreadable, empty). The job kind is `None` for `is_too_large` + /// placeholders that have no hunk to highlight. fn build_diff_file_for_path( &self, - highlighter: &SyntaxHighlighter, abs_path: &Path, file_size: u64, - ) -> Option { + ) -> Option<(DiffFile, Option)> { // Binary check first so a too-large binary is skipped (not surfaced // as a misleading is_too_large text placeholder), and so single-file // mode (which never went through `collect_text_files`) is also guarded. @@ -176,16 +179,19 @@ impl FileBackend { if file_size > MAX_FILE_BYTES { let hunks: Vec = Vec::new(); let content_hash = DiffFile::compute_content_hash(&hunks); - return Some(DiffFile { - old_path: None, - new_path: Some(rel_path), - status: FileStatus::Added, - hunks, - is_binary: false, - is_too_large: true, - is_commit_message: false, - content_hash, - }); + return Some(( + DiffFile { + old_path: None, + new_path: Some(rel_path), + status: FileStatus::Added, + hunks, + is_binary: false, + is_too_large: true, + is_commit_message: false, + content_hash, + }, + None, + )); } let content = std::fs::read_to_string(abs_path).ok()?; @@ -202,26 +208,15 @@ impl FileBackend { // Build line contents and origins for syntax highlighting let line_contents: Vec = lines.iter().map(|l| super::tabify(l)).collect(); let line_origins: Vec = vec![render_origin; line_contents.len()]; - - // Apply syntax highlighting - let highlight_sequences = + let sequences = SyntaxHighlighter::split_diff_lines_for_highlighting(&line_contents, &line_origins); - let new_highlighted_lines = - highlighter.highlight_file_lines(abs_path, &highlight_sequences.new_lines); - // Build DiffLines + // Build DiffLines without inline highlighting; the streaming worker + // will populate `highlighted_spans` once the diff is on screen. let mut diff_lines = Vec::with_capacity(lines.len()); for (i, content) in line_contents.iter().enumerate() { let line_num = (i + 1) as u32; - let highlighted_spans = highlighter.highlighted_line_for_diff_with_background( - None, - new_highlighted_lines.as_deref(), - None, - highlight_sequences.new_line_indices[i], - render_origin, - ); - // Pristine context lines need both old_lineno and new_lineno // populated so the side-by-side and unified renderers walk the // gutter math correctly. @@ -235,7 +230,7 @@ impl FileBackend { content: content.clone(), old_lineno, new_lineno: Some(line_num), - highlighted_spans, + highlighted_spans: None, }); } @@ -267,7 +262,7 @@ impl FileBackend { let hunks = vec![hunk]; let content_hash = DiffFile::compute_content_hash(&hunks); - Some(DiffFile { + let file = DiffFile { old_path: None, new_path: Some(rel_path), status: file_status, @@ -276,7 +271,18 @@ impl FileBackend { is_too_large: false, is_commit_message: false, content_hash, - }) + }; + + let job_kind = HighlightJobKind::Hunk { + hunk_idx: 0, + old_lines: sequences.old_lines, + new_lines: sequences.new_lines, + old_line_indices: sequences.old_line_indices, + new_line_indices: sequences.new_line_indices, + line_origins, + }; + + Some((file, Some(job_kind))) } } @@ -285,18 +291,29 @@ impl VcsBackend for FileBackend { &self.info } - fn get_working_tree_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { - let diff_files: Vec = self - .files - .iter() - .filter_map(|(p, size)| self.build_diff_file_for_path(highlighter, p, *size)) - .collect(); + fn get_working_tree_diff(&self) -> Result { + let mut diff_files: Vec = Vec::with_capacity(self.files.len()); + let mut jobs: Vec = Vec::with_capacity(self.files.len()); + + for (abs_path, size) in &self.files { + let Some((file, job_kind)) = self.build_diff_file_for_path(abs_path, *size) else { + continue; + }; + if let Some(kind) = job_kind { + jobs.push(HighlightJob { + file_idx: diff_files.len(), + syntax_path: abs_path.clone(), + kind, + }); + } + diff_files.push(file); + } if diff_files.is_empty() { return Err(TuicrError::NoChanges); } - Ok(diff_files) + Ok((diff_files, jobs)) } fn fetch_context_lines( @@ -401,11 +418,6 @@ mod tests { use std::fs; use super::*; - use crate::syntax::SyntaxHighlighter; - - fn highlighter() -> SyntaxHighlighter { - SyntaxHighlighter::default() - } #[test] fn single_file_mode_returns_one_diff_file() { @@ -414,9 +426,11 @@ mod tests { fs::write(&path, "alpha\nbeta\n").unwrap(); let backend = FileBackend::new(path.to_str().unwrap()).unwrap(); - let diffs = backend.get_working_tree_diff(&highlighter()).unwrap(); + let (diffs, jobs) = backend.get_working_tree_diff().unwrap(); assert_eq!(diffs.len(), 1); + assert_eq!(jobs.len(), 1); + assert_eq!(jobs[0].file_idx, 0); assert_eq!( diffs[0].new_path.as_deref().unwrap(), Path::new("hello.txt") @@ -432,7 +446,7 @@ mod tests { fs::write(dir.path().join("ignored.txt"), "skip me\n").unwrap(); let backend = FileBackend::new(dir.path().to_str().unwrap()).unwrap(); - let diffs = backend.get_working_tree_diff(&highlighter()).unwrap(); + let (diffs, _jobs) = backend.get_working_tree_diff().unwrap(); let names: Vec<_> = diffs .iter() @@ -459,7 +473,7 @@ mod tests { fs::write(sub.join("inner.txt"), "x\n").unwrap(); let backend = FileBackend::new(dir.path().to_str().unwrap()).unwrap(); - let diffs = backend.get_working_tree_diff(&highlighter()).unwrap(); + let (diffs, _jobs) = backend.get_working_tree_diff().unwrap(); assert_eq!(diffs.len(), 1); assert_eq!( @@ -481,7 +495,7 @@ mod tests { .unwrap(); let root = dir.path().canonicalize().unwrap(); let backend = FileBackend::new_pristine(vec![path.clone()], root).unwrap(); - let diffs = backend.get_working_tree_diff(&highlighter()).unwrap(); + let (diffs, _jobs) = backend.get_working_tree_diff().unwrap(); assert_eq!(diffs.len(), 1); let hunk = &diffs[0].hunks[0]; @@ -504,7 +518,7 @@ mod tests { fs::write(dir.path().join("a.txt"), "alpha\nbeta\n").unwrap(); let backend = FileBackend::new(dir.path().to_str().unwrap()).unwrap(); - let diffs = backend.get_working_tree_diff(&highlighter()).unwrap(); + let (diffs, _jobs) = backend.get_working_tree_diff().unwrap(); assert_eq!(diffs.len(), 1); let hunk = &diffs[0].hunks[0]; @@ -524,7 +538,7 @@ mod tests { let root = dir.path().canonicalize().unwrap(); let backend = FileBackend::new_pristine(vec![text_path.clone(), bin_path], root).unwrap(); - let diffs = backend.get_working_tree_diff(&highlighter()).unwrap(); + let (diffs, _jobs) = backend.get_working_tree_diff().unwrap(); assert_eq!(diffs.len(), 1); assert_eq!(diffs[0].new_path.as_deref().unwrap(), Path::new("text.txt")); diff --git a/src/vcs/git/cli.rs b/src/vcs/git/cli.rs index bb2531d9..3c647289 100644 --- a/src/vcs/git/cli.rs +++ b/src/vcs/git/cli.rs @@ -10,13 +10,13 @@ use chrono::{TimeZone, Utc}; use crate::error::{Result, TuicrError}; use crate::model::{DiffFile, DiffHunk, DiffLine, FileStatus, LineOrigin, LineSide}; -use crate::syntax::SyntaxHighlighter; +use crate::syntax::{HighlightJob, HighlightJobKind}; use crate::vcs::diff_parser::{self, DiffFormat}; use crate::vcs::{ - ChangeKind, CommitInfo, DiffWhitespaceMode, ResolvedRevisionRange, RevisionDiffTarget, - VcsBackend, VcsChangeStatus, VcsInfo, + ChangeKind, CommitInfo, DiffWhitespaceMode, DiffWithJobs, ResolvedRevisionRange, + RevisionDiffTarget, VcsBackend, VcsChangeStatus, VcsInfo, }; -use crate::vcs::{container_file_paths, enhance_with_full_file_highlight, tabify}; +use crate::vcs::{append_container_full_file_jobs, container_file_paths, tabify}; use super::{ GitRepoMode, RevisionExpression, git_bool_config_enabled, git_command_error, @@ -89,19 +89,18 @@ impl GitCliBackend { include_untracked: bool, old_source: GitContentSource<'_>, new_source: GitContentSource<'_>, - highlighter: &SyntaxHighlighter, - ) -> Result> { + ) -> Result { if self.whitespace_mode.ignores_all() { args.insert(1, "--ignore-all-space".to_string()); } - let mut files = match run_git_diff_command(&self.root_path, args, highlighter) { - Ok(files) => files, - Err(TuicrError::NoChanges) => Vec::new(), + let (mut files, mut jobs) = match run_git_diff_command(&self.root_path, args) { + Ok(diff) => diff, + Err(TuicrError::NoChanges) => (Vec::new(), Vec::new()), Err(err) => return Err(err), }; if include_untracked { - append_untracked_cli_diffs(&self.root_path, &mut files, highlighter)?; + append_untracked_cli_diffs(&self.root_path, &mut files, &mut jobs)?; } normalize_git_cli_paths(&mut files); @@ -113,9 +112,10 @@ impl GitCliBackend { git_source_content_cache(&self.root_path, old_source, &files, LineSide::Old); let new_cache = git_source_content_cache(&self.root_path, new_source, &files, LineSide::New); - enhance_with_full_file_highlight( - &mut files, - highlighter, + + append_container_full_file_jobs( + &files, + &mut jobs, |path| { read_path_from_git_source_cached( &self.root_path, @@ -133,7 +133,7 @@ impl GitCliBackend { ) }, ); - Ok(files) + Ok((files, jobs)) } fn read_file_content( @@ -187,17 +187,16 @@ impl VcsBackend for GitCliBackend { true } - fn get_working_tree_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { self.get_cli_diff( strings(["diff", "--no-ext-diff", "--binary", "HEAD", "--"]), true, GitContentSource::Revision("HEAD"), GitContentSource::Workdir, - highlighter, ) } - fn get_staged_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { + fn get_staged_diff(&self) -> Result { let old_source = if run_git_command(&self.root_path, &["rev-parse", "--verify", "HEAD"]).is_ok() { GitContentSource::Revision("HEAD") @@ -209,17 +208,15 @@ impl VcsBackend for GitCliBackend { false, old_source, GitContentSource::Index, - highlighter, ) } - fn get_unstaged_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { + fn get_unstaged_diff(&self) -> Result { self.get_cli_diff( strings(["diff", "--no-ext-diff", "--binary", "--"]), true, GitContentSource::Index, GitContentSource::Workdir, - highlighter, ) } @@ -326,8 +323,7 @@ impl VcsBackend for GitCliBackend { fn get_commit_range_diff( &self, revision_range: &ResolvedRevisionRange<'_>, - highlighter: &SyntaxHighlighter, - ) -> Result> { + ) -> Result { if revision_range.commit_ids.is_empty() { return Err(TuicrError::NoChanges); } @@ -354,7 +350,6 @@ impl VcsBackend for GitCliBackend { false, GitContentSource::Revision(&base_rev), GitContentSource::Revision(&newest_rev), - highlighter, ) } @@ -375,11 +370,7 @@ impl VcsBackend for GitCliBackend { Ok(parse_commit_records(&output, &branch_tip_names)) } - fn get_working_tree_with_commits_diff( - &self, - commit_ids: &[String], - highlighter: &SyntaxHighlighter, - ) -> Result> { + fn get_working_tree_with_commits_diff(&self, commit_ids: &[String]) -> Result { if commit_ids.is_empty() { return Err(TuicrError::NoChanges); } @@ -396,7 +387,6 @@ impl VcsBackend for GitCliBackend { true, GitContentSource::Revision(&base_rev), GitContentSource::Workdir, - highlighter, ) } @@ -578,11 +568,7 @@ fn has_untracked_changes(workdir: &Path, pathspecs: &[String]) -> Result { Ok(false) } -fn run_git_diff_command( - workdir: &Path, - args: Vec, - highlighter: &SyntaxHighlighter, -) -> Result> { +fn run_git_diff_command(workdir: &Path, args: Vec) -> Result { let mut child = Command::new("git") .current_dir(workdir) .args(&args) @@ -608,8 +594,7 @@ fn run_git_diff_command( let diff_lines = BufReader::new(stdout) .lines() .map(|line| line.map(Cow::Owned).map_err(TuicrError::from)); - let parse_result = - diff_parser::parse_unified_diff_lines(diff_lines, DiffFormat::GitStyle, highlighter); + let parse_result = diff_parser::parse_unified_diff_lines(diff_lines, DiffFormat::GitStyle); let status = child.wait()?; let stderr = stderr_reader @@ -630,16 +615,19 @@ fn run_git_diff_command( fn append_untracked_cli_diffs( workdir: &Path, files: &mut Vec, - highlighter: &SyntaxHighlighter, + jobs: &mut Vec, ) -> Result { let pathspecs = sparse_checkout_untracked_pathspecs(workdir)?; let previous_len = files.len(); for_each_untracked_path(workdir, &pathspecs, |path| { let full_path = workdir.join(&path); - let Some(file) = build_untracked_diff_file(&path, &full_path, highlighter) else { + let Some((file, job)) = build_untracked_diff_file(&path, &full_path, files.len()) else { return Ok(()); }; files.push(file); + if let Some(job) = job { + jobs.push(job); + } Ok(()) })?; Ok(files.len().saturating_sub(previous_len)) @@ -690,16 +678,16 @@ fn is_simple_sparse_path(pattern: &str) -> bool { fn build_untracked_diff_file( path: &Path, full_path: &Path, - highlighter: &SyntaxHighlighter, -) -> Option { + file_idx: usize, +) -> Option<(DiffFile, Option)> { let metadata = full_path.metadata().ok()?; if metadata.len() > MAX_UNTRACKED_FILE_SIZE { - return Some(diff_file_without_hunks(path, false, true)); + return Some((diff_file_without_hunks(path, false, true), None)); } let bytes = fs::read(full_path).ok()?; if bytes.contains(&0) { - return Some(diff_file_without_hunks(path, true, false)); + return Some((diff_file_without_hunks(path, true, false), None)); } let content = String::from_utf8_lossy(&bytes); @@ -709,10 +697,30 @@ fn build_untracked_diff_file( .collect(); if lines.is_empty() { - return Some(diff_file_without_hunks(path, false, false)); - } + return Some((diff_file_without_hunks(path, false, false), None)); + } + + let line_count = lines.len(); + let line_origins = vec![LineOrigin::Addition; line_count]; + let new_line_indices: Vec> = (0..line_count).map(Some).collect(); + // Untracked files surface as a single all-additions hunk; emit a per-hunk + // highlight job mirroring what `parse_unified_diff_lines` would have + // produced. Container grammars (Vue/Svelte/...) will be picked up by the + // FullFile pass in `get_cli_diff`, so always emit the Hunk job here and + // let the worker drop it if the syntax set has no grammar for the path. + let job = HighlightJob { + file_idx, + syntax_path: path.to_path_buf(), + kind: HighlightJobKind::Hunk { + hunk_idx: 0, + old_lines: Vec::new(), + new_lines: lines.clone(), + old_line_indices: vec![None; line_count], + new_line_indices, + line_origins, + }, + }; - let highlighted = highlighter.highlight_file_lines(path, &lines); let diff_lines: Vec = lines .into_iter() .enumerate() @@ -721,13 +729,7 @@ fn build_untracked_diff_file( content, old_lineno: None, new_lineno: Some((idx + 1) as u32), - highlighted_spans: highlighter.highlighted_line_for_diff_with_background( - None, - highlighted.as_deref(), - None, - Some(idx), - LineOrigin::Addition, - ), + highlighted_spans: None, }) .collect(); @@ -742,16 +744,19 @@ fn build_untracked_diff_file( }]; let content_hash = DiffFile::compute_content_hash(&hunks); - Some(DiffFile { - old_path: None, - new_path: Some(path.to_path_buf()), - status: FileStatus::Added, - hunks, - is_binary: false, - is_too_large: false, - is_commit_message: false, - content_hash, - }) + Some(( + DiffFile { + old_path: None, + new_path: Some(path.to_path_buf()), + status: FileStatus::Added, + hunks, + is_binary: false, + is_too_large: false, + is_commit_message: false, + content_hash, + }, + Some(job), + )) } fn diff_file_without_hunks(path: &Path, is_binary: bool, is_too_large: bool) -> DiffFile { @@ -1199,10 +1204,9 @@ mod tests { fs::remove_file(workdir.join(path)).expect("failed to remove file"); } - fn summarize_files( - files: Vec, - ) -> Vec<(Option, Option, FileStatus)> { - let mut summary: Vec<_> = files + fn summarize_files(diff: DiffWithJobs) -> Vec<(Option, Option, FileStatus)> { + let mut summary: Vec<_> = diff + .0 .into_iter() .map(|file| (file.old_path, file.new_path, file.status)) .collect(); @@ -1395,14 +1399,11 @@ mod tests { fn reads_commit_range_diff_in_sparse_index() { let (_temp_dir, backend, ids) = setup_sparse_index_repo(); - let files = backend - .get_commit_range_diff( - &ResolvedRevisionRange::from_owned_commit_ids( - vec![ids[1].clone()], - RevisionDiffTarget::CommitList, - ), - &SyntaxHighlighter::default(), - ) + let (files, _) = backend + .get_commit_range_diff(&ResolvedRevisionRange::from_owned_commit_ids( + vec![ids[1].clone()], + RevisionDiffTarget::CommitList, + )) .expect("failed to get sparse commit range diff"); assert_eq!(files.len(), 1); @@ -1417,7 +1418,7 @@ mod tests { let (_temp_dir, backend, _ids) = setup_sparse_index_repo(); assert!(matches!( - backend.get_working_tree_diff(&SyntaxHighlighter::default()), + backend.get_working_tree_diff(), Err(TuicrError::NoChanges) )); } @@ -1430,8 +1431,8 @@ mod tests { write_file(workdir, "keep/new.txt", "new sparse file\n"); write_file(workdir, "hidden/outside.txt", "outside cone\n"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) + let (files, _) = backend + .get_working_tree_diff() .expect("failed to get sparse working tree diff"); let paths: Vec<_> = files @@ -1452,8 +1453,8 @@ mod tests { backend .stage_file(Path::new("keep/file.txt")) .expect("failed to stage file"); - let files = backend - .get_staged_diff(&SyntaxHighlighter::default()) + let (files, _) = backend + .get_staged_diff() .expect("failed to get sparse staged diff"); assert_eq!(files.len(), 1); @@ -1514,37 +1515,28 @@ mod tests { #[test] fn cli_diff_outputs_match_libgit2_for_shared_git_operations() { let (_temp_dir, cli_backend, repo, ids) = setup_standard_parity_repo(); - let highlighter = SyntaxHighlighter::default(); assert_eq!( - summarize_files(cli_backend.get_working_tree_diff(&highlighter).unwrap()), + summarize_files(cli_backend.get_working_tree_diff().unwrap()), summarize_files( - diff::get_working_tree_diff(&repo, DiffWhitespaceMode::Normal, &highlighter) - .unwrap() + diff::get_working_tree_diff(&repo, DiffWhitespaceMode::Normal).unwrap() ) ); assert_eq!( - summarize_files(cli_backend.get_staged_diff(&highlighter).unwrap()), - summarize_files( - diff::get_staged_diff(&repo, DiffWhitespaceMode::Normal, &highlighter).unwrap() - ) + summarize_files(cli_backend.get_staged_diff().unwrap()), + summarize_files(diff::get_staged_diff(&repo, DiffWhitespaceMode::Normal).unwrap()) ); assert_eq!( - summarize_files(cli_backend.get_unstaged_diff(&highlighter).unwrap()), - summarize_files( - diff::get_unstaged_diff(&repo, DiffWhitespaceMode::Normal, &highlighter).unwrap() - ) + summarize_files(cli_backend.get_unstaged_diff().unwrap()), + summarize_files(diff::get_unstaged_diff(&repo, DiffWhitespaceMode::Normal).unwrap()) ); assert_eq!( summarize_files( cli_backend - .get_commit_range_diff( - &ResolvedRevisionRange::from_owned_commit_ids( - vec![ids[1].clone()], - RevisionDiffTarget::CommitList, - ), - &highlighter - ) + .get_commit_range_diff(&ResolvedRevisionRange::from_owned_commit_ids( + vec![ids[1].clone()], + RevisionDiffTarget::CommitList, + )) .unwrap() ), summarize_files( @@ -1555,7 +1547,6 @@ mod tests { RevisionDiffTarget::CommitList, ), DiffWhitespaceMode::Normal, - &highlighter, ) .unwrap() ) @@ -1563,7 +1554,7 @@ mod tests { assert_eq!( summarize_files( cli_backend - .get_working_tree_with_commits_diff(&[ids[1].clone()], &highlighter) + .get_working_tree_with_commits_diff(&[ids[1].clone()]) .unwrap() ), summarize_files( @@ -1571,7 +1562,6 @@ mod tests { &repo, &[ids[1].clone()], DiffWhitespaceMode::Normal, - &highlighter, ) .unwrap() ) @@ -1616,7 +1606,6 @@ mod tests { // endpoints, not the first selected commit's parent. // Otherwise changes already present in the left ref appear in review. let (_temp_dir, cli_backend, repo, left_id, right_id) = setup_merge_range_repo(); - let highlighter = SyntaxHighlighter::default(); let revisions = format!("{left_id}..{right_id}"); let cli_range = cli_backend @@ -1630,7 +1619,7 @@ mod tests { } ); let cli_files = cli_backend - .get_commit_range_diff(&cli_range, &highlighter) + .get_commit_range_diff(&cli_range) .expect("failed to get cli range diff"); let libgit2_range = @@ -1642,13 +1631,9 @@ mod tests { head: right_id, } ); - let libgit2_files = diff::get_commit_range_diff( - &repo, - &libgit2_range, - DiffWhitespaceMode::Normal, - &highlighter, - ) - .expect("failed to get libgit2 range diff"); + let libgit2_files = + diff::get_commit_range_diff(&repo, &libgit2_range, DiffWhitespaceMode::Normal) + .expect("failed to get libgit2 range diff"); assert_eq!( summarize_files(cli_files), @@ -1686,13 +1671,13 @@ mod tests { write_file(workdir, "file.txt", " alpha \n beta\n"); assert!(matches!( - backend.get_working_tree_diff(&SyntaxHighlighter::default()), + backend.get_working_tree_diff(), Err(TuicrError::NoChanges) )); write_file(workdir, "file.txt", " alpha \ngamma\n"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) + let (files, _) = backend + .get_working_tree_diff() .expect("non-whitespace edit should still produce a diff"); assert_eq!(files.len(), 1); } diff --git a/src/vcs/git/diff.rs b/src/vcs/git/diff.rs index 4bf448c7..7ac86e21 100644 --- a/src/vcs/git/diff.rs +++ b/src/vcs/git/diff.rs @@ -3,17 +3,16 @@ use std::path::{Path, PathBuf}; use crate::error::{Result, TuicrError}; use crate::model::{DiffFile, DiffHunk, DiffLine, FileStatus, LineOrigin}; -use crate::syntax::{SyntaxHighlighter, needs_full_file_highlight}; +use crate::syntax::{HighlightJob, HighlightJobKind, SyntaxHighlighter, needs_full_file_highlight}; use crate::vcs::traits::{ ChangeKind, DiffWhitespaceMode, ResolvedRevisionRange, RevisionDiffTarget, }; -use crate::vcs::{enhance_with_full_file_highlight, tabify}; +use crate::vcs::{DiffWithJobs, append_container_full_file_jobs, tabify}; pub fn get_working_tree_diff( repo: &Repository, whitespace_mode: DiffWhitespaceMode, - highlighter: &SyntaxHighlighter, -) -> Result> { +) -> Result { // Unborn HEAD (fresh `git init` / `git clone` of an empty remote) has no // tree to compare against; diff against an empty baseline so freshly // staged/added files still surface in the working-tree review. @@ -25,17 +24,17 @@ pub fn get_working_tree_diff( opts.recurse_untracked_dirs(true); let diff = repo.diff_tree_to_workdir_with_index(head.as_ref(), Some(&mut opts))?; - let mut files = parse_diff(&diff, highlighter)?; - enhance_with_full_file_highlight( - &mut files, - highlighter, + let (files, mut jobs) = parse_diff(&diff)?; + append_container_full_file_jobs( + &files, + &mut jobs, |path| { head.as_ref() .and_then(|tree| read_path_from_tree(repo, tree, path)) }, |path| read_path_from_workdir(repo, path), ); - Ok(files) + Ok((files, jobs)) } /// Get the staged diff (index vs HEAD) @@ -43,27 +42,25 @@ pub fn get_working_tree_diff( pub fn get_staged_diff( repo: &Repository, whitespace_mode: DiffWhitespaceMode, - highlighter: &SyntaxHighlighter, -) -> Result> { +) -> Result { let head = repo.head().ok().and_then(|h| h.peel_to_tree().ok()); let index = repo.index()?; let mut opts = diff_options(whitespace_mode); let diff = repo.diff_tree_to_index(head.as_ref(), Some(&index), Some(&mut opts))?; - let mut files = parse_diff(&diff, highlighter)?; - enhance_with_full_file_highlight( - &mut files, - highlighter, + let (files, mut jobs) = parse_diff(&diff)?; + append_container_full_file_jobs( + &files, + &mut jobs, |path| { head.as_ref() .and_then(|tree| read_path_from_tree(repo, tree, path)) }, |path| read_path_from_index(repo, &index, path), ); - Ok(files) + Ok((files, jobs)) } -/// Get the unstaged diff (working tree vs index) /// List the changed paths for either the staged or unstaged diff, without /// materializing hunks or running the syntax highlighter. Lets callers verify /// `get_change_status` against ignore rules cheaply — full diff parsing only @@ -103,11 +100,11 @@ pub fn list_changed_paths(repo: &Repository, kind: ChangeKind) -> Result Result> { +) -> Result { let index = repo.index()?; let mut opts = diff_options(whitespace_mode); opts.include_untracked(true); @@ -115,14 +112,14 @@ pub fn get_unstaged_diff( opts.recurse_untracked_dirs(true); let diff = repo.diff_index_to_workdir(Some(&index), Some(&mut opts))?; - let mut files = parse_diff(&diff, highlighter)?; - enhance_with_full_file_highlight( - &mut files, - highlighter, + let (files, mut jobs) = parse_diff(&diff)?; + append_container_full_file_jobs( + &files, + &mut jobs, |path| read_path_from_index(repo, &index, path), |path| read_path_from_workdir(repo, path), ); - Ok(files) + Ok((files, jobs)) } /// Get the diff for a range of commits. @@ -132,8 +129,7 @@ pub fn get_commit_range_diff( repo: &Repository, revision_range: &ResolvedRevisionRange<'_>, whitespace_mode: DiffWhitespaceMode, - highlighter: &SyntaxHighlighter, -) -> Result> { +) -> Result { let (old_tree, new_tree) = match &revision_range.diff_target { RevisionDiffTarget::CommitList => { commit_list_range_trees(repo, &revision_range.commit_ids)? @@ -148,7 +144,7 @@ pub fn get_commit_range_diff( } }; - diff_commit_trees(repo, old_tree, new_tree, whitespace_mode, highlighter) + diff_commit_trees(repo, old_tree, new_tree, whitespace_mode) } fn commit_list_range_trees<'repo>( @@ -186,15 +182,14 @@ fn diff_commit_trees( old_tree: Option>, new_tree: git2::Tree<'_>, whitespace_mode: DiffWhitespaceMode, - highlighter: &SyntaxHighlighter, -) -> Result> { +) -> Result { let mut opts = diff_options(whitespace_mode); let diff = repo.diff_tree_to_tree(old_tree.as_ref(), Some(&new_tree), Some(&mut opts))?; - let mut files = parse_diff(&diff, highlighter)?; - enhance_with_full_file_highlight( - &mut files, - highlighter, + let (files, mut jobs) = parse_diff(&diff)?; + append_container_full_file_jobs( + &files, + &mut jobs, |path| { old_tree .as_ref() @@ -202,7 +197,7 @@ fn diff_commit_trees( }, |path| read_path_from_tree(repo, &new_tree, path), ); - Ok(files) + Ok((files, jobs)) } /// Get a combined diff from the parent of the oldest commit through to the working tree. @@ -211,8 +206,7 @@ pub fn get_working_tree_with_commits_diff( repo: &Repository, commit_ids: &[String], whitespace_mode: DiffWhitespaceMode, - highlighter: &SyntaxHighlighter, -) -> Result> { +) -> Result { if commit_ids.is_empty() { return Err(TuicrError::NoChanges); } @@ -232,10 +226,10 @@ pub fn get_working_tree_with_commits_diff( opts.recurse_untracked_dirs(true); let diff = repo.diff_tree_to_workdir_with_index(old_tree.as_ref(), Some(&mut opts))?; - let mut files = parse_diff(&diff, highlighter)?; - enhance_with_full_file_highlight( - &mut files, - highlighter, + let (files, mut jobs) = parse_diff(&diff)?; + append_container_full_file_jobs( + &files, + &mut jobs, |path| { old_tree .as_ref() @@ -243,7 +237,7 @@ pub fn get_working_tree_with_commits_diff( }, |path| read_path_from_workdir(repo, path), ); - Ok(files) + Ok((files, jobs)) } fn diff_options(whitespace_mode: DiffWhitespaceMode) -> DiffOptions { @@ -268,11 +262,12 @@ fn read_path_from_index(repo: &Repository, index: &git2::Index, path: &Path) -> Some(String::from_utf8_lossy(blob.content()).into_owned()) } -fn parse_diff(diff: &Diff, highlighter: &SyntaxHighlighter) -> Result> { +fn parse_diff(diff: &Diff) -> Result { let mut files: Vec = Vec::new(); + let mut jobs: Vec = Vec::new(); // Untracked files larger than this are shown in the file list but their - // content is not parsed — they are likely logs, dumps, or build artefacts. + // content is not parsed - they are likely logs, dumps, or build artefacts. const MAX_UNTRACKED_FILE_SIZE: u64 = 10 * 1_024 * 1_024; for (delta_idx, delta) in diff.deltas().enumerate() { @@ -292,10 +287,11 @@ fn parse_diff(diff: &Diff, highlighter: &SyntaxHighlighter) -> Result MAX_UNTRACKED_FILE_SIZE; let syntax_path = new_path.as_ref().or(old_path.as_ref()).map(|p| p.as_path()); + let file_idx = files.len(); let hunks = if is_binary || is_too_large { Vec::new() } else { - parse_hunks(diff, delta_idx, highlighter, syntax_path)? + parse_hunks(diff, delta_idx, file_idx, syntax_path, &mut jobs)? }; let content_hash = DiffFile::compute_content_hash(&hunks); @@ -315,14 +311,15 @@ fn parse_diff(diff: &Diff, highlighter: &SyntaxHighlighter) -> Result, + jobs: &mut Vec, ) -> Result> { let mut hunks: Vec = Vec::new(); @@ -360,37 +357,38 @@ fn parse_hunks( line_numbers.push((line.old_lineno(), line.new_lineno())); } - let sequences = - SyntaxHighlighter::split_diff_lines_for_highlighting(&line_contents, &line_origins); - // Container grammars skip per-hunk highlighting; the full-file - // post-pass overwrites these spans anyway. - let (old_highlighted, new_highlighted) = match file_path { - Some(path) if !needs_full_file_highlight(path) => ( - highlighter.highlight_file_lines(path, &sequences.old_lines), - highlighter.highlight_file_lines(path, &sequences.new_lines), - ), - _ => (None, None), - }; - let mut lines: Vec = Vec::with_capacity(line_contents.len()); - for (idx, content) in line_contents.into_iter().enumerate() { - let origin = line_origins[idx]; + for (idx, content) in line_contents.iter().enumerate() { let (old_lineno, new_lineno) = line_numbers[idx]; - - let highlighted_spans = highlighter.highlighted_line_for_diff_with_background( - old_highlighted.as_deref(), - new_highlighted.as_deref(), - sequences.old_line_indices[idx], - sequences.new_line_indices[idx], - origin, - ); - lines.push(DiffLine { - origin, - content, + origin: line_origins[idx], + content: content.clone(), old_lineno, new_lineno, - highlighted_spans, + highlighted_spans: None, + }); + } + + // Container grammars skip per-hunk highlighting; the full-file + // post-pass will fill spans for the whole file in one shot. + if let Some(path) = file_path + && !needs_full_file_highlight(path) + { + let sequences = SyntaxHighlighter::split_diff_lines_for_highlighting( + &line_contents, + &line_origins, + ); + jobs.push(HighlightJob { + file_idx, + syntax_path: path.to_path_buf(), + kind: HighlightJobKind::Hunk { + hunk_idx, + old_lines: sequences.old_lines, + new_lines: sequences.new_lines, + old_line_indices: sequences.old_line_indices, + new_line_indices: sequences.new_line_indices, + line_origins, + }, }); } @@ -440,9 +438,8 @@ mod tests { let diff = repo .diff_tree_to_tree(Some(&head), Some(&head), None) .unwrap(); - let highlighter = SyntaxHighlighter::default(); - let result = parse_diff(&diff, &highlighter); + let result = parse_diff(&diff); assert!(matches!(result, Err(TuicrError::NoChanges))); } @@ -464,12 +461,8 @@ mod tests { ) .expect("failed to update file"); - let files = get_working_tree_diff( - &repo, - DiffWhitespaceMode::Normal, - &SyntaxHighlighter::default(), - ) - .expect("failed to get diff"); + let (files, _jobs) = + get_working_tree_diff(&repo, DiffWhitespaceMode::Normal).expect("failed to get diff"); assert_eq!(files.len(), 1); let lines = &files[0].hunks[0].lines; @@ -492,14 +485,17 @@ mod tests { let edited = "\n\n\n"; fs::write(temp_dir.path().join("App.vue"), edited).expect("failed to update file"); - let files = get_working_tree_diff( - &repo, - DiffWhitespaceMode::Normal, - &SyntaxHighlighter::default(), - ) - .expect("failed to get diff"); + let (mut files, jobs) = + get_working_tree_diff(&repo, DiffWhitespaceMode::Normal).expect("failed to get diff"); assert_eq!(files.len(), 1); + // Drive the streaming highlight pipeline synchronously so we can + // assert on the populated spans. + let highlighter = SyntaxHighlighter::default(); + for update in crate::syntax::streaming::run_blocking(jobs, &highlighter) { + crate::syntax::streaming::apply_update(&mut files, &highlighter, update); + } + let changed_lines: Vec<_> = files[0].hunks[0] .lines .iter() @@ -530,13 +526,11 @@ mod tests { fs::write(temp_dir.path().join("file.txt"), "unstaged\n").expect("failed to update file"); - let highlighter = SyntaxHighlighter::default(); - - let unstaged = get_unstaged_diff(&repo, DiffWhitespaceMode::Normal, &highlighter) - .expect("unstaged diff failed"); - assert_eq!(unstaged.len(), 1); + let (unstaged_files, _) = + get_unstaged_diff(&repo, DiffWhitespaceMode::Normal).expect("unstaged diff failed"); + assert_eq!(unstaged_files.len(), 1); assert!(matches!( - get_staged_diff(&repo, DiffWhitespaceMode::Normal, &highlighter), + get_staged_diff(&repo, DiffWhitespaceMode::Normal), Err(TuicrError::NoChanges) )); @@ -546,11 +540,11 @@ mod tests { .expect("failed to add file to index"); index.write().expect("failed to write index"); - let staged = get_staged_diff(&repo, DiffWhitespaceMode::Normal, &highlighter) - .expect("staged diff failed"); - assert_eq!(staged.len(), 1); + let (staged_files, _) = + get_staged_diff(&repo, DiffWhitespaceMode::Normal).expect("staged diff failed"); + assert_eq!(staged_files.len(), 1); assert!(matches!( - get_unstaged_diff(&repo, DiffWhitespaceMode::Normal, &highlighter), + get_unstaged_diff(&repo, DiffWhitespaceMode::Normal), Err(TuicrError::NoChanges) )); } @@ -567,12 +561,8 @@ mod tests { index.write().expect("write index"); // when - let files = get_working_tree_diff( - &repo, - DiffWhitespaceMode::Normal, - &SyntaxHighlighter::default(), - ) - .expect("unborn HEAD should produce a diff against an empty tree"); + let (files, _jobs) = get_working_tree_diff(&repo, DiffWhitespaceMode::Normal) + .expect("unborn HEAD should produce a diff against an empty tree"); // then the staged file shows up as an addition rather than crashing // with `reference 'refs/heads/main' not found` @@ -590,24 +580,16 @@ mod tests { fs::write(temp_dir.path().join("file.txt"), " alpha \n beta\n") .expect("failed to update file"); - let files = get_working_tree_diff( - &repo, - DiffWhitespaceMode::IgnoreAll, - &SyntaxHighlighter::default(), - ) - .expect("whitespace-only edit may surface as a no-op diff file"); + let (files, _) = get_working_tree_diff(&repo, DiffWhitespaceMode::IgnoreAll) + .expect("whitespace-only edit may surface as a no-op diff file"); assert_eq!(files.len(), 1); assert!(files[0].hunks.is_empty()); fs::write(temp_dir.path().join("file.txt"), " alpha \ngamma\n") .expect("failed to update file"); - let files = get_working_tree_diff( - &repo, - DiffWhitespaceMode::IgnoreAll, - &SyntaxHighlighter::default(), - ) - .expect("non-whitespace edit should still produce a diff"); + let (files, _) = get_working_tree_diff(&repo, DiffWhitespaceMode::IgnoreAll) + .expect("non-whitespace edit should still produce a diff"); assert_eq!(files.len(), 1); } @@ -627,12 +609,8 @@ mod tests { permissions.set_mode(0o755); fs::set_permissions(path, permissions).expect("failed to update mode"); - let files = get_working_tree_diff( - &repo, - DiffWhitespaceMode::IgnoreAll, - &SyntaxHighlighter::default(), - ) - .expect("mode-only edit should still produce a diff"); + let (files, _) = get_working_tree_diff(&repo, DiffWhitespaceMode::IgnoreAll) + .expect("mode-only edit should still produce a diff"); assert_eq!(files.len(), 1); assert!(files[0].hunks.is_empty()); } diff --git a/src/vcs/git/libgit2.rs b/src/vcs/git/libgit2.rs index 6c72791b..bb2e7e74 100644 --- a/src/vcs/git/libgit2.rs +++ b/src/vcs/git/libgit2.rs @@ -3,12 +3,12 @@ use std::path::{Path, PathBuf}; use std::sync::Once; use crate::error::{Result, TuicrError}; -use crate::model::{DiffFile, DiffLine, FileStatus}; -use crate::syntax::SyntaxHighlighter; +use crate::model::{DiffLine, FileStatus}; use super::{context, diff, repository, staging}; use crate::vcs::traits::{ - ChangeKind, CommitInfo, DiffWhitespaceMode, ResolvedRevisionRange, VcsBackend, VcsInfo, VcsType, + ChangeKind, CommitInfo, DiffWhitespaceMode, DiffWithJobs, ResolvedRevisionRange, VcsBackend, + VcsInfo, VcsType, }; /// Git backend implementation using the git2/libgit2 library. @@ -99,16 +99,16 @@ impl VcsBackend for Libgit2Backend { false } - fn get_working_tree_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { - diff::get_working_tree_diff(&self.repo, self.whitespace_mode, highlighter) + fn get_working_tree_diff(&self) -> Result { + diff::get_working_tree_diff(&self.repo, self.whitespace_mode) } - fn get_staged_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { - diff::get_staged_diff(&self.repo, self.whitespace_mode, highlighter) + fn get_staged_diff(&self) -> Result { + diff::get_staged_diff(&self.repo, self.whitespace_mode) } - fn get_unstaged_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { - diff::get_unstaged_diff(&self.repo, self.whitespace_mode, highlighter) + fn get_unstaged_diff(&self) -> Result { + diff::get_unstaged_diff(&self.repo, self.whitespace_mode) } fn list_changed_paths(&self, kind: ChangeKind) -> Result> { @@ -165,14 +165,8 @@ impl VcsBackend for Libgit2Backend { fn get_commit_range_diff( &self, revision_range: &ResolvedRevisionRange<'_>, - highlighter: &SyntaxHighlighter, - ) -> Result> { - diff::get_commit_range_diff( - &self.repo, - revision_range, - self.whitespace_mode, - highlighter, - ) + ) -> Result { + diff::get_commit_range_diff(&self.repo, revision_range, self.whitespace_mode) } fn get_commits_info(&self, ids: &[String]) -> Result> { @@ -191,17 +185,8 @@ impl VcsBackend for Libgit2Backend { .collect()) } - fn get_working_tree_with_commits_diff( - &self, - commit_ids: &[String], - highlighter: &SyntaxHighlighter, - ) -> Result> { - diff::get_working_tree_with_commits_diff( - &self.repo, - commit_ids, - self.whitespace_mode, - highlighter, - ) + fn get_working_tree_with_commits_diff(&self, commit_ids: &[String]) -> Result { + diff::get_working_tree_with_commits_diff(&self.repo, commit_ids, self.whitespace_mode) } fn stage_file(&self, path: &Path) -> Result<()> { diff --git a/src/vcs/git/mod.rs b/src/vcs/git/mod.rs index 1ee25aa8..defd83b9 100644 --- a/src/vcs/git/mod.rs +++ b/src/vcs/git/mod.rs @@ -9,13 +9,12 @@ use std::ffi::OsStr; use std::path::{Path, PathBuf}; use crate::error::{Result, TuicrError}; -use crate::model::{DiffFile, DiffLine, FileStatus}; +use crate::model::{DiffLine, FileStatus}; use crate::process::{CommandOutputError, CommandOutputErrorKind, run_command_output}; -use crate::syntax::SyntaxHighlighter; use super::traits::{ - ChangeKind, CommitInfo, DiffWhitespaceMode, ResolvedRevisionRange, VcsBackend, VcsChangeStatus, - VcsInfo, + ChangeKind, CommitInfo, DiffWhitespaceMode, DiffWithJobs, ResolvedRevisionRange, VcsBackend, + VcsChangeStatus, VcsInfo, }; use cli::GitCliBackend; pub use libgit2::Libgit2Backend; @@ -248,24 +247,24 @@ impl VcsBackend for GitBackend { } } - fn get_working_tree_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { match self { - Self::Libgit2(backend) => backend.get_working_tree_diff(highlighter), - Self::Cli(backend) => backend.get_working_tree_diff(highlighter), + Self::Libgit2(backend) => backend.get_working_tree_diff(), + Self::Cli(backend) => backend.get_working_tree_diff(), } } - fn get_staged_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { + fn get_staged_diff(&self) -> Result { match self { - Self::Libgit2(backend) => backend.get_staged_diff(highlighter), - Self::Cli(backend) => backend.get_staged_diff(highlighter), + Self::Libgit2(backend) => backend.get_staged_diff(), + Self::Cli(backend) => backend.get_staged_diff(), } } - fn get_unstaged_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { + fn get_unstaged_diff(&self) -> Result { match self { - Self::Libgit2(backend) => backend.get_unstaged_diff(highlighter), - Self::Cli(backend) => backend.get_unstaged_diff(highlighter), + Self::Libgit2(backend) => backend.get_unstaged_diff(), + Self::Cli(backend) => backend.get_unstaged_diff(), } } @@ -338,11 +337,10 @@ impl VcsBackend for GitBackend { fn get_commit_range_diff( &self, revision_range: &ResolvedRevisionRange<'_>, - highlighter: &SyntaxHighlighter, - ) -> Result> { + ) -> Result { match self { - Self::Libgit2(backend) => backend.get_commit_range_diff(revision_range, highlighter), - Self::Cli(backend) => backend.get_commit_range_diff(revision_range, highlighter), + Self::Libgit2(backend) => backend.get_commit_range_diff(revision_range), + Self::Cli(backend) => backend.get_commit_range_diff(revision_range), } } @@ -353,18 +351,10 @@ impl VcsBackend for GitBackend { } } - fn get_working_tree_with_commits_diff( - &self, - commit_ids: &[String], - highlighter: &SyntaxHighlighter, - ) -> Result> { + fn get_working_tree_with_commits_diff(&self, commit_ids: &[String]) -> Result { match self { - Self::Libgit2(backend) => { - backend.get_working_tree_with_commits_diff(commit_ids, highlighter) - } - Self::Cli(backend) => { - backend.get_working_tree_with_commits_diff(commit_ids, highlighter) - } + Self::Libgit2(backend) => backend.get_working_tree_with_commits_diff(commit_ids), + Self::Cli(backend) => backend.get_working_tree_with_commits_diff(commit_ids), } } diff --git a/src/vcs/hg/mod.rs b/src/vcs/hg/mod.rs index c2a02a81..fa547b00 100644 --- a/src/vcs/hg/mod.rs +++ b/src/vcs/hg/mod.rs @@ -6,14 +6,13 @@ use std::process::Command; use chrono::{TimeZone, Utc}; use crate::error::{Result, TuicrError}; -use crate::model::{DiffFile, DiffLine, FileStatus, LineOrigin}; -use crate::syntax::SyntaxHighlighter; +use crate::model::{DiffLine, FileStatus, LineOrigin}; use crate::vcs::diff_parser::{self, DiffFormat}; use crate::vcs::traits::{ - CommitInfo, DiffWhitespaceMode, ResolvedRevisionRange, RevisionDiffTarget, VcsBackend, VcsInfo, - VcsType, + CommitInfo, DiffWhitespaceMode, DiffWithJobs, ResolvedRevisionRange, RevisionDiffTarget, + VcsBackend, VcsInfo, VcsType, }; -use crate::vcs::{BATCH_BOUNDARY, apply_container_full_file_highlight, parse_batched_files}; +use crate::vcs::{BATCH_BOUNDARY, append_container_full_file_jobs_from_rev, parse_batched_files}; /// Parse an hg description into (summary, optional body). fn parse_hg_description(desc: &str) -> (String, Option) { @@ -101,7 +100,7 @@ impl VcsBackend for HgBackend { &self.info } - fn get_working_tree_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { let args = self.diff_args(&["diff"]); let diff_output = run_hg_command(&self.info.root_path, args.iter().copied())?; @@ -109,16 +108,16 @@ impl VcsBackend for HgBackend { return Err(TuicrError::NoChanges); } - let mut files = diff_parser::parse_unified_diff(&diff_output, DiffFormat::Hg, highlighter)?; - apply_container_full_file_highlight( + let (files, mut jobs) = diff_parser::parse_unified_diff(&diff_output, DiffFormat::Hg)?; + append_container_full_file_jobs_from_rev( &self.info.root_path, ".", None, - &mut files, - highlighter, + &files, + &mut jobs, hg_cat_batch, )?; - Ok(files) + Ok((files, jobs)) } fn fetch_context_lines( @@ -267,8 +266,7 @@ impl VcsBackend for HgBackend { fn get_commit_range_diff( &self, revision_range: &ResolvedRevisionRange<'_>, - highlighter: &SyntaxHighlighter, - ) -> Result> { + ) -> Result { let commit_ids = &revision_range.commit_ids; if commit_ids.is_empty() { return Err(TuicrError::NoChanges); @@ -321,16 +319,16 @@ impl VcsBackend for HgBackend { return Err(TuicrError::NoChanges); } - let mut files = diff_parser::parse_unified_diff(&diff_output, DiffFormat::Hg, highlighter)?; - apply_container_full_file_highlight( + let (files, mut jobs) = diff_parser::parse_unified_diff(&diff_output, DiffFormat::Hg)?; + append_container_full_file_jobs_from_rev( &self.info.root_path, &from_rev, Some(newest_short), - &mut files, - highlighter, + &files, + &mut jobs, hg_cat_batch, )?; - Ok(files) + Ok((files, jobs)) } fn get_commits_info(&self, ids: &[String]) -> Result> { @@ -394,11 +392,7 @@ impl VcsBackend for HgBackend { Ok(ids.iter().filter_map(|id| by_id.remove(id)).collect()) } - fn get_working_tree_with_commits_diff( - &self, - commit_ids: &[String], - highlighter: &SyntaxHighlighter, - ) -> Result> { + fn get_working_tree_with_commits_diff(&self, commit_ids: &[String]) -> Result { if commit_ids.is_empty() { return Err(TuicrError::NoChanges); } @@ -436,16 +430,16 @@ impl VcsBackend for HgBackend { return Err(TuicrError::NoChanges); } - let mut files = diff_parser::parse_unified_diff(&diff_output, DiffFormat::Hg, highlighter)?; - apply_container_full_file_highlight( + let (files, mut jobs) = diff_parser::parse_unified_diff(&diff_output, DiffFormat::Hg)?; + append_container_full_file_jobs_from_rev( &self.info.root_path, &from_rev, None, - &mut files, - highlighter, + &files, + &mut jobs, hg_cat_batch, )?; - Ok(files) + Ok((files, jobs)) } } @@ -604,9 +598,7 @@ mod tests { assert_eq!(backend.info().root_path, expected_path); assert_eq!(backend.info().vcs_type, VcsType::Mercurial); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (files, _jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); assert_eq!(files.len(), 1); assert_eq!( @@ -630,14 +622,14 @@ mod tests { .expect("Failed to create hg backend"); assert!(matches!( - backend.get_working_tree_diff(&SyntaxHighlighter::default()), + backend.get_working_tree_diff(), Err(TuicrError::NoChanges) )); fs::write(temp.path().join("hello.txt"), " hello ship \n") .expect("Failed to write non-whitespace edit"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) + let (files, _) = backend + .get_working_tree_diff() .expect("non-whitespace edit should still produce a diff"); assert_eq!(files.len(), 1); } @@ -674,20 +666,14 @@ mod tests { .expect("Expected whitespace commit"); assert!(matches!( - backend.get_working_tree_with_commits_diff( - std::slice::from_ref(&whitespace_commit.id), - &SyntaxHighlighter::default() - ), + backend.get_working_tree_with_commits_diff(std::slice::from_ref(&whitespace_commit.id)), Err(TuicrError::NoChanges) )); fs::write(temp.path().join("hello.txt"), " hello ship \n") .expect("Failed to write non-whitespace edit"); - let files = backend - .get_working_tree_with_commits_diff( - std::slice::from_ref(&whitespace_commit.id), - &SyntaxHighlighter::default(), - ) + let (files, _) = backend + .get_working_tree_with_commits_diff(std::slice::from_ref(&whitespace_commit.id)) .expect("non-whitespace edit should still produce a diff"); assert_eq!(files.len(), 1); } @@ -816,17 +802,15 @@ mod tests { // Get diff for the last two commits (Second and Third) let commit_ids = vec![commits[1].id.clone(), commits[0].id.clone()]; - let diff_result = backend.get_commit_range_diff( - &ResolvedRevisionRange::from_owned_commit_ids( + let diff_result = + backend.get_commit_range_diff(&ResolvedRevisionRange::from_owned_commit_ids( commit_ids, RevisionDiffTarget::CommitList, - ), - &SyntaxHighlighter::default(), - ); + )); // Note: Sapling (Meta's hg fork) may fail with "id_dag_snapshot()" error // in certain temporary directory configurations. Skip the test in that case. - let diff = match diff_result { + let (diff, _jobs) = match diff_result { Ok(d) => d, Err(TuicrError::VcsCommand(msg)) if msg.contains("id_dag_snapshot") => { eprintln!("Skipping test: Sapling-specific issue with tempdir repos"); @@ -906,9 +890,7 @@ mod tests { let backend = HgBackend::from_path(temp.path().to_path_buf(), DiffWhitespaceMode::Normal) .expect("Failed to create hg backend"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (files, _jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); // hg should show the rename assert!(!files.is_empty(), "Expected at least one file change"); @@ -979,9 +961,7 @@ mod tests { let backend = HgBackend::from_path(temp.path().to_path_buf(), DiffWhitespaceMode::Normal) .expect("Failed to create hg backend"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (files, _jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); assert!(!files.is_empty(), "Expected at least one file change"); @@ -1039,9 +1019,7 @@ mod tests { let backend = HgBackend::from_path(temp.path().to_path_buf(), DiffWhitespaceMode::Normal) .expect("Failed to create hg backend"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (files, _jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); assert_eq!(files.len(), 1, "Expected one file"); @@ -1095,11 +1073,14 @@ mod tests { let backend = HgBackend::from_path(temp.path().to_path_buf(), DiffWhitespaceMode::Normal) .expect("Failed to create hg backend"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (mut files, jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); assert_eq!(files.len(), 1); + let highlighter = crate::syntax::SyntaxHighlighter::default(); + for update in crate::syntax::streaming::run_blocking(jobs, &highlighter) { + crate::syntax::streaming::apply_update(&mut files, &highlighter, update); + } + let changed_lines: Vec<_> = files[0].hunks[0] .lines .iter() @@ -1147,9 +1128,7 @@ mod tests { let backend = HgBackend::from_path(temp.path().to_path_buf(), DiffWhitespaceMode::Normal) .expect("Failed to create hg backend"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (files, _jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); assert_eq!(files.len(), 1, "Expected one file"); diff --git a/src/vcs/jj/mod.rs b/src/vcs/jj/mod.rs index bd879136..4b470a8b 100644 --- a/src/vcs/jj/mod.rs +++ b/src/vcs/jj/mod.rs @@ -8,14 +8,13 @@ use std::process::Command; use chrono::{DateTime, Utc}; use crate::error::{Result, TuicrError}; -use crate::model::{DiffFile, DiffLine, FileStatus, LineOrigin}; -use crate::syntax::SyntaxHighlighter; +use crate::model::{DiffLine, FileStatus, LineOrigin}; use crate::vcs::diff_parser::{self, DiffFormat}; use crate::vcs::traits::{ - CommitInfo, DiffWhitespaceMode, ResolvedRevisionRange, RevisionDiffTarget, VcsBackend, VcsInfo, - VcsType, + CommitInfo, DiffWhitespaceMode, DiffWithJobs, ResolvedRevisionRange, RevisionDiffTarget, + VcsBackend, VcsInfo, VcsType, }; -use crate::vcs::{BATCH_BOUNDARY, apply_container_full_file_highlight, parse_batched_files}; +use crate::vcs::{BATCH_BOUNDARY, append_container_full_file_jobs_from_rev, parse_batched_files}; /// Parse a jj description into (summary, optional body). fn parse_description(desc: &str) -> (String, Option) { @@ -138,7 +137,7 @@ impl VcsBackend for JjBackend { &self.info } - fn get_working_tree_diff(&self, highlighter: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { let args = self.diff_args(&["diff", "--git"]); let diff_output = run_jj_command(&self.info.root_path, args.iter().copied())?; @@ -146,17 +145,17 @@ impl VcsBackend for JjBackend { return Err(TuicrError::NoChanges); } - let mut files = - diff_parser::parse_unified_diff(&diff_output, DiffFormat::GitStyle, highlighter)?; - apply_container_full_file_highlight( + let (files, mut jobs) = + diff_parser::parse_unified_diff(&diff_output, DiffFormat::GitStyle)?; + append_container_full_file_jobs_from_rev( &self.info.root_path, "@-", None, - &mut files, - highlighter, + &files, + &mut jobs, jj_show_batch, )?; - Ok(files) + Ok((files, jobs)) } fn fetch_context_lines( @@ -324,8 +323,7 @@ impl VcsBackend for JjBackend { fn get_commit_range_diff( &self, revision_range: &ResolvedRevisionRange<'_>, - highlighter: &SyntaxHighlighter, - ) -> Result> { + ) -> Result { let commit_ids = &revision_range.commit_ids; if commit_ids.is_empty() { return Err(TuicrError::NoChanges); @@ -346,17 +344,17 @@ impl VcsBackend for JjBackend { return Err(TuicrError::NoChanges); } - let mut files = - diff_parser::parse_unified_diff(&diff_output, DiffFormat::GitStyle, highlighter)?; - apply_container_full_file_highlight( + let (files, mut jobs) = + diff_parser::parse_unified_diff(&diff_output, DiffFormat::GitStyle)?; + append_container_full_file_jobs_from_rev( &self.info.root_path, &from_rev, Some(newest), - &mut files, - highlighter, + &files, + &mut jobs, jj_show_batch, )?; - Ok(files) + Ok((files, jobs)) } fn get_commits_info(&self, ids: &[String]) -> Result> { @@ -410,11 +408,7 @@ impl VcsBackend for JjBackend { Ok(ids.iter().filter_map(|id| by_id.remove(id)).collect()) } - fn get_working_tree_with_commits_diff( - &self, - commit_ids: &[String], - highlighter: &SyntaxHighlighter, - ) -> Result> { + fn get_working_tree_with_commits_diff(&self, commit_ids: &[String]) -> Result { if commit_ids.is_empty() { return Err(TuicrError::NoChanges); } @@ -432,17 +426,17 @@ impl VcsBackend for JjBackend { return Err(TuicrError::NoChanges); } - let mut files = - diff_parser::parse_unified_diff(&diff_output, DiffFormat::GitStyle, highlighter)?; - apply_container_full_file_highlight( + let (files, mut jobs) = + diff_parser::parse_unified_diff(&diff_output, DiffFormat::GitStyle)?; + append_container_full_file_jobs_from_rev( &self.info.root_path, &from_rev, None, - &mut files, - highlighter, + &files, + &mut jobs, jj_show_batch, )?; - Ok(files) + Ok((files, jobs)) } } @@ -601,9 +595,7 @@ mod tests { assert_eq!(backend.info().root_path, expected_path); assert_eq!(backend.info().vcs_type, VcsType::Jujutsu); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (files, _jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); assert_eq!(files.len(), 1); assert_eq!( @@ -626,16 +618,16 @@ mod tests { JjBackend::from_path(temp.path().to_path_buf(), DiffWhitespaceMode::IgnoreAll) .expect("Failed to create jj backend"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) + let (files, _) = backend + .get_working_tree_diff() .expect("whitespace-only edit may surface as a no-op diff file"); assert_eq!(files.len(), 1); assert!(files[0].hunks.is_empty()); fs::write(temp.path().join("hello.txt"), " hello ship \n") .expect("Failed to write non-whitespace edit"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) + let (files, _) = backend + .get_working_tree_diff() .expect("non-whitespace edit should still produce a diff"); assert_eq!(files.len(), 1); } @@ -671,22 +663,16 @@ mod tests { .find(|commit| commit.summary == "Whitespace commit") .expect("Expected whitespace commit"); - let files = backend - .get_working_tree_with_commits_diff( - std::slice::from_ref(&whitespace_commit.id), - &SyntaxHighlighter::default(), - ) + let (files, _) = backend + .get_working_tree_with_commits_diff(std::slice::from_ref(&whitespace_commit.id)) .expect("whitespace-only edit may surface as a no-op diff file"); assert_eq!(files.len(), 1); assert!(files[0].hunks.is_empty()); fs::write(temp.path().join("hello.txt"), " hello ship \n") .expect("Failed to write non-whitespace edit"); - let files = backend - .get_working_tree_with_commits_diff( - std::slice::from_ref(&whitespace_commit.id), - &SyntaxHighlighter::default(), - ) + let (files, _) = backend + .get_working_tree_with_commits_diff(std::slice::from_ref(&whitespace_commit.id)) .expect("non-whitespace edit should still produce a diff"); assert_eq!(files.len(), 1); } @@ -841,14 +827,11 @@ mod tests { let newest = &named_commits[0]; // Third commit let commit_ids = vec![oldest.id.clone(), newest.id.clone()]; - let diff = backend - .get_commit_range_diff( - &ResolvedRevisionRange::from_owned_commit_ids( - commit_ids, - RevisionDiffTarget::CommitList, - ), - &SyntaxHighlighter::default(), - ) + let (diff, _jobs) = backend + .get_commit_range_diff(&ResolvedRevisionRange::from_owned_commit_ids( + commit_ids, + RevisionDiffTarget::CommitList, + )) .expect("Failed to get commit range diff"); // Should have changes @@ -901,9 +884,7 @@ mod tests { let backend = JjBackend::from_path(temp.path().to_path_buf(), DiffWhitespaceMode::Normal) .expect("Failed to create jj backend"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (files, _jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); // jj should detect the rename // Note: jj may show this as delete + add if it doesn't detect the rename @@ -952,9 +933,7 @@ mod tests { let backend = JjBackend::from_path(temp.path().to_path_buf(), DiffWhitespaceMode::Normal) .expect("Failed to create jj backend"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (files, _jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); assert_eq!(files.len(), 1, "Expected one file"); @@ -987,9 +966,7 @@ mod tests { let backend = JjBackend::from_path(temp.path().to_path_buf(), DiffWhitespaceMode::Normal) .expect("Failed to create jj backend"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (files, _jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); assert_eq!(files.len(), 1, "Expected one file"); @@ -1080,11 +1057,14 @@ mod tests { let backend = JjBackend::from_path(temp.path().to_path_buf(), DiffWhitespaceMode::Normal) .expect("Failed to create jj backend"); - let files = backend - .get_working_tree_diff(&SyntaxHighlighter::default()) - .expect("Failed to get diff"); + let (mut files, jobs) = backend.get_working_tree_diff().expect("Failed to get diff"); assert_eq!(files.len(), 1); + let highlighter = crate::syntax::SyntaxHighlighter::default(); + for update in crate::syntax::streaming::run_blocking(jobs, &highlighter) { + crate::syntax::streaming::apply_update(&mut files, &highlighter, update); + } + let changed_lines: Vec<_> = files[0].hunks[0] .lines .iter() diff --git a/src/vcs/mod.rs b/src/vcs/mod.rs index 3ed2b433..d951d06a 100644 --- a/src/vcs/mod.rs +++ b/src/vcs/mod.rs @@ -26,8 +26,8 @@ pub use hg::HgBackend; pub use jj::JjBackend; pub use pr_noop::PrNoopVcs; pub use traits::{ - ChangeKind, CommitInfo, DiffWhitespaceMode, ResolvedRevisionRange, RevisionDiffTarget, - VcsBackend, VcsChangeStatus, VcsInfo, + ChangeKind, CommitInfo, DiffWhitespaceMode, DiffWithJobs, ResolvedRevisionRange, + RevisionDiffTarget, VcsBackend, VcsChangeStatus, VcsInfo, }; use std::collections::HashMap; @@ -35,9 +35,7 @@ use std::path::{Path, PathBuf}; use crate::error::{Result, TuicrError}; use crate::model::{DiffFile, LineSide}; -use crate::syntax::{ - HighlightedLines, HighlightedSpans, SyntaxHighlighter, needs_full_file_highlight, -}; +use crate::syntax::{HighlightJob, HighlightJobKind, needs_full_file_highlight}; /// Boundary marker emitted between files in batched `hg cat` / `jj file show` /// output. The long random suffix makes accidental collision with real source @@ -75,6 +73,29 @@ pub(crate) fn read_workdir_file(root: &Path, rel: &Path) -> Option { std::fs::read_to_string(root.join(rel)).ok() } +/// Drop files for which `keep` returns false, then re-index highlight jobs to +/// match the kept files' new positions. Jobs whose file was dropped are also +/// dropped. Order of kept files (and remaining jobs) is preserved. +pub fn filter_diff_with_jobs(diff: DiffWithJobs, keep: impl Fn(&DiffFile) -> bool) -> DiffWithJobs { + let (files, mut jobs) = diff; + let mut remap: HashMap = HashMap::with_capacity(files.len()); + let mut kept: Vec = Vec::with_capacity(files.len()); + for (old_idx, file) in files.into_iter().enumerate() { + if keep(&file) { + remap.insert(old_idx, kept.len()); + kept.push(file); + } + } + jobs.retain_mut(|job| match remap.get(&job.file_idx) { + Some(&new_idx) => { + job.file_idx = new_idx; + true + } + None => false, + }); + (kept, jobs) +} + /// Parse the output of a batched `hg cat` / `jj file show` invocation whose /// template prefixed each file with `\n{BATCH_BOUNDARY}\n{path}\n` before /// emitting `{data}`. Returns a `path → data` map. @@ -92,17 +113,20 @@ pub(crate) fn parse_batched_files(output: &str) -> HashMap { .collect() } -/// Re-highlight container-grammar files (Vue, Svelte, etc) using their full -/// content at the requested revisions. `new_rev = None` reads the new side -/// from the working tree on disk instead of calling `fetch_batch`. The -/// `fetch_batch` closure is the backend-specific batched-fetch primitive -/// (`hg cat -r REV ...` or `jj file show -r REV ...`). -pub(crate) fn apply_container_full_file_highlight( +/// Batch-fetch container-grammar files from a VCS at a given revision, then +/// emit a `FullFile` highlight job for each. `new_rev = None` reads the new +/// side from disk instead of calling `fetch_batch`. The `fetch_batch` closure +/// is the backend-specific batched-fetch primitive (`hg cat -r REV ...` or +/// `jj file show -r REV ...`). +/// +/// The fetch happens on the parse thread because backends here drive +/// subprocesses; the actual highlighting runs later on the worker thread. +pub(crate) fn append_container_full_file_jobs_from_rev( root: &Path, old_rev: &str, new_rev: Option<&str>, - files: &mut [DiffFile], - highlighter: &SyntaxHighlighter, + files: &[DiffFile], + jobs: &mut Vec, fetch_batch: F, ) -> Result<()> where @@ -123,9 +147,9 @@ where let workdir = new_rev.is_none().then(|| root.to_path_buf()); - enhance_with_full_file_highlight( + append_container_full_file_jobs( files, - highlighter, + jobs, |p| old_map.get(p).cloned(), |p| match (new_map.get(p), workdir.as_deref()) { (Some(content), _) => Some(content.clone()), @@ -137,36 +161,23 @@ where Ok(()) } -/// Files larger than this skip the full-file highlight pass and fall back to -/// per-hunk highlighting. Keeps a runaway-cost ceiling on diffs that include -/// huge generated artefacts (lockfiles, vendored bundles, fixtures). -const MAX_HIGHLIGHT_FILE_BYTES: usize = 1024 * 1024; - -/// Re-highlight each diff line using full-file context, for files whose -/// grammar needs it (Vue, Svelte, Astro, MDX). Other files keep their existing -/// per-hunk highlighting unchanged. -/// -/// `fetch_old`/`fetch_new` return the entire content of the file at the old -/// and new sides respectively (or `None` if unavailable). When a side is -/// available, every diff line on that side is replaced with the span at its -/// 1-based lineno from the full-file highlight. Lines whose side could not be -/// fetched keep whatever the parser already assigned. +/// Walk `files` and append one `FullFile` highlight job per container-grammar +/// file (Vue, Svelte, ...). The closures fetch the file's full content on +/// each side; either may return `None` if that side is unavailable. /// -/// Runs in three phases: fetch (serial, since fetch closures may close over -/// `!Send` state such as `git2::Repository`), highlight (parallel via -/// `std::thread::scope`, since syntect's `SyntaxSet` and `Theme` are `Sync` -/// and each file's syntect work is independent), and apply spans (serial, -/// needs `&mut files`). -pub(crate) fn enhance_with_full_file_highlight( - files: &mut [DiffFile], - highlighter: &SyntaxHighlighter, +/// Fetching happens here, in the parse phase, because the closures may close +/// over VCS state that is `!Send` (e.g. `git2::Repository`, an `hg` child +/// process). The highlighting itself runs later on the worker thread, against +/// the `String`s embedded in the job. +pub(crate) fn append_container_full_file_jobs( + files: &[DiffFile], + jobs: &mut Vec, mut fetch_old: F, mut fetch_new: G, ) where F: FnMut(&Path) -> Option, G: FnMut(&Path) -> Option, { - let mut jobs: Vec = Vec::new(); for (idx, file) in files.iter().enumerate() { if file.is_binary || file.is_too_large || file.hunks.is_empty() { continue; @@ -185,116 +196,12 @@ pub(crate) fn enhance_with_full_file_highlight( jobs.push(HighlightJob { file_idx: idx, syntax_path: syntax_path.to_path_buf(), - old_content, - new_content, + kind: HighlightJobKind::FullFile { + old_content, + new_content, + }, }); } - - if jobs.is_empty() { - return; - } - - let results = highlight_jobs_parallel(&jobs, highlighter); - - for (idx, old, new) in results { - if old.is_none() && new.is_none() { - continue; - } - apply_full_file_spans(&mut files[idx], highlighter, old.as_deref(), new.as_deref()); - } -} - -struct HighlightJob { - file_idx: usize, - syntax_path: PathBuf, - old_content: Option, - new_content: Option, -} - -type HighlightResult = (usize, Option, Option); - -fn highlight_jobs_parallel( - jobs: &[HighlightJob], - highlighter: &SyntaxHighlighter, -) -> Vec { - let parallelism = std::thread::available_parallelism() - .map(|n| n.get()) - .unwrap_or(1) - .min(jobs.len()); - - if parallelism <= 1 { - return jobs - .iter() - .map(|j| highlight_single_job(j, highlighter)) - .collect(); - } - - let chunk_size = jobs.len().div_ceil(parallelism); - std::thread::scope(|scope| { - let handles: Vec<_> = jobs - .chunks(chunk_size) - .map(|chunk| { - scope.spawn(move || { - chunk - .iter() - .map(|j| highlight_single_job(j, highlighter)) - .collect::>() - }) - }) - .collect(); - handles - .into_iter() - .flat_map(|h| h.join().expect("highlight thread panicked")) - .collect() - }) -} - -fn highlight_single_job(job: &HighlightJob, highlighter: &SyntaxHighlighter) -> HighlightResult { - let old = job - .old_content - .as_deref() - .and_then(|c| highlight_content(highlighter, &job.syntax_path, c)); - let new = job - .new_content - .as_deref() - .and_then(|c| highlight_content(highlighter, &job.syntax_path, c)); - (job.file_idx, old, new) -} - -fn highlight_content( - highlighter: &SyntaxHighlighter, - path: &Path, - content: &str, -) -> Option { - if content.len() > MAX_HIGHLIGHT_FILE_BYTES || content.as_bytes().contains(&0u8) { - return None; - } - let lines: Vec = content.lines().map(tabify).collect(); - highlighter.highlight_file_lines(path, &lines) -} - -fn apply_full_file_spans( - file: &mut DiffFile, - highlighter: &SyntaxHighlighter, - old_highlight: Option<&[Option]>, - new_highlight: Option<&[Option]>, -) { - for hunk in &mut file.hunks { - for line in &mut hunk.lines { - let old_idx = line.old_lineno.map(|n| n.saturating_sub(1) as usize); - let new_idx = line.new_lineno.map(|n| n.saturating_sub(1) as usize); - let spans = highlighter.highlighted_line_for_diff_with_background( - old_highlight, - new_highlight, - old_idx, - new_idx, - line.origin, - ); - if spans.is_some() { - line.highlighted_spans = spans; - } - } - } } /// Detect the VCS type and return the appropriate backend. @@ -436,6 +343,7 @@ mod tests { fn highlight_n_vue_files(n: usize) -> Vec { use crate::syntax::SyntaxHighlighter; + use crate::syntax::streaming::{apply_update, run_blocking}; let mut files = Vec::with_capacity(n); let mut content_map: HashMap = HashMap::new(); @@ -447,12 +355,16 @@ mod tests { } let highlighter = SyntaxHighlighter::default(); - enhance_with_full_file_highlight( - &mut files, - &highlighter, + let mut jobs = Vec::new(); + append_container_full_file_jobs( + &files, + &mut jobs, |p| content_map.get(p).map(|(o, _)| o.clone()), |p| content_map.get(p).map(|(_, n)| n.clone()), ); + for update in run_blocking(jobs, &highlighter) { + apply_update(&mut files, &highlighter, update); + } files } @@ -477,124 +389,22 @@ mod tests { } #[test] - fn enhance_full_file_highlight_serial_path_one_file() { - // Single file takes the serial branch in highlight_jobs_parallel. + fn streaming_full_file_highlight_serial_one_file() { + // Single file takes the serial branch in the streaming worker. let files = highlight_n_vue_files(1); assert_all_lines_highlighted(&files); } #[test] - fn enhance_full_file_highlight_parallel_path_many_files() { - // 12 files exceeds typical parallelism, forcing chunked thread::scope. + fn streaming_full_file_highlight_parallel_many_files() { + // 12 files exceeds typical parallelism, forcing the scoped pool. let files = highlight_n_vue_files(12); assert_eq!(files.len(), 12); assert_all_lines_highlighted(&files); } - fn synth_vue_file(idx: usize) -> (DiffFile, String, String) { - let mut html = String::from("\n\n"); - - let mut script = - String::from("\n\n"); - - let mut style = String::from("\n"); - - let old = format!("{html}{script}{style}"); - let new = old.replace("const value0 = ref(0)", "const value0 = ref(42)"); - - let target_line = new - .lines() - .position(|l| l.starts_with("const value0 = ref(42)")) - .expect("synth content must contain target line") as u32 - + 1; - let file = vue_diff_file( - idx, - "const value0 = ref(0)", - "const value0 = ref(42)", - target_line, - ); - (file, old, new) - } - - /// Manual bench: parallel vs serial highlight at realistic scales. - /// Run with: `cargo test --release vcs::tests::bench_highlight_parallel_vs_serial -- --ignored --nocapture` - #[test] - #[ignore] - fn bench_highlight_parallel_vs_serial() { - use crate::syntax::SyntaxHighlighter; - use std::time::Instant; - - let highlighter = SyntaxHighlighter::default(); - let scales = [1usize, 5, 12, 25, 50]; - let runs = 5; - - for &n in &scales { - let mut files_template = Vec::with_capacity(n); - let mut content_map: HashMap = HashMap::new(); - for i in 0..n { - let (file, old, new) = synth_vue_file(i); - content_map.insert(file.new_path.clone().unwrap(), (old, new)); - files_template.push(file); - } - - let jobs: Vec = files_template - .iter() - .enumerate() - .map(|(idx, f)| { - let path = f.new_path.clone().unwrap(); - let (old, new) = content_map.get(&path).unwrap(); - HighlightJob { - file_idx: idx, - syntax_path: path, - old_content: Some(old.clone()), - new_content: Some(new.clone()), - } - }) - .collect(); - - // Warmup - let _ = highlight_jobs_parallel(&jobs, &highlighter); - - let mut par_total = std::time::Duration::ZERO; - for _ in 0..runs { - let t = Instant::now(); - let _ = highlight_jobs_parallel(&jobs, &highlighter); - par_total += t.elapsed(); - } - let par_mean = par_total / runs as u32; - - let mut ser_total = std::time::Duration::ZERO; - for _ in 0..runs { - let t = Instant::now(); - let _: Vec = jobs - .iter() - .map(|j| highlight_single_job(j, &highlighter)) - .collect(); - ser_total += t.elapsed(); - } - let ser_mean = ser_total / runs as u32; - - let speedup = ser_mean.as_secs_f64() / par_mean.as_secs_f64().max(1e-9); - println!( - "N={n:>3}: serial={ser_mean:>10.2?} parallel={par_mean:>10.2?} speedup={speedup:.2}x" - ); - } - } - #[test] - fn enhance_full_file_highlight_results_match_input_order() { + fn streaming_full_file_highlight_results_match_input_order() { // Each file's highlighted spans must land on that file's hunk lines, // not a neighbour's. Distinguishable by line content. let files = highlight_n_vue_files(6); diff --git a/src/vcs/pr_noop.rs b/src/vcs/pr_noop.rs index a9d8f502..ceb9b193 100644 --- a/src/vcs/pr_noop.rs +++ b/src/vcs/pr_noop.rs @@ -12,10 +12,9 @@ use std::path::Path; use crate::error::{Result, TuicrError}; -use crate::model::{DiffFile, DiffLine, FileStatus}; -use crate::syntax::SyntaxHighlighter; +use crate::model::{DiffLine, FileStatus}; -use super::traits::{VcsBackend, VcsInfo}; +use super::traits::{DiffWithJobs, VcsBackend, VcsInfo}; pub struct PrNoopVcs { info: VcsInfo, @@ -32,7 +31,7 @@ impl VcsBackend for PrNoopVcs { &self.info } - fn get_working_tree_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_working_tree_diff(&self) -> Result { Err(TuicrError::UnsupportedOperation( "PR mode does not read from the local working tree".to_string(), )) @@ -96,9 +95,7 @@ mod tests { // given let vcs = PrNoopVcs::new(info()); // when - let err = vcs - .get_working_tree_diff(&SyntaxHighlighter::default()) - .unwrap_err(); + let err = vcs.get_working_tree_diff().unwrap_err(); // then assert!( err.to_string() diff --git a/src/vcs/traits.rs b/src/vcs/traits.rs index f49e322e..362d10c3 100644 --- a/src/vcs/traits.rs +++ b/src/vcs/traits.rs @@ -4,7 +4,11 @@ use std::path::{Path, PathBuf}; use crate::error::Result; use crate::model::{DiffFile, DiffLine, FileStatus}; -use crate::syntax::SyntaxHighlighter; +use crate::syntax::HighlightJob; + +/// Result of a diff fetch: parsed files (with `highlighted_spans = None` on +/// every line) plus the highlight work the streaming worker will run. +pub type DiffWithJobs = (Vec, Vec); /// Information about the VCS type #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -174,18 +178,20 @@ pub trait VcsBackend: Send { false } - /// Get the working tree diff (staged + unstaged changes) - fn get_working_tree_diff(&self, highlighter: &SyntaxHighlighter) -> Result>; + /// Get the working tree diff (staged + unstaged changes). + /// Returns the parsed diff plus highlight jobs the streaming worker + /// will run on a background thread. + fn get_working_tree_diff(&self) -> Result; /// Get the staged diff (index vs HEAD) - fn get_staged_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_staged_diff(&self) -> Result { Err(crate::error::TuicrError::UnsupportedOperation( "Staged diff not supported for this VCS".into(), )) } /// Get the unstaged diff (working tree vs index) - fn get_unstaged_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + fn get_unstaged_diff(&self) -> Result { Err(crate::error::TuicrError::UnsupportedOperation( "Unstaged diff not supported for this VCS".into(), )) @@ -248,8 +254,7 @@ pub trait VcsBackend: Send { fn get_commit_range_diff( &self, _revision_range: &ResolvedRevisionRange<'_>, - _highlighter: &SyntaxHighlighter, - ) -> Result> { + ) -> Result { Err(crate::error::TuicrError::UnsupportedOperation( "Commit range diff not supported for this VCS".into(), )) @@ -264,11 +269,7 @@ pub trait VcsBackend: Send { /// Get a combined diff from the parent of the oldest commit through to the working tree. /// This shows both committed and working tree changes in a single diff. /// Returns error if not supported (default). - fn get_working_tree_with_commits_diff( - &self, - _commit_ids: &[String], - _highlighter: &SyntaxHighlighter, - ) -> Result> { + fn get_working_tree_with_commits_diff(&self, _commit_ids: &[String]) -> Result { Err(crate::error::TuicrError::UnsupportedOperation( "Working tree + commits diff not supported for this VCS".into(), )) From cff3017bbe79ef495e9da63d35e9fac4bb3cffd2 Mon Sep 17 00:00:00 2001 From: Pierre-Henri Toussaint Date: Tue, 12 May 2026 15:48:45 +0200 Subject: [PATCH 2/4] perf(syntax): prioritize highlight jobs around the focused file Pending jobs now live in an Arc>> instead of an Arc with an atomic cursor. The worker still pops jobs in queue order, but the App can reorder the remaining queue by file-index distance whenever the user moves between files. Why: when the user clicks the last file in a 565-file commit, the worker is still grinding through files near index 0 that aren't on screen. The visible diff sits unhighlighted for several seconds while the worker walks the queue. Sorting pending jobs by abs_diff from the current file_idx puts visible work first. Called from: * jump_to_file (file list click, } / { navigation) * update_current_file_from_cursor (cursor / scroll moved into a new file) The sort is O(n log n) on the remaining queue, run under a mutex held only long enough to reorder. Workers contend on the same mutex per pop, but the lock is held for microseconds and navigation events are human-paced, so contention is negligible in practice. --- src/app.rs | 36 ++++++++++++------ src/syntax/streaming.rs | 83 +++++++++++++++++++---------------------- 2 files changed, 63 insertions(+), 56 deletions(-) diff --git a/src/app.rs b/src/app.rs index 5ba57489..774bf67b 100644 --- a/src/app.rs +++ b/src/app.rs @@ -5613,7 +5613,9 @@ impl App { self.primed_walk_prev = false; self.down_released_since_arm = false; self.up_released_since_arm = false; + let prev = self.diff_state.current_file_idx; self.diff_state.current_file_idx = idx; + self.reprioritize_highlights_if_file_changed(prev); self.diff_state.cursor_line = self.calculate_file_scroll_offset(idx); let max_scroll = self.max_scroll_offset(); self.diff_state.scroll_offset = self.diff_state.cursor_line.min(max_scroll); @@ -6176,26 +6178,38 @@ impl App { if self.is_single_file_view { return; } + let prev = self.diff_state.current_file_idx; + if let Some(new_idx) = self.file_idx_at_cursor() { + self.diff_state.current_file_idx = new_idx; + self.file_list_state.select(new_idx); + } + self.reprioritize_highlights_if_file_changed(prev); + } + + fn file_idx_at_cursor(&self) -> Option { + if self.diff_files.is_empty() { + return None; + } let mut cumulative = self.review_comments_render_height(); if self.diff_state.cursor_line < cumulative { - if !self.diff_files.is_empty() { - self.diff_state.current_file_idx = 0; - self.file_list_state.select(0); - } - return; + return Some(0); } for (i, file) in self.diff_files.iter().enumerate() { let height = self.file_render_height(i, file); if cumulative + height > self.diff_state.cursor_line { - self.diff_state.current_file_idx = i; - self.file_list_state.select(i); - return; + return Some(i); } cumulative += height; } - if !self.diff_files.is_empty() { - self.diff_state.current_file_idx = self.diff_files.len() - 1; - self.file_list_state.select(self.diff_files.len() - 1); + Some(self.diff_files.len() - 1) + } + + fn reprioritize_highlights_if_file_changed(&self, prev: usize) { + if prev == self.diff_state.current_file_idx { + return; + } + if let Some(session) = self.highlight_session.as_ref() { + session.prioritize_around(self.diff_state.current_file_idx); } } diff --git a/src/syntax/streaming.rs b/src/syntax/streaming.rs index b8f533aa..042ef977 100644 --- a/src/syntax/streaming.rs +++ b/src/syntax/streaming.rs @@ -10,11 +10,11 @@ //! `SyntaxSet` / `Theme` are `Sync`, so the highlight phase parallelises //! cleanly across threads as long as inputs are owned strings. -use std::collections::HashMap; +use std::collections::{HashMap, VecDeque}; use std::path::{Path, PathBuf}; -use std::sync::Arc; -use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::mpsc::{self, Sender}; +use std::sync::{Arc, Mutex}; use std::thread; use crate::model::diff_types::LineOrigin; @@ -70,16 +70,21 @@ pub(crate) enum HighlightUpdateKind { }, } +/// Shared queue of pending jobs. Workers pop from the front under the +/// mutex; the App can reorder the queue by file-index proximity when the +/// user navigates, so visible files get highlighted first. +pub(crate) type SharedQueue = Arc>>; + /// Spawn the highlight worker. Returns the join handle; the caller usually /// doesn't need to wait on it (the worker exits when its send fails on a /// dropped receiver, or when `cancel` is set). pub(crate) fn spawn_highlight_worker( - jobs: Vec, + queue: SharedQueue, highlighter: Arc, cancel: Arc, tx: Sender, ) -> thread::JoinHandle<()> { - thread::spawn(move || run_worker(jobs, highlighter, cancel, tx)) + thread::spawn(move || run_worker(queue, highlighter, cancel, tx)) } /// Run the highlight pipeline synchronously on the current thread. Used by @@ -89,54 +94,31 @@ pub(crate) fn run_blocking( jobs: Vec, highlighter: &SyntaxHighlighter, ) -> Vec { - let mut out = Vec::with_capacity(jobs.len()); - for job in jobs { - if let Some(update) = run_job(&job, highlighter) { - out.push(update); - } - } - out + jobs.iter() + .filter_map(|job| run_job(job, highlighter)) + .collect() } fn run_worker( - jobs: Vec, + queue: SharedQueue, highlighter: Arc, cancel: Arc, tx: Sender, ) { - if jobs.is_empty() { + let initial_len = queue.lock().unwrap().len(); + if initial_len == 0 { return; } let parallelism = thread::available_parallelism() .map(|n| n.get()) .unwrap_or(1) - .min(jobs.len()) + .min(initial_len) .max(1); - if parallelism <= 1 { - for job in &jobs { - if cancel.load(Ordering::Relaxed) { - return; - } - if let Some(update) = run_job(job, &highlighter) - && tx.send(update).is_err() - { - return; - } - } - return; - } - - // Atomic-cursor work stealing: every worker pulls the next index from - // a shared counter. Beats static chunking when job sizes are skewed - // (e.g. one giant file + many small ones). - let cursor = Arc::new(AtomicUsize::new(0)); - let jobs = Arc::new(jobs); thread::scope(|s| { for _ in 0..parallelism { - let cursor = Arc::clone(&cursor); - let jobs = Arc::clone(&jobs); + let queue = Arc::clone(&queue); let cancel = Arc::clone(&cancel); let tx = tx.clone(); let highlighter = Arc::clone(&highlighter); @@ -145,11 +127,11 @@ fn run_worker( if cancel.load(Ordering::Relaxed) { return; } - let idx = cursor.fetch_add(1, Ordering::Relaxed); - if idx >= jobs.len() { + let job = queue.lock().unwrap().pop_front(); + let Some(job) = job else { return; - } - if let Some(update) = run_job(&jobs[idx], &highlighter) + }; + if let Some(update) = run_job(&job, &highlighter) && tx.send(update).is_err() { return; @@ -224,12 +206,13 @@ fn highlight_full( h.highlight_file_lines(path, &lines) } -/// A running highlight session: receiver for streamed results plus the -/// cancel token shared with the worker. The two are always twinned, so they -/// live behind a single `Option` on `App`. +/// A running highlight session: receiver for streamed results, the cancel +/// token shared with the worker, and the shared job queue (so the App can +/// reprioritize pending work when the user navigates). pub(crate) struct HighlightSession { rx: mpsc::Receiver, cancel: Arc, + queue: SharedQueue, } impl HighlightSession { @@ -242,10 +225,11 @@ impl HighlightSession { if jobs.is_empty() { return None; } + let queue: SharedQueue = Arc::new(Mutex::new(VecDeque::from(jobs))); let cancel = Arc::new(AtomicBool::new(false)); let (tx, rx) = mpsc::channel(); - spawn_highlight_worker(jobs, highlighter, Arc::clone(&cancel), tx); - Some(Self { rx, cancel }) + spawn_highlight_worker(Arc::clone(&queue), highlighter, Arc::clone(&cancel), tx); + Some(Self { rx, cancel, queue }) } /// Re-resolve each job's `file_idx` by `syntax_path` against @@ -286,6 +270,15 @@ impl HighlightSession { pub(crate) fn try_recv(&self) -> Result { self.rx.try_recv() } + + /// Reorder the pending queue so jobs nearest `file_idx` run next. + /// Called when the user navigates to a new file, so the visible + /// viewport gets highlighted before far-away files. + pub(crate) fn prioritize_around(&self, file_idx: usize) { + let mut q = self.queue.lock().unwrap(); + q.make_contiguous() + .sort_by_key(|job| job.file_idx.abs_diff(file_idx)); + } } /// Patch a streamed highlight result into the in-memory diff. Idempotent: From 5a6a160ec9857b7a4f0167e4bffddf35c846a9bd Mon Sep 17 00:00:00 2001 From: Pierre-Henri Toussaint Date: Tue, 12 May 2026 15:59:16 +0200 Subject: [PATCH 3/4] perf(input): drain queued events before each redraw The main loop used to read one event and redraw, read one event and redraw. On a big diff where a single render takes ~10 ms, holding j or wheel-scrolling fast queues events faster than the loop can consume them, and the backlog shows up as 200-600 ms of input lag - the worst case being a direction reversal, where the user has to wait for the Up backlog to drain before the first Down is even read. Now after the blocking event::poll(poll_timeout), the loop keeps pulling events with event::poll(Duration::ZERO) until the queue is empty (or a 64-event cap is hit, in case of a paste or stuck key), then redraws once. The cap is a safety; in practice almost all bursts drain in well under 64 events. The existing 'continue' shortcuts inside the match (Ctrl+C, ZQ, dd, ;X, pending state setters) now use 'continue 'main' so they short- circuit straight to the outer loop's render path. Those keys aren't the high-throughput case batching is meant to help with, and keeping them per-event means each one renders before the next is read - which is what the user expects for atomic-action keys. --- src/main.rs | 543 +++++++++++++++++++++++++++------------------------- 1 file changed, 284 insertions(+), 259 deletions(-) diff --git a/src/main.rs b/src/main.rs index 1b6711c7..1471a124 100644 --- a/src/main.rs +++ b/src/main.rs @@ -36,6 +36,11 @@ const IDLE_POLL_TIMEOUT: Duration = Duration::from_millis(100); /// ~30 ms keeps perceived latency between worker emit and on-screen update /// under one frame; CPU cost of the extra wakeups is negligible. const STREAMING_POLL_TIMEOUT: Duration = Duration::from_millis(30); +/// Cap on events drained per tick. Holding j or wheel-scrolling fast queues +/// events faster than the per-event render can consume them; processing the +/// batch and rendering once kills perceived input lag. A cap keeps a paste +/// or stuck-key from monopolising the loop for too long before redraw. +const MAX_EVENTS_PER_TICK: usize = 64; fn main() -> anyhow::Result<()> { profile::init_from_env(); @@ -306,7 +311,7 @@ fn main() -> anyhow::Result<()> { let mut pending_ctrl_c: Option = None; // Main loop - loop { + 'main: loop { // Check for update result (non-blocking) if let Some(ref rx) = update_rx && let Ok( @@ -357,315 +362,335 @@ fn main() -> anyhow::Result<()> { IDLE_POLL_TIMEOUT }; - // Handle events + // Drain queued events before redrawing; see MAX_EVENTS_PER_TICK. if event::poll(poll_timeout)? { - let event = event::read()?; - // Down/Up Release flips the `*_released_since_arm` flag so the - // primed two-press file walk in single-file view requires a - // deliberate release + press; held-key auto-repeat (Repeat - // events) never satisfies the gate. Terminals without kitty - // REPORT_EVENT_TYPES support never emit Release, in which case - // `supports_keyboard_enhancement` is false and `cursor_down` / - // `cursor_up` skip the gate entirely. - if let Event::Key(key) = &event - && key.kind == KeyEventKind::Release - { - if matches!( - key.code, - crossterm::event::KeyCode::Down | crossterm::event::KeyCode::Char('j') - ) { - app.down_released_since_arm = true; - } - if matches!( - key.code, - crossterm::event::KeyCode::Up | crossterm::event::KeyCode::Char('k') - ) { - app.up_released_since_arm = true; - } - } - match event { - Event::Key(key) - if matches!(key.kind, KeyEventKind::Press | KeyEventKind::Repeat) => + for _ in 0..MAX_EVENTS_PER_TICK { + let event = event::read()?; + // Down/Up Release flips the `*_released_since_arm` flag so the + // primed two-press file walk in single-file view requires a + // deliberate release + press; held-key auto-repeat (Repeat + // events) never satisfies the gate. Terminals without kitty + // REPORT_EVENT_TYPES support never emit Release, in which case + // `supports_keyboard_enhancement` is false and `cursor_down` / + // `cursor_up` skip the gate entirely. + if let Event::Key(key) = &event + && key.kind == KeyEventKind::Release { - // Handle Ctrl+C twice to exit (works across all input modes) - // In Comment mode, first Ctrl+C also cancels the comment - if key.code == crossterm::event::KeyCode::Char('c') - && key - .modifiers - .contains(crossterm::event::KeyModifiers::CONTROL) + if matches!( + key.code, + crossterm::event::KeyCode::Down | crossterm::event::KeyCode::Char('j') + ) { + app.down_released_since_arm = true; + } + if matches!( + key.code, + crossterm::event::KeyCode::Up | crossterm::event::KeyCode::Char('k') + ) { + app.up_released_since_arm = true; + } + } + match event { + Event::Key(key) + if matches!(key.kind, KeyEventKind::Press | KeyEventKind::Repeat) => { - // If in comment mode, cancel the comment first - if app.input_mode == InputMode::Comment { - app.exit_comment_mode(); - } - - if let Some(first_press) = pending_ctrl_c - && first_press.elapsed() < CTRL_C_EXIT_TIMEOUT + // Handle Ctrl+C twice to exit (works across all input modes) + // In Comment mode, first Ctrl+C also cancels the comment + if key.code == crossterm::event::KeyCode::Char('c') + && key + .modifiers + .contains(crossterm::event::KeyModifiers::CONTROL) { - // Second Ctrl+C within timeout - exit immediately - app.should_quit = true; - continue; + // If in comment mode, cancel the comment first + if app.input_mode == InputMode::Comment { + app.exit_comment_mode(); + } + + if let Some(first_press) = pending_ctrl_c + && first_press.elapsed() < CTRL_C_EXIT_TIMEOUT + { + // Second Ctrl+C within timeout - exit immediately + app.should_quit = true; + continue 'main; + } + // First Ctrl+C (or timeout expired) - show warning and start timer + pending_ctrl_c = Some(Instant::now()); + app.set_message("Press Ctrl+C again to exit"); + continue 'main; } - // First Ctrl+C (or timeout expired) - show warning and start timer - pending_ctrl_c = Some(Instant::now()); - app.set_message("Press Ctrl+C again to exit"); - continue; - } - // Any other key clears the pending Ctrl+C state and message - if pending_ctrl_c.is_some() { - pending_ctrl_c = None; - app.message = None; - } + // Any other key clears the pending Ctrl+C state and message + if pending_ctrl_c.is_some() { + pending_ctrl_c = None; + app.message = None; + } - // Handle pending z command for zz/zt/zb viewport positioning - if pending_z { - pending_z = false; - match key.code { - crossterm::event::KeyCode::Char('z') => { - app.center_cursor(); - continue; - } - crossterm::event::KeyCode::Char('t') => { - app.cursor_to_top(); - continue; - } - crossterm::event::KeyCode::Char('b') => { - app.cursor_to_bottom(); - continue; + // Handle pending z command for zz/zt/zb viewport positioning + if pending_z { + pending_z = false; + match key.code { + crossterm::event::KeyCode::Char('z') => { + app.center_cursor(); + continue 'main; + } + crossterm::event::KeyCode::Char('t') => { + app.cursor_to_top(); + continue 'main; + } + crossterm::event::KeyCode::Char('b') => { + app.cursor_to_bottom(); + continue 'main; + } + _ => {} // Fall through to normal handling } - _ => {} // Fall through to normal handling } - } - // Handle pending Z command for ZZ (export+quit) / ZQ (quit) - if pending_shift_z { - pending_shift_z = false; - match key.code { - crossterm::event::KeyCode::Char('Z') => { - // ZZ: save session, export, and quit (same as :wq) - let _ = app.save_current_session_merging_external(); - if app.session.has_comments() { - handler::handle_export_and_quit(&mut app); - } else { + // Handle pending Z command for ZZ (export+quit) / ZQ (quit) + if pending_shift_z { + pending_shift_z = false; + match key.code { + crossterm::event::KeyCode::Char('Z') => { + // ZZ: save session, export, and quit (same as :wq) + let _ = app.save_current_session_merging_external(); + if app.session.has_comments() { + handler::handle_export_and_quit(&mut app); + } else { + app.should_quit = true; + } + continue 'main; + } + crossterm::event::KeyCode::Char('Q') => { + // ZQ: quit without exporting (same as q) app.should_quit = true; + continue 'main; } - continue; + _ => {} // Fall through to normal handling } - crossterm::event::KeyCode::Char('Q') => { - // ZQ: quit without exporting (same as q) - app.should_quit = true; - continue; - } - _ => {} // Fall through to normal handling } - } - // Handle pending d command for dd delete comment - if pending_d { - pending_d = false; - if key.code == crossterm::event::KeyCode::Char('d') { - if app.cursor_on_locked_comment() { - app.set_message( - "Comment already pushed to GitHub — read only in tuicr", - ); - } else if !app.delete_comment_at_cursor() { - if app.cursor_on_remote_thread() { - app.set_message("GitHub comment — read only in tuicr"); - } else { - app.set_message("No comment at cursor"); + // Handle pending d command for dd delete comment + if pending_d { + pending_d = false; + if key.code == crossterm::event::KeyCode::Char('d') { + if app.cursor_on_locked_comment() { + app.set_message( + "Comment already pushed to GitHub — read only in tuicr", + ); + } else if !app.delete_comment_at_cursor() { + if app.cursor_on_remote_thread() { + app.set_message("GitHub comment — read only in tuicr"); + } else { + app.set_message("No comment at cursor"); + } } + continue 'main; } - continue; + // Otherwise fall through to normal handling } - // Otherwise fall through to normal handling - } - // Handle pending leader command for panel focus, file list toggle, and review comments. - if pending_leader { - pending_leader = false; - match key.code { - crossterm::event::KeyCode::Char('e') => { - app.toggle_file_list(); - continue; - } - crossterm::event::KeyCode::Char('h') => { - if app.show_file_list { - app.focused_panel = app::FocusedPanel::FileList; + // Handle pending leader command for panel focus, file list toggle, and review comments. + if pending_leader { + pending_leader = false; + match key.code { + crossterm::event::KeyCode::Char('e') => { + app.toggle_file_list(); + continue 'main; } - continue; - } - crossterm::event::KeyCode::Char('l') => { - app.focused_panel = app::FocusedPanel::Diff; - continue; - } - crossterm::event::KeyCode::Char('k') => { - if app.focused_panel == app::FocusedPanel::Comments { - app.focused_panel = app::FocusedPanel::FileList; - } else if app.has_inline_commit_selector() { - app.focused_panel = app::FocusedPanel::CommitSelector; + crossterm::event::KeyCode::Char('h') => { + if app.show_file_list { + app.focused_panel = app::FocusedPanel::FileList; + } + continue 'main; } - continue; - } - crossterm::event::KeyCode::Char('j') => { - if app.focused_panel == app::FocusedPanel::FileList - && app.has_comment_navigator_items() - { - app.focused_panel = app::FocusedPanel::Comments; - } else { + crossterm::event::KeyCode::Char('l') => { app.focused_panel = app::FocusedPanel::Diff; + continue 'main; } - continue; - } - crossterm::event::KeyCode::Char('c') => { - app.enter_review_comment_mode(); - continue; - } - crossterm::event::KeyCode::Char('f') => { - app.toggle_single_file_view(); - continue; + crossterm::event::KeyCode::Char('k') => { + if app.focused_panel == app::FocusedPanel::Comments { + app.focused_panel = app::FocusedPanel::FileList; + } else if app.has_inline_commit_selector() { + app.focused_panel = app::FocusedPanel::CommitSelector; + } + continue 'main; + } + crossterm::event::KeyCode::Char('j') => { + if app.focused_panel == app::FocusedPanel::FileList + && app.has_comment_navigator_items() + { + app.focused_panel = app::FocusedPanel::Comments; + } else { + app.focused_panel = app::FocusedPanel::Diff; + } + continue 'main; + } + crossterm::event::KeyCode::Char('c') => { + app.enter_review_comment_mode(); + continue 'main; + } + crossterm::event::KeyCode::Char('f') => { + app.toggle_single_file_view(); + continue 'main; + } + _ => {} } - _ => {} + // Otherwise fall through to normal handling } - // Otherwise fall through to normal handling - } - // Editing the PR-tab filter is a sub-state of CommitSelect; - // route through the filter-specific key map so typed - // characters update the filter buffer rather than driving - // commit-list navigation. - let mut action = - if app.input_mode == InputMode::CommitSelect && app.pr_filter_editing() { + // Editing the PR-tab filter is a sub-state of CommitSelect; + // route through the filter-specific key map so typed + // characters update the filter buffer rather than driving + // commit-list navigation. + let mut action = if app.input_mode == InputMode::CommitSelect + && app.pr_filter_editing() + { map_target_filter_mode(key) } else { map_key_to_action(key, app.input_mode, app.leader_key) }; - // Handle pending command setters (these work in any mode) - match action { - Action::PendingZCommand => { - pending_z = true; - app.pending_count = None; - continue; - } - Action::PendingShiftZCommand => { - pending_shift_z = true; - app.pending_count = None; - continue; - } - Action::PendingDCommand => { - pending_d = true; - app.pending_count = None; - continue; - } - Action::PendingLeaderCommand => { - pending_leader = true; - app.pending_count = None; - continue; - } - _ => {} - } - - // Vim-style {count}{motion} (Normal mode only): digits accumulate - // into `pending_count`, then a following motion either scales its - // inner count parameter or is dispatched repeatedly. `{count}G` - // jumps to source line `count` (already existing behaviour). - if app.input_mode == InputMode::Normal { + // Handle pending command setters (these work in any mode) match action { - Action::Digit(d) => { - let n = app.pending_count.unwrap_or(0); - app.pending_count = Some( - (n.saturating_mul(10).saturating_add(d as usize)).min(999_999), - ); - continue; + Action::PendingZCommand => { + pending_z = true; + app.pending_count = None; + continue 'main; } - Action::GoToBottom if app.pending_count.is_some() => { - let count = app.pending_count.unwrap().max(1); + Action::PendingShiftZCommand => { + pending_shift_z = true; app.pending_count = None; - app.go_to_source_line(count as u32, tuicr::model::LineSide::New); - continue; + continue 'main; } - _ => { - if let Some(count) = app.pending_count.take() { - let count = count.max(1); - match &mut action { - Action::CursorDown(n) - | Action::CursorUp(n) - | Action::ScrollLeft(n) - | Action::ScrollRight(n) - | Action::ScrollViewDown(n) - | Action::ScrollViewUp(n) => { - *n = n.saturating_mul(count); - } - Action::NextFile - | Action::PrevFile - | Action::NextHunk - | Action::PrevHunk => { - // Dispatch `count - 1` extra times; the - // last one runs through normal dispatch - // below. - for _ in 1..count { - dispatch_action(&mut app, action.clone()); + Action::PendingDCommand => { + pending_d = true; + app.pending_count = None; + continue 'main; + } + Action::PendingLeaderCommand => { + pending_leader = true; + app.pending_count = None; + continue 'main; + } + _ => {} + } + + // Vim-style {count}{motion} (Normal mode only): digits accumulate + // into `pending_count`, then a following motion either scales its + // inner count parameter or is dispatched repeatedly. `{count}G` + // jumps to source line `count` (already existing behaviour). + if app.input_mode == InputMode::Normal { + match action { + Action::Digit(d) => { + let n = app.pending_count.unwrap_or(0); + app.pending_count = Some( + (n.saturating_mul(10).saturating_add(d as usize)) + .min(999_999), + ); + continue 'main; + } + Action::GoToBottom if app.pending_count.is_some() => { + let count = app.pending_count.unwrap().max(1); + app.pending_count = None; + app.go_to_source_line( + count as u32, + tuicr::model::LineSide::New, + ); + continue 'main; + } + _ => { + if let Some(count) = app.pending_count.take() { + let count = count.max(1); + match &mut action { + Action::CursorDown(n) + | Action::CursorUp(n) + | Action::ScrollLeft(n) + | Action::ScrollRight(n) + | Action::ScrollViewDown(n) + | Action::ScrollViewUp(n) => { + *n = n.saturating_mul(count); + } + Action::NextFile + | Action::PrevFile + | Action::NextHunk + | Action::PrevHunk => { + // Dispatch `count - 1` extra times; the + // last one runs through normal dispatch + // below. + for _ in 1..count { + dispatch_action(&mut app, action.clone()); + } + } + _ => { + // Count silently discarded for non-motion + // actions (mode changes, edits, etc.). } - } - _ => { - // Count silently discarded for non-motion - // actions (mode changes, edits, etc.). } } } } } - } - dispatch_action(&mut app, action); - if let Some(target) = app.take_pending_editor_target() { - match run_editor_from_tui(&mut terminal, &target) { - Ok(Ok(())) => { - if app.diff_source.includes_worktree_changes() { - match app.reload_diff_files() { - Ok((count, invalidated)) => { - let invalidated_suffix = if invalidated > 0 { - format!(", {invalidated} changed since last review") - } else { - String::new() - }; - app.set_message(format!( - "Opened {} and reloaded {count} files{invalidated_suffix}", - target.path.display() - )); - } - Err(err) => { - app.set_error(format!( - "Reload after editor failed: {err}" - )); + dispatch_action(&mut app, action); + if let Some(target) = app.take_pending_editor_target() { + match run_editor_from_tui(&mut terminal, &target) { + Ok(Ok(())) => { + if app.diff_source.includes_worktree_changes() { + match app.reload_diff_files() { + Ok((count, invalidated)) => { + let invalidated_suffix = if invalidated > 0 { + format!( + ", {invalidated} changed since last review" + ) + } else { + String::new() + }; + app.set_message(format!( + "Opened {} and reloaded {count} files{invalidated_suffix}", + target.path.display() + )); + } + Err(err) => { + app.set_error(format!( + "Reload after editor failed: {err}" + )); + } } + } else { + app.set_message(format!( + "Opened {}", + target.path.display() + )); } - } else { - app.set_message(format!("Opened {}", target.path.display())); + } + Ok(Err(err)) => app.set_error(err.to_string()), + Err(err) => { + app.set_error(format!("Failed to restore terminal: {err}")) } } - Ok(Err(err)) => app.set_error(err.to_string()), - Err(err) => app.set_error(format!("Failed to restore terminal: {err}")), + // Editor took over the terminal; force a redraw + // before reading any further queued events. + continue 'main; } } - } - Event::Mouse(mouse_event) => handle_mouse_event(&mut app, mouse_event), - Event::Paste(text) => { - // Bracketed-paste payload — route to whichever handler is - // currently accepting text input. Other modes ignore. - let action = Action::Paste(text); - match app.input_mode { - InputMode::Comment => handle_comment_action(&mut app, action), - InputMode::Command => handle_command_action(&mut app, action), - InputMode::Search => handle_search_action(&mut app, action), - InputMode::CommitSelect if app.pr_filter_editing() => { - handle_commit_select_action(&mut app, action) + Event::Mouse(mouse_event) => handle_mouse_event(&mut app, mouse_event), + Event::Paste(text) => { + // Bracketed-paste payload — route to whichever handler is + // currently accepting text input. Other modes ignore. + let action = Action::Paste(text); + match app.input_mode { + InputMode::Comment => handle_comment_action(&mut app, action), + InputMode::Command => handle_command_action(&mut app, action), + InputMode::Search => handle_search_action(&mut app, action), + InputMode::CommitSelect if app.pr_filter_editing() => { + handle_commit_select_action(&mut app, action) + } + _ => {} } - _ => {} } + _ => {} + } + if app.should_quit || !event::poll(Duration::ZERO)? { + break; } - _ => {} } } From b31ccccbb36beeffb39bf75b6ba165b9aa52a1d0 Mon Sep 17 00:00:00 2001 From: Pierre-Henri Toussaint Date: Tue, 12 May 2026 16:36:02 +0200 Subject: [PATCH 4/4] fix(ui): preserve manual file-list scroll when diff panel is focused When the user wheel-scrolls the file list while the diff panel has focus, the render-time 'sync selection to diff's current file' path selects the diff's file, and ratatui's List widget then auto-adjusts the offset on render to keep that selection visible - undoing the wheel. Net effect: the file list stays glued to whichever file the diff cursor is on, even when the user is trying to scroll past it. Track an 'I have manually scrolled the file list' flag, set by file_list_viewport_scroll_up/down. While the flag is set, skip the render-time sync entirely, so the user's offset stays put. The flag is cleared whenever the diff's current_file_idx changes for a reason other than file-list wheel (jump_to_file, cursor scrolling into a different file via update_current_file_from_cursor), which is the natural moment to resume tracking. Pre-existing bug, surfaced now that the rest of the loop is fast enough that wheel-scrolling the file list is actually responsive enough to expose the snap-back. --- src/app.rs | 13 ++++++++++++- src/ui/file_list.rs | 10 +++------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/app.rs b/src/app.rs index 774bf67b..a6bff690 100644 --- a/src/app.rs +++ b/src/app.rs @@ -945,6 +945,13 @@ pub struct App { pub file_list_state: FileListState, pub comment_navigator_state: CommentNavigatorState, + /// True while the user has manually scrolled the file list independently + /// of the diff cursor. Suppresses the render-time "sync selection to + /// diff's current file" path so wheel scrolls aren't snapped back by + /// ratatui's auto-adjust-on-selection-change. Cleared when something + /// else changes the diff's current file (cursor moving into a new file, + /// click jump, } / { navigation). + pub manual_file_list_scroll: bool, pub diff_state: DiffState, pub help_state: HelpState, pub command_buffer: String, @@ -1803,6 +1810,7 @@ impl App { diff_view_mode: DiffViewMode::Unified, file_list_state: FileListState::default(), comment_navigator_state: CommentNavigatorState::default(), + manual_file_list_scroll: false, diff_state: DiffState::default(), help_state: HelpState::default(), command_buffer: String::new(), @@ -5064,6 +5072,7 @@ impl App { if self.file_list_state.selected() < new_offset { self.file_list_state.select(new_offset); } + self.manual_file_list_scroll = true; } /// Scroll the file-list viewport up by `lines` without moving the @@ -5080,6 +5089,7 @@ impl App { if self.file_list_state.selected() > max_visible { self.file_list_state.select(max_visible); } + self.manual_file_list_scroll = true; } pub fn diff_annotation_at_screen_row(&self, screen_row: u16) -> Option { @@ -6204,10 +6214,11 @@ impl App { Some(self.diff_files.len() - 1) } - fn reprioritize_highlights_if_file_changed(&self, prev: usize) { + fn reprioritize_highlights_if_file_changed(&mut self, prev: usize) { if prev == self.diff_state.current_file_idx { return; } + self.manual_file_list_scroll = false; if let Some(session) = self.highlight_session.as_ref() { session.prioritize_around(self.diff_state.current_file_idx); } diff --git a/src/ui/file_list.rs b/src/ui/file_list.rs index e8c874bf..7c6efe16 100644 --- a/src/ui/file_list.rs +++ b/src/ui/file_list.rs @@ -67,20 +67,16 @@ pub(super) fn render_file_list(frame: &mut Frame, app: &mut App, area: Rect) { } let scroll_x = app.file_list_state.scroll_x; - // When diff panel is focused, sync file list selection to current file - // But preserve the current offset to not interfere with manual scrolling - if app.focused_panel == FocusedPanel::Diff { + // Skip when the user has wheel-scrolled the file list independently; + // see App::manual_file_list_scroll. + if app.focused_panel == FocusedPanel::Diff && !app.manual_file_list_scroll { let current_file_idx = app.diff_state.current_file_idx; for (tree_idx, item) in visible_items.iter().enumerate() { if let FileTreeItem::File { file_idx, .. } = item && *file_idx == current_file_idx { if app.file_list_state.selected() != tree_idx { - // Save current offset before changing selection - let current_offset = app.file_list_state.list_state.offset(); app.file_list_state.select(tree_idx); - // Restore offset to prevent auto-scrolling - *app.file_list_state.list_state.offset_mut() = current_offset; } break; }