feat(example-app): (React) new GlassModal and Editor page upgrades (saving, histories, fullscreen, bug fixes) - #278
Merged
Conversation
- Fixes broken dragging in Editor of React App
…nfirmation after reset button is click
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a modal component system ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (7)
example-apps/react-js/src/global-styles/components.css (1)
18-21: Stylelint: Expected empty line before declaration.The
colordeclaration at line 21 follows a nested ruleset without an empty line separator.🔧 Proposed fix
* { color: unset; } + color: var(--color-text);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/global-styles/components.css` around lines 18 - 21, The CSS has a missing blank line before the standalone "color: var(--color-text);" declaration (it immediately follows the universal selector block "* { color: unset; }"), so add a single empty line before the "color: var(--color-text);" declaration to satisfy stylelint; locate the declaration by searching for the "color: var(--color-text);" token or the universal selector "*" and insert the empty line separator so declarations are properly separated.example-apps/react-js/src/components/GlassModal.module.css (1)
23-33: Close button lacks explicit background color.Without a
backgroundproperty, the button may inherit or default to unexpected styles depending on global resets. Consider addingbackground: transparentor the appropriate color.💡 Proposed fix
.closeButton { position: absolute; top: 0.5rem; right: 0.5rem; border: none; + background: transparent; cursor: pointer;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/components/GlassModal.module.css` around lines 23 - 33, The .closeButton CSS class lacks an explicit background which can lead to inconsistent visuals; update the .closeButton rule to set an explicit background (e.g., background: transparent or your design-system close button color) so it doesn't inherit unexpected styles from global resets, and ensure the chosen background works with existing padding/alignments in the .closeButton rule.example-apps/react-js/src/components/HamburgerMenu.module.css (2)
41-45: The.openclass outside the media query has no practical effect on desktop.On desktop,
.navListisdisplay: flexand already visible. The.openclass properties (opacity, visibility, transform) don't affect a visible flex container. Consider moving this rule inside the media query to clarify its mobile-only purpose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/components/HamburgerMenu.module.css` around lines 41 - 45, The .open rule is currently defined outside the mobile media query but has no effect on desktop because .navList is display:flex and already visible; move the .open selector (the rule setting opacity, visibility, transform) into the existing mobile-only media query alongside the .navList mobile rules so .open only applies on small screens and controls the toggled mobile menu (refer to the .open class and .navList selector to locate the relevant CSS).
99-111: Consider avoiding ID selectors in CSS modules.Using
#nav-menucreates implicit coupling between CSS and the hardcoded ID in the JSX. CSS modules work best when scoped through class selectors. If these styles need high specificity, consider using:where()with the module class or moving anchor-specific styles to the module class directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/components/HamburgerMenu.module.css` around lines 99 - 111, The styles use an ID selector ("#nav-menu a", "a:link", "a:visited") which couples the CSS module to a hardcoded DOM id; replace the ID selector with a module-scoped class (e.g., create a .navMenu or .navAnchor class in HamburgerMenu.module.css and move the anchor rules there) or scope via :where(.navModuleClass) to keep specificity while staying module-scoped, then update the component JSX to apply that className to the nav container or anchors instead of relying on the `#nav-menu` id.example-apps/react-js/src/components/GlassCard.jsx (1)
6-10: Consider adding displayName for better debugging.When using
forwardRef, React DevTools will show the component as "Anonymous" unless you setdisplayName. This aids debugging.💡 Proposed fix
const GlassCard = forwardRef( ({ as: Tag = "div", children, ...rest }, ref) => ( <Tag {...rest} className={`text-center glass ${styles.card} ${rest.className || ""}`} ref={ref}> {children} </Tag> )); + +GlassCard.displayName = "GlassCard";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/components/GlassCard.jsx` around lines 6 - 10, The GlassCard component is created via forwardRef and will appear as "Anonymous" in React DevTools; set a displayName to improve debugging by adding GlassCard.displayName = "GlassCard" after the forwardRef declaration (reference the GlassCard identifier and the forwardRef usage) so the component shows a readable name in tooling.example-apps/react-js/src/pages/Editor/EditorControls.jsx (2)
40-59: Consider usingonafterprintfor more reliable iframe cleanup.The current 100ms timeout before printing works but is fragile. The iframe is removed immediately after
print()is called, which may interrupt the print dialog on some browsers. Usingonafterprintwould ensure cleanup happens after the user dismisses the print dialog.♻️ Optional improvement for more reliable cleanup
// Give browser a moment to render setTimeout(() => { iframe.contentWindow.focus(); + iframe.contentWindow.onafterprint = () => { + document.body.removeChild(iframe); + }; iframe.contentWindow.print(); - document.body.removeChild(iframe); + // Fallback cleanup for browsers that don't support onafterprint + setTimeout(() => { + if (iframe.parentNode) { + document.body.removeChild(iframe); + } + }, 60000); }, 100);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx` around lines 40 - 59, The printSvg function removes the temporary iframe immediately after calling print(), which can interrupt the print dialog; update printSvg to attach an onafterprint handler on iframe.contentWindow (or iframe) that removes the iframe and cleans up the handler when printing completes, and keep the existing setTimeout fallback for older browsers; reference the iframe and printSvg symbols and ensure you remove listeners and the iframe in the onafterprint callback to avoid leaks.
102-107: Add error handling for clipboard API.
navigator.clipboard.writeText()returns a Promise that can reject if clipboard permissions are denied. Silently failing could confuse users.♻️ Proposed fix with user feedback
<Tooltip content="Copy SVG code to clipboard"> - <button onClick={() => navigator.clipboard.writeText(svg)} className="button"> + <button + onClick={() => + navigator.clipboard.writeText(svg) + .then(() => alert("SVG copied to clipboard!")) + .catch(() => alert("Failed to copy to clipboard")) + } + className="button" + > <Copy /> Copy SVG </button> </Tooltip>Consider replacing
alert()with a toast notification for better UX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx` around lines 102 - 107, navigator.clipboard.writeText(svg) can reject (permissions/blocked); update the onClick handler on the button inside Tooltip to handle Promise rejections by using async/await or .then/.catch around navigator.clipboard.writeText(svg), and surface success/failure to the user (replace any alert usage with the app's toast/notification API) so Copy button (and Copy component) shows a success toast on resolve and an error toast on rejection with a helpful message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example-apps/react-js/src/components/GlassModal.jsx`:
- Around line 34-39: The modal root div rendered in GlassModal.jsx (the element
using className={styles.backdrop}, role="dialog", and
onClick={handleBackdropClick}) is missing the aria-modal attribute; update that
element to include aria-modal="true" so assistive technologies know background
content is inert (keep the existing role, className, and onClick as-is).
- Around line 60-68: GlassModal.propTypes is missing the style prop declaration;
update GlassModal.propTypes to include style (e.g., style: PropTypes.object or
style: PropTypes.oneOfType([PropTypes.object, PropTypes.array])) so the
component's accepted inline styles are validated, keeping the rest of the
propTypes (isOpen, onClose, children, size, showCloseButton,
closeOnBackdropClick, className) unchanged.
In `@example-apps/react-js/src/components/GlassModal.module.css`:
- Around line 35-43: Rename the keyframe identifiers from fadeIn and scaleIn to
kebab-case (fade-in and scale-in) in GlassModal.module.css and update any local
references that use those names (e.g., animation or animation-name declarations
that reference fadeIn/scaleIn) so they point to fade-in and scale-in; ensure
both the `@keyframes` blocks and all places where those names are referenced
within this CSS module are changed consistently.
In `@example-apps/react-js/src/components/HamburgerMenu.jsx`:
- Around line 10-11: The default icon props are inverted: swap the default JSX
values so OpenMenuIcon defaults to the hamburger (Menu) and CloseMenuIcon
defaults to the close (X); specifically change the defaults for the OpenMenuIcon
and CloseMenuIcon variables (or props) so OpenMenuIcon = <Menu size={20} /> and
CloseMenuIcon = <X size={20} />, and verify wherever the component uses these
props (e.g., the toggle render that chooses between OpenMenuIcon and
CloseMenuIcon) still shows the hamburger when closed and the X when open.
- Around line 76-80: The current React.Children.map + React.cloneElement in
HamburgerMenu.jsx overwrites any child's existing className; change the clone
logic to merge the child's existing className with "button" instead of replacing
it by reading child.props.className and concatenating (or using a classnames
utility) when creating the new props in React.cloneElement so existing classes
like styles.themeToggle are preserved.
- Around line 29-39: The outside-click handler closes the menu when the toggle
button (which sits outside the current menuRef) is clicked, causing a mousedown
close then a click reopen; fix by ensuring the outside-click check includes the
toggle: either move menuRef to wrap both the toggle and the <ul> (so menuRef
covers the toggle and menu) or add a separate toggleRef and update the
onClickOutside handler in the useEffect to ignore events when e.target is inside
toggleRef.current (in addition to the existing menuRef check), keeping the same
listener attach/remove logic around onClickOutside and the same dependencies
([isOpen]).
In `@example-apps/react-js/src/global-styles/components.css`:
- Around line 9-11: The CSS rule "button { all: unset; }" wipes inherited font
styles (negating the "font: inherit" set in base.css), so update the styles for
the button selector to preserve font inheritance: either apply "font: inherit"
immediately after the "all: unset" reset for the button selector, or replace
"all: unset" with a targeted reset that excludes font-related properties (e.g.,
reset padding/margin/background/border but keep font), ensuring the unique
selector "button" and the existing "font: inherit" rule are preserved.
- Around line 35-37: The selector `.button *:active` targets descendant elements
rather than the button itself; change the selector to `.button:active` so the
active transform applies to the `.button` element (update the rule that
currently reads `.button *:active` to `.button:active` and keep the transform
declaration intact).
- Around line 30-41: The hover/active rules currently target only the .button
selector causing native <button> elements to miss interactive feedback; update
the selectors so they include bare button elements (e.g., change ".button:hover"
to "button:hover, .button:hover", ".button *:active" to "button *:active,
.button *:active", and ".button *:hover .externalIcon" to "button *:hover
.externalIcon, .button *:hover .externalIcon") so native buttons (such as those
in EditorControls.jsx) get the same hover/active/transform/opacity behavior.
In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx`:
- Around line 143-148: The Tooltip text is misleading: the Tooltip wrapping the
Save button currently reads "Share the image with others" but the click handler
(setModalOpen) opens the save/download modal; update the Tooltip's content prop
to accurately describe the action (e.g., "Save image" or "Download image") so
Tooltip, the Save icon/component and the span that calls setModalOpen are
consistent; locate the Tooltip around the Save component in EditorControls.jsx
and change its content string accordingly.
- Around line 89-92: The current <button> using href={svgUrl} and
download={`${fileName}.svg`} won't work because those attributes only apply to
anchors; update the JSX in EditorControls.jsx to either replace the <button>
with an <a> element (keep className="button", set href={svgUrl} and
download={`${fileName}.svg`} and preserve the <Download /> and <span>Original
SVG</span> children) or implement an onClick handler (e.g., downloadSvg) that
programmatically creates an <a> with href=svgUrl and download=fileName + '.svg',
clicks it, and cleans up; reference svgUrl and fileName when wiring the handler.
- Around line 20-24: The current useMemo block that creates svgUrl via
URL.createObjectURL(blob) (symbol: svgUrl) leaks memory because the Blob URL is
never revoked; replace this useMemo with a useEffect that creates the Blob and
object URL when svg changes and returns a cleanup function that calls
URL.revokeObjectURL(svgUrl) (and sets svgUrl state/variable to null or triggers
re-create) so the blob is revoked on dependency changes and on unmount; update
any references to svgUrl accordingly and ensure the effect depends on [svg].
In `@example-apps/react-js/src/pages/Editor/EditorControls.module.css`:
- Around line 7-9: The `.hamburger` rule using `top: 10%` is ineffective without
a position and conflicts with the mobile `top: calc(100% + var(--spacing-sm))`
on the parent; either remove the `top: 10%` declaration or make it apply only on
desktop with a proper positioning (e.g., add `position: relative` to
`.hamburger` inside a desktop media query) so it takes effect without overriding
the mobile positioning used by the HamburgerMenu `<ul>`.
In `@example-apps/react-js/src/pages/Editor/index.jsx`:
- Around line 284-292: The keybinding handler in the useEffect that registers
window.addEventListener("keydown", handler) must handle Mac Cmd keys and prevent
the browser default: update the handler to treat (e.ctrlKey || e.metaKey) when
checking for "z" and "y" and call e.preventDefault() before invoking undo() or
redo(); keep undo and redo as the invoked functions and ensure the same updated
handler is removed in the cleanup to avoid leaks.
- Around line 347-354: The Confirm button's onClick should not only call
restoreHistory(initialSnapshot) but also reset the internal history state to
match the visual snapshot: in the Confirm handler (the onClick for the Confirm
button) call the state setters to set history to an array containing
initialSnapshot and set historyIndex to 0 (e.g., setHistory([initialSnapshot])
and setHistoryIndex(0) or the equivalent state updater used in this component)
before closing the modal so undo/redo reflects the reset.
- Around line 272-282: The pointerrawupdate event listener on viewportRef (in
useEffect) doesn't work in Safari; add a fallback by wiring the same handler
into the GlassCard element as an onPointerMove prop so Safari receives pointer
updates: locate the GlassCard JSX where viewportRef is used and add
onPointerMove={onPointerMove} (or equivalent prop name used by that component),
keeping the existing pointerrawupdate listener for high-frequency Chrome updates
and ensuring you don't duplicate handlers or break existing cleanup logic tied
to onPointerMove/viewportRef.
---
Nitpick comments:
In `@example-apps/react-js/src/components/GlassCard.jsx`:
- Around line 6-10: The GlassCard component is created via forwardRef and will
appear as "Anonymous" in React DevTools; set a displayName to improve debugging
by adding GlassCard.displayName = "GlassCard" after the forwardRef declaration
(reference the GlassCard identifier and the forwardRef usage) so the component
shows a readable name in tooling.
In `@example-apps/react-js/src/components/GlassModal.module.css`:
- Around line 23-33: The .closeButton CSS class lacks an explicit background
which can lead to inconsistent visuals; update the .closeButton rule to set an
explicit background (e.g., background: transparent or your design-system close
button color) so it doesn't inherit unexpected styles from global resets, and
ensure the chosen background works with existing padding/alignments in the
.closeButton rule.
In `@example-apps/react-js/src/components/HamburgerMenu.module.css`:
- Around line 41-45: The .open rule is currently defined outside the mobile
media query but has no effect on desktop because .navList is display:flex and
already visible; move the .open selector (the rule setting opacity, visibility,
transform) into the existing mobile-only media query alongside the .navList
mobile rules so .open only applies on small screens and controls the toggled
mobile menu (refer to the .open class and .navList selector to locate the
relevant CSS).
- Around line 99-111: The styles use an ID selector ("#nav-menu a", "a:link",
"a:visited") which couples the CSS module to a hardcoded DOM id; replace the ID
selector with a module-scoped class (e.g., create a .navMenu or .navAnchor class
in HamburgerMenu.module.css and move the anchor rules there) or scope via
:where(.navModuleClass) to keep specificity while staying module-scoped, then
update the component JSX to apply that className to the nav container or anchors
instead of relying on the `#nav-menu` id.
In `@example-apps/react-js/src/global-styles/components.css`:
- Around line 18-21: The CSS has a missing blank line before the standalone
"color: var(--color-text);" declaration (it immediately follows the universal
selector block "* { color: unset; }"), so add a single empty line before the
"color: var(--color-text);" declaration to satisfy stylelint; locate the
declaration by searching for the "color: var(--color-text);" token or the
universal selector "*" and insert the empty line separator so declarations are
properly separated.
In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx`:
- Around line 40-59: The printSvg function removes the temporary iframe
immediately after calling print(), which can interrupt the print dialog; update
printSvg to attach an onafterprint handler on iframe.contentWindow (or iframe)
that removes the iframe and cleans up the handler when printing completes, and
keep the existing setTimeout fallback for older browsers; reference the iframe
and printSvg symbols and ensure you remove listeners and the iframe in the
onafterprint callback to avoid leaks.
- Around line 102-107: navigator.clipboard.writeText(svg) can reject
(permissions/blocked); update the onClick handler on the button inside Tooltip
to handle Promise rejections by using async/await or .then/.catch around
navigator.clipboard.writeText(svg), and surface success/failure to the user
(replace any alert usage with the app's toast/notification API) so Copy button
(and Copy component) shows a success toast on resolve and an error toast on
rejection with a helpful message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa845163-5a3e-48dc-bef5-44ca0dc780e8
📒 Files selected for processing (13)
example-apps/react-js/src/components/GlassCard.jsxexample-apps/react-js/src/components/GlassModal.jsxexample-apps/react-js/src/components/GlassModal.module.cssexample-apps/react-js/src/components/HamburgerMenu.jsxexample-apps/react-js/src/components/HamburgerMenu.module.cssexample-apps/react-js/src/components/NavBar.jsxexample-apps/react-js/src/components/NavBar.module.cssexample-apps/react-js/src/global-styles/base.cssexample-apps/react-js/src/global-styles/components.cssexample-apps/react-js/src/pages/Editor/Editor.module.cssexample-apps/react-js/src/pages/Editor/EditorControls.jsxexample-apps/react-js/src/pages/Editor/EditorControls.module.cssexample-apps/react-js/src/pages/Editor/index.jsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
example-apps/react-js/src/pages/Editor/EditorControls.jsx (2)
150-155:⚠️ Potential issue | 🟡 MinorMisleading tooltip text.
The tooltip says "Share the image with others" but clicking opens the download/save modal, not a share dialog. The actual share functionality is on the separate "Share" menu item below.
📝 Proposed fix
- <Tooltip content="Share the image with others"> + <Tooltip content="Save or download the image">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx` around lines 150 - 155, The Tooltip text for the save button is misleading; update the Tooltip content around the Save button (the Tooltip wrapping the span that calls setModalOpen and contains the <Save /> icon and "Save" label) to accurately describe the action (e.g., "Save the image" or "Download the image") so it matches the modal opened by setModalOpen rather than saying "Share the image with others".
28-32:⚠️ Potential issue | 🟠 MajorMemory leak: Blob URL is never revoked.
URL.createObjectURLallocates memory that persists until explicitly released. UsinguseMemoprevents proper cleanup when the component unmounts or whensvgchanges.🔧 Proposed fix using useEffect for proper cleanup
- const svgUrl = useMemo(() => { - if (!svg) return null; - const blob = new Blob([svg], { type: "image/svg+xml;charset=utf-8" }); - return URL.createObjectURL(blob); - }, [svg]); + const [svgUrl, setSvgUrl] = useState(null); + + useEffect(() => { + if (!svg) { + setSvgUrl(null); + return; + } + const blob = new Blob([svg], { type: "image/svg+xml;charset=utf-8" }); + const url = URL.createObjectURL(blob); + setSvgUrl(url); + + return () => URL.revokeObjectURL(url); + }, [svg]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx` around lines 28 - 32, The current useMemo block creating svgUrl with URL.createObjectURL leaks memory because blob URLs are never revoked; replace the useMemo with a useEffect that creates the blob URL when svg changes (using the same blob creation logic) and sets it into state or a ref (svgUrl), and in the effect cleanup call URL.revokeObjectURL on the created URL so it is released both when svg changes and when the component unmounts; reference the existing svgUrl variable, the useMemo usage, and URL.createObjectURL/URL.revokeObjectURL to locate and update the code.example-apps/react-js/src/components/GlassModal.jsx (1)
40-44:⚠️ Potential issue | 🟡 MinorAdd
aria-modal="true"for proper accessibility.Per issue
#114requirements, the modal should havearia-modalattribute to indicate to assistive technologies that content behind the modal is inert.♿ Proposed fix
<div className={styles.backdrop} onClick={handleBackdropClick} role="dialog" + aria-modal="true" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/components/GlassModal.jsx` around lines 40 - 44, The modal container in GlassModal.jsx (the div using className={styles.backdrop}, role="dialog", and onClick={handleBackdropClick}) is missing the aria-modal attribute; update that element to include aria-modal="true" to indicate the rest of the page is inert for assistive technologies, preserving the existing role, className, and handler.example-apps/react-js/src/pages/Editor/index.jsx (3)
284-292:⚠️ Potential issue | 🟡 MinorKeyboard shortcuts should support Mac and prevent default browser behavior.
Two issues:
- Only checks
ctrlKey, missingmetaKeyfor Mac users expecting Cmd+Z/Cmd+Y- Missing
e.preventDefault()which could cause browser's native undo to also triggerAdditionally,
undoandredoare recreated on each render, causing the handler to use stale closures. Consider usinguseCallbackor referencing state directly.🛠️ Proposed fix
useEffect(() => { const handler = (e) => { - if (e.ctrlKey && e.key === "z") undo(); - if (e.ctrlKey && e.key === "y") redo(); + const mod = e.ctrlKey || e.metaKey; + if (mod && e.key === "z") { + e.preventDefault(); + undo(); + } + if (mod && e.key === "y") { + e.preventDefault(); + redo(); + } }; window.addEventListener("keydown", handler); return () => window.removeEventListener("keydown", handler); }, [historyIndex, history]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/index.jsx` around lines 284 - 292, Update the keydown handler inside the useEffect that attaches to window (the anonymous handler passed to window.addEventListener) so it checks for e.ctrlKey || e.metaKey (to support Cmd on Mac) and calls e.preventDefault() when intercepting undo/redo; also ensure the handler uses stable undo/redo references by memoizing those functions with useCallback or by reading them from refs so the effect’s dependency array doesn’t capture stale closures (adjust the useEffect dependencies accordingly and keep window.removeEventListener in the cleanup).
272-282:⚠️ Potential issue | 🟠 MajorStale closure:
onPointerMoveis not in dependency array.The
useEffectregistersonPointerMoveas an event listener but has an empty dependency array. SinceonPointerMovereferencespointerState,pinchRef,activePointersRef, andupdateTransform, the handler will use stale closures if any of these change.Additionally, this listener won't work in Safari (as noted in past reviews). Add
onPointerMoveas a prop fallback on the GlassCard element.🔧 Proposed fix
useEffect(() => { const el = viewportRef.current; if (!el) return; el.addEventListener("pointerrawupdate", onPointerMove); return () => { el.removeEventListener("pointerrawupdate", onPointerMove); }; - }, []); + }, [onPointerMove]);And for Safari fallback, add to the GlassCard element (around line 361-368):
<GlassCard className={`flex-center ${styles.viewport}`} ref={viewportRef} onWheel={handleWheel} onPointerDown={onPointerDown} + onPointerMove={onPointerMove} onPointerUp={onPointerUp} onPointerCancel={onPointerUp} >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/index.jsx` around lines 272 - 282, The effect registers onPointerMove with an empty dependency array causing a stale closure; wrap or memoize onPointerMove using useCallback with its real dependencies (pointerState, pinchRef, activePointersRef, updateTransform) so it stays up-to-date, then include that stable onPointerMove in the useEffect dependency array that uses viewportRef; additionally pass this onPointerMove as a prop to the GlassCard component (so GlassCard can attach it as a pointermove fallback for Safari) and ensure GlassCard uses that prop to register a pointermove listener.
346-353:⚠️ Potential issue | 🟠 MajorReset confirmation doesn't update history state, causing undo/redo inconsistency.
When the user confirms reset,
restoreHistory(initialSnapshot)only updates the visual state (CSS classes), buthistoryandhistoryIndexremain unchanged. After reset, pressing undo will restore the previous colored state unexpectedly.🛠️ Proposed fix
<button onClick={() => { restoreHistory(initialSnapshot); + setHistory([initialSnapshot]); + setHistoryIndex(0); setModalOpen(false); }} className="button" > Confirm </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/index.jsx` around lines 346 - 353, When confirming reset, also update the undo state so history and historyIndex reflect the new snapshot: after calling restoreHistory(initialSnapshot) in the Confirm button handler, set the history to an array containing initialSnapshot and set historyIndex to 0 (or whatever index you use for the current entry) so future undo/redo behaves correctly; keep setModalOpen(false) as-is. Locate the Confirm button handler where restoreHistory(initialSnapshot) and setModalOpen(false) are invoked and add the state updates for history and historyIndex (use the existing state setters or dispatch functions for history and historyIndex).
🧹 Nitpick comments (3)
example-apps/react-js/src/pages/Editor/EditorControls.jsx (2)
135-145: Consider disabling undo/redo buttons when actions are unavailable.The buttons are always enabled, but clicking them when there's nothing to undo/redo has no effect, which may confuse users. Consider passing
canUndo/canRedoprops to control the disabled state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx` around lines 135 - 145, The undo/redo buttons in EditorControls.jsx always remain enabled; update the button elements that call onUndo and onRedo to accept and use boolean props (e.g., canUndo and canRedo) and set the button disabled attribute accordingly, so the Undo/Redo buttons are disabled when actions are unavailable; also ensure the Tooltip still displays (wrap the button but keep the tooltip content) and propagate appropriate aria-disabled or title text if needed for accessibility.
21-26: Missing error handling for clipboard API.The
navigator.clipboard.writeTextpromise has no.catch()handler. If clipboard access fails (e.g., permissions denied), the error is silently swallowed.🔧 Proposed fix
const copySvg = () => { navigator.clipboard.writeText(svg).then(() => { setCopied(true); setTimeout(() => setCopied(false), 1500); - }); + }).catch((err) => { + console.error("Failed to copy SVG:", err); + // Optionally show user feedback + }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx` around lines 21 - 26, The copySvg function calls navigator.clipboard.writeText(svg) without handling rejections; update copySvg to add a .catch handler (or use try/catch if converted to async) to handle permission or write errors, e.g., log the error and set an error state or user-visible feedback instead of silently swallowing it; keep the existing success flow (setCopied(true) and setTimeout) and ensure the error path clears any transient UI if needed (referencing copySvg and setCopied to locate the code).example-apps/react-js/src/components/GlassModal.jsx (1)
8-69: PropTypes validation is missing entirely.The component accepts multiple props (
isOpen,onClose,children,size,showCloseButton,closeOnBackdropClick,className,style) but has no PropTypes block for validation.🔧 Proposed fix: Add PropTypes
+GlassModal.propTypes = { + isOpen: PropTypes.bool.isRequired, + onClose: PropTypes.func.isRequired, + children: PropTypes.node.isRequired, + size: PropTypes.oneOfType([ + PropTypes.oneOf(["sm", "md", "lg"]), + PropTypes.string, + ]), + showCloseButton: PropTypes.bool, + closeOnBackdropClick: PropTypes.bool, + className: PropTypes.string, + style: PropTypes.object, +};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/components/GlassModal.jsx` around lines 8 - 69, Add PropTypes validation for the GlassModal component: import PropTypes and define GlassModal.propTypes validating isOpen (bool), onClose (func), children (node), size (oneOf or string), showCloseButton (bool), closeOnBackdropClick (bool), className (string), and style (object), and also add GlassModal.defaultProps for optional defaults (size, showCloseButton, closeOnBackdropClick, className, style) to match the existing defaults in the function signature; reference the exported function GlassModal to attach these propTypes/defaultProps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx`:
- Line 148: The className is using a string literal instead of resolving the CSS
module, so `styles.hamburger` is rendered as text; update the HamburgerMenu prop
(component HamburgerMenu, prop className) to use the actual CSS module
value—either pass styles.hamburger directly or use a template literal with
${styles.hamburger}—so the correct class name from the styles object is applied.
---
Duplicate comments:
In `@example-apps/react-js/src/components/GlassModal.jsx`:
- Around line 40-44: The modal container in GlassModal.jsx (the div using
className={styles.backdrop}, role="dialog", and onClick={handleBackdropClick})
is missing the aria-modal attribute; update that element to include
aria-modal="true" to indicate the rest of the page is inert for assistive
technologies, preserving the existing role, className, and handler.
In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx`:
- Around line 150-155: The Tooltip text for the save button is misleading;
update the Tooltip content around the Save button (the Tooltip wrapping the span
that calls setModalOpen and contains the <Save /> icon and "Save" label) to
accurately describe the action (e.g., "Save the image" or "Download the image")
so it matches the modal opened by setModalOpen rather than saying "Share the
image with others".
- Around line 28-32: The current useMemo block creating svgUrl with
URL.createObjectURL leaks memory because blob URLs are never revoked; replace
the useMemo with a useEffect that creates the blob URL when svg changes (using
the same blob creation logic) and sets it into state or a ref (svgUrl), and in
the effect cleanup call URL.revokeObjectURL on the created URL so it is released
both when svg changes and when the component unmounts; reference the existing
svgUrl variable, the useMemo usage, and URL.createObjectURL/URL.revokeObjectURL
to locate and update the code.
In `@example-apps/react-js/src/pages/Editor/index.jsx`:
- Around line 284-292: Update the keydown handler inside the useEffect that
attaches to window (the anonymous handler passed to window.addEventListener) so
it checks for e.ctrlKey || e.metaKey (to support Cmd on Mac) and calls
e.preventDefault() when intercepting undo/redo; also ensure the handler uses
stable undo/redo references by memoizing those functions with useCallback or by
reading them from refs so the effect’s dependency array doesn’t capture stale
closures (adjust the useEffect dependencies accordingly and keep
window.removeEventListener in the cleanup).
- Around line 272-282: The effect registers onPointerMove with an empty
dependency array causing a stale closure; wrap or memoize onPointerMove using
useCallback with its real dependencies (pointerState, pinchRef,
activePointersRef, updateTransform) so it stays up-to-date, then include that
stable onPointerMove in the useEffect dependency array that uses viewportRef;
additionally pass this onPointerMove as a prop to the GlassCard component (so
GlassCard can attach it as a pointermove fallback for Safari) and ensure
GlassCard uses that prop to register a pointermove listener.
- Around line 346-353: When confirming reset, also update the undo state so
history and historyIndex reflect the new snapshot: after calling
restoreHistory(initialSnapshot) in the Confirm button handler, set the history
to an array containing initialSnapshot and set historyIndex to 0 (or whatever
index you use for the current entry) so future undo/redo behaves correctly; keep
setModalOpen(false) as-is. Locate the Confirm button handler where
restoreHistory(initialSnapshot) and setModalOpen(false) are invoked and add the
state updates for history and historyIndex (use the existing state setters or
dispatch functions for history and historyIndex).
---
Nitpick comments:
In `@example-apps/react-js/src/components/GlassModal.jsx`:
- Around line 8-69: Add PropTypes validation for the GlassModal component:
import PropTypes and define GlassModal.propTypes validating isOpen (bool),
onClose (func), children (node), size (oneOf or string), showCloseButton (bool),
closeOnBackdropClick (bool), className (string), and style (object), and also
add GlassModal.defaultProps for optional defaults (size, showCloseButton,
closeOnBackdropClick, className, style) to match the existing defaults in the
function signature; reference the exported function GlassModal to attach these
propTypes/defaultProps.
In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx`:
- Around line 135-145: The undo/redo buttons in EditorControls.jsx always remain
enabled; update the button elements that call onUndo and onRedo to accept and
use boolean props (e.g., canUndo and canRedo) and set the button disabled
attribute accordingly, so the Undo/Redo buttons are disabled when actions are
unavailable; also ensure the Tooltip still displays (wrap the button but keep
the tooltip content) and propagate appropriate aria-disabled or title text if
needed for accessibility.
- Around line 21-26: The copySvg function calls
navigator.clipboard.writeText(svg) without handling rejections; update copySvg
to add a .catch handler (or use try/catch if converted to async) to handle
permission or write errors, e.g., log the error and set an error state or
user-visible feedback instead of silently swallowing it; keep the existing
success flow (setCopied(true) and setTimeout) and ensure the error path clears
any transient UI if needed (referencing copySvg and setCopied to locate the
code).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a26c2051-2d70-4a50-8cfe-f5473edf9eb3
📒 Files selected for processing (4)
docker-compose.ymlexample-apps/react-js/src/components/GlassModal.jsxexample-apps/react-js/src/pages/Editor/EditorControls.jsxexample-apps/react-js/src/pages/Editor/index.jsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
example-apps/react-js/src/pages/Editor/index.jsx (2)
272-282:⚠️ Potential issue | 🟠 MajorAdd a standard
pointermovefallback for browsers withoutpointerrawupdate.This is still Safari / older-Firefox incompatible: the raw-update listener is registered unconditionally, and the viewport never receives
onPointerMove. In unsupported browsers, drag and pinch stop updating.🛠️ Proposed fix
+const supportsRawUpdate = + typeof window !== "undefined" && "onpointerrawupdate" in window; + // Speed boost - better than React version useEffect(() => { const el = viewportRef.current; - if (!el) return; + if (!el || !supportsRawUpdate) return; el.addEventListener("pointerrawupdate", onPointerMove); return () => { el.removeEventListener("pointerrawupdate", onPointerMove); @@ <GlassCard className={`flex-center ${styles.viewport}`} ref={viewportRef} onWheel={handleWheel} onPointerDown={onPointerDown} + onPointerMove={supportsRawUpdate ? undefined : onPointerMove} onPointerUp={onPointerUp} onPointerCancel={onPointerUp} >Does the `pointerrawupdate` Pointer Events API fire in current Safari and Firefox releases, and is `pointermove` the recommended fallback when it is unsupported?Also applies to: 370-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/index.jsx` around lines 272 - 282, The effect currently always registers "pointerrawupdate" on viewportRef.current, so browsers that don't support pointerrawupdate (Safari/older Firefox) never get pointer updates; update the useEffect that references viewportRef and onPointerMove to feature-detect and register "pointerrawupdate" only if supported (e.g., check "onpointerrawupdate" in Element.prototype or "PointerEvent" in window && "onpointerrawupdate" in document.createElement("div")), otherwise register the standard "pointermove" as a fallback; ensure the cleanup removes whichever listener was added (removeEventListener for the same event string used) and apply the same conditional registration logic to the other similar effect that also uses viewportRef and onPointerMove.
355-358:⚠️ Potential issue | 🟠 MajorRecord reset in history instead of only repainting the SVG.
restoreHistory(initialSnapshot)changes the DOM, buthistoryandhistoryIndexstill point at the pre-reset timeline. That leaves undo/redo out of sync with what the user sees. If reset should be undoable, append it as the next history entry here.🛠️ Proposed fix
<button onClick={() => { restoreHistory(initialSnapshot); + const newHistory = [ + ...history.slice(0, historyIndex + 1), + initialSnapshot, + ]; + setHistory(newHistory); + setHistoryIndex(newHistory.length - 1); setModalOpen(false); }} className="button" >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/index.jsx` around lines 355 - 358, The reset button calls restoreHistory(initialSnapshot) but doesn't update the undo stack; update the history state so the reset becomes an undoable action by appending the new snapshot to the history and advancing historyIndex (or replacing the tail if you maintain redo semantics). Concretely, in the button handler (same block that calls restoreHistory(initialSnapshot) and setModalOpen(false)), push initialSnapshot (or a serialized snapshot used by your history logic) onto the history array used by your Editor state and set historyIndex to point at that new entry so undo/redo reflect the reset; ensure you use the same helpers/state mutators that manage history in this component so behavior stays consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example-apps/react-js/src/pages/Editor/index.jsx`:
- Around line 220-232: Before mutating history or DOM, bail if the tapped region
is already colored: check shape.classList.contains(styles.coloredRegion) and
return early if true. In the handler containing shape, svgRoot,
styles.coloredRegion, currentShapes, history, historyIndex, setHistory and
setHistoryIndex, add this guard before calling shape.classList.add(...) so you
don’t push an identical snapshot; alternatively compute the post-click
currentShapes and compare to history[historyIndex] and skip
setHistory/setHistoryIndex when they are identical.
- Around line 286-297: The keyboard handler currently calls e.preventDefault()
for any modifier key and swallows all Cmd/Ctrl shortcuts; change the logic in
the handler function so you first ignore events coming from editable targets
(check e.target.isContentEditable or tagName input/textarea/select) and then
only call e.preventDefault() when you detect a handled combo: Ctrl/Cmd+Z for
undo, Ctrl/Cmd+Y for redo, and Shift+Cmd+Z for macOS redo; update the key checks
to detect e.shiftKey for Shift+Cmd+Z and ensure other modifier combos (e.g.,
Cmd+S) are not prevented.
---
Duplicate comments:
In `@example-apps/react-js/src/pages/Editor/index.jsx`:
- Around line 272-282: The effect currently always registers "pointerrawupdate"
on viewportRef.current, so browsers that don't support pointerrawupdate
(Safari/older Firefox) never get pointer updates; update the useEffect that
references viewportRef and onPointerMove to feature-detect and register
"pointerrawupdate" only if supported (e.g., check "onpointerrawupdate" in
Element.prototype or "PointerEvent" in window && "onpointerrawupdate" in
document.createElement("div")), otherwise register the standard "pointermove" as
a fallback; ensure the cleanup removes whichever listener was added
(removeEventListener for the same event string used) and apply the same
conditional registration logic to the other similar effect that also uses
viewportRef and onPointerMove.
- Around line 355-358: The reset button calls restoreHistory(initialSnapshot)
but doesn't update the undo stack; update the history state so the reset becomes
an undoable action by appending the new snapshot to the history and advancing
historyIndex (or replacing the tail if you maintain redo semantics). Concretely,
in the button handler (same block that calls restoreHistory(initialSnapshot) and
setModalOpen(false)), push initialSnapshot (or a serialized snapshot used by
your history logic) onto the history array used by your Editor state and set
historyIndex to point at that new entry so undo/redo reflect the reset; ensure
you use the same helpers/state mutators that manage history in this component so
behavior stays consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cea48f83-da30-465f-9531-36fd9812840b
📒 Files selected for processing (1)
example-apps/react-js/src/pages/Editor/index.jsx
Owner
Author
|
@Krasner, please will you give your opinion on this. I'd just like to know if this is a good addition and also if there is anything you think should be added or changed. |
Collaborator
Looks good. Just tried it out. |
Ryan-Millard
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
example-apps/react-js/src/pages/Editor/EditorControls.jsx (1)
31-35:⚠️ Potential issue | 🟠 Major
svgUrlobject URL is still leaked (no cleanup on change/unmount).Line 34 creates a persistent blob URL, but there is no matching cleanup for this specific
svgUrl. This was previously reported and is still present.🔧 Proposed fix
-import { useMemo, useState, useId } from "react"; +import { useEffect, useState, useId } from "react"; @@ - const svgUrl = useMemo(() => { - if (!svg) return null; - const blob = new Blob([svg], { type: "image/svg+xml;charset=utf-8" }); - return URL.createObjectURL(blob); - }, [svg]); + const [svgUrl, setSvgUrl] = useState(null); + + useEffect(() => { + if (!svg) { + setSvgUrl(null); + return; + } + const blob = new Blob([svg], { type: "image/svg+xml;charset=utf-8" }); + const url = URL.createObjectURL(blob); + setSvgUrl(url); + return () => URL.revokeObjectURL(url); + }, [svg]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx` around lines 31 - 35, The blob URL created in the useMemo for svgUrl (via URL.createObjectURL) is never revoked, leaking object URLs; update the hook to revoke the previous URL when svg changes and on unmount by storing the created URL (svgUrl) and calling URL.revokeObjectURL(oldUrl) before creating a new one and inside a cleanup function (return cleanup) from useMemo or switch to useEffect that creates the blob URL from svg and revokes it on cleanup; reference svgUrl, useMemo/useEffect, URL.createObjectURL and URL.revokeObjectURL when locating the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example-apps/react-js/src/pages/Editor/EditorControls.module.css`:
- Around line 46-53: Update the .srOnly CSS rule to replace the deprecated clip
property with a modern clip-path usage: remove the clip declaration and add
clip-path: inset(50%); retain the existing positioning, size, padding, margin,
overflow, white-space and border rules so the utility remains accessible;
optionally include -webkit-clip-path for broader browser support.
---
Duplicate comments:
In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx`:
- Around line 31-35: The blob URL created in the useMemo for svgUrl (via
URL.createObjectURL) is never revoked, leaking object URLs; update the hook to
revoke the previous URL when svg changes and on unmount by storing the created
URL (svgUrl) and calling URL.revokeObjectURL(oldUrl) before creating a new one
and inside a cleanup function (return cleanup) from useMemo or switch to
useEffect that creates the blob URL from svg and revokes it on cleanup;
reference svgUrl, useMemo/useEffect, URL.createObjectURL and URL.revokeObjectURL
when locating the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82f1bf20-debe-4a2d-accb-9723feb2a357
📒 Files selected for processing (6)
example-apps/react-js/src/components/GlassModal.jsxexample-apps/react-js/src/components/GlassSwitch.module.cssexample-apps/react-js/src/global-styles/base.cssexample-apps/react-js/src/global-styles/utilities/layout.cssexample-apps/react-js/src/pages/Editor/EditorControls.jsxexample-apps/react-js/src/pages/Editor/EditorControls.module.css
💤 Files with no reviewable changes (1)
- example-apps/react-js/src/components/GlassSwitch.module.css
✅ Files skipped from review due to trivial changes (1)
- example-apps/react-js/src/global-styles/utilities/layout.css
🚧 Files skipped from review as they are similar to previous changes (2)
- example-apps/react-js/src/global-styles/base.css
- example-apps/react-js/src/components/GlassModal.jsx
Ryan-Millard
force-pushed
the
feat/example-app/react/download-svg
branch
from
March 21, 2026 23:12
8524a23 to
6389ea8
Compare
Ryan-Millard
force-pushed
the
feat/example-app/react/download-svg
branch
from
March 21, 2026 23:23
3b39f5c to
184e7ca
Compare
Ryan-Millard
changed the title
Owner
Author
|
@coderabbitai are you able to review this or has the limit been hit? |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
example-apps/react-js/src/components/GlassModal.jsx (1)
1-1:⚠️ Potential issue | 🟠 MajorThe dialog is still unnamed.
role="dialog"withoutaria-modaland an accessible name misses the#114requirements.EditorControls.jsxalready passestitle="Save image", but this component drops it, so assistive tech gets an unnamed dialog.♿ Proposed fix
-import { useEffect } from "react"; +import { useEffect, useId } from "react"; ... export default function GlassModal({ isOpen, onClose, children, + title, size = "lg", // sm | md | lg | any CSS width showCloseButton = true, closeOnBackdropClick = true, className = "", style = {}, }) { + const titleId = useId(); + ... - <div className={styles.backdrop} onClick={handleBackdropClick} role="dialog"> + <div + className={styles.backdrop} + onClick={handleBackdropClick} + role="dialog" + aria-modal="true" + aria-labelledby={title ? titleId : undefined} + > <GlassCard as="div" className={`${styles.modal} ${className}`} style={{ flex: "0 0 auto", width: width, ...style, }} > + {title ? <h2 id={titleId}>{title}</h2> : null}Also applies to: 8-17, 37-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/react-js/src/components/GlassModal.jsx` at line 1, The modal rendered by the GlassModal component is missing an accessible name and aria-modal; update the GlassModal component to accept the title prop (passed from EditorControls.jsx as title="Save image") and apply it as an accessible name (either aria-label={title} or aria-labelledby pointing to a visible header element) and add aria-modal="true" alongside role="dialog" so assistive tech can announce it; ensure the header element (if used) has a matching id and that the component signature (GlassModal / props) uses the title prop when rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example-apps/react-js/src/hooks/useFullscreen.js`:
- Around line 19-33: The open() function currently pushes a history entry and
adds a popstate listener before fullscreen actually starts, and close() only
removes the listener so history accumulates; update useFullscreen so it
registers a document-level "fullscreenchange" handler that pushes a history
state when the element actually enters fullscreen and removes/pops that history
entry when it actually exits, and remove the premature window.history.pushState
call from open() and the blind listener removal in close(); also ensure the
Escape key handler in useFullscreen is gated by actual fullscreen state (or
removed) so it does not race with the Escape handler in GlassModal.jsx—use
document.fullscreenElement (or vendor-prefixed equivalents) inside the
fullscreenchange handler to decide when to push/pop history and when to run
close().
In `@example-apps/react-js/src/pages/Editor/EditorControls.jsx`:
- Line 4: Remove the unused blob URL plumbing by deleting the state/variable
svgUrl and the useEffect that creates/revokes the object URL (the effect that
calls URL.createObjectURL and URL.revokeObjectURL) so nothing references svgUrl;
update any related declarations (e.g., the useState/useId lines that declare
svgUrl) and ensure no other code in EditorControls.jsx references svgUrl or the
blob URL logic (remove createObjectURL/revokeObjectURL calls and their
imports/usages).
- Around line 79-125: The save modal (GlassModal) currently only offers SVG, raw
text, copySvg and printSvg but lacks PNG/JPG/PDF export required to close issue
`#85`; add UI buttons alongside the existing list items and implement handlers
exportRaster(format) and exportPDF() (or exportDocument()) that take the current
svg and fileName, render the SVG onto a canvas, then use
canvas.toBlob()/toDataURL to produce PNG/JPEG and call the existing download
helper, and for PDF use a lightweight approach (e.g., draw the canvas image into
a jsPDF or use PDFKit/browser API) to produce a PDF blob and download it; wire
the new buttons to call exportRaster("image/png"), exportRaster("image/jpeg")
and exportPDF(), and ensure accessibility props (aria-pressed/aria-hidden) match
the pattern used by copySvg/printSvg.
In `@example-apps/react-js/src/pages/Editor/index.jsx`:
- Around line 122-131: Pinch math assumes transform-origin at (0,0); because
.inner now uses transform-origin:center center the midpoint calculations drift.
Update the coordinate conversions in the pinch handlers (where
transformRef.current and pinchRef.current.startCx/startCy are set and where
nextTx/nextTy are computed) to account for the element’s transform-origin
(element center) by converting the midpoint into the element’s local coordinates
relative to that origin (subtract elementCenter before dividing by scale, and
add back when computing tx/ty), or revert .inner to transform-origin: 0 0 so the
existing formulas remain valid.
- Line 17: Remove the stray useFullscreen() call that mounts Escape/fullscreen
effects but whose destructured values (ref, toggle) are never used; either
delete the line "const { ref, toggle } = useFullscreen();" from the Editor
component or replace it with the intended usage (attach ref to the editor
element and call toggle where needed) so the hook isn't mounted unnecessarily
and duplicate listeners/lint errors are eliminated.
---
Duplicate comments:
In `@example-apps/react-js/src/components/GlassModal.jsx`:
- Line 1: The modal rendered by the GlassModal component is missing an
accessible name and aria-modal; update the GlassModal component to accept the
title prop (passed from EditorControls.jsx as title="Save image") and apply it
as an accessible name (either aria-label={title} or aria-labelledby pointing to
a visible header element) and add aria-modal="true" alongside role="dialog" so
assistive tech can announce it; ensure the header element (if used) has a
matching id and that the component signature (GlassModal / props) uses the title
prop when rendering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6a57606c-d0f0-48c2-b77f-ddfee4fb2a2f
📒 Files selected for processing (9)
example-apps/react-js/src/components/GlassCard.jsxexample-apps/react-js/src/components/GlassModal.jsxexample-apps/react-js/src/components/GlassModal.module.cssexample-apps/react-js/src/global-styles/base.cssexample-apps/react-js/src/hooks/useFullscreen.jsexample-apps/react-js/src/pages/Editor/Editor.module.cssexample-apps/react-js/src/pages/Editor/EditorControls.jsxexample-apps/react-js/src/pages/Editor/EditorControls.module.cssexample-apps/react-js/src/pages/Editor/index.jsx
✅ Files skipped from review due to trivial changes (1)
- example-apps/react-js/src/pages/Editor/EditorControls.module.css
🚧 Files skipped from review as they are similar to previous changes (2)
- example-apps/react-js/src/global-styles/base.css
- example-apps/react-js/src/components/GlassCard.jsx
Comment on lines
+122
to
+131
| const t = transformRef.current; | ||
|
|
||
| pinchRef.current.active = true; | ||
| pinchRef.current.startDist = Math.hypot(p1.x - p2.x, p1.y - p2.y); | ||
| pinchRef.current.startScale = transform.scale; | ||
| pinchRef.current.startTx = transform.tx; | ||
| pinchRef.current.startTy = transform.ty; | ||
| pinchRef.current.startScale = t.scale; | ||
| pinchRef.current.startTx = t.tx; | ||
| pinchRef.current.startTy = t.ty; | ||
|
|
||
| // content coordinate under the pinch midpoint at start | ||
| pinchRef.current.startCx = (midX - transform.tx) / transform.scale; | ||
| pinchRef.current.startCy = (midY - transform.ty) / transform.scale; | ||
| pinchRef.current.startCx = (midX - t.tx) / t.scale; | ||
| pinchRef.current.startCy = (midY - t.ty) / t.scale; |
Contributor
There was a problem hiding this comment.
Pinch zoom still assumes a top-left transform origin.
startCx/startCy = (mid - tx) / scale and nextTx/nextTy = mid - startC * scale are only correct when .inner scales from (0,0). After Editor.module.css switched .inner to transform-origin: center center, the pinch midpoint will drift instead of staying under the fingers. Either revert the CSS origin or include the element center in these coordinate conversions.
Also applies to: 164-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example-apps/react-js/src/pages/Editor/index.jsx` around lines 122 - 131,
Pinch math assumes transform-origin at (0,0); because .inner now uses
transform-origin:center center the midpoint calculations drift. Update the
coordinate conversions in the pinch handlers (where transformRef.current and
pinchRef.current.startCx/startCy are set and where nextTx/nextTy are computed)
to account for the element’s transform-origin (element center) by converting the
midpoint into the element’s local coordinates relative to that origin (subtract
elementCenter before dividing by scale, and add back when computing tx/ty), or
revert .inner to transform-origin: 0 0 so the existing formulas remain valid.
Owner
Author
There was a problem hiding this comment.
Show me how to include the element center.
Contributor
There was a problem hiding this comment.
@Ryan-Millard, here's how to include the element center (ox, oy) in the pinch coordinate conversions.
Why it drifts
With transform-origin: center center, the browser's effective transform is:
screen_x = ox + tx + (local_x - ox) × scale
screen_y = oy + ty + (local_y - oy) × scale
where ox = element.offsetWidth / 2, oy = element.offsetHeight / 2. The original formula assumed ox = oy = 0 (i.e., top-left origin).
Fix
Step 1 — capture origin at pinch start in onPointerDown:
if (activePointersRef.current.size === 2) {
const [p1, p2] = Array.from(activePointersRef.current.values());
const midX = (p1.x + p2.x) / 2;
const midY = (p1.y + p2.y) / 2;
const t = transformRef.current;
// Capture the transform-origin (center of .inner in layout space)
const innerEl = innerRef.current;
const ox = innerEl ? innerEl.offsetWidth / 2 : 0;
const oy = innerEl ? innerEl.offsetHeight / 2 : 0;
pinchRef.current.active = true;
pinchRef.current.startDist = Math.hypot(p1.x - p2.x, p1.y - p2.y);
pinchRef.current.startScale = t.scale;
pinchRef.current.startTx = t.tx;
pinchRef.current.startTy = t.ty;
pinchRef.current.ox = ox; // ← store origin
pinchRef.current.oy = oy; // ← store origin
// Convert midpoint to local element space, accounting for center origin:
// local = (screen - ox - tx) / scale + ox
pinchRef.current.startCx = (midX - ox - t.tx) / t.scale + ox;
pinchRef.current.startCy = (midY - oy - t.ty) / t.scale + oy;
pointerState.current.moved = true;
}Step 2 — use the stored origin when computing nextTx/nextTy in onPointerMove:
if (pinchRef.current.startDist > 0) {
const scaleFactor = dist / pinchRef.current.startDist;
const nextScale = clamp(pinchRef.current.startScale * scaleFactor, 0.25, 6);
const { startCx, startCy, ox, oy } = pinchRef.current;
// Solve for tx: midX = ox + nextTx + (startCx - ox) * nextScale
const nextTx = midX - ox - (startCx - ox) * nextScale;
const nextTy = midY - oy - (startCy - oy) * nextScale;
updateTransform((t) => {
t.scale = nextScale;
t.tx = nextTx;
t.ty = nextTy;
});
}Quick sanity check
If you set ox = oy = 0, the formulas collapse back to the originals:
startCx = (midX - tx) / scale✓nextTx = midX - startCx * nextScale✓
So this is a backward-compatible generalisation — if you ever revert the CSS origin back to 0 0 these formulas still work correctly.
🧠 Learnings used
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.706Z
Learning: In the Ryan-Millard/Img2Num repository, a PR body containing generic boilerplate such as "This contribution was made as part of an open-source documentation improvement initiative. All changes are meant to be helpful additions to the project." is a strong signal of an AI-generated slop PR and should be explicitly flagged during review.
Owner
Author
There was a problem hiding this comment.
Your implementation doesn't work and I don't want to handle it now because it might take a while. Please create an issue for this and then let me know once you have done that.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo Ryan-Millard/Img2Num \
--title "bug(example-app/react): Pinch zoom drifts when .inner uses transform-origin: center center" \
--body "## Summary
Pinch-zoom in the Editor page drifts instead of staying anchored under the fingers when \`.inner\` uses \`transform-origin: center center\` (introduced in this PR).
## Root cause
The coordinate conversions in \`onPointerDown\` and \`onPointerMove\` (in \`example-apps/react-js/src/pages/Editor/index.jsx\`) were written assuming \`transform-origin: 0 0\` (top-left). After \`Editor.module.css\` switched \`.inner\` to \`transform-origin: center center\`, those formulas are no longer correct and the pinch midpoint drifts.
The affected calculations are:
- \`startCx / startCy\` in \`onPointerDown\`
- \`nextTx / nextTy\` in \`onPointerMove\`
## Expected behaviour
The content under the pinch midpoint should remain stationary while the user zooms in or out with two fingers.
## Steps to reproduce
1. Open the Editor page with an uploaded image.
2. Use two fingers to pinch-zoom on a touch device or touch-capable screen.
3. Observe that the content shifts/drifts rather than scaling around the pinch centre.
## Possible fix
The coordinate conversions need to account for the element's transform-origin (its centre point). See the discussion in PR `#278` (comment: https://github.com/Ryan-Millard/Img2Num/pull/278#discussion_r3034928747) for a proposed approach — it needs further work as the initial attempt did not function correctly.
## References
- PR: https://github.com/Ryan-Millard/Img2Num/pull/278
- Review comment: https://github.com/Ryan-Millard/Img2Num/pull/278#discussion_r3034928747
Reported by `@Ryan-Millard`." \
--assignee Ryan-Millard
Length of output: 50
@Ryan-Millard, done! I've opened issue #294 to track this:
bug(example-app/react): Pinch zoom drifts when .inner uses transform-origin: center center
It's assigned to you and includes a summary of the root cause, steps to reproduce, and a reference back to this PR comment thread for context when you're ready to revisit it.
🧠 Learnings used
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 0
File: :0-0
Timestamp: 2026-04-01T20:45:20.706Z
Learning: In the Ryan-Millard/Img2Num repository, a PR body containing generic boilerplate such as "This contribution was made as part of an open-source documentation improvement initiative. All changes are meant to be helpful additions to the project." is a strong signal of an AI-generated slop PR and should be explicitly flagged during review.
Ryan-Millard
force-pushed
the
feat/example-app/react/download-svg
branch
from
April 4, 2026 01:54
1423f85 to
f56519f
Compare
- useCallback where necessary, remove unused vars, etc. - Position ToolTips at top
Collaborator
|
this seems to work well for zooming and panning |
Ryan-Millard
changed the title
Ryan-Millard
changed the title
- Introduce jsPDF dependency for PDF export - Add exportRaster function for PNG/JPG downloads - Reorganize GlassModal into Export, Clipboard, Other sections - Update CSS for modal layout and responsive toolbar spacing
Ryan-Millard
force-pushed
the
feat/example-app/react/download-svg
branch
from
April 4, 2026 19:12
3d3e363 to
4931be6
Compare
This was referenced May 28, 2026
This was referenced Jun 22, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed & why
New buttons to Editor page to make it more usable.
Fixes: #114
Fixes: #85
Fixes: #288
Changes
New GlassModal and HamburgerMenu. Navbar refactored, Editor page, too.
Testing & Verification
Additional Resources
Summary by CodeRabbit
New Features
Style