Feat/editor/colored image preview - #201
Conversation
…image - Bug: zooming, panning, dragging causes SVG to re-render, removing the ID & causing color to be lost
|
Warning Rate limit exceeded@Ryan-Millard has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughParses incoming SVG into React nodes, introduces a color-mode toggle with per-shape reveal on click, adds wheel zoom and pointer pan with transform state, restructures SVG layout/CSS to viewport/inner, and reduces navigation payload to pass only Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Editor as Editor Component
participant Parser as html-react-parser
participant State as React State
User->>Editor: Open editor route (state `{ svg }`)
Editor->>Parser: parse(svgString)
Parser-->>Editor: React SVG nodes
Editor->>State: init { scale:1, tx:0, ty:0, colorMode:false, colored:[] }
rect rgb(240,248,255)
note right of User: Pan & Zoom
User->>Editor: Wheel event
Editor->>State: set scale = clamp(scale * factor, min, max)
State-->>Editor: re-render with transform(scale,tx,ty)
User->>Editor: Pointer down → move → up (drag)
Editor->>State: update tx, ty
State-->>Editor: re-render with updated translation
end
rect rgb(255,240,245)
note right of User: Color-mode interactions
User->>Editor: Toggle GlassSwitch
Editor->>State: colorMode = !colorMode
User->>Editor: Click on viewport
Editor->>Editor: find nearest shape element
alt colorMode = true
Editor->>State: add shape id/class to colored
State-->>Editor: re-render (shape shows fill)
else
Editor-->>User: hint to enable color-mode
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
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 |
|
@Krasner, I hope this fixes the problem you faced. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Fix all issues with AI Agents 🤖
In @src/pages/Editor/Editor.module.css:
- Around line 56-64: The CSS block contains a duplicated selector ".inner svg
path:hover" which is repeated; remove the duplicate so the selector list reads
".inner svg path:hover, .inner svg rect:hover, .inner svg circle:hover, .inner
svg polygon:hover, .inner svg g:hover { stroke: var(--color-primary);
stroke-width: 0.9; }" ensuring no repeated selectors remain.
- Around line 2-13: The .viewport CSS rule contains a duplicated property:
remove the redundant second "position: relative" declaration inside the
.viewport block so the class only declares position once; update the .viewport
rule in Editor.module.css to keep a single "position: relative" entry and leave
the other properties unchanged.
In @src/pages/Editor/index.jsx:
- Line 1: The import list in Editor/index.jsx includes the unused React hook
useEffect which triggers a lint error; remove useEffect from the import
statement (leaving useRef and useState) or if side-effect logic is intended,
implement it inside the component by referencing useEffect. Update the import
line that currently references useEffect so only actually used hooks (useRef,
useState) are imported.
- Around line 33-41: Remove the unused handleSvgClick function: delete the
entire const handleSvgClick = (e) => { ... } block (which references innerRef
and styles.coloredRegion) since pointer/click logic is now handled in
onPointerUp; also ensure there are no remaining references to handleSvgClick
elsewhere in this module and remove any now-unused imports or variables that
existed solely for it.
- Around line 95-99: The code incorrectly assigns styles.coloredRegion (a
CSS-module hashed string) to shape.id and the CSS expects literal
#coloredRegion; instead use a class or data attribute: update the CSS selector
from :not(#coloredRegion) to :not(.coloredRegion) (or use
[data-colored-region!="true"]), then in the event handler replace shape.id =
styles.coloredRegion with shape.classList.add(styles.coloredRegion) (or
shape.setAttribute('data-colored-region','true')), and ensure you remove the
class/attribute where appropriate; locate usages by SHAPE_SELECTOR, svgRoot, and
styles.coloredRegion to make the coordinated JS/CSS change.
- Line 62: The code calls viewportRef.current.classList.add('grabbing') (and the
corresponding remove at line ~87) which adds a literal class name that won't
match CSS module hashes; replace these with the CSS-module reference e.g. use
viewportRef.current.classList.add(styles.grabbing) and
viewportRef.current.classList.remove(styles.grabbing) (ensure styles is imported
from './Editor.module.css' and that viewportRef.current is non-null before
calling).
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Editor/index.jsx (1)
16-30: Critical: Hooks called conditionally after early return — violates Rules of Hooks.React hooks must be called unconditionally and in the same order on every render. The early return on lines 16-23 causes all hooks on lines 25-30 to be called conditionally, which will break React's state management.
Move all hooks before the guard clause and render the fallback UI conditionally instead.
🔎 Proposed fix
export default function Editor() { const { state } = useLocation(); const { svg } = state || {}; - if (!svg) { - return ( - <GlassCard className="text-center p-8"> - <h2>No SVG data found</h2> - <p>Please upload an image first.</p> - </GlassCard> - ); - } - - const [svgElements] = useState(() => parse(svg)); + const [svgElements] = useState(() => (svg ? parse(svg) : null)); const [isColorMode, setIsColorMode] = useState(true); const viewportRef = useRef(null); const innerRef = useRef(null); const [transform, setTransform] = useState({ scale: 1, tx: 0, ty: 0 }); const pointerState = useRef({ dragging: false, lastX: 0, lastY: 0, moving: false }); + if (!svg) { + return ( + <GlassCard className="text-center p-8"> + <h2>No SVG data found</h2> + <p>Please upload an image first.</p> + </GlassCard> + ); + } + // Handle clicks: robustly find the nearest shape ...
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
package.jsonsrc/components/WasmImageProcessor.jsxsrc/pages/Editor/Editor.module.csssrc/pages/Editor/index.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Editor/index.jsx (4)
src/components/WasmImageProcessor.jsx (1)
step(68-68)src/pages/Editor/EditorHelmet.jsx (1)
EditorHelmet(4-11)src/components/GlassCard.jsx (1)
GlassCard(5-9)src/components/GlassSwitch.jsx (1)
GlassSwitch(5-21)
🪛 Biome (2.1.2)
src/pages/Editor/Editor.module.css
[error] 11-11: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
position is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
src/pages/Editor/index.jsx
[error] 25-25: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 26-26: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 27-27: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 28-28: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 29-29: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 30-30: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
Hooks should not be called after an early return.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🪛 GitHub Actions: CI
src/pages/Editor/index.jsx
[error] 1-1: ESLint: 'useEffect' is defined but never used. (no-unused-vars)
[error] 1-1: eslint: Unused variable detected in lint step. Consider removing or using 'useEffect' in this file.
🪛 GitHub Check: Lint Code
src/pages/Editor/index.jsx
[failure] 1-1:
'useEffect' is defined but never used. Allowed unused vars must match /^[A-Z_]/u
[failure] 33-33:
'handleSvgClick' is assigned a value but never used. Allowed unused vars must match /^[A-Z_]/u
[failure] 30-30:
React Hook "useRef" is called conditionally. React Hooks must be called in the exact same order in every component render
[failure] 29-29:
React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render
[failure] 28-28:
React Hook "useRef" is called conditionally. React Hooks must be called in the exact same order in every component render
[failure] 27-27:
React Hook "useRef" is called conditionally. React Hooks must be called in the exact same order in every component render
[failure] 26-26:
React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render
[failure] 25-25:
React Hook "useState" is called conditionally. React Hooks must be called in the exact same order in every component render
🔇 Additional comments (7)
package.json (1)
164-164: LGTM!The
html-react-parserdependency is a suitable choice for parsing SVG strings into interactive React elements, enabling the click-to-color functionality in the editor.src/components/WasmImageProcessor.jsx (1)
125-127: LGTM!The navigation state is appropriately simplified to pass only
{ svg }, aligning with the Editor's updated data requirements.src/pages/Editor/Editor.module.css (1)
27-53: LGTM!The
.innerwrapper withtransform-origin: 0 0andwill-change: transformis correctly set up for performant pan/zoom transforms. The pointer-events and transition styling for SVG primitives enables smooth interactions.src/pages/Editor/index.jsx (4)
44-52: LGTM!The wheel zoom implementation with scale clamping (0.25–6) and step factor is well-designed. The multiplicative zoom step provides natural zoom behavior.
55-81: LGTM!The panning implementation with movement threshold detection (5px) correctly distinguishes between clicks and drags, enabling both coloring and panning interactions on the same element.
10-11: LGTM!Defining the shape selector as a constant improves maintainability and covers the common SVG shape primitives.
112-142: LGTM!The UI structure with mode toggle, interactive viewport, and user guidance hint is well-organized. The
GlassSwitchintegration with dynamicariaLabeland icon content provides good accessibility.
There was a problem hiding this comment.
Actionable comments posted: 5
Fix all issues with AI Agents 🤖
In @src/pages/Editor/index.jsx:
- Around line 1-11: Prettier found formatting violations in
src/pages/Editor/index.jsx; run the formatter (e.g., execute `prettier --write
src/pages/Editor/index.jsx`) or apply the project's Prettier settings/VSCode
formatting to fix whitespace/line breaks and import ordering so the file
(including top-level imports and the SHAPE_SELECTOR constant) conforms to the
repo style; commit the formatted file.
- Line 67: The code calls viewportRef.current?.classList.remove('grabbing')
which removes the literal class name but your CSS module uses hashed names;
replace that literal with the module reference (e.g., use styles.grabbing) so
the correct hashed class is removed; locate the usage in the component where
viewportRef is referenced and update classList.remove('grabbing') to
classList.remove(styles.grabbing) (and similarly for any classList.add calls)
ensuring styles is imported from the CSS module.
- Line 42: The code adds a literal "grabbing" string via
viewportRef.current.classList.add('grabbing'), which won't match the hashed CSS
module name; replace this with the module class reference (use styles.grabbing)
when adding/removing the class on viewportRef.current (and null-check
viewportRef.current before calling classList) so the CSS module's hashed class
is applied correctly; update any corresponding remove call to use
styles.grabbing as well.
- Line 16: The state initialization calls parse(svg) immediately via useState(()
=> parse(svg)) which can throw when svg is undefined; move the early null guard
for svg (the existing !svg check) to run before initializing svgElements so you
only call parse when svg is present, e.g., check if (!svg) and return the early
fallback (include <EditorHelmet /> in that fallback) before invoking
useState/parse; update references to svgElements/useState initialization to only
run after the null check so parsing is safe.
- Line 79: The code is wrongly assigning a CSS-module id via shape.id =
styles.coloredRegion and expecting a literal #coloredRegion selector; change to
apply a class or data attribute instead (e.g., use
shape.classList.add(styles.coloredRegion) or
shape.setAttribute('data-colored-region','true') rather than setting shape.id),
update the CSS selector from :not(#coloredRegion) to :not(.coloredRegion) or
:not([data-colored-region]) accordingly, and ensure the CSS module defines the
.coloredRegion rule (or the data-attribute rule) to achieve the intended fill
behavior while avoiding duplicate IDs.
🧹 Nitpick comments (1)
src/pages/Editor/index.jsx (1)
102-102: Consider moving inline styles to CSS module.The inline style object
{{ display: 'flex', justifyContent: 'flex-end' }}could be extracted to the CSS module for better maintainability and consistency.🔎 Proposed refactor
In
Editor.module.css:.controls { display: flex; justify-content: flex-end; }In the JSX:
- <div className="controls" style={{ display: 'flex', justifyContent: 'flex-end' }}> + <div className={styles.controls}>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Editor/index.jsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:35:30.143Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T21:35:30.143Z
Learning: In the Img2Num project, files that import React hooks must use the `.jsx` extension (not `.js`), as `npm run dev` fails otherwise. This is a build configuration requirement.
Applied to files:
src/pages/Editor/index.jsx
🧬 Code graph analysis (1)
src/pages/Editor/index.jsx (2)
src/components/GlassCard.jsx (1)
GlassCard(5-9)src/components/GlassSwitch.jsx (1)
GlassSwitch(5-21)
🪛 GitHub Actions: CI
src/pages/Editor/index.jsx
[warning] 1-1: Prettier formatting issues detected in this file. Run 'prettier --write' to fix.
🔇 Additional comments (1)
src/pages/Editor/index.jsx (1)
45-61: LGTM!The pointer move handler correctly implements panning with a movement threshold to distinguish clicks from drags.
Yes! That's really helpful |
There was a problem hiding this comment.
Actionable comments posted: 6
Fix all issues with AI Agents 🤖
In @src/pages/Editor/Editor.module.css:
- Around line 2-15: The .viewport CSS rule declares position: relative twice;
remove the redundant duplicate (the second position: relative) from the
.viewport block so only the single initial declaration remains, leaving all
other properties unchanged.
- Around line 17-21: The CSS uses :not(#coloredRegion) but JS sets shape.id =
styles.coloredRegion (a hashed CSS module) and also assigns identical IDs to
multiple shapes; change the CSS to target a class instead (e.g., .coloredRegion)
and add a specific rule for .colorMode .viewport svg path.coloredRegion to
preserve colored fills and make cursor default; then update the JS where
shape.id = styles.coloredRegion (index.jsx) to use
shape.classList.add(styles.coloredRegion) so the selector matches and you avoid
duplicate IDs.
In @src/pages/Editor/index.jsx:
- Around line 73-84: The code currently sets shape.id = styles.coloredRegion
inside the click handler in src/pages/Editor/index.jsx which is incorrect
because styles.coloredRegion is a hashed CSS module name and IDs must be unique;
instead update the handler to add a class or data attribute to the matched
element (e.g., shape.classList.add(styles.coloredRegion) or
shape.dataset.colored = 'true') and remove the id assignment; also update the
corresponding CSS selector (previously using :not(#coloredRegion)) to target the
class or data attribute so styling behaves correctly.
- Around line 65-71: The onPointerUp handler uses
viewportRef.current.classList.remove('grabbing') with a literal class name which
breaks when using CSS modules; replace the literal with the imported CSS module
class reference (e.g., styles.grabbing) so update onPointerUp to call
viewportRef.current.classList.remove(styles.grabbing) (ensure the module is
imported as styles and handle undefined viewportRef.current defensively).
- Around line 35-45: The onPointerDown handler is adding the literal class
'grabbing' which won't match CSS module hashed names; update the handler to use
the CSS module export (e.g., styles.grabbing) instead of the string and ensure
the same module key is used when removing the class elsewhere (look for
onPointerUp/onPointerMove handlers that call classList.remove). Import the CSS
module if not already (Editor.module.css -> styles) and replace
viewportRef.current.classList.add('grabbing') with using the module reference
(viewportRef.current.classList.add(styles.grabbing)), keeping safe null checks
for viewportRef.current.
- Around line 13-21: The component calls parse(svg) during state initialization
which can run when svg is undefined; move the null/guard check to the top of the
Editor component (before any useState/useRef calls) so you validate that state
and svg exist (from useLocation) and handle missing svg (redirect, show
fallback, or set a safe default) before calling parse; update references to
svgElements / parse in the component to rely on the validated svg and avoid
calling parse(undefined).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/Editor/Editor.module.csssrc/pages/Editor/index.jsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:35:30.143Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2025-12-17T21:35:30.143Z
Learning: In the Img2Num project, files that import React hooks must use the `.jsx` extension (not `.js`), as `npm run dev` fails otherwise. This is a build configuration requirement.
Applied to files:
src/pages/Editor/index.jsx
🧬 Code graph analysis (1)
src/pages/Editor/index.jsx (2)
src/components/GlassCard.jsx (1)
GlassCard(5-9)src/components/GlassSwitch.jsx (1)
GlassSwitch(5-21)
🪛 Biome (2.1.2)
src/pages/Editor/Editor.module.css
[error] 13-13: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
position is already defined here.
Remove or rename the duplicate property to ensure consistent styling.
(lint/suspicious/noDuplicateProperties)
That's great news |
c787113 to
c03ae88
Compare
c03ae88 to
8de4dd2
Compare
2 participants
✨ Feature Pull Request
📌 Description
🔗 Issue
📦 Type of Change
✔️ Checklist
📸 Screenshots / Demo
Editor.Img2Num.-.Google.Chrome.2026-01-06.03-59-55.mp4
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.