Skip to content

bug: Fix external go to definition#10099

Merged
Light2Dark merged 2 commits into
marimo-team:mainfrom
yairchu:fix-external-go-to-definition
Jul 8, 2026
Merged

bug: Fix external go to definition#10099
Light2Dark merged 2 commits into
marimo-team:mainfrom
yairchu:fix-external-go-to-definition

Conversation

@yairchu

@yairchu yairchu commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

This pull request includes contributions from a coding agent.

📝 Summary

Closes #7135

📋 Pre-Review Checklist

  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • Any AI generated code has been reviewed line-by-line by the human PR author, who stands by it.
  • Video or media evidence is provided for any visual changes (optional).

✅ Merge Checklist

  • I have read the contributor guidelines.
  • Documentation has been updated where applicable, including docstrings for API changes.
  • Tests have been added for the changes made.

Copilot AI review requested due to automatic review settings July 8, 2026 09:10
@vercel

vercel Bot commented Jul 8, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
marimo-docs Ready Ready Preview, Comment Jul 8, 2026 9:19am

Request Review

@yairchu yairchu changed the title Fix external go to definition bug: Fix external go to definition Jul 8, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Updates the CodeMirror cell go-to-definition keybinding so it can “fall through” to lower-priority keymaps (e.g., LSP-provided go-to-definition) when marimo’s reactive-variable resolver can’t handle the symbol, addressing external code navigation (closes #7135).

Changes:

  • Adjust cell go-to-definition keybinding to return the resolver result instead of always consuming the shortcut.
  • Add a regression test to ensure a lower-priority keymap can handle go-to-definition when the marimo handler returns false.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
frontend/src/core/codemirror/cells/extensions.ts Lets the go-to-definition keybinding fall through by returning the resolver boolean.
frontend/src/core/codemirror/tests/setup.test.ts Adds a test asserting fall-through to a lower-priority keymap handler.

Comment thread frontend/src/core/codemirror/__tests__/setup.test.ts
yairchu and others added 2 commits July 8, 2026 12:18
Co-authored-by: OpenAI Codex <codex@openai.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Let the local notebook go-to-definition keybinding return false when it cannot resolve a symbol, so lower-precedence LSP keybindings can handle external definitions.

Co-authored-by: OpenAI Codex <codex@openai.com>
@yairchu

yairchu commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

CI is failing on missing PR label, but I can't seem to be able to edit the labels for it

@Light2Dark Light2Dark added the bug Something isn't working label Jul 8, 2026
@Light2Dark
Light2Dark merged commit 452cb64 into marimo-team:main Jul 8, 2026
27 of 28 checks passed
@yairchu
yairchu deleted the fix-external-go-to-definition branch July 8, 2026 14:27
Light2Dark added a commit that referenced this pull request Jul 9, 2026
…ck (#10102)

**This pull request was authored by a coding agent.**

## 📝 Summary

Follow-up to #10099 / #7135.

F12 go-to-definition already falls through to the language server when
marimo cannot resolve a symbol. This PR extends that same fallback to
the other entry points:

- Context menu "Go to Definition"
- Cmd/Ctrl+click navigation

Adds a shared `goToDefinitionWithLspFallback` helper that tries marimo's
notebook resolver first, then triggers the LSP keybinding via
`runScopeHandlers`.

## 📋 Pre-Review Checklist

- [x] For large changes, or changes that affect the public API: this
change was discussed or approved through an issue, on
[Discord](https://marimo.io/discord?ref=pr), or the community
[discussions](https://github.com/marimo-team/marimo/discussions) (Please
provide a link if applicable).
- [x] Any AI generated code has been reviewed line-by-line by the human
PR author, who stands by it.
- [ ] Video or media evidence is provided for any visual changes
(optional).

## ✅ Merge Checklist

- [x] I have read the [contributor
guidelines](https://github.com/marimo-team/marimo/blob/main/CONTRIBUTING.md).
- [x] Documentation has been updated where applicable, including
docstrings for API changes.
- [x] Tests have been added for the changes made.

Made with [Cursor](https://cursor.com)

---------

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Go to definition should also work for external code

3 participants