Skip to content

BezierCurveEditor add Y-axis scale input, positive range mode, extension lines & bug fixes#20

Open
GuoLei1990 wants to merge 17 commits intogalacean:mainfrom
GuoLei1990:pr/opt-curve
Open

BezierCurveEditor add Y-axis scale input, positive range mode, extension lines & bug fixes#20
GuoLei1990 wants to merge 17 commits intogalacean:mainfrom
GuoLei1990:pr/opt-curve

Conversation

@GuoLei1990
Copy link
Member

@GuoLei1990 GuoLei1990 commented Feb 16, 2026

Summary

  • Y轴刻度输入:新增 yTickScale 属性,支持动态调整Y轴范围,自适应宽度输入框(Enter 提交,Escape 还原)
  • Y轴范围模式:新增 yRangeMode("symmetric" | "positive"),positive 模式限制Y轴为非负值
  • 曲线延伸线:当首个关键帧 time > 0 或末尾关键帧 time < 1 时,向视口边缘绘制水平延伸线,匹配引擎运行时行为。延伸线使用较浅颜色($orange6)与主曲线($orange10)区分
  • 删除关键帧:右键菜单和键盘快捷键(Delete/Backspace)删除悬停的关键帧,最少保留1个点
  • 小窗预览同步:缩略图预览也渲染延伸线

Bug 修复

  • 修复缩放时曲线与点位置偏移 — 路径生成对 zoom 做了二次缩放,而点坐标在 denormalize 时已包含 zoom
  • getScreenCTM() 增加空值保护,防止 SVG 卸载时崩溃
  • 补充 CurveAnimation useMemo 缺失的 algo 依赖

代码清理

  • 移除未使用的 Grid vertical variant 和辅助函数参数
  • zoomLimit 类型收紧为 [number, number]
  • 移除曲线拖拽操作,简化交互

测试计划

  • 关键帧 time > 0 或 < 1 时显示水平延伸线至视口边缘
  • 添加/拖拽/删除后延伸线自动更新
  • 仅剩1个点时显示完整水平线,可点击添加新关键帧
  • 缩放后曲线与点位置对齐
  • 右键关键帧 → "Delete Keyframe" 可用;仅剩1点时禁用
  • 悬停关键帧 + Delete/Backspace 可删除
  • 缩略图预览显示延伸线
  • Y轴刻度输入:Enter 提交,Escape 还原
  • positive 模式限制Y轴为非负值

@coderabbitai
Copy link

coderabbitai bot commented Feb 16, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Added per-point IDs and clamping, anchor-based zooming with constrained pan, editable Y‑tick scaling and precision helpers, grid/tick clipping and formatting, simplified BezierCurve hover/preview (removed ref forwarding), and Point/BezierPoint prop/signature/API changes across the editor stack.

Changes

Cohort / File(s) Summary
Curve Rendering & Hover
packages/ui/src/BezierCurveEditor/BezierCurve.tsx
Removed forwardRef/mergeRefs; replaced multiple refs/state with `matrixedPoint: IPoint
Editor Core & Interaction
packages/ui/src/BezierCurveEditor/BezierCurveEditor.tsx
Major refactor: per-point IDs and immutable updates, input clamping/normalization, anchor-based zooming (applyZoomAtAnchor + wheel handler), clampOffsetForZoom, zoom limit changes, Y‑tick scale UI and editable overlay, reset/anchor zoom controls, and new public props for Y-scale/yRangeMode.
Point Components & Dragging
packages/ui/src/BezierCurveEditor/Point.tsx, packages/ui/src/BezierCurveEditor/BezierPoint.tsx
API changes: index/zoom removed → pointId: string, getRoot added, onPointChange now (pointId, point) or svgPos; added hoverLabel and pointClipPath, hover visuals/labels, hit/ring radii constants, internal drag logic and SVG-coordinate handling.
Grid, Ticks & Clipping
packages/ui/src/BezierCurveEditor/Grid.tsx, packages/ui/src/BezierCurveEditor/tick.ts
Grid: axisLabel flattened to string, added overlay, yTickScale, axisYScale, generated clipPaths for tick groups, tick precision/formatting and scaled Y-ticks. New tick.ts: tick precision/formatting utilities (TICK_EPSILON, getTickPrecision, formatTick, getAlignedTickPrecision).
Public API & Types
packages/ui/src/BezierCurveEditor/index.tsx, packages/ui/src/BezierCurveEditor/types.ts
BezierCurveEditor public props extended via CurveScaleProps: axisLabel: string, yTickScale, yRangeMode, onYTickScaleChange, yTickScaleMin, yTickScaleMax; removed pan/grid and nested axis label/name shapes; props wired through editor.
Helpers & Misc
packages/ui/src/BezierCurveEditor/helper.ts, packages/ui/src/BezierCurveEditor/CurveAnimation.tsx, packages/ui/src/BezierCurveEditor/Preset.tsx
Helper: introduced scalePoints, adjusted generateCurve/generateLine paths; removed unused exports and tightened signatures. Minor renames/import tidy and typo fix in Preset; small animation rename.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant SVG as SVGElement
  participant Editor as BezierCurveEditor
  participant Store as PointStore
  participant Curve as BezierCurve
  participant Grid as Grid

  User->>SVG: wheel(event)
  SVG->>Editor: handleWheel(event) -> compute anchor from bounds
  Editor->>Editor: clampZoom(newZoom)
  Editor->>Editor: applyZoomAtAnchor(anchor, newZoom) -> update offset
  Editor->>Store: set zoom & offset
  Editor->>Curve: props updated (zoom, offset, yTickScale)
  Editor->>Grid: props updated (axisYScale, yTickScale)
  Curve->>Curve: clear matrixedPoint on zoom change
  User->>Curve: pointer move -> getCursorPoint -> update matrixedPoint (preview)
  User->>Store: click -> addPoint(pointId, normalizedPoint)
  Store->>Editor: points updated -> re-render Curve & Grid
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hop on IDs and snap each line,
Anchors hold zoom while ticks align,
Previews tiptoe under pointer light,
Grids clip and format numbers right,
A joyful hop — these curves take flight. 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Merge Conflict Detection ⚠️ Warning ❌ Merge conflicts detected (26 files):

⚔️ README.md (content)
⚔️ packages/gui/package.json (content)
⚔️ packages/gui/src/components/FormItem/FormItem.tsx (content)
⚔️ packages/gui/src/components/FormItemGroup/FormItemGroup.tsx (content)
⚔️ packages/gui/src/components/FormItemSegmentControl/FormItemSegmentControl.tsx (content)
⚔️ packages/gui/src/components/GUIRoot/GUIRoot.tsx (content)
⚔️ packages/gui/tsup.config.ts (content)
⚔️ packages/ui/src/Badge/Badge.stories.tsx (content)
⚔️ packages/ui/src/Badge/index.tsx (content)
⚔️ packages/ui/src/BezierCurveEditor/BezierCurve.tsx (content)
⚔️ packages/ui/src/BezierCurveEditor/BezierCurveEditor.tsx (content)
⚔️ packages/ui/src/BezierCurveEditor/BezierPoint.tsx (content)
⚔️ packages/ui/src/BezierCurveEditor/CurveAnimation.tsx (content)
⚔️ packages/ui/src/BezierCurveEditor/Grid.tsx (content)
⚔️ packages/ui/src/BezierCurveEditor/Point.tsx (content)
⚔️ packages/ui/src/BezierCurveEditor/Preset.tsx (content)
⚔️ packages/ui/src/BezierCurveEditor/helper.ts (content)
⚔️ packages/ui/src/BezierCurveEditor/index.tsx (content)
⚔️ packages/ui/src/BezierCurveEditor/types.ts (content)
⚔️ packages/ui/src/Collapsible/Collapsible.tsx (content)
⚔️ packages/ui/src/EditableText/index.tsx (content)
⚔️ packages/ui/src/Menu/Menu.tsx (content)
⚔️ packages/ui/src/SegmentControl/SegmentControl.stories.tsx (content)
⚔️ packages/ui/src/SegmentControl/SegmentControl.tsx (content)
⚔️ packages/ui/src/Typography/index.tsx (content)
⚔️ pnpm-lock.yaml (content)

These conflicts must be resolved before merging into main.
Resolve conflicts locally and push changes to this branch.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main changes: Y-axis scale input, positive range mode, extension lines (via clipping), and bug fixes across multiple BezierCurveEditor components.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@GuoLei1990 GuoLei1990 added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Feb 16, 2026
@GuoLei1990 GuoLei1990 changed the title Optimization Curve WIP: Optimization Curve Feb 16, 2026
@GuoLei1990 GuoLei1990 requested a review from Copilot February 16, 2026 05:13
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@packages/ui/src/BezierCurveEditor/BezierCurveEditor.tsx`:
- Around line 185-197: ensurePointIds currently truncates IDs from the end which
misaligns keys when a parent removes a point from the middle; update
ensurePointIds to reconcile IDs with the actual points array instead of blindly
slicing: when reducing length, remove the specific IDs that correspond to
removed points (by comparing the current points array / controlled propPoints to
the previous points and splicing out IDs at the removed indices) so remaining
IDs keep their original order; keep usage of pointIdsRef.current and
createPointId for generating new IDs when length grows, and ensure
ensurePointIds still returns a length-matching array.
🧹 Nitpick comments (7)
packages/ui/src/BezierCurveEditor/BezierCurve.tsx (1)

86-88: controlPoint: null contradicts IBezierPoint type definition.

IBezierPoint.controlPoint is typed as IPoint (non-nullable), but this code assigns null. This is a pre-existing pattern used elsewhere (e.g., helper.ts), but now is a good time to fix the type to controlPoint: IPoint | null so it accurately reflects runtime usage.

packages/ui/src/BezierCurveEditor/BezierCurveEditor.tsx (4)

146-156: Duplicated Y-tick computation between BezierCurveEditor.tsx and Grid.tsx.

Lines 148-154 here replicate the same tick value calculation that Grid.tsx performs in renderYAxisTicks (lines 138-149 in Grid.tsx). Both compute worldY, normalize, scale by yTickScale, and then derive aligned precision. If the formula changes in one place, the other could drift out of sync.

Consider extracting the shared tick-value generation into a helper in tick.ts or passing the pre-computed display value from Grid via a callback, so the editor only needs yScaleDisplayValue without recomputing the full tick array.


228-252: Confusing variable shadowing in handlePointChange map callback.

On line 236-237, the .map((point, index) => ...) callback parameter point shadows the outer scope and is an IBezierPoint, while the mapped object's .point field also holds an IBezierPoint. This creates .point.point.x semantics that are hard to follow.

♻️ Suggested rename for clarity
-      const nextItems = prevPoints.map((point, index) => ({
-        point: index === targetIndex ? clampedPoint : point,
+      const nextItems = prevPoints.map((existingPoint, index) => ({
+        point: index === targetIndex ? clampedPoint : existingPoint,
         pointId: pointIds[index],
         order: index,
       }))

436-444: Live onChange dispatches handleYTickScaleChange on every keystroke.

While valid numbers trigger immediate scale updates (providing live preview), this means typing "1.5" fires changes for "1", "1.", and "1.5" in sequence. The intermediate value "1." would parse as 1 via Number("1."), causing a potentially redundant update. This is a deliberate UX trade-off but could cause unnecessary re-renders in the parent. Consider debouncing or only committing on blur/Enter if live preview isn't essential.


352-355: Reset effect uses primitive deps — correct but fragile.

defaultOffset is a fresh object each render, so using [defaultOffset.x, defaultOffset.y, defaultZoom] as deps is correct. However, if getDefaultOffset ever returns additional properties that affect behavior, this won't pick them up. A memoized defaultOffset via useMemo would be more robust.

♻️ Memoize defaultOffset
- const defaultOffset = yRangeMode === 'positive' ? { x: 0, y: -height } : getDefaultOffset(width, height)
+ const defaultOffset = React.useMemo(
+   () => (yRangeMode === 'positive' ? { x: 0, y: -height } : getDefaultOffset(width, height)),
+   [yRangeMode, width, height]
+ )

Then the effect deps can be simplified to [defaultOffset, defaultZoom].

packages/ui/src/BezierCurveEditor/index.tsx (1)

115-120: JSON.parse without try-catch could crash on malformed localStorage data.

If the stored value is corrupted or not valid JSON, JSON.parse will throw and break the component on mount. This is pre-existing code but worth hardening since it's in the same component being modified.

🛡️ Suggested defensive parse
  useEffect(() => {
    const getLocalPresets = localStorage.getItem(LocalStorageKey)
    if (getLocalPresets) {
-     setPresets(JSON.parse(getLocalPresets))
+     try {
+       setPresets(JSON.parse(getLocalPresets))
+     } catch {
+       // Corrupted data — ignore
+     }
    }
  }, [])
packages/ui/src/BezierCurveEditor/Point.tsx (1)

107-134: useEffect dependency array [moving] omits startPos and onPointChange.

The closure captures stale startPos and onPointChange if they change while moving is true. In practice, startPos is set once before moving flips (React 18 batching ensures the effect sees the updated value), and onPointChange is stable during a drag. However, adding a ref for onPointChange would be strictly safer.

This is a pre-existing pattern; no action needed for this PR.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces significant enhancements to the BezierCurveEditor component, focusing on optimization and improved user interaction capabilities. The changes transform the editor from a basic curve editing tool into a more sophisticated component with configurable Y-axis scaling, improved zoom mechanics with anchor preservation, and enhanced visual feedback through hover labels.

Changes:

  • Added Y-axis range mode selection (symmetric vs. positive) and configurable Y-tick scaling with interactive input
  • Implemented anchor-based zoom that preserves the point under the cursor when zooming
  • Enhanced point interaction with hover labels showing coordinates and visual improvements to prevent text selection during drag

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
packages/ui/src/BezierCurveEditor/types.ts Updated interface to remove deprecated props (draggable, pan, grid) and add new Y-axis configuration props (yTickScale, yRangeMode, onYTickScaleChange, etc.) and simplified axisLabel from object to string
packages/ui/src/BezierCurveEditor/tick.ts New utility module providing tick precision calculation, formatting, and epsilon-based equality checks for improved axis label rendering
packages/ui/src/BezierCurveEditor/index.tsx Added support for new scale props, fixed ref initialization, corrected axis label from "timer" to "time", and added onOpenAutoFocus handler
packages/ui/src/BezierCurveEditor/Point.tsx Enhanced with hover state management, coordinate labels on hover/drag, improved event handling with selection clearing, and clip path support for boundary constraints
packages/ui/src/BezierCurveEditor/Grid.tsx Refactored tick rendering with proper precision alignment, added overlay support for rendering elements above the grid, improved clip path management with unique IDs, and optimized grid line rendering to avoid duplicates
packages/ui/src/BezierCurveEditor/BezierPoint.tsx Changed from index-based to ID-based point tracking, added hover label and clip path support, and improved prop consistency
packages/ui/src/BezierCurveEditor/BezierCurveEditor.tsx Major refactoring with anchor-based zoom implementation, point ID management for stable rendering, Y-scale input UI, removed pan/drag functionality, changed default zoom limits from [0.5, 2] to [0.2, 1], and added point boundary snapping
packages/ui/src/BezierCurveEditor/BezierCurve.tsx Cleaned up unused refs and state, fixed typo in getCursorPoint function name, improved temporary point preview behavior
Comments suppressed due to low confidence (2)

packages/ui/src/BezierCurveEditor/Point.tsx:134

  • The useEffect hook is missing onPointChange in its dependency array. Since onPointChange is passed as a prop and used inside the effect's mousemove handler (line 118), this could cause stale closure issues where the handler calls an outdated version of the callback.

Add onPointChange to the dependency array on line 134.

  useEffect(() => {
    const onViewMouseUp = () => {
      setMoving(false)
    }

    const onViewMouseMove = (event: MouseEvent) => {
      if (moving) {
        event.preventDefault()
        const { clientX, clientY } = event
        const deltaX = clientX - startPos.x
        const deltaY = clientY - startPos.y
        onPointChange({ x: deltaX, y: deltaY })
      }
    }

    if (moving) {
      window.addEventListener('mouseup', onViewMouseUp)
      window.addEventListener('mousemove', onViewMouseMove)
    } else {
      window.removeEventListener('mouseup', onViewMouseUp)
      window.removeEventListener('mousemove', onViewMouseMove)
    }

    return () => {
      window.removeEventListener('mouseup', onViewMouseUp)
      window.removeEventListener('mousemove', onViewMouseMove)
    }
  }, [moving])

packages/ui/src/BezierCurveEditor/Point.tsx:134

  • The useEffect hook for handling mouse move and mouse up events has a missing dependency: startPos. The mousemove handler uses startPos.x and startPos.y (lines 116-117), but startPos is not included in the dependency array. This means the handler will use stale values of startPos when it's called, resulting in incorrect delta calculations when dragging a point.

Add startPos to the dependency array on line 134, or restructure the code to capture startPos in a ref to avoid unnecessary effect re-runs.

  useEffect(() => {
    const onViewMouseUp = () => {
      setMoving(false)
    }

    const onViewMouseMove = (event: MouseEvent) => {
      if (moving) {
        event.preventDefault()
        const { clientX, clientY } = event
        const deltaX = clientX - startPos.x
        const deltaY = clientY - startPos.y
        onPointChange({ x: deltaX, y: deltaY })
      }
    }

    if (moving) {
      window.addEventListener('mouseup', onViewMouseUp)
      window.addEventListener('mousemove', onViewMouseMove)
    } else {
      window.removeEventListener('mouseup', onViewMouseUp)
      window.removeEventListener('mousemove', onViewMouseMove)
    }

    return () => {
      window.removeEventListener('mouseup', onViewMouseUp)
      window.removeEventListener('mousemove', onViewMouseMove)
    }
  }, [moving])

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
packages/ui/src/BezierCurveEditor/Point.tsx (2)

89-91: Ref assignment during render is discouraged in React 18 concurrent mode.

Assigning onPointChangeRef.current in the render body can be called more than once before commit in concurrent features. Since it's only read from event handlers, this is unlikely to cause real bugs here, but the safer pattern is to sync it in an effect.

♻️ Optional: move ref sync into useEffect
  const onPointChangeRef = useRef(onPointChange)
- onPointChangeRef.current = onPointChange
+
+ useEffect(() => {
+   onPointChangeRef.current = onPointChange
+ }, [onPointChange])

130-141: Hover label background width is approximated by character count.

hoverLabel.length * 6 + 8 assumes ~6px per character at 10px font size. This can under- or over-size the background for variable-width characters, wide glyphs, or non-Latin text. If pixel-perfect sizing matters, consider measuring via getBBox() on the <text> element or using a monospace font for the label.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
packages/ui/src/BezierCurveEditor/Preset.tsx (1)

127-143: ⚠️ Potential issue | 🟠 Major

genPath called with potentially undefined points.

points is declared as optional (points?: IPoint[]) in PresetItemProps (Line 118), but genPath expects IPoint[]. On Line 143, genPath(points) will pass undefined when no points prop is provided, causing a runtime error at points.length.

🐛 Proposed fix
-        <PresetPath d={genPath(points)} />
+        <PresetPath d={points ? genPath(points) : ''} />
packages/ui/src/BezierCurveEditor/CurveAnimation.tsx (1)

42-44: ⚠️ Potential issue | 🟠 Major

Missing algo in useMemo dependency array.

convertBezierPointToPoint uses algo to decide between bezier and linear conversion, but algo is not listed in the deps. If the parent changes algo without changing propPoints, the memoized points will be stale and the animation will use the wrong curve type.

🐛 Proposed fix
   const points = useMemo(() => {
     return convertBezierPointToPoint(propPoints, algo)
-  }, [propPoints])
+  }, [propPoints, algo])
🤖 Fix all issues with AI agents
In `@packages/ui/src/BezierCurveEditor/BezierCurve.tsx`:
- Around line 51-58: getCursorPoint can call matrixTransform(undefined) because
getRoot().getScreenCTM() may return null; guard by capturing const screenCTM =
getRoot().getScreenCTM() and only call matrixTransform(screenCTM.inverse()) when
screenCTM is non-null. If screenCTM is null, return a sensible fallback (e.g.,
convert event.clientX/clientY to SVG space by returning { x: event.clientX, y:
event.clientY } or the untransformed SVGPoint) and keep using pointerRef and
getRoot().createSVGPoint() as before; update the getCursorPoint function to
perform this null-check before calling inverse()/matrixTransform.
🧹 Nitpick comments (6)
packages/ui/src/BezierCurveEditor/Grid.tsx (1)

105-106: Consider memoizing tick transform functions.

getNormalizedTickY and getScaledTickY are recreated every render. Since they capture height, axisYScale, zoom, and yTickScale, they could be wrapped in useCallback or extracted as plain utility calls to avoid allocating new closures on each render. This is a minor optimization given the component's scope.

packages/ui/src/BezierCurveEditor/CurveAnimation.tsx (1)

46-54: useImperativeHandle captures stale closures of startTransition and stopTransition.

The dependency array [playing, points] doesn't include startTransition or stopTransition, which close over points, algo, circleRef, rafRef, and progressRef. When points changes (which is already in deps), the handle is recreated, so startTransition picks up the latest points. However, playing in getIsPlaying returns a potentially stale snapshot because setPlaying is async — getIsPlaying will lag behind the actual state. This is a known pattern limitation and likely acceptable here.

packages/ui/src/BezierCurveEditor/Point.tsx (1)

128-140: Hover label width estimation is fragile for non-monospace fonts.

The width hoverLabel.length * 6 + 8 assumes ~6px per character. This works for digits and ASCII but could clip or overflow with wider characters. Consider using a <text> getBBox() measurement or a slightly more generous multiplier if the label might contain wider content.

packages/ui/src/BezierCurveEditor/BezierCurveEditor.tsx (3)

219-243: handlePointChange wraps every point in a new object even for unchanged indices.

Line 228 always creates { point, pointId, order } for every element, but only the target index changes. The point field for non-target indices just passes through the existing item, which is fine functionally, but the intermediate objects and the subsequent .map on Line 242 to unwrap them add allocation overhead on every drag frame.

♻️ Optional: avoid wrapping unchanged points
   const handlePointChange = (pointId: string, bezierPoint: IBezierPoint) => {
     setPoints((prevPoints) => {
       if (!prevPoints) return prevPoints
       const pointIds = ensurePointIds(prevPoints.length)
       const targetIndex = pointIds.indexOf(pointId)
       if (targetIndex === -1) return prevPoints
 
       const clampedPoint = clampBezierPoint(bezierPoint)
-      const nextItems = prevPoints.map((point, index) => ({
-        point: index === targetIndex ? clampedPoint : point,
-        pointId: pointIds[index],
-        order: index,
-      }))
-
-      nextItems.sort((a, b) => {
-        const deltaX = a.point.point.x - b.point.point.x
-        if (Math.abs(deltaX) <= TICK_EPSILON) {
-          return a.order - b.order
-        }
-        return deltaX
-      })
-
-      pointIdsRef.current = nextItems.map((item) => item.pointId)
-      return nextItems.map((item) => item.point)
+      const nextPoints = prevPoints.slice()
+      nextPoints[targetIndex] = clampedPoint
+      const nextIds = pointIds.slice()
+
+      // Build index array and sort by x, stable by original order
+      const indices = nextPoints.map((_, i) => i)
+      indices.sort((a, b) => {
+        const deltaX = nextPoints[a].point.x - nextPoints[b].point.x
+        return Math.abs(deltaX) <= TICK_EPSILON ? a - b : deltaX
+      })
+
+      pointIdsRef.current = indices.map((i) => nextIds[i])
+      return indices.map((i) => nextPoints[i])
     })
   }

416-435: Y-scale input fires onYTickScaleChange on every keystroke.

The onChange handler (Line 427–435) calls handleYTickScaleChange for every valid numeric input while the user is still typing. This triggers the external callback on each keypress, which could cause undesirable side effects if the parent does expensive work in onYTickScaleChange. Consider debouncing or only applying changes on commit (blur/Enter).


452-462: Zoom button labels appear swapped — IconZoomInFilled zooms out visually.

Line 454: zoom - 0.05 makes the zoom value smaller (more zoomed in/detailed), but uses IconZoomInFilled. Line 460: zoom + 0.05 makes the zoom value larger (more zoomed out/wider view), and uses IconZoomOutFilled.

Given that a lower zoom value means more detail (since denormalizeZoom = 1 / zoom), the icons and their actions are actually correctly pairedZoomIn gives more detail, ZoomOut gives less. However, the naming is counterintuitive because zoom - 0.05 reads as "decreasing zoom." A comment clarifying this inverted relationship would help future readers.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@packages/ui/src/BezierCurveEditor/BezierCurveEditor.tsx`:
- Around line 408-425: The Escape behavior is broken because onChange calls
handleYTickScaleChange every keystroke and the blur/commit path
(commitYScaleInput) reads a stale value when setYScaleInputText is batched; to
fix, stop committing live updates from onChange (remove handleYTickScaleChange
from the onChange handler) and only call handleYTickScaleChange from
commitYScaleInput (on blur/Enter) using a separate “original/committed” value
(e.g., store yScaleDisplayValue or committedYScale in state/ref), and add an
escapeFlag ref that onKeyDown Escape sets (and also resets the input via
setYScaleInputText(yScaleDisplayValue)) so commitYScaleInput checks the flag and
skips committing or explicitly reverts by calling
handleYTickScaleChange(committedYScale) when Escape is pressed.
- Around line 453-459: The reset ActionButton currently only checks zoom ===
defaultZoom, so it stays disabled when offset has drifted; update the disabled
prop on the ActionButton used with setZoom and setOffset to be true only when
both zoom equals defaultZoom AND offset equals defaultOffset (use a stable
equality check for offset, e.g., compare offset.x and offset.y or add a small
areOffsetsEqual(offset, defaultOffset) helper) so the button becomes enabled
whenever either zoom or offset differs from their defaults.
🧹 Nitpick comments (2)
packages/ui/src/BezierCurveEditor/BezierCurveEditor.tsx (2)

343-343: Mutating a ref during render body.

ensurePointIds mutates pointIdsRef.current when called at line 343 during the render phase. In React Strict Mode (and future concurrent features), the render function may be invoked more than once, which would increment pointIdCounterRef multiple times. Consider moving this to an effect or memoizing with useMemo.


278-296: Wheel handler re-registers on every zoom change.

handleWheel depends on zoom (via closure and applyZoomAtAnchor), causing the useEffect to remove and re-add the wheel listener on every zoom step. This works correctly but adds overhead during continuous scrolling. A common optimization is to store zoom in a ref and read from that inside the handler, allowing a stable handleWheel reference.

Also applies to: 323-332

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Fix all issues with AI agents
In `@packages/ui/src/BezierCurveEditor/BezierCurveEditor.tsx`:
- Around line 364-380: The JSX accesses points without a null guard
(points.map(...)) inside the BezierCurveEditor render, which can throw if points
is undefined; change the map to use a safe access such as (points ??
[]).map(...) or points?.map(...) and keep the existing pointIds[index]! usage,
or add an explicit assertion earlier so points is non-null before rendering;
update the mapping expression in the BezierCurveEditor component to use the
null-safe form to prevent runtime errors.

In `@packages/ui/src/BezierCurveEditor/helper.ts`:
- Around line 3-8: Update the IBezierPoint type to make controlPoint nullable
(IPoint | null) so its declaration matches usage, then adjust any code that
assumes non-null by adding null-safe handling: update the IBezierPoint
declaration, ensure scalePoints and convertPointsToBezierPoints continue
returning controlPoint as null when appropriate, and review usages such as
generateCurve and BezierCurve.tsx (where controlPoint.x/.y are accessed) to add
null checks or early returns/guards for null controlPoint so TypeScript enforces
safety.

In `@packages/ui/src/BezierCurveEditor/Point.tsx`:
- Around line 97-105: toSvgPoint can call matrixTransform(undefined) when
getRootRef.current().getScreenCTM() returns null; update to first retrieve the
CTM into a variable (e.g., const ctm = getRootRef.current().getScreenCTM()),
check if ctm is truthy before calling ctm.inverse(), and only call
pointerRef.current.matrixTransform with the inverse when present; if ctm is
null/undefined, return a safe fallback (e.g., raw client coords mapped to an SVG
point or an identity transform) so matrixTransform never receives undefined.
Ensure you modify the toSvgPoint function and reference pointerRef and
getRootRef accordingly.
🧹 Nitpick comments (3)
packages/ui/src/BezierCurveEditor/helper.ts (1)

82-94: normalizePoint operator precedence is correct but could be clearer.

Line 90: point.x / scaleX * zoom evaluates as (point.x / scaleX) * zoom which is the intended inverse of denormalizePoint's point.x * scaleX * zoom (given that denormalize uses 1/zoom and normalize uses zoom). However, adding parentheses would make the intent more explicit.

♻️ Optional: clarify with parentheses
-      x: point.x / scaleX * zoom,
-      y: -point.y / scaleY * zoom
+      x: (point.x / scaleX) * zoom,
+      y: (-point.y / scaleY) * zoom
packages/ui/src/BezierCurveEditor/Grid.tsx (1)

172-194: Returning null from .map() leaves gaps in the React key sequence.

When x is near the left border, null is returned but key={index} is used for rendered elements. This works functionally but produces a sparse key sequence (e.g., keys 0, 2, 3…). Consider using filter before map, or using a Fragment with a stable key for the null case — though this is a very minor nit.

packages/ui/src/BezierCurveEditor/BezierCurveEditor.tsx (1)

250-268: applyZoomAtAnchor captures offset and zoom in its closure — potential stale state with rapid scroll events.

Both offset and zoom are in the dependency array, so the callback is recreated on every zoom/offset change. However, handleWheel also depends on zoom (Line 284: zoom + zoomSpeed), and between rapid wheel events the state may not have flushed. This could cause anchor drift during fast scrolling.

Consider using functional updates or refs for zoom/offset to avoid stale closures during rapid interactions.

@GuoLei1990 GuoLei1990 changed the title WIP: Optimization Curve Optimization Curve Feb 16, 2026
…rticleProps and pass scale properties to BezierCurveEditor
@GuoLei1990 GuoLei1990 changed the title Optimization Curve feat(BezierCurveEditor): add Y-axis scale input, positive range mode & bug fixes Feb 16, 2026
@GuoLei1990 GuoLei1990 self-assigned this Feb 16, 2026
@GuoLei1990 GuoLei1990 changed the title feat(BezierCurveEditor): add Y-axis scale input, positive range mode & bug fixes feat(BezierCurveEditor): add Y-axis scale input, positive range mode, extension lines & bug fixes Feb 18, 2026
@GuoLei1990 GuoLei1990 changed the title feat(BezierCurveEditor): add Y-axis scale input, positive range mode, extension lines & bug fixes BezierCurveEditor add Y-axis scale input, positive range mode, extension lines & bug fixes Feb 18, 2026
@GuoLei1990 GuoLei1990 requested a review from MrKou47 February 18, 2026 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants