From fb47844b51497a6b1e49d3a3046d0138f3dd4aa3 Mon Sep 17 00:00:00 2001 From: Jonas Ehrlich Date: Tue, 5 Aug 2025 20:44:15 +0200 Subject: [PATCH] Revert "More Git Dialog UI cleanup" --- backend/app/src/actors/git.rs | 20 ++- backend/app/src/web/api/v1/git.rs | 16 +- backend/git2-ox/src/diff.rs | 170 ++++++++---------- backend/git2-ox/src/repository.rs | 33 +++- frontend/src/client.ts | 4 +- frontend/src/components/badge-group.tsx | 35 ---- frontend/src/components/create-git-rev.tsx | 3 +- frontend/src/components/diff-viewer/file.tsx | 1 + frontend/src/components/diff-viewer/index.tsx | 54 +++--- frontend/src/components/flows-dialog.tsx | 7 +- frontend/src/components/gh-tabs.tsx | 38 ---- frontend/src/components/git-dialog.tsx | 86 +++------ .../src/components/git-revisions-panel.tsx | 9 +- frontend/src/components/git-stats.tsx | 15 +- frontend/src/components/git-status-card.tsx | 8 +- frontend/src/components/icon-select.tsx | 1 + frontend/src/components/ui/button.tsx | 2 +- frontend/src/types/api.ts | 37 ++-- 18 files changed, 217 insertions(+), 322 deletions(-) delete mode 100644 frontend/src/components/badge-group.tsx delete mode 100644 frontend/src/components/gh-tabs.tsx diff --git a/backend/app/src/actors/git.rs b/backend/app/src/actors/git.rs index 193fe2b..5feec35 100644 --- a/backend/app/src/actors/git.rs +++ b/backend/app/src/actors/git.rs @@ -90,20 +90,26 @@ impl Handler for GitActor { } } -#[message(response = Result)] -pub struct GetDiff { +#[message(response = Result, git2_ox::error::Error>)] +pub struct ListDiffs { pub base_rev: Option, pub head_rev: Option, } -impl Handler for GitActor { +impl Handler for GitActor { async fn handle( &mut self, _ctx: &mut Context, - msg: GetDiff, - ) -> Result { - self.repository - .diff(msg.base_rev.as_deref(), msg.head_rev.as_deref()) + msg: ListDiffs, + ) -> Result, git2_ox::error::Error> { + let diffs_iter = self + .repository + .iter_diffs_between_revisions(msg.base_rev.as_deref(), msg.head_rev.as_deref())?; + let mut diffs = Vec::new(); + for diff_result in diffs_iter { + diffs.push(diff_result?); + } + Ok(diffs) } } diff --git a/backend/app/src/web/api/v1/git.rs b/backend/app/src/web/api/v1/git.rs index 914a80e..d12ef52 100644 --- a/backend/app/src/web/api/v1/git.rs +++ b/backend/app/src/web/api/v1/git.rs @@ -13,14 +13,14 @@ pub fn router() -> routing::Router { routing::get(get_revision).post(checkout_revision), ) .route("/commits", routing::get(list_commits)) - .route("/diff", routing::get(get_diff)) + .route("/diffs", routing::get(list_diffs)) .route("/tags", routing::get(list_tags).post(create_tag)) .route("/branches", routing::get(list_branches).post(create_branch)) .route("/repository/status", routing::get(get_repository_status)) } #[derive(utoipa::OpenApi)] -#[openapi(paths(get_revision, checkout_revision, list_commits, list_tags, create_tag, list_branches, create_branch, get_repository_status, get_diff), tags((name = "Git Repository", description="Git Repository related endpoints")) )] +#[openapi(paths(get_revision, checkout_revision, list_commits, list_tags, create_tag, list_branches, create_branch, get_repository_status, list_diffs), tags((name = "Git Repository", description="Git Repository related endpoints")) )] pub(super) struct ApiDoc; #[utoipa::path( @@ -149,12 +149,12 @@ struct CommitRangeQuery { #[serde(rename_all = "camelCase")] struct ListDiffsResponse { /// Array of diffs in this commit range - diff: git2_ox::Diff, + diffs: Vec, } #[utoipa::path( get, - path = "/diff", + path = "/diffs", summary = "List diffs", description = "List the diffs in a commit range", params(CommitRangeQuery), @@ -163,17 +163,17 @@ struct ListDiffsResponse { (status = http::StatusCode::INTERNAL_SERVER_ERROR, description = "Internal server error", body = api::ApiStatusDetailResponse), ) )] -async fn get_diff( +async fn list_diffs( State(state): State, Query(query): Query, ) -> Result, api::AppError> { let actor = state.git_actor(); - let msg = actors::git::GetDiff { + let msg = actors::git::ListDiffs { base_rev: query.base_rev, head_rev: query.head_rev, }; - let diff = actor.call(msg).await??; - Ok(Json(ListDiffsResponse { diff })) + let diffs = actor.call(msg).await??; + Ok(Json(ListDiffsResponse { diffs })) } #[derive(ToSchema, Serialize, Deserialize, IntoParams)] diff --git a/backend/git2-ox/src/diff.rs b/backend/git2-ox/src/diff.rs index 8edac5a..8f530bc 100644 --- a/backend/git2-ox/src/diff.rs +++ b/backend/git2-ox/src/diff.rs @@ -1,6 +1,14 @@ -use std::collections::hash_map; - -use crate::{Result, error}; +#[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] +#[cfg_attr( + feature = "serde", + derive(serde::Serialize), + serde(rename_all = "lowercase") +)] +#[derive(PartialEq)] +enum DiffKind { + Binary, + Text, +} #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[cfg_attr( @@ -8,34 +16,41 @@ use crate::{Result, error}; derive(serde::Serialize), serde(rename_all = "camelCase") )] -pub(crate) struct DiffStats { - /// Number of files changed - files_changed: usize, - /// Number of insertions - insertions: usize, - /// Number of deletions - deletions: usize, - /// Number of lines in the old versions of all affected files - total_old_num_lines: usize, +struct DiffFile { + /// Path to the diff file + path: Option, + /// Content of the diff file + content: Option, } -impl DiffStats { - fn from_stats_and_total_old_num_lines( - stats: &git2::DiffStats, - total_old_num_lines: usize, - ) -> Self { - Self { - files_changed: stats.files_changed(), - insertions: stats.insertions(), - deletions: stats.deletions(), - total_old_num_lines, +impl DiffFile { + pub fn try_from_repo_and_diff_file( + repo: &git2::Repository, + diff_file: &git2::DiffFile, + ) -> Option { + let oid = diff_file.id(); + if oid.is_zero() { + return None; } - } -} -type Path = String; -type FileContent = String; + let content = match repo.find_blob(oid) { + Ok(blob) if diff_file.is_not_binary() => { + Some(String::from_utf8_lossy(blob.content()).into_owned()) + } + _ => { + log::warn!("Did not find blob for {:?}", diff_file.path()); + None + } + }; + Some(DiffFile { + path: diff_file + .path() + .map(|p| p.to_str().unwrap_or("unknown").to_owned()), + content, + }) + } +} #[cfg_attr(feature = "utoipa", derive(utoipa::ToSchema))] #[cfg_attr( feature = "serde", @@ -43,76 +58,49 @@ type FileContent = String; serde(rename_all = "camelCase") )] pub struct Diff { + /// Old file path and content + old: Option, + /// New file path and content + new: Option, + /// Kind of the diff + kind: DiffKind, /// Patch between old and new patch: String, - /// Stats of the diff - stats: DiffStats, - /// Map of old source paths to the old content - old_sources: hash_map::HashMap, } - impl Diff { - pub fn try_from_repo_and_diff(repo: &git2::Repository, diff: &git2::Diff) -> Result { - let mut patch_output = String::new(); - let mut total_num_lines: usize = 0; - let mut old_files: hash_map::HashMap = hash_map::HashMap::new(); - // Collect old file contents from each delta - diff.foreach( - &mut |delta, _| { - let oid = delta.old_file().id(); - if !oid.is_zero() { - let path = delta - .old_file() - .path() - .map(|p| p.to_string_lossy().to_string()) - .unwrap_or_else(|| "".to_string()); - - if let Ok(blob) = repo.find_blob(oid) { - if let Ok(content) = std::str::from_utf8(blob.content()) { - let text = content.to_string(); - total_num_lines += text.lines().count(); - old_files.insert(path, text); - } else { - old_files.insert(path, "".to_string()); - } - } - } - true - }, - None, - None, - None, - ) - .map_err(|e| error::Error::from_ctx_and_error("Error getting old file contents", e))?; - - diff.print(git2::DiffFormat::Patch, |_delta, _hunk, line| { - match line.origin() { - // For Addition, Deletion, and Context lines, the prefix needs to be prepended - '+' | '-' | ' ' | '@' => { - patch_output.push(line.origin()); - } - // For any other line type (e.g., file headers, hunk headers), - // the content is already fully formatted and should not be prefixed. - _ => {} - } - if let Ok(text) = str::from_utf8(line.content()) { - patch_output.push_str(text); + pub fn from_repo_and_patch(repo: &git2::Repository, patch: &mut git2::Patch) -> Diff { + let (old, new, diff_type) = { + let delta = patch.delta(); + let diff_type = if delta.flags().is_binary() { + DiffKind::Binary } else { - patch_output.push_str(""); - } - true - }) - .map_err(|e| error::Error::from_ctx_and_error("Error creating patch", e))?; + DiffKind::Text + }; + ( + DiffFile::try_from_repo_and_diff_file(repo, &delta.old_file()), + DiffFile::try_from_repo_and_diff_file(repo, &delta.new_file()), + diff_type, + ) + }; - Ok(Self { - patch: patch_output, - stats: DiffStats::from_stats_and_total_old_num_lines( - &diff - .stats() - .map_err(|e| error::Error::from_ctx_and_error("Error getting diff stats", e))?, - total_num_lines, - ), - old_sources: old_files, - }) + let patch_buf = patch.to_buf().expect("failed unwrapping patch buffer"); + let patch_text = patch_buf.as_str().unwrap_or("").to_owned(); + Self { + old, + new, + kind: diff_type, + patch: patch_text, + } + } + + pub fn binary_from_repo_and_delta(repo: &git2::Repository, delta: &git2::DiffDelta) -> Diff { + let old = DiffFile::try_from_repo_and_diff_file(repo, &delta.old_file()); + let new = DiffFile::try_from_repo_and_diff_file(repo, &delta.new_file()); + Self { + old, + new, + kind: DiffKind::Binary, + patch: "".to_string(), + } } } diff --git a/backend/git2-ox/src/repository.rs b/backend/git2-ox/src/repository.rs index 44f7787..f858e24 100644 --- a/backend/git2-ox/src/repository.rs +++ b/backend/git2-ox/src/repository.rs @@ -92,11 +92,15 @@ impl Repository { Commit::try_for_revision(&self.repo, rev) } - fn git2_diff_for_revisions( + /// Returns an iterator over diffs between two revisions `base_rev` and `head_rev` + /// + /// * `base_rev` - Base revision to use as a tree, uses initial commit if set to `None` + /// * `head_rev` - Head revision until which to diff. Using current `HEAD` if set to `None` + pub fn iter_diffs_between_revisions( &self, base_rev: Option<&str>, head_rev: Option<&str>, - ) -> Result { + ) -> Result>> { let head = head_rev.unwrap_or("HEAD"); let tree = utils::get_tree_for_revision(&self.repo, head)?; @@ -118,15 +122,28 @@ impl Repository { // Enable rename detection with DiffFindOptions let mut find_opts = git2::DiffFindOptions::new(); find_opts.renames(true); - // Transform a diff marking file renames, copies, etc. + // Transform the diff, marking file renames, copies, etc. diff.find_similar(Some(&mut find_opts)) .map_err(|e| Error::from_ctx_and_error("Failed to find similar files in diff", e))?; - Ok(diff) - } - pub fn diff(&self, base_rev: Option<&str>, head_rev: Option<&str>) -> Result { - let diff = self.git2_diff_for_revisions(base_rev, head_rev)?; - Diff::try_from_repo_and_diff(self.repo(), &diff) + let num_deltas = diff.deltas().len(); + + Ok((0..num_deltas).filter_map(move |delta_idx| { + // `diff` is owned by this function and must not be moved into the closure. + // Using `move` only for delta_idx, not for diff. + match git2::Patch::from_diff(&diff, delta_idx) { + Err(e) => Some(Err(Error::from_ctx_and_error("Failed to create patch", e))), + Ok(Some(mut patch)) => Some(Ok(Diff::from_repo_and_patch(&self.repo, &mut patch))), + Ok(None) => { + if let Some(delta) = diff.get_delta(delta_idx) { + Some(Ok(Diff::binary_from_repo_and_delta(&self.repo, &delta))) + } else { + log::error!("Failed to get delta for idx {delta_idx}"); + None + } + } + } + })) } /// Returns an iterator over tags in the repository which names contain `filter` diff --git a/frontend/src/client.ts b/frontend/src/client.ts index 32b8970..2714e10 100644 --- a/frontend/src/client.ts +++ b/frontend/src/client.ts @@ -144,13 +144,13 @@ export const fetchDiffs = async (range?: { baseRev?: string; headRev?: string; }) => { - const { data, error } = await client.GET("/api/v1/git/diff", { + const { data, error } = await client.GET("/api/v1/git/diffs", { params: { query: range }, }); if (error) { throw new Error(`Error fetching diffs: ${error.message}`); } - return data.diff; + return data.diffs; }; export async function fetchCommitsMetadata( diff --git a/frontend/src/components/badge-group.tsx b/frontend/src/components/badge-group.tsx deleted file mode 100644 index 6730749..0000000 --- a/frontend/src/components/badge-group.tsx +++ /dev/null @@ -1,35 +0,0 @@ -import { cn } from "@/lib/utils"; -import React from "react"; -import { Badge } from "./ui/badge"; - -type BadgeProps = React.ComponentProps; - -interface BadgeGroupProps { - segments: BadgeProps[]; -} - -export const BadgeGroup = React.memo( - ({ segments, ...props }: BadgeGroupProps & BadgeProps) => { - return ( - - {segments.map((segment, idx) => { - const classNames = []; - if (idx !== 0) { - classNames.push("rounded-l-none"); - } - if (idx !== segments.length - 1) { - classNames.push("rounded-r-none"); - } - return ( - - ); - })} - - ); - }, -); diff --git a/frontend/src/components/create-git-rev.tsx b/frontend/src/components/create-git-rev.tsx index 5fdbe4c..e001315 100644 --- a/frontend/src/components/create-git-rev.tsx +++ b/frontend/src/components/create-git-rev.tsx @@ -83,6 +83,7 @@ export const CreateGitRevisionInput: React.FC = ({ /> - diff --git a/frontend/src/components/diff-viewer/file.tsx b/frontend/src/components/diff-viewer/file.tsx index 4af2d59..038e7fb 100644 --- a/frontend/src/components/diff-viewer/file.tsx +++ b/frontend/src/components/diff-viewer/file.tsx @@ -177,6 +177,7 @@ export const DiffFile = React.memo(
This diff is large, load it manually.
); }; - -/** - * Simple diff viewer component only supporting inline diff view - */ -export const SimpleInlineDiffViewer = ({ diff }: { diff?: ApiDiff }) => { - if (!diff || diff.stats.filesChanged === 0) { - return ( -
- No diffs to display -
- ); - } - const files = parseDiff(diff.patch); - return ( -
-
- {files.map((file, idx) => ( - - ))} -
-
- ); -}; diff --git a/frontend/src/components/flows-dialog.tsx b/frontend/src/components/flows-dialog.tsx index 0363195..0ccff59 100644 --- a/frontend/src/components/flows-dialog.tsx +++ b/frontend/src/components/flows-dialog.tsx @@ -245,7 +245,11 @@ export const FlowsDialog: React.FC = ({ {flow.name}{" "} - @@ -281,6 +285,7 @@ export const FlowsDialog: React.FC = ({ {/* --- SECTION ONE FOOTER --- */}
@@ -85,11 +84,11 @@ export const GitRevisionsPanel = () => { /> )} {hasRevisions && ( - + Git Revisions - + {pinnedGitRevisions.map( (rev, index) => rev && ( diff --git a/frontend/src/components/git-stats.tsx b/frontend/src/components/git-stats.tsx index f0f7774..fb98763 100644 --- a/frontend/src/components/git-stats.tsx +++ b/frontend/src/components/git-stats.tsx @@ -24,16 +24,13 @@ export const GitStats = ({ return ( <> {insertedLines !== 0 && ( - + +{insertedLines} )} {deletedLines !== 0 && ( -{deletedLines} @@ -59,8 +56,7 @@ export const GitStatsChart = React.memo( insertedLines, deletedLines, oldSourceNumLines, - ...props - }: GitStatsChartProps & GitStatsProps & React.ComponentProps<"div">) => { + }: GitStatsChartProps & GitStatsProps) => { // Prevent division by zero and handle the case with no lines. const hasLines = oldSourceNumLines > 0; @@ -87,10 +83,7 @@ export const GitStatsChart = React.memo( }); return ( -
+
{segments.map((colorClass, index) => ( diff --git a/frontend/src/components/git-status-card.tsx b/frontend/src/components/git-status-card.tsx index 149c513..9f0ad9e 100644 --- a/frontend/src/components/git-status-card.tsx +++ b/frontend/src/components/git-status-card.tsx @@ -20,7 +20,7 @@ export function GitStatusCard({ status, footer }: GitStatusCardProps) { const isDetached = !isBranchMetadata(revision); return ( - + Git Status @@ -34,13 +34,13 @@ export function GitStatusCard({ status, footer }: GitStatusCardProps) { )} - +
-
+
{formatGitRevision(revision)}
-
{revision.summary}
+
{revision.summary}
{footer && ( {footer(status)} diff --git a/frontend/src/components/icon-select.tsx b/frontend/src/components/icon-select.tsx index 7883ac4..934d7b3 100644 --- a/frontend/src/components/icon-select.tsx +++ b/frontend/src/components/icon-select.tsx @@ -21,6 +21,7 @@ export const IconSelect = ({