Skip to content

feat: Adjust call hierarchy navigation to match VS Code behavior#9

Open
qufeiyan wants to merge 1 commit intoretran:mainfrom
qufeiyan:refjump
Open

feat: Adjust call hierarchy navigation to match VS Code behavior#9
qufeiyan wants to merge 1 commit intoretran:mainfrom
qufeiyan:refjump

Conversation

@qufeiyan
Copy link
Copy Markdown

When viewing call hierarchy, selecting an element now jumps to the first reference line and shows the total reference count, improving consistency with VS Code's "show call hierarchy" feature.

When viewing call hierarchy, selecting an element now jumps to the first
reference line and shows the total reference count, improving
consistency with VS Code's "show call hierarchy" feature.
@retran retran self-assigned this Apr 12, 2026
Copy link
Copy Markdown
Owner

@retran retran left a comment

Choose a reason for hiding this comment

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

Thank you for this contribution, @qufeiyan! The idea is solid — using fromRanges[1] to jump to the actual call site (matching VS Code behavior) and showing the reference count are genuinely useful improvements.

After a thorough review I've found several bugs that need to be addressed before this can be merged:


Critical: Type hierarchy is broken

The changes to hierarchy.lua assume node.lsp_item is always a wrapped { item = ..., fromRanges = ... } table, but type hierarchy nodes store a plain LSP item directly. Specifically:

  • update_node (line ~305): self.strategy.fetch_children(self.client, node.lsp_item.item, ...).item is nil for type hierarchy nodes, so fetch_children in type.lua receives nil.
  • _setup_preview: The guard node.lsp_item.item.uri will error on type hierarchy nodes.

The call/type hierarchy paths need to be differentiated. The cleanest fix is to handle the wrapping inside the call strategy itself rather than in the generic Hierarchy class.


Critical: Root node has no fromRanges — nil crash

The root node is wrapped as { item = root_item } (no fromRanges key). In _setup_preview, the PR accesses node.lsp_item.fromRanges[1] — this will crash with a nil index error when viewing the root node of a call hierarchy.

Fix: guard with node.lsp_item.fromRanges and node.lsp_item.fromRanges[1] (the or chain already does this in the render function, just needs to be consistent in preview).


Bug: Placeholder guard in expand_action is now dead code

Original:

if not node or (node.lsp_item and node.lsp_item.is_placeholder) then return end

PR changes to:

if not node or (node.lsp_item and node.lsp_item.item and node.lsp_item.is_placeholder) then return end

For wrapped call hierarchy nodes, node.lsp_item.is_placeholder is always nil (the flag lives on .item). The check should be node.lsp_item.item.is_placeholder for wrapped nodes.


Bug: Jump keymap fires for both hierarchy types incorrectly

The first condition not node.lsp_item.is_placeholder is always true for wrapped items (the field is nil), so both branches match. The first branch then passes the wrapped table to util.jump_to_item, which relies on absence of .uri to detect wrapping — fragile.


Minor issues

  • Commented-out code in call.lua (-- return direction_key == ..., -- local get_item_key = ...) should be removed before merging.
  • ReferencesCount highlight group is used but never defined. Consider defining it in plugin/meow-yarn.lua alongside MeowYarnPreview, or falling back to a built-in group.
  • focusable = true on preview popup — is this intentional? It was previously false. If intentional, it would be good to mention it in the PR description.
  • Root node ID key still uses get_item_key while child nodes use get_caller_or_callee_item_key — this inconsistency could cause issues with path-dependent ID generation.
  • direction_key:find("call") heuristic for deciding whether to wrap is fragile. Better to have the call strategy signal this explicitly (e.g., a wraps_items = true flag on the strategy).

The core logic of preserving fromRanges alongside the item is the right approach. I'd suggest moving the wrapping responsibility fully into the call strategy (so hierarchy.lua doesn't need to know about it), and having strategy-specific accessor functions for get_lsp_item, get_uri, get_range, etc. that both strategies implement.

Happy to discuss the approach further — looking forward to a revised version!

Copy link
Copy Markdown
Owner

@retran retran left a comment

Choose a reason for hiding this comment

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

Several bugs need to be fixed before this can be merged — details in the inline comment above. The core idea is good and worth getting right. Looking forward to a revised version!

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