perf(ui): unify chrome motion for smoother drawers and marketing - #210
Conversation
… ErrorBoundary - Guard getHTML() calls against null ProseMirror schema in NoteEditor - Add React ErrorBoundary around lazy panels (PythonCodePanel, AlgorithmInsightPanel) - Remove unused Turnstile script tag from index.html
…s Arabic, English, and French locales
github-actions
Bot
added
documentation
📝 WalkthroughWalkthroughThe pull request centralizes UI-chrome motion presets, adds reduced-motion support across panels and marketing surfaces, defers heavy editor and video mounting, removes blur styling from overlays, adds lazy-panel error boundaries, guards editor readiness, introduces video sharing, removes Turnstile loading, and updates localized product copy. ChangesUI motion and responsive surfaces
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant User
participant VisualizerApp
participant useVideoExporter
participant ShareExportModal
participant Analytics
User->>VisualizerApp: Complete video download
VisualizerApp->>useVideoExporter: Read export blob and filename
VisualizerApp->>ShareExportModal: Open sharing dialog
User->>ShareExportModal: Choose native share, X, or clipboard
ShareExportModal->>Analytics: Record sharing platform
ShareExportModal-->>User: Share or copy result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Preview for Bayan Flow Staging ready!
Preview alias |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/landing/LearnYourWay.jsx (1)
36-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAvoid mixing
marketingEnter’s viewport entrance with its mountanimatetarget.
marketingEnter()returns bothanimateandwhileInViewwith the same values, so the header, feature cards, and tagline render at the scroll-into-view final state immediately and the viewport behavior is overridden. Remove the mountanimatetarget from these use sites or splitmarketingEnterinto a scroll-only variant. Also applies to lines 48-56 and 100-106.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/landing/LearnYourWay.jsx` around lines 36 - 44, Update the LearnYourWay motion elements using marketingEnter()—the heading wrapper, feature cards, and tagline—to avoid applying its mount animate target alongside whileInView. Remove the mount animate value at these call sites or use a scroll-only variant, while preserving the existing viewport-triggered entrance behavior.
🧹 Nitpick comments (6)
src/test/framerMotionMock.jsx (1)
48-69: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMock re-fires
onAnimationCompleteon every render, unlike real Framer Motion.Since
onAnimationCompleteis typically an inline callback (new reference each render), including it in the effect's dependency array causes the mock to call it on every re-render of the consumer, not once per completed animation as real Framer Motion does. Current usages (e.g.setMountEditor(true)) are idempotent so this doesn't currently fail, but it could mask a future consumer that isn't guarded against repeated calls.🔧 Proposed fix
return function MotionComponent({ children, onAnimationComplete, animate, ...props }) { const Tag = tag; + const onAnimationCompleteRef = useRef(onAnimationComplete); + onAnimationCompleteRef.current = onAnimationComplete; useEffect(() => { - if (typeof onAnimationComplete !== 'function') return; + if (typeof onAnimationCompleteRef.current !== 'function') return; const definition = typeof animate === 'string' ? animate : animate === undefined ? 'visible' : animate; - onAnimationComplete(definition); - }, [onAnimationComplete, animate]); + onAnimationCompleteRef.current(definition); + }, [animate]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/framerMotionMock.jsx` around lines 48 - 69, Update the useEffect in createMotionComponent so onAnimationComplete callback identity changes do not re-fire the mock completion; track the latest callback separately while triggering the effect only when the animation definition changes, preserving the existing definition resolution and callback behavior for each animation.src/motion/chromeMotion.test.js (1)
7-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage gaps for
bannerInitial/Animate/Exit,modalPanelAnimate/Exit,menuAnimate/Exit, andHOVER_SPRING.Given this module is imported by nearly every chrome UI surface in this cohort, consider adding assertions for the untested exports to guard against accidental shape regressions (e.g., banner exit losing its y-offset, or HOVER_SPRING losing its spring type).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/motion/chromeMotion.test.js` around lines 7 - 77, The chromeMotion tests do not cover the exported banner, modal, menu animation states, or HOVER_SPRING. Extend the existing chromeMotion test suite to assert the expected shapes and key values of bannerInitial, bannerAnimate, bannerExit, modalPanelAnimate, modalPanelExit, menuAnimate, menuExit, and HOVER_SPRING, including banner exit’s y-offset and the spring transition type.src/components/UserMenu.jsx (1)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
reduceMotionisn't applied to this file'swhileHover/whileTapscale effects.
reduceMotionis introduced here but only feeds the account-menu open/close animation (lines 201-204). The sign-in button (lines 146-159) and avatar trigger button (lines 171-196) still unconditionally scale on hover/tap.CookieConsentBanner.jsxguards the equivalent buttons withreduceMotion ? {} : { scale: ... }— consider mirroring that here for consistency with the rest of this PR's reduced-motion contract.♻️ Suggested alignment with CookieConsentBanner's pattern
whileHover={{ scale: 1.02 }} whileTap={{ scale: 0.98 }} + // or: whileHover={reduceMotion ? {} : { scale: 1.02 }} + // whileTap={reduceMotion ? {} : { scale: 0.98 }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/UserMenu.jsx` at line 31, Apply the existing reduceMotion flag to the whileHover and whileTap scale effects on the sign-in button and avatar trigger button, matching the guarded pattern used by CookieConsentBanner. Preserve the current scale animations when reduced motion is disabled, and provide empty animation props when reduceMotion is enabled.src/components/SettingsPanel.jsx (1)
171-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo call sites invent local
{opacity, y}entrance/exit variants instead of extendingchromeMotion.js. Both duplicate a near-identical shape with slightly diverging offsets, which is exactly the drift the module's own contract ("do not invent local spring params for chrome UI") aims to prevent.
src/components/SettingsPanel.jsx#L171-L173: replace the inlinereduceMotion ? { opacity: 1, y: 0 } : { opacity: 0, y: ENTER_Y }/getChromeTransition(reduceMotion)with a new shared helper (e.g.panelInitial(reduceMotion)/panelAnimate()) added tochromeMotion.js.src/pages/ProfileSettingsPage.jsx#L573-L581: replace the inline toastinitial/exitternaries andgetChromeTransition(reduceMotion, CHROME_DURATION_FAST)with the same new shared helper (or a dedicatedtoastInitial/Animate/Exitset), instead of hand-rolling a slightly different y-offset (12 in, 8 out) locally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/SettingsPanel.jsx` around lines 171 - 173, Centralize the chrome UI entrance and exit variants in chromeMotion.js instead of defining local opacity/y objects. Update src/components/SettingsPanel.jsx lines 171-173 and src/pages/ProfileSettingsPage.jsx lines 573-581 to use the shared helper(s), preserving reduced-motion behavior and the existing toast transition timing while removing the local 12/8px offset definitions.src/components/InsightFloatingActionButton.jsx (1)
93-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConflicting top-margin declarations.
mt-44(Tailwind, ≈11rem) inclassNameand the inlinestyle={{ marginTop: '10rem' }}both set top margin; the inline style always wins, makingmt-44dead and the intended offset ambiguous. Consolidate to one declaration.🧹 Proposed fix
className={` flex - fixed ${isRTL ? 'left-0' : 'right-0'} top-1/2 -translate-y-1/2 z-50 mt-44 + fixed ${isRTL ? 'left-0' : 'right-0'} top-1/2 -translate-y-1/2 z-50 h-32 w-14 bg-amber-500 hover:bg-amber-600 disabled:bg-disabled-bg disabled:cursor-not-allowed text-white ${isRTL ? 'rounded-r-xl' : 'rounded-l-xl'} shadow-lg hover:shadow-xl flex-col items-center justify-center gap-2 transition-all duration-200 focus:outline-none focus:ring-2 focus:ring-amber-400 focus:ring-offset-2 `} style={{ marginTop: '10rem' }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/InsightFloatingActionButton.jsx` around lines 93 - 104, Consolidate the top-margin styling on the InsightFloatingActionButton by retaining either the Tailwind mt-44 class or the inline style marginTop, and remove the other declaration. Keep the resulting offset intentional and unambiguous in the component’s className/style configuration.src/components/FloatingActionButton.jsx (1)
26-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicated
isMobiledetection causes a position flash and is repeated verbatim across both FAB components. Both files initializeisMobiletofalseand only correct it after a mount effect, so mobile viewports briefly render the desktop side-bar FAB before flipping to the mobile variant; the same responsive/motion setup logic (isMobile,reduceMotion,enterTransition) is duplicated line-for-line between the two files.
src/components/FloatingActionButton.jsx#L26-L44: extract a shared hook (e.g.useResponsiveChromeMotion()) that returns{ isMobile, reduceMotion, enterTransition }, initializingisMobilewith a lazy state initializer (useState(() => window.innerWidth < 768)) to avoid the flash.src/components/InsightFloatingActionButton.jsx#L23-L44: adopt the same shared hook instead of re-declaring the identicalisMobile/enterTransitionlogic.🔧 Suggested shared-hook sketch
// src/hooks/useResponsiveChromeMotion.js import { useEffect, useState } from 'react'; import { useReducedMotion } from 'framer-motion'; import { getChromeTransition } from '../motion/chromeMotion'; export function useResponsiveChromeMotion(breakpoint = 768) { const reduceMotion = useReducedMotion(); const [isMobile, setIsMobile] = useState( () => typeof window !== 'undefined' && window.innerWidth < breakpoint ); useEffect(() => { const checkMobile = () => setIsMobile(window.innerWidth < breakpoint); checkMobile(); window.addEventListener('resize', checkMobile); return () => window.removeEventListener('resize', checkMobile); }, [breakpoint]); return { isMobile, reduceMotion, enterTransition: getChromeTransition(reduceMotion) }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/FloatingActionButton.jsx` around lines 26 - 44, Extract the duplicated responsive and motion setup into a shared useResponsiveChromeMotion hook, initializing isMobile lazily with a window-safe breakpoint check and retaining resize updates. Update src/components/FloatingActionButton.jsx lines 26-44 and src/components/InsightFloatingActionButton.jsx lines 23-44 to consume the hook’s isMobile, reduceMotion, and enterTransition values, removing their local declarations; both sites require these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ArrayVisualizer.jsx`:
- Line 184: Replace the dimming-only complexity-limit overlays with
SignInPromptModal so gated content is blocked after the anonymous view limit is
exceeded. Apply this change to the complexity panel rendering in
src/components/ArrayVisualizer.jsx (184-184),
src/components/GraphAlgorithmMatrixVisualizer.jsx (106-106),
src/components/GraphVisualizer.jsx (324-324), src/components/GridVisualizer.jsx
(185-185), and src/components/TreeVisualizer.jsx (206-206), preserving each
component’s existing gating behavior.
In `@src/components/ErrorBoundary.jsx`:
- Around line 14-34: Update ErrorBoundary to support recovery by accepting a
resetKey and clearing hasError when that key changes, then pass stable reset
keys such as isOpen or algorithmKey from both PythonCodePanel and
AlgorithmInsightPanel call sites. Replace the null default in
ErrorBoundary.render with a minimal visible fallback that offers a retry action,
while preserving any explicitly provided fallback prop.
In `@src/components/landing/AlgorithmTypes.jsx`:
- Around line 60-62: Update the entrance usage in the AlgorithmTypes component
to omit the immediate animate prop returned by marketingEnter(reduceMotion),
while preserving its whileInView and viewport-triggered behavior so the card
starts only when it enters the viewport.
In `@src/components/landing/Features.jsx`:
- Around line 73-76: Update the scroll-triggered feature header and card motion
elements using marketingEnter so they do not spread its animate property; retain
initial, whileInView, and transition to keep reveals tied to viewport entry,
matching the icon pattern.
In `@src/components/PythonCodePanel.jsx`:
- Around line 353-359: Update the focus-trap keydown handler in the existing
focus-trap effect to query the dialog’s current focusable elements on each
keydown and derive firstElement/lastElement dynamically, rather than capturing
them when isOpen changes. Preserve the existing Tab and Shift+Tab wrapping
behavior while ensuring newly mounted Monaco content is included.
---
Outside diff comments:
In `@src/components/landing/LearnYourWay.jsx`:
- Around line 36-44: Update the LearnYourWay motion elements using
marketingEnter()—the heading wrapper, feature cards, and tagline—to avoid
applying its mount animate target alongside whileInView. Remove the mount
animate value at these call sites or use a scroll-only variant, while preserving
the existing viewport-triggered entrance behavior.
---
Nitpick comments:
In `@src/components/FloatingActionButton.jsx`:
- Around line 26-44: Extract the duplicated responsive and motion setup into a
shared useResponsiveChromeMotion hook, initializing isMobile lazily with a
window-safe breakpoint check and retaining resize updates. Update
src/components/FloatingActionButton.jsx lines 26-44 and
src/components/InsightFloatingActionButton.jsx lines 23-44 to consume the hook’s
isMobile, reduceMotion, and enterTransition values, removing their local
declarations; both sites require these changes.
In `@src/components/InsightFloatingActionButton.jsx`:
- Around line 93-104: Consolidate the top-margin styling on the
InsightFloatingActionButton by retaining either the Tailwind mt-44 class or the
inline style marginTop, and remove the other declaration. Keep the resulting
offset intentional and unambiguous in the component’s className/style
configuration.
In `@src/components/SettingsPanel.jsx`:
- Around line 171-173: Centralize the chrome UI entrance and exit variants in
chromeMotion.js instead of defining local opacity/y objects. Update
src/components/SettingsPanel.jsx lines 171-173 and
src/pages/ProfileSettingsPage.jsx lines 573-581 to use the shared helper(s),
preserving reduced-motion behavior and the existing toast transition timing
while removing the local 12/8px offset definitions.
In `@src/components/UserMenu.jsx`:
- Line 31: Apply the existing reduceMotion flag to the whileHover and whileTap
scale effects on the sign-in button and avatar trigger button, matching the
guarded pattern used by CookieConsentBanner. Preserve the current scale
animations when reduced motion is disabled, and provide empty animation props
when reduceMotion is enabled.
In `@src/motion/chromeMotion.test.js`:
- Around line 7-77: The chromeMotion tests do not cover the exported banner,
modal, menu animation states, or HOVER_SPRING. Extend the existing chromeMotion
test suite to assert the expected shapes and key values of bannerInitial,
bannerAnimate, bannerExit, modalPanelAnimate, modalPanelExit, menuAnimate,
menuExit, and HOVER_SPRING, including banner exit’s y-offset and the spring
transition type.
In `@src/test/framerMotionMock.jsx`:
- Around line 48-69: Update the useEffect in createMotionComponent so
onAnimationComplete callback identity changes do not re-fire the mock
completion; track the latest callback separately while triggering the effect
only when the animation definition changes, preserving the existing definition
resolution and callback behavior for each animation.
🪄 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 Plus
Run ID: af6c398c-3dca-459f-a913-68b2d03dab5b
📒 Files selected for processing (53)
docs/AGENTS_REFERENCE.mddocs/ARCHITECTURE.mdindex.htmlsrc/components/AlgorithmDropdown.jsxsrc/components/AlgorithmInsightPanel.jsxsrc/components/AlgorithmInsightPanel.test.jsxsrc/components/ArrayVisualizer.jsxsrc/components/AutoHidingLegend.jsxsrc/components/ComplexityPanel.jsxsrc/components/ControlPanel.jsxsrc/components/CookieConsentBanner.jsxsrc/components/ErrorBoundary.jsxsrc/components/ErrorBoundary.test.jsxsrc/components/ExportProgressModal.jsxsrc/components/FavoritesDropdown.jsxsrc/components/FloatingActionButton.jsxsrc/components/FloatingActionButton.test.jsxsrc/components/GraphAlgorithmMatrixVisualizer.jsxsrc/components/GraphAlgorithmMatrixVisualizer.test.jsxsrc/components/GraphScenarioDropdown.jsxsrc/components/GraphVisualizer.jsxsrc/components/GraphVisualizer.test.jsxsrc/components/GridVisualizer.jsxsrc/components/InsightFloatingActionButton.jsxsrc/components/LanguageSwitcher.jsxsrc/components/NoteEditor.jsxsrc/components/NoteEditor.test.jsxsrc/components/PythonCodePanel.jsxsrc/components/PythonCodePanel.test.jsxsrc/components/SettingsPanel.jsxsrc/components/SignInPromptModal.jsxsrc/components/SwipeTutorial.jsxsrc/components/TreeVisualizer.jsxsrc/components/UserMenu.jsxsrc/components/landing/AlgorithmTypes.jsxsrc/components/landing/ClaritySection.jsxsrc/components/landing/Features.jsxsrc/components/landing/Hero.jsxsrc/components/landing/LearnYourWay.jsxsrc/components/landing/RoadmapCTA.jsxsrc/components/landing/TechPattern.jsxsrc/components/roadmap/RoadmapHero.jsxsrc/components/roadmap/Timeline.jsxsrc/components/roadmap/TimelineItem.jsxsrc/i18n/locales/ar/translation.jsonsrc/i18n/locales/en/translation.jsonsrc/i18n/locales/fr/translation.jsonsrc/motion/chromeMotion.jssrc/motion/chromeMotion.test.jssrc/pages/ProfileSettingsPage.jsxsrc/pages/VisualizerApp.jsxsrc/test/framerMotionMock.jsxsupabase/functions/_shared/transactionalEmails.ts
💤 Files with no reviewable changes (1)
- index.html
| initial={{ opacity: 0 }} | ||
| animate={{ opacity: 1 }} | ||
| className="absolute inset-0 backdrop-blur-md bg-black/30 z-10" | ||
| className="absolute inset-0 bg-black/30 z-10" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
The complexity-limit overlay no longer blocks gated content.
Each component renders ComplexityPanel beneath bg-black/30, which only dims the content and leaves it readable after the anonymous view limit is exceeded. Replace this with the required blocked state or SignInPromptModal.
src/components/ArrayVisualizer.jsx#L184-L184: block the gated array complexity panel.src/components/GraphAlgorithmMatrixVisualizer.jsx#L106-L106: block the gated matrix complexity panel.src/components/GraphVisualizer.jsx#L324-L324: block the gated graph complexity panel.src/components/GridVisualizer.jsx#L185-L185: block the gated grid complexity panel.src/components/TreeVisualizer.jsx#L206-L206: block the gated tree complexity panel.
As per coding guidelines, gated features must be blocked with SignInPromptModal.
📍 Affects 5 files
src/components/ArrayVisualizer.jsx#L184-L184(this comment)src/components/GraphAlgorithmMatrixVisualizer.jsx#L106-L106src/components/GraphVisualizer.jsx#L324-L324src/components/GridVisualizer.jsx#L185-L185src/components/TreeVisualizer.jsx#L206-L206
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ArrayVisualizer.jsx` at line 184, Replace the dimming-only
complexity-limit overlays with SignInPromptModal so gated content is blocked
after the anonymous view limit is exceeded. Apply this change to the complexity
panel rendering in src/components/ArrayVisualizer.jsx (184-184),
src/components/GraphAlgorithmMatrixVisualizer.jsx (106-106),
src/components/GraphVisualizer.jsx (324-324), src/components/GridVisualizer.jsx
(185-185), and src/components/TreeVisualizer.jsx (206-206), preserving each
component’s existing gating behavior.
Source: Coding guidelines
| class ErrorBoundary extends Component { | ||
| constructor(props) { | ||
| super(props); | ||
| this.state = { hasError: false }; | ||
| } | ||
|
|
||
| static getDerivedStateFromError() { | ||
| return { hasError: true }; | ||
| } | ||
|
|
||
| componentDidCatch(error, info) { | ||
| console.error('[ErrorBoundary]', error, info?.componentStack); | ||
| } | ||
|
|
||
| render() { | ||
| if (this.state.hasError) { | ||
| return this.props.fallback ?? null; | ||
| } | ||
| return this.props.children; | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Error boundary never recovers and provides no fallback UI — failures become permanent and silent.
hasError is only ever set, never reset (no componentDidUpdate/resetKeys comparison, no remount trigger). Combined with both call sites in src/pages/VisualizerApp.jsx (lines 957-979) omitting the fallback prop, any render error in PythonCodePanel or AlgorithmInsightPanel permanently renders null for the rest of the session — the FAB buttons stay clickable but nothing happens, with no user-facing error message or retry option. This undermines the "resilience" goal of wrapping these lazy panels.
🔧 Suggested fix: add reset support + a minimal fallback
class ErrorBoundary extends Component {
constructor(props) {
super(props);
this.state = { hasError: false };
}
static getDerivedStateFromError() {
return { hasError: true };
}
componentDidCatch(error, info) {
console.error('[ErrorBoundary]', error, info?.componentStack);
}
+ componentDidUpdate(prevProps) {
+ if (this.state.hasError && prevProps.resetKey !== this.props.resetKey) {
+ this.setState({ hasError: false });
+ }
+ }
+
render() {
if (this.state.hasError) {
- return this.props.fallback ?? null;
+ return this.props.fallback ?? this.props.children ? null : null;
}
return this.props.children;
}
}Then pass a resetKey (e.g. isOpen or algorithmKey) from call sites so closing/reopening the panel clears the error, and consider a lightweight visible fallback instead of null.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ErrorBoundary.jsx` around lines 14 - 34, Update ErrorBoundary
to support recovery by accepting a resetKey and clearing hasError when that key
changes, then pass stable reset keys such as isOpen or algorithmKey from both
PythonCodePanel and AlgorithmInsightPanel call sites. Replace the null default
in ErrorBoundary.render with a minimal visible fallback that offers a retry
action, while preserving any explicitly provided fallback prop.
| useEffect(() => { | ||
| if (!isOpen) { | ||
| setMountEditor(false); | ||
| } else if (reduceMotion) { | ||
| setMountEditor(true); | ||
| } | ||
| }, [isOpen, reduceMotion]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Deferred Monaco mount stales the focus-trap boundaries, breaking keyboard navigation into the editor.
The focus trap (existing effect around lines 93-124) computes firstElement/lastElement once when isOpen becomes true, but at that point mountEditor is still false (Monaco not yet in the DOM) unless reduceMotion is set. Once Monaco mounts later via onAnimationComplete, the trap's cached lastElement still points to whatever was last focusable before Monaco existed (e.g. the "python" tab button). Tabbing forward from that element re-triggers document.activeElement === lastElement and forces focus back to firstElement, so keyboard-only users can never reach Monaco.
Recompute the focusable boundaries dynamically (inside the keydown handler) instead of caching stale DOM references, so newly-mounted content is always included:
♿ Proposed fix
useEffect(() => {
if (isOpen && panelRef.current) {
- const focusableElements = panelRef.current.querySelectorAll(
- 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'
- );
- const firstElement = focusableElements[0];
- const lastElement = focusableElements[focusableElements.length - 1];
-
const handleKeyDown = event => {
+ const focusableElements = panelRef.current.querySelectorAll(
+ 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'
+ );
+ const firstElement = focusableElements[0];
+ const lastElement = focusableElements[focusableElements.length - 1];
if (event.key === 'Escape') {
onClose();
} else if (event.key === 'Tab') {
if (event.shiftKey) {
if (document.activeElement === firstElement) {
event.preventDefault();
lastElement.focus();
}
} else {
if (document.activeElement === lastElement) {
event.preventDefault();
firstElement.focus();
}
}
}
};
document.addEventListener('keydown', handleKeyDown);
- firstElement?.focus();
+ panelRef.current
+ .querySelector('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])')
+ ?.focus();
return () => document.removeEventListener('keydown', handleKeyDown);
}
}, [isOpen, onClose]);Also applies to: 509-577
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/PythonCodePanel.jsx` around lines 353 - 359, Update the
focus-trap keydown handler in the existing focus-trap effect to query the
dialog’s current focusable elements on each keydown and derive
firstElement/lastElement dynamically, rather than capturing them when isOpen
changes. Preserve the existing Tab and Shift+Tab wrapping behavior while
ensuring newly mounted Monaco content is included.
07c9c01 to
0e03f54
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/VisualizerApp.jsx (1)
958-966: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd loading feedback around the lazy
PythonCodePanel.
VisualizerAppnow usesSuspense fallback={null}, andPythonCodePanelstarts withmountEditor=false, so the drawer opens before any internal loading state. Wrap the lazy component with an explicit loader or show an internal loading/skeleton overlay as soon asisOpenis true until the editor/chart content mounts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/VisualizerApp.jsx` around lines 958 - 966, Update the Suspense boundary in VisualizerApp around PythonCodePanel to provide visible loading feedback instead of fallback={null}. Show an explicit loader or skeleton immediately when isPythonPanelOpen is true, and preserve the existing panel behavior once the lazy component and its editor/chart content mount.
♻️ Duplicate comments (1)
src/components/landing/AlgorithmTypes.jsx (1)
61-62: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
marketingEnter()reintroduces the animate/whileInView scroll-gating conflict via the new shared helper.
marketingEnter(reduceMotion, delay)(chromeMotion.js) spreadsanimate: { opacity: 1, y: 0 }alongsidewhileInView: { opacity: 1, y: 0 }with the same target. Per Framer Motion's documented behavior,animatefires immediately on mount regardless of viewport, so the heading (line 61) and each algorithm card (line 79) will render at full opacity/position as soon as they mount rather than waiting to scroll into view — theviewport={{ once: true }}gating on both sites becomes ineffective.This is the same conflict flagged and previously "addressed" on this file in an earlier commit, but the fix doesn't appear to have carried over into the new centralized
marketingEnter()helper — meaning the regression is now reintroduced here and propagates to every other landing section that adoptsmarketingEnter().Fix belongs in
chromeMotion.js: dropanimatefrommarketingEnter's non-reduced-motion branch (rely oninitial+whileInViewfor scroll-gated entrance); keepanimate(or none) for the reduced-motion branch where immediate visibility is intended anyway.Does Framer Motion animate prop override or race with whileInView when targeting the same values?Also applies to: 79-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/landing/AlgorithmTypes.jsx` around lines 61 - 62, Update the non-reduced-motion branch of marketingEnter in chromeMotion.js to omit animate, leaving initial and whileInView to control the scroll-gated entrance; preserve the reduced-motion branch’s immediate visibility behavior. This ensures callers such as the heading and algorithm cards in AlgorithmTypes do not become visible before entering the viewport.
🧹 Nitpick comments (3)
src/components/ControlPanel.test.jsx (1)
13-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
stubMatchMediahelper across test files. Both files define an identical helper that stubsmatchMediaagainstBELOW_LG_MEDIA_QUERY; as more specs adoptuseIsBelowLg, this copy-paste will keep spreading.
src/components/ControlPanel.test.jsx#L13-L25: replace with an import from a shared helper (e.g.src/test/matchMediaMock.jsor an addition tosrc/test/testUtils.jsx).src/components/FloatingActionButton.test.jsx#L12-L24: same — import the shared helper instead of redefining it locally.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/ControlPanel.test.jsx` around lines 13 - 25, Extract the duplicated stubMatchMedia helper into a shared test utility, then remove the local definitions and import the shared helper in src/components/ControlPanel.test.jsx lines 13-25 and src/components/FloatingActionButton.test.jsx lines 12-24. Preserve its existing BELOW_LG_MEDIA_QUERY matching behavior and update both test files to use the shared symbol.src/hooks/useIsBelowLg.test.js (1)
11-72: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a regression test for
matchMediareturningundefined.This suite covers the mocked "happy path" but not the exact scenario causing the current CI failures (
matchMediapresent as a function but returningundefined/null), nor the SSR/matchMedia-unavailable fallback branch. Once the hook is patched, a regression test here would guard against reintroducing the crash.As per path instructions,
src/**/*.{test,spec}.{js,jsx}: "update focused tests when adding algorithms, scenarios, sound events, or export behavior" — this hook's runtime-guard behavior is exactly the kind of edge case that should have focused coverage. Want me to add a test stubbingmatchMediato returnundefined?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useIsBelowLg.test.js` around lines 11 - 72, Add focused regression coverage in the useIsBelowLg test suite for matchMedia being available but returning undefined or null, and verify the hook uses its safe fallback without throwing. Also cover the matchMedia-unavailable/SSR path if supported by the hook’s runtime guard, preserving the existing happy-path tests.Source: Path instructions
src/components/InsightFloatingActionButton.jsx (1)
23-118: 📐 Maintainability & Code Quality | 🔵 TrivialCorrect reduced-motion wiring; consider extracting shared FAB motion logic.
The reduced-motion, mobile/desktop branching, and hover/tap wiring here is functionally correct and mirrors
FloatingActionButton.jsxalmost line-for-line (sameisMobilebranch,enterTransition,whileHover/whileTapshapes). That duplication will need to be kept in sync manually for any future FAB motion tweaks.Consider extracting a shared hook (e.g.
useFabMotion(reduceMotion, isGated, disabled)returningenterTransition/whileHover/whileTap) or a genericFabButtonwrapper that both FABs compose, keeping only color/icon/position as call-site differences.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/InsightFloatingActionButton.jsx` around lines 23 - 118, Extract the duplicated FAB motion behavior from InsightFloatingActionButton and FloatingActionButton into a shared useFabMotion hook or generic FabButton component. Centralize reduced-motion handling, enterTransition, whileHover, and whileTap behavior while preserving the existing mobile/desktop positioning and each button’s visual content and styling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/ControlPanel.jsx`:
- Around line 333-361: Update the more-actions disclosure in ControlPanel around
isMoreOpen and featureButtons to use semantics matching its current plain-Tab
interaction: remove role="menu" from the dropdown and aria-haspopup="menu" from
the toggle, and use a non-menu grouping role if needed. Preserve the existing
toggle, positioning, and feature button behavior without adding menu keyboard
navigation.
In `@src/components/SettingsSheet.jsx`:
- Around line 52-101: Implement a focus trap for the open dialog controlled by
panelRef, intercepting Tab and Shift+Tab to cycle between the sheet’s first and
last focusable elements. Attach and clean up the keyboard handling with the
modal lifecycle, preserve the existing initial close-button focus behavior, and
ensure focus cannot move into the background while isOpen is true.
In `@src/i18n/locales/fr/translation.json`:
- Around line 368-370: Update the openSettings, closeSettings, and
settingsSheetTitle translations in the French locale to use “Paramètres”
consistently, matching the established settings.title terminology while
preserving the existing translation meaning.
---
Outside diff comments:
In `@src/pages/VisualizerApp.jsx`:
- Around line 958-966: Update the Suspense boundary in VisualizerApp around
PythonCodePanel to provide visible loading feedback instead of fallback={null}.
Show an explicit loader or skeleton immediately when isPythonPanelOpen is true,
and preserve the existing panel behavior once the lazy component and its
editor/chart content mount.
---
Duplicate comments:
In `@src/components/landing/AlgorithmTypes.jsx`:
- Around line 61-62: Update the non-reduced-motion branch of marketingEnter in
chromeMotion.js to omit animate, leaving initial and whileInView to control the
scroll-gated entrance; preserve the reduced-motion branch’s immediate visibility
behavior. This ensures callers such as the heading and algorithm cards in
AlgorithmTypes do not become visible before entering the viewport.
---
Nitpick comments:
In `@src/components/ControlPanel.test.jsx`:
- Around line 13-25: Extract the duplicated stubMatchMedia helper into a shared
test utility, then remove the local definitions and import the shared helper in
src/components/ControlPanel.test.jsx lines 13-25 and
src/components/FloatingActionButton.test.jsx lines 12-24. Preserve its existing
BELOW_LG_MEDIA_QUERY matching behavior and update both test files to use the
shared symbol.
In `@src/components/InsightFloatingActionButton.jsx`:
- Around line 23-118: Extract the duplicated FAB motion behavior from
InsightFloatingActionButton and FloatingActionButton into a shared useFabMotion
hook or generic FabButton component. Centralize reduced-motion handling,
enterTransition, whileHover, and whileTap behavior while preserving the existing
mobile/desktop positioning and each button’s visual content and styling.
In `@src/hooks/useIsBelowLg.test.js`:
- Around line 11-72: Add focused regression coverage in the useIsBelowLg test
suite for matchMedia being available but returning undefined or null, and verify
the hook uses its safe fallback without throwing. Also cover the
matchMedia-unavailable/SSR path if supported by the hook’s runtime guard,
preserving the existing happy-path tests.
🪄 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 Plus
Run ID: fa159e2c-977c-4527-a661-b2b88b892859
📒 Files selected for processing (55)
docs/AGENTS_REFERENCE.mddocs/ARCHITECTURE.mdsrc/components/AlgorithmDropdown.jsxsrc/components/AlgorithmInsightPanel.jsxsrc/components/AlgorithmInsightPanel.test.jsxsrc/components/ArrayBar.jsxsrc/components/ArrayVisualizer.jsxsrc/components/AutoHidingLegend.jsxsrc/components/ComplexityPanel.jsxsrc/components/ControlPanel.jsxsrc/components/ControlPanel.test.jsxsrc/components/CookieConsentBanner.jsxsrc/components/ExportProgressModal.jsxsrc/components/FavoritesDropdown.jsxsrc/components/FloatingActionButton.jsxsrc/components/FloatingActionButton.test.jsxsrc/components/GraphAlgorithmMatrixVisualizer.jsxsrc/components/GraphAlgorithmMatrixVisualizer.test.jsxsrc/components/GraphScenarioDropdown.jsxsrc/components/GraphVisualizer.jsxsrc/components/GraphVisualizer.test.jsxsrc/components/GridVisualizer.jsxsrc/components/Header.jsxsrc/components/InsightFloatingActionButton.jsxsrc/components/LanguageSwitcher.jsxsrc/components/ProWaitlistBanner.jsxsrc/components/PythonCodePanel.jsxsrc/components/PythonCodePanel.test.jsxsrc/components/SettingsPanel.jsxsrc/components/SettingsSheet.jsxsrc/components/SettingsSheet.test.jsxsrc/components/SignInPromptModal.jsxsrc/components/SwipeTutorial.jsxsrc/components/TreeVisualizer.jsxsrc/components/UserMenu.jsxsrc/components/landing/AlgorithmTypes.jsxsrc/components/landing/ClaritySection.jsxsrc/components/landing/Features.jsxsrc/components/landing/Hero.jsxsrc/components/landing/LearnYourWay.jsxsrc/components/landing/RoadmapCTA.jsxsrc/components/landing/TechPattern.jsxsrc/components/roadmap/RoadmapHero.jsxsrc/components/roadmap/Timeline.jsxsrc/components/roadmap/TimelineItem.jsxsrc/hooks/useIsBelowLg.jssrc/hooks/useIsBelowLg.test.jssrc/i18n/locales/ar/translation.jsonsrc/i18n/locales/en/translation.jsonsrc/i18n/locales/fr/translation.jsonsrc/motion/chromeMotion.jssrc/motion/chromeMotion.test.jssrc/pages/ProfileSettingsPage.jsxsrc/pages/VisualizerApp.jsxsrc/test/framerMotionMock.jsx
🚧 Files skipped from review as they are similar to previous changes (33)
- src/components/SwipeTutorial.jsx
- src/components/GraphAlgorithmMatrixVisualizer.test.jsx
- src/components/landing/RoadmapCTA.jsx
- src/components/GraphAlgorithmMatrixVisualizer.jsx
- src/components/GraphVisualizer.test.jsx
- src/components/roadmap/RoadmapHero.jsx
- src/test/framerMotionMock.jsx
- src/pages/ProfileSettingsPage.jsx
- src/components/AlgorithmInsightPanel.test.jsx
- src/components/GridVisualizer.jsx
- src/components/GraphVisualizer.jsx
- src/components/ExportProgressModal.jsx
- src/components/CookieConsentBanner.jsx
- src/components/UserMenu.jsx
- src/components/SignInPromptModal.jsx
- src/components/AutoHidingLegend.jsx
- src/components/roadmap/Timeline.jsx
- src/components/landing/ClaritySection.jsx
- src/motion/chromeMotion.js
- src/components/GraphScenarioDropdown.jsx
- src/components/landing/LearnYourWay.jsx
- src/components/FavoritesDropdown.jsx
- src/components/landing/Hero.jsx
- src/components/landing/Features.jsx
- src/components/ComplexityPanel.jsx
- docs/AGENTS_REFERENCE.md
- src/components/TreeVisualizer.jsx
- src/components/SettingsPanel.jsx
- src/motion/chromeMotion.test.js
- src/components/roadmap/TimelineItem.jsx
- src/components/landing/TechPattern.jsx
- src/components/AlgorithmInsightPanel.jsx
- src/components/PythonCodePanel.jsx
| return ( | ||
| <AnimatePresence> | ||
| {isOpen ? ( | ||
| <> | ||
| <motion.div | ||
| key="settings-sheet-backdrop" | ||
| className={`fixed inset-0 ${OVERLAY_CLASS} z-50`} | ||
| initial={{ opacity: 0 }} | ||
| animate={{ opacity: 1 }} | ||
| exit={{ opacity: 0 }} | ||
| transition={fadeOverlayTransition(reduceMotion)} | ||
| onClick={onClose} | ||
| aria-hidden="true" | ||
| /> | ||
| <motion.div | ||
| key="settings-sheet-panel" | ||
| ref={panelRef} | ||
| role="dialog" | ||
| aria-modal="true" | ||
| aria-label={t('controls.settingsSheetTitle')} | ||
| className="fixed inset-0 z-50 flex flex-col bg-surface shadow-2xl overflow-hidden overscroll-contain" | ||
| variants={sheetPanelVariants()} | ||
| initial="hidden" | ||
| animate="visible" | ||
| exit="exit" | ||
| transition={getChromeTransition(reduceMotion)} | ||
| > | ||
| <div className="flex items-center justify-between gap-3 shrink-0 px-4 pt-[max(0.75rem,env(safe-area-inset-top))] pb-2 border-b border-[var(--color-border-strong)]"> | ||
| <h2 className="text-base font-semibold text-text-primary"> | ||
| {t('controls.settingsSheetTitle')} | ||
| </h2> | ||
| <button | ||
| type="button" | ||
| data-settings-sheet-close | ||
| onClick={onClose} | ||
| className="inline-flex items-center justify-center h-touch w-touch min-h-touch min-w-touch rounded-lg bg-surface-elevated text-text-primary hover:bg-border touch-manipulation" | ||
| aria-label={t('controls.closeSettings')} | ||
| > | ||
| <X size={20} weight="bold" aria-hidden="true" /> | ||
| </button> | ||
| </div> | ||
| <div className="flex-1 min-h-0 overflow-y-auto overscroll-contain px-3 pb-[max(1rem,env(safe-area-inset-bottom))] pt-3"> | ||
| <SettingsPanel {...settingsPanelProps} /> | ||
| </div> | ||
| </motion.div> | ||
| </> | ||
| ) : null} | ||
| </AnimatePresence> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Missing focus trap for a modal dialog (aria-modal="true").
The sheet sets role="dialog" aria-modal="true" and focuses the close button on open, but nothing constrains Tab/Shift+Tab to the dialog's focusable elements. Keyboard and screen-reader users can tab out into the (visually hidden but still-mounted) background UI, breaking the modal contract aria-modal asserts.
Add a focus trap (e.g., cycle focus between the first/last focusable elements within panelRef on Tab, or use a small dependency such as focus-trap-react) so keyboard focus stays inside the sheet while open.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/SettingsSheet.jsx` around lines 52 - 101, Implement a focus
trap for the open dialog controlled by panelRef, intercepting Tab and Shift+Tab
to cycle between the sheet’s first and last focusable elements. Attach and clean
up the keyboard handling with the modal lifecycle, preserve the existing initial
close-button focus behavior, and ensure focus cannot move into the background
while isOpen is true.
| "openSettings": "Réglages", | ||
| "closeSettings": "Fermer les réglages", | ||
| "settingsSheetTitle": "Réglages de l’algorithme", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the established French settings terminology.
settings.title uses “Paramètres” at Line 128, but these new labels use “Réglages”. Use “Paramètres” consistently for the settings button, close label, and sheet title.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/i18n/locales/fr/translation.json` around lines 368 - 370, Update the
openSettings, closeSettings, and settingsSheetTitle translations in the French
locale to use “Paramètres” consistently, matching the established settings.title
terminology while preserving the existing translation meaning.
…lose buttons - Legend: center info icon and close button on mobile (44px touch target), match SettingsSheet card pattern for close button - Array viz: wrap items into multiple rows instead of horizontal scroll - Control panel: remove More Actions dropdown, show all feature buttons inline in second row alongside Settings gear - Close buttons: unify Insight and Code panels to match SettingsSheet card pattern (bg-surface-elevated, h-touch w-touch) on mobile - i18n: add missing 'legend.show' key for English locale
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/AutoHidingLegend.jsx (1)
95-100: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate the collapsed legend button’s CSS hover motion for reduced-motion users.
useReducedMotion()only controls the Framer Motion props; the Tailwindhover:scale-105 transition-allremains active. Scope the hover transform and transition withmotion-safe:or remove the scale effect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/AutoHidingLegend.jsx` around lines 95 - 100, Update the collapsed legend button’s className in AutoHidingLegend to gate the hover scale transform and transition behind Tailwind’s motion-safe variant, or remove the scale effect, so reduced-motion users do not receive CSS hover motion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/AutoHidingLegend.jsx`:
- Around line 77-80: Update the JSX <div> element in AutoHidingLegend so its key
and className props appear on the same line, preserving their existing values
and behavior.
---
Outside diff comments:
In `@src/components/AutoHidingLegend.jsx`:
- Around line 95-100: Update the collapsed legend button’s className in
AutoHidingLegend to gate the hover scale transform and transition behind
Tailwind’s motion-safe variant, or remove the scale effect, so reduced-motion
users do not receive CSS hover motion.
🪄 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 Plus
Run ID: cfaeeb5e-50fa-45f3-9506-8e6dff4ed384
📒 Files selected for processing (8)
src/components/AlgorithmInsightPanel.jsxsrc/components/ArrayBar.jsxsrc/components/ArrayVisualizer.jsxsrc/components/AutoHidingLegend.jsxsrc/components/ControlPanel.jsxsrc/components/ControlPanel.test.jsxsrc/components/PythonCodePanel.jsxsrc/i18n/locales/en/translation.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/ArrayBar.jsx
- src/components/AlgorithmInsightPanel.jsx
- src/i18n/locales/en/translation.json
- src/components/ArrayVisualizer.jsx
- src/components/PythonCodePanel.jsx
…e display - Add ShareExportModal with native share, X (Twitter), and clipboard copy - Add generateShareCaption utility with smart dedup (avoids 'Algorithm Algorithm') - Pass activeAlgorithmName directly as string prop (fixes race condition with export session state) - Add share translations (en/fr/ar) and share-related export preview strings - Add VIDEO_SHARED analytics event and tracking function - Add getExportBlob and exportFileName to useVideoExporter - Add exportAlgorithmMeta tracking in useVideoExporter for metadata preservation - Remove unused shareMeta state and dead shareExport.notNow keys from locales - Fix AutoHidingLegend key prop formatting
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/pages/VisualizerApp.test.jsx (1)
216-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the share-modal integration contract.
The mock ignores the newly introduced blob, filename, metadata, and share callback props, so this suite cannot detect broken wiring after download completion. Add a focused test that opens the modal and verifies those props plus close behavior.
Also applies to: 449-451
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/VisualizerApp.test.jsx` around lines 216 - 225, Extend the ShareExportModal mock and add a focused VisualizerApp test that opens the share modal after download completion, captures and verifies the blob, filename, metadata, and share callback props, and confirms onClose closes the modal. Keep the existing closed-state behavior intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/Footer.jsx`:
- Around line 184-186: Update the LICENSE link in the Footer component to build
its href from the existing GITHUB_REPO_URL constant, targeting the main branch
with the /blob/main/LICENSE suffix. Remove the hardcoded repository URL and
develop branch while preserving the existing link attributes.
In `@src/components/ShareExportModal.jsx`:
- Around line 20-22: Update ShareExportModal so generated user-editable captions
and native-share titles come from shareExport translations instead of hardcoded
English templates. Keep the canonical URL as a separate untranslated value, add
or update the corresponding English, French, and Arabic locale entries, and
preserve Arabic RTL behavior.
- Around line 42-43: Update the ShareExportModal state handling around
shareData, caption, and copied so that whenever open changes to true, caption is
reset from the current shareData.fullShareText and copied is reset to false. Add
a rerender test covering consecutive exports with different algorithmName values
while reusing the mounted modal.
---
Nitpick comments:
In `@src/pages/VisualizerApp.test.jsx`:
- Around line 216-225: Extend the ShareExportModal mock and add a focused
VisualizerApp test that opens the share modal after download completion,
captures and verifies the blob, filename, metadata, and share callback props,
and confirms onClose closes the modal. Keep the existing closed-state behavior
intact.
🪄 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 Plus
Run ID: 34e31cc1-da2e-4a90-a3c4-29698f567f6c
📒 Files selected for processing (14)
src/components/AutoHidingLegend.jsxsrc/components/Footer.jsxsrc/components/ShareExportModal.jsxsrc/components/ShareExportModal.test.jsxsrc/i18n/locales/ar/translation.jsonsrc/i18n/locales/en/translation.jsonsrc/i18n/locales/fr/translation.jsonsrc/pages/VisualizerApp.jsxsrc/pages/VisualizerApp.test.jsxsrc/services/analyticsEvents.jssrc/utils/shareCaption.jssrc/utils/shareCaption.test.jssrc/video/useVideoExporter.jssrc/video/useVideoExporter.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/i18n/locales/ar/translation.json
- src/pages/VisualizerApp.jsx
| href="https://github.com/ayoub3bidi/bayan-flow/blob/develop/LICENSE" | ||
| target="_blank" | ||
| rel="noopener noreferrer" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find Footer.jsx and relevant symbols"
fd -a 'Footer\.jsx$' . | sed 's#^\./##'
echo
echo "Search GITHUB_REPO_URL usage:"
rg -n "GITHUB_REPO_URL|github\.com/ayoub3bidi/bayan-flow|/blob/(develop|main)/|LICEN[CS]E" -S .
echo
echo "Footer outline:"
footer="$(fd 'Footer\.jsx$' . | head -n 1)"
if [ -n "${footer:-}" ]; then
ast-grep outline "$footer" --view expanded || true
echo
echo "Footer relevant lines:"
nl -ba "$footer" | sed -n '1,240p' | grep -nE "GITHUB_REPO_URL|LICENSE|href=|target|repo|github" -C 3
fiRepository: ayoub3bidi/bayan-flow
Length of output: 30807
🏁 Script executed:
#!/bin/bash
set -euo pipefail
footer="$(fd 'Footer\.jsx$' . | head -n 1)"
constants="$(fd 'githubRepo\.js$' . | head -n 1)"
echo "Footer.jsx lines 1-215"
python3 - <<'PY' "$footer"
import sys
path=sys.argv[1]
for i,line in enumerate(open(path, encoding='utf-8'),1):
if 1 <= i <= 215:
print(f"{i:4d}: {line}", end='')
PY
echo
echo "githubRepo constants"
python3 - <<'PY' "$constants"
import sys
path=sys.argv[1]
for i,line in enumerate(open(path, encoding='utf-8'),1):
print(f"{i:4d}: {line}", end='')
PY
echo
echo "Repository branch references in GitHub URLs"
python3 - <<'PY'
import re, sys
patterns = [re.compile(r'https?://github\.com/([^/\s]+)/([^/\s]+?)(?:\.git)?(?=\s|$|\n)'),
re.compile(r'/blob/([A-Za-z0-9._-]+)/LICENSE'),
re.compile(r'/tree/([A-Za-z0-9._-]+)/')]
for name in ['src/components/Footer.jsx','AGENTS.md','CONTRIBUTING.md','package.json']:
try:
data=open(name,encoding='utf-8').read()
except Exception as e:
print(f"{name}: ERROR {e}")
continue
for r in data.splitlines():
if 'github.com' in r or 'LICENSE' in r:
print(f"{name}: {r}")
PYRepository: ayoub3bidi/bayan-flow
Length of output: 11106
Use the canonical repository URL and branch for the LICENSE link.
This link hardcodes github.com/ayoub3bidi/bayan-flow and develop in src/components/Footer.jsx, while the Footer already uses GITHUB_REPO_URL and main for repository links. Generate it as ${GITHUB_REPO_URL}/blob/main/LICENSE so the legal link stays consistent with the repository configuration and production branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/Footer.jsx` around lines 184 - 186, Update the LICENSE link in
the Footer component to build its href from the existing GITHUB_REPO_URL
constant, targeting the main branch with the /blob/main/LICENSE suffix. Remove
the hardcoded repository URL and develop branch while preserving the existing
link attributes.
|
|
||
| /** | ||
| * Modal shown after video download offering to share on social media. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Localize the generated share copy.
These strings appear in the user-editable caption and native-share title, but remain English for French and Arabic users. Move the templates to shareExport translations and preserve the canonical URL separately. As per coding guidelines, “Audit all English, French, and Arabic locales … for user-facing category or algorithm renames; preserve Arabic RTL behavior.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ShareExportModal.jsx` around lines 20 - 22, Update
ShareExportModal so generated user-editable captions and native-share titles
come from shareExport translations instead of hardcoded English templates. Keep
the canonical URL as a separate untranslated value, add or update the
corresponding English, French, and Arabic locale entries, and preserve Arabic
RTL behavior.
Source: Coding guidelines
| const shareData = generateShareCaption(algorithmName); | ||
| const [caption, setCaption] = useState(shareData.fullShareText); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'ShareExportModal.jsx|shareCaption.js' . | sed 's#^\./##'
echo "== ShareExportModal outline =="
file=$(fd 'ShareExportModal.jsx' . | head -n 1)
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" || true
echo "== relevant ShareExportModal lines =="
cat -n "$file" | sed -n '1,220p'
fi
echo "== shareCaption relevant lines =="
for f in $(fd 'shareCaption.(js|jsx)$' .); do
echo "--- $f ---"
wc -l "$f"
cat -n "$f" | sed -n '1,220p'
done
echo "== usages of ShareExportModal =="
rg -n "ShareExportModal|shareData|fullShareText" .Repository: ayoub3bidi/bayan-flow
Length of output: 11530
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== VisualizerApp ShareExportModal section =="
cat -n src/pages/VisualizerApp.jsx | sed -n '920,1015p'
echo "== ShareExportModal tests =="
cat -n src/components/ShareExportModal.test.jsx | sed -n '1,160p'
echo "== programmatic state initializer behavior probe =="
node - <<'JS'
function generateShareCaption(algorithmName) {
const name = algorithmName || 'Algorithm';
const displayName = name.toLowerCase().endsWith('algorithm')
? name
: `${name} Algorithm`;
const caption = `${displayName}. Step-by-step visualization from Bayan Flow.`;
const title = `${name} Visualization — Bayan Flow`;
const PRODUCTION_ORIGIN = 'https://bayanflow.com';
const fullShareText = `${caption}\n\n${PRODUCTION_ORIGIN}/app`;
return { caption, title, fullShareText };
}
class FakeComponent {
constructor(algorithmName) {
this.algorithmName = algorithmName;
this.componentDidMount = true;
this.props = { algorithmName };
this.fullShareTextSeed = generateShareCaption(this.props.algorithmName).fullShareText;
this.caption = this.initialCaptionFromSource();
}
// useState(initial) preserves its initial value across re-renders unless state setter is used.
initialCaptionFromSource() {
return this.fullShareTextSeed;
}
rerender(newAlgorithmName) {
this.algorithmName = newAlgorithmName;
this.fullShareTextSeed = generateShareCaption(this.algorithmName).fullShareText;
this.componentDidMount = false;
}
}
const flow = new FakeComponent('Bubble Sort');
flow.rerender('Quick Sort');
console.log(JSON.stringify({
firstSeed: 'Bubble Sort Algorithm.\n\nhttps://bayanflow.com/app',
currentFullShareTextSource: flow.fullShareTextSeed,
currentCaptionAfterRerenderWithoutStateSetter: flow.caption,
captionMatchesNewAlgorithm: flow.caption.includes(flow.fullShareTextSeed)
}, null, 2));
JSRepository: ayoub3bidi/bayan-flow
Length of output: 9210
Reset the exported caption when the share modal opens.
caption is initialized only on mount, so reopening the mounted modal with a different algorithmName keeps the previous export’s caption. Reset caption and copied when open changes to true, and add a rerender test for consecutive exports.
Proposed fix
-import { useState } from 'react';
+import { useEffect, useState } from 'react';
@@
const [caption, setCaption] = useState(shareData.fullShareText);
const [copied, setCopied] = useState(false);
+
+ useEffect(() => {
+ if (open) {
+ setCaption(shareData.fullShareText);
+ setCopied(false);
+ }
+ }, [open, shareData.fullShareText]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const shareData = generateShareCaption(algorithmName); | |
| const [caption, setCaption] = useState(shareData.fullShareText); | |
| import { useEffect, useState } from 'react'; | |
| ... | |
| const shareData = generateShareCaption(algorithmName); | |
| const [caption, setCaption] = useState(shareData.fullShareText); | |
| const [copied, setCopied] = useState(false); | |
| useEffect(() => { | |
| if (open) { | |
| setCaption(shareData.fullShareText); | |
| setCopied(false); | |
| } | |
| }, [open, shareData.fullShareText]); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/ShareExportModal.jsx` around lines 42 - 43, Update the
ShareExportModal state handling around shareData, caption, and copied so that
whenever open changes to true, caption is reset from the current
shareData.fullShareText and copied is reset to false. Add a rerender test
covering consecutive exports with different algorithmName values while reusing
the mounted modal.
- Make matchMedia mock unconditional in setup.js (was skipped when JSDOM already defined it) - Stub matchMedia for mobile breakpoint in AlgorithmInsightPanel tests (useIsBelowLg uses matchMedia, not innerWidth) - Add visualizationsRemaining to ControlPanel mock in VisualizerApp tests
fix: address all CodeRabbit review comments from PR #210
1 participant
Contribution workflow
develop: This PR targetsdevelop, notmain. (If the base is wrong, edit the PR on GitHub and change the base branch.)Description
Unifies chrome (non-visualization) motion across the app for smoother, more consistent drawer/modal/menu transitions while respecting
prefers-reduced-motion. Introduces a shared motion preset module (src/motion/chromeMotion.js), responsive viewport detection (useIsBelowLg), a full-screen mobile settings sheet, and a post-export video share modal. Also improves mobile UX, adds ErrorBoundary protection for lazy panels, and fixes a ProseMirror race condition.Type of Change
Related Issues
Fixes #
Changes Made
src/motion/chromeMotion.jsshared chrome motion presets (ease curves, durations, drawer/sheet/modal/menu/banner variants, marketing enter, reduced-motion support)useIsBelowLghook for Tailwindlgbreakpoint viewport detection viamatchMediaSettingsSheetcomponent - full-viewport bottom sheet for mobile (< lg) with scroll lock, Escape close, and backdrop dismissShareExportModal- post-export video sharing with editable smart captions, native Web Share, X (Twitter) sharing, and clipboard copygenerateShareCaption()utility with smart dedup (avoids "Algorithm Algorithm")ErrorBoundarycomponent wrappingPythonCodePanelandAlgorithmInsightPanelto contain render crashesNoteEditorgetHTML()calls against null ProseMirror schemaindex.htmlshareExport.*) in all three localesonAnimationCompletefor deferred contentmatchMediamock in test setup to be unconditional (was skipped when JSDOM defined a stub)ControlPanelmock to rendervisualizationsRemainingpropErrorBoundary,SettingsSheet,ShareExportModal,useIsBelowLg,chromeMotion,shareCaption,useVideoExporterextrasAlgorithm Details (if applicable)
N/A - this PR does not add new algorithms.
Testing
pnpm test:run)Test Results
Screenshots/GIFs
Code Quality
pnpm lint)pnpm format)Performance Impact
Accessibility
Breaking Changes
Checklist