fix: only show "Go to Definition" cell action on variables#10070
Open
akshayka wants to merge 2 commits into
Open
fix: only show "Go to Definition" cell action on variables#10070akshayka wants to merge 2 commits into
akshayka wants to merge 2 commits into
Conversation
This PR updates the cell context menu to only show the Go to Definition action when the user right-clicks a variable that actually has a definition to jump to. Previously, the cell context menu always offered "Go to Definition" regardless of where the user right-clicked (even if they clicked on outputs!). Worse, it resolved the target from the text caret rather than the click, so it could silently do nothing (no token under the caret) or jump somewhere unrelated to what was clicked.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
for more information, see https://pre-commit.ci
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found and verified against the latest diff
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/components/editor/cell/cell-context-menu.tsx">
<violation number="1" location="frontend/src/components/editor/cell/cell-context-menu.tsx:220">
P2: Keyboard users can lose the Go to Definition action because the menu position is derived only from mouse coordinates. Falling back to the current selection head when the event has no pointer coordinates preserves the previous accessible behavior while keeping right-click targeting.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the CodeMirror “Go to Definition” behavior so the cell context menu only offers it when the user right-clicks on a symbol that can actually be resolved to a definition, and ensures the navigation target is derived from the click position (not the current caret).
Changes:
- Added position-based APIs (
goToDefinitionAtPosition,hasDefinitionAtPosition) and refactored definition resolution to support “check without navigating”. - Exported reusable navigation/lookup helpers (
goToPosition,findVariableDefinitionPosition) and updated call sites accordingly. - Updated the cell context menu to compute the right-click document position and conditionally show/execute “Go to Definition”, plus added tests for the new behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontend/src/core/codemirror/go-to-definition/utils.ts | Adds position-based go-to-definition helpers and refactors resolution logic to support context-menu usage. |
| frontend/src/core/codemirror/go-to-definition/commands.ts | Exposes goToPosition and adds findVariableDefinitionPosition helper used by the refactor. |
| frontend/src/core/codemirror/go-to-definition/tests/utils.test.ts | Adds unit tests for position-based resolution and definition availability checks. |
| frontend/src/components/editor/cell/cell-context-menu.tsx | Computes right-click position and hides/shows “Go to Definition” based on whether it resolves at that click location. |
Comment on lines
+40
to
+46
| function getWordAtPosition(state: EditorState, pos: number) { | ||
| const { startToken, endToken } = getPositionAtWordBounds(state.doc, pos); | ||
| return { | ||
| position: startToken, | ||
| word: state.doc.sliceString(startToken, endToken), | ||
| }; | ||
| } |
Comment on lines
+230
to
+239
| test("is a no-op when the position is not on a word", () => { | ||
| const code = "a + b"; | ||
| const view = createEditor(code, 0); | ||
| views.push(view); | ||
|
|
||
| // The `+` operator is flanked by whitespace, so no identifier resolves. | ||
| const result = goToDefinitionAtPosition(view, code.indexOf("+")); | ||
|
|
||
| expect(result).toBe(false); | ||
| }); |
akshayka
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR updates the cell context menu to only show the Go to Definition action when the user right-clicks a variable that actually has a definition to jump to.
Previously, the cell context menu always offered "Go to Definition" regardless of where the user right-clicked (even if they clicked on outputs!). Worse, it resolved the target from the text caret rather than the click, so it could silently do nothing or jump somewhere unrelated to what was clicked.