fix: persist GitHub star sync immediately and guard against empty-backend overwrite#142
Conversation
…kend overwrite Two related data-loss bugs when using the backend sync feature: 1. src/components/Header.tsx After fetching starred repos from GitHub, the store update was only flushed to the backend after a 2-second debounce. If the user refreshed the page within that window the debounce timer was destroyed, the PUT never fired, and the data was lost on the next load (when syncFromBackend overwrites the local store with the still-empty backend). Fix: call forceSyncToBackend() immediately after setRepositories() so the data reaches the backend before the user can navigate away. 2. src/services/autoSync.ts (syncFromBackend) On every page load, syncFromBackend fetches from the backend and overwrites the Zustand store. If the backend has zero repositories (e.g. first-time setup, backend was reset, or the debounce race above occurred), it silently clears whatever was cached in localStorage. Fix: skip the setRepositories call when the backend returns an empty list but the local store already has repositories, so locally-cached data is never replaced with an empty response. Together these two changes make starred-repo data survive a page refresh even in scenarios where the backend has not yet received a successful sync. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughautoSync now preserves local repositories when the backend appears as an initial empty bootstrap; Header.handleSync now calls forceSyncToBackend immediately after updating the local repositories (fire-and-forget, errors logged). ChangesBackend Sync Guard and Header Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
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/autoSync.ts`:
- Around line 136-139: The current guard prevents applying an empty backend repo
list when localRepos is non-empty, blocking legitimate “delete all repos” from
another device; update the condition around state.setRepositories so an empty
backend is accepted when it represents a new authoritative backend state.
Specifically, in the block that references backendRepos, localRepos,
state.setRepositories, _lastHash.repos and hashes.repos, change the if to also
allow assignment when hashes.repos has changed since _lastHash.repos (e.g.,
backendRepos.length > 0 || localRepos.length === 0 || hashes.repos !==
_lastHash.repos) so an authoritative empty backend will overwrite local repos
and convergence can occur.
🪄 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: e15ff28c-da89-4c7f-a1b0-2978e8de71b3
📒 Files selected for processing (2)
src/components/Header.tsxsrc/services/autoSync.ts
- Differentiate bootstrap-empty from authoritative-empty backend: only preserve local cache on first-ever sync (_lastHash.repos === ''), accept empty backend on subsequent syncs so cross-device deletes converge. (addresses CodeRabbit review) - Replace void forceSyncToBackend() with .catch(console.error) so sync failures are logged instead of silently swallowed.
Summary
Two related data-loss bugs when the backend sync feature is enabled. Both were found during a self-hosted deployment and reproduce reliably.
Bug 1 — Starred repos lost if page is refreshed within 2 seconds of sync (
Header.tsx)After fetching starred repos from GitHub, the new repository list is written to the Zustand store but only flushed to the backend after a 2-second debounce. If the user refreshes the page within that window:
PUT /api/repositoriesnever fires.syncFromBackendfetches the still-empty backend and overwrites the local store.Fix: call
forceSyncToBackend()immediately aftersetRepositories()inHeader.tsx, so the data reaches the backend before the user can navigate away.Bug 2 — Empty backend silently wipes locally-cached repos (
autoSync.ts)On every page load,
syncFromBackendfetches repos from the backend and unconditionally overwrites the Zustand store — even when the backend returns an empty array. This triggers in several scenarios:Any repos cached in
localStoragefrom a previous session are silently cleared.Fix: skip
setRepositorieswhen the backend returns zero repositories but the local store is non-empty, so locally-cached data is never replaced by an empty backend response.Reproduction
API_SECRET).Or:
localStorage, open the app. → Repos disappear immediately aftersyncFromBackendruns (Bug 2).Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit