feat(desktop): GitHub PR-style infinite scroll diff viewer for changes#868
Merged
feat(desktop): GitHub PR-style infinite scroll diff viewer for changes#868
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a scroll-sync context and a workspace-aware, infinite-scroll Changes UI: new ScrollProvider/ScrollContext, InfiniteScrollView with categorized sections and per-file diff components, DiffViewer scroll/fit options, toolbar, sidebar plumbing for scroll-vs-pane open, and branch/status fetching to drive rendering. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Sidebar as Sidebar / FileItem
participant ScrollCtx as ScrollProvider
participant Infinite as InfiniteScrollView
participant Trpc as electronTrpc
participant Diff as DiffViewer
User->>Sidebar: click file
Sidebar->>ScrollCtx: setActiveFileKey(key)
alt expanded view (scroll-mode)
Sidebar->>ScrollCtx: scrollToFile(key)
ScrollCtx->>Infinite: scroll container to file element
else normal mode (pane)
Sidebar->>Infinite: addFileViewerPane(file)
end
User->>Infinite: expand file row
Infinite->>Trpc: getFileContents(worktreePath, path, base)
Trpc-->>Infinite: file content
Infinite->>Diff: render diff (captureScroll/fitContent)
Diff-->>User: display diff with focus/blur scroll handling
sequenceDiagram
actor User
participant FileSection as FileDiffSection
participant Trpc as electronTrpc
participant ScrollCtx as ScrollProvider
User->>FileSection: stage / unstage / discard
FileSection->>Trpc: run mutation (stage/unstage/discard/delete)
Trpc-->>FileSection: success / error
FileSection->>Trpc: refetch status (onSuccess)
Trpc-->>FileSection: updated status
FileSection->>ScrollCtx: setFileViewed(key, true)
FileSection-->>User: update UI / show toast on error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx`:
- Around line 109-117: The handleCopyPath callback sets a 2s timeout that calls
setIsCopied(false) which can run after the component unmounts; to fix, store the
timeout id in a ref (e.g., copyTimeoutRef), clear any existing timeout before
creating a new one inside handleCopyPath, and add a useEffect cleanup that
clears copyTimeoutRef.current on unmount so setIsCopied is never called on an
unmounted component; reference handleCopyPath, setIsCopied, and the new
copyTimeoutRef in your changes.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/InfiniteScrollView.tsx`:
- Around line 108-109: The key format is inconsistent; update the committed-file
key construction in InfiniteScrollView so it uses the same double-colon pattern
as the other categories (e.g., construct fileKey as
"committed::${commit.hash}::${file.path}" instead of
"committed:${commit.hash}:${file.path}"), and apply the same normalization to
the other occurrences referenced (the other fileKey constructions around the
other map loops at the noted locations) so all categories use the identical
double-colon delimiter scheme; edit the fileKey assignments inside the files.map
callbacks and the other similar key builders in this component
(InfiniteScrollView) accordingly.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/context/ScrollContext.tsx`:
- Around line 100-119: The useMemo value includes setActiveFileKey but the
dependency array omits it; update the dependencies for the memo that constructs
value (which references registerFileRef, scrollToFile, containerRef,
viewedFiles, setFileViewed, viewedCount, activeFileKey, setActiveFileKey) to
also include setActiveFileKey so the dependency list matches the returned object
and remains consistent/clear for future maintainers.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/context/ScrollContext.tsx (2)
21-41: Consider using object parameters for context methods.Per coding guidelines, functions with 2+ parameters should use object parameters for self-documentation. The current interface uses positional arguments for
registerFileRef(4 params) andscrollToFile(3 params).Example refactor
interface ScrollContextValue { - registerFileRef: ( - file: ChangedFile, - category: ChangeCategory, - commitHash: string | undefined, - ref: HTMLDivElement | null, - ) => void; - scrollToFile: ( - file: ChangedFile, - category: ChangeCategory, - commitHash?: string, - ) => void; + registerFileRef: (params: { + file: ChangedFile; + category: ChangeCategory; + commitHash: string | undefined; + ref: HTMLDivElement | null; + }) => void; + scrollToFile: (params: { + file: ChangedFile; + category: ChangeCategory; + commitHash?: string; + }) => void; // ... rest unchanged }
68-84: Extract magic number to a named constant.The hardcoded
16on line 75 should be extracted to a named constant at module top for clarity per coding guidelines.Proposed fix
+const SCROLL_OFFSET_PX = 16; + // ... at line 75: - const scrollTop = element.offsetTop - container.offsetTop - 16; + const scrollTop = element.offsetTop - container.offsetTop - SCROLL_OFFSET_PX;
Comment on lines
+108
to
+109
| {files.map((file) => { | ||
| const fileKey = `committed:${commit.hash}:${file.path}`; |
Contributor
There was a problem hiding this comment.
Inconsistent file key format across categories.
The file key format is inconsistent: committed:hash:path uses single colons, while against-base::path, staged::path, and unstaged::path use double colons. This could cause issues if keys are parsed or compared elsewhere.
Suggested fix for consistent key format
Either use single colons everywhere:
-const fileKey = `against-base::${file.path}`;
+const fileKey = `against-base:${file.path}`;Or use double colons for committed files too (with empty middle segment):
-const fileKey = `committed:${commit.hash}:${file.path}`;
+const fileKey = `committed:${commit.hash}::${file.path}`;Also applies to: 399-400, 451-452, 487-488
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/InfiniteScrollView.tsx`
around lines 108 - 109, The key format is inconsistent; update the
committed-file key construction in InfiniteScrollView so it uses the same
double-colon pattern as the other categories (e.g., construct fileKey as
"committed::${commit.hash}::${file.path}" instead of
"committed:${commit.hash}:${file.path}"), and apply the same normalization to
the other occurrences referenced (the other fileKey constructions around the
other map loops at the noted locations) so all categories use the identical
double-colon delimiter scheme; edit the fileKey assignments inside the files.map
callbacks and the other similar key builders in this component
(InfiniteScrollView) accordingly.
Comment on lines
+100
to
+119
| const value = useMemo( | ||
| () => ({ | ||
| registerFileRef, | ||
| scrollToFile, | ||
| containerRef, | ||
| viewedFiles, | ||
| setFileViewed, | ||
| viewedCount, | ||
| activeFileKey, | ||
| setActiveFileKey, | ||
| }), | ||
| [ | ||
| registerFileRef, | ||
| scrollToFile, | ||
| viewedFiles, | ||
| setFileViewed, | ||
| viewedCount, | ||
| activeFileKey, | ||
| ], | ||
| ); |
Contributor
There was a problem hiding this comment.
Missing setActiveFileKey in useMemo dependency array.
setActiveFileKey is included in the memoized value but not in the dependency array. While useState setters are stable and this won't cause runtime issues, it's inconsistent with the other included items and could confuse future maintainers.
Proposed fix
const value = useMemo(
() => ({
registerFileRef,
scrollToFile,
containerRef,
viewedFiles,
setFileViewed,
viewedCount,
activeFileKey,
setActiveFileKey,
}),
[
registerFileRef,
scrollToFile,
viewedFiles,
setFileViewed,
viewedCount,
activeFileKey,
+ setActiveFileKey,
],
);🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/context/ScrollContext.tsx`
around lines 100 - 119, The useMemo value includes setActiveFileKey but the
dependency array omits it; update the dependencies for the memo that constructs
value (which references registerFileRef, scrollToFile, containerRef,
viewedFiles, setFileViewed, viewedCount, activeFileKey, setActiveFileKey) to
also include setActiveFileKey so the dependency list matches the returned object
and remains consistent/clear for future maintainers.
Kitenite
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx`:
- Around line 113-116: The clipboard write call in FileDiffSection (inside the
click handler using e.stopPropagation and navigator.clipboard.writeText) lacks
error handling and always sets setIsCopied(true); change it to await or
then/catch the Promise from navigator.clipboard.writeText(file.path) and only
call setIsCopied(true) on success, revert with setIsCopied(false) or keep
existing state on failure, and surface/log the error (e.g., with console.error
or an existing logger) so failures (permission denied, etc.) do not show a false
"Copied!" message.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx (1)
111-119: Memory leak with uncleared timeout.The
setTimeoutinhandleCopyPathcan cause a state update on an unmounted component. Store the timeout ID in a ref and clear it on unmount.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx (1)
141-145: Consider extracting magic values to named constants.The rootMargin and threshold values are hardcoded. Extracting them improves readability and makes tuning easier.
const INTERSECTION_ROOT_MARGIN = "-100px 0px -60% 0px"; const INTERSECTION_THRESHOLD = 0.1;
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/components/FileDiffHeader/FileDiffHeader.tsx`:
- Around line 78-88: The filename trigger in FileDiffHeader.tsx is a
non-focusable <span> with only onClick (the element with class "group/filename"
and the onOpenInEditor handler), making it inaccessible to keyboard users;
replace that <span> with a semantic, focusable control (preferably a <button
type="button"> or an <a> if it navigates) keeping the same classes and
onOpenInEditor prop, remove the two biome-ignore a11y comments, and add any
necessary ARIA text (e.g., aria-label={`${file.path} — open in editor`}) so the
trigger remains visually identical but keyboard- and screen-reader-accessible.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx`:
- Around line 20-45: The file defines a small helper component
DiffViewerFitContent; inline its JSX directly where it's used (replace usages of
<DiffViewerFitContent ... /> with the equivalent <div className="bg-background">
containing the <DiffViewer ... fitContent /> props) and remove the
DiffViewerFitContent function declaration; also find and inline the other
duplicate small component instance referenced around the other usage (the noted
210-215 region) so the file contains a single top-level component and no
standalone DiffViewerFitContent helper.
- Around line 84-90: The renderer concatenates paths manually in
handleOpenInEditor which can break on Windows; instead of building absolutePath
using `${worktreePath}/${file.path}`, call openInEditorMutation.mutate with cwd:
worktreePath and path: file.path (pass the file path separately) so the backend
can use resolvePath/nodePath.resolve to join safely (follow the same pattern
used in Terminal/helpers.ts and update the handler schema if needed to accept a
relative path when cwd is provided).
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/components/DiffToolbar/DiffToolbar.tsx`:
- Around line 75-117: The icon-only toolbar buttons in DiffToolbar.tsx lack
accessible names and state; update the button that toggles view mode (where
onClick calls onDiffViewModeChange and uses diffViewMode) to include a
descriptive aria-label (e.g., "Switch to inline diff" or "Switch to side-by-side
diff" based on diffViewMode), and update the hide-unchanged toggle button
(onClick calls onToggleHideUnchangedRegions, uses hideUnchangedRegions) to
include both an aria-label (e.g., "Hide unchanged regions" / "Show all lines")
and aria-pressed={hideUnchangedRegions} to expose its toggle state to assistive
tech.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/components/FileItem/FileItem.tsx`:
- Around line 97-98: createFileKey has been refactored to accept a single params
object instead of three positional args; update all call sites (including the
one computing isScrollSyncActive in FileItem.tsx where activeFileKey is
compared) to call createFileKey({ file, category, commitHash }) and adjust any
type/imports as needed; ensure the same change is applied in the other three use
sites (ScrollContext, FileDiffSection, and this FileItem) so signatures match
and comparisons with activeFileKey continue to work.
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx`:
- Around line 47-82: Change the three callbacks handleFileOpenPane,
handleFileOpenPinnedPane, and handleFileScrollTo to accept a single params
object (e.g., { file, category, commitHash }) instead of positional args; inside
each, read params.file, params.category, params.commitHash, then perform the
same logic (addFileViewerPane and invalidateFileContent or scrollToFile) and
update their useCallback dependency arrays if needed. Then update all call sites
(including ChangesView and its children) to pass a single object with keys file,
category, commitHash when invoking these handlers. Ensure the payload passed to
addFileViewerPane still constructs filePath, diffCategory, oldPath, isPinned,
and commitHash from the params object.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/components/FileItem/FileItem.tsx (1)
31-32: Import directly from the concrete ScrollContext file to avoid barrel coupling.The imports of
createFileKeyanduseScrollContextare re-exported through a barrel file. Per coding guidelines, import from the concrete file directly:♻️ Suggested update
-import { createFileKey, useScrollContext } from "../../../../ChangesContent"; +import { createFileKey, useScrollContext } from "../../../../ChangesContent/context/ScrollContext";apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/components/CommitSection/CommitSection.tsx (1)
54-65: Prefer sharedcreateFileKeyto avoid key-format drift.
FileDiffSectionalready derives keys viacreateFileKey(file, category, commitHash). Hardcoding the string here risks mismatches if the helper changes, which could desync viewed/collapsed state. Consider reusing the same helper.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/InfiniteScrollView.tsx (1)
101-144: Align log prefixes with the [domain/operation] pattern.Current logs use
[InfiniteScrollView], which doesn’t match the required[domain/operation]format. Consider using[changes/stage],[changes/unstage], etc.🛠️ Proposed log prefix updates
- console.error( - `[InfiniteScrollView] Failed to stage file ${variables.filePath}:`, - error, - ); + console.error( + `[changes/stage] Failed to stage file ${variables.filePath}:`, + error, + ); @@ - console.error( - `[InfiniteScrollView] Failed to unstage file ${variables.filePath}:`, - error, - ); + console.error( + `[changes/unstage] Failed to unstage file ${variables.filePath}:`, + error, + ); @@ - console.error( - `[InfiniteScrollView] Failed to discard changes for ${variables.filePath}:`, - error, - ); + console.error( + `[changes/discard] Failed to discard changes for ${variables.filePath}:`, + error, + ); @@ - console.error( - `[InfiniteScrollView] Failed to delete ${variables.filePath}:`, - error, - ); + console.error( + `[changes/delete-untracked] Failed to delete ${variables.filePath}:`, + error, + );As per coding guidelines, use the [domain/operation] log prefix pattern.
Comment on lines
+84
to
+90
| const handleOpenInEditor = useCallback( | ||
| (e: React.MouseEvent) => { | ||
| e.stopPropagation(); | ||
| if (worktreePath) { | ||
| const absolutePath = `${worktreePath}/${file.path}`; | ||
| openInEditorMutation.mutate({ path: absolutePath, cwd: worktreePath }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -C5 "openFileInEditor|openInEditorMutation"Repository: superset-sh/superset
Length of output: 12834
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/external/index.ts | sed -n '84,120p'Repository: superset-sh/superset
Length of output: 733
🏁 Script executed:
rg -n "resolvePath" apps/desktop/src/lib/trpc/routers/external/index.ts -B2 -A2Repository: superset-sh/superset
Length of output: 392
🏁 Script executed:
rg -n "function resolvePath|export.*resolvePath|const resolvePath" apps/desktop/src/lib/trpcRepository: superset-sh/superset
Length of output: 304
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/external/helpers.ts | sed -n '141,160p'Repository: superset-sh/superset
Length of output: 751
🏁 Script executed:
cat -n apps/desktop/src/lib/trpc/routers/external/helpers.ts | sed -n '141,175p'Repository: superset-sh/superset
Length of output: 1260
🏁 Script executed:
# Check if there's already a different pattern used elsewhere or if all usages follow same pattern
rg -n "openFileInEditor.*mutate" --type ts --type tsx -A2Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
rg -n "openFileInEditor.*mutate" -A2 -t tsRepository: superset-sh/superset
Length of output: 493
🏁 Script executed:
rg -n "openFileInEditor\.mutate\|openInEditorMutation\.mutate" -B1 -A3Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
rg -n "\.mutate\({" apps/desktop/src/renderer --type ts -A2 | grep -A2 "openFileInEditor\|openInEditor"Repository: superset-sh/superset
Length of output: 149
🏁 Script executed:
rg -n "mutate.*path.*absolutePath\|mutate.*path.*worktreePath" apps/desktop/src/renderer -A1 -B1Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
rg "openFileInEditor" apps/desktop/src/renderer -A3 -B1 | grep -A3 "mutate"Repository: superset-sh/superset
Length of output: 1559
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/TabsContent/Terminal/helpers.ts | sed -n '255,270p'Repository: superset-sh/superset
Length of output: 564
Use platform-safe path joining in the backend.
The renderer currently concatenates paths using a forward slash (${worktreePath}/${file.path}), which is not platform-safe on Windows. The backend already has resolvePath() with platform-safe nodePath.resolve(), but this is bypassed when an absolute path is passed. Pass cwd and filePath separately instead, allowing the backend to handle safe joining—this pattern is already used in Terminal/helpers.ts (line 266).
🛠️ Safer call pattern (requires handler update)
- if (worktreePath) {
- const absolutePath = `${worktreePath}/${file.path}`;
- openInEditorMutation.mutate({ path: absolutePath, cwd: worktreePath });
- }
+ if (worktreePath) {
+ openInEditorMutation.mutate({
+ path: file.path,
+ cwd: worktreePath,
+ });
+ }Then update the handler schema to accept path as relative when cwd is provided, or keep current schema and pass file.path for backend joining.
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx`
around lines 84 - 90, The renderer concatenates paths manually in
handleOpenInEditor which can break on Windows; instead of building absolutePath
using `${worktreePath}/${file.path}`, call openInEditorMutation.mutate with cwd:
worktreePath and path: file.path (pass the file path separately) so the backend
can use resolvePath/nodePath.resolve to join safely (follow the same pattern
used in Terminal/helpers.ts and update the handler schema if needed to accept a
relative path when cwd is provided).
Comment on lines
+97
to
+98
| const isScrollSyncActive = | ||
| category && activeFileKey === createFileKey(file, category, commitHash); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n "createFileKey" apps/desktop/src/renderer -g '*.ts*'Repository: superset-sh/superset
Length of output: 1719
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/context/ScrollContext.tsx | head -50Repository: superset-sh/superset
Length of output: 1773
🏁 Script executed:
sed -n '55,75p' apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/context/ScrollContext.tsxRepository: superset-sh/superset
Length of output: 669
🏁 Script executed:
sed -n '75,82p' apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsxRepository: superset-sh/superset
Length of output: 382
Refactor createFileKey to accept a params object.
The function currently accepts 3 positional arguments. Per repo guidelines for **/*.{ts,tsx} files, update it to accept a single params object and update all 4 call sites across FileItem.tsx, ScrollContext.tsx, and FileDiffSection.tsx.
Suggested call-site update
-const isScrollSyncActive =
- category && activeFileKey === createFileKey(file, category, commitHash);
+const isScrollSyncActive =
+ category && activeFileKey === createFileKey({ file, category, commitHash });🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/components/FileItem/FileItem.tsx`
around lines 97 - 98, createFileKey has been refactored to accept a single
params object instead of three positional args; update all call sites (including
the one computing isScrollSyncActive in FileItem.tsx where activeFileKey is
compared) to call createFileKey({ file, category, commitHash }) and adjust any
type/imports as needed; ensure the same change is applied in the other three use
sites (ScrollContext, FileDiffSection, and this FileItem) so signatures match
and comparisons with activeFileKey continue to work.
Comment on lines
+47
to
+82
| const handleFileOpenPane = useCallback( | ||
| (file: ChangedFile, category: ChangeCategory, commitHash?: string) => { | ||
| if (!workspaceId || !worktreePath) return; | ||
| addFileViewerPane(workspaceId, { | ||
| filePath: file.path, | ||
| diffCategory: category, | ||
| commitHash, | ||
| oldPath: file.oldPath, | ||
| isPinned: false, | ||
| }); | ||
| invalidateFileContent(file.path); | ||
| }, | ||
| [workspaceId, worktreePath, addFileViewerPane, invalidateFileContent], | ||
| ); | ||
|
|
||
| const handleFileOpenPinnedPane = useCallback( | ||
| (file: ChangedFile, category: ChangeCategory, commitHash?: string) => { | ||
| if (!workspaceId || !worktreePath) return; | ||
| addFileViewerPane(workspaceId, { | ||
| filePath: file.path, | ||
| diffCategory: category, | ||
| commitHash, | ||
| oldPath: file.oldPath, | ||
| isPinned: true, | ||
| }); | ||
| invalidateFileContent(file.path); | ||
| }, | ||
| [workspaceId, worktreePath, addFileViewerPane, invalidateFileContent], | ||
| ); | ||
|
|
||
| const handleFileScrollTo = useCallback( | ||
| (file: ChangedFile, category: ChangeCategory, commitHash?: string) => { | ||
| scrollToFile(file, category, commitHash); | ||
| }, | ||
| [scrollToFile], | ||
| ); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use a params object for multi-argument handlers.
These handlers take 2–3 positional args; the guideline requires a single params object. This also makes the callback prop signatures clearer for ChangesView and its children. As per coding guidelines, please switch to a params object and update the call sites accordingly.
♻️ Suggested refactor (update call sites accordingly)
+type FileOpenParams = {
+ file: ChangedFile;
+ category: ChangeCategory;
+ commitHash?: string;
+};
const handleFileOpenPane = useCallback(
- (file: ChangedFile, category: ChangeCategory, commitHash?: string) => {
+ ({ file, category, commitHash }: FileOpenParams) => {
if (!workspaceId || !worktreePath) return;
addFileViewerPane(workspaceId, {
filePath: file.path,
diffCategory: category,
commitHash,
oldPath: file.oldPath,
isPinned: false,
});
invalidateFileContent(file.path);
},
[workspaceId, worktreePath, addFileViewerPane, invalidateFileContent],
);
const handleFileOpenPinnedPane = useCallback(
- (file: ChangedFile, category: ChangeCategory, commitHash?: string) => {
+ ({ file, category, commitHash }: FileOpenParams) => {
if (!workspaceId || !worktreePath) return;
addFileViewerPane(workspaceId, {
filePath: file.path,
diffCategory: category,
commitHash,
oldPath: file.oldPath,
isPinned: true,
});
invalidateFileContent(file.path);
},
[workspaceId, worktreePath, addFileViewerPane, invalidateFileContent],
);
const handleFileScrollTo = useCallback(
- (file: ChangedFile, category: ChangeCategory, commitHash?: string) => {
- scrollToFile(file, category, commitHash);
- },
+ ({ file, category, commitHash }: FileOpenParams) =>
+ scrollToFile(file, category, commitHash),
[scrollToFile],
);📝 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.
Suggested change
| const handleFileOpenPane = useCallback( | |
| (file: ChangedFile, category: ChangeCategory, commitHash?: string) => { | |
| if (!workspaceId || !worktreePath) return; | |
| addFileViewerPane(workspaceId, { | |
| filePath: file.path, | |
| diffCategory: category, | |
| commitHash, | |
| oldPath: file.oldPath, | |
| isPinned: false, | |
| }); | |
| invalidateFileContent(file.path); | |
| }, | |
| [workspaceId, worktreePath, addFileViewerPane, invalidateFileContent], | |
| ); | |
| const handleFileOpenPinnedPane = useCallback( | |
| (file: ChangedFile, category: ChangeCategory, commitHash?: string) => { | |
| if (!workspaceId || !worktreePath) return; | |
| addFileViewerPane(workspaceId, { | |
| filePath: file.path, | |
| diffCategory: category, | |
| commitHash, | |
| oldPath: file.oldPath, | |
| isPinned: true, | |
| }); | |
| invalidateFileContent(file.path); | |
| }, | |
| [workspaceId, worktreePath, addFileViewerPane, invalidateFileContent], | |
| ); | |
| const handleFileScrollTo = useCallback( | |
| (file: ChangedFile, category: ChangeCategory, commitHash?: string) => { | |
| scrollToFile(file, category, commitHash); | |
| }, | |
| [scrollToFile], | |
| ); | |
| type FileOpenParams = { | |
| file: ChangedFile; | |
| category: ChangeCategory; | |
| commitHash?: string; | |
| }; | |
| const handleFileOpenPane = useCallback( | |
| ({ file, category, commitHash }: FileOpenParams) => { | |
| if (!workspaceId || !worktreePath) return; | |
| addFileViewerPane(workspaceId, { | |
| filePath: file.path, | |
| diffCategory: category, | |
| commitHash, | |
| oldPath: file.oldPath, | |
| isPinned: false, | |
| }); | |
| invalidateFileContent(file.path); | |
| }, | |
| [workspaceId, worktreePath, addFileViewerPane, invalidateFileContent], | |
| ); | |
| const handleFileOpenPinnedPane = useCallback( | |
| ({ file, category, commitHash }: FileOpenParams) => { | |
| if (!workspaceId || !worktreePath) return; | |
| addFileViewerPane(workspaceId, { | |
| filePath: file.path, | |
| diffCategory: category, | |
| commitHash, | |
| oldPath: file.oldPath, | |
| isPinned: true, | |
| }); | |
| invalidateFileContent(file.path); | |
| }, | |
| [workspaceId, worktreePath, addFileViewerPane, invalidateFileContent], | |
| ); | |
| const handleFileScrollTo = useCallback( | |
| ({ file, category, commitHash }: FileOpenParams) => | |
| scrollToFile(file, category, commitHash), | |
| [scrollToFile], | |
| ); |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/index.tsx`
around lines 47 - 82, Change the three callbacks handleFileOpenPane,
handleFileOpenPinnedPane, and handleFileScrollTo to accept a single params
object (e.g., { file, category, commitHash }) instead of positional args; inside
each, read params.file, params.category, params.commitHash, then perform the
same logic (addFileViewerPane and invalidateFileContent or scrollToFile) and
update their useCallback dependency arrays if needed. Then update all call sites
(including ChangesView and its children) to pass a single object with keys file,
category, commitHash when invoking these handlers. Ensure the payload passed to
addFileViewerPane still constructs filePath, diffCategory, oldPath, isPinned,
and commitHash from the params object.
- Pass baseBranch prop to FileDiffSection instead of re-fetching branches, ensuring diff content matches the user-selected base branch - Include baseBranch in query invalidation to properly bust cache after mutations - Use scroll container as IntersectionObserver root for accurate active file tracking
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/components/DiffToolbar/DiffToolbar.tsx`:
- Around line 99-103: The tooltip text in DiffToolbar.tsx is inconsistent: when
diffViewMode !== "side-by-side" the TooltipContent displays "Switch to side by
side diff" but the UI uses the hyphenated form elsewhere; update the string in
the TooltipContent (inside the DiffToolbar component where diffViewMode is
checked) to "Switch to side-by-side diff" so both branches use the hyphenated
"side-by-side" phrasing consistent with the rest of the UI.
♻️ Duplicate comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx (1)
60-66: Avoid manual path concatenation for editor open (Windows-safe joining).String-joining
${worktreePath}/${file.path}can break on Windows. Prefer passing the relative path + cwd and let the backend resolve.🛠️ Suggested fix
- if (worktreePath) { - const absolutePath = `${worktreePath}/${file.path}`; - openInEditorMutation.mutate({ path: absolutePath, cwd: worktreePath }); - } + if (worktreePath) { + openInEditorMutation.mutate({ path: file.path, cwd: worktreePath }); + }
🧹 Nitpick comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/FileDiffSection/FileDiffSection.tsx (1)
43-44: Extract magic numbers into module-level constants.The timeout and observer thresholds are hardcoded; pulling them into named constants improves readability and maintainability. As per coding guidelines, extract magic numbers and hardcoded values to named constants at module top.
♻️ Suggested refactor
+const COPY_RESET_MS = 2000; +const ACTIVE_FILE_THRESHOLD = 0.1; +const ACTIVE_FILE_ROOT_MARGIN = "-100px 0px -60% 0px"; + export function FileDiffSection({ file, category, commitHash, @@ - copyTimeoutRef.current = setTimeout(() => setIsCopied(false), 2000); + copyTimeoutRef.current = setTimeout(() => setIsCopied(false), COPY_RESET_MS); @@ - if (entry.isIntersecting && entry.intersectionRatio > 0.1) { + if (entry.isIntersecting && entry.intersectionRatio > ACTIVE_FILE_THRESHOLD) { setActiveFileKey(fileKey); } } }, { root: container, - rootMargin: "-100px 0px -60% 0px", - threshold: [0.1], + rootMargin: ACTIVE_FILE_ROOT_MARGIN, + threshold: [ACTIVE_FILE_THRESHOLD], }, );Also applies to: 71-135
Comment on lines
+99
to
+103
| <TooltipContent side="bottom" showArrow={false}> | ||
| {diffViewMode === "side-by-side" | ||
| ? "Switch to inline diff" | ||
| : "Switch to side by side diff"} | ||
| </TooltipContent> |
Contributor
There was a problem hiding this comment.
Align tooltip copy with the rest of the UI text.
Tooltip text says “side by side” while the rest uses “side-by-side”; this is a minor UI copy inconsistency.
🔧 Suggested tweak
- : "Switch to side by side diff"}
+ : "Switch to side-by-side diff"}📝 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.
Suggested change
| <TooltipContent side="bottom" showArrow={false}> | |
| {diffViewMode === "side-by-side" | |
| ? "Switch to inline diff" | |
| : "Switch to side by side diff"} | |
| </TooltipContent> | |
| <TooltipContent side="bottom" showArrow={false}> | |
| {diffViewMode === "side-by-side" | |
| ? "Switch to inline diff" | |
| : "Switch to side-by-side diff"} | |
| </TooltipContent> |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/screens/main/components/WorkspaceView/ChangesContent/components/InfiniteScrollView/components/DiffToolbar/DiffToolbar.tsx`
around lines 99 - 103, The tooltip text in DiffToolbar.tsx is inconsistent: when
diffViewMode !== "side-by-side" the TooltipContent displays "Switch to side by
side diff" but the UI uses the hyphenated form elsewhere; update the string in
the TooltipContent (inside the DiffToolbar component where diffViewMode is
checked) to "Switch to side-by-side diff" so both branches use the hyphenated
"side-by-side" phrasing consistent with the rest of the UI.
7 tasks
7 tasks
This was referenced Mar 19, 2026
This was referenced Mar 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.