feat: add ImageDiff component to load and show before/after images #11456
Conversation
|
@nshcr is attempting to deploy a commit to the GitButler Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
rust
There was a problem hiding this comment.
I avoided using project.open_git2() as I saw it is marked deprecated, and used the gix API instead.
I'm not entirely sure if this is the correct approach here.
There was a problem hiding this comment.
I think it's a good call. The repo needs to be opened anyway, and then using gix is faster.
Since it's all in legacy-land, I think it's fine to do what's needed.
However, I don't think that anything but full hashes may be supported here. The frontend already has blob hashes for anything one would want to diff.
Also I think there is a way to read an arbitrary file from within the worktree, in case of files that want comparison between worktree/Git.
|
@Byron — could you please help review the Rust changes? I added a new function to @PavelLaptev — could you please help review the frontend changes and adjust the styling so that the UI better matches GitButler's design? Thanks in advance for taking the time to review this and for any feedback or improvements you can suggest! |
|
Oh, this PR could fix #9946. |
Byron
left a comment
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot for getting this started, that's such a useful feature!
I just went ahead and made the changes I'd need to see with some explanation in the commit message. Probably I don't understand where the need for arbitrary revspecs is coming from.
With that it looks good on the Rust side.
Something I could use your help with is to understand how your are diffing worktree/Git changes. I also vaguely remember the presence of a function to obtain a worktree file, but I can't find it now. Ah, right, it's readFromWorkspace and it's already used.
There was a problem hiding this comment.
I think it's a good call. The repo needs to be opened anyway, and then using gix is faster.
Since it's all in legacy-land, I think it's fine to do what's needed.
However, I don't think that anything but full hashes may be supported here. The frontend already has blob hashes for anything one would want to diff.
Also I think there is a way to read an arbitrary file from within the worktree, in case of files that want comparison between worktree/Git.
|
Oh, and allow me unleash Copilot on this PR as I actually value what it has to say, most of the time 😅. Please feel free to ignore entirely. |
There was a problem hiding this comment.
Pull request overview
This PR adds visual diff previews for image files in the diff view, enabling before-and-after comparisons for image changes. It introduces a new ImageDiff Svelte component and supporting backend functions to load image content from commits, workspaces, and blob objects. The feature intelligently detects image files by extension and displays them with appropriate visual treatment for additions, modifications, renames, and deletions.
Key Changes
- Added
get_blob_fileRust API function to retrieve file content directly from Git blob objects - Extended
FileServicewithreadFromCommitandreadFromBlobmethods for flexible image loading - Created new
ImageDiffcomponent with responsive layout and support for all Git change types
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
crates/but-api/src/legacy/repo.rs |
Added get_blob_file function to retrieve files by blob ID; enhanced get_commit_file to support refs like "HEAD" |
crates/gitbutler-tauri/src/main.rs |
Registered new get_blob_file command for Tauri app |
crates/but-server/src/lib.rs |
Registered new get_blob_file command for CLI/server |
apps/desktop/src/lib/files/fileService.ts |
Added readFromCommit and readFromBlob methods to support different image loading strategies |
apps/desktop/src/components/UnifiedDiffView.svelte |
Added isImageFile helper and conditional rendering to show ImageDiff for image files |
apps/desktop/src/components/ImageDiff.svelte |
New component implementing visual diff with before/after panels, responsive layout, and error handling |
| <script lang="ts"> | ||
| import { FILE_SERVICE } from '$lib/files/fileService'; | ||
| import { inject } from '@gitbutler/core/context'; | ||
| import type { TreeChange } from '$lib/hunks/change'; | ||
| type Props = { | ||
| projectId: string; | ||
| change: TreeChange; | ||
| /** If provided, this is a commit diff (not a workspace diff). */ | ||
| commitId?: string; | ||
| }; | ||
| type ImageSource = | ||
| | { type: 'workspace'; path: string } | ||
| | { type: 'commit'; path: string; commitId: string } | ||
| | { type: 'blob'; path: string; blobId: string }; | ||
| type LoadStrategy = { | ||
| before: ImageSource | null; | ||
| after: ImageSource | null; | ||
| }; | ||
| const { projectId, change, commitId }: Props = $props(); | ||
| const fileService = inject(FILE_SERVICE); | ||
| let beforeImageUrl = $state<string | null>(null); | ||
| let afterImageUrl = $state<string | null>(null); | ||
| let loadError = $state<string | null>(null); | ||
| // Decide image sources for before/after panels without changing logic. | ||
| function getLoadStrategy(): LoadStrategy { | ||
| const { status, path } = change; | ||
| const isCommitDiff = !!commitId; | ||
| switch (status.type) { | ||
| case 'Addition': | ||
| return isCommitDiff | ||
| ? { before: null, after: { type: 'commit' as const, path, commitId: commitId! } } | ||
| : { before: null, after: { type: 'workspace' as const, path } }; | ||
| case 'Deletion': | ||
| return isCommitDiff | ||
| ? { before: { type: 'commit' as const, path, commitId: `${commitId}^` }, after: null } | ||
| : { | ||
| before: { type: 'blob' as const, path, blobId: status.subject.previousState.id }, | ||
| after: null | ||
| }; | ||
| case 'Modification': | ||
| return isCommitDiff | ||
| ? { | ||
| before: { type: 'commit' as const, path, commitId: `${commitId}^` }, | ||
| after: { type: 'commit' as const, path, commitId: commitId! } | ||
| } | ||
| : { | ||
| before: { type: 'blob' as const, path, blobId: status.subject.previousState.id }, | ||
| after: { type: 'workspace' as const, path } | ||
| }; | ||
| case 'Rename': | ||
| return isCommitDiff | ||
| ? { | ||
| before: { | ||
| type: 'commit' as const, | ||
| path: status.subject.previousPath, | ||
| commitId: `${commitId}^` | ||
| }, | ||
| after: { type: 'commit' as const, path, commitId: commitId! } | ||
| } | ||
| : { | ||
| before: { | ||
| type: 'blob' as const, | ||
| path: status.subject.previousPath, | ||
| blobId: status.subject.previousState.id | ||
| }, | ||
| after: { type: 'workspace' as const, path } | ||
| }; | ||
| } | ||
| } | ||
| // Load image from workspace, commit, or blob. | ||
| async function loadImage(source: ImageSource | null): Promise<string | null> { | ||
| if (!source) return null; | ||
| try { | ||
| let fileInfo; | ||
| if (source.type === 'workspace') { | ||
| const { data } = await fileService.readFromWorkspace(source.path, projectId); | ||
| fileInfo = data; | ||
| } else if (source.type === 'commit') { | ||
| fileInfo = await fileService.readFromCommit(source.path, projectId, source.commitId); | ||
| } else { | ||
| // type === 'blob' | ||
| fileInfo = await fileService.readFromBlob(source.path, projectId, source.blobId); | ||
| } | ||
| if (fileInfo?.content && fileInfo?.mimeType) { | ||
| return `data:${fileInfo.mimeType};base64,${fileInfo.content}`; | ||
| } | ||
| return null; | ||
| } catch (err) { | ||
| console.warn(`Failed to load image from ${source.type}: ${source.path}`, err); | ||
| return null; | ||
| } | ||
| } | ||
| // Load both images according to the strategy. | ||
| async function loadImages() { | ||
| loadError = null; | ||
| beforeImageUrl = null; | ||
| afterImageUrl = null; | ||
| try { | ||
| const strategy = getLoadStrategy(); | ||
| const [before, after] = await Promise.all([ | ||
| loadImage(strategy.before), | ||
| loadImage(strategy.after) | ||
| ]); | ||
| beforeImageUrl = before; | ||
| afterImageUrl = after; | ||
| if (!before && !after) { | ||
| loadError = 'Failed to load images'; | ||
| } | ||
| } catch (err) { | ||
| console.error('Failed to load images:', err); | ||
| loadError = 'Failed to load images'; | ||
| } | ||
| } | ||
| $effect(() => { | ||
| loadImages(); | ||
| }); | ||
| </script> | ||
|
|
||
| {#if loadError} | ||
| <div class="error-message"> | ||
| <p>{loadError}</p> | ||
| </div> | ||
| {:else} | ||
| <div class="image-diff-container"> | ||
| {#if beforeImageUrl || afterImageUrl} | ||
| <div class="image-comparison"> | ||
| {#if beforeImageUrl} | ||
| <div class="image-panel before"> | ||
| <div class="image-header"> | ||
| <span class="label">Before</span> | ||
| </div> | ||
| <div class="image-wrapper"> | ||
| <img src={beforeImageUrl} alt="Before" /> | ||
| </div> | ||
| </div> | ||
| {/if} | ||
|
|
||
| {#if afterImageUrl} | ||
| <div class="image-panel after"> | ||
| <div class="image-header"> | ||
| <span class="label">After</span> | ||
| </div> | ||
| <div class="image-wrapper"> | ||
| <img src={afterImageUrl} alt="After" /> | ||
| </div> | ||
| </div> | ||
| {/if} | ||
| </div> | ||
| {:else} | ||
| <div class="loading">Loading images...</div> | ||
| {/if} | ||
| </div> | ||
| {/if} | ||
|
|
||
| <style lang="postcss"> | ||
| .error-message { | ||
| padding: 20px; | ||
| color: var(--clr-scale-warn-40); | ||
| text-align: center; | ||
| } | ||
| .image-diff-container { | ||
| padding: 14px; | ||
| border: 1px solid var(--clr-border-3); | ||
| border-radius: var(--radius-m); | ||
| background: var(--clr-bg-2); | ||
| } | ||
| .image-comparison { | ||
| display: grid; | ||
| grid-template-columns: repeat(auto-fit, minmax(300px, 1fr)); | ||
| gap: 14px; | ||
| } | ||
| .image-panel { | ||
| overflow: hidden; | ||
| border-radius: var(--radius-s); | ||
| background: var(--clr-bg-1); | ||
| &.before { | ||
| border: 2px solid var(--clr-scale-err-40); | ||
| } | ||
| &.after { | ||
| border: 2px solid var(--clr-scale-succ-40); | ||
| } | ||
| } | ||
| .image-header { | ||
| padding: 8px 12px; | ||
| font-weight: 600; | ||
| font-size: 12px; | ||
| letter-spacing: 0.5px; | ||
| text-transform: uppercase; | ||
| .before & { | ||
| background: var(--clr-scale-err-40); | ||
| color: var(--clr-core-ntrl-100); | ||
| } | ||
| .after & { | ||
| background: var(--clr-scale-succ-40); | ||
| color: var(--clr-core-ntrl-100); | ||
| } | ||
| } | ||
| .label { | ||
| display: inline-block; | ||
| } | ||
| .image-wrapper { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| min-height: 200px; | ||
| padding: 14px; | ||
| background-image: | ||
| linear-gradient(45deg, var(--clr-bg-3) 25%, transparent 25%), | ||
| linear-gradient(-45deg, var(--clr-bg-3) 25%, transparent 25%), | ||
| linear-gradient(45deg, transparent 75%, var(--clr-bg-3) 75%), | ||
| linear-gradient(-45deg, transparent 75%, var(--clr-bg-3) 75%); | ||
| background-position: | ||
| 0 0, | ||
| 0 10px, | ||
| 10px -10px, | ||
| -10px 0px; | ||
| background-size: 20px 20px; | ||
| } | ||
| .image-wrapper img { | ||
| display: block; | ||
| max-width: 100%; | ||
| max-height: 600px; | ||
| object-fit: contain; | ||
| border-radius: var(--radius-s); | ||
| } | ||
| .loading { | ||
| padding: 40px; | ||
| color: var(--clr-scale-ntrl-50); | ||
| text-align: center; | ||
| } | ||
| </style> |
There was a problem hiding this comment.
The new ImageDiff component lacks test coverage. Given that the desktop app has comprehensive E2E testing (e.g., unifiedDiffView.cy.ts), this new feature should have corresponding tests to verify:
- Image loading for different change types (Addition, Deletion, Modification, Rename)
- Both workspace and commit diff scenarios
- Error handling when images fail to load
- Layout behavior for different window sizes
Consider adding E2E tests in a file like apps/desktop/cypress/e2e/imageDiff.cy.ts to cover these scenarios.
| function isImageFile(path: string): boolean { | ||
| const imageExtensions = [ | ||
| '.png', | ||
| '.jpg', | ||
| '.jpeg', | ||
| '.gif', | ||
| '.webp', | ||
| '.bmp', | ||
| '.ico', | ||
| '.heic', | ||
| '.heif', | ||
| '.avif' | ||
| ]; | ||
| const lowerPath = path.toLowerCase(); | ||
| return imageExtensions.some((ext) => lowerPath.endsWith(ext)); | ||
| } |
There was a problem hiding this comment.
[nitpick] The isImageFile function is defined inline within the component, but this utility function would be better placed in a shared utilities file since it could be reused elsewhere in the codebase. Consider extracting this to a shared file like apps/desktop/src/lib/utils/files.ts or similar.
Additionally, the function could benefit from a more comprehensive list of image extensions. Consider adding .tiff/.tif for completeness.
Added a doc comment to the get_blob_file function explaining its purpose, usage, and arguments. This improves code clarity for developers working with image diff previews and blob retrieval.
Updated the alt attributes for before and after images to include the file path and context, enhancing accessibility and clarity for screen readers.
Enhanced error handling to provide more specific messages when before or after images fail to load. Error messages now indicate which image failed and include error details from exceptions.
Introduces AbortSignal handling to image loading functions, allowing image fetches to be cancelled if the component unmounts or the effect is cleaned up. This improves resource management and prevents unnecessary work when the component is no longer active.
Replaces the inline image diff logic in the desktop app with a new reusable ImageDiff component in the UI package, supporting 2-up, swipe, and onion-skin comparison modes, metadata display, and loading states. Updates RangeInput to use a simpler, more accessible range fill style. Adjusts EmptyStatePlaceholder and SkeletonBone styles for consistency. Adds Storybook stories for ImageDiff. Updates exports in the UI index.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
@PavelLaptev Sorry I haven't had time to handle this PR over the past few days, and thank you so much for the updates you've made! If Byron thinks the Rust changes look good and are ready to be merged, I'm completely fine with that as well — honestly, I don't really have a clear idea of what should be changed on the Rust side... As for the frontend changes — they're fantastic! Far beyond what I originally expected for this feature. The Swipe and Onion Skin modes are incredibly useful. I was thinking: for Swipe mode, would it be more intuitive to place the drag handle in the center of the image and use a semi-transparent center line to separate the before / after images? (similar to Squoosh) Also, I think replacing the "before / after" text with "deleted / added" would be more appropriate and consistent with the labels used by GitHub. Looking back, using "before / after" was probably not the best choice on my part. |
* use `open_isolated_repo()` to indicate we just need ODB access and nothing else matters. * don't make the impression we support anything but full hashes to refer to objects. The UI knows these.
|
This looks fantastic now, amazing work! Regarding Rust, I have rebased my refactor so now I'd think we are good on the Rust side. CI seems to fail genuinely, but I wouldn't know how to fix it. But besides that, I think it's good to be merged.
Let's debug this in a follow-up. It's probably something about the backend side being made with text files in mind, somewhere, but I didn't try to reproduce it yet. |
| function getLoadStrategy(): LoadStrategy { | ||
| const { status, path } = change; | ||
| const isCommitDiff = !!commitId; | ||
| switch (status.type) { | ||
| case 'Addition': | ||
| return isCommitDiff | ||
| ? { before: null, after: { type: 'commit' as const, path, commitId: commitId! } } | ||
| : { before: null, after: { type: 'workspace' as const, path } }; | ||
| case 'Deletion': | ||
| return isCommitDiff | ||
| ? { before: { type: 'commit' as const, path, commitId: `${commitId}^` }, after: null } | ||
| : { | ||
| before: { type: 'blob' as const, path, blobId: status.subject.previousState.id }, | ||
| after: null | ||
| }; | ||
| case 'Modification': | ||
| return isCommitDiff | ||
| ? { | ||
| before: { type: 'commit' as const, path, commitId: `${commitId}^` }, | ||
| after: { type: 'commit' as const, path, commitId: commitId! } | ||
| } | ||
| : { | ||
| before: { type: 'blob' as const, path, blobId: status.subject.previousState.id }, | ||
| after: { type: 'workspace' as const, path } | ||
| }; | ||
| case 'Rename': | ||
| return isCommitDiff | ||
| ? { | ||
| before: { | ||
| type: 'commit' as const, | ||
| path: status.subject.previousPath, | ||
| commitId: `${commitId}^` | ||
| }, | ||
| after: { type: 'commit' as const, path, commitId: commitId! } | ||
| } | ||
| : { | ||
| before: { | ||
| type: 'blob' as const, | ||
| path: status.subject.previousPath, | ||
| blobId: status.subject.previousState.id | ||
| }, | ||
| after: { type: 'workspace' as const, path } | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
The getLoadStrategy function handles complex logic for determining how to load images based on different change types (Addition, Deletion, Modification, Rename) and contexts (commit diff vs workspace diff). This critical logic should have unit tests to ensure correctness across all scenarios. Consider adding tests to verify the strategy selection for each combination of change type and context.
| async function loadImage( | ||
| source: ImageSource | null, | ||
| signal?: AbortSignal | ||
| ): Promise<string | null> { | ||
| if (!source) return null; | ||
| try { | ||
| let fileInfo; | ||
| if (source.type === 'workspace') { | ||
| const { data } = await fileService.readFromWorkspace(source.path, projectId); | ||
| fileInfo = data; | ||
| } else if (source.type === 'commit') { | ||
| fileInfo = await fileService.readFromCommit(source.path, projectId, source.commitId); | ||
| } else { | ||
| // type === 'blob' | ||
| fileInfo = await fileService.readFromBlob(source.path, projectId, source.blobId); | ||
| } | ||
| if (signal?.aborted) return null; | ||
| if (fileInfo?.content && fileInfo?.mimeType) { | ||
| return `data:${fileInfo.mimeType};base64,${fileInfo.content}`; | ||
| } | ||
| return null; | ||
| } catch (err) { | ||
| if (signal?.aborted) return null; | ||
| console.warn(`Failed to load image from ${source.type}: ${source.path}`, err); | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The loadImage function includes error handling and abort signal logic that should be covered by tests. This would ensure proper handling of loading failures, race conditions with abort signals, and correct data URL generation from file content.
Added error handling to the image metadata loading logic in ImageDiff.svelte. Now, if loading metadata fails for either image, an error is logged and the corresponding metadata is set to null.
Clarified that backend service methods do not support AbortSignal for IO cancellation, and the signal is only used to prevent processing after abortion in the loadImage function.
Refactored the swipe comparison mode to support drag interaction using mouse and touch events. Added a visual handle for the swipe divider, improved accessibility with ARIA attributes, and separated swipe and onion-skin modes for clearer logic and styling.
Moved the isImageFile function from UnifiedDiffView.svelte to packages/shared/src/lib/utils/file.ts for reuse and improved maintainability. UnifiedDiffView now imports isImageFile from the shared utils module.
Good idea. Done Screen.Recording.2025-12-08.at.10.46.46.mov |
"After"/"Before" works well when comparing two images. It clearly shows what changed when an image was replaced. "Removed"/"Added" is more descriptive for single images, but less intuitive than "After"/"Before" for replacements. Screen.Recording.2025-12-08.at.10.57.05.mov |
Moves sample image data URLs to the top of the file and sets them as default args for the ImageDiff story. This improves demonstration clarity and removes redundant code.
| case 'Deletion': | ||
| return isCommitDiff | ||
| ? { before: { type: 'commit' as const, path, commitId: `${commitId}^` }, after: null } |
There was a problem hiding this comment.
For commit diffs with deletions, the code constructs ${commitId}^ to reference the parent commit. This assumes the commit has a parent, but the initial commit in a repository has no parent. While this is likely an edge case, consider adding error handling or documentation about this limitation. The backend will likely return an error for the initial commit, but the error message might not be user-friendly.
| async function loadImageMetadata(url: string): Promise<ImageMetadata> { | ||
| return new Promise((resolve, reject) => { | ||
| const img = new Image(); | ||
| img.onload = async () => { | ||
| const metadata: ImageMetadata = { | ||
| width: img.naturalWidth, | ||
| height: img.naturalHeight | ||
| }; | ||
| // Try to fetch file size | ||
| try { | ||
| const response = await fetch(url); | ||
| const blob = await response.blob(); | ||
| metadata.size = blob.size; | ||
| } catch { | ||
| // Size not available, continue without it | ||
| } | ||
| resolve(metadata); | ||
| }; | ||
| img.onerror = () => reject(new Error('Failed to load image')); | ||
| img.src = url; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The loadImageMetadata function fetches the image twice - once via new Image() to get dimensions, and then again via fetch() to get the file size. For data URLs (which is what this component receives), the second fetch will re-download/re-decode the entire image. Consider calculating the size from the data URL itself (by decoding the base64 string) or accepting it as a prop from the parent component, which already has access to the FileInfo with size information.
Introduces keyboard navigation for the image comparison slider, allowing users to adjust the swipe position using arrow keys, Home, End, and Shift for larger steps. Enhances accessibility and usability for keyboard users.
Eliminates the col-resize cursor style from the header element in ImageDiff.svelte, likely to improve UX or clarify that the header is not resizable.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Added a typeof check for TouchEvent before using it in handleDragMove to prevent errors in environments where TouchEvent is undefined.
| $effect(() => { | ||
| if (beforeImageUrl) { | ||
| loadImageMetadata(beforeImageUrl) | ||
| .then((metadata) => (beforeImageMetadata = metadata)) | ||
| .catch((error) => { | ||
| console.error('Failed to load before image metadata:', error); | ||
| beforeImageMetadata = null; | ||
| }); | ||
| } | ||
| if (afterImageUrl) { | ||
| loadImageMetadata(afterImageUrl) | ||
| .then((metadata) => (afterImageMetadata = metadata)) | ||
| .catch((error) => { | ||
| console.error('Failed to load after image metadata:', error); | ||
| afterImageMetadata = null; | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The $effect hook that loads image metadata (lines 80-98) doesn't clean up when beforeImageUrl or afterImageUrl changes. If the URLs change while metadata is still loading, the old promises will still resolve and update state, potentially causing a race condition where outdated metadata is displayed. Consider using an AbortController or a similar cleanup mechanism to cancel in-flight requests when dependencies change.
| function handleKeyDown(e: KeyboardEvent) { | ||
| const step = e.shiftKey ? 10 : 1; // Larger steps with Shift key | ||
| switch (e.key) { | ||
| case 'ArrowLeft': | ||
| case 'ArrowDown': | ||
| e.preventDefault(); | ||
| swipePosition = Math.max(0, swipePosition - step); | ||
| break; | ||
| case 'ArrowRight': | ||
| case 'ArrowUp': | ||
| e.preventDefault(); | ||
| swipePosition = Math.min(100, swipePosition + step); | ||
| break; | ||
| case 'Home': |
There was a problem hiding this comment.
The event listeners for swipe functionality are only conditionally added when viewMode === 'swipe', but the event handlers (handleDragMove, handleDragEnd) check isDragging which could be true from a previous interaction. If a user starts dragging in swipe mode, then switches to another mode while dragging, isDragging will remain true and the drag handlers could be called if the user re-enables swipe mode. Consider resetting isDragging = false when the effect cleanup runs or when the view mode changes.
| <div class="imagediff-placeholder"> | ||
| <EmptyStatePlaceholder image={emptyFileSvg} gap={12} topBottomPadding={34}> | ||
| {#snippet caption()} | ||
| Can't preview this file type |
There was a problem hiding this comment.
The error message displayed to users ("Can't preview this file type") doesn't match the actual error scenarios. The loadError variable contains specific error messages like "Failed to load before image" or "Failed to load images: [error message]", but the UI always shows the generic "Can't preview this file type" message. Consider either:
- Displaying the actual error message from
loadErrorto help users understand what went wrong, or - Using more specific user-friendly messages that reflect the actual error condition
The current implementation makes debugging difficult for users as they can't tell if it's a network issue, missing file, or an actual unsupported file type.
| Can't preview this file type | |
| {loadError} |
| export function isImageFile(path: string): boolean { | ||
| const lowerPath = path.toLowerCase(); | ||
| return IMAGE_EXTENSIONS.some((ext) => lowerPath.endsWith(ext)); | ||
| } |
There was a problem hiding this comment.
The new isImageFile function lacks test coverage. Since this file has comprehensive tests for other functions (see file.test.ts), this new function should also have test coverage to ensure it correctly identifies image files with various extensions and handles edge cases (e.g., uppercase extensions, files without extensions, etc.).
| /// Retrieves file content directly from a Git blob object by its blob ID. | ||
| /// | ||
| /// This function is used for displaying image diff previews when the file | ||
| /// isn't available in the current workspace or a specific commit (e.g., for | ||
| /// deleted files or when comparing against a previous state). | ||
| /// | ||
| /// # Arguments | ||
| /// * `blob_id` - Git blob object ID as a hexadecimal string | ||
| #[but_api] | ||
| #[instrument(err(Debug))] | ||
| pub fn get_blob_file( | ||
| project_id: ProjectId, | ||
| relative_path: PathBuf, | ||
| blob_id: HexHash, | ||
| ) -> Result<FileInfo> { | ||
| let project = gitbutler_project::get(project_id)?; | ||
| let repo = project.open_isolated_repo()?; | ||
| let object = repo.find_object(blob_id).context("Failed to find blob")?; | ||
| let blob = object.try_into_blob().context("Object is not a blob")?; | ||
| Ok(FileInfo::from_content(&relative_path, &blob.data)) | ||
| } |
There was a problem hiding this comment.
The get_blob_file function lacks test coverage. Since this is a new backend API function that handles file content retrieval, it should have tests to verify:
- Correct blob loading from valid blob IDs
- Proper error handling when blob ID is invalid or not found
- Correct MIME type detection and content encoding
This is especially important as this function is used by the image diff feature and errors here could lead to broken image previews.
3 participants
This PR adds before-and-after visual diff previews for image files in the diff view.
Most of the code was generated with the help of Claude Sonnet 4.5, and was reviewed by me before submitting the PR to ensure code quality and correct functionality.
It covers the following scenarios: