fix: 修复 PR #90 审计问题及白屏问题#92
Conversation
1. 修复 githubApi.ts 中重复的 forks_count 键 2. 修复 subscriptionChannels normalization - 保留用户自定义名称 3. 修复 subscriptionRepos/subscriptionLastRefresh/subscriptionIsLoading 的 spread 顺序，确保持久化数据优先 4. 添加 tablet 导航 aria-label 属性 5. 修复 RepositoryCard.tsx 中的 race condition，添加 mounted ref Co-Authored-By: Claude Opus 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded aria-labels to non-mobile header navigation buttons; made RepositoryCard safe against post-unmount async updates by guarding updates with an isMounted ref; adjusted trending search to stop assigning Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are focused on UI accessibility, component lifecycle safety, and state migration without a new multi-component sequential flow that benefits from a diagram.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/Header.tsx (1)
212-222:⚠️ Potential issue | 🟡 MinorDesktop "Trending" button missing
aria-label/titlewhen icon-only.The sibling desktop buttons (repositories, releases, settings at lines 179-180, 201-202, 225-226) were updated to set
aria-labelandtitlewhenisTextWrappedis true (icon-only state), but the subscription/Trending button here was missed. When the nav collapses to icons on desktop, this button will be unlabeled for screen readers — the same a11y gap the PR is fixing elsewhere.Proposed fix
<button onClick={() => setCurrentView('subscription')} + aria-label={isTextWrapped ? t('趋势', 'Trending') : undefined} + title={isTextWrapped ? t('趋势', 'Trending') : undefined} className={`${isTextWrapped ? 'p-2.5' : 'px-4 py-2'} rounded-lg font-medium transition-colors ${🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Header.tsx` around lines 212 - 222, The desktop "Trending"/subscription button in Header.tsx is missing the aria-label/title when rendered icon-only; update the button (the element that calls setCurrentView('subscription') and uses currentView, isTextWrapped and the <TrendingUp /> icon) to include aria-label and title attributes when isTextWrapped is true (use the same label text used by t('趋势', 'Trending') so screen readers get "Trending"); ensure the attributes mirror the pattern used on the other icon-only buttons for consistency.
🤖 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/RepositoryCard.tsx`:
- Around line 323-343: The current isMountedRef check is preventing store writes
and dropping AI results; always call the Zustand action updateRepository with
the successful updatedRepo or the failedRepo (constructed in the catch)
regardless of isMountedRef, and only gate the alert(...) calls with
isMountedRef.current so UI notifications are skipped when unmounted; adjust the
success path around updateRepository(updatedRepo) and the error path around
updateRepository(failedRepo) to remove the isMountedRef guard while keeping
alerts conditional.
In `@src/store/useAppStore.ts`:
- Around line 256-263: The migration step currently overwrites persisted
default-channel names with defaultCh.name before the later merge preserves
ch.name; update the migration logic (the function that runs before the
persisted.map merge) to use the same name-preservation rule as in the
persisted.map block: when merging a persisted SubscriptionChannel with defaultCh
from defaultChannelsMap, set name to ch.name || defaultCh.name and set nameEn to
ch.nameEn || defaultCh.nameEn || ch.name so user-customized names are never lost
during migration.
- Around line 229-250: The spread order currently places safePersisted.* after
the default channel entries which causes persisted subscription data to be
overwritten; change each object construction (subscriptionRepos,
subscriptionLastRefresh, subscriptionIsLoading, subscriptionChannels) to spread
the default channel values first and then spread safePersisted.* so persisted
values win (e.g., default keys like
'most-stars','most-forks','most-dev','trending' before
...safePersisted.subscriptionRepos). Also ensure TypeScript safety by either
adding subscriptionRepos, subscriptionLastRefresh, subscriptionIsLoading, and
subscriptionChannels to the PersistedAppState type or uniformly casting
safePersisted accesses to the same type (e.g., as Record<string, unknown>) for
all four properties to satisfy strict mode and avoid inconsistent casts.
---
Outside diff comments:
In `@src/components/Header.tsx`:
- Around line 212-222: The desktop "Trending"/subscription button in Header.tsx
is missing the aria-label/title when rendered icon-only; update the button (the
element that calls setCurrentView('subscription') and uses currentView,
isTextWrapped and the <TrendingUp /> icon) to include aria-label and title
attributes when isTextWrapped is true (use the same label text used by t('趋势',
'Trending') so screen readers get "Trending"); ensure the attributes mirror the
pattern used on the other icon-only buttons for consistency.
🪄 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: 6af14923-04a6-441c-9fbe-e296b862f3dd
📒 Files selected for processing (4)
src/components/Header.tsxsrc/components/RepositoryCard.tsxsrc/services/githubApi.tssrc/store/useAppStore.ts
💤 Files with no reviewable changes (1)
- src/services/githubApi.ts
| // Only update store if component is still mounted (prevents race condition on unmount) | ||
| if (isMountedRef.current) { | ||
| updateRepository(updatedRepo); | ||
| const successMessage = repository.analyzed_at | ||
| ? (language === 'zh' ? 'AI重新分析完成！' : 'AI re-analysis completed!') | ||
| : (language === 'zh' ? 'AI分析完成！' : 'AI analysis completed!'); | ||
| alert(successMessage); | ||
| } | ||
| } catch (error) { | ||
| console.error('AI analysis failed:', error); | ||
| // 标记为分析失败 | ||
| const failedRepo = { | ||
| ...repository, | ||
| analyzed_at: new Date().toISOString(), | ||
| analysis_failed: true | ||
| }; | ||
|
|
||
| updateRepository(failedRepo); | ||
|
|
||
| alert(language === 'zh' ? 'AI分析失败，请检查AI配置和网络连接。' : 'AI analysis failed. Please check AI configuration and network connection.'); | ||
|
|
||
| // Only update store if component is still mounted (prevents race condition on unmount) | ||
| if (isMountedRef.current) { | ||
| const failedRepo = { | ||
| ...repository, | ||
| analyzed_at: new Date().toISOString(), | ||
| analysis_failed: true | ||
| }; | ||
| updateRepository(failedRepo); | ||
| alert(language === 'zh' ? 'AI分析失败，请检查AI配置和网络连接。' : 'AI analysis failed. Please check AI configuration and network connection.'); | ||
| } |
There was a problem hiding this comment.
Gating updateRepository on isMountedRef silently drops AI analysis results on unmount.
updateRepository is a Zustand store action (line 111), not a component-local setState, so it is safe to call after the card unmounts and does not produce the legacy "state update on unmounted component" warning (which React 18 removed anyway). With the current guard, if the user changes filter/category, scrolls past a virtualized card, or navigates away while aiService.analyzeRepository is still in flight (it can take tens of seconds), both the success payload (ai_summary, ai_tags, ai_platforms, analyzed_at) and the failure flag (analysis_failed) are silently discarded. That wastes AI tokens and leaves the repo permanently stuck in its pre-analysis state — a regression vs. the pre-PR behavior.
Only the alert() is genuinely tied to "is the user still looking at this card"; the store write should always happen.
🔧 Proposed fix — always persist, only skip the alert
- // Only update store if component is still mounted (prevents race condition on unmount)
- if (isMountedRef.current) {
- updateRepository(updatedRepo);
- const successMessage = repository.analyzed_at
- ? (language === 'zh' ? 'AI重新分析完成！' : 'AI re-analysis completed!')
- : (language === 'zh' ? 'AI分析完成！' : 'AI analysis completed!');
- alert(successMessage);
- }
+ // Always persist results — updateRepository is a Zustand store action and is
+ // safe to call after unmount. Only skip the user-facing alert if the card is
+ // no longer mounted.
+ updateRepository(updatedRepo);
+ if (isMountedRef.current) {
+ const successMessage = repository.analyzed_at
+ ? (language === 'zh' ? 'AI重新分析完成！' : 'AI re-analysis completed!')
+ : (language === 'zh' ? 'AI分析完成！' : 'AI analysis completed!');
+ alert(successMessage);
+ }
} catch (error) {
console.error('AI analysis failed:', error);
- // Only update store if component is still mounted (prevents race condition on unmount)
- if (isMountedRef.current) {
- const failedRepo = {
- ...repository,
- analyzed_at: new Date().toISOString(),
- analysis_failed: true
- };
- updateRepository(failedRepo);
- alert(language === 'zh' ? 'AI分析失败，请检查AI配置和网络连接。' : 'AI analysis failed. Please check AI configuration and network connection.');
- }
+ const failedRepo = {
+ ...repository,
+ analyzed_at: new Date().toISOString(),
+ analysis_failed: true,
+ };
+ updateRepository(failedRepo);
+ if (isMountedRef.current) {
+ alert(language === 'zh' ? 'AI分析失败，请检查AI配置和网络连接。' : 'AI analysis failed. Please check AI configuration and network connection.');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Only update store if component is still mounted (prevents race condition on unmount) | |
| if (isMountedRef.current) { | |
| updateRepository(updatedRepo); | |
| const successMessage = repository.analyzed_at | |
| ? (language === 'zh' ? 'AI重新分析完成！' : 'AI re-analysis completed!') | |
| : (language === 'zh' ? 'AI分析完成！' : 'AI analysis completed!'); | |
| alert(successMessage); | |
| } | |
| } catch (error) { | |
| console.error('AI analysis failed:', error); | |
| // 标记为分析失败 | |
| const failedRepo = { | |
| ...repository, | |
| analyzed_at: new Date().toISOString(), | |
| analysis_failed: true | |
| }; | |
| updateRepository(failedRepo); | |
| alert(language === 'zh' ? 'AI分析失败，请检查AI配置和网络连接。' : 'AI analysis failed. Please check AI configuration and network connection.'); | |
| // Only update store if component is still mounted (prevents race condition on unmount) | |
| if (isMountedRef.current) { | |
| const failedRepo = { | |
| ...repository, | |
| analyzed_at: new Date().toISOString(), | |
| analysis_failed: true | |
| }; | |
| updateRepository(failedRepo); | |
| alert(language === 'zh' ? 'AI分析失败，请检查AI配置和网络连接。' : 'AI analysis failed. Please check AI configuration and network connection.'); | |
| } | |
| // Always persist results — updateRepository is a Zustand store action and is | |
| // safe to call after unmount. Only skip the user-facing alert if the card is | |
| // no longer mounted. | |
| updateRepository(updatedRepo); | |
| if (isMountedRef.current) { | |
| const successMessage = repository.analyzed_at | |
| ? (language === 'zh' ? 'AI重新分析完成！' : 'AI re-analysis completed!') | |
| : (language === 'zh' ? 'AI分析完成！' : 'AI analysis completed!'); | |
| alert(successMessage); | |
| } | |
| } catch (error) { | |
| console.error('AI analysis failed:', error); | |
| const failedRepo = { | |
| ...repository, | |
| analyzed_at: new Date().toISOString(), | |
| analysis_failed: true, | |
| }; | |
| updateRepository(failedRepo); | |
| if (isMountedRef.current) { | |
| alert(language === 'zh' ? 'AI分析失败，请检查AI配置和网络连接。' : 'AI analysis failed. Please check AI configuration and network connection.'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/RepositoryCard.tsx` around lines 323 - 343, The current
isMountedRef check is preventing store writes and dropping AI results; always
call the Zustand action updateRepository with the successful updatedRepo or the
failedRepo (constructed in the catch) regardless of isMountedRef, and only gate
the alert(...) calls with isMountedRef.current so UI notifications are skipped
when unmounted; adjust the success path around updateRepository(updatedRepo) and
the error path around updateRepository(failedRepo) to remove the isMountedRef
guard while keeping alerts conditional.
| // 确保 subscription 相关状态包含 trending 键（默认放后面，这样 persisted 数据优先） | ||
| subscriptionRepos: { | ||
| ...(safePersisted.subscriptionRepos as Record<string, unknown> || {}), | ||
| 'most-stars': [], | ||
| 'most-forks': [], | ||
| 'most-dev': [], | ||
| 'trending': [], | ||
| ...(safePersisted.subscriptionRepos as Record<string, unknown> || {}), | ||
| }, | ||
| subscriptionLastRefresh: { | ||
| ...((safePersisted as Record<string, unknown>).subscriptionLastRefresh as Record<string, unknown> || {}), | ||
| 'most-stars': null, | ||
| 'most-forks': null, | ||
| 'most-dev': null, | ||
| 'trending': null, | ||
| ...((safePersisted as Record<string, unknown>).subscriptionLastRefresh as Record<string, unknown> || {}), | ||
| }, | ||
| subscriptionIsLoading: { | ||
| ...((safePersisted as Record<string, unknown>).subscriptionIsLoading as Record<string, unknown> || {}), | ||
| 'most-stars': false, | ||
| 'most-forks': false, | ||
| 'most-dev': false, | ||
| 'trending': false, | ||
| ...((safePersisted as Record<string, unknown>).subscriptionIsLoading as Record<string, unknown> || {}), | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect whether PersistedAppState declares the subscription fields read during normalization.
# Expected: subscriptionRepos/subscriptionChannels/subscriptionLastRefresh should be part of PersistedAppState
# or accessed through a validated unknown record.
sed -n '135,170p' src/store/useAppStore.ts
printf '\n--- subscription persisted-state reads ---\n'
rg -n -C2 'safePersisted\.subscription|subscriptionRepos|subscriptionLastRefresh|subscriptionIsLoading|subscriptionChannels' src/store/useAppStore.tsRepository: AmintaCCCP/GithubStarsManager
Length of output: 5382
Flip spread order to prevent persisted subscription data from being overwritten with empty defaults.
Line 231 spreads safePersisted.subscriptionRepos first, then immediately overwrites the four default channels ('most-stars', 'most-forks', 'most-dev', 'trending') with []. Lines 238 and 245 do the same with null and false. This discards any hydrated subscription cache and risks re-persisting the emptied state.
The fix moves the default channel initialization before the spread, allowing persisted values to override defaults:
Proposed fix
- // 确保 subscription 相关状态包含 trending 键（默认放后面，这样 persisted 数据优先）
+ // 确保 subscription 相关状态包含 trending 键（默认放前面，这样 persisted 数据优先）
subscriptionRepos: {
- ...(safePersisted.subscriptionRepos as Record<string, unknown> || {}),
'most-stars': [],
'most-forks': [],
'most-dev': [],
'trending': [],
+ ...((safePersisted as Record<string, unknown>).subscriptionRepos as Record<string, unknown> || {}),
},
subscriptionLastRefresh: {
- ...((safePersisted as Record<string, unknown>).subscriptionLastRefresh as Record<string, unknown> || {}),
'most-stars': null,
'most-forks': null,
'most-dev': null,
'trending': null,
+ ...((safePersisted as Record<string, unknown>).subscriptionLastRefresh as Record<string, unknown> || {}),
},
subscriptionIsLoading: {
- ...((safePersisted as Record<string, unknown>).subscriptionIsLoading as Record<string, unknown> || {}),
'most-stars': false,
'most-forks': false,
'most-dev': false,
'trending': false,
+ ...((safePersisted as Record<string, unknown>).subscriptionIsLoading as Record<string, unknown> || {}),
},Additionally, subscriptionRepos, subscriptionLastRefresh, subscriptionIsLoading, and subscriptionChannels are not declared in the PersistedAppState type definition (lines 135–169 show only a curated Pick of AppState fields). Line 231 directly accesses safePersisted.subscriptionRepos without a type guard; lines 238, 245, and 253 access these properties inconsistently (some with casts, others without). Add these fields to PersistedAppState or apply consistent as Record<string, unknown> casts to all such accesses to satisfy TypeScript strict mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/useAppStore.ts` around lines 229 - 250, The spread order currently
places safePersisted.* after the default channel entries which causes persisted
subscription data to be overwritten; change each object construction
(subscriptionRepos, subscriptionLastRefresh, subscriptionIsLoading,
subscriptionChannels) to spread the default channel values first and then spread
safePersisted.* so persisted values win (e.g., default keys like
'most-stars','most-forks','most-dev','trending' before
...safePersisted.subscriptionRepos). Also ensure TypeScript safety by either
adding subscriptionRepos, subscriptionLastRefresh, subscriptionIsLoading, and
subscriptionChannels to the PersistedAppState type or uniformly casting
safePersisted accesses to the same type (e.g., as Record<string, unknown>) for
all four properties to satisfy strict mode and avoid inconsistent casts.
| // 合并：使用 persisted 的频道，但补全缺失的字段（保留用户自定义的 name） | ||
| return persisted.map((ch: SubscriptionChannel) => { | ||
| const defaultCh = defaultChannelsMap.get(ch.id); | ||
| if (defaultCh) { | ||
| return { | ||
| ...ch, | ||
| name: defaultCh.name, // 始终使用中文名称（默认定义） | ||
| nameEn: ch.nameEn || defaultCh.nameEn || ch.name || defaultCh.nameEn, | ||
| name: ch.name || defaultCh.name, // 保留用户自定义名称，否则使用默认 | ||
| nameEn: ch.nameEn || defaultCh.nameEn || ch.name, |
There was a problem hiding this comment.
Apply the same name-preservation rule during migration.
The merge normalization now preserves ch.name, but migration runs first and Line 1064 still overwrites persisted default-channel names with defaultCh.name. Users migrating from an older persisted version can still lose custom channel names before this code runs.
🐛 Proposed fix
if (defaultCh) {
+ const name = typeof ch.name === 'string' && ch.name ? ch.name : defaultCh.name;
return {
...ch,
- name: defaultCh.name, // 始终使用中文名称
- nameEn: ch.nameEn || defaultCh.nameEn || ch.name || defaultCh.nameEn,
+ name,
+ nameEn: ch.nameEn || defaultCh.nameEn || name,
icon: ch.icon || defaultCh.icon,
description: ch.description || defaultCh.description,
};
}Also applies to: 1056-1065
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/useAppStore.ts` around lines 256 - 263, The migration step
currently overwrites persisted default-channel names with defaultCh.name before
the later merge preserves ch.name; update the migration logic (the function that
runs before the persisted.map merge) to use the same name-preservation rule as
in the persisted.map block: when merging a persisted SubscriptionChannel with
defaultCh from defaultChannelsMap, set name to ch.name || defaultCh.name and set
nameEn to ch.nameEn || defaultCh.nameEn || ch.name so user-customized names are
never lost during migration.
在 persist store 的迁移逻辑中添加了对 subscriptionRepos、subscriptionLastRefresh、subscriptionIsLoading 的完整性检查和修复，确保所有频道键（most-stars、most-forks、most-dev、trending）都存在。 🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 participant
Summary by CodeRabbit
Accessibility
Bug Fixes
Improvements