Skip to content

feat(desktop): git changes sidebar with resource-oriented API#3177

Merged
saddlepaddle merged 3 commits into
mainfrom
saddlepaddle/v2-diff
Apr 5, 2026
Merged

feat(desktop): git changes sidebar with resource-oriented API#3177
saddlepaddle merged 3 commits into
mainfrom
saddlepaddle/v2-diff

Conversation

@saddlepaddle
Copy link
Copy Markdown
Collaborator

@saddlepaddle saddlepaddle commented Apr 5, 2026

Summary

  • New git tRPC router with GitHub-aligned types mirroring GraphQL/REST schema
  • Resource-oriented endpoints: getStatus, listBranches, listCommits, getCommitFiles, getDiff, getPullRequest, getPullRequestThreads
  • ChangesTab sidebar with card-style header showing branch info, commit count, ahead/behind status, file stats
  • Commit filter dropdown (All changes / Uncommitted / Select range / individual commits)
  • Base branch selector with searchable popover
  • Branch rename (unpushed branches only) with inline edit
  • Tab registry pattern: hooks return SidebarTabDefinition, only active tab mounted
  • Filter state persisted in workspace local state

Test plan

  • Open v2 workspace, switch to Changes tab
  • Verify branch name and commit count display correctly
  • Change base branch via dropdown — file list updates
  • Switch commit filter: All changes → Uncommitted → specific commit
  • Select commit range via modal
  • Rename branch (only shows pencil icon on unpushed branches)
  • Verify ahead/behind origin status messages
  • File list groups by folder correctly

Summary by cubic

Introduce a Git Changes sidebar backed by a resource‑oriented git API. It shows accurate changes against base/staged/unstaged, supports commit/range filtering, base branch selection, and inline branch rename (unpushed only).

  • New Features

    • New git router with GitHub‑aligned types and endpoints: getStatus, listBranches, listCommits, getCommitFiles, getDiff, getPullRequest, getPullRequestThreads, renameBranch.
    • Changes tab header: branch info (inline rename), ahead/behind, commit count, file stats, and a searchable base branch selector.
    • Commit filter: All, Uncommitted, single commit, or range (modal). Filter and base branch persist per workspace.
    • Grouped file list with status badges and +/- counts; clicking a file triggers the diff callback.
  • Bug Fixes

    • Fixed unstaged diff to compare index :0: vs working tree (not HEAD).
    • Added warnings for GitHub thread fetch errors in getPullRequestThreads.
    • Reset range modal selection when the modal closes.

Written for commit b843f08. Summary will update on new commits.

Summary by CodeRabbit

  • New Features
    • Added a full "Changes" tab in the workspace sidebar showing repository status.
    • View and filter changes by against-base, staged, unstaged, commit, or commit range.
    • Base branch selector to compare against different branches.
    • Commit filter dropdown and interactive commit-range modal.
    • Navigable file change list with status badges and addition/deletion counts.
    • Inline branch rename and improved commit browsing/details.

Backend:
- Git tRPC router with GitHub-aligned types (PatchStatus, PullRequestState,
  CheckStatusState, etc.) mirroring GitHub's GraphQL/REST schema
- Resource-oriented endpoints: getStatus, listBranches, listCommits,
  getCommitFiles, getDiff, getPullRequest, getPullRequestThreads
- Proper file status detection using git status.files index/working_dir pairs
- Branch rename mutation (unpushed branches only)
- Router organized into git.ts + types.ts + utils/ per repo guidelines

Frontend:
- Tab registry pattern: each sidebar tab is a hook returning SidebarTabDefinition
- ChangesTab with card-style header: branch name (editable), base branch
  selector, commit count, ahead/behind origin status, file stats
- Commit filter dropdown: All changes / Uncommitted / Select range / individual
  commits. Filter state persisted in workspace local state.
- Commit range selection modal using Dialog + ScrollArea
- File list grouped by folder with status indicators
- Polling-based refresh (3s for status/commits, 30s for branches)
- Only active tab mounted, inactive tabs unmount
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 5, 2026

Caution

Review failed

Pull request was closed or merged during review

📝 Walkthrough

Walkthrough

Adds a "Changes" tab to the WorkspaceSidebar via a new useChangesTab hook, new UI components (branch selector, commit filter, file list, range modal), expanded git tRPC endpoints and helpers, and persisted sidebar state/types for filtering and base-branch selection.

Changes

Cohort / File(s) Summary
Sidebar refactor
apps/desktop/.../WorkspaceSidebar/WorkspaceSidebar.tsx, .../SidebarHeader/SidebarHeader.tsx, .../types.ts
Converted tab rendering to use SidebarTabDefinition[]; added optional onSelectDiffFile prop; header derives actions from active tab definition.
useChangesTab hook
.../hooks/useChangesTab/useChangesTab.tsx, .../hooks/useChangesTab/index.ts
New hook that builds the "Changes" tab definition, persists filter/baseBranch, runs TRPC queries (status, branches, commits, diffs), and composes ChangesHeader + ChangesFileList content.
Changes UI components
.../useChangesTab/components/BaseBranchSelector/*, .../ChangesFileList/*, .../CommitFilterDropdown/*, .../CommitRow/*, .../RangeModal/*
New UI: base branch popover with search, grouped changes file list with badges and grouping, commit filter dropdown with range modal, commit row display and selection handling.
Local schema / state
apps/desktop/.../providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
Added changesFilter discriminated union and baseBranch to workspace local sidebar state with defaults.
Git TRPC router & types
packages/host-service/src/trpc/router/git/git.ts, .../types.ts, .../index.ts
Replaced old status procedure with richer git router: listBranches, getStatus, listCommits, getCommitFiles, getDiff, renameBranch, getPullRequest, getPullRequestThreads; added shared types for branches, commits, changed files, PR data.
Git helpers & GraphQL parsing
packages/host-service/src/trpc/router/git/utils/git-helpers.ts, .../graphql.ts, .../resolve-worktree.ts
Added parsing utilities (numstat/name-status), status mapping, branch metadata builders, changed-file extraction, GraphQL review-threads parsing, and worktree path resolver for TRPC context.
Barrel re-exports
multiple index.ts under component dirs and useChangesTab/index.ts
Added local index re-exports for new components and hook types to simplify imports.

Sequence Diagram

sequenceDiagram
    actor User
    participant Sidebar as WorkspaceSidebar
    participant Hook as useChangesTab
    participant TRPC as Git TRPC Router
    participant Git as Git Utils
    participant DB as Database

    User->>Sidebar: Open Changes tab
    Sidebar->>Hook: initialize with workspaceId, onSelectFile?
    Hook->>TRPC: listBranches / getStatus / listCommits / getCommitFiles
    TRPC->>Git: run git commands, parse outputs
    Git->>DB: (resolve worktree path / read repo)
    Git-->>TRPC: branches, changed files, commits
    TRPC-->>Hook: structured responses
    Hook-->>Sidebar: tab content (ChangesFileList, CommitFilterDropdown, etc.)
    Sidebar-->>User: render changes UI
    User->>Sidebar: select file / change filter / pick base branch
    Sidebar->>Hook: emit selection/update
    Hook->>TRPC: refetch with updated params
    TRPC-->>Hook: updated data
    Hook-->>Sidebar: rerender updated lists
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 I nibble branches, hop through commits,
I chase the diffs in tiny skits.
A popover here, a modal there,
Files grouped and badges everywhere.
Hooray — the Changes tab blooms with wits!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.17% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'feat(desktop): git changes sidebar with resource-oriented API' clearly and concisely summarizes the main changes: a new git changes sidebar feature backed by a resource-oriented API. It aligns well with the substantial feature additions across the codebase.
Description check ✅ Passed The PR description includes a comprehensive summary of changes (git router, endpoints, sidebar features, filter/branch selectors, and persistence), a detailed test plan with checkboxes, and an auto-generated summary by cubic. It covers the Description, Type of Change (implied as New feature), and Testing sections, though it doesn't explicitly link related issues or include optional sections like Screenshots.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch saddlepaddle/v2-diff

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 5, 2026

🚀 Preview Deployment

🔗 Preview Links

Service Status Link
Neon Database (Neon) View Branch
Fly.io Electric (Fly.io) View App
Vercel API (Vercel) Open Preview
Vercel Web (Vercel) Open Preview
Vercel Marketing (Vercel) Open Preview
Vercel Admin (Vercel) Open Preview
Vercel Docs (Vercel) Open Preview

Preview updates automatically with new commits

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9 issues found across 22 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="packages/host-service/src/trpc/router/git/git.ts">

<violation number="1" location="packages/host-service/src/trpc/router/git/git.ts:53">
P2: Silent `catch {}` swallows GitHub API errors (auth failures, rate limits, network errors) without logging. The user will see empty review threads with no way to diagnose why. At minimum, log a warning with the error context.

(Based on your team's feedback about avoiding silently swallowed failures.) [FEEDBACK_USED]</violation>

<violation number="2" location="packages/host-service/src/trpc/router/git/git.ts:272">
P1: Incorrect base for unstaged diff. Unstaged changes should compare the index (`:0:`) against the working tree, not HEAD. When a file has both staged and unstaged modifications, this shows the staged changes as part of the unstaged diff.</violation>

<violation number="3" location="packages/host-service/src/trpc/router/git/git.ts:289">
P2: Use `path.basename()` instead of `split("/").pop()` for cross-platform path handling. On Windows, git may return paths with backslashes, causing this to return the full path instead of just the filename.

(Based on your team's feedback about using cross-platform path utilities instead of split.) [FEEDBACK_USED]</violation>
</file>

<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx">

<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx:325">
P1: Uncommitted mode labels staged files as `unstaged`, so file selection callbacks receive the wrong diff category.</violation>
</file>

<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx">

<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx:60">
P1: Changes-tab file selection is effectively disabled when `onSelectDiffFile` is not provided. Add a fallback to `onSelectFile` so selecting changed files still opens a file.</violation>
</file>

<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/components/RangeModal/RangeModal.tsx">

<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/components/RangeModal/RangeModal.tsx:66">
P2: Reset range selection when the modal closes; otherwise stale commit selections persist across reopen.</violation>
</file>

<file name="packages/host-service/src/trpc/router/git/utils/git-helpers.ts">

<violation number="1" location="packages/host-service/src/trpc/router/git/utils/git-helpers.ts:73">
P2: Catching all diff errors and returning `[]` masks real failures as a clean diff. At minimum, emit diagnostics in this catch path so failures are observable.

(Based on your team's feedback about handling async failures explicitly and avoiding empty catch blocks.) [FEEDBACK_USED]</violation>

<violation number="2" location="packages/host-service/src/trpc/router/git/utils/git-helpers.ts:116">
P2: Do not silently swallow git command errors in `buildBranch`; add warning-level diagnostics so incorrect fallback branch metadata is visible.

(Based on your team's feedback about handling async failures explicitly and avoiding empty catch blocks.) [FEEDBACK_USED]</violation>
</file>

<file name="packages/host-service/src/trpc/router/git/utils/graphql.ts">

<violation number="1" location="packages/host-service/src/trpc/router/git/utils/graphql.ts:11">
P2: Review threads are truncated to 100 because this GraphQL request is not paginated.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment thread packages/host-service/src/trpc/router/git/git.ts
<ChangesFileList
files={[...status.data.staged, ...status.data.unstaged]}
onSelectFile={onSelectFile}
category="unstaged"
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Uncommitted mode labels staged files as unstaged, so file selection callbacks receive the wrong diff category.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx, line 325:

<comment>Uncommitted mode labels staged files as `unstaged`, so file selection callbacks receive the wrong diff category.</comment>

<file context>
@@ -0,0 +1,395 @@
+				<ChangesFileList
+					files={[...status.data.staged, ...status.data.unstaged]}
+					onSelectFile={onSelectFile}
+					category="unstaged"
+				/>
+			);
</file context>
Fix with Cubic

/>
const changesTab = useChangesTab({
workspaceId,
onSelectFile: onSelectDiffFile,
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Changes-tab file selection is effectively disabled when onSelectDiffFile is not provided. Add a fallback to onSelectFile so selecting changed files still opens a file.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx, line 60:

<comment>Changes-tab file selection is effectively disabled when `onSelectDiffFile` is not provided. Add a fallback to `onSelectFile` so selecting changed files still opens a file.</comment>

<file context>
@@ -43,50 +47,60 @@ function IconButton({
-			/>
+	const changesTab = useChangesTab({
+		workspaceId,
+		onSelectFile: onSelectDiffFile,
+	});
 
</file context>
Suggested change
onSelectFile: onSelectDiffFile,
onSelectFile:
onSelectDiffFile ??
((path) => {
onSelectFile(path);
}),
Fix with Cubic

Comment thread packages/host-service/src/trpc/router/git/git.ts
} catch {}
}

const fileName = input.path.split("/").pop() ?? input.path;
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Use path.basename() instead of split("/").pop() for cross-platform path handling. On Windows, git may return paths with backslashes, causing this to return the full path instead of just the filename.

(Based on your team's feedback about using cross-platform path utilities instead of split.)

View Feedback

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/git/git.ts, line 289:

<comment>Use `path.basename()` instead of `split("/").pop()` for cross-platform path handling. On Windows, git may return paths with backslashes, causing this to return the full path instead of just the filename.

(Based on your team's feedback about using cross-platform path utilities instead of split.) </comment>

<file context>
@@ -1,24 +1,440 @@
+				} catch {}
+			}
+
+			const fileName = input.path.split("/").pop() ?? input.path;
 			return {
-				current: status.current,
</file context>
Fix with Cubic

const [behind, ahead] = counts.split("\t").map(Number);
aheadCount = ahead ?? 0;
behindCount = behind ?? 0;
} catch {}
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Do not silently swallow git command errors in buildBranch; add warning-level diagnostics so incorrect fallback branch metadata is visible.

(Based on your team's feedback about handling async failures explicitly and avoiding empty catch blocks.)

View Feedback

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/git/utils/git-helpers.ts, line 116:

<comment>Do not silently swallow git command errors in `buildBranch`; add warning-level diagnostics so incorrect fallback branch metadata is visible.

(Based on your team's feedback about handling async failures explicitly and avoiding empty catch blocks.) </comment>

<file context>
@@ -0,0 +1,160 @@
+			const [behind, ahead] = counts.split("\t").map(Number);
+			aheadCount = ahead ?? 0;
+			behindCount = behind ?? 0;
+		} catch {}
+	}
+
</file context>
Fix with Cubic

"--short",
]);
return ref.trim().replace(/^origin\//, "");
} catch {
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Catching all diff errors and returning [] masks real failures as a clean diff. At minimum, emit diagnostics in this catch path so failures are observable.

(Based on your team's feedback about handling async failures explicitly and avoiding empty catch blocks.)

View Feedback

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/git/utils/git-helpers.ts, line 73:

<comment>Catching all diff errors and returning `[]` masks real failures as a clean diff. At minimum, emit diagnostics in this catch path so failures are observable.

(Based on your team's feedback about handling async failures explicitly and avoiding empty catch blocks.) </comment>

<file context>
@@ -0,0 +1,160 @@
+			"--short",
+		]);
+		return ref.trim().replace(/^origin\//, "");
+	} catch {
+		return null;
+	}
</file context>
Fix with Cubic

query($owner: String!, $name: String!, $prNumber: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $prNumber) {
reviewThreads(first: 100) {
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Review threads are truncated to 100 because this GraphQL request is not paginated.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/host-service/src/trpc/router/git/utils/graphql.ts, line 11:

<comment>Review threads are truncated to 100 because this GraphQL request is not paginated.</comment>

<file context>
@@ -0,0 +1,83 @@
+	query($owner: String!, $name: String!, $prNumber: Int!) {
+		repository(owner: $owner, name: $name) {
+			pullRequest(number: $prNumber) {
+				reviewThreads(first: 100) {
+					nodes {
+						id
</file context>
Fix with Cubic

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 5, 2026

Greptile Summary

This PR introduces a full git changes sidebar for the v2 desktop workspace, replacing a "coming soon" placeholder with a production-ready feature. It adds a resource-oriented tRPC git router (getStatus, listBranches, listCommits, getCommitFiles, getDiff, getPullRequest, getPullRequestThreads) backed by simple-git, and wires a ChangesTab UI with a branch info header, commit filter dropdown, base branch selector, and file list grouped by folder. The tab registry pattern (SidebarTabDefinition) is a clean upgrade that lets each hook own its tab definition, including badge counts.

Key changes:

  • New git tRPC router with GitHub-aligned types replacing the old stub status endpoint
  • useChangesTab hook returns a SidebarTabDefinition with live queries and badge count
  • Commit filter supports All / Uncommitted / single commit / range via modal
  • Branch rename (unpushed only) with inline edit, validated server-side via ls-remote
  • Filter and base branch persisted in workspace local state via Zod schema

Two issues need attention before shipping: RangeModal state is not reset on Cancel (stale selection persists on next open), and renameBranch mutation does not invalidate related queries (UI stays stale for up to 3 s).

Confidence Score: 4/5

Safe to merge after addressing the two P1 issues (RangeModal state reset and renameBranch query invalidation)

The backend router is well-structured with proper error handling and GitHub-aligned types. The two P1 issues are visible UX bugs — stale range selection and a 3-second stale branch name after rename — that should be fixed before shipping to users, but they carry no data-loss or security risk. The remaining P2 items are style/cleanup. All other aspects of the implementation are solid.

RangeModal.tsx (state reset on cancel) and useChangesTab.tsx (mutation invalidation + redundant scroll wrapper)

Important Files Changed

Filename Overview
packages/host-service/src/trpc/router/git/git.ts Comprehensive git tRPC router replacing old stub; solid error handling throughout, getPullRequest hardcodes body: null as a placeholder
packages/host-service/src/trpc/router/git/utils/git-helpers.ts Well-structured git utility helpers; binary file numstat handled correctly, rename/copy parsing correct
packages/host-service/src/trpc/router/git/utils/graphql.ts Clean GraphQL query and parser for PR review threads; handles null authors and derives thread line from first comment
packages/host-service/src/trpc/router/git/utils/resolve-worktree.ts Simple workspace path resolver; correctly throws NOT_FOUND when worktree path is absent
packages/host-service/src/trpc/router/git/types.ts Well-documented type definitions mirroring GitHub's GraphQL/REST schema; clean separation of platform enums and local resource types
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx Core hook wiring all git queries; missing query invalidation after renameBranch mutation and has redundant scroll wrapper around file list
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/components/RangeModal/RangeModal.tsx Range selection modal; state not reset on Cancel causing stale selection on next open
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx Clean refactor to tab registry pattern using SidebarTabDefinition; correctly mounts only the active tab
apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts Correctly adds changesFilter discriminated union and baseBranch with proper Zod defaults
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx Solid file list with folder grouping; scroll container wrapper on the component is redundant given the caller already provides one

Sequence Diagram

sequenceDiagram
    participant UI as WorkspaceSidebar
    participant Hook as useChangesTab
    participant tRPC as git tRPC Router
    participant Git as simple-git
    participant GH as GitHub API

    UI->>Hook: render(workspaceId)
    Hook->>tRPC: getStatus(workspaceId, baseBranch?)
    tRPC->>Git: revparse HEAD + status() + diff --name-status
    Git-->>tRPC: branch info + file changes
    tRPC-->>Hook: {currentBranch, againstBase, staged, unstaged}

    Hook->>tRPC: listCommits(workspaceId, baseBranch?)
    tRPC->>Git: git log origin/base..HEAD
    Git-->>tRPC: commit list
    tRPC-->>Hook: {commits[]}

    Hook->>tRPC: listBranches(workspaceId)
    tRPC->>Git: git branch --list
    Git-->>tRPC: branch names + ahead/behind counts
    tRPC-->>Hook: {branches[]}

    Note over Hook: filter = commit or range
    Hook->>tRPC: getCommitFiles(workspaceId, commitHash, fromHash?)
    tRPC->>Git: diff --name-status from..to
    Git-->>tRPC: changed files
    tRPC-->>Hook: {files[]}

    Note over Hook: user renames branch
    Hook->>tRPC: renameBranch(oldName, newName)
    tRPC->>Git: ls-remote --heads origin oldName
    Git-->>tRPC: empty = not pushed
    tRPC->>Git: git branch -m oldName newName
    Git-->>tRPC: success
    tRPC-->>Hook: {name: newName}
    Note over Hook: ⚠️ queries NOT invalidated; stale for up to 3 s

    Note over tRPC: getPullRequestThreads
    tRPC->>GH: GraphQL reviewThreads query
    GH-->>tRPC: thread nodes
    tRPC->>GH: REST issues.listComments (paginated)
    GH-->>tRPC: conversation comments
    tRPC-->>UI: {reviewThreads[], conversationComments[]}
Loading

Comments Outside Diff (1)

  1. apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx, line 504-516 (link)

    P2 Redundant scroll container inside ChangesFileList

    ChangesFileList wraps its output in <div className="min-h-0 flex-1 overflow-y-auto"> (both in the single-list and three-section paths). In useChangesTab, this component is already placed inside:

    <div className="min-h-0 flex-1 overflow-y-auto">{fileList}</div>

    Because the outer div is the actual scroll container and is not a flex container, the inner flex-1 has no effect and the inner overflow-y-auto never activates — all scrolling is handled by the outer wrapper. The inner classes are dead code and may mislead future maintainers into thinking ChangesFileList manages its own scroll context. Consider removing the wrapper div from ChangesFileList so the caller owns the scroll container.

Reviews (1): Last reviewed commit: "feat(desktop): git changes sidebar with ..." | Re-trigger Greptile

Comment on lines +89 to +91
>
<CommitRow commit={commit} />
</button>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Range selection state not reset on Cancel

When the dialog is dismissed via the Cancel button (or clicking outside), fromIdx and toIdx are not reset. The next time the user opens the range modal, the previously highlighted range will still appear selected, making it look like a selection is already active.

handleApply correctly resets state (setFromIdx(null); setToIdx(null)) after applying, but the cancel/dismiss path does not. The simplest fix is to also reset state on onOpenChange:

// Wrap the Dialog's onOpenChange:
<Dialog
  open={open}
  onOpenChange={(o) => {
    if (!o) { setFromIdx(null); setToIdx(null); }
    onOpenChange(o);
  }}
  modal
>

This ensures Escape-key dismissal also clears the state.

Comment on lines +130 to +148
{currentBranch.aheadCount} local not pushed,{" "}
{currentBranch.behindCount} remote to pull
</div>
</div>
)}
{currentBranch.aheadCount > 0 && currentBranch.behindCount === 0 && (
<div className="text-[11px] text-muted-foreground">
<div>
{currentBranch.aheadCount}{" "}
{currentBranch.aheadCount === 1 ? "commit" : "commits"} ahead of
</div>
<div className="font-medium text-foreground">
origin/{currentBranch.name}
</div>
</div>
)}
{currentBranch.behindCount > 0 && currentBranch.aheadCount === 0 && (
<div className="text-[11px] text-muted-foreground">
<div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Branch name stale for up to 3 s after rename

After renameBranchMutation.mutateAsync resolves, neither the status nor branches queries are invalidated. Both have refetchInterval: 3_000, so the sidebar header will continue displaying the old branch name — and the inline edit field will show the stale name — for up to 3 seconds. During that window the user could attempt to rename again.

Consider invalidating immediately on success:

const utils = workspaceTrpc.useUtils();
const renameBranchMutation = workspaceTrpc.git.renameBranch.useMutation({
  onSuccess: () => {
    void utils.git.getStatus.invalidate({ workspaceId });
    void utils.git.listBranches.invalidate({ workspaceId });
  },
});

Comment on lines +204 to +207
const files = await getChangedFilesForDiff(git, [from, input.commitHash]);

return { files };
}),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 body field is always null

The body field in getPullRequest is hardcoded to null as string | null. Any downstream UI that renders the PR description will always receive null. If the pullRequests table stores a body/description column, wire it up here; otherwise consider omitting the field from the response type so callers don't silently depend on it having a value.

Comment on lines +254 to +263

// Can only rename if branch hasn't been pushed (aheadCount === total commits means nothing pushed)
const canRenameBranch = !status.data?.currentBranch.upstream;

const commitFilesInput =
filter.kind === "commit"
? { workspaceId, commitHash: filter.hash }
: filter.kind === "range"
? { workspaceId, commitHash: filter.toHash, fromHash: filter.fromHash }
: { workspaceId, commitHash: "" };
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Empty commitHash in the fallback query input

When filter.kind is "all" or "uncommitted", commitFilesInput is { workspaceId, commitHash: "" }. The query is correctly disabled, but if the enabled guard is ever relaxed or a future refactor moves the input construction, the server will receive an empty hash and attempt git diff ""^.."", silently returning [] rather than an error.

A safer pattern:

const commitFilesInput =
  filter.kind === "commit"
    ? { workspaceId, commitHash: filter.hash }
    : filter.kind === "range"
      ? { workspaceId, commitHash: filter.toHash, fromHash: filter.fromHash }
      : undefined;

const commitFiles = workspaceTrpc.git.getCommitFiles.useQuery(
  commitFilesInput!,
  { enabled: commitFilesInput !== undefined },
);

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (4)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx (1)

149-165: Add expanded-state accessibility metadata to section toggles.

The collapsible header button should expose expansion state and control target for assistive tech.

Proposed accessibility tweak
 function Section({
@@
 }) {
 	const [isOpen, setIsOpen] = useState(defaultOpen);
 	const groups = useMemo(() => groupByFolder(files), [files]);
+	const sectionId = `changes-section-${title.toLowerCase().replace(/\s+/g, "-")}`;
@@
 			<button
 				type="button"
 				className="flex w-full items-center gap-1 px-2 py-1.5 text-[11px] font-semibold uppercase tracking-wider text-muted-foreground hover:bg-accent/50"
 				onClick={() => setIsOpen(!isOpen)}
+				aria-expanded={isOpen}
+				aria-controls={sectionId}
 			>
@@
-			{isOpen &&
-				groups.map((group) => (
-					<FolderGroup
-						key={group.folder}
-						folder={group.folder}
-						files={group.files}
-						category={category}
-						onSelectFile={onSelectFile}
-					/>
-				))}
+			{isOpen && (
+				<div id={sectionId}>
+					{groups.map((group) => (
+						<FolderGroup
+							key={group.folder}
+							folder={group.folder}
+							files={group.files}
+							category={category}
+							onSelectFile={onSelectFile}
+						/>
+					))}
+				</div>
+			)}
 		</div>
 	);
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx
around lines 149 - 165, The collapsible header button is missing accessibility
metadata; update the toggle button in ChangesFileList to include
aria-expanded={isOpen} and aria-controls referencing the ID of the collapsible
panel (e.g., aria-controls={`changes-panel-${someUniqueKey}`}), and add a
matching id attribute on the container element that renders the groups/rows
(e.g., id={`changes-panel-${someUniqueKey}`}) so assistive tech can associate
the control with the content; generate a stable unique key (use workspaceId or
the title prop plus an index) to avoid duplicate IDs and keep existing
setIsOpen/isOpen behavior.
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx (1)

93-103: Consider memoizing the tabs array.

The tabs array is recreated on every render. If SidebarHeader or child components perform reference equality checks on this prop, it could cause unnecessary re-renders. This is a minor optimization that can be deferred.

♻️ Optional: Memoize tabs array
-	const tabs = [filesTab, changesTab, checksTab];
+	const tabs = useMemo(
+		() => [filesTab, changesTab, checksTab],
+		[filesTab, changesTab, checksTab],
+	);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx
around lines 93 - 103, The tabs array is recreated each render which can trigger
unnecessary re-renders downstream; wrap the construction of tabs (currently
"const tabs = [filesTab, changesTab, checksTab];") in a React useMemo keyed on
the dependent variables (e.g. filesTab, changesTab, checksTab, and optionally
activeTab if used) so it returns a stable reference passed into SidebarHeader
and used for activeTabDef; update any references to tabs and activeTabDef
accordingly to use the memoized value.
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/components/RangeModal/RangeModal.tsx (1)

32-45: Selection state persists after Cancel.

When the user clicks Cancel, the modal closes but fromIdx/toIdx are not reset. Reopening the modal shows the previous partial selection. If this is intentional (allowing users to resume selection), this is fine. Otherwise, consider resetting state when the modal closes:

♻️ Optional: Reset selection on modal close
 export function RangeModal({
 	open,
 	onOpenChange,
 	commits,
 	onSelect,
 }: RangeModalProps) {
 	const [fromIdx, setFromIdx] = useState<number | null>(null);
 	const [toIdx, setToIdx] = useState<number | null>(null);

+	const handleOpenChange = (nextOpen: boolean) => {
+		if (!nextOpen) {
+			setFromIdx(null);
+			setToIdx(null);
+		}
+		onOpenChange(nextOpen);
+	};
+
 	// ... rest of component
 
-	<Dialog open={open} onOpenChange={onOpenChange} modal>
+	<Dialog open={open} onOpenChange={handleOpenChange} modal>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/components/RangeModal/RangeModal.tsx
around lines 32 - 45, The modal currently keeps selection state (fromIdx/toIdx)
across opens; when the modal closes the previous selection still appears. In
RangeModal reset the selection by clearing fromIdx and toIdx (setFromIdx(null);
setToIdx(null)) when the modal is closed—either call those setters inside the
existing onClose handler or add a useEffect that watches the modal open flag
(e.g., isOpen or visible prop) and clears state when it becomes false; update
RangeModal's cleanup path so reopening starts with null selection.
apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/BaseBranchSelector/BaseBranchSelector.tsx (1)

43-50: Consider adding an accessible label to the search input.

The search input lacks an accessible label for screen readers. Adding aria-label would improve accessibility.

♿ Optional: Add aria-label for accessibility
 				<input
+					aria-label="Search branches"
 					placeholder="Search branches..."
 					value={search}
 					onChange={(e) => setSearch(e.target.value)}
 					className="w-full bg-transparent text-sm outline-none placeholder:text-muted-foreground"
 				/>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/BaseBranchSelector/BaseBranchSelector.tsx
around lines 43 - 50, The search input in BaseBranchSelector lacks an accessible
label; add an aria-label (or associate a visible/visually-hidden <label> with an
id) to the input used with the search state (value={search},
onChange={setSearch}) so screen readers can announce its purpose (e.g.,
aria-label="Search branches"). Ensure the attribute is added to the same input
element inside the BaseBranchSelector component so accessibility tools can
identify the control.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx:
- Around line 21-24: The current onSelectFile callback signature (onSelectFile?:
(path: string, category: "against-base" | "staged" | "unstaged") => void) is too
shallow and loses per-file diff metadata (oldPath, source, commit/range ids)
needed to open the correct patch; update the signature used in useChangesTab and
any callers to accept a richer selection object (e.g., { path, category,
oldPath?, source?, commitId?, range? }) and propagate that object from the row
click handlers in the ChangesTab rows so renamed files, staged/uncommitted rows,
and commit/range rows retain their metadata when selected. Ensure all references
to onSelectFile in useChangesTab, row renderers, and parent components are
updated to accept and forward this selection payload.

In `@packages/host-service/src/trpc/router/git/git.ts`:
- Around line 244-251: The getDiff protectedProcedure currently uses
client-controlled input.path directly when building file paths (e.g.,
`${worktreePath}/${input.path}`), allowing path traversal; fix by validating and
normalizing input.path before any disk access: ensure input.path is not
absolute, reject or canonicalize any '..' segments, use path.resolve(pathRoot,
input.path) or path.join+path.normalize and then verify the resolved path has
the repository worktree root as its prefix (e.g.,
resolved.startsWith(path.resolve(worktreePath) + path.sep)); add a Zod
refinement on the input schema for path to enforce these constraints and apply
the same validation for the other occurrences referenced (lines ~282-285) so no
client-supplied path can escape the repository.
- Around line 45-66: The branch selector currently lists only local names and
later blindly prefixes selections with origin/, causing local-only branches to
resolve to non-existent remote refs; update the listBranches behavior (where
git.raw, branchNames, currentBranchName, defaultBranchName and buildBranch are
used) to either include remote-tracking refs (e.g., list refs under
refs/remotes/* as well as refs/heads/* and pass the full ref name into
buildBranch) or add a small resolver (e.g., resolveBranchRef) that, given a
selected name, checks for an existing local ref (refs/heads/<name>), a
remote-tracking ref (refs/remotes/<remote>/<name>), or uses
git.rev-parse/--symbolic-full-name to produce the actual ref and return that
full ref to the downstream diff/status/commit procedures; apply the same fix to
the other locations that currently rewrite selections to origin/ (the blocks
around the other uses of buildBranch/rewrite to origin).
- Around line 277-286: The code uses git.show([`HEAD:${input.path}`]) to
populate originalContent which incorrectly includes staged changes; change that
call to use the index snapshot (e.g. git.show([`:${input.path}`])) so the
"unstaged" left-side reflects the index, not HEAD, while keeping the existing
try/catch behavior; update the git.show call that sets originalContent (and
leave the readFile for modifiedContent and worktreePath/input.path as-is).
- Around line 221-238: The current remote probe using git.raw(["ls-remote",
"--heads", "origin", input.oldName]) can fail open on network/auth errors;
replace it with a local upstream check instead: call git.revparse (or git.raw
with ["rev-parse", "--abbrev-ref", `${input.oldName}@{u}`]) or use git.branch to
read the branch's configured upstream for the branch name referenced in this
code block, and if that upstream exists treat the branch as pushed and throw the
same TRPCError (code "PRECONDITION_FAILED", message "Cannot rename a branch that
has been pushed to remote"); only treat the branch as safe-to-rename when
rev-parse/branch returns “no upstream” (handle that specific case as non-error)
and propagate other unexpected errors. Ensure you update the code around the
git.raw ls-remote usage and keep the TRPCError behavior when an upstream is
found.

---

Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/BaseBranchSelector/BaseBranchSelector.tsx:
- Around line 43-50: The search input in BaseBranchSelector lacks an accessible
label; add an aria-label (or associate a visible/visually-hidden <label> with an
id) to the input used with the search state (value={search},
onChange={setSearch}) so screen readers can announce its purpose (e.g.,
aria-label="Search branches"). Ensure the attribute is added to the same input
element inside the BaseBranchSelector component so accessibility tools can
identify the control.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx:
- Around line 149-165: The collapsible header button is missing accessibility
metadata; update the toggle button in ChangesFileList to include
aria-expanded={isOpen} and aria-controls referencing the ID of the collapsible
panel (e.g., aria-controls={`changes-panel-${someUniqueKey}`}), and add a
matching id attribute on the container element that renders the groups/rows
(e.g., id={`changes-panel-${someUniqueKey}`}) so assistive tech can associate
the control with the content; generate a stable unique key (use workspaceId or
the title prop plus an index) to avoid duplicate IDs and keep existing
setIsOpen/isOpen behavior.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/components/RangeModal/RangeModal.tsx:
- Around line 32-45: The modal currently keeps selection state (fromIdx/toIdx)
across opens; when the modal closes the previous selection still appears. In
RangeModal reset the selection by clearing fromIdx and toIdx (setFromIdx(null);
setToIdx(null)) when the modal is closed—either call those setters inside the
existing onClose handler or add a useEffect that watches the modal open flag
(e.g., isOpen or visible prop) and clears state when it becomes false; update
RangeModal's cleanup path so reopening starts with null selection.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx:
- Around line 93-103: The tabs array is recreated each render which can trigger
unnecessary re-renders downstream; wrap the construction of tabs (currently
"const tabs = [filesTab, changesTab, checksTab];") in a React useMemo keyed on
the dependent variables (e.g. filesTab, changesTab, checksTab, and optionally
activeTab if used) so it returns a stable reference passed into SidebarHeader
and used for activeTabDef; update any references to tabs and activeTabDef
accordingly to use the memoized value.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c5c1b659-8de2-4225-a91f-78cd006cab9f

📥 Commits

Reviewing files that changed from the base of the PR and between 864977d and 29d7879.

📒 Files selected for processing (22)
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/WorkspaceSidebar.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/components/SidebarHeader/SidebarHeader.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/BaseBranchSelector/BaseBranchSelector.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/BaseBranchSelector/index.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/ChangesFileList.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/ChangesFileList/index.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/CommitFilterDropdown.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/components/CommitRow/CommitRow.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/components/CommitRow/index.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/components/RangeModal/RangeModal.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/components/RangeModal/index.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/components/CommitFilterDropdown/index.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/index.ts
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx
  • apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/components/WorkspaceSidebar/types.ts
  • apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/dashboardSidebarLocal/schema.ts
  • packages/host-service/src/trpc/router/git/git.ts
  • packages/host-service/src/trpc/router/git/index.ts
  • packages/host-service/src/trpc/router/git/types.ts
  • packages/host-service/src/trpc/router/git/utils/git-helpers.ts
  • packages/host-service/src/trpc/router/git/utils/graphql.ts
  • packages/host-service/src/trpc/router/git/utils/resolve-worktree.ts

Comment on lines +21 to +24
onSelectFile?: (
path: string,
category: "against-base" | "staged" | "unstaged",
) => void;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Carry per-file diff metadata through selection.

These branches flatten files from different sources and then assign one category to every row. A staged row in the uncommitted view, a renamed row that needs oldPath, and any commit/range row all lose the context needed to open the correct diff, so row selection can only show the wrong patch. Pass a richer selection payload than just (path, category).

Also applies to: 311-344

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/`$workspaceId/components/WorkspaceSidebar/hooks/useChangesTab/useChangesTab.tsx
around lines 21 - 24, The current onSelectFile callback signature
(onSelectFile?: (path: string, category: "against-base" | "staged" | "unstaged")
=> void) is too shallow and loses per-file diff metadata (oldPath, source,
commit/range ids) needed to open the correct patch; update the signature used in
useChangesTab and any callers to accept a richer selection object (e.g., { path,
category, oldPath?, source?, commitId?, range? }) and propagate that object from
the row click handlers in the ChangesTab rows so renamed files,
staged/uncommitted rows, and commit/range rows retain their metadata when
selected. Ensure all references to onSelectFile in useChangesTab, row renderers,
and parent components are updated to accept and forward this selection payload.

Comment on lines +45 to +66
let branchNames: string[] = [];
try {
const raw = await git.raw([
"branch",
"--list",
"--format=%(refname:short)",
]);
branchNames = raw.trim().split("\n").filter(Boolean);
} catch {}

const branches = await Promise.all(
branchNames.map((name) =>
buildBranch(
git,
name,
name === currentBranchName,
defaultBranchName ? `origin/${defaultBranchName}` : undefined,
),
),
);

return { branches };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't treat every selected base branch as origin/<name>.

listBranches exposes local branch names, but the other procedures always rewrite the selection to an origin/... ref. Selecting a local-only branch from the popover will therefore produce empty status, commit, and diff results. Either surface remote-tracking refs in the selector or resolve the chosen branch to a real local/remote ref before diffing.

Also applies to: 83-87, 164-166, 261-264

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/host-service/src/trpc/router/git/git.ts` around lines 45 - 66, The
branch selector currently lists only local names and later blindly prefixes
selections with origin/, causing local-only branches to resolve to non-existent
remote refs; update the listBranches behavior (where git.raw, branchNames,
currentBranchName, defaultBranchName and buildBranch are used) to either include
remote-tracking refs (e.g., list refs under refs/remotes/* as well as
refs/heads/* and pass the full ref name into buildBranch) or add a small
resolver (e.g., resolveBranchRef) that, given a selected name, checks for an
existing local ref (refs/heads/<name>), a remote-tracking ref
(refs/remotes/<remote>/<name>), or uses git.rev-parse/--symbolic-full-name to
produce the actual ref and return that full ref to the downstream
diff/status/commit procedures; apply the same fix to the other locations that
currently rewrite selections to origin/ (the blocks around the other uses of
buildBranch/rewrite to origin).

Comment on lines +221 to +238
// Check if branch has been pushed to remote
try {
const remote = await git.raw([
"ls-remote",
"--heads",
"origin",
input.oldName,
]);
if (remote.trim()) {
throw new TRPCError({
code: "PRECONDITION_FAILED",
message: "Cannot rename a branch that has been pushed to remote",
});
}
} catch (error) {
if (error instanceof TRPCError) throw error;
// ls-remote failed — probably no remote, safe to rename
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

The rename guard fails open when the remote check errors.

Any auth/network failure in ls-remote falls through the catch and still renames the branch, so an already-pushed branch can be renamed while offline. The server-side gate should use local upstream config, not a best-effort remote probe.

🔒 Suggested server-side guard
-			// Check if branch has been pushed to remote
-			try {
-				const remote = await git.raw([
-					"ls-remote",
-					"--heads",
-					"origin",
-					input.oldName,
-				]);
-				if (remote.trim()) {
-					throw new TRPCError({
-						code: "PRECONDITION_FAILED",
-						message: "Cannot rename a branch that has been pushed to remote",
-					});
-				}
-			} catch (error) {
-				if (error instanceof TRPCError) throw error;
-				// ls-remote failed — probably no remote, safe to rename
-			}
+			const remote = (
+				await git.raw(["config", `branch.${input.oldName}.remote`]).catch(() => "")
+			).trim();
+			const merge = (
+				await git.raw(["config", `branch.${input.oldName}.merge`]).catch(() => "")
+			).trim();
+			if (remote && merge) {
+				throw new TRPCError({
+					code: "PRECONDITION_FAILED",
+					message: "Cannot rename a branch that has been pushed to remote",
+				});
+			}
📝 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
// Check if branch has been pushed to remote
try {
const remote = await git.raw([
"ls-remote",
"--heads",
"origin",
input.oldName,
]);
if (remote.trim()) {
throw new TRPCError({
code: "PRECONDITION_FAILED",
message: "Cannot rename a branch that has been pushed to remote",
});
}
} catch (error) {
if (error instanceof TRPCError) throw error;
// ls-remote failed — probably no remote, safe to rename
}
const remote = (
await git.raw(["config", `branch.${input.oldName}.remote`]).catch(() => "")
).trim();
const merge = (
await git.raw(["config", `branch.${input.oldName}.merge`]).catch(() => "")
).trim();
if (remote && merge) {
throw new TRPCError({
code: "PRECONDITION_FAILED",
message: "Cannot rename a branch that has been pushed to remote",
});
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/host-service/src/trpc/router/git/git.ts` around lines 221 - 238, The
current remote probe using git.raw(["ls-remote", "--heads", "origin",
input.oldName]) can fail open on network/auth errors; replace it with a local
upstream check instead: call git.revparse (or git.raw with ["rev-parse",
"--abbrev-ref", `${input.oldName}@{u}`]) or use git.branch to read the branch's
configured upstream for the branch name referenced in this code block, and if
that upstream exists treat the branch as pushed and throw the same TRPCError
(code "PRECONDITION_FAILED", message "Cannot rename a branch that has been
pushed to remote"); only treat the branch as safe-to-rename when
rev-parse/branch returns “no upstream” (handle that specific case as non-error)
and propagate other unexpected errors. Ensure you update the code around the
git.raw ls-remote usage and keep the TRPCError behavior when an upstream is
found.

Comment on lines +244 to +251
getDiff: protectedProcedure
.input(
z.object({
workspaceId: z.string(),
path: z.string(),
category: z.enum(["against-base", "staged", "unstaged"]),
baseBranch: z.string().optional(),
}),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Validate input.path before reading from disk.

path is client-controlled here. Interpolating it into ${worktreePath}/${input.path} lets ../../... escape the repository and read arbitrary files through the unstaged diff endpoint.

🛡️ Suggested hardening
 import { readFile } from "node:fs/promises";
+import { relative, resolve, sep } from "node:path";
 import { TRPCError } from "@trpc/server";
 ...
+function resolveWorktreeFilePath(worktreePath: string, filePath: string): string {
+	const root = resolve(worktreePath);
+	const absolutePath = resolve(root, filePath);
+	const relativePath = relative(root, absolutePath);
+
+	if (
+		relativePath === ".." ||
+		relativePath.startsWith(`..${sep}`)
+	) {
+		throw new TRPCError({
+			code: "BAD_REQUEST",
+			message: "Invalid file path",
+		});
+	}
+
+	return absolutePath;
+}
+
 ...
 				try {
 					modifiedContent = await readFile(
-						`${worktreePath}/${input.path}`,
+						resolveWorktreeFilePath(worktreePath, input.path),
 						"utf-8",
 					);
 				} catch {}

Also applies to: 282-285

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/host-service/src/trpc/router/git/git.ts` around lines 244 - 251, The
getDiff protectedProcedure currently uses client-controlled input.path directly
when building file paths (e.g., `${worktreePath}/${input.path}`), allowing path
traversal; fix by validating and normalizing input.path before any disk access:
ensure input.path is not absolute, reject or canonicalize any '..' segments, use
path.resolve(pathRoot, input.path) or path.join+path.normalize and then verify
the resolved path has the repository worktree root as its prefix (e.g.,
resolved.startsWith(path.resolve(worktreePath) + path.sep)); add a Zod
refinement on the input schema for path to enforce these constraints and apply
the same validation for the other occurrences referenced (lines ~282-285) so no
client-supplied path can escape the repository.

Comment on lines +277 to +286
} else {
try {
originalContent = await git.show([`HEAD:${input.path}`]);
} catch {}
try {
modifiedContent = await readFile(
`${worktreePath}/${input.path}`,
"utf-8",
);
} catch {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use the index, not HEAD, for unstaged diffs.

When a file has both staged and unstaged hunks, HEAD:${input.path} makes the “unstaged” view include the staged delta too. The left side here should be the index snapshot so only working-tree-only edits are shown.

🩹 Suggested fix
 			} else {
 				try {
-					originalContent = await git.show([`HEAD:${input.path}`]);
+					originalContent = await git.show([`:0:${input.path}`]);
 				} catch {}
 				try {
 					modifiedContent = await readFile(
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/host-service/src/trpc/router/git/git.ts` around lines 277 - 286, The
code uses git.show([`HEAD:${input.path}`]) to populate originalContent which
incorrectly includes staged changes; change that call to use the index snapshot
(e.g. git.show([`:${input.path}`])) so the "unstaged" left-side reflects the
index, not HEAD, while keeping the existing try/catch behavior; update the
git.show call that sets originalContent (and leave the readFile for
modifiedContent and worktreePath/input.path as-is).

- Fix unstaged diff base: compare index (:0:) vs working tree, not HEAD
- Add console.warn for GitHub API errors in getPullRequestThreads
- Reset range modal selection when modal closes
@saddlepaddle saddlepaddle merged commit 1eddeb3 into main Apr 5, 2026
13 of 14 checks passed
MocA-Love pushed a commit to MocA-Love/superset that referenced this pull request Apr 7, 2026
…et-sh#3177)

* WIP

* feat(desktop): git changes sidebar with resource-oriented API

Backend:
- Git tRPC router with GitHub-aligned types (PatchStatus, PullRequestState,
  CheckStatusState, etc.) mirroring GitHub's GraphQL/REST schema
- Resource-oriented endpoints: getStatus, listBranches, listCommits,
  getCommitFiles, getDiff, getPullRequest, getPullRequestThreads
- Proper file status detection using git status.files index/working_dir pairs
- Branch rename mutation (unpushed branches only)
- Router organized into git.ts + types.ts + utils/ per repo guidelines

Frontend:
- Tab registry pattern: each sidebar tab is a hook returning SidebarTabDefinition
- ChangesTab with card-style header: branch name (editable), base branch
  selector, commit count, ahead/behind origin status, file stats
- Commit filter dropdown: All changes / Uncommitted / Select range / individual
  commits. Filter state persisted in workspace local state.
- Commit range selection modal using Dialog + ScrollArea
- File list grouped by folder with status indicators
- Polling-based refresh (3s for status/commits, 30s for branches)
- Only active tab mounted, inactive tabs unmount

* fix: address PR review feedback

- Fix unstaged diff base: compare index (:0:) vs working tree, not HEAD
- Add console.warn for GitHub API errors in getPullRequestThreads
- Reset range modal selection when modal closes
MocA-Love added a commit to MocA-Love/superset that referenced this pull request Apr 7, 2026
cherry-pick済み:
- e728ebd feat(desktop): wire up missing hotkeys for v2 workspace (superset-sh#3190)
- 1eddeb3 feat(desktop): git changes sidebar with resource-oriented API (superset-sh#3177)
- 11ed4f8 V2 terminal env (superset-sh#3184)
- 0c52ecc feat(desktop): pane context menus + binary tree layout (superset-sh#3196)
- 5578746 fix(desktop): resolve file icons from origin instead of href (superset-sh#3199)
- 5a1e5d1 feat(panes): prefer sibling pane when closing active pane (superset-sh#3198)
- d670c4a V2 top bar: right sidebar toggle, org dropdown in sidebar, unified open-in button (superset-sh#3197)
- 2573fa2 fix(desktop): remove macOS background-to-tray quit interception (superset-sh#3205)
- 4a29342 feat: Superset CLI + CLI framework + Better Auth 1.5.6 (superset-sh#3194)
- 700cd65 fix(desktop): revert broken file icon origin fix + bundle all icon sources (superset-sh#3218)

フォーク独自対応:
- cleanupMainWindowResources()をexit pathに移動維持 (#3205対応)
- BROWSER_HARD_RELOAD/SEARCH_IN_FILESをv2 workspaceに配線
- BROWSER_RELOAD/HARD_RELOADのuseHotkey配線修正(リマップ対応)
- ansi_up依存維持
@Kitenite Kitenite deleted the saddlepaddle/v2-diff branch May 6, 2026 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant