feat: Add diffjump and difffold features #166
Conversation
Add ++diffjump[={commitish}] option to enable diff jump functionality on
buffers with git diff output. Add ++filetype option to allow custom
filetype specification. Auto-enable ++diffjump in show action.
Changes:
- Add ++diffjump option parsing in buffer command
- Add ++filetype option for custom filetype specification
- Store diffjump and filetype in bufname params
- Initialize diff jump when ++diffjump is present
- Set filetype based on ++filetype or default to gin-buffer
- Auto-enable ++diffjump in show action with commit-specific target
- Add ftplugin for gin-buffer filetype with diffjump mappings
- Add g:gin_buffer_disable_default_mappings variable
- Update documentation with ++diffjump and ++filetype usage
WalkthroughAdds a new diff-jump subsystem and diff-fold feature, integrates ++diffjump/++difffold/++filetype options into GinBuffer flows, replaces the legacy diff jump implementation with feat/diffjump modules and dispatcher, updates buffer wiring, parsers, tests, docs, and ftplugins accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Vim as Gin buffer
participant Denops
participant DiffJumpMain as feat/diffjump.main
participant DiffJump as feat/diffjump.jump
participant Parser as feat/diffjump.parser
participant Buf as buffer.open/execEdit
User->>Vim: trigger <Plug>(gin-diffjump-*)
Vim->>Denops: dispatch diffjump:(old|new|smart)
Denops->>DiffJumpMain: handler(namespace, mods)
DiffJumpMain->>DiffJump: call jumpOld/jumpNew/jumpSmart(commitish or map, mods)
DiffJump->>Vim: batch get cursor, buffer lines, bufname
DiffJump->>Parser: parse(lineIndex, content)
Parser-->>DiffJump: Jump target (old/new/both) or undefined
alt resolved
DiffJump->>Buf: open/edit target (WORKTREE | INDEX | commitish) at lnum
Buf-->>User: file opened at lnum
else undefined
DiffJump-->>User: no-op
end
sequenceDiagram
autonumber
participant Cmd as :GinBuffer (with ++diffjump/++difffold/++filetype)
participant Exec as buffer.command.exec
participant Edit as buffer.edit.exec
participant DJInit as feat/diffjump.init
participant DFInit as feat/difffold.init
Cmd->>Exec: ExecOptions{diffjump, difffold, filetype}
Exec->>Edit: open buffer (set bufname/filetype)
Edit->>DJInit: init(denops, bufnr, "buffer") if diffjump present
Edit->>DFInit: init(denops, bufnr) if difffold true
Edit-->>Cmd: buffer ready with mappings/folds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 5
🧹 Nitpick comments (4)
ftplugin/gin-buffer.vim (1)
5-9: Consider adding<silent>modifier to mappings.The mappings are correctly implemented using
nmapfor<Plug>targets and includezvfor fold navigation. However, consider adding the<silent>modifier to suppress command-line messages during execution.Apply this diff to add the
<silent>modifier:if !get(g:, 'gin_buffer_disable_default_mappings') - nmap <buffer> <CR> <Plug>(gin-diffjump-smart)zv - nmap <buffer> g<CR> <Plug>(gin-diffjump-old)zv - nmap <buffer> <C-g><CR> <Plug>(gin-diffjump-new)zv + nmap <buffer><silent> <CR> <Plug>(gin-diffjump-smart)zv + nmap <buffer><silent> g<CR> <Plug>(gin-diffjump-old)zv + nmap <buffer><silent> <C-g><CR> <Plug>(gin-diffjump-new)zv endifftplugin/gin-diff.vim (1)
6-6: LGTM! Consider adding configurability.Setting
foldlevel=1is a sensible default for diff files, ensuring top-level sections are visible while deeper folds remain closed. This enhances readability for the difffold feature.Consider allowing users to configure this value:
setlocal foldlevel=1 if exists('g:gin_diff_foldlevel') let &l:foldlevel = g:gin_diff_foldlevel endifThis would provide flexibility for users who prefer different folding behavior.
denops/gin/feat/difffold/parser_test.ts (1)
4-84: Consider adding test coverage for incomplete file sections.The existing test cases correctly validate the main scenarios. However, consider adding a test case for an incomplete file section (e.g., a
---line without a matching+++line) to verify the parser's behavior in such edge cases.Looking at the parser implementation, if a section has an
oldPathbut nonewPath, the non-null assertion (currentSection.newPath!) could cause a runtime error. A test for this scenario would help identify and document the expected behavior.Example test case to add:
Deno.test("parse() - incomplete file section", () => { const content = [ "--- a/file1.txt", "@@ -1,3 +1,4 @@", " line1", ]; const result = parse(content); // Assert expected behavior: empty array or error? assertEquals(result, []); });denops/gin/command/buffer/edit.ts (1)
57-57: Consider using a constant for the literal "buffer" string.The hardcoded string
"buffer"passed toinitDiffJumpmight benefit from being defined as a named constant or enum value for better maintainability and type safety.Example refactor:
// At the top of the file or in a shared constants file const DIFFJUMP_SOURCE_BUFFER = "buffer"; // Then use it in the call await initDiffJump(denops, bufnr, DIFFJUMP_SOURCE_BUFFER);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
denops/gin/action/show.ts(1 hunks)denops/gin/command/buffer/command.ts(2 hunks)denops/gin/command/buffer/edit.ts(4 hunks)denops/gin/command/buffer/main.ts(5 hunks)denops/gin/command/diff/commitish.ts(1 hunks)denops/gin/command/diff/commitish_test.ts(1 hunks)denops/gin/command/diff/edit.ts(2 hunks)denops/gin/command/diff/jump.ts(0 hunks)denops/gin/command/diff/jump_test.ts(0 hunks)denops/gin/command/diff/main.ts(3 hunks)denops/gin/feat/difffold/fold.ts(1 hunks)denops/gin/feat/difffold/parser.ts(1 hunks)denops/gin/feat/difffold/parser_test.ts(1 hunks)denops/gin/feat/diffjump/jump.ts(1 hunks)denops/gin/feat/diffjump/main.ts(1 hunks)denops/gin/feat/diffjump/parser.ts(1 hunks)denops/gin/feat/diffjump/parser_test.ts(1 hunks)doc/gin.txt(3 hunks)ftplugin/gin-buffer.vim(1 hunks)ftplugin/gin-diff.vim(1 hunks)
💤 Files with no reviewable changes (2)
- denops/gin/command/diff/jump.ts
- denops/gin/command/diff/jump_test.ts
🧰 Additional context used
🧬 Code graph analysis (6)
denops/gin/feat/diffjump/jump.ts (1)
denops/gin/feat/diffjump/parser.ts (2)
DiffLocation(4-7)parse(106-131)
denops/gin/feat/diffjump/parser_test.ts (1)
denops/gin/feat/diffjump/parser.ts (2)
Jump(12-15)parse(106-131)
denops/gin/feat/difffold/fold.ts (1)
denops/gin/feat/difffold/parser.ts (1)
parse(41-89)
denops/gin/feat/difffold/parser_test.ts (1)
denops/gin/feat/difffold/parser.ts (2)
parse(41-89)FileSection(4-13)
denops/gin/command/diff/main.ts (1)
denops/gin/command/diff/commitish.ts (1)
parseCommitish(17-31)
denops/gin/feat/diffjump/main.ts (1)
denops/gin/feat/diffjump/jump.ts (4)
CommitishMap(16-23)jumpOld(78-93)jumpNew(95-110)jumpSmart(112-129)
🪛 GitHub Actions: Test
denops/gin/feat/difffold/fold.ts
[error] 4-4: TS2552 Cannot find name 'denops'. Did you mean 'Deno'?
[error] 4-4: TS2304 Cannot find name 'bufnr'.
denops/gin/feat/difffold/parser_test.ts
[error] 2-2: deno fmt --check reported: Found 1 not formatted file in 124 files.
🪛 LanguageTool
doc/gin.txt
[grammar] ~679-~679: There might be a mistake here.
Context: ... g:gin_buffer_disable_default_mappings Disable default mappings on buffers show...
(QB_NEW_EN)
🔇 Additional comments (24)
ftplugin/gin-buffer.vim (2)
1-3: LGTM!The guard clause follows the standard Vim ftplugin pattern to prevent re-execution.
11-11: LGTM!The flag setting follows the standard Vim ftplugin pattern to mark the plugin as loaded.
denops/gin/command/buffer/command.ts (2)
14-16: LGTM! Clean interface extension.The three new optional fields (
diffjump,difffold,filetype) are well-typed and extend theExecOptionsinterface cleanly to support the new diff navigation features.
39-41: Implementation follows existing patterns correctly.The options are correctly propagated into the bufname params:
diffjumpis passed through as-is, allowing for optional commitish valuesdifffoldfollows the boolean flag pattern (convertstrueto"",false/undefinedtoundefined) consistent withmonochromeandemojifyfiletypeprovides a sensible default of"gin-buffer"Note: An empty string for
diffjumpwould be preserved in the URL params (e.g.,diffjump=). If this represents "enabled without a specific commitish" vs.undefinedmeaning "disabled", the current implementation is correct.If needed, verify that empty string is an intentional valid value for the
diffjumpparameter, or consider usingunnullishto filter empty strings:diffjump: unnullish(options.diffjump, (v) => v || undefined),denops/gin/command/diff/edit.ts (2)
17-18: LGTM! Clean imports for the new feature modules.The import statements correctly reference the new diffjump and difffold feature modules with clear naming.
91-96: LGTM! Good refactoring to modular initialization.The migration from inline key mappings to dedicated initializer functions improves code organization and maintainability. The initialization happens after buffer setup completes, which is the correct sequence.
The
"diff"parameter passed toinitDiffJumpappears to indicate the context/source type—verify this aligns with the feature's design, especially given the PR mentions "auto-enable in show action" which suggests different contexts may exist.ftplugin/gin-diff.vim (1)
8-12: Inconsistency between AI summary and code annotations.The AI summary states: "Adds a new key mapping: maps to (gin-diffjump-new)zv". However, line 11 is not marked with a
~, indicating it was not changed in this PR.The mappings themselves are well-designed:
- The
zvsuffix ensures the jumped-to location is visible after folding- The three variants (smart/old/new) provide flexible navigation options
- The guard allows users to disable default mappings
denops/gin/feat/difffold/parser_test.ts (1)
4-84: Well-structured test cases with correct assertions.The test cases are well-organized and correctly validate the parser's functionality. The expected line numbers (1-based indexing) and file paths align with the parser's implementation. The coverage of main scenarios (single file, multiple files, empty content, no headers) provides a solid foundation for validating the parser's behavior.
doc/gin.txt (3)
142-168: Clear and thorough explanation of the new GinBuffer options.Examples and cross-references make it easy to understand how to enable diff jumping and folding. Nicely documented.
679-684: Helpful global toggle documentation.This makes the new default mappings control easy to discover for users who rely on custom keymaps.
842-853: Good clarification of mapping availability.Stating the diffjump prerequisite here keeps the mapping section accurate after the new feature split.
denops/gin/feat/difffold/parser.ts (2)
1-13: LGTM!The FileSection type definition is clear, well-documented, and appropriate for representing diff file sections.
15-16: LGTM!The regex patterns correctly match unified diff file headers and handle optional tab-separated metadata.
denops/gin/feat/difffold/fold.ts (1)
22-42: Well-structured fold initialization.The implementation correctly:
- Reads buffer content and parses it into file sections
- Exits early when no sections are found
- Uses batch operations to efficiently create folds
- Sets foldmethod to manual before creating folds
- Uses the correct Vim fold command syntax
The batched denops parameter on line 30 appropriately shadows the outer denops—this is the standard pattern for batch operations.
denops/gin/action/show.ts (1)
63-64: LGTM — ++diffjump and ++difffold are correctly supported The dispatcher validates both options andedit.tsinitializes diffjump and difffold as intended.denops/gin/command/buffer/edit.ts (6)
19-20: LGTM!The imports for diff jump and diff fold initialization are correctly added to support the new optional features.
47-50: LGTM!The filetype option is properly extracted from params using
unnullishwith appropriate type validation.
60-63: LGTM!The difffold initialization correctly checks for the presence of the
difffoldflag in params and callsinitDiffFoldappropriately.
72-72: LGTM!The addition of the optional
filetypeproperty toExecOptionsis consistent with its usage throughout the file.
131-133: LGTM!The conditional setting of filetype only when explicitly provided is the correct behavior, allowing the default filetype to be preserved when not specified.
54-58:initDiffJumpdoesn’t accept a commitish The function signature isinit(denops, bufnr, namespace)with no options parameter, sojumpCommitishis only used to gate initialization—not forwarded.Likely an incorrect or invalid review comment.
denops/gin/command/buffer/main.ts (3)
3-7: Imports look correct and scoped.New dependencies align with added features; versions are consistent with existing std modules.
Also applies to: 19-19
100-103: Opts wiring LGTM; confirm exec expects these types.Pass-through for diffjump/difffold/filetype looks correct; difffold defaults to true. Verify exec accepts diffjump as string|boolean and difffold as boolean.
Also applies to: 111-114
53-84: Replace ensure-based commitish resolution to guard bare flags
parseBufnamesets a bare++diffjumptotrue, soensure(jumpCommitish || "HEAD", is.String)will throw; use(and similarly forconst commitish = is.String(jumpCommitish) && jumpCommitish.length > 0 ? jumpCommitish : "HEAD"; return `${commitish}^`;new, omitting the caret).- Extract this logic into a small helper to avoid duplication.
- Verify behavior when
HEAD^doesn’t exist (initial commit) and handle fallback if needed.
| @@ -0,0 +1,84 @@ | |||
| import { assertEquals } from "jsr:@std/assert@^1.0.0"; | |||
| import { parse, type FileSection } from "./parser.ts"; | |||
There was a problem hiding this comment.
Fix the formatting issue.
The pipeline failure indicates that deno fmt --check found this file is not properly formatted. Run deno fmt to auto-format the file.
#!/bin/bash
# Format the file
deno fmt denops/gin/feat/difffold/parser_test.ts🧰 Tools
🪛 GitHub Actions: Test
[error] 2-2: deno fmt --check reported: Found 1 not formatted file in 124 files.
🤖 Prompt for AI Agents
In denops/gin/feat/difffold/parser_test.ts around line 2 the file fails deno fmt
--check; run the formatter to fix whitespace/ordering issues by executing `deno
fmt denops/gin/feat/difffold/parser_test.ts` (or `deno fmt` for the repo) and
commit the updated file so the import line and surrounding formatting match
Deno's style.
| if (currentSection?.start !== undefined) { | ||
| sections.push({ | ||
| start: currentSection.start, | ||
| end: i, // Previous line (0-based i = 1-based i) | ||
| oldPath: currentSection.oldPath!, | ||
| newPath: currentSection.newPath!, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Non-null assertions without validation could cause runtime errors.
The code uses non-null assertions (!) on oldPath and newPath when pushing sections, but doesn't validate that newPath was actually set. If the diff is malformed (e.g., a --- line without a following +++ line), newPath will be undefined, resulting in a section with an undefined newPath field.
Apply this diff to add validation:
// Start of a new file section
const oldMatch = line.match(patternOld);
if (oldMatch) {
// Save previous section if exists
- if (currentSection?.start !== undefined) {
+ if (
+ currentSection?.start !== undefined &&
+ currentSection.oldPath !== undefined &&
+ currentSection.newPath !== undefined
+ ) {
sections.push({
start: currentSection.start,
end: i, // Previous line (0-based i = 1-based i)
- oldPath: currentSection.oldPath!,
- newPath: currentSection.newPath!,
+ oldPath: currentSection.oldPath,
+ newPath: currentSection.newPath,
});
}
// Start new section
currentSection = {
start: lnum,
oldPath: oldMatch[1],
};
continue;
}
// New file path (should immediately follow old file path)
const newMatch = line.match(patternNew);
if (newMatch && currentSection) {
currentSection.newPath = newMatch[1];
continue;
}
}
// Save the last section
- if (currentSection?.start !== undefined) {
+ if (
+ currentSection?.start !== undefined &&
+ currentSection.oldPath !== undefined &&
+ currentSection.newPath !== undefined
+ ) {
sections.push({
start: currentSection.start,
end: content.length,
- oldPath: currentSection.oldPath!,
- newPath: currentSection.newPath!,
+ oldPath: currentSection.oldPath,
+ newPath: currentSection.newPath,
});
}Also applies to: 79-86
🤖 Prompt for AI Agents
In denops/gin/feat/difffold/parser.ts around lines 53-60 (and similarly 79-86),
the code pushes a section using non-null assertions on currentSection.oldPath
and currentSection.newPath which can be undefined for malformed diffs; change
this to validate that both oldPath and newPath are defined before pushing (e.g.,
check currentSection && currentSection.oldPath !== undefined &&
currentSection.newPath !== undefined), and if newPath is missing either skip
emitting that section or handle the error case explicitly (log/throw) instead of
using the `!` operator; update both occurrences accordingly to avoid runtime
errors.
| if (currentSection?.start !== undefined) { | ||
| sections.push({ | ||
| start: currentSection.start, | ||
| end: i, // Previous line (0-based i = 1-based i) |
There was a problem hiding this comment.
Clarify confusing comment.
The comment "Previous line (0-based i = 1-based i)" is unclear. Consider revising to better explain why end: i correctly represents the 1-based line number of the previous line.
Apply this diff:
sections.push({
start: currentSection.start,
- end: i, // Previous line (0-based i = 1-based i)
+ end: i, // i is 0-based, so i equals the 1-based line number of the previous line
oldPath: currentSection.oldPath,
newPath: currentSection.newPath,
});🤖 Prompt for AI Agents
In denops/gin/feat/difffold/parser.ts around line 56, the comment "Previous line
(0-based i = 1-based i)" is confusing; replace it with a clear explanation that
end uses a 1-based line number and why i is already the correct 1-based value
(e.g., "end: i, // previous line number (1-based): i already reflects the
1-based index used by the rest of the parser/consumer"), making explicit how i
is derived so future readers understand the indexing assumption.
| export function parse(index: number, content: string[]): Jump | undefined { | ||
| const line = content[index]; | ||
|
|
||
| // Cannot jump from special lines (headers) | ||
| if (patternSpc.test(line)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| if (line.startsWith("-")) { | ||
| // Deleted line: only exists in old file | ||
| const oldTarget = parseTarget(index, content, "old"); | ||
| return oldTarget ? { type: "old", old: oldTarget } : undefined; | ||
| } else if (line.startsWith("+")) { | ||
| // Added line: only exists in new file | ||
| const newTarget = parseTarget(index, content, "new"); | ||
| return newTarget ? { type: "new", new: newTarget } : undefined; | ||
| } else { | ||
| // Context line: exists in both files | ||
| const oldTarget = parseTarget(index, content, "old"); | ||
| const newTarget = parseTarget(index, content, "new"); | ||
| if (oldTarget && newTarget) { | ||
| return { type: "both", old: oldTarget, new: newTarget }; | ||
| } | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
Avoid crashing on diff metadata lines.
Line 109 currently lets any line that isn’t @@, ---, or +++ fall through to the “context” branch. When the cursor sits on metadata such as diff --git …, index …, or the \ No newline at end of file marker, parseTarget walks upward, fails to find a hunk header, and throws No range pattern found in diff content. That exception bubbles up and breaks every <Plug>(gin-diffjump-*) mapping on those lines. Please bail out early for anything that isn’t an actual hunk line (leading space, -, or +) before calling parseTarget.
- const line = content[index];
+ const line = content[index] ?? "";
// Cannot jump from special lines (headers)
if (patternSpc.test(line)) {
return undefined;
}
- if (line.startsWith("-")) {
+ const head = line[0];
+ if (head !== "-" && head !== "+" && head !== " ") {
+ return undefined;
+ }
+
+ if (head === "-") {
// Deleted line: only exists in old file
const oldTarget = parseTarget(index, content, "old");
return oldTarget ? { type: "old", old: oldTarget } : undefined;
- } else if (line.startsWith("+")) {
+ } else if (head === "+") {
// Added line: only exists in new file
const newTarget = parseTarget(index, content, "new");
return newTarget ? { type: "new", new: newTarget } : undefined;
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function parse(index: number, content: string[]): Jump | undefined { | |
| const line = content[index]; | |
| // Cannot jump from special lines (headers) | |
| if (patternSpc.test(line)) { | |
| return undefined; | |
| } | |
| if (line.startsWith("-")) { | |
| // Deleted line: only exists in old file | |
| const oldTarget = parseTarget(index, content, "old"); | |
| return oldTarget ? { type: "old", old: oldTarget } : undefined; | |
| } else if (line.startsWith("+")) { | |
| // Added line: only exists in new file | |
| const newTarget = parseTarget(index, content, "new"); | |
| return newTarget ? { type: "new", new: newTarget } : undefined; | |
| } else { | |
| // Context line: exists in both files | |
| const oldTarget = parseTarget(index, content, "old"); | |
| const newTarget = parseTarget(index, content, "new"); | |
| if (oldTarget && newTarget) { | |
| return { type: "both", old: oldTarget, new: newTarget }; | |
| } | |
| return undefined; | |
| } | |
| export function parse(index: number, content: string[]): Jump | undefined { | |
| const line = content[index] ?? ""; | |
| // Cannot jump from special lines (headers) | |
| if (patternSpc.test(line)) { | |
| return undefined; | |
| } | |
| const head = line[0]; | |
| if (head !== "-" && head !== "+" && head !== " ") { | |
| return undefined; | |
| } | |
| if (head === "-") { | |
| // Deleted line: only exists in old file | |
| const oldTarget = parseTarget(index, content, "old"); | |
| return oldTarget ? { type: "old", old: oldTarget } : undefined; | |
| } else if (head === "+") { | |
| // Added line: only exists in new file | |
| const newTarget = parseTarget(index, content, "new"); | |
| return newTarget ? { type: "new", new: newTarget } : undefined; | |
| } else { | |
| // Context line: exists in both files | |
| const oldTarget = parseTarget(index, content, "old"); | |
| const newTarget = parseTarget(index, content, "new"); | |
| if (oldTarget && newTarget) { | |
| return { type: "both", old: oldTarget, new: newTarget }; | |
| } | |
| return undefined; | |
| } | |
| } |
🤖 Prompt for AI Agents
In denops/gin/feat/diffjump/parser.ts around lines 106 to 130, add an early
bail-out for any line that is not an actual hunk line (leading space, '-' or
'+') before calling parseTarget: after the existing patternSpc check, detect the
first character of the line and if it is not ' ', '-' or '+' return undefined
immediately so metadata lines like "diff --git …", "index …", or "\ No newline
at end of file" don't fall through and cause parseTarget to throw; keep the
existing handling for '-' and '+' and only treat other lines as context when the
first char is a space.
Add ++difffold option to enable automatic folding for file sections in unified diff output. Auto-enable in show action for better navigation of multi-file diffs. Changes: - Add ++difffold option parsing in buffer command - Initialize difffold when ++difffold is present - Auto-enable ++difffold in show action - Update documentation with ++difffold usage examples
lambdalisue
force-pushed
the
global-diff-jump
branch
from
08f7148 to
2e75c2a
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #166 +/- ##
==========================================
- Coverage 65.07% 61.59% -3.48%
==========================================
Files 19 21 +2
Lines 1065 1156 +91
Branches 163 163
==========================================
+ Hits 693 712 +19
- Misses 371 443 +72
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
denops/gin/feat/difffold/parser_test.ts (1)
74-84: Consider adding edge case tests for improved robustness.While the current tests cover the main scenarios well, consider adding tests for edge cases that could reveal parser robustness issues:
- Incomplete file header: Test content with only
---line without matching+++line- Multiple
+++lines: Test behavior when multiple+++lines follow a single---line- Empty file section: Test a file section with headers but no hunk content between them
These tests would help ensure the parser handles malformed or unusual diff formats gracefully, especially since the parser implementation uses non-null assertions (
oldPath!,newPath!) that could fail with incomplete headers.Example test for incomplete header:
Deno.test("parse() - incomplete file header", () => { const content = [ "--- a/file1.txt", "@@ -1,3 +1,4 @@", " line1", ]; const result = parse(content); // Should handle gracefully - either skip incomplete section or throw clear error // Current implementation may fail due to newPath! assertion });ftplugin/gin-diff.vim (1)
6-7: Consider gating foldlevel change behind difffold or a toggleHard-setting foldlevel=1 alters user fold view even when no folds exist. Consider applying only when difffold is active or behind a variable (e.g., g:gin_diff_foldlevel) to avoid overriding user preferences.
doc/gin.txt (1)
142-168: Clarify commitish defaults and merge/root edge casesDocs read well. One addition: note that for merge commits, the first parent is used (commitish^1), and root commits have no parent (old side is effectively empty). This sets user expectations for jump targets from git show/log -p.
denops/gin/command/buffer/main.ts (1)
53-85: Nit: improve error message and deduplicate bufname parsing
- Message “jump must be string” → “‘diffjump’ must be string”.
- Extract common bufname/param parsing into a helper to avoid duplication.
Apply this minimal tweak for the message:
- { message: "jump must be string" }, + { message: "'diffjump' must be string" },Optional helper sketch:
async function getJumpCommitish(denops: Denops, bufnr: number): Promise<string | undefined> { const bufname = await fn.bufname(denops, bufnr); const { params } = parseBufname(bufname); const v = params?.diffjump; return v === undefined ? undefined : ensure(v || "HEAD", is.String, { message: "'diffjump' must be string" }); }Use it in both old/new resolvers to reduce duplication.
denops/gin/command/buffer/edit.ts (1)
54-63: Init order OK; consider parallelizing initializersInitialization happens after content is populated, which is correct. You can run diffjump and difffold initializers in parallel to cut latency:
Example:
const inits: Promise<unknown>[] = []; if (jumpCommitish !== undefined) inits.push(initDiffJump(denops, bufnr, "buffer")); if ("difffold" in (params ?? {})) inits.push(initDiffFold(denops, bufnr)); await Promise.all(inits);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
denops/gin/action/show.ts(1 hunks)denops/gin/command/buffer/command.ts(2 hunks)denops/gin/command/buffer/edit.ts(4 hunks)denops/gin/command/buffer/main.ts(5 hunks)denops/gin/command/diff/edit.ts(2 hunks)denops/gin/feat/difffold/fold.ts(1 hunks)denops/gin/feat/difffold/parser.ts(1 hunks)denops/gin/feat/difffold/parser_test.ts(1 hunks)doc/gin.txt(3 hunks)ftplugin/gin-diff.vim(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- denops/gin/feat/difffold/parser.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- denops/gin/command/diff/edit.ts
- denops/gin/feat/difffold/fold.ts
- denops/gin/command/buffer/command.ts
- denops/gin/action/show.ts
🧰 Additional context used
🧬 Code graph analysis (1)
denops/gin/feat/difffold/parser_test.ts (1)
denops/gin/feat/difffold/parser.ts (2)
parse(41-89)FileSection(4-13)
🪛 LanguageTool
doc/gin.txt
[grammar] ~679-~679: There might be a mistake here.
Context: ... g:gin_buffer_disable_default_mappings Disable default mappings on buffers show...
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (macos-latest, 2.x, v9.1.0448, v0.10.0)
🔇 Additional comments (6)
denops/gin/feat/difffold/parser_test.ts (1)
4-84: LGTM! Test suite provides solid coverage of core functionality.The test cases correctly validate:
- Single file diff parsing with proper boundaries
- Multiple file diff parsing with correct section boundaries
- Empty content handling
- Missing file headers handling
All test expectations align with the parser implementation and follow Deno testing conventions.
doc/gin.txt (2)
842-853: Diffjump mapping availability docs LGTMThe availability notes for Plug mappings are clear and consistent with ++diffjump behavior.
679-684: Grammar fix: “buffers shown”Change “buffers show” → “buffers shown”.
Apply this diff:
-*g:gin_buffer_disable_default_mappings* - Disable default mappings on buffers show by |:GinBuffer| with - |++diffjump| option (filetype "gin-buffer"). +*g:gin_buffer_disable_default_mappings* + Disable default mappings on buffers shown by |:GinBuffer| with + |++diffjump| option (filetype "gin-buffer").Likely an incorrect or invalid review comment.
denops/gin/command/buffer/main.ts (1)
100-114: Option threading LGTMNew options (diffjump, difffold, filetype) are validated and passed through consistently.
Please confirm command/buffer/command.ts encodes these into the bufname params so edit() can read them (especially filetype default “gin-buffer”).
denops/gin/command/buffer/edit.ts (2)
47-51: Filetype threading LGTMOptional: if “gin-buffer” is the default, ensure command/buffer/command.ts always supplies it so this path sets the ftplugin reliably.
130-134: Conditional filetype set is safeSetting 'filetype' only when provided avoids clobbering user/local settings. LGTM.
| mainDiffJump(denops, "buffer", { | ||
| commitishMap: { | ||
| old: async ({ bufnr }) => { | ||
| const bufname = await fn.bufname(denops, bufnr); | ||
| const { params } = parseBufname(bufname); | ||
| const jumpCommitish = params?.diffjump; | ||
| if (jumpCommitish === undefined) { | ||
| return "HEAD^"; | ||
| } | ||
| const commitish = ensure( | ||
| jumpCommitish || "HEAD", | ||
| is.String, | ||
| { message: "jump must be string" }, | ||
| ); | ||
| return `${commitish}^`; | ||
| }, | ||
| new: async ({ bufnr }) => { | ||
| const bufname = await fn.bufname(denops, bufnr); | ||
| const { params } = parseBufname(bufname); | ||
| const jumpCommitish = params?.diffjump; | ||
| if (jumpCommitish === undefined) { | ||
| return "HEAD"; | ||
| } | ||
| const commitish = ensure( | ||
| jumpCommitish || "HEAD", | ||
| is.String, | ||
| { message: "jump must be string" }, | ||
| ); | ||
| return commitish; | ||
| }, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Handle root commits (no parent) in commitishMap
old→${commitish}^ (or "HEAD^") fails on a root commit. Ensure jump gracefully handles no-parent commits (e.g., treat old side as empty) or guard with a parent check.
You can verify if root commits are handled in the diffjump layer:
If not present, consider guarding here or in feat/diffjump to avoid resolving ${commitish}^ when parent count is zero.
🏁 Script executed:
#!/bin/bash
# Look for explicit handling of root/no-parent commits in diffjump implementation
rg -n -C3 -S -g '!**/test/**' --type=ts \
'(no parent|root commit|^.*parent.*\bcount\b|HEAD\^|[^A-Za-z]zero-parents)' denopsLength of output: 888
Guard against root commits in commitishMap
Appending ^ to a root commit (e.g. HEAD^ or ${commitish}^) will error—detect zero‐parent commits (e.g. via git rev-list --parents) or catch failures and return an empty old revision. Apply this guard in both:
- denops/gin/command/buffer/main.ts (commitishMap old handler)
- denops/gin/feat/diffjump/main.ts (default old = "HEAD^")
Summary
Add two new independent features for improved diff navigation:
Changes
feat/diffjump Module
feat/difffold Module
GinBuffer Enhancements
Refactoring
Test Plan
Files Changed
21 files changed, 898 insertions(+), 295 deletions(-)
Key additions:
feat/diffjump/- Independent diff jump modulefeat/difffold/- Independent diff fold moduleftplugin/gin-buffer.vim- Buffer-specific mappingsSummary by CodeRabbit
New Features
Documentation