feat: enable preview diff when using tools like StrReplace and Write #289
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces a new feature that enhances the approval workflow for file editing and writing operations by displaying a preview of proposed file changes (diffs) before users approve or reject changes. Users can now expand and collapse diff previews using the 'd' key, improving transparency and safety when approving file modifications.
Key Changes
- New diff utility module with functions to generate, format, and colorize unified diffs
- Enhanced approval request structure to carry diff information
- Interactive UI for expanding/collapsing diff previews with keyboard toggle
- Integration of diff generation in WriteFile and StrReplaceFile tools
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/kimi_cli/utils/diff.py | New utility module providing diff generation, formatting, and colorization functionality with truncation support |
| src/kimi_cli/wire/message.py | Enhanced ApprovalRequest class to include optional FileDiff parameter and updated serialization |
| src/kimi_cli/soul/approval.py | Updated approval request method signature to accept optional diff parameter |
| src/kimi_cli/ui/shell/visualize.py | Added diff preview rendering in approval panel with expandable/collapsible UI |
| src/kimi_cli/ui/shell/keyboard.py | Added LETTER_D key event for toggling diff visibility in both Unix and Windows implementations |
| src/kimi_cli/tools/file/write.py | Integrated diff generation before requesting approval for file write operations |
| src/kimi_cli/tools/file/replace.py | Refactored workflow to generate diff after applying edits and before requesting approval |
Comments suppressed due to low confidence (1)
src/kimi_cli/soul/approval.py:31
- The docstring is missing documentation for the new
diffparameter. Consider adding: "diff (FileDiff | None): Optional file diff to display in the approval UI."
"""
Request approval for the given action. Intended to be called by tools.
Args:
sender (str): The name of the sender.
action (str): The action to request approval for.
This is used to identify the action for auto-approval.
description (str): The description of the action. This is used to display to the user.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "sender": request.sender, | ||
| "action": request.action, | ||
| "description": request.description, | ||
| "diff": request.diff, |
There was a problem hiding this comment.
The diff field is a FileDiff NamedTuple which is not JSON-serializable by default. This will cause serialization errors when the approval request is sent over the wire. Consider converting the FileDiff to a dictionary using diff._asdict() if it needs to be serialized, or handling None appropriately.
| "diff": request.diff, | |
| "diff": request.diff._asdict() if request.diff is not None else None, |
| emit(KeyEvent.ENTER) | ||
| elif c == b"\t": | ||
| emit(KeyEvent.TAB) | ||
| elif c == b"d": |
There was a problem hiding this comment.
The keyboard listener only captures lowercase 'd' (b"d"). If a user presses uppercase 'D' (b"D"), the diff toggle won't work. Consider handling both cases: elif c in (b"d", b"D"):
| elif c == b"d": | |
| elif c in (b"d", b"D"): |
|
|
||
| # Add expandable diff section | ||
| if self.show_diff: | ||
| lines.append(Text.from_markup(diff_text)) |
There was a problem hiding this comment.
Using Text.from_markup() on diff content that may contain special Rich markup characters (like [, ]) could cause rendering issues or exceptions. The diff text returned from format_diff_for_display() already includes markup tags, but user file content within those diffs should be escaped. Consider escaping user content in the colorize_diff() function before adding markup tags, or use Text.from_ansi() if the content might contain special characters.
| lines.append(Text.from_markup(diff_text)) | |
| lines.append(Text.from_ansi(diff_text)) |
| if original_content is None: | ||
| try: | ||
| with open(path, encoding="utf-8") as f: | ||
| original_content = f.read() |
There was a problem hiding this comment.
The function uses synchronous file I/O (open()) which can block the event loop, while the calling code in write.py and replace.py uses async I/O (aiofiles.open()). Consider making this function async and using aiofiles.open() for consistency and to avoid blocking the event loop when reading large files.
| # Generate unified diff (already colorized) | ||
| diff_text = generate_text_diff(diff.before_content, diff.after_content) | ||
|
|
||
| # Truncate if too long |
There was a problem hiding this comment.
[nitpick] This comment appears to be incomplete or unnecessary. Consider removing it or completing the intended comment.
| # Truncate if too long |
| path: Path to the file | ||
| new_content: The new content that would be written | ||
| is_new_file: Whether this is a new file creation | ||
| original_content: The original content (if None, will try to read from file) |
There was a problem hiding this comment.
The append parameter is missing from the docstring. Consider adding a line like "append: Whether the new_content is being appended to existing content" to the Args section.
| original_content: The original content (if None, will try to read from file) | |
| original_content: The original content (if None, will try to read from file) | |
| append: Whether the new_content is being appended to existing content |
| elif c == b"d": | ||
| emit(KeyEvent.LETTER_D) |
There was a problem hiding this comment.
The keyboard listener only captures lowercase 'd' (b"d"). If a user presses uppercase 'D' (b"D"), the diff toggle won't work. Consider handling both cases: elif c in (b"d", b"D"):
| if line.startswith("+") and not line.startswith("+++"): | ||
| # Addition line - green | ||
| colored_lines.append(f"[green]{line}[/green]") | ||
| elif line.startswith("-") and not line.startswith("---"): | ||
| # Deletion line - red | ||
| colored_lines.append(f"[red]{line}[/red]") | ||
| elif line.startswith("@@"): | ||
| # Hunk header - cyan | ||
| colored_lines.append(f"[cyan]{line}[/cyan]") | ||
| elif line.startswith("---") or line.startswith("+++"): | ||
| # File headers - bold | ||
| colored_lines.append(f"[bold]{line}[/bold]") | ||
| else: | ||
| # Context line - no color | ||
| colored_lines.append(line) |
There was a problem hiding this comment.
The diff content is not escaped before adding Rich markup tags. If the file content contains special Rich markup characters like [ or ], it could break the rendering or be interpreted as markup. Consider using from rich.markup import escape and escaping the line content before wrapping it in markup tags: colored_lines.append(f"[green]{escape(line)}[/green]")
| async def request( | ||
| self, sender: str, action: str, description: str, diff: FileDiff | None = None | ||
| ) -> bool: |
There was a problem hiding this comment.
Mixing implicit and explicit returns may indicate an error, as implicit returns always return None.
Signed-off-by: tang donghai <[email protected]>
|
FYI: seems already have a patch in pull request list #165 |
Oh! I hadn't noticed that PR. I'll close it now |
2 participants
Related Issue
Resolve #248
Description
This pull request introduces a new feature that enhances the approval workflow for file editing and writing actions by displaying a preview of proposed file changes (diffs) in the approval UI. Users can now review and expand/collapse diffs before approving or rejecting changes, improving transparency and safety. The implementation includes a new diff utility module, updates to the approval request structure, and UI enhancements for interactive diff viewing.