Load video trim node (Design Prototype)#13293
Draft
uytieu wants to merge 12 commits into
Draft
Conversation
Add utilities to detect MP4 average frame-rate and HTTP resource byte size and wire them into the video filmstrip loading flow. Files added: probeVideoFrameRate.ts (+test) and httpResourceByteSize.ts (+test). useVideoFilmstrip now probes frame-rate and fetches file size, exposes fps and fileSize refs, and uses those values when computing totalFrames. Removed redundant fileSize fetching from useLoadVideoPreview and updated components/stories/tests to consume the filmstrip-provided fileSize. Also fix fps usage to read ref.value and add a small layout spacing tweak (mt-2). Tests added/updated for the new behavior.
Contributor
📝 WalkthroughWalkthroughAdds LoadVideo trim support across MP4 probing, preview derivation, filmstrip generation, trim controls, widget registration, and LoadVideo node layout updates. ChangesLoadVideo video trim feature
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 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 |
uytieu
added
the
preview-cpu
🌐 Website E2ETip All tests passed.
🔗 Website PreviewWebsite Preview: https://comfy-website-preview-pr-13293.vercel.app This commit: https://website-frontend-cjyqo2qps-comfyui.vercel.app Last updated: 2026-06-30T09:03:41Z for |
🎭 Playwright: ❌ 1617 passed, 36 failed · 2 flaky❌ Failed Tests📊 Browser Reports
🎨 Storybook: ✅ Built — View Storybook📦 Bundle: 7.79 MB gzip 🔴 +20.9 kBDetailsSummary
Category Glance App Entry Points — 47.4 kB (baseline 47.4 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.25 MB (baseline 1.25 MB) • 🔴 +1.05 kBGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 98.1 kB (baseline 97.7 kB) • 🔴 +419 BTop-level views, pages, and routed surfaces
Status: 12 added / 12 removed Panels & Settings — 546 kB (baseline 546 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 24 added / 24 removed / 3 unchanged User & Accounts — 26.9 kB (baseline 26.9 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 8 added / 8 removed / 2 unchanged Editors & Dialogs — 120 kB (baseline 117 kB) • 🔴 +2.9 kBModals, dialogs, drawers, and in-app editors
Status: 6 added / 5 removed UI Components — 63 kB (baseline 57.2 kB) • 🔴 +5.81 kBReusable component library chunks
Status: 12 added / 11 removed / 2 unchanged Data & Services — 270 kB (baseline 270 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 14 added / 14 removed / 2 unchanged Utilities & Hooks — 3.37 MB (baseline 3.37 MB) • 🔴 +4.28 kBHelpers, composables, and utility bundles
Status: 21 added / 21 removed / 12 unchanged Vendor & Third-Party — 15.3 MB (baseline 15.3 MB) • 🔴 +24.6 kBExternal libraries and shared vendor chunks
Status: 2 added / 2 removed / 14 unchanged Other — 11.8 MB (baseline 11.7 MB) • 🔴 +52.6 kBBundles that do not match a named category
Status: 141 added / 139 removed / 26 unchanged ⚡ Performance
|
![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: 21
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/extensions/vueNodes/components/LGraphNode.vue (1)
496-513: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep external width updates enabled in shrink-wrap mode.
Line 505 exits before
--node-widthis refreshed, so Canvas/External size changes stop affecting LoadVideo nodes whenever preview shrink-wrap is active.Suggested fix
function handleLayoutChange(change: LayoutChange) { // Only handle Canvas or External source (extensions calling setSize) if ( change.source !== LayoutSource.Canvas && change.source !== LayoutSource.External ) return if (layoutStore.isResizingVueNodes.value) return if (isCollapsed.value) return - if (loadVideoShrinkWrapBody.value) return const el = nodeContainerRef.value if (!el) return const newSize = size.value - const fullHeight = newSize.height + LiteGraph.NODE_TITLE_HEIGHT el.style.setProperty('--node-width', `${newSize.width}px`) - el.style.setProperty('--node-height', `${fullHeight}px`) + if (loadVideoShrinkWrapBody.value) { + el.style.removeProperty('--node-height') + return + } + + const fullHeight = newSize.height + LiteGraph.NODE_TITLE_HEIGHT + el.style.setProperty('--node-height', `${fullHeight}px`) }🤖 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/components/LGraphNode.vue` around lines 496 - 513, The shrink-wrap guard in handleLayoutChange is preventing External and Canvas width updates from reaching LoadVideo nodes, so adjust the early-return conditions to allow width refreshes while still preserving the body-height behavior for shrink-wrap. Keep the existing source checks and the isCollapsed/layoutStore guards, but move or narrow the loadVideoShrinkWrapBody check so --node-width is still updated from size.value in LGraphNode.vue when setSize or Canvas changes arrive.
🤖 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 @.gitignore:
- Around line 99-101: The .gitignore rule using .env* is also hiding
.env.example, so explicitly exempt that template if the repo relies on it.
Update the ignore pattern set near the existing .amp and .vercel entries so
.env.example remains tracked while other env files stay ignored, preserving the
documented template for developers.
In `@src/components/ui/tooltip/TooltipProvider.vue`:
- Around line 5-10: The TooltipProvider wrapper is using defineProps as a whole
object instead of the repo’s reactive props destructuring pattern. Update the
TooltipProvider component to destructure the props with a rest binding in the
script block, then pass that rest object through the TooltipProvider wrapper so
it matches the other UI wrappers and the Reka UI guideline.
In `@src/components/ui/tooltip/TooltipTrigger.vue`:
- Around line 5-10: The TooltipTrigger wrapper is binding the raw defineProps()
object instead of following the reactive props destructuring pattern used by
other src/components/ui wrappers. Update TooltipTrigger.vue to destructure
TooltipTriggerProps with a class alias and rest props, then pass the rest object
into the TooltipTrigger component so it matches the Reka UI wrapper convention.
In `@src/components/video/LoadVideoTrimPanel.vue`:
- Around line 444-451: The frame calculation in handleTimeUpdate() is
inconsistent with the seeking logic because it maps currentTime back to frames
using fps instead of the inverse of frameToTime(). Update handleTimeUpdate() to
derive the playhead frame from the same duration/frameMax-based mapping used by
frameToTime(), so scrubbing and playback stay aligned even when duration * fps
does not match frameMax. Keep the existing clamp logic, but make the frame
conversion consistent with the functions and state in LoadVideoTrimPanel.vue
such as frameToTime(), frameMax, and playheadFrame.
- Around line 22-23: The remove action in LoadVideoTrimPanel is only exposed
through isVideoHovered, which depends on mouseenter/mouseleave and makes the
control unreachable by keyboard. Update the remove control so it can be focused
and activated from the keyboard as well, and do not rely solely on hover state
to render it; use the same approach for the related remove UI around the other
affected block as well.
- Around line 471-480: The metadata labels in formatDuration and formatFileSize
are hardcoded user-facing strings and should be moved to vue-i18n. Update these
helpers to read localized values instead of returning raw literals like 0s, B,
KB, MB, and —, and add the needed translation entries in
src/locales/en/main.json so the LoadVideoTrimPanel stays consistent with the
repo’s localization rules.
- Around line 347-363: The playback toggle flow in handlePlaybackChange ignores
the promise returned by video.play(), so a rejected play attempt can leave
isPlaying stuck true while the video stays paused. Update the isPlaying
watcher/handlePlaybackChange logic to await or catch video.play() and, on
failure, roll back isPlaying to false and propagate or surface the error
appropriately. Use the existing symbols watch(isPlaying), handlePlaybackChange,
and videoRef to locate the change.
In `@src/components/video/MediaUploadEmpty.test.ts`:
- Around line 25-34: The renderEmpty test helper is typed too loosely with
Record<string, unknown>, which bypasses prop validation for MediaUploadEmpty.
Update the props parameter to use Partial<ComponentProps<typeof
MediaUploadEmpty>> in renderEmpty so test inputs stay type-checked while still
allowing overrides, and keep the helper’s default accept prop merge behavior
intact.
In `@src/components/video/MediaUploadEmpty.vue`:
- Around line 54-55: The drag-drop handler in MediaUploadEmpty.vue is dropping
the promise from onDragDrop, which can cause unhandled rejections. Update the
event path around onDragDrop to properly await or return the promise and add
error handling so failures are propagated or surfaced instead of being ignored.
Use the onDragDrop callback and the drag/drop event handler in
MediaUploadEmpty.vue to locate the fix.
In `@src/composables/video/probeVideoFrameRate.ts`:
- Around line 96-107: Update probeVideoFrameRate to handle both mdhd version 0
and version 1 before reading timescale and duration. The current logic in the
mdhd parsing block assumes fixed offsets, so adjust the offsets based on the
mdhd version (read from the box header) and use the 64-bit duration layout for
version 1 while keeping the existing version 0 path. Keep the sampleSizes/stsz
handling unchanged and make sure the track duration calculation still falls back
correctly when the box is unsupported.
- Around line 143-147: The fetchRange helper in probeVideoFrameRate currently
accepts any 200 OK response and buffers the full video, which can defeat the
range probe. Update fetchRange to only treat a partial-content response as valid
by requiring the expected range status from the response object and returning
undefined for full-body responses. Keep the logic centered around the fetch call
and response status check so the leading/trailing probes never download the
entire asset.
In `@src/composables/video/useVideoFilmstrip.ts`:
- Around line 123-126: The video load flow is probing the same remote resource
size twice because `useVideoFilmstrip` calls both `probeVideoFrameRate()` and
`fetchHttpResourceByteSize()` in the same `Promise.all`, while
`probeVideoFrameRate()` already performs the byte-size fetch internally. Update
the `useVideoFilmstrip` load path to avoid the duplicate HEAD/Range request by
either having `probeVideoFrameRate()` return both `frameRate` and `byteSize`, or
by fetching the size once in `useVideoFilmstrip` and passing it into
`probeVideoFrameRate()`. Keep the change localized around `probeVideoFrameRate`,
`fetchHttpResourceByteSize`, and the load/sampling logic in `useVideoFilmstrip`.
In `@src/renderer/extensions/vueNodes/composables/useProcessedWidgets.ts`:
- Around line 463-470: The `loadVideoHasPreview` computed in
`useProcessedWidgets` is missing a dependency on preview-image state, so it can
stay stale when `nodePreviewImages` changes without `nodeOutputs` changing.
Update the dependency tracking in this computed to also read from the
preview-image source used by `nodeHasLoadVideoPreview()` (alongside
`nodeOutputs` and the `file` widget) so `loadVideoTrimFillsSpace` recalculates
when previews arrive from cache.
- Around line 34-36: The imports in useProcessedWidgets currently duplicate
symbols from nodeIdentification, causing a duplicate identifier issue.
Consolidate createNodeExecutionId and NodeExecutionId into a single import from
`@/types/nodeIdentification`, and keep useNodeOutputStore separate so the module
has only one nodeIdentification import block.
In
`@src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.test.ts`:
- Around line 317-335: The WidgetSelectDropdown tests are asserting the
wrapper’s opacity class instead of the dropdown’s actual disabled behavior.
Update the affected cases in WidgetSelectDropdown.test.ts to check the control’s
disabled or aria-disabled state on the rendered button from
renderLoadVideoDropdown, rather than inspecting parentElement class names. Keep
the assertions focused on behavior for the load video dropdown state when
mockLoadVideoNode.isUploading is true and when the preview resolves from the
file widget fallback.
In
`@src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue`:
- Around line 27-30: The node lookup in WidgetSelectDropdown.vue is failing
type-checking because the nodeId prop is declared as a plain string while
getNodeById expects LiteGraph’s branded NodeId. Update the Props interface for
nodeId to use the branded NodeId at the boundary, or convert the prop to NodeId
immediately before the getNodeById call in the WidgetSelectDropdown component so
the types match cleanly.
In `@src/renderer/extensions/vueNodes/widgets/components/WidgetVideoTrim.vue`:
- Around line 28-38: The WidgetVideoTrim component is importing NodeId from a
module that does not export it, which breaks type-checking. Update the import in
WidgetVideoTrim.vue to use the correct NodeId source used elsewhere in the
codebase, and keep the defineProps typing for nodeId aligned with that symbol so
the component compiles cleanly.
In `@src/renderer/extensions/vueNodes/widgets/composables/useComboWidget.ts`:
- Around line 164-165: The LoadVideo guard in useComboWidget should run before
the specDefault lookup so it can preserve the empty-selection behavior in cloud
mode. Update the relevant branch in the widget selection flow to short-circuit
on nodeType === 'LoadVideo' before any schema default matching, and keep the
rest of the logic unchanged so LoadVideo never auto-selects a cloud asset from
specDefault.
In
`@src/renderer/extensions/vueNodes/widgets/composables/useImageUploadWidget.ts`:
- Around line 126-129: The image upload widget is stringifying the combo value
before validating it, so unset values can still be saved as literal strings like
undefined or null. Update the logic in useImageUploadWidget’s node output
handling to check the raw fileComboWidget.value first for missing/placeholder
states, and only then convert it with String(...) before calling
nodeOutputStore.setNodeOutputs.
In `@src/renderer/extensions/vueNodes/widgets/composables/useVideoTrimWidget.ts`:
- Around line 31-115: Document migration guidance for the new LoadVideo widget
surface introduced in useVideoTrimWidget, since adding trim, trim_enabled,
start_frame, and end_frame changes node.widgets names/order seen by downstream
extensions. Add a concise migration note near the change or in the relevant
changelog/docs explaining the new widget contract and any extension adjustments
needed, referencing useVideoTrimWidget and the LoadVideo node widget setup so
future maintainers can locate it.
In `@src/utils/httpResourceByteSize.ts`:
- Around line 36-43: In fetchHttpResourceByteSize(), the Range probe only parses
Content-Range, so a 200 OK response that ignores Range can still return
undefined even when Content-Length is available. Update the existing
range-handling branch to treat 200 as a valid fallback and extract the total
size from Content-Length when Content-Range is missing, while still keeping 206
parsing via parseContentRangeTotal(). This will let probeVideoFrameRate()
получить the byte size it needs for the trailing chunk path and avoid falling
back to the default FPS for tail-moov MP4s.
---
Outside diff comments:
In `@src/renderer/extensions/vueNodes/components/LGraphNode.vue`:
- Around line 496-513: The shrink-wrap guard in handleLayoutChange is preventing
External and Canvas width updates from reaching LoadVideo nodes, so adjust the
early-return conditions to allow width refreshes while still preserving the
body-height behavior for shrink-wrap. Keep the existing source checks and the
isCollapsed/layoutStore guards, but move or narrow the loadVideoShrinkWrapBody
check so --node-width is still updated from size.value in LGraphNode.vue when
setSize or Canvas changes arrive.
🪄 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: 1e6a22d3-3dfe-42ca-9c94-6dde553b0b9d
📒 Files selected for processing (49)
.gitignorepackages/design-system/src/css/style.csssrc/components/ui/tooltip/Tooltip.vuesrc/components/ui/tooltip/TooltipContent.vuesrc/components/ui/tooltip/TooltipHint.stories.tssrc/components/ui/tooltip/TooltipHint.vuesrc/components/ui/tooltip/TooltipProvider.vuesrc/components/ui/tooltip/TooltipTrigger.vuesrc/components/video/LoadVideoTrimPanel.stories.tssrc/components/video/LoadVideoTrimPanel.test.tssrc/components/video/LoadVideoTrimPanel.vuesrc/components/video/MediaUploadEmpty.stories.tssrc/components/video/MediaUploadEmpty.test.tssrc/components/video/MediaUploadEmpty.vuesrc/components/video/VideoFilmstripTrim.test.tssrc/components/video/VideoFilmstripTrim.vuesrc/components/video/timelineInsetStyle.tssrc/composables/useRangeEditor.test.tssrc/composables/useRangeEditor.tssrc/composables/video/probeVideoFrameRate.test.tssrc/composables/video/probeVideoFrameRate.tssrc/composables/video/useLoadVideoPreview.test.tssrc/composables/video/useLoadVideoPreview.tssrc/composables/video/useVideoFilmstrip.test.tssrc/composables/video/useVideoFilmstrip.tssrc/extensions/core/index.tssrc/extensions/core/loadVideoTrim.tssrc/lib/litegraph/src/types/widgets.tssrc/lib/litegraph/src/widgets/VideoTrimWidget.tssrc/lib/litegraph/src/widgets/widgetMap.tssrc/locales/en/main.jsonsrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/components/NodeWidgets.vuesrc/renderer/extensions/vueNodes/composables/useProcessedWidgets.tssrc/renderer/extensions/vueNodes/composables/useVueNodeResizeTracking.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelect.vuesrc/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vuesrc/renderer/extensions/vueNodes/widgets/components/WidgetVideoTrim.vuesrc/renderer/extensions/vueNodes/widgets/components/layout/index.tssrc/renderer/extensions/vueNodes/widgets/composables/useComboWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useComboWidget.tssrc/renderer/extensions/vueNodes/widgets/composables/useImageUploadWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useImageUploadWidget.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoTrimWidget.test.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoTrimWidget.tssrc/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.tssrc/utils/httpResourceByteSize.test.tssrc/utils/httpResourceByteSize.ts
Comment on lines
+99
to
+101
| .amp | ||
| .vercel | ||
| .env* |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider explicitly tracking .env.example if used as a template.
The .env* pattern also ignores .env.example. If your repository uses .env.example as a documented template for required environment variables, add an explicit exception:
.env*
+!.env.exampleThis is a common pattern to prevent accidental secret commits while keeping the template visible to developers.
📝 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
| .amp | |
| .vercel | |
| .env* | |
| .amp | |
| .vercel | |
| .env* | |
| !.env.example |
🤖 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 @.gitignore around lines 99 - 101, The .gitignore rule using .env* is also
hiding .env.example, so explicitly exempt that template if the repo relies on
it. Update the ignore pattern set near the existing .amp and .vercel entries so
.env.example remains tracked while other env files stay ignored, preserving the
documented template for developers.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/extensions/vueNodes/components/LGraphNode.vue (2)
754-768: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the collapsed height var when shrink-wrap turns on.
This watcher only removes
--node-height. If the preview appears while the node is collapsed,--node-height-xsurvives and the collapse watcher later copies that stale fixed height back on expand.♻️ Proposed fix
if (shrinkWrap) { el.style.removeProperty('--node-height') + el.style.removeProperty('--node-height-x') } else { initSizeStyles() }🤖 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/components/LGraphNode.vue` around lines 754 - 768, The shrink-wrap watcher in LGraphNode.vue only clears --node-height when loadVideoShrinkWrapBody turns on, leaving --node-height-x behind and causing stale collapsed height to be restored later. Update this watch(loadVideoShrinkWrapBody, ...) handler to clear the collapsed height variables together, using the existing nodeContainerRef and initSizeStyles flow so the expand/collapse logic stays consistent.
748-752: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTrack preview-cache updates in
loadVideoShrinkWrapBody.
nodeHasLoadVideoPreview()is also driven by preview-image cache state, but this computed only touchesnodeOutputs. When a cached preview arrives without a new output payload, shrink-wrap never recomputes and the node stays in the non-preview layout until some unrelated update happens.♻️ Proposed fix
const loadVideoShrinkWrapBody = computed(() => { if (nodeData.type !== 'LoadVideo') return false void nodeOutputs.nodeOutputs + void nodeOutputs.nodePreviewImages return nodeHasLoadVideoPreview(lgraphNode.value) })🤖 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/components/LGraphNode.vue` around lines 748 - 752, The loadVideoShrinkWrapBody computed only depends on nodeOutputs, so it misses preview-cache changes and can stay stale after a cached image arrives. Update the dependency tracking inside loadVideoShrinkWrapBody in LGraphNode.vue so it also touches the preview-cache state used by nodeHasLoadVideoPreview, ensuring the LoadVideo layout recomputes when preview data changes even without a new output payload.
♻️ Duplicate comments (2)
src/composables/video/useVideoFilmstrip.ts (1)
123-130:⚠️ Potential issue | 🟠 MajorThe size probe still runs twice when the first lookup returns
undefined.
detectedFileSizeis passed through here, butprobeVideoFrameRate()still resolvesbyteSize ?? fetchHttpResourceByteSize(url). Any asset that hides total-size headers will therefore pay the HEAD/Range flow twice before frame sampling starts. Pass an explicit sentinel for “already attempted but unknown”, or a separate flag, so this path can skip the second probe.🤖 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 123 - 130, The size probe is being repeated in the frame-rate path because `probeVideoFrameRate()` falls back to `fetchHttpResourceByteSize(url)` when `detectedFileSize` is `undefined`. Update the `useVideoFilmstrip` flow so the initial `fetchHttpResourceByteSize` attempt is treated as “already tried but unknown” and pass that state into `probeVideoFrameRate()` (or equivalent), allowing it to skip its internal re-probe when the size is still unavailable. Keep the fix localized around `detectedFileSize` and `probeVideoFrameRate()` so the second HEAD/Range lookup is avoided.src/components/video/MediaUploadEmpty.vue (1)
54-56: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDon't swallow
onDragDrop()failures.Line 55 converts a rejected drop handler into a silent no-op, so failed drops cannot be surfaced or recovered. Please propagate that failure through an error path instead of
.catch(() => {}). As per coding guidelines, "Implement proper error propagation" and "Disallow floating promises."🤖 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/video/MediaUploadEmpty.vue` around lines 54 - 56, The drag-and-drop handler in MediaUploadEmpty.vue is swallowing failures by turning a rejected onDragDrop() call into a silent no-op. Update the drop handling logic around the onDragDrop/event branch to propagate errors through the component’s existing error path instead of using a blank catch, while still addressing the floating promise by explicitly awaiting or otherwise handling the promise in a way that preserves rejection visibility. Use the onDragDrop handler site to locate the change.Source: Coding guidelines
🤖 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/video/LoadVideoTrimPanel.test.ts`:
- Around line 149-151: The test is querying the remove control by data-testid
even though it already has a stable accessible role and name. Update the
assertion setup in LoadVideoTrimPanel.test.ts to use the button’s accessible
query via getByRole with the name “Remove” instead of
getByTestId('video-remove-button'), so the keyboard interaction remains aligned
with how users access the control.
In `@src/composables/video/useVideoFilmstrip.ts`:
- Around line 123-131: Reset the previously derived video metadata at the start
of the load flow in useVideoFilmstrip before the sequential fetch/probe awaits
run, so stale values do not persist while a new URL is loading. Clear the refs
that hold duration, totalFrames, width, height, fps, and fileSize right before
this serial load path begins, then let the existing fetchHttpResourceByteSize
and probeVideoFrameRate logic repopulate them once the new asset finishes
probing.
In
`@src/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vue`:
- Line 18: The WidgetSelectDropdown component is importing NodeId from the wrong
module, which breaks the prop type in the component definition. Update the
import in WidgetSelectDropdown.vue to reference the project’s वास्तविक NodeId
declaration module instead of the litegraph barrel, and ensure the NodeId type
used in the props definition stays aligned with that source.
---
Outside diff comments:
In `@src/renderer/extensions/vueNodes/components/LGraphNode.vue`:
- Around line 754-768: The shrink-wrap watcher in LGraphNode.vue only clears
--node-height when loadVideoShrinkWrapBody turns on, leaving --node-height-x
behind and causing stale collapsed height to be restored later. Update this
watch(loadVideoShrinkWrapBody, ...) handler to clear the collapsed height
variables together, using the existing nodeContainerRef and initSizeStyles flow
so the expand/collapse logic stays consistent.
- Around line 748-752: The loadVideoShrinkWrapBody computed only depends on
nodeOutputs, so it misses preview-cache changes and can stay stale after a
cached image arrives. Update the dependency tracking inside
loadVideoShrinkWrapBody in LGraphNode.vue so it also touches the preview-cache
state used by nodeHasLoadVideoPreview, ensuring the LoadVideo layout recomputes
when preview data changes even without a new output payload.
---
Duplicate comments:
In `@src/components/video/MediaUploadEmpty.vue`:
- Around line 54-56: The drag-and-drop handler in MediaUploadEmpty.vue is
swallowing failures by turning a rejected onDragDrop() call into a silent no-op.
Update the drop handling logic around the onDragDrop/event branch to propagate
errors through the component’s existing error path instead of using a blank
catch, while still addressing the floating promise by explicitly awaiting or
otherwise handling the promise in a way that preserves rejection visibility. Use
the onDragDrop handler site to locate the change.
In `@src/composables/video/useVideoFilmstrip.ts`:
- Around line 123-130: The size probe is being repeated in the frame-rate path
because `probeVideoFrameRate()` falls back to `fetchHttpResourceByteSize(url)`
when `detectedFileSize` is `undefined`. Update the `useVideoFilmstrip` flow so
the initial `fetchHttpResourceByteSize` attempt is treated as “already tried but
unknown” and pass that state into `probeVideoFrameRate()` (or equivalent),
allowing it to skip its internal re-probe when the size is still unavailable.
Keep the fix localized around `detectedFileSize` and `probeVideoFrameRate()` so
the second HEAD/Range lookup is avoided.
🪄 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: d20b664d-0c75-4db9-910a-a15f5bffda65
📒 Files selected for processing (22)
.gitignorepackages/design-system/src/css/style.csssrc/components/ui/tooltip/TooltipProvider.vuesrc/components/ui/tooltip/TooltipTrigger.vuesrc/components/video/LoadVideoTrimPanel.test.tssrc/components/video/LoadVideoTrimPanel.vuesrc/components/video/MediaUploadEmpty.test.tssrc/components/video/MediaUploadEmpty.vuesrc/composables/video/probeVideoFrameRate.tssrc/composables/video/useVideoFilmstrip.tssrc/locales/en/main.jsonsrc/renderer/extensions/vueNodes/components/LGraphNode.vuesrc/renderer/extensions/vueNodes/composables/useProcessedWidgets.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.test.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vuesrc/renderer/extensions/vueNodes/widgets/components/WidgetVideoTrim.vuesrc/renderer/extensions/vueNodes/widgets/components/form/dropdown/FormDropdownInput.vuesrc/renderer/extensions/vueNodes/widgets/composables/useComboWidget.tssrc/renderer/extensions/vueNodes/widgets/composables/useImageUploadWidget.tssrc/renderer/extensions/vueNodes/widgets/composables/useVideoTrimWidget.tssrc/utils/httpResourceByteSize.test.tssrc/utils/httpResourceByteSize.ts
Comment on lines
+149
to
+151
| const removeButton = screen.getByTestId('video-remove-button') | ||
| removeButton.focus() | ||
| await user.keyboard('{Enter}') |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Query the remove action by role/name.
This button already has a stable accessible name, so getByRole('button', { name: 'Remove' }) would keep the test aligned with the keyboard behavior it's asserting instead of coupling it to data-testid. Based on learnings, tests in this repo should prefer accessible queries over data-testid when the element already has a stable role/name.
♻️ Proposed change
- const removeButton = screen.getByTestId('video-remove-button')
+ const removeButton = screen.getByRole('button', { name: 'Remove' })
removeButton.focus()
await user.keyboard('{Enter}')📝 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 removeButton = screen.getByTestId('video-remove-button') | |
| removeButton.focus() | |
| await user.keyboard('{Enter}') | |
| const removeButton = screen.getByRole('button', { name: 'Remove' }) | |
| removeButton.focus() | |
| await user.keyboard('{Enter}') |
🤖 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/video/LoadVideoTrimPanel.test.ts` around lines 149 - 151, The
test is querying the remove control by data-testid even though it already has a
stable accessible role and name. Update the assertion setup in
LoadVideoTrimPanel.test.ts to use the button’s accessible query via getByRole
with the name “Remove” instead of getByTestId('video-remove-button'), so the
keyboard interaction remains aligned with how users access the control.
Source: Learnings
Comment on lines
+123
to
+131
| const detectedFileSize = await fetchHttpResourceByteSize(url) | ||
|
|
||
| if (isLoadStale(loadId, url)) return | ||
|
|
||
| const detectedFrameRate = await probeVideoFrameRate( | ||
| url, | ||
| videoDuration, | ||
| detectedFileSize | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reset the previous probe results before this serial load path runs.
These awaits now run one after the other. Since the composable keeps the previous video's duration, totalFrames, width/height, fps, and fileSize until they finish, switching URLs can keep rendering stale trim bounds and metadata for the new asset throughout this block. Clear the derived refs at the start of the load so the panel goes blank instead of showing old data while the new probes are in flight.
♻️ Proposed change
async function loadVideo(url: string) {
const loadId = ++activeLoadId
loading.value = true
error.value = null
thumbnails.value = []
+ duration.value = 0
+ totalFrames.value = 0
+ width.value = 0
+ height.value = 0
+ fps.value = options.fps ?? DEFAULT_VIDEO_FPS
+ fileSize.value = undefined
const video = document.createElement('video')📝 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 detectedFileSize = await fetchHttpResourceByteSize(url) | |
| if (isLoadStale(loadId, url)) return | |
| const detectedFrameRate = await probeVideoFrameRate( | |
| url, | |
| videoDuration, | |
| detectedFileSize | |
| ) | |
| async function loadVideo(url: string) { | |
| const loadId = ++activeLoadId | |
| loading.value = true | |
| error.value = null | |
| thumbnails.value = [] | |
| duration.value = 0 | |
| totalFrames.value = 0 | |
| width.value = 0 | |
| height.value = 0 | |
| fps.value = options.fps ?? DEFAULT_VIDEO_FPS | |
| fileSize.value = undefined | |
| const video = document.createElement('video') |
🤖 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 123 - 131, Reset the
previously derived video metadata at the start of the load flow in
useVideoFilmstrip before the sequential fetch/probe awaits run, so stale values
do not persist while a new URL is loading. Clear the refs that hold duration,
totalFrames, width, height, fps, and fileSize right before this serial load path
begins, then let the existing fetchHttpResourceByteSize and probeVideoFrameRate
logic repopulate them once the new asset finishes probing.
dosubot
Bot
added
the
size:XXL
uytieu
added
preview-cpu
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/extensions/vueNodes/widgets/components/WidgetVideoTrim.vue (1)
83-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset the node upload flag during removal.
handleRemove()clears the file and preview state, but it never clearscurrentNode.isUploading. Both this component andWidgetSelectDropdown.vuenow use that flag as their busy-state source, so removing a video while the node is marked uploading can leave the trim UI and dropdown stuck disabled/spinning even though the file is gone.Proposed fix
useNodeOutputStore().removeNodeOutputsForNode(currentNode) + currentNode.isUploading = false currentNode.imgs = undefined currentNode.videoContainer = 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/renderer/extensions/vueNodes/widgets/components/WidgetVideoTrim.vue` around lines 83 - 116, `handleRemove()` in `WidgetVideoTrim.vue` clears the file and preview state but leaves `currentNode.isUploading` unchanged, which can keep the trim widget and shared upload UI busy after removal. Update `handleRemove()` to reset the node’s upload flag alongside the other cleanup in this function, using the existing `currentNode` object so `WidgetSelectDropdown.vue` and this trim component both see the node as no longer uploading.
🤖 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.
Outside diff comments:
In `@src/renderer/extensions/vueNodes/widgets/components/WidgetVideoTrim.vue`:
- Around line 83-116: `handleRemove()` in `WidgetVideoTrim.vue` clears the file
and preview state but leaves `currentNode.isUploading` unchanged, which can keep
the trim widget and shared upload UI busy after removal. Update `handleRemove()`
to reset the node’s upload flag alongside the other cleanup in this function,
using the existing `currentNode` object so `WidgetSelectDropdown.vue` and this
trim component both see the node as no longer uploading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 57fcb180-9116-4f62-8b6d-cd6d77e1f923
📒 Files selected for processing (4)
src/renderer/extensions/vueNodes/composables/useProcessedWidgets.tssrc/renderer/extensions/vueNodes/widgets/components/WidgetSelect.vuesrc/renderer/extensions/vueNodes/widgets/components/WidgetSelectDropdown.vuesrc/renderer/extensions/vueNodes/widgets/components/WidgetVideoTrim.vue
💤 Files with no reviewable changes (1)
- src/renderer/extensions/vueNodes/composables/useProcessedWidgets.ts
Author
|
This PR serves as a prototype and reference ONLY.
UPDATE: Added to summary above |
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Improved the Load Video node to be similar to new Load Image node pattern (ex. empty state, select from asset library or from device) and extended it to have video trim functionality.
Changes
useLoadVideoPreviewignores placeholder values;WidgetVideoTrimonly shows uploading during actual uploads)TooltipHint(Reka UI) for centered, overflow-safe tooltips on remove and trim action buttonsWidgetInputActionButtonClassfor shared enabled/disabled widget button styling (cursor-not-allowed, Comfy disabled tokens)Review Focus
filewidget,widgetValueStore, andnodeOutputStorereliably returns to the empty drag-and-drop state (including after remove and on newly placed nodes)[startFrame, endFrame]when trim is enabled, including click/drag in dimmed regions outside the selectioncursor-not-allowedshownoverflow-hiddenTooltipHintpositioning and hover delay feel consistent across remove + set start/end buttonsScreenshots