Fix: Page number tracking in Reading and Translation modes #2587
Conversation
…ponsivene and RTL support (#2558) * Update PopularButton and ChapterCard styles for improved responsiveness and RTL support * Update HeroButtons styles to enforce nowrap text wrapping for better layout consistency * Update PopularButton styles to set fixed widths for improved layout on different screen sizes * Remove extra newline in PopularButton styles * Remove unnecessary whitespace in PopularButton styles for cleaner code * Implement PopularDropdown component and integrate it into HomePageHero; refactor PopularButton to accept onClick prop for dropdown handling * Remove PopularButton styles as part of the component refactor; streamline codebase by eliminating unused SCSS file.
…servers Move intersection observer registration and data attributes from VerseText and ChapterHeader components to Line and TranslationViewCell components for better consistency in page tracking. Update useSyncChapterPage hook to detect navigation changes and sync pages accordingly. This refactoring improves code organization without altering functionality.
WalkthroughAdds intersection-observer registration and data-attribute metadata to reading and translation view elements, syncs navigation detection in chapter/page synchronization hook, and removes observer-related refs/attributes from verse text and chapter header components. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Line / TranslationViewCell
participant Observer as Global Intersection Observer
participant Hook as useSyncChapterPage
participant Store as Redux
UI->>Observer: register element via mergedRef (auto-scroll + observer)
Observer-->>UI: emits visibility events (using data-* metadata)
Note right of Hook: Navigation detection
Hook->>Hook: detect hasNavigated & update initialDataRef
Hook->>Store: dispatch update to lastReadVerse.page (firstPageNumber)
Store-->>UI: updated lastReadVerseKey reflected in UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (11)**/*.*📄 CodeRabbit inference engine (.cursor/rules/bug-handling-with-todo-comments.mdc)
Files:
**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/next-js-conventions.mdc)
Files:
src/components/**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/react-functional-components.mdc)
Files:
**/*.{tsx,jsx}📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Files:
**/*.tsx📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Files:
**/*.{ts,tsx,d.ts}📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
{src,types}/**/*.ts?(x)📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.tsx📄 CodeRabbit inference engine (CLAUDE.md)
Files:
{src,types}/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/components/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (15)📓 Common learnings📚 Learning: 2025-10-26T10:22:52.381ZApplied to files:
📚 Learning: 2025-10-19T11:34:07.609ZApplied to files:
📚 Learning: 2025-10-28T09:59:48.894ZApplied to files:
📚 Learning: 2025-07-31T15:31:22.667ZApplied to files:
📚 Learning: 2025-08-01T08:51:28.930ZApplied to files:
📚 Learning: 2025-08-01T08:50:54.518ZApplied to files:
📚 Learning: 2025-08-01T08:50:54.518ZApplied to files:
📚 Learning: 2025-10-25T12:14:44.062ZApplied to files:
📚 Learning: 2025-10-18T11:22:53.139ZApplied to files:
📚 Learning: 2025-10-22T20:42:09.214ZApplied to files:
📚 Learning: 2025-08-01T08:50:54.518ZApplied to files:
📚 Learning: 2025-08-01T08:50:54.518ZApplied to files:
📚 Learning: 2025-08-01T08:51:05.924ZApplied to files:
📚 Learning: 2025-08-01T08:50:54.518ZApplied to files:
🧬 Code graph analysis (1)src/components/QuranReader/TranslationView/TranslationViewCell.tsx (1)
🔇 Additional comments (2)
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}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/components/QuranReader/ReadingView/Line.tsx(3 hunks)src/components/QuranReader/TranslationView/TranslationViewCell.tsx(3 hunks)src/components/QuranReader/hooks/useSyncChapterPage.ts(4 hunks)src/components/Verse/VerseText.tsx(1 hunks)src/components/chapters/ChapterHeader/index.tsx(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/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
**/*.{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/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
src/components/**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/react-functional-components.mdc)
src/components/**/*.tsx: Always use React functional components with hooks.
Use React.FC for functional components with props.
Files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
**/*.{tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
**/*.{tsx,jsx}: Use functional components over class components
Keep components small and focused
Extract reusable logic into custom hooks
Use composition over inheritance
Split large components into smaller, focused ones
Follow the Rules of Hooks
Use custom hooks for reusable logic
Use appropriate dependency arrays in useEffect
Implement cleanup in useEffect when needed
Avoid nested hooks
Use useState for local component state
Avoid prop drilling through proper state management
Implement proper memoization (useMemo, useCallback)
Use React.memo for expensive components
Avoid unnecessary re-renders
Implement proper lazy loading
Use proper key props in lists
Profile and optimize render performance
Show appropriate loading and error states
Handle async errors properly
Show user-friendly error messages
Implement proper fallback UI
Log errors appropriately
Handle edge cases gracefully
Use semantic HTML elements
Implement proper ARIA attributes
Ensure keyboard navigation
Handle focus management
Provide proper alt text for images
Use proper imports/exports
Document complex component logic
Files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Implement proper prop types with TypeScript
**/*.tsx: Prefix interfaces for React props with 'Props' (e.g., ButtonProps)
Implement proper error boundaries
Files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/react.mdc)
Use xstate for complex state logic
Files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
**/*.{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/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
{src,types}/**/*.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript throughout the codebase with strict configuration
Files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer React functional components for UI implementation
Files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
{src,types}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use path aliases @/ for src and @/dls/* for design system imports
Files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
src/components/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Organize React components by feature and place DLS components under src/components/dls
Files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
🧠 Learnings (26)
📓 Common learnings
Learnt from: afifvdin
Repo: quran/quran.com-frontend-next PR: 2540
File: src/components/QuranReader/TranslationView/TranslationText/TranslationAndReference.tsx:60-60
Timestamp: 2025-10-26T10:22:52.381Z
Learning: In src/components/QuranReader/TranslationView/TranslationText/TranslationAndReference.tsx, verse references displayed alongside chapter names should use English/Western numerals only, not localized numerals, as per design specifications.
Learnt from: zonetecde
Repo: quran/quran.com-frontend-next PR: 2523
File: src/components/QuranicCalendar/MyProgress/MonthCard.tsx:48-49
Timestamp: 2025-10-09T11:21:33.919Z
Learning: In the Quran in a Year calendar feature (src/components/QuranicCalendar/MyProgress/MonthCard.tsx), instant scrolling (window.scrollTo({ top: 0 }) without smooth behavior) is preferred over smooth scrolling when selecting a week. This is because instant scrolling provides immediate visual feedback of the verses and date changing, whereas smooth scrolling makes it look like nothing is changing during the animation.
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{tsx,jsx} : Handle focus management
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{test,spec}.{tsx,jsx} : Test user interactions
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{tsx,jsx} : Ensure keyboard navigation
Learnt from: afifvdin
Repo: quran/quran.com-frontend-next PR: 2540
File: src/components/Verse/PlainVerseText/index.tsx:4-4
Timestamp: 2025-10-19T11:34:07.609Z
Learning: In src/components/Verse/PlainVerseText/index.tsx, the translation for verse titles should be wired directly to the component using next-translate rather than passed as props from parent components, as this ensures the wording from locales is centralized and only needs to be changed in one place.
Learnt from: afifvdin
Repo: quran/quran.com-frontend-next PR: 2557
File: src/components/QuranReader/ReflectionView/ReflectionBodyContainer/ReflectionBody/index.tsx:88-0
Timestamp: 2025-10-28T09:59:48.894Z
Learning: In src/components/QuranReader/ReflectionView/ReflectionBodyContainer/ReflectionBody/index.tsx, the empty `<div className={styles.separatorContainer} />` rendered when `isModal` is true is required for maintaining consistent spacing in modal view and should not be removed.
Learnt from: yousefdergham
Repo: quran/quran.com-frontend-next PR: 2547
File: src/pages/learning-plans/[slug]/lessons/[lessonSlugOrId]/index.tsx:35-59
Timestamp: 2025-10-22T20:42:09.214Z
Learning: Preference for quran.com-frontend-next: keep single-use, page-specific components inline. Example: NotEnrolledMessage in src/pages/learning-plans/[slug]/lessons/[lessonSlugOrId]/index.tsx should remain inline; extract only if reused elsewhere.
📚 Learning: 2025-10-26T10:22:52.381Z
Learnt from: afifvdin
Repo: quran/quran.com-frontend-next PR: 2540
File: src/components/QuranReader/TranslationView/TranslationText/TranslationAndReference.tsx:60-60
Timestamp: 2025-10-26T10:22:52.381Z
Learning: In src/components/QuranReader/TranslationView/TranslationText/TranslationAndReference.tsx, verse references displayed alongside chapter names should use English/Western numerals only, not localized numerals, as per design specifications.
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
📚 Learning: 2025-10-28T09:59:48.894Z
Learnt from: afifvdin
Repo: quran/quran.com-frontend-next PR: 2557
File: src/components/QuranReader/ReflectionView/ReflectionBodyContainer/ReflectionBody/index.tsx:88-0
Timestamp: 2025-10-28T09:59:48.894Z
Learning: In src/components/QuranReader/ReflectionView/ReflectionBodyContainer/ReflectionBody/index.tsx, the empty `<div className={styles.separatorContainer} />` rendered when `isModal` is true is required for maintaining consistent spacing in modal view and should not be removed.
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
📚 Learning: 2025-10-19T11:34:07.609Z
Learnt from: afifvdin
Repo: quran/quran.com-frontend-next PR: 2540
File: src/components/Verse/PlainVerseText/index.tsx:4-4
Timestamp: 2025-10-19T11:34:07.609Z
Learning: In src/components/Verse/PlainVerseText/index.tsx, the translation for verse titles should be wired directly to the component using next-translate rather than passed as props from parent components, as this ensures the wording from locales is centralized and only needs to be changed in one place.
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
📚 Learning: 2025-10-09T11:21:33.919Z
Learnt from: zonetecde
Repo: quran/quran.com-frontend-next PR: 2523
File: src/components/QuranicCalendar/MyProgress/MonthCard.tsx:48-49
Timestamp: 2025-10-09T11:21:33.919Z
Learning: In the Quran in a Year calendar feature (src/components/QuranicCalendar/MyProgress/MonthCard.tsx), instant scrolling (window.scrollTo({ top: 0 }) without smooth behavior) is preferred over smooth scrolling when selecting a week. This is because instant scrolling provides immediate visual feedback of the verses and date changing, whereas smooth scrolling makes it look like nothing is changing during the animation.
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsx
📚 Learning: 2025-10-22T20:33:55.176Z
Learnt from: yousefdergham
Repo: quran/quran.com-frontend-next PR: 2547
File: src/components/Course/CourseDetails/Tabs/Syllabus/index.tsx:44-49
Timestamp: 2025-10-22T20:33:55.176Z
Learning: Preference: In quran.com-frontend-next, keep concise “why” comments for analytics/UX decision points (e.g., guest vs enrolled click handlers) but avoid verbose JSDoc that repeats params/types. Convert multi-line JSDoc to brief inline rationale comments, especially in src/components/Course/CourseDetails/Tabs/Syllabus/index.tsx and similar analytics handlers.
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/chapters/ChapterHeader/index.tsx
📚 Learning: 2025-10-22T20:42:09.214Z
Learnt from: yousefdergham
Repo: quran/quran.com-frontend-next PR: 2547
File: src/pages/learning-plans/[slug]/lessons/[lessonSlugOrId]/index.tsx:35-59
Timestamp: 2025-10-22T20:42:09.214Z
Learning: Preference for quran.com-frontend-next: keep single-use, page-specific components inline. Example: NotEnrolledMessage in src/pages/learning-plans/[slug]/lessons/[lessonSlugOrId]/index.tsx should remain inline; extract only if reused elsewhere.
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/Verse/VerseText.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{tsx,jsx} : Use React.memo for expensive components
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsx
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{tsx,jsx} : Implement proper memoization (useMemo, useCallback)
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsx
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{tsx,jsx} : Use custom hooks for reusable logic
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
📚 Learning: 2025-08-01T08:50:22.602Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react-functional-components.mdc:0-0
Timestamp: 2025-08-01T08:50:22.602Z
Learning: Applies to src/components/**/*.tsx : Always use React functional components with hooks.
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.ts
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/use*.{tsx,jsx} : Keep hooks focused and simple
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{tsx,jsx} : Extract reusable logic into custom hooks
Applied to files:
src/components/QuranReader/ReadingView/Line.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsx
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{tsx,jsx} : Use functional components over class components
Applied to files:
src/components/QuranReader/ReadingView/Line.tsx
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{tsx,jsx} : Document complex component logic
Applied to files:
src/components/QuranReader/ReadingView/Line.tsx
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{test,spec}.{tsx,jsx} : Use React Testing Library
Applied to files:
src/components/QuranReader/ReadingView/Line.tsx
📚 Learning: 2025-07-31T15:31:22.667Z
Learnt from: AhmedCodeGuy
Repo: quran/quran.com-frontend-next PR: 2477
File: src/components/QuranReader/TranslationView/TranslationViewCell.module.scss:204-207
Timestamp: 2025-07-31T15:31:22.667Z
Learning: AhmedCodeGuy suggested that the Separator component should be refactored in the future to avoid needing !important declarations for width overrides in contexts like .verseSeparator in TranslationViewCell.module.scss.
Applied to files:
src/components/Verse/VerseText.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsx
📚 Learning: 2025-10-25T12:14:44.062Z
Learnt from: yousefdergham
Repo: quran/quran.com-frontend-next PR: 2547
File: src/pages/learning-plans/[slug]/lessons/[lessonSlugOrId]/index.tsx:58-64
Timestamp: 2025-10-25T12:14:44.062Z
Learning: In quran.com-frontend-next, router.query values (like slug) are not considered reactive dependencies by the project's react-hooks/exhaustive-deps rule and don't need to be included in useEffect dependency arrays. Toast functions from useToast() are also stable and don't need to be in deps.
Applied to files:
src/components/Verse/VerseText.tsxsrc/components/QuranReader/TranslationView/TranslationViewCell.tsx
📚 Learning: 2025-08-01T08:50:03.171Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/next-js-conventions.mdc:0-0
Timestamp: 2025-08-01T08:50:03.171Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Rely on Next.js Pages Router for state changes.
Applied to files:
src/components/Verse/VerseText.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.ts
📚 Learning: 2025-08-01T08:49:45.154Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/next-js-app-router-rule.mdc:0-0
Timestamp: 2025-08-01T08:49:45.154Z
Learning: Applies to app/**/*.tsx : Follow Next.js documentation for best practices in data fetching, rendering, and routing
Applied to files:
src/components/Verse/VerseText.tsx
📚 Learning: 2025-10-18T11:22:53.139Z
Learnt from: jonybekov
Repo: quran/quran.com-frontend-next PR: 2537
File: src/redux/slices/session.ts:36-42
Timestamp: 2025-10-18T11:22:53.139Z
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.
Applied to files:
src/components/Verse/VerseText.tsxsrc/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{tsx,jsx} : Use appropriate dependency arrays in useEffect
Applied to files:
src/components/QuranReader/hooks/useSyncChapterPage.ts
📚 Learning: 2025-08-01T08:51:05.924Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 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.
Applied to files:
src/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/QuranReader/TranslationView/TranslationViewCell.tsxsrc/components/chapters/ChapterHeader/index.tsx
📚 Learning: 2025-08-01T08:50:54.518Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: .cursor/rules/react.mdc:0-0
Timestamp: 2025-08-01T08:50:54.518Z
Learning: Applies to **/*.{tsx,jsx} : Use useState for local component state
Applied to files:
src/components/QuranReader/hooks/useSyncChapterPage.tssrc/components/chapters/ChapterHeader/index.tsx
📚 Learning: 2025-08-01T08:51:05.924Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 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/components/QuranReader/hooks/useSyncChapterPage.ts
📚 Learning: 2025-10-07T08:37:58.213Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-07T08:37:58.213Z
Learning: Applies to src/redux/**/*.ts : Use Redux Toolkit for state management and persistence (redux-persist) within the redux layer
Applied to files:
src/components/QuranReader/hooks/useSyncChapterPage.ts
🧬 Code graph analysis (2)
src/components/QuranReader/ReadingView/Line.tsx (1)
src/components/QuranReader/observer.ts (1)
QURAN_READER_OBSERVER_ID(13-13)
src/components/QuranReader/TranslationView/TranslationViewCell.tsx (1)
src/components/QuranReader/observer.ts (1)
QURAN_READER_OBSERVER_ID(13-13)
| // Register with intersection observer for page tracking | ||
| const observerRef = useRef(null); | ||
| useIntersectionObserver(observerRef, QURAN_READER_OBSERVER_ID); | ||
|
|
||
| // Merge refs for both auto-scroll and observer | ||
| const mergedRef = useCallback( | ||
| (node: HTMLDivElement) => { | ||
| // Update both refs | ||
| if (selectedItemRef) { | ||
| (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node; | ||
| } | ||
| observerRef.current = node; | ||
| }, | ||
| [selectedItemRef], | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mirror the null-safe ref typing here too
Same note as TranslationViewCell: the ref callback should accept HTMLDivElement | null, and the stored refs should allow null to keep the types aligned with React’s expectations and avoid casting null into a non-null slot.
- const observerRef = useRef(null);
+ const observerRef = useRef<HTMLDivElement | null>(null);
@@
- const mergedRef = useCallback(
- (node: HTMLDivElement) => {
+ const mergedRef = useCallback(
+ (node: HTMLDivElement | null) => {
if (selectedItemRef) {
- (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node;
+ (selectedItemRef as React.MutableRefObject<HTMLDivElement | null>).current = node;
}
observerRef.current = node;
},
[selectedItemRef],
);🤖 Prompt for AI Agents
In src/components/QuranReader/ReadingView/Line.tsx around lines 45 to 59, the
ref callback and stored refs should be null-safe: change the mergedRef signature
to accept node: HTMLDivElement | null, ensure observerRef is typed as
React.MutableRefObject<HTMLDivElement | null> (useRef<HTMLDivElement |
null>(null)), and assign node directly to both selectedItemRef.current and
observerRef.current without casting so both refs can hold null per React
expectations.
| // Register this cell with the global intersection observer for page tracking | ||
| const observerRef = useRef(null); | ||
| useIntersectionObserver(observerRef, QURAN_READER_OBSERVER_ID); | ||
|
|
||
| // Callback ref to merge both selectedItemRef and observerRef | ||
| const mergedRef = useCallback( | ||
| (node: HTMLDivElement) => { | ||
| // Update both refs | ||
| if (selectedItemRef) { | ||
| (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node; | ||
| } | ||
| observerRef.current = node; | ||
| }, | ||
| [selectedItemRef], | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Allow the merged ref to handle null
React ref callbacks receive null during unmount/re-mount. By typing the callback as HTMLDivElement and casting to MutableRefObject<HTMLDivElement>, we end up writing null into a ref that TypeScript thinks is always non-null. Widen the callback argument and the stored ref types to HTMLDivElement | null so we preserve correct nullability and keep the typings safe.
- const observerRef = useRef(null);
+ const observerRef = useRef<HTMLDivElement | null>(null);
@@
- const mergedRef = useCallback(
- (node: HTMLDivElement) => {
+ const mergedRef = useCallback(
+ (node: HTMLDivElement | null) => {
if (selectedItemRef) {
- (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node;
+ (selectedItemRef as React.MutableRefObject<HTMLDivElement | null>).current = node;
}
observerRef.current = node;
},
[selectedItemRef],
);🤖 Prompt for AI Agents
In src/components/QuranReader/TranslationView/TranslationViewCell.tsx around
lines 76 to 90, the merged ref callback and stored refs currently assume a
non-null HTMLDivElement which breaks TypeScript nullability when React passes
null on unmount; change observerRef to useRef<HTMLDivElement | null>(null),
widen the mergedRef parameter to (node: HTMLDivElement | null), and assign node
into both refs using the nullable types (i.e., cast selectedItemRef as
React.MutableRefObject<HTMLDivElement | null> before setting .current), ensuring
you guard against selectedItemRef being undefined and keep the same dependency
array for useCallback.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes page number tracking in Reading and Translation modes by implementing a unified observer strategy. The changes move intersection observer logic from VerseText and ChapterHeader components to the Line and TranslationViewCell container components, eliminating conflicts from multiple observers reporting different page numbers.
Key Changes
- Moved page tracking observers from child components (VerseText, ChapterHeader) to container components (Line, TranslationViewCell)
- Enhanced navigation detection in useSyncChapterPage to handle chapter, juz, and page changes
- Cleaned up redundant observer logic and data attributes
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/components/chapters/ChapterHeader/index.tsx | Removed intersection observer and related data attributes that were interfering with page tracking |
| src/components/Verse/VerseText.tsx | Removed observer logic, data attributes, and unused imports to eliminate conflicts |
| src/components/QuranReader/hooks/useSyncChapterPage.ts | Added navigation change detection using initialData reference tracking |
| src/components/QuranReader/TranslationView/TranslationViewCell.tsx | Added intersection observer with merged ref pattern for page tracking in translation mode |
| src/components/QuranReader/ReadingView/Line.tsx | Added intersection observer with merged ref pattern for page tracking in reading mode |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const initialDataRef = useRef(initialData); | ||
|
|
||
| useEffect(() => { | ||
| // Detect if initialData changed (indicates navigation to different chapter/juz/page) | ||
| const hasNavigated = initialDataRef.current !== initialData; | ||
| if (hasNavigated) { | ||
| initialDataRef.current = initialData; |
There was a problem hiding this comment.
This reference comparison will always be true when initialData is a new object reference, even if the data inside hasn't changed. This could cause unnecessary page updates. Consider using a deep equality check or comparing specific properties (e.g., chapter ID) instead of the entire object reference.
| const initialDataRef = useRef(initialData); | |
| useEffect(() => { | |
| // Detect if initialData changed (indicates navigation to different chapter/juz/page) | |
| const hasNavigated = initialDataRef.current !== initialData; | |
| if (hasNavigated) { | |
| initialDataRef.current = initialData; | |
| // Store the last seen chapterId (or other unique identifier) to detect navigation | |
| const getInitialDataId = (data: any) => data?.chapterId ?? null; | |
| const initialDataIdRef = useRef(getInitialDataId(initialData)); | |
| useEffect(() => { | |
| // Detect if the unique identifier in initialData changed (indicates navigation) | |
| const currentId = getInitialDataId(initialData); | |
| const hasNavigated = initialDataIdRef.current !== currentId; | |
| if (hasNavigated) { | |
| initialDataIdRef.current = currentId; |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- chaptersData is from context and stable; omitting to prevent unnecessary re-renders | ||
| }, [firstPageNumber, lastReadVerseKey, dispatch]); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps -- Trigger on firstPageNumber, chapterId, or initialData changes | ||
| }, [firstPageNumber, lastReadVerseKey?.chapterId, initialData, dispatch]); |
There was a problem hiding this comment.
Including initialData in the dependency array combined with the hasNavigated check creates redundant logic. When initialData changes, the effect will run and trigger a page update. Then, when firstPageNumber changes (which is derived from initialData), the effect will run again and potentially update the page a second time. Consider removing initialData from the dependency array and relying solely on firstPageNumber changes, or restructuring the logic to avoid double updates.
| // Update both refs | ||
| if (selectedItemRef) { | ||
| (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node; |
There was a problem hiding this comment.
The selectedItemRef returned from useScroll is a RefObject<HTMLDivElement>, which has a read-only current property. Attempting to assign to it with (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node is a type violation and may not work as expected. Consider using a callback ref pattern for useScroll or using useMemo to create a proper merged ref callback that works with both refs.
| // Update both refs | |
| if (selectedItemRef) { | |
| (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node; | |
| // Update both refs safely | |
| if (typeof selectedItemRef === 'function') { | |
| selectedItemRef(node); | |
| } else if (selectedItemRef && 'current' in selectedItemRef) { | |
| (selectedItemRef as React.RefObject<HTMLDivElement>).current = node; |
| (node: HTMLDivElement) => { | ||
| // Update both refs | ||
| if (selectedItemRef) { | ||
| (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node; |
There was a problem hiding this comment.
The selectedItemRef returned from useScrollWithContextMenuOffset is a RefObject<HTMLDivElement>, which has a read-only current property. Attempting to assign to it with (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node is a type violation and may not work as expected. Consider using a callback ref pattern for useScrollWithContextMenuOffset or using useMemo to create a proper merged ref callback that works with both refs.
| (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node; | |
| if (typeof selectedItemRef === 'function') { | |
| selectedItemRef(node); | |
| } else if ('current' in selectedItemRef) { | |
| (selectedItemRef as React.MutableRefObject<HTMLDivElement | null>).current = node; | |
| } |
|
|
||
| return ( | ||
| <div ref={selectedItemRef}> | ||
| <div> |
There was a problem hiding this comment.
[nitpick] This extra wrapping div creates an unnecessary DOM node. Consider applying the mergedRef and data attributes directly to the outer div element (the one that was previously assigned selectedItemRef), or document why this extra wrapper is necessary.
| const mergedRef = useCallback( | ||
| (node: HTMLDivElement) => { | ||
| // Update both refs | ||
| if (selectedItemRef) { | ||
| (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node; | ||
| } | ||
| observerRef.current = node; | ||
| }, | ||
| [selectedItemRef], | ||
| ); |
There was a problem hiding this comment.
The ref merging implementation has a type safety issue. selectedItemRef is typed as RefObject<HTMLDivElement> (from useScroll), which has a readonly current property. Casting it to MutableRefObject and assigning to current bypasses TypeScript's safety checks and could lead to runtime errors.
Consider using a callback ref pattern that works with the existing RefObject type, or store the node in a local ref and call scrollIntoView directly on it when needed.
| // Callback ref to merge both selectedItemRef and observerRef | ||
| const mergedRef = useCallback( | ||
| (node: HTMLDivElement) => { | ||
| // Update both refs | ||
| if (selectedItemRef) { | ||
| (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node; | ||
| } | ||
| observerRef.current = node; |
There was a problem hiding this comment.
The ref merging implementation has the same type safety issue as in Line.tsx. selectedItemRef is typed as RefObject<HTMLDivElement> (from useScrollWithContextMenuOffset), which has a readonly current property. Casting it to MutableRefObject and assigning to current bypasses TypeScript's safety checks.
Consider using a callback ref pattern that works with the existing RefObject type, or store the node in a local ref and call the scroll function with it when needed.
| // Callback ref to merge both selectedItemRef and observerRef | |
| const mergedRef = useCallback( | |
| (node: HTMLDivElement) => { | |
| // Update both refs | |
| if (selectedItemRef) { | |
| (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node; | |
| } | |
| observerRef.current = node; | |
| // Callback ref to merge both selectedItemRef and observerRef safely | |
| const mergedRef = useCallback( | |
| (node: HTMLDivElement | null) => { | |
| // Update observerRef | |
| observerRef.current = node; | |
| // If selectedItemRef is a function (callback ref), call it | |
| if (typeof selectedItemRef === 'function') { | |
| selectedItemRef(node); | |
| } | |
| // If selectedItemRef is an object ref, do not mutate .current (readonly) | |
| // If useScrollWithContextMenuOffset requires the node, ensure it is passed via callback or local state |
| // Merge refs for both auto-scroll and observer | ||
| const mergedRef = useCallback( | ||
| (node: HTMLDivElement) => { | ||
| // Update both refs | ||
| if (selectedItemRef) { | ||
| (selectedItemRef as React.MutableRefObject<HTMLDivElement>).current = node; | ||
| } | ||
| observerRef.current = node; | ||
| }, | ||
| [selectedItemRef], | ||
| ); |
There was a problem hiding this comment.
The ref merging pattern is duplicated between Line.tsx and TranslationViewCell.tsx with identical implementation. This violates the DRY (Don't Repeat Yourself) principle and makes the code harder to maintain.
Consider extracting this pattern into a custom hook like useMergedRefs(ref1, ref2) that can be reused across both components.
|
|
||
| return ( | ||
| <div ref={selectedItemRef}> | ||
| <div> |
There was a problem hiding this comment.
[nitpick] The outer wrapper <div> (line 93) appears to serve no semantic or styling purpose and creates unnecessary DOM nesting. The mergedRef, data attributes, and styling are all applied to the inner div.
Consider removing the outer wrapper and applying the ref and attributes directly to the container div, moving the <Separator> outside as a sibling if needed.
osamasayed
requested review from
mzulqarnain118
and removed request for
osamasayed
Refine chapter navigation detection in useSyncChapterPage hook by tracking previous chapterId instead of initialData, ensuring accurate page syncing when navigating between chapters. Improve type safety in ReadingView and TranslationView components by updating ref types to include null and adjusting mergedRef callbacks. Replace div wrapper with React Fragment in TranslationViewCell for cleaner rendering.
|
@AhmedCodeGuy this merge commit has build issue |
Summary
Closes: 3925
Fixed page number tracking that was not updating correctly during scroll in both Reading and Translation modes. The page number display in the context menu now updates in real-time as users scroll through verses.
Changes
Unified Observer Strategy
LinecomponentTranslationViewCellcomponentBug Fixes
Technical Details
Architecture Improvement
Implemented a cleaner separation of concerns:
Files Modified
src/components/QuranReader/ReadingView/Line.tsx- Added observer with merged refssrc/components/QuranReader/TranslationView/TranslationViewCell.tsx- Added observer with merged refssrc/components/Verse/VerseText.tsx- Removed observer logic and data attributessrc/components/chapters/ChapterHeader/index.tsx- Removed interfering observersrc/components/QuranReader/hooks/useSyncChapterPage.ts- Fixed navigation detectionTesting
Tested and verified:
?hideArabic=trueparameterFixes
Resolves the issue where page numbers were stuck at chapter boundaries and only updated after user scrolled.
Summary by CodeRabbit