fix diff parse error #52
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #51 by fixing a diff parse error and adding support for two new hunk expansion directions: "up-all" and "down-all". The parse error fix changes the condition from !line to line === null to properly handle empty lines that return "\n". The new expansion directions allow expanding all remaining hidden lines in a hunk at once, complementing the existing incremental "up" and "down" options.
- Fixed diff parse error by using strict null check instead of falsy check
- Added "up-all" and "down-all" expansion directions to DiffFile API
- Updated TypeScript definitions across all framework packages to reflect new API
Reviewed changes
Copilot reviewed 3 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/parse/diff-parse.ts | Fixed parse error by changing line check from falsy to strict null comparison |
| packages/core/src/diff-file.ts | Implemented "up-all" and "down-all" expansion logic for both split and unified views |
| packages/core/index.d.ts | Updated type definitions to include new expansion direction options |
| packages/react/index.d.ts | Updated type definitions to match core API changes |
| packages/vue/index.d.ts | Updated type definitions to match core API changes |
| packages/solid/index.d.ts | Updated type definitions to match core API changes |
| packages/file/index.d.ts | Updated type definitions to match core API changes |
| ui/react-example/package.json | Added --force flag to vite dev script for cache clearing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| current.splitInfo = { | ||
| ...current.splitInfo, | ||
| startHiddenIndex: current.splitInfo.endHiddenIndex, | ||
| }; |
There was a problem hiding this comment.
The "down-all" implementation is missing the plainText property update. Compare with the "down" case (lines 1194-1198) which updates plainText when !current.isLast. The plainText should be updated to reflect the new hunk header after expanding all lines downward, similar to how it's done in the "down" case.
| current.splitInfo = { | |
| ...current.splitInfo, | |
| startHiddenIndex: current.splitInfo.endHiddenIndex, | |
| }; | |
| if (current.isLast) { | |
| current.splitInfo = { | |
| ...current.splitInfo, | |
| startHiddenIndex: current.splitInfo.endHiddenIndex, | |
| }; | |
| } else { | |
| current.splitInfo = { | |
| ...current.splitInfo, | |
| startHiddenIndex: current.splitInfo.endHiddenIndex, | |
| plainText: `@@ -${current.splitInfo.oldStartIndex},${current.splitInfo.oldLength} +${current.splitInfo.newStartIndex},${current.splitInfo.newLength}`, | |
| }; | |
| } |
| } else if (dir === "up-all") { | ||
| if (current.isLast) { | ||
| if (__DEV__) { | ||
| console.error("the last hunk can not expand up!"); | ||
| } | ||
| return; | ||
| } | ||
| for (let i = current.splitInfo.startHiddenIndex; i < current.splitInfo.endHiddenIndex; i++) { | ||
| const leftLine = this.#splitLeftLines[i]; | ||
| const rightLine = this.#splitRightLines[i]; | ||
| if (leftLine?.isHidden) leftLine.isHidden = false; | ||
| if (rightLine?.isHidden) rightLine.isHidden = false; | ||
| } | ||
| current.splitInfo = { | ||
| ...current.splitInfo, | ||
| endHiddenIndex: current.splitInfo.startHiddenIndex, | ||
| }; | ||
|
|
||
| delete this.#splitHunksLines?.[index]; | ||
|
|
||
| this.#splitHunksLines![current.splitInfo.endHiddenIndex] = current; |
There was a problem hiding this comment.
The "up-all" implementation is missing critical hunk header metadata updates. Compare with the "up" case (lines 1224-1236) which updates oldStartIndex, oldLength, newStartIndex, newLength, and plainText properties. When expanding all lines upward, the hunk header information needs to be recalculated to reflect the full expansion, otherwise the diff display will show incorrect hunk boundaries.
| } | ||
| current.unifiedInfo = { | ||
| ...current.unifiedInfo, | ||
| startHiddenIndex: current.unifiedInfo.endHiddenIndex, |
There was a problem hiding this comment.
The "down-all" implementation is missing the plainText property update. Compare with the "down" case (lines 1325-1329) which updates plainText when !current.isLast. The plainText should be updated to reflect the new hunk header after expanding all lines downward.
| startHiddenIndex: current.unifiedInfo.endHiddenIndex, | |
| startHiddenIndex: current.unifiedInfo.endHiddenIndex, | |
| plainText: `@@ -${current.unifiedInfo.oldStartIndex},${current.unifiedInfo.oldLength} +${current.unifiedInfo.newStartIndex},${current.unifiedInfo.newLength}`, |
| } else if (dir === "up-all") { | ||
| if (current.isLast) { | ||
| if (__DEV__) { | ||
| console.error("the last hunk can not expand up!"); | ||
| } | ||
| return; | ||
| } | ||
| for (let i = current.unifiedInfo.startHiddenIndex; i < current.unifiedInfo.endHiddenIndex; i++) { | ||
| const unifiedLine = this.#unifiedLines[i]; | ||
| if (unifiedLine?.isHidden) unifiedLine.isHidden = false; | ||
| } | ||
| current.unifiedInfo = { | ||
| ...current.unifiedInfo, | ||
| endHiddenIndex: current.unifiedInfo.startHiddenIndex, | ||
| }; | ||
|
|
||
| delete this.#unifiedHunksLines?.[index]; | ||
|
|
||
| this.#unifiedHunksLines![current.unifiedInfo.endHiddenIndex] = current; |
There was a problem hiding this comment.
The "up-all" implementation is missing critical hunk header metadata updates. Compare with the "up" case (lines 1351-1363) which updates oldStartIndex, oldLength, newStartIndex, newLength, and plainText properties. When expanding all lines upward, the hunk header information needs to be recalculated to reflect the full expansion, otherwise the diff display will show incorrect hunk boundaries.
fix #51