fix: avoid redundant scroll restore after sync#61
Conversation
📝 WalkthroughWalkthroughThe pull request integrates category selection into app-wide state management, persisting category preferences and enhancing scroll restoration during background syncs. It updates asset references, adjusts Nginx proxy configuration for resolver-based routing, and refactors component state dependencies. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CategorySidebar
participant App
participant useAppStore
participant RepositoryList
User->>CategorySidebar: Select Category
CategorySidebar->>App: Call onCategorySelect(category)
App->>useAppStore: Call setSelectedCategory(category)
useAppStore->>useAppStore: Update selectedCategory state
useAppStore->>useAppStore: Persist selectedCategory
App->>RepositoryList: Pass updated selectedCategory prop
RepositoryList->>RepositoryList: Detect category change
RepositoryList->>RepositoryList: Scroll to top
RepositoryList->>RepositoryList: Reset visibleCount & filters
RepositoryList->>RepositoryList: Recompute filterResetKey
sequenceDiagram
participant RepositoryList
participant Store as Background Sync
participant Browser as Browser Engine
RepositoryList->>Store: Sync starts (isSyncing=true)
Store->>RepositoryList: handleSyncVisualState() called
RepositoryList->>RepositoryList: Disable card animations
RepositoryList->>RepositoryList: Save current scrollY position
RepositoryList->>RepositoryList: Cancel pending restore frame
Store->>RepositoryList: Sync completes (isSyncing=false)
RepositoryList->>RepositoryList: handleSyncVisualState() called again
RepositoryList->>Browser: requestAnimationFrame `#1`
Browser->>RepositoryList: Enable card animations
RepositoryList->>Browser: requestAnimationFrame `#2`
Browser->>RepositoryList: Restore scrollY position
RepositoryList->>RepositoryList: Clear saved scroll state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/store/useAppStore.ts (1)
150-168: Consider validatingselectedCategoryduring rehydration.The
normalizePersistedStatefunction doesn't validateselectedCategory. If a user had a custom category selected that was later deleted, the persisted value could reference a non-existent category, potentially causing UI inconsistencies.🛡️ Suggested defensive validation
return { ...currentState, ...safePersisted, repositories, releases, searchResults: repositories, releaseSubscriptions: normalizeNumberSet(safePersisted.releaseSubscriptions), readReleases: normalizeNumberSet(safePersisted.readReleases), searchFilters: { ...initialSearchFilters, sortBy: savedSortBy, sortOrder: savedSortOrder, }, webdavConfigs: Array.isArray(safePersisted.webdavConfigs) ? safePersisted.webdavConfigs : [], customCategories: Array.isArray(safePersisted.customCategories) ? safePersisted.customCategories : [], assetFilters: Array.isArray(safePersisted.assetFilters) ? safePersisted.assetFilters : [], language: safePersisted.language || 'zh', isAuthenticated: !!(safePersisted.user && safePersisted.githubToken), + selectedCategory: typeof safePersisted.selectedCategory === 'string' ? safePersisted.selectedCategory : 'all', };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/useAppStore.ts` around lines 150 - 168, During state rehydration ensure the persisted selectedCategory is validated and reset if it no longer exists: in the normalizePersistedState / rehydration path where safePersisted is merged into the returned state, check safePersisted.selectedCategory against the resolved customCategories (Array.isArray(safePersisted.customCategories) ? safePersisted.customCategories : []) and initialSearchFilters categories, and if it’s missing set selectedCategory to a sane default (e.g., initialSearchFilters.selectedCategory or null). Update the returned object (the block merging safePersisted into currentState) to use this validated selectedCategory instead of blindly using safePersisted.selectedCategory to avoid referencing deleted categories in the UI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/LoginScreen.tsx`:
- Around line 70-73: In LoginScreen.tsx replace the fragile string path on the
img element by importing the asset as a module (e.g., import icon from
'src/assets/icon.png') and bind the imported variable to the img src (the <img
... /> in the JSX) so Vite will bundle and cache-bust the asset; update the top
of the file to add the import and change the img src attribute to use the
imported identifier.
---
Nitpick comments:
In `@src/store/useAppStore.ts`:
- Around line 150-168: During state rehydration ensure the persisted
selectedCategory is validated and reset if it no longer exists: in the
normalizePersistedState / rehydration path where safePersisted is merged into
the returned state, check safePersisted.selectedCategory against the resolved
customCategories (Array.isArray(safePersisted.customCategories) ?
safePersisted.customCategories : []) and initialSearchFilters categories, and if
it’s missing set selectedCategory to a sane default (e.g.,
initialSearchFilters.selectedCategory or null). Update the returned object (the
block merging safePersisted into currentState) to use this validated
selectedCategory instead of blindly using safePersisted.selectedCategory to
avoid referencing deleted categories in the UI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cff13f0e-4f78-47e7-bdf6-b2ce0331a9b8
📒 Files selected for processing (7)
index.htmlnginx.confsrc/App.tsxsrc/components/LoginScreen.tsxsrc/components/RepositoryList.tsxsrc/store/useAppStore.tssrc/types/index.ts
| <div className="flex items-center justify-center w-16 h-16 bg-white rounded-2xl mx-auto mb-4 shadow-lg ring-1 ring-blue-100 overflow-hidden"> | ||
| <img | ||
| src="./icon.png" | ||
| alt="GitHub Stars Manager" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify where icon.png actually exists and how it's referenced.
fd -a 'icon\.png$'
rg -n --type=tsx 'src="\./icon\.png"'
rg -n --type=tsx "from ['\"].*icon\.png['\"]"Repository: AmintaCCCP/GithubStarsManager
Length of output: 187
🏁 Script executed:
cd /home/jailuser/git && find . -name "LoginScreen.tsx" -type fRepository: AmintaCCCP/GithubStarsManager
Length of output: 105
🏁 Script executed:
cd /home/jailuser/git && head -100 src/components/LoginScreen.tsx | tail -40Repository: AmintaCCCP/GithubStarsManager
Length of output: 1791
🏁 Script executed:
cd /home/jailuser/git && sed -n '1,10p' src/components/LoginScreen.tsxRepository: AmintaCCCP/GithubStarsManager
Length of output: 562
🏁 Script executed:
cd /home/jailuser/git && sed -n '65,80p' src/components/LoginScreen.tsxRepository: AmintaCCCP/GithubStarsManager
Length of output: 772
🏁 Script executed:
cd /home/jailuser/git && rg 'icon\.png' --no-filenameRepository: AmintaCCCP/GithubStarsManager
Length of output: 250
🏁 Script executed:
cd /home/jailuser/git && find . -maxdepth 2 -name "vite.config.*" -o -name "webpack.config.*" -o -name "tsconfig.json" -o -name "package.json" | head -10Repository: AmintaCCCP/GithubStarsManager
Length of output: 165
🏁 Script executed:
cd /home/jailuser/git && cat package.json | grep -A 5 -B 5 "build\|vite\|webpack" | head -30Repository: AmintaCCCP/GithubStarsManager
Length of output: 1034
🏁 Script executed:
cd /home/jailuser/git && ls -la assets/ public/ 2>/dev/null | head -20Repository: AmintaCCCP/GithubStarsManager
Length of output: 521
🏁 Script executed:
cd /home/jailuser/git && rg "from.*assets" src/ --type tsx | head -10Repository: AmintaCCCP/GithubStarsManager
Length of output: 100
Import the icon as a bundled asset instead of using a relative string path.
Line 72 uses src="./icon.png", which is fragile in routed paths. In a Vite app, relative paths resolve to the current route location, not the project root. Since icon.png exists in src/assets/, import it as a module and bind src to that URL for proper bundling and cache-busting.
Suggested fix
+import icon from '../assets/icon.png';
...
<img
- src="./icon.png"
+ src={icon}
alt="GitHub Stars Manager"
className="w-full h-full object-cover"
/>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/LoginScreen.tsx` around lines 70 - 73, In LoginScreen.tsx
replace the fragile string path on the img element by importing the asset as a
module (e.g., import icon from 'src/assets/icon.png') and bind the imported
variable to the img src (the <img ... /> in the JSX) so Vite will bundle and
cache-bust the asset; update the top of the file to add the import and change
the img src attribute to use the imported identifier.
|
Closing per request: fix should go into PR #60 branch. |
2 participants
Problem: During sync visual state restore, scroll is always forced back to the saved position even when the scroll position never changed. This can cause a subtle jitter after sync. Fix: Only restore scroll when window.scrollY actually differs from the saved position, and clear the saved value otherwise. Testing: not run (UI change).
Summary by CodeRabbit
New Features
Bug Fixes
Style