Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,18 @@ import { useSettingStore } from '@/platform/settings/settingStore'
import { useWorkflowService } from '@/platform/workflow/core/services/workflowService'
import { useWorkflowStore } from '@/platform/workflow/management/stores/workflowStore'
import { useTemplateUrlLoader } from '@/platform/workflow/templates/composables/useTemplateUrlLoader'
import type { ComfyWorkflowJSON } from '@/platform/workflow/validation/schemas/workflowSchema'
import { api } from '@/scripts/api'
import { app as comfyApp } from '@/scripts/app'
import { getStorageValue, setStorageValue } from '@/scripts/utils'
import {
getWithAccessTracking,
setWithEviction
} from '@/platform/workflow/persistence/services/workflowSessionStorageService'
import { useCommandStore } from '@/stores/commandStore'

const WORKFLOW_KEY_PATTERN = /^workflow:/

export function useWorkflowPersistence() {
const workflowStore = useWorkflowStore()
const settingStore = useSettingStore()
Expand Down Expand Up @@ -44,39 +51,30 @@ export function useWorkflowPersistence() {

const persistCurrentWorkflow = () => {
if (!workflowPersistenceEnabled.value) return
const workflow = JSON.stringify(comfyApp.rootGraph.serialize())
const workflowData = comfyApp.rootGraph.serialize()
const workflowJson = JSON.stringify(workflowData)

try {
localStorage.setItem('workflow', workflow)
if (api.clientId) {
sessionStorage.setItem(`workflow:${api.clientId}`, workflow)
}
localStorage.setItem('workflow', workflowJson)
} catch (error) {
// Only log our own keys and aggregate stats
const ourKeys = Object.keys(sessionStorage).filter(
(key) => key.startsWith('workflow:') || key === 'workflow'
console.warn(
'[WorkflowPersistence] Failed to save to localStorage:',
error
)
console.error('QuotaExceededError details:', {
workflowSizeKB: Math.round(workflow.length / 1024),
totalStorageItems: Object.keys(sessionStorage).length,
ourWorkflowKeys: ourKeys.length,
ourWorkflowSizes: ourKeys.map((key) => ({
key,
sizeKB: Math.round(sessionStorage[key].length / 1024)
})),
error: error instanceof Error ? error.message : String(error)
})
throw error
}

if (api.clientId) {
const key = `workflow:${api.clientId}`
setWithEviction(key, workflowData, WORKFLOW_KEY_PATTERN)
}
}

const loadWorkflowFromStorage = async (
json: string | null,
const loadWorkflowFromData = async (
workflowData: ComfyWorkflowJSON | null,
workflowName: string | null
) => {
if (!json) return false
const workflow = JSON.parse(json)
await comfyApp.loadGraphData(workflow, true, true, workflowName)
if (!workflowData) return false
await comfyApp.loadGraphData(workflowData, true, true, workflowName)
return true
}

Expand All @@ -86,15 +84,28 @@ export function useWorkflowPersistence() {

// Try loading from session storage first
if (clientId) {
const sessionWorkflow = sessionStorage.getItem(`workflow:${clientId}`)
if (await loadWorkflowFromStorage(sessionWorkflow, workflowName)) {
const sessionWorkflow = getWithAccessTracking<ComfyWorkflowJSON>(
`workflow:${clientId}`
)
if (await loadWorkflowFromData(sessionWorkflow, workflowName)) {
return true
}
}

// Fall back to local storage
const localWorkflow = localStorage.getItem('workflow')
return await loadWorkflowFromStorage(localWorkflow, workflowName)
// Fall back to local storage (raw JSON, no LRU wrapper)
const localWorkflowJson = localStorage.getItem('workflow')
if (localWorkflowJson) {
try {
const localWorkflow = JSON.parse(localWorkflowJson) as ComfyWorkflowJSON
return await loadWorkflowFromData(localWorkflow, workflowName)
} catch {
console.warn(
'[WorkflowPersistence] Failed to parse localStorage workflow'
)
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

# Find and read the target file
find . -name "useWorkflowPersistence.ts" -type f | head -5

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 143


🏁 Script executed:

# Look at the structure around the lines in question
find . -path "*/workflow/persistence/composables/*" -type f

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 290


🏁 Script executed:

# Search for ComfyWorkflowJSON type definition and any Zod schemas
rg "ComfyWorkflowJSON" --type ts -A 3 -B 3

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 48392


🏁 Script executed:

# Search for loadGraphData function to check its validation
rg "loadGraphData" --type ts -A 15 | head -100

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 8193


🏁 Script executed:

# Check for Zod schema validation patterns in the codebase
rg "safeParse|schema\.parse" --type ts | head -20

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 2216


🏁 Script executed:

# Read the file around the problematic lines
cat -n src/platform/workflow/persistence/composables/useWorkflowPersistence.ts | sed -n '80,115p'

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 1538


🏁 Script executed:

# Check if validateComfyWorkflow is imported or used in this file
rg "validateComfyWorkflow" src/platform/workflow/persistence/composables/useWorkflowPersistence.ts -A 5 -B 5

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 52


🏁 Script executed:

# Check the loadWorkflowFromData function to see if it does validation
rg "loadWorkflowFromData" src/platform/workflow/persistence/composables/useWorkflowPersistence.ts -A 10

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 1029


🏁 Script executed:

# Check loadGraphData to see if it validates the input
rg "async loadGraphData" src/scripts/app.ts -A 30 | head -60

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 961


🏁 Script executed:

# Check if loadGraphData has any validation logic
rg "loadGraphData" src/scripts/app.ts -A 80 | grep -A 80 "async loadGraphData"

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 2577


🏁 Script executed:

# Check if there's any validation in the workflow validation composable that might be called
rg "useWorkflowValidation" src/platform/workflow/persistence/composables/useWorkflowPersistence.ts -B 3 -A 3

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 52


🏁 Script executed:

# Look at the validateComfyWorkflow function to understand the pattern
cat -n src/platform/workflow/validation/schemas/workflowSchema.ts | sed -n '350,400p'

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 1903


🏁 Script executed:

# Find validateComfyWorkflow signature and pattern
cat -n src/platform/workflow/validation/schemas/workflowSchema.ts | sed -n '490,520p'

Repository: Comfy-Org/ComfyUI_frontend

Length of output: 1165


Consider validating parsed localStorage workflow data with Zod schema.

The as ComfyWorkflowJSON cast on line 99 accepts any parsed JSON without validation. While loadGraphData includes optional schema validation (if Comfy.Validation.Workflows is enabled), corrupted or invalid data could slip through if validation is disabled. Following the codebase pattern, use validateComfyWorkflow() instead:

try {
  const validated = await validateComfyWorkflow(JSON.parse(localWorkflowJson))
  if (!validated) {
    console.warn('[WorkflowPersistence] Invalid localStorage workflow')
    return false
  }
  return await loadWorkflowFromData(validated, workflowName)
} catch {
  console.warn('[WorkflowPersistence] Failed to parse localStorage workflow')
}

This validates data at the entry point and aligns with existing patterns in the codebase (e.g., src/platform/remote/comfyui/jobs/fetchJobs.ts).

🤖 Prompt for AI Agents
In `@src/platform/workflow/persistence/composables/useWorkflowPersistence.ts`
around lines 95 - 106, Replace the unchecked cast of parsed localStorage JSON
with Zod validation: parse localStorage.getItem('workflow'), pass the parsed
value into validateComfyWorkflow(), and if validation fails log
'[WorkflowPersistence] Invalid localStorage workflow' and return false; if
validation succeeds call loadWorkflowFromData(validated, workflowName). Update
the try/catch around JSON.parse to use validateComfyWorkflow() instead of "as
ComfyWorkflowJSON" and preserve the existing catch that logs parse failures.


return false
}

const loadDefaultWorkflow = async () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,195 @@
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'

import {
getWithAccessTracking,
removeFromStorage,
setWithEviction
} from './workflowSessionStorageService'

describe('workflowSessionStorageService', () => {
beforeEach(() => {
sessionStorage.clear()
vi.clearAllMocks()
})

afterEach(() => {
vi.restoreAllMocks()
})

describe('setWithEviction', () => {
it('stores data with accessedAt wrapper', () => {
const data = { nodes: [], links: [] }
const result = setWithEviction('workflow:test', data)

expect(result).toBe(true)

const stored = sessionStorage.getItem('workflow:test')
const parsed = JSON.parse(stored!)
expect(parsed).toHaveProperty('accessedAt')
expect(parsed).toHaveProperty('data')
expect(parsed.data).toEqual(data)
})

it('evicts oldest entries when quota is exceeded', () => {
const oldEntry = { accessedAt: 1000, data: { old: 'data' } }
const newEntry = { accessedAt: 2000, data: { new: 'data' } }

sessionStorage.setItem('workflow:old', JSON.stringify(oldEntry))
sessionStorage.setItem('workflow:new', JSON.stringify(newEntry))

let callCount = 0
const originalSetItem = sessionStorage.setItem.bind(sessionStorage)
vi.spyOn(sessionStorage, 'setItem').mockImplementation((key, value) => {
callCount++
if (key === 'workflow:current' && callCount === 1) {
throw new DOMException('Quota exceeded', 'QuotaExceededError')
}
return originalSetItem(key, value)
})

const result = setWithEviction(
'workflow:current',
{ current: 'data' },
/^workflow:/
)

expect(result).toBe(true)
expect(sessionStorage.getItem('workflow:old')).toBeNull()
expect(sessionStorage.getItem('workflow:new')).toBeTruthy()
})

it('returns false after max eviction attempts', () => {
const spy = vi.spyOn(sessionStorage, 'setItem').mockImplementation(() => {
throw new DOMException('Quota exceeded', 'QuotaExceededError')
})

const result = setWithEviction('key', { test: 'data' })
expect(result).toBe(false)

spy.mockRestore()
})
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

it('returns false on unexpected errors', () => {
const spy = vi.spyOn(sessionStorage, 'setItem').mockImplementation(() => {
throw new Error('Unexpected error')
})

const result = setWithEviction('key', { test: 'data' })
expect(result).toBe(false)

spy.mockRestore()
})
})

describe('getWithAccessTracking', () => {
it('returns null for non-existent key', () => {
expect(getWithAccessTracking('nonexistent')).toBeNull()
})

it('returns unwrapped data for new format entries', () => {
const entry = { accessedAt: Date.now(), data: { nodes: [], links: [] } }
sessionStorage.setItem('workflow:test', JSON.stringify(entry))

const result = getWithAccessTracking('workflow:test')
expect(result).toEqual({ nodes: [], links: [] })
})

it('handles legacy format with accessedAt: 0', () => {
const legacyData = { nodes: [1, 2, 3], links: [] }
sessionStorage.setItem('workflow:legacy', JSON.stringify(legacyData))

const result = getWithAccessTracking('workflow:legacy')
expect(result).toEqual(legacyData)
})

it('updates accessedAt when reading', () => {
const oldTime = 1000
const entry = { accessedAt: oldTime, data: { test: 'data' } }
sessionStorage.setItem('workflow:test', JSON.stringify(entry))

getWithAccessTracking('workflow:test', true)

const stored = JSON.parse(sessionStorage.getItem('workflow:test')!)
expect(stored.accessedAt).toBeGreaterThan(oldTime)
})

it('does not update accessedAt when updateAccessTime is false', () => {
const oldTime = 1000
const entry = { accessedAt: oldTime, data: { test: 'data' } }
sessionStorage.setItem('workflow:test', JSON.stringify(entry))

getWithAccessTracking('workflow:test', false)

const stored = JSON.parse(sessionStorage.getItem('workflow:test')!)
expect(stored.accessedAt).toBe(oldTime)
})

it('returns null for invalid JSON', () => {
sessionStorage.setItem('workflow:invalid', 'not json')

const result = getWithAccessTracking('workflow:invalid')
expect(result).toBeNull()
})
})

describe('removeFromStorage', () => {
it('removes item from session storage', () => {
sessionStorage.setItem('key', 'value')
expect(sessionStorage.getItem('key')).toBe('value')

removeFromStorage('key')
expect(sessionStorage.getItem('key')).toBeNull()
})
})

describe('eviction order', () => {
it('evicts legacy entries (accessedAt: 0) before new entries', () => {
const legacyData = { nodes: [], links: [] }
const newEntry = { accessedAt: Date.now(), data: { new: true } }

sessionStorage.setItem('workflow:legacy', JSON.stringify(legacyData))
sessionStorage.setItem('workflow:new', JSON.stringify(newEntry))

let callCount = 0
const originalSetItem = sessionStorage.setItem.bind(sessionStorage)
vi.spyOn(sessionStorage, 'setItem').mockImplementation((key, value) => {
callCount++
if (key === 'workflow:current' && callCount === 1) {
throw new DOMException('Quota exceeded', 'QuotaExceededError')
}
return originalSetItem(key, value)
})

setWithEviction('workflow:current', { current: true }, /^workflow:/)

expect(sessionStorage.getItem('workflow:legacy')).toBeNull()
expect(sessionStorage.getItem('workflow:new')).toBeTruthy()
})

it('does not evict protected keys', () => {
const workspaceEntry = { accessedAt: 0, data: { workspace: true } }
const workflowEntry = { accessedAt: 1000, data: { workflow: true } }

sessionStorage.setItem(
'workspace.settings',
JSON.stringify(workspaceEntry)
)
sessionStorage.setItem('workflow:old', JSON.stringify(workflowEntry))

let callCount = 0
const originalSetItem = sessionStorage.setItem.bind(sessionStorage)
vi.spyOn(sessionStorage, 'setItem').mockImplementation((key, value) => {
callCount++
if (key === 'workflow:current' && callCount === 1) {
throw new DOMException('Quota exceeded', 'QuotaExceededError')
}
return originalSetItem(key, value)
})

setWithEviction('workflow:current', { current: true }, /^workflow:/)

expect(sessionStorage.getItem('workspace.settings')).toBeTruthy()
expect(sessionStorage.getItem('workflow:old')).toBeNull()
})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Test doesn't exercise the isProtectedKey logic.

The pattern /^workflow:/ (line 185) doesn't match workspace.settings, so the key is excluded by pattern mismatch before isProtectedKey is ever checked. This test passes for the wrong reason.

To actually verify protected key behavior, use a pattern that matches both keys:

🔧 Suggested fix
  it('does not evict protected keys', () => {
    const workspaceEntry = { accessedAt: 0, data: { workspace: true } }
    const workflowEntry = { accessedAt: 1000, data: { workflow: true } }

    sessionStorage.setItem(
      'workspace.settings',
      JSON.stringify(workspaceEntry)
    )
    sessionStorage.setItem('workflow:old', JSON.stringify(workflowEntry))

    let callCount = 0
    const originalSetItem = sessionStorage.setItem.bind(sessionStorage)
    vi.spyOn(sessionStorage, 'setItem').mockImplementation((key, value) => {
      callCount++
      if (key === 'workflow:current' && callCount === 1) {
        throw new DOMException('Quota exceeded', 'QuotaExceededError')
      }
      return originalSetItem(key, value)
    })

-    setWithEviction('workflow:current', { current: true }, /^workflow:/)
+    setWithEviction('workflow:current', { current: true }, /^work/)

    expect(sessionStorage.getItem('workspace.settings')).toBeTruthy()
    expect(sessionStorage.getItem('workflow:old')).toBeNull()
  })
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('does not evict protected keys', () => {
const workspaceEntry = { accessedAt: 0, data: { workspace: true } }
const workflowEntry = { accessedAt: 1000, data: { workflow: true } }
sessionStorage.setItem(
'workspace.settings',
JSON.stringify(workspaceEntry)
)
sessionStorage.setItem('workflow:old', JSON.stringify(workflowEntry))
let callCount = 0
const originalSetItem = sessionStorage.setItem.bind(sessionStorage)
vi.spyOn(sessionStorage, 'setItem').mockImplementation((key, value) => {
callCount++
if (key === 'workflow:current' && callCount === 1) {
throw new DOMException('Quota exceeded', 'QuotaExceededError')
}
return originalSetItem(key, value)
})
setWithEviction('workflow:current', { current: true }, /^workflow:/)
expect(sessionStorage.getItem('workspace.settings')).toBeTruthy()
expect(sessionStorage.getItem('workflow:old')).toBeNull()
})
it('does not evict protected keys', () => {
const workspaceEntry = { accessedAt: 0, data: { workspace: true } }
const workflowEntry = { accessedAt: 1000, data: { workflow: true } }
sessionStorage.setItem(
'workspace.settings',
JSON.stringify(workspaceEntry)
)
sessionStorage.setItem('workflow:old', JSON.stringify(workflowEntry))
let callCount = 0
const originalSetItem = sessionStorage.setItem.bind(sessionStorage)
vi.spyOn(sessionStorage, 'setItem').mockImplementation((key, value) => {
callCount++
if (key === 'workflow:current' && callCount === 1) {
throw new DOMException('Quota exceeded', 'QuotaExceededError')
}
return originalSetItem(key, value)
})
setWithEviction('workflow:current', { current: true }, /^work/)
expect(sessionStorage.getItem('workspace.settings')).toBeTruthy()
expect(sessionStorage.getItem('workflow:old')).toBeNull()
})
🤖 Prompt for AI Agents
In
`@src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts`
around lines 165 - 189, The test currently uses pattern /^workflow:/ which
doesn't match 'workspace.settings' so isProtectedKey is never exercised; update
the test to use a pattern that matches both keys (for example a regex matching
either workspace.* or a catch-all) so calling
setWithEviction('workflow:current', ...) will trigger eviction logic while still
allowing isProtectedKey to protect 'workspace.settings'; ensure you keep
references to sessionStorage.setItem spy and the existing keys
'workspace.settings' and 'workflow:old' and the function under test
setWithEviction so the protected-key behavior is actually asserted.

})
})
Loading