[QF-3097] bookmark icons sync issue #2582
Conversation
WalkthroughThis PR adds SWR cache invalidation logic for bookmark operations. When bookmarks are deleted, the components now compute the mushafId, retrieve affected bookmark data, and explicitly delete corresponding cache entries from both the single bookmark and bookmark collections URLs. Changes
Sequence DiagramsequenceDiagram
participant User
participant Component
participant API
participant Cache
rect rgb(200, 220, 255)
Note over User,Cache: Before: Bookmark Deletion (Without Cache Invalidation)
User->>Component: Delete Bookmark
Component->>API: Send deletion request
API-->>Component: Success
Cache->>Cache: ❌ Stale cache remains
end
rect rgb(220, 240, 220)
Note over User,Cache: After: Bookmark Deletion (With Cache Invalidation)
User->>Component: Delete Bookmark
Component->>Component: Compute mushafId from styles
Component->>API: Send deletion request
API-->>Component: Success
Component->>Cache: cache.delete(bookmarkUrl)
Component->>Cache: cache.delete(collectionsUrl)
Cache->>Cache: ✓ Stale cache cleared
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsx(6 hunks)src/components/Verses/BookmarkedVersesList.tsx(5 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
**/*.*
📄 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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.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 boundariesReact: use functional components only; extract reusable logic into custom hooks; apply proper memoization (React.memo/useMemo/useCallback)
Files:
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.tsx
**/*.{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/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.tsx
{src,types}/**/*.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript throughout the codebase with strict configuration
Files:
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.tsx
src/**/*.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Prefer React functional components for UI implementation
Files:
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.tsx
{src,types}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use path aliases @/ for src and @/dls/* for design system imports
Files:
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.tsx
**/*.{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/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.tsx
**/*.{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/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.tsx
🧠 Learnings (13)
📚 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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsx
📚 Learning: 2025-09-19T13:57:41.697Z
Learnt from: mohsinayoob
Repo: quran/quran.com-frontend-next PR: 2495
File: src/utils/auth/constants.ts:37-50
Timestamp: 2025-09-19T13:57:41.697Z
Learning: In the quran.com-frontend-next codebase, prefer TypeScript's readonly modifier over Object.freeze() for array immutability when compile-time protection is sufficient.
Applied to files:
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.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/Collection/CollectionDetailContainer/CollectionDetailContainer.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/Collection/CollectionDetailContainer/CollectionDetailContainer.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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsx
📚 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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.tsx
📚 Learning: 2025-10-07T08:38:28.343Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T08:38:28.343Z
Learning: Applies to pages/**/*.{ts,tsx} : Use Next.js Pages Router patterns (do not use App Router)
Applied to files:
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsx
📚 Learning: 2025-10-07T08:38:28.343Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T08:38:28.343Z
Learning: Applies to **/*.{ts,tsx} : Implement i18n with next-translate in components and utilities
Applied to files:
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.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/Collection/CollectionDetailContainer/CollectionDetailContainer.tsxsrc/components/Verses/BookmarkedVersesList.tsx
📚 Learning: 2025-10-20T15:04:10.434Z
Learnt from: afifvdin
Repo: quran/quran.com-frontend-next PR: 2540
File: src/components/Verse/VerseAndTranslation.tsx:8-11
Timestamp: 2025-10-20T15:04:10.434Z
Learning: In the quran.com-frontend-next codebase, the Error component imported from '@/components/Error' should not be renamed to avoid shadowing the global Error object, as this is the established convention consistently used across the codebase.
Applied to files:
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.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/Verses/BookmarkedVersesList.tsx
📚 Learning: 2025-10-07T08:38:28.343Z
Learnt from: CR
Repo: quran/quran.com-frontend-next PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-07T08:38:28.343Z
Learning: Applies to **/*.tsx : React: use functional components only; extract reusable logic into custom hooks; apply proper memoization (React.memo/useMemo/useCallback)
Applied to files:
src/components/Verses/BookmarkedVersesList.tsx
🧬 Code graph analysis (2)
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsx (3)
src/redux/slices/QuranReader/styles.ts (1)
selectQuranReaderStyles(141-141)src/utils/api.ts (1)
getMushafId(65-77)src/utils/auth/apiPaths.ts (2)
makeBookmarkUrl(200-206)makeBookmarkCollectionsUrl(175-181)
src/components/Verses/BookmarkedVersesList.tsx (6)
src/components/dls/Toast/Toast.tsx (1)
useToast(86-96)src/redux/slices/QuranReader/bookmarks.ts (1)
selectBookmarks(63-63)src/utils/api.ts (1)
getMushafId(65-77)src/utils/auth/login.ts (1)
isLoggedIn(17-17)src/utils/auth/apiPaths.ts (3)
makeBookmarksUrl(62-63)makeBookmarkUrl(200-206)makeBookmarkCollectionsUrl(175-181)src/utils/auth/api.ts (1)
deleteBookmarkById(440-442)
🔇 Additional comments (7)
src/components/Verses/BookmarkedVersesList.tsx (4)
10-10: LGTM: Cache invalidation imports added correctly.The new imports support the cache invalidation fix and are properly organized.
Also applies to: 20-20, 23-27
40-40: LGTM: SWR cache access configured correctly.
48-53: LGTM: Centralized URL construction improves consistency.Using
makeBookmarksUrlhelper ensures consistent API URL construction across the codebase.
80-124: LGTM: Cache invalidation logic correctly implemented.The bookmark deletion flow is well-structured:
- Early return prevents errors when bookmark not found
mutate()revalidates the bookmarks list- Cache entries for both bookmark URL and bookmark collections URL are invalidated
- Error handling with user-friendly toast message
The cache invalidation ensures the UI reflects the deletion immediately when users navigate to the ayah page.
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsx (3)
6-7: LGTM: Cache invalidation imports added correctly.The new imports support the cache invalidation fix and maintain consistency with the pattern used in BookmarkedVersesList.tsx.
Also applies to: 19-22
61-64: LGTM: Redux and SWR configuration set up correctly.Using
shallowEqualwith the selector prevents unnecessary re-renders, following React best practices.
114-136: LGTM: Bookmark deletion handlers correctly pass bookmarkId.Both deletion paths (direct bookmark deletion and collection-bookmark removal) now pass
bookmarkIdtoonUpdated, enabling targeted cache invalidation.
| /* eslint-disable max-lines */ | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider refactoring to address max-lines violation.
The max-lines disable suggests this component exceeds recommended size limits. Per coding guidelines, components should follow the single responsibility principle and be split when they become too complex.
🤖 Prompt for AI Agents
In
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsx
lines 1-2, the file currently disables the max-lines rule indicating the
component is too large; refactor it by extracting distinct responsibilities into
smaller modules: move complex hooks and data-fetching logic into a custom hook
file (e.g., useCollectionDetail.ts), pull out large render blocks into
presentational subcomponents (e.g., CollectionHeader, CollectionMetadata,
CollectionItems) placed in the same folder, relocate any helper functions or
constants to a utils file, and keep the main container focused on orchestration
and state composition while updating imports and tests accordingly.
| const onUpdated = (bookmarkId: string) => { | ||
| mutate(); | ||
|
|
||
| const mushafId = getMushafId(quranReaderStyles.quranFont, quranReaderStyles.mushafLines).mushaf; | ||
| const bookmark = bookmarks.find((b) => b.id === bookmarkId); | ||
|
|
||
| if (bookmark) { | ||
| cache.delete( | ||
| makeBookmarkUrl(mushafId, bookmark.key, BookmarkType.Ayah, bookmark.verseNumber), | ||
| ); | ||
|
|
||
| cache.delete( | ||
| makeBookmarkCollectionsUrl(mushafId, bookmark.key, BookmarkType.Ayah, bookmark.verseNumber), | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM: Cache invalidation correctly implemented.
The cache invalidation logic properly:
- Revalidates data with
mutate() - Computes mushafId from current styles
- Finds the deleted bookmark and invalidates both cache entries
The conditional cache deletion is acceptable since the bookmark may not be in the currently loaded pages (due to pagination).
Optional: Consider memoizing mushafId at component level for consistency.
For consistency with the broader caching strategy and to avoid recomputing mushafId on every deletion:
+const mushafId = useMemo(
+ () => getMushafId(quranReaderStyles.quranFont, quranReaderStyles.mushafLines).mushaf,
+ [quranReaderStyles.quranFont, quranReaderStyles.mushafLines]
+);
+
const onUpdated = (bookmarkId: string) => {
mutate();
- const mushafId = getMushafId(quranReaderStyles.quranFont, quranReaderStyles.mushafLines).mushaf;
const bookmark = bookmarks.find((b) => b.id === bookmarkId);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onUpdated = (bookmarkId: string) => { | |
| mutate(); | |
| const mushafId = getMushafId(quranReaderStyles.quranFont, quranReaderStyles.mushafLines).mushaf; | |
| const bookmark = bookmarks.find((b) => b.id === bookmarkId); | |
| if (bookmark) { | |
| cache.delete( | |
| makeBookmarkUrl(mushafId, bookmark.key, BookmarkType.Ayah, bookmark.verseNumber), | |
| ); | |
| cache.delete( | |
| makeBookmarkCollectionsUrl(mushafId, bookmark.key, BookmarkType.Ayah, bookmark.verseNumber), | |
| ); | |
| } | |
| }; | |
| const mushafId = useMemo( | |
| () => getMushafId(quranReaderStyles.quranFont, quranReaderStyles.mushafLines).mushaf, | |
| [quranReaderStyles.quranFont, quranReaderStyles.mushafLines] | |
| ); | |
| const onUpdated = (bookmarkId: string) => { | |
| mutate(); | |
| const bookmark = bookmarks.find((b) => b.id === bookmarkId); | |
| if (bookmark) { | |
| cache.delete( | |
| makeBookmarkUrl(mushafId, bookmark.key, BookmarkType.Ayah, bookmark.verseNumber), | |
| ); | |
| cache.delete( | |
| makeBookmarkCollectionsUrl(mushafId, bookmark.key, BookmarkType.Ayah, bookmark.verseNumber), | |
| ); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsx
around lines 80 to 95, memoize the computed mushafId instead of recomputing it
inside onUpdated: compute mushafId once at the component level (e.g., using
useMemo or a ref) based on quranReaderStyles.quranFont and
quranReaderStyles.mushafLines and reference that memoized value within
onUpdated; ensure the memo updates when the relevant quranReaderStyles change so
cache deletion uses a stable, consistent mushafId.
| /* eslint-disable max-lines, react-func/max-lines-per-function */ | ||
| /* eslint-disable jsx-a11y/no-static-element-interactions */ | ||
| /* eslint-disable jsx-a11y/click-events-have-key-events */ | ||
| /* eslint-disable jsx-a11y/anchor-is-valid */ | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider refactoring to remove eslint-disable directives.
Multiple eslint-disable directives indicate code quality concerns:
max-linessuggests the component exceeds recommended size limits- Accessibility disables (
jsx-a11y/*) may impact keyboard navigation and screen reader support
Per coding guidelines, components should be small, focused, and accessible. Consider splitting this component and addressing the accessibility issues.
🤖 Prompt for AI Agents
In src/components/Verses/BookmarkedVersesList.tsx lines 1-5, the file currently
disables multiple ESLint rules (max-lines and jsx-a11y rules); refactor to
remove these disables by splitting the large component into smaller focused
subcomponents (e.g., header, list item, controls) so each file stays under
max-lines, restore the jsx-a11y rules and replace non-accessible elements with
semantic equivalents (use <button> or <a href> instead of plain divs/anchors
without href), add proper keyboard handlers (onKeyDown) and tabIndex where
needed, and include relevant ARIA attributes/roles so keyboard and screen-reader
interaction is preserved; run ESLint and adjust minor behavior into the
extracted components until all disabled rules can be removed.
| const mushafId = getMushafId(quranReaderStyles.quranFont, quranReaderStyles.mushafLines).mushaf; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider memoizing mushafId for performance.
The mushafId is recomputed on every render. While the performance impact is minimal, wrapping it in useMemo with quranReaderStyles as a dependency would follow best practices.
+const mushafId = useMemo(
+ () => getMushafId(quranReaderStyles.quranFont, quranReaderStyles.mushafLines).mushaf,
+ [quranReaderStyles.quranFont, quranReaderStyles.mushafLines]
+);
-const mushafId = getMushafId(quranReaderStyles.quranFont, quranReaderStyles.mushafLines).mushaf;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/Verses/BookmarkedVersesList.tsx around lines 46 to 47,
mushafId is recomputed on every render; wrap the call to getMushafId(...) in a
useMemo, using quranReaderStyles (or specific properties used) as the dependency
array so mushafId is only recalculated when styles change, and ensure you import
useMemo from React if not already.
PR ReviewHey! Thanks for working on this bug fix. I've reviewed the changes and found some issues that need addressing before we can merge. The good news is they're straightforward to fix! 👍 🚨 Critical Issue:
|
Testing ChecklistThis is the manual testing checklist to make there are no bugs or regressions, those will be covered with end to end testing using playwright in the future In shaa Allah, but for the time being we need to test them manually. ✅ Quick Smoke TestsBasic Bookmark Deletion
Collection Deletion
🔍 Critical Tests (Must Pass)Test 1: Multi-Tab Sync (CRITICAL!)
Current Status: ❌ Fails with Test 2: Multi-Collection Scenario
Test 3: Full Delete vs Remove from CollectionPath A - Full Delete:
Path B - Remove from Collection:
|
|
Brother "Ahmed Hussein" handling this ticket |
Bookmark icons sync issue
Fixes: QF-3097
This PR fixes an issue where deleting a bookmark or removing an ayah from a collection did not correctly update the bookmark icon and collection checkbox on the ayah page. The ayah remained visually "bookmarked" even after deletion, causing user confusion and red error messages when attempting to re-toggle the bookmark state.
The issue was caused by stale SWR cache entries not being invalidated for deleted bookmarks and their associated collection records.
Type of change
Test plan
Checklist
Summary by CodeRabbit