-
Notifications
You must be signed in to change notification settings - Fork 420
Allow graph navigation by browser forward/backward #6811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 7 commits
82735d9
7b283a8
a23f8d1
a460b34
0d22b34
59e9bda
eb362ab
a87f4c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,17 @@ | ||
| import QuickLRU from '@alloc/quick-lru' | ||
| import { defineStore } from 'pinia' | ||
| import { computed, ref, shallowRef, watch } from 'vue' | ||
| import { useRouteHash } from '@vueuse/router' | ||
|
|
||
| import type { DragAndScaleState } from '@/lib/litegraph/src/DragAndScale' | ||
| import type { Subgraph } from '@/lib/litegraph/src/litegraph' | ||
| import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore' | ||
| import { useWorkflowService } from '@/platform/workflow/core/services/workflowService' | ||
| import { useCanvasStore } from '@/renderer/core/canvas/canvasStore' | ||
| import router from '@/router' | ||
| import { app } from '@/scripts/app' | ||
| import { findSubgraphPathById } from '@/utils/graphTraversalUtil' | ||
| import { isNonNullish } from '@/utils/typeGuardUtil' | ||
| import { isNonNullish, isSubgraph } from '@/utils/typeGuardUtil' | ||
|
|
||
| /** | ||
| * Stores the current subgraph navigation state; a stack representing subgraph | ||
|
|
@@ -38,8 +41,6 @@ export const useSubgraphNavigationStore = defineStore( | |
| */ | ||
| const getCurrentRootGraphId = () => { | ||
| const canvas = canvasStore.getCanvas() | ||
| if (!canvas) return 'root' | ||
|
|
||
| return canvas.graph?.rootGraph?.id ?? 'root' | ||
| } | ||
|
|
||
|
|
@@ -157,6 +158,72 @@ export const useSubgraphNavigationStore = defineStore( | |
| onNavigated(newValue, oldValue) | ||
| } | ||
| ) | ||
| const routeHash = useRouteHash() | ||
| let blockHashUpdate = false | ||
| let initialLoad = true | ||
|
|
||
| //Allow navigation with forward/back buttons | ||
| //TODO: Extend for dialogues? | ||
| //TODO: force update widget.promoted | ||
| async function navigateToHash(newHash: string | undefined | null) { | ||
| const root = app.graph | ||
| const locatorId = newHash?.slice(1) ?? root.id | ||
| const canvas = canvasStore.getCanvas() | ||
| if (canvas.graph?.id === locatorId) return | ||
| const targetGraph = | ||
| (locatorId || root.id) !== root.id | ||
| ? root.subgraphs.get(locatorId) | ||
| : root | ||
| if (targetGraph) return canvas.setGraph(targetGraph) | ||
|
|
||
| //Search all open workflows | ||
| for (const workflow of workflowStore.openWorkflows) { | ||
| const { activeState } = workflow | ||
| if (!activeState) continue | ||
| const subgraphs = activeState.definitions?.subgraphs ?? [] | ||
| for (const graph of [activeState, ...subgraphs]) { | ||
| if (graph.id !== locatorId) continue | ||
| //This will trigger a navigation, which can break forward history | ||
| try { | ||
| blockHashUpdate = true | ||
| await useWorkflowService().openWorkflow(workflow) | ||
| } finally { | ||
| blockHashUpdate = false | ||
| } | ||
| const targetGraph = | ||
| app.graph.id === locatorId | ||
| ? app.graph | ||
| : app.graph.subgraphs.get(locatorId) | ||
| if (!targetGraph) { | ||
| console.error('subgraph poofed after load?') | ||
| return | ||
| } | ||
|
|
||
| return canvas.setGraph(targetGraph) | ||
| } | ||
| } | ||
| } | ||
|
Comment on lines
+168
to
+205
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainScript executed: #!/bin/bash
# Search for setGraph method definition to check if it's async
rg -n "setGraph\s*\(" --type ts --type tsx -A 5 | head -100Repository: Comfy-Org/ComfyUI_frontend <shell_result> stderr: </shell_result> Script executed: #!/bin/bash
# Find canvas-related files
fd "canvas" --type f | grep -E "\.(ts|tsx|js)$" | head -20Repository: Comfy-Org/ComfyUI_frontend <shell_result> stdout: </shell_result> Script executed: #!/bin/bash
# Get the file line count and read the specific function
wc -l src/stores/subgraphNavigationStore.ts
echo "---"
sed -n '168,205p' src/stores/subgraphNavigationStore.tsRepository: Comfy-Org/ComfyUI_frontend <shell_result> stdout: </shell_result> Script executed: #!/bin/bash
# Search for setGraph implementation
ast-grep --pattern $'setGraph($$$) {
$$$
}'Repository: Comfy-Org/ComfyUI_frontend <shell_result> </shell_result> Script executed: #!/bin/bash
# Alternative search using rg with context
rg "setGraph" -A 10 --type ts --type tsx | grep -v node_modules | head -150Repository: Comfy-Org/ComfyUI_frontend <shell_result> stderr: </shell_result> Script executed: #!/bin/bash
# Look for canvas store to understand setGraph
fd -e ts -e tsx | xargs rg -l "setGraph" | head -10Repository: Comfy-Org/ComfyUI_frontend <shell_result> stdout: </shell_result> Script executed: #!/bin/bash
# Search in canvasStore.ts for setGraph method
cat -n src/renderer/core/canvas/canvasStore.ts | grep -A 20 "setGraph"Repository: Comfy-Org/ComfyUI_frontend <shell_result> </shell_result> Script executed: #!/bin/bash
# Also check LGraphCanvas for setGraph
cat -n src/lib/litegraph/src/LGraphCanvas.ts | grep -A 15 "setGraph"Repository: Comfy-Org/ComfyUI_frontend <shell_result> stdout: </shell_result> Script executed: #!/bin/bash
# Get the full definition by looking at canvasStore
head -100 src/renderer/core/canvas/canvasStore.tsRepository: Comfy-Org/ComfyUI_frontend <shell_result> stdout: </shell_result> Script executed: #!/bin/bash
# Find setGraph method definition with more context
rg -n "setGraph\s*\(" -A 8 src/renderer/core/canvas/canvasStore.tsRepository: Comfy-Org/ComfyUI_frontend <shell_result> </shell_result> Script executed: #!/bin/bash
# Check the entire canvasStore structure
wc -l src/renderer/core/canvas/canvasStore.ts
head -200 src/renderer/core/canvas/canvasStore.tsRepository: Comfy-Org/ComfyUI_frontend <shell_result> stdout: </shell_result> Script executed: #!/bin/bash
# Find calls to navigateToHash
rg -n "navigateToHash" src/stores/subgraphNavigationStore.ts -B 3 -A 3Repository: Comfy-Org/ComfyUI_frontend <shell_result> stdout: </shell_result> Script executed: #!/bin/bash
# Look for watchers or event listeners that call this function
rg -n "navigateToHash" --type ts -B 2 -A 2 | head -100Repository: Comfy-Org/ComfyUI_frontend <shell_result> stdout: </shell_result> Script executed: #!/bin/bash
# Get more context on the full subgraphNavigationStore to see where navigateToHash is called
head -170 src/stores/subgraphNavigationStore.ts | tail -30Repository: Comfy-Org/ComfyUI_frontend <shell_result> stdout: </shell_result> Script executed: #!/bin/bash
# Get the context around navigateToHash function definition
sed -n '150,240p' src/stores/subgraphNavigationStore.tsRepository: Comfy-Org/ComfyUI_frontend <shell_result> stdout: </shell_result> Remove the first concern; the second concern about error handling needs refinement. After verification, I found critical issues with the original review comment:
The console.error usage is reasonable for this rare edge case, though converting it to throw an error would allow callers to handle it if they awaited the function. 🤖 Prompt for AI Agents |
||
|
|
||
| function updateHash() { | ||
| if (blockHashUpdate) return | ||
| if (!routeHash.value) { | ||
| router.replace('#' + (window.location.hash.slice(1) || app.graph.id)) | ||
| } else if (initialLoad) { | ||
| initialLoad = false | ||
| navigateToHash(routeHash.value) | ||
| const graph = canvasStore.getCanvas().graph | ||
| if (isSubgraph(graph)) workflowStore.activeSubgraph = graph | ||
| return | ||
| } | ||
| const newId = canvasStore.getCanvas().graph?.id ?? '' | ||
| const currentId = window.location.hash.slice(1) | ||
| if (!newId || newId === (currentId || app.graph.id)) return | ||
| router.push('#' + newId) | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| //update navigation hash | ||
| //NOTE: Doesn't apply on workflow load | ||
| watch(() => canvasStore.currentGraph, updateHash) | ||
| watch(routeHash, navigateToHash) | ||
|
|
||
| return { | ||
| activeSubgraph, | ||
|
|
@@ -165,6 +232,7 @@ export const useSubgraphNavigationStore = defineStore( | |
| exportState, | ||
| saveViewport, | ||
| restoreViewport, | ||
| updateHash, | ||
| viewportCache | ||
| } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're doing a lot imperatively here that could be done through Route definitions and use of the Router, but I don't know for sure how easy it would be to transition to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that it would be particularly hard. The navigation to a new graph happens before any event listeners or watched values change. The correct router based approach would mean initiating a navigation to the new address which causes the new graph or subgraph to load.