fix: memoize selectUserState selector#2537
fix: memoize selectUserState selector#2537jonybekov wants to merge 338 commits intoquran:testingfrom
Conversation
Fix: HTML tags rendering as text in UI - Resolved issue where HTML tags were displayed as plain text instead of being rendered.
* fix copy link button * feat: preview mode * updates * remove extra slash * add a fallback for copy link and enhance the preview mode UI * add all query params to the url
* Update Q&A urls * Fix TS issue * Update Q&A urls
* add generate copy link and copy to clipboard safari support * add generate your video button * fix title * fix aspect ratio for orientation
* QF-2121 generate link button showing in chrome ios * QF-2123 exceeding download images limit showing the wrong error
* Initial commit * Updates * Updates * Fix tests
* Load week on clicking * Add special 1st of April Ayahs * Hide homepage section before Shawwal * Add description field
…#2363) * updated page title and meta desc, previously they were generic, but now we show use the question and the summary of the answer * updated page title to be localized. * Using Template Literals. * Fix linting issues --------- Co-authored-by: Osama Sayed <toto777_90@hotmail.com>
* change the copy button place in reading mode * change the copy button place in the translation view
* change the copy button place in reading mode * change the copy button place in the translation view * update
* change the copy button place in reading mode * change the copy button place in the translation view * update * update
* change the copy button place in reading mode * change the copy button place in the translation view * update * update * ignore max lines linting rules causing deploy to fail
* change the copy button place in reading mode * change the copy button place in the translation view * update * update * ignore max lines linting rules causing deploy to fail * update
* Fix "Tafsirs" button triggering Surah selection * Simplification of bug fixing. preventDefault is sufficient. * Revert to the old solution: focusing the modal is important.
…e next.config.js for additional image domains (quran#2521) * QF-3354 ref: AuthorInfo component with image error handling and update next.config.js for additional image domains * QF-3354 ref: AuthorInfo component to use centralized AUTHOR_DEFAULT_IMAGE and improve image URL handling with getImageSrc utility function. * QF-3354 ref: Enhance AuthorInfo component by utilizing useCallback for image error handling and updating getImageSrc utility to accept undefined URLs. * QF-3354 ref: Update next.config.js to add additional image domains for Facebook integration * QF-3354 ref: Remove Facebook image domains from next.config.js * QF-3354 chore: add comments explains the image domains for Google and Facebook OAuth user avatars in next.config.js
…e next.config.js for additional image domains (quran#2521) * QF-3354 ref: AuthorInfo component with image error handling and update next.config.js for additional image domains * QF-3354 ref: AuthorInfo component to use centralized AUTHOR_DEFAULT_IMAGE and improve image URL handling with getImageSrc utility function. * QF-3354 ref: Enhance AuthorInfo component by utilizing useCallback for image error handling and updating getImageSrc utility to accept undefined URLs. * QF-3354 ref: Update next.config.js to add additional image domains for Facebook integration * QF-3354 ref: Remove Facebook image domains from next.config.js * QF-3354 chore: add comments explains the image domains for Google and Facebook OAuth user avatars in next.config.js
* Fix "Tafsirs" button triggering Surah selection * Simplification of bug fixing. preventDefault is sufficient. * Revert to the old solution: focusing the modal is important.
* Fix line breaks in note body * Justify the text * Use a tabWidth of 4
fix room's url
* feat: ensure horizontal scrolling in ExploreTopicsSection * test: add Playwright test for horizontal scroll in ExploreTopicsSection * fix: improve error handling and RTL clarity in scroll test - Add explicit error handling for missing scrollable container - Rename scrollRightAndWait to scrollByAndWait for RTL clarity - Add JSDoc explaining RTL behavior and fallback strategy * refactor: improve scroll test helper and fix CSS formatting - Consolidate RTL fallback logic in scrollByAndWait helper - Encapsulate forward/reverse scroll attempts with identical polling - Simplify test to single scrollByAndWait call - Fix scrollbar CSS formatting inconsistencies - Remove deprecated -webkit-overflow-scrolling property
* Fix: reciter menu too large * added playwright test to ensure reciter div is scrollable * applied coderrabbit suggestions * removal of the desktop-only test restriction * removed page.waitForTimeout
Fixed line height
…e from overlapping with the close cross
…the title from overlapping with the close cross" This reverts commit f232489.
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/redux/slices/session.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.*
📄 CodeRabbit inference engine (.cursor/rules/bug-handling-with-todo-comments.mdc)
If you encounter a bug in existing code, or the instructions lead to suboptimal or buggy code, add comments starting with "TODO:" outlining the problems.
**/*.*: Utilize Early Returns: Use early returns to avoid nested conditions and improve readability.
Conditional Classes: Prefer conditional classes over ternary operators for class attributes.
**/*.*: Use comments sparingly, and when you do, make them meaningful.
Don't comment on obvious things. Excessive or unclear comments can clutter the codebase and become outdated.
Use comments to convey the 'why' behind specific actions or explain unusual behavior and potential pitfalls.
Provide meaningful information about the function's behavior and explain unusual behavior and potential pitfalls.
**/*.*: Write short functions that only do one thing.
Follow the single responsibility principle (SRP), which means that a function should have one purpose and perform it effectively.
If a function becomes too long or complex, consider breaking it into smaller, more manageable functions.Order functions with those that are composing other functions appearing earlier in the file. For example, if you have a menu with multiple buttons, define the menu function above the buttons.
**/*.*: Always add helpful comments to the code explaining what you are doing.
Never delete old comments, unless they are no longer relevant because the code has been rewritten or deleted.
**/*.*: Choose names for variables, functions, and classes that reflect their purpose and behavior.
A name should tell you why it exists, what it does, and how it is used. If a name requires a comment, then the name does not reveal its intent.
Use specific names that provide a clearer understanding of what the variables represent and how they are used.
Files:
src/redux/slices/session.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/next-js-conventions.mdc)
**/*.{js,jsx,ts,tsx}: Rely on Next.js Pages Router for state changes.
Minimize 'use client' usage: Prefer server components and Next.js SSR features.
Minimize 'use client' usage: Use 'use client' only for Web API access in small components.
Minimize 'use client' usage: Avoid using 'use client' for data fetching or state management.
**/*.{js,jsx,ts,tsx}: Optimize Web Vitals (LCP, CLS, FID)
Use dynamic loading for non-critical components using @src/components/dls/Spinner/Spinner.tsx
Files:
src/redux/slices/session.ts
**/{store,redux}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Implement Redux Toolkit for efficient Redux development for medium-complex state logic
Files:
src/redux/slices/session.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Use xstate for complex state logic
**/*.{ts,tsx}: TypeScript: prefer interfaces over type aliases for object shapes; avoid any; add explicit return types for public functions
Error handling: define and use custom error types; use async/await correctly; show user-friendly error messages
Use Redux Toolkit for state management and XState for complex state machines
Implement i18n with next-translate in components and utilities
Files:
src/redux/slices/session.ts
src/redux/**/*
📄 CodeRabbit inference engine (.cursor/rules/redux-folder-structure.mdc)
Follow this folder structure: src/redux/actions/, src/redux/slices/, src/redux/RootState.ts, src/redux/store.ts, src/redux/migrations.ts
Files:
src/redux/slices/session.ts
src/redux/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/redux-toolkit-best-practices.mdc)
src/redux/**/*.ts: Use Redux Toolkit for efficient Redux development.
Implement slice pattern for organizing Redux code.
Use selectors for accessing state in components.
Use Redux hooks (useSelector, useDispatch) in components.
Follow Redux style guide for naming conventionsUse Redux Toolkit for state management and persistence (redux-persist) within the redux layer
Files:
src/redux/slices/session.ts
**/*.{ts,tsx,d.ts}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx,d.ts}: Prefer interfaces over types for object definitions
Use type for unions, intersections, and mapped types
Avoid usingany, preferunknownfor unknown types
Leverage TypeScript's built-in utility types
Use generics for reusable type patterns
Use PascalCase for type names and interfaces
Use camelCase for variables and functions
Use UPPER_CASE for constants
Use descriptive names with auxiliary verbs (e.g., isLoading, hasError)
Use explicit return types for public functions
Use arrow functions for callbacks and methods
Implement proper error handling with custom error types
Use function overloads for complex type scenarios
Prefer async/await over Promises
Use readonly for immutable properties
Leverage discriminated unions for type safety
Use type guards for runtime type checking
Implement proper null checking
Avoid type assertions unless necessary
Create custom error types for domain-specific errors
Use Result types for operations that can fail
Use try-catch blocks with typed catch clauses
Handle Promise rejections properly
Files:
src/redux/slices/session.ts
{src,types}/**/*.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript throughout the codebase with strict configuration
Files:
src/redux/slices/session.ts
{src,types}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use path aliases @/ for src and @/dls/* for design system imports
Files:
src/redux/slices/session.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Order imports: React first, then external, then internal; alphabetize within groups with blank lines between groups
Naming: camelCase for variables/functions; PascalCase for types, interfaces, and React components; UPPER_CASE for constants
Functions should be <= 30 lines, prefer early returns, single responsibility, and descriptive names
Files:
src/redux/slices/session.ts
**/*.{ts,tsx,js,jsx,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Use Prettier formatting: single quotes, 2-space indentation, 100-character line width, and trailing commas
Files:
src/redux/slices/session.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
PR: quran/quran.com-frontend-next#0
File: .cursor/rules/redux-toolkit-best-practices.mdc:0-0
Timestamp: 2025-08-01T08:51:05.924Z
Learning: Applies to src/redux/**/*.ts : Use selectors for accessing state in components.
📚 Learning: 2025-08-01T08:51:05.924Z
Learnt from: CR
PR: quran/quran.com-frontend-next#0
File: .cursor/rules/redux-toolkit-best-practices.mdc:0-0
Timestamp: 2025-08-01T08:51:05.924Z
Learning: Applies to src/redux/**/*.ts : Use selectors for accessing state in components.
Applied to files:
src/redux/slices/session.ts
📚 Learning: 2025-08-01T08:51:05.924Z
Learnt from: CR
PR: quran/quran.com-frontend-next#0
File: .cursor/rules/redux-toolkit-best-practices.mdc:0-0
Timestamp: 2025-08-01T08:51:05.924Z
Learning: Applies to src/redux/**/*.ts : Implement slice pattern for organizing Redux code.
Applied to files:
src/redux/slices/session.ts
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
PR: quran/quran.com-frontend-next#0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*slice.{ts,tsx} : Implement slice pattern for organizing Redux code
Applied to files:
src/redux/slices/session.ts
🧬 Code graph analysis (1)
src/redux/slices/session.ts (1)
src/utils/auth/login.ts (1)
isLoggedIn(15-15)
🔇 Additional comments (1)
src/redux/slices/session.ts (1)
1-1: Good addition of createSelector import.The import is correct and necessary for memoizing selectors.
| export const selectUserState = createSelector([selectSessionCount], (sessionCount: number) => { | ||
| const isGuest = !isLoggedIn(); | ||
| return { | ||
| isGuest, | ||
| isFirstTimeGuest: isGuest && state.session.count === 2, | ||
| isFirstTimeGuest: isGuest && sessionCount === 2, | ||
| }; | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Critical: Memoization is incomplete due to external state dependency.
The memoization won't fully resolve the Redux warning because isLoggedIn() is called inside the result function. Since isLoggedIn() reads from cookies (external state not tracked by Redux), the selector can return new object references even when sessionCount hasn't changed:
- If the user logs in/out, the cookie changes but
sessionCountremains the same - The memoized result won't update because the input (
sessionCount) hasn't changed - On subsequent calls,
isLoggedIn()might return a different value, causing a new object to be created
Selectors should only depend on Redux state. Consider one of these approaches:
Solution 1: Add a selector for logged-in state (recommended)
First, check if the auth/user state is stored in Redux. If so, create a selector for it:
// Assuming auth state exists in Redux
export const selectIsLoggedIn = (state: RootState) => state.auth.isLoggedIn; // adjust path as needed
export const selectUserState = createSelector(
[selectSessionCount, selectIsLoggedIn],
(sessionCount: number, isLoggedIn: boolean) => {
const isGuest = !isLoggedIn;
return {
isGuest,
isFirstTimeGuest: isGuest && sessionCount === 2,
};
}
);Solution 2: Store logged-in state in Redux
If the logged-in state isn't in Redux, add it to a slice (e.g., auth slice or session slice) and update it when the user logs in/out, then use Solution 1.
🤖 Prompt for AI Agents
In src/redux/slices/session.ts around lines 36-42, the selector calls
isLoggedIn() (reads cookies) inside the result function which breaks selector
memoization because it depends on external state; change the selector to only
depend on Redux state by adding/using a selector for login status (e.g.,
selectIsLoggedIn from auth or session slice) and include it in createSelector's
input array so the result function receives both sessionCount and isLoggedIn and
returns the object; if login state is not in Redux, add a boolean field to an
auth/session slice, update it on login/logout, and use that selector instead.
There was a problem hiding this comment.
isLoggedIn is not a reactive value nor lives inside redux state.
Instead of relying on isLoggedIninside redux selector, it'd be more correct to use useIsLogedIn hook in ReadingSection component (that's the only place that uses selectUserState selector)
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Finding usages of selectUserState ==="
rg -n "selectUserState" --type=ts --type=tsx -C2
echo -e "\n=== Finding useIsLoggedIn hook (checking both spellings) ==="
rg -n "useIsLoggedIn|useIsLogedIn" --type=ts --type=tsx -C2
echo -e "\n=== Finding ReadingSection component ==="
fd -e ts -e tsx "ReadingSection" --exec echo "Found: {}"
Length of output: 201
🏁 Script executed:
#!/bin/bash
echo "=== Finding usages of selectUserState ==="
rg -n "selectUserState" -g "*.ts" -g "*.tsx" -C2
echo -e "\n=== Finding useIsLoggedIn hook (checking both spellings) ==="
rg -n "useIsLoggedIn|useIsLogedIn" -g "*.ts" -g "*.tsx" -C2
echo -e "\n=== Finding ReadingSection component files ==="
fd -e ts -e tsx "ReadingSection"
echo -e "\n=== Finding where ReadingSection is defined ==="
rg -n "const ReadingSection|function ReadingSection|export.*ReadingSection" -g "*.ts" -g "*.tsx" -C1
Length of output: 3223
🏁 Script executed:
#!/bin/bash
echo "=== Looking at useIsLoggedIn hook implementation ==="
cat src/hooks/auth/useIsLoggedIn.ts
echo -e "\n=== Looking at ReadingSection component to see how selectUserState is used ==="
cat src/components/HomePage/ReadingSection/index.tsx | head -50
Length of output: 2772
@jonybekov You're absolutely right! That's a better approach. Since useIsLoggedIn is a reactive hook (polls every second and listens to storage events) and selectUserState is only used in the ReadingSection component, moving the logic to the component level makes much more sense.
Here's the refactoring:
1. Simplify the selector in src/redux/slices/session.ts:
// Remove selectUserState entirely since it's mixing concerns
// Keep only selectSessionCount which is already exported
export const selectSessionCount = (state: RootState) => state.session.count;2. Update ReadingSection component (src/components/HomePage/ReadingSection/index.tsx):
import useIsLoggedIn from '@/hooks/auth/useIsLoggedIn';
import { selectSessionCount } from '@/redux/slices/session';
const ReadingSection: React.FC<Props> = () => {
const { t } = useTranslation('home');
const { isLoggedIn } = useIsLoggedIn();
const sessionCount = useSelector(selectSessionCount);
const isGuest = !isLoggedIn;
const isFirstTimeGuest = isGuest && sessionCount === 2;
const { recentlyReadVerseKeys } = useGetRecentlyReadVerseKeys(false);
// ... rest of the componentThis approach:
- ✅ Removes external state dependency from the Redux selector
- ✅ Makes the selector properly memoized (only depends on Redux state)
- ✅ Keeps reactivity through the
useIsLoggedInhook - ✅ Simplifies the architecture by keeping concerns separated
🧬 Code Graph Analysis Results
[relevant_code_snippets]
-
File: src/utils/auth/login.ts
Line range: 15-15
Code:export const isLoggedIn = () => !!getUserIdCookie(); -
File: src/redux/slices/session.ts
Summary: selectUserState selector
Description:- Parameters: state: RootState
- Returns: { isGuest: boolean; isFirstTimeGuest: boolean }
- Implementation details: derives isGuest by calling isLoggedIn() (imported from src/utils/auth/login); determines isFirstTimeGuest as true when isGuest is true and sessionCount === 2; relies on selectSessionCount to obtain sessionCount; not reactive on state changes beyond Redux-derived values, and uses a non-reactive isLoggedIn() call which reads cookies.
✏️ Learnings added
Learnt from: jonybekov
PR: quran/quran.com-frontend-next#2537
File: src/redux/slices/session.ts:36-42
Timestamp: 2025-10-18T11:22:53.120Z
Learning: In Redux selectors, avoid calling functions that read from external state (cookies, localStorage, etc.) like `isLoggedIn()` from utils/auth/login. Instead, use reactive hooks like `useIsLoggedIn` directly in components and keep selectors focused only on Redux state to ensure proper memoization.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
PR: quran/quran.com-frontend-next#0
File: .cursor/rules/redux-toolkit-best-practices.mdc:0-0
Timestamp: 2025-08-01T08:51:05.924Z
Learning: Applies to src/redux/**/*.ts : Use selectors for accessing state in components.
Learnt from: CR
PR: quran/quran.com-frontend-next#0
File: .cursor/rules/redux-toolkit-best-practices.mdc:0-0
Timestamp: 2025-08-01T08:51:05.924Z
Learning: Applies to src/redux/**/*.ts : Use Redux hooks (useSelector, useDispatch) in components.
9c8660c to
b9c054d
Compare
Summary
When I first installed project locally and run the dev server, I encountered the following error in the console:
This is reasonable warning because any selector that returns new object should be memoized.
So, I memoized selectUserState selector using built-in createSelector function from @redux/toolkit package.
Type of change
Test plan
This should state how this PR have been tested.
Checklist
Summary by CodeRabbit