Enable react-next ESLint rules#43
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ESLint plugin and config, introduces useComponentEffect and useReducerState hooks, and refactors many components/pages/providers/UI primitives to use React.memo and the new hook; preserves existing behavior while standardizing side-effect handling. ChangesApp-wide memoization, custom hooks, and ESLint wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #43 +/- ##
==========================================
+ Coverage 75.53% 76.06% +0.53%
==========================================
Files 33 34 +1
Lines 846 865 +19
Branches 217 216 -1
==========================================
+ Hits 639 658 +19
Misses 189 189
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/components/ui/toggle-group.tsx (1)
17-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
refsupport while memoizing these primitivesThese wrappers are memoized but don't expose
refto the underlying Radix root/item elements, which breaks focus-management and imperative integrations. Additionally, the context value at line 36 is recreated as a new object literal on each render, negating the memoization benefits for context consumers.Suggested pattern (`memo(forwardRef(...))`)
-const ToggleGroup = React.memo(function ToggleGroup(props) { ... }) +const ToggleGroup = React.memo( + React.forwardRef< + React.ElementRef<typeof ToggleGroupPrimitive.Root>, + React.ComponentProps<typeof ToggleGroupPrimitive.Root> & + VariantProps<typeof toggleVariants> + >(function ToggleGroup(props, ref) { + return <ToggleGroupPrimitive.Root ref={ref} ... /> + }), +) -const ToggleGroupItem = React.memo(function ToggleGroupItem(props) { ... }) +const ToggleGroupItem = React.memo( + React.forwardRef< + React.ElementRef<typeof ToggleGroupPrimitive.Item>, + React.ComponentProps<typeof ToggleGroupPrimitive.Item> & + VariantProps<typeof toggleVariants> + >(function ToggleGroupItem(props, ref) { + return <ToggleGroupPrimitive.Item ref={ref} ... /> + }), +)For the context value, memoize it with
useMemoto preserve object identity across renders.Also applies to: 43-50
🤖 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/ui/toggle-group.tsx` around lines 17 - 24, The memoized wrapper components (e.g., ToggleGroup and the related ToggleItem wrappers wrapping ToggleGroupPrimitive.Root/Item) currently drop refs and recreate context on every render; change their definitions to use memo(forwardRef(...)) so the forwarded ref is passed through to the underlying Radix primitives (ensure the forwarded ref is applied to ToggleGroupPrimitive.Root / ToggleGroupPrimitive.Item), and memoize the context value with useMemo (based on the props/state that determine it) so the object identity is stable across renders; update the component signatures (ToggleGroup, ToggleItem) to accept ref and forward it, and wrap the final export with React.memo(forwardRef(...)) while using useMemo for the context value creation.src/components/ui/chart.tsx (1)
98-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape or validate values before writing raw CSS into the
<style>tag.Lines 99-115 interpolate caller-supplied
id, config keys, and color strings directly intodangerouslySetInnerHTML. That makes this component an HTML/CSS injection sink, and even benignidvalues with spaces/quotes will break the[data-chart=${id}]selector.🤖 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/ui/chart.tsx` around lines 98 - 115, The inline CSS builder is injecting unvalidated values (id, theme prefixes, config keys and color strings) into dangerouslySetInnerHTML, making it vulnerable to CSS/HTML injection and breaking selectors; fix by validating/escaping every dynamic piece before interpolation: call CSS.escape on id and any selector fragments (e.g., theme prefix keys from THEMES and any config keys used in selectors), and validate colorConfig values with a strict color whitelist/regex (allow only safe hex/rgb/rgba/hsl formats) or normalize them through a sanitizer function before emitting; update the code that constructs the style string (the block using THEMES, id, colorConfig and dangerouslySetInnerHTML) to use these escaping/validation helpers so no raw caller-supplied value is written into the style tag.src/components/animations/ConfettiAnimation.tsx (1)
106-109:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
durationandonCompletebecome stale after initial mount.The timer store is created once using
useRefand never rebuilt, so if the component receives newdurationoronCompleteprops after the initial render, those values won't be used. The next trigger will callstart()with the original duration and callback captured in the store's closure.Replace
useRefwithuseMemoand includedurationandonCompletein the dependency array to ensure the store is recreated whenever these props change:Suggested fix
- const timerStoreRef = useRef<ReturnType<typeof createTimerStore> | null>(null) - if (!timerStoreRef.current) { - timerStoreRef.current = createTimerStore(duration, onComplete) - } + const timerStore = useMemo( + () => createTimerStore(duration, onComplete), + [duration, onComplete], + ) const isActive = useSyncExternalStore( - timerStoreRef.current.subscribe, - timerStoreRef.current.getSnapshot, - timerStoreRef.current.getServerSnapshot, + timerStore.subscribe, + timerStore.getSnapshot, + timerStore.getServerSnapshot, ) // ... later ... - timerStoreRef.current.start() + timerStore.start()🤖 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/animations/ConfettiAnimation.tsx` around lines 106 - 109, The timer store is created once with timerStoreRef and captures initial duration and onComplete, so updates to those props become stale; replace the useRef-based creation with useMemo to recreate the store whenever duration or onComplete change (i.e. create the store via useMemo(() => createTimerStore(duration, onComplete), [duration, onComplete])) so that the createTimerStore closure always sees current duration and onComplete and subsequent calls like start() use latest values.src/app/(main)/home/_components/ContributionGraph.tsx (1)
374-401:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWidth observation can be skipped after the loading-first render
When the initial render hits the loading branch,
elementRef.currentisnulland this effect exits. With deps set to[elementRef], it may not rerun after the container mounts, leavingcontainerWidthstucknulland the heatmap at fallback sizing.💡 Suggested fix
- const containerWidth = useObservedElementWidth(containerRef) + const containerWidth = useObservedElementWidth(containerRef, !isLoading)-function useObservedElementWidth<T extends HTMLElement>( - elementRef: React.RefObject<T | null>, -): number | null { +function useObservedElementWidth<T extends HTMLElement>( + elementRef: React.RefObject<T | null>, + enabled: boolean, +): number | null { const [elementWidth, setElementWidth] = useState<number | null>(null) useComponentEffect(() => { + if (!enabled) return const element = elementRef.current if (!element) { return } @@ - }, [elementRef]) + }, [enabled, elementRef]) return elementWidth }🤖 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/app/`(main)/home/_components/ContributionGraph.tsx around lines 374 - 401, The effect in useComponentEffect inside ContributionGraph exits early when elementRef.current is null on the initial loading render and never re-runs because the deps are [elementRef]; change the dependency to watch the actual ref value so the effect re-runs when the container mounts (e.g., use [elementRef.current] or [elementRef.current ?? null]) and keep the same logic (updateWidth, ResizeObserver fallback, cleanup) so containerWidth is set after the element becomes available; update references to element inside the effect to use a local const element = elementRef.current to avoid stale reads.src/app/oauth/callback/page.tsx (1)
39-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd cleanup for redirect/status timers
The two
setTimeoutcallbacks are never cleared. If the component unmounts/remounts quickly, stale callbacks can still fire and mutate UI flow late.Suggested cleanup pattern
useComponentEffect(() => { + let redirectTimer: number | undefined + let successTimer: number | undefined + const state = searchParams.get('state') @@ - setTimeout(() => { + redirectTimer = window.setTimeout(() => { window.location.href = deepLink }, 100) @@ - setTimeout(() => { + successTimer = window.setTimeout(() => { setStatus('success') }, 2000) @@ void createTokenAndRedirect() + return () => { + if (redirectTimer !== undefined) window.clearTimeout(redirectTimer) + if (successTimer !== undefined) window.clearTimeout(successTimer) + } }, [searchParams])🤖 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/app/oauth/callback/page.tsx` around lines 39 - 108, The two setTimeout calls inside createTokenAndRedirect (in the useComponentEffect callback) create timers that are never cleared and can fire after the component unmounts; modify the effect to capture the timeout IDs (e.g., redirectTimeoutId and successTimeoutId) and return a cleanup function that calls clearTimeout on them (and optionally set a mounted flag to avoid calling setStatus after unmount). Update createTokenAndRedirect / useComponentEffect to store and clear those timers so stale callbacks cannot mutate state after unmounting.src/components/animations/LevelUpAnimation.tsx (1)
67-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
onCompletecan become stale after prop updatesBoth components create the visibility store once and capture the initial
onComplete. IfonCompletechanges later, the timeout still calls the old callback.Suggested fix
export const LevelUpAnimation = React.memo(function LevelUpAnimation({ level, show, onComplete, className, }: LevelUpAnimationProps) { + const onCompleteRef = useRef(onComplete) + onCompleteRef.current = onComplete const prevShowRef = useRef(false) // Create stable visibility store const storeRef = useRef<ReturnType<typeof createVisibilityStore> | null>(null) if (!storeRef.current) { - storeRef.current = createVisibilityStore(4000, onComplete) + storeRef.current = createVisibilityStore(4000, () => + onCompleteRef.current?.(), + ) }function MilestoneLevelUpAnimation({ level, show, milestone, reward, onComplete, className, }: LevelUpAnimationProps & { milestone: string reward?: string }) { + const onCompleteRef = useRef(onComplete) + onCompleteRef.current = onComplete const prevShowRef = useRef(false) // Create stable visibility store with longer duration for milestones const storeRef = useRef<ReturnType<typeof createVisibilityStore> | null>( null, ) if (!storeRef.current) { - storeRef.current = createVisibilityStore(5000, onComplete) + storeRef.current = createVisibilityStore(5000, () => + onCompleteRef.current?.(), + ) }Also applies to: 138-143
🤖 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/animations/LevelUpAnimation.tsx` around lines 67 - 70, The visibility store is created once and closes over the initial onComplete, so later prop updates are ignored; fix by making the store call a stable wrapper that reads a mutable ref to the latest onComplete: create a ref like latestOnCompleteRef.current = onComplete in a useEffect, and when creating the store (storeRef = createVisibilityStore(...)) pass a stable function (e.g. () => latestOnCompleteRef.current?.()) instead of onComplete; update the same pattern for the second occurrence (lines ~138-143) so the store always invokes the current prop callback without recreating the store on every render.
🧹 Nitpick comments (4)
src/components/ui/toggle-group.tsx (1)
36-38: ⚡ Quick winStabilize context value identity
At Line 36,
value={{ variant, size }}creates a new object each render, so allToggleGroupItemconsumers re-render even when values are unchanged.Suggested fix
+ const contextValue = React.useMemo(() => ({ variant, size }), [variant, size]) - <ToggleGroupContext value={{ variant, size }}> + <ToggleGroupContext value={contextValue}> {children} </ToggleGroupContext>🤖 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/ui/toggle-group.tsx` around lines 36 - 38, The context value passed to ToggleGroupContext is recreated each render (value={{ variant, size }}), causing unnecessary re-renders of consumers like ToggleGroupItem; fix by memoizing the context value in the ToggleGroup component (useMemo or a stable ref) e.g. derive a stable object from variant and size and pass that memoized object to <ToggleGroupContext value={...}>, ensuring the identity only changes when variant or size change.src/app/(main)/skill-tree/SkillTreeView.tsx (1)
381-381: ⚡ Quick winUse a stable
onOpenChangereference forTaskPoolDrawer.The
TaskPoolDrawercomponent is memoized but line 381 creates a new callback function on every render, defeating the memoization optimization. Pass the setter function directly instead.♻️ Suggested change
- onOpenChange={(open: boolean) => setDrawerOpen(open)} + onOpenChange={setDrawerOpen}🤖 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/app/`(main)/skill-tree/SkillTreeView.tsx at line 381, TaskPoolDrawer is memoized but you're creating a new arrow callback each render with onOpenChange={(open: boolean) => setDrawerOpen(open)} which breaks the memoization; replace that inline callback by passing the stable setter directly (onOpenChange={setDrawerOpen}) so TaskPoolDrawer receives a stable reference and memoization can work as intended.src/app/(main)/home/_components/AddTodoForm.tsx (1)
82-85: ⚡ Quick winUse one stable
onOpenChangehandler.Two identical inline handlers at lines 84 and 117 are recreated on every render. Since the component is already wrapped in
React.memo(), passing fresh callback references on each render breaks memoization benefits. Extract a singleuseCallback-memoized handler to keep props stable.♻️ Suggested refactor
-import React, { useState } from 'react' +import React, { useCallback, useState } from 'react' ... const [isNotesOpen, setIsNotesOpen] = useState(false) + const handleNotesOpenChange = useCallback((open: boolean) => { + setIsNotesOpen(open) + }, []) ... - <Collapsible - open={isNotesOpen} - onOpenChange={(open: boolean) => setIsNotesOpen(open)} - > + <Collapsible open={isNotesOpen} onOpenChange={handleNotesOpenChange}> ... - <Collapsible - open={isNotesOpen} - onOpenChange={(open: boolean) => setIsNotesOpen(open)} - > + <Collapsible open={isNotesOpen} onOpenChange={handleNotesOpenChange}>🤖 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/app/`(main)/home/_components/AddTodoForm.tsx around lines 82 - 85, The inline onOpenChange handlers in AddTodoForm.tsx recreate new functions each render and break React.memo benefits; extract a single memoized handler using useCallback (e.g., const handleNotesOpenChange = useCallback((open: boolean) => setIsNotesOpen(open), [setIsNotesOpen])) and replace both inline onOpenChange props on the Collapsible(s) with this stable handleNotesOpenChange so the prop reference remains stable across renders.src/app/(main)/home/_components/TodoItem.tsx (1)
118-121: ⚡ Quick winStabilize
onOpenChangecallback identity.At lines 120 and 153, identical inline handlers are recreated every render, undermining memoization benefits on the
Collapsiblecomponent. Extract to a singleuseCallbackhandler with an empty dependency array.♻️ Suggested refactor
-import React, { useState } from 'react' +import React, { useCallback, useState } from 'react' ... const [isNotesOpen, setIsNotesOpen] = useState(false) + const handleNotesOpenChange = useCallback((open: boolean) => { + setIsNotesOpen(open) + }, []) ... - <Collapsible - open={isNotesOpen} - onOpenChange={(open: boolean) => setIsNotesOpen(open)} - > + <Collapsible open={isNotesOpen} onOpenChange={handleNotesOpenChange}> ... - <Collapsible - open={isNotesOpen} - onOpenChange={(open: boolean) => setIsNotesOpen(open)} - > + <Collapsible open={isNotesOpen} onOpenChange={handleNotesOpenChange}>🤖 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/app/`(main)/home/_components/TodoItem.tsx around lines 118 - 121, The inline onOpenChange handlers passed to the Collapsible component (currently (open: boolean) => setIsNotesOpen(open)) are recreated each render and break memoization; extract a stable handler using React's useCallback (e.g., const handleNotesOpenChange = useCallback((open: boolean) => setIsNotesOpen(open), [])) and replace the inline prop on Collapsible with handleNotesOpenChange so its identity remains stable; update both occurrences (lines using isNotesOpen/setIsNotesOpen) to use this single callback.
🤖 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/app/`(main)/home/_components/TodoList.tsx:
- Around line 226-228: The effect that sets localPendingTodos uses
pendingTodosFromQuery (which itself relies on pendingData and categoryMap via
mapTodos) but only lists pendingData in the dependency array; update the
useComponentEffect for setLocalPendingTodos so it re-runs when the derived todos
change by adding pendingTodosFromQuery (or categoryMap) to the dependency list
instead of only pendingData, ensuring localPendingTodos picks up
categoryName/categoryColor updates.
In `@src/components/animations/AchievementAnimation.tsx`:
- Around line 119-132: handleComplete mutates shouldReset inside the
setAchievements updater so setCurrentIndex returns before that mutation, causing
out-of-bounds currentIndex; fix by reading the current state synchronously
before scheduling updates (use current values of currentIndex and achievements),
then call setAchievements and setCurrentIndex based on that snapshot (e.g.,
compute isLast = currentIndex >= achievements.length - 1, then
setAchievements(isLast ? [] : prev => prev) and setCurrentIndex(isLast ? 0 :
currentIndex + 1)); also add currentIndex and achievements (and the setters) to
the handleComplete dependency array so the hook uses up-to-date state.
In `@src/components/electron/ConfigurationSettings.tsx`:
- Around line 160-176: The updateConfig function allows NaN to be written into
config; add a guard before assigning to current[lastKey] that detects numeric
NaN values and prevents them from being written (e.g., if typeof value ===
'number' && isNaN(value') then skip the assignment and return or leave existing
value intact). Modify the code inside updateConfig (referencing updateConfig,
current, lastKey, and config) so invalid numeric inputs are rejected at the
write boundary instead of storing NaN in state.
In `@src/components/ui/chart.tsx`:
- Line 68: ChartContext is being passed a new object literal each render in
ChartContainer which forces all useChart() consumers to rerender; fix this by
memoizing the context value inside ChartContainer (useMemo) so the same
reference is reused when config is unchanged—compute a memoized value (dependent
on config) and pass that to <ChartContext value={...}> instead of creating `{
config }` inline.
In `@src/components/ui/menubar.tsx`:
- Around line 81-83: The menubar content className string is missing the
close-state animation token; in the MenubarContent JSX where you build className
with cn(...) (the block starting with 'data-[state=open]:animate-in ...'),
insert the token data-[state=closed]:animate-out immediately after
data-[state=open]:animate-in so the closed-state animation will trigger (match
the pattern used in other components like Popover/Select/DropdownMenu).
In `@src/hooks/useReducerState.ts`:
- Around line 21-26: The dispatch returned from useReducerState is re-created
whenever the reducer reference changes because it's memoized with [reducer]; to
honor the hook's docstring promise of a stable dispatch function, rewrite
dispatch to use a ref-backed stable callback: store a ref to the current reducer
(update it in an effect) and create a single dispatch function (no reducer
dependency) that calls setState(prev => reducerRef.current(prev, action));
update references to reducerRef and keep setState usage intact so the returned
dispatch identity never changes even if the reducer prop changes.
In `@src/lib/redux/providers.tsx`:
- Line 17: The usage example incorrectly imports memo from the local module;
update the example import so that ReduxProvider still comes from
'@/lib/redux/providers' but memo is imported from 'react' (i.e., import { memo }
from 'react') to avoid a broken import — adjust the example line that currently
reads "import { memo, ReduxProvider } from '@/lib/redux/providers'" to separate
memo's import from the local ReduxProvider export.
In `@src/types/react.d.ts`:
- Around line 1-2: Remove the conflicting default type import and keep the
namespace import: delete the line `import type React from 'react'` and retain
`import type * as React from 'react'`; ensure usages like `React.Dispatch`
continue to use the namespace import and no other default `React` type import
remains in this declaration file.
---
Outside diff comments:
In `@src/app/`(main)/home/_components/ContributionGraph.tsx:
- Around line 374-401: The effect in useComponentEffect inside ContributionGraph
exits early when elementRef.current is null on the initial loading render and
never re-runs because the deps are [elementRef]; change the dependency to watch
the actual ref value so the effect re-runs when the container mounts (e.g., use
[elementRef.current] or [elementRef.current ?? null]) and keep the same logic
(updateWidth, ResizeObserver fallback, cleanup) so containerWidth is set after
the element becomes available; update references to element inside the effect to
use a local const element = elementRef.current to avoid stale reads.
In `@src/app/oauth/callback/page.tsx`:
- Around line 39-108: The two setTimeout calls inside createTokenAndRedirect (in
the useComponentEffect callback) create timers that are never cleared and can
fire after the component unmounts; modify the effect to capture the timeout IDs
(e.g., redirectTimeoutId and successTimeoutId) and return a cleanup function
that calls clearTimeout on them (and optionally set a mounted flag to avoid
calling setStatus after unmount). Update createTokenAndRedirect /
useComponentEffect to store and clear those timers so stale callbacks cannot
mutate state after unmounting.
In `@src/components/animations/ConfettiAnimation.tsx`:
- Around line 106-109: The timer store is created once with timerStoreRef and
captures initial duration and onComplete, so updates to those props become
stale; replace the useRef-based creation with useMemo to recreate the store
whenever duration or onComplete change (i.e. create the store via useMemo(() =>
createTimerStore(duration, onComplete), [duration, onComplete])) so that the
createTimerStore closure always sees current duration and onComplete and
subsequent calls like start() use latest values.
In `@src/components/animations/LevelUpAnimation.tsx`:
- Around line 67-70: The visibility store is created once and closes over the
initial onComplete, so later prop updates are ignored; fix by making the store
call a stable wrapper that reads a mutable ref to the latest onComplete: create
a ref like latestOnCompleteRef.current = onComplete in a useEffect, and when
creating the store (storeRef = createVisibilityStore(...)) pass a stable
function (e.g. () => latestOnCompleteRef.current?.()) instead of onComplete;
update the same pattern for the second occurrence (lines ~138-143) so the store
always invokes the current prop callback without recreating the store on every
render.
In `@src/components/ui/chart.tsx`:
- Around line 98-115: The inline CSS builder is injecting unvalidated values
(id, theme prefixes, config keys and color strings) into
dangerouslySetInnerHTML, making it vulnerable to CSS/HTML injection and breaking
selectors; fix by validating/escaping every dynamic piece before interpolation:
call CSS.escape on id and any selector fragments (e.g., theme prefix keys from
THEMES and any config keys used in selectors), and validate colorConfig values
with a strict color whitelist/regex (allow only safe hex/rgb/rgba/hsl formats)
or normalize them through a sanitizer function before emitting; update the code
that constructs the style string (the block using THEMES, id, colorConfig and
dangerouslySetInnerHTML) to use these escaping/validation helpers so no raw
caller-supplied value is written into the style tag.
In `@src/components/ui/toggle-group.tsx`:
- Around line 17-24: The memoized wrapper components (e.g., ToggleGroup and the
related ToggleItem wrappers wrapping ToggleGroupPrimitive.Root/Item) currently
drop refs and recreate context on every render; change their definitions to use
memo(forwardRef(...)) so the forwarded ref is passed through to the underlying
Radix primitives (ensure the forwarded ref is applied to
ToggleGroupPrimitive.Root / ToggleGroupPrimitive.Item), and memoize the context
value with useMemo (based on the props/state that determine it) so the object
identity is stable across renders; update the component signatures (ToggleGroup,
ToggleItem) to accept ref and forward it, and wrap the final export with
React.memo(forwardRef(...)) while using useMemo for the context value creation.
---
Nitpick comments:
In `@src/app/`(main)/home/_components/AddTodoForm.tsx:
- Around line 82-85: The inline onOpenChange handlers in AddTodoForm.tsx
recreate new functions each render and break React.memo benefits; extract a
single memoized handler using useCallback (e.g., const handleNotesOpenChange =
useCallback((open: boolean) => setIsNotesOpen(open), [setIsNotesOpen])) and
replace both inline onOpenChange props on the Collapsible(s) with this stable
handleNotesOpenChange so the prop reference remains stable across renders.
In `@src/app/`(main)/home/_components/TodoItem.tsx:
- Around line 118-121: The inline onOpenChange handlers passed to the
Collapsible component (currently (open: boolean) => setIsNotesOpen(open)) are
recreated each render and break memoization; extract a stable handler using
React's useCallback (e.g., const handleNotesOpenChange = useCallback((open:
boolean) => setIsNotesOpen(open), [])) and replace the inline prop on
Collapsible with handleNotesOpenChange so its identity remains stable; update
both occurrences (lines using isNotesOpen/setIsNotesOpen) to use this single
callback.
In `@src/app/`(main)/skill-tree/SkillTreeView.tsx:
- Line 381: TaskPoolDrawer is memoized but you're creating a new arrow callback
each render with onOpenChange={(open: boolean) => setDrawerOpen(open)} which
breaks the memoization; replace that inline callback by passing the stable
setter directly (onOpenChange={setDrawerOpen}) so TaskPoolDrawer receives a
stable reference and memoization can work as intended.
In `@src/components/ui/toggle-group.tsx`:
- Around line 36-38: The context value passed to ToggleGroupContext is recreated
each render (value={{ variant, size }}), causing unnecessary re-renders of
consumers like ToggleGroupItem; fix by memoizing the context value in the
ToggleGroup component (useMemo or a stable ref) e.g. derive a stable object from
variant and size and pass that memoized object to <ToggleGroupContext
value={...}>, ensuring the identity only changes when variant or size change.
🪄 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: ff95a3e0-6b0c-4801-bb1a-9abc45f509be
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (117)
eslint.config.jspackage.jsonsrc/app/(main)/home/_components/AddTodoForm.tsxsrc/app/(main)/home/_components/Category.tsxsrc/app/(main)/home/_components/CategoryFilterChips.tsxsrc/app/(main)/home/_components/CategoryManageDialog.tsxsrc/app/(main)/home/_components/CompletedTodos.tsxsrc/app/(main)/home/_components/ContributionGraph.tsxsrc/app/(main)/home/_components/DayDetailDialog.tsxsrc/app/(main)/home/_components/LogoutButton.tsxsrc/app/(main)/home/_components/SortableTodoItem.tsxsrc/app/(main)/home/_components/SundayDigestCard.tsxsrc/app/(main)/home/_components/TodoItem.tsxsrc/app/(main)/home/_components/TodoList.tsxsrc/app/(main)/home/_components/WeeklySummaryCard.tsxsrc/app/(main)/home/_components/YearInReviewModal.tsxsrc/app/(main)/home/page.tsxsrc/app/(main)/layout.tsxsrc/app/(main)/skill-tree/SkillTreeView.tsxsrc/app/(main)/skill-tree/components/ConstellationCanvas.tsxsrc/app/(main)/skill-tree/components/DragOverlayCard.tsxsrc/app/(main)/skill-tree/components/NodePopover.tsxsrc/app/(main)/skill-tree/components/SkillNodeCircle.test.tsxsrc/app/(main)/skill-tree/components/SkillNodeCircle.tsxsrc/app/(main)/skill-tree/components/TaskPoolCard.test.tsxsrc/app/(main)/skill-tree/components/TaskPoolCard.tsxsrc/app/(main)/skill-tree/components/TaskPoolDrawer.stories.tsxsrc/app/(main)/skill-tree/components/TaskPoolDrawer.tsxsrc/app/(main)/skill-tree/components/XpBadge.tsxsrc/app/(main)/skill-tree/page.tsxsrc/app/braindump/page.tsxsrc/app/floating-navigator/page.tsxsrc/app/layout.tsxsrc/app/login/[[...login]]/page.tsxsrc/app/oauth/callback/page.tsxsrc/app/oauth/sso-callback/page.tsxsrc/app/oauth/start/page.tsxsrc/app/page.tsxsrc/app/settings/page.tsxsrc/app/sign-up/[[...sign-up]]/page.tsxsrc/components/AppSidebar.tsxsrc/components/ThemeSelector.tsxsrc/components/ThemeSelectorMenuItem.tsxsrc/components/animations/AchievementAnimation.tsxsrc/components/animations/ConfettiAnimation.tsxsrc/components/animations/LevelUpAnimation.tsxsrc/components/auth/ElectronLoginForm.tsxsrc/components/auth/ElectronOAuthButtons.tsxsrc/components/auth/ElectronSignUpForm.tsxsrc/components/braindump/BrainDumpEditor.tsxsrc/components/electron/BrainDumpSettings.tsxsrc/components/electron/ConfigurationSettings.tsxsrc/components/electron/ElectronSettingsPage.tsxsrc/components/electron/ElectronStartupSync.tsxsrc/components/electron/FloatingWindowSettings.tsxsrc/components/electron/NotificationSettings.tsxsrc/components/electron/ShortcutSettings.tsxsrc/components/electron/WindowStateSettings.tsxsrc/components/floating-navigator/FloatingCategoryManager.tsxsrc/components/floating-navigator/FloatingNavigator.tsxsrc/components/floating-navigator/FloatingNavigatorContainer.tsxsrc/components/floating-navigator/SortableFloatingTodoItem.tsxsrc/components/floating-navigator/useFloatingNavigatorMenuActions.tssrc/components/ui/accordion.tsxsrc/components/ui/alert-dialog.tsxsrc/components/ui/alert.tsxsrc/components/ui/aspect-ratio.tsxsrc/components/ui/avatar.tsxsrc/components/ui/badge.tsxsrc/components/ui/breadcrumb.tsxsrc/components/ui/button.tsxsrc/components/ui/calendar.tsxsrc/components/ui/card.tsxsrc/components/ui/carousel.tsxsrc/components/ui/chart.tsxsrc/components/ui/checkbox.tsxsrc/components/ui/collapsible.tsxsrc/components/ui/command.tsxsrc/components/ui/context-menu.tsxsrc/components/ui/dialog.tsxsrc/components/ui/drawer.tsxsrc/components/ui/dropdown-menu.tsxsrc/components/ui/form.tsxsrc/components/ui/hover-card.tsxsrc/components/ui/input-otp.tsxsrc/components/ui/input.tsxsrc/components/ui/label.tsxsrc/components/ui/menubar.tsxsrc/components/ui/navigation-menu.tsxsrc/components/ui/pagination.tsxsrc/components/ui/popover.tsxsrc/components/ui/progress.tsxsrc/components/ui/radio-group.tsxsrc/components/ui/resizable.tsxsrc/components/ui/scroll-area.tsxsrc/components/ui/select.tsxsrc/components/ui/separator.tsxsrc/components/ui/sheet.tsxsrc/components/ui/sidebar.tsxsrc/components/ui/skeleton.tsxsrc/components/ui/slider.tsxsrc/components/ui/sonner.tsxsrc/components/ui/switch.tsxsrc/components/ui/table.tsxsrc/components/ui/tabs.tsxsrc/components/ui/textarea.tsxsrc/components/ui/toggle-group.tsxsrc/components/ui/toggle.tsxsrc/components/ui/tooltip.tsxsrc/hooks/useComponentEffect.tssrc/hooks/useReducerState.tssrc/lib/orpc/electron-auth-provider.tsxsrc/lib/redux/providers.tsxsrc/lib/utils.tssrc/providers/QueryClientProvider.tsxsrc/providers/ThemeProvider.tsxsrc/types/react.d.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/animations/ConfettiAnimation.tsx (1)
121-125: ⚡ Quick winConsider moving the side effect to an effect hook.
Calling
timerStore.start()during render triggers listener notifications synchronously, which can cause React to warn about updates during render. With React 19's concurrent features, side effects during render can lead to unpredictable behavior.Moving this to an effect would be safer:
♻️ Suggested refactor
- // Detect trigger rising edge during render (not in effect) - if (trigger && !prevTriggerRef.current && !isActive) { - particleSeedRef.current = Date.now() - timerStore.start() - } - prevTriggerRef.current = trigger + // Detect trigger rising edge and start animation + useEffect(() => { + if (trigger && !prevTriggerRef.current && !isActive) { + particleSeedRef.current = Date.now() + timerStore.start() + } + prevTriggerRef.current = trigger + }, [trigger, isActive, timerStore])🤖 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/animations/ConfettiAnimation.tsx` around lines 121 - 125, The render currently calls timerStore.start() when (trigger && !prevTriggerRef.current && !isActive), causing a side effect during render; move this logic into a useEffect that depends on trigger and isActive (and refs as needed) so the timerStore.start() call runs after commit; inside the effect, when trigger becomes true and prevTriggerRef.current is false and !isActive, set particleSeedRef.current = Date.now(), call timerStore.start(), and then update prevTriggerRef.current = trigger (or update prevTriggerRef.current in a separate effect) to preserve the same behavior without causing updates during render.
🤖 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/app/oauth/callback/page.tsx`:
- Around line 39-45: The effect in useComponentEffect is re-running because
searchParams (from useSearchParams) has an unstable identity; extract the
primitives by moving const state = searchParams.get('state'), const error =
searchParams.get('error') and const errorDescription =
searchParams.get('error_description') to component scope (outside
useComponentEffect) and then use those primitive variables in the effect's
dependency list instead of searchParams; update useComponentEffect to depend
only on state, error, and errorDescription so the POST to
/api/oauth/create-signin-token (and related redirect/timer logic in
useComponentEffect) runs only when the actual URL params change.
In `@src/components/ui/toggle-group.tsx`:
- Around line 58-70: The ToggleGroup context default is set to a truthy
'default' which prevents item-level props from ever being used; change the
ToggleGroupContext default values to undefined and update usages in
ToggleGroupItem to use optional chaining and nullish coalescing so item props
can fall back correctly (e.g. replace context.variant || variant and
context.size || size with context?.variant ?? variant and context?.size ?? size)
for all places including data-variant, data-size, and the call to toggleVariants
inside ToggleGroupPrimitive.Item.
---
Nitpick comments:
In `@src/components/animations/ConfettiAnimation.tsx`:
- Around line 121-125: The render currently calls timerStore.start() when
(trigger && !prevTriggerRef.current && !isActive), causing a side effect during
render; move this logic into a useEffect that depends on trigger and isActive
(and refs as needed) so the timerStore.start() call runs after commit; inside
the effect, when trigger becomes true and prevTriggerRef.current is false and
!isActive, set particleSeedRef.current = Date.now(), call timerStore.start(),
and then update prevTriggerRef.current = trigger (or update
prevTriggerRef.current in a separate effect) to preserve the same behavior
without causing updates during render.
🪄 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: af666bba-f65d-4253-90bb-23a536111674
📒 Files selected for processing (16)
src/app/(main)/home/_components/AddTodoForm.tsxsrc/app/(main)/home/_components/ContributionGraph.tsxsrc/app/(main)/home/_components/TodoItem.tsxsrc/app/(main)/home/_components/TodoList.tsxsrc/app/(main)/skill-tree/SkillTreeView.tsxsrc/app/oauth/callback/page.tsxsrc/components/animations/AchievementAnimation.tsxsrc/components/animations/ConfettiAnimation.tsxsrc/components/animations/LevelUpAnimation.tsxsrc/components/electron/ConfigurationSettings.tsxsrc/components/ui/chart.tsxsrc/components/ui/menubar.tsxsrc/components/ui/toggle-group.tsxsrc/hooks/useReducerState.tssrc/lib/redux/providers.tsxsrc/types/react.d.ts
✅ Files skipped from review due to trivial changes (1)
- src/types/react.d.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/components/animations/AchievementAnimation.tsx
- src/components/electron/ConfigurationSettings.tsx
- src/app/(main)/home/_components/TodoList.tsx
- src/app/(main)/home/_components/TodoItem.tsx
- src/components/animations/LevelUpAnimation.tsx
- src/lib/redux/providers.tsx
- src/app/(main)/home/_components/ContributionGraph.tsx
- src/components/ui/menubar.tsx
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.
Summary
Validation
Summary by CodeRabbit
Refactor
New Features / Chores
Tests