Skip to content

Conversation

@lambdalisue
Copy link
Owner

@lambdalisue lambdalisue commented Oct 12, 2025

Summary

Adds a new :GinBlame command that provides an interactive 3-window git blame interface with commit navigation and history tracking.

Features

3-Window Layout

  • Top (10 lines): Commit details with full message and metadata
  • Bottom-left (60 cols): Navigation pane showing line numbers and commit info
  • Bottom-right: File content with syntax highlighting and commit dividers

Interactive Navigation

  • <CR>: Jump to commit under cursor
    • If already viewing that commit, navigates to parent commit
    • Automatically tracks file renames using git blame's "previous" information
  • <C-O>/<C-I>: Browser-style history navigation
    • Restores exact cursor position from previous views
    • Supports unlimited navigation depth

Smart Synchronization

  • scrollbind and cursorbind: All windows stay in sync
  • Moving cursor in any window updates commit details automatically
  • Compound filetype (e.g., typescript.gin-blame) preserves syntax highlighting

Customization

  • Plug mappings: <Plug>(gin-blame-switch-commit), <Plug>(gin-blame-navigate-older), <Plug>(gin-blame-navigate-newer)
  • Disable defaults: let g:gin_blame_disable_default_mappings = 1
  • ++emojify: Convert :emoji-name: in commit messages

Implementation Highlights

  • Modular architecture with 7 TypeScript modules
  • Efficient data structures: lineMap and fileLineToPhysical for O(1) lookups
  • Handles edge cases: file renames, missing files in old commits, buffer lifecycle
  • Comprehensive documentation in doc/gin.txt

Test Plan

  • Basic blame view opens correctly
  • Cursor synchronization across all windows
  • Enter navigation with file rename tracking
  • Ctrl-O/Ctrl-I history navigation
  • Commit detail updates on cursor move
  • Compound filetype with syntax highlighting
  • All windows close together
  • Plug mappings work correctly

Summary by CodeRabbit

  • New Features
    • GinBlame: interactive three‑pane blame UI with synchronized scrolling, commit detail pane, history navigation, GinBlame command, default/configurable mappings, optional emoji summaries, syntax highlighting, and divider signs.
  • Documentation
    • Added user docs for GinBlame and a contributor guideline mandating English code comments with examples.
  • Tests
    • Added parser unit tests and benchmarks, plus tests for human‑readable relative time formatting.

lambdalisue and others added 4 commits October 12, 2025 14:07
Implement a robust parser for git blame --porcelain output that:
- Parses commit metadata and line-by-line blame information
- Handles multi-line commits with proper group size tracking
- Supports boundary commits and previous commit information
- Provides type-safe interfaces for all data structures
- Achieves excellent performance (50K lines in ~18ms)

The parser is designed to be the foundation for the upcoming
GinBlame command implementation.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Add relativeTime function to format dates as human-readable relative time strings
(e.g., "2 hours ago", "in 3 days"). Supports past and future dates with accurate
calendar-based calculations for months and years.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@coderabbitai
Copy link

coderabbitai bot commented Oct 12, 2025

Walkthrough

Adds a complete gin-blame feature: Denops dispatcher and commands, git-blame porcelain parser with tests/benchmarks, three-buffer blame UI (file/nav/detail) with history and detail updates, Vim plugin/ftplugins/syntax/after-ftplugin, relative-time utility, and documentation; integrates blame into Denops initialization.

Changes

Cohort / File(s) Summary
Documentation
CLAUDE.md, doc/gin.txt
Adds CLAUDE.md with English-comment coding guidelines and updates doc/gin.txt with GinBlame command documentation, options, mappings, and behavior.
Vim plugin & ftplugins
plugin/gin-blame.vim, ftplugin/gin-blame.vim, ftplugin/gin-blame-nav.vim, after/ftplugin/gin-blame.vim, syntax/gin-blame-nav.vim
New plugin/ftplugin/after-ftplugin and syntax for gin-blame and gin-blame-nav: command registration, autocmds, highlight/sign, buffer-local Plug mappings, configurable default mappings, winfixbuf behavior, and syntax groups for nav lines.
Denops blame dispatcher
denops/gin/command/blame/main.ts, denops/gin/main.ts
Adds blame dispatcher wiring (main) and registers it in central denops/gin/main.ts; dispatcher exposes handlers for command, edit, nav, switch_to_commit, navigate_history, and update_detail.
Blame command & exec
denops/gin/command/blame/command.ts, denops/gin/command/blame/types.ts
Implements command exec flow and ExecOptions type: resolves worktree/filename, composes ginblame:// buffer names, and opens buffers with configurable opener/cmdarg/mods/bang.
Three-buffer UI & navigation
denops/gin/command/blame/edit.ts, denops/gin/command/blame/detail.ts, denops/gin/command/blame/history.ts, denops/gin/command/blame/buffer_utils.ts
Implements three-buffer blame UI lifecycle (ginblame, ginblamenav, ginblamedetail), formatting, synchronized layout, cursor/scroll bindings, detail updates (git show formatting + emojify), history management (get/save/add/update), buffer-context helpers, and navigation (switchToCommit, navigateHistory).
Git blame parser, tests & bench
denops/gin/command/blame/parser.ts, denops/gin/command/blame/parser_test.ts, denops/gin/command/blame/parser_bench.ts
Adds full git blame --porcelain parser with public types (GitPerson, GitCommit, GitBlameLine, GitBlameResult), comprehensive unit tests covering edge cases, and Deno benchmarks generating varied dataset sizes.
Utility: relative time
denops/gin/util/relative_time.ts, denops/gin/util/relative_time_test.ts
Adds relativeTime utility (minutes→years) with month/year helpers and extensive unit tests for past/future thresholds and pluralization.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant Vim as Vim/Neovim
  participant Plugin as plugin/gin-blame.vim
  participant DenopsInit as denops/gin/main.ts
  participant BlameDispatch as denops/gin/command/blame/main.ts
  participant CommandExec as denops/gin/command/blame/command.ts
  participant EditModule as denops/gin/command/blame/edit.ts
  Note over Vim,DenopsInit: startup
  DenopsInit->>BlameDispatch: main(denops)
  Note over User,Vim: User runs :GinBlame [args]
  User->>Vim: :GinBlame
  Vim->>Plugin: UserCommand -> queue denops request
  Plugin->>DenopsInit: enqueue "blame:command" (mods/args)
  DenopsInit->>BlameDispatch: dispatch "blame:command"
  BlameDispatch->>CommandExec: exec(denops, filename, options)
  CommandExec->>EditModule: exec → run git blame, format buffers
  EditModule-->>Vim: open/arrange 3-window layout, set mappings, autocmds
  Note over EditModule,Vim: CursorMoved => "blame:update_detail"
Loading
sequenceDiagram
  autonumber
  participant VimNav as gin-blamenav buffer
  participant BlameDispatch as denops/gin/command/blame/main.ts
  participant EditModule as denops/gin/command/blame/edit.ts
  Note over VimNav: user triggers mapping (Enter / <Plug>)
  VimNav->>BlameDispatch: "blame:switch_to_commit"
  BlameDispatch->>EditModule: switchToCommit()
  EditModule->>EditModule: updateBlameBuffers(commitish)
  EditModule-->>VimNav: refresh nav/file/detail buffers, restore cursors
  alt navigate history
    VimNav->>BlameDispatch: "blame:navigate_history"(older/newer)
    BlameDispatch->>EditModule: navigateHistory(direction)
    EditModule-->>VimNav: update buffers, persist history
  end
Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Poem

I hop through lines of porcelain night,
Three panes align, each commit in sight.
I tap a mapping, history unwinds,
Time reads "just now" in tidy lines.
A rabbit cheers — blame traced right. 🥕🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 78.95% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly and concisely summarizes the main feature of the changeset by stating that a GinBlame command is being added for interactive git blame navigation, matching the pull request’s implementation and objectives.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch add-ginbame

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🧹 Nitpick comments (19)
CLAUDE.md (2)

39-44: Make the JSDoc “Bad” example actually non-English

Update the “Bad” block to demonstrate non-English comments (keeping the same structure):

 /**
- * ❌ Bad (using non-English comments)
- * Parse blame output
- * @param content - Blame output
- * @returns Parsed result
+ * ❌ Bad (non-English comments)
+ * ブレームの出力を解析する
+ * @param content - ブレームの出力
+ * @returns 解析結果
  */

1-5: Consider relocating/renaming for discoverability

Move this to CONTRIBUTING.md (or docs/coding-guidelines.md) and link from README to ensure contributors see it.

ftplugin/gin-blame-nav.vim (1)

1-19: LGTM! Implementation is correct.

The ftplugin logic correctly:

  • Guards against re-execution
  • Prevents buffer switching with winfixbuf
  • Defines Plug mappings for blame navigation
  • Provides default mappings with opt-out via g:gin_blame_disable_default_mappings

Note: This file has nearly identical logic to after/ftplugin/gin-blame.vim. While this appears intentional (serving different filetypes: gin-blame-nav vs gin-blame), consider whether the duplication could be reduced through a shared function if maintenance becomes an issue.

denops/gin/util/relative_time_test.ts (1)

91-104: Stabilize month-based tests with a fixed base date.

Using Date() can hit EOM/DST quirks with setMonth. Prefer a fixed base (e.g., 2024-06-15T00:00:00Z) inside this step to keep results deterministic across environments.

ftplugin/gin-blame.vim (2)

10-12: Prefer denops#notify() for mappings to avoid blocking UI.

Mappings need no return value; notify reduces latency and avoids RPC blocking on the main thread.

Apply this diff:

-nnoremap <buffer> <silent> <Plug>(gin-blame-switch-commit) <Cmd>call denops#request('gin', 'blame:switch_to_commit', [])<CR>
-nnoremap <buffer> <silent> <Plug>(gin-blame-navigate-older) <Cmd>call denops#request('gin', 'blame:navigate_history', ['older'])<CR>
-nnoremap <buffer> <silent> <Plug>(gin-blame-navigate-newer) <Cmd>call denops#request('gin', 'blame:navigate_history', ['newer'])<CR>
+nnoremap <buffer> <silent> <Plug>(gin-blame-switch-commit) <Cmd>call denops#notify('gin', 'blame:switch_to_commit', [])<CR>
+nnoremap <buffer> <silent> <Plug>(gin-blame-navigate-older) <Cmd>call denops#notify('gin', 'blame:navigate_history', ['older'])<CR>
+nnoremap <buffer> <silent> <Plug>(gin-blame-navigate-newer) <Cmd>call denops#notify('gin', 'blame:navigate_history', ['newer'])<CR>

1-7: Add b:undo_ftplugin to cleanly undo options and mappings.

ftplugins should define undo logic for buffer-local changes.

Apply this diff:

 let b:did_ftplugin = 1
+let b:undo_ftplugin = 'setlocal nowinfixbuf | ' .
+      \ 'silent! nunmap <buffer> <CR> | ' .
+      \ 'silent! nunmap <buffer> <C-O> | ' .
+      \ 'silent! nunmap <buffer> <C-I> | ' .
+      \ 'silent! unmap <buffer> <Plug>(gin-blame-switch-commit) | ' .
+      \ 'silent! unmap <buffer> <Plug>(gin-blame-navigate-older) | ' .
+      \ 'silent! unmap <buffer> <Plug>(gin-blame-navigate-newer)'

Also applies to: 14-19

denops/gin/command/blame/parser_test.ts (1)

125-149: Add cases for filenames with spaces and previous filenames with spaces.

Git porcelain may emit filenames containing spaces. Add tests to ensure parser captures them correctly for both filename and previous fields.

syntax/gin-blame-nav.vim (1)

8-11: Use default highlight definitions to respect user colorschemes.

Prefer "hi default" and "hi default link" so themes can override.

Apply this diff:

-highlight link GinBlameLineNumber LineNr
-highlight GinBlameTime guifg=#87afaf ctermfg=109 gui=NONE cterm=NONE
-highlight GinBlameAuthor guifg=#ffaf5f ctermfg=215 gui=bold cterm=bold
-highlight link GinBlameSummary Normal
+highlight default link GinBlameLineNumber LineNr
+highlight default GinBlameTime guifg=#87afaf ctermfg=109 gui=NONE cterm=NONE
+highlight default GinBlameAuthor guifg=#ffaf5f ctermfg=215 gui=bold cterm=bold
+highlight default link GinBlameSummary Normal
denops/gin/command/blame/detail.ts (2)

55-60: Scope the “last displayed commit” to the detail buffer.

Store and read gin_blame_detail_last_commit on the detail buffer to avoid redundant updates when switching source buffers.

Apply this diff:

-  const lastCommitSha = await vars.b.get(denops, "gin_blame_detail_last_commit") as string | undefined;
+  const bufnrDetail = await fn.bufnr(denops, bufnameDetail);
+  if (bufnrDetail === -1) {
+    return;
+  }
+  const lastCommitSha = await fn.getbufvar(denops, bufnrDetail, "gin_blame_detail_last_commit") as string | undefined;
   if (lastCommitSha === commitSha) {
     return;
   }
@@
-  const bufnrDetail = await fn.bufnr(denops, bufnameDetail);
-  if (bufnrDetail === -1) {
-    return;
-  }
-
   await buffer.replace(denops, bufnrDetail, formattedContent);
-  await vars.b.set(denops, "gin_blame_detail_last_commit", commitSha);
+  await fn.setbufvar(denops, bufnrDetail, "gin_blame_detail_last_commit", commitSha);

Also applies to: 71-78


68-76: Decode using the target buffer’s encoding.

Use bufnrDetail for buffer.decode to match the buffer you write to.

Apply this diff:

-  const { content } = await buffer.decode(denops, bufnrCurrent, stdout, {});
+  const { content } = await buffer.decode(denops, bufnrDetail, stdout, {});
denops/gin/util/relative_time.ts (2)

27-51: Improve month/year thresholds and i18n.

Using 30/365-day constants can yield surprising outputs around month-end and leap years. Consider:

  • Using calendar cutovers (compare calendar month/year deltas) instead of 30/365 ms.
  • Using Intl.RelativeTimeFormat for correct pluralization and localization.

If you keep this approach, add tests for end-of-month and leap-year boundaries.


56-71: Minor API cleanup.

calculateMonthsDifference/Years don’t need isFuture; derive earlier/later by comparing dates to reduce call-site coupling and chance of inversion.

Also applies to: 76-94

denops/gin/command/blame/main.ts (1)

41-45: Wrap edit:nav and update_detail with friendlyCall.

Uncaught errors can break UX and differ from other handlers.

Apply:

     "blame:edit:nav": (bufnr, bufname) => {
       assert(bufnr, is.Number, { name: "bufnr" });
       assert(bufname, is.String, { name: "bufname" });
-      //return helper.friendlyCall(denops, () => editNav(denops, bufnr, bufname));
-      return editNav(denops, bufnr, bufname);
+      return helper.friendlyCall(denops, () => editNav(denops, bufnr, bufname));
     },
@@
     "blame:update_detail": () => {
-      return updateDetail(denops);
+      return helper.friendlyCall(denops, () => updateDetail(denops));
     },

Also applies to: 56-58

denops/gin/command/blame/parser.ts (1)

78-81: Support uppercase SHAs.

Git SHAs can be upper-case in some contexts. Relax regex to accept A-F.

Apply:

-const SHA_REGEX = /^([0-9a-f]{40}) (\d+) (\d+)(?: (\d+))?$/;
-const PREVIOUS_REGEX = /^previous ([0-9a-f]{40}) (.+)$/;
+const SHA_REGEX = /^([0-9a-fA-F]{40}) (\d+) (\d+)(?: (\d+))?$/;
+const PREVIOUS_REGEX = /^previous ([0-9a-fA-F]{40}) (.+)$/;
denops/gin/command/blame/edit.ts (5)

70-75: Prefer const for fileFragment

Not reassigned; make const.

-  let fileFragment = await vars.b.get(denops, "gin_blame_file_fragment") as string | undefined;
+  const fileFragment = await vars.b.get(denops, "gin_blame_file_fragment") as string | undefined;

97-97: Remove unused variable

currentWin is never used.

-    const currentWin = await fn.winnr(denops);

300-301: bufwinid expects a buffer; resolve bufnr first

Avoid passing the name; resolve to bufnr to be safe.

-  const winidNav = await fn.bufwinid(denops, bufnameNav);
+  const existingBufnrNav = await fn.bufnr(denops, bufnameNav);
+  const winidNav = await fn.bufwinid(denops, existingBufnrNav);

307-308: Remove unused variable

bufnrDetail is never used.

-    const bufnrDetail = await fn.bufnr(denops, bufnameDetail);

487-505: Return values of updateBlameBuffers

You return new buffer numbers but ignore them in navigateHistory. Either use the return or consistently resolve actual bufnrs by name (as you already do). Minor consistency nit.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a75dfa2 and adecd34.

📒 Files selected for processing (20)
  • CLAUDE.md (1 hunks)
  • after/ftplugin/gin-blame.vim (1 hunks)
  • denops/gin/command/blame/buffer_utils.ts (1 hunks)
  • denops/gin/command/blame/command.ts (1 hunks)
  • denops/gin/command/blame/detail.ts (1 hunks)
  • denops/gin/command/blame/edit.ts (1 hunks)
  • denops/gin/command/blame/history.ts (1 hunks)
  • denops/gin/command/blame/main.ts (1 hunks)
  • denops/gin/command/blame/parser.ts (1 hunks)
  • denops/gin/command/blame/parser_bench.ts (1 hunks)
  • denops/gin/command/blame/parser_test.ts (1 hunks)
  • denops/gin/command/blame/types.ts (1 hunks)
  • denops/gin/main.ts (2 hunks)
  • denops/gin/util/relative_time.ts (1 hunks)
  • denops/gin/util/relative_time_test.ts (1 hunks)
  • doc/gin.txt (1 hunks)
  • ftplugin/gin-blame-nav.vim (1 hunks)
  • ftplugin/gin-blame.vim (1 hunks)
  • plugin/gin-blame.vim (1 hunks)
  • syntax/gin-blame-nav.vim (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
denops/gin/command/blame/types.ts (1)
denops/gin/command/blame/command.ts (1)
  • ExecOptions (9-17)
denops/gin/command/blame/parser_test.ts (1)
denops/gin/command/blame/parser.ts (3)
  • parseGitBlamePorcelain (224-292)
  • GitCommit (20-41)
  • GitBlameLine (47-58)
denops/gin/command/blame/command.ts (3)
denops/gin/command/blame/types.ts (1)
  • ExecOptions (16-22)
denops/gin/command/blame/edit.ts (1)
  • exec (175-401)
denops/gin/git/worktree.ts (1)
  • findWorktreeFromDenops (123-133)
denops/gin/command/blame/history.ts (1)
denops/gin/command/blame/types.ts (1)
  • HistoryEntry (4-11)
denops/gin/command/blame/detail.ts (3)
denops/gin/command/blame/edit.ts (1)
  • updateDetail (484-484)
denops/gin/command/blame/parser.ts (1)
  • GitBlameResult (64-75)
denops/gin/command/blame/buffer_utils.ts (1)
  • getBlameLine (84-103)
denops/gin/command/blame/parser_bench.ts (1)
denops/gin/command/blame/parser.ts (1)
  • parseGitBlamePorcelain (224-292)
denops/gin/command/blame/buffer_utils.ts (1)
denops/gin/command/blame/parser.ts (2)
  • GitBlameResult (64-75)
  • GitBlameLine (47-58)
denops/gin/util/relative_time_test.ts (1)
denops/gin/util/relative_time.ts (1)
  • relativeTime (4-51)
denops/gin/command/blame/main.ts (5)
denops/gin/main.ts (1)
  • main (22-41)
denops/gin/util/cmd.ts (3)
  • parseSilent (8-15)
  • fillCmdArgs (42-63)
  • normCmdArgs (17-24)
denops/gin/command/blame/edit.ts (6)
  • edit (32-54)
  • editNav (61-159)
  • switchToCommit (694-869)
  • navigateHistory (889-983)
  • updateDetail (484-484)
  • exec (175-401)
denops/gin/command/blame/detail.ts (1)
  • updateDetail (16-78)
denops/gin/command/blame/command.ts (1)
  • exec (19-50)
denops/gin/command/blame/edit.ts (5)
denops/gin/command/blame/command.ts (2)
  • exec (19-50)
  • ExecOptions (9-17)
denops/gin/command/blame/types.ts (1)
  • ExecOptions (16-22)
denops/gin/command/blame/parser.ts (4)
  • parseGitBlamePorcelain (224-292)
  • GitBlameResult (64-75)
  • GitBlameLine (47-58)
  • GitCommit (20-41)
denops/gin/util/relative_time.ts (1)
  • relativeTime (4-51)
denops/gin/command/blame/buffer_utils.ts (2)
  • getBlameContext (27-78)
  • getBlameLine (84-103)
🪛 GitHub Actions: Test
CLAUDE.md

[error] 4-7: Type-checking failed during deno test. TS2304 cannot find 'disableVisualLineModifications', TS2552 cannot find 'denops', and cannot find 'bufnr'. Process exited with code 1.

denops/gin/command/blame/edit.ts

[error] 23-23: deno lint: no-unused-vars. 'HistoryEntry is never used'. Command: 'deno lint'.

🪛 GitHub Check: check (ubuntu-latest, 2.x)
denops/gin/command/blame/edit.ts

[failure] 307-307:
�[0m�[1mbufnrDetail is never used


[failure] 97-97:
�[0m�[1mcurrentWin is never used


[failure] 70-70:
�[0m�[1mfileFragment is never reassigned


[failure] 25-25:
�[0m�[1mupdateDetail is never used


[failure] 23-23:
�[0m�[1mHistoryEntry is never used

🔇 Additional comments (11)
denops/gin/command/blame/types.ts (1)

4-11: LGTM!

The HistoryEntry type is well-structured with clear documentation for blame navigation tracking.

denops/gin/main.ts (1)

8-8: LGTM!

The blame feature is correctly integrated into the main initialization sequence. The placement before mainBranch maintains the alphabetical ordering of command initializations.

Also applies to: 28-28

doc/gin.txt (1)

422-475: LGTM! Comprehensive documentation.

The documentation for :GinBlame is well-structured and covers:

  • The 3-window layout with clear descriptions
  • The ++emojify option with example
  • Default mappings and their behavior
  • Plug mappings for customization
  • Compound filetype support
  • Integration with default/persistent args

The documentation follows the existing style and provides sufficient detail for users.

plugin/gin-blame.vim (1)

1-24: LGTM! Solid plugin implementation.

The plugin correctly:

  • Guards against re-loading
  • Registers BufReadCmd autocmds for ginblame:// and ginblamenav:// URI schemes
  • Implements proper denops wait handling before executing commands
  • Defines the :GinBlame command with appropriate flags (-bang -bar -nargs=*)
  • Provides highlight and sign definitions for visual dividers
after/ftplugin/gin-blame.vim (2)

1-9: Excellent explanation of after/ftplugin/ placement.

The detailed comment clearly explains why this file must be in after/ftplugin/ to ensure gin-blame mappings override conflicting mappings from base filetypes (like TypeScript's <C-O>/<C-I>) in compound filetypes such as typescript.gin-blame.


10-25: LGTM! Implementation matches ftplugin/gin-blame-nav.vim.

The ftplugin logic is correct and intentionally mirrors ftplugin/gin-blame-nav.vim to provide consistent navigation mappings across both gin-blame and gin-blame-nav filetypes.

denops/gin/command/blame/parser_bench.ts (2)

4-46: LGTM! Well-structured benchmark data generation.

The generateBlameData function correctly synthesizes git blame porcelain output with:

  • Randomized commit selection
  • Realistic metadata (author, committer, timestamps)
  • Variable group sizes
  • Appropriate content lines

The logic handles the porcelain format requirements properly.


48-113: LGTM! Comprehensive benchmark coverage.

The benchmarks effectively test:

  • Various file sizes (100 to 50K lines)
  • Worst-case scenario (1K unique commits)
  • Edge cases (empty input, single line)

Pre-generating the data is a good practice to avoid measurement noise.

denops/gin/command/blame/command.ts (1)

19-50: LGTM! Correct buffer opening logic.

The exec function properly:

  • Resolves the worktree with verbose support
  • Computes relative path handling both absolute and relative inputs
  • Constructs the buffer name with correct scheme (ginblame://) and parameters
  • Opens the buffer with sensible defaults (opener: "tabedit")
  • Properly handles the emojify option in the buffer name params
ftplugin/gin-blame.vim (1)

17-18: Heads-up: collides with in many terminals.

is often indistinguishable from in Normal mode. Confirm this is acceptable for these buffers.

denops/gin/command/blame/edit.ts (1)

421-469: Good structure in formatGitBlameResult

Clear mapping and divider placement; O(1) lookups are handy. LGTM.

Comment on lines +273 to +276
await denops.cmd(
`autocmd CursorMoved <buffer> call denops#request('gin', 'blame:update_detail', [])`,
);
await fn.sign_unplace(denops, "*", { buffer: bufnr });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Avoid duplicate CursorMoved autocmds; wrap in augroup and clear per-buffer

Repeated exec() calls will stack CursorMoved autocmds. Use augroup + autocmd! .

-      await denops.cmd(
-        `autocmd CursorMoved <buffer> call denops#request('gin', 'blame:update_detail', [])`,
-      );
+      await denops.cmd([
+        "augroup GinBlameUpdateDetail",
+        "  autocmd! * <buffer>",
+        "  autocmd CursorMoved <buffer> call denops#request('gin', 'blame:update_detail', [])",
+        "augroup END",
+      ].join("\n"));
📝 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.

Suggested change
await denops.cmd(
`autocmd CursorMoved <buffer> call denops#request('gin', 'blame:update_detail', [])`,
);
await fn.sign_unplace(denops, "*", { buffer: bufnr });
await denops.cmd([
"augroup GinBlameUpdateDetail",
" autocmd! * <buffer>",
" autocmd CursorMoved <buffer> call denops#request('gin', 'blame:update_detail', [])",
"augroup END",
].join("\n"));
await fn.sign_unplace(denops, "*", { buffer: bufnr });
🤖 Prompt for AI Agents
In denops/gin/command/blame/edit.ts around lines 273 to 276, the code adds a
CursorMoved autocmd repeatedly which will stack on repeated exec() calls; wrap
the autocmd in a uniquely named augroup per buffer and clear existing autocmds
for that buffer first. Create an augroup name that incorporates the buffer
number, call autocmd! <buffer> inside that augroup to clear previous handlers
for the buffer, then define the CursorMoved autocmd within the same augroup so
it replaces prior duplicates; implement this using denops.cmd with the augroup,
autocmd!, autocmd, and augroup END sequence and keep the existing denops#request
call and buffer-scoped autocmd.

Comment on lines +585 to +589
await denops.call("nvim_buf_set_name", bufnrBlame, newBufnameBlame);

// Get the new buffer number after rename (nvim_buf_set_name may create a new buffer)
const actualBufnrBlame = await fn.bufnr(denops, newBufnameBlame);
bufnrBlame = actualBufnrBlame;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Neovim-only API used; add Vim fallback for buffer renaming

nvim_buf_set_name breaks on Vim. Guard with has('nvim') and fall back to :file on Vim.

-    await denops.call("nvim_buf_set_name", bufnrBlame, newBufnameBlame);
+    const hasNvim = await fn.has(denops, "nvim");
+    if (hasNvim) {
+      await denops.call("nvim_buf_set_name", bufnrBlame, newBufnameBlame);
+    } else {
+      if (winidBlame !== -1) {
+        await fn.win_execute(denops, winidBlame, `noautocmd file ${newBufnameBlame}`);
+      } else {
+        await buffer.ensure(denops, bufnrBlame, async () => {
+          await denops.cmd(`noautocmd file ${newBufnameBlame}`);
+        });
+      }
+    }
@@
-    await denops.call("nvim_buf_set_name", bufnrNav, newBufnameNav);
+    if (hasNvim) {
+      await denops.call("nvim_buf_set_name", bufnrNav, newBufnameNav);
+    } else {
+      if (winidNav !== -1) {
+        await fn.win_execute(denops, winidNav, `noautocmd file ${newBufnameNav}`);
+      } else {
+        await buffer.ensure(denops, bufnrNav, async () => {
+          await denops.cmd(`noautocmd file ${newBufnameNav}`);
+        });
+      }
+    }

Note: consider fnameescape() for safety if names can include spaces.

Also applies to: 631-635

🤖 Prompt for AI Agents
In denops/gin/command/blame/edit.ts around lines 585-589 (and similarly
631-635), the code uses the Neovim-only API nvim_buf_set_name which breaks in
Vim; wrap the rename in a has('nvim') conditional and call nvim_buf_set_name
only when has('nvim') is true, otherwise use the Vim fallback by executing the
:file command (use fnameescape on the new name) to rename the buffer; apply the
same guarded logic to the other occurrence so both Neovim and Vim are supported.

Comment on lines +39 to +67
export function createHistoryUpdate(
history: HistoryEntry[],
historyIndex: number,
commitish: string,
physicalLine: number,
filename: string,
): { updatedHistory: HistoryEntry[]; updatedIndex: number } {
let updatedHistory: HistoryEntry[];

if (historyIndex < 0) {
// First time: create initial entry
updatedHistory = [{
commitish,
physicalLine,
filename,
}];
} else {
// Update current entry, then truncate forward history
const temp = [...history];
temp[historyIndex] = {
commitish,
physicalLine,
filename,
};
updatedHistory = temp.slice(0, historyIndex + 1);
}

return { updatedHistory, updatedIndex: historyIndex };
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix history index initialization.

createHistoryUpdate returns updatedIndex = historyIndex, which keeps it at -1 on first save. Initialize to 0 on first entry.

Apply:

 export function createHistoryUpdate(
@@
 ): { updatedHistory: HistoryEntry[]; updatedIndex: number } {
   let updatedHistory: HistoryEntry[];
@@
-  return { updatedHistory, updatedIndex: historyIndex };
+  return {
+    updatedHistory,
+    updatedIndex: historyIndex < 0 ? 0 : historyIndex,
+  };
 }
📝 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.

Suggested change
export function createHistoryUpdate(
history: HistoryEntry[],
historyIndex: number,
commitish: string,
physicalLine: number,
filename: string,
): { updatedHistory: HistoryEntry[]; updatedIndex: number } {
let updatedHistory: HistoryEntry[];
if (historyIndex < 0) {
// First time: create initial entry
updatedHistory = [{
commitish,
physicalLine,
filename,
}];
} else {
// Update current entry, then truncate forward history
const temp = [...history];
temp[historyIndex] = {
commitish,
physicalLine,
filename,
};
updatedHistory = temp.slice(0, historyIndex + 1);
}
return { updatedHistory, updatedIndex: historyIndex };
}
export function createHistoryUpdate(
history: HistoryEntry[],
historyIndex: number,
commitish: string,
physicalLine: number,
filename: string,
): { updatedHistory: HistoryEntry[]; updatedIndex: number } {
let updatedHistory: HistoryEntry[];
if (historyIndex < 0) {
// First time: create initial entry
updatedHistory = [{
commitish,
physicalLine,
filename,
}];
} else {
// Update current entry, then truncate forward history
const temp = [...history];
temp[historyIndex] = {
commitish,
physicalLine,
filename,
};
updatedHistory = temp.slice(0, historyIndex + 1);
}
return {
updatedHistory,
updatedIndex: historyIndex < 0 ? 0 : historyIndex,
};
}
🤖 Prompt for AI Agents
In denops/gin/command/blame/history.ts around lines 39 to 67,
createHistoryUpdate sets updatedIndex to the incoming historyIndex which leaves
it at -1 when creating the first entry; change the function so that when
historyIndex < 0 (first time) you set updatedIndex to 0 (the new entry index)
and return that value, otherwise return the unchanged historyIndex for
subsequent updates; keep the rest of the logic (creating initial entry vs
updating and truncating) the same.

Comment on lines +68 to +69
args = await fillCmdArgs(denops, args, "edit");
args = await normCmdArgs(denops, args);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Bug: wrong command key for default/persistent args.

fillCmdArgs uses "edit", so g:gin_blame_* args are ignored and g:gin_edit_* are used. Use "blame".

Apply:

-  args = await fillCmdArgs(denops, args, "edit");
+  args = await fillCmdArgs(denops, args, "blame");
📝 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.

Suggested change
args = await fillCmdArgs(denops, args, "edit");
args = await normCmdArgs(denops, args);
args = await fillCmdArgs(denops, args, "blame");
args = await normCmdArgs(denops, args);
🤖 Prompt for AI Agents
In denops/gin/command/blame/main.ts around lines 68-69 the call to fillCmdArgs
uses the "edit" command key so it picks up g:gin_edit_* defaults/persistent args
instead of g:gin_blame_*. Replace the command key passed to fillCmdArgs with
"blame" (i.e., call fillCmdArgs(denops, args, "blame")) so default and
persistent args resolve from g:gin_blame_*; leave normCmdArgs call unchanged.

Comment on lines +16 to +22
export type ExecOptions = {
worktree: string;
commitish?: string;
encoding?: string;
fileformat?: string;
emojify?: boolean;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Critical: Type name conflict with command.ts.

The ExecOptions type is also exported in denops/gin/command/blame/command.ts (lines 9-17) with a different shape:

  • This file: worktree: string (required), plus encoding?, fileformat?
  • command.ts: worktree?: string (optional), plus opener?, cmdarg?, mods?, bang?

Having two different types with the same name in related modules will cause confusion and potential type errors when importing.

Consider renaming to distinguish their purposes, such as:

  • This file: EditExecOptions (used by edit.ts for buffer population)
  • command.ts: CommandExecOptions (used for command entry point with UI options)

Apply this diff to rename the type in this file:

 /**
  * Options for exec function
  */
-export type ExecOptions = {
+export type EditExecOptions = {
   worktree: string;
   commitish?: string;
   encoding?: string;
   fileformat?: string;
   emojify?: boolean;
 };

Then update the import in denops/gin/command/blame/edit.ts accordingly.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In denops/gin/command/blame/types.ts around lines 16 to 22, the exported type
ExecOptions conflicts with a different ExecOptions in
denops/gin/command/blame/command.ts; rename this type to a distinct name (e.g.,
EditExecOptions) to reflect its role for buffer population, update its export
accordingly, and then update the import in denops/gin/command/blame/edit.ts to
import the new name; ensure the shape stays the same and update any local
references in this module and edit.ts to use EditExecOptions.

- Remove unused HistoryEntry import (re-exported from types.ts)
- Remove unused updateDetail import (re-exported from detail.ts)
- Remove unused currentWin variable in editNav
- Remove unused bufnrDetail variable
- Change fileFragment from let to const (never reassigned)
Change typescript code blocks to plain text to avoid type checking
errors during deno test --doc execution
@codecov
Copy link

codecov bot commented Oct 12, 2025

Codecov Report

❌ Patch coverage is 89.85507% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.07%. Comparing base (a75dfa2) to head (9a803ec).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
denops/gin/util/relative_time.ts 82.08% 12 Missing ⚠️
denops/gin/command/blame/parser.ts 93.57% 8 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #165      +/-   ##
==========================================
+ Coverage   59.09%   65.07%   +5.97%     
==========================================
  Files          17       19       +2     
  Lines         858     1065     +207     
  Branches      120      163      +43     
==========================================
+ Hits          507      693     +186     
- Misses        351      371      +20     
- Partials        0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
CLAUDE.md (1)

21-31: Restore the self-contained TypeScript example and fix the non-English “Bad” comment.
The snippet still lacks the // @ts-nocheck + stubs, so deno doc keeps erroring on the unknown symbols, and the “Bad” example remains in English despite the heading. Please apply the previously requested update.

-Good example:
-```
-// Disable visual features that affect line number display
-await disableVisualLineModifications(denops, bufnr);
-```
-
-Bad example (using non-English comments):
-```
-// Disable visual line modifications
-await disableVisualLineModifications(denops, bufnr);
-```
+Good vs. bad example:
+```typescript
+// @ts-nocheck
+declare const denops: unknown;
+declare const bufnr: number;
+async function disableVisualLineModifications(_: unknown, __: number): Promise<void> {}
+
+// ✅ Good — leave comments in English
+// Disable visual features that affect line number display
+await disableVisualLineModifications(denops, bufnr);
+
+// ❌ Bad — non-English comment
+// 行番号の表示に影響する視覚的な機能を無効化する
+await disableVisualLineModifications(denops, bufnr);
+```

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used**: CodeRabbit UI

**Review profile**: CHILL

**Plan**: Pro

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 5e8dd3e76b99ea72835f5c64c8064d05c6f7daa0 and ca5f0cb5d5853f26536f3ca921f6df8673eab329.

</details>

<details>
<summary>📒 Files selected for processing (1)</summary>

* `CLAUDE.md` (1 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>

<details>
<summary>CLAUDE.md</summary>

22-22: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

28-28: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

34-34: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

43-43: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Comment on lines 21 to 49
Good example:
```
// Disable visual features that affect line number display
await disableVisualLineModifications(denops, bufnr);
```

Bad example (using non-English comments):
```
// Disable visual line modifications
await disableVisualLineModifications(denops, bufnr);
```

Good example (JSDoc):
```
/**
* Parse git blame porcelain output
* @param content - Raw output from git blame --porcelain
* @returns Parsed blame result with commits and lines
*/
```

Bad example (vague comments):
```
/**
* Parse blame output
* @param content - Blame output
* @returns Parsed result
*/
```
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add language identifiers to every fenced code block.
Markdownlint (MD040) is flagging these fences; please tag them with typescript so tooling knows how to highlight and lint them.

-Good example (JSDoc):
-```
+Good example (JSDoc):
+```typescript
 /**
  * Parse git blame porcelain output
  * @param content - Raw output from git blame --porcelain
  * @returns Parsed blame result with commits and lines
  */

-Bad example (vague comments):
- +Bad example (vague comments): +typescript
/**

  • Parse blame output
  • @param content - Blame output
  • @returns Parsed result
    */

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>

22-22: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

28-28: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

34-34: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

43-43: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

</details>

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

In CLAUDE.md around lines 21 to 49, all fenced code blocks lack language
identifiers causing markdownlint MD040; update each triple-backtick fence in
this range to include "typescript" (e.g. ```typescript) for every example block
(both good and bad examples and JSDoc snippets) so syntax highlighting and
linting work correctly.


</details>

<!-- fingerprinting:phantom:triton:sabertoothed -->

<!-- This is an auto-generated comment by CodeRabbit -->

- Use denops#notify() instead of denops#request() for mappings (non-blocking)
- Add b:undo_ftplugin to properly clean up ftplugin settings
- Fix relative time test to use fixed base date (avoid EOM/DST issues)
- Update CLAUDE.md bad example to use actual non-English comments
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (8)
CLAUDE.md (2)

24-27: Fix MD040 and deno doc type-check: add language id and make snippet self-contained

Tag fence with typescript and add ts-nocheck and stubs so deno doc doesn’t type-check unknown symbols.

-Good example:
-```
-// Disable visual features that affect line number display
-await disableVisualLineModifications(denops, bufnr);
-```
+Good example:
+```typescript
+// @ts-nocheck
+declare const denops: unknown;
+declare const bufnr: number;
+async function disableVisualLineModifications(_: unknown, __: number): Promise<void> {}
+// Disable visual features that affect line number display
+await disableVisualLineModifications(denops, bufnr);
+```

31-34: Make “Bad” example actually non-English and tag fence

Replace the English comment with a real non-English sentence and add typescript fence; keep the call unchanged.

-Bad example (using non-English comments):
-```
-// Disable visual line modifications
-await disableVisualLineModifications(denops, bufnr);
-```
+Bad example (non-English comment):
+```typescript
+// @ts-nocheck
+declare const denops: unknown;
+declare const bufnr: number;
+async function disableVisualLineModifications(_: unknown, __: number): Promise<void> {}
+// 行番号の表示に影響する視覚的な機能を無効化する
+await disableVisualLineModifications(denops, bufnr);
+```
denops/gin/command/blame/edit.ts (4)

281-284: Prevent duplicate CursorMoved autocmds; wrap in augroup and clear per-buffer

Without an augroup + autocmd! , repeated exec() stacks handlers.

-      await denops.cmd(
-        `autocmd CursorMoved <buffer> call denops#request('gin', 'blame:update_detail', [])`,
-      );
+      await denops.cmd([
+        "augroup GinBlameUpdateDetail",
+        "  autocmd! * <buffer>",
+        "  autocmd CursorMoved <buffer> call denops#request('gin', 'blame:update_detail', [])",
+        "augroup END",
+      ].join("\n"));

362-365: Same fix for nav buffer autocmd

Mirror the augroup approach to avoid duplicates.

-      await denops.cmd(
-        `autocmd CursorMoved <buffer> call denops#request('gin', 'blame:update_detail', [])`,
-      );
+      await denops.cmd([
+        "augroup GinBlameUpdateDetail",
+        "  autocmd! * <buffer>",
+        "  autocmd CursorMoved <buffer> call denops#request('gin', 'blame:update_detail', [])",
+        "augroup END",
+      ].join("\n"));

598-606: Neovim-only API: add Vim fallback for buffer rename

Guard nvim_buf_set_name with has('nvim'), fallback to :file on Vim; also pass strings explicitly.

-    const oldBufnrBlame = bufnrBlame;
-    await denops.call("nvim_buf_set_name", bufnrBlame, newBufnameBlame);
+    const oldBufnrBlame = bufnrBlame;
+    const hasNvim = await fn.has(denops, "nvim");
+    if (hasNvim) {
+      await denops.call("nvim_buf_set_name", bufnrBlame, newBufnameBlame.toString());
+    } else {
+      if (winidBlame !== -1) {
+        await fn.win_execute(
+          denops,
+          winidBlame,
+          `noautocmd file ${fn.fnameescape
+            ? await fn.fnameescape(denops, newBufnameBlame.toString())
+            : newBufnameBlame.toString()}`,
+        );
+      } else {
+        await buffer.ensure(denops, bufnrBlame, async () => {
+          await denops.cmd(
+            `noautocmd file ${newBufnameBlame.toString().replaceAll(" ", "\\ ")}`,
+          );
+        });
+      }
+    }
@@
-    const oldBufnrNav = bufnrNav;
-    await denops.call("nvim_buf_set_name", bufnrNav, newBufnameNav);
+    const oldBufnrNav = bufnrNav;
+    if (hasNvim) {
+      await denops.call("nvim_buf_set_name", bufnrNav, newBufnameNav.toString());
+    } else {
+      if (winidNav !== -1) {
+        await fn.win_execute(
+          denops,
+          winidNav,
+          `noautocmd file ${fn.fnameescape
+            ? await fn.fnameescape(denops, newBufnameNav.toString())
+            : newBufnameNav.toString()}`,
+        );
+      } else {
+        await buffer.ensure(denops, bufnrNav, async () => {
+          await denops.cmd(
+            `noautocmd file ${newBufnameNav.toString().replaceAll(" ", "\\ ")}`,
+          );
+        });
+      }
+    }

Also applies to: 677-684


1086-1094: Use actual (post-rename) blame bufnr when restoring cursor in history

bufnrBlame may be stale after rename; use actualBufnrBlame.

-    const winidBlame = await fn.bufwinid(denops, bufnrBlame);
+    const winidBlame = await fn.bufwinid(denops, actualBufnrBlame);
     if (winidBlame !== -1) {
       await fn.win_execute(
         denops,
         winidBlame,
         `call cursor(${historyEntry.physicalLine}, 0)`,
       );
     }
denops/gin/command/blame/history.ts (1)

80-81: Fix first-entry index: initialize to 0

createHistoryUpdate returns -1 for the first entry; set updatedIndex to 0 when historyIndex < 0.

-  return { updatedHistory, updatedIndex: historyIndex };
+  return {
+    updatedHistory,
+    updatedIndex: historyIndex < 0 ? 0 : historyIndex,
+  };
denops/gin/command/blame/buffer_utils.ts (1)

48-56: Validate paired nav buffer existence (bufnrNav) to avoid downstream errors

Add -1 checks after resolving the paired buffer.

@@
   } else if (scheme === "ginblame") {
     bufnrBlame = bufnrCurrent;
     const bufnameNav = await vars.b.get(denops, "gin_blame_nav_bufname") as
       | string
       | undefined;
     if (!bufnameNav) {
       throw new Error("Cannot find associated ginblamenav buffer");
     }
     bufnrNav = await fn.bufnr(denops, bufnameNav);
+    if (bufnrNav === -1) {
+      throw new Error("Ginblamenav buffer not found");
+    }
   } else {
@@
-  if (bufnrBlame === -1) {
+  if (bufnrBlame === -1) {
     throw new Error("Ginblame buffer not found");
   }

Also applies to: 76-83

🧹 Nitpick comments (2)
CLAUDE.md (1)

38-44: Tag fenced code blocks with language identifier

Add typescript to satisfy MD040 and enable highlighting.

-Good example (JSDoc):
-```
+Good example (JSDoc):
+```typescript
@@
-Bad example (vague comments):
-```
+Bad example (vague comments):
+```typescript

Also applies to: 48-54

denops/gin/command/blame/detail.ts (1)

62-93: Cache last commit in the detail buffer (avoid redundant updates across windows)

Store/read gin_blame_detail_last_commit on the ginblamedetail buffer, not the current buffer. Prevents double updates when CursorMoved fires in both windows.

-  // Check if already displaying this commit
-  const lastCommitSha = await vars.b.get(
-    denops,
-    "gin_blame_detail_last_commit",
-  ) as string | undefined;
-  if (lastCommitSha === commitSha) {
-    return;
-  }
+  // Resolve detail buffer number early
+  const bufnrDetail = await fn.bufnr(denops, bufnameDetail);
+  if (bufnrDetail === -1) {
+    return;
+  }
+  // Check if already displaying this commit (read from detail buffer)
+  const lastCommitSha = await fn.getbufvar(
+    denops,
+    bufnrDetail,
+    "gin_blame_detail_last_commit",
+    "",
+  ) as string;
+  if (lastCommitSha === commitSha) {
+    return;
+  }
@@
-  const bufnrDetail = await fn.bufnr(denops, bufnameDetail);
-  if (bufnrDetail === -1) {
-    return;
-  }
-
   await buffer.replace(denops, bufnrDetail, formattedContent);
-  await vars.b.set(denops, "gin_blame_detail_last_commit", commitSha);
+  await fn.setbufvar(
+    denops,
+    bufnrDetail,
+    "gin_blame_detail_last_commit",
+    commitSha,
+  );
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ca5f0cb and 166a424.

📒 Files selected for processing (6)
  • CLAUDE.md (1 hunks)
  • denops/gin/command/blame/buffer_utils.ts (1 hunks)
  • denops/gin/command/blame/detail.ts (1 hunks)
  • denops/gin/command/blame/edit.ts (1 hunks)
  • denops/gin/command/blame/history.ts (1 hunks)
  • denops/gin/command/blame/main.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • denops/gin/command/blame/main.ts
🧰 Additional context used
🧬 Code graph analysis (4)
denops/gin/command/blame/buffer_utils.ts (1)
denops/gin/command/blame/parser.ts (2)
  • GitBlameResult (64-75)
  • GitBlameLine (47-58)
denops/gin/command/blame/history.ts (1)
denops/gin/command/blame/types.ts (1)
  • HistoryEntry (4-11)
denops/gin/command/blame/detail.ts (2)
denops/gin/command/blame/parser.ts (1)
  • GitBlameResult (64-75)
denops/gin/command/blame/buffer_utils.ts (1)
  • getBlameLine (96-117)
denops/gin/command/blame/edit.ts (5)
denops/gin/command/blame/command.ts (2)
  • exec (19-50)
  • ExecOptions (9-17)
denops/gin/command/blame/types.ts (1)
  • ExecOptions (16-22)
denops/gin/command/blame/parser.ts (4)
  • parseGitBlamePorcelain (224-292)
  • GitBlameResult (64-75)
  • GitBlameLine (47-58)
  • GitCommit (20-41)
denops/gin/util/relative_time.ts (1)
  • relativeTime (4-51)
denops/gin/command/blame/buffer_utils.ts (2)
  • getBlameContext (27-90)
  • getBlameLine (96-117)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md

24-24: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


31-31: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


38-38: Fenced code blocks should have a language specified

(MD040, fenced-code-language)


48-48: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

Comment on lines +207 to +213
const { fileContent, navContent, dividers, lineMap, fileLineToPhysical } =
formatGitBlameResult(
blameResult,
options.emojify,
);

// Prepare buffer names for paired buffers
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Divider signs misaligned between nav and blame; track and place per-buffer dividers

The single dividers array uses nav offsets and is reused for blame, causing misplaced signs. Compute fileDividers and navDividers separately and use them per buffer.

-  const { fileContent, navContent, dividers, lineMap, fileLineToPhysical } =
+  const {
+    fileContent,
+    navContent,
+    fileDividers,
+    navDividers,
+    lineMap,
+    fileLineToPhysical,
+  } =
     formatGitBlameResult(
       blameResult,
       options.emojify,
     );
@@
-      await fn.sign_placelist(
+      await fn.sign_placelist(
         denops,
-        dividers.map((divider) => ({
+        fileDividers.map((divider) => ({
           id: divider,
           name: "GinBlameDividerSign",
           buffer: bufnr,
           lnum: divider,
         })),
       );
@@
-      await fn.sign_placelist(
+      await fn.sign_placelist(
         denops,
-        dividers.map((divider) => ({
+        navDividers.map((divider) => ({
           id: divider,
           name: "GinBlameDividerSign",
           buffer: bufnrNav,
           lnum: divider,
         })),
       );
@@
-function formatGitBlameResult(
+function formatGitBlameResult(
   result: GitBlameResult,
   shouldEmojify?: boolean,
 ): {
   fileContent: string[];
   navContent: string[];
-  dividers: number[];
+  fileDividers: number[];
+  navDividers: number[];
   lineMap: Record<number, GitBlameLine>;
   fileLineToPhysical: Record<number, number>;
 } {
   const { commits, lines } = result;
   const fileContent: string[] = [];
   const navContent: string[] = [];
-  const dividers: number[] = [];
+  const fileDividers: number[] = [];
+  const navDividers: number[] = [];
   const lineMap: Record<number, GitBlameLine> = {};
   const fileLineToPhysical: Record<number, number> = {};
 
   let lineNumber = 1;
-  let offset = 1;
   let previousCommitSha: string | undefined;
 
   const maxLineNumberLength = lines.at(-1)?.lineNumber.toString().length ?? 1;
 
   for (const line of lines) {
     if (previousCommitSha !== line.commitSha) {
       previousCommitSha = line.commitSha;
-      fileContent.push("");
-      navContent.push("");
-      navContent.push(
+      // Insert divider lines and record per-buffer divider positions
+      fileContent.push("");
+      fileDividers.push(fileContent.length);
+      navContent.push("");
+      navDividers.push(navContent.length);
+      navContent.push(
         `${lineNumber.toString().padStart(maxLineNumberLength)} ${
           formatNavLine(line, commits, shouldEmojify)
         }`,
       );
-      dividers.push(offset);
-      offset += 2;
     } else {
-      offset += 1;
       navContent.push(`${lineNumber.toString().padStart(maxLineNumberLength)}`);
     }
     fileContent.push(line.content);
     // Map physical line to blame line
     const physicalLine = fileContent.length;
     lineMap[physicalLine] = line;
     // Create reverse map: file line number -> physical line
     fileLineToPhysical[line.lineNumber] = physicalLine;
     lineNumber += 1;
   }
-  return { fileContent, navContent, dividers, lineMap, fileLineToPhysical };
+  return {
+    fileContent,
+    navContent,
+    fileDividers,
+    navDividers,
+    lineMap,
+    fileLineToPhysical,
+  };
 }
@@
-  const { fileContent, navContent, dividers, lineMap, fileLineToPhysical } =
+  const {
+    fileContent,
+    navContent,
+    fileDividers,
+    navDividers,
+    lineMap,
+    fileLineToPhysical,
+  } =
     formatGitBlameResult(
       newBlameResult,
       emojify,
     );
@@
-      await fn.sign_placelist(
+      await fn.sign_placelist(
         denops,
-        dividers.map((divider) => ({
+        fileDividers.map((divider) => ({
           id: divider,
           name: "GinBlameDividerSign",
           buffer: bufnrBlame,
           lnum: divider,
         })),
       );
@@
-      await fn.sign_placelist(
+      await fn.sign_placelist(
         denops,
-        dividers.map((divider) => ({
+        navDividers.map((divider) => ({
           id: divider,
           name: "GinBlameDividerSign",
           buffer: bufnrNav,
           lnum: divider,
         })),
       );

Also applies to: 286-294, 367-374, 435-483, 551-556, 653-663, 701-711

🤖 Prompt for AI Agents
In denops/gin/command/blame/edit.ts around lines 207-213 (and also apply same
change at 286-294, 367-374, 435-483, 551-556, 653-663, 701-711): the code
currently reuses a single dividers array (computed using nav offsets) for both
the nav buffer and the blame/file buffer which causes divider signs to be placed
at incorrect positions for the blame view; change the implementation to compute
two separate divider arrays (e.g., navDividers and fileDividers) using the
appropriate offsets for each buffer, and when rendering/inserting divider signs
use navDividers for the nav buffer and fileDividers for the blame/file buffer so
each buffer gets correctly aligned dividers.

@lambdalisue lambdalisue merged commit 4033d58 into main Oct 12, 2025
6 checks passed
@lambdalisue lambdalisue deleted the add-ginbame branch October 12, 2025 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants