fix: React Hooks & Performance Improvements (exhaustive-deps, ErrorBoundary, memoization)#1182
Open
akhil06232 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Open
Conversation
…undary, memoization) - Enable react-hooks/exhaustive-deps ESLint rule as warn - Add ErrorBoundary component to catch render errors gracefully - Fix useSortedImages stale closure bug (missing data dependency) - Memoize ImageCard, MediaThumbnails, NavigationButtons, ZoomControls with React.memo - Wrap toggleAITagging and deleteFolder with useCallback in useFolderOperations Closes AOSSIE-Org/PictoPy issue 1112 Closes AOSSIE-Org/PictoPy issue 937
Contributor
📝 WalkthroughWalkthroughIntroduces global error boundary for route error handling, wraps performance-sensitive Media components with React.memo for optimization, updates ESLint configuration with stricter React hooks rule enforcement, and refines hook dependency arrays for data freshness. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
frontend/src/components/ErrorBoundary.tsx (2)
23-25:componentDidCatchonly logs to console — no integration with an error reporting service.For a production app, this is the ideal hook for forwarding the error to a monitoring/observability service (e.g., Sentry, Datadog). Without it, render crashes in production are silent beyond the local console.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ErrorBoundary.tsx` around lines 23 - 25, Replace the console-only behavior in ErrorBoundary.componentDidCatch with forwarding the error and errorInfo to your monitoring/observability service (e.g., call Sentry.captureException or Datadog client) and include contextual data (component stack / errorInfo) and any relevant user/session identifiers; keep the console.error for local dev but ensure the reporting call is made (synchronously or awaited in a safe way) and does not throw, so production render crashes are reported to the external service.
27-29:handleResetsilently re-triggers the boundary for persistent render errors.
handleResetclearshasErrorand re-renders the same child tree. If the underlying error is a persistent render bug (not a transient side-effect), the component immediately throws again and the user is sent right back to the error screen. The "Try Again" label implies the operation might succeed, which is misleading for non-recoverable crashes. Based on learnings, the ErrorBoundary is intended for transient errors — consider adding a guard (e.g., a retry counter) or updating the button label to set accurate expectations.💡 Example: limit soft-reset attempts
interface ErrorBoundaryState { hasError: boolean; error: Error | null; + retryCount: number; } // In constructor: - this.state = { hasError: false, error: null }; + this.state = { hasError: false, error: null, retryCount: 0 }; // In handleReset: handleReset = (): void => { - this.setState({ hasError: false, error: null }); + this.setState((s) => ({ + hasError: false, + error: null, + retryCount: s.retryCount + 1, + })); }; // In render, disable "Try Again" after N attempts: - <button onClick={this.handleReset} ...> + <button onClick={this.handleReset} disabled={this.state.retryCount >= 3} ...> Try Again </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/ErrorBoundary.tsx` around lines 27 - 29, The handleReset method currently clears hasError and error and immediately re-renders the same child tree, which rethrows persistent render errors; update the ErrorBoundary to guard soft resets by adding a retry counter (e.g., state key retryCount) and a maxRetries constant, increment retryCount in handleReset and only clear hasError/error if retryCount < maxRetries (otherwise keep the error state and change UI to show a non-recoverable message or a destructive fallback like "Reload app"); alternatively update the retry button label based on retryCount (e.g., "Try Again" while retries remain, then "Reload app / Report issue") so handleReset, retryCount, hasError, and the render fallback behavior reflect the new retry limit.frontend/src/hooks/Sortimage.ts (1)
15-45: PreferuseMemooveruseEffect+useStatefor synchronous data transformation.
parseAndSortImageDatais a pure, synchronous function. UsinguseEffect+useStateguarantees a double render on every data change: the first pass renderssortedImages = [], the effect fires post-commit,setSortedImagesschedules a second render with the actual data.useMemocomputes inline during render and eliminates this empty-state flash entirely.♻️ Proposed refactor to `useMemo`
-import { useEffect, useMemo, useState } from 'react'; +import { useMemo } from 'react'; function useSortedImages(data: any): Image[] { - const [sortedImages, setSortedImages] = useState<Image[]>([]); - - useEffect(() => { - const parseAndSortImageData = (data: any): Image[] => { - const images: Image[] = []; - - for (const filePath in data) { - if (Object.prototype.hasOwnProperty.call(data, filePath)) { - const tags = data[filePath].split(', '); - const fileName = filePath.substring(filePath.lastIndexOf('/') + 1); - - const image: Image = { - id: fileName, - date: '', - title: data[filePath], - popularity: tags.length, - src: filePath, - tags: tags, - }; - - images.push(image); - } - } - - images.sort((a, b) => b.popularity - a.popularity); - - return images; - }; - - const sortedImages = parseAndSortImageData(data); - - setSortedImages(sortedImages); - }, [data]); - - return sortedImages; + return useMemo(() => { + if (!data) return []; + const images: Image[] = []; + for (const filePath in data) { + if (Object.prototype.hasOwnProperty.call(data, filePath)) { + const tags = data[filePath].split(', '); + const fileName = filePath.substring(filePath.lastIndexOf('/') + 1); + images.push({ + id: fileName, + date: '', + title: data[filePath], + popularity: tags.length, + src: filePath, + tags, + }); + } + } + images.sort((a, b) => b.popularity - a.popularity); + return images; + }, [data]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/Sortimage.ts` around lines 15 - 45, The current useEffect block that defines parseAndSortImageData and calls setSortedImages causes an extra render; replace that pattern with useMemo by computing sortedImages = useMemo(() => { /* reuse parseAndSortImageData logic or inline it */ }, [data]) and remove the useEffect and the setSortedImages state update; ensure you keep the same parsing/sorting logic (parseAndSortImageData, tags, fileName, popularity, images.sort) and depend only on data so sortedImages is memoized and updated synchronously during render.frontend/src/App.tsx (1)
29-36:GlobalLoaderandInfoDialogrender outsideErrorBoundary.Both components are rendered after the
</BrowserRouter>closing tag, leaving them unprotected. If either crashes, the error propagates to the React root uncaught. Given they're Redux-connected UI components, risk is low but worth noting — consider either moving them inside theErrorBoundary, or wrapping them individually.💡 Move shared UI inside the boundary
<BrowserRouter> <ErrorBoundary> <AppRoutes /> + <GlobalLoader loading={loading} message={message} /> + <InfoDialog + isOpen={isOpen} + title={title} + message={infoMessage} + variant={variant} + showCloseButton={showCloseButton} + /> </ErrorBoundary> </BrowserRouter> - <GlobalLoader loading={loading} message={message} /> - <InfoDialog - isOpen={isOpen} - title={title} - message={infoMessage} - variant={variant} - showCloseButton={showCloseButton} - />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.tsx` around lines 29 - 36, Move GlobalLoader and InfoDialog so they are rendered inside the ErrorBoundary to ensure crashes are caught; locate the ErrorBoundary wrapper (likely where <ErrorBoundary> surrounds <BrowserRouter> or route content in App) and either place <GlobalLoader loading={loading} message={message} /> and the <InfoDialog ... /> JSX before the closing </ErrorBoundary> tag (instead of after </BrowserRouter>), or wrap each with the ErrorBoundary component directly (e.g., <ErrorBoundary><GlobalLoader .../></ErrorBoundary> and <ErrorBoundary><InfoDialog .../></ErrorBoundary>); update imports/props as needed and verify App renders without moving BrowserRouter semantics.frontend/src/components/Media/ImageCard.tsx (2)
5-5: Consider using the namedmemoimport instead ofReact.memo.Since React 17+ with the new JSX transform, the
Reactnamespace is no longer needed for JSX. The only reasonReactis imported here is forReact.memo. Using the named import keeps the namespace clean and is the idiomatic pattern for modern React codebases.♻️ Proposed refactor
-import React, { useCallback, useState } from 'react'; +import { memo, useCallback, useState } from 'react'; -export const ImageCard = React.memo(function ImageCard({ +export const ImageCard = memo(function ImageCard({Also applies to: 20-20, 104-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Media/ImageCard.tsx` at line 5, The import currently brings in the whole React namespace just to use React.memo; update the import line to include the named memo (e.g., import React, { useCallback, useState, memo } from 'react') and then replace any usages of React.memo in this file (instances around the component export and any other occurrences at the noted lines) with the named memo identifier (memo(Component)) so the React namespace is no longer required solely for memoization.
32-36: NarrowuseCallbackdependency fromimagetoimage.id.The callback body only reads
image.id. Depending on the fullimageobject means the callback is recreated whenever any field changes (e.g.,isFavouritetoggling), which undermines theReact.memowrapper on the parent by propagating a new function reference to any child that receives this callback.♻️ Proposed fix
- }, [image, toggleFavourite]); + }, [image.id, toggleFavourite]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/components/Media/ImageCard.tsx` around lines 32 - 36, The handleToggleFavourite useCallback currently depends on the whole image object causing unnecessary re-creation; change its dependency to only the image id by either reading const imageId = image?.id above and using [imageId, toggleFavourite] or directly using image?.id in the dependency array, ensuring handleToggleFavourite only depends on image id and toggleFavourite (referencing the handleToggleFavourite function, useCallback hook, image.id and toggleFavourite symbols).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/src/components/Media/ImageCard.tsx`:
- Line 80: Remove the debug console.log that prints the entire image object in
the ImageCard component; locate the log statement (console.log(image)) inside
the ImageCard's favorite-button click handler (e.g., onFavoriteToggle /
handleFavoriteClick) and delete it so sensitive file paths/metadata are no
longer emitted to the console before merging.
In `@frontend/src/hooks/useFolderOperations.tsx`:
- Around line 147-160: The callbacks toggleAITagging and deleteFolder currently
depend on whole mutation objects (enableAITaggingMutation,
disableAITaggingMutation, deleteFolderMutation) which are unstable; instead
destructure and capture the stable .mutate functions (e.g., const { mutate:
enableAITagging } = enableAITaggingMutation, const { mutate: disableAITagging }
= disableAITaggingMutation, const { mutate: deleteFolder } =
deleteFolderMutation) and update the useCallback bodies to call those mutate
functions, then list only those mutate symbols (enableAITagging,
disableAITagging, deleteFolder) in the dependency arrays for toggleAITagging and
deleteFolder respectively so the callbacks remain stable.
---
Nitpick comments:
In `@frontend/src/App.tsx`:
- Around line 29-36: Move GlobalLoader and InfoDialog so they are rendered
inside the ErrorBoundary to ensure crashes are caught; locate the ErrorBoundary
wrapper (likely where <ErrorBoundary> surrounds <BrowserRouter> or route content
in App) and either place <GlobalLoader loading={loading} message={message} />
and the <InfoDialog ... /> JSX before the closing </ErrorBoundary> tag (instead
of after </BrowserRouter>), or wrap each with the ErrorBoundary component
directly (e.g., <ErrorBoundary><GlobalLoader .../></ErrorBoundary> and
<ErrorBoundary><InfoDialog .../></ErrorBoundary>); update imports/props as
needed and verify App renders without moving BrowserRouter semantics.
In `@frontend/src/components/ErrorBoundary.tsx`:
- Around line 23-25: Replace the console-only behavior in
ErrorBoundary.componentDidCatch with forwarding the error and errorInfo to your
monitoring/observability service (e.g., call Sentry.captureException or Datadog
client) and include contextual data (component stack / errorInfo) and any
relevant user/session identifiers; keep the console.error for local dev but
ensure the reporting call is made (synchronously or awaited in a safe way) and
does not throw, so production render crashes are reported to the external
service.
- Around line 27-29: The handleReset method currently clears hasError and error
and immediately re-renders the same child tree, which rethrows persistent render
errors; update the ErrorBoundary to guard soft resets by adding a retry counter
(e.g., state key retryCount) and a maxRetries constant, increment retryCount in
handleReset and only clear hasError/error if retryCount < maxRetries (otherwise
keep the error state and change UI to show a non-recoverable message or a
destructive fallback like "Reload app"); alternatively update the retry button
label based on retryCount (e.g., "Try Again" while retries remain, then "Reload
app / Report issue") so handleReset, retryCount, hasError, and the render
fallback behavior reflect the new retry limit.
In `@frontend/src/components/Media/ImageCard.tsx`:
- Line 5: The import currently brings in the whole React namespace just to use
React.memo; update the import line to include the named memo (e.g., import
React, { useCallback, useState, memo } from 'react') and then replace any usages
of React.memo in this file (instances around the component export and any other
occurrences at the noted lines) with the named memo identifier (memo(Component))
so the React namespace is no longer required solely for memoization.
- Around line 32-36: The handleToggleFavourite useCallback currently depends on
the whole image object causing unnecessary re-creation; change its dependency to
only the image id by either reading const imageId = image?.id above and using
[imageId, toggleFavourite] or directly using image?.id in the dependency array,
ensuring handleToggleFavourite only depends on image id and toggleFavourite
(referencing the handleToggleFavourite function, useCallback hook, image.id and
toggleFavourite symbols).
In `@frontend/src/hooks/Sortimage.ts`:
- Around line 15-45: The current useEffect block that defines
parseAndSortImageData and calls setSortedImages causes an extra render; replace
that pattern with useMemo by computing sortedImages = useMemo(() => { /* reuse
parseAndSortImageData logic or inline it */ }, [data]) and remove the useEffect
and the setSortedImages state update; ensure you keep the same parsing/sorting
logic (parseAndSortImageData, tags, fileName, popularity, images.sort) and
depend only on data so sortedImages is memoized and updated synchronously during
render.
| }`} | ||
| }`} | ||
| onClick={(e) => { | ||
| console.log(image); |
Contributor
There was a problem hiding this comment.
Remove debug console.log before merging.
This statement logs the full image object (containing file paths and metadata) to the console on every favourite-button click. It's a debug artifact that should not ship.
🐛 Proposed fix
onClick={(e) => {
- console.log(image);
e.stopPropagation();
handleToggleFavourite();
}}📝 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.
Suggested change
| console.log(image); | |
| onClick={(e) => { | |
| e.stopPropagation(); | |
| handleToggleFavourite(); | |
| }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/Media/ImageCard.tsx` at line 80, Remove the debug
console.log that prints the entire image object in the ImageCard component;
locate the log statement (console.log(image)) inside the ImageCard's
favorite-button click handler (e.g., onFavoriteToggle / handleFavoriteClick) and
delete it so sensitive file paths/metadata are no longer emitted to the console
before merging.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hey @rahulharpal1603 , This PR fixes some React hooks and performance issues in the frontend.
References Issues #1112 and #937
I turned on the exhaustive-deps lint rule (as a warning) so we can catch missing hook dependencies going forward. I also added an Error Boundary component that wraps the app routes — so if something crashes during rendering, users see a friendly error screen with a reload button instead of a blank white page.
I found and fixed a bug in Sortimage.ts where the useEffect had an empty dependency array but was using data inside it. This meant it would never update when the data changed — basically showing stale results.I also wrapped four components (ImageCard, MediaThumbnails, NavigationButtons, ZoomControls) with React.memo so they don't re-render unnecessarily, especially in image galleries with lots of items. And I added useCallback to two handler functions in useFolderOperationsthat were being recreated on every render.9 files changed, everything compiles cleanly with tsc --noEmit (zero errors).
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Performance
Chores