test: CodeRabbit DrJKL review canary#13515
Closed
huang47 wants to merge 21 commits into
Closed
Conversation
Route changed .test.ts files through CodeRabbit guidance that explicitly names .agents/checks/test-quality.md, docs/testing/README.md, and docs/guidance/vitest.md as required review context. Verification: parsed .coderabbit.yaml with Ruby YAML, ran git diff --check, and ran pnpm typecheck in this worktree.
Move litegraph-only test guidance out of the blanket **/*.test.ts rule and into src/lib/litegraph/**/*.test.ts so unrelated Vitest files are not nudged toward litegraph factories or litegraph class mocking rules. Verification: parsed .coderabbit.yaml with Ruby YAML, ran git diff --check, and ran pnpm typecheck in this worktree.
Route changed browser_tests/**/*.spec.ts and apps/website/e2e/**/*.spec.ts files through CodeRabbit guidance that explicitly names .agents/checks/test-quality.md and docs/testing/README.md as required review context. Verification: parsed .coderabbit.yaml with Ruby YAML, ran git diff --check, and ran pnpm typecheck in this worktree.
Teach the shared Vitest path instruction to require rejected-path cleanup assertions and direct runtime-entrypoint coverage. This targets the missed DrJKL pattern where success cleanup existed but rejected queuePrompt cleanup was unverified. Verification: parsed .coderabbit.yaml with Ruby YAML.load_file successfully; ran git diff --check successfully; inspected staged diff to confirm only the shared Vitest path instruction changed.
Add a targeted app test path instruction for queuePrompt reviews so CodeRabbit asks for rejected-path authToken/apiKey cleanup coverage, matching the DrJKL review pattern on app.core.test.ts. Verification: parsed .coderabbit.yaml with Ruby YAML.load_file successfully; ran git diff --check successfully; inspected diff to confirm only src/scripts/app*.test.ts path guidance changed.
Add targeted LGraph production-path guidance so CodeRabbit asks for direct configure() regression coverage when deserialization or nullable link handling changes. This targets the DrJKL review pattern where helper-only null-link coverage did not prove graph deserialization behavior. Verification: parsed .coderabbit.yaml with Ruby YAML.load_file successfully; ran git diff --check successfully; inspected diff to confirm only src/lib/litegraph/src/LGraph.ts path guidance changed.
Strengthen app-specific CodeRabbit guidance after the canary still missed DrJKL’s rejected queuePrompt credential cleanup finding. Add production-path guidance for src/scripts/app.ts and make rejection-without-cleanup assertions explicitly reviewable. Verification: parsed .coderabbit.yaml with Ruby YAML.load_file successfully; ran git diff --check successfully; inspected diff to confirm only app queue cleanup guidance changed.
Add a deterministic CodeRabbit pre-merge check for the DrJKL app queuePrompt finding: when app.ts temporarily assigns api.authToken/api.apiKey around api.queuePrompt, require rejected-path tests that prove both credentials are cleared. Path instructions alone did not reliably surface this on the canary. Verification: parsed .coderabbit.yaml with Ruby YAML.load_file and confirmed the custom check list includes App queue credential cleanup on rejection; ran git diff --check successfully.
Covers the reusable DrJKL feedback patterns from PR #13362: double assertions, as-never casts, hand-rolled fixtures, inline required_input_missing errors, plain createPinia in store tests, unrestored mocks/globals, and dropped async helpers. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Covers DrJKL feedback on PR #13362 for repeated addDropHandler setup leaking document/canvas listeners and vi.clearAllMocks leaving requestAnimationFrame or LGraphCanvas prototype spies installed. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Covers DrJKL feedback on PR #13362 requiring a direct PartnerNodesList component test when traversal changes from app.graph to app.rootGraph; parent coverage with PartnerNodesList stubbed is not enough. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Covers DrJKL feedback on PR #13362 for navigator.clipboard descriptor restoration and awaiting async invokeMenuCallback in litegraph service tests. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Covers DrJKL feedback on PR #13362 where module-scope fetch and URL mocks cause vi.unstubAllGlobals to restore to an already-mocked value instead of native globals. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Covers DrJKL feedback on PR #13362 requiring compressWidgetInputSlots coverage for malformed legacy links shaped like links: [null, validLink]. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Covers DrJKL feedback on PR #13362 for repeated inline required_input_missing fixtures and store tests using createPinia instead of createTestingPinia({ stubActions: false }). Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Covers DrJKL feedback on PR #13362 for console.warn spies in api.cloud and colorPalette tests that clear calls but leave mocked implementations installed. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughThis PR hardens link deserialization to tolerate null entries in ChangesNull-safe link handling
app.graph to app.rootGraph migration and app.ts typing
New and expanded test suites
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Author
|
@coderabbitai review Created by Codex |
🎭 Playwright: ⏳ Running...🎨 Storybook: ✅ Built — View Storybook📦 Bundle Size
⚡ Performance Report
Absolute values
Raw data{
"timestamp": "2026-07-08T16:58:43.407Z",
"gitSha": "0a4c793ea9fde6b682af0b09d1af34ee036479cb",
"branch": "codex/coderabbit-drjkl-canary-2",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2044.4770000000005,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.842000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 449.55699999999996,
"heapDeltaBytes": -5355472,
"heapUsedBytes": 46587968,
"domNodes": -291,
"jsHeapTotalBytes": 16904192,
"scriptDurationMs": 16.440000000000005,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "canvas-idle",
"durationMs": 2055.4989999999975,
"styleRecalcs": 11,
"styleRecalcDurationMs": 10.048,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 422.75299999999993,
"heapDeltaBytes": -7267460,
"heapUsedBytes": 62085268,
"domNodes": -277,
"jsHeapTotalBytes": 18505728,
"scriptDurationMs": 16.747000000000003,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1930.848999999995,
"styleRecalcs": 80,
"styleRecalcDurationMs": 43.953,
"layouts": 12,
"layoutDurationMs": 3.9360000000000004,
"taskDurationMs": 872.096,
"heapDeltaBytes": -12926772,
"heapUsedBytes": 56377444,
"domNodes": -241,
"jsHeapTotalBytes": 19292160,
"scriptDurationMs": 123.506,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1801.7069999999649,
"styleRecalcs": 71,
"styleRecalcDurationMs": 43.342000000000006,
"layouts": 12,
"layoutDurationMs": 3.707,
"taskDurationMs": 803.994,
"heapDeltaBytes": 5575420,
"heapUsedBytes": 57794328,
"domNodes": -212,
"jsHeapTotalBytes": 17690624,
"scriptDurationMs": 117.902,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1740.0349999999776,
"styleRecalcs": 31,
"styleRecalcDurationMs": 16.214,
"layouts": 6,
"layoutDurationMs": 0.631,
"taskDurationMs": 349.97299999999996,
"heapDeltaBytes": -3789076,
"heapUsedBytes": 65780480,
"domNodes": -224,
"jsHeapTotalBytes": 13262848,
"scriptDurationMs": 18.622,
"eventListeners": -184,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1738.3330000000115,
"styleRecalcs": 32,
"styleRecalcDurationMs": 18.061999999999998,
"layouts": 6,
"layoutDurationMs": 0.7509999999999999,
"taskDurationMs": 350.643,
"heapDeltaBytes": 5852024,
"heapUsedBytes": 57790128,
"domNodes": -239,
"jsHeapTotalBytes": 2224128,
"scriptDurationMs": 19.910999999999998,
"eventListeners": -185,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 602.4449999999888,
"styleRecalcs": 10,
"styleRecalcDurationMs": 6.414999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 365.622,
"heapDeltaBytes": 4337444,
"heapUsedBytes": 56322212,
"domNodes": -292,
"jsHeapTotalBytes": 11923456,
"scriptDurationMs": 58.346999999999994,
"eventListeners": -201,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "dom-widget-clipping",
"durationMs": 593.3290000000397,
"styleRecalcs": 12,
"styleRecalcDurationMs": 8.372000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 360.887,
"heapDeltaBytes": 14570436,
"heapUsedBytes": 67177488,
"domNodes": -291,
"jsHeapTotalBytes": 2486272,
"scriptDurationMs": 55.204,
"eventListeners": -203,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "large-graph-idle",
"durationMs": 2041.5540000000192,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.7909999999999995,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 520.297,
"heapDeltaBytes": -8801668,
"heapUsedBytes": 58428556,
"domNodes": -269,
"jsHeapTotalBytes": -1839104,
"scriptDurationMs": 77.843,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2044.6779999999762,
"styleRecalcs": 8,
"styleRecalcDurationMs": 6.696000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 517.97,
"heapDeltaBytes": -1068768,
"heapUsedBytes": 58648932,
"domNodes": -276,
"jsHeapTotalBytes": -1609728,
"scriptDurationMs": 79.36999999999999,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 2070.633999999984,
"styleRecalcs": 68,
"styleRecalcDurationMs": 18.028,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 948.76,
"heapDeltaBytes": 28904824,
"heapUsedBytes": 81435764,
"domNodes": -273,
"jsHeapTotalBytes": 204800,
"scriptDurationMs": 341.96999999999997,
"eventListeners": -197,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-pan",
"durationMs": 2078.3690000000092,
"styleRecalcs": 68,
"styleRecalcDurationMs": 18.068,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 978.9219999999999,
"heapDeltaBytes": 28903316,
"heapUsedBytes": 81853848,
"domNodes": -277,
"jsHeapTotalBytes": 1515520,
"scriptDurationMs": 360.159,
"eventListeners": -197,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3049.226000000033,
"styleRecalcs": 65,
"styleRecalcDurationMs": 20.012999999999998,
"layouts": 60,
"layoutDurationMs": 8.258999999999999,
"taskDurationMs": 1267.719,
"heapDeltaBytes": -5474332,
"heapUsedBytes": 69049284,
"domNodes": 14,
"jsHeapTotalBytes": 9056256,
"scriptDurationMs": 473.597,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-zoom",
"durationMs": 3059.821999999997,
"styleRecalcs": 63,
"styleRecalcDurationMs": 18.773999999999997,
"layouts": 60,
"layoutDurationMs": 8.347,
"taskDurationMs": 1247.417,
"heapDeltaBytes": 14016732,
"heapUsedBytes": 68030804,
"domNodes": 6,
"jsHeapTotalBytes": 0,
"scriptDurationMs": 469.03700000000003,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "minimap-idle",
"durationMs": 2019.0079999999853,
"styleRecalcs": 8,
"styleRecalcDurationMs": 5.778999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 483.97399999999993,
"heapDeltaBytes": -9136776,
"heapUsedBytes": 60218292,
"domNodes": -264,
"jsHeapTotalBytes": -1314816,
"scriptDurationMs": 73.625,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "minimap-idle",
"durationMs": 2025.7709999999634,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.146,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 481.173,
"heapDeltaBytes": 10566992,
"heapUsedBytes": 71603004,
"domNodes": 16,
"jsHeapTotalBytes": 5505024,
"scriptDurationMs": 77.282,
"eventListeners": 6,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 624.7650000000249,
"styleRecalcs": 47,
"styleRecalcDurationMs": 16.198000000000004,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 408.7970000000001,
"heapDeltaBytes": -23630472,
"heapUsedBytes": 45917836,
"domNodes": -281,
"jsHeapTotalBytes": 5660672,
"scriptDurationMs": 129.417,
"eventListeners": -193,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66666666666665,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 609.2170000000579,
"styleRecalcs": 47,
"styleRecalcDurationMs": 11.908,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 402.10599999999994,
"heapDeltaBytes": -21974312,
"heapUsedBytes": 47507540,
"domNodes": -275,
"jsHeapTotalBytes": 5922816,
"scriptDurationMs": 122.841,
"eventListeners": -197,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666682,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2018.303000000003,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.531999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 378.39500000000004,
"heapDeltaBytes": 20062272,
"heapUsedBytes": 71927472,
"domNodes": -291,
"jsHeapTotalBytes": 15331328,
"scriptDurationMs": 12.831,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-idle",
"durationMs": 1991.9539999999643,
"styleRecalcs": 9,
"styleRecalcDurationMs": 7.63,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 334.93300000000005,
"heapDeltaBytes": -1872400,
"heapUsedBytes": 56997200,
"domNodes": 18,
"jsHeapTotalBytes": 25165824,
"scriptDurationMs": 12.624999999999998,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1749.3100000000084,
"styleRecalcs": 76,
"styleRecalcDurationMs": 37.150000000000006,
"layouts": 16,
"layoutDurationMs": 4.317,
"taskDurationMs": 685.411,
"heapDeltaBytes": 16241360,
"heapUsedBytes": 68535964,
"domNodes": -273,
"jsHeapTotalBytes": 14020608,
"scriptDurationMs": 87.48,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1734.2200000000503,
"styleRecalcs": 77,
"styleRecalcDurationMs": 39.68,
"layouts": 16,
"layoutDurationMs": 4.05,
"taskDurationMs": 685.783,
"heapDeltaBytes": -16679076,
"heapUsedBytes": 52895988,
"domNodes": -233,
"jsHeapTotalBytes": 20340736,
"scriptDurationMs": 86.622,
"eventListeners": -199,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-transition-enter",
"durationMs": 949.3569999999636,
"styleRecalcs": 18,
"styleRecalcDurationMs": 31.889,
"layouts": 14,
"layoutDurationMs": 14.024000000000001,
"taskDurationMs": 747.5159999999998,
"heapDeltaBytes": 29894840,
"heapUsedBytes": 102151752,
"domNodes": 13673,
"jsHeapTotalBytes": 16252928,
"scriptDurationMs": 26.810000000000002,
"eventListeners": 2533,
"totalBlockingTimeMs": 148,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "viewport-pan-sweep",
"durationMs": 8140.559999999994,
"styleRecalcs": 251,
"styleRecalcDurationMs": 60.882000000000005,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 3455.082,
"heapDeltaBytes": -543720,
"heapUsedBytes": 71019948,
"domNodes": 20,
"jsHeapTotalBytes": 16572416,
"scriptDurationMs": 1141.506,
"eventListeners": 20,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8163.791000000003,
"styleRecalcs": 251,
"styleRecalcDurationMs": 62.705999999999996,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 3902.4459999999995,
"heapDeltaBytes": 4318236,
"heapUsedBytes": 76210808,
"domNodes": 20,
"jsHeapTotalBytes": 21028864,
"scriptDurationMs": 1359.697,
"eventListeners": 20,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 13570.221000000003,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 13554.476999999999,
"heapDeltaBytes": -34323272,
"heapUsedBytes": 172116188,
"domNodes": -3311,
"jsHeapTotalBytes": 23040000,
"scriptDurationMs": 530.808,
"eventListeners": -16374,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.223333333333358,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 12846.528999999919,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 12830.223999999998,
"heapDeltaBytes": -41675132,
"heapUsedBytes": 163309772,
"domNodes": -3300,
"jsHeapTotalBytes": -1339392,
"scriptDurationMs": 516.6299999999999,
"eventListeners": -16380,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.220000000000073,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 15975.66999999998,
"styleRecalcs": 80,
"styleRecalcDurationMs": 21.016000000000034,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 15952.74,
"heapDeltaBytes": -52155932,
"heapUsedBytes": 165396048,
"domNodes": -3300,
"jsHeapTotalBytes": 12292096,
"scriptDurationMs": 806.624,
"eventListeners": -16374,
"totalBlockingTimeMs": 35,
"frameDurationMs": 17.223333333333358,
"p95FrameDurationMs": 16.80000000000291
},
{
"name": "vue-large-graph-pan",
"durationMs": 16206.142999999996,
"styleRecalcs": 89,
"styleRecalcDurationMs": 20.379000000000037,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 16183.56,
"heapDeltaBytes": -60442392,
"heapUsedBytes": 157205392,
"domNodes": -3300,
"jsHeapTotalBytes": 5214208,
"scriptDurationMs": 841.0819999999999,
"eventListeners": -16372,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.77333333333336,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "workflow-execution",
"durationMs": 436.5970000000061,
"styleRecalcs": 16,
"styleRecalcDurationMs": 23.804000000000002,
"layouts": 5,
"layoutDurationMs": 1.5440000000000003,
"taskDurationMs": 162.655,
"heapDeltaBytes": -6647120,
"heapUsedBytes": 46474520,
"domNodes": -279,
"jsHeapTotalBytes": 1437696,
"scriptDurationMs": 16.796000000000003,
"eventListeners": -177,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "workflow-execution",
"durationMs": 494.14500000000317,
"styleRecalcs": 18,
"styleRecalcDurationMs": 34.182,
"layouts": 5,
"layoutDurationMs": 1.441,
"taskDurationMs": 217.781,
"heapDeltaBytes": -22091444,
"heapUsedBytes": 48701804,
"domNodes": -181,
"jsHeapTotalBytes": 4612096,
"scriptDurationMs": 18.779000000000003,
"eventListeners": -132,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
}
]
} |
Contributor
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/litegraph/src/LGraph.ts (1)
2494-2501: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing required regression test for sparse
linksinconfigure().The null guard fix here is correct, but no direct test constructing an
LGraphand callingconfigure()with alinks: [null, validLink]v0.4 payload is included in this PR. Please add a colocated test inLGraph.test.tsassertingconfigure()does not throw and the valid link is still created.As per path instructions, "When changes touch
LGraph.configure(), graph deserialization, link loading, or nullable serialized links, require a direct regression test that constructs anLGraphand callsconfigure()with the changed serialized shape... For sparse legacy links, expect a v0.4 workflow withlinks: [null, validLink]to not throw and still create the valid link."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/litegraph/src/LGraph.ts` around lines 2494 - 2501, The null-guard change in LGraph.configure() needs a direct regression test for sparse legacy links. Add a colocated case in LGraph.test.ts that constructs an LGraph, calls configure() with a v0.4 payload containing links: [null, validLink], and asserts configure() does not throw while the valid LLink is still created in the graph.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/renderer/extensions/linearMode/PartnerNodesList.vue`:
- Line 24: Add a focused PartnerNodesList component test for the graph traversal
change: the `creditsBadges` logic in `PartnerNodesList.vue` now uses
`mapAllNodes(app.rootGraph, ...)`, so create a test that places different badge
nodes in `app.graph` versus `app.rootGraph` and asserts the rendered badges come
from `rootGraph`. Keep the test specific to `PartnerNodesList` itself, since
`LinearControls.test.ts` stubs this component and won’t cover the traversal
behavior.
In `@src/scripts/api.cloud.test.ts`:
- Around line 50-66: The test setup in beforeEach is only clearing mocks, so any
console.warn spy created by earlier tests can stay installed and affect later
cases in api.cloud.test.ts. Update the suite around the existing beforeEach and
the console.warn spy usage so spies are restored after each test, using
mockRestore(), vi.restoreAllMocks(), or a try/finally pattern. Keep the fix
localized to the test helpers and the tests that spy on console.warn so the
WebSocket and auth-header cases run with a clean console state.
In `@src/scripts/app.ts`:
- Line 1170: The graphData parameter in the workflow loading logic is typed to
accept unknown[] even though the subsequent validation path in the same flow
immediately rejects arrays and falls back to defaultGraph. Update the signature
on the relevant function in app.ts back to ComfyWorkflowJSON | undefined unless
array inputs are truly intended, and if arrays must be supported, handle them
explicitly in the graphData validation branch instead of silently defaulting.
Use the graphData checks near the defaultGraph fallback to keep the runtime
behavior aligned with the declared type.
In `@src/scripts/changeTracker.test.ts`:
- Around line 908-960: `ChangeTracker.graphEqual` is being tested with `null`
via an unsafe `as never` cast; replace that bypass with a safer typed null
assertion in the test, or better, widen `graphEqual`’s input type to accept
`ComfyWorkflowJSON | null` so the null case is covered without any cast. Keep
the fix localized to the `graphEqual` test block and the
`ChangeTracker.graphEqual` signature if you choose the production-side change.
In `@src/scripts/pnginfo.test.ts`:
- Around line 357-378: The importA1111 test block is leaving spies and mocked
console implementations active across tests, which can suppress output in later
cases. Add cleanup in the relevant test scope so spies on console.error,
console.warn, LiteGraph.createNode, and api.getEmbeddings are restored after
each test, using vi.restoreAllMocks() or individual mockRestore() calls. Make
sure the cleanup is applied around the importA1111 describe block so tests like
“stops when a required base node cannot be created” and “skips missing
LoraLoader nodes” do not leak into later tests.
In `@src/services/colorPaletteService.test.ts`:
- Line 169: The console.warn spies in colorPaletteService.test.ts are not being
restored, so the mocked implementation can leak into later tests even though
beforeEach only clears call history. Update the test setup around the existing
console.warn spies to restore them after each test, using afterEach with
mockRestore or vi.restoreAllMocks so the original console.warn behavior is
reinstated. Make sure this applies to both spy locations in the test file and
keeps vi.clearAllMocks from being the only cleanup.
- Around line 294-304: The light-theme test in colorPaletteService.test.ts
leaves global localStorage keys behind, which can leak state into later tests.
Update the test setup around useColorPaletteService() and loadColorPalette() to
clear localStorage before each test or add an afterEach cleanup, so
comfy-splash-bg and comfy-splash-fg are reset after the assertion block.
In `@src/services/litegraphService.core.test.ts`:
- Around line 460-462: The test teardown in litegraphService.core.test.ts only
unstubs globals, which is not enough when console spies are used. Update the
afterEach cleanup alongside vi.unstubAllGlobals() to also call
vi.restoreAllMocks(), so any console.warn/console.error spies created in the
suite are fully restored even if a test exits early. Keep the fix near the
existing afterEach and the console spy tests in litegraphService.core.test.ts.
- Around line 503-507: The test is dropping the promise returned by
invokeMenuCallback by using void, which can let assertions run before async menu
handling finishes. Update the test around invokeMenuCallback and
mockFavoritedWidgetsStore.toggleFavorite to await the callback invocation, or
assert on the returned promise with resolves/rejects if appropriate. Keep the
existing expectation for toggleFavorite but ensure the async helper completes
before asserting.
- Around line 367-378: The test helper installClipboard currently replaces
navigator.clipboard via Object.defineProperty without restoring the original
descriptor, so the mock leaks across Vitest worker tests. Update
installClipboard in the test file to save the existing navigator.clipboard
property descriptor before stubbing, and add teardown in the same test setup to
restore that descriptor after each test instead of relying only on
vi.unstubAllGlobals(). Keep the fix localized around installClipboard and the
afterEach cleanup so the clipboard mock is fully reverted.
In `@src/services/mediaCacheService.test.ts`:
- Around line 56-62: The response() test helper is constructing a fake Response
object and casting it with as Response, which should be replaced with a real
Response instance. Update the helper in mediaCacheService.test.ts to use the
Response constructor directly so it returns an actual platform-owned Response
with the provided blob and the correct status based on the ok argument; this
should preserve the existing blob() behavior and keep the tests aligned with the
Response type used by mediaCacheService.
- Line 115: The `console.warn` spy in `mediaCacheService` tests is leaking
across cases because it is never restored, so add spy cleanup in the test
lifecycle and restore all `vi.spyOn` mocks after each test. Update the existing
`afterEach` in `mediaCacheService.test.ts` to call `vi.restoreAllMocks()`
alongside the current global unstubbing so the `console.warn` spy is reset while
preserving the `vi.fn()` mocks used in this suite.
- Around line 9-13: Remove the module-scope mutations of `global.fetch` and
`global.URL` in `mediaCacheService.test.ts`; they are redundant because
`beforeEach` already installs these globals with `vi.stubGlobal`. Keep the
global setup inside the test lifecycle (using `vi.stubGlobal` and
`vi.unstubAllGlobals`), so teardown restores the native globals instead of the
module-level mocks. After removing those top-level assignments, also clean up
any now-unused import such as `fromPartial`.
In `@src/stores/subgraphNavigationStore.viewport.test.ts`:
- Around line 140-156: The repeated temporary `app.canvas = undefined` setup in
`subgraphNavigationStore.viewport.test.ts` should be extracted into a small
helper to remove duplication and keep restoration consistent. Add a reusable
helper like `withoutCanvas(fn)` near the `useSubgraphNavigationStore` tests, and
update the three canvas-unavailable cases to run their assertions through it
instead of repeating the type assertion and try/finally block.
In `@src/utils/uuid.test.ts`:
- Around line 11-15: Remove the tautological assertion from the uuid test and
replace it with a real behavioral check for createUuidv4 or delete the test
entirely if no observable fallback behavior is exposed. Use the existing
uuid.test.ts suite and the createUuidv4 symbol as the place to verify a
meaningful output rather than comparing a string literal to itself.
- Around line 17-21: The test in uuid.test.ts is tautological and uses an
запрещенный-looking `as never` cast, so remove this entire case and any similar
no-op assertions. Keep the test suite focused on exercising `createUuidv4`
behavior with meaningful inputs and assertions rather than checking that
`undefined` is `undefined`.
In `@src/utils/vintageClipboard.test.ts`:
- Around line 43-49: The empty-data early-return test in deserialiseAndCreate is
too narrow because it only verifies emitBeforeChange is skipped. Update the
vintageClipboard.test case to also assert that canvas.graph!.add and
canvas.selectNodes are not called, so the test fully covers that
deserialiseAndCreate short-circuits all side effects when clipboard data is
empty.
---
Outside diff comments:
In `@src/lib/litegraph/src/LGraph.ts`:
- Around line 2494-2501: The null-guard change in LGraph.configure() needs a
direct regression test for sparse legacy links. Add a colocated case in
LGraph.test.ts that constructs an LGraph, calls configure() with a v0.4 payload
containing links: [null, validLink], and asserts configure() does not throw
while the valid LLink is still created in the graph.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 04b5a8f0-87e1-4aa6-93eb-cce6db1f288e
📒 Files selected for processing (21)
src/extensions/core/load3d.tssrc/lib/litegraph/src/LGraph.tssrc/lib/litegraph/src/types/serialisation.tssrc/renderer/extensions/linearMode/PartnerNodesList.vuesrc/scripts/api.cloud.test.tssrc/scripts/app.core.test.tssrc/scripts/app.tssrc/scripts/changeTracker.test.tssrc/scripts/pnginfo.test.tssrc/services/colorPaletteService.test.tssrc/services/litegraphService.core.test.tssrc/services/mediaCacheService.test.tssrc/services/subgraphService.test.tssrc/stores/executionErrorStore.test.tssrc/stores/subgraphNavigationStore.test.tssrc/stores/subgraphNavigationStore.viewport.test.tssrc/utils/__tests__/executionErrorTestUtils.tssrc/utils/linkFixer.tssrc/utils/litegraphUtil.tssrc/utils/uuid.test.tssrc/utils/vintageClipboard.test.ts
💤 Files with no reviewable changes (1)
- src/extensions/core/load3d.ts
|
|
||
| const creditsBadges = computed(() => | ||
| mapAllNodes(app.graph, (node) => { | ||
| mapAllNodes(app.rootGraph, (node) => { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Missing focused component test for the app.graph → app.rootGraph traversal change.
This switches which graph creditsBadges traverses (mapAllNodes(app.graph, ...) → mapAllNodes(app.rootGraph, ...)), but no PartnerNodesList-specific test accompanies this change in the reviewed files. Coverage via LinearControls.test.ts would be insufficient since PartnerNodesList is stubbed there.
Add a focused test that arranges different badge nodes in a subgraph vs. the root graph and asserts the rendered badge list is sourced from rootGraph, not the (possibly nested) active app.graph.
As per path instructions: "When behavior changes which graph PartnerNodesList traverses, require a focused component test for this component itself... Coverage through LinearControls.test.ts is insufficient if PartnerNodesList is stubbed there."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/extensions/linearMode/PartnerNodesList.vue` at line 24, Add a
focused PartnerNodesList component test for the graph traversal change: the
`creditsBadges` logic in `PartnerNodesList.vue` now uses
`mapAllNodes(app.rootGraph, ...)`, so create a test that places different badge
nodes in `app.graph` versus `app.rootGraph` and asserts the rendered badges come
from `rootGraph`. Keep the test specific to `PartnerNodesList` itself, since
`LinearControls.test.ts` stubs this component and won’t cover the traversal
behavior.
Source: Path instructions
Comment on lines
+50
to
+66
| beforeEach(() => { | ||
| vi.clearAllMocks() | ||
| vi.unstubAllGlobals() | ||
| FakeWebSocket.instances = [] | ||
| window.name = '' | ||
| sessionStorage.clear() | ||
| mockAuthStore.isInitialized = true | ||
| mockAuthStore.getAuthHeader.mockResolvedValue(null) | ||
| mockAuthStore.getAuthToken.mockResolvedValue(null) | ||
| vi.mocked(shouldRemintCloudRequest).mockResolvedValue(false) | ||
| vi.mocked(fetchWithUnifiedRemint).mockResolvedValue( | ||
| new Response(JSON.stringify({ ok: true }), { | ||
| headers: { 'Content-Type': 'application/json' } | ||
| }) | ||
| ) | ||
| vi.stubGlobal('WebSocket', FakeWebSocket) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
vi.clearAllMocks() does not restore console.warn spies from prior tests.
Tests at lines 94 and 133 call vi.spyOn(console, 'warn').mockImplementation(() => {}). vi.clearAllMocks() (line 51) only resets call records — it leaves the mocked implementation installed. This means test 3 ("adds the cloud auth token to websocket URLs", line 115) runs with console.warn still silently suppressed from test 2, potentially hiding real warnings.
Per the path instructions for src/scripts/api*.test.ts: "When a test spies on console.warn or another console method, require the spy to be restored with mockRestore(), vi.restoreAllMocks(), or try/finally."
🔧 Proposed fix: add afterEach restoration
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' describe('ComfyApi cloud mode', () => {
+ afterEach(() => {
+ vi.restoreAllMocks()
+ })
+
beforeEach(() => {
vi.clearAllMocks()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| beforeEach(() => { | |
| vi.clearAllMocks() | |
| vi.unstubAllGlobals() | |
| FakeWebSocket.instances = [] | |
| window.name = '' | |
| sessionStorage.clear() | |
| mockAuthStore.isInitialized = true | |
| mockAuthStore.getAuthHeader.mockResolvedValue(null) | |
| mockAuthStore.getAuthToken.mockResolvedValue(null) | |
| vi.mocked(shouldRemintCloudRequest).mockResolvedValue(false) | |
| vi.mocked(fetchWithUnifiedRemint).mockResolvedValue( | |
| new Response(JSON.stringify({ ok: true }), { | |
| headers: { 'Content-Type': 'application/json' } | |
| }) | |
| ) | |
| vi.stubGlobal('WebSocket', FakeWebSocket) | |
| }) | |
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | |
| describe('ComfyApi cloud mode', () => { | |
| afterEach(() => { | |
| vi.restoreAllMocks() | |
| }) | |
| beforeEach(() => { | |
| vi.clearAllMocks() | |
| vi.unstubAllGlobals() | |
| FakeWebSocket.instances = [] | |
| window.name = '' | |
| sessionStorage.clear() | |
| mockAuthStore.isInitialized = true | |
| mockAuthStore.getAuthHeader.mockResolvedValue(null) | |
| mockAuthStore.getAuthToken.mockResolvedValue(null) | |
| vi.mocked(shouldRemintCloudRequest).mockResolvedValue(false) | |
| vi.mocked(fetchWithUnifiedRemint).mockResolvedValue( | |
| new Response(JSON.stringify({ ok: true }), { | |
| headers: { 'Content-Type': 'application/json' } | |
| }) | |
| ) | |
| vi.stubGlobal('WebSocket', FakeWebSocket) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/scripts/api.cloud.test.ts` around lines 50 - 66, The test setup in
beforeEach is only clearing mocks, so any console.warn spy created by earlier
tests can stay installed and affect later cases in api.cloud.test.ts. Update the
suite around the existing beforeEach and the console.warn spy usage so spies are
restored after each test, using mockRestore(), vi.restoreAllMocks(), or a
try/finally pattern. Keep the fix localized to the test helpers and the tests
that spy on console.warn so the WebSocket and auth-header cases run with a clean
console state.
Source: Path instructions
|
|
||
| async loadGraphData( | ||
| graphData?: ComfyWorkflowJSON, | ||
| graphData?: ComfyWorkflowJSON | unknown[], |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Widened type accepts unknown[] that is immediately rejected at runtime.
graphData now types as ComfyWorkflowJSON | unknown[], but the validation a few lines later (!graphData || typeof graphData !== 'object' || Array.isArray(graphData)) discards any array input and falls back to defaultGraph. The unknown[] branch of the type has no meaningful runtime handling, so it advertises support for an input shape that is always silently discarded.
Consider narrowing the signature back to ComfyWorkflowJSON | undefined, or if arrays are truly expected from some caller (e.g. malformed file parses), handle them explicitly (e.g. warn/log) instead of silently defaulting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/scripts/app.ts` at line 1170, The graphData parameter in the workflow
loading logic is typed to accept unknown[] even though the subsequent validation
path in the same flow immediately rejects arrays and falls back to defaultGraph.
Update the signature on the relevant function in app.ts back to
ComfyWorkflowJSON | undefined unless array inputs are truly intended, and if
arrays must be supported, handle them explicitly in the graphData validation
branch instead of silently defaulting. Use the graphData checks near the
defaultGraph fallback to keep the runtime behavior aligned with the declared
type.
Comment on lines
+908
to
+960
| describe('graphEqual', () => { | ||
| it('compares workflow nodes as an unordered set and ignores extra.ds', () => { | ||
| const first = createState(2) | ||
| const second = fromPartial<ComfyWorkflowJSON>({ | ||
| ...createState(), | ||
| nodes: [...first.nodes].reverse(), | ||
| links: first.links, | ||
| groups: first.groups, | ||
| extra: { ds: { scale: 2 } } | ||
| } as PartialDeep<ComfyWorkflowJSON>) | ||
|
|
||
| expect(ChangeTracker.graphEqual(first, first)).toBe(true) | ||
| expect(ChangeTracker.graphEqual(first, second)).toBe(true) | ||
| }) | ||
|
|
||
| it('returns false for non-object values and meaningful graph differences', () => { | ||
| const first = createState(1) | ||
| const differentNodes = createState(2) | ||
| const differentLinks = fromPartial<ComfyWorkflowJSON>({ | ||
| ...first, | ||
| links: [[1, 1, 0, 2, 0, 'MODEL']] | ||
| } as PartialDeep<ComfyWorkflowJSON>) | ||
|
|
||
| expect(ChangeTracker.graphEqual(first, null as never)).toBe(false) | ||
| expect(ChangeTracker.graphEqual(first, differentNodes)).toBe(false) | ||
| expect(ChangeTracker.graphEqual(first, differentLinks)).toBe(false) | ||
| }) | ||
|
|
||
| it('returns false for extra properties other than viewport state', () => { | ||
| const first = createState() | ||
| const second = fromPartial<ComfyWorkflowJSON>({ | ||
| ...first, | ||
| extra: { custom: true } | ||
| } as PartialDeep<ComfyWorkflowJSON>) | ||
|
|
||
| expect(ChangeTracker.graphEqual(first, second)).toBe(false) | ||
| }) | ||
|
|
||
| it.each([ | ||
| 'floatingLinks', | ||
| 'reroutes', | ||
| 'groups', | ||
| 'definitions', | ||
| 'subgraphs' | ||
| ] as const)('returns false when %s differs', (key) => { | ||
| const first = createState() | ||
| const second = fromPartial<ComfyWorkflowJSON>({ | ||
| ...first, | ||
| [key]: [{ id: 1 }] | ||
| } as PartialDeep<ComfyWorkflowJSON>) | ||
|
|
||
| expect(ChangeTracker.graphEqual(first, second)).toBe(false) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Replace null as never with a safer type bypass.
Line 931 uses null as never to test graphEqual with null input. Path instructions explicitly flag as never casts because they bypass type safety entirely (never is assignable to any type, hiding real type errors). Since graphEqual is expected to handle null gracefully (per the test's intent), the production signature should ideally accept ComfyWorkflowJSON | null. As a test-only workaround until the production type is widened, prefer a direct assertion to the parameter type rather than routing through never:
🔧 Suggested fix
- expect(ChangeTracker.graphEqual(first, null as never)).toBe(false)
+ expect(ChangeTracker.graphEqual(first, null as unknown as ComfyWorkflowJSON)).toBe(false)Or better, widen the production signature in a follow-up PR:
- static graphEqual(a: ComfyWorkflowJSON, b: ComfyWorkflowJSON): boolean
+ static graphEqual(a: ComfyWorkflowJSON, b: ComfyWorkflowJSON | null): booleanThen the test simplifies to:
- expect(ChangeTracker.graphEqual(first, null as never)).toBe(false)
+ expect(ChangeTracker.graphEqual(first, null)).toBe(false)As per path instructions: "Build partial mocks with fromPartial() from @total-typescript/shoehorn; flag as unknown as double assertions, as never, and fromAny()." Note that as unknown as ComfyWorkflowJSON is also flagged by path instructions, so the preferred resolution is widening the production type to accept null.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| describe('graphEqual', () => { | |
| it('compares workflow nodes as an unordered set and ignores extra.ds', () => { | |
| const first = createState(2) | |
| const second = fromPartial<ComfyWorkflowJSON>({ | |
| ...createState(), | |
| nodes: [...first.nodes].reverse(), | |
| links: first.links, | |
| groups: first.groups, | |
| extra: { ds: { scale: 2 } } | |
| } as PartialDeep<ComfyWorkflowJSON>) | |
| expect(ChangeTracker.graphEqual(first, first)).toBe(true) | |
| expect(ChangeTracker.graphEqual(first, second)).toBe(true) | |
| }) | |
| it('returns false for non-object values and meaningful graph differences', () => { | |
| const first = createState(1) | |
| const differentNodes = createState(2) | |
| const differentLinks = fromPartial<ComfyWorkflowJSON>({ | |
| ...first, | |
| links: [[1, 1, 0, 2, 0, 'MODEL']] | |
| } as PartialDeep<ComfyWorkflowJSON>) | |
| expect(ChangeTracker.graphEqual(first, null as never)).toBe(false) | |
| expect(ChangeTracker.graphEqual(first, differentNodes)).toBe(false) | |
| expect(ChangeTracker.graphEqual(first, differentLinks)).toBe(false) | |
| }) | |
| it('returns false for extra properties other than viewport state', () => { | |
| const first = createState() | |
| const second = fromPartial<ComfyWorkflowJSON>({ | |
| ...first, | |
| extra: { custom: true } | |
| } as PartialDeep<ComfyWorkflowJSON>) | |
| expect(ChangeTracker.graphEqual(first, second)).toBe(false) | |
| }) | |
| it.each([ | |
| 'floatingLinks', | |
| 'reroutes', | |
| 'groups', | |
| 'definitions', | |
| 'subgraphs' | |
| ] as const)('returns false when %s differs', (key) => { | |
| const first = createState() | |
| const second = fromPartial<ComfyWorkflowJSON>({ | |
| ...first, | |
| [key]: [{ id: 1 }] | |
| } as PartialDeep<ComfyWorkflowJSON>) | |
| expect(ChangeTracker.graphEqual(first, second)).toBe(false) | |
| }) | |
| describe('graphEqual', () => { | |
| it('compares workflow nodes as an unordered set and ignores extra.ds', () => { | |
| const first = createState(2) | |
| const second = fromPartial<ComfyWorkflowJSON>({ | |
| ...createState(), | |
| nodes: [...first.nodes].reverse(), | |
| links: first.links, | |
| groups: first.groups, | |
| extra: { ds: { scale: 2 } } | |
| } as PartialDeep<ComfyWorkflowJSON>) | |
| expect(ChangeTracker.graphEqual(first, first)).toBe(true) | |
| expect(ChangeTracker.graphEqual(first, second)).toBe(true) | |
| }) | |
| it('returns false for non-object values and meaningful graph differences', () => { | |
| const first = createState(1) | |
| const differentNodes = createState(2) | |
| const differentLinks = fromPartial<ComfyWorkflowJSON>({ | |
| ...first, | |
| links: [[1, 1, 0, 2, 0, 'MODEL']] | |
| } as PartialDeep<ComfyWorkflowJSON>) | |
| expect(ChangeTracker.graphEqual(first, null as unknown as ComfyWorkflowJSON)).toBe(false) | |
| expect(ChangeTracker.graphEqual(first, differentNodes)).toBe(false) | |
| expect(ChangeTracker.graphEqual(first, differentLinks)).toBe(false) | |
| }) | |
| it('returns false for extra properties other than viewport state', () => { | |
| const first = createState() | |
| const second = fromPartial<ComfyWorkflowJSON>({ | |
| ...first, | |
| extra: { custom: true } | |
| } as PartialDeep<ComfyWorkflowJSON>) | |
| expect(ChangeTracker.graphEqual(first, second)).toBe(false) | |
| }) | |
| it.each([ | |
| 'floatingLinks', | |
| 'reroutes', | |
| 'groups', | |
| 'definitions', | |
| 'subgraphs' | |
| ] as const)('returns false when %s differs', (key) => { | |
| const first = createState() | |
| const second = fromPartial<ComfyWorkflowJSON>({ | |
| ...first, | |
| [key]: [{ id: 1 }] | |
| } as PartialDeep<ComfyWorkflowJSON>) | |
| expect(ChangeTracker.graphEqual(first, second)).toBe(false) | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/scripts/changeTracker.test.ts` around lines 908 - 960,
`ChangeTracker.graphEqual` is being tested with `null` via an unsafe `as never`
cast; replace that bypass with a safer typed null assertion in the test, or
better, widen `graphEqual`’s input type to accept `ComfyWorkflowJSON | null` so
the null case is covered without any cast. Keep the fix localized to the
`graphEqual` test block and the `ChangeTracker.graphEqual` signature if you
choose the production-side change.
Source: Path instructions
Comment on lines
+357
to
+378
| it('stops when a required base node cannot be created', async () => { | ||
| const graph = createGraph() | ||
| vi.spyOn(console, 'error').mockImplementation(() => {}) | ||
| vi.spyOn(api, 'getEmbeddings').mockResolvedValue([]) | ||
| vi.spyOn(LiteGraph, 'createNode').mockImplementation((type) => | ||
| type === 'KSampler' ? null : createNode(type) | ||
| ) | ||
|
|
||
| await importA1111( | ||
| graph, | ||
| [ | ||
| 'prompt', | ||
| 'Negative prompt: blurry', | ||
| 'Steps: 20, Sampler: Euler a, Size: 512x512' | ||
| ].join('\n') | ||
| ) | ||
|
|
||
| expect(graph.clear).not.toHaveBeenCalled() | ||
| expect(console.error).toHaveBeenCalledWith( | ||
| 'Failed to create required nodes for A1111 import' | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore console and module spies after each test.
Multiple tests in the importA1111 block spy on console.error/console.warn with mockImplementation(() => {}) and on LiteGraph.createNode/api.getEmbeddings, but no afterEach restores them. The lingering mockImplementation(() => {}) suppresses console output in subsequent tests that don't re-spy — e.g., the console.error spy from "stops when a required base node cannot be created" (line 359) persists into "builds a basic graph" (line 380) and beyond, and the console.warn spy from "skips missing LoraLoader nodes" (line 462) persists into "returns from latent hires setup" (line 484). Per test quality guidelines, vi.clearAllMocks() is insufficient for console method spies; require vi.restoreAllMocks() or mockRestore().
🛡️ Proposed fix
describe('importA1111', () => {
+ afterEach(() => {
+ vi.restoreAllMocks()
+ })
+
function widget(As per path instructions: "When a test spies on console.warn or another console method, require the spy to be restored with mockRestore(), vi.restoreAllMocks(), or try/finally. vi.clearAllMocks() alone leaves the mocked implementation installed and can hide warnings in later tests."
Also applies to: 380-427, 429-457, 459-482, 484-506, 508-546, 548-579
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/scripts/pnginfo.test.ts` around lines 357 - 378, The importA1111 test
block is leaving spies and mocked console implementations active across tests,
which can suppress output in later cases. Add cleanup in the relevant test scope
so spies on console.error, console.warn, LiteGraph.createNode, and
api.getEmbeddings are restored after each test, using vi.restoreAllMocks() or
individual mockRestore() calls. Make sure the cleanup is applied around the
importA1111 describe block so tests like “stops when a required base node cannot
be created” and “skips missing LoraLoader nodes” do not leak into later tests.
Source: Path instructions
| }) | ||
|
|
||
| it('stores an error entry when fetch fails', async () => { | ||
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore the console.warn spy to prevent silencing warnings in subsequent tests.
vi.spyOn(console, 'warn') at line 115 is never restored. The afterEach calls vi.unstubAllGlobals() which does not restore spy implementations. The mocked console.warn (which silently swallows all warnings) leaks into every subsequent test in this file, potentially hiding real warnings from the cache eviction or error-path tests.
As per path instructions: "Require vi.restoreAllMocks(), mockRestore(), or explicit descriptor restoration" when tests spy on console methods.
🛡️ Proposed fix: add vi.restoreAllMocks() to afterEach
afterEach(() => {
window.dispatchEvent(new Event('beforeunload'))
vi.useRealTimers()
vi.unstubAllGlobals()
+ vi.restoreAllMocks()
})vi.restoreAllMocks() restores all vi.spyOn spies to their original implementations without affecting the vi.fn() mocks (mockFetch, mockCreateObjectURL, mockRevokeObjectURL), which are reset via mockReset() in beforeEach.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) | |
| afterEach(() => { | |
| window.dispatchEvent(new Event('beforeunload')) | |
| vi.useRealTimers() | |
| vi.unstubAllGlobals() | |
| vi.restoreAllMocks() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/services/mediaCacheService.test.ts` at line 115, The `console.warn` spy
in `mediaCacheService` tests is leaking across cases because it is never
restored, so add spy cleanup in the test lifecycle and restore all `vi.spyOn`
mocks after each test. Update the existing `afterEach` in
`mediaCacheService.test.ts` to call `vi.restoreAllMocks()` alongside the current
global unstubbing so the `console.warn` spy is reset while preserving the
`vi.fn()` mocks used in this suite.
Source: Path instructions
Comment on lines
+140
to
+156
| it('does not save when canvas is unavailable', () => { | ||
| const store = useSubgraphNavigationStore() | ||
| const canvas = app.canvas | ||
| const appWithOptionalCanvas = app as { | ||
| canvas: typeof app.canvas | undefined | ||
| } | ||
| appWithOptionalCanvas.canvas = undefined | ||
|
|
||
| try { | ||
| store.saveViewport('root') | ||
|
|
||
| expect(store.viewportCache.has(':root')).toBe(false) | ||
| } finally { | ||
| appWithOptionalCanvas.canvas = canvas | ||
| } | ||
| }) | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated "temporarily undefine app.canvas" scaffolding across three tests.
The same type-assertion + try/finally pattern for temporarily setting app.canvas to undefined is repeated three times (Lines 140-156, 185-201, 203-219). Consider extracting a small test helper, e.g. withoutCanvas(fn), to reduce duplication and ensure restoration is consistent.
♻️ Example helper
function withoutCanvas(fn: () => void) {
const canvas = app.canvas
;(app as { canvas: typeof app.canvas | undefined }).canvas = undefined
try {
fn()
} finally {
;(app as { canvas: typeof app.canvas | undefined }).canvas = canvas
}
}Also applies to: 185-220
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/stores/subgraphNavigationStore.viewport.test.ts` around lines 140 - 156,
The repeated temporary `app.canvas = undefined` setup in
`subgraphNavigationStore.viewport.test.ts` should be extracted into a small
helper to remove duplication and keep restoration consistent. Add a reusable
helper like `withoutCanvas(fn)` near the `useSubgraphNavigationStore` tests, and
update the three canvas-unavailable cases to run their assertions through it
instead of repeating the type assertion and try/finally block.
Comment on lines
+11
to
+15
| it('exposes the all-zero UUID fallback value', () => { | ||
| expect('10000000-1000-4000-8000-100000000000').toBe( | ||
| '10000000-1000-4000-8000-100000000000' | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the tautological test.
This test asserts a string literal equals itself — it exercises no behavior of createUuidv4 and provides no real coverage. As per path instructions, avoid change-detector tests that assert implementation details rather than observable outputs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/uuid.test.ts` around lines 11 - 15, Remove the tautological
assertion from the uuid test and replace it with a real behavioral check for
createUuidv4 or delete the test entirely if no observable fallback behavior is
exposed. Use the existing uuid.test.ts suite and the createUuidv4 symbol as the
place to verify a meaningful output rather than comparing a string literal to
itself.
Source: Path instructions
Comment on lines
+17
to
+21
| it('accepts impossible UUID input', () => { | ||
| const impossible = undefined as never | ||
|
|
||
| expect(impossible).toBeUndefined() | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the as never cast and the tautological test.
Line 18 uses as never, which is explicitly flagged by path instructions. The test itself only asserts that undefined is undefined, exercising no behavior of createUuidv4.
🧹 Proposed fix: remove both tautological tests
- it('exposes the all-zero UUID fallback value', () => {
- expect('10000000-1000-4000-8000-100000000000').toBe(
- '10000000-1000-4000-8000-100000000000'
- )
- })
-
- it('accepts impossible UUID input', () => {
- const impossible = undefined as never
-
- expect(impossible).toBeUndefined()
- })
-
it('uses crypto.randomUUID when available', () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('accepts impossible UUID input', () => { | |
| const impossible = undefined as never | |
| expect(impossible).toBeUndefined() | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/uuid.test.ts` around lines 17 - 21, The test in uuid.test.ts is
tautological and uses an запрещенный-looking `as never` cast, so remove this
entire case and any similar no-op assertions. Keep the test suite focused on
exercising `createUuidv4` behavior with meaningful inputs and assertions rather
than checking that `undefined` is `undefined`.
Source: Path instructions
Comment on lines
+43
to
+49
| it('does nothing for empty clipboard data', () => { | ||
| const canvas = createCanvas() | ||
|
|
||
| deserialiseAndCreate('', canvas) | ||
|
|
||
| expect(canvas.emitBeforeChange).not.toHaveBeenCalled() | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Thin assertion for the empty-data early-return case.
Only emitBeforeChange not being called is checked. Consider also asserting canvas.graph!.add and canvas.selectNodes were not called to more fully confirm the early return short-circuits all side effects.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/vintageClipboard.test.ts` around lines 43 - 49, The empty-data
early-return test in deserialiseAndCreate is too narrow because it only verifies
emitBeforeChange is skipped. Update the vintageClipboard.test case to also
assert that canvas.graph!.add and canvas.selectNodes are not called, so the test
fully covers that deserialiseAndCreate short-circuits all side effects when
clipboard data is empty.
Covers the DrJKL PR #13362 feedback that repeated addDropHandler calls install document/canvas listeners that are not removed by vi.clearAllMocks, making later drop dispatches order-dependent. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Covers the DrJKL PR #13362 feedback for inline required_input_missing fixtures and setActivePinia(createPinia()) in executionErrorStore tests; these should be flagged explicitly, not inferred from generic test guidance. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Covers the DrJKL PR #13362 feedback requiring direct compressWidgetInputSlots coverage for links: [null, validLink], rather than accepting unrelated linkFixer coverage. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Covers the DrJKL PR #13362 feedback for hand-rolled LiteGraph node, canvas, workflow, and subgraph builders in app, pnginfo, subgraphService, and vintageClipboard tests. Verified with: ruby -e 'require "yaml"; YAML.load_file(".coderabbit.yaml")' and git diff --check.
Replays representative PR #13362 patterns on top of PR #13486 so CodeRabbit can prove the updated path instructions catch DrJKL-style feedback: credential cleanup, sparse link coverage, listener/global teardown, rootGraph coverage, helper reuse, change-detector tests, as-never casts, and dropped async callbacks. Verified with: pnpm typecheck and git diff --check.
huang47
force-pushed
the
codex/coderabbit-drjkl-canary-2
branch
from
3b903f0 to
39faa04
Compare
Contributor
Author
|
@coderabbitai review Created by Codex |
Contributor
|
Action performedReview triggered.
|
huang47
force-pushed
the
codex/coderabbit-test-quality-path-instructions
branch
from
479bf2f to
3a7ec3e
Compare
Contributor
Author
|
Closing this broader canary because #13486 was simplified back to the three original path-instruction commits. Created by Codex |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Draft canary PR for validating #13486 CodeRabbit path instructions against representative DrJKL review feedback from #13362.
This intentionally replays bad or incomplete test patterns so CodeRabbit can prove it reports similar feedback before #13486 is merged.
Validation
Created by Codex