Merge pull request #9864 from gitbutlerapp/fix-broken-history-previews

Fix broken history previews

Author: Caleb-T-Owens

apps/desktop/src/components/FeedItem.svelte

@@ -2,6 +2,7 @@
 	import FeedActionDiff from '$components/FeedActionDiff.svelte';
 	import FeedItemKind from '$components/FeedItemKind.svelte';
 	import FeedStreamMessage from '$components/FeedStreamMessage.svelte';
+	import ReduxResult from '$components/ReduxResult.svelte';
 	import {
 		allCommitsUpdated,
 		ButlerAction,
@@ -14,6 +15,7 @@
 	} from '$lib/actions/types';
 	import butbotSvg from '$lib/assets/butbot-actions.svg?raw';
 	import { isFeedMessage, isInProgressAssistantMessage, type FeedEntry } from '$lib/feed/feed';
+	import { HISTORY_SERVICE } from '$lib/history/history';
 	import { Snapshot } from '$lib/history/types';
 	import { USER } from '$lib/user/user';
 	import { inject } from '@gitbutler/shared/context';
@@ -27,6 +29,14 @@
 	const { action, projectId }: Props = $props();
 
 	const user = inject(USER);
+	const historyService = inject(HISTORY_SERVICE);
+
+	// Get snapshot diff for Snapshot feed items
+	const snapshotDiff = $derived(
+		action instanceof Snapshot
+			? historyService.snapshotDiff({ projectId, snapshotId: action.id })
+			: null
+	);
 	let failedToLoadImage = $state(false);
 
 	function workflowTriggerTooltip(workflow: Workflow): string {
@@ -142,9 +152,15 @@
 				{#if action.details?.body}
 					<Markdown content={action.details?.body} />
 				{/if}
-				{#each action.filesChanged as file}
-					<span class="text-greyer">{file}</span>
-				{/each}
+				{#if snapshotDiff}
+					<ReduxResult result={snapshotDiff.current} {projectId}>
+						{#snippet children(files)}
+							{#each files as file}
+								<span class="text-greyer">{file.path}</span>
+							{/each}
+						{/snippet}
+					</ReduxResult>
+				{/if}
 			</span>
 		</div>
 	{:else if action instanceof Workflow}

apps/desktop/src/components/SnapshotCard.svelte

@@ -1,6 +1,7 @@
 <script lang="ts">
+	import ReduxResult from '$components/ReduxResult.svelte';
 	import SnapshotAttachment from '$components/SnapshotAttachment.svelte';
-	import { createdOnDay } from '$lib/history/history';
+	import { createdOnDay, HISTORY_SERVICE } from '$lib/history/history';
 	import { MODE_SERVICE } from '$lib/mode/modeService';
 	import { toHumanReadableTime } from '$lib/utils/time';
 	import { inject } from '@gitbutler/shared/context';
@@ -168,6 +169,9 @@
 
 	const modeService = inject(MODE_SERVICE);
 	const mode = $derived(modeService.mode({ projectId }));
+
+	const historyService = inject(HISTORY_SERVICE);
+	const snapshotDiff = $derived(historyService.snapshotDiff({ projectId, snapshotId: entry.id }));
 </script>
 
 <div
@@ -214,25 +218,25 @@
 			{/if}
 		</div>
 
-		{#if entry.filesChanged.length > 0 && !isRestoreSnapshot}
-			{@const files = entry.filesChanged}
-			<SnapshotAttachment
-				foldable={entry.filesChanged.length > 2}
-				foldedAmount={entry.filesChanged.length}
-			>
-				<div class="files-attacment">
-					{#each files as filePath}
-						<FileListItem
-							listMode="list"
-							{filePath}
-							onclick={() => onDiffClick(filePath)}
-							selected={selectedFile?.path === filePath && selectedFile?.entryId === entry.id}
-							hideBorder={filePath === files[files.length - 1]}
-						/>
-					{/each}
-				</div>
-			</SnapshotAttachment>
-		{/if}
+		<ReduxResult result={snapshotDiff.current} {projectId}>
+			{#snippet children(files)}
+				{#if files.length > 0 && !isRestoreSnapshot}
+					<SnapshotAttachment foldable={files.length > 2} foldedAmount={files.length}>
+						<div class="files-attacment">
+							{#each files as file, idx}
+								<FileListItem
+									listMode="list"
+									filePath={file.path}
+									onclick={() => onDiffClick(file.path)}
+									selected={selectedFile?.path === file.path && selectedFile?.entryId === entry.id}
+									hideBorder={idx === files.length - 1}
+								/>
+							{/each}
+						</div>
+					</SnapshotAttachment>
+				{/if}
+			{/snippet}
+		</ReduxResult>
 
 		{#if isRestoreSnapshot}
 			<SnapshotAttachment>

apps/desktop/src/lib/history/types.ts

@@ -56,9 +56,6 @@ export class SnapshotDetails {
 
 export class Snapshot {
 	id!: string;
-	linesAdded!: number;
-	linesRemoved!: number;
-	filesChanged!: string[];
 	@Type(() => SnapshotDetails)
 	details?: SnapshotDetails;
 	@Transform((obj) => new Date(obj.value * 1000))

apps/desktop/src/routes/[projectId]/history/+page.svelte

@@ -11,13 +11,11 @@
 	import emptyFolderSvg from '$lib/assets/empty-state/empty-folder.svg?raw';
 	import { HISTORY_SERVICE, createdOnDay } from '$lib/history/history';
 	import { ID_SELECTION } from '$lib/selection/idSelection.svelte';
-	import { createSnapshotSelection } from '$lib/selection/key';
+	import { createSnapshotSelection, type SelectionId } from '$lib/selection/key';
 	import { inject } from '@gitbutler/shared/context';
 	import { EmptyStatePlaceholder, Icon, FileViewHeader } from '@gitbutler/ui';
 	import { stickyHeader } from '@gitbutler/ui/utils/stickyHeader';
 	import type { Snapshot } from '$lib/history/types';
-	import type { TreeChange } from '$lib/hunks/change';
-	import type { SelectedFile } from '$lib/selection/key';
 
 	// TODO: Refactor so we don't need non-null assertion.
 	const projectId = $derived(page.params.projectId!);
@@ -37,9 +35,14 @@
 
 	const withinRestoreItems = $derived(findRestorationRanges($snapshots));
 
-	let snapshotFilesTempStore: { entryId: string; diffs: TreeChange[] } | undefined =
-		$state(undefined);
-	let selectedFile: (SelectedFile & { type: 'snapshot' }) | undefined = $state(undefined);
+	let currentSelectionId: SelectionId | undefined = $state(undefined);
+
+	// Derive selectedFile from the selection service
+	const selectedFile = $derived.by(() => {
+		if (!currentSelectionId) return undefined;
+		const selections = idSelection.values(currentSelectionId);
+		return selections.length > 0 ? selections[0] : undefined;
+	});
 
 	async function onLastInView() {
 		if (!$loading && !$isAllLoaded) await snapshotManager.loadMore();
@@ -67,16 +70,15 @@
 		return ranges.map((snapshot) => snapshot.id);
 	}
 
-	function updateFilePreview(entry: Snapshot, path: string) {
-		if (!snapshotFilesTempStore) return;
-
-		const file = snapshotFilesTempStore.diffs
-			.map((tc, i) => [tc, i] as const)
-			.find(([tc]) => tc.path === path);
-		if (!file) return;
-
+	async function updateFilePreview(entry: Snapshot, path: string) {
 		const selectionId = createSnapshotSelection({ snapshotId: entry.id });
-		idSelection.set(path, selectionId, file[1]);
+		// Get the diff data to find the file index
+		const diffs = await historyService.getSnapshotDiff(projectId, entry.id);
+		const fileIndex = diffs.findIndex((tc) => tc.path === path);
+		if (fileIndex === -1) return;
+
+		idSelection.set(path, selectionId, fileIndex);
+		currentSelectionId = selectionId;
 	}
 </script>
 
@@ -130,21 +132,20 @@
 									// Until we have that figured out, we need to reload the page.
 									location.reload();
 								}}
-								onDiffClick={async (path) => {
-									if (snapshotFilesTempStore?.entryId === entry.id) {
-										if (selectedFile?.path === path) {
-											selectedFile = undefined;
-										} else {
-											updateFilePreview(entry, path);
-										}
+								onDiffClick={(path) => {
+									if (
+										selectedFile?.path === path &&
+										selectedFile?.type === 'snapshot' &&
+										selectedFile.snapshotId === entry.id
+									) {
+										currentSelectionId = undefined;
 									} else {
-										snapshotFilesTempStore = {
-											entryId: entry.id,
-											diffs: await historyService.getSnapshotDiff(projectId, entry.id)
-										};
 										updateFilePreview(entry, path);
 									}
 								}}
+								selectedFile={currentSelectionId && selectedFile && selectedFile.type === 'snapshot'
+									? { entryId: selectedFile.snapshotId, path: selectedFile.path }
+									: undefined}
 							/>
 						{/if}
 					{/each}
@@ -204,16 +205,12 @@
 							filePath={selectedFile.path}
 							draggable={false}
 							oncloseclick={() => {
-								selectedFile = undefined;
+								currentSelectionId = undefined;
 							}}
 						/>
 					</div>
 
-					<SelectionView
-						{projectId}
-						diffOnly
-						selectionId={createSnapshotSelection({ snapshotId: selectedFile.snapshotId })}
-					/>
+					<SelectionView {projectId} diffOnly selectionId={currentSelectionId} />
 				</ConfigurableScrollableContainer>
 			</div>
 		{:else}

crates/gitbutler-branch-actions/tests/virtual_branches/oplog.rs

@@ -364,16 +364,6 @@ fn restores_gitbutler_workspace() -> anyhow::Result<()> {
         &all_snapshots[3..],
     );
 
-    let first_snapshot = all_snapshots.last().unwrap();
-    assert_eq!(
-        (
-            first_snapshot.lines_added,
-            first_snapshot.lines_removed,
-            first_snapshot.files_changed.len()
-        ),
-        (0, 0, 0),
-        "The first snapshot is intentionally not listing everything as changed"
-    );
     Ok(())
 }
 

crates/gitbutler-oplog/src/entry.rs

@@ -1,7 +1,6 @@
 use std::{
     fmt,
     fmt::{Debug, Display, Formatter},
-    path::PathBuf,
     str::FromStr,
 };
 
@@ -21,12 +20,6 @@ pub struct Snapshot {
     /// Snapshot creation time in seconds from Unix epoch seconds, based on a commit as `commit_id`.
     #[serde(serialize_with = "gitbutler_serde::as_time_seconds_from_unix_epoch")]
     pub created_at: git2::Time,
-    /// The number of working directory lines added in the snapshot
-    pub lines_added: usize,
-    /// The number of working directory lines removed in the snapshot
-    pub lines_removed: usize,
-    /// The list of working directory files that were changed in the snapshot
-    pub files_changed: Vec<PathBuf>,
     /// Snapshot details as persisted in the commit message, or `None` if the details couldn't be parsed.
     pub details: Option<SnapshotDetails>,
 }

crates/gitbutler-oplog/src/oplog.rs

@@ -29,7 +29,6 @@ use gitbutler_project::{
 use gitbutler_repo::RepositoryExt;
 use gitbutler_repo::SignaturePurpose;
 use gitbutler_stack::{Stack, VirtualBranchesHandle, VirtualBranchesState};
-use gix::object::tree::diff::Change;
 use gix::prelude::ObjectIdExt;
 use gix::{bstr::ByteSlice, ObjectId};
 use tracing::instrument;
@@ -197,7 +196,6 @@ impl OplogExt for CommandContext {
         .attach(&repo);
 
         let mut snapshots = Vec::new();
-        let mut wd_trees_cache: HashMap<gix::ObjectId, gix::ObjectId> = HashMap::new();
 
         for commit_info in traversal_root_id.ancestors().all()? {
             if snapshots.len() == limit {
@@ -224,14 +222,6 @@ impl OplogExt for CommandContext {
                 continue;
             }
 
-            // Get tree id from cache or calculate it
-            let wd_tree = repo.find_tree(get_workdir_tree(
-                Some(&mut wd_trees_cache),
-                commit_id,
-                &repo,
-                self,
-            )?)?;
-
             let commit_id = gix_to_git2_oid(commit_id);
             let details = commit
                 .message_raw()?
@@ -245,63 +235,12 @@ impl OplogExt for CommandContext {
                 }
             }
 
-            if let Some(parent_id) = first_parent {
-                // Get tree id from cache or calculate it
-
-                let mut files_changed = Vec::new();
-                let mut resource_cache = repo.diff_resource_cache_for_tree_diff()?;
-                let (mut lines_added, mut lines_removed) = (0, 0);
-                let parent_tree = repo.find_tree(get_workdir_tree(
-                    Some(&mut wd_trees_cache),
-                    parent_id,
-                    &repo,
-                    self,
-                )?)?;
-                parent_tree
-                    .changes()?
-                    .options(|opts| {
-                        opts.track_rewrites(None).track_path();
-                    })
-                    .for_each_to_obtain_tree(&wd_tree, |change| -> Result<_> {
-                        match change {
-                            Change::Addition { location, .. } => {
-                                files_changed.push(gix::path::from_bstr(location).into_owned());
-                            }
-                            Change::Deletion { .. }
-                            | Change::Modification { .. }
-                            | Change::Rewrite { .. } => {}
-                        }
-                        if let Some(counts) = change
-                            .diff(&mut resource_cache)
-                            .ok()
-                            .and_then(|mut platform| platform.line_counts().ok().flatten())
-                        {
-                            lines_added += u64::from(counts.insertions);
-                            lines_removed += u64::from(counts.removals);
-                        }
-                        resource_cache.clear_resource_cache_keep_allocation();
-
-                        Ok(gix::object::tree::diff::Action::Continue)
-                    })?;
-
-                snapshots.push(Snapshot {
-                    commit_id,
-                    details,
-                    lines_added: lines_added as usize,
-                    lines_removed: lines_removed as usize,
-                    files_changed,
-                    created_at: commit_time,
-                });
-            } else {
-                // this is the very first snapshot
-                snapshots.push(Snapshot {
-                    commit_id,
-                    details,
-                    lines_added: 0,
-                    lines_removed: 0,
-                    files_changed: Vec::new(),
-                    created_at: commit_time,
-                });
+            snapshots.push(Snapshot {
+                commit_id,
+                details,
+                created_at: commit_time,
+            });
+            if first_parent.is_none() {
                 break;
             }
         }

crates/gitbutler-oplog/tests/mod.rs

@@ -94,7 +94,7 @@ mod operation_kind {
 }
 
 mod snapshot_details {
-    use std::{path::PathBuf, str::FromStr};
+    use std::str::FromStr;
 
     use gitbutler_oplog::entry::{OperationKind, Snapshot, SnapshotDetails, Trailer, Version};
 
@@ -109,9 +109,6 @@ mod snapshot_details {
         let snapshot = Snapshot {
             commit_id: commit_sha,
             created_at,
-            lines_added: 1,
-            lines_removed: 1,
-            files_changed: vec![PathBuf::from("foo.txt")],
             details: Some(details),
         };
         assert_eq!(snapshot.commit_id, commit_sha);