Conversation
To further eliminate the need to pass the parser object to visualization components, the "focus" path is now computed on the fly. That means that every node computes for itself whether it should be open and whether it should be highlighted. This is possible because the tree adapter can answer the question whether a node contains children in cursor range or not. This commit also changes the behavior for nodes that have been opened via focus: If nodes loose focus, they used to stay open. Now, they will close as long as they haven't been explicitly opened.
See the comment in `focusNodes` for more information.
![callstackai-action[bot]](https://avatars.githubusercontent.com/in/959308?s=60&v=4){kind=small}
| nodes.values().next().value.current.scrollIntoView(); | ||
| } else if (size > 1) { | ||
| const rootRect = root.getBoundingClientRect(); | ||
| const center = (rootRect.y + rootRect.height) / 2 + rootRect.y; |
There was a problem hiding this comment.
🐛 Bug
The calculation of the center on line 37 is incorrect. It adds rootRect.y twice, which will result in an incorrect vertical center. This will lead to inaccurate determination of which element to scroll into view.
Suggested change
| const center = (rootRect.y + rootRect.height) / 2 + rootRect.y; | |
| const center = (rootRect.y + rootRect.height) / 2; |
Comment on lines
+50
to
+87
| switch (currentState) { | ||
| case OPEN_STATES.DEFAULT: | ||
| case OPEN_STATES.CLOSED: | ||
| switch (event) { | ||
| case EVENTS.DEEP_OPEN: | ||
| return OPEN_STATES.DEEP_OPEN; | ||
| case EVENTS.GAIN_FOCUS: | ||
| return OPEN_STATES.FOCUS_OPEN; | ||
| case EVENTS.LOOSE_FOCUS: | ||
| return OPEN_STATES.DEFAULT; | ||
| } | ||
| break; | ||
|
|
||
| _shouldAutoFocus(thisProps, nextProps) { | ||
| const {focusPath: thisFocusPath} = thisProps; | ||
| const {settings: nextSettings, focusPath: nextFocusPath} = nextProps; | ||
| case OPEN_STATES.OPEN: | ||
| switch (event) { | ||
| case EVENTS.DEEP_OPEN: | ||
| return OPEN_STATES.DEEP_OPEN; | ||
| case EVENTS.GAIN_FOCUS: | ||
| case EVENTS.LOOSE_FOCUS: | ||
| return currentState; | ||
| } | ||
| break; | ||
|
|
||
| return ( | ||
| thisFocusPath !== nextFocusPath && | ||
| nextFocusPath.indexOf(nextProps.value) > -1 && | ||
| nextSettings.autofocus | ||
| ); | ||
| } | ||
| case OPEN_STATES.DEEP_OPEN: | ||
| return OPEN_STATES.DEEP_OPEN; | ||
|
|
||
| componentDidMount() { | ||
| if (this.props.settings.autofocus) { | ||
| this._scrollIntoView(); | ||
| } | ||
| case OPEN_STATES.FOCUS_OPEN: | ||
| switch (event) { | ||
| case EVENTS.GAIN_FOCUS: | ||
| return OPEN_STATES.FOCUS_OPEN; | ||
| case EVENTS.LOOSE_FOCUS: | ||
| return OPEN_STATES.DEFAULT; | ||
| case EVENTS.DEEP_OPEN: | ||
| return OPEN_STATES.DEEP_OPEN; | ||
| } | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
🐛 Bug
The function lacks a default case in the switch statements, which could lead to undefined behavior if an unexpected state or event is passed.
Suggested change
| switch (currentState) { | |
| case OPEN_STATES.DEFAULT: | |
| case OPEN_STATES.CLOSED: | |
| switch (event) { | |
| case EVENTS.DEEP_OPEN: | |
| return OPEN_STATES.DEEP_OPEN; | |
| case EVENTS.GAIN_FOCUS: | |
| return OPEN_STATES.FOCUS_OPEN; | |
| case EVENTS.LOOSE_FOCUS: | |
| return OPEN_STATES.DEFAULT; | |
| } | |
| break; | |
| _shouldAutoFocus(thisProps, nextProps) { | |
| const {focusPath: thisFocusPath} = thisProps; | |
| const {settings: nextSettings, focusPath: nextFocusPath} = nextProps; | |
| case OPEN_STATES.OPEN: | |
| switch (event) { | |
| case EVENTS.DEEP_OPEN: | |
| return OPEN_STATES.DEEP_OPEN; | |
| case EVENTS.GAIN_FOCUS: | |
| case EVENTS.LOOSE_FOCUS: | |
| return currentState; | |
| } | |
| break; | |
| return ( | |
| thisFocusPath !== nextFocusPath && | |
| nextFocusPath.indexOf(nextProps.value) > -1 && | |
| nextSettings.autofocus | |
| ); | |
| } | |
| case OPEN_STATES.DEEP_OPEN: | |
| return OPEN_STATES.DEEP_OPEN; | |
| componentDidMount() { | |
| if (this.props.settings.autofocus) { | |
| this._scrollIntoView(); | |
| } | |
| case OPEN_STATES.FOCUS_OPEN: | |
| switch (event) { | |
| case EVENTS.GAIN_FOCUS: | |
| return OPEN_STATES.FOCUS_OPEN; | |
| case EVENTS.LOOSE_FOCUS: | |
| return OPEN_STATES.DEFAULT; | |
| case EVENTS.DEEP_OPEN: | |
| return OPEN_STATES.DEEP_OPEN; | |
| } | |
| break; | |
| } | |
| } | |
| function transition(currentState, event) { | |
| switch (currentState) { | |
| // ... existing cases ... | |
| default: | |
| console.warn(`Unexpected state: ${currentState}`); | |
| return currentState; | |
| } | |
| } |
Comment on lines
+73
to
+74
| case OPEN_STATES.DEEP_OPEN: | ||
| return OPEN_STATES.DEEP_OPEN; |
There was a problem hiding this comment.
🐛 Bug
The OPEN_STATES.DEEP_OPEN case always returns OPEN_STATES.DEEP_OPEN without considering the event. This might lead to unexpected behavior if other events should affect this state.
Suggested change
| case OPEN_STATES.DEEP_OPEN: | |
| return OPEN_STATES.DEEP_OPEN; | |
| case OPEN_STATES.DEEP_OPEN: | |
| switch (event) { | |
| case EVENTS.LOOSE_FOCUS: | |
| return OPEN_STATES.OPEN; | |
| default: | |
| return OPEN_STATES.DEEP_OPEN; | |
| } |
Comment on lines
+104
to
106
| if (!isInRange && wasInRange && ownOpenState === OPEN_STATES.CLOSED) { | ||
| setOwnOpenState(OPEN_STATES.DEFAULT); | ||
| } |
There was a problem hiding this comment.
🐛 Bug
The condition if (!isInRange && wasInRange && ownOpenState === OPEN_STATES.CLOSED) sets ownOpenState to OPEN_STATES.DEFAULT, which might lead to unexpected behavior. If an element is closed and goes out of range, it should probably remain closed rather than reverting to the default state.
Suggested change
| if (!isInRange && wasInRange && ownOpenState === OPEN_STATES.CLOSED) { | |
| setOwnOpenState(OPEN_STATES.DEFAULT); | |
| } | |
| if (!isInRange && wasInRange && ownOpenState === OPEN_STATES.CLOSED) { | |
| // Do nothing, keep the closed state | |
| } |
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
2 participants
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.
✨
Description by Cal
This PR introduces a new feature for handling selected nodes in a tree visualization, including context management and focus handling.
Diagrams of code changes
sequenceDiagram participant User participant PR participant TreeVisualization participant ASTOutput participant TreeAdapter User->>PR: Initiates PR review PR->>TreeVisualization: Adds new functionality TreeVisualization->>ASTOutput: Updates visualization ASTOutput->>TreeAdapter: Enhances tree adaptation TreeAdapter->>TreeVisualization: Provides improved node handling TreeVisualization->>User: Displays enhanced AST visualizationKey Issues
None
Files Changed
File: /website/src/components/visualization/SelectedNodeContext.js
Added context management for selected nodes.File: /website/src/components/visualization/Tree.js
Updated tree component to utilize selected node context and manage focus.File: /website/src/components/visualization/focusNodes.js
Implemented logic to manage focus on tree nodes.File: /website/src/components/visualization/tree/Element.js
Refactored element rendering to support focus and selection.File: /website/src/core/TreeAdapter.js
Enhanced tree adapter to support range and focus management.