Allow graph navigation by browser forward/backward #6811
Conversation
dosubot
bot
added
the
size:M
🎭 Playwright Test Results🕵🏻 No test results found ⏰ Completed at: 11/22/2025, 04:58:48 PM UTC 📊 Test Reports by Browser
🎉 Click on the links above to view detailed test results for each browser configuration. |
🎨 Storybook Build Status❌ Build failed! ⏰ Completed at: 11/22/2025, 04:57:34 PM UTC 🔗 Links
|
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 3.13 MB (baseline 3.13 MB) • 🔴 +1.78 kBMain entry bundles and manifests
Status: 3 added / 3 removed Graph Workspace — 945 kB (baseline 945 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 7.97 kB (baseline 7.97 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 1 added / 1 removed Panels & Settings — 306 kB (baseline 306 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 6 added / 6 removed UI Components — 141 kB (baseline 141 kB) • ⚪ 0 BReusable component library chunks
Status: 6 added / 6 removed Data & Services — 12.5 kB (baseline 12.5 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 2 added / 2 removed Utilities & Hooks — 2.94 kB (baseline 2.94 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 1 added / 1 removed Vendor & Third-Party — 5.32 MB (baseline 5.32 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 3.87 MB (baseline 3.87 MB) • ⚪ 0 BBundles that do not match a named category
Status: 18 added / 18 removed |
| //Allow navigation with forward/back buttons | ||
| //TODO: Extend for dialogues? | ||
| //TODO: force update widget.promoted | ||
| function onHashChange() { |
There was a problem hiding this comment.
How about on page reload? I.e., initial load
|
Would consider using https://vueuse.org/router/useRouteHash/ |
Already got pinged by Alex for this and made the change. Will push once I fix your suggestion of making navigation work on initial load. |
📝 WalkthroughWalkthroughThe changes implement hash-based URL navigation for subgraphs with browser back/forward button support. A new routing dependency is added, the subgraph navigation store is extended with hash synchronization logic and workflow integration, and a WebSocket fixture is updated to exclude URL fragments from connection URLs. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Router
participant SubgraphNav as SubgraphNav Store
participant Workflow as Workflow Service
participant App
rect rgb(200, 220, 255)
Note over Browser,App: Initial Load
App->>SubgraphNav: updateHash()
SubgraphNav->>Router: Get current hash
SubgraphNav->>SubgraphNav: Set initialLoad = true
end
rect rgb(220, 240, 220)
Note over Browser,App: Hash Navigation (back/forward or manual)
Browser->>Router: Hash changes
Router->>SubgraphNav: Route hash updated
SubgraphNav->>SubgraphNav: Check blockHashUpdate guard
alt blockHashUpdate is false
SubgraphNav->>Workflow: Check if subgraph in open workflow
alt Subgraph in open workflow
SubgraphNav->>Workflow: Open corresponding workflow
Note over Workflow: Wait for completion
end
SubgraphNav->>SubgraphNav: Navigate to target subgraph
end
end
rect rgb(240, 220, 220)
Note over Browser,App: Internal Navigation (user clicks)
SubgraphNav->>SubgraphNav: Set blockHashUpdate = true
SubgraphNav->>SubgraphNav: Update navigationStack
SubgraphNav->>SubgraphNav: Call updateHash()
SubgraphNav->>Router: Push/replace hash
SubgraphNav->>SubgraphNav: Set blockHashUpdate = false
end
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/scripts/app.ts (1)
60-60: New subgraphNavigationStore usage introduces a circular import but is currently safeImporting
useSubgraphNavigationStorehere whilesrc/stores/subgraphNavigationStore.tsalready imports{ app }creates a module cycle, but sinceuseSubgraphNavigationStore().updateHash()is only called from theComfyAppinstance method (afterappis constructed), this shouldn’t break initialization.If we ever start using
useSubgraphNavigationStore()at module top‑level elsewhere, it’d be worth revisiting this and possibly extracting hash-sync concerns into a smaller helper to avoid tighter coupling betweenapp.tsand the store. For now, the placement ofupdateHash()afterafterLoadNewGraph()looks appropriate and keeps the URL in sync with the loaded graph.Also applies to: 1309-1310
browser_tests/fixtures/ws.ts (1)
30-36: Hash stripping for WebSocket URL is correct; consider simplifying URL constructionDropping the fragment via
window.location.toString().split('#')[0]avoids producing invalid URLs like#graphIdwsonce graph IDs live in the hash, which is the right direction here.If you want this fixture to be more explicit and robust, you could construct the WS URL directly from
window.location.origin, clearing search/hash instead of relying on string concat:- if (!url) { - // If no URL specified, use page URL - const u = new URL(window.location.toString().split('#')[0]) - u.protocol = 'ws:' - u.pathname = '/' - url = u.toString() + 'ws' - } + if (!url) { + // If no URL specified, connect to `${origin}/ws` + const u = new URL(window.location.origin) + u.protocol = 'ws:' + u.pathname = '/ws' + u.search = '' + u.hash = '' + url = u.toString() + }Not mandatory, but it avoids odd query strings like
?foo=barwsif tests ever run with query parameters.src/stores/subgraphNavigationStore.ts (2)
161-227: Hash navigation logic: fix operator precedence, initial-load race, and hash normalizationThe overall approach (using
useRouteHash, syncing canvas graph ↔ URL hash, and guarding withblockHashUpdate/initialLoad) looks solid, but there are a few concrete issues and edge cases worth tightening:
Operator precedence bug in
router.replace(...)(Line 210)router.replace('#' + window.location.hash.slice(1) || app.graph.id)Because
+has higher precedence than||, this always evaluates as:router.replace(('#' + window.location.hash.slice(1)) || app.graph.id)so the fallback to
app.graph.idnever runs. On an initial load with no hash, this produces'#'instead of'#<graphId>', and later thenewId === (currentId || app.graph.id)check prevents correcting it.Suggestion:
if (!routeHash.value) {router.replace('#' + window.location.hash.slice(1) || app.graph.id)
if (!routeHash.value) {const raw = window.location.hash.slice(1) || app.graph.idrouter.replace({ hash: `#${raw}` }) }Using the object form also aligns better with vue-router’s documented API.
Potential race on initial load when hash targets another workflow (Lines 211–216)
In theinitialLoadbranch:initialLoad = false navigateToHash(routeHash.value) const graph = canvasStore.getCanvas().graph if (isSubgraph(graph)) workflowStore.activeSubgraph = graph
navigateToHashisasync(itawaitsuseWorkflowService().openWorkflow(workflow)), but it’s not awaited here. If the hash points into a different workflow,graphcan still be the old workflow’s root when you read it, soactiveSubgraphmay never be updated for the target subgraph.Two options to avoid the race:
- Make
updateHashasync and awaitnavigateToHashin the initial-load branch, or- Move the
activeSubgraphupdate intonavigateToHashaftercanvas.setGraph(targetGraph)in both code paths (same-workflow and cross-workflow), so it always runs once navigation completes.Either approach keeps
workflowStore.activeSubgraphconsistent with the visible graph.Hash normalization in
navigateToHash(Line 169)const locatorId = newHash?.slice(1) ?? root.idThis assumes
newHashalways includes a leading#. Today that’s probably true for youruseRouteHash()+ vue-router setup, but ifuseRouteHashever returns a bare id (or its behavior changes), you’d silently drop the first character.A more defensive version that works with both
'#id'and'id':
- async function navigateToHash(newHash: string | undefined | null) {
const root = app.graphconst locatorId = newHash?.slice(1) ?? root.id
- async function navigateToHash(newHash: string | undefined | null) {
const root = app.graphconst locatorId = newHash? newHash.startsWith('#')? newHash.slice(1): newHash: root.id
Optional: guard against missing canvas (Lines 171, 214, 218)
BothnavigateToHashandupdateHashcallcanvasStore.getCanvas()and immediately dereference.graph. If this store is ever instantiated before the canvas is ready (or in tests), that could throw. A simple guard would make it safer:const canvas = canvasStore.getCanvas() if (!canvas) returnNot strictly required if you know the store is only used after setup, but low-cost defensive coding.
235-236: ExposingupdateHashfrom the store is useful; consider documenting its intended call sitesReturning
updateHashhere to be called fromapp.loadGraphDatais a good way to handle the “workflow load doesn’t emitcanvasStore.currentGraphchanges” gap.Given it has some internal state (
blockHashUpdate,initialLoad), it may be worth a brief JSDoc comment near the function saying it’s meant to be invoked:
- After a new graph is loaded (e.g.,
afterLoadNewGraph), and- Not during internal hash-driven navigation (where
blockHashUpdatesuppresses it).That will help future maintainers avoid calling it in the wrong phase and accidentally fighting the router/watchers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
browser_tests/fixtures/ws.ts(1 hunks)package.json(1 hunks)src/components/breadcrumb/SubgraphBreadcrumbItem.vue(0 hunks)src/scripts/app.ts(2 hunks)src/stores/subgraphNavigationStore.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/components/breadcrumb/SubgraphBreadcrumbItem.vue
🧰 Additional context used
🧬 Code graph analysis (2)
src/scripts/app.ts (1)
src/stores/subgraphNavigationStore.ts (1)
useSubgraphNavigationStore(21-239)
src/stores/subgraphNavigationStore.ts (5)
src/scripts/app.ts (2)
app(1710-1710)graph(161-163)src/platform/workflow/management/stores/workflowStore.ts (1)
activeState(60-62)src/lib/litegraph/src/LGraph.ts (1)
subgraphs(383-385)src/platform/workflow/core/services/workflowService.ts (1)
useWorkflowService(22-413)src/utils/typeGuardUtil.ts (1)
isSubgraph(18-20)
🪛 ESLint
src/stores/subgraphNavigationStore.ts
[error] 4-4: Unable to resolve path to module '@vueuse/router'.
(import-x/no-unresolved)
[error] 8-8: Unable to resolve path to module '@/platform/workflow/management/stores/workflowStore'.
(import-x/no-unresolved)
[error] 9-9: Unable to resolve path to module '@/platform/workflow/core/services/workflowService'.
(import-x/no-unresolved)
[error] 10-10: Unable to resolve path to module '@/renderer/core/canvas/canvasStore'.
(import-x/no-unresolved)
[error] 11-11: Unable to resolve path to module '@/router'.
(import-x/no-unresolved)
[error] 12-12: Unable to resolve path to module '@/scripts/app'.
(import-x/no-unresolved)
[error] 13-13: Unable to resolve path to module '@/utils/graphTraversalUtil'.
(import-x/no-unresolved)
[error] 14-14: Unable to resolve path to module '@/utils/typeGuardUtil'.
(import-x/no-unresolved)
🔇 Additional comments (1)
package.json (1)
152-155: Ensure @vueuse/router version is compatible with your existing VueUse / router stackAdding
@vueuse/routeras^14.0.0looks fine, but since@vueuse/coreandvue-routerare pulled in via thecatalog:indirection, please double‑check that:
- The
@vueuse/routerversion you get is intended to pair with the@vueuse/coreversion from your catalog.- The workspace lockfile has been updated (
pnpm install) so tools like eslint/import‑x no longer reportno-unresolvedon@vueuse/router.
| @@ -1,2 +1,2 @@ | |||
| <template> | |||
| <a | |||
There was a problem hiding this comment.
From quick testing, it wouldn't be a quick substitution. It's causing errors on click about missing state.
Perhaps we move in the opposite direction and replace the with a
| //Allow navigation with forward/back buttons | ||
| //TODO: Extend for dialogues? | ||
| //TODO: force update widget.promoted | ||
| async function navigateToHash(newHash: string | undefined | null) { |
There was a problem hiding this comment.
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.
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.
graph.idas location hashwindow.onhashchangeto navigate to the targetgraph.ideither in the current, or any other loaded workflow.canvasStore.currentGraphdoes not trigger whenapp.loadGraphDatais called. A trigger could be forced here, but I'm concerned about side effects. InsteadupdateHashis manually called.Code search shows that there are no current custom nodes using
onhashchange┆Issue is synchronized with this Notion page by Unito