feat: improve library dashboard state management#15
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughCentralized client-side library dashboard state added via a new Changes
Sequence DiagramsequenceDiagram
participant User
participant IsbnTab as IsbnLookupTab
participant Scanner as useIsbnScanner
participant API as /api/books
participant State as useLibraryDashboardState
participant Dashboard as library/index.vue
User->>IsbnTab: Click Add Book
IsbnTab->>State: getLoadedPages()
State-->>IsbnTab: loadedPages
IsbnTab->>API: POST /api/books (new book)
API-->>IsbnTab: LibraryBook
IsbnTab->>State: addBook(addedBook)
State->>State: insert/update, adjust pagination
State-->>IsbnTab: ack
IsbnTab->>State: markNeedsSync(loadedPages)
State->>State: set shouldSync & syncTargetPages
State-->>IsbnTab: ack
alt bulk scan flow
User->>Scanner: Add selected ISBNs
Scanner->>API: POST /api/books (bulk)
API-->>Scanner: added list
Scanner->>State: markNeedsSync(getLoadedPages())
end
Dashboard->>State: observe shouldSync
State-->>Dashboard: syncTargetPages
Dashboard->>API: GET pages 1..N
API-->>Dashboard: page data
Dashboard->>State: clearNeedsSync()
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/pages/library/index.vue (1)
64-83: Consider adding error handling for the sync operation.The
syncAndRestoreasync function is invoked withvoid(fire-and-forget), but ifsyncLoadedPagesthrows (e.g., network failure duringrefresh()), the error will be silently swallowed. Consider wrapping in try/catch to show a toast notification on failure.🛠️ Suggested improvement
onMounted(() => { const syncAndRestore = async () => { - if (shouldSync.value) { - await syncLoadedPages(syncTargetPages.value) - - clearNeedsSync() + try { + if (shouldSync.value) { + await syncLoadedPages(syncTargetPages.value) + clearNeedsSync() + } + } catch (err: unknown) { + const message = err instanceof Error ? err.message : 'Failed to sync library' + toast.add({ + title: 'Sync failed', + description: message, + color: 'error' + }) + clearNeedsSync() } if (!shouldRestoreScroll.value) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/pages/library/index.vue` around lines 64 - 83, Wrap the body of the onMounted closure's async function syncAndRestore in a try/catch: call syncLoadedPages(syncTargetPages.value) inside try, and in catch log/notify the error (e.g., show a user toast or call a notify/toast helper) so network or refresh failures aren’t silently swallowed; ensure finally still performs the scroll restore logic (the nextTick/requestAnimationFrame/window.scrollTo and setting shouldRestoreScroll.value = false) and keep existing clearNeedsSync() behavior when the sync succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/pages/library/index.vue`:
- Around line 64-83: Wrap the body of the onMounted closure's async function
syncAndRestore in a try/catch: call syncLoadedPages(syncTargetPages.value)
inside try, and in catch log/notify the error (e.g., show a user toast or call a
notify/toast helper) so network or refresh failures aren’t silently swallowed;
ensure finally still performs the scroll restore logic (the
nextTick/requestAnimationFrame/window.scrollTo and setting
shouldRestoreScroll.value = false) and keep existing clearNeedsSync() behavior
when the sync succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8ce896d3-26b1-47ad-b0b5-bde6f994c125
📒 Files selected for processing (7)
app/components/add/IsbnLookupTab.vueapp/composables/useIsbnScanner.tsapp/composables/useLibraryDashboardState.tsapp/pages/library/[id].vueapp/pages/library/index.vuepackage.jsontest/unit/library-dashboard-state.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/pages/library/index.vue`:
- Around line 67-70: Snapshot the current dashboard state (e.g., allBooks and
page.value) at the start of syncLoadedPages() and wrap the existing sync logic
in a try/catch; on error restore the snapshot (restore allBooks and page.value
to previous values) before rethrowing so UI isn't left truncated if a later page
fails. Update syncLoadedPages() to perform the snapshot/restore and ensure any
callers (the mount-sync path that calls refresh() and the batch-delete sync
path) continue to delegate to syncLoadedPages() so both paths get the rollback
behavior.
- Around line 98-104: The route-leave handler onBeforeRouteLeave incorrectly
treats the Add Book path as a detail route because isOpeningBookDetails uses
/^\/library\/[^/]+$/. Update the check in onBeforeRouteLeave (the
isOpeningBookDetails logic) to exclude the literal "add" (e.g., use
/^\/library\/(?!add)[^/]+$/ or explicitly ensure to.path !== '/library/add') so
that navigating to /library/add does not set scrollY/shouldRestoreScroll; keep
the rest of the logic (setting scrollY.value and shouldRestoreScroll.value)
unchanged for true detail routes.
- Around line 41-47: useFetch is currently auto-watching the reactive query
(computed using page.value/pageSize.value) which causes duplicate requests when
you also call refresh() in syncLoadedPages; update the useFetch calls (the ones
that assign { data, refresh, status }) to disable auto-watch (e.g., pass watch:
false) so pagination changes are only fetched when you explicitly call
refresh(), and ensure syncLoadedPages continues to call refresh() to load pages;
apply the same change to the other useFetch at lines 159-168 and keep the query
computed as-is but rely on manual refresh to avoid out-of-order/double requests
for allBooks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de5da207-bc39-45ab-84b8-865504b4df99
📒 Files selected for processing (1)
app/pages/library/index.vue
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
1 participant
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 by CodeRabbit
New Features
Style
Tests
Chores