Improve apply_patch diff UI and validation#47
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
Changesapply_patch staging and UI patch handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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 (1)
src/apply-patch.test.ts (1)
151-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the patched file content too.
This test can pass on diff shape alone; add a readback assertion so it also catches regressions where the emitted patch has two hunks but the file was not updated correctly.
🧪 Proposed test strengthening
assert.equal(splitHunkResult.patch.match(/^@@ /gm)?.length, 2); +assert.equal( + await readFile(join(splitHunkRoot, "long.txt"), "utf8"), + [ + "1", "two", "3", "4", "5", "6", "7", "8", "9", "10", + "11", "12", "13", "14", "15", "16", "17", "eighteen", "19", "20", + ].join("\n") + "\n", +);🤖 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 `@src/apply-patch.test.ts` at line 151, The apply-patch test currently only checks patch shape, so strengthen it by asserting the resulting file content after applying the patch as well. In src/apply-patch.test.ts, update the test around splitHunkResult so it reads back the patched file and verifies the expected content, using the existing applyPatch/splitHunkResult flow to ensure the patch emitted by the patching logic actually updates the file correctly.
🤖 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 `@src/apply-patch.ts`:
- Line 434: The diff rendering in apply-patch should preserve intentional
trailing spaces on the final line instead of removing them with trimEnd().
Update the logic around the trimEnd() call to strip only the trailing newline
from the generated diff, keeping any spaces that are part of the last diff line;
use the surrounding diff-building code in apply-patch to locate the exact spot.
---
Nitpick comments:
In `@src/apply-patch.test.ts`:
- Line 151: The apply-patch test currently only checks patch shape, so
strengthen it by asserting the resulting file content after applying the patch
as well. In src/apply-patch.test.ts, update the test around splitHunkResult so
it reads back the patched file and verifies the expected content, using the
existing applyPatch/splitHunkResult flow to ensure the patch emitted by the
patching logic actually updates the file correctly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 32c094a6-916c-4b71-ad3d-d3e84cd0b17b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
package.jsonsrc/apply-patch.test.tssrc/apply-patch.tssrc/server.tssrc/ui/card-types.test.tssrc/ui/card-types.tssrc/ui/workspace-app.tsx
|
@coderabbitai review it again |
|
✅ Action performedFull review finished. |
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 `@src/apply-patch.ts`:
- Around line 374-380: The final commit in applyPatch is still applied
incrementally, so a failure in writeTextFile or rm can leave a partial workspace
state. Update the commit path in applyPatch to make the write/delete phase
transactional or rollback-safe, using the staged map and the existing validation
flow to ensure all filesystem mutations either complete together or are reverted
on error; focus on the two loops that apply file.content and delete missing
files.
- Around line 362-365: The move handling in apply-patch.ts is using raw
path-string comparison to decide whether to stage the source as deleted, which
breaks case-only renames on case-insensitive filesystems. Update the logic
around the destination/absolute check in the patch application flow so `Foo.ts`
-> `foo.ts` is treated as the same filesystem entry and does not trigger a
delete of the newly written file. Use the existing move/staging symbols like
`readStagedOptional`, `staged.set`, and `unifiedFilePatch` to locate the rename
path and replace the string inequality with a filesystem-identity-aware check.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d175ebf-a573-42af-b94e-3fb46a345294
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
package.jsonsrc/apply-patch.test.tssrc/apply-patch.tssrc/server.tssrc/ui/card-types.test.tssrc/ui/card-types.tssrc/ui/patch-display.test.tssrc/ui/patch-display.tssrc/ui/workspace-app.tsx
|
@coderabbitai regarding the write/delete phase transactional rollback comment: we are intentionally not tackling that in this PR. This branch improves validation-time safety: patch actions are parsed, paths are resolved, hunks are applied, and final file states are prepared before any filesystem writes happen. That prevents validation/hunk failures from leaving earlier patch actions applied. Making the final filesystem mutation phase fully transactional or rollback-safe is a larger and different guarantee. Once writes/deletes begin, true all-or-nothing behavior across multiple files is difficult to guarantee portably, and a best-effort rollback path would add meaningful complexity while still being fallible under I/O errors, permission changes, crashes, cross-device behavior, etc. We also removed the tool-description wording that implied staged/atomic writes, so the implementation no longer over-promises this guarantee to model callers. For this PR, we fixed the concrete case-only move issue and are leaving full rollback-safe commits as a separate design decision. |
|
Tip For best results, initiate chat on the files or code changes.
I agree this PR’s guarantee is specifically validation-time safety before filesystem mutation, not a fully transactional/rollback-safe commit phase. Since the tool description was also updated to avoid implying stronger atomicity than the implementation provides, I’m withdrawing that rollback comment for (ᵔᴥᵔ) ✏️ Learnings added
|
Waishnav
changed the title
1 participant
Summary
Tests
Summary by CodeRabbit
apply_patchoutput with more accurate unified-diff formatting (headers, file modes) and consistent trailing-newline handling.