Go to definition functionality added to hotkey and context menu #1411

robmck1995 · 2024-05-19T12:07:59Z

For this one, I set the hotkey to Mod+Shift+, but let me know if there's a better one.

I'm unsure if this is structured correctly, specifically, the utility functions specified in cell-editor.tsx. Let me know if there is a better way to structure it. Basically, I wasn't sure how to ensure we could use the same functionality with the right-click menu and the hotkeys. The solution works, but there is a bit of code repetition.

Created this with the help of Glide! The change in this document only covers adding the hotkey, I manually added the right-click after as it was a small change:
https://glide.agenticlabs.com/sharing?userId=65bae76b7fbfb1c20ac1f3b8&taskId=ghM1mqX

vercel · 2024-05-19T12:08:03Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
marimo-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 20, 2024 10:06pm
marimo-storybook	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 20, 2024 10:06pm

sourcery-ai

Hey @robmck1995 - I've reviewed your changes and they look great!

Here's what I looked at during the review

🟡 General issues: 5 issues found
🟢 Security: all looks good
🟡 Testing: 3 issues found
🟡 Complexity: 1 issue found

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

_{Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.}

sourcery-ai · 2024-05-19T12:09:05Z

frontend/src/components/editor/cell/code/cell-editor.tsx

+  const focusByVariableName = useCallback(() => {
+    if (editorViewRef.current) {
+      const { state } = editorViewRef.current;
+      const variableName = getWordUnderCursor(state);
+      const focusCellId = getCellIdOfDefinition(variables, variableName);
+
+      if (focusCellId) {
+        focusCellAtDefinition({
+          cellId: focusCellId,
+          variableName: variableName,


issue (bug_risk): Missing dependency in useCallback hook.

The focusByVariableName function uses getWordUnderCursor and getCellIdOfDefinition, but these are not included in the dependency array. This could lead to stale closures if these functions change.

sourcery-ai · 2024-05-19T12:09:05Z

frontend/src/components/editor/cell/code/cell-editor.tsx

+  const focusCellId = variable.declaredBy[0];
+  return focusCellId;


question: Potential issue with multiple declarations.

The getCellIdOfDefinition function only returns the first cell ID where the variable is declared. If a variable is declared in multiple cells, this might not be the desired behavior. Consider handling multiple declarations or documenting this behavior.

I think this is probably fine as multiple definitions, while supported, is not a state the notebook should stay in? So the first definition is probably fine?

yea this is ok - you can define something twice, but its an error and going to the first one will be ok

sourcery-ai · 2024-05-19T12:09:06Z

frontend/src/components/editor/cell/cell-context-menu.tsx

+    {
+      label: "Go to Definition",
+      icon: <SearchIcon size={13} strokeWidth={1.5} />,
+      handle: () => {
+        const { getEditorView } = props;
+        const editorView = getEditorView();
+        if (editorView) {
+          const variableName = getWordUnderCursor(editorView?.state);
+          const focusCellId = getCellIdOfDefinition(variables, variableName);
+          if (focusCellId) {


suggestion (bug_risk): Error handling for undefined variable.

In the 'Go to Definition' action, if getWordUnderCursor returns an undefined or empty string, the function should handle this case gracefully, possibly with a user notification.

I think that getWordUnderCursor should always return some string. And getCellIdofDefinition will only return a value if the variable is defined in the variables array so I think this should not cause errors, but not 100% sure about this

i can play around with some cases - vscode does always show you Go To Definition, but does no-op when you don't have a work under it

sourcery-ai · 2024-05-19T12:09:06Z

frontend/src/core/cells/scrollCellIntoView.ts

+    } else if (codeFocus) {
+      goToDefinition(editor, codeFocus);


question: Clarify the behavior of goToDefinition.

The goToDefinition function is called if codeFocus is neither '<>' nor '<>'. Ensure that goToDefinition handles all edge cases and that its behavior is well-documented.

sourcery-ai · 2024-05-19T12:09:06Z

frontend/src/core/codemirror/cells/extensions.ts

@@ -38,6 +39,7 @@
    createBelow,
    moveUp,
    moveDown,
+    focusByVariableName,


issue (complexity): Consider keeping the splitCell functionality and modularizing keybinding logic.

The new code introduces increased complexity and removes existing functionality. Here are the main points:

Increased Interface Complexity: The MovementCallbacks interface now includes an additional method focusByVariableName, which increases the surface area of the interface and makes it harder to maintain.

Removed Functionality: The splitCell functionality has been removed, which might be critical. Removing such functionality without proper context or replacement can lead to incomplete features or broken functionality.

Keybinding Changes: The keybinding for splitCell has been replaced with goToDefinition. This change should be clearly documented and justified. The new keybinding logic adds complexity by introducing a new behavior that developers need to understand and maintain.

Code Duplication: The new keybinding logic for focusByVariableName is similar to the existing keybinding logic, leading to code duplication and making the codebase harder to maintain and more prone to bugs.

To address these issues, consider the following approach:

Keep the splitCell Functionality: Retain the splitCell functionality if it is still needed and add the new functionality without removing the existing one.

Modularize Keybinding Logic: Extract common keybinding logic into reusable functions to reduce duplication.

Here is a revised code snippet:

import { CellId, HTMLCellId } from "@/core/cells/ids"; import { Extension, Prec } from "@codemirror/state"; import { formatKeymapExtension } from "../extensions"; import { CellActions } from "@/core/cells/cells"; import { getEditorCodeAsPython } from "../language/utils"; import { formattingChangeEffect } from "../format"; import { closeCompletion, completionStatus } from "@codemirror/autocomplete"; import { isAtEndOfEditor, isAtStartOfEditor } from "../utils"; export interface MovementCallbacks extends Pick<CellActions, "sendToTop" | "sendToBottom" | "moveToNextCell"> { onRun: () => void; deleteCell: () => void; createAbove: () => void; createBelow: () => void; moveUp: () => void; moveDown: () => void; focusUp: () => void; focusDown: () => void; toggleHideCode: () => boolean; aiCellCompletion: () => boolean; focusByVariableName?: () => void; // Optional if not always needed splitCell?: (params: { cellId: CellId; cursorPos: number }) => void; // Optional if not always needed } /** * Extensions for cell movement */ export function cellMovementBundle( cellId: CellId, callbacks: MovementCallbacks, ): Extension[] { const { onRun, deleteCell, createAbove, createBelow, moveUp, moveDown, focusUp, focusDown, sendToTop, sendToBottom, moveToNextCell, toggleHideCode, aiCellCompletion, focusByVariableName, splitCell, } = callbacks; const hotkeys: KeyBinding[] = [ { key: HOTKEYS.getHotkey("cell.run").key, preventDefault: true, stopPropagation: true, run: (ev) => { onRun(); return true; }, }, { key: HOTKEYS.getHotkey("cell.goToDefinition").key, preventDefault: true, stopPropagation: true, run: () => { if (focusByVariableName) { focusByVariableName(); } return true; }, }, { key: HOTKEYS.getHotkey("cell.splitCell").key, preventDefault: true, stopPropagation: true, run: (ev) => { if (splitCell) { const cursorPos = ev.state.selection.main.head; splitCell({ cellId, cursorPos }); requestAnimationFrame(() => { ev.contentDOM.blur(); moveToNextCell({ cellId, before: false }); // focus new cell }); } return true; }, }, ]; // Highest priority so that we can override the default keymap return [Prec.highest(keymap.of(hotkeys))]; } export interface CodeCallbacks { updateCellCode: CellActions["updateCellCode"]; } /** * Extensions for cell code editing */ export function cellCodeEditingBundle( cellId: CellId, callbacks: CodeCallbacks, ): Extension[] { const { updateCellCode } = callbacks; // Add your code editing extensions here return []; }

This approach maintains the existing functionality while adding the new feature in a less complex and more maintainable way.

mscolnick · 2024-05-19T14:57:03Z

frontend/src/core/hotkeys/hotkeys.ts

+  "cell.goToDefinition": {
+    name: "Go to Definition",
+    group: "Navigation",
+    key: "Mod-Shift-,",


vscode's Go To Definition is F12. Maybe we can just keep it like that?

mscolnick · 2024-05-19T14:58:49Z

frontend/src/core/cells/scrollCellIntoView.ts

@@ -14,7 +15,7 @@ export function focusAndScrollCellIntoView({
  cellId: CellId;
  cell: RefObject<CellHandle>;
  config: CellConfig;
-  codeFocus: "top" | "bottom" | undefined;
+  codeFocus: string | undefined;


we can still keep this as codeFocus: "<<top>>" | "<<bottom>>" | string | undefined; which can help document the signature

actually might be better to just create a new field for variableName - otherwise we lose type-safety downstream

mscolnick · 2024-05-19T15:01:32Z

frontend/src/core/cells/cells.ts

@@ -231,6 +231,25 @@ const {
      scrollKey: null,
    };
  },
+  focusCellAtDefinition: (


Even though its kinda similar to focusCell, I think we can make this a function outside of the reducer in core/codemirror/go-to-definition. You can access the store by store.get(atomName).

mscolnick · 2024-05-19T15:03:02Z

frontend/src/components/editor/cell/code/cell-editor.tsx

@@ -410,3 +434,29 @@ const CellCodeMirrorEditor = React.forwardRef(
 CellCodeMirrorEditor.displayName = "CellCodeMirrorEditor";

 export const CellEditor = memo(CellEditorInternal);
+
+export const getWordUnderCursor = (state: EditorState) => {


i think we can move all this logic to another file core/codemirror/go-to-definition.ts.
we should be able to just expose one function goToDefinition(view: EditorView)

mscolnick · 2024-05-19T15:06:21Z

frontend/src/components/editor/cell/cell-context-menu.tsx

+    {
+      label: "Go to Definition",
+      icon: <SearchIcon size={13} strokeWidth={1.5} />,
+      handle: () => {
+        const { getEditorView } = props;
+        const editorView = getEditorView();
+        if (editorView) {
+          const variableName = getWordUnderCursor(editorView?.state);
+          const focusCellId = getCellIdOfDefinition(variables, variableName);
+          if (focusCellId) {


i can play around with some cases - vscode does always show you Go To Definition, but does no-op when you don't have a work under it

mscolnick · 2024-05-20T01:56:47Z

frontend/src/core/codemirror/cells/extensions.ts

@@ -52,6 +52,7 @@ export function cellMovementBundle(
    toggleHideCode,
    aiCellCompletion,
  } = callbacks;
+  const variables = useVariables()


you can use store.get(variables) and can likely push this into goToDefinition

Will add, thanks for the tip

mscolnick

nice contrib!

github-actions · 2024-05-20T22:18:21Z

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.6.1-dev16

…nu (#1411)" This reverts commit d2a253f.

robmck1995 added 4 commits May 16, 2024 17:10

Goes to the right cell but not the pos within the cell

43c0782

Add focus Cell at definition

9b89610

Add Go To Definition to the context menu

9226dd3

fmt

6bb81b0

sourcery-ai bot reviewed May 19, 2024

View reviewed changes

vercel bot deployed to Preview – marimo-storybook May 19, 2024 12:09 View deployment

vercel bot deployed to Preview – marimo-docs May 19, 2024 12:10 View deployment

Merge branch 'main' into go-to-def

66ccf90

vercel bot deployed to Preview – marimo-storybook May 19, 2024 13:09 View deployment

vercel bot deployed to Preview – marimo-docs May 19, 2024 13:11 View deployment

marimo-team deleted a comment from sourcery-ai bot May 19, 2024

mscolnick reviewed May 19, 2024

View reviewed changes

robmck1995 added 2 commits May 19, 2024 20:15

Tests

d2e1d1b

Refactor

46e2cd2

vercel bot had a problem deploying to Preview – marimo-storybook May 20, 2024 00:16 Failure

vercel bot deployed to Preview – marimo-docs May 20, 2024 00:18 View deployment

robmck1995 added 2 commits May 19, 2024 20:23

Fixes

cd7f971

Remove unnecessary functions

d67dd0d

vercel bot deployed to Preview – marimo-docs May 20, 2024 00:28 View deployment

Additional fixes

9c7d2f3

vercel bot deployed to Preview – marimo-storybook May 20, 2024 00:31 View deployment

Lint fixes

b46acdb

vercel bot deployed to Preview – marimo-docs May 20, 2024 00:37 View deployment

vercel bot deployed to Preview – marimo-storybook May 20, 2024 00:39 View deployment

Remove the call through

af3dc51

vercel bot deployed to Preview – marimo-storybook May 20, 2024 00:43 View deployment

vercel bot deployed to Preview – marimo-docs May 20, 2024 00:45 View deployment

mscolnick reviewed May 20, 2024

View reviewed changes

robmck1995 added 4 commits May 20, 2024 17:58

Get the variables from the store

005595a

fmt

68bf0e6

Remove useVariables

5f9ca1c

Merge branch 'main' into go-to-def

c9af5b3

vercel bot deployed to Preview – marimo-storybook May 20, 2024 22:05 View deployment

vercel bot deployed to Preview – marimo-docs May 20, 2024 22:06 View deployment

mscolnick approved these changes May 20, 2024

View reviewed changes

mscolnick merged commit d2a253f into marimo-team:main May 20, 2024
27 checks passed

akshayka added a commit that referenced this pull request May 26, 2024

Revert "Go to definition functionality added to hotkey and context me…

43af210

…nu (#1411)" This reverts commit d2a253f.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go to definition functionality added to hotkey and context menu #1411

Go to definition functionality added to hotkey and context menu #1411

robmck1995 commented May 19, 2024

vercel bot commented May 19, 2024 •

edited

sourcery-ai bot left a comment

sourcery-ai bot May 19, 2024

sourcery-ai bot May 19, 2024

robmck1995 May 19, 2024

mscolnick May 19, 2024

sourcery-ai bot May 19, 2024

robmck1995 May 19, 2024

mscolnick May 19, 2024

sourcery-ai bot May 19, 2024

sourcery-ai bot May 19, 2024

mscolnick May 19, 2024

mscolnick May 19, 2024

mscolnick May 19, 2024

mscolnick May 19, 2024

mscolnick May 19, 2024

robmck1995 May 19, 2024

mscolnick May 19, 2024

mscolnick May 20, 2024

robmck1995 May 20, 2024

mscolnick left a comment

github-actions bot commented May 20, 2024

		const focusCellId = variable.declaredBy[0];
		return focusCellId;

Go to definition functionality added to hotkey and context menu #1411

Go to definition functionality added to hotkey and context menu #1411

Conversation

robmck1995 commented May 19, 2024

vercel bot commented May 19, 2024 • edited

sourcery-ai bot left a comment

Choose a reason for hiding this comment

sourcery-ai bot May 19, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourcery-ai bot May 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sourcery-ai bot May 19, 2024

Choose a reason for hiding this comment

sourcery-ai bot May 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscolnick left a comment

Choose a reason for hiding this comment

github-actions bot commented May 20, 2024

vercel bot commented May 19, 2024 •

edited