Uh oh!
There was an error while loading. Please reload this page.
feat: improve diff tool display and token efficiency - #1146
Merged
Conversation
- Truncate full commit SHAs to 7 characters for compact display - Use RepoBadge component for consistent repository rendering - Convert JSON output to git-diff format for better token efficiency Co-authored-by: Brendan Kellam <brendan@sourcebot.dev>
Contributor
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughConverts the diff tool output from JSON to git-unified diff text, enriches tool metadata with resolved repoInfo, truncates commit SHAs to 7 chars in the UI, and replaces inline repo text with a RepoBadge component. Adds a formatter utility and tests; throws if repo lookup fails. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client/UI
participant Component as Diff UI Component
participant Tool as get_diff Tool (Server)
participant Repo as Repo Service
participant DiffSvc as Diff Service
Component->>Tool: request diff(repo, base, head)
Tool->>Repo: getRepoInfoByName(repo)
Repo-->>Tool: repoInfo (or not found)
alt repo found
Tool->>DiffSvc: fetch structured diff
DiffSvc-->>Tool: getDiffResponseSchema JSON
Tool->>Tool: formatDiffAsGitDiff(response) -> git-unified diff
Tool-->>Component: { diffText, metadata: { repoInfo, base, head, ... } }
Component->>Component: render RepoBadge(repoInfo) and truncated SHAs
Component-->>User: display git-unified diff and badges
else repo not found
Tool-->>Component: throw "Repository \"<repo>\" not found."
Component-->>User: show error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Co-authored-by: Brendan Kellam <brendan@sourcebot.dev>
Co-authored-by: Brendan Kellam <brendan@sourcebot.dev>
brendan-kellam
marked this pull request as ready for review
April 23, 2026 18:26
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/web/src/features/tools/getDiff.ts (1)
71-74: Redundant null check.
getRepoInfoByNameis wrapped withsew()and returnsT | ServiceError— it never resolves tonull/undefined. The!repoInfoResultbranch is dead code;isServiceErroralone is sufficient.- if (isServiceError(repoInfoResult) || !repoInfoResult) {+ if (isServiceError(repoInfoResult)) { throw new Error(`Repository "${repo}" not found.`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/features/tools/getDiff.ts` around lines 71 - 74, The null/undefined guard after calling getRepoInfoByName is redundant because getRepoInfoByName (wrapped by sew) returns either a valid repo info or a ServiceError; remove the "!repoInfoResult" check and keep only the isServiceError(repoInfoResult) branch, throwing the same Error when isServiceError(...) is true so that the repository-not-found path is driven solely by isServiceError; update the repository retrieval block around getRepoInfoByName and repoInfoResult accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/features/tools/getDiff.test.ts`:
- Around line 86-91: The test expectations in getDiff.test.ts currently assert
non-standard paths like "+++ b//dev/null" and "--- a//dev/null"; update the
expected diff strings (the `expected` variables around the blocks at lines
shown) to use the corrected `/dev/null` form without the `a/` or `b/` prefix and
without the double slash (i.e., change "+++ b//dev/null" -> "+++ /dev/null" and
"--- a//dev/null" -> "--- /dev/null"), so the assertions match the fixed
formatter output from getDiff.ts.
- Around line 5-35: The test duplicates the implementation of
formatDiffAsGitDiff; instead export the canonical function from getDiff.ts and
import it in getDiff.test.ts so tests use the single source of truth. Remove the
local formatDiffAsGitDiff from packages/web/src/features/tools/getDiff.test.ts,
add an export of formatDiffAsGitDiff in
packages/web/src/features/tools/getDiff.ts (or ensure it is exported), then
update the test to import { formatDiffAsGitDiff } from the getDiff module and
use that imported function in assertions.
In `@packages/web/src/features/tools/getDiff.ts`:
- Around line 26-31: The diff headers currently produce `--- a//dev/null` and
`+++ b//dev/null` when oldPath/newPath are nullish; update the loop in getDiff
(the block that defines oldPath/newPath and appends the `---`/`+++` lines) so
that if file.oldPath or file.newPath is nullish you print `--- /dev/null` or
`+++ /dev/null` (no a/ or b/ prefix), otherwise continue to print `---
a/${oldPath}` and `+++ b/${newPath}`; adjust the variables used to build the two
header lines (the oldPath/newPath handling and the output += lines) accordingly.
- Around line 23-53: The helper function formatDiffAsGitDiff is not exported,
causing tests to copy-paste its body instead of using the real implementation;
export the function (retain its signature and GetDiffResult type) from
packages/web/src/features/tools/getDiff.ts so production code and tests share
the same implementation, then update the companion test (getDiff.test.ts) to
import formatDiffAsGitDiff from that module instead of duplicating the function
body.
---
Nitpick comments:
In `@packages/web/src/features/tools/getDiff.ts`:
- Around line 71-74: The null/undefined guard after calling getRepoInfoByName is
redundant because getRepoInfoByName (wrapped by sew) returns either a valid repo
info or a ServiceError; remove the "!repoInfoResult" check and keep only the
isServiceError(repoInfoResult) branch, throwing the same Error when
isServiceError(...) is true so that the repository-not-found path is driven
solely by isServiceError; update the repository retrieval block around
getRepoInfoByName and repoInfoResult accordingly.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a902810e-371d-43b9-9647-19f6c727aec5
📒 Files selected for processing (4)
CHANGELOG.mdpackages/web/src/features/chat/components/chatThread/tools/getDiffToolComponent.tsxpackages/web/src/features/tools/getDiff.test.tspackages/web/src/features/tools/getDiff.ts
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR implements the feedback from the Slack thread on the diff tool:
read_file,list_commits, etc.Changes
getDiff.tsto format the API response into git-diff format and fetch repo infogetDiffToolComponent.tsxto truncate SHAs and use RepoBadgeGetDiffRepoInfotype to metadata for repository informationSlack Thread
Summary by CodeRabbit
New Features
Improvements
Tests