Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions src/composables/graph/useSelectionState.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { createSharedComposable, whenever } from '@vueuse/core'
import { storeToRefs } from 'pinia'
import { computed } from 'vue'

Expand All @@ -22,7 +23,7 @@ export interface NodeSelectionState {
* Centralized computed selection state + shared helper actions to avoid duplication
* between selection toolbox, context menus, and other UI affordances.
*/
export function useSelectionState() {
function useSelectionStateInternal() {
const canvasStore = useCanvasStore()
const nodeDefStore = useNodeDefStore()
const sidebarTabStore = useSidebarTabStore()
Expand Down Expand Up @@ -94,6 +95,17 @@ export function useSelectionState() {
computeSelectionStatesFromNodes(selectedNodes.value)
)

// Keep help panel in sync when it is open and the user changes selection.
whenever(
() => (nodeHelpStore.isHelpOpen ? nodeDef.value : null),
(def) => {
const currentHelpNode = nodeHelpStore.currentHelpNode
if (currentHelpNode?.nodePath === def.nodePath) return

nodeHelpStore.openHelp(def)
}
)
Comment on lines +99 to +107
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The watcher callback can receive null as the def parameter when help is open and the selection changes to multiple nodes or no nodes (since nodeDef.value returns null in those cases). This would result in calling nodeHelpStore.openHelp(null), which is likely incorrect.

Add a null check before calling openHelp:

whenever(
  () => (nodeHelpStore.isHelpOpen ? nodeDef.value : null),
  (def) => {
    if (!def) return
    const currentHelpNode = nodeHelpStore.currentHelpNode
    if (currentHelpNode?.nodePath === def.nodePath) return

    nodeHelpStore.openHelp(def)
  }
)

Copilot uses AI. Check for mistakes.

// On-demand computation (non-reactive) so callers can fetch fresh flags
const computeSelectionFlags = (): NodeSelectionState =>
computeSelectionStatesFromNodes(selectedNodes.value)
Expand All @@ -105,12 +117,11 @@ export function useSelectionState() {

const isSidebarActive =
sidebarTabStore.activeSidebarTabId === nodeLibraryTabId
const currentHelpNode: any = nodeHelpStore.currentHelpNode
const currentHelpNode = nodeHelpStore.currentHelpNode
const isSameNodeHelpOpen =
isSidebarActive &&
nodeHelpStore.isHelpOpen &&
currentHelpNode &&
currentHelpNode.nodePath === def.nodePath
currentHelpNode?.nodePath === def.nodePath

if (isSameNodeHelpOpen) {
nodeHelpStore.closeHelp()
Expand Down Expand Up @@ -141,3 +152,7 @@ export function useSelectionState() {
computeSelectionFlags
}
}

export const useSelectionState = createSharedComposable(
useSelectionStateInternal
)
145 changes: 122 additions & 23 deletions tests-ui/tests/composables/graph/useSelectionState.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { flushPromises, mount } from '@vue/test-utils'
import type { VueWrapper } from '@vue/test-utils'
import { createPinia, setActivePinia } from 'pinia'
import { beforeEach, describe, expect, test, vi } from 'vitest'
import { type Ref, ref } from 'vue'
import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'
import { ref } from 'vue'
import type { Ref } from 'vue'

import { useSelectionState } from '@/composables/graph/useSelectionState'
import { useNodeLibrarySidebarTab } from '@/composables/sidebarTabs/useNodeLibrarySidebarTab'
Expand Down Expand Up @@ -78,9 +81,50 @@ const mockComment = { type: 'comment', isNode: false }
const mockConnection = { type: 'connection', isNode: false }

describe('useSelectionState', () => {
const mountedWrappers: VueWrapper[] = []
// Mock store instances
let mockSelectedItems: Ref<MockedItem[]>

const mountSelectionStateComposable = () => {
let selectionState: ReturnType<typeof useSelectionState>
const wrapper = mount({
template: '<div />',
setup() {
selectionState = useSelectionState()
return {}
}
})
mountedWrappers.push(wrapper)
return { selectionState: selectionState! }
}

const mountHelpSyncHarness = () => {
const nodeA = createTestNode({ type: 'NodeA' })
const nodeB = createTestNode({ type: 'NodeB' })

const wrapper = mount({
template: `
<div>
<button data-test="select-a" @click="select(nodeA)">A</button>
<button data-test="select-b" @click="select(nodeB)">B</button>
</div>
`,
setup() {
const select = (node: TestNode) => {
mockSelectedItems.value = [node]
}

useSelectionState()

return { select, nodeA, nodeB }
}
})

mountedWrappers.push(wrapper)

return { wrapper, nodeA, nodeB }
}

beforeEach(() => {
vi.clearAllMocks()
setActivePinia(createPinia())
Expand Down Expand Up @@ -181,10 +225,14 @@ describe('useSelectionState', () => {
)
})

afterEach(() => {
mountedWrappers.splice(0).forEach((wrapper) => wrapper.unmount())
})

describe('Selection Detection', () => {
test('should return false when nothing selected', () => {
const { hasAnySelection } = useSelectionState()
expect(hasAnySelection.value).toBe(false)
const { selectionState } = mountSelectionStateComposable()
expect(selectionState.hasAnySelection.value).toBe(false)
})

test('should return true when items selected', () => {
Expand All @@ -193,8 +241,8 @@ describe('useSelectionState', () => {
const node2 = createTestNode()
mockSelectedItems.value = [node1, node2]

const { hasAnySelection } = useSelectionState()
expect(hasAnySelection.value).toBe(true)
const { selectionState } = mountSelectionStateComposable()
expect(selectionState.hasAnySelection.value).toBe(true)
})
})

Expand All @@ -204,9 +252,9 @@ describe('useSelectionState', () => {
const graphNode = createTestNode()
mockSelectedItems.value = [graphNode, mockComment, mockConnection]

const { selectedNodes } = useSelectionState()
expect(selectedNodes.value).toHaveLength(1)
expect(selectedNodes.value[0]).toEqual(graphNode)
const { selectionState } = mountSelectionStateComposable()
expect(selectionState.selectedNodes.value).toHaveLength(1)
expect(selectionState.selectedNodes.value[0]).toEqual(graphNode)
})
})

Expand All @@ -216,8 +264,8 @@ describe('useSelectionState', () => {
const bypassedNode = createTestNode({ mode: LGraphEventMode.BYPASS })
mockSelectedItems.value = [bypassedNode]

const { selectedNodes } = useSelectionState()
const isBypassed = selectedNodes.value.some(
const { selectionState } = mountSelectionStateComposable()
const isBypassed = selectionState.selectedNodes.value.some(
(n) => n.mode === LGraphEventMode.BYPASS
)
expect(isBypassed).toBe(true)
Expand All @@ -229,42 +277,93 @@ describe('useSelectionState', () => {
const collapsedNode = createTestNode({ flags: { collapsed: true } })
mockSelectedItems.value = [pinnedNode, collapsedNode]

const { selectedNodes } = useSelectionState()
const isPinned = selectedNodes.value.some((n) => n.pinned === true)
const isCollapsed = selectedNodes.value.some(
const { selectionState } = mountSelectionStateComposable()
const isPinned = selectionState.selectedNodes.value.some(
(n) => n.pinned === true
)
const isCollapsed = selectionState.selectedNodes.value.some(
(n) => n.flags?.collapsed === true
)
const isBypassed = selectedNodes.value.some(
const isBypassed = selectionState.selectedNodes.value.some(
(n) => n.mode === LGraphEventMode.BYPASS
)
expect(isPinned).toBe(true)
expect(isCollapsed).toBe(true)
expect(isBypassed).toBe(false)
})

test('should provide non-reactive state computation', () => {
test('should provide non-reactive state computation', async () => {
// Update the mock data before creating the composable
const node = createTestNode({ pinned: true })
mockSelectedItems.value = [node]

const { selectedNodes } = useSelectionState()
const isPinned = selectedNodes.value.some((n) => n.pinned === true)
const isCollapsed = selectedNodes.value.some(
const { selectionState } = mountSelectionStateComposable()
const isPinned = selectionState.selectedNodes.value.some(
(n) => n.pinned === true
)
const isCollapsed = selectionState.selectedNodes.value.some(
(n) => n.flags?.collapsed === true
)
const isBypassed = selectedNodes.value.some(
const isBypassed = selectionState.selectedNodes.value.some(
(n) => n.mode === LGraphEventMode.BYPASS
)

expect(isPinned).toBe(true)
expect(isCollapsed).toBe(false)
expect(isBypassed).toBe(false)

// Test with empty selection using new composable instance
// Test with empty selection using updated selection
mockSelectedItems.value = []
const { selectedNodes: newSelectedNodes } = useSelectionState()
const newIsPinned = newSelectedNodes.value.some((n) => n.pinned === true)
await flushPromises()

const newIsPinned = selectionState.selectedNodes.value.some(
(n) => n.pinned === true
)
expect(newIsPinned).toBe(false)
})
})

describe('Help Sync', () => {
beforeEach(() => {
const nodeDefStore = useNodeDefStore() as any
nodeDefStore.fromLGraphNode.mockImplementation((node: TestNode) => ({
nodePath: node.type
}))
})

test('opens help for newly selected node when help is open', async () => {
const nodeHelpStore = useNodeHelpStore() as any
nodeHelpStore.isHelpOpen = true
nodeHelpStore.currentHelpNode = { nodePath: 'NodeA' }

const { wrapper } = mountHelpSyncHarness()

await wrapper.find('[data-test="select-a"]').trigger('click')
await wrapper.find('[data-test="select-b"]').trigger('click')
await flushPromises()

expect(nodeHelpStore.openHelp).toHaveBeenCalledWith({
nodePath: 'NodeB'
})
})

test('does not reopen help when selection is unchanged or closed', async () => {
const nodeHelpStore = useNodeHelpStore() as any

const { wrapper } = mountHelpSyncHarness()

// Help closed -> no call
nodeHelpStore.isHelpOpen = false
await wrapper.find('[data-test="select-a"]').trigger('click')
await flushPromises()
expect(nodeHelpStore.openHelp).not.toHaveBeenCalled()

// Help open but same node -> no call
nodeHelpStore.isHelpOpen = true
nodeHelpStore.currentHelpNode = { nodePath: 'NodeA' }
await wrapper.find('[data-test="select-a"]').trigger('click')
await flushPromises()
expect(nodeHelpStore.openHelp).not.toHaveBeenCalled()
})
Comment on lines +350 to +367
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Major: Test logic doesn't properly verify the guard condition.

The second scenario (lines 362-366) doesn't properly test the guard condition. It clicks select-a twice without changing the selection in between (the selection is already [nodeA] from line 357). Since the selection doesn't change, nodeDef doesn't change, so the watcher won't fire at all. This tests that the watcher doesn't fire on unchanged selection, not that the guard prevents redundant openHelp calls.

To properly test the guard condition, the selection should change and then return to the same node:

   test('does not reopen help when selection is unchanged or closed', async () => {
     const nodeHelpStore = useNodeHelpStore() as any
 
     const { wrapper } = mountHelpSyncHarness()
 
     // Help closed -> no call
-    nodeHelpStore.isHelpOpen = false
+    mockIsHelpOpen.value = false
     await wrapper.find('[data-test="select-a"]').trigger('click')
     await flushPromises()
     expect(nodeHelpStore.openHelp).not.toHaveBeenCalled()
 
-    // Help open but same node -> no call
-    nodeHelpStore.isHelpOpen = true
-    nodeHelpStore.currentHelpNode = { nodePath: 'NodeA' }
+    // Help open, select different node, then reselect same node -> no call for reselection
+    mockIsHelpOpen.value = true
+    mockCurrentHelpNode.value = { nodePath: 'NodeA' }
     await wrapper.find('[data-test="select-a"]').trigger('click')
+    await flushPromises()
+    
+    // Clear previous calls
+    nodeHelpStore.openHelp.mockClear()
+    
+    // Select different node, then back to NodeA - guard should prevent second call
+    await wrapper.find('[data-test="select-b"]').trigger('click')
+    await flushPromises()
+    mockCurrentHelpNode.value = { nodePath: 'NodeB' }
+    
+    await wrapper.find('[data-test="select-a"]').trigger('click')
     await flushPromises()
     expect(nodeHelpStore.openHelp).not.toHaveBeenCalled()
   })

Committable suggestion skipped: line range outside the PR's diff.

})
Comment on lines +326 to +368
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

The test coverage for the help sync feature is incomplete. It doesn't test the case where help is open and the user selects multiple nodes or deselects all nodes, which would cause nodeDef.value to become null. This edge case should be covered to ensure the watcher handles it correctly (e.g., not calling openHelp(null) or potentially closing the help panel).

Copilot uses AI. Check for mistakes.
})
Loading