feat: action block highlight and jump #16
Conversation
|
Warning Rate limit exceeded@qvalentin has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds an action_highlight feature that highlights the current Helm action block and a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ft as ftplugin/helm.lua
participant match as lua/helm-ls/matchparen.lua
participant queries as lua/helm-ls/queries.lua
participant ts as Tree-sitter
participant nvim as Neovim
User->>ft: Press %
ft->>match: jump_to_matching_keyword()
match->>ts: Get node at cursor / find containing action
match->>queries: Load action_parts
match->>ts: Execute query on action node
ts-->>match: start/middle/end captures
alt Cursor on start
match->>nvim: Jump to middle or end
else Cursor on middle
match->>nvim: Jump to next middle or end
else Cursor on end
match->>nvim: Jump to start
else No jump
match-->>ft: return false
ft->>nvim: feed default "%"
end
sequenceDiagram
participant Neovim
participant core as lua/helm-ls.lua
participant highlight as lua/helm-ls/action-highlight.lua
participant queries as lua/helm-ls/queries.lua
participant ts as Tree-sitter
participant buf as Buffer
Neovim->>core: CursorMoved / CursorMovedI
core->>highlight: highlight_current_block()
highlight->>ts: Get parser & root
alt Parser unavailable
highlight-->>core: return
else Parser ready
highlight->>ts: Find containing action node
highlight->>queries: Load action_parts
highlight->>ts: Execute query within window range
loop for each capture
highlight->>highlight: Check nesting
alt Not nested
highlight->>buf: Set extmark (Visual)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 0
🧹 Nitpick comments (2)
lua/helm-ls/matchparen.lua (1)
42-51: Consider using indexed iteration for query captures.Lines 42-51 iterate over match using
pairs(match), wherematchis an array of capture nodes indexed by capture ID. The code assumes nodes are single-element arrays (nodes[1]), which works but could be more explicit.Consider this more explicit approach:
- for id, nodes in pairs(match) do + for id, nodes in pairs(match) do local capture_name = query.captures[id] if capture_name == "start" then - start_node = nodes[1] + start_node = nodes elseif capture_name == "middle" then - middle_node = nodes[1] + middle_node = nodes elseif capture_name == "end" then - end_node = nodes[1] + end_node = nodes end endOr add a comment explaining that each capture returns an array of nodes and you're taking the first one.
lua/helm-ls/action_highlight.lua (1)
62-65: Clarify boundary condition logic.Lines 62-65 have complex cursor position checks that are difficult to follow. The logic appears to exclude cases where the cursor is on the same line as the action block boundaries but outside the column range.
Consider adding a comment or extracting this logic into a helper function for clarity:
if cursor_row >= start_row and cursor_row <= end_row then - if (cursor_row == start_row and cursor_col < start_col) or (cursor_row == end_row and cursor_col > end_col) then - -- Cursor is outside the node on the same line, so we continue searching for a parent block - else + -- Check if cursor is truly inside the block (not just on the same line) + local is_before_start = cursor_row == start_row and cursor_col < start_col + local is_after_end = cursor_row == end_row and cursor_col > end_col + if not (is_before_start or is_after_end) then -- Cursor is inside the block, highlight the keywords and stop.This makes the intent clearer: we want to highlight when the cursor is within the block boundaries.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(2 hunks)ftplugin/helm.lua(1 hunks)lua/helm-ls.lua(5 hunks)lua/helm-ls/action_highlight.lua(1 hunks)lua/helm-ls/matchparen.lua(1 hunks)lua/helm-ls/queries.lua(1 hunks)
🔇 Additional comments (17)
README.md (3)
11-12: LGTM! Clear feature documentation.The feature descriptions are concise and accurately reflect the new capabilities added in this PR.
16-20: LGTM! Clear keymap documentation.The keymap documentation properly explains the % key behavior for block navigation.
63-66: LGTM! Configuration documentation is consistent.The action_highlight configuration is properly documented and matches the implementation in lua/helm-ls.lua.
ftplugin/helm.lua (1)
4-10: LGTM! Keymap implementation with proper fallback.The implementation correctly attempts the custom jump and falls back to the default "%" behavior when needed. The use of
nvim_feedkeyswith proper term code replacement is appropriate for simulating the default behavior.lua/helm-ls.lua (6)
6-6: LGTM! Type annotation is consistent.The action_highlight field properly extends the Config class following the same pattern as existing features.
15-17: LGTM! Default configuration is appropriate.The default of
enabled = trueis reasonable for a new highlighting feature, and matches the pattern of other features.
36-38: LGTM! Validation follows established pattern.The type validation for action_highlight configuration is consistent with the validation for other features.
45-60: LGTM! Proper lazy loading and initialization.The module is loaded only when enabled, following the same pattern as conceal and indent_hints features. The setup call is appropriate.
62-62: LGTM! Early return condition updated correctly.The condition now properly checks all three features before deciding to skip autocommand creation.
94-96: LGTM! Cursor event integration is appropriate.The highlight_current_block() call on CursorMoved/CursorMovedI events follows the same pattern as the other features and is appropriately guarded by the enabled check.
lua/helm-ls/matchparen.lua (2)
54-67: LGTM! Navigation logic is well-structured.The jump logic correctly handles all three cases (start -> middle/end, middle -> end, end -> start) and properly accounts for optional middle nodes. The innermost-first iteration ensures the most specific match is chosen.
6-12: Boundary check is correct. Tree-sitter uses 0-based, half-open ranges (end_col is exclusive), so usingcursor_col < end_colis the right check.lua/helm-ls/action_highlight.lua (4)
8-18: LGTM! Helper function is clear and correct.The
highlight_nodefunction properly handles nil nodes and correctly applies extmarks with the Visual highlight group.
20-37: LGTM! Proper setup and cleanup.The function correctly retrieves the parser, clears previous highlights, and parses the query. The early returns for missing parser or query are appropriate.
40-77: Verify iteration order matches comment intent.Line 39 states "from innermost to outermost" but the iteration over
query:iter_matches()may not guarantee this order. Unlike matchparen.lua which explicitly collects matches into a table and iterates backwards, this code iterates directly without explicit ordering.If innermost-first order is critical (to prefer nested blocks), consider matching the approach in matchparen.lua:
- -- Iterate over all matches, from innermost to outermost - for _, match in query:iter_matches(root, bufnr) do + -- Collect all matches first, then iterate from innermost to outermost + local matches = {} + for _, match in query:iter_matches(root, bufnr) do + table.insert(matches, match) + end + + for i = #matches, 1, -1 do + local match = matches[i]Otherwise, clarify the comment or verify that the current iteration order achieves the desired behavior.
79-85: LGTM! Public API is simple and appropriate.The setup function is a reasonable no-op placeholder, and highlight_current_block properly delegates to the internal function. This provides a clean public interface.
lua/helm-ls/queries.lua (1)
3-11: Verify Tree-sitter query for "else if"
No occurrences found; confirm Helm’s Tree-sitter grammar representselse ifas a single literal node or update the query to match separateelseandifnodes.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lua/helm-ls.lua (1)
67-74: Parser check is incorrect; language.add() returns nil.
vim.treesitter.language.add("helm")returns nil, soif not ... thenis always true → you always warn and return; features never run. Replace with require_language or a guarded get_parser.- if not vim.treesitter.language.add("helm") then - vim.notify( - "Helm-ls.nvim: tree-sitter parser for helm not installed, some features will not work. Make sure you have nvim-treesitter and then install it with :TSInstall helm", - vim.log.levels.WARN - ) - return - end + local ok = pcall(vim.treesitter.language.require_language, "helm", nil, true) + if not ok then + vim.notify( + "Helm-ls.nvim: tree-sitter parser for helm not installed. Install via nvim-treesitter: :TSInstall helm", + vim.log.levels.WARN + ) + return + end
🧹 Nitpick comments (3)
lua/helm-ls.lua (2)
57-60: Harden module load (avoid hard failure if module missing).Use protected require and disable feature on failure; also tolerate missing setup().
- if M.config.action_highlight.enabled then - action_highlight = require("helm-ls.action_highlight") - action_highlight.setup(M.config.action_highlight) - end + if M.config.action_highlight.enabled then + local ok, mod = pcall(require, "helm-ls.action_highlight") + if ok then + action_highlight = mod + if type(action_highlight.setup) == "function" then + action_highlight.setup(M.config.action_highlight) + end + else + vim.notify("Helm-ls.nvim: failed to load action_highlight; disabling", vim.log.levels.WARN) + end + end
81-98: Throttle CursorMoved work to avoid jank on large files.Three calls per move can be heavy; add a light throttle (configurable).
- vim.api.nvim_create_autocmd({ "CursorMoved", "CursorMovedI" }, { + local last = 0 + local throttle_ms = (M.config.action_highlight and M.config.action_highlight.throttle_ms) or 25 + vim.api.nvim_create_autocmd({ "CursorMoved", "CursorMovedI" }, { group = group_id, pattern = file_patterns, - callback = function() + callback = function() + local now = (vim.uv or vim.loop).now() + if now - last < throttle_ms then return end + last = now if vim.bo.filetype ~= "helm" then return end if indent_hints then indent_hints.add_indent_hints() end if conceal then conceal.update_conceal_templates() end if action_highlight then action_highlight.highlight_current_block() end end, })lua/helm-ls/matchparen.lua (1)
47-51: Parse and cache the query once; protect with pcall.Avoid re‑parsing on every jump and handle invalid query gracefully.
-local parts_query = vim.treesitter.query.parse("helm", queries.action_parts) -if not parts_query then - return false -end +M._ACTION_PARTS_Q = M._ACTION_PARTS_Q or (function() + local ok, q = pcall(vim.treesitter.query.parse, "helm", queries.action_parts) + return ok and q or nil +end)() +local parts_query = M._ACTION_PARTS_Q +if not parts_query then return false end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lua/helm-ls.lua(6 hunks)lua/helm-ls/action_highlight.lua(1 hunks)lua/helm-ls/matchparen.lua(1 hunks)lua/helm-ls/queries.lua(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lua/helm-ls/queries.lua
- lua/helm-ls/action_highlight.lua
🔇 Additional comments (7)
lua/helm-ls.lua (5)
6-6: Config surface extension looks good.Doc-annotating action_highlight on Config is consistent with existing fields.
15-17: Sensible default.Defaulting action_highlight.enabled to true is fine; ensure README reflects the default.
36-38: Type‑safety check added.Good guard for args.action_highlight type.
45-45: Local handle for feature module.Keeps callback cheap when disabled.
78-78: Pattern set extended.Adding NOTES.txt is fine; guarded by filetype check below, so negligible overhead.
lua/helm-ls/matchparen.lua (2)
6-12: Node hit‑testing looks fine.Single‑line token assumption is OK for start/middle/end keywords.
97-106: Jump logic reads well.Flow for start→middle/end, middle→next/end, end→start is clear.
Also applies to: 108-117, 119-126
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
lua/helm-ls/matchparen.lua (4)
15-21: Guard parser acquisition to avoid runtime errors.
get_parsercan throw an exception if the Helm parser is not installed. This issue was previously flagged.
25-28: Ensure cross-version compatibility for node retrieval.The
get_nodeAPI requires proper parameters for correct positioning. This issue was previously flagged.
55-80: Bound iter_captures to action_node range.The call to
iter_capturesshould be explicitly bounded to the action_node's range. This issue was previously flagged.
87-89: Sort by both row and column.The comparator only considers row, leaving ties unordered. This issue was previously flagged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lua/helm-ls.lua(6 hunks)lua/helm-ls/action-highlight.lua(1 hunks)lua/helm-ls/matchparen.lua(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lua/helm-ls.lua
🔇 Additional comments (3)
lua/helm-ls/action-highlight.lua (2)
8-18: LGTM!The highlight helper correctly applies extmarks with defensive nil-checking.
81-87: LGTM!Public API is clean. The placeholder
setupis appropriately documented.lua/helm-ls/matchparen.lua (1)
92-114: LGTM!The jump logic correctly handles navigation between start, middle, and end nodes with appropriate fallbacks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
lua/helm-ls/matchparen.lua (3)
6-12: Handle multi-line nodes in cursor detection.
Current check only matches start_row; make it robust for multi-line spans.local function is_cursor_on_node(cursor_row, cursor_col, node) - local start_row, start_col, _, end_col = node:range() - if cursor_row == start_row and cursor_col >= start_col and cursor_col < end_col then - return true - end - return false + local start_row, start_col, end_row, end_col = node:range() + if cursor_row == start_row and cursor_row == end_row then + return cursor_col >= start_col and cursor_col < end_col + elseif cursor_row == start_row then + return cursor_col >= start_col + elseif cursor_row == end_row then + return cursor_col < end_col + elseif cursor_row > start_row and cursor_row < end_row then + return true + end + return false end
25-29: Compat: get node at cursor across Neovim versions.
Prefer get_node with pos; fallback to get_node_at_pos.- local cursor_node = vim.treesitter.get_node({ bufnr = bufnr, include_anonymous = true }) + local cursor_node + if vim.treesitter.get_node then + local ok, node = pcall(vim.treesitter.get_node, { + bufnr = bufnr, + pos = { cursor_row, cursor_col }, + include_anonymous = true, + }) + if ok then cursor_node = node end + end + if not cursor_node and vim.treesitter.get_node_at_pos then + cursor_node = vim.treesitter.get_node_at_pos(bufnr, cursor_row, cursor_col, { include_anonymous = true }) + end
15-21: Guard parser acquisition and add cheap filetype gate.
get_parser can throw when the helm parser isn’t installed; bail cleanly.local bufnr = vim.api.nvim_get_current_buf() - local parser = vim.treesitter.get_parser(bufnr, "helm") - if not parser then - return false - end - parser:parse() + if vim.bo[bufnr].filetype ~= "helm" then + return false + end + local ok, parser = pcall(vim.treesitter.get_parser, bufnr, "helm") + if not ok or not parser then + return false + end + parser:parse()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lua/helm-ls/matchparen.lua(1 hunks)
🔇 Additional comments (2)
lua/helm-ls/matchparen.lua (2)
55-81: Nice: captures bounded to action range.
Limiting iter_captures to [start_row, end_row+1) prevents bleed from siblings and improves perf/correctness.
87-95: Good: sort by (row, col).
Stable lexicographic ordering fixes same-line ties.
Summary by CodeRabbit
New Features
Documentation