feat(persistence): add draft store and tab state management#8519
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a per-tab session-scoped composable for workflow pointers, a new Pinia workflow draft store (V2) persisting per-draft payloads in localStorage with LRU eviction/quota handling, storage IO validation helpers, and tests for both features. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Store as Draft Store
participant LS as LocalStorage
participant Graph as comfyApp Graph
Client->>Store: loadPersistedWorkflow(options)
alt preferredPath present & payload exists
Store->>LS: read payload by preferredPath
LS-->>Store: payload
else preferred missing, fallbackToLatestDraft
Store->>Store: getMostRecentPath()
Store->>LS: read payload by mostRecentPath
LS-->>Store: payload
else legacy sessionStorage/localStorage fallback
Store->>LS: read legacy sessionStorage payload
LS-->>Store: payload or null
alt none in sessionStorage
Store->>LS: read legacy localStorage payload
LS-->>Store: payload or null
end
end
Store->>Graph: tryLoadGraph(payload)
alt load success
Graph-->>Store: success
Store-->>Client: true
else failure
Store->>LS: cleanup/remove draft
Store-->>Client: false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 02/20/2026, 07:46:06 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
|
Playwright: ✅ 516 passed, 0 failed · 5 flaky 📊 Browser Reports
|
436fa0b to
c2e62c7
Compare
0c88af0 to
07ae110
Compare
c2e62c7 to
c5de23a
Compare
07ae110 to
f1bc97f
Compare
c5de23a to
7c42cd4
Compare
f1bc97f to
84ac4bd
Compare
7c42cd4 to
85a6545
Compare
a00870f to
dd6de90
Compare
b107ee5 to
4d6cd54
Compare
dd6de90 to
b77c98a
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Add the V2 draft store and tab state management: - workflowDraftStoreV2: Uses per-draft localStorage keys instead of a single blob. Handles LRU eviction with loop on quota exceeded. Maintains in-memory index cache synced with localStorage. - useWorkflowTabState: Manages sessionStorage pointers scoped by api.clientId. Validates workspaceId on read to prevent cross-workspace contamination. The V2 store is not yet wired to the app - the old store remains active. This allows the integration to happen in a separate commit. Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019c16f4-05a2-779d-aa0e-a0e098308a95
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/platform/workflow/persistence/stores/workflowDraftStoreV2.test.ts (1)
80-100: Consider importingMAX_DRAFTSconstant instead of magic number.The test uses hardcoded
32which duplicates theMAX_DRAFTSconstant fromdraftTypes.ts. If the constant changes, this test could silently test incorrect behavior.♻️ Suggested improvement
+import { MAX_DRAFTS } from '../base/draftTypes' + // ... it('evicts oldest when over limit', () => { const store = useWorkflowDraftStoreV2() - // Save 32 drafts (MAX_DRAFTS) - for (let i = 0; i < 32; i++) { + // Save MAX_DRAFTS drafts + for (let i = 0; i < MAX_DRAFTS; i++) { store.saveDraft(`workflows/draft${i}.json`, `{"id":${i}}`, { name: `draft${i}`, isTemporary: true }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.test.ts` around lines 80 - 100, Replace the hardcoded 32 in the test with the shared MAX_DRAFTS constant from draftTypes.ts: import MAX_DRAFTS at the top of the test file and use it in the save loop (the test around useWorkflowDraftStoreV2() that currently iterates for (let i = 0; i < 32; i++)), so the test stays correct when MAX_DRAFTS changes and clearly documents the limit being tested.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/workflow/persistence/base/storageIO.ts`:
- Around line 54-63: The writeIndex (and similarly writePayload) write paths
only treat DOMException with name 'QuotaExceededError' as a quota condition;
update both functions around the localStorage.setItem calls (referencing
StorageKeys.draftIndex and the writePayload storage key usages) to detect quota
errors across browsers by checking: error is a DOMException and (error.name ===
'QuotaExceededError' || error.name === 'NS_ERROR_DOM_QUOTA_REACHED' ||
error.code === 22 || error.code === 1014) or, more generally, using typeof
error.code === 'number' and matching 22/1014; if matched return false (graceful
degradation) and otherwise rethrow the error.
- Around line 31-43: Add strict runtime validation for all four readers:
readIndex, readPayload, readActivePath, and readOpenPaths by defining Zod
schemas (or equivalent type guards) for DraftIndexV2, DraftPayloadV2, active
path, and open paths and using schema.safeParse(parsed) before returning; for
readIndex ensure parsed.v === 2 and validate updatedAt is a number, order is
string[], and entries is an object with DraftEntryMeta values; for readPayload
validate the full DraftPayloadV2 shape; for readActivePath validate it's either
null or a string and for readOpenPaths validate it's an array of strings; on
validation failure return null (and optionally log or swallow error) so no
malformed storage data is cast and propagated.
---
Nitpick comments:
In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.test.ts`:
- Around line 80-100: Replace the hardcoded 32 in the test with the shared
MAX_DRAFTS constant from draftTypes.ts: import MAX_DRAFTS at the top of the test
file and use it in the save loop (the test around useWorkflowDraftStoreV2() that
currently iterates for (let i = 0; i < 32; i++)), so the test stays correct when
MAX_DRAFTS changes and clearly documents the limit being tested.
| export function readIndex(workspaceId: string): DraftIndexV2 | null { | ||
| if (!storageAvailable) return null | ||
|
|
||
| try { | ||
| const key = StorageKeys.draftIndex(workspaceId) | ||
| const json = localStorage.getItem(key) | ||
| if (!json) return null | ||
|
|
||
| const parsed = JSON.parse(json) | ||
| if (parsed.v !== 2) return null | ||
|
|
||
| return parsed as DraftIndexV2 | ||
| } catch { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/platform/workflow/persistence/base/storageIO.ts | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3156
🏁 Script executed:
cat -n src/platform/workflow/persistence/base/storageIO.ts | sed -n '100,230p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3905
🏁 Script executed:
# Check the type definitions to understand the expected shapes
cat -n src/platform/workflow/persistence/base/draftTypes.ts | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2740
🏁 Script executed:
# Search for all call sites of these read functions
rg -n 'readIndex\s*\(' src/ --type ts --type vue -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 97
🏁 Script executed:
# Search for all call sites of these read functions
rg -n 'readIndex\s*\(' src/ --type ts -A 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1661
🏁 Script executed:
# Check how readIndex is used in the main consumer
cat -n src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts | sed -n '60,90p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1126
🏁 Script executed:
# Check the removeOrphanedEntries function to see if it validates the stored data
rg -n 'removeOrphanedEntries' src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts -B 5 -A 10Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1001
🏁 Script executed:
# Check the draftCacheV2 file for the removeOrphanedEntries implementation
cat -n src/platform/workflow/persistence/base/draftCacheV2.ts | head -150Repository: Comfy-Org/ComfyUI_frontend
Length of output: 4448
🏁 Script executed:
cat -n src/platform/workflow/persistence/base/draftCacheV2.ts | sed -n '150,200p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1481
🏁 Script executed:
# Search for readPayload call sites
rg -n 'readPayload\s*\(' src/ --type ts -A 2 -B 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3805
🏁 Script executed:
# Check how readPayload is used in the store
cat -n src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts | sed -n '225,260p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1166
🏁 Script executed:
# Search for readActivePath and readOpenPaths call sites
rg -n 'readActivePath\s*\(|readOpenPaths\s*\(' src/ --type ts -A 2 -B 2Repository: Comfy-Org/ComfyUI_frontend
Length of output: 3667
🏁 Script executed:
cat -n src/platform/workflow/persistence/composables/useWorkflowTabState.ts | sed -n '35,85p'Repository: Comfy-Org/ComfyUI_frontend
Length of output: 1825
🏁 Script executed:
# Check if Zod is used in the project
fd -t f 'package.json' -x cat {} | grep -i zodRepository: Comfy-Org/ComfyUI_frontend
Length of output: 168
🏁 Script executed:
# Check for existing Zod schema patterns in the codebase
rg -n 'z\.object\|ZodSchema\|z\.parse' src/ --type ts -B 2 -A 5 | head -100Repository: Comfy-Org/ComfyUI_frontend
Length of output: 52
🏁 Script executed:
# Search for Zod imports
rg -n "from 'zod'|from \"zod\"" src/ --type tsRepository: Comfy-Org/ComfyUI_frontend
Length of output: 1255
Validate parsed storage data before returning.
localStorage/sessionStorage values are user-modifiable; returning parsed JSON without shape validation risks propagating corrupt data. Add type guards or Zod schemas for all four functions:
readIndex: Only checksv === 2, lacks validation forupdatedAt(number),order(string[]), andentries(object with DraftEntryMeta values)readPayload: No validation; directly casts parsed JSON as DraftPayloadV2readActivePathandreadOpenPaths: No validation in readers (though call sites in useWorkflowTabState.ts validateworkspaceId, the data shape itself is unchecked)
Zod and zod-validation-error are available in the project. Use type guards or Zod schemas to ensure shape correctness before returning.
Applies to lines 31–46 (readIndex), 69–84 (readPayload), 175–185 (readActivePath), and 205–215 (readOpenPaths).
Per coding guidelines: "Validate trusted sources before processing".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/workflow/persistence/base/storageIO.ts` around lines 31 - 43,
Add strict runtime validation for all four readers: readIndex, readPayload,
readActivePath, and readOpenPaths by defining Zod schemas (or equivalent type
guards) for DraftIndexV2, DraftPayloadV2, active path, and open paths and using
schema.safeParse(parsed) before returning; for readIndex ensure parsed.v === 2
and validate updatedAt is a number, order is string[], and entries is an object
with DraftEntryMeta values; for readPayload validate the full DraftPayloadV2
shape; for readActivePath validate it's either null or a string and for
readOpenPaths validate it's an array of strings; on validation failure return
null (and optionally log or swallow error) so no malformed storage data is cast
and propagated.
There was a problem hiding this comment.
Addressed — added isValidIndex type guard that validates v === 2, updatedAt is a number, order is an array, and entries is a non-null object.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the current implementation of all four reader functions to verify validation
cat -n src/platform/workflow/persistence/base/storageIO.ts | sed -n '26,230p'
Length of output: 6193
@christian-byrne, thanks for adding the isValidIndex type guard—readIndex is now properly validated! 👍
However, the original review flagged four functions that need validation. The remaining three still cast parsed JSON without validation:
readPayload(line 98): Should validate thatdatais a string andupdatedAtis a number before returning asDraftPayloadV2readActivePath(line 197): Should validate thatworkspaceIdandpathare strings before returning asActivePathPointerreadOpenPaths(line 227): Should validate thatworkspaceIdis a string,pathsis a string array, andactiveIndexis a number before returning asOpenPathsPointer
You can follow the same pattern as isValidIndex—create type guards like isValidPayload, isValidActivePath, and isValidOpenPaths to ensure malformed storage data doesn't propagate through the system.
🧬 Code Graph Analysis Results
- File: src/platform/workflow/persistence/base/draftTypes.ts, lines 30-39
export interface DraftIndexV2 {
/** Schema version */
v: 2
/** Last update timestamp */
updatedAt: number
/** LRU order: oldest → newest (draftKey array) */
order: string[]
/** Metadata keyed by draftKey (hash of path) */
entries: Record<string, DraftEntryMeta>
}
- File: src/platform/workflow/persistence/base/draftTypes.ts, lines 46-51
export interface DraftPayloadV2 {
/** Serialized workflow JSON */
data: string
/** Last update timestamp */
updatedAt: number
}
- File: src/platform/workflow/persistence/base/draftTypes.ts, lines 59-64
export interface ActivePathPointer {
/** Workspace ID for validation */
workspaceId: string
/** Path to the active workflow */
path: string
}
- File: src/platform/workflow/persistence/base/draftTypes.ts, lines 72-79
export interface OpenPathsPointer {
/** Workspace ID for validation */
workspaceId: string
/** Ordered list of open workflow paths */
paths: string[]
/** Index of the active workflow in paths array */
activeIndex: number
}
- File: src/platform/workflow/persistence/base/storageKeys.ts, lines 40-93
export const StorageKeys = {
/**
* Draft index key for localStorage.
* Contains LRU order and metadata for all drafts.
*/
draftIndex(workspaceId: string = CURRENT_WORKSPACE_ID): string {
return `Comfy.Workflow.DraftIndex.v2:${workspaceId}`
},
/**
* Individual draft payload key for localStorage.
* `@param` path - Workflow path (will be hashed to create key)
*/
draftPayload(
path: string,
workspaceId: string = CURRENT_WORKSPACE_ID
): string {
const draftKey = hashPath(path)
return `Comfy.Workflow.Draft.v2:${workspaceId}:${draftKey}`
},
/**
* Creates a draft key (hash) from a workflow path.
*/
draftKey(path: string): string {
return hashPath(path)
},
/**
* Active workflow pointer key for sessionStorage.
* `@param` clientId - Browser tab identifier from api.clientId
*/
activePath(clientId: string): string {
return `Comfy.Workflow.ActivePath:${clientId}`
},
/**
* Open workflows pointer key for sessionStorage.
* `@param` clientId - Browser tab identifier from api.clientId
*/
openPaths(clientId: string): string {
return `Comfy.Workflow.OpenPaths:${clientId}`
},
/**
* Prefix patterns for cleanup operations.
*/
prefixes: {
draftIndex: 'Comfy.Workflow.DraftIndex.v2:',
draftPayload: 'Comfy.Workflow.Draft.v2:',
activePath: 'Comfy.Workflow.ActivePath:',
openPaths: 'Comfy.Workflow.OpenPaths:'
}
} as const
🧠 Learnings used
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7746
File: src/platform/assets/services/assetService.ts:484-491
Timestamp: 2026-01-06T19:20:56.167Z
Learning: In `src/platform/assets/services/assetService.ts`, prefer using `schema.safeParse()` over `schema.parse()` for validating API responses to avoid throwing ZodError with internal schema details; follow the existing pattern: call `safeParse()`, check `result.success`, and use `fromZodError(result.error)` to format error messages for logging while throwing user-friendly errors.
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 8307
File: src/stores/bootstrapStore.ts:61-78
Timestamp: 2026-01-26T18:39:26.986Z
Learning: In src/stores/**/*.ts: Do not suggest exposing internal state or error refs from stores unless there is demonstrated active usage of those properties elsewhere in the codebase. Keep internal state private by default; only expose what is actively consumed.
b77c98a to
e07f341
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts (3)
39-48: Consider exporting the interfaces for external consumers.
DraftMetaandLoadPersistedWorkflowOptionsare defined internally but may be needed by callers ofsaveDraftandloadPersistedWorkflow. If these types are intended for internal use only, this is fine. Otherwise, consider exporting them or moving todraftTypes.tsfor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts` around lines 39 - 48, The interfaces DraftMeta and LoadPersistedWorkflowOptions are currently unexported but are used by external callers of saveDraft and loadPersistedWorkflow; export them (or move them into a shared draftTypes.ts and export from there) so callers can import the types. Update the declarations for DraftMeta and LoadPersistedWorkflowOptions to be exported and/or add corresponding exports from draftTypes.ts, then update any imports in callers of saveDraft/loadPersistedWorkflow to reference the exported types.
134-182: Consider adding a maximum eviction count to prevent runaway loops.The eviction loop continues while
order.length > 0. If storage is persistently full (e.g., other apps consuming localStorage), this could evict all drafts before failing. Consider adding a maximum iteration count to fail faster and preserve more user data.💡 Suggested improvement
function handleQuotaExceeded( path: string, data: string, meta: DraftMeta ): boolean { const index = loadIndex() const draftKey = hashPath(path) + const maxEvictions = Math.min(5, index.order.length) + let evictionCount = 0 // Try evicting oldest entries until we can write let currentIndex = index - while (currentIndex.order.length > 0) { + while (currentIndex.order.length > 0 && evictionCount < maxEvictions) { const oldestKey = currentIndex.order[0] if (oldestKey === draftKey) break // Don't evict the one we're trying to save // ... rest of loop + evictionCount++ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts` around lines 134 - 182, The eviction loop in handleQuotaExceeded can run until all drafts are evicted; add a max eviction limit to avoid runaway deletion: introduce a constant like MAX_EVICTIONS (e.g., 10), add a counter (evictions = 0) and increment it each time you successfully remove an entry in the while loop inside handleQuotaExceeded, and break out when evictions >= MAX_EVICTIONS (then call markStorageUnavailable() and return false) so you stop evicting early; ensure you still avoid evicting the draftKey and that any partially-updated index state is persisted via persistIndex(finalIndex) only when a new payload write succeeds (as currently done).
260-275: Consider logging the error with structured context.The
catchblock logs the error but doesn't include which path failed. Adding the path would help debugging persisted workflow issues.💡 Suggested improvement
} catch (err) { - console.error('Failed to load persisted workflow', err) + console.error('Failed to load persisted workflow:', { payload: payload?.slice(0, 100), err }) onFailure?.() return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts` around lines 260 - 275, The catch in tryLoadGraph currently logs the error without context; update the error logging in function tryLoadGraph (which calls comfyApp.loadGraphData) to include identifying context such as the workflowName and a safe/truncated representation of payload (or the persistence path if available) along with the error, and prefer the project logging utility (e.g., processLogger or a structured logger) over console.error so logs are structured and actionable; ensure onFailure() is still invoked and the function returns false on error.src/platform/workflow/persistence/composables/useWorkflowTabState.ts (1)
22-24: Verify API initialization edge case.The
getClientIdfunction returnsnullif neitherclientIdnorinitialClientIdis set. This silently prevents any storage operations. Consider whether this edge case (API not yet initialized) should log a warning in development mode to help debug issues where tab state unexpectedly doesn't persist.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/persistence/composables/useWorkflowTabState.ts` around lines 22 - 24, getClientId currently returns null when both api.clientId and api.initialClientId are absent, causing silent failures of storage; update getClientId to detect this "API not initialized" edge case and, in development mode, emit a clear warning (e.g., using console.warn or the project's logger) that includes which properties are missing (api.clientId and api.initialClientId) so developers can see why tab state isn't persisting; leave the return behavior unchanged but only log in dev (use NODE_ENV or the project's isDev helper) and reference getClientId, api.clientId, and api.initialClientId when implementing the check and message.src/platform/workflow/persistence/base/storageIO.ts (1)
111-118: Consider addingstorageAvailableguard for consistency.
deletePayloadlacks theif (!storageAvailable) returnguard that other functions have. While errors are caught and ignored, adding the guard would provide consistent early-exit behavior and avoid unnecessary try/catch overhead when storage is known to be unavailable.♻️ Suggested improvement
export function deletePayload(workspaceId: string, draftKey: string): void { + if (!storageAvailable) return + try { const key = `${StorageKeys.prefixes.draftPayload}${workspaceId}:${draftKey}` localStorage.removeItem(key) } catch { // Ignore errors during deletion } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/persistence/base/storageIO.ts` around lines 111 - 118, The deletePayload function should start by checking the storageAvailable flag and return early if storage is unavailable to match other functions' behavior; update deletePayload (which builds the key using StorageKeys.prefixes.draftPayload and the workspaceId:draftKey pattern) to include an if (!storageAvailable) return guard before attempting localStorage.removeItem, leaving the try/catch only as backup for unexpected runtime errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/platform/workflow/persistence/base/storageIO.ts`:
- Around line 75-80: The parsed JSON for draft payload is being cast directly to
DraftPayloadV2 without validation; update the read routine that builds the key
using StorageKeys.prefixes.draftPayload (the block that calls
localStorage.getItem and JSON.parse) to verify the parsed object has the
expected shape (e.g., typeof parsed.data === 'string' and typeof
parsed.updatedAt === 'number') before casting and returning, and return null (or
handle error) when validation fails, mirroring the approach used in readIndex.
- Around line 175-185: readActivePath and readOpenPaths currently JSON.parse and
cast directly to ActivePathPointer/OpenPathsPointer without validating the
object shape; update both functions (identify by name readActivePath and
readOpenPaths and the key builders StorageKeys.activePath /
StorageKeys.openPaths) to: parse the JSON, perform explicit runtime checks for
required properties and types expected by ActivePathPointer/OpenPathsPointer
(e.g., presence and types of id/path/timestamps or array entries), return the
typed object only if validation passes, otherwise return null; keep try/catch to
swallow malformed JSON but use the validation result to avoid unsafe casts.
- Around line 39-42: The parsed JSON is only checked for parsed.v === 2 but then
blindly cast to DraftIndexV2; add a shape validation step (either a Zod schema
or an explicit type guard) after parsing to verify parsed.updatedAt is a number,
parsed.order is an array of strings, and parsed.entries is a record with the
expected entry shape before returning; if validation fails, return null (or
handle error) so malformed localStorage data cannot be cast to DraftIndexV2.
- Around line 58-63: The catch block in storageIO.ts only checks for error
instanceof DOMException && error.name === 'QuotaExceededError', so to handle
cross-browser quota errors update that condition (in the catch handling around
the storage read/write function) to also detect legacy codes/names: treat as
quota if (error instanceof DOMException && (error.name === 'QuotaExceededError'
|| error.name === 'NS_ERROR_DOM_QUOTA_REACHED')) || (typeof (error as any).code
=== 'number' && ((error as any).code === 22 || (error as any).code === 1014));
when that condition matches return false, otherwise rethrow the error; this
ensures Chrome/Safari numeric code 22 and Firefox code 1014/name
NS_ERROR_DOM_QUOTA_REACHED are handled.
- Around line 100-105: The catch block in the storageIO write path only checks
for DOMException with name 'QuotaExceededError'; update it to detect
cross-browser quota errors like the fix applied to writeIndex by either calling
the shared isQuotaExceeded helper (if available) or expanding the condition to
also check for DOMException.name === 'NS_ERROR_DOM_QUOTA_REACHED' and legacy
numeric codes (e.g. error.code === 22 or error.code === 1014), returning false
for those cases and rethrowing otherwise; reference the catch block in
storageIO.ts and mirror the exact logic used for writeIndex.
---
Nitpick comments:
In `@src/platform/workflow/persistence/base/storageIO.ts`:
- Around line 111-118: The deletePayload function should start by checking the
storageAvailable flag and return early if storage is unavailable to match other
functions' behavior; update deletePayload (which builds the key using
StorageKeys.prefixes.draftPayload and the workspaceId:draftKey pattern) to
include an if (!storageAvailable) return guard before attempting
localStorage.removeItem, leaving the try/catch only as backup for unexpected
runtime errors.
In `@src/platform/workflow/persistence/composables/useWorkflowTabState.ts`:
- Around line 22-24: getClientId currently returns null when both api.clientId
and api.initialClientId are absent, causing silent failures of storage; update
getClientId to detect this "API not initialized" edge case and, in development
mode, emit a clear warning (e.g., using console.warn or the project's logger)
that includes which properties are missing (api.clientId and
api.initialClientId) so developers can see why tab state isn't persisting; leave
the return behavior unchanged but only log in dev (use NODE_ENV or the project's
isDev helper) and reference getClientId, api.clientId, and api.initialClientId
when implementing the check and message.
In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts`:
- Around line 39-48: The interfaces DraftMeta and LoadPersistedWorkflowOptions
are currently unexported but are used by external callers of saveDraft and
loadPersistedWorkflow; export them (or move them into a shared draftTypes.ts and
export from there) so callers can import the types. Update the declarations for
DraftMeta and LoadPersistedWorkflowOptions to be exported and/or add
corresponding exports from draftTypes.ts, then update any imports in callers of
saveDraft/loadPersistedWorkflow to reference the exported types.
- Around line 134-182: The eviction loop in handleQuotaExceeded can run until
all drafts are evicted; add a max eviction limit to avoid runaway deletion:
introduce a constant like MAX_EVICTIONS (e.g., 10), add a counter (evictions =
0) and increment it each time you successfully remove an entry in the while loop
inside handleQuotaExceeded, and break out when evictions >= MAX_EVICTIONS (then
call markStorageUnavailable() and return false) so you stop evicting early;
ensure you still avoid evicting the draftKey and that any partially-updated
index state is persisted via persistIndex(finalIndex) only when a new payload
write succeeds (as currently done).
- Around line 260-275: The catch in tryLoadGraph currently logs the error
without context; update the error logging in function tryLoadGraph (which calls
comfyApp.loadGraphData) to include identifying context such as the workflowName
and a safe/truncated representation of payload (or the persistence path if
available) along with the error, and prefer the project logging utility (e.g.,
processLogger or a structured logger) over console.error so logs are structured
and actionable; ensure onFailure() is still invoked and the function returns
false on error.
📦 Bundle: 4.27 MB gzip ⚪ 0 BDetailsSummary
Category Glance App Entry Points — 21.4 kB (baseline 21.4 kB) • ⚪ 0 BMain entry bundles and manifests
Graph Workspace — 914 kB (baseline 914 kB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Views & Navigation — 68.6 kB (baseline 68.6 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Panels & Settings — 430 kB (baseline 430 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
User & Accounts — 16 kB (baseline 16 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Editors & Dialogs — 706 B (baseline 706 B) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
UI Components — 42.3 kB (baseline 42.3 kB) • ⚪ 0 BReusable component library chunks
Data & Services — 2.4 MB (baseline 2.4 MB) • ⚪ 0 BStores, services, APIs, and repositories
Utilities & Hooks — 57.6 kB (baseline 57.6 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Vendor & Third-Party — 8.7 MB (baseline 8.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.38 MB (baseline 7.38 MB) • ⚪ 0 BBundles that do not match a named category
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/platform/workflow/persistence/stores/workflowDraftStoreV2.test.ts (2)
124-142: Consider adding test formoveDraftwrite failure scenario.Given the data loss risk identified in the implementation, a test verifying behavior when
writePayloadfails during move would improve coverage and serve as a regression guard.Would you like me to draft a test case that mocks
writePayloadto fail and verifies the old payload is preserved?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.test.ts` around lines 124 - 142, Add a test for the moveDraft failure path that mocks the underlying write operation to throw and verifies the original draft is preserved; specifically, in workflowDraftStoreV2.test.ts create a new test alongside the existing moveDraft spec that uses useWorkflowDraftStoreV2, calls saveDraft('workflows/old.json', ...), stubs or mocks writePayload (the internal persistence method used by moveDraft) to throw an error when writing the new path, then call store.moveDraft('workflows/old.json','workflows/new.json','new') and assert the call results in the expected error/exception and that store.getDraft('workflows/old.json') still returns the original payload while store.getDraft('workflows/new.json') is null (ensuring no data loss).
80-100: Consider importingMAX_DRAFTSconstant for clarity.The test uses hardcoded
32which works but could drift if the constant changes. ImportingMAX_DRAFTSwould make the test self-documenting and resilient.♻️ Proposed improvement
+import { MAX_DRAFTS } from '../base/draftTypes' + // ... it('evicts oldest when over limit', () => { const store = useWorkflowDraftStoreV2() - // Save 32 drafts (MAX_DRAFTS) - for (let i = 0; i < 32; i++) { + // Save MAX_DRAFTS drafts + for (let i = 0; i < MAX_DRAFTS; i++) { store.saveDraft(`workflows/draft${i}.json`, `{"id":${i}}`, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.test.ts` around lines 80 - 100, The test hardcodes 32 instead of using the MAX_DRAFTS constant which can drift; update the test to import MAX_DRAFTS and use it when creating drafts and iterating (replace the literal 32 in the for loop and comment with MAX_DRAFTS) so the eviction assertion around useWorkflowDraftStoreV2.saveDraft and getDraft stays correct if MAX_DRAFTS changes.src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts (1)
148-152: Simplify entry lookup using direct key access.
entriesis keyed bydraftKey, so you can access the entry directly instead of iterating through all values.♻️ Proposed simplification
- const oldestEntry = Object.values(currentIndex.entries).find( - (e) => hashPath(e.path) === oldestKey - ) - if (!oldestEntry) break + const oldestEntry = currentIndex.entries[oldestKey] + if (!oldestEntry) break🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts` around lines 148 - 152, The lookup for the oldest entry is inefficiently scanning Object.values(currentIndex.entries) and comparing hashPath(e.path) === oldestKey; since currentIndex.entries is keyed by draftKey (the same key stored in oldestKey), replace the find with a direct property access currentIndex.entries[oldestKey] and keep the null check (if not found then break). Update references around hashPath(e.path) === oldestKey to use the direct key lookup to simplify and speed up eviction logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts`:
- Around line 200-218: The moveDraft function currently reads the old payload
(readPayload), calls writePayload to write to result.newKey, then
unconditionally calls deletePayload and persistIndex; if writePayload fails this
causes data loss. Change moveDraft (and the local flow around moveEntry,
readPayload, writePayload, deletePayload, persistIndex) to verify the
writePayload result or catch its thrown error before deleting the old payload:
attempt the write inside a try/catch (or check its return value), only call
deletePayload(workspaceId, result.oldKey) and persistIndex(result.index) after a
confirmed successful write, and on failure log the error and abort/return
without mutating the index or deleting the original payload (or propagate the
error) so the draft remains intact. Ensure workspaceId/result keys are preserved
in the error handling logic.
---
Nitpick comments:
In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.test.ts`:
- Around line 124-142: Add a test for the moveDraft failure path that mocks the
underlying write operation to throw and verifies the original draft is
preserved; specifically, in workflowDraftStoreV2.test.ts create a new test
alongside the existing moveDraft spec that uses useWorkflowDraftStoreV2, calls
saveDraft('workflows/old.json', ...), stubs or mocks writePayload (the internal
persistence method used by moveDraft) to throw an error when writing the new
path, then call store.moveDraft('workflows/old.json','workflows/new.json','new')
and assert the call results in the expected error/exception and that
store.getDraft('workflows/old.json') still returns the original payload while
store.getDraft('workflows/new.json') is null (ensuring no data loss).
- Around line 80-100: The test hardcodes 32 instead of using the MAX_DRAFTS
constant which can drift; update the test to import MAX_DRAFTS and use it when
creating drafts and iterating (replace the literal 32 in the for loop and
comment with MAX_DRAFTS) so the eviction assertion around
useWorkflowDraftStoreV2.saveDraft and getDraft stays correct if MAX_DRAFTS
changes.
In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts`:
- Around line 148-152: The lookup for the oldest entry is inefficiently scanning
Object.values(currentIndex.entries) and comparing hashPath(e.path) ===
oldestKey; since currentIndex.entries is keyed by draftKey (the same key stored
in oldestKey), replace the find with a direct property access
currentIndex.entries[oldestKey] and keep the null check (if not found then
break). Update references around hashPath(e.path) === oldestKey to use the
direct key lookup to simplify and speed up eviction logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts`:
- Around line 166-175: The index persistence result is ignored in the
quota-eviction branch: after calling upsertEntry and persistIndex in the block
around upsertEntry/persistIndex, check persistIndex's boolean result (same
pattern used in saveDraft) and if it returns false, remove the newly-written
payload (call the same cleanup function used elsewhere, e.g. deleteDraftPayload
or deletePayload) and return false instead of true; otherwise return true.
Ensure you reference the upsertEntry, persistIndex and the payload-delete helper
used by saveDraft so the behavior mirrors saveDraft's error handling.
---
Duplicate comments:
In `@src/platform/workflow/persistence/base/storageIO.ts`:
- Around line 36-62: isValidIndex only checks top-level types and lets
user-modified storage leak invalid shapes; extend runtime validation by adding
deeper guards (e.g., isValidPayload, isValidActivePath, isValidOpenPaths or Zod
schemas with safeParse) and use them inside readIndex (and any
payload/tab-pointer readers) to validate that order is an array of strings,
entries is a map whose values match the DraftPayload shape, and active/open path
pointers match expected string/array formats; if any of these new validators
fail, return null instead of casting/propagating the parsed JSON.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts`:
- Around line 142-152: The eviction loop in workflowDraftStoreV2.ts can
prematurely break when the draft being saved (draftKey) is the oldest
(currentIndex.order[0]), preventing other entries from being evicted; modify the
loop that uses currentIndex, currentIndex.order, oldestKey and draftKey so that
when oldestKey === draftKey you skip that key instead of breaking (e.g., advance
to the next key or remove that single key from the temporary scan list) and
continue eviction attempts until either space is freed or no other entries
remain; ensure you still avoid evicting the draft being saved and preserve the
original order semantics for remaining entries.
- Around line 317-328: Wrap direct sessionStorage/localStorage reads inside
try-catch in loadPersistedWorkflow in workflowDraftStoreV2 (around the
clientId/sessionPayload and localPayload reads) so thrown DOMExceptions become
soft misses: for the sessionStorage block, guard retrieving
`sessionStorage.getItem(\`workflow:${clientId}\`)` in a try-catch and only call
`tryLoadGraph(sessionPayload, workflowName)` if the read succeeded; for the
localStorage fallback, similarly guard `localStorage.getItem('workflow')` in a
try-catch and return await tryLoadGraph(localPayload, workflowName) only when
read succeeded, otherwise proceed as a miss (return false or continue the
existing flow) so storage access failures do not throw out of
loadPersistedWorkflow.
src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts`:
- Around line 203-218: moveDraft currently writes the new payload then deletes
the old payload before ensuring persistIndex succeeded, risking orphaned/new
payload loss; change the sequence to write the new payload (writePayload with
result.newKey), then call persistIndex(result.index) and only if persistIndex
succeeds deletePayload(workspaceId, result.oldKey); if persistIndex fails, roll
back by deleting the newly written payload (deletePayload(workspaceId,
result.newKey)) and return/fail gracefully; refer to moveDraft, loadIndex,
moveEntry, readPayload, writePayload, deletePayload, and persistIndex to locate
the change.
| function moveDraft(oldPath: string, newPath: string, name: string): void { | ||
| const index = loadIndex() | ||
| const result = moveEntry(index, oldPath, newPath, name) | ||
|
|
||
| if (result) { | ||
| const oldPayload = readPayload(workspaceId, result.oldKey) | ||
| if (oldPayload) { | ||
| const written = writePayload(workspaceId, result.newKey, { | ||
| data: oldPayload.data, | ||
| updatedAt: Date.now() | ||
| }) | ||
| if (!written) return | ||
| deletePayload(workspaceId, result.oldKey) | ||
| } | ||
| persistIndex(result.index) | ||
| } |
There was a problem hiding this comment.
Prevent draft loss when index persistence fails in moveDraft.
If the index write fails after the new payload is written, the old payload can be deleted while the stored index still points to the old key. On reload, the new payload becomes orphaned and is removed, losing the draft. Persist the index before deleting the old payload and roll back on failure.
🐛 Proposed fix
function moveDraft(oldPath: string, newPath: string, name: string): void {
const index = loadIndex()
const result = moveEntry(index, oldPath, newPath, name)
if (result) {
+ const previousIndex = indexCache.value ?? index
const oldPayload = readPayload(workspaceId, result.oldKey)
if (oldPayload) {
const written = writePayload(workspaceId, result.newKey, {
data: oldPayload.data,
updatedAt: Date.now()
})
if (!written) return
- deletePayload(workspaceId, result.oldKey)
- }
- persistIndex(result.index)
+ const persisted = persistIndex(result.index)
+ if (!persisted) {
+ indexCache.value = previousIndex
+ deletePayload(workspaceId, result.newKey)
+ return
+ }
+ deletePayload(workspaceId, result.oldKey)
+ return
+ }
+ if (!persistIndex(result.index)) {
+ indexCache.value = previousIndex
+ }
}
}As per coding guidelines: “Implement proper error handling”.
📝 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.
| function moveDraft(oldPath: string, newPath: string, name: string): void { | |
| const index = loadIndex() | |
| const result = moveEntry(index, oldPath, newPath, name) | |
| if (result) { | |
| const oldPayload = readPayload(workspaceId, result.oldKey) | |
| if (oldPayload) { | |
| const written = writePayload(workspaceId, result.newKey, { | |
| data: oldPayload.data, | |
| updatedAt: Date.now() | |
| }) | |
| if (!written) return | |
| deletePayload(workspaceId, result.oldKey) | |
| } | |
| persistIndex(result.index) | |
| } | |
| function moveDraft(oldPath: string, newPath: string, name: string): void { | |
| const index = loadIndex() | |
| const result = moveEntry(index, oldPath, newPath, name) | |
| if (result) { | |
| const previousIndex = indexCache.value ?? index | |
| const oldPayload = readPayload(workspaceId, result.oldKey) | |
| if (oldPayload) { | |
| const written = writePayload(workspaceId, result.newKey, { | |
| data: oldPayload.data, | |
| updatedAt: Date.now() | |
| }) | |
| if (!written) return | |
| const persisted = persistIndex(result.index) | |
| if (!persisted) { | |
| indexCache.value = previousIndex | |
| deletePayload(workspaceId, result.newKey) | |
| return | |
| } | |
| deletePayload(workspaceId, result.oldKey) | |
| return | |
| } | |
| if (!persistIndex(result.index)) { | |
| indexCache.value = previousIndex | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/platform/workflow/persistence/stores/workflowDraftStoreV2.ts` around
lines 203 - 218, moveDraft currently writes the new payload then deletes the old
payload before ensuring persistIndex succeeded, risking orphaned/new payload
loss; change the sequence to write the new payload (writePayload with
result.newKey), then call persistIndex(result.index) and only if persistIndex
succeeds deletePayload(workspaceId, result.oldKey); if persistIndex fails, roll
back by deleting the newly written payload (deletePayload(workspaceId,
result.newKey)) and return/fail gracefully; refer to moveDraft, loadIndex,
moveEntry, readPayload, writePayload, deletePayload, and persistIndex to locate
the change.
## Summary Adds the Pinia store for managing workflow drafts and a composable for tracking open workflow tabs per browser tab. Uses sessionStorage for tab-specific state to support multiple ComfyUI tabs without conflicts. ## Changes - **What**: - `workflowDraftStoreV2.ts` - Pinia store wrapping the LRU cache with save/load/remove operations - `useWorkflowTabState.ts` - Composable for tracking active workflow path and open tabs in sessionStorage (scoped by clientId) - **Why**: Browser tabs need independent workflow state, but the current system uses shared localStorage keys causing tab conflicts ## Review Focus - Store API design in `workflowDraftStoreV2.ts` - Session vs local storage split in `useWorkflowTabState.ts` --- *Part 3 of 4 in the workflow persistence improvements stack* --------- Co-authored-by: Amp <amp@ampcode.com>
- Category sidebar: nested expanded subfolders wrap in bg-secondary-background container - Chevrons appear on sidebar hover for categories with children - Filter chips: three visual states (active/applied/default) with applied count - Preview card pricing badge truncation for overflow - Pass appliedFilters prop to NodeSearchFilterBar - Fix paddingLeft to preserve depth hierarchy when chevrons are visible - Deduplicate button template in NodeSearchCategoryTreeNode - Restore draftTypes.ts removed by merge conflict with #8993/#8519 Amp-Thread-ID: https://ampcode.com/threads/T-019c8410-57a4-7432-85f0-44ffb23788e2
- Category sidebar: nested expanded subfolders wrap in bg-secondary-background container - Chevrons appear on sidebar hover for categories with children - Filter chips: three visual states (active/applied/default) with applied count - Preview card pricing badge truncation for overflow - Pass appliedFilters prop to NodeSearchFilterBar - Fix paddingLeft to preserve depth hierarchy when chevrons are visible - Deduplicate button template in NodeSearchCategoryTreeNode - Restore draftTypes.ts removed by merge conflict with #8993/#8519 Amp-Thread-ID: https://ampcode.com/threads/T-019c8410-57a4-7432-85f0-44ffb23788e2
- Category sidebar: nested expanded subfolders wrap in bg-secondary-background container - Chevrons appear on sidebar hover for categories with children - Filter chips: three visual states (active/applied/default) with applied count - Preview card pricing badge truncation for overflow - Pass appliedFilters prop to NodeSearchFilterBar - Fix paddingLeft to preserve depth hierarchy when chevrons are visible - Deduplicate button template in NodeSearchCategoryTreeNode - Restore draftTypes.ts removed by merge conflict with #8993/#8519 Amp-Thread-ID: https://ampcode.com/threads/T-019c87f4-aa28-7290-bdf0-ea5f86aacde3

Summary
Adds the Pinia store for managing workflow drafts and a composable for tracking open workflow tabs per browser tab. Uses sessionStorage for tab-specific state to support multiple ComfyUI tabs without conflicts.
Changes
workflowDraftStoreV2.ts- Pinia store wrapping the LRU cache with save/load/remove operationsuseWorkflowTabState.ts- Composable for tracking active workflow path and open tabs in sessionStorage (scoped by clientId)Review Focus
workflowDraftStoreV2.tsuseWorkflowTabState.tsPart 3 of 4 in the workflow persistence improvements stack