Skip to content

[QF-3097] bookmark icons sync issue #2582

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
al-imam wants to merge 2 commits into from
Closed

[QF-3097] bookmark icons sync issue #2582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d23a093
fix: bookmark icons sync issue invalidate bookmark & collection cache
al-imam Nov 10, 2025
2fa93e4
refactor: simplify mushafId retrieval in BookmarkedVersesList
al-imam Nov 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions src/components/Collection/CollectionDetailContainer/CollectionDetailContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/* eslint-disable max-lines */

Comment on lines +1 to +2
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

import classNames from 'classnames';
import { useRouter } from 'next/router';
import useTranslation from 'next-translate/useTranslation';
import { shallowEqual, useSelector } from 'react-redux';
import { useSWRConfig } from 'swr';
import useSWRInfinite from 'swr/infinite';

import layoutStyles from '../../../pages/index.module.scss';
Expand All @@ -12,6 +16,10 @@ import Spinner, { SpinnerSize } from '@/dls/Spinner/Spinner';
import { ToastStatus, useToast } from '@/dls/Toast/Toast';
import ArrowLeft from '@/icons/west.svg';
import Error from '@/pages/_error';
import { selectQuranReaderStyles } from '@/redux/slices/QuranReader/styles';
import BookmarkType from '@/types/BookmarkType';
import { getMushafId } from '@/utils/api';
import { makeBookmarkCollectionsUrl, makeBookmarkUrl } from '@/utils/auth/apiPaths';
import { logButtonClick } from '@/utils/eventLogger';
import { getLanguageAlternates } from '@/utils/locale';
import {
Expand Down Expand Up @@ -50,6 +58,10 @@ const CollectionDetailContainer = ({
const collectionId = router.query.collectionId as string;
const toast = useToast();

const quranReaderStyles = useSelector(selectQuranReaderStyles, shallowEqual);

const { cache } = useSWRConfig();

const { data, size, setSize, mutate, isValidating, error } =
useSWRInfinite<GetBookmarkCollectionsIdResponse>(getSWRKey, privateFetcher);

Expand All @@ -65,8 +77,21 @@ const CollectionDetailContainer = ({
);
}

const onUpdated = () => {
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),
);
}
};
Comment on lines +80 to 95
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

LGTM: Cache invalidation correctly implemented.

The cache invalidation logic properly:

  1. Revalidates data with mutate()
  2. Computes mushafId from current styles
  3. 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.

Suggested change
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.


const lastPageData = data[data.length - 1];
Expand All @@ -90,7 +115,7 @@ const CollectionDetailContainer = ({
if (shouldDeleteBookmark) {
deleteBookmarkById(bookmarkId)
.then(() => {
onUpdated();
onUpdated(bookmarkId);
})
.catch(() => {
toast(t('common:error.general'), {
Expand All @@ -100,7 +125,7 @@ const CollectionDetailContainer = ({
} else {
deleteCollectionBookmarkById(collectionId, bookmarkId)
.then(() => {
onUpdated();
onUpdated(bookmarkId);
})
.catch(() => {
toast(t('common:error.general'), {
Expand Down
41 changes: 34 additions & 7 deletions src/components/Verses/BookmarkedVersesList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
/* eslint-disable max-lines */
/* 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 */

Comment on lines +1 to +5
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Consider refactoring to remove eslint-disable directives.

Multiple eslint-disable directives indicate code quality concerns:

  • max-lines suggests 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.

import React, { useMemo } from 'react';

import useTranslation from 'next-translate/useTranslation';
import { shallowEqual, useSelector, useDispatch } from 'react-redux';
import useSWR from 'swr';
import useSWR, { useSWRConfig } from 'swr';

import styles from './BookmarkedVersesList.module.scss';
import BookmarkedVersesListSkeleton from './BookmarkedVesesListSkeleton';
Expand All @@ -16,9 +17,14 @@ import Link from '@/dls/Link/Link';
import { ToastStatus, useToast } from '@/dls/Toast/Toast';
import { selectBookmarks, toggleVerseBookmark } from '@/redux/slices/QuranReader/bookmarks';
import { selectQuranReaderStyles } from '@/redux/slices/QuranReader/styles';
import BookmarkType from '@/types/BookmarkType';
import { getMushafId } from '@/utils/api';
import { deleteBookmarkById, privateFetcher } from '@/utils/auth/api';
import { makeBookmarksUrl } from '@/utils/auth/apiPaths';
import {
makeBookmarkCollectionsUrl,
makeBookmarksUrl,
makeBookmarkUrl,
} from '@/utils/auth/apiPaths';
import { isLoggedIn } from '@/utils/auth/login';
import { logButtonClick } from '@/utils/eventLogger';
import { getVerseAndChapterNumbersFromKey, makeVerseKey } from '@/utils/verse';
Expand All @@ -31,17 +37,17 @@ const BookmarkedVersesList = () => {

const quranReaderStyles = useSelector(selectQuranReaderStyles, shallowEqual);
const dispatch = useDispatch();
const { cache } = useSWRConfig();

const toast = useToast();

const bookmarkedVerses = useSelector(selectBookmarks, shallowEqual);

const mushafId = getMushafId(quranReaderStyles.quranFont, quranReaderStyles.mushafLines).mushaf;

Comment on lines +46 to +47
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

const { data, isValidating, mutate } = useSWR<Bookmark[]>(
isLoggedIn() // only fetch the data when user is loggedIn
? makeBookmarksUrl(
getMushafId(quranReaderStyles.quranFont, quranReaderStyles.mushafLines).mushaf,
BOOKMARKS_API_LIMIT,
)
? makeBookmarksUrl(mushafId, BOOKMARKS_API_LIMIT)
: null,
privateFetcher,
);
Expand Down Expand Up @@ -73,6 +79,7 @@ const BookmarkedVersesList = () => {

const onBookmarkDeleted = (verseKey: string) => {
logButtonClick('bookmarked_verses_list_delete');

if (isLoggedIn()) {
const selectedBookmark = data.find((bookmark) => {
const [chapter, verseNumber] = getVerseAndChapterNumbersFromKey(verseKey);
Expand All @@ -82,9 +89,29 @@ const BookmarkedVersesList = () => {
);
});

if (!selectedBookmark) return;

deleteBookmarkById(selectedBookmark.id)
.then(() => {
mutate();

cache.delete(
makeBookmarkUrl(
mushafId,
selectedBookmark.key,
BookmarkType.Ayah,
selectedBookmark.verseNumber,
),
);

cache.delete(
makeBookmarkCollectionsUrl(
mushafId,
selectedBookmark.key,
BookmarkType.Ayah,
selectedBookmark.verseNumber,
),
);
})
.catch(() => {
toast(t('common:error.general'), {
Expand Down