fix: reduce jank when analyzing repository cards#199
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR defers serialized state writes to idle time, avoids replacing repository lists when records are equal, narrows RepositoryCard subscriptions and adds local analyzing state with timing/logging, precomputes category filter keywords, and removes synchronous localStorage backup writes. ChangesPerformance optimization for large repository lists
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/store/useAppStore.ts (1)
69-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQueued writes can recreate state after
removeItem().
timeoutId,idleTaskId, andlatestValuelive insidesetItem's closure, soremoveItemhas no way to cancel them. Any reset/clear path that deletes the persisted snapshot while a debounced write is pending will write the old snapshot back afterward, resurrecting deleted repositories and persisted credentials.Suggested refactor
-const debouncedPersistStorage: PersistStorage<any> = { +let persistTimeoutId: ReturnType<typeof setTimeout> | null = null; +let persistIdleTaskId: number | null = null; +let latestPersistValue: StorageValue<any> | null = null; + +const debouncedPersistStorage: PersistStorage<any> = { getItem: async (name) => { const str = await indexedDBStorage.getItem(name); if (!str) return null; try { return JSON.parse(str); @@ - setItem: (() => { - let timeoutId: ReturnType<typeof setTimeout> | null = null; - let idleTaskId: number | null = null; - let latestValue: StorageValue<any> | null = null; - return (name: string, value: StorageValue<any>) => { - latestValue = value; - if (timeoutId) clearTimeout(timeoutId); - if (idleTaskId !== null) { - cancelIdleTask(idleTaskId); - idleTaskId = null; + setItem: (name: string, value: StorageValue<any>) => { + latestPersistValue = value; + if (persistTimeoutId) clearTimeout(persistTimeoutId); + if (persistIdleTaskId !== null) { + cancelIdleTask(persistIdleTaskId); + persistIdleTaskId = null; } - timeoutId = setTimeout(() => { - timeoutId = null; - idleTaskId = scheduleIdleTask(() => { + persistTimeoutId = setTimeout(() => { + persistTimeoutId = null; + persistIdleTaskId = scheduleIdleTask(() => { const startedAt = performance.now(); try { - const str = JSON.stringify(latestValue); + const str = JSON.stringify(latestPersistValue); @@ } finally { - idleTaskId = null; + persistIdleTaskId = null; } }); }, 1000); - }; - })(), + }, removeItem: (name) => { - indexedDBStorage.removeItem(name); + latestPersistValue = null; + if (persistTimeoutId) { + clearTimeout(persistTimeoutId); + persistTimeoutId = null; + } + if (persistIdleTaskId !== null) { + cancelIdleTask(persistIdleTaskId); + persistIdleTaskId = null; + } + return indexedDBStorage.removeItem(name); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/useAppStore.ts` around lines 69 - 114, The bug is that timeoutId, idleTaskId and latestValue are closed over inside setItem so removeItem cannot cancel pending debounced/idle writes and an old snapshot can be written back after removal; fix by hoisting those variables to shared scope (or to the surrounding store object) so both setItem and removeItem can access them, and update removeItem (the indexedDBStorage.removeItem caller) to clearTimeout(timeoutId), cancelIdleTask(idleTaskId) and reset idleTaskId/timeoutId/latestValue (or bump a write-version token) so the scheduled task checks that token/latestValue is still valid before calling indexedDBStorage.setItem(name, ...); ensure the scheduled closure returns early when latestValue is null or the token/version mismatches to prevent resurrecting removed state.
🧹 Nitpick comments (1)
src/services/indexedDbStorage.ts (1)
100-100: ⚡ Quick winUpdate stale comment to reflect removal of dual writes.
The comment header mentions "dual write:" but the code no longer writes to both IndexedDB and localStorage (only IndexedDB, as correctly noted in lines 103-104). This inconsistency could confuse future maintainers.
📝 Suggested fix
-/** - * IndexedDB-backed Zustand persist storage with seamless migration + dual write: - * - First read from IndexedDB - * - If empty, fall back to existing localStorage snapshot and migrate to IndexedDB - * - Writes go to IndexedDB only; localStorage is read as a one-time legacy fallback. - * Keeping large repository snapshots out of localStorage avoids synchronous main-thread stalls. - */ +/** + * IndexedDB-backed Zustand persist storage with seamless migration: + * - First read from IndexedDB + * - If empty, fall back to existing localStorage snapshot and migrate to IndexedDB + * - Writes go to IndexedDB only; localStorage is read as a one-time legacy fallback. + * Keeping large repository snapshots out of localStorage avoids synchronous main-thread stalls. + */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/indexedDbStorage.ts` at line 100, Update the top-of-file comment that currently reads "IndexedDB-backed Zustand persist storage with seamless migration + dual write:" to reflect that dual write was removed (e.g., "IndexedDB-backed Zustand persist storage with seamless migration" or "single-write to IndexedDB"). Locate the header comment above the exported storage implementation (the module-level comment that precedes the IndexedDB storage functions such as the exported storage creator/handler in this file) and edit it so it no longer references "dual write" or implies writing to both IndexedDB and localStorage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/store/useAppStore.test.ts`:
- Around line 6-37: The test imports the real useAppStore singleton (via
vi.importActual) while the store uses async persistence (name:
'github-stars-manager', storage: debouncedPersistStorage/ indexedDBStorage
fallback), so persisted snapshot can rehydrate and break assertions; fix by
isolating this suite from persistence: before importing useAppStore, either
stub/disable the persist layer (mock debouncedPersistStorage/indexedDBStorage to
a no-op) or clear the persisted key ('github-stars-manager') from storage
(indexedDB/localStorage) so hydration has nothing to restore, then import
useAppStore (the code using vi.importActual and the beforeAll/createRepository
helpers will work unchanged).
In `@src/store/useAppStore.ts`:
- Around line 80-109: The delayed persistence path using timeoutId +
scheduleIdleTask causes recent updates to be lost if the page is hidden/closed
before the idle callback runs; add a flush function that cancels timeoutId and
idleTaskId, synchronously stringifies latestValue and calls
indexedDBStorage.setItem(name, str) (handling errors via logger.errorFromError)
and invoke it on visibilitychange/pagehide (when document.visibilityState ===
'hidden') and beforeunload so critical writes bypass the idle queue; keep
scheduleIdleTask usage for normal runs but ensure the new flush references
timeoutId, idleTaskId, latestValue, indexedDBStorage.setItem, scheduleIdleTask,
logger and name to locate integration points.
---
Outside diff comments:
In `@src/store/useAppStore.ts`:
- Around line 69-114: The bug is that timeoutId, idleTaskId and latestValue are
closed over inside setItem so removeItem cannot cancel pending debounced/idle
writes and an old snapshot can be written back after removal; fix by hoisting
those variables to shared scope (or to the surrounding store object) so both
setItem and removeItem can access them, and update removeItem (the
indexedDBStorage.removeItem caller) to clearTimeout(timeoutId),
cancelIdleTask(idleTaskId) and reset idleTaskId/timeoutId/latestValue (or bump a
write-version token) so the scheduled task checks that token/latestValue is
still valid before calling indexedDBStorage.setItem(name, ...); ensure the
scheduled closure returns early when latestValue is null or the token/version
mismatches to prevent resurrecting removed state.
---
Nitpick comments:
In `@src/services/indexedDbStorage.ts`:
- Line 100: Update the top-of-file comment that currently reads
"IndexedDB-backed Zustand persist storage with seamless migration + dual write:"
to reflect that dual write was removed (e.g., "IndexedDB-backed Zustand persist
storage with seamless migration" or "single-write to IndexedDB"). Locate the
header comment above the exported storage implementation (the module-level
comment that precedes the IndexedDB storage functions such as the exported
storage creator/handler in this file) and edit it so it no longer references
"dual write" or implies writing to both IndexedDB and localStorage.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9276130c-a19b-4db5-a034-8b068e6871c6
📒 Files selected for processing (5)
src/components/RepositoryCard.tsxsrc/components/RepositoryList.tsxsrc/services/indexedDbStorage.tssrc/store/useAppStore.test.tssrc/store/useAppStore.ts
|
Addressed the AI review feedback in 357e0bf:
Validation:
@coderabbitai review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/store/useAppStore.ts (1)
967-979: ⚡ Quick winReuse the first replacement when both lists share identity.
Line 966 and Lines 1011-1014 keep
repositoriesandsearchResultsaliased on the common unfiltered path. This branch still does the samefindIndex/copy work twice and then breaks that shared reference, which adds avoidable cost on large lists.💡 Targeted refactor
updateRepository: (repo) => set((state) => { const repositoriesResult = replaceRepositoryInList(state.repositories, repo); - const searchResultsResult = replaceRepositoryInList(state.searchResults, repo); + const searchResultsResult = + state.searchResults === state.repositories + ? { + repositories: repositoriesResult.repositories, + changed: repositoriesResult.changed, + found: repositoriesResult.found, + } + : replaceRepositoryInList(state.searchResults, repo); if (!repositoriesResult.changed && !searchResultsResult.changed) { return state; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/store/useAppStore.ts` around lines 967 - 979, The updateRepository handler is doing two separate replaceRepositoryInList calls even when state.repositories and state.searchResults are the same array; compute the replacement once in updateRepository (call replaceRepositoryInList on state.repositories), then if state.repositories === state.searchResults reuse that single result for both repositories and searchResults to avoid repeating findIndex/copy work; otherwise run replaceRepositoryInList on searchResults and merge results as before, preserving the existing changed checks and returned shape.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/store/useAppStore.ts`:
- Around line 91-100: The indexedDBStorage.setItem promise calls in
useAppStore.ts (the one that currently measures writeMs and calls logger.warn)
lack rejection handlers; add a .catch to both indexedDBStorage.setItem(...)
usages to handle failures: compute writeMs like the success path, call
logger.error('store.persist', 'IndexedDB write failed', { source, writeMs,
bytes: str.length, error: err }) and surface the failure to the app state (e.g.,
set a boolean persistenceFailed / call an existing store mutation) so the UI can
react instead of silently dropping persistence. Ensure you reference the same
variables used there (writeStartedAt, str, source) and keep the success .then
logic intact.
---
Nitpick comments:
In `@src/store/useAppStore.ts`:
- Around line 967-979: The updateRepository handler is doing two separate
replaceRepositoryInList calls even when state.repositories and
state.searchResults are the same array; compute the replacement once in
updateRepository (call replaceRepositoryInList on state.repositories), then if
state.repositories === state.searchResults reuse that single result for both
repositories and searchResults to avoid repeating findIndex/copy work; otherwise
run replaceRepositoryInList on searchResults and merge results as before,
preserving the existing changed checks and returned shape.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2f7dc80-220a-4797-9b80-bd264ec814b0
📒 Files selected for processing (3)
src/services/indexedDbStorage.tssrc/store/useAppStore.test.tssrc/store/useAppStore.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/store/useAppStore.test.ts
- src/services/indexedDbStorage.ts
|
Addressed the latest CodeRabbit review in 45ed403:
Validation:
@coderabbitai review |
|
✅ Action performedReview finished.
|
Clear migrated localStorage snapshots after IndexedDB becomes authoritative so a later IndexedDB read failure cannot hydrate stale data. Keep localStorage writes only as the constrained-environment fallback when IndexedDB is unavailable or a write fails. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
补充修复提交： 原因：前一版移除了 localStorage 双写，但读取路径仍会在 IndexedDB 失败时 fallback 到 localStorage。如果迁移后的旧 localStorage 快照不清理，后续 IndexedDB 读取超时/失败时可能 hydrate 到陈旧状态，表现为数据回滚。同时 IndexedDB 不可用的受限环境下也会失去新的持久化写入。 本提交处理：
已验证： 说明： |
|
已在原 PR 分支追加修复提交： 修复原因：前一版移除了 localStorage 双写，但读取路径仍会在 IndexedDB 失败时 fallback 到 localStorage。如果迁移后的旧 localStorage 快照不清理，后续 IndexedDB 读取超时/失败时可能 hydrate 到陈旧状态，表现为数据回滚。同时 IndexedDB 不可用的受限环境下也会失去新的持久化写入。 本提交处理：
验证： 说明： |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/indexedDbStorage.ts`:
- Around line 24-30: safeLocalStorageSet currently swallows localStorage.setItem
errors; change it to return a boolean (or throw) indicating success so callers
can detect failure: update function safeLocalStorageSet(key: string, value:
string) to return true on success and false (or rethrow the caught error) on
failure, and then update the code paths that call safeLocalStorageSet (the
IndexedDB fallback logic around the persist/save snapshot routine) to treat a
false result as a write failure and reject or propagate an error when neither
IndexedDB nor localStorage succeeded so the upstream persist-failure handling
can run.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 863a5a17-e837-4d7a-bf79-14f8f6320b4f
📒 Files selected for processing (2)
src/services/indexedDbStorage.tssrc/store/useAppStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/store/useAppStore.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/indexedDbStorage.ts`:
- Around line 24-30: safeLocalStorageSet currently swallows localStorage.setItem
errors; change it to return a boolean (or throw) indicating success so callers
can detect failure: update function safeLocalStorageSet(key: string, value:
string) to return true on success and false (or rethrow the caught error) on
failure, and then update the code paths that call safeLocalStorageSet (the
IndexedDB fallback logic around the persist/save snapshot routine) to treat a
false result as a write failure and reject or propagate an error when neither
IndexedDB nor localStorage succeeded so the upstream persist-failure handling
can run.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 863a5a17-e837-4d7a-bf79-14f8f6320b4f
📒 Files selected for processing (2)
src/services/indexedDbStorage.tssrc/store/useAppStore.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/store/useAppStore.test.ts
🛑 Comments failed to post (1)
src/services/indexedDbStorage.ts (1)
24-30:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate the fallback write failure instead of swallowing it.
On Line 24-Line 30 and Line 151-Line 156,
safeLocalStorageSetturnslocalStorage.setItemfailures into a successfulsetItemcall. If IndexedDB already failed, that makes “nothing was persisted” look like success, so the new persist-failure handling upstream never fires and the latest snapshot can be dropped silently. Return a success flag from the helper and reject when neither backend stores the value. This conflicts with the PR objective to surface failed persistence paths.Suggested fix
-const safeLocalStorageSet = (key: string, value: string): void => { +const safeLocalStorageSet = (key: string, value: string): boolean => { try { window.localStorage.setItem(key, value); + return true; } catch { // Quota/security errors are expected in some environments; ignore. + return false; } }; ... - safeLocalStorageSet(name, value); + if (!safeLocalStorageSet(name, value)) { + throw new Error('[storage] localStorage fallback write failed'); + } },Also applies to: 151-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/indexedDbStorage.ts` around lines 24 - 30, safeLocalStorageSet currently swallows localStorage.setItem errors; change it to return a boolean (or throw) indicating success so callers can detect failure: update function safeLocalStorageSet(key: string, value: string) to return true on success and false (or rethrow the caught error) on failure, and then update the code paths that call safeLocalStorageSet (the IndexedDB fallback logic around the persist/save snapshot routine) to treat a false result as a write failure and reject or propagate an error when neither IndexedDB nor localStorage succeeded so the upstream persist-failure handling can run.
Return a success flag from the localStorage fallback writer and throw when neither IndexedDB nor localStorage can persist the snapshot. This lets the upstream persistence error handling report a real write failure instead of treating a dropped snapshot as success. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
已追加修复 CodeRabbit 最新审计问题： 原因： 本提交处理：
验证：
说明： |
2 participants
Fixes #198
Summary
analyzingRepositoryIdsSet.searchResultswhen the repo is not present there.Validation
npm run buildnpx vitest run src/store/useAppStore.test.ts src/utils/markdownUtils.test.tsNote
npm test -- --runstill reports one unrelated existing failure inMarkdownRenderer.test.tsx:Code Blocks > should show line numbers for code blocks with more than 3 lines.Summary by CodeRabbit
Bug Fixes
Performance
Observability
Tests
Chores