fix(desktop): correct renamed file diffs and reset expanded commits on workspace switch#327
fix(desktop): correct renamed file diffs and reset expanded commits on workspace switch#327
Conversation
…n workspace switch - Fix renamed files showing blank/incorrect originals in diff viewer by passing oldPath to getFileContents and using it for the "original" side lookup - Reset expandedCommits state when workspace changes to prevent querying old commit hashes against the new worktree 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughThis PR enhances the file content retrieval workflow by adding an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsx (1)
2-2:useEffectimport is only needed because of the reset effect—consider removing the effect entirely (see below).apps/desktop/src/lib/trpc/routers/changes/changes.ts (2)
226-236:oldPathaddition to the tRPC input is the right surface-area change for rename diffs.
One minor hardening: use nullish coalescing so an accidental empty string doesn’t overridefilePath.- oldPath: z.string().optional(), + oldPath: z.string().optional(),(Then use
??below.)
239-316:originalPathwiring fixes renamed-file “original side” retrieval across categories.
Suggestion: switch to??to avoid treating""as intentional, and keep behavior consistent.- const originalPath = input.oldPath || input.filePath; + const originalPath = input.oldPath ?? input.filePath;Also worth double-checking (manual test) the “initial commit” case where
${commitHash}^doesn’t exist; your try/catch makes it safe, but it’s easy to regress later.apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/ChangesContent.tsx (1)
36-48: PassingoldPaththrough togetFileContentsis the correct plumbing for rename diffs.
Optional: if you want the payload cleaner, you can omit the key when undefined (not required).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src/lib/trpc/routers/changes/changes.ts(5 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/ChangesContent.tsx(2 hunks)apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsx(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/desktop/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
For Electron interprocess communication, ALWAYS use tRPC as defined in
src/lib/trpc
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/ChangesContent.tsx
apps/desktop/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/desktop/AGENTS.md)
apps/desktop/**/*.{ts,tsx}: Please use alias as defined intsconfig.jsonwhen possible
Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/ChangesContent.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain type safety and avoid using
anyunless absolutely necessary in TypeScript code
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/ChangesContent.tsx
**/components/**/[A-Z]*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
Use one folder per component with structure ComponentName/ComponentName.tsx + index.ts barrel export
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/ChangesContent.tsx
apps/desktop/src/renderer/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules in renderer process or shared code - only in src/main/
Files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/ChangesContent.tsx
apps/desktop/src/lib/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Never import Node.js modules like node:fs, node:path, node:os in src/lib/electron-router-dom.ts or similar shared code
Files:
apps/desktop/src/lib/trpc/routers/changes/changes.ts
🧠 Learnings (2)
📚 Learning: 2025-12-08T23:32:32.232Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-08T23:32:32.232Z
Learning: Applies to apps/desktop/src/main/lib/*{workspace,worktree}-*.ts : Use git worktree-based workspace management in the desktop app through workspace-manager.ts and worktree-manager.ts
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxapps/desktop/src/lib/trpc/routers/changes/changes.tsapps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/ChangesContent.tsx
📚 Learning: 2025-11-24T21:33:13.267Z
Learnt from: CR
Repo: superset-sh/superset PR: 0
File: apps/desktop/AGENTS.md:0-0
Timestamp: 2025-11-24T21:33:13.267Z
Learning: Applies to apps/desktop/**/*.{ts,tsx} : Prefer zustand for state management if it makes sense. Do not use effect unless absolutely necessary
Applied to files:
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsx
⏰ 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: Build
🔇 Additional comments (1)
apps/desktop/src/renderer/screens/main/components/WorkspaceView/ContentView/ChangesContent/ChangesContent.tsx (1)
111-125: Layout refactor looks safe (no behavioral coupling to diff rendering).
Just ensureoverflow-hidden+ innerflex-1doesn’t clip tooltips/menus fromDiffViewerif it renders portals/popovers inside the container.
| // Reset expanded commits when workspace changes to avoid querying | ||
| // old commit hashes against the new worktree | ||
| // biome-ignore lint/correctness/useExhaustiveDependencies: intentionally resets on worktreePath change | ||
| useEffect(() => { | ||
| setExpandedCommits(new Set()); | ||
| }, [worktreePath]); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxRepository: superset-sh/superset
Length of output: 8037
🏁 Script executed:
# Search for all usages of expandedCommits in the file
rg -n "expandedCommits" apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsxRepository: superset-sh/superset
Length of output: 300
Resetting expandedCommits in useEffect allows stale getCommitFiles queries to be registered against the new worktree.
When worktreePath changes, the component re-renders with trpc.useQueries(...) (line 61-68) still using the old expandedCommits state. Only after the render completes does the effect run to reset the state. This means queries for previous commit hashes execute against the new worktree before being cleared.
A more robust approach is to scope expansion state by worktreePath so switching workspaces naturally starts with an empty set without needing an effect:
- import { useEffect, useState } from "react";
+ import { useState } from "react";
...
- const [expandedCommits, setExpandedCommits] = useState<Set<string>>(
- new Set(),
- );
-
- // Reset expanded commits when workspace changes to avoid querying
- // old commit hashes against the new worktree
- // biome-ignore lint/correctness/useExhaustiveDependencies: intentionally resets on worktreePath change
- useEffect(() => {
- setExpandedCommits(new Set());
- }, [worktreePath]);
+ const [expandedCommitsByWorktree, setExpandedCommitsByWorktree] = useState<
+ Record<string, Set<string>>
+ >({});
+
+ const expandedCommits = worktreePath
+ ? (expandedCommitsByWorktree[worktreePath] ?? new Set<string>())
+ : new Set<string>();
...
const handleCommitToggle = (hash: string) => {
- setExpandedCommits((prev) => {
- const next = new Set(prev);
- if (next.has(hash)) next.delete(hash);
- else next.add(hash);
- return next;
- });
+ if (!worktreePath) return;
+ setExpandedCommitsByWorktree((prev) => {
+ const current = prev[worktreePath] ?? new Set<string>();
+ const next = new Set(current);
+ if (next.has(hash)) next.delete(hash);
+ else next.add(hash);
+ return { ...prev, [worktreePath]: next };
+ });
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
apps/desktop/src/renderer/screens/main/components/WorkspaceView/Sidebar/ChangesView/ChangesView.tsx
around lines 54 to 60, the effect-based reset of expandedCommits runs after
render which allows stale getCommitFiles queries for previous commits to fire
against a new worktree; instead, make the expansion state scoped to worktreePath
(e.g. track expanded commits per-worktree via an object/Map keyed by
worktreePath or include worktreePath in the state shape) so the initial render
uses an empty expansion set for a new worktree; update all accesses and setters
to read/write the per-worktree entry and initialize useState with the entry for
the current worktreePath (removing the post-render useEffect reset).
1 participant
Summary
oldPathtogetFileContentsand using it for the "original" side lookupexpandedCommitsstate when workspace changes to prevent querying old commit hashes against the new worktreeTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
✏️ Tip: You can customize this high-level summary in your review settings.