Merged
Conversation
…xtensions Co-authored-by: Copilot <[email protected]>
WalkthroughAdds a complete chart annotations subsystem: types, utilities, rendering components, snapping and manager hooks, an annotation UI controller with portal integration into Recharts devtools, documentation, and a package.json version/dependency bump. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as AnnotationsController
participant Manager as useAnnotationsManager
participant Render as RenderAnnotations
participant Chart as Chart/Blanket
participant Snap as useSnap
User->>UI: start annotation (type)
UI->>Manager: startAddingAnnotation(type)
Manager->>Render: isAdding = true
Render->>Chart: render Blanket overlay
User->>Chart: mouse move
Chart->>Snap: onMouseMove(event)
Snap->>Manager: followerPosition (SnapResult)
Manager->>Render: update preview
alt two-point type
User->>Chart: first click
Chart->>Manager: onChartClick(snap)
Manager->>Manager: set firstClickPosition
User->>Chart: second click
Chart->>Manager: onChartClick(snap)
Manager->>Manager: create and append annotation
else single-point type
User->>Chart: click
Chart->>Manager: onChartClick(snap)
Manager->>Manager: create annotation
end
Manager->>UI: annotations updated
UI->>User: show annotation in list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
43-50:⚠️ Potential issue | 🟠 MajorAlign the declared dependency floor with the runtime you now test against.
This block targets React 19 and Recharts 3.8 in development, but it type-checks against
@types/react/@types/react-dom18 and advertisesrecharts3.3 as a valid peer. The@typesmismatch will hide React-19-specific typing issues locally, and the Recharts floor is too low—the code importsDefaultZIndexesandZIndexLayerwhich require Recharts 3.4+.Suggested dependency alignment
- "@types/react": "^18.0.0", - "@types/react-dom": "^18.0.0", + "@types/react": "^19.0.0", + "@types/react-dom": "^19.0.0",- "recharts": "^3.3.0" + "recharts": "^3.4.0"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 43 - 50, Update package.json so dev/runtime and peer dependency floors match what you actually test: bump peerDependencies "react" and "react-dom" to include ^19.0.0, align devDependencies/@types to React 19 types (replace or upgrade `@types/react` and `@types/react-dom` to ^19), and raise the peer "recharts" floor to at least ^3.4.0 (since the code imports DefaultZIndexes and ZIndexLayer which require Recharts 3.4+); ensure the "dependencies"/"devDependencies" sections consistently reflect these versions.
🧹 Nitpick comments (8)
src/annotations/getDistance.ts (1)
1-1: Use a type-only import here.
SnapResultis only used in type positions, soimport typekeeps this dependency purely compile-time and avoids a needless runtime edge touseSnap.jsunder TS emit modes that preserve ESM imports.Proposed fix
-import { SnapResult } from './useSnap.js'; +import type { SnapResult } from './useSnap.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/getDistance.ts` at line 1, Replace the runtime import of SnapResult with a type-only import to avoid bringing in './useSnap.js' at runtime: change the import statement that references SnapResult in src/annotations/getDistance.ts to a type-only import (import type { SnapResult } from './useSnap.js') so SnapResult is used only at compile-time; keep all existing usages of SnapResult in type positions unchanged.DEVELOPING.md (1)
3-8: Add a preflight step before tagging and publishing.Right now this flow can publish a broken release if tests, type-checking, or the build are already failing locally. I'd add an explicit
npm test && npm run type-check && npm run buildstep beforenpm run release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DEVELOPING.md` around lines 3 - 8, Update the release flow in DEVELOPING.md to require a preflight check before running npm run release: add an explicit step that runs npm test && npm run type-check && npm run build (or the equivalent CI-local scripts) and only proceed to update package.json/version and run npm run release if those commands succeed; reference the existing npm run release step and the package.json version bump so readers run the preflight checks first to prevent publishing broken releases.src/components/RechartsDevtools.tsx (1)
68-68: Consider cleaner pattern for conditional destructuring.The
selectedInspector ?? {}pattern works at runtime due to the guard at lines 76-77, but it's slightly misleading sinceInspectorDefrequiresInspectorto be non-optional. A more explicit approach would improve clarity:♻️ Suggested refactor
- const { Inspector, Overlay } = selectedInspector ?? {}; + const Inspector = selectedInspector?.Inspector; + const Overlay = selectedInspector?.Overlay;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RechartsDevtools.tsx` at line 68, The conditional destructuring "const { Inspector, Overlay } = selectedInspector ?? {};" is misleading because Inspector is required on InspectorDef; change to an explicit, clearer pattern: either early-return when selectedInspector is falsy (e.g., if (!selectedInspector) return null) and then safely destructure "const { Inspector, Overlay } = selectedInspector;", or use optional property access "const Inspector = selectedInspector?.Inspector; const Overlay = selectedInspector?.Overlay;" so types match InspectorDef and intent is explicit; update usages accordingly (references to Inspector, Overlay) to assume non-null only after the guard or handle undefined when using the optional-access approach.src/components/RechartsAnnotations.tsx (1)
32-35: Portal container lookup may fail if container mounts after this component.The
useEffectruns once whenportalIdchanges, but ifRechartsAnnotationsPortalrenders afterRechartsAnnotations,document.getElementByIdreturnsnulland the controller UI never appears. Consider using a retry mechanism orMutationObserver.♻️ Proposed fix using interval-based retry
React.useEffect(() => { - const el = document.getElementById(portalId); - setContainer(el); + const findContainer = () => { + const el = document.getElementById(portalId); + if (el) { + setContainer(el); + return true; + } + return false; + }; + + if (!findContainer()) { + // Retry a few times in case portal mounts after this component + const interval = setInterval(() => { + if (findContainer()) { + clearInterval(interval); + } + }, 100); + return () => clearInterval(interval); + } }, [portalId]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/RechartsAnnotations.tsx` around lines 32 - 35, The portal container lookup in RechartsAnnotations currently calls document.getElementById(portalId) once in the useEffect and can miss containers that mount later; modify the effect in the RechartsAnnotations component so it retries until the element exists (e.g., periodic check with setInterval) or uses a MutationObserver to watch for DOM additions, setContainer when found, and clean up the interval/observer on unmount; ensure the effect still depends on portalId and handles switching ids by clearing previous observers/intervals and removing listeners to avoid leaks.src/annotations/AnnotationsController.tsx (1)
233-276: StalesnappedCoordinateanddataPointwhen editingpointBcoordinates.When updating
pointB.interactionCoordinate, the code preserves the existingsnappedCoordinateanddataPointvalues (lines 242-243, 263-264). These values will become stale and inconsistent with the new pixel coordinates. If these fields are used elsewhere (e.g., for data-mode rendering), the annotation may render incorrectly.Consider clearing
snappedCoordinateanddataPointwhen manually editing coordinates, or recalculating them:♻️ Proposed fix
pointB: { - snappedCoordinate: annotation.pointB?.snappedCoordinate, - dataPoint: annotation.pointB?.dataPoint, + snappedCoordinate: undefined, + dataPoint: undefined, interactionCoordinate: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/AnnotationsController.tsx` around lines 233 - 276, The pointB coordinate input handlers in AnnotationsController.tsx currently copy existing snappedCoordinate and dataPoint into the updated annotation, which makes them stale; update the onChange logic for the X2/Y2 inputs (the handlers that call onUpdateAnnotation) to either clear snappedCoordinate and dataPoint (set them to undefined/null) or trigger recalculation before calling onUpdateAnnotation so those fields reflect the new interactionCoordinate; specifically modify the update object you pass to onUpdateAnnotation for pointB so snappedCoordinate and dataPoint are not preserved (or are recomputed) whenever interactionCoordinate.x or .y changes.src/annotations/Crosshair.tsx (1)
88-115: Label positioning may clip at chart boundaries.The hardcoded offsets (
x + 5,y - 25) don't account for proximity to chart edges. Labels near the right or top edges may be clipped. This is acceptable for an initial implementation but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/Crosshair.tsx` around lines 88 - 115, The label rectangle and text (rendered when showLabels is true) use fixed offsets from xPixel and yPixel (xPixel + 5, yPixel - 25) which can be clipped at chart edges; update the positioning logic in the Crosshair component to clamp or flip the label when it would overflow the chart bounds by using textWidth and the chart's width/height to compute a safe x and y (e.g., shift left if xPixel + textWidth + padding > chartWidth, shift down if yPixel - labelHeight < 0), and apply those computed coordinates to the rect and text elements that render coordinateText so labels remain fully visible near edges.src/annotations/CircleAnnotation.tsx (1)
36-38: Redundant null check forpointB.
pointBis already checked on line 29 with an early return. The!pointBcondition on line 36 will always befalseat this point.♻️ Simplified check
if (positionType === 'data') { // Use ReferenceDot for data-based positioning - if (!pointA.dataPoint || !pointB) { + if (!pointA.dataPoint) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/CircleAnnotation.tsx` around lines 36 - 38, The conditional in the CircleAnnotation component redundantly checks pointB even though pointB is already validated earlier; update the early-return condition to only check pointA.dataPoint (remove the `!pointB` clause) so the code becomes `if (!pointA.dataPoint) return null;`, keeping the existing flow in the CircleAnnotation function/component and preserving prior early-return logic that validated pointB.src/annotations/useSnap.ts (1)
64-65: Typo in variable names:Scapeshould beScale.
xInverseTickSnapScapeandyInverseTickSnapScapeshould bexInverseTickSnapScaleandyInverseTickSnapScalefor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/useSnap.ts` around lines 64 - 65, Rename the misspelled variables xInverseTickSnapScape and yInverseTickSnapScape to xInverseTickSnapScale and yInverseTickSnapScale everywhere they are declared and referenced (these variables receive the return values of the hooks useXAxisInverseTickSnapScale and useYAxisInverseTickSnapScale); update any usages, destructuring, exports, or tests that reference the old names to the corrected identifiers to ensure consistency and avoid runtime/reference errors.
🤖 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/annotations/annotationColors.ts`:
- Around line 21-22: getAnnotationColor currently uses index %
ANNOTATION_COLORS.length which yields negative indices for negative inputs;
update getAnnotationColor to normalize the index into the valid 0..length-1
range (e.g. via ((index % length) + length) % length) before indexing
ANNOTATION_COLORS so negative integers return a valid string from the palette
and the function signature remains string.
In `@src/annotations/AnnotationsController.tsx`:
- Around line 352-369: The condition "'text' in annotation" is always false
because Annotation doesn't have a text property; instead check for a label-based
annotation by testing annotation.label (or annotation.label?.value). Update the
rendering condition to something like "annotation.label != null" or "typeof
annotation.label?.value === 'string'", and keep using annotation.label?.value
for the input value and onUpdateAnnotation (e.g., ensure onUpdateAnnotation
updates label.value). Modify the conditional around the input and its onChange
to reference annotation.label (and annotation.label?.value) so the label-edit UI
becomes reachable.
In `@src/annotations/CircleAnnotation.tsx`:
- Around line 32-52: The CircleAnnotation component uses a pixel-space radius r
(computed by getDistance from interactionCoordinate) while rendering
data-positioned ReferenceDot, causing size mismatch; update the data-mode branch
in CircleAnnotation to either convert r into data-space units before passing to
ReferenceDot (compute distance using pointA.dataPoint and pointB.dataPoint) or
render the circle entirely in pixel-space by using interactionCoordinate
positions instead of dataPoint, ensuring consistency between positionType, the
source of coordinates (pointA.dataPoint/pointB.dataPoint vs.
interactionCoordinate), and the radius passed to ReferenceDot.
In `@src/annotations/FreeformLine.tsx`:
- Around line 30-42: The segment coordinates are being passed as strings to the
Recharts ReferenceLine, which expects numeric values; update the JSX in
FreeformLine by removing the String(...) conversions and pass numeric values
directly for the two points (i.e., use pointA.dataPoint.x, pointA.dataPoint.y,
pointB.dataPoint.x, pointB.dataPoint.y) when constructing the segment prop on
the ReferenceLine component so the segment array contains numbers instead of
strings.
In `@src/annotations/getDistance.ts`:
- Around line 3-7: getDistance currently computes distance using
SnapResult.interactionCoordinate (raw pointer click) which causes snapped
annotations to be measured from the click point instead of the actual snapped
positions; update getDistance to use SnapResult.snappedCoordinate.x and .y for
both parameters (a and b) when available (fall back to interactionCoordinate if
snappedCoordinate is undefined) so radii/positions reflect the rendered snap
targets. Ensure you reference the getDistance function and the SnapResult fields
snappedCoordinate and interactionCoordinate when making the change.
In `@src/annotations/HorizontalLine.tsx`:
- Around line 21-35: The block that returns ReferenceLine when positionType ===
'data' does not guard against pointA.dataPoint being undefined; instead of
passing String(pointA.dataPoint?.y) (which can become "undefined"), add a null
check similar to Crosshair.tsx: if pointA.dataPoint is missing, return null;
otherwise pass the actual numeric y (or its string) to ReferenceLine. Update the
condition around positionType === 'data' to first verify pointA.dataPoint exists
before rendering ReferenceLine (referencing positionType, pointA.dataPoint, and
ReferenceLine).
In `@src/annotations/RenderAnnotations.tsx`:
- Around line 149-159: The handleBlanketMouseMove callback closes over the snap
function but its dependency array only includes onChartMouseMove, causing a
stale closure when snap (or snapMode) changes; update the dependency array for
handleBlanketMouseMove (or change the memoization key) to include snap (or
snapMode) so the callback is recreated when snapping behavior changes, ensuring
handleBlanketMouseMove calls the current snap implementation.
In `@src/annotations/useAnnotationsManager.ts`:
- Around line 52-55: The id generation in useAnnotationsManager currently uses
Date.now(), which can collide; replace it with a collision-resistant source
(e.g., a monotonic counter stored in a React ref inside useAnnotationsManager
like nextIdRef.current++ or a UUID via crypto.randomUUID()) and return that
value instead of Date.now() where the annotation object is created (the return
object building type/id/positionType). Ensure all operations that rely on the id
(delete, update, selection) continue to use the new id shape.
- Around line 184-206: The callbacks onChartMouseUp and onChartClick reference
snapMode when calling createAnnotation but don't include it in their useCallback
dependency arrays; update both useCallback dependency lists to include snapMode
so the callbacks are recreated when snapMode changes, ensuring the snapMode
passed to createAnnotation matches the SnapResult coordinates (refer to
onChartMouseUp, onChartClick, createAnnotation, and snapMode).
In `@src/annotations/useSnap.ts`:
- Around line 131-138: The callback inside the useSnap hook depends on scaleX
and scaleY but they are missing from the memo/dependency array; update the
dependency array (which currently lists snapMode, xInverseDataSnapScale,
yInverseDataSnapScale, xInverseTickSnapScape, yInverseTickSnapScape) to also
include scaleX and scaleY so the callback recomputes when those scale objects
change.
- Line 89: The call to getRelativeCoordinate is passing a React synthetic event
(e) instead of the native DOM event; change the call where interactionCoordinate
is set to pass the underlying native event (e.nativeEvent) so
getRelativeCoordinate receives the expected HTML/SVG pointer or touch event
type, and update any local typings if necessary to reflect that
getRelativeCoordinate consumes the native event rather than the React synthetic
wrapper.
---
Outside diff comments:
In `@package.json`:
- Around line 43-50: Update package.json so dev/runtime and peer dependency
floors match what you actually test: bump peerDependencies "react" and
"react-dom" to include ^19.0.0, align devDependencies/@types to React 19 types
(replace or upgrade `@types/react` and `@types/react-dom` to ^19), and raise the
peer "recharts" floor to at least ^3.4.0 (since the code imports DefaultZIndexes
and ZIndexLayer which require Recharts 3.4+); ensure the
"dependencies"/"devDependencies" sections consistently reflect these versions.
---
Nitpick comments:
In `@DEVELOPING.md`:
- Around line 3-8: Update the release flow in DEVELOPING.md to require a
preflight check before running npm run release: add an explicit step that runs
npm test && npm run type-check && npm run build (or the equivalent CI-local
scripts) and only proceed to update package.json/version and run npm run release
if those commands succeed; reference the existing npm run release step and the
package.json version bump so readers run the preflight checks first to prevent
publishing broken releases.
In `@src/annotations/AnnotationsController.tsx`:
- Around line 233-276: The pointB coordinate input handlers in
AnnotationsController.tsx currently copy existing snappedCoordinate and
dataPoint into the updated annotation, which makes them stale; update the
onChange logic for the X2/Y2 inputs (the handlers that call onUpdateAnnotation)
to either clear snappedCoordinate and dataPoint (set them to undefined/null) or
trigger recalculation before calling onUpdateAnnotation so those fields reflect
the new interactionCoordinate; specifically modify the update object you pass to
onUpdateAnnotation for pointB so snappedCoordinate and dataPoint are not
preserved (or are recomputed) whenever interactionCoordinate.x or .y changes.
In `@src/annotations/CircleAnnotation.tsx`:
- Around line 36-38: The conditional in the CircleAnnotation component
redundantly checks pointB even though pointB is already validated earlier;
update the early-return condition to only check pointA.dataPoint (remove the
`!pointB` clause) so the code becomes `if (!pointA.dataPoint) return null;`,
keeping the existing flow in the CircleAnnotation function/component and
preserving prior early-return logic that validated pointB.
In `@src/annotations/Crosshair.tsx`:
- Around line 88-115: The label rectangle and text (rendered when showLabels is
true) use fixed offsets from xPixel and yPixel (xPixel + 5, yPixel - 25) which
can be clipped at chart edges; update the positioning logic in the Crosshair
component to clamp or flip the label when it would overflow the chart bounds by
using textWidth and the chart's width/height to compute a safe x and y (e.g.,
shift left if xPixel + textWidth + padding > chartWidth, shift down if yPixel -
labelHeight < 0), and apply those computed coordinates to the rect and text
elements that render coordinateText so labels remain fully visible near edges.
In `@src/annotations/getDistance.ts`:
- Line 1: Replace the runtime import of SnapResult with a type-only import to
avoid bringing in './useSnap.js' at runtime: change the import statement that
references SnapResult in src/annotations/getDistance.ts to a type-only import
(import type { SnapResult } from './useSnap.js') so SnapResult is used only at
compile-time; keep all existing usages of SnapResult in type positions
unchanged.
In `@src/annotations/useSnap.ts`:
- Around line 64-65: Rename the misspelled variables xInverseTickSnapScape and
yInverseTickSnapScape to xInverseTickSnapScale and yInverseTickSnapScale
everywhere they are declared and referenced (these variables receive the return
values of the hooks useXAxisInverseTickSnapScale and
useYAxisInverseTickSnapScale); update any usages, destructuring, exports, or
tests that reference the old names to the corrected identifiers to ensure
consistency and avoid runtime/reference errors.
In `@src/components/RechartsAnnotations.tsx`:
- Around line 32-35: The portal container lookup in RechartsAnnotations
currently calls document.getElementById(portalId) once in the useEffect and can
miss containers that mount later; modify the effect in the RechartsAnnotations
component so it retries until the element exists (e.g., periodic check with
setInterval) or uses a MutationObserver to watch for DOM additions, setContainer
when found, and clean up the interval/observer on unmount; ensure the effect
still depends on portalId and handles switching ids by clearing previous
observers/intervals and removing listeners to avoid leaks.
In `@src/components/RechartsDevtools.tsx`:
- Line 68: The conditional destructuring "const { Inspector, Overlay } =
selectedInspector ?? {};" is misleading because Inspector is required on
InspectorDef; change to an explicit, clearer pattern: either early-return when
selectedInspector is falsy (e.g., if (!selectedInspector) return null) and then
safely destructure "const { Inspector, Overlay } = selectedInspector;", or use
optional property access "const Inspector = selectedInspector?.Inspector; const
Overlay = selectedInspector?.Overlay;" so types match InspectorDef and intent is
explicit; update usages accordingly (references to Inspector, Overlay) to assume
non-null only after the guard or handle undefined when using the optional-access
approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e97ddc0-149a-4cd1-a754-8f3049746b8e
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
DEVELOPING.mdpackage.jsonsrc/annotations/AnnotationsController.tsxsrc/annotations/Blanket.tsxsrc/annotations/CircleAnnotation.tsxsrc/annotations/Crosshair.tsxsrc/annotations/FreeformLine.tsxsrc/annotations/HorizontalLine.tsxsrc/annotations/LabelAnnotation.tsxsrc/annotations/RectangleAnnotation.tsxsrc/annotations/RenderAnnotations.tsxsrc/annotations/VerticalLine.tsxsrc/annotations/annotationColors.tssrc/annotations/freeform/README.mdsrc/annotations/getDistance.tssrc/annotations/index.tssrc/annotations/snap/README.mdsrc/annotations/types.tssrc/annotations/useAnnotationsManager.tssrc/annotations/useSnap.tssrc/components/RechartsAnnotations.tsxsrc/components/RechartsDevtools.tsxsrc/constants.tssrc/context/RechartsDevtoolsContext.tsxsrc/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/annotations/useSnap.ts (3)
60-73: Return type| undefinedis never used.The function signature declares
SnapResult | undefinedas the return type, but the callback always returns aSnapResultobject (neverundefined). Consider simplifying to justSnapResultunless you intend to add cases that returnundefined.✨ Proposed fix
export function useSnap( snapMode: SnapMode, -): (e: MouseEvent<SVGGraphicsElement>) => SnapResult | undefined { +): (e: MouseEvent<SVGGraphicsElement>) => SnapResult {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/useSnap.ts` around lines 60 - 73, The declared return type for useSnap currently includes | undefined but the returned callback (created via useCallback) always returns a SnapResult; update the signature of useSnap to return (e: MouseEvent<SVGGraphicsElement>) => SnapResult (remove | undefined) and ensure the useCallback generic/return type and the inner callback parameter types (the React.MouseEvent signature) match SnapResult as the definite return type; adjust any related type annotations referencing useSnap, SnapMode, or SnapResult accordingly.
66-67: Typo in variable names:Scapeshould beScale.The variable names
xInverseTickSnapScapeandyInverseTickSnapScapeappear to contain a typo. They should likely bexInverseTickSnapScaleandyInverseTickSnapScaleto match the naming convention of the data snap scale variables on lines 64-65.✏️ Proposed fix
- const xInverseTickSnapScape = useXAxisInverseTickSnapScale(); - const yInverseTickSnapScape = useYAxisInverseTickSnapScale(); + const xInverseTickSnapScale = useXAxisInverseTickSnapScale(); + const yInverseTickSnapScale = useYAxisInverseTickSnapScale();Also update references at lines 77-78, 110-111, and 162-163.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/useSnap.ts` around lines 66 - 67, Rename the misspelled variables xInverseTickSnapScape and yInverseTickSnapScape to xInverseTickSnapScale and yInverseTickSnapScale respectively where they are declared from the hooks useXAxisInverseTickSnapScale() and useYAxisInverseTickSnapScale(), and update all usages of those identifiers (the later references that currently use the "Scape" spelling in the same file) so they match the corrected names; ensure you only change the identifier spelling and not the hook calls or surrounding logic.
100-114: Remove unnecessary nested braces.The extra
{ }blocks around thesnappedDataPointassignments serve no purpose and add visual noise.✨ Proposed simplification
if (snapMode === 'data') { - { - snappedDataPoint = { - x: xInverseDataSnapScale(interactionCoordinate.relativeX), - y: yInverseDataSnapScale(interactionCoordinate.relativeY), - }; - } + snappedDataPoint = { + x: xInverseDataSnapScale(interactionCoordinate.relativeX), + y: yInverseDataSnapScale(interactionCoordinate.relativeY), + }; } else if (snapMode === 'tick') { - { - snappedDataPoint = { - x: xInverseTickSnapScape(interactionCoordinate.relativeX), - y: yInverseTickSnapScape(interactionCoordinate.relativeY), - }; - } + snappedDataPoint = { + x: xInverseTickSnapScale(interactionCoordinate.relativeX), + y: yInverseTickSnapScale(interactionCoordinate.relativeY), + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/useSnap.ts` around lines 100 - 114, Remove the unnecessary nested blocks surrounding the snappedDataPoint assignments inside the conditional handling for snapMode in useSnap.ts; specifically edit the if (snapMode === 'data') and else if (snapMode === 'tick') branches that wrap the snappedDataPoint = { ... } assignments in extra { } braces (references: snappedDataPoint, xInverseDataSnapScale, yInverseDataSnapScale, xInverseTickSnapScape, yInverseTickSnapScape) so the assignments are direct and adjust indentation accordingly.
🤖 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/annotations/useSnap.ts`:
- Around line 60-73: The declared return type for useSnap currently includes |
undefined but the returned callback (created via useCallback) always returns a
SnapResult; update the signature of useSnap to return (e:
MouseEvent<SVGGraphicsElement>) => SnapResult (remove | undefined) and ensure
the useCallback generic/return type and the inner callback parameter types (the
React.MouseEvent signature) match SnapResult as the definite return type; adjust
any related type annotations referencing useSnap, SnapMode, or SnapResult
accordingly.
- Around line 66-67: Rename the misspelled variables xInverseTickSnapScape and
yInverseTickSnapScape to xInverseTickSnapScale and yInverseTickSnapScale
respectively where they are declared from the hooks
useXAxisInverseTickSnapScale() and useYAxisInverseTickSnapScale(), and update
all usages of those identifiers (the later references that currently use the
"Scape" spelling in the same file) so they match the corrected names; ensure you
only change the identifier spelling and not the hook calls or surrounding logic.
- Around line 100-114: Remove the unnecessary nested blocks surrounding the
snappedDataPoint assignments inside the conditional handling for snapMode in
useSnap.ts; specifically edit the if (snapMode === 'data') and else if (snapMode
=== 'tick') branches that wrap the snappedDataPoint = { ... } assignments in
extra { } braces (references: snappedDataPoint, xInverseDataSnapScale,
yInverseDataSnapScale, xInverseTickSnapScape, yInverseTickSnapScape) so the
assignments are direct and adjust indentation accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da725650-e9cb-43c7-920f-307ae7add006
📒 Files selected for processing (1)
src/annotations/useSnap.ts
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/annotations/useAnnotationsManager.ts (2)
37-37: Non-English TODO comment.The TODO comment is in Czech. For consistency and team accessibility, consider translating it to English.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/useAnnotationsManager.ts` at line 37, Replace the Czech TODO comment in the useAnnotationsManager hook with an English version for team clarity: locate the inline comment referencing "priste tohle prepis z factory na renderer, aby to umelo jak pixely tak data" inside the useAnnotationsManager file and update it to an English TODO that conveys the same intent (e.g., "TODO: refactor from factory to renderer so it can handle both pixels and data") so the purpose and next steps remain clear to all contributors.
62-98: Remove commented-out code.This large block of dead code should be removed. If it's needed for reference, it can be retrieved from version control history.
♻️ Suggested cleanup
label: undefined, }; - - // switch (type) { - // case 'horizontalLine': - // return { ...base, type: 'horizontalLine', y }; - // case 'verticalLine': - // return { ...base, type: 'verticalLine', x }; - // case 'circle': - // // If x2/y2 provided, calculate radius from distance - // const r = - // x2 !== undefined && y2 !== undefined - // ? Math.sqrt(Math.pow(x2 - x, 2) + Math.pow(y2 - y, 2)) - // : 10; - // return { ...base, type: 'circle', x, y, r }; - // case 'rectangle': - // return { - // ...base, - // type: 'rectangle', - // x1: x, - // y1: y, - // x2: x2 ?? x + 50, - // y2: y2 ?? y + 50, - // }; - // case 'label': - // return { ...base, type: 'label', x, y, text: 'Label' }; - // case 'freeformLine': - // return { - // ...base, - // type: 'freeformLine', - // x1: x, - // y1: y, - // x2: x2 ?? x + 50, - // y2: y2 ?? y + 50, - // }; - // case 'crosshair': - // return { ...base, type: 'crosshair', x, y }; - // default: - // throw new Error(`Unknown annotation type: ${type}`); - // } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/useAnnotationsManager.ts` around lines 62 - 98, Remove the large commented-out switch block that constructs annotation objects (the commented cases for 'horizontalLine', 'verticalLine', 'circle', 'rectangle', 'label', 'freeformLine', 'crosshair' and the default throw) from useAnnotationsManager.ts; delete the entire commented block so no dead code remains, then run a quick lint/format and a local type-check/build to ensure no trailing comment tokens or unused imports remain.src/annotations/useSnap.ts (1)
100-114: Remove unnecessary nested braces.The extra braces at lines 101-106 and 108-113 don't serve any purpose and reduce readability.
♻️ Suggested cleanup
if (snapMode === 'data') { - { - snappedDataPoint = { - x: xInverseDataSnapScale(interactionCoordinate.relativeX), - y: yInverseDataSnapScale(interactionCoordinate.relativeY), - }; - } + snappedDataPoint = { + x: xInverseDataSnapScale(interactionCoordinate.relativeX), + y: yInverseDataSnapScale(interactionCoordinate.relativeY), + }; } else if (snapMode === 'tick') { - { - snappedDataPoint = { - x: xInverseTickSnapScale(interactionCoordinate.relativeX), - y: yInverseTickSnapScale(interactionCoordinate.relativeY), - }; - } + snappedDataPoint = { + x: xInverseTickSnapScale(interactionCoordinate.relativeX), + y: yInverseTickSnapScale(interactionCoordinate.relativeY), + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/useSnap.ts` around lines 100 - 114, The code block in useSnap handling snapMode ('data' and 'tick') contains redundant nested braces around the snappedDataPoint assignments; remove the extra { } wrappers so the assignments run directly under each branch (refer to snapMode, snappedDataPoint, xInverseDataSnapScale, yInverseDataSnapScale, xInverseTickSnapScale, yInverseTickSnapScale) to improve readability without changing behavior.src/annotations/AnnotationsController.tsx (1)
359-363: Potential issue spreadingundefinedlabel.When
annotation.labelisundefined, spreading it with...annotation.labelworks but is unclear. Consider being explicit about the label structure.♻️ Suggested improvement
onChange={(e) => onUpdateAnnotation({ ...annotation, - label: { ...annotation.label, value: e.target.value }, + label: { value: e.target.value }, }) }If
annotation.labelhas other properties that should be preserved, consider using a more explicit merge pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/AnnotationsController.tsx` around lines 359 - 363, The onChange handler currently spreads annotation.label which may be undefined; update the onChange in AnnotationsController to merge explicitly by treating annotation.label as a possibly-missing object (e.g., use a default empty object or construct a new label object) before adding value, so call onUpdateAnnotation with {...annotation, label: { ...(annotation.label || {}), value: e.target.value }} or equivalent to ensure label is always an object; reference the onChange callback that invokes onUpdateAnnotation and the annotation.label field when making this change.
🤖 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/annotations/AnnotationsController.tsx`:
- Around line 359-363: The onChange handler currently spreads annotation.label
which may be undefined; update the onChange in AnnotationsController to merge
explicitly by treating annotation.label as a possibly-missing object (e.g., use
a default empty object or construct a new label object) before adding value, so
call onUpdateAnnotation with {...annotation, label: { ...(annotation.label ||
{}), value: e.target.value }} or equivalent to ensure label is always an object;
reference the onChange callback that invokes onUpdateAnnotation and the
annotation.label field when making this change.
In `@src/annotations/useAnnotationsManager.ts`:
- Line 37: Replace the Czech TODO comment in the useAnnotationsManager hook with
an English version for team clarity: locate the inline comment referencing
"priste tohle prepis z factory na renderer, aby to umelo jak pixely tak data"
inside the useAnnotationsManager file and update it to an English TODO that
conveys the same intent (e.g., "TODO: refactor from factory to renderer so it
can handle both pixels and data") so the purpose and next steps remain clear to
all contributors.
- Around line 62-98: Remove the large commented-out switch block that constructs
annotation objects (the commented cases for 'horizontalLine', 'verticalLine',
'circle', 'rectangle', 'label', 'freeformLine', 'crosshair' and the default
throw) from useAnnotationsManager.ts; delete the entire commented block so no
dead code remains, then run a quick lint/format and a local type-check/build to
ensure no trailing comment tokens or unused imports remain.
In `@src/annotations/useSnap.ts`:
- Around line 100-114: The code block in useSnap handling snapMode ('data' and
'tick') contains redundant nested braces around the snappedDataPoint
assignments; remove the extra { } wrappers so the assignments run directly under
each branch (refer to snapMode, snappedDataPoint, xInverseDataSnapScale,
yInverseDataSnapScale, xInverseTickSnapScale, yInverseTickSnapScale) to improve
readability without changing behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5e5b301-37a2-4b3a-9d10-231932b379ab
📒 Files selected for processing (11)
DEVELOPING.mdpackage.jsonsrc/annotations/AnnotationsController.tsxsrc/annotations/Blanket.tsxsrc/annotations/CircleAnnotation.tsxsrc/annotations/RenderAnnotations.tsxsrc/annotations/annotationColors.tssrc/annotations/getDistance.tssrc/annotations/useAnnotationsManager.tssrc/annotations/useSnap.tssrc/components/RechartsDevtools.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- DEVELOPING.md
- src/components/RechartsDevtools.tsx
- src/annotations/getDistance.ts
- src/annotations/CircleAnnotation.tsx
- package.json
- src/annotations/Blanket.tsx
- src/annotations/RenderAnnotations.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/annotations/useSnap.ts (1)
15-15: Minor inconsistency inMouseEventtype reference.Line 15 imports
MouseEventfrom React, line 62 uses it directly asMouseEvent<SVGGraphicsElement>, but line 73 usesReact.MouseEvent<SVGGraphicsElement>. Consider using one style consistently.♻️ Suggested fix to use imported MouseEvent consistently
- (e: React.MouseEvent<SVGGraphicsElement>): SnapResult => { + (e: MouseEvent<SVGGraphicsElement>): SnapResult => {Also applies to: 62-62, 73-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/annotations/useSnap.ts` at line 15, In useSnap.ts there is an inconsistent type reference for mouse events: you import MouseEvent from React at the top but use both MouseEvent<SVGGraphicsElement> and React.MouseEvent<SVGGraphicsElement> in handlers; pick one style and apply it consistently—either remove the named import and use React.MouseEvent everywhere, or keep the named import and replace all occurrences of React.MouseEvent<SVGGraphicsElement> with MouseEvent<SVGGraphicsElement> (check usages in the mouse event handlers around lines where MouseEvent is referenced).
🤖 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/annotations/useSnap.ts`:
- Line 15: In useSnap.ts there is an inconsistent type reference for mouse
events: you import MouseEvent from React at the top but use both
MouseEvent<SVGGraphicsElement> and React.MouseEvent<SVGGraphicsElement> in
handlers; pick one style and apply it consistently—either remove the named
import and use React.MouseEvent everywhere, or keep the named import and replace
all occurrences of React.MouseEvent<SVGGraphicsElement> with
MouseEvent<SVGGraphicsElement> (check usages in the mouse event handlers around
lines where MouseEvent is referenced).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0b39041-22bd-4d36-b330-d7c9f2cedcc5
📒 Files selected for processing (2)
src/annotations/useAnnotationsManager.tssrc/annotations/useSnap.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
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 by CodeRabbit
New Features
Documentation
Dependencies