fix: add LRU eviction for session storage to prevent QuotaExceededError#8101
fix: add LRU eviction for session storage to prevent QuotaExceededError#8101christian-byrne wants to merge 7 commits intomainfrom
Conversation
Implement LRU-based session storage service for workflow data to prevent QuotaExceededError that was breaking workspace/teams features on mobile. Changes: - Add sessionStorageLruService with try-evict-retry logic - Store workflow data with embedded accessedAt timestamps for LRU tracking - Evict oldest entries when quota is exceeded (legacy entries first) - Graceful degradation: log warnings instead of throwing on persistent failures - Backward compatibility: legacy unwrapped data treated as oldest Fixes: https://comfy-org.sentry.io/issues/6955016837/ Amp-Thread-ID: https://ampcode.com/threads/T-019bc566-eb91-7545-a462-658c2b33083b Co-authored-by: Amp <amp@ampcode.com>
dosubot
bot
added
the
size:XL
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
📝 WalkthroughWalkthroughRefactors workflow persistence to store JSON-structured workflow payloads and adds a session-storage service with eviction and access-tracking. The composable now uses eviction-backed session storage when Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Composable as useWorkflowPersistence
participant SessionSvc as workflowSessionStorageService
participant SessionStorage as Browser SessionStorage
participant LocalStorage as Browser LocalStorage
User->>Composable: Save workflow (ComfyWorkflowJSON)
Composable->>SessionSvc: setWithEviction(key, workflowJSON)
SessionSvc->>SessionStorage: store wrapped {accessedAt, data}
alt QuotaExceededError
SessionStorage-->>SessionSvc: QuotaExceededError
SessionSvc->>SessionSvc: getEvictableKeys(evictionPattern)
SessionSvc->>SessionSvc: evictOldestEntries(...)
SessionSvc->>SessionStorage: remove(oldest)
SessionSvc->>SessionStorage: retry store
SessionStorage-->>SessionSvc: success / error
end
SessionSvc-->>Composable: success/failure
User->>Composable: Load workflow
Composable->>SessionSvc: getWithAccessTracking(key, updateAccessTime=true)
SessionSvc->>SessionStorage: retrieve wrapped
alt Found in session
SessionStorage-->>SessionSvc: {accessedAt, data}
SessionSvc->>SessionStorage: update accessedAt wrapper
SessionSvc-->>Composable: ComfyWorkflowJSON
else Not in session
Composable->>LocalStorage: getItem(key)
LocalStorage-->>Composable: raw JSON string/null
Composable->>Composable: safe JSON.parse -> ComfyWorkflowJSON / warn
Composable-->>User: workflow or failure
end
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright Tests: ❌ FailedResults: 501 passed, 3 failed, 0 flaky, 8 skipped (Total: 512) ❌ Failed Tests📊 Browser Reports
|
🎨 Storybook Build Status✅ Build completed successfully! ⏰ Completed at: 01/28/2026, 05:03:37 AM UTC 🔗 Links🎉 Your Storybook is ready for review! |
coderabbitai
bot
requested review from
AustinMroz,
DrJKL,
KarryCharon,
Yorha4D,
jtydhr88 and
shinshin86
Bundle Size ReportSummary
Category Glance Per-category breakdownApp Entry Points — 25.9 kB (baseline 25.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 959 kB (baseline 960 kB) • 🟢 -613 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 80.7 kB (baseline 80.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed Panels & Settings — 471 kB (baseline 471 kB) • 🟢 -8 BConfiguration panels, inspectors, and settings screens
Status: 12 added / 12 removed User & Accounts — 3.94 kB (baseline 3.94 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 3 added / 3 removed Editors & Dialogs — 2.86 kB (baseline 2.86 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 33.7 kB (baseline 33.7 kB) • ⚪ 0 BReusable component library chunks
Status: 4 added / 4 removed Data & Services — 2.71 MB (baseline 2.71 MB) • 🔴 +124 BStores, services, APIs, and repositories
Status: 8 added / 8 removed Utilities & Hooks — 25.3 kB (baseline 25.3 kB) • ⚪ 0 BHelpers, composables, and utility bundles
Status: 7 added / 7 removed Vendor & Third-Party — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Other — 7.04 MB (baseline 7.04 MB) • 🟢 -195 BBundles that do not match a named category
Status: 34 added / 34 removed |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/platform/workflow/persistence/services/sessionStorageLruService.ts`:
- Around line 28-30: The function isProtectedKey currently checks
PROTECTED_KEY_PREFIXES.some(prefix => key.includes(prefix)), which matches the
prefix anywhere in the key; if the intent is true prefix matching change the
check to use startsWith instead of includes (i.e., in isProtectedKey replace the
includes-based predicate with prefix => key.startsWith(prefix)); if the broader
"contains" behavior is intended, add a clarifying comment above
PROTECTED_KEY_PREFIXES explaining that entries may appear anywhere in the key.
- Around line 205-211: The comparison entry.accessedAt !== Date.now() is
pointless because Date.now() changes each call; in sessionStorageLruService.ts
inside the block guarded by updateAccessTime, remove the Date.now() equality
check and always update entry.accessedAt = Date.now() when updateAccessTime is
true, then write back via sessionStorage.setItem(key,
wrapForStorage(entry.data)) inside the existing try/catch to avoid quota errors;
reference symbols: updateAccessTime, entry.accessedAt, Date.now(),
sessionStorage.setItem, wrapForStorage, key.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/services/sessionStorageLruService.ts
🧰 Additional context used
📓 Path-based instructions (11)
src/**/*.{vue,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{vue,ts}: Leverage VueUse functions for performance-enhancing styles
Implement proper error handling
Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
Files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety
src/**/*.ts: Derive component types usingvue-component-type-helpers(ComponentProps,ComponentSlots) instead of separate type files
Use es-toolkit for utility functions
Minimize the surface area (exported values) of each module and composable
Favor pure functions, especially testable ones
Files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
src/**/{services,composables}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/{services,composables}/**/*.{ts,tsx}: Useapi.apiURL()for backend endpoints instead of constructing URLs directly
Useapi.fileURL()for static file access instead of constructing URLs directly
Files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.{ts,tsx,vue}: Sanitize HTML with DOMPurify to prevent XSS attacks
Avoid using@ts-expect-error; use proper TypeScript types instead
Use es-toolkit for utility functions instead of other utility libraries
Implement proper TypeScript types throughout the codebase
src/**/*.{ts,tsx,vue}: Use separateimport typestatements instead of inlinetypein mixed imports
Apply Prettier formatting with 2-space indentation, single quotes, no trailing semicolons, 80-character width
Sort and group imports by plugin, runpnpm formatbefore committing
Never useanytype - use proper TypeScript types
Never useas anytype assertions - fix the underlying type issue
Write code that is expressive and self-documenting - avoid unnecessary comments
Do not add or retain redundant comments - clean as you go
Avoid mutable state - prefer immutability and assignment at point of declaration
Watch out for Code Smells and refactor to avoid them
Files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Follow Vue 3 composition API style guide
Files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
src/**/*.{ts,vue}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,vue}: Usereffor reactive state,computed()for derived values, andwatch/watchEffectfor side effects in Composition API
Avoid usingrefwithwatchif acomputedwould suffice - minimize refs and derived state
Useprovide/injectfor dependency injection only when simpler alternatives (Store or shared composable) won't work
Leverage VueUse functions for performance-enhancing composables
Use VueUse function for useI18n in composition API for string literals
Files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Keep functions short and functional
Minimize nesting (if statements, for loops, etc.)
Use function declarations instead of function expressions when possible
Files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
+(tests-ui|src)/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
+(tests-ui|src)/**/*.test.ts: Unit and component tests belong intests-ui/orsrc/**/*.test.tsusing Vitest
Write tests for all changes, especially bug fixes to catch future regressions
Do not write tests dependent on non-behavioral features like utility classes or styles
Do not write tests that just test the mocks - ensure tests fail when code behaves unexpectedly
Leverage Vitest's utilities for mocking where possible
Keep module mocks contained - do not use global mutable state within test files; usevi.hoisted()if necessary
Use Vue Test Utils for Component testing and follow best practices for making components easy to test
Aim for behavioral coverage of critical and new features in unit tests
Files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
+(tests-ui|src|browser_tests)/**/*.+(test.ts|spec.ts)
📄 CodeRabbit inference engine (AGENTS.md)
+(tests-ui|src|browser_tests)/**/*.+(test.ts|spec.ts): Do not write change detector tests - avoid tests that only assert default values
Be parsimonious in testing - do not write redundant tests
Don't Mock What You Don't Own
Files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
src/**/{composables,components}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Clean up subscriptions in state management to prevent memory leaks
Files:
src/platform/workflow/persistence/composables/useWorkflowPersistence.ts
src/**/{components,composables}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Use vue-i18n for ALL user-facing strings by adding them to
src/locales/en/main.json
Files:
src/platform/workflow/persistence/composables/useWorkflowPersistence.ts
🧠 Learnings (21)
📚 Learning: 2025-11-24T19:47:34.324Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:34.324Z
Learning: Applies to src/**/stores/**/*.{ts,tsx} : Maintain clear public interfaces and restrict extension access in stores
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.ts
📚 Learning: 2025-12-09T03:39:54.501Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
📚 Learning: 2025-12-13T11:03:11.264Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7416
File: src/stores/imagePreviewStore.ts:5-7
Timestamp: 2025-12-13T11:03:11.264Z
Learning: In the ComfyUI_frontend repository, lint rules require keeping 'import type' statements separate from non-type imports, even if importing from the same module. Do not suggest consolidating them into a single import statement. Ensure type imports remain on their own line (import type { ... } from 'module') and regular imports stay on separate lines.
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
📚 Learning: 2025-12-17T00:40:09.635Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.stories.ts:45-55
Timestamp: 2025-12-17T00:40:09.635Z
Learning: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository. Function declarations are more functional-leaning, offer better hoisting clarity, and can improve readability and tooling consistency. Apply this guideline across TypeScript files in Comfy-Org/ComfyUI_frontend, including story and UI component code, except where a function expression is semantically required (e.g., callbacks, higher-order functions with closures).
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
📚 Learning: 2025-12-30T22:22:33.836Z
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 7805
File: src/composables/useCoreCommands.ts:439-439
Timestamp: 2025-12-30T22:22:33.836Z
Learning: When accessing reactive properties from Pinia stores in TypeScript files, avoid using .value on direct property access (e.g., useStore().isOverlayExpanded). Pinia auto-wraps refs when accessed directly, returning the primitive value. The .value accessor is only needed when destructuring store properties or when using storeToRefs().
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
📚 Learning: 2025-12-11T12:25:15.470Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
📚 Learning: 2026-01-12T17:39:27.738Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7906
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:545-552
Timestamp: 2026-01-12T17:39:27.738Z
Learning: In Vue/TypeScript files (src/**/*.{ts,tsx,vue}), prefer if/else statements over ternary operators when performing side effects or actions (e.g., mutating state, calling methods with side effects). Ternaries should be reserved for computing and returning values.
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.tssrc/platform/workflow/persistence/services/sessionStorageLruService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src)/**/*.test.ts : Leverage Vitest's utilities for mocking where possible
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src)/**/*.test.ts : Write tests for all changes, especially bug fixes to catch future regressions
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src)/**/*.test.ts : Aim for behavioral coverage of critical and new features in unit tests
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src)/**/*.test.ts : Keep module mocks contained - do not use global mutable state within test files; use `vi.hoisted()` if necessary
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src)/**/*.test.ts : Unit and component tests belong in `tests-ui/` or `src/**/*.test.ts` using Vitest
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src|browser_tests)/**/*.+(test.ts|spec.ts) : Be parsimonious in testing - do not write redundant tests
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Use `vitest` for unit testing in this project
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2025-11-24T19:47:56.371Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: src/lib/litegraph/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:56.371Z
Learning: Applies to src/lib/litegraph/**/*.{test,spec}.{js,ts,jsx,tsx} : When adding features, always write vitest unit tests using cursor rules in @.cursor
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src)/**/*.test.ts : Do not write tests dependent on non-behavioral features like utility classes or styles
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2025-12-10T03:09:13.807Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7303
File: src/components/topbar/CurrentUserPopover.test.ts:199-205
Timestamp: 2025-12-10T03:09:13.807Z
Learning: In test files, prefer selecting or asserting on accessible properties (text content, aria-label, role, accessible name) over data-testid attributes. This ensures tests validate actual user-facing behavior and accessibility, reducing reliance on implementation details like test IDs.
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2025-12-30T01:31:04.927Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7797
File: tests-ui/tests/lib/litegraph/src/widgets/ComboWidget.test.ts:648-648
Timestamp: 2025-12-30T01:31:04.927Z
Learning: In Vitest v4, when mocking functions that may be called as constructors (using new), the mock implementation must use function() or class syntax rather than an arrow function. Arrow mocks can cause '<anonymous> is not a constructor' errors. This is a breaking change from Vitest v3 where mocks could use an arrow function. Apply this guideline to test files that mock constructor-like calls (e.g., in tests under tests-ui, such as ComboWidget.test.ts) and ensure mock implementations are defined with function() { ... } or class { ... } to preserve constructor behavior.
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2026-01-08T02:40:15.482Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7894
File: src/renderer/extensions/vueNodes/widgets/components/WidgetToggleSwitch.test.ts:11-14
Timestamp: 2026-01-08T02:40:15.482Z
Learning: In TypeScript test files (e.g., any test under src), avoid duplicating interface/type definitions. Import real type definitions from the component modules under test and reference them directly, so there is a single source of truth and to prevent type drift. This improves maintainability and consistency across tests.
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
📚 Learning: 2026-01-09T02:07:54.558Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7898
File: src/composables/usePaste.test.ts:248-248
Timestamp: 2026-01-09T02:07:54.558Z
Learning: In test files (e.g., any .test.ts or .test.tsx under src/...), when you create mock objects that partially implement an interface (such as LGraphNode), prefer casting with as Partial<InterfaceType> as InterfaceType rather than as any or as unknown as InterfaceType. This makes the incomplete implementation explicit while preserving type safety, improving readability and maintainability of tests.
Applied to files:
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts
🧬 Code graph analysis (2)
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts (1)
src/platform/workflow/persistence/services/sessionStorageLruService.ts (4)
setWithLruEviction(146-186)getWithLruTracking(195-218)removeFromStorage(223-225)getStorageStats(230-280)
src/platform/workflow/persistence/composables/useWorkflowPersistence.ts (4)
src/scripts/api.ts (1)
api(1304-1304)src/platform/workflow/persistence/services/sessionStorageLruService.ts (3)
setWithLruEviction(146-186)getStorageStats(230-280)getWithLruTracking(195-218)src/platform/workflow/validation/schemas/workflowSchema.ts (1)
ComfyWorkflowJSON(459-461)src/scripts/utils.ts (1)
getStorageValue(96-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test
- GitHub Check: lint-and-format
- GitHub Check: setup
- GitHub Check: collect
🔇 Additional comments (17)
src/platform/workflow/persistence/services/sessionStorageLruService.test.ts (5)
1-18: LGTM! Test setup follows Vitest best practices.The setup uses
vi.clearAllMocks()inbeforeEachandvi.restoreAllMocks()inafterEach, ensuring clean isolation between tests. Based on learnings, this leverages Vitest's utilities appropriately.
42-69: Well-structured eviction test with proper mock isolation.The mock correctly simulates
QuotaExceededErroron the first attempt, then allows subsequent writes via the originalsetItem. The assertions verify both the eviction (oldest entry removed) and successful storage (newer entry preserved). This tests the actual LRU eviction behavior.
97-103: Good coverage for legacy format backward compatibility.This test ensures legacy entries (raw workflow JSON without LRU wrapper) are handled correctly, which is critical for the migration period mentioned in the PR objectives.
166-188: Critical tests for LRU ordering semantics.These tests verify that entries are sorted by
accessedAt(oldest first) and that legacy entries receiveaccessedAt: 0, ensuring they're prioritized for eviction. This is essential for the migration strategy.
191-237: Comprehensive LRU eviction order verification.These tests confirm the eviction priority: legacy entries (with
accessedAt: 0) are evicted before newer entries, and among new-format entries, oldest are evicted first. This validates the core LRU behavior.src/platform/workflow/persistence/services/sessionStorageLruService.ts (8)
1-26: LGTM! Well-documented module with clear deprecation timeline.The JSDoc header clearly explains the module's purpose and the deprecation notice for legacy format support provides a clear migration path. The
StorageEntry<T>interface correctly uses generics for type safety.
39-43: LGTM! Legacy format detection is straightforward.The check for both
accessedAtanddataproperties correctly distinguishes between legacy raw workflow JSON and the new wrapped format.
62-73: LGTM! Legacy handling with eviction priority.Assigning
accessedAt: 0to legacy entries ensures they're evicted first during the migration period, which is a smart approach for backward compatibility.
88-101: Good defensive error handling for malformed entries.Entries that fail to parse are still included with
accessedAt: 0, ensuring they can be evicted. This prevents corrupt entries from blocking storage operations.
111-136: LGTM! Core LRU eviction logic is correct.The entries are sorted by
accessedAtascending (oldest first), filtered to exclude the current key, and evicted in batches. The freed bytes calculation enables retry decisions.
146-186: LGTM! Robust retry logic with escalating eviction.The implementation correctly:
- Attempts storage first before any eviction
- Escalates eviction count (1 → 2 → 3 entries) on successive failures
- Bails out early if no entries are available for eviction
- Distinguishes
QuotaExceededErrorfrom other errors
222-225: LGTM! Simple and correct.
229-279: LGTM! Comprehensive storage statistics.The size estimation using
raw.length * 2correctly accounts for JavaScript's UTF-16 string encoding. The function provides useful debugging information while maintaining consistency with the eviction logic (unparseable entries getaccessedAt: 0).src/platform/workflow/persistence/composables/useWorkflowPersistence.ts (4)
14-25: LGTM! Imports follow coding guidelines.The type import
ComfyWorkflowJSONis correctly on a separate line from non-type imports, adhering to the repository's lint rules. Based on learnings, this pattern is required.
53-86: LGTM! Dual storage strategy with LRU for session-specific persistence.The implementation correctly:
- Always persists to localStorage as a fallback (line 59)
- Uses LRU-backed sessionStorage when
api.clientIdexists (lines 67-85)- Logs comprehensive debug info on failure including workflow size and storage stats
88-95: LGTM! Clean refactor with proper typing.The function now accepts
ComfyWorkflowJSON | nulldirectly, eliminating the need for internal JSON parsing. The null guard ensures safe usage.
97-125: LGTM! Robust loading with session-first, localStorage-fallback strategy.The implementation:
- Tries LRU-tracked sessionStorage first (client-specific)
- Falls back to localStorage (shared across sessions)
- Handles JSON parse failures gracefully with a warning
The type assertion
as ComfyWorkflowJSONat line 115 is acceptable since JSON.parse returnsunknownand validation would add overhead for trusted local data.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| function isProtectedKey(key: string): boolean { | ||
| return PROTECTED_KEY_PREFIXES.some((prefix) => key.includes(prefix)) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify: includes vs startsWith for protected key matching.
The constant is named PROTECTED_KEY_PREFIXES, but includes() matches anywhere in the string, not just at the start. If keys like foo.workspace.bar should also be protected, this is correct. Otherwise, consider using startsWith() for true prefix matching.
♻️ If prefix-only matching is intended
function isProtectedKey(key: string): boolean {
- return PROTECTED_KEY_PREFIXES.some((prefix) => key.includes(prefix))
+ return PROTECTED_KEY_PREFIXES.some((prefix) => key.startsWith(prefix))
}📝 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 isProtectedKey(key: string): boolean { | |
| return PROTECTED_KEY_PREFIXES.some((prefix) => key.includes(prefix)) | |
| } | |
| function isProtectedKey(key: string): boolean { | |
| return PROTECTED_KEY_PREFIXES.some((prefix) => key.startsWith(prefix)) | |
| } |
🤖 Prompt for AI Agents
In `@src/platform/workflow/persistence/services/sessionStorageLruService.ts`
around lines 28 - 30, The function isProtectedKey currently checks
PROTECTED_KEY_PREFIXES.some(prefix => key.includes(prefix)), which matches the
prefix anywhere in the key; if the intent is true prefix matching change the
check to use startsWith instead of includes (i.e., in isProtectedKey replace the
includes-based predicate with prefix => key.startsWith(prefix)); if the broader
"contains" behavior is intended, add a clarifying comment above
PROTECTED_KEY_PREFIXES explaining that entries may appear anywhere in the key.
| if (updateAccessTime && entry.accessedAt !== Date.now()) { | ||
| try { | ||
| sessionStorage.setItem(key, wrapForStorage(entry.data)) | ||
| } catch { | ||
| // Ignore quota errors when updating access time | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The Date.now() comparison is effectively always true.
The condition entry.accessedAt !== Date.now() will almost always evaluate to true since Date.now() returns the current time at evaluation, not the time when the entry was stored. This comparison seems intended to skip unnecessary writes, but it provides minimal benefit and adds complexity.
Consider simplifying to just check updateAccessTime:
♻️ Simplified access time update
- if (updateAccessTime && entry.accessedAt !== Date.now()) {
+ if (updateAccessTime) {
try {
sessionStorage.setItem(key, wrapForStorage(entry.data))
} catch {
// Ignore quota errors when updating access time
}
}📝 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.
| if (updateAccessTime && entry.accessedAt !== Date.now()) { | |
| try { | |
| sessionStorage.setItem(key, wrapForStorage(entry.data)) | |
| } catch { | |
| // Ignore quota errors when updating access time | |
| } | |
| } | |
| if (updateAccessTime) { | |
| try { | |
| sessionStorage.setItem(key, wrapForStorage(entry.data)) | |
| } catch { | |
| // Ignore quota errors when updating access time | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/platform/workflow/persistence/services/sessionStorageLruService.ts`
around lines 205 - 211, The comparison entry.accessedAt !== Date.now() is
pointless because Date.now() changes each call; in sessionStorageLruService.ts
inside the block guarded by updateAccessTime, remove the Date.now() equality
check and always update entry.accessedAt = Date.now() when updateAccessTime is
true, then write back via sessionStorage.setItem(key,
wrapForStorage(entry.data)) inside the existing try/catch to avoid quota errors;
reference symbols: updateAccessTime, entry.accessedAt, Date.now(),
sessionStorage.setItem, wrapForStorage, key.
|
Keeping out of draft so I can get CR review. But can be considered draft. |
- Rename to workflowSessionStorageService with semantic names - Remove verbose JSDoc and byte tracking - Streamline logging to specific error cases - Simplify eviction flow (280 → 115 lines) Amp-Thread-ID: https://ampcode.com/threads/T-019bc597-fbea-778a-ae15-92061b3d0812 Co-authored-by: Amp <amp@ampcode.com>
dosubot
bot
added
size:L
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/platform/workflow/persistence/composables/useWorkflowPersistence.ts`:
- Around line 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.
In
`@src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts`:
- Around line 61-70: Remove the redundant explicit mock restoration calls in the
tests: delete the spy.mockRestore() invocations in the test cases that mock
sessionStorage.setItem (the tests calling setWithEviction('key', ...) and the
other similar test), since the test suite already uses vi.restoreAllMocks() in
afterEach; leave the spy creation (vi.spyOn(...).mockImplementation(...)) and
assertions intact and rely on vi.restoreAllMocks() to restore the mocks
automatically.
In `@src/platform/workflow/persistence/services/workflowSessionStorageService.ts`:
- Around line 12-16: isProtectedKey currently uses PROTECTED_KEY_PREFIXES with
Array.prototype.includes which checks anywhere in the string; change the match
to prefix-only by using String.prototype.startsWith instead (i.e., in function
isProtectedKey replace key.includes(prefix) with key.startsWith(prefix)) so keys
are only considered protected when they begin with the listed prefixes (keep the
existing PROTECTED_KEY_PREFIXES values like 'workspace.' and 'Workspace.').
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
src/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
🧰 Additional context used
📓 Path-based instructions (11)
src/**/*.{vue,ts}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.{vue,ts}: Leverage VueUse functions for performance-enhancing styles
Implement proper error handling
Use vue-i18n in composition API for any string literals. Place new translation entries in src/locales/en/main.json
Files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
src/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/**/*.ts: Use es-toolkit for utility functions
Use TypeScript for type safety
src/**/*.ts: Derive component types usingvue-component-type-helpers(ComponentProps,ComponentSlots) instead of separate type files
Use es-toolkit for utility functions
Minimize the surface area (exported values) of each module and composable
Favor pure functions, especially testable ones
Files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
src/**/{services,composables}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/{services,composables}/**/*.{ts,tsx}: Useapi.apiURL()for backend endpoints instead of constructing URLs directly
Useapi.fileURL()for static file access instead of constructing URLs directly
Files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
src/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.{ts,tsx,vue}: Sanitize HTML with DOMPurify to prevent XSS attacks
Avoid using@ts-expect-error; use proper TypeScript types instead
Use es-toolkit for utility functions instead of other utility libraries
Implement proper TypeScript types throughout the codebase
src/**/*.{ts,tsx,vue}: Use separateimport typestatements instead of inlinetypein mixed imports
Apply Prettier formatting with 2-space indentation, single quotes, no trailing semicolons, 80-character width
Sort and group imports by plugin, runpnpm formatbefore committing
Never useanytype - use proper TypeScript types
Never useas anytype assertions - fix the underlying type issue
Write code that is expressive and self-documenting - avoid unnecessary comments
Do not add or retain redundant comments - clean as you go
Avoid mutable state - prefer immutability and assignment at point of declaration
Watch out for Code Smells and refactor to avoid them
Files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
src/**/*.{vue,ts,tsx}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Follow Vue 3 composition API style guide
Files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
+(tests-ui|src)/**/*.test.ts
📄 CodeRabbit inference engine (AGENTS.md)
+(tests-ui|src)/**/*.test.ts: Unit and component tests belong intests-ui/orsrc/**/*.test.tsusing Vitest
Write tests for all changes, especially bug fixes to catch future regressions
Do not write tests dependent on non-behavioral features like utility classes or styles
Do not write tests that just test the mocks - ensure tests fail when code behaves unexpectedly
Leverage Vitest's utilities for mocking where possible
Keep module mocks contained - do not use global mutable state within test files; usevi.hoisted()if necessary
Use Vue Test Utils for Component testing and follow best practices for making components easy to test
Aim for behavioral coverage of critical and new features in unit tests
Files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
src/**/*.{ts,vue}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,vue}: Usereffor reactive state,computed()for derived values, andwatch/watchEffectfor side effects in Composition API
Avoid usingrefwithwatchif acomputedwould suffice - minimize refs and derived state
Useprovide/injectfor dependency injection only when simpler alternatives (Store or shared composable) won't work
Leverage VueUse functions for performance-enhancing composables
Use VueUse function for useI18n in composition API for string literals
Files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{ts,tsx}: Keep functions short and functional
Minimize nesting (if statements, for loops, etc.)
Use function declarations instead of function expressions when possible
Files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
+(tests-ui|src|browser_tests)/**/*.+(test.ts|spec.ts)
📄 CodeRabbit inference engine (AGENTS.md)
+(tests-ui|src|browser_tests)/**/*.+(test.ts|spec.ts): Do not write change detector tests - avoid tests that only assert default values
Be parsimonious in testing - do not write redundant tests
Don't Mock What You Don't Own
Files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
src/**/{composables,components}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Clean up subscriptions in state management to prevent memory leaks
Files:
src/platform/workflow/persistence/composables/useWorkflowPersistence.ts
src/**/{components,composables}/**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (src/CLAUDE.md)
Use vue-i18n for ALL user-facing strings by adding them to
src/locales/en/main.json
Files:
src/platform/workflow/persistence/composables/useWorkflowPersistence.ts
🧠 Learnings (16)
📚 Learning: 2025-11-24T19:47:22.909Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: browser_tests/CLAUDE.md:0-0
Timestamp: 2025-11-24T19:47:22.909Z
Learning: Applies to browser_tests/**/*.{e2e,spec}.{ts,tsx,js,jsx} : Test user workflows in browser tests
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src)/**/*.test.ts : Write tests for all changes, especially bug fixes to catch future regressions
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src)/**/*.test.ts : Aim for behavioral coverage of critical and new features in unit tests
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src)/**/*.test.ts : Keep module mocks contained - do not use global mutable state within test files; use `vi.hoisted()` if necessary
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
📚 Learning: 2026-01-10T00:24:17.695Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T00:24:17.695Z
Learning: Applies to +(tests-ui|src)/**/*.test.ts : Leverage Vitest's utilities for mocking where possible
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
📚 Learning: 2025-11-24T19:48:09.318Z
Learnt from: CR
Repo: Comfy-Org/ComfyUI_frontend PR: 0
File: .cursor/rules/unit-test.mdc:0-0
Timestamp: 2025-11-24T19:48:09.318Z
Learning: Applies to test/**/*.{test,spec}.{js,ts,jsx,tsx} : Use `vitest` for unit testing in this project
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
📚 Learning: 2025-12-09T03:39:54.501Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7169
File: src/platform/remote/comfyui/jobs/jobTypes.ts:1-107
Timestamp: 2025-12-09T03:39:54.501Z
Learning: In the ComfyUI_frontend project, Zod is on v3.x. Do not suggest Zod v4 standalone validators (z.uuid, z.ulid, z.cuid2, z.nanoid) until an upgrade to Zod 4 is performed. When reviewing TypeScript files (e.g., src/platform/remote/comfyui/jobs/jobTypes.ts) validate against Zod 3 capabilities and avoid introducing v4-specific features; flag any proposal to upgrade or incorporate v4-only validators and propose staying with compatible 3.x patterns.
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
📚 Learning: 2025-12-13T11:03:11.264Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7416
File: src/stores/imagePreviewStore.ts:5-7
Timestamp: 2025-12-13T11:03:11.264Z
Learning: In the ComfyUI_frontend repository, lint rules require keeping 'import type' statements separate from non-type imports, even if importing from the same module. Do not suggest consolidating them into a single import statement. Ensure type imports remain on their own line (import type { ... } from 'module') and regular imports stay on separate lines.
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
📚 Learning: 2025-12-17T00:40:09.635Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7537
File: src/components/ui/button/Button.stories.ts:45-55
Timestamp: 2025-12-17T00:40:09.635Z
Learning: Prefer pure function declarations over function expressions (e.g., use function foo() { ... } instead of const foo = () => { ... }) for pure functions in the repository. Function declarations are more functional-leaning, offer better hoisting clarity, and can improve readability and tooling consistency. Apply this guideline across TypeScript files in Comfy-Org/ComfyUI_frontend, including story and UI component code, except where a function expression is semantically required (e.g., callbacks, higher-order functions with closures).
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
📚 Learning: 2025-12-30T22:22:33.836Z
Learnt from: kaili-yang
Repo: Comfy-Org/ComfyUI_frontend PR: 7805
File: src/composables/useCoreCommands.ts:439-439
Timestamp: 2025-12-30T22:22:33.836Z
Learning: When accessing reactive properties from Pinia stores in TypeScript files, avoid using .value on direct property access (e.g., useStore().isOverlayExpanded). Pinia auto-wraps refs when accessed directly, returning the primitive value. The .value accessor is only needed when destructuring store properties or when using storeToRefs().
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
📚 Learning: 2025-12-10T03:09:13.807Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7303
File: src/components/topbar/CurrentUserPopover.test.ts:199-205
Timestamp: 2025-12-10T03:09:13.807Z
Learning: In test files, prefer selecting or asserting on accessible properties (text content, aria-label, role, accessible name) over data-testid attributes. This ensures tests validate actual user-facing behavior and accessibility, reducing reliance on implementation details like test IDs.
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
📚 Learning: 2025-12-30T01:31:04.927Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7797
File: tests-ui/tests/lib/litegraph/src/widgets/ComboWidget.test.ts:648-648
Timestamp: 2025-12-30T01:31:04.927Z
Learning: In Vitest v4, when mocking functions that may be called as constructors (using new), the mock implementation must use function() or class syntax rather than an arrow function. Arrow mocks can cause '<anonymous> is not a constructor' errors. This is a breaking change from Vitest v3 where mocks could use an arrow function. Apply this guideline to test files that mock constructor-like calls (e.g., in tests under tests-ui, such as ComboWidget.test.ts) and ensure mock implementations are defined with function() { ... } or class { ... } to preserve constructor behavior.
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
📚 Learning: 2026-01-08T02:40:15.482Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7894
File: src/renderer/extensions/vueNodes/widgets/components/WidgetToggleSwitch.test.ts:11-14
Timestamp: 2026-01-08T02:40:15.482Z
Learning: In TypeScript test files (e.g., any test under src), avoid duplicating interface/type definitions. Import real type definitions from the component modules under test and reference them directly, so there is a single source of truth and to prevent type drift. This improves maintainability and consistency across tests.
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
📚 Learning: 2026-01-09T02:07:54.558Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7898
File: src/composables/usePaste.test.ts:248-248
Timestamp: 2026-01-09T02:07:54.558Z
Learning: In test files (e.g., any .test.ts or .test.tsx under src/...), when you create mock objects that partially implement an interface (such as LGraphNode), prefer casting with as Partial<InterfaceType> as InterfaceType rather than as any or as unknown as InterfaceType. This makes the incomplete implementation explicit while preserving type safety, improving readability and maintainability of tests.
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
📚 Learning: 2025-12-11T12:25:15.470Z
Learnt from: christian-byrne
Repo: Comfy-Org/ComfyUI_frontend PR: 7358
File: src/components/dialog/content/signin/SignUpForm.vue:45-54
Timestamp: 2025-12-11T12:25:15.470Z
Learning: This repository uses CI automation to format code (pnpm format). Do not include manual formatting suggestions in code reviews for Comfy-Org/ComfyUI_frontend. If formatting issues are detected, rely on the CI formatter or re-run pnpm format. Focus reviews on correctness, readability, performance, accessibility, and maintainability rather than style formatting.
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
📚 Learning: 2026-01-12T17:39:27.738Z
Learnt from: DrJKL
Repo: Comfy-Org/ComfyUI_frontend PR: 7906
File: src/components/sidebar/tabs/AssetsSidebarTab.vue:545-552
Timestamp: 2026-01-12T17:39:27.738Z
Learning: In Vue/TypeScript files (src/**/*.{ts,tsx,vue}), prefer if/else statements over ternary operators when performing side effects or actions (e.g., mutating state, calling methods with side effects). Ternaries should be reserved for computing and returning values.
Applied to files:
src/platform/workflow/persistence/services/workflowSessionStorageService.test.tssrc/platform/workflow/persistence/composables/useWorkflowPersistence.tssrc/platform/workflow/persistence/services/workflowSessionStorageService.ts
🧬 Code graph analysis (2)
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts (1)
src/platform/workflow/persistence/services/workflowSessionStorageService.ts (3)
setWithEviction(89-125)getWithAccessTracking(131-154)removeFromStorage(156-158)
src/platform/workflow/persistence/composables/useWorkflowPersistence.ts (4)
src/scripts/api.ts (1)
api(1304-1304)src/platform/workflow/management/stores/workflowStore.ts (1)
key(55-57)src/platform/workflow/persistence/services/workflowSessionStorageService.ts (2)
setWithEviction(89-125)getWithAccessTracking(131-154)src/platform/workflow/validation/schemas/workflowSchema.ts (1)
ComfyWorkflowJSON(459-461)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: setup
- GitHub Check: test
- GitHub Check: collect
- GitHub Check: lint-and-format
🔇 Additional comments (14)
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts (3)
84-133: LGTM!Comprehensive test coverage for
getWithAccessTrackingincluding both wrapped and legacy format handling, access time updates, and error scenarios.
135-143: LGTM!Simple and appropriate test for the
removeFromStoragefunction.
145-167: LGTM!Good tests verifying eviction order prioritizes legacy entries and respects protected keys. The mock pattern correctly simulates quota exceeded on first attempt, allowing the eviction logic to be exercised.
src/platform/workflow/persistence/composables/useWorkflowPersistence.ts (3)
18-25: LGTM!Imports are correctly separated with type imports on their own line. The
WORKFLOW_KEY_PATTERNconstant appropriately matches the pattern used for eviction in the service.
52-70: LGTM!The dual storage approach (localStorage as fallback, session storage with eviction as primary) provides good resilience. The ignored return value from
setWithEvictionaligns with the graceful degradation design documented in the PR objectives.
72-79: LGTM!Clean refactor with proper TypeScript typing. The null check with early return allows the caller to handle fallback logic cleanly.
src/platform/workflow/persistence/services/workflowSessionStorageService.ts (8)
18-26: LGTM!Legacy format detection is correctly implemented with appropriate deprecation notice. The logic properly identifies entries that lack the
{ accessedAt, data }wrapper structure.
28-30: LGTM!Clean wrapper function that adds the access timestamp for LRU tracking.
32-41: LGTM!Correctly handles both wrapped and legacy formats. Assigning
accessedAt: 0to legacy entries ensures they're prioritized for eviction.
43-68: LGTM!Well-implemented eviction candidate selection. The approach of treating parse errors as legacy (oldest) is sensible for cleanup. Sorting by
accessedAtascending ensures proper LRU ordering.
70-83: LGTM!Clean eviction logic that removes the oldest entries first and returns the count for retry logic.
89-125: LGTM!Solid retry-with-eviction implementation. The progressive eviction strategy (1, 2, 3 entries per retry) balances between recovering from quota issues and not over-evicting. Error handling properly distinguishes quota errors from unexpected failures.
131-154: LGTM!Well-designed read API with proper access tracking. The nested try-catch correctly handles quota errors during timestamp updates without failing the read operation.
156-158: LGTM!Simple and appropriate wrapper providing a consistent API surface for the storage service.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // 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' | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Find and read the target file
find . -name "useWorkflowPersistence.ts" -type f | head -5Repository: 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 fRepository: 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 3Repository: 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 -100Repository: 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 -20Repository: 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 5Repository: 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 10Repository: 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 -60Repository: 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 3Repository: 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.
src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts
Outdated
Show resolved
Hide resolved
src/platform/workflow/persistence/services/workflowSessionStorageService.ts
Outdated
Show resolved
Hide resolved
vi.restoreAllMocks() in afterEach already handles cleanup
Prevents false positives from substring matches
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/platform/workflow/persistence/services/workflowSessionStorageService.test.ts`:
- Around line 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.
| 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() | ||
| }) |
There was a problem hiding this comment.
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.
| 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.
mockRestore() calls are needed for these specific mocks
Main introduced workflowDraftStore with built-in QuotaExceededError handling, making the session storage LRU service redundant. Amp-Thread-ID: https://ampcode.com/threads/T-019bc597-fbea-778a-ae15-92061b3d0812 Co-authored-by: Amp <amp@ampcode.com>
dosubot
bot
added
size:XS
…aExceededError Instead of storing full workflow JSON in sessionStorage (which can hit quota on mobile), store only a small pointer to the draft path. The actual workflow data lives in the draft store which has LRU eviction. Loading priority: 1. SessionStorage pointer -> draft store 2. Preferred path -> draft store 3. Most recent draft (fallback) 4. Legacy sessionStorage payload (backward compat, remove after 2026-07-15) 5. Legacy localStorage payload (backward compat, remove after 2026-07-15) This maintains duplicate-tab support while eliminating the quota issue. Amp-Thread-ID: https://ampcode.com/threads/T-019bc597-fbea-778a-ae15-92061b3d0812 Co-authored-by: Amp <amp@ampcode.com>
dosubot
bot
added
size:M
|
Replaced by #8520 |
1 participant
Summary
Implement LRU-based session storage service for workflow data to prevent
QuotaExceededErrorthat was breaking workspace/teams features, particularly on mobile devices.Problem
QuotaExceededError: Failed to execute 'setItem' on 'Storage'when storing workflow dataSolution
Add an LRU eviction system that:
{ accessedAt, data }metadata for trackingQuotaExceededError: scans for evictable entries, sorts by age, evicts oldestBackward Compatibility
Legacy entries (raw workflow JSON without wrapper) are detected and treated as
accessedAt: 0, prioritizing them for eviction. This provides natural migration.Follow-up
A tracking issue should be created to remove legacy format support after 2026-07-15 (6 months), once all active users have naturally migrated. Code is marked with
@deprecated Remove after 2026-07-15comments.┆Issue is synchronized with this Notion page by Unito