Fix: Address PR #401 review suggestions#402
Merged
Conversation
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: #401 (review)
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (70.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## develop #402 +/- ##
===========================================
- Coverage 78.64% 78.62% -0.03%
===========================================
Files 167 167
Lines 7538 7540 +2
Branches 1846 1846
===========================================
Hits 5928 5928
- Misses 1125 1127 +2
Partials 485 485
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies follow-up fixes from PR #401 review feedback, mainly around React Query key consistency, targeted cache invalidation, and a shelf ID edge case.
Changes:
- Removes an unused React import in
usePageTitle. - Adjusts
useShelfBooksto treatshelfId = 0as valid (null-only guard). - Adds a
/journalspecial-case in pull-to-refresh to avoid full-cache invalidation. - Aligns
useLibraryData’s query key construction with the centralizedqueryKeysfactory.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/hooks/usePageTitle.ts | Removes unused useRef import. |
| hooks/useShelfBooks.ts | Updates null-checking for shelf fetching logic. |
| hooks/usePullToRefreshLogic.ts | Adds targeted invalidation for /journal instead of invalidating all queries. |
| hooks/useLibraryData.ts | Uses queryKeys.library.books() as the base for the infinite query key. |
You can also share your feedback on Copilot code review. Take the survey.
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses review feedback from PR #401 by tightening query key consistency, fixing a shelf ID edge case, and reducing unnecessary React Query invalidations to improve refresh performance.
Changes:
- Fixed
shelfId === 0being treated as “missing” by switching falsy checks to strictnullchecks inuseShelfBooks. - Added journal base query keys and used them to invalidate journal caches via prefix matching on pull-to-refresh.
- Updated library query key construction to consistently use the centralized query key factory.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| lib/query-keys.ts | Adds base journal keys for prefix invalidation (entriesBase, archiveBase). |
| lib/hooks/usePageTitle.ts | Removes an unused React import. |
| hooks/useShelfBooks.ts | Fixes shelf ID 0 handling by using strict null checks. |
| hooks/usePullToRefreshLogic.ts | Adds targeted invalidation for /journal via base keys (but /settings handling appears missing vs PR description). |
| hooks/useLibraryData.ts | Uses queryKeys.library.books() as the base for the infinite query key to prevent drift. |
Comments suppressed due to low confidence (1)
hooks/usePullToRefreshLogic.ts:62
- PR description says pull-to-refresh adds targeted invalidation for
/settings(e.g.['user-preferences']), but/settingsis still not mapped here, so it will fall through toinvalidateQueries()and invalidate the entire cache. Either add an explicit/settingsinvalidation key (and define it inlib/query-keys.ts) or update the PR description to match the actual behavior.
// Map pages to their query key invalidation
const invalidationsByPath: Record<string, readonly unknown[]> = {
"/": queryKeys.dashboard.all(),
"/library": queryKeys.library.books(),
"/read-next": queryKeys.readNext.base(),
"/series": queryKeys.series.all(),
"/stats": queryKeys.stats.all(),
"/goals": queryKeys.goals.base(),
"/streak": queryKeys.streak.base(),
"/shelves": queryKeys.shelf.base(),
"/tags": queryKeys.tags.base(),
};
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: queryKeys.journal.entriesBase() }),
queryClient.invalidateQueries({ queryKey: queryKeys.journal.archiveBase() }),
]);
} else {
// Fallback: invalidate all queries if we don't have specific keys
await queryClient.invalidateQueries();
}
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
199
to
+205
| 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, | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses 4 code quality improvements identified in PR #401 review feedback.
Changes
1. Remove unused import (hooks/usePageTitle.ts)
useRefwas imported but never used2. Fix shelf ID 0 handling bug (hooks/useShelfBooks.ts) 🐛
if (!shelfId)treats0as falsy, incorrectly skipping shelf ID 0if (shelfId === null)for strict null checkingshelfIdis typed asnumber | null, so this aligns with the type system3. Add targeted query invalidations (hooks/usePullToRefreshLogic.ts)
/journaland/settingsfell back to invalidating all queries/journal→["journal-entries", "journal-archive"]/settings→["user-preferences"]4. Use query key factory consistently (hooks/useLibraryData.ts)
'library-books'string while refresh usedqueryKeys.library.books()[...queryKeys.library.books(), ...]for consistent factory usageTesting
streaks-coverage.test.ts(unrelated, already failing on develop)Files Changed
lib/hooks/usePageTitle.ts- Removed unused importhooks/useShelfBooks.ts- Fixed null check for shelf ID 0hooks/usePullToRefreshLogic.ts- Added targeted invalidationshooks/useLibraryData.ts- Use query key factory consistentlyReview Feedback Source
PR #401 Review Comment Thread