feat(bookmarks): implement bookmarks broadcast functionality across components#3082
feat(bookmarks): implement bookmarks broadcast functionality across components#3082rrrokhtar wants to merge 2 commits intoproductionfrom
Conversation
…omponents Signed-off-by: Mokhtar <rrrokhtar@gmail.com>
… various components Signed-off-by: Mokhtar <rrrokhtar@gmail.com>
There was a problem hiding this comment.
Pull request overview
Implements a cross-tab “bookmarks/collections sync” mechanism so bookmark-related mutations (collections CRUD, adding/removing verses from collections, reading bookmark changes, bulk deletes) can invalidate/update relevant SWR caches and guest reading-bookmark Redux state across open tabs via a global listener.
Changes:
- Added
useBookmarksBroadcast(broadcast + listener) usingBroadcastChannelwith alocalStoragetransport fallback, and SWR cache invalidation/update logic driven by payload flags. - Integrated broadcasting calls into collection CRUD and bookmark mutation flows (Save Bookmark modal, reading bookmark, collection detail delete, bulk delete).
- Added/updated unit tests to validate broadcast payloads and listener-driven cache invalidation behavior.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hooks/useCollections.ts | Broadcast sync events after collection create/update/delete. |
| src/hooks/useCollections.test.ts | Adds tests asserting broadcasts fire (or don’t) for collection mutations. |
| src/hooks/useBookmarksBroadcast.ts | New broadcast/listener implementation with SWR cache invalidation and guest reading-bookmark sync. |
| src/hooks/useBookmarksBroadcast.test.tsx | Tests transport paths (BroadcastChannel + storage) and listener cache invalidation/deduping. |
| src/components/Verse/SaveBookmarkModal/useSaveBookmarkModal.ts | Broadcast after creating a collection + adding a verse to it. |
| src/components/Verse/SaveBookmarkModal/useSaveBookmarkModal.test.tsx | Adds coverage for the create-collection broadcast. |
| src/components/Verse/SaveBookmarkModal/ReadingBookmarkSection/useReadingBookmark.ts | Broadcast reading bookmark changes (logged-in cache update vs guest Redux payload). |
| src/components/Verse/SaveBookmarkModal/ReadingBookmarkSection/useReadingBookmark.test.tsx | Adds assertions for reading-bookmark broadcasting in guest/logged-in flows. |
| src/components/Verse/SaveBookmarkModal/Collections/hooks/useCollectionToggle.ts | Broadcast after adding/removing a verse to/from a collection. |
| src/components/Verse/SaveBookmarkModal/Collections/hooks/useCollectionToggle.test.ts | Adds tests for collection-toggle broadcasts. |
| src/components/MyQuran/CollectionDetailView/hooks/useCollectionDetailViewController.ts | Broadcast after deleting an item from a collection detail view (incl. affected surah). |
| src/components/MyQuran/CollectionDetailView/hooks/useCollectionDetailViewController.test.ts | Adds broadcast assertions for item deletion success/failure. |
| src/components/MyQuran/CollectionDetailView/hooks/useCollectionBulkActions.ts | Broadcast after bulk delete with derived affected surah numbers. |
| src/components/MyQuran/CollectionDetailView/hooks/useCollectionBulkActions.test.ts | Adds broadcast assertions for bulk delete flow. |
| src/components/GlobalListeners.tsx | Mounts the broadcast listener globally so cross-tab events are handled app-wide. |
| const broadcastCollectionUpdated = useCallback((collectionId: string) => { | ||
| broadcastBookmarksUpdate({ | ||
| touchesCollectionsList: true, | ||
| touchesCollectionDetail: true, | ||
| affectedCollectionIds: [collectionId], |
There was a problem hiding this comment.
broadcastCollectionUpdated duplicates the same payload as broadcastCollectionCreated (touchesCollectionsList/touchesCollectionDetail/affectedCollectionIds). Consider extracting a single helper (e.g., broadcastCollectionUpserted) to reduce duplication and keep future payload tweaks consistent.
| cache: Cache, | ||
| globalMutate: ScopedMutator, | ||
| matcher: (key: string) => boolean, | ||
| data?: T | ((currentData: T | undefined) => T), | ||
| options: { revalidate?: boolean } = { revalidate: true }, | ||
| ) => { | ||
| const keys = getStringCacheKeys(cache); | ||
| const matchedKeys = keys.filter(matcher); | ||
| matchedKeys.forEach((key) => { | ||
| globalMutate(key, data as any, options); | ||
| }); | ||
| }; | ||
|
|
||
| const revalidateCollectionsListCaches = (cache: Cache, globalMutate: ScopedMutator) => | ||
| mutateMatchingKeys( | ||
| cache, | ||
| globalMutate, | ||
| (key) => key.includes(BOOKMARK_CACHE_PATHS.COLLECTIONS), | ||
| undefined, | ||
| { revalidate: true }, | ||
| ); | ||
|
|
||
| const revalidateBookmarksListCaches = (cache: Cache, globalMutate: ScopedMutator) => | ||
| mutateMatchingKeys( | ||
| cache, | ||
| globalMutate, | ||
| (key) => key.includes(BOOKMARK_CACHE_PATHS.BOOKMARKS_LIST), | ||
| undefined, | ||
| { revalidate: true }, | ||
| ); | ||
|
|
||
| const revalidateBookmarkCollectionsCaches = (cache: Cache, globalMutate: ScopedMutator) => { | ||
| mutateMatchingKeys( | ||
| cache, | ||
| globalMutate, | ||
| (key) => key.includes(BOOKMARK_CACHE_PATHS.BOOKMARK_COLLECTIONS), | ||
| undefined, | ||
| { revalidate: true }, | ||
| ); | ||
| mutateMatchingKeys( | ||
| cache, |
There was a problem hiding this comment.
mutateMatchingKeys recomputes and filters the full SWR cache key list on every call. Since applyPayloadSync may call several of these revalidation helpers per event, consider computing const keys = getStringCacheKeys(cache) once per incoming message and reusing it to reduce repeated work.
| cache: Cache, | |
| globalMutate: ScopedMutator, | |
| matcher: (key: string) => boolean, | |
| data?: T | ((currentData: T | undefined) => T), | |
| options: { revalidate?: boolean } = { revalidate: true }, | |
| ) => { | |
| const keys = getStringCacheKeys(cache); | |
| const matchedKeys = keys.filter(matcher); | |
| matchedKeys.forEach((key) => { | |
| globalMutate(key, data as any, options); | |
| }); | |
| }; | |
| const revalidateCollectionsListCaches = (cache: Cache, globalMutate: ScopedMutator) => | |
| mutateMatchingKeys( | |
| cache, | |
| globalMutate, | |
| (key) => key.includes(BOOKMARK_CACHE_PATHS.COLLECTIONS), | |
| undefined, | |
| { revalidate: true }, | |
| ); | |
| const revalidateBookmarksListCaches = (cache: Cache, globalMutate: ScopedMutator) => | |
| mutateMatchingKeys( | |
| cache, | |
| globalMutate, | |
| (key) => key.includes(BOOKMARK_CACHE_PATHS.BOOKMARKS_LIST), | |
| undefined, | |
| { revalidate: true }, | |
| ); | |
| const revalidateBookmarkCollectionsCaches = (cache: Cache, globalMutate: ScopedMutator) => { | |
| mutateMatchingKeys( | |
| cache, | |
| globalMutate, | |
| (key) => key.includes(BOOKMARK_CACHE_PATHS.BOOKMARK_COLLECTIONS), | |
| undefined, | |
| { revalidate: true }, | |
| ); | |
| mutateMatchingKeys( | |
| cache, | |
| keys: string[], | |
| globalMutate: ScopedMutator, | |
| matcher: (key: string) => boolean, | |
| data?: T | ((currentData: T | undefined) => T), | |
| options: { revalidate?: boolean } = { revalidate: true }, | |
| ) => { | |
| const matchedKeys = keys.filter(matcher); | |
| matchedKeys.forEach((key) => { | |
| globalMutate(key, data as any, options); | |
| }); | |
| }; | |
| const revalidateCollectionsListCaches = (cache: Cache, globalMutate: ScopedMutator) => { | |
| const keys = getStringCacheKeys(cache); | |
| mutateMatchingKeys( | |
| keys, | |
| globalMutate, | |
| (key) => key.includes(BOOKMARK_CACHE_PATHS.COLLECTIONS), | |
| undefined, | |
| { revalidate: true }, | |
| ); | |
| } | |
| const revalidateBookmarksListCaches = (cache: Cache, globalMutate: ScopedMutator) => { | |
| const keys = getStringCacheKeys(cache); | |
| mutateMatchingKeys( | |
| keys, | |
| globalMutate, | |
| (key) => key.includes(BOOKMARK_CACHE_PATHS.BOOKMARKS_LIST), | |
| undefined, | |
| { revalidate: true }, | |
| ); | |
| } | |
| const revalidateBookmarkCollectionsCaches = (cache: Cache, globalMutate: ScopedMutator) => { | |
| const keys = getStringCacheKeys(cache); | |
| mutateMatchingKeys( | |
| keys, | |
| globalMutate, | |
| (key) => key.includes(BOOKMARK_CACHE_PATHS.BOOKMARK_COLLECTIONS), | |
| undefined, | |
| { revalidate: true }, | |
| ); | |
| mutateMatchingKeys( | |
| keys, |
| updates.forEach(({ mushafId, surahNumber, verseKey, bookmark }) => { | ||
| const targetKey = SURAH_BOOKMARKS_KEY(mushafId, surahNumber); | ||
| const existingKeys = new Set(getStringCacheKeys(cache)); |
There was a problem hiding this comment.
applySurahBookmarkUpdates rebuilds existingKeys (and traverses the full cache) for every update entry. You can compute the cache key set once outside the forEach and reuse it for all updates to avoid redundant work when multiple verses are updated in a single payload.
| updates.forEach(({ mushafId, surahNumber, verseKey, bookmark }) => { | |
| const targetKey = SURAH_BOOKMARKS_KEY(mushafId, surahNumber); | |
| const existingKeys = new Set(getStringCacheKeys(cache)); | |
| const existingKeys = new Set(getStringCacheKeys(cache)); | |
| updates.forEach(({ mushafId, surahNumber, verseKey, bookmark }) => { | |
| const targetKey = SURAH_BOOKMARKS_KEY(mushafId, surahNumber); |
| try { | ||
| window.localStorage.setItem(STORAGE_KEY, JSON.stringify(message)); | ||
| } catch { | ||
| // Swallow localStorage exceptions (quota/private mode) and keep BroadcastChannel path. | ||
| } |
There was a problem hiding this comment.
postWithStorageFallback always writes the message to localStorage even when BroadcastChannel is available. This adds synchronous storage churn and also causes duplicate cross-tab deliveries (BroadcastChannel + storage event) that then rely on eventId deduping; consider only using the storage path when BroadcastChannel is unavailable (or when posting fails).
| touchesBookmarkCollections: true, | ||
| touchesCollectionDetail: true, | ||
| affectedCollectionIds: ['123'], | ||
| affectedSurahNumbers: ['1'], | ||
| }), |
There was a problem hiding this comment.
In this test, Bookmark.key is a number (chapter/surah number), but the assertion expects affectedSurahNumbers: ['1'] (string). This can mask type issues and diverges from production behavior; use numeric keys in the fixtures and expect [1] here.
| return Array.from( | ||
| new Set( | ||
| bookmarks | ||
| .filter((bookmark) => deletedIds.includes(bookmark.id)) |
There was a problem hiding this comment.
getAffectedSurahNumbers does an includes lookup on deletedIds for every bookmark, which is O(n*m). Converting deletedIds to a Set (or iterating deletedIds and indexing bookmarks) would avoid repeated linear scans for large bulk deletes.
| return Array.from( | |
| new Set( | |
| bookmarks | |
| .filter((bookmark) => deletedIds.includes(bookmark.id)) | |
| const deletedIdsSet = new Set(deletedIds); | |
| return Array.from( | |
| new Set( | |
| bookmarks | |
| .filter((bookmark) => deletedIdsSet.has(bookmark.id)) |
| window.dispatchEvent( | ||
| new StorageEvent('storage', { | ||
| key: 'qdc:bookmarks-sync', | ||
| newValue: JSON.stringify(storageMessage), | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Superfluous arguments passed to function StorageEvent.
| window.dispatchEvent( | |
| new StorageEvent('storage', { | |
| key: 'qdc:bookmarks-sync', | |
| newValue: JSON.stringify(storageMessage), | |
| }), | |
| ); | |
| const storageEvent = new StorageEvent('storage'); | |
| Object.assign(storageEvent, { | |
| key: 'qdc:bookmarks-sync', | |
| newValue: JSON.stringify(storageMessage), | |
| } as StorageEvent); | |
| window.dispatchEvent(storageEvent); |
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
@rrrokhtar I've opened a new pull request, #3128, to work on those changes. Once the pull request is ready, I'll request review from you. |
Closes: QF-4694