feat(chat): enable jump-to-file in regular chat diff accordions (#997)#1011
feat(chat): enable jump-to-file in regular chat diff accordions (#997)#1011TrawenCZ wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChat diff previews now provide a jump-to-file action for tool responses with paths. The callback normalizes paths for ChangesJump-to-file action
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webview-ui/src/components/chat/ChatRow.tsx (1)
423-429: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve an existing
./path prefix.This currently produces
././src/file.tswhentool.pathis already workspace-relative with a./prefix. MatchFileChangesPanel.tsxand normalize conditionally so theopenFilepayload remains canonical.Proposed fix
return () => vscode.postMessage({ type: "openFile", text: "./" + tool.path }) + return () => + vscode.postMessage({ + type: "openFile", + text: tool.path.startsWith("./") ? tool.path : "./" + tool.path, + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview-ui/src/components/chat/ChatRow.tsx` around lines 423 - 429, Update onJumpToFile in ChatRow.tsx to preserve an existing "./" prefix instead of always prepending another one. Normalize tool.path conditionally, matching FileChangesPanel.tsx, so the openFile message receives a canonical workspace-relative path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@webview-ui/src/components/chat/ChatRow.tsx`:
- Around line 423-429: Update onJumpToFile in ChatRow.tsx to preserve an
existing "./" prefix instead of always prepending another one. Normalize
tool.path conditionally, matching FileChangesPanel.tsx, so the openFile message
receives a canonical workspace-relative path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 96ba4f81-6396-46e1-89e1-2788d1f7977e
📒 Files selected for processing (2)
webview-ui/src/components/chat/ChatRow.tsxwebview-ui/src/components/chat/__tests__/ChatRow.diff-actions.spec.tsx
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
github-actions
Bot
added
the
awaiting-review
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Thanks! Looking forward to this feature, can you increase test coverage a bit.
| type: "openFile", | ||
| text: "./src/file.ts", | ||
| }) | ||
| }) |
There was a problem hiding this comment.
This covers appliedDiff — do editedExistingFile, searchAndReplace, and insertContent also need a click-asserts-openFile test? They share the same onJumpToFile code path in ChatRow.tsx (lines 500 and 539), so a future gate on tool type there would go uncaught.
| const { container } = renderChatRow(message) | ||
| const openFileIcon = container.querySelector(".codicon-link-external") as HTMLElement | null | ||
|
|
||
| expect(openFileIcon).toBeInTheDocument() |
There was a problem hiding this comment.
Worth adding a negative case — when path is omitted from the tool message, is the icon confirmed absent? ChatRow.tsx:424 returns undefined for onJumpToFile when tool.path is falsy, and CodeAccordion only renders the icon when both onJumpToFile and path are truthy — so the guard exists, but it is untested.
| }) | ||
| }) | ||
|
|
||
| it("shows jump-to-file affordance for appliedDiff", () => { |
There was a problem hiding this comment.
Is there a test anywhere that asserts the icon is absent for a non-file tool (e.g. executeCommand or listFilesTopLevel)? A render path that unconditionally injected .codicon-link-external would pass all current tests.
| const onJumpToCreatedFile = useMemo(() => { | ||
| if (!tool || tool.tool !== "newFileCreated" || !tool.path) { | ||
| const onJumpToFile = useMemo(() => { | ||
| if (!tool || !tool.path) { |
There was a problem hiding this comment.
The return statement below unconditionally prepends "./", while the sibling in FileChangesPanel.tsx:168 guards with path.startsWith("./") ? path : "./" + path to stay idempotent. Currently safe since getReadablePath always returns bare relative paths, but worth mirroring for consistency?
github-actions
Bot
added
awaiting-author
Sure, will update those. I just wanted to go with minimal changes (and not change the logic where not needed), but I agree we can improve this a bit as part of the PR. |
4e3270a to
b750b8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@webview-ui/src/components/chat/__tests__/ChatRow.diff-actions.spec.tsx`:
- Around line 192-200: Update the message fixture in “does not show jump-to-file
affordance for non-file tools” to include a valid path such as “src/file.ts”,
while keeping the non-file tool and existing assertion unchanged.
- Around line 144-179: Update the parameterized test around renderChatRow and
mockPostMessage to clear the shared mock immediately before clicking the
external-link icon, then assert the click produces exactly one call with the
expected openFile payload. Keep the existing icon lookup and message values
unchanged.
🪄 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 Plus
Run ID: 529f0ac0-1e66-4525-8f2c-f25b41f81449
📒 Files selected for processing (2)
webview-ui/src/components/chat/ChatRow.tsxwebview-ui/src/components/chat/__tests__/ChatRow.diff-actions.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- webview-ui/src/components/chat/ChatRow.tsx
b750b8c to
d3fde81
Compare
|
@edelauna the test coverage check is now failing because of that line for idempotency, and I'm not sure if that something you want me to test and if so, than how, eg write the case for each of the tools that use it? |
2 participants
Related GitHub Issue
Description
Previously the jump-to-file icon in regular chat diff accordions only appeared for
newFileCreatedtools. Edited files (appliedDiff,editedExistingFile,searchAndReplace, etc.) andinsertContentshared the sameCodeAccordionbut had no jump-to-file option.This PR generalizes the existing
onJumpToCreatedFilehandler inChatRow.tsxtoonJumpToFile, and wires it up for every file-edit and insert-content tool that renders aCodeAccordion.Test Procedure
webview-ui/src/components/chat/__tests__/ChatRow.diff-actions.spec.tsxasserting thatappliedDiffnow renders the external-link icon and postsopenFilewith the correct./prefixed path.Pre-Submission Checklist
Screenshots / Videos
Previous state
2026-07-24.21-51-33.mp4
New state
2026-07-24.21-52-48.mp4
Documentation Updates
Additional Notes
None.
Get in Touch
@trawen at Discord
Summary by CodeRabbit
Summary by CodeRabbit
./format.pathis missing or when the tool isn’t file-based.