feat(video): VIDEO_EDIT rich widget with trim and crop editors - #14118
Closed
jtydhr88 wants to merge 1 commit into
Closed
feat(video): VIDEO_EDIT rich widget with trim and crop editors#14118jtydhr88 wants to merge 1 commit into
jtydhr88 wants to merge 1 commit into
Conversation
dosubot
Bot
added
the
size:XXL
🌐 Website E2ETip All tests passed.
🔗 Website PreviewWebsite Preview: https://comfy-website-preview-pr-14118.vercel.app This commit: https://website-frontend-7g873mebd-comfyui.vercel.app Last updated: 2026-07-28T02:55:07Z for |
🎭 Playwright: ✅ 1738 passed, 0 failed · 1 flaky📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle: 8.14 MB gzip 🔴 +17.2 kBDetailsSummary
Category Glance App Entry Points — 3.64 kB (baseline 3.64 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.28 MB (baseline 1.28 MB) • 🔴 +211 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed / 1 unchanged Views & Navigation — 112 kB (baseline 112 kB) • 🔴 +419 BTop-level views, pages, and routed surfaces
Status: 13 added / 13 removed / 3 unchanged Panels & Settings — 551 kB (baseline 551 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 11 added / 11 removed / 15 unchanged User & Accounts — 29.1 kB (baseline 29.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 7 added / 7 removed / 3 unchanged Editors & Dialogs — 124 kB (baseline 121 kB) • 🔴 +2.99 kBModals, dialogs, drawers, and in-app editors
Status: 6 added / 5 removed / 1 unchanged UI Components — 70.5 kB (baseline 64.6 kB) • 🔴 +5.89 kBReusable component library chunks
Status: 7 added / 6 removed / 8 unchanged Data & Services — 3.38 MB (baseline 3.37 MB) • 🔴 +2.19 kBStores, services, APIs, and repositories
Status: 14 added / 14 removed / 3 unchanged Utilities & Hooks — 372 kB (baseline 357 kB) • 🔴 +15.1 kBHelpers, composables, and utility bundles
Status: 19 added / 18 removed / 17 unchanged Vendor & Third-Party — 15.7 MB (baseline 15.7 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 16 unchanged Other — 12.5 MB (baseline 12.5 MB) • 🔴 +41 kBBundles that do not match a named category
Status: 81 added / 79 removed / 198 unchanged ⚡ Performance Report
Show regressions
All metrics
Historical variance (last 15 runs)
Trend (last 15 commits on main)
Raw data{
"timestamp": "2026-07-28T03:06:03.947Z",
"gitSha": "130865bf372429805ddfdaecbd195e5acf42c36f",
"branch": "feat/trim-video-rich-widget",
"measurements": [
{
"name": "canvas-idle",
"durationMs": 2059.8190000000045,
"styleRecalcs": 8,
"styleRecalcDurationMs": 8.038,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 573.898,
"heapDeltaBytes": 3788956,
"heapUsedBytes": 71526884,
"domNodes": 16,
"jsHeapTotalBytes": 20582400,
"scriptDurationMs": 21.410999999999994,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-idle",
"durationMs": 2032.9910000000382,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.733,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 505.48499999999996,
"heapDeltaBytes": 3605592,
"heapUsedBytes": 71401152,
"domNodes": 18,
"jsHeapTotalBytes": 20320256,
"scriptDurationMs": 20.866999999999997,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1869.512000000043,
"styleRecalcs": 75,
"styleRecalcDurationMs": 39.148,
"layouts": 12,
"layoutDurationMs": 3.636,
"taskDurationMs": 949.609,
"heapDeltaBytes": -16499400,
"heapUsedBytes": 51470844,
"domNodes": -278,
"jsHeapTotalBytes": 20971520,
"scriptDurationMs": 132.47299999999998,
"eventListeners": -146,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66999999999998,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-mouse-sweep",
"durationMs": 1843.6480000000302,
"styleRecalcs": 77,
"styleRecalcDurationMs": 40.213,
"layouts": 12,
"layoutDurationMs": 3.2700000000000005,
"taskDurationMs": 877.7159999999999,
"heapDeltaBytes": -1678612,
"heapUsedBytes": 66006228,
"domNodes": 62,
"jsHeapTotalBytes": 21368832,
"scriptDurationMs": 132.006,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1753.9479999999799,
"styleRecalcs": 31,
"styleRecalcDurationMs": 19.97,
"layouts": 6,
"layoutDurationMs": 0.6399999999999999,
"taskDurationMs": 436.541,
"heapDeltaBytes": 7339384,
"heapUsedBytes": 75160900,
"domNodes": 77,
"jsHeapTotalBytes": 20058112,
"scriptDurationMs": 25.110000000000003,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "canvas-zoom-sweep",
"durationMs": 1718.4090000000651,
"styleRecalcs": 31,
"styleRecalcDurationMs": 17.551000000000005,
"layouts": 6,
"layoutDurationMs": 0.5689999999999998,
"taskDurationMs": 436.424,
"heapDeltaBytes": 7024992,
"heapUsedBytes": 74854416,
"domNodes": 76,
"jsHeapTotalBytes": 20320256,
"scriptDurationMs": 26.307000000000002,
"eventListeners": 19,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "dom-widget-clipping",
"durationMs": 615.1049999999714,
"styleRecalcs": 12,
"styleRecalcDurationMs": 8.030000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 397.45,
"heapDeltaBytes": -12142888,
"heapUsedBytes": 55901136,
"domNodes": 20,
"jsHeapTotalBytes": 21368832,
"scriptDurationMs": 61.770999999999994,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "dom-widget-clipping",
"durationMs": 610.904000000005,
"styleRecalcs": 13,
"styleRecalcDurationMs": 9.799,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 401.112,
"heapDeltaBytes": -12184668,
"heapUsedBytes": 55873652,
"domNodes": 22,
"jsHeapTotalBytes": 21106688,
"scriptDurationMs": 61.940999999999995,
"eventListeners": 2,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-idle",
"durationMs": 2029.8980000000029,
"styleRecalcs": 8,
"styleRecalcDurationMs": 9.368000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 740.566,
"heapDeltaBytes": 6773356,
"heapUsedBytes": 66008600,
"domNodes": -276,
"jsHeapTotalBytes": 4526080,
"scriptDurationMs": 117.542,
"eventListeners": -142,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-idle",
"durationMs": 2030.6779999999662,
"styleRecalcs": 9,
"styleRecalcDurationMs": 9.371,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 796.8449999999999,
"heapDeltaBytes": 6488852,
"heapUsedBytes": 65273308,
"domNodes": -275,
"jsHeapTotalBytes": 5050368,
"scriptDurationMs": 125.109,
"eventListeners": -142,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 2183.5230000000365,
"styleRecalcs": 68,
"styleRecalcDurationMs": 13.345000000000002,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1326.617,
"heapDeltaBytes": 4355820,
"heapUsedBytes": 64043412,
"domNodes": -276,
"jsHeapTotalBytes": 4206592,
"scriptDurationMs": 440.47,
"eventListeners": -142,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "large-graph-pan",
"durationMs": 2255.1599999999326,
"styleRecalcs": 68,
"styleRecalcDurationMs": 13.929,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 1396.006,
"heapDeltaBytes": 5823020,
"heapUsedBytes": 65558932,
"domNodes": -277,
"jsHeapTotalBytes": 4993024,
"scriptDurationMs": 470.411,
"eventListeners": -144,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.670000000000012,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3246.1000000000126,
"styleRecalcs": 66,
"styleRecalcDurationMs": 16.783999999999995,
"layouts": 60,
"layoutDurationMs": 7.356,
"taskDurationMs": 1597.5469999999998,
"heapDeltaBytes": 1788444,
"heapUsedBytes": 63214804,
"domNodes": -280,
"jsHeapTotalBytes": 7409664,
"scriptDurationMs": 548.454,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "large-graph-zoom",
"durationMs": 3365.2640000000247,
"styleRecalcs": 65,
"styleRecalcDurationMs": 15.418000000000001,
"layouts": 60,
"layoutDurationMs": 7.771,
"taskDurationMs": 1652.978,
"heapDeltaBytes": 6286224,
"heapUsedBytes": 68347896,
"domNodes": -283,
"jsHeapTotalBytes": 6885376,
"scriptDurationMs": 579.056,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2038.8740000000212,
"styleRecalcs": 6,
"styleRecalcDurationMs": 6.304999999999998,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 753.8460000000001,
"heapDeltaBytes": 6845208,
"heapUsedBytes": 67784376,
"domNodes": -280,
"jsHeapTotalBytes": 4526080,
"scriptDurationMs": 121.726,
"eventListeners": -146,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "minimap-idle",
"durationMs": 2013.716000000045,
"styleRecalcs": 8,
"styleRecalcDurationMs": 7.776999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 842.778,
"heapDeltaBytes": 6119560,
"heapUsedBytes": 66518052,
"domNodes": -278,
"jsHeapTotalBytes": 5050368,
"scriptDurationMs": 128.864,
"eventListeners": -142,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 605.1879999999983,
"styleRecalcs": 47,
"styleRecalcDurationMs": 12.101,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 422.1550000000001,
"heapDeltaBytes": -11623056,
"heapUsedBytes": 56345524,
"domNodes": 20,
"jsHeapTotalBytes": 21106688,
"scriptDurationMs": 123.36899999999999,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "subgraph-dom-widget-clipping",
"durationMs": 657.7079999999569,
"styleRecalcs": 48,
"styleRecalcDurationMs": 13.328999999999999,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 440.507,
"heapDeltaBytes": -11933324,
"heapUsedBytes": 55878976,
"domNodes": 22,
"jsHeapTotalBytes": 22417408,
"scriptDurationMs": 127.96200000000002,
"eventListeners": 8,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2005.5720000000292,
"styleRecalcs": 9,
"styleRecalcDurationMs": 8.729000000000001,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 480.60499999999996,
"heapDeltaBytes": 3709968,
"heapUsedBytes": 71631424,
"domNodes": 18,
"jsHeapTotalBytes": 20320256,
"scriptDurationMs": 16.631,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-idle",
"durationMs": 2021.2430000000268,
"styleRecalcs": 10,
"styleRecalcDurationMs": 9.659,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 564.0459999999999,
"heapDeltaBytes": -20373940,
"heapUsedBytes": 47555864,
"domNodes": -273,
"jsHeapTotalBytes": 20971520,
"scriptDurationMs": 18.991,
"eventListeners": -148,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1711.7470000000026,
"styleRecalcs": 76,
"styleRecalcDurationMs": 37.80799999999999,
"layouts": 16,
"layoutDurationMs": 4.936999999999999,
"taskDurationMs": 825.063,
"heapDeltaBytes": -5646188,
"heapUsedBytes": 62327692,
"domNodes": 62,
"jsHeapTotalBytes": 21630976,
"scriptDurationMs": 98.03300000000002,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-mouse-sweep",
"durationMs": 1699.1100000000188,
"styleRecalcs": 75,
"styleRecalcDurationMs": 35.938,
"layouts": 16,
"layoutDurationMs": 4.0280000000000005,
"taskDurationMs": 817.7919999999999,
"heapDeltaBytes": -5609064,
"heapUsedBytes": 62276036,
"domNodes": 62,
"jsHeapTotalBytes": 21368832,
"scriptDurationMs": 99.37599999999999,
"eventListeners": 4,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.699999999999818
},
{
"name": "subgraph-transition-enter",
"durationMs": 1401.544000000058,
"styleRecalcs": 17,
"styleRecalcDurationMs": 30.709,
"layouts": 13,
"layoutDurationMs": 13.122000000000002,
"taskDurationMs": 991.784,
"heapDeltaBytes": 29135328,
"heapUsedBytes": 99280312,
"domNodes": 13673,
"jsHeapTotalBytes": 15990784,
"scriptDurationMs": 37.61200000000001,
"eventListeners": 2371,
"totalBlockingTimeMs": 146,
"frameDurationMs": 16.66333333333335,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8487.586000000021,
"styleRecalcs": 250,
"styleRecalcDurationMs": 41.321999999999996,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4766.001,
"heapDeltaBytes": 6973540,
"heapUsedBytes": 65885020,
"domNodes": -277,
"jsHeapTotalBytes": 6275072,
"scriptDurationMs": 1440.113,
"eventListeners": -128,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "viewport-pan-sweep",
"durationMs": 8837.23299999997,
"styleRecalcs": 249,
"styleRecalcDurationMs": 38.647,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 4973.14,
"heapDeltaBytes": 10081032,
"heapUsedBytes": 69330044,
"domNodes": -276,
"jsHeapTotalBytes": 7323648,
"scriptDurationMs": 1517.888,
"eventListeners": -126,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-idle",
"durationMs": 17045.241999999973,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17022.66,
"heapDeltaBytes": -42124132,
"heapUsedBytes": 167091796,
"domNodes": -8312,
"jsHeapTotalBytes": -14094336,
"scriptDurationMs": 587.08,
"eventListeners": -16387,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-idle",
"durationMs": 17460.83199999998,
"styleRecalcs": 0,
"styleRecalcDurationMs": 0,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 17426.896000000004,
"heapDeltaBytes": -37068788,
"heapUsedBytes": 171724616,
"domNodes": -8312,
"jsHeapTotalBytes": -8327168,
"scriptDurationMs": 615.124,
"eventListeners": -16389,
"totalBlockingTimeMs": 0,
"frameDurationMs": 17.776666666666642,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "vue-large-graph-pan",
"durationMs": 20888.660999999956,
"styleRecalcs": 145,
"styleRecalcDurationMs": 18.645000000000024,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 20854.641,
"heapDeltaBytes": -52990884,
"heapUsedBytes": 166307780,
"domNodes": -8312,
"jsHeapTotalBytes": -15142912,
"scriptDurationMs": 884.8179999999999,
"eventListeners": -16385,
"totalBlockingTimeMs": 221,
"frameDurationMs": 17.776666666666763,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "vue-large-graph-pan",
"durationMs": 21344.16799999997,
"styleRecalcs": 148,
"styleRecalcDurationMs": 19.710000000000004,
"layouts": 0,
"layoutDurationMs": 0,
"taskDurationMs": 21294.55,
"heapDeltaBytes": -29362640,
"heapUsedBytes": 179019964,
"domNodes": -8312,
"jsHeapTotalBytes": -10510336,
"scriptDurationMs": 962.496,
"eventListeners": -16383,
"totalBlockingTimeMs": 349,
"frameDurationMs": 17.776666666666763,
"p95FrameDurationMs": 16.799999999999272
},
{
"name": "workflow-execution",
"durationMs": 459.8109999999451,
"styleRecalcs": 14,
"styleRecalcDurationMs": 23.066000000000003,
"layouts": 3,
"layoutDurationMs": 1.0849999999999997,
"taskDurationMs": 125.483,
"heapDeltaBytes": -15952132,
"heapUsedBytes": 50972184,
"domNodes": 130,
"jsHeapTotalBytes": 7999488,
"scriptDurationMs": 11.468,
"eventListeners": 67,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.66333333333332,
"p95FrameDurationMs": 16.700000000000728
},
{
"name": "workflow-execution",
"durationMs": 488.99599999992915,
"styleRecalcs": 19,
"styleRecalcDurationMs": 25.719999999999995,
"layouts": 3,
"layoutDurationMs": 1.2609999999999997,
"taskDurationMs": 129.2,
"heapDeltaBytes": -16045316,
"heapUsedBytes": 50742064,
"domNodes": 145,
"jsHeapTotalBytes": 7737344,
"scriptDurationMs": 10.714000000000002,
"eventListeners": 65,
"totalBlockingTimeMs": 0,
"frameDurationMs": 16.666666666666668,
"p95FrameDurationMs": 16.800000000000182
}
]
} |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete ChangesVideo editing widget
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WidgetVideoEdit
participant VideoEditPanel
participant useVideoFilmstrip
participant useTrimPlayback
participant HTMLVideoElement
WidgetVideoEdit->>useVideoFilmstrip: load video URL and metadata
useVideoFilmstrip-->>VideoEditPanel: provide thumbnails and frame metadata
VideoEditPanel->>useTrimPlayback: handle scrub or playback change
useTrimPlayback->>HTMLVideoElement: seek or play preview
HTMLVideoElement-->>VideoEditPanel: emit metadata and time updates
VideoEditPanel-->>WidgetVideoEdit: update trim and crop model values
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #14118 +/- ##
==========================================
- Coverage 79.24% 79.13% -0.12%
==========================================
Files 1730 1750 +20
Lines 96155 97671 +1516
Branches 30905 31339 +434
==========================================
+ Hits 76197 77289 +1092
- Misses 19578 19991 +413
- Partials 380 391 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 45 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![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: 11
🤖 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/components/videoEdit/VideoEditPanel.vue`:
- Around line 269-279: Update the useTrimPlayback call in VideoEditPanel to pass
the reactive trimEnabled toggle to its trimEnabled option instead of the static
hasTrim feature flag, so playback clamping follows the user’s current Trim Video
setting.
In `@src/components/videoEdit/VideoFilmstripTrim.vue`:
- Around line 198-204: Replace the hardcoded 1rem and 2rem offsets in
timelineInsetLeftStyle, leftDimStyle, rightDimStyle, and selectionStyle with
values derived from HANDLE_WIDTH_PX, preserving the existing normalized
positioning and dimming behavior while keeping all overlay calculations
consistent with the px-based pointer math.
- Around line 28-43: Add a keyboard-accessible scrubbing path to the track
element handled by startScrubDrag and the corresponding playhead handle around
the referenced template section. Make the interactive control focusable with an
appropriate tabindex, expose its current position and bounds accessibly, and
handle keyboard input to move the playhead using established frame-update logic
while preserving pointer scrubbing and disabled behavior.
In `@src/composables/video/useVideoEditModel.ts`:
- Around line 44-74: Update the startFrame and endFrame setters in the trim
computed values to clamp each dragged handle against the other handle’s current
position, preventing non-positive durations when handles cross. Preserve
duration 0 only for the intentional “trim to video end” case, so endFrame does
not jump to frameMax after crossing; add a regression test in
useVideoEditModel.test.ts covering this behavior.
In `@src/composables/video/useVideoFilmstrip.ts`:
- Around line 132-142: Update the sampling flow in useVideoFilmstrip around
sampleFilmstripFrames so it accepts and invokes an isStale callback after each
sampled frame, using the existing loadId/url staleness state. Stop the sampling
loop immediately when the callback reports stale, while preserving the final
isLoadStale guard before assigning thumbnails.value.
- Around line 14-31: Update waitForEvent to reject after a bounded timeout in
addition to the existing event and error handlers, ensuring cleanup clears the
timer and removes listeners on every completion path. Keep successful event
resolution unchanged so sampleFilmstripFrames cannot remain loading indefinitely
when seeked never arrives.
In `@src/composables/video/useVideoSourceUrl.test.ts`:
- Around line 64-124: Add coverage in the useVideoSourceUrl test suite for the
subgraph-resolution path: configure getInputNode to return a node recognized by
isSubgraphNode(), mock resolveSubgraphOutputLink() with a representative output,
and assert mountSource resolves that output as videoUrl. Also cover the relevant
null/empty fallback behavior for the resolver while preserving existing direct
upstream and unlinked-input cases.
In `@src/composables/video/useVideoSourceUrl.ts`:
- Around line 91-102: The updateVideoUrl watchers in the video source composable
currently deep-watch graph-wide nodeOutputs and nodePreviewImages for every
widget; replace them with per-source-node reactive lookups keyed to the resolved
source node so unrelated graph mutations do not retrigger this widget. Also
ensure the reactive dependencies include video-input rewiring, so
resolveSourceNode changes cause videoUrl to refresh before the new source
executes, while preserving updates for the source file widget value and relevant
output/preview changes.
In `@src/lib/litegraph/src/types/widgets.ts`:
- Around line 391-394: Extract and export a named VideoEditTrim type from the
widgets types module, containing start_time and duration, then replace the
inline trim shape in VideoEditValue and the duplicated trim types used by
setTrim and the default-value construction in useVideoEditWidget.ts with
VideoEditTrim. Update imports as needed while preserving the existing optional
trim behavior.
In
`@src/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.test.ts`:
- Around line 38-41: Update the options assertion in the relevant video edit
widget test to also verify the rendering contract: canvasOnly must be false and
hideInPanel must be true, while retaining the existing feature and serialization
assertions.
In `@src/utils/videoMetadataUtil.ts`:
- Around line 5-12: Update the zVideoMetadata schema to reject invalid values
before timeline calculations: require width and height to be positive integers,
size and duration to be nonnegative numbers, fps to be positive when present,
and frame_count to be a nonnegative integer when present. Preserve the existing
nullable behavior for fps, duration, and frame_count.
🪄 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: 0822b032-03ec-4ea8-86d6-f3fe97102b91
📒 Files selected for processing (41)
packages/design-system/src/css/style.csssrc/components/videoEdit/VideoCropOverlay.test.tssrc/components/videoEdit/VideoCropOverlay.vuesrc/components/videoEdit/VideoEditPanel.test.tssrc/components/videoEdit/VideoEditPanel.vuesrc/components/videoEdit/VideoFilmstripTrim.test.tssrc/components/videoEdit/VideoFilmstripTrim.vuesrc/components/videoEdit/WidgetVideoEdit.test.tssrc/components/videoEdit/WidgetVideoEdit.vuesrc/composables/useRangeEditor.test.tssrc/composables/useRangeEditor.tssrc/composables/video/useCropBoxEditor.test.tssrc/composables/video/useCropBoxEditor.tssrc/composables/video/useCropRatioLock.test.tssrc/composables/video/useCropRatioLock.tssrc/composables/video/useTimelineScrub.tssrc/composables/video/useTrimPlayback.test.tssrc/composables/video/useTrimPlayback.tssrc/composables/video/useVideoEditFormats.test.tssrc/composables/video/useVideoEditFormats.tssrc/composables/video/useVideoEditModel.test.tssrc/composables/video/useVideoEditModel.tssrc/composables/video/useVideoFilmstrip.test.tssrc/composables/video/useVideoFilmstrip.tssrc/composables/video/useVideoSourceUrl.test.tssrc/composables/video/useVideoSourceUrl.tssrc/lib/litegraph/src/types/widgets.tssrc/lib/litegraph/src/widgets/VideoEditWidget.tssrc/lib/litegraph/src/widgets/widgetMap.tssrc/locales/en/main.jsonsrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/widgets/components/layout/index.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.tssrc/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.tssrc/schemas/nodeDef/nodeDefSchemaV2.tssrc/scripts/widgets.tssrc/utils/videoFrameUtil.test.tssrc/utils/videoFrameUtil.tssrc/utils/videoMetadataUtil.test.tssrc/utils/videoMetadataUtil.ts
jtydhr88
force-pushed
the
feat/trim-video-rich-widget
branch
from
July 26, 2026 20:39
ec1328d to
b244d6f
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
src/components/videoEdit/VideoEditPanel.vue (1)
269-279: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrim "enabled" toggle vs. trim "feature exists" flag inconsistently applied.
useTrimPlaybackis wired withhasTrimTimeline: hasTrim(269-279) — the static "trim feature exists" flag — whiletrimEnabled(the user's toggle,defineModelat line 249) is never passed to the composable at all. Similarly,metadataRows(375-393) branches onhasTrim.valuealone to decide whether to show "selected of total" duration/frame text, rather thanhasTrim.value && trimEnabled.valueas is correctly done elsewhere in this same file (lines 80, 87, 94).Net effect: after a user trims a clip and then turns the "Trim Video" toggle off (retaining stale
startFrame/endFrame), the preview playback clamping and the metadata panel may still behave/display as if trimming were active, since neither path checks the reactivetrimEnabledstate.A previous review round flagged this exact confusion for the (differently-named)
useTrimPlaybackparameter and was marked addressed, but the current code still funnels the statichasTrimflag in under the newhasTrimTimelinekey — worth confirming whether the composable now expects a separatetrimEnabledinput that simply isn't being passed.🐛 Proposed fix for `metadataRows`
{ label: t('videoEdit.duration'), - value: hasTrim.value + value: hasTrim.value && trimEnabled.value ? t('videoEdit.selectedOfTotal', { selected: formatDuration(selectedDurationSeconds.value), total: formatDuration(duration) }) : formatDuration(duration) }, { label: t('videoEdit.frames'), - value: hasTrim.value + value: hasTrim.value && trimEnabled.value ? t('videoEdit.selectedOfTotal', { selected: selectedFrameCount.value, total: effectiveTotalFrames.value }) : String(effectiveTotalFrames.value) },Please verify
useTrimPlayback's contract forhasTrimTimelineto determine iftrimEnabledalso needs to be passed for correct clamping behavior:#!/bin/bash fd -a 'useTrimPlayback.ts' src/composables cat -n src/composables/video/useTrimPlayback.ts rg -n 'hasTrimTimeline|trimEnabled' src/composables/video/useTrimPlayback.ts src/composables/video/useTrimPlayback.test.tsAlso applies to: 375-393
🤖 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/components/videoEdit/VideoEditPanel.vue` around lines 269 - 279, Update the VideoEditPanel.vue integration so trim behavior depends on the reactive trimEnabled state: verify the useTrimPlayback contract and pass trimEnabled separately if required instead of relying only on hasTrimTimeline, while preserving the static feature-availability flag. In metadataRows, require both hasTrim.value and trimEnabled.value before displaying selected-versus-total trim duration or frame text.
🤖 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/composables/video/useTimelineScrub.ts`:
- Around line 1-97: Add a colocated useTimelineScrub.test.ts covering
pointer-to-frame boundary clamping, scrubToFrame enforcement of scrubMin and
scrubMax, and startScrubDrag lifecycle cleanup including pointer release and
scope disposal. Follow the existing useRangeEditor.test.ts testing pattern and
include edge cases where pointer positions fall outside the track bounds.
- Around line 35-46: Extract the shared pointer-to-normalized-position
calculation from pointerToFrame and use it in both pointerToFrame and
useRangeEditor’s pointerToValue. The helper should accept clientX, the element’s
bounding rect, and the horizontal inset, preserve clamping to [0,1] and the
minimum content width, and leave each caller’s frame/value denormalization
unchanged.
In `@src/composables/video/useTrimPlayback.ts`:
- Around line 40-50: Update waitForVideoSeek to include the same bounded timeout
behavior as useVideoFilmstrip.ts’s waitForEvent, using the established
SEEK_EVENT_TIMEOUT_MS symbol if available. Ensure the timeout settles the
promise and removes both seeked/error listeners, while preserving immediate
resolution when either event fires so isSeeking cannot remain stuck.
In `@src/composables/video/useVideoEditModel.ts`:
- Around line 87-113: Clamp cropBounds setter values in cropBounds within
useVideoEditModel.ts to the source dimensions before storing them, constraining
x/y and the crop’s right/bottom edges to the valid [0, width.value] and [0,
height.value] ranges while preserving the full-frame reset behavior; reuse the
existing clamp approach used by startFrame/endFrame. In
src/composables/video/useVideoEditModel.test.ts lines 108-147, add a regression
test for an out-of-bounds cropBounds write, such as negative x or oversized
width, and assert the stored bounds are clamped.
In `@src/composables/video/useVideoFilmstrip.test.ts`:
- Around line 99-198: Add an error-path test for useVideoFilmstrip that makes
the mocked video element emit error instead of loadedmetadata, then await
loading to finish and assert error.value is set while thumbnails, totalFrames,
and other derived state are reset appropriately. Reuse the existing
runWithScope, video mock setup, and cleanup patterns, and target the loadVideo
failure branch.
In `@src/composables/video/useVideoFilmstrip.ts`:
- Around line 145-178: Separate the metadata-loading flow from thumbnail
sampling so errors from sampleFilmstripFrames or its per-frame callback are
handled without entering the catch that resets duration, totalFrames, width,
height, fps, and fileSize. Preserve the successfully resolved metadata and only
clear thumbnails or record the thumbnail-specific failure, while keeping
stale-load checks and existing metadata error handling intact.
In
`@src/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.test.ts`:
- Around line 46-58: Add a test alongside the existing trim-only case in
useVideoEditWidget that invokes the widget with features set to only crop and
verifies the created value contains only the crop defaults, with options
preserving features: ['crop']. This should specifically exercise the crop
feature gate independently of trim.
In `@src/utils/videoMetadataUtil.test.ts`:
- Around line 61-67: The malformed-response coverage in the fetchVideoMetadata
tests does not exercise the numeric schema guards. Add a test using the expected
metadata shape but an invalid numeric value, such as a negative width, and
assert that fetchVideoMetadata returns undefined; include the relevant positive,
nonnegative, or integer boundary cases as appropriate while preserving the
existing malformed-shape test.
---
Duplicate comments:
In `@src/components/videoEdit/VideoEditPanel.vue`:
- Around line 269-279: Update the VideoEditPanel.vue integration so trim
behavior depends on the reactive trimEnabled state: verify the useTrimPlayback
contract and pass trimEnabled separately if required instead of relying only on
hasTrimTimeline, while preserving the static feature-availability flag. In
metadataRows, require both hasTrim.value and trimEnabled.value before displaying
selected-versus-total trim duration or frame text.
🪄 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: 4fdfad45-0c40-4e46-b9e1-95c88f0c43e8
📒 Files selected for processing (41)
packages/design-system/src/css/style.csssrc/components/videoEdit/VideoCropOverlay.test.tssrc/components/videoEdit/VideoCropOverlay.vuesrc/components/videoEdit/VideoEditPanel.test.tssrc/components/videoEdit/VideoEditPanel.vuesrc/components/videoEdit/VideoFilmstripTrim.test.tssrc/components/videoEdit/VideoFilmstripTrim.vuesrc/components/videoEdit/WidgetVideoEdit.test.tssrc/components/videoEdit/WidgetVideoEdit.vuesrc/composables/useRangeEditor.test.tssrc/composables/useRangeEditor.tssrc/composables/video/useCropBoxEditor.test.tssrc/composables/video/useCropBoxEditor.tssrc/composables/video/useCropRatioLock.test.tssrc/composables/video/useCropRatioLock.tssrc/composables/video/useTimelineScrub.tssrc/composables/video/useTrimPlayback.test.tssrc/composables/video/useTrimPlayback.tssrc/composables/video/useVideoEditFormats.test.tssrc/composables/video/useVideoEditFormats.tssrc/composables/video/useVideoEditModel.test.tssrc/composables/video/useVideoEditModel.tssrc/composables/video/useVideoFilmstrip.test.tssrc/composables/video/useVideoFilmstrip.tssrc/composables/video/useVideoSourceUrl.test.tssrc/composables/video/useVideoSourceUrl.tssrc/lib/litegraph/src/types/widgets.tssrc/lib/litegraph/src/widgets/VideoEditWidget.tssrc/lib/litegraph/src/widgets/widgetMap.tssrc/locales/en/main.jsonsrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/widgets/components/layout/index.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.tssrc/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.tssrc/schemas/nodeDef/nodeDefSchemaV2.tssrc/scripts/widgets.tssrc/utils/videoFrameUtil.test.tssrc/utils/videoFrameUtil.tssrc/utils/videoMetadataUtil.test.tssrc/utils/videoMetadataUtil.ts
Comment on lines
+1
to
+97
| import { onScopeDispose, ref } from 'vue' | ||
| import type { Ref } from 'vue' | ||
|
|
||
| import { clamp } from 'es-toolkit' | ||
|
|
||
| import { denormalize } from '@/utils/mathUtil' | ||
|
|
||
| interface UseTimelineScrubOptions { | ||
| trackRef: Ref<HTMLElement | null> | ||
| frameMax: Ref<number> | ||
| scrubMin: Ref<number> | ||
| scrubMax: Ref<number> | ||
| contentInsetX: number | ||
| isDisabled: () => boolean | ||
| onScrub: (frame: number) => void | ||
| } | ||
|
|
||
| export function useTimelineScrub( | ||
| playheadFrame: Ref<number>, | ||
| options: UseTimelineScrubOptions | ||
| ) { | ||
| const { | ||
| trackRef, | ||
| frameMax, | ||
| scrubMin, | ||
| scrubMax, | ||
| contentInsetX, | ||
| isDisabled, | ||
| onScrub | ||
| } = options | ||
|
|
||
| const isScrubDragging = ref(false) | ||
| let cleanupScrubDrag: (() => void) | null = null | ||
|
|
||
| function pointerToFrame(event: PointerEvent) { | ||
| const el = trackRef.value | ||
| if (!el) return playheadFrame.value | ||
| const rect = el.getBoundingClientRect() | ||
| const contentWidth = Math.max(rect.width - 2 * contentInsetX, 1) | ||
| const normalized = clamp( | ||
| (event.clientX - rect.left - contentInsetX) / contentWidth, | ||
| 0, | ||
| 1 | ||
| ) | ||
| return Math.round(denormalize(normalized, 0, frameMax.value)) | ||
| } | ||
|
|
||
| function scrubToFrame(frame: number) { | ||
| const clamped = clamp(frame, scrubMin.value, scrubMax.value) | ||
| playheadFrame.value = clamped | ||
| onScrub(clamped) | ||
| } | ||
|
|
||
| function updateScrubFromPointer(event: PointerEvent) { | ||
| const frame = pointerToFrame(event) | ||
| if (frame === playheadFrame.value) return | ||
| scrubToFrame(frame) | ||
| } | ||
|
|
||
| function startScrubDrag(event: PointerEvent) { | ||
| if (isDisabled() || event.button !== 0) return | ||
|
|
||
| const el = trackRef.value | ||
| if (!el) return | ||
|
|
||
| cleanupScrubDrag?.() | ||
|
|
||
| isScrubDragging.value = true | ||
| scrubToFrame(pointerToFrame(event)) | ||
| el.setPointerCapture(event.pointerId) | ||
|
|
||
| const onMove = (moveEvent: PointerEvent) => { | ||
| updateScrubFromPointer(moveEvent) | ||
| } | ||
|
|
||
| const endDrag = () => { | ||
| isScrubDragging.value = false | ||
| el.removeEventListener('pointermove', onMove) | ||
| el.removeEventListener('pointerup', endDrag) | ||
| el.removeEventListener('lostpointercapture', endDrag) | ||
| cleanupScrubDrag = null | ||
| } | ||
|
|
||
| cleanupScrubDrag = endDrag | ||
|
|
||
| el.addEventListener('pointermove', onMove) | ||
| el.addEventListener('pointerup', endDrag) | ||
| el.addEventListener('lostpointercapture', endDrag) | ||
| } | ||
|
|
||
| onScopeDispose(() => { | ||
| isScrubDragging.value = false | ||
| cleanupScrubDrag?.() | ||
| }) | ||
|
|
||
| return { isScrubDragging, startScrubDrag } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Missing unit tests for useTimelineScrub.
This composable performs non-trivial pointer-to-frame math and drag-state clamping, similar in complexity to useRangeEditor.ts and useCropBoxEditor.ts, both of which have colocated test files. No useTimelineScrub.test.ts is present here. Consider adding tests for pointerToFrame clamping, scrubToFrame bounds (scrubMin/scrubMax), and drag lifecycle cleanup, following the existing pattern in useRangeEditor.test.ts.
As per path instructions, .agents/checks/test-quality.md requires "cover key edge cases beyond happy paths ... boundary collisions/clamping" for new behaviors such as this scrub-clamping logic.
🤖 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/composables/video/useTimelineScrub.ts` around lines 1 - 97, Add a
colocated useTimelineScrub.test.ts covering pointer-to-frame boundary clamping,
scrubToFrame enforcement of scrubMin and scrubMax, and startScrubDrag lifecycle
cleanup including pointer release and scope disposal. Follow the existing
useRangeEditor.test.ts testing pattern and include edge cases where pointer
positions fall outside the track bounds.
Source: Path instructions
Comment on lines
+35
to
+46
| function pointerToFrame(event: PointerEvent) { | ||
| const el = trackRef.value | ||
| if (!el) return playheadFrame.value | ||
| const rect = el.getBoundingClientRect() | ||
| const contentWidth = Math.max(rect.width - 2 * contentInsetX, 1) | ||
| const normalized = clamp( | ||
| (event.clientX - rect.left - contentInsetX) / contentWidth, | ||
| 0, | ||
| 1 | ||
| ) | ||
| return Math.round(denormalize(normalized, 0, frameMax.value)) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate pointer-to-normalized-position math vs. useRangeEditor.ts.
This inset-aware normalization (clientX - rect.left - inset, rect.width - 2*inset, clamp to [0,1]) is nearly identical to pointerToValue in src/composables/useRangeEditor.ts (lines 35-41). Extracting a shared helper (e.g. pointerToNormalized(clientX, rect, inset)) would prevent the two implementations from drifting apart as inset handling evolves.
🤖 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/composables/video/useTimelineScrub.ts` around lines 35 - 46, Extract the
shared pointer-to-normalized-position calculation from pointerToFrame and use it
in both pointerToFrame and useRangeEditor’s pointerToValue. The helper should
accept clientX, the element’s bounding rect, and the horizontal inset, preserve
clamping to [0,1] and the minimum content width, and leave each caller’s
frame/value denormalization unchanged.
Comment on lines
+87
to
+113
| const cropBounds = computed<Bounds>({ | ||
| get: () => { | ||
| const crop = modelValue.value.crop | ||
| if (!crop || crop.width <= 0 || crop.height <= 0) { | ||
| return { x: 0, y: 0, width: width.value, height: height.value } | ||
| } | ||
| return { ...crop } | ||
| }, | ||
| set: (next) => { | ||
| const coversFullFrame = | ||
| next.x <= 0 && | ||
| next.y <= 0 && | ||
| next.width >= width.value && | ||
| next.height >= height.value | ||
| modelValue.value = { | ||
| ...modelValue.value, | ||
| crop: coversFullFrame | ||
| ? { x: 0, y: 0, width: 0, height: 0 } | ||
| : { | ||
| x: Math.round(next.x), | ||
| y: Math.round(next.y), | ||
| width: Math.round(next.width), | ||
| height: Math.round(next.height) | ||
| } | ||
| } | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Crop bounds aren't clamped to source dimensions, and no test covers it. The cropBounds setter in useVideoEditModel.ts only special-cases the "covers full frame" reset; any other out-of-range value (negative x/y, or x+width/y+height beyond the source size) is stored verbatim, unlike the meticulously-clamped trim setters in the same file.
src/composables/video/useVideoEditModel.ts#L87-L113: clampnext.x/y/width/heightagainst[0, width.value]/[0, height.value]before storing, mirroring theclamp()usage already used forstartFrame/endFrame.src/composables/video/useVideoEditModel.test.ts#L108-L147: add a regression test asserting that an out-of-boundscropBoundswrite (e.g. negativexor oversizedwidth) gets clamped, following the same pattern used for the trim-handle-crossover regression tests at lines 84-105.
📍 Affects 2 files
src/composables/video/useVideoEditModel.ts#L87-L113(this comment)src/composables/video/useVideoEditModel.test.ts#L108-L147
🤖 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/composables/video/useVideoEditModel.ts` around lines 87 - 113, Clamp
cropBounds setter values in cropBounds within useVideoEditModel.ts to the source
dimensions before storing them, constraining x/y and the crop’s right/bottom
edges to the valid [0, width.value] and [0, height.value] ranges while
preserving the full-frame reset behavior; reuse the existing clamp approach used
by startFrame/endFrame. In src/composables/video/useVideoEditModel.test.ts lines
108-147, add a regression test for an out-of-bounds cropBounds write, such as
negative x or oversized width, and assert the stored bounds are clamped.
Comment on lines
+145
to
+178
| const metadata = await fetchVideoMetadata(url) | ||
|
|
||
| if (isLoadStale(loadId, url)) return | ||
|
|
||
| fps.value = metadata?.fps ?? options.fps ?? DEFAULT_VIDEO_FPS | ||
| fileSize.value = metadata?.size | ||
| totalFrames.value = | ||
| metadata?.frame_count ?? | ||
| Math.max(Math.round(videoDuration * fps.value), 1) | ||
|
|
||
| const sampledThumbnails = await sampleFilmstripFrames( | ||
| video, | ||
| canvas, | ||
| context, | ||
| videoDuration, | ||
| sampleCount, | ||
| () => isLoadStale(loadId, url) | ||
| ) | ||
|
|
||
| if (isLoadStale(loadId, url)) return | ||
|
|
||
| thumbnails.value = sampledThumbnails | ||
| } catch (loadError) { | ||
| if (isLoadStale(loadId, url)) return | ||
| error.value = | ||
| loadError instanceof Error ? loadError.message : 'Failed to load video' | ||
| duration.value = 0 | ||
| totalFrames.value = 0 | ||
| width.value = 0 | ||
| height.value = 0 | ||
| fps.value = options.fps ?? DEFAULT_VIDEO_FPS | ||
| fileSize.value = undefined | ||
| thumbnails.value = [] | ||
| } finally { |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Isolate thumbnail-sampling failures from already-fetched metadata.
The single try/catch around lines 134-184 means any exception thrown while sampling filmstrip frames (e.g. captureFrame's canvas.toDataURL throwing a SecurityError on a tainted canvas, or any other per-frame draw failure) falls into the same catch block that resets duration, totalFrames, width, height, fps, and fileSize — even though those were already successfully resolved from loadedmetadata and fetchVideoMetadata before sampling began. A thumbnail-only failure shouldn't wipe out valid, already-fetched metadata.
♻️ Proposed fix
- const sampledThumbnails = await sampleFilmstripFrames(
- video,
- canvas,
- context,
- videoDuration,
- sampleCount,
- () => isLoadStale(loadId, url)
- )
-
- if (isLoadStale(loadId, url)) return
-
- thumbnails.value = sampledThumbnails
+ let sampledThumbnails: string[] = []
+ try {
+ sampledThumbnails = await sampleFilmstripFrames(
+ video,
+ canvas,
+ context,
+ videoDuration,
+ sampleCount,
+ () => isLoadStale(loadId, url)
+ )
+ } catch {
+ // Preserve already-fetched duration/fps/size even if capture fails
+ }
+
+ if (isLoadStale(loadId, url)) return
+
+ thumbnails.value = sampledThumbnails📝 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 metadata = await fetchVideoMetadata(url) | |
| if (isLoadStale(loadId, url)) return | |
| fps.value = metadata?.fps ?? options.fps ?? DEFAULT_VIDEO_FPS | |
| fileSize.value = metadata?.size | |
| totalFrames.value = | |
| metadata?.frame_count ?? | |
| Math.max(Math.round(videoDuration * fps.value), 1) | |
| const sampledThumbnails = await sampleFilmstripFrames( | |
| video, | |
| canvas, | |
| context, | |
| videoDuration, | |
| sampleCount, | |
| () => isLoadStale(loadId, url) | |
| ) | |
| if (isLoadStale(loadId, url)) return | |
| thumbnails.value = sampledThumbnails | |
| } catch (loadError) { | |
| if (isLoadStale(loadId, url)) return | |
| error.value = | |
| loadError instanceof Error ? loadError.message : 'Failed to load video' | |
| duration.value = 0 | |
| totalFrames.value = 0 | |
| width.value = 0 | |
| height.value = 0 | |
| fps.value = options.fps ?? DEFAULT_VIDEO_FPS | |
| fileSize.value = undefined | |
| thumbnails.value = [] | |
| } finally { | |
| const metadata = await fetchVideoMetadata(url) | |
| if (isLoadStale(loadId, url)) return | |
| fps.value = metadata?.fps ?? options.fps ?? DEFAULT_VIDEO_FPS | |
| fileSize.value = metadata?.size | |
| totalFrames.value = | |
| metadata?.frame_count ?? | |
| Math.max(Math.round(videoDuration * fps.value), 1) | |
| let sampledThumbnails: string[] = [] | |
| try { | |
| sampledThumbnails = await sampleFilmstripFrames( | |
| video, | |
| canvas, | |
| context, | |
| videoDuration, | |
| sampleCount, | |
| () => isLoadStale(loadId, url) | |
| ) | |
| } catch { | |
| // Preserve already-fetched duration/fps/size even if capture fails | |
| } | |
| if (isLoadStale(loadId, url)) return | |
| thumbnails.value = sampledThumbnails | |
| } catch (loadError) { | |
| if (isLoadStale(loadId, url)) return | |
| error.value = | |
| loadError instanceof Error ? loadError.message : 'Failed to load video' | |
| duration.value = 0 | |
| totalFrames.value = 0 | |
| width.value = 0 | |
| height.value = 0 | |
| fps.value = options.fps ?? DEFAULT_VIDEO_FPS | |
| fileSize.value = undefined | |
| thumbnails.value = [] | |
| } finally { |
🤖 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/composables/video/useVideoFilmstrip.ts` around lines 145 - 178, Separate
the metadata-loading flow from thumbnail sampling so errors from
sampleFilmstripFrames or its per-frame callback are handled without entering the
catch that resets duration, totalFrames, width, height, fps, and fileSize.
Preserve the successfully resolved metadata and only clear thumbnails or record
the thumbnail-specific failure, while keeping stale-load checks and existing
metadata error handling intact.
Comment on lines
+46
to
+58
| it('only creates the sections listed in features', () => { | ||
| const { node, addWidget } = createNode() | ||
|
|
||
| useVideoEditWidget()(node, { | ||
| type: 'VIDEO_EDIT', | ||
| name: 'trim', | ||
| features: ['trim'] | ||
| }) | ||
|
|
||
| const [, , value, , options] = addWidget.mock.calls[0] | ||
| expect(value).toEqual({ trim: { start_time: 0, duration: 0 } }) | ||
| expect(options).toMatchObject({ features: ['trim'] }) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Cover the crop-only feature gate.
The suite verifies trim-only and all-features defaults, but not features: ['crop']. A crop branch accidentally gated on trim would still pass current tests.
Proposed test
+ it('only creates crop when crop is the enabled feature', () => {
+ const { node, addWidget } = createNode()
+
+ useVideoEditWidget()(node, {
+ type: 'VIDEO_EDIT',
+ name: 'crop',
+ features: ['crop']
+ })
+
+ const [, , value, , options] = addWidget.mock.calls[0]
+ expect(value).toEqual({ crop: { x: 0, y: 0, width: 0, height: 0 } })
+ expect(options).toMatchObject({ features: ['crop'] })
+ })📝 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('only creates the sections listed in features', () => { | |
| const { node, addWidget } = createNode() | |
| useVideoEditWidget()(node, { | |
| type: 'VIDEO_EDIT', | |
| name: 'trim', | |
| features: ['trim'] | |
| }) | |
| const [, , value, , options] = addWidget.mock.calls[0] | |
| expect(value).toEqual({ trim: { start_time: 0, duration: 0 } }) | |
| expect(options).toMatchObject({ features: ['trim'] }) | |
| }) | |
| it('only creates the sections listed in features', () => { | |
| const { node, addWidget } = createNode() | |
| useVideoEditWidget()(node, { | |
| type: 'VIDEO_EDIT', | |
| name: 'trim', | |
| features: ['trim'] | |
| }) | |
| const [, , value, , options] = addWidget.mock.calls[0] | |
| expect(value).toEqual({ trim: { start_time: 0, duration: 0 } }) | |
| expect(options).toMatchObject({ features: ['trim'] }) | |
| }) | |
| it('only creates crop when crop is the enabled feature', () => { | |
| const { node, addWidget } = createNode() | |
| useVideoEditWidget()(node, { | |
| type: 'VIDEO_EDIT', | |
| name: 'crop', | |
| features: ['crop'] | |
| }) | |
| const [, , value, , options] = addWidget.mock.calls[0] | |
| expect(value).toEqual({ crop: { x: 0, y: 0, width: 0, height: 0 } }) | |
| expect(options).toMatchObject({ features: ['crop'] }) | |
| }) |
🤖 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/vueNodes/widgets/composables/useVideoEditWidget.test.ts`
around lines 46 - 58, Add a test alongside the existing trim-only case in
useVideoEditWidget that invokes the widget with features set to only crop and
verifies the created value contains only the crop defaults, with options
preserving features: ['crop']. This should specifically exercise the crop
feature gate independently of trim.
Source: Path instructions
Comment on lines
+61
to
+67
| it('returns undefined for malformed responses', async () => { | ||
| mockResponse(true, { unexpected: true }) | ||
|
|
||
| const result = await fetchVideoMetadata('/api/view?filename=a.mp4') | ||
|
|
||
| expect(result).toBeUndefined() | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider testing the new numeric guards directly.
The malformed-response test only checks a wholly different shape ({ unexpected: true }). Since the previous review's schema hardening (.positive(), .nonnegative(), .int()) was the main fix in this file, add a case with valid-shaped-but-invalid values (e.g., negative width) to confirm those guards actually reject bad metadata.
♻️ Suggested addition
+ it('rejects metadata with invalid numeric values', async () => {
+ mockResponse(true, { ...metadata, width: -10 })
+
+ const result = await fetchVideoMetadata('/api/view?filename=a.mp4')
+
+ expect(result).toBeUndefined()
+ })As per path instructions ("Treat .agents/checks/test-quality.md ... as required review context"), which asks to "cover key edge cases beyond happy paths (empty/undefined inputs, error/exception flows, disabled interactions, boundary collisions/clamping)".
📝 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('returns undefined for malformed responses', async () => { | |
| mockResponse(true, { unexpected: true }) | |
| const result = await fetchVideoMetadata('/api/view?filename=a.mp4') | |
| expect(result).toBeUndefined() | |
| }) | |
| it('returns undefined for malformed responses', async () => { | |
| mockResponse(true, { unexpected: true }) | |
| const result = await fetchVideoMetadata('/api/view?filename=a.mp4') | |
| expect(result).toBeUndefined() | |
| }) | |
| it('rejects metadata with invalid numeric values', async () => { | |
| mockResponse(true, { ...metadata, width: -10 }) | |
| const result = await fetchVideoMetadata('/api/view?filename=a.mp4') | |
| expect(result).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/videoMetadataUtil.test.ts` around lines 61 - 67, The
malformed-response coverage in the fetchVideoMetadata tests does not exercise
the numeric schema guards. Add a test using the expected metadata shape but an
invalid numeric value, such as a negative width, and assert that
fetchVideoMetadata returns undefined; include the relevant positive,
nonnegative, or integer boundary cases as appropriate while preserving the
existing malformed-shape test.
Source: Path instructions
jtydhr88
marked this pull request as draft
July 26, 2026 21:25
This was referenced Jul 26, 2026
jtydhr88
force-pushed
the
feat/trim-video-rich-widget
branch
from
July 26, 2026 22:20
b244d6f to
680c75b
Compare
jtydhr88
added a commit
that referenced
this pull request
Jul 26, 2026
#14120) This is seperate part for the big PR of video edit, crop, trim PR #14118 ## Summary - Optional contentInsetX maps pointer positions onto a track whose value range is horizontally inset (e.g. tracks with handle gutters) - Expose activeHandle so consumers can render per-handle UI while dragging
jtydhr88
force-pushed
the
feat/trim-video-rich-widget
branch
from
July 26, 2026 22:45
680c75b to
1811e2f
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/components/videoEdit/VideoCropOverlay.test.ts`:
- Line 1: Remove the leading U+FEFF byte-order mark from
VideoCropOverlay.test.ts so the eslint-disable directive begins at the first
character of the file; leave the directive and remaining test content unchanged.
- Around line 23-27: Update the VideoCropOverlay tests to exercise the
lockedRatio option in renderOverlay, preferably by adding a case with
lockedRatio: 1 that resizes the crop box and asserts the resulting bounds remain
square; otherwise remove the unused option and related ratio-specific setup.
In `@src/components/videoEdit/VideoEditPanel.test.ts`:
- Around line 122-131: Extend the VideoEditPanel test suite to cover the reset
start/end frame controls by locating them via their existing aria-labels,
asserting each emits the corresponding update event with 0 or frameMax, and
verifying each is disabled at its respective extreme. Reuse the existing
trim-enabled setup and test helpers without changing panel behavior.
In `@src/components/videoEdit/VideoFilmstripTrim.test.ts`:
- Around line 45-49: Remove the mirrored expectedFrameAt helper and replace its
usages in the VideoFilmstripTrim tests with literal expected frame values, such
as 50 for clientX 100. Assert the resulting playheadFrame state directly so the
tests document intended behavior rather than duplicating the component’s inset,
normalization, and rounding logic.
In `@src/components/videoEdit/WidgetVideoEdit.test.ts`:
- Around line 16-42: The mock setup in
src/components/videoEdit/WidgetVideoEdit.test.ts lines 16-42 should stop using
require('vue') and eslint suppressions; add a top-level ref import and use it in
both useVideoSourceUrl and useVideoFilmstrip mock factories. In
src/components/videoEdit/VideoFilmstripTrim.test.ts lines 6-19, keep only a
plain mutable holder in vi.hoisted for activeHandle and create the Vue ref
inside the useRangeEditor mock factory.
In `@src/components/videoEdit/WidgetVideoEdit.vue`:
- Line 52: Update the computed node lookup to optional-chain app.canvas before
accessing graph, preserving the existing graph and getNodeById optional chaining
so evaluation remains safe before setup assigns the canvas.
In `@src/composables/video/useCropRatioLock.test.ts`:
- Around line 98-114: Extend the tests around createLock to cover enabling the
lock with initial bounds of width 0 and height 0. Assert that lockedRatio
remains null or finite and never captures NaN, preserving the expected startup
behavior before metadata resolves.
In `@src/composables/video/useCropRatioLock.ts`:
- Around line 72-82: Guard the ratio capture in the isLockEnabled setter so
locking only stores bounds.value.width / bounds.value.height when the computed
ratio is finite. If the dimensions produce NaN or Infinity, leave lockedRatio
unset so the lock remains disabled; preserve the existing unlock behavior and
valid-ratio flow.
🪄 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: c2cfd801-3348-4595-9b0e-77203b922884
📒 Files selected for processing (39)
packages/design-system/src/css/style.csssrc/components/videoEdit/VideoCropOverlay.test.tssrc/components/videoEdit/VideoCropOverlay.vuesrc/components/videoEdit/VideoEditPanel.test.tssrc/components/videoEdit/VideoEditPanel.vuesrc/components/videoEdit/VideoFilmstripTrim.test.tssrc/components/videoEdit/VideoFilmstripTrim.vuesrc/components/videoEdit/WidgetVideoEdit.test.tssrc/components/videoEdit/WidgetVideoEdit.vuesrc/composables/video/useCropBoxEditor.test.tssrc/composables/video/useCropBoxEditor.tssrc/composables/video/useCropRatioLock.test.tssrc/composables/video/useCropRatioLock.tssrc/composables/video/useTimelineScrub.tssrc/composables/video/useTrimPlayback.test.tssrc/composables/video/useTrimPlayback.tssrc/composables/video/useVideoEditFormats.test.tssrc/composables/video/useVideoEditFormats.tssrc/composables/video/useVideoEditModel.test.tssrc/composables/video/useVideoEditModel.tssrc/composables/video/useVideoFilmstrip.test.tssrc/composables/video/useVideoFilmstrip.tssrc/composables/video/useVideoSourceUrl.test.tssrc/composables/video/useVideoSourceUrl.tssrc/lib/litegraph/src/types/widgets.tssrc/lib/litegraph/src/widgets/VideoEditWidget.tssrc/lib/litegraph/src/widgets/widgetMap.tssrc/locales/en/main.jsonsrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/widgets/components/layout/index.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.tssrc/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.tssrc/schemas/nodeDef/nodeDefSchemaV2.tssrc/scripts/widgets.tssrc/utils/videoFrameUtil.test.tssrc/utils/videoFrameUtil.tssrc/utils/videoMetadataUtil.test.tssrc/utils/videoMetadataUtil.ts
| @@ -0,0 +1,140 @@ | |||
| /* eslint-disable testing-library/prefer-user-event -- crop dragging needs low-level pointer events */ | |||
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Strip the stray BOM at the start of the file.
Line 1 begins with a U+FEFF byte-order mark before the eslint directive, which is an accidental artifact and can confuse tooling that expects a directive comment as the first token.
🤖 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/components/videoEdit/VideoCropOverlay.test.ts` at line 1, Remove the
leading U+FEFF byte-order mark from VideoCropOverlay.test.ts so the
eslint-disable directive begins at the first character of the file; leave the
directive and remaining test content unchanged.
Comment on lines
+23
to
+27
| function renderOverlay({ | ||
| bounds = { x: 100, y: 100, width: 200, height: 200 }, | ||
| disabled = false, | ||
| lockedRatio = null as number | null | ||
| } = {}) { |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
lockedRatio is accepted but never exercised.
No test passes a locked ratio, so the ratio-constrained resize path through useCropBoxEditor.ratioResize has no component-level coverage here. Add a case (e.g. lockedRatio: 1) asserting the resulting box stays square, or drop the unused option.
As per path instructions, .agents/checks/test-quality.md asks to check for "missing coverage for any newly added behaviors".
🤖 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/components/videoEdit/VideoCropOverlay.test.ts` around lines 23 - 27,
Update the VideoCropOverlay tests to exercise the lockedRatio option in
renderOverlay, preferably by adding a case with lockedRatio: 1 that resizes the
crop box and asserts the resulting bounds remain square; otherwise remove the
unused option and related ratio-specific setup.
Source: Path instructions
Comment on lines
+122
to
+131
| it('keeps the filmstrip visible but collapses trim controls until enabled', async () => { | ||
| renderPanel({ features: ['trim'] }) | ||
|
|
||
| expect(screen.getByTestId('stub-filmstrip')).toBeTruthy() | ||
| expect(screen.queryByTestId('stub-number-input')).toBeNull() | ||
|
|
||
| await userEvent.click(screen.getByTestId('toggle-trim_enabled')) | ||
|
|
||
| expect(screen.getAllByTestId('stub-number-input')).toHaveLength(2) | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Cover the reset start/end frame buttons.
setStartFrame/setEndFrame (VideoEditPanel.vue lines 293-303) are real panel logic — they stop playback, snap the bound to 0/frameMax, and are disabled at the extremes — but no test clicks them. Their aria-labels (Reset start frame / Reset end frame) are already in the test i18n messages, so a case asserting the emitted update:startFrame and the disabled state is cheap.
As per path instructions, .agents/checks/test-quality.md asks to check for "missing coverage for any newly added behaviors".
🤖 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/components/videoEdit/VideoEditPanel.test.ts` around lines 122 - 131,
Extend the VideoEditPanel test suite to cover the reset start/end frame controls
by locating them via their existing aria-labels, asserting each emits the
corresponding update event with 0 or frameMax, and verifying each is disabled at
its respective extreme. Reuse the existing trim-enabled setup and test helpers
without changing panel behavior.
Source: Path instructions
Comment on lines
+45
to
+49
| function expectedFrameAt(clientX: number, width = 200, frameMax = 100) { | ||
| const contentWidth = Math.max(width - 32, 1) | ||
| const norm = Math.min(Math.max((clientX - 16) / contentWidth, 0), 1) | ||
| return Math.round(norm * frameMax) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Prefer literal expected frames over a mirrored formula.
expectedFrameAt re-implements the component's inset/normalize/round math, so the assertions describe the algorithm rather than the intended outcome. expect(playheadFrame.value).toBe(50) for clientX: 100 documents the behavior more directly and fails loudly if the inset changes.
As per path instructions, .agents/checks/test-quality.md asks to avoid change-detector tests that assert internals instead of resulting state.
🤖 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/components/videoEdit/VideoFilmstripTrim.test.ts` around lines 45 - 49,
Remove the mirrored expectedFrameAt helper and replace its usages in the
VideoFilmstripTrim tests with literal expected frame values, such as 50 for
clientX 100. Assert the resulting playheadFrame state directly so the tests
document intended behavior rather than duplicating the component’s inset,
normalization, and rounding logic.
Source: Path instructions
Comment on lines
+16
to
+42
| vi.mock('@/composables/video/useVideoSourceUrl', () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const { ref: createRef } = require('vue') | ||
| return { | ||
| useVideoSourceUrl: () => ({ | ||
| videoUrl: createRef('/api/view?filename=clip.mp4') | ||
| }) | ||
| } | ||
| }) | ||
|
|
||
| vi.mock('@/composables/video/useVideoFilmstrip', () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| const { ref: createRef } = require('vue') | ||
| return { | ||
| DEFAULT_VIDEO_FPS: 20, | ||
| useVideoFilmstrip: () => ({ | ||
| thumbnails: createRef([]), | ||
| duration: createRef(10), | ||
| totalFrames: createRef(101), | ||
| width: createRef(1920), | ||
| height: createRef(1080), | ||
| fps: createRef(10), | ||
| fileSize: createRef(1024), | ||
| loading: createRef(false) | ||
| }) | ||
| } | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Vue refs are created through require('vue') CJS interop in mock setup in both new test files. Both rely on require (plus eslint suppressions) to obtain ref, which is only necessary inside vi.hoisted; vi.mock factories run lazily and can use the ESM import directly.
src/components/videoEdit/WidgetVideoEdit.test.ts#L16-L42: remove bothrequire('vue')calls and the eslint suppressions, and use a top-levelimport { ref } from 'vue'inside the two mock factories.src/components/videoEdit/VideoFilmstripTrim.test.ts#L6-L19: move ref creation out ofvi.hoistedinto thevi.mock('@/composables/useRangeEditor')factory, keeping only a plain mutable holder hoisted for the test to driveactiveHandle.
📍 Affects 2 files
src/components/videoEdit/WidgetVideoEdit.test.ts#L16-L42(this comment)src/components/videoEdit/VideoFilmstripTrim.test.ts#L6-L19
🤖 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/components/videoEdit/WidgetVideoEdit.test.ts` around lines 16 - 42, The
mock setup in src/components/videoEdit/WidgetVideoEdit.test.ts lines 16-42
should stop using require('vue') and eslint suppressions; add a top-level ref
import and use it in both useVideoSourceUrl and useVideoFilmstrip mock
factories. In src/components/videoEdit/VideoFilmstripTrim.test.ts lines 6-19,
keep only a plain mutable holder in vi.hoisted for activeHandle and create the
Vue ref inside the useRangeEditor mock factory.
| (): VideoEditFeature[] => widget.options?.features ?? ['trim', 'crop'] | ||
| ) | ||
|
|
||
| const node = computed(() => app.canvas.graph?.getNodeById(nodeId)) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts --type=vue 'app\.canvas\??\.graph' -g '!**/*.test.ts' | head -40Repository: Comfy-Org/ComfyUI_frontend
Length of output: 192
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate files:"
fd -a 'WidgetVideoEdit\.vue$' . || true
echo
echo "Search app.canvas.graph and app.canvas patterns in Vue/TS files:"
rg -n --glob '!**/*.test.ts' --glob '!**/*.test.js' --glob '!**/*.spec.ts' --glob '!**/*.spec.js' 'app\.canvas(\?\.|\.)(graph|getCanvas)' | head -80 || true
echo
echo "Inspect WidgetVideoEdit outline and relevant lines:"
if [ -f src/components/videoEdit/WidgetVideoEdit.vue ]; then
wc -l src/components/videoEdit/WidgetVideoEdit.vue
ast-grep outline src/components/videoEdit/WidgetVideoEdit.vue --view compact || true
sed -n '1,90p' src/components/videoEdit/WidgetVideoEdit.vue
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "WidgetVideoEdit relevant content:"
wc -l src/components/videoEdit/WidgetVideoEdit.vue
sed -n '1,110p' src/components/videoEdit/WidgetVideoEdit.vue
echo
echo "Search app.canvas references (all files, excluding tests if present):"
rg -n 'app\.canvas' . | head -120 || true
echo
echo "Type definitions for app.canvas (if available):"
rg -n 'interface .*ComfyApp|type .*ComfyApp|class .*ComfyApp|declare const app|const app|canvas\?|canvas:' src -g '*.ts' -g '*.d.ts' | head -120 || trueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 26389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate app definition:"
fd -a 'app\.(ts|js)$|app/|app\.ts' src scripts | head -40 || true
echo
echo "Search ComfyApp.canvas declaration/assignment:"
rg -n 'canvas(\??)=|canvas\??:' src scripts -g '*.ts' -g '*.js' -g '*.d.ts' | head -120 || true
echo
echo "Inspect app scripts around createApp/comfyApp:"
for f in $(fd -a 'app\.ts$|Comfy.*Class|script.*app' src scripts | head -20); do
echo "--- $f"
wc -l "$f"
sed -n '1,140p' "$f"
done
echo
echo "Check WidgetVideoEdit tests mocks:"
sed -n '1,120p' src/components/videoEdit/WidgetVideoEdit.test.ts || trueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 21233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Canvas-related declarations and assignments in src/scripts/app.ts:"
rg -n 'canvas|LGraphCanvas|createLiteGraph|resizeCanvas' src/scripts/app.ts -C 3 | head -220
echo
echo "Standalone JS read-only probe for current expression shape:"
node - <<'JS'
const app = {}
function expression(canvas) {
app.canvas = canvas
return app.canvas.graph?.getNodeById?.('node-1')
}
function safeExpression(canvas) {
app.canvas = canvas
return app.canvas?.graph?.getNodeById?.('node-1')
}
console.log('undefined canvas shape:', expression(undefined))
console.log('safe undefined canvas shape:', safeExpression(undefined))
try {
expression({})
console.log('object canvas no crash')
} catch (error) {
console.log('object canvas crash:', error.name + ': ' + error.message)
}
JS
echo
echo "Search App lifecycle that might mount WidgetVideoEdit before canvas init:"
rg -n "WidgetVideoEdit|videoEdit|createLiteGraph|resizeCanvas|app\.canvas" src -g '*.ts' -g '*.vue' | head -200 || trueRepository: Comfy-Org/ComfyUI_frontend
Length of output: 7801
Optional-chain app.canvas before reading graph.
ComfyApp.canvas is declared non-null, but Vue 3 can evaluate this computed before setup() assigns it, so app.canvas.graph?.getNodeById(nodeId) throws with app.canvas === undefined. Use app.canvas?.graph?.getNodeById(nodeId) here.
🤖 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/components/videoEdit/WidgetVideoEdit.vue` at line 52, Update the computed
node lookup to optional-chain app.canvas before accessing graph, preserving the
existing graph and getNodeById optional chaining so evaluation remains safe
before setup assigns the canvas.
jtydhr88
force-pushed
the
feat/trim-video-rich-widget
branch
from
July 27, 2026 15:11
1811e2f to
d61ae17
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
src/utils/videoMetadataUtil.test.ts (1)
71-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStill missing coverage for the numeric schema guards.
The malformed-response test only covers a wholly different shape (
{ unexpected: true }). The schema hardening added invideoMetadataUtil.ts(.positive(),.nonnegative(),.int()) has no test confirming it actually rejects a validly-shaped-but-invalid payload (e.g., negativewidth).♻️ Suggested addition
+ it('rejects metadata with invalid numeric values', async () => { + mockResponse(true, { ...metadata, width: -10 }) + + const result = await fetchVideoMetadata('/api/view?filename=a.mp4') + + expect(result).toBeUndefined() + })As per path instructions (
.agents/checks/test-quality.md), which asks to flag "missing edge-case coverage" and cover error/exception flows.🤖 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/videoMetadataUtil.test.ts` around lines 71 - 77, Add a test alongside the malformed-response case in the video metadata test suite that supplies a structurally valid response with an invalid numeric field, such as a negative width, then verifies fetchVideoMetadata returns undefined. Ensure the case exercises the numeric guards added in the video metadata schema, including the relevant positive, nonnegative, or integer constraint.Source: Path instructions
src/components/videoEdit/VideoCropOverlay.test.ts (1)
23-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
lockedRatiois accepted but never exercised.No test passes a locked ratio, so the ratio-constrained resize path through
useCropBoxEditor's ratio-lock logic has no component-level coverage in this file.♻️ Suggested addition
+ it('keeps the crop box square when resizing with a locked ratio', async () => { + const { model } = renderOverlay({ lockedRatio: 1 }) + + const handle = screen.getByTestId('crop-handle-se') + handle.setPointerCapture = vi.fn() + + await fireEvent.pointerDown(handle, { clientX: 0, clientY: 0, button: 0, pointerId: 1 }) + await fireEvent.pointerMove(handle, { clientX: 10, clientY: 5, pointerId: 1 }) + await fireEvent.pointerUp(handle, { pointerId: 1 }) + + expect(model.value.width).toBe(model.value.height) + })As per path instructions (
.agents/checks/test-quality.md), which asks to flag "missing coverage for any newly added behaviors."🤖 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/components/videoEdit/VideoCropOverlay.test.ts` around lines 23 - 27, Add component-level coverage in VideoCropOverlay tests by rendering through renderOverlay with a non-null lockedRatio and exercising the resize behavior that uses useCropBoxEditor’s ratio-lock logic. Assert the resulting crop bounds preserve the requested aspect ratio, while leaving existing unlocked and disabled scenarios unchanged.Source: Path instructions
src/composables/video/useVideoFilmstrip.ts (1)
149-190: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThumbnail-sampling failures still wipe out already-resolved metadata.
The single try/catch spanning metadata resolution and
sampleFilmstripFramesmeans any exception during frame capture (e.g. a tainted-canvasSecurityErrorfromcanvas.toDataURL) falls into the same catch that callsresetVideoState(), discardingduration,width,height,fps, andfileSizethat were already successfully resolved vialoadedmetadata/fetchVideoMetadata.🩺 Proposed fix
- const sampledThumbnails = await sampleFilmstripFrames( - video, - canvas, - context, - effectiveDuration, - sampleCount, - () => isLoadStale(loadId, url) - ) - - if (isLoadStale(loadId, url)) return - - thumbnails.value = sampledThumbnails + let sampledThumbnails: string[] = [] + try { + sampledThumbnails = await sampleFilmstripFrames( + video, + canvas, + context, + effectiveDuration, + sampleCount, + () => isLoadStale(loadId, url) + ) + } catch { + // Preserve already-fetched duration/fps/size even if capture fails + } + + if (isLoadStale(loadId, url)) return + + thumbnails.value = sampledThumbnails🤖 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/composables/video/useVideoFilmstrip.ts` around lines 149 - 190, Separate metadata resolution from thumbnail sampling in the load flow around fetchVideoMetadata and sampleFilmstripFrames. Preserve resolved duration, dimensions, fps, fileSize, and totalFrames when sampling fails; handle sampling errors independently by recording the load failure without calling resetVideoState() on already-resolved metadata.src/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.test.ts (1)
46-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the crop-only feature gate.
The suite verifies trim-only and all-features defaults, but not
features: ['crop']. A crop branch accidentally gated ontrimwould still pass current tests.✅ Proposed test
+ it('only creates crop when crop is the enabled feature', () => { + const { node, addWidget } = createNode() + + useVideoEditWidget()(node, { + type: 'VIDEO_EDIT', + name: 'crop', + features: ['crop'] + }) + + const [, , value, , options] = addWidget.mock.calls[0] + expect(value).toEqual({ crop: { x: 0, y: 0, width: 0, height: 0 } }) + expect(options).toMatchObject({ features: ['crop'] }) + })🤖 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/vueNodes/widgets/composables/useVideoEditWidget.test.ts` around lines 46 - 58, Add a test beside the existing trim-only case for useVideoEditWidget with features set to ['crop']. Verify the generated value contains only the crop default and the widget options preserve the crop-only feature list, ensuring crop initialization is independently gated from trim.src/components/videoEdit/WidgetVideoEdit.vue (1)
52-52: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winOptional-chain
app.canvasbefore readinggraph.
ComfyApp.canvasis declared non-null, but this computed can evaluate beforesetup()assigns it, causingapp.canvas.graph?.getNodeById(nodeId)to throw whenapp.canvas === undefined.🛡️ Proposed fix
-const node = computed(() => app.canvas.graph?.getNodeById(nodeId)) +const node = computed(() => app.canvas?.graph?.getNodeById(nodeId))🤖 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/components/videoEdit/WidgetVideoEdit.vue` at line 52, Update the node computed property to optional-chain app.canvas before accessing graph, so it safely returns undefined while setup has not assigned the canvas; preserve the existing getNodeById(nodeId) behavior once canvas and graph are available.src/composables/video/useTimelineScrub.ts (1)
1-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing unit tests for
useTimelineScrub.This composable performs non-trivial pointer-to-frame math and drag-state clamping (similar in complexity to
useCropBoxEditor.ts, which has a colocated test file), but nouseTimelineScrub.test.tsexists. Consider coveringpointerToFrameclamping,scrubToFramebounds enforcement (scrubMin/scrubMax), and drag lifecycle cleanup.As per path instructions,
.agents/checks/test-quality.mdrequires flagging "missing edge-case coverage for the new video edit logic".🤖 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/composables/video/useTimelineScrub.ts` around lines 1 - 98, Add colocated unit tests for useTimelineScrub, covering pointerToFrame behavior at and beyond both track boundaries, scrubToFrame enforcement of scrubMin and scrubMax, and startScrubDrag lifecycle cleanup including pointer movement, pointerup, lostpointercapture, and disposal. Follow the existing useCropBoxEditor test conventions and flag the missing edge-case coverage for the new video edit logic.Source: Path instructions
src/composables/video/useVideoEditModel.ts (1)
87-113: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCrop bounds aren't clamped to source dimensions, and no test covers it. The
cropBoundssetter only special-cases the "covers full frame" reset; any other out-of-range value (negativex/y, orx+width/y+heightbeyond the source size) is stored verbatim, unlike the meticulously-clamped trim setters in the same file. This was previously flagged and remains unresolved.
src/composables/video/useVideoEditModel.ts#L87-L113: clampnext.x/y/width/heightagainst[0, width.value]/[0, height.value]before storing, mirroring theclamp()usage already used forstartFrame/endFrame.src/composables/video/useVideoEditModel.test.ts#L108-L147: add a regression test asserting that an out-of-boundscropBoundswrite (e.g. negativexor oversizedwidth) gets clamped, following the pattern used for the trim-handle-crossover regression tests.🤖 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/composables/video/useVideoEditModel.ts` around lines 87 - 113, Clamp cropBounds values in the cropBounds setter of useVideoEditModel against the source dimensions, constraining x/y and the resulting width/height to valid frame bounds while preserving the full-frame reset behavior. Add a regression test in src/composables/video/useVideoEditModel.test.ts:108-147 covering an out-of-bounds cropBounds write, such as negative x or oversized width, and assert the stored bounds are clamped; follow the existing trim-handle-crossover test pattern there.src/composables/video/useCropRatioLock.ts (1)
72-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winNon-finite ratio capture is unguarded, and no test pins the degenerate-box case that triggers it. Both sites share one root cause:
isLockEnabled's setter capturesbounds.value.width / bounds.value.heightwithout checking it's finite and positive.
src/composables/video/useCropRatioLock.ts#L72-L82: guard the capture —Number.isFinite(ratio) && ratio > 0 ? ratio : null— before assigninglockedRatio.value.src/composables/video/useCropRatioLock.test.ts#L71-L114: add a case that enables the lock whileboundsis{x:0,y:0,width:0,height:0}(the pre-metadata startup state) and assertslockedRatio.valuestaysnull/finite, neverNaN.🤖 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/composables/video/useCropRatioLock.ts` around lines 72 - 82, Guard ratio capture in isLockEnabled’s setter so only finite, positive width/height ratios are assigned; otherwise keep lockedRatio.value null. In src/composables/video/useCropRatioLock.ts:72-82, update the capture logic accordingly. In src/composables/video/useCropRatioLock.test.ts:71-114, add coverage enabling the lock with zero-sized bounds and assert lockedRatio.value remains null and never becomes NaN.src/composables/video/useTrimPlayback.ts (1)
40-50: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
waitForVideoSeekstill has no timeout — stalled seeks permanently wedgeisSeeking.This is the same gap flagged in a prior review (and marked as addressed in commits eaafcd1–1811e2f), but the code in this diff still has no bound: if
seeked/errornever fire (stalled network, revoked source mid-seek), the promise never resolves,isSeekingstaystrueforever, andhandleTimeUpdate(Line 113) permanently stops syncing the playhead / auto-stopping at the trim end. It also leaves the listeners attached to a possibly-detached video element if the component unmounts mid-seek.🛡️ Proposed fix
+const SEEK_TIMEOUT_MS = 5000 + function waitForVideoSeek(video: HTMLVideoElement): Promise<void> { return new Promise((resolve) => { - const finish = () => { + const timer = setTimeout(finish, SEEK_TIMEOUT_MS) + function finish() { + clearTimeout(timer) video.removeEventListener('seeked', finish) video.removeEventListener('error', finish) resolve() } video.addEventListener('seeked', finish, { once: true }) video.addEventListener('error', finish, { once: true }) }) }🤖 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/composables/video/useTrimPlayback.ts` around lines 40 - 50, Update waitForVideoSeek to guarantee completion when seeked/error never fire by adding a timeout fallback that resolves the promise. Ensure finish clears the timeout and removes both listeners, preserving cleanup for stalled seeks and preventing isSeeking from remaining true indefinitely.src/components/videoEdit/VideoEditPanel.test.ts (1)
106-175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMissing coverage for the reset start/end frame controls.
The i18n messages already include
setStartFrame/setEndFramelabels (Lines 26-27), but no test exercises the actual reset buttons inVideoEditPanel.vue. A case asserting the emittedupdate:startFrame/update:endFrameand the disabled state at the extremes would close this gap cheaply, reusing the existingrenderPanel({ features: ['trim'] })setup.🤖 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/components/videoEdit/VideoEditPanel.test.ts` around lines 106 - 175, The VideoEditPanel tests lack coverage for the trim reset controls. Add a test using renderPanel({ features: ['trim'] }) that verifies the start/end reset buttons emit update:startFrame and update:endFrame with the expected reset values, and confirms each button is disabled when its frame is already at the corresponding extreme.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/composables/video/useCropRatioLock.ts`:
- Line 39: Extract and export a shared minWidthForRatio(ratio) helper alongside
MIN_CROP_SIZE in useCropBoxEditor.ts, returning the existing minimum-size
formula. Update both useCropBoxEditor.ts and useCropRatioLock.ts to call this
helper instead of duplicating Math.max(MIN_CROP_SIZE, MIN_CROP_SIZE * ratio),
preserving current sizing behavior.
In `@src/composables/video/useVideoEditModel.test.ts`:
- Around line 108-147: The crop bounds tests in the “crop bounds” describe block
lack coverage for out-of-bounds writes. Add a regression test through
createModel that assigns negative x/y and oversized width/height to cropBounds,
then assert modelValue.crop is clamped to the valid video-frame bounds.
---
Duplicate comments:
In `@src/components/videoEdit/VideoCropOverlay.test.ts`:
- Around line 23-27: Add component-level coverage in VideoCropOverlay tests by
rendering through renderOverlay with a non-null lockedRatio and exercising the
resize behavior that uses useCropBoxEditor’s ratio-lock logic. Assert the
resulting crop bounds preserve the requested aspect ratio, while leaving
existing unlocked and disabled scenarios unchanged.
In `@src/components/videoEdit/VideoEditPanel.test.ts`:
- Around line 106-175: The VideoEditPanel tests lack coverage for the trim reset
controls. Add a test using renderPanel({ features: ['trim'] }) that verifies the
start/end reset buttons emit update:startFrame and update:endFrame with the
expected reset values, and confirms each button is disabled when its frame is
already at the corresponding extreme.
In `@src/components/videoEdit/WidgetVideoEdit.vue`:
- Line 52: Update the node computed property to optional-chain app.canvas before
accessing graph, so it safely returns undefined while setup has not assigned the
canvas; preserve the existing getNodeById(nodeId) behavior once canvas and graph
are available.
In `@src/composables/video/useCropRatioLock.ts`:
- Around line 72-82: Guard ratio capture in isLockEnabled’s setter so only
finite, positive width/height ratios are assigned; otherwise keep
lockedRatio.value null. In src/composables/video/useCropRatioLock.ts:72-82,
update the capture logic accordingly. In
src/composables/video/useCropRatioLock.test.ts:71-114, add coverage enabling the
lock with zero-sized bounds and assert lockedRatio.value remains null and never
becomes NaN.
In `@src/composables/video/useTimelineScrub.ts`:
- Around line 1-98: Add colocated unit tests for useTimelineScrub, covering
pointerToFrame behavior at and beyond both track boundaries, scrubToFrame
enforcement of scrubMin and scrubMax, and startScrubDrag lifecycle cleanup
including pointer movement, pointerup, lostpointercapture, and disposal. Follow
the existing useCropBoxEditor test conventions and flag the missing edge-case
coverage for the new video edit logic.
In `@src/composables/video/useTrimPlayback.ts`:
- Around line 40-50: Update waitForVideoSeek to guarantee completion when
seeked/error never fire by adding a timeout fallback that resolves the promise.
Ensure finish clears the timeout and removes both listeners, preserving cleanup
for stalled seeks and preventing isSeeking from remaining true indefinitely.
In `@src/composables/video/useVideoEditModel.ts`:
- Around line 87-113: Clamp cropBounds values in the cropBounds setter of
useVideoEditModel against the source dimensions, constraining x/y and the
resulting width/height to valid frame bounds while preserving the full-frame
reset behavior. Add a regression test in
src/composables/video/useVideoEditModel.test.ts:108-147 covering an
out-of-bounds cropBounds write, such as negative x or oversized width, and
assert the stored bounds are clamped; follow the existing trim-handle-crossover
test pattern there.
In `@src/composables/video/useVideoFilmstrip.ts`:
- Around line 149-190: Separate metadata resolution from thumbnail sampling in
the load flow around fetchVideoMetadata and sampleFilmstripFrames. Preserve
resolved duration, dimensions, fps, fileSize, and totalFrames when sampling
fails; handle sampling errors independently by recording the load failure
without calling resetVideoState() on already-resolved metadata.
In
`@src/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.test.ts`:
- Around line 46-58: Add a test beside the existing trim-only case for
useVideoEditWidget with features set to ['crop']. Verify the generated value
contains only the crop default and the widget options preserve the crop-only
feature list, ensuring crop initialization is independently gated from trim.
In `@src/utils/videoMetadataUtil.test.ts`:
- Around line 71-77: Add a test alongside the malformed-response case in the
video metadata test suite that supplies a structurally valid response with an
invalid numeric field, such as a negative width, then verifies
fetchVideoMetadata returns undefined. Ensure the case exercises the numeric
guards added in the video metadata schema, including the relevant positive,
nonnegative, or integer constraint.
🪄 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: 76d894b7-0fb3-4dc8-92f7-b2993eab11af
📒 Files selected for processing (39)
packages/design-system/src/css/style.csssrc/components/videoEdit/VideoCropOverlay.test.tssrc/components/videoEdit/VideoCropOverlay.vuesrc/components/videoEdit/VideoEditPanel.test.tssrc/components/videoEdit/VideoEditPanel.vuesrc/components/videoEdit/VideoFilmstripTrim.test.tssrc/components/videoEdit/VideoFilmstripTrim.vuesrc/components/videoEdit/WidgetVideoEdit.test.tssrc/components/videoEdit/WidgetVideoEdit.vuesrc/composables/video/useCropBoxEditor.test.tssrc/composables/video/useCropBoxEditor.tssrc/composables/video/useCropRatioLock.test.tssrc/composables/video/useCropRatioLock.tssrc/composables/video/useTimelineScrub.tssrc/composables/video/useTrimPlayback.test.tssrc/composables/video/useTrimPlayback.tssrc/composables/video/useVideoEditFormats.test.tssrc/composables/video/useVideoEditFormats.tssrc/composables/video/useVideoEditModel.test.tssrc/composables/video/useVideoEditModel.tssrc/composables/video/useVideoFilmstrip.test.tssrc/composables/video/useVideoFilmstrip.tssrc/composables/video/useVideoSourceUrl.test.tssrc/composables/video/useVideoSourceUrl.tssrc/lib/litegraph/src/types/widgets.tssrc/lib/litegraph/src/widgets/VideoEditWidget.tssrc/lib/litegraph/src/widgets/widgetMap.tssrc/locales/en/main.jsonsrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/widgets/components/layout/index.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.tssrc/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.tssrc/schemas/nodeDef/nodeDefSchemaV2.tssrc/scripts/widgets.tssrc/utils/videoFrameUtil.test.tssrc/utils/videoFrameUtil.tssrc/utils/videoMetadataUtil.test.tssrc/utils/videoMetadataUtil.ts
| Math.max(sourceW - x, 0) / width, | ||
| Math.max(sourceH - y, 0) / height | ||
| ) | ||
| width = Math.max(width * scale, MIN_CROP_SIZE, MIN_CROP_SIZE * ratio) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract the shared min-width-for-ratio formula.
Math.max(MIN_CROP_SIZE, MIN_CROP_SIZE * ratio) here duplicates the identical expression in src/composables/video/useCropBoxEditor.ts (Line 87). Since this file already imports MIN_CROP_SIZE from that module, a shared minWidthForRatio(ratio) helper exported alongside MIN_CROP_SIZE would keep the two composables' minimum-size math from drifting independently.
🤖 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/composables/video/useCropRatioLock.ts` at line 39, Extract and export a
shared minWidthForRatio(ratio) helper alongside MIN_CROP_SIZE in
useCropBoxEditor.ts, returning the existing minimum-size formula. Update both
useCropBoxEditor.ts and useCropRatioLock.ts to call this helper instead of
duplicating Math.max(MIN_CROP_SIZE, MIN_CROP_SIZE * ratio), preserving current
sizing behavior.
Comment on lines
+108
to
+147
| describe('crop bounds', () => { | ||
| it('shows the full frame for zero crop values', () => { | ||
| const { cropBounds } = createModel() | ||
|
|
||
| expect(cropBounds.value).toEqual({ | ||
| x: 0, | ||
| y: 0, | ||
| width: 1920, | ||
| height: 1080 | ||
| }) | ||
| }) | ||
|
|
||
| it('rounds and stores a partial crop', () => { | ||
| const { modelValue, cropBounds } = createModel() | ||
|
|
||
| cropBounds.value = { x: 10.4, y: 20.6, width: 640.2, height: 360.5 } | ||
|
|
||
| expect(modelValue.value.crop).toEqual({ | ||
| x: 10, | ||
| y: 21, | ||
| width: 640, | ||
| height: 361 | ||
| }) | ||
| }) | ||
|
|
||
| it('normalizes a full-frame crop back to zeros', () => { | ||
| const { modelValue, cropBounds } = createModel({ | ||
| crop: { x: 10, y: 10, width: 100, height: 100 } | ||
| }) | ||
|
|
||
| cropBounds.value = { x: 0, y: 0, width: 1920, height: 1080 } | ||
|
|
||
| expect(modelValue.value.crop).toEqual({ | ||
| x: 0, | ||
| y: 0, | ||
| width: 0, | ||
| height: 0 | ||
| }) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Missing regression test for out-of-bounds cropBounds clamping.
No test asserts that an out-of-bounds cropBounds write (negative x/y, or oversized width/height) gets clamped — mirroring the unresolved clamping gap in useVideoEditModel.ts (see that file's review comment).
🤖 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/composables/video/useVideoEditModel.test.ts` around lines 108 - 147, The
crop bounds tests in the “crop bounds” describe block lack coverage for
out-of-bounds writes. Add a regression test through createModel that assigns
negative x/y and oversized width/height to cropBounds, then assert
modelValue.crop is clamped to the valid video-frame bounds.
jtydhr88
force-pushed
the
feat/trim-video-rich-widget
branch
from
July 27, 2026 15:32
d61ae17 to
b366170
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/composables/video/useTrimPlayback.ts (1)
40-50: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
waitForVideoSeekstill has no timeout —isSeekingcan wedge permanently.The previous round marked this as addressed, but the current code has no timer: if neither
seekednorerrorfires (stalled network, source revoked mid-seek), the promise never settles andisSeekingstaystrue, permanently disablinghandleTimeUpdate(Line 113) — trim sync and auto-stop-at-end stop working until reload.🛡️ Proposed fix
+const SEEK_TIMEOUT_MS = 5000 + function waitForVideoSeek(video: HTMLVideoElement): Promise<void> { return new Promise((resolve) => { - const finish = () => { + function finish() { + clearTimeout(timer) video.removeEventListener('seeked', finish) video.removeEventListener('error', finish) resolve() } + const timer = setTimeout(finish, SEEK_TIMEOUT_MS) video.addEventListener('seeked', finish, { once: true }) video.addEventListener('error', finish, { once: true }) }) }🤖 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/composables/video/useTrimPlayback.ts` around lines 40 - 50, Update waitForVideoSeek to include a bounded timeout fallback that resolves the promise when neither the seeked nor error event fires. Ensure the timeout is cleared and both event listeners are removed when finish runs, preserving normal event-driven completion while preventing isSeeking from remaining stuck.src/composables/video/useVideoEditModel.ts (1)
87-113: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCrop geometry is never validated against the source frame. Nothing in the crop path enforces that a rectangle stays within
[0, sourceWidth] × [0, sourceHeight]with at leastMIN_CROP_SIZEextent, so invalid boxes enter the model and then corrupt the resize math downstream.
src/composables/video/useVideoEditModel.ts#L87-L113: clampnext.x/yand the resulting right/bottom edges againstwidth.value/height.valuein thecropBoundssetter before storing, preserving the existing full-frame reset branch.src/composables/video/useCropBoxEditor.ts#L29-L50: floor thew/nclamp upper bounds withMath.max(x2 - MIN_CROP_SIZE, 0)/Math.max(y2 - MIN_CROP_SIZE, 0)so a sub-minimum incoming box can't invert the clamp range.src/composables/video/useVideoEditModel.test.ts#L108-L147: add a regression test writing negativexand oversizedwidthtocropBoundsand assert the stored crop is clamped.🤖 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/composables/video/useVideoEditModel.ts` around lines 87 - 113, Validate crop geometry before storing it in cropBounds: clamp x/y and the resulting right/bottom edges to the source frame while preserving the existing full-frame reset branch. In src/composables/video/useCropBoxEditor.ts lines 29-50, bound the w/n clamp maxima with Math.max(x2 - MIN_CROP_SIZE, 0) and Math.max(y2 - MIN_CROP_SIZE, 0). In src/composables/video/useVideoEditModel.test.ts lines 108-147, add a regression test that writes negative x and oversized width through cropBounds and verifies the stored crop is clamped.
🤖 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/components/videoEdit/VideoFilmstripTrim.test.ts`:
- Around line 292-304: Add a test alongside “hides trim handles when disabled”
that renders the filmstrip with disabled true, triggers the track pointer
interaction used by startScrubDrag, and verifies scrubbing does not begin or
update the playhead. Reuse the existing test helpers and observable
callbacks/state assertions to cover the isDisabled() guard without changing
production behavior.
In `@src/composables/video/useVideoEditFormats.test.ts`:
- Around line 35-43: Add exact boundary assertions to the “picks the unit by
magnitude” test around formatFileSize: verify 1024 selects kilobytes and 1024 *
1024 selects megabytes, preserving the expected localized output values.
In `@src/composables/video/useVideoFilmstrip.test.ts`:
- Around line 46-56: Update the mock addEventListener implementation in the test
fixture to recognize the options-object form used by waitForEvent, specifically
{ once: true }, while retaining support for the boolean form if needed. Ensure
the one-time listener wrapper is installed and removes itself after the first
event so waitForEvent tests exercise the same once semantics as production.
In `@src/composables/video/useVideoFilmstrip.ts`:
- Around line 79-89: Move the video.currentTime = target assignment into the
!alreadyAtTarget branch alongside waitForEvent in the seeking loop. Preserve the
existing timeout handling and skip both assignment and waiting when the video is
already at the requested target.
In `@src/utils/videoMetadataUtil.ts`:
- Around line 47-60: Update fetchVideoMetadata to pass a short
AbortSignal.timeout(...) through the fetchApi options for the /video_metadata
request, ensuring stuck requests are cancelled and the existing undefined
fallback remains intact.
---
Duplicate comments:
In `@src/composables/video/useTrimPlayback.ts`:
- Around line 40-50: Update waitForVideoSeek to include a bounded timeout
fallback that resolves the promise when neither the seeked nor error event
fires. Ensure the timeout is cleared and both event listeners are removed when
finish runs, preserving normal event-driven completion while preventing
isSeeking from remaining stuck.
In `@src/composables/video/useVideoEditModel.ts`:
- Around line 87-113: Validate crop geometry before storing it in cropBounds:
clamp x/y and the resulting right/bottom edges to the source frame while
preserving the existing full-frame reset branch. In
src/composables/video/useCropBoxEditor.ts lines 29-50, bound the w/n clamp
maxima with Math.max(x2 - MIN_CROP_SIZE, 0) and Math.max(y2 - MIN_CROP_SIZE, 0).
In src/composables/video/useVideoEditModel.test.ts lines 108-147, add a
regression test that writes negative x and oversized width through cropBounds
and verifies the stored crop is clamped.
🪄 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: 877e84b8-0c59-41ee-a5c5-308ea163edbc
📒 Files selected for processing (39)
packages/design-system/src/css/style.csssrc/components/videoEdit/VideoCropOverlay.test.tssrc/components/videoEdit/VideoCropOverlay.vuesrc/components/videoEdit/VideoEditPanel.test.tssrc/components/videoEdit/VideoEditPanel.vuesrc/components/videoEdit/VideoFilmstripTrim.test.tssrc/components/videoEdit/VideoFilmstripTrim.vuesrc/components/videoEdit/WidgetVideoEdit.test.tssrc/components/videoEdit/WidgetVideoEdit.vuesrc/composables/video/useCropBoxEditor.test.tssrc/composables/video/useCropBoxEditor.tssrc/composables/video/useCropRatioLock.test.tssrc/composables/video/useCropRatioLock.tssrc/composables/video/useTimelineScrub.tssrc/composables/video/useTrimPlayback.test.tssrc/composables/video/useTrimPlayback.tssrc/composables/video/useVideoEditFormats.test.tssrc/composables/video/useVideoEditFormats.tssrc/composables/video/useVideoEditModel.test.tssrc/composables/video/useVideoEditModel.tssrc/composables/video/useVideoFilmstrip.test.tssrc/composables/video/useVideoFilmstrip.tssrc/composables/video/useVideoSourceUrl.test.tssrc/composables/video/useVideoSourceUrl.tssrc/lib/litegraph/src/types/widgets.tssrc/lib/litegraph/src/widgets/VideoEditWidget.tssrc/lib/litegraph/src/widgets/widgetMap.tssrc/locales/en/main.jsonsrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/widgets/components/layout/index.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoEditWidget.tssrc/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.tssrc/schemas/nodeDef/nodeDefSchemaV2.tssrc/scripts/widgets.tssrc/utils/videoFrameUtil.test.tssrc/utils/videoFrameUtil.tssrc/utils/videoMetadataUtil.test.tssrc/utils/videoMetadataUtil.ts
Comment on lines
+292
to
+304
| it('hides trim handles when disabled', () => { | ||
| renderFilmstrip({ | ||
| totalFrames: 100, | ||
| thumbnails: ['data:image/jpeg;base64,one'], | ||
| startFrame: 10, | ||
| endFrame: 80, | ||
| playheadFrame: 10, | ||
| disabled: true | ||
| }) | ||
|
|
||
| expect(screen.queryByTestId('handle-start')).toBeNull() | ||
| expect(screen.queryByTestId('handle-end')).toBeNull() | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test for scrub/drag being blocked while disabled.
disabled is verified to hide the trim handles, but no test confirms that pointer scrubbing on the track itself (startScrubDrag → isDisabled()) is a no-op when disabled is true — this is real behavior gating a runtime path.
As per path instructions, .agents/checks/test-quality.md calls to review changed tests for "missing edge-case coverage (error/disabled/empty states)".
🤖 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/components/videoEdit/VideoFilmstripTrim.test.ts` around lines 292 - 304,
Add a test alongside “hides trim handles when disabled” that renders the
filmstrip with disabled true, triggers the track pointer interaction used by
startScrubDrag, and verifies scrubbing does not begin or update the playhead.
Reuse the existing test helpers and observable callbacks/state assertions to
cover the isDisabled() guard without changing production behavior.
Source: Path instructions
Comment on lines
+46
to
+56
| addEventListener(type: string, listener: VideoListener, options?: boolean) { | ||
| if (options === true) { | ||
| const wrapped = (event: Event) => { | ||
| this.removeEventListener(type, wrapped) | ||
| listener(event) | ||
| } | ||
| this.getListeners(type).add(wrapped) | ||
| return | ||
| } | ||
| this.getListeners(type).add(listener) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Mock never emulates once because production passes an options object, not a boolean.
waitForEvent in useVideoFilmstrip.ts (Lines 44-45) calls addEventListener(name, fn, { once: true }). The options === true branch here only matches the legacy boolean capture form, so the auto-removal wrapper is dead code and once semantics go untested — a regression that dropped cleanup() would still pass.
♻️ Proposed fix
- addEventListener(type: string, listener: VideoListener, options?: boolean) {
- if (options === true) {
+ addEventListener(
+ type: string,
+ listener: VideoListener,
+ options?: boolean | AddEventListenerOptions
+ ) {
+ if (options === true || (typeof options === 'object' && options.once)) {As per path instructions, .agents/checks/test-quality.md asks that assertions exercise real behavior rather than wiring that can never run.
📝 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
| addEventListener(type: string, listener: VideoListener, options?: boolean) { | |
| if (options === true) { | |
| const wrapped = (event: Event) => { | |
| this.removeEventListener(type, wrapped) | |
| listener(event) | |
| } | |
| this.getListeners(type).add(wrapped) | |
| return | |
| } | |
| this.getListeners(type).add(listener) | |
| } | |
| addEventListener( | |
| type: string, | |
| listener: VideoListener, | |
| options?: boolean | AddEventListenerOptions | |
| ) { | |
| if (options === true || (typeof options === 'object' && options.once)) { | |
| const wrapped = (event: Event) => { | |
| this.removeEventListener(type, wrapped) | |
| listener(event) | |
| } | |
| this.getListeners(type).add(wrapped) | |
| return | |
| } | |
| this.getListeners(type).add(listener) | |
| } |
🤖 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/composables/video/useVideoFilmstrip.test.ts` around lines 46 - 56, Update
the mock addEventListener implementation in the test fixture to recognize the
options-object form used by waitForEvent, specifically { once: true }, while
retaining support for the boolean form if needed. Ensure the one-time listener
wrapper is installed and removes itself after the first event so waitForEvent
tests exercise the same once semantics as production.
Source: Path instructions
Comment on lines
+79
to
+89
| const alreadyAtTarget = | ||
| Math.abs(video.currentTime - target) < 0.001 && video.readyState >= 2 | ||
| video.currentTime = target | ||
| if (!alreadyAtTarget) { | ||
| try { | ||
| await waitForEvent(video, 'seeked', SEEK_EVENT_TIMEOUT_MS) | ||
| } catch (waitError) { | ||
| if (waitError instanceof EventTimeoutError) continue | ||
| throw waitError | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline src/composables/video/useVideoFilmstrip.ts --view expanded || true
echo "== relevant file lines =="
cat -n src/composables/video/useVideoFilmstrip.ts | sed -n '1,140p'
echo "== search waitForEvent usages =="
rg -n "function waitForEvent|const waitForEvent|export .*waitForEvent|waitForEvent\(" -S .
echo "== deterministic browser-like event queue probe =="
node - <<'JS'
console.log("Node cannot run browser seeking algorithms; inspecting source call graph is sufficient.")
JSRepository: Comfy-Org/ComfyUI_frontend
Length of output: 8787
🌐 Web query:
HTML spec seeked event seeked event firing algorithm currentTime attribute firing time seeked
💡 Result:
The seeked event is a non-cancelable, non-bubbling event fired by an HTMLMediaElement when a seek operation has completed [1]. This event signifies that the media element has finished moving to a new position, the current playback position has been updated, and the seeking IDL attribute has been set to false [1][2]. When a user or script sets the currentTime attribute of a media element, it triggers the media element's seek algorithm [3][4]. During this process: 1. The seeking IDL attribute is set to true, and a seeking event is fired [5][6]. 2. The user agent performs the necessary operations to update the media's playback position to the requested time [3]. 3. Once the seek operation is complete, the seeking IDL attribute is set back to false [1]. 4. The seeked event is then fired to notify the document that the transition to the new playback position is finished [1]. The seeked event is intended to be used as a confirmation that the media has successfully jumped to the specified time and that the currentTime attribute reflects the new, stable position [1]. Because the algorithm is complex and involves asynchronous tasks, the seeking and seeked events serve as the primary mechanism for developers to track the lifecycle of a seek request [2].
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/seeked_event
- 2: https://lists.whatwg.org/pipermail/whatwg-whatwg.org/2013-December/041733.html
- 3: https://html.spec.whatwg.org/dev/media.html
- 4: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement
- 5: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/seeking_event
- 6: https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement/seeking
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== loadVideo remainder =="
cat -n src/composables/video/useVideoFilmstrip.ts | sed -n '140,185p'
echo "== sample loop behavior with stale flag sample edge =="
python3 - <<'PY'
def sample_times(sample_count, duration):
for index in range(sample_count):
time = 0 if sample_count <= 1 else (duration * index) / (sample_count - 1)
target = min(time, max(duration - 0.001, 0))
yield index, target, time
for sample_count in [2, 10, 20]:
print(f"sample_count={sample_count}")
for index, target, time in sample_times(sample_count, 60.0):
print(index, round(target, 3), round(time, 3), round(abs(target - time), 6))
PYRepository: Comfy-Org/ComfyUI_frontend
Length of output: 2628
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== HTMLMediaElement currentTime currentTime setter spec =="
python3 - <<'PY'
spec_url = "https://html.spec.whatwg.org/dev/media.html#dom-htmlmediaelement-currenttime-setter"
print(spec_url)
PY
echo "== inspect whether a runtime DOM/video model exists in package =="
fd -a 'video|media|html' . | head -80Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5882
Move video.currentTime = target under !alreadyAtTarget.
When alreadyAtTarget is true, the loop still assigns currentTime, which can queue another seeking algorithm and deliver a seeked event during the next waitForEvent. That event can let captureFrame run before the requested frame is rendered, causing repeated filmstrip frames.
🐛 Proposed fix
- video.currentTime = target
- if (!alreadyAtTarget) {
+ if (!alreadyAtTarget) {
+ video.currentTime = target
try {
await waitForEvent(video, 'seeked', SEEK_EVENT_TIMEOUT_MS)
} catch (waitError) {
if (waitError instanceof EventTimeoutError) continue
throw waitError
}
}📝 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 alreadyAtTarget = | |
| Math.abs(video.currentTime - target) < 0.001 && video.readyState >= 2 | |
| video.currentTime = target | |
| if (!alreadyAtTarget) { | |
| try { | |
| await waitForEvent(video, 'seeked', SEEK_EVENT_TIMEOUT_MS) | |
| } catch (waitError) { | |
| if (waitError instanceof EventTimeoutError) continue | |
| throw waitError | |
| } | |
| } | |
| if (!alreadyAtTarget) { | |
| video.currentTime = target | |
| try { | |
| await waitForEvent(video, 'seeked', SEEK_EVENT_TIMEOUT_MS) | |
| } catch (waitError) { | |
| if (waitError instanceof EventTimeoutError) continue | |
| throw waitError | |
| } | |
| } |
🤖 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/composables/video/useVideoFilmstrip.ts` around lines 79 - 89, Move the
video.currentTime = target assignment into the !alreadyAtTarget branch alongside
waitForEvent in the seeking loop. Preserve the existing timeout handling and
skip both assignment and waiting when the video is already at the requested
target.
Comment on lines
+47
to
+60
| export async function fetchVideoMetadata( | ||
| videoUrl: string | ||
| ): Promise<VideoMetadata | undefined> { | ||
| const query = viewQueryFromUrl(videoUrl) | ||
| if (!query) return undefined | ||
|
|
||
| try { | ||
| const response = await api.fetchApi(`/video_metadata?${query}`) | ||
| if (!response.ok) return undefined | ||
| return zVideoMetadata.parse(await response.json()) | ||
| } catch { | ||
| return undefined | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'fetchApi($$$) { $$$ }' --lang typescript src/scripts/api.ts
rg -nP -C5 '\bfetchApi\s*\(' src/scripts/api.ts | head -60Repository: Comfy-Org/ComfyUI_frontend
Length of output: 2335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Candidate files =="
fd -a 'videoMetadataUtil\.ts$|api\.ts$' src | sed 's#^\./##'
echo "== videoMetadataUtil outline =="
FILE="$(fd 'videoMetadataUtil\.ts$' src | head -1)"
if [ -n "${FILE:-}" ]; then
ast-grep outline "$FILE" || true
echo "== videoMetadataUtil contents =="
cat -n "$FILE"
fi
echo "== api.ts fetchApi relevant section =="
API="$(fd '^api\.ts$' src/scripts | head -1)"
if [ -n "${API:-}" ]; then
sed -n '430,500p' "$API" | cat -n -v "$API" | sed -n '430,500p'
fi
echo "== fetchApi implementations and request usage =="
rg -n 'async fetchApi|fetchApi\(|signal:' src/scripts/api.ts src/utils/videoMetadataUtil.ts src | head -120Repository: Comfy-Org/ComfyUI_frontend
Length of output: 5460
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate useVideoFilmstrip =="
rg -n "useVideoFilmstrip|loadVideo|METADATA_EVENT_TIMEOUT_MS|SEEK_EVENT_TIMEOUT_MS" src --glob '*.{ts,vue}' | head -160
echo "== candidate video filmstrip files =="
fd 'video|filmstrip' src --type f | sed 's#^\./##' | head -80
echo "== api.ts fetchWithUnifiedRemint =="
rg -n "fetchWithUnifiedRemint|return fetch|new Request" src/scripts/api.ts | head -80
sed -n '1,120p' src/scripts/api.ts | cat -n
sed -n '420,500p' src/scripts/api.ts | cat -nRepository: Comfy-Org/ComfyUI_frontend
Length of output: 12767
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== unified remint files =="
fd -a 'remintRetry\.ts$' src | sed 's#^\./##'
FILE="$(fd 'remintRetry\.ts$' src | head -1)"
if [ -n "${FILE:-}" ]; then
wc -l "$FILE"
cat -n "$FILE"
fi
echo "== useVideoFilmstrip relevant sections =="
FILE="$(fd '^useVideoFilmstrip\.ts$' src/composables/video | head -1)"
if [ -n "${FILE:-}" ]; then
sed -n '1,135p' "$FILE" | cat -n
fi
echo "== fetchVideoMetadata usages and tests =="
rg -n "fetchVideoMetadata|videoMetadata" src --glob '*.{ts,vue}' | head -120
echo "== behavioral probe: request options forward signal (static ReadRequestInit and RequestInit semantics only) =="
python3 - <<'PY'
from pathlib import Path
p=Path('src/scripts/api.ts')
src=p.read_text()
start=src.find('async fetchApi')
end=src.find(' /**', start+1)
print(src[start:end])
PYRepository: Comfy-Org/ComfyUI_frontend
Length of output: 49914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== loadVideo continuation =="
FILE="$(fd '^useVideoFilmstrip\.ts$' src/composables/video | head -1)"
if [ -n "${FILE:-}" ]; then
sed -n '129,215p' "$FILE" | cat -n
fiRepository: Comfy-Org/ComfyUI_frontend
Length of output: 3082
Add a short AbortSignal timeout to the metadata fetch.
fetchApi passes RequestInit through to fetchWithUnifiedRemint, which calls fetch(input, init), so AbortSignal.timeout(...) can cancel a stuck /video_metadata request. A hung metadata fetch currently blocks loadVideo after loadedmetadata before the fallback element-derived metadata path is reached.
🛡️ Proposed fix
+const METADATA_REQUEST_TIMEOUT_MS = 10_000
+
export async function fetchVideoMetadata(
videoUrl: string
): Promise<VideoMetadata | undefined> {
const query = viewQueryFromUrl(videoUrl)
if (!query) return undefined
try {
- const response = await api.fetchApi(`/video_metadata?${query}`)
+ const response = await api.fetchApi(`/video_metadata?${query}`, {
+ signal: AbortSignal.timeout(METADATA_REQUEST_TIMEOUT_MS)
+ })
if (!response.ok) return undefined
return zVideoMetadata.parse(await response.json())
} catch {
return undefined
}
}📝 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
| export async function fetchVideoMetadata( | |
| videoUrl: string | |
| ): Promise<VideoMetadata | undefined> { | |
| const query = viewQueryFromUrl(videoUrl) | |
| if (!query) return undefined | |
| try { | |
| const response = await api.fetchApi(`/video_metadata?${query}`) | |
| if (!response.ok) return undefined | |
| return zVideoMetadata.parse(await response.json()) | |
| } catch { | |
| return undefined | |
| } | |
| } | |
| const METADATA_REQUEST_TIMEOUT_MS = 10_000 | |
| export async function fetchVideoMetadata( | |
| videoUrl: string | |
| ): Promise<VideoMetadata | undefined> { | |
| const query = viewQueryFromUrl(videoUrl) | |
| if (!query) return undefined | |
| try { | |
| const response = await api.fetchApi(`/video_metadata?${query}`, { | |
| signal: AbortSignal.timeout(METADATA_REQUEST_TIMEOUT_MS) | |
| }) | |
| if (!response.ok) return undefined | |
| return zVideoMetadata.parse(await response.json()) | |
| } catch { | |
| return undefined | |
| } | |
| } |
🤖 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/videoMetadataUtil.ts` around lines 47 - 60, Update
fetchVideoMetadata to pass a short AbortSignal.timeout(...) through the fetchApi
options for the /video_metadata request, ensuring stuck requests are cancelled
and the existing undefined fallback remains intact.
dante01yoon
pushed a commit
that referenced
this pull request
Jul 27, 2026
#14120) This is seperate part for the big PR of video edit, crop, trim PR #14118 ## Summary - Optional contentInsetX maps pointer positions onto a track whose value range is horizontally inset (e.g. tracks with handle gutters) - Expose activeHandle so consumers can render per-handle UI while dragging
jtydhr88
added a commit
that referenced
this pull request
Jul 27, 2026
This is seperate part for the big PR of video edit, crop, trim PR #14118 ## Summary - useVideoSourceUrl resolves a playable url for the video a node operates on: the linked upstream node (with subgraph passthrough) or the node itself, preferring executed output previews over the file widget; keyed store subscription, rand cache-buster stripped - useVideoFilmstrip samples thumbnail strips off a hidden video element with seek timeouts and stale-load bail-out - videoMetadataUtil fetches exact fps/frame count/duration/size from the backend /video_metadata endpoint (falls back gracefully when absent); videoFrameUtil holds frame/time conversion helpers
jtydhr88
force-pushed
the
feat/trim-video-rich-widget
branch
from
July 27, 2026 17:44
b366170 to
9927fe8
Compare
jtydhr88
added a commit
that referenced
this pull request
Jul 27, 2026
This is seperate part for the big PR of video edit, crop, trim PR #14118 ## Summary - useCropBoxEditor: drag/move and eight-direction resize of a crop rect in source-pixel space, clamped to the frame with a minimum size; optional locked aspect ratio anchored on the opposite edge/corner - useCropRatioLock: aspect-ratio presets and lock state (same presets as the image crop widget), reshaping the bounds on selection - VideoCropOverlay: percent-positioned crop rectangle with handles rendered over a media preview, following the imagecrop visual language
jtydhr88
force-pushed
the
feat/trim-video-rich-widget
branch
from
July 27, 2026 18:50
9927fe8 to
3fce8da
Compare
jtydhr88
force-pushed
the
feat/trim-video-rich-widget
branch
3 times, most recently
from
July 28, 2026 02:31
feecf3b to
c3b1e0d
Compare
jtydhr88
force-pushed
the
feat/trim-video-rich-widget
branch
from
July 28, 2026 02:53
c3b1e0d to
ba1ad30
Compare
jtydhr88
added a commit
that referenced
this pull request
Jul 28, 2026
This is seperate part for the big PR of video edit, crop, trim PR #14118 ## Summary Timeline building blocks for the upcoming VIDEO_EDIT rich widget: - VideoFilmstripTrim: filmstrip strip with trim range handles (useRangeEditor), scrub playhead, and selected-range shading - useTimelineScrub: pointer scrubbing on the filmstrip track mapped to frame positions - useTrimPlayback: play/pause within the trimmed range, restarting from the trim start when playback reaches the trim end - useVideoEditFormats: duration and file-size display formatting - design tokens for the filmstrip/timeline surfaces
jtydhr88
added a commit
that referenced
this pull request
Jul 29, 2026
This is seperate last part for the big PR of video edit, crop, trim PR #14118 ## Summary Wires the VIDEO_EDIT input type end to end and assembles the editor UI from the previously merged building blocks: - litegraph: VideoEditValue/VideoEditTrim widget value types, VideoEditWidget class and widgetMap/constructor registration - schema/registry: VIDEO_EDIT zod spec in nodeDefSchemaV2, widget registry entry rendering WidgetVideoEdit - useVideoEditModel: canonical seconds/pixels edit state with frame-based setters, enable toggles, and handle crossover clamps - VideoEditPanel: trim timeline (filmstrip + range handles + playback) and crop overlay with ratio lock, driven by backend video metadata - WidgetVideoEdit: widget shell resolving the source video via useVideoSourceUrl and suppressing the default node media preview
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
For testing and feedback. open sepearte PRs to break it into small parts:
Separate PRs of wave1:
Separate PRs of wave2:
TODO
Summary
BE is Comfy-Org/ComfyUI#15090
Screenshots (if applicable)