Range editor#10936
Merged
christian-byrne merged 3 commits intomainfrom Apr 10, 2026
Merged
Conversation
dosubot
Bot
added
the
size:XXL
Contributor
📝 WalkthroughWalkthroughAdds a new RangeEditor UI and related widget plumbing (component, composable, utils, types, schema, and tests). Moves Changes
Sequence DiagramsequenceDiagram
participant User
participant RangeEditor
participant Composable as useRangeEditor
participant Model as RangeValue
participant Inputs as ScrubableNumberInput
User->>RangeEditor: pointerdown on track or handle
RangeEditor->>Composable: handleTrackPointerDown(event)
Composable->>Composable: choose nearest handle (min/max/midpoint)
Composable->>Composable: startDrag(handle, event)
loop dragging
User->>RangeEditor: pointermove
RangeEditor->>Composable: pointermove event
Composable->>Composable: compute normalized position, clamp & constrain
Composable->>Model: update min/max/midpoint
Model->>RangeEditor: re-render handles and visuals
RangeEditor->>Inputs: update numeric inputs
end
User->>RangeEditor: pointerup / lostpointercapture
Composable->>Composable: remove listeners, reset activeHandle
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🎭 Playwright: ✅ 994 passed, 0 failed · 1 flaky📊 Browser Reports
|
🎨 Storybook: ✅ Built — View Storybook |
📦 Bundle: 5.13 MB gzip 🔴 +5.6 kBDetailsSummary
Category Glance App Entry Points — 22.3 kB (baseline 22.3 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.2 MB (baseline 1.2 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 76.6 kB (baseline 76.6 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 11 added / 11 removed Panels & Settings — 484 kB (baseline 484 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 22 added / 22 removed User & Accounts — 17.1 kB (baseline 17.1 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 7 added / 7 removed Editors & Dialogs — 109 kB (baseline 109 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 2 added / 2 removed UI Components — 60.3 kB (baseline 60.3 kB) • ⚪ 0 BReusable component library chunks
Status: 11 added / 11 removed / 2 unchanged Data & Services — 2.98 MB (baseline 2.98 MB) • 🔴 +2.52 kBStores, services, APIs, and repositories
Status: 14 added / 14 removed / 3 unchanged Utilities & Hooks — 339 kB (baseline 338 kB) • 🔴 +153 BHelpers, composables, and utility bundles
Status: 20 added / 19 removed / 7 unchanged Vendor & Third-Party — 9.8 MB (baseline 9.8 MB) • ⚪ 0 BExternal libraries and shared vendor chunks
Status: 1 added / 1 removed / 15 unchanged Other — 8.55 MB (baseline 8.53 MB) • 🔴 +16.9 kBBundles that do not match a named category
Status: 120 added / 119 removed / 15 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/range/RangeEditor.test.ts`:
- Around line 29-31: The test is brittle because it asserts exact stringified
float values for SVG geometry (attributes('x') and attributes('width') in
RangeEditor.test.ts and the similar assertions at lines 122-123); update these
assertions to parse the attribute values to numbers and assert numerically
(e.g., using toBeCloseTo or an absolute-delta check) so small floating-point
formatting differences won’t break the test—locate the assertions that call
highlight.attributes('x') and highlight.attributes('width') (and the matching
assertions later in the file) and replace the strict string equality checks with
numeric comparisons within a small tolerance.
In `@src/components/range/RangeEditor.vue`:
- Around line 231-236: Clamp the normalized coordinates returned by normalize
before using them: update the computed values minNorm and maxNorm (which call
normalize(modelValue.value.min, valueMin, valueMax) and
normalize(modelValue.value.max, valueMin, valueMax)) to constrain their outputs
to [0,1] (e.g., via a clamp using Math.max(0, Math.min(1, ...)) or a shared
clamp util) and apply the same clamping to the other nearby computed normalized
values referenced later in the file (the additional computed normalized
positions mentioned around the same block) so handles and overlays cannot render
outside the track when modelValue is out of bounds.
In `@src/components/range/rangeUtils.ts`:
- Around line 50-55: The type guard isRangeValue currently allows NaN/Infinity
and doesn't validate an optional midpoint; update it to reject non-finite
numbers by using Number.isFinite for v.min and v.max, and if v.midpoint exists
ensure typeof v.midpoint === 'number' && Number.isFinite(v.midpoint); keep the
existing object/null/array checks and the cast to Record<string, unknown>, but
replace the typeof checks for min/max with Number.isFinite checks to tighten
validation and avoid leaking invalid numeric payloads into rendering/math paths.
🪄 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: CHILL
Plan: Pro
Run ID: 9d4bd94f-673e-4af5-8a98-2ded1594ebb2
📒 Files selected for processing (19)
src/components/curve/CurveEditor.vuesrc/components/curve/curveUtils.test.tssrc/components/curve/curveUtils.tssrc/components/range/RangeEditor.test.tssrc/components/range/RangeEditor.vuesrc/components/range/WidgetRange.vuesrc/components/range/rangeUtils.test.tssrc/components/range/rangeUtils.tssrc/composables/useRangeEditor.tssrc/lib/litegraph/src/types/widgets.tssrc/lib/litegraph/src/widgets/RangeWidget.tssrc/lib/litegraph/src/widgets/widgetMap.tssrc/renderer/extensions/vueNodes/widgets/composables/useRangeWidget.tssrc/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.tssrc/schemas/nodeDef/nodeDefSchemaV2.tssrc/schemas/nodeDefSchema.tssrc/scripts/widgets.tssrc/utils/histogramUtil.test.tssrc/utils/histogramUtil.ts
💤 Files with no reviewable changes (1)
- src/components/curve/curveUtils.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/composables/useRangeEditor.ts (2)
31-34: Consider usingclampfor consistency.Since
clampis already imported from es-toolkit and used elsewhere (line 55), consider using it here as well for uniformity.♻️ Suggested refactor
- const normalized = Math.max( - 0, - Math.min(1, (e.clientX - rect.left) / rect.width) - ) + const normalized = clamp((e.clientX - rect.left) / rect.width, 0, 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/useRangeEditor.ts` around lines 31 - 34, The computation of normalized uses Math.max/Math.min instead of the existing clamp utility; replace the Math.max/Math.min expression that sets the normalized variable with a call to clamp((e.clientX - rect.left) / rect.width, 0, 1) to keep consistency with other uses (see clamp import and usages around line 55) and ensure normalized remains between 0 and 1.
63-66: Same consistency opportunity withclamp.The manual clamping on line 65 can use the already-imported
clampfunction.♻️ Suggested refactor
const midNorm = range > 0 ? normalize(clamped, current.min, current.max) : 0 - const midpoint = Math.max(0, Math.min(1, midNorm)) + const midpoint = clamp(midNorm, 0, 1) modelValue.value = { ...current, midpoint }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/useRangeEditor.ts` around lines 63 - 66, Replace the manual clamping of midpoint with the imported clamp utility: compute midNorm as before, then set midpoint = clamp(midNorm, 0, 1) instead of using Math.max/Math.min; update the assignment to modelValue.value = { ...current, midpoint } in the useRangeEditor logic so it uses clamp consistently (referencing midNorm, midpoint, and the clamp function).src/components/range/RangeEditor.test.ts (1)
46-50: Histogram dimming assertions use exact string comparisons.Same pattern as above - these exact string assertions (
'0.2','0.8') work because the values serialize consistently, but numeric comparisons would be more resilient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/range/RangeEditor.test.ts` around lines 46 - 50, The histogram dimming assertions in RangeEditor.test.ts rely on exact string matches for attributes which is fragile; change the assertions to parse the attribute values from the elements found by wrapper.find('[data-testid="range-dim-left"]') and wrapper.find('[data-testid="range-dim-right"]') into numbers (e.g., via Number(...) or parseFloat(...)) and assert numerically (use toBeCloseTo(...) for floats or toBe(...) for exact numbers) instead of comparing to the string literals '0.2' and '0.8'.src/components/range/WidgetRange.vue (2)
44-52: Histogram array conversion lacks type validation.
new Uint32Array(data)assumes all elements indataare valid numeric values. If the backend sends unexpected data (strings, floats, etc.), theUint32Arrayconstructor will coerce them, potentially producing incorrect histogram bins.Safer histogram conversion
const key = `histogram_${widget.name}` const data = (output as Record<string, unknown>)?.[key] if (!Array.isArray(data) || data.length === 0) return null - return new Uint32Array(data) + if (!data.every((v) => typeof v === 'number' && Number.isFinite(v))) + return null + return new Uint32Array(data as number[]) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/range/WidgetRange.vue` around lines 44 - 52, The computed histogram currently does new Uint32Array(data) which blindly coerces values; update the histogram computed (and the local key/locatorId handling) to validate and sanitize the extracted data first: check that data is an array, iterate over elements and keep only finite numeric values, convert to integers (e.g., Math.floor) and clamp to the uint32 range (0..0xFFFFFFFF), or bail out (return null) if elements are invalid; then construct the Uint32Array from this sanitized numeric array so widget, nodeOutputStore.nodeOutputs and the key `histogram_${widget.name}` produce a safe Uint32Array instead of relying on implicit coercion.
65-69: Consider simplifyingeffectiveValuecomputed.The
{ value: ... }wrapper object and immediate unwrap in the template (:model-value="effectiveValue.value") adds indirection without clear benefit.Simplified approach
-const effectiveValue = computed(() => - isDisabled.value && upstreamValue.value - ? { value: upstreamValue.value } - : { value: modelValue.value } -) +const effectiveValue = computed(() => + isDisabled.value && upstreamValue.value + ? upstreamValue.value + : modelValue.value +)Then in template:
- :model-value="effectiveValue.value" + :model-value="effectiveValue"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/range/WidgetRange.vue` around lines 65 - 69, The computed effectiveValue currently returns an object wrapper ({ value: ... }) which forces callers to unwrap with .value; simplify by making effectiveValue a computed that returns the raw value (use upstreamValue.value when isDisabled.value && upstreamValue.value, otherwise modelValue.value) and update the template binding from :model-value="effectiveValue.value" to :model-value="effectiveValue" (or use effectiveValue directly where used). Update any other usages expecting the object wrapper to use the plain value instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/range/RangeEditor.test.ts`:
- Around line 46-50: The histogram dimming assertions in RangeEditor.test.ts
rely on exact string matches for attributes which is fragile; change the
assertions to parse the attribute values from the elements found by
wrapper.find('[data-testid="range-dim-left"]') and
wrapper.find('[data-testid="range-dim-right"]') into numbers (e.g., via
Number(...) or parseFloat(...)) and assert numerically (use toBeCloseTo(...) for
floats or toBe(...) for exact numbers) instead of comparing to the string
literals '0.2' and '0.8'.
In `@src/components/range/WidgetRange.vue`:
- Around line 44-52: The computed histogram currently does new Uint32Array(data)
which blindly coerces values; update the histogram computed (and the local
key/locatorId handling) to validate and sanitize the extracted data first: check
that data is an array, iterate over elements and keep only finite numeric
values, convert to integers (e.g., Math.floor) and clamp to the uint32 range
(0..0xFFFFFFFF), or bail out (return null) if elements are invalid; then
construct the Uint32Array from this sanitized numeric array so widget,
nodeOutputStore.nodeOutputs and the key `histogram_${widget.name}` produce a
safe Uint32Array instead of relying on implicit coercion.
- Around line 65-69: The computed effectiveValue currently returns an object
wrapper ({ value: ... }) which forces callers to unwrap with .value; simplify by
making effectiveValue a computed that returns the raw value (use
upstreamValue.value when isDisabled.value && upstreamValue.value, otherwise
modelValue.value) and update the template binding from
:model-value="effectiveValue.value" to :model-value="effectiveValue" (or use
effectiveValue directly where used). Update any other usages expecting the
object wrapper to use the plain value instead.
In `@src/composables/useRangeEditor.ts`:
- Around line 31-34: The computation of normalized uses Math.max/Math.min
instead of the existing clamp utility; replace the Math.max/Math.min expression
that sets the normalized variable with a call to clamp((e.clientX - rect.left) /
rect.width, 0, 1) to keep consistency with other uses (see clamp import and
usages around line 55) and ensure normalized remains between 0 and 1.
- Around line 63-66: Replace the manual clamping of midpoint with the imported
clamp utility: compute midNorm as before, then set midpoint = clamp(midNorm, 0,
1) instead of using Math.max/Math.min; update the assignment to modelValue.value
= { ...current, midpoint } in the useRangeEditor logic so it uses clamp
consistently (referencing midNorm, midpoint, and the clamp function).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 835ecb2b-411e-41db-9f50-db18ec4cab3d
📒 Files selected for processing (19)
src/components/curve/CurveEditor.vuesrc/components/curve/curveUtils.test.tssrc/components/curve/curveUtils.tssrc/components/range/RangeEditor.test.tssrc/components/range/RangeEditor.vuesrc/components/range/WidgetRange.vuesrc/components/range/rangeUtils.test.tssrc/components/range/rangeUtils.tssrc/composables/useRangeEditor.tssrc/lib/litegraph/src/types/widgets.tssrc/lib/litegraph/src/widgets/RangeWidget.tssrc/lib/litegraph/src/widgets/widgetMap.tssrc/renderer/extensions/vueNodes/widgets/composables/useRangeWidget.tssrc/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.tssrc/schemas/nodeDef/nodeDefSchemaV2.tssrc/schemas/nodeDefSchema.tssrc/scripts/widgets.tssrc/utils/histogramUtil.test.tssrc/utils/histogramUtil.ts
💤 Files with no reviewable changes (1)
- src/components/curve/curveUtils.ts
✅ Files skipped from review due to trivial changes (4)
- src/components/curve/CurveEditor.vue
- src/lib/litegraph/src/widgets/RangeWidget.ts
- src/utils/histogramUtil.ts
- src/components/range/rangeUtils.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/components/curve/curveUtils.test.ts
- src/scripts/widgets.ts
- src/renderer/extensions/vueNodes/widgets/registry/widgetRegistry.ts
- src/schemas/nodeDef/nodeDefSchemaV2.ts
- src/utils/histogramUtil.test.ts
- src/renderer/extensions/vueNodes/widgets/composables/useRangeWidget.ts
- src/schemas/nodeDefSchema.ts
- src/lib/litegraph/src/types/widgets.ts
- src/components/range/RangeEditor.vue
| const bestDist = Math.min(dMin, dMax) | ||
| if (midpoint !== undefined) { | ||
| const midAbs = min + midpoint * (max - min) | ||
| if (Math.abs(value - midAbs) < bestDist) { |
Contributor
There was a problem hiding this comment.
issue: nearestHandle snaps to invisible midpoint when showMidpoint is false.
When a user clicks the track, nearestHandle checks if (midpoint !== undefined) but does not check whether the midpoint handle is actually visible. If showMidpoint=false but the model carries a midpoint value, clicking near the center silently modifies the hidden midpoint instead of min/max -- the visible handles appear frozen.
Suggested fix: pass showMidpoint as a Ref<boolean> into useRangeEditor and gate the midpoint branch:
if (midpoint !== undefined && showMidpoint.value)
| return position.toFixed(2) | ||
| } | ||
|
|
||
| export function constrainRange( |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): constrainRange and formatMidpointLabel are defined but never called.
constrainRange exists to clamp values and enforce min<=max, but nothing invokes it. Deserialized workflows with min > max could cause inverted behavior. formatMidpointLabel is also unused -- the component inlines .toFixed(2) instead.
Consider either wiring constrainRange into the initialization path or removing both functions to avoid dead code.
|
|
||
| import { clamp } from 'es-toolkit' | ||
|
|
||
| import { denormalize, normalize } from '@/components/range/rangeUtils' |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): inverted dependency direction -- composable imports from component directory.
useRangeEditor (a shared composable under src/composables/) imports normalize/denormalize from @/components/range/rangeUtils. This inverts the expected dependency direction; lower-level composables should not depend on higher-level component internals. The CurveEditor composable avoids this pattern.
Would it make sense to move normalize/denormalize to src/utils/mathUtil.ts or similar?
| } | ||
| }) | ||
|
|
||
| const effectiveValue = computed(() => |
Contributor
There was a problem hiding this comment.
suggestion (non-blocking): effectiveValue wraps in { value: ... } causing double .value.value indirection.
The computed returns { value: upstreamValue.value } / { value: modelValue.value }, so the template binds :model-value="effectiveValue.value" -- a double .value access. WidgetCurve returns the value directly from its computed without the wrapper object.
Returning RangeValue directly and binding :model-value="effectiveValue" would simplify this and be consistent with the Curve pattern.
|
|
||
| import type { RangeValue } from '@/lib/litegraph/src/types/widgets' | ||
|
|
||
| export { clamp } |
Contributor
There was a problem hiding this comment.
nitpick (non-blocking): clamp is re-exported from rangeUtils but useRangeEditor imports it directly from es-toolkit.
This creates two import paths for the same function. Removing the re-export and importing clamp directly from es-toolkit everywhere would be more consistent.
| midpoint_scale?: 'linear' | 'gamma' | ||
| value_min?: number | ||
| value_max?: number | ||
| histogram?: Uint32Array | null |
Contributor
There was a problem hiding this comment.
nitpick (non-blocking): histogram field on IWidgetRangeOptions appears unused.
The component fetches histogram data from nodeOutputStore at runtime, not from widget options. If this field is not used as static config, consider removing it to keep the options interface clean.
| const el = trackRef.value | ||
| if (!el) return valueMin.value | ||
| const rect = el.getBoundingClientRect() | ||
| const normalized = Math.max( |
Contributor
There was a problem hiding this comment.
nitpick (non-blocking): Math.max(0, Math.min(1, ...)) could be clamp(value, 0, 1).
clamp is already imported from es-toolkit on line 4.
| } | ||
|
|
||
| export function positionToGamma(position: number): number { | ||
| const clamped = clamp(position, 0.001, 0.999) |
Contributor
There was a problem hiding this comment.
nitpick (non-blocking): magic numbers 0.001 and 0.999 in positionToGamma are unexplained.
A short comment noting these prevent log2(0) / log2(1) edge cases would help readability.
christian-byrne
assigned
jtydhr88,
christian-byrne and
pythongosssss and unassigned
christian-byrne
dosubot
Bot
added
size:XL
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/utils/mathUtil.test.ts (1)
25-35: Add a degenerate-range test fordenormalizeas a guardrail.A dedicated
min === maxtest would lock expected behavior for future refactors.Based on learnings, "In tests for manually-coded math/geometry modules (e.g., interpolators, path generators, LUT generators like curveUtils), ensure extensive unit test coverage that exercises permutations, edge cases, and deterministic behavior."Suggested test addition
describe('denormalize', () => { it('converts normalized value back to range', () => { expect(denormalize(0.5, 0, 256)).toBe(128) expect(denormalize(0, 0, 255)).toBe(0) expect(denormalize(1, 0, 255)).toBe(255) }) + it('returns min when min equals max', () => { + expect(denormalize(0, 5, 5)).toBe(5) + expect(denormalize(0.5, 5, 5)).toBe(5) + expect(denormalize(1, 5, 5)).toBe(5) + }) + it('round-trips with normalize', () => { expect(denormalize(normalize(100, 0, 255), 0, 255)).toBeCloseTo(100) }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/mathUtil.test.ts` around lines 25 - 35, Add a degenerate-range unit test for denormalize to assert behavior when min === max: in src/utils/mathUtil.test.ts add a case under the 'denormalize' suite that calls denormalize with identical min and max (e.g., denormalize(0.0, 5, 5), denormalize(1.0, 5, 5), and denormalize(0.5, 5, 5)) and expects the result to equal that constant bound (5) for all inputs; reference the denormalize function and the existing 'denormalize' describe block to place the test so future refactors keep this guardrail.src/components/range/WidgetRange.vue (1)
44-52: Consider validating histogram array element types.The histogram data is cast from
unknownand converted toUint32Array. WhileArray.isArray(data)guards against non-arrays, it doesn't guarantee the elements are numeric. If the backend sends unexpected data,new Uint32Array(data)will silently coerce non-numbers to 0.This is low risk since the backend contract should send numeric histogram bins, but for defensive coding you could add an element type check:
♻️ Optional: Add element validation
const histogram = computed(() => { const locatorId = widget.nodeLocatorId if (!locatorId) return null const output = nodeOutputStore.nodeOutputs[locatorId] const key = `histogram_${widget.name}` const data = (output as Record<string, unknown>)?.[key] - if (!Array.isArray(data) || data.length === 0) return null + if ( + !Array.isArray(data) || + data.length === 0 || + !data.every((v) => typeof v === 'number') + ) + return null return new Uint32Array(data) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/range/WidgetRange.vue` around lines 44 - 52, The computed histogram currently trusts that data from nodeOutputStore.nodeOutputs[locatorId][`histogram_${widget.name}`] is numeric and constructs a new Uint32Array directly, which can silently coerce non-numeric elements to 0; update the histogram computed to validate that data is an array of numbers (e.g., check every element typeof === 'number' and isFinite) before creating the Uint32Array, and return null (or filter/transform to numbers) if validation fails so that new Uint32Array(data) only receives safe numeric input; reference the histogram computed getter, locatorId, nodeOutputStore.nodeOutputs, the key `histogram_${widget.name}`, and the Uint32Array construction when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/composables/useRangeEditor.ts`:
- Around line 29-35: The pointerToValue function can produce NaN when the track
element has zero width; update pointerToValue to guard against rect.width === 0
(or <= 0) by returning a safe fallback (e.g., valueMin.value or the nearest
valid bound) before computing normalized and calling denormalize; refer to
pointerToValue, trackRef, valueMin/valueMax, clamp and denormalize when making
this change so the drag logic never divides by rect.width.
- Around line 74-81: The function startDrag currently sets activeHandle.value
before verifying trackRef.value exists, which can leave activeHandle stale if
trackRef is null; move the assignment to activeHandle.value so it occurs after
the null-check for trackRef (i.e., after const el = trackRef.value and the if
(!el) return), and keep cleanupDrag() call and other logic intact so
activeHandle is only set when a valid track element exists.
---
Nitpick comments:
In `@src/components/range/WidgetRange.vue`:
- Around line 44-52: The computed histogram currently trusts that data from
nodeOutputStore.nodeOutputs[locatorId][`histogram_${widget.name}`] is numeric
and constructs a new Uint32Array directly, which can silently coerce non-numeric
elements to 0; update the histogram computed to validate that data is an array
of numbers (e.g., check every element typeof === 'number' and isFinite) before
creating the Uint32Array, and return null (or filter/transform to numbers) if
validation fails so that new Uint32Array(data) only receives safe numeric input;
reference the histogram computed getter, locatorId, nodeOutputStore.nodeOutputs,
the key `histogram_${widget.name}`, and the Uint32Array construction when making
the change.
In `@src/utils/mathUtil.test.ts`:
- Around line 25-35: Add a degenerate-range unit test for denormalize to assert
behavior when min === max: in src/utils/mathUtil.test.ts add a case under the
'denormalize' suite that calls denormalize with identical min and max (e.g.,
denormalize(0.0, 5, 5), denormalize(1.0, 5, 5), and denormalize(0.5, 5, 5)) and
expects the result to equal that constant bound (5) for all inputs; reference
the denormalize function and the existing 'denormalize' describe block to place
the test so future refactors keep this guardrail.
🪄 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: CHILL
Plan: Pro
Run ID: ca0c3621-1fa8-453f-8aa3-6698192ab283
📒 Files selected for processing (8)
src/components/range/RangeEditor.vuesrc/components/range/WidgetRange.vuesrc/components/range/rangeUtils.test.tssrc/components/range/rangeUtils.tssrc/composables/useRangeEditor.tssrc/lib/litegraph/src/types/widgets.tssrc/utils/mathUtil.test.tssrc/utils/mathUtil.ts
✅ Files skipped from review due to trivial changes (1)
- src/components/range/rangeUtils.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/range/RangeEditor.vue
- src/lib/litegraph/src/types/widgets.ts
Comment on lines
+29
to
+35
| function pointerToValue(e: PointerEvent): number { | ||
| const el = trackRef.value | ||
| if (!el) return valueMin.value | ||
| const rect = el.getBoundingClientRect() | ||
| const normalized = clamp((e.clientX - rect.left) / rect.width, 0, 1) | ||
| return denormalize(normalized, valueMin.value, valueMax.value) | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against zero-width track to prevent NaN model updates.
When rect.width is 0, normalization can become NaN/Infinity and propagate into modelValue during drag updates.
💡 Proposed fix
function pointerToValue(e: PointerEvent): number {
const el = trackRef.value
if (!el) return valueMin.value
const rect = el.getBoundingClientRect()
+ if (rect.width <= 0) return valueMin.value
const normalized = clamp((e.clientX - rect.left) / rect.width, 0, 1)
return denormalize(normalized, valueMin.value, valueMax.value)
}📝 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
| function pointerToValue(e: PointerEvent): number { | |
| const el = trackRef.value | |
| if (!el) return valueMin.value | |
| const rect = el.getBoundingClientRect() | |
| const normalized = clamp((e.clientX - rect.left) / rect.width, 0, 1) | |
| return denormalize(normalized, valueMin.value, valueMax.value) | |
| } | |
| function pointerToValue(e: PointerEvent): number { | |
| const el = trackRef.value | |
| if (!el) return valueMin.value | |
| const rect = el.getBoundingClientRect() | |
| if (rect.width <= 0) return valueMin.value | |
| const normalized = clamp((e.clientX - rect.left) / rect.width, 0, 1) | |
| return denormalize(normalized, valueMin.value, valueMax.value) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/composables/useRangeEditor.ts` around lines 29 - 35, The pointerToValue
function can produce NaN when the track element has zero width; update
pointerToValue to guard against rect.width === 0 (or <= 0) by returning a safe
fallback (e.g., valueMin.value or the nearest valid bound) before computing
normalized and calling denormalize; refer to pointerToValue, trackRef,
valueMin/valueMax, clamp and denormalize when making this change so the drag
logic never divides by rect.width.
Comment on lines
+74
to
+81
| function startDrag(handle: HandleType, e: PointerEvent) { | ||
| if (e.button !== 0) return | ||
| cleanupDrag?.() | ||
|
|
||
| activeHandle.value = handle | ||
| const el = trackRef.value | ||
| if (!el) return | ||
|
|
Contributor
There was a problem hiding this comment.
Move activeHandle assignment after trackRef null-check.
If trackRef.value is null, the function returns early but leaves activeHandle set, causing stale drag state.
💡 Proposed fix
function startDrag(handle: HandleType, e: PointerEvent) {
if (e.button !== 0) return
cleanupDrag?.()
- activeHandle.value = handle
const el = trackRef.value
if (!el) return
+ activeHandle.value = handle
el.setPointerCapture(e.pointerId)📝 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
| function startDrag(handle: HandleType, e: PointerEvent) { | |
| if (e.button !== 0) return | |
| cleanupDrag?.() | |
| activeHandle.value = handle | |
| const el = trackRef.value | |
| if (!el) return | |
| function startDrag(handle: HandleType, e: PointerEvent) { | |
| if (e.button !== 0) return | |
| cleanupDrag?.() | |
| const el = trackRef.value | |
| if (!el) return | |
| activeHandle.value = handle |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/composables/useRangeEditor.ts` around lines 74 - 81, The function
startDrag currently sets activeHandle.value before verifying trackRef.value
exists, which can leave activeHandle stale if trackRef is null; move the
assignment to activeHandle.value so it occurs after the null-check for trackRef
(i.e., after const el = trackRef.value and the if (!el) return), and keep
cleanupDrag() call and other logic intact so activeHandle is only set when a
valid track element exists.
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #10936 +/- ##
=======================================
Coverage ? 43.24%
=======================================
Files ? 1342
Lines ? 68396
Branches ? 18031
=======================================
Hits ? 29576
Misses ? 38230
Partials ? 590
🚀 New features to boost your workflow:
|
| const range = current.max - current.min | ||
| const midNorm = | ||
| range > 0 ? normalize(clamped, current.min, current.max) : 0 | ||
| const midpoint = Math.max(0, Math.min(1, midNorm)) |
Member
There was a problem hiding this comment.
Considering you've imported clamp and are using it above, should you standardize to use it here too
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/composables/useRangeEditor.ts (2)
78-81:⚠️ Potential issue | 🟡 MinorSet
activeHandleonly after confirmingtrackRefexists.Line 78 assigns
activeHandlebefore the null check on Line 80. IftrackRef.valueis null, drag state can remain stale.💡 Proposed fix
function startDrag(handle: HandleType, e: PointerEvent) { if (e.button !== 0) return cleanupDrag?.() - activeHandle.value = handle const el = trackRef.value if (!el) return + activeHandle.value = handle el.setPointerCapture(e.pointerId)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/useRangeEditor.ts` around lines 78 - 81, The code sets activeHandle.value before verifying the existence of trackRef.value which can leave drag state stale if trackRef is null; move the assignment of activeHandle.value = handle to occur after the null-check for trackRef (i.e., check const el = trackRef.value and return if falsy, then set activeHandle.value), updating the logic in the useRangeEditor handler where activeHandle and trackRef are referenced so the handle is only activated when the track element exists.
32-34:⚠️ Potential issue | 🟠 MajorGuard zero-width track before normalization.
On Line 33, dividing by
rect.widthwithout a guard can produce invalid normalized values when width is0. This can propagate unstable updates tomodelValue.💡 Proposed fix
function pointerToValue(e: PointerEvent): number { const el = trackRef.value if (!el) return valueMin.value const rect = el.getBoundingClientRect() + if (rect.width <= 0) return valueMin.value const normalized = clamp((e.clientX - rect.left) / rect.width, 0, 1) return denormalize(normalized, valueMin.value, valueMax.value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/composables/useRangeEditor.ts` around lines 32 - 34, Guard against a zero-width track before normalizing: after calling el.getBoundingClientRect() check if rect.width === 0 and, if so, return denormalize(0, valueMin.value, valueMax.value) (or another safe default) instead of dividing by rect.width; otherwise compute normalized as before with clamp((e.clientX - rect.left) / rect.width, 0, 1) and return denormalize(normalized, valueMin.value, valueMax.value). Ensure you touch the code around el.getBoundingClientRect(), the normalization expression using e.clientX and rect.width, and the denormalize call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/composables/useRangeEditor.ts`:
- Around line 78-81: The code sets activeHandle.value before verifying the
existence of trackRef.value which can leave drag state stale if trackRef is
null; move the assignment of activeHandle.value = handle to occur after the
null-check for trackRef (i.e., check const el = trackRef.value and return if
falsy, then set activeHandle.value), updating the logic in the useRangeEditor
handler where activeHandle and trackRef are referenced so the handle is only
activated when the track element exists.
- Around line 32-34: Guard against a zero-width track before normalizing: after
calling el.getBoundingClientRect() check if rect.width === 0 and, if so, return
denormalize(0, valueMin.value, valueMax.value) (or another safe default) instead
of dividing by rect.width; otherwise compute normalized as before with
clamp((e.clientX - rect.left) / rect.width, 0, 1) and return
denormalize(normalized, valueMin.value, valueMax.value). Ensure you touch the
code around el.getBoundingClientRect(), the normalization expression using
e.clientX and rect.width, and the denormalize call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e55dca72-3340-4471-bd88-59e8e7dcdbd3
📒 Files selected for processing (2)
src/composables/useRangeEditor.tssrc/utils/mathUtil.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/mathUtil.ts
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
3 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.
BE change Comfy-Org/ComfyUI#13322
Summary
Add RANGE widget for image levels adjustment
- Add RangeEditor widget with three display modes: plain, gradient, and histogram
Screenshots (if applicable)
┆Issue is synchronized with this Notion page by Unito