Replace expandable commits with hover previews for improved UX #381
Conversation
|
@coderabbitai Please review the changes |
|
@mehmetozguldev: I'll review the changes in this PR that refactors the git commit history component from an expandable interface to a hover-based preview system. ✅ Actions performedReview triggered.
|
WalkthroughRefactored git commit history view to replace inline expanded state with hover-based preview. Changed CommitItem component to trigger hover events and portal-anchored preview. Updated internal state management for hover lifecycle with auto-close timeout, replacing prior expansion tracking. Changes
Sequence DiagramsequenceDiagram
participant User
participant CommitItem
participant GitCommitHistory
participant CommitHoverPreview
participant Portal
User->>CommitItem: Mouse Enter (hover)
CommitItem->>GitCommitHistory: onHover(commitHash, anchorRect)
GitCommitHistory->>GitCommitHistory: Set hoveredCommit state
GitCommitHistory->>CommitHoverPreview: Render with anchorRect
rect rgb(200, 220, 255)
note over Portal: Portal renders preview<br/>near commit row
end
CommitHoverPreview->>Portal: Render anchored panel
User->>CommitItem: Mouse Leave
CommitItem->>GitCommitHistory: onHoverEnd()
GitCommitHistory->>GitCommitHistory: Schedule close (HOVER_CLOSE_DELAY_MS)
alt User hovers preview before timeout
User->>CommitHoverPreview: Mouse Enter
CommitHoverPreview->>GitCommitHistory: onKeepOpen()
GitCommitHistory->>GitCommitHistory: Clear timeout
else Timeout expires
GitCommitHistory->>GitCommitHistory: Clear hoveredCommit
CommitHoverPreview->>Portal: Unmount preview
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/version-control/git/views/git-commit-history.tsx (1)
13-15: Consider the hover delay for UX responsiveness.The 250ms delay provides good stability to prevent accidental closures, but may feel slightly sluggish for power users rapidly exploring commits. Consider user testing to validate this timing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/version-control/git/views/git-commit-history.tsx(10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check format and cargo check
🔇 Additional comments (5)
src/version-control/git/views/git-commit-history.tsx (5)
1-3: LGTM!The imports are appropriate for the hover preview functionality.
createPortalenables rendering the hover card outside the normal DOM hierarchy, andMouseEventis correctly typed for hover handlers.
381-461: LGTM!The scroll handler properly clears the hover state when scrolling begins, preventing the preview card from becoming misaligned. The complex setup/teardown logic ensures the listener is only attached when needed and is properly cleaned up.
463-469: LGTM!This defensive check prevents displaying a preview for a commit that has been removed from the list, which could happen during branch switches or history reloads.
492-530: LGTM!The rendering logic properly wires up the hover interactions and conditionally displays the preview. The
isActiveprop provides visual feedback for which commit is currently hovered.
209-209: Replacescrollbar-thinwithcustom-scrollbar-thinon line 209.The class
scrollbar-thindoes not exist. The custom scrollbar styling is defined as.custom-scrollbar-thininsrc/styles.cssand should be used consistently across the codebase, as done in other components (e.g.,src/components/ui/command.tsx,src/features/global-search/components/file-preview.tsx).Change:
className={`${filesScrollable ? "custom-scrollbar-thin pr-1" : ""} space-y-1`}Likely an incorrect or invalid review comment.
| const handleCommitClick = useCallback(() => { | ||
| onViewCommitDiff(commit.hash); | ||
| }, [commit.hash, onViewCommitDiff]); |
There was a problem hiding this comment.
Add undefined check for optional callback.
The onViewCommitDiff prop is optional in GitCommitHistoryProps but is called unconditionally here. If the parent doesn't provide this callback, clicking a commit will throw a runtime error.
Apply this diff to add a safety check:
const handleCommitClick = useCallback(() => {
- onViewCommitDiff(commit.hash);
+ onViewCommitDiff?.(commit.hash);
}, [commit.hash, onViewCommitDiff]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleCommitClick = useCallback(() => { | |
| onViewCommitDiff(commit.hash); | |
| }, [commit.hash, onViewCommitDiff]); | |
| const handleCommitClick = useCallback(() => { | |
| onViewCommitDiff?.(commit.hash); | |
| }, [commit.hash, onViewCommitDiff]); |
🤖 Prompt for AI Agents
In src/version-control/git/views/git-commit-history.tsx around lines 53 to 55,
the click handler calls the optional onViewCommitDiff prop unconditionally which
can throw if it's undefined; update the useCallback to check that
onViewCommitDiff is defined before calling (e.g., if (onViewCommitDiff)
onViewCommitDiff(commit.hash)), keeping commit.hash and onViewCommitDiff in the
dependency array so the hook stays correct.
| return ( | ||
| <div className="border-border border-b last:border-b-0"> | ||
| {/* Commit Header */} | ||
| <div className="cursor-pointer px-3 py-2 hover:bg-hover" onClick={handleCommitClick}> | ||
| <div className="flex items-start gap-2"> | ||
| <button | ||
| onClick={handleToggleClick} | ||
| className="mt-0.5 text-text-lighter transition-colors hover:text-text" | ||
| > | ||
| {isExpanded ? <ChevronDown size={10} /> : <ChevronRight size={10} />} | ||
| </button> | ||
| <div className="min-w-0 flex-1"> | ||
| <div className="mb-1 font-medium text-[10px] text-text leading-tight"> | ||
| {commit.message} | ||
| </div> | ||
|
|
||
| <div className="flex items-center gap-3 text-[9px] text-text-lighter"> | ||
| <span className="flex items-center gap-1"> | ||
| <User size={8} /> | ||
| {commit.author} | ||
| </span> | ||
|
|
||
| <span className="flex items-center gap-1"> | ||
| <Clock size={8} /> | ||
| {formattedDate} | ||
| </span> | ||
| </div> | ||
| <div | ||
| onMouseEnter={handleMouseEnter} | ||
| onMouseLeave={onHoverEnd} | ||
| onClick={handleCommitClick} | ||
| className={`cursor-pointer px-3 py-2 hover:bg-hover ${isActive ? "bg-hover" : ""}`} | ||
| > | ||
| <div className="flex items-start gap-2"> | ||
| <div className="min-w-0 flex-1"> | ||
| <div className="mb-1 font-medium text-[10px] text-text leading-tight"> | ||
| {commit.message} | ||
| </div> | ||
|
|
||
| <div className="flex items-center gap-3 text-[9px] text-text-lighter"> | ||
| <span className="flex items-center gap-1"> | ||
| <User size={8} /> | ||
| {commit.author} | ||
| </span> | ||
|
|
||
| <span className="flex items-center gap-1"> | ||
| <Clock size={8} /> | ||
| {formattedDate} | ||
| </span> | ||
| </div> | ||
| </div> | ||
| </div> | ||
|
|
||
| {/* Expanded Commit Details */} | ||
| {isExpanded && ( | ||
| <ExpandedCommitDetails | ||
| commit={commit} | ||
| files={files} | ||
| isLoading={isLoading} | ||
| isCopied={isCopied} | ||
| onViewCommitDiff={onViewCommitDiff} | ||
| onCopyHash={onCopyHash} | ||
| /> | ||
| )} | ||
| </div> | ||
| ); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Consider keyboard accessibility for hover interactions.
The hover-based preview is not accessible via keyboard navigation. Users who rely on keyboard-only navigation won't be able to preview commit details without clicking.
Consider adding keyboard event handlers (e.g., onFocus, onBlur) or ensuring the click action provides equivalent functionality.
🤖 Prompt for AI Agents
In src/version-control/git/views/git-commit-history.tsx around lines 64 to 93,
the hover preview is not keyboard accessible; add keyboard support by making the
commit element focusable and firing the same handlers on keyboard focus/blur and
Enter/Space key press. Specifically, add tabIndex={0},
onFocus={handleMouseEnter} and onBlur={onHoverEnd}, and an onKeyDown that calls
handleCommitClick when Enter or Space is pressed; also ensure appropriate ARIA
role (e.g., role="button") so screen readers treat it as an interactive element.
| const handleCommitHover = useCallback( | ||
| (commit: any, target: HTMLElement) => { | ||
| clearHoverTimeout(); | ||
|
|
||
| const container = scrollContainerRef.current; | ||
| if (!container) return; | ||
|
|
||
| const targetRect = target.getBoundingClientRect(); | ||
| const scrollbarWidth = Math.max(container.offsetWidth - container.clientWidth, 0); | ||
| setHoveredCommit({ | ||
| commit, | ||
| anchorRect: { | ||
| top: targetRect.top, | ||
| bottom: targetRect.bottom, | ||
| left: targetRect.left, | ||
| right: targetRect.right, | ||
| width: targetRect.width, | ||
| height: targetRect.height, | ||
| scrollbarWidth, | ||
| }, | ||
| }); | ||
|
|
||
| if (!commitFiles[commit.hash] && repoPath) { | ||
| setLoadingCommits((prev) => { | ||
| if (prev.has(commit.hash)) return prev; | ||
| const next = new Set(prev); | ||
| next.add(commit.hash); | ||
| return next; | ||
| }); | ||
|
|
||
| getCommitDiff(repoPath, commit.hash) | ||
| .then((diffs) => { | ||
| setCommitFiles((prev) => ({ | ||
| ...prev, | ||
| [commit.hash]: diffs || [], | ||
| })); | ||
| }) | ||
| .catch(() => {}) | ||
| .finally(() => { | ||
| setLoadingCommits((prev) => { | ||
| const next = new Set(prev); | ||
| next.delete(commit.hash); | ||
| return next; | ||
| }); | ||
| }); | ||
| } | ||
| }, | ||
| [commitFiles, repoPath], | ||
| [clearHoverTimeout, commitFiles, repoPath], | ||
| ); |
There was a problem hiding this comment.
Add request cancellation to prevent race conditions.
When hovering rapidly over multiple commits, all getCommitDiff requests complete and update state. A slower request could overwrite a faster one, displaying stale data for the wrong commit. Additionally, if the component unmounts or the user navigates away, in-flight requests will still update state.
Add an AbortController to cancel in-flight requests:
+const abortControllerRef = useRef<AbortController | null>(null);
+
const handleCommitHover = useCallback(
(commit: any, target: HTMLElement) => {
clearHoverTimeout();
+
+ // Cancel previous fetch
+ if (abortControllerRef.current) {
+ abortControllerRef.current.abort();
+ }
const container = scrollContainerRef.current;
if (!container) return;
const targetRect = target.getBoundingClientRect();
const scrollbarWidth = Math.max(container.offsetWidth - container.clientWidth, 0);
setHoveredCommit({
commit,
anchorRect: {
top: targetRect.top,
bottom: targetRect.bottom,
left: targetRect.left,
right: targetRect.right,
width: targetRect.width,
height: targetRect.height,
scrollbarWidth,
},
});
if (!commitFiles[commit.hash] && repoPath) {
setLoadingCommits((prev) => {
if (prev.has(commit.hash)) return prev;
const next = new Set(prev);
next.add(commit.hash);
return next;
});
+ abortControllerRef.current = new AbortController();
+ const currentController = abortControllerRef.current;
+
- getCommitDiff(repoPath, commit.hash)
+ getCommitDiff(repoPath, commit.hash, currentController.signal)
.then((diffs) => {
+ if (currentController.signal.aborted) return;
setCommitFiles((prev) => ({
...prev,
[commit.hash]: diffs || [],
}));
})
- .catch(() => {})
+ .catch((error) => {
+ if (error.name === 'AbortError') return;
+ console.error('Failed to load commit diff:', error);
+ })
.finally(() => {
+ if (currentController.signal.aborted) return;
setLoadingCommits((prev) => {
const next = new Set(prev);
next.delete(commit.hash);
return next;
});
});
}
},
[clearHoverTimeout, commitFiles, repoPath],
);Also update the cleanup effect to abort on unmount:
useEffect(() => {
return () => {
clearHoverTimeout();
+ if (abortControllerRef.current) {
+ abortControllerRef.current.abort();
+ }
};
}, [clearHoverTimeout]);Note: Verify that getCommitDiff accepts an AbortSignal parameter. If not, you'll need to update that function as well.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/version-control/git/views/git-commit-history.tsx around lines 298 to 346,
hovering rapidly can leave multiple in-flight getCommitDiff requests running and
a slow response may overwrite state for the wrong commit or update state after
unmount; fix by creating an AbortController per hover, pass its signal into
getCommitDiff (update getCommitDiff to accept an AbortSignal if needed), store
the controller (e.g. ref) and call controller.abort() before starting a new
request so the previous request is cancelled, handle abort errors in the promise
chain to avoid updating state, and ensure the component cleanup effect aborts
any outstanding controller on unmount to prevent state updates after unmount.
- Remove expandable commit rows and chevron toggle buttons - Add hover-based commit preview cards with smart positioning - Centralize hover state management with timeout delays - Implement viewport-aware positioning for hover cards - Simplify CommitItem component by removing expansion props - Use React portals for rendering hover previews outside scroll container
mehmetozguldev
force-pushed
the
git-commit-history-beautify
branch
from
3410362 to
a5dc565
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the git commit history from an expandable accordion interface to a hover-based preview system, improving UX by reducing clicks needed to view commit details.
Key changes:
- Removed expandable commit rows with chevron toggles, replaced with hover-triggered previews
- Implemented portal-based hover cards with viewport-aware positioning
- Simplified component props and state management by removing expansion tracking
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div | ||
| className="pointer-events-auto fixed z-50 w-60 rounded border border-border bg-secondary-bg p-3 text-[9px] shadow-lg" | ||
| style={{ top: verticalPosition, left: horizontalPosition, width: cardWidth }} | ||
| onMouseEnter={onKeepOpen} | ||
| onMouseLeave={onRequestClose} |
There was a problem hiding this comment.
The hover preview lacks keyboard accessibility. Users cannot open the preview using keyboard navigation (Tab, Enter), and there's no way to focus or interact with the file list using only the keyboard. Consider adding keyboard event handlers and ARIA attributes to make the preview accessible to keyboard and screen reader users.
| if (!container) return; | ||
|
|
||
| const targetRect = target.getBoundingClientRect(); | ||
| const scrollbarWidth = Math.max(container.offsetWidth - container.clientWidth, 0); |
There was a problem hiding this comment.
The scrollbar width calculation uses offsetWidth - clientWidth, which works for most cases but may not account for all browser rendering modes or zoom levels accurately. Consider caching this value or using a more robust method like getComputedStyle to ensure accuracy across different browsers and rendering contexts.
| const handleCommitClick = useCallback(() => { | ||
| onViewCommitDiff(commit.hash); | ||
| }, [commit.hash, onViewCommitDiff]); |
There was a problem hiding this comment.
The handleViewCommitDiff callback in CommitItem no longer passes the filePath parameter to onViewCommitDiff, but CommitHoverPreview still passes it when clicking on files. This inconsistency means clicking the commit row and clicking a file in the preview have different behaviors. Ensure this is intentional and consider documenting the difference or making the behavior consistent.
| if (hoveredCommit) { | ||
| clearHoverTimeout(); | ||
| setHoveredCommit(null); | ||
| } | ||
|
|
There was a problem hiding this comment.
The hover preview is cleared whenever the user scrolls, which could be disruptive if they are trying to read the hover preview content while scrolling. Consider only clearing the hover preview if the hovered commit scrolls out of view, or allow users to keep the preview open if their mouse is over it (which is already handled, but gets cleared here before they can move their mouse).
| if (hoveredCommit) { | |
| clearHoverTimeout(); | |
| setHoveredCommit(null); | |
| } |
| <div | ||
| className="pointer-events-auto fixed z-50 w-60 rounded border border-border bg-secondary-bg p-3 text-[9px] shadow-lg" | ||
| style={{ top: verticalPosition, left: horizontalPosition, width: cardWidth }} |
There was a problem hiding this comment.
The hover preview positioning doesn't account for the anchor element scrolling out of view. If the user hovers over a commit, then scrolls the list while keeping their mouse on the hover card (via onKeepOpen), the preview will remain at its original position even though the anchor commit has moved. This creates a visual disconnect. Consider either recalculating position on scroll or closing the preview when the anchor moves significantly.
| if (!commitFiles[commit.hash] && repoPath) { | ||
| setLoadingCommits((prev) => { | ||
| if (prev.has(commit.hash)) return prev; | ||
| const next = new Set(prev); | ||
| next.add(commit.hash); | ||
| return next; | ||
| }); | ||
|
|
||
| getCommitDiff(repoPath, commit.hash) | ||
| .then((diffs) => { | ||
| setCommitFiles((prev) => ({ | ||
| ...prev, | ||
| [commit.hash]: diffs || [], | ||
| })); | ||
| }) | ||
| .catch(() => {}) | ||
| .finally(() => { | ||
| setLoadingCommits((prev) => { | ||
| const next = new Set(prev); | ||
| next.delete(commit.hash); | ||
| return next; | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Multiple rapid hovers could trigger multiple concurrent getCommitDiff requests for the same commit. The check on line 323 only prevents a new request if loading is already in progress, but if the user hovers, leaves, and quickly hovers again before the first request completes, a second request could be initiated. Consider tracking pending requests and canceling or reusing them.
| return () => { | ||
| clearHoverTimeout(); | ||
| }; | ||
| }, [clearHoverTimeout]); |
There was a problem hiding this comment.
The hover timeout cleanup on unmount references clearHoverTimeout in the dependency array, which could cause the effect to re-run unnecessarily. Since clearHoverTimeout is a useCallback with no dependencies that change, this is stable, but the effect itself doesn't need dependencies - it's only meant to run on unmount. Consider using an empty dependency array or moving the cleanup logic directly into the return function.
| }, [clearHoverTimeout]); | |
| }, []); |
| useEffect(() => { | ||
| if (!hoveredCommit) return; | ||
| const stillExists = commits.some((commit) => commit.hash === hoveredCommit.commit.hash); | ||
| if (!stillExists) { | ||
| setHoveredCommit(null); | ||
| } | ||
| }, [commits, hoveredCommit]); |
There was a problem hiding this comment.
The commit existence check effect runs on every commits or hoveredCommit change, but it only needs to check when hoveredCommit changes while commits also change. This could cause unnecessary re-renders. Consider optimizing by only checking when both values change together, or using a more specific dependency.
| const verticalPosition = (() => { | ||
| if (!windowHeight) return top; | ||
| const minTop = viewportPadding; | ||
| const maxTop = Math.max(windowHeight - estimatedHeight - viewportPadding, viewportPadding); | ||
| return Math.min(Math.max(top, minTop), maxTop); | ||
| })(); | ||
|
|
||
| const desiredLeft = right + scrollbarWidth + 4; | ||
| const horizontalPosition = (() => { | ||
| if (!windowWidth) return desiredLeft; | ||
| const maxLeft = windowWidth - cardWidth - viewportPadding; | ||
| const clamped = Math.min(desiredLeft, maxLeft); | ||
| return Math.max(clamped, viewportPadding); | ||
| })(); |
There was a problem hiding this comment.
The fixed positioning of the hover preview uses viewport coordinates calculated at hover time. If the window is resized while the preview is open, the positioning calculations (windowHeight, windowWidth) remain stale and the preview might overflow the new viewport boundaries. Consider recalculating position on window resize or closing the preview.
| const scheduleHoverClear = useCallback(() => { | ||
| clearHoverTimeout(); | ||
| hoverTimeoutRef.current = setTimeout(() => { | ||
| setHoveredCommit(null); | ||
| }, HOVER_CLOSE_DELAY_MS); |
There was a problem hiding this comment.
The hover delay (HOVER_CLOSE_DELAY_MS = 250ms) is applied when leaving both the commit item and the hover card. This means if a user moves their mouse from the commit to the hover card, there's a 250ms window where the card might close if they don't reach it in time. This creates a poor user experience, especially for users with motor control challenges. Consider removing the delay when leaving the commit item if the mouse is moving toward the hover card, or increase the delay.
2 participants
Pull Request
Refactored the git commit history component from an expandable interface to a hover-based preview system, improving the user experience and reducing visual clutter.
Description
Summary by CodeRabbit