Right click a feature on the model to center the code pane on it #7447 #7664
Conversation
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
Contributor
rajgandhi1
commented
Jul 2, 2025
rajgandhi1
commented
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
* fix: we really cannot do this * fix: limiting footprint
…e' mode * add ability to select deoffset plane where starting a new sketch * use selectDefaultSketchPlane * refactor: remove some duplication * warning cleanups * feature tree items selectable depedngin on no face sketch mode * lint * fix small jump because of border:none when going into and back from 'No face sketch' mode * grey out items other than offset planes in 'No face sketch' mode * start sketching on plane in context menu * sketch on offset plane with context menu * add ability to right click on default plane and start sketch on it * default planes in feature tree should be selectable because of right click context menu * add right click Start sketch option for selected plane on the canvas * selectDefaultSketchPlane returns error now * circular deps * move select functions to lib/selections.ts to avoid circular deps * add test for clicking on feature tree after starting a new sketch * graphite suggestion * fix bug of not being able to create offset plane using another offset plane with command bar * add ability to select default plane on feature when going through the Offset plane command bar flow
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
src/components/ViewControlMenu.tsx
Outdated
Comment on lines
34
to
63
| // Check if there's a valid selection with source range for "View KCL source code" | ||
| const hasValidSelection = useMemo(() => { | ||
| const selections = modelingState.context.selectionRanges.graphSelections | ||
| return ( | ||
| selections.length > 0 && | ||
| selections.some((selection) => { | ||
| return ( | ||
| selection.codeRef?.range && | ||
| selection.codeRef.range[0] !== undefined && | ||
| selection.codeRef.range[1] !== undefined | ||
| ) | ||
| }) | ||
| ) | ||
| }, [modelingState.context.selectionRanges.graphSelections]) | ||
|
|
||
|
|
||
| // Check if there's a valid selection with source range for "View KCL source code" | ||
| const hasValidSelection = useMemo(() => { | ||
| const selections = modelingState.context.selectionRanges.graphSelections | ||
| return ( | ||
| selections.length > 0 && | ||
| selections.some((selection) => { | ||
| return ( | ||
| selection.codeRef?.range && | ||
| selection.codeRef.range[0] !== undefined && | ||
| selection.codeRef.range[1] !== undefined | ||
| ) | ||
| }) | ||
| ) | ||
| }, [modelingState.context.selectionRanges.graphSelections]) |
There was a problem hiding this comment.
There appears to be a duplicate implementation of the hasValidSelection function in the useViewControlMenuItems hook. The same code block is defined twice (lines 35-47 and 51-63), with identical logic checking for valid selections with source ranges. This duplication will cause the second definition to override the first one.
Consider removing one of these duplicate implementations to avoid potential confusion and maintain clean code.
Suggested change
| // Check if there's a valid selection with source range for "View KCL source code" | |
| const hasValidSelection = useMemo(() => { | |
| const selections = modelingState.context.selectionRanges.graphSelections | |
| return ( | |
| selections.length > 0 && | |
| selections.some((selection) => { | |
| return ( | |
| selection.codeRef?.range && | |
| selection.codeRef.range[0] !== undefined && | |
| selection.codeRef.range[1] !== undefined | |
| ) | |
| }) | |
| ) | |
| }, [modelingState.context.selectionRanges.graphSelections]) | |
| // Check if there's a valid selection with source range for "View KCL source code" | |
| const hasValidSelection = useMemo(() => { | |
| const selections = modelingState.context.selectionRanges.graphSelections | |
| return ( | |
| selections.length > 0 && | |
| selections.some((selection) => { | |
| return ( | |
| selection.codeRef?.range && | |
| selection.codeRef.range[0] !== undefined && | |
| selection.codeRef.range[1] !== undefined | |
| ) | |
| }) | |
| ) | |
| }, [modelingState.context.selectionRanges.graphSelections]) | |
| // Check if there's a valid selection with source range for "View KCL source code" | |
| const hasValidSelection = useMemo(() => { | |
| const selections = modelingState.context.selectionRanges.graphSelections | |
| return ( | |
| selections.length > 0 && | |
| selections.some((selection) => { | |
| return ( | |
| selection.codeRef?.range && | |
| selection.codeRef.range[0] !== undefined && | |
| selection.codeRef.range[1] !== undefined | |
| ) | |
| }) | |
| ) | |
| }, [modelingState.context.selectionRanges.graphSelections]) | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
src/components/ViewControlMenu.tsx
Outdated
Comment on lines
99
to
204
| sel.codeRef?.range && | ||
| sel.codeRef.range[0] !== undefined && | ||
| sel.codeRef.range[1] !== undefined | ||
| ) | ||
|
|
||
| if (selection?.codeRef?.range) { | ||
| // First, open the code pane if it's not already open | ||
| if (!modelingState.context.store.openPanes.includes('code')) { | ||
| modelingSend({ | ||
| type: 'Set context', | ||
| data: { | ||
| openPanes: [...modelingState.context.store.openPanes, 'code'], | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| // Navigate to the source code location | ||
| modelingSend({ | ||
| type: 'Set selection', | ||
| data: { | ||
| selectionType: 'singleCodeCursor', | ||
| selection: { | ||
| artifact: selection.artifact, | ||
| codeRef: selection.codeRef, | ||
| }, | ||
| scrollIntoView: true, | ||
| }, | ||
| }) | ||
| } | ||
| }} | ||
| disabled={!hasValidSelection} | ||
| > | ||
| View KCL source code | ||
| </ContextMenuItem>, | ||
| <ContextMenuDivider />, | ||
| <ContextMenuItem | ||
| onClick={() => { | ||
| if (selectedPlaneId) { | ||
| sceneInfra.modelingSend({ | ||
| type: 'Enter sketch', | ||
| data: { forceNewSketch: true }, | ||
| }) | ||
|
|
||
| const defaultSketchPlaneSelected = | ||
| selectDefaultSketchPlane(selectedPlaneId) | ||
| if ( | ||
| !err(defaultSketchPlaneSelected) && | ||
| defaultSketchPlaneSelected | ||
| ) { | ||
| return | ||
| } | ||
|
|
||
| const artifact = kclManager.artifactGraph.get(selectedPlaneId) | ||
| void selectOffsetSketchPlane(artifact) | ||
| } | ||
| }} | ||
| disabled={!selectedPlaneId} | ||
| > | ||
| Start sketch on selection | ||
| </ContextMenuItem>, | ||
| <ContextMenuItem | ||
| onClick={() => { | ||
| // Get the first valid selection with a source range | ||
| const selection = | ||
| modelingState.context.selectionRanges.graphSelections.find( | ||
| (sel) => | ||
| sel.codeRef?.range && | ||
| sel.codeRef.range[0] !== undefined && | ||
| sel.codeRef.range[1] !== undefined | ||
| ) | ||
|
|
||
| if (selection?.codeRef?.range) { | ||
| // First, open the code pane if it's not already open | ||
| if (!modelingState.context.store.openPanes.includes('code')) { | ||
| modelingSend({ | ||
| type: 'Set context', | ||
| data: { | ||
| openPanes: [...modelingState.context.store.openPanes, 'code'], | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| // Navigate to the source code location | ||
| modelingSend({ | ||
| type: 'Set selection', | ||
| data: { | ||
| selectionType: 'singleCodeCursor', | ||
| selection: { | ||
| artifact: selection.artifact, | ||
| codeRef: selection.codeRef, | ||
| }, | ||
| scrollIntoView: true, | ||
| }, | ||
| }) | ||
| } | ||
| }} | ||
| disabled={!hasValidSelection} | ||
| > | ||
| View KCL source code | ||
| </ContextMenuItem>, |
There was a problem hiding this comment.
There appears to be a duplicate implementation of the "View KCL source code" context menu item in the useViewControlMenuItems hook. The same functionality is implemented twice (once at lines 99-138 and again at lines 165-204), which would result in two identical menu items appearing in the context menu.
Please remove one of these duplicate implementations to avoid confusing users with redundant options.
Suggested change
| <ContextMenuItem | |
| onClick={() => { | |
| // Get the first valid selection with a source range | |
| const selection = | |
| modelingState.context.selectionRanges.graphSelections.find( | |
| (sel) => | |
| sel.codeRef?.range && | |
| sel.codeRef.range[0] !== undefined && | |
| sel.codeRef.range[1] !== undefined | |
| ) | |
| if (selection?.codeRef?.range) { | |
| // First, open the code pane if it's not already open | |
| if (!modelingState.context.store.openPanes.includes('code')) { | |
| modelingSend({ | |
| type: 'Set context', | |
| data: { | |
| openPanes: [...modelingState.context.store.openPanes, 'code'], | |
| }, | |
| }) | |
| } | |
| // Navigate to the source code location | |
| modelingSend({ | |
| type: 'Set selection', | |
| data: { | |
| selectionType: 'singleCodeCursor', | |
| selection: { | |
| artifact: selection.artifact, | |
| codeRef: selection.codeRef, | |
| }, | |
| scrollIntoView: true, | |
| }, | |
| }) | |
| } | |
| }} | |
| disabled={!hasValidSelection} | |
| > | |
| View KCL source code | |
| </ContextMenuItem>, | |
| <ContextMenuDivider />, | |
| <ContextMenuItem | |
| onClick={() => { | |
| if (selectedPlaneId) { | |
| sceneInfra.modelingSend({ | |
| type: 'Enter sketch', | |
| data: { forceNewSketch: true }, | |
| }) | |
| const defaultSketchPlaneSelected = | |
| selectDefaultSketchPlane(selectedPlaneId) | |
| if ( | |
| !err(defaultSketchPlaneSelected) && | |
| defaultSketchPlaneSelected | |
| ) { | |
| return | |
| } | |
| const artifact = kclManager.artifactGraph.get(selectedPlaneId) | |
| void selectOffsetSketchPlane(artifact) | |
| } | |
| }} | |
| disabled={!selectedPlaneId} | |
| > | |
| Start sketch on selection | |
| </ContextMenuItem>, | |
| <ContextMenuItem | |
| onClick={() => { | |
| // Get the first valid selection with a source range | |
| const selection = | |
| modelingState.context.selectionRanges.graphSelections.find( | |
| (sel) => | |
| sel.codeRef?.range && | |
| sel.codeRef.range[0] !== undefined && | |
| sel.codeRef.range[1] !== undefined | |
| ) | |
| if (selection?.codeRef?.range) { | |
| // First, open the code pane if it's not already open | |
| if (!modelingState.context.store.openPanes.includes('code')) { | |
| modelingSend({ | |
| type: 'Set context', | |
| data: { | |
| openPanes: [...modelingState.context.store.openPanes, 'code'], | |
| }, | |
| }) | |
| } | |
| // Navigate to the source code location | |
| modelingSend({ | |
| type: 'Set selection', | |
| data: { | |
| selectionType: 'singleCodeCursor', | |
| selection: { | |
| artifact: selection.artifact, | |
| codeRef: selection.codeRef, | |
| }, | |
| scrollIntoView: true, | |
| }, | |
| }) | |
| } | |
| }} | |
| disabled={!hasValidSelection} | |
| > | |
| View KCL source code | |
| </ContextMenuItem>, | |
| <ContextMenuItem | |
| onClick={() => { | |
| // Get the first valid selection with a source range | |
| const selection = | |
| modelingState.context.selectionRanges.graphSelections.find( | |
| (sel) => | |
| sel.codeRef?.range && | |
| sel.codeRef.range[0] !== undefined && | |
| sel.codeRef.range[1] !== undefined | |
| ) | |
| if (selection?.codeRef?.range) { | |
| // First, open the code pane if it's not already open | |
| if (!modelingState.context.store.openPanes.includes('code')) { | |
| modelingSend({ | |
| type: 'Set context', | |
| data: { | |
| openPanes: [...modelingState.context.store.openPanes, 'code'], | |
| }, | |
| }) | |
| } | |
| // Navigate to the source code location | |
| modelingSend({ | |
| type: 'Set selection', | |
| data: { | |
| selectionType: 'singleCodeCursor', | |
| selection: { | |
| artifact: selection.artifact, | |
| codeRef: selection.codeRef, | |
| }, | |
| scrollIntoView: true, | |
| }, | |
| }) | |
| } | |
| }} | |
| disabled={!hasValidSelection} | |
| > | |
| View KCL source code | |
| </ContextMenuItem>, | |
| <ContextMenuDivider />, | |
| <ContextMenuItem | |
| onClick={() => { | |
| if (selectedPlaneId) { | |
| sceneInfra.modelingSend({ | |
| type: 'Enter sketch', | |
| data: { forceNewSketch: true }, | |
| }) | |
| const defaultSketchPlaneSelected = | |
| selectDefaultSketchPlane(selectedPlaneId) | |
| if ( | |
| !err(defaultSketchPlaneSelected) && | |
| defaultSketchPlaneSelected | |
| ) { | |
| return | |
| } | |
| const artifact = kclManager.artifactGraph.get(selectedPlaneId) | |
| void selectOffsetSketchPlane(artifact) | |
| } | |
| }} | |
| disabled={!selectedPlaneId} | |
| > | |
| Start sketch on selection | |
| </ContextMenuItem>, |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
@nicboone8 is the implementation correct? Do I need to modify something? |
pierremtb
reviewed
Jul 14, 2025
There was a problem hiding this comment.
Hey @rajgandhi1, thanks a lot for creating this! Was doing some issue triage last week and this would effectively close #3603 I believe.
Did you see the Graphite bot suggestions, incl. the one on code duplication? They seem relevant if you could check them out.
I would also like to see a basic playwright test for this as I'm not sure we have other avenues of testing scene selection yet. Likely a straightforward right-click in the middle of the scene to avoid aboslute pixel coords. I can help recreate the PR and add the test, it's been in my todo for a bit but haven't had the chance to get to it yet, sorry!
|
Hey @pierremtb |
|
@pierremtb I made the changes |
pierremtb
requested changes
Jul 18, 2025
| test.describe('View KCL source code functionality', () => { | ||
| test.setTimeout(90_000) | ||
|
|
||
| test('@web "View KCL source code" handles edge cases', async ({ |
There was a problem hiding this comment.
Hey! Thanks for adding this @rajgandhi1
Two things here:
- I'd want to find a way not to add a new test with absolute coordinates as we're trying to slowly replace them with other approaches that are more robust to UI changes. For instance here, maybe finding the size of the scene (not the window, the video element really) and dividing that by two would probably work?
- The three cases are good to have, but it's really the case where we can click and that checks on the highlight that I think we're missing here?
Those are things I'd be happy to help on as well, just lacking time at the moment but will likely be able to help soon. The next step will be for me to recreate this PR from your branch so we can get the tests to green
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.