From a69c897d42a732191acd5c365a2e2b06f345792a Mon Sep 17 00:00:00 2001 From: Mason Fox Date: Sun, 15 Mar 2026 20:40:12 -0400 Subject: [PATCH 1/3] fix: address PR #401 review suggestions Implements 4 code quality improvements from PR review feedback: 1. Remove unused import in usePageTitle.ts - Removed unused useRef import that would fail lint checks 2. Fix null check bug in useShelfBooks.ts (critical) - Changed '!shelfId' to 'shelfId === null' to properly handle shelf ID 0 - Prevents incorrectly skipping shelf with ID 0 3. Add targeted query invalidations for journal/settings - Added specific invalidation keys for /journal and /settings routes - Prevents expensive fallback that invalidates all queries - Improves pull-to-refresh performance on these pages 4. Use query key factory consistently in useLibraryData.ts - Changed from hardcoded 'library-books' to queryKeys.library.books() - Prevents query key drift between invalidation and query construction - Ensures refresh and invalidation stay coupled to factory All fixes verified with test suite (4006/4007 tests passing, 1 pre-existing failure unrelated to changes). Resolves review feedback from: https://github.com/masonfox/tome/pull/401#pullrequestreview-3933453275 --- hooks/useLibraryData.ts | 3 ++- hooks/usePullToRefreshLogic.ts | 2 ++ hooks/useShelfBooks.ts | 2 +- lib/hooks/usePageTitle.ts | 2 +- 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/hooks/useLibraryData.ts b/hooks/useLibraryData.ts index 4944eaf5..7633f08e 100644 --- a/hooks/useLibraryData.ts +++ b/hooks/useLibraryData.ts @@ -20,8 +20,9 @@ export function useLibraryData(initialFilters?: Partial) { const service = useMemo(() => libraryService, []); // Create a stable query key based on filter values (excluding pagination.skip) + // Use queryKeys.library.books() as base to ensure consistency with refresh/invalidation const queryKey = useMemo(() => [ - 'library-books', + ...queryKeys.library.books(), filters.status, filters.search, filters.tags, diff --git a/hooks/usePullToRefreshLogic.ts b/hooks/usePullToRefreshLogic.ts index 1c8f690e..36b586b0 100644 --- a/hooks/usePullToRefreshLogic.ts +++ b/hooks/usePullToRefreshLogic.ts @@ -44,6 +44,8 @@ export function usePullToRefreshLogic() { "/streak": queryKeys.streak.base(), "/shelves": queryKeys.shelf.base(), "/tags": queryKeys.tags.base(), + "/journal": ["journal-entries", "journal-archive"], // Invalidate both entries and archive + "/settings": ["user-preferences"], // Settings page data }; const queryKey = invalidationsByPath[pathname]; diff --git a/hooks/useShelfBooks.ts b/hooks/useShelfBooks.ts index b7171585..c6b7e8f0 100644 --- a/hooks/useShelfBooks.ts +++ b/hooks/useShelfBooks.ts @@ -26,7 +26,7 @@ export function useShelfBooks( ? queryKeys.shelf.detail(shelfId, { orderBy, direction }) : ['shelf-empty'], queryFn: async () => { - if (!shelfId) return null; + if (shelfId === null) return null; const shelf = await shelfApi.get(shelfId, { withBooks: true, diff --git a/lib/hooks/usePageTitle.ts b/lib/hooks/usePageTitle.ts index 463dd394..7c66a0fb 100644 --- a/lib/hooks/usePageTitle.ts +++ b/lib/hooks/usePageTitle.ts @@ -1,4 +1,4 @@ -import { useLayoutEffect, useRef, useEffect } from "react"; +import { useLayoutEffect, useEffect } from "react"; /** * Custom hook to set the page title From 8ccd0b945d297f510cb7bfa3c58fd95cb930295a Mon Sep 17 00:00:00 2001 From: Mason Fox Date: Thu, 19 Mar 2026 09:12:31 -0400 Subject: [PATCH 2/3] fix: correct journal invalidation logic in pull-to-refresh The previous implementation incorrectly passed an array ['journal-entries', 'journal-archive'] as a single query key to invalidateQueries. This would not match the actual query keys used in the journal page (['journal-entries', timezone] and ['journal-archive', timezone]). Fixed by using explicit parallel invalidation with Promise.all to properly invalidate both journal entries and archive queries using prefix matching. Also removed unnecessary settings page entry since that page doesn't use React Query. --- hooks/usePullToRefreshLogic.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/hooks/usePullToRefreshLogic.ts b/hooks/usePullToRefreshLogic.ts index 36b586b0..5d2f3330 100644 --- a/hooks/usePullToRefreshLogic.ts +++ b/hooks/usePullToRefreshLogic.ts @@ -44,14 +44,18 @@ export function usePullToRefreshLogic() { "/streak": queryKeys.streak.base(), "/shelves": queryKeys.shelf.base(), "/tags": queryKeys.tags.base(), - "/journal": ["journal-entries", "journal-archive"], // Invalidate both entries and archive - "/settings": ["user-preferences"], // Settings page data }; const queryKey = invalidationsByPath[pathname]; if (queryKey) { await queryClient.invalidateQueries({ queryKey }); + } else if (pathname === "/journal") { + // Special case: Invalidate both journal entries and archive using prefix matching + await Promise.all([ + queryClient.invalidateQueries({ queryKey: ['journal-entries'] }), + queryClient.invalidateQueries({ queryKey: ['journal-archive'] }), + ]); } else { // Fallback: invalidate all queries if we don't have specific keys await queryClient.invalidateQueries(); From 0844ad1d9b926dd88d35ccc89ae415792da4db16 Mon Sep 17 00:00:00 2001 From: Mason Fox Date: Thu, 19 Mar 2026 09:22:40 -0400 Subject: [PATCH 3/3] fix: address GitHub Copilot review feedback Implemented all suggestions from Copilot review: 1. Complete shelfId=0 bug fix in useShelfBooks.ts - Replaced all 7 instances of !shelfId with shelfId === null - Fixes: addBooksMutation, removeBookMutation, removeBooksMutation, reorderBooksMutation, moveBooksMutation, moveToTopMutation, moveToBottomMutation - Now shelf ID 0 is properly supported across all operations 2. Add query key factory base methods - Added queryKeys.journal.entriesBase() returning ['journal-entries'] - Added queryKeys.journal.archiveBase() returning ['journal-archive'] - Prevents key drift between query definition and invalidation - Aligns with Query Key Factory Pattern from patterns.md 3. Use factory methods in usePullToRefreshLogic.ts - Replaced hardcoded strings with queryKeys.journal.entriesBase() - Replaced hardcoded strings with queryKeys.journal.archiveBase() - Ensures consistency with centralized query key factory All 4007 tests pass. --- hooks/usePullToRefreshLogic.ts | 4 ++-- hooks/useShelfBooks.ts | 14 +++++++------- lib/query-keys.ts | 6 ++++++ 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/hooks/usePullToRefreshLogic.ts b/hooks/usePullToRefreshLogic.ts index 5d2f3330..d0860772 100644 --- a/hooks/usePullToRefreshLogic.ts +++ b/hooks/usePullToRefreshLogic.ts @@ -53,8 +53,8 @@ export function usePullToRefreshLogic() { } else if (pathname === "/journal") { // Special case: Invalidate both journal entries and archive using prefix matching await Promise.all([ - queryClient.invalidateQueries({ queryKey: ['journal-entries'] }), - queryClient.invalidateQueries({ queryKey: ['journal-archive'] }), + queryClient.invalidateQueries({ queryKey: queryKeys.journal.entriesBase() }), + queryClient.invalidateQueries({ queryKey: queryKeys.journal.archiveBase() }), ]); } else { // Fallback: invalidate all queries if we don't have specific keys diff --git a/hooks/useShelfBooks.ts b/hooks/useShelfBooks.ts index c6b7e8f0..96616433 100644 --- a/hooks/useShelfBooks.ts +++ b/hooks/useShelfBooks.ts @@ -53,7 +53,7 @@ export function useShelfBooks( */ const addBooksMutation = useMutation({ mutationFn: async (bookIds: number[]) => { - if (!shelfId || bookIds.length === 0) { + if (shelfId === null || bookIds.length === 0) { throw new Error("No shelf ID or book IDs provided"); } return shelfApi.addBooks(shelfId, { bookIds }); @@ -73,7 +73,7 @@ export function useShelfBooks( */ const removeBookMutation = useMutation({ mutationFn: async (bookId: number) => { - if (!shelfId) { + if (shelfId === null) { throw new Error("No shelf ID provided"); } return shelfApi.removeBook(shelfId, bookId); @@ -127,7 +127,7 @@ export function useShelfBooks( */ const removeBooksMutation = useMutation({ mutationFn: async (bookIds: number[]) => { - if (!shelfId || bookIds.length === 0) { + if (shelfId === null || bookIds.length === 0) { throw new Error("No shelf ID or book IDs provided"); } return shelfApi.removeBooks(shelfId, { bookIds }); @@ -182,7 +182,7 @@ export function useShelfBooks( */ const reorderBooksMutation = useMutation({ mutationFn: async (bookIds: number[]) => { - if (!shelfId) { + if (shelfId === null) { throw new Error("No shelf ID provided"); } return shelfApi.reorderBooks(shelfId, { bookIds }); @@ -245,7 +245,7 @@ export function useShelfBooks( bookIds: number[]; targetShelfName?: string; }) => { - if (!shelfId || bookIds.length === 0) { + if (shelfId === null || bookIds.length === 0) { throw new Error("No shelf ID or book IDs provided"); } return shelfApi.moveBooks(shelfId, targetShelfId, { bookIds }); @@ -329,7 +329,7 @@ export function useShelfBooks( */ const moveToTopMutation = useMutation({ mutationFn: async (bookId: number) => { - if (!shelfId) { + if (shelfId === null) { throw new Error("No shelf ID provided"); } @@ -399,7 +399,7 @@ export function useShelfBooks( const moveToBottomMutation = useMutation({ mutationFn: async (bookId: number) => { - if (!shelfId) { + if (shelfId === null) { throw new Error("No shelf ID provided"); } diff --git a/lib/query-keys.ts b/lib/query-keys.ts index e74e8332..658ae618 100644 --- a/lib/query-keys.ts +++ b/lib/query-keys.ts @@ -197,6 +197,12 @@ export const queryKeys = { // JOURNAL // ============================================================================ journal: { + /** Base key for journal entries (prefix matching): ['journal-entries'] */ + entriesBase: () => ['journal-entries'] as const, + + /** Base key for journal archive (prefix matching): ['journal-archive'] */ + archiveBase: () => ['journal-archive'] as const, + /** Journal entries for timezone: ['journal-entries', timezone] */ entries: (timezone: string) => ['journal-entries', timezone] as const,