fix: keep PR #90 and avoid mac desktop white screen crash#95
Merged
AmintaCCCP merged 8 commits intomainfrom Apr 22, 2026
Merged
fix: keep PR #90 and avoid mac desktop white screen crash#95AmintaCCCP merged 8 commits intomainfrom
AmintaCCCP merged 8 commits intomainfrom
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughRemoved three stylesheet links from Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as DiscoveryView (UI)
participant Store as useAppStore
participant API as GitHub API
participant AI as AIAnalysisOptimizer
UI->>Store: read safeDiscoveryChannels, selectedChannel, currentPage
UI->>Store: set currentPage / request refreshChannel(channel,page,append?)
Store->>API: fetch repos (mode: trending/hot/topic/search)
API-->>Store: return repo list (page)
Store->>Store: merge repos, preserve AI fields, update paging/hasMore/lastRefresh
Store-->>UI: updated repos for channel
UI->>UI: user triggers Analyze Page
UI->>API: prefetch README files (concurrent)
UI->>AI: submit repos for analysis (concurrent)
AI-->>Store: per-repo analysis results or failures
Store-->>UI: progress updates and final repo updates
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 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/store/useAppStore.ts (1)
1269-1288: Duplicate normalization betweenmigrateandnormalizePersistedState.The
discoveryChannelsremapping here is identical to the logic innormalizePersistedStateat lines 277-296, andmergealways runs aftermigrateduring rehydration — so the migrate-time version is effectively dead work, and two copies of the invariant will drift over time. Consider extracting a singlenormalizeDiscoveryChannels(persisted)helper used by both paths (and similarly for thesubscriptionChannelsmigration vs. merge logic).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/useAppStore.ts` around lines 1269 - 1288, The duplicate remapping logic for discoveryChannels in migrate and normalizePersistedState should be extracted into a shared helper to avoid drift; add a function normalizeDiscoveryChannels(persisted: unknown[] | undefined, defaultDiscoveryChannels) that implements the current remapping (finding persisted by id, merging defaults, and treating enabled as persisted.enabled !== false), then replace the inline logic in both migrate and normalizePersistedState to call normalizeDiscoveryChannels(state.discoveryChannels, defaultDiscoveryChannels); also apply the same consolidation pattern for subscriptionChannels (extract normalizeSubscriptionChannels and call from both migrate and merge/normalize paths) so merge still runs but normalization logic is centralized.
🤖 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/store/useAppStore.ts`:
- Around line 297-338: The persisted rehydration for discoveryRepos,
discoveryLastRefresh, and discoveryTotalCount in useAppStore trusts the
persisted object shape and can be overwritten by malformed per-channel values;
update each initializer (discoveryRepos, discoveryLastRefresh,
discoveryTotalCount) to iterate the known DiscoveryChannelId keys and
coerce/validate per-key values from safePersisted: for discoveryRepos ensure
each channel value is an array (coerce any non-array to []), for
discoveryLastRefresh ensure each channel value is a string or null (coerce
non-strings to null), and for discoveryTotalCount ensure each channel value is a
finite number (coerce non-finite/non-number to 0); merge only the validated
per-key entries over the defaults so downstream code never receives malformed
types.
---
Nitpick comments:
In `@src/store/useAppStore.ts`:
- Around line 1269-1288: The duplicate remapping logic for discoveryChannels in
migrate and normalizePersistedState should be extracted into a shared helper to
avoid drift; add a function normalizeDiscoveryChannels(persisted: unknown[] |
undefined, defaultDiscoveryChannels) that implements the current remapping
(finding persisted by id, merging defaults, and treating enabled as
persisted.enabled !== false), then replace the inline logic in both migrate and
normalizePersistedState to call
normalizeDiscoveryChannels(state.discoveryChannels, defaultDiscoveryChannels);
also apply the same consolidation pattern for subscriptionChannels (extract
normalizeSubscriptionChannels and call from both migrate and merge/normalize
paths) so merge still runs but normalization logic is centralized.
🪄 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: 29395ae0-5417-40dc-8c8f-0bfa3c8b148e
📒 Files selected for processing (4)
index.htmlsrc/components/DiscoveryView.tsxsrc/store/useAppStore.tstailwind.config.js
💤 Files with no reviewable changes (1)
- index.html
Comment on lines
297
to
338
| discoveryRepos: (() => { | ||
| const persisted = (safePersisted as Record<string, unknown>).discoveryRepos; | ||
| if (persisted && typeof persisted === 'object' && !Array.isArray(persisted)) { | ||
| return persisted as Record<DiscoveryChannelId, DiscoveryRepo[]>; | ||
| return { | ||
| 'trending': [], | ||
| 'hot-release': [], | ||
| 'most-popular': [], | ||
| 'topic': [], | ||
| 'search': [], | ||
| ...(persisted as Record<DiscoveryChannelId, DiscoveryRepo[]>), | ||
| }; | ||
| } | ||
| return { 'trending': [], 'hot-release': [], 'most-popular': [], 'topic': [], 'search': [] } as Record<DiscoveryChannelId, DiscoveryRepo[]>; | ||
| })(), | ||
| discoveryLastRefresh: (() => { | ||
| const persisted = (safePersisted as Record<string, unknown>).discoveryLastRefresh; | ||
| if (persisted && typeof persisted === 'object' && !Array.isArray(persisted)) { | ||
| return persisted as Record<string, string | null>; | ||
| return { | ||
| 'trending': null, | ||
| 'hot-release': null, | ||
| 'most-popular': null, | ||
| 'topic': null, | ||
| 'search': null, | ||
| ...(persisted as Record<string, string | null>), | ||
| }; | ||
| } | ||
| return { 'trending': null, 'hot-release': null, 'most-popular': null, 'topic': null, 'search': null }; | ||
| })(), | ||
| discoveryTotalCount: (() => { | ||
| const persisted = (safePersisted as Record<string, unknown>).discoveryTotalCount; | ||
| if (persisted && typeof persisted === 'object' && !Array.isArray(persisted)) { | ||
| return persisted as Record<string, number>; | ||
| return { | ||
| 'trending': 0, | ||
| 'hot-release': 0, | ||
| 'most-popular': 0, | ||
| 'topic': 0, | ||
| 'search': 0, | ||
| ...(persisted as Record<string, number>), | ||
| }; | ||
| } | ||
| return { 'trending': 0, 'hot-release': 0, 'most-popular': 0, 'topic': 0, 'search': 0 }; | ||
| })(), |
Contributor
There was a problem hiding this comment.
Per-key values in discovery maps are not validated; malformed entries can still crash downstream.
The normalization here only checks that the persisted value is a non-array object and then spreads it over the defaults. If a persisted entry has an unexpected per-key type (e.g. discoveryRepos.trending is a plain object {}, a string, or 0), the spread will overwrite the default empty-array/null/0 baseline and the malformed value will flow through. Downstream state.discoveryRepos[channel] || [] only guards null/undefined, so a truthy non-array would reach allRepos.find(...) / allRepos.slice(...) / [...allRepos, ...repos] and throw — exactly the kind of rehydration crash this PR is trying to prevent.
Given the PR's hardening goal, consider validating each channel's value type (e.g., coerce non-arrays to [] for discoveryRepos, non-strings to null for discoveryLastRefresh, non-finite numbers to 0 for discoveryTotalCount).
🛡️ Sketch of per-key validation
discoveryRepos: (() => {
const persisted = (safePersisted as Record<string, unknown>).discoveryRepos;
- if (persisted && typeof persisted === 'object' && !Array.isArray(persisted)) {
- return {
- 'trending': [],
- 'hot-release': [],
- 'most-popular': [],
- 'topic': [],
- 'search': [],
- ...(persisted as Record<DiscoveryChannelId, DiscoveryRepo[]>),
- };
- }
- return { 'trending': [], 'hot-release': [], 'most-popular': [], 'topic': [], 'search': [] } as Record<DiscoveryChannelId, DiscoveryRepo[]>;
+ const base: Record<DiscoveryChannelId, DiscoveryRepo[]> = {
+ 'trending': [], 'hot-release': [], 'most-popular': [], 'topic': [], 'search': [],
+ };
+ if (persisted && typeof persisted === 'object' && !Array.isArray(persisted)) {
+ for (const key of Object.keys(base) as DiscoveryChannelId[]) {
+ const v = (persisted as Record<string, unknown>)[key];
+ if (Array.isArray(v)) base[key] = v as DiscoveryRepo[];
+ }
+ }
+ return base;
})(),(Apply the same pattern to discoveryLastRefresh and discoveryTotalCount.)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/useAppStore.ts` around lines 297 - 338, The persisted rehydration
for discoveryRepos, discoveryLastRefresh, and discoveryTotalCount in useAppStore
trusts the persisted object shape and can be overwritten by malformed
per-channel values; update each initializer (discoveryRepos,
discoveryLastRefresh, discoveryTotalCount) to iterate the known
DiscoveryChannelId keys and coerce/validate per-key values from safePersisted:
for discoveryRepos ensure each channel value is an array (coerce any non-array
to []), for discoveryLastRefresh ensure each channel value is a string or null
(coerce non-strings to null), and for discoveryTotalCount ensure each channel
value is a finite number (coerce non-finite/non-number to 0); merge only the
validated per-key entries over the defaults so downstream code never receives
malformed types.
Root causes fixed:
1. normalizePersistedState was missing defensive handling for 4 discovery
runtime state fields (discoveryIsLoading, discoveryHasMore, discoveryNextPage,
discoveryScrollPositions). If old persisted data had these fields in a wrong
format (e.g. array instead of object), spreading them in store actions like
{ ...state.discoveryIsLoading, [channel]: loading } would produce corrupted
state and cause React render errors → white screen.
2. migrate() function also lacked a reset for discoveryIsLoading /
discoveryScrollPositions, so old-format data survived into the merge step.
3. DiscoveryView had no local ErrorBoundary. Any render error inside it
propagated all the way to the root boundary (main.tsx), wiping the entire
UI. Wrapping it in a scoped ErrorBoundary now shows a recovery UI instead
of a blank page when switching to the discovery tab.
Changes:
- src/store/useAppStore.ts: add normalizePersistedState entries for
discoveryIsLoading (reset to false), discoveryHasMore (safe object merge),
discoveryNextPage (safe object merge), discoveryScrollPositions (reset to 0);
add migrate() block to reset discoveryIsLoading and discoveryScrollPositions.
- src/App.tsx: wrap <DiscoveryView /> in <ErrorBoundary> for scoped recovery.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/store/useAppStore.ts (1)
297-373:⚠️ Potential issue | 🟠 MajorValidate per-channel values before spreading persisted discovery maps.
The container check is good, but malformed per-key values still overwrite safe defaults. For example, a truthy non-array
discoveryRepos.trendingbypasses|| []inDiscoveryViewand violates the expected array contract; the same applies to non-string refresh timestamps, non-finite counts/pages, and non-booleanhasMore.🛡️ Sketch of safer per-key normalization
+ const discoveryChannelIds = ['trending', 'hot-release', 'most-popular', 'topic', 'search'] as const; + discoveryRepos: (() => { const persisted = (safePersisted as Record<string, unknown>).discoveryRepos; - if (persisted && typeof persisted === 'object' && !Array.isArray(persisted)) { - return { - 'trending': [], - 'hot-release': [], - 'most-popular': [], - 'topic': [], - 'search': [], - ...(persisted as Record<DiscoveryChannelId, DiscoveryRepo[]>), - }; + const base = { 'trending': [], 'hot-release': [], 'most-popular': [], 'topic': [], 'search': [] } as Record<DiscoveryChannelId, DiscoveryRepo[]>; + if (persisted && typeof persisted === 'object' && !Array.isArray(persisted)) { + for (const channel of discoveryChannelIds) { + const value = (persisted as Record<string, unknown>)[channel]; + if (Array.isArray(value)) base[channel] = value as DiscoveryRepo[]; + } } - return { 'trending': [], 'hot-release': [], 'most-popular': [], 'topic': [], 'search': [] } as Record<DiscoveryChannelId, DiscoveryRepo[]>; + return base; })(),Apply the same per-key pattern for:
discoveryLastRefresh: string ornull, otherwisenulldiscoveryTotalCount: finite number, otherwise0discoveryHasMore: boolean, otherwisefalsediscoveryNextPage: finite positive number, otherwise1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/useAppStore.ts` around lines 297 - 373, The persisted discovery maps (discoveryRepos, discoveryLastRefresh, discoveryTotalCount, discoveryHasMore, discoveryNextPage) currently spread persisted objects directly, which lets malformed per-channel values overwrite defaults; update each initializer to iterate the known channel keys and perform per-key normalization before merging: for discoveryRepos ensure each channel value is an array (fallback to []), for discoveryLastRefresh ensure each value is string or null (fallback null), for discoveryTotalCount and discoveryNextPage ensure finite (and positive for nextPage) numbers (fallback 0 and 1 respectively), and for discoveryHasMore ensure boolean (fallback false); implement this normalization in the respective IIFEs that reference safePersisted so the spread only adds validated values for keys that pass the checks.
🤖 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/store/useAppStore.ts`:
- Around line 197-206: PersistedAppState is missing discoveryChannels and
discoverySelectedTopic which causes type errors when migrate() reads
state.discoveryChannels and partialize() persists those fields; update the
PersistedAppState type definition to include discoveryChannels (array/type
matching how channels are represented elsewhere) and discoverySelectedTopic
(type matching the selected topic usage) so the type aligns with the migrate and
partialize logic.
---
Duplicate comments:
In `@src/store/useAppStore.ts`:
- Around line 297-373: The persisted discovery maps (discoveryRepos,
discoveryLastRefresh, discoveryTotalCount, discoveryHasMore, discoveryNextPage)
currently spread persisted objects directly, which lets malformed per-channel
values overwrite defaults; update each initializer to iterate the known channel
keys and perform per-key normalization before merging: for discoveryRepos ensure
each channel value is an array (fallback to []), for discoveryLastRefresh ensure
each value is string or null (fallback null), for discoveryTotalCount and
discoveryNextPage ensure finite (and positive for nextPage) numbers (fallback 0
and 1 respectively), and for discoveryHasMore ensure boolean (fallback false);
implement this normalization in the respective IIFEs that reference
safePersisted so the spread only adds validated values for keys that pass the
checks.
🪄 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: 21ab9f4a-5762-4af6-b3d0-614d77027f0e
📒 Files selected for processing (2)
src/App.tsxsrc/store/useAppStore.ts
Comment on lines
+197
to
+206
| | 'discoveryRepos' | ||
| | 'discoveryLastRefresh' | ||
| | 'discoveryTotalCount' | ||
| | 'discoveryHasMore' | ||
| | 'discoveryNextPage' | ||
| | 'selectedDiscoveryChannel' | ||
| | 'discoveryPlatform' | ||
| | 'discoveryLanguage' | ||
| | 'discoverySortBy' | ||
| | 'discoverySortOrder' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify persisted discovery fields are represented in PersistedAppState
# and show direct migration access sites.
python - <<'PY'
from pathlib import Path
text = Path("src/store/useAppStore.ts").read_text()
type_block = text[text.index("type PersistedAppState"):text.index("const normalizeNumberSet")]
for key in ["discoveryChannels", "discoverySelectedTopic"]:
print(f"{key} in PersistedAppState:", key in type_block)
for i, line in enumerate(text.splitlines(), start=1):
if "state.discoveryChannels" in line or "discoveryChannels:" in line or "discoverySelectedTopic:" in line:
print(f"{i}: {line}")
PYRepository: AmintaCCCP/GithubStarsManager
Length of output: 877
🏁 Script executed:
# Read the PersistedAppState type definition and migrate function
sed -n '170,220p' src/store/useAppStore.tsRepository: AmintaCCCP/GithubStarsManager
Length of output: 1375
🏁 Script executed:
# Check the migrate function signature and context around line 1303
sed -n '1295,1325p' src/store/useAppStore.tsRepository: AmintaCCCP/GithubStarsManager
Length of output: 1360
🏁 Script executed:
# Find where PersistedAppState is used as a type annotation in migrate
rg "migrate|PersistedAppState" src/store/useAppStore.ts -A 2 -B 2 | head -50Repository: AmintaCCCP/GithubStarsManager
Length of output: 1606
🏁 Script executed:
# Get exact context around the state declaration and first access
sed -n '1300,1310p' src/store/useAppStore.tsRepository: AmintaCCCP/GithubStarsManager
Length of output: 729
🏁 Script executed:
# Check if TypeScript strict mode is enabled in tsconfig
find . -name "tsconfig.json" -o -name "tsconfig*.json" | head -3 | xargs cat 2>/dev/null | grep -A 10 '"compilerOptions"'Repository: AmintaCCCP/GithubStarsManager
Length of output: 667
Add missing discovery fields to PersistedAppState type.
The migrate function accesses state.discoveryChannels (line 1303) and state.discoverySelectedTopic, while partialize persists both fields (lines 1249, 1260), but these fields are absent from the PersistedAppState type definition. With "strict": true enabled in tsconfig.json, this creates a type inconsistency. Update the type to include these fields:
| 'releaseSearchQuery'
+ | 'discoveryChannels'
| 'discoveryRepos'
| 'discoveryLastRefresh'
| 'discoveryTotalCount'
| 'discoveryHasMore'
| 'discoveryNextPage'
| 'selectedDiscoveryChannel'
| 'discoveryPlatform'
| 'discoveryLanguage'
| 'discoverySortBy'
| 'discoverySortOrder'
+ | 'discoverySelectedTopic'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/useAppStore.ts` around lines 197 - 206, PersistedAppState is
missing discoveryChannels and discoverySelectedTopic which causes type errors
when migrate() reads state.discoveryChannels and partialize() persists those
fields; update the PersistedAppState type definition to include
discoveryChannels (array/type matching how channels are represented elsewhere)
and discoverySelectedTopic (type matching the selected topic usage) so the type
aligns with the migrate and partialize logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/store/useAppStore.ts (2)
168-216:⚠️ Potential issue | 🔴 CriticalAdd the missing persisted discovery fields to
PersistedAppState.
partializepersistsdiscoveryChannelsanddiscoverySelectedTopic, andmigratereadsstate.discoveryChannels, but the picked persisted type still omits both fields. This can fail strict TypeScript builds.🔧 Proposed type alignment
| 'releaseSelectedFilters' | 'releaseSearchQuery' + | 'discoveryChannels' | 'discoveryRepos' | 'discoveryLastRefresh' | 'discoveryTotalCount' | 'discoveryHasMore' | 'discoveryNextPage' | 'selectedDiscoveryChannel' | 'discoveryPlatform' | 'discoveryLanguage' | 'discoverySortBy' | 'discoverySortOrder' + | 'discoverySelectedTopic' | 'subscriptionRepos'Also applies to: 1248-1265, 1304-1325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/useAppStore.ts` around lines 168 - 216, PersistedAppState omits persisted discovery fields (discoveryChannels and discoverySelectedTopic) even though partialize writes them and migrate reads state.discoveryChannels; update the PersistedAppState type (the Pick<> list or the intersection object) to include discoveryChannels and discoverySelectedTopic so the static type matches runtime persistence behavior, and mirror the same additions wherever PersistedAppState is duplicated (references around the file and the other noted ranges) to satisfy strict TypeScript builds.
297-372:⚠️ Potential issue | 🟠 MajorValidate discovery map entries per channel before merging persisted data.
The object-level guard still allows malformed per-channel values to overwrite defaults, e.g.
discoveryRepos.trending = {}ordiscoveryNextPage.search = "2". Those values can later hit.slice, spreads, or API pagination calls.🛡️ Proposed validation approach
discoveryRepos: (() => { const persisted = (safePersisted as Record<string, unknown>).discoveryRepos; - if (persisted && typeof persisted === 'object' && !Array.isArray(persisted)) { - return { - 'trending': [], - 'hot-release': [], - 'most-popular': [], - 'topic': [], - 'search': [], - ...(persisted as Record<DiscoveryChannelId, DiscoveryRepo[]>), - }; + const base: Record<DiscoveryChannelId, DiscoveryRepo[]> = { + 'trending': [], + 'hot-release': [], + 'most-popular': [], + 'topic': [], + 'search': [], + }; + if (persisted && typeof persisted === 'object' && !Array.isArray(persisted)) { + for (const key of Object.keys(base) as DiscoveryChannelId[]) { + const value = (persisted as Record<string, unknown>)[key]; + if (Array.isArray(value)) base[key] = value as DiscoveryRepo[]; + } } - return { 'trending': [], 'hot-release': [], 'most-popular': [], 'topic': [], 'search': [] } as Record<DiscoveryChannelId, DiscoveryRepo[]>; + return base; })(),Use the same pattern for
discoveryLastRefresh,discoveryTotalCount,discoveryHasMore, anddiscoveryNextPage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/store/useAppStore.ts` around lines 297 - 372, The persisted objects for discoveryRepos, discoveryLastRefresh, discoveryTotalCount, discoveryHasMore, and discoveryNextPage can contain per-channel values with the wrong type (e.g., discoveryRepos.trending = {} or discoveryNextPage.search = "2"); update each initializer (discoveryRepos, discoveryLastRefresh, discoveryTotalCount, discoveryHasMore, discoveryNextPage) to validate and coerce per-channel entries before merging: iterate the canonical channel list ['trending','hot-release','most-popular','topic','search'] and for each channel, if persisted hasOwnProperty(channel) and the value passes a type check for that map (array for discoveryRepos, string|null for discoveryLastRefresh, number for discoveryTotalCount and discoveryNextPage, boolean for discoveryHasMore) use the persisted value, otherwise keep the default; factor this logic into a small helper (e.g., normalizeDiscoveryMap) or inline the per-key checks so malformed per-channel values cannot overwrite defaults.
🤖 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/DiscoveryView.tsx`:
- Around line 723-737: The merge is currently using allRepos (from
selectedDiscoveryChannel) when building mergedRepos, which causes wrong or
dropped AI fields when refreshing a non-selected channel; update the lookup to
use the target channel's existing repo list (the channel whose result is being
processed in refreshChannel/refreshAll) instead of
allRepos/selectedDiscoveryChannel so existingRepo is found by matching
newRepo.id against that channel's repos; apply the same change to the other
occurrence around lines 944-949 (the second mergedRepos block) so both places
reference the channel-specific repo collection when copying ai_summary, ai_tags,
ai_platforms, analyzed_at, and analysis_failed.
- Around line 1271-1274: The JSX passes a desktopSafeMode prop to
SubscriptionRepoCard but SubscriptionRepoCardProps only defines repo, onStar,
and onAnalyze, causing a type error; either remove the desktopSafeMode prop
where SubscriptionRepoCard is rendered (in the map over currentPageRepos) or add
desktopSafeMode?: boolean to the SubscriptionRepoCardProps interface and
propagate it through the SubscriptionRepoCard component (update prop types and
any internal usage in SubscriptionRepoCard) so the prop is properly typed.
- Around line 1052-1055: The select's onChange currently casts empty string to
TopicCategory | null which can store '' into discoverySelectedTopic; change the
handler in DiscoveryView (the select using discoverySelectedTopic and
setDiscoverySelectedTopic) to check e.target.value === '' and call
setDiscoverySelectedTopic(null) for that case, otherwise cast and pass the
non-empty value as TopicCategory so the state never contains an invalid ''
value.
- Around line 927-932: handleSearch currently calls
setDiscoverySearchQuery(searchInput) then immediately calls
refreshChannel('search', 1, false), which can use stale discoverySearchQuery;
change handleSearch to pass the submitted searchInput into refreshChannel (use
it as the search query and reset to page 1) instead of relying on the async
state update, and update refreshChannel to prefer an explicit
options.searchQuery (e.g., use options.searchQuery ?? discoverySearchQuery) when
performing the 'search' branch so callers can supply the fresh search term
directly; reference functions: handleSearch, setDiscoverySearchQuery,
refreshChannel, and discoverySearchQuery.
- Around line 653-668: Move the const refreshChannel = useCallback(...)
declaration so it appears before the useEffect that references it (the effect
that resets currentPage/scroll and auto-loads), and ensure refreshChannel is
memoized with stable dependencies so it doesn't recreate often; then remove
refreshChannel from the effect's dependency array (keep only
selectedDiscoveryChannel) to avoid callback churn resetting pagination
mid-fetch. If refreshChannel must reference changing filters/repos, narrow its
dependency list or wrap those changing values with refs so refreshChannel
remains stable while still using up-to-date data.
- Around line 934-942: handlePageChange currently sets setCurrentPage but only
triggers a single refreshChannel call for currentNextPage, so jumping e.g. from
page 1 to page 10 leaves intermediate pages unloaded and currentPageRepos empty.
Update handlePageChange to compute how many page-loads are required for the
requested page (use ITEMS_PER_PAGE and allRepos.length to determine pages
already loaded vs pages needed) and then either loop triggering refreshChannel
repeatedly until enough pages are fetched (or until
currentHasMore/currentIsLoading stops you) or call a new refreshChannel variant
that accepts a target page to fetch directly; reference handlePageChange,
setCurrentPage, ITEMS_PER_PAGE, allRepos, currentHasMore, currentIsLoading,
currentNextPage, refreshChannel and selectedDiscoveryChannel when implementing
the loop or adding the direct-fetch API.
In `@src/store/useAppStore.ts`:
- Around line 277-295: The persisted discoveryChannels mapping currently spreads
arbitrary persisted fields onto defaultChannel (in the discoveryChannels
initializer where safePersisted is read and when reconstructing
state.discoveryChannels in the migrate branch), which can inject
non-string/non-primitive values into props; fix by sanitizing each
persistedChannel field before merging: for each expected key (id, name, nameEn,
description, icon, etc.) validate type and fallback to defaultChannel's value if
the persisted value is not the expected primitive (and coerce enabled to a
boolean), then merge only these sanitized fields into defaultChannel instead of
spreading persistedChannel wholesale; apply the same sanitization logic to the
migrate branch that rebuilds state.discoveryChannels so both code paths are
safe.
---
Duplicate comments:
In `@src/store/useAppStore.ts`:
- Around line 168-216: PersistedAppState omits persisted discovery fields
(discoveryChannels and discoverySelectedTopic) even though partialize writes
them and migrate reads state.discoveryChannels; update the PersistedAppState
type (the Pick<> list or the intersection object) to include discoveryChannels
and discoverySelectedTopic so the static type matches runtime persistence
behavior, and mirror the same additions wherever PersistedAppState is duplicated
(references around the file and the other noted ranges) to satisfy strict
TypeScript builds.
- Around line 297-372: The persisted objects for discoveryRepos,
discoveryLastRefresh, discoveryTotalCount, discoveryHasMore, and
discoveryNextPage can contain per-channel values with the wrong type (e.g.,
discoveryRepos.trending = {} or discoveryNextPage.search = "2"); update each
initializer (discoveryRepos, discoveryLastRefresh, discoveryTotalCount,
discoveryHasMore, discoveryNextPage) to validate and coerce per-channel entries
before merging: iterate the canonical channel list
['trending','hot-release','most-popular','topic','search'] and for each channel,
if persisted hasOwnProperty(channel) and the value passes a type check for that
map (array for discoveryRepos, string|null for discoveryLastRefresh, number for
discoveryTotalCount and discoveryNextPage, boolean for discoveryHasMore) use the
persisted value, otherwise keep the default; factor this logic into a small
helper (e.g., normalizeDiscoveryMap) or inline the per-key checks so malformed
per-channel values cannot overwrite defaults.
🪄 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: de05eb0e-ff56-4259-9db2-cc731fc501f5
📒 Files selected for processing (2)
src/components/DiscoveryView.tsxsrc/store/useAppStore.ts
Comment on lines
+653
to
+668
| // 切换频道时重置页码、恢复滚动位置，并自动加载空数据 | ||
| useEffect(() => { | ||
| setCurrentPage(1); | ||
| // 恢复当前频道的滚动位置（从 ref 读取最新值，避免订阅整个 map） | ||
| if (scrollContainerRef.current) { | ||
| const savedPosition = discoveryScrollPositionsRef.current[selectedDiscoveryChannel] || 0; | ||
| scrollContainerRef.current.scrollTop = savedPosition; | ||
| } | ||
|
|
||
| // 取消持久化后，首次打开或切换到空频道时自动加载 | ||
| const hasRepos = useAppStore.getState().discoveryRepos[selectedDiscoveryChannel]?.length > 0; | ||
| const isLoading = useAppStore.getState().discoveryIsLoading[selectedDiscoveryChannel]; | ||
| if (!hasRepos && !isLoading) { | ||
| refreshChannel(selectedDiscoveryChannel, 1, false); | ||
| } | ||
| }, [selectedDiscoveryChannel, refreshChannel]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm the channel-switch effect appears before refreshChannel declaration.
python - <<'PY'
from pathlib import Path
text = Path("src/components/DiscoveryView.tsx").read_text()
effect = text.index(" // 切换频道时重置页码")
callback = text.index(" const refreshChannel = useCallback")
print("effect_before_refreshChannel =", effect < callback)
PYRepository: AmintaCCCP/GithubStarsManager
Length of output: 108
Move refreshChannel before this effect and avoid callback churn resetting pagination.
Line 653's useEffect references refreshChannel in its dependency array (line 668), but refreshChannel is not declared until line 677. This causes a JavaScript Temporal Dead Zone ReferenceError at runtime.
Move the const refreshChannel = useCallback(...) declaration before this effect. Additionally, avoid adding deps to this effect that change when repos/filters change—callback churn can cause re-runs that reset currentPage back to 1 mid-fetch/append.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/DiscoveryView.tsx` around lines 653 - 668, Move the const
refreshChannel = useCallback(...) declaration so it appears before the useEffect
that references it (the effect that resets currentPage/scroll and auto-loads),
and ensure refreshChannel is memoized with stable dependencies so it doesn't
recreate often; then remove refreshChannel from the effect's dependency array
(keep only selectedDiscoveryChannel) to avoid callback churn resetting
pagination mid-fetch. If refreshChannel must reference changing filters/repos,
narrow its dependency list or wrap those changing values with refs so
refreshChannel remains stable while still using up-to-date data.
Comment on lines
+927
to
+932
| const handleSearch = useCallback(() => { | ||
| if (selectedDiscoveryChannel === 'search') { | ||
| setDiscoverySearchQuery(searchInput); | ||
| refreshChannel('search', 1, false); | ||
| } | ||
| }, [selectedDiscoveryChannel, searchInput, setDiscoverySearchQuery, refreshChannel]); |
Contributor
There was a problem hiding this comment.
Use the submitted search text for the refresh.
setDiscoverySearchQuery(searchInput) is asynchronous, so the immediate refreshChannel('search', ...) still reads the previous discoverySearchQuery from its closure. The first search can therefore query the old value, often an empty string. Also reset to page 1 for a new search.
🐛 Proposed fix sketch
- const handleSearch = useCallback(() => {
+ const handleSearch = useCallback(() => {
if (selectedDiscoveryChannel === 'search') {
- setDiscoverySearchQuery(searchInput);
- refreshChannel('search', 1, false);
+ const query = searchInput.trim();
+ setCurrentPage(1);
+ setDiscoverySearchQuery(query);
+ refreshChannel('search', 1, false, { searchQuery: query });
}
}, [selectedDiscoveryChannel, searchInput, setDiscoverySearchQuery, refreshChannel]);Then update refreshChannel to prefer options.searchQuery ?? discoverySearchQuery in the search branch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/DiscoveryView.tsx` around lines 927 - 932, handleSearch
currently calls setDiscoverySearchQuery(searchInput) then immediately calls
refreshChannel('search', 1, false), which can use stale discoverySearchQuery;
change handleSearch to pass the submitted searchInput into refreshChannel (use
it as the search query and reset to page 1) instead of relying on the async
state update, and update refreshChannel to prefer an explicit
options.searchQuery (e.g., use options.searchQuery ?? discoverySearchQuery) when
performing the 'search' branch so callers can supply the fresh search term
directly; reference functions: handleSearch, setDiscoverySearchQuery,
refreshChannel, and discoverySearchQuery.
Comment on lines
+934
to
+942
| const handlePageChange = useCallback((page: number) => { | ||
| setCurrentPage(page); | ||
|
|
||
| // 如果目标页的数据还没有加载，先加载数据 | ||
| const requiredItems = page * ITEMS_PER_PAGE; | ||
| if (allRepos.length < requiredItems && currentHasMore && !currentIsLoading) { | ||
| refreshChannel(selectedDiscoveryChannel, currentNextPage, true); | ||
| } | ||
| }, [allRepos.length, currentHasMore, currentIsLoading, currentNextPage, refreshChannel, selectedDiscoveryChannel]); |
Contributor
There was a problem hiding this comment.
Don’t jump to unloaded pages while only fetching one next page.
The page-jump controls can request page N, but handlePageChange only appends currentNextPage once. Jumping from page 1 to page 10 leaves currentPageRepos empty because pages 3–10 were never loaded.
🐛 Proposed guard
const handlePageChange = useCallback((page: number) => {
- setCurrentPage(page);
+ const loadedPages = Math.ceil(allRepos.length / ITEMS_PER_PAGE);
+ const targetPage = page > loadedPages + 1 ? loadedPages + 1 : page;
+ setCurrentPage(targetPage);
// 如果目标页的数据还没有加载，先加载数据
- const requiredItems = page * ITEMS_PER_PAGE;
+ const requiredItems = targetPage * ITEMS_PER_PAGE;
if (allRepos.length < requiredItems && currentHasMore && !currentIsLoading) {
refreshChannel(selectedDiscoveryChannel, currentNextPage, true);
}Alternatively, store repos by page and fetch the requested page directly instead of appending into a flat list.
📝 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.
Suggested change
| const handlePageChange = useCallback((page: number) => { | |
| setCurrentPage(page); | |
| // 如果目标页的数据还没有加载，先加载数据 | |
| const requiredItems = page * ITEMS_PER_PAGE; | |
| if (allRepos.length < requiredItems && currentHasMore && !currentIsLoading) { | |
| refreshChannel(selectedDiscoveryChannel, currentNextPage, true); | |
| } | |
| }, [allRepos.length, currentHasMore, currentIsLoading, currentNextPage, refreshChannel, selectedDiscoveryChannel]); | |
| const handlePageChange = useCallback((page: number) => { | |
| const loadedPages = Math.ceil(allRepos.length / ITEMS_PER_PAGE); | |
| const targetPage = page > loadedPages + 1 ? loadedPages + 1 : page; | |
| setCurrentPage(targetPage); | |
| // 如果目标页的数据还没有加载，先加载数据 | |
| const requiredItems = targetPage * ITEMS_PER_PAGE; | |
| if (allRepos.length < requiredItems && currentHasMore && !currentIsLoading) { | |
| refreshChannel(selectedDiscoveryChannel, currentNextPage, true); | |
| } | |
| }, [allRepos.length, currentHasMore, currentIsLoading, currentNextPage, refreshChannel, selectedDiscoveryChannel]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/DiscoveryView.tsx` around lines 934 - 942, handlePageChange
currently sets setCurrentPage but only triggers a single refreshChannel call for
currentNextPage, so jumping e.g. from page 1 to page 10 leaves intermediate
pages unloaded and currentPageRepos empty. Update handlePageChange to compute
how many page-loads are required for the requested page (use ITEMS_PER_PAGE and
allRepos.length to determine pages already loaded vs pages needed) and then
either loop triggering refreshChannel repeatedly until enough pages are fetched
(or until currentHasMore/currentIsLoading stops you) or call a new
refreshChannel variant that accepts a target page to fetch directly; reference
handlePageChange, setCurrentPage, ITEMS_PER_PAGE, allRepos, currentHasMore,
currentIsLoading, currentNextPage, refreshChannel and selectedDiscoveryChannel
when implementing the loop or adding the direct-fetch API.
Comment on lines
+1052
to
+1055
| <select | ||
| value={discoverySelectedTopic || ''} | ||
| onChange={(e) => setDiscoverySelectedTopic(e.target.value as TopicCategory | null)} | ||
| className="px-2.5 py-1.5 rounded-lg text-xs font-medium bg-gray-100/80 text-gray-700 dark:bg-gray-700/80 dark:text-gray-300 border border-gray-200 dark:border-gray-600 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" |
Contributor
There was a problem hiding this comment.
Convert the empty topic option back to null.
The current cast stores '' in a TopicCategory | null field, which defeats the type contract and can persist an invalid topic value.
🧹 Proposed fix
- onChange={(e) => setDiscoverySelectedTopic(e.target.value as TopicCategory | null)}
+ onChange={(e) => {
+ const value = e.target.value;
+ setDiscoverySelectedTopic(value ? value as TopicCategory : null);
+ }}📝 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.
Suggested change
| <select | |
| value={discoverySelectedTopic || ''} | |
| onChange={(e) => setDiscoverySelectedTopic(e.target.value as TopicCategory | null)} | |
| className="px-2.5 py-1.5 rounded-lg text-xs font-medium bg-gray-100/80 text-gray-700 dark:bg-gray-700/80 dark:text-gray-300 border border-gray-200 dark:border-gray-600 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | |
| <select | |
| value={discoverySelectedTopic || ''} | |
| onChange={(e) => { | |
| const value = e.target.value; | |
| setDiscoverySelectedTopic(value ? value as TopicCategory : null); | |
| }} | |
| className="px-2.5 py-1.5 rounded-lg text-xs font-medium bg-gray-100/80 text-gray-700 dark:bg-gray-700/80 dark:text-gray-300 border border-gray-200 dark:border-gray-600 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/DiscoveryView.tsx` around lines 1052 - 1055, The select's
onChange currently casts empty string to TopicCategory | null which can store ''
into discoverySelectedTopic; change the handler in DiscoveryView (the select
using discoverySelectedTopic and setDiscoverySelectedTopic) to check
e.target.value === '' and call setDiscoverySelectedTopic(null) for that case,
otherwise cast and pass the non-empty value as TopicCategory so the state never
contains an invalid '' value.
Comment on lines
+1271
to
+1274
| <div className={isDesktopSafeMode ? 'space-y-3' : 'space-y-4'}> | ||
| {currentPageRepos.map(repo => ( | ||
| <SubscriptionRepoCard key={repo.id} repo={repo} desktopSafeMode={isDesktopSafeMode} /> | ||
| ))} |
Contributor
There was a problem hiding this comment.
Remove or type the desktopSafeMode prop.
The provided SubscriptionRepoCardProps only accepts repo, onStar, and onAnalyze, so this extra prop will fail TypeScript checking unless the card interface is updated too.
🔧 Proposed fix if the prop is not needed
- <SubscriptionRepoCard key={repo.id} repo={repo} desktopSafeMode={isDesktopSafeMode} />
+ <SubscriptionRepoCard key={repo.id} repo={repo} />📝 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.
Suggested change
| <div className={isDesktopSafeMode ? 'space-y-3' : 'space-y-4'}> | |
| {currentPageRepos.map(repo => ( | |
| <SubscriptionRepoCard key={repo.id} repo={repo} desktopSafeMode={isDesktopSafeMode} /> | |
| ))} | |
| <div className={isDesktopSafeMode ? 'space-y-3' : 'space-y-4'}> | |
| {currentPageRepos.map(repo => ( | |
| <SubscriptionRepoCard key={repo.id} repo={repo} /> | |
| ))} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/DiscoveryView.tsx` around lines 1271 - 1274, The JSX passes a
desktopSafeMode prop to SubscriptionRepoCard but SubscriptionRepoCardProps only
defines repo, onStar, and onAnalyze, causing a type error; either remove the
desktopSafeMode prop where SubscriptionRepoCard is rendered (in the map over
currentPageRepos) or add desktopSafeMode?: boolean to the
SubscriptionRepoCardProps interface and propagate it through the
SubscriptionRepoCard component (update prop types and any internal usage in
SubscriptionRepoCard) so the prop is properly typed.
Comment on lines
+277
to
+295
| discoveryChannels: (() => { | ||
| const persisted = (safePersisted as Record<string, unknown>).discoveryChannels; | ||
| if (!Array.isArray(persisted)) return defaultDiscoveryChannels; | ||
|
|
||
| return defaultDiscoveryChannels.map((defaultChannel) => { | ||
| const persistedChannel = persisted.find((channel: unknown) => { | ||
| return (channel as Record<string, unknown>)?.id === defaultChannel.id; | ||
| }) as Record<string, unknown> | undefined; | ||
|
|
||
| if (!persistedChannel) { | ||
| return defaultChannel; | ||
| } | ||
|
|
||
| return { | ||
| ...defaultChannel, | ||
| ...(persistedChannel as Partial<typeof defaultChannel>), | ||
| enabled: persistedChannel.enabled !== false, | ||
| }; | ||
| }); |
Contributor
There was a problem hiding this comment.
Sanitize persisted discovery channel fields before rendering them.
The code validates channel IDs but then spreads arbitrary persisted fields over defaults. A malformed persisted value like name: {} or nameEn: [] can still reach JSX and white-screen with “Objects are not valid as a React child.”
🛡️ Proposed sanitization pattern
return defaultDiscoveryChannels.map((defaultChannel) => {
const persistedChannel = persisted.find((channel: unknown) => {
return (channel as Record<string, unknown>)?.id === defaultChannel.id;
}) as Record<string, unknown> | undefined;
if (!persistedChannel) {
return defaultChannel;
}
return {
...defaultChannel,
- ...(persistedChannel as Partial<typeof defaultChannel>),
+ name: typeof persistedChannel.name === 'string' ? persistedChannel.name : defaultChannel.name,
+ nameEn: typeof persistedChannel.nameEn === 'string' ? persistedChannel.nameEn : defaultChannel.nameEn,
+ description: typeof persistedChannel.description === 'string' ? persistedChannel.description : defaultChannel.description,
enabled: persistedChannel.enabled !== false,
};
});Apply the same sanitization in the migrate branch that rebuilds state.discoveryChannels.
Also applies to: 1307-1325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/store/useAppStore.ts` around lines 277 - 295, The persisted
discoveryChannels mapping currently spreads arbitrary persisted fields onto
defaultChannel (in the discoveryChannels initializer where safePersisted is read
and when reconstructing state.discoveryChannels in the migrate branch), which
can inject non-string/non-primitive values into props; fix by sanitizing each
persistedChannel field before merging: for each expected key (id, name, nameEn,
description, icon, etc.) validate type and fallback to defaultChannel's value if
the persisted value is not the expected primitive (and coerce enabled to a
boolean), then merge only these sanitized fields into defaultChannel instead of
spreading persistedChannel wholesale; apply the same sanitization logic to the
migrate branch that rebuilds state.discoveryChannels so both code paths are
safe.
…8 memory crashes on tab switch
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/SubscriptionRepoCard.tsx (1)
29-29:useCallbackmemoizations are defeated by unstablet.
tis re-created on every render (line 29) and is listed as a dependency forexecuteUnstar(line 133),handleStar(line 191), andhandleAnalyze(line 267). As a result, all three callbacks are recomputed every render and theuseCallbackwrappers don't actually stabilize any identity passed to children or effects. Either wraptwithuseCallback(depending only onlanguage) or readlanguageinside the handlers and droptfrom the dep arrays.♻️ Proposed refactor
- const t = (zh: string, en: string) => language === 'zh' ? zh : en; + const t = useCallback( + (zh: string, en: string) => (language === 'zh' ? zh : en), + [language] + );Also applies to: 133-133, 191-191, 267-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/SubscriptionRepoCard.tsx` at line 29, The inline translator helper t is recreated every render which defeats memoization of callbacks; change t into a stable function by wrapping it with useCallback dependent only on language (i.e. replace the inline const t = (...) with a useCallback that returns zh or en based on language) so executeUnstar, handleStar, and handleAnalyze can keep t in their dependency arrays without causing re-creation, and ensure useCallback is imported if not already; alternatively, if you prefer not to change t, read language directly inside executeUnstar/handleStar/handleAnalyze and remove t from those deps.
🤖 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/SubscriptionRepoCard.tsx`:
- Around line 99-133: executeUnstar and handleStar use repo.full_name.split('/')
without guarding for missing full_name which can throw and bypass proper error
handling; modify both executeUnstar and handleStar to defensively derive owner
and name (e.g., use repo.full_name ? repo.full_name.split('/') :
[repo.owner?.login || '', repo.name || '']) before calling GitHubApiService, and
if owner or name are falsy, handle early by resetting state (call
setIsStarring(false), setOptimisticStarred(null), setPendingUnstarAction(null)
as relevant), logging/alerting the user, and returning early so
deleteRepository, forceSyncToBackend, and API calls are only invoked with valid
values. Ensure references to repo.full_name are replaced with the guarded
variables in both functions.
- Around line 358-366: The GitHub anchor in the SubscriptionRepoCard component
is missing an event stop so clicks bubble up and trigger handleCardClick
(opening the Readme modal); add an onClick handler to the <a> that calls
e.stopPropagation() (e.g. onClick={(e) => e.stopPropagation()}) so the
ExternalLink anchor does not trigger handleCardClick and behaves consistently
with the Analyze/ZRead/Star buttons in SubscriptionRepoCard.
---
Nitpick comments:
In `@src/components/SubscriptionRepoCard.tsx`:
- Line 29: The inline translator helper t is recreated every render which
defeats memoization of callbacks; change t into a stable function by wrapping it
with useCallback dependent only on language (i.e. replace the inline const t =
(...) with a useCallback that returns zh or en based on language) so
executeUnstar, handleStar, and handleAnalyze can keep t in their dependency
arrays without causing re-creation, and ensure useCallback is imported if not
already; alternatively, if you prefer not to change t, read language directly
inside executeUnstar/handleStar/handleAnalyze and remove t from those deps.
🪄 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: edc109e0-863b-421f-8f28-a520e87a9b1d
📒 Files selected for processing (2)
src/components/SubscriptionRepoCard.tsxsrc/store/useAppStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/store/useAppStore.ts
Comment on lines
+99
to
+133
| const executeUnstar = useCallback(async () => { | ||
| if (!githubToken) return; | ||
|
|
||
| setIsStarring(true); | ||
|
|
||
| try { | ||
| const githubApi = new GitHubApiService(githubToken); | ||
| const [owner, name] = repo.full_name.split('/'); | ||
|
|
||
| // 乐观更新：立即更新UI状态 | ||
| setOptimisticStarred(false); | ||
|
|
||
| await githubApi.unstarRepository(owner, name); | ||
|
|
||
| // 从本地删除 | ||
| const existingRepo = repositories.find(r => r.full_name === repo.full_name); | ||
| if (existingRepo) { | ||
| deleteRepository(existingRepo.id); | ||
| } | ||
|
|
||
| await forceSyncToBackend(); | ||
|
|
||
| // 操作成功，清除乐观状态 | ||
| setOptimisticStarred(null); | ||
| } catch (error) { | ||
| // 操作失败，回滚乐观状态 | ||
| setOptimisticStarred(null); | ||
| console.error('Failed to unstar repository:', error); | ||
| const errorMessage = t('取消 Star 失败，请检查网络连接或 GitHub Token 权限。', 'Failed to unstar repository. Please check your network connection or GitHub Token permissions.'); | ||
| alert(errorMessage); | ||
| } finally { | ||
| setIsStarring(false); | ||
| setPendingUnstarAction(null); | ||
| } | ||
| }, [githubToken, repo, repositories, deleteRepository, t]); |
Contributor
There was a problem hiding this comment.
Unsafe repo.full_name.split('/') — guard against missing full_name.
cardTitle on line 279 has an explicit fallback (${repo.owner?.login || ''}/${repo.name || ''}), implying repo.full_name can be absent. Both executeUnstar (line 106) and handleStar (line 152) call repo.full_name.split('/') unguarded and will throw a TypeError if that's ever the case — leaving isStarring=true locked on (no finally reset path executes before the throw in handleStar since it happens before the try... wait, it's inside the try, so the finally will reset isStarring, but the GitHub API call is skipped silently and the user gets no feedback). Derive owner/name defensively:
🛡️ Proposed fix
- const githubApi = new GitHubApiService(githubToken);
- const [owner, name] = repo.full_name.split('/');
+ const githubApi = new GitHubApiService(githubToken);
+ const owner = repo.owner?.login;
+ const name = repo.name ?? repo.full_name?.split('/')[1];
+ if (!owner || !name) {
+ throw new Error('Invalid repository identifier');
+ }Apply the same guard in executeUnstar.
Also applies to: 136-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/SubscriptionRepoCard.tsx` around lines 99 - 133, executeUnstar
and handleStar use repo.full_name.split('/') without guarding for missing
full_name which can throw and bypass proper error handling; modify both
executeUnstar and handleStar to defensively derive owner and name (e.g., use
repo.full_name ? repo.full_name.split('/') : [repo.owner?.login || '', repo.name
|| '']) before calling GitHubApiService, and if owner or name are falsy, handle
early by resetting state (call setIsStarring(false), setOptimisticStarred(null),
setPendingUnstarAction(null) as relevant), logging/alerting the user, and
returning early so deleteRepository, forceSyncToBackend, and API calls are only
invoked with valid values. Ensure references to repo.full_name are replaced with
the guarded variables in both functions.
Comment on lines
358
to
366
| <a | ||
| href={repo.html_url} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="flex-shrink-0 text-gray-400 hover:text-blue-500" | ||
| className="flex items-center justify-center w-8 h-8 rounded-lg bg-gray-100 text-gray-500 dark:bg-gray-700 dark:text-gray-400 hover:bg-gray-200 dark:hover:bg-gray-600 transition-colors" | ||
| title={t('在GitHub打开', 'Open on GitHub')} | ||
| > | ||
| <ExternalLink className="w-3.5 h-3.5" /> | ||
| <ExternalLink className="w-4 h-4" /> | ||
| </a> |
Contributor
There was a problem hiding this comment.
GitHub link click bubbles and also opens the README modal.
Unlike the Analyze/ZRead/Star buttons, the <a> on lines 358-366 does not call e.stopPropagation(). In non-desktopSafeMode, the parent div's onClick={handleCardClick} will still fire when the link is clicked, causing the ReadmeModal to open behind the newly opened GitHub tab. Stop propagation here for consistency with the other action buttons.
🐛 Proposed fix
<a
href={repo.html_url}
target="_blank"
rel="noopener noreferrer"
+ onClick={(e) => e.stopPropagation()}
className="flex items-center justify-center w-8 h-8 rounded-lg bg-gray-100 text-gray-500 dark:bg-gray-700 dark:text-gray-400 hover:bg-gray-200 dark:hover:bg-gray-600 transition-colors"
title={t('在GitHub打开', 'Open on GitHub')}
>
<ExternalLink className="w-4 h-4" />
</a>📝 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.
Suggested change
| <a | |
| href={repo.html_url} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| className="flex-shrink-0 text-gray-400 hover:text-blue-500" | |
| className="flex items-center justify-center w-8 h-8 rounded-lg bg-gray-100 text-gray-500 dark:bg-gray-700 dark:text-gray-400 hover:bg-gray-200 dark:hover:bg-gray-600 transition-colors" | |
| title={t('在GitHub打开', 'Open on GitHub')} | |
| > | |
| <ExternalLink className="w-3.5 h-3.5" /> | |
| <ExternalLink className="w-4 h-4" /> | |
| </a> | |
| <a | |
| href={repo.html_url} | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| onClick={(e) => e.stopPropagation()} | |
| className="flex items-center justify-center w-8 h-8 rounded-lg bg-gray-100 text-gray-500 dark:bg-gray-700 dark:text-gray-400 hover:bg-gray-200 dark:hover:bg-gray-600 transition-colors" | |
| title={t('在GitHub打开', 'Open on GitHub')} | |
| > | |
| <ExternalLink className="w-4 h-4" /> | |
| </a> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/SubscriptionRepoCard.tsx` around lines 358 - 366, The GitHub
anchor in the SubscriptionRepoCard component is missing an event stop so clicks
bubble up and trigger handleCardClick (opening the Readme modal); add an onClick
handler to the <a> that calls e.stopPropagation() (e.g. onClick={(e) =>
e.stopPropagation()}) so the ExternalLink anchor does not trigger
handleCardClick and behaves consistently with the Analyze/ZRead/Star buttons in
SubscriptionRepoCard.
…DZ reference error during production build initialization
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
src/components/DiscoveryView.tsx (5)
701-735:⚠️ Potential issue | 🟠 MajorMerge AI metadata from the channel being refreshed.
allReposbelongs toselectedDiscoveryChannel, butrefreshAll()can refresh other channels. Read the target channel’s repos instead; this also lets you dropallReposfromrefreshChanneldeps.🐛 Proposed fix
// 合并AI分析结果（如果仓库之前被分析过） + const existingChannelRepos = useAppStore.getState().discoveryRepos[channelId] || []; const mergedRepos = result.repos.map((newRepo: DiscoveryRepo) => { - const existingRepo = allRepos.find(r => r.id === newRepo.id); + const existingRepo = existingChannelRepos.find(r => r.id === newRepo.id); if (existingRepo && existingRepo.analyzed_at) { return { ...newRepo, @@ - }, [githubToken, t, setDiscoveryLoading, setDiscoveryRepos, setDiscoveryLastRefresh, discoveryPlatform, discoveryLanguage, discoverySortBy, discoverySortOrder, discoverySearchQuery, discoverySelectedTopic, setDiscoveryHasMore, setDiscoveryNextPage, setDiscoveryTotalCount, appendDiscoveryRepos, allRepos]); + }, [githubToken, t, setDiscoveryLoading, setDiscoveryRepos, setDiscoveryLastRefresh, discoveryPlatform, discoveryLanguage, discoverySortBy, discoverySortOrder, discoverySearchQuery, discoverySelectedTopic, setDiscoveryHasMore, setDiscoveryNextPage, setDiscoveryTotalCount, appendDiscoveryRepos]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DiscoveryView.tsx` around lines 701 - 735, The merge step in refreshChannel uses allRepos (from selectedDiscoveryChannel) which is wrong when refreshAll refreshes other channels; change the merge to read the target channel's repos (e.g., getDiscoveryRepos or discovery state keyed by channelId) instead of allRepos when computing mergedRepos from result.repos, so AI metadata (ai_summary/ai_tags/ai_platforms/analyzed_at/analysis_failed) are copied from the target channel's repo list; update refreshChannel/refreshAll to call appendDiscoveryRepos/setDiscoveryRepos with mergedRepos as before and remove allRepos from the refreshChannel dependency array since it is no longer needed.
1054-1057:⚠️ Potential issue | 🟡 MinorConvert the empty topic option back to
null.The empty option currently stores
''in aTopicCategory | nullfield.🧹 Proposed fix
- onChange={(e) => setDiscoverySelectedTopic(e.target.value as TopicCategory | null)} + onChange={(e) => { + const value = e.target.value; + setDiscoverySelectedTopic(value ? value as TopicCategory : null); + }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DiscoveryView.tsx` around lines 1054 - 1057, The select's empty option currently stores an empty string into discoverySelectedTopic (typed TopicCategory | null); update the select binding and change handler so the value uses discoverySelectedTopic ?? '' and the onChange maps an empty string to null before calling setDiscoverySelectedTopic (i.e., if e.target.value === '' then setDiscoverySelectedTopic(null) else setDiscoverySelectedTopic(e.target.value as TopicCategory)), referencing the select element, discoverySelectedTopic, setDiscoverySelectedTopic, and TopicCategory.
737-752:⚠️ Potential issue | 🟠 MajorKeep this effect tied to channel changes only.
Because
refreshChannelis recreated when repo/filter/search state changes, this effect can re-run after loads/appends and resetcurrentPageback to 1 without a channel switch.🐛 Proposed direction
- }, [selectedDiscoveryChannel, refreshChannel]); + }, [selectedDiscoveryChannel]);If exhaustive-deps is enforced, keep the latest
refreshChannelin a ref and call that ref from this channel-switch-only effect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DiscoveryView.tsx` around lines 737 - 752, The effect that resets pagination and restores scroll is currently dependent on refreshChannel which causes unwanted reruns; change it to depend only on selectedDiscoveryChannel, and introduce a mutable ref (e.g., refreshChannelRef) that you update whenever the refreshChannel function changes so the effect calls refreshChannelRef.current(selectedDiscoveryChannel, 1, false) instead of refreshChannel directly; keep the existing logic (setCurrentPage, scrollContainerRef, discoveryScrollPositionsRef and the useAppStore.getState checks) inside the effect but remove refreshChannel from the dependency array to ensure it only runs on channel switches.
655-695:⚠️ Potential issue | 🟠 MajorUse the submitted search text for the refresh.
setDiscoverySearchQuery(searchInput)is async, so the immediaterefreshChannel()can search with the previous query. Pass the trimmed query directly and reset to page 1.🐛 Proposed fix sketch
- const refreshChannel = useCallback(async (channelId: DiscoveryChannelId, page: number = 1, append: boolean = false) => { + const refreshChannel = useCallback(async ( + channelId: DiscoveryChannelId, + page: number = 1, + append: boolean = false, + options?: { searchQuery?: string } + ) => { @@ case 'search': - if (discoverySearchQuery.trim()) { + const query = (options?.searchQuery ?? discoverySearchQuery).trim(); + if (query) { result = await githubApi.searchRepositories( - discoverySearchQuery, + query, discoveryPlatform, discoveryLanguage, discoverySortBy, @@ const handleSearch = useCallback(() => { if (selectedDiscoveryChannel === 'search') { - setDiscoverySearchQuery(searchInput); - refreshChannel('search', 1, false); + const query = searchInput.trim(); + setCurrentPage(1); + setDiscoverySearchQuery(query); + refreshChannel('search', 1, false, { searchQuery: query }); } }, [selectedDiscoveryChannel, searchInput, setDiscoverySearchQuery, refreshChannel]);Also applies to: 929-934
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DiscoveryView.tsx` around lines 655 - 695, The refresh currently reads discoverySearchQuery after calling setDiscoverySearchQuery(searchInput) which is async; change the search flow so the submitted trimmed query is passed directly to refreshChannel instead of relying on state—e.g., trim the input, call setDiscoverySearchQuery(trimmed) and then call refreshChannel('search', 1, false) with the trimmed string (or modify refreshChannel signature to accept an explicit query param) so the immediate refresh uses the new text and resets to page 1; update both the search submit handler and the related invocation around refreshChannel (affecting refreshChannel, setDiscoverySearchQuery, and discoverySearchQuery usage at the other noted call sites).
936-944:⚠️ Potential issue | 🟠 MajorDon’t jump beyond the loaded page window.
This only fetches one next page, so jumping from page 1 to page 10 can set
currentPageto an unloaded slice.🐛 Proposed guard
const handlePageChange = useCallback((page: number) => { - setCurrentPage(page); + const loadedPages = Math.ceil(allRepos.length / ITEMS_PER_PAGE); + const targetPage = page > loadedPages + 1 ? loadedPages + 1 : page; + setCurrentPage(targetPage); // 如果目标页的数据还没有加载，先加载数据 - const requiredItems = page * ITEMS_PER_PAGE; + const requiredItems = targetPage * ITEMS_PER_PAGE; if (allRepos.length < requiredItems && currentHasMore && !currentIsLoading) { refreshChannel(selectedDiscoveryChannel, currentNextPage, true); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/DiscoveryView.tsx` around lines 936 - 944, handlePageChange currently sets setCurrentPage immediately and only triggers a single next-page fetch, allowing a jump (e.g., page 1 -> 10) to point to an unloaded slice; change it so you do not setCurrentPage beyond what is already loaded: compute maxPageLoaded = Math.ceil(allRepos.length / ITEMS_PER_PAGE) (minimum 1), and if requested page > maxPageLoaded and currentHasMore && !currentIsLoading, call refreshChannel(selectedDiscoveryChannel, currentNextPage, true) to load more but do not setCurrentPage to the requested page yet (either keep the old page or set to maxPageLoaded); once additional data arrives (where allRepos.length >= page * ITEMS_PER_PAGE) update currentPage to the requested page—use the same symbols handlePageChange, setCurrentPage, ITEMS_PER_PAGE, allRepos, currentHasMore, currentIsLoading, refreshChannel, currentNextPage, selectedDiscoveryChannel to locate code to implement this guard-and-defer logic.
🤖 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/DiscoveryView.tsx`:
- Around line 754-759: When topic changes while selectedDiscoveryChannel ===
'topic', currentPage isn't reset so the view can show an empty slice; update the
effect that calls refreshChannel (the useEffect watching discoverySelectedTopic
and selectedDiscoveryChannel) to also set currentPage to 1 before/when calling
refreshChannel (e.g. call the state setter for currentPage or invoke whatever
pagination state update is used) so the component and refreshChannel use page 1
consistently when discoverySelectedTopic changes.
- Around line 1166-1217: The issue: changing filters
(language/sort/order/platform) doesn't reset pagination, leaving the page index
stale; fix by updating each select's onChange handler (the handlers that call
setDiscoveryLanguage, setDiscoverySortBy, setDiscoverySortOrder,
setDiscoveryPlatform) to also reset the discovery page state (call
setDiscoveryPage(1) or the pagination setter used in this component) so any
filter change returns results from page 1; apply this to all four select
elements (language, sortBy, sortOrder, platform).
- Around line 872-895: The current result handling overwrites existing discovery
metadata (rank, channel, platform) when constructing updatedRepo/failedRepo;
change the logic in the result callback that builds DiscoveryRepo objects (the
branches that call updateDiscoveryRepo) to preserve existing result.repo.rank,
result.repo.channel, and result.repo.platform when they exist instead of
unconditionally setting rank: 0, channel: selectedDiscoveryChannel, platform:
discoveryPlatform; only set/overwrite AI-specific fields (ai_summary, ai_tags,
ai_platforms), analyzed_at, and analysis_failed, and fall back to the new values
only if the original repo lacks those properties.
---
Duplicate comments:
In `@src/components/DiscoveryView.tsx`:
- Around line 701-735: The merge step in refreshChannel uses allRepos (from
selectedDiscoveryChannel) which is wrong when refreshAll refreshes other
channels; change the merge to read the target channel's repos (e.g.,
getDiscoveryRepos or discovery state keyed by channelId) instead of allRepos
when computing mergedRepos from result.repos, so AI metadata
(ai_summary/ai_tags/ai_platforms/analyzed_at/analysis_failed) are copied from
the target channel's repo list; update refreshChannel/refreshAll to call
appendDiscoveryRepos/setDiscoveryRepos with mergedRepos as before and remove
allRepos from the refreshChannel dependency array since it is no longer needed.
- Around line 1054-1057: The select's empty option currently stores an empty
string into discoverySelectedTopic (typed TopicCategory | null); update the
select binding and change handler so the value uses discoverySelectedTopic ?? ''
and the onChange maps an empty string to null before calling
setDiscoverySelectedTopic (i.e., if e.target.value === '' then
setDiscoverySelectedTopic(null) else setDiscoverySelectedTopic(e.target.value as
TopicCategory)), referencing the select element, discoverySelectedTopic,
setDiscoverySelectedTopic, and TopicCategory.
- Around line 737-752: The effect that resets pagination and restores scroll is
currently dependent on refreshChannel which causes unwanted reruns; change it to
depend only on selectedDiscoveryChannel, and introduce a mutable ref (e.g.,
refreshChannelRef) that you update whenever the refreshChannel function changes
so the effect calls refreshChannelRef.current(selectedDiscoveryChannel, 1,
false) instead of refreshChannel directly; keep the existing logic
(setCurrentPage, scrollContainerRef, discoveryScrollPositionsRef and the
useAppStore.getState checks) inside the effect but remove refreshChannel from
the dependency array to ensure it only runs on channel switches.
- Around line 655-695: The refresh currently reads discoverySearchQuery after
calling setDiscoverySearchQuery(searchInput) which is async; change the search
flow so the submitted trimmed query is passed directly to refreshChannel instead
of relying on state—e.g., trim the input, call setDiscoverySearchQuery(trimmed)
and then call refreshChannel('search', 1, false) with the trimmed string (or
modify refreshChannel signature to accept an explicit query param) so the
immediate refresh uses the new text and resets to page 1; update both the search
submit handler and the related invocation around refreshChannel (affecting
refreshChannel, setDiscoverySearchQuery, and discoverySearchQuery usage at the
other noted call sites).
- Around line 936-944: handlePageChange currently sets setCurrentPage
immediately and only triggers a single next-page fetch, allowing a jump (e.g.,
page 1 -> 10) to point to an unloaded slice; change it so you do not
setCurrentPage beyond what is already loaded: compute maxPageLoaded =
Math.ceil(allRepos.length / ITEMS_PER_PAGE) (minimum 1), and if requested page >
maxPageLoaded and currentHasMore && !currentIsLoading, call
refreshChannel(selectedDiscoveryChannel, currentNextPage, true) to load more but
do not setCurrentPage to the requested page yet (either keep the old page or set
to maxPageLoaded); once additional data arrives (where allRepos.length >= page *
ITEMS_PER_PAGE) update currentPage to the requested page—use the same symbols
handlePageChange, setCurrentPage, ITEMS_PER_PAGE, allRepos, currentHasMore,
currentIsLoading, refreshChannel, currentNextPage, selectedDiscoveryChannel to
locate code to implement this guard-and-defer logic.
🪄 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: 6336d821-b36a-40d8-bbfb-590c14d9b009
📒 Files selected for processing (1)
src/components/DiscoveryView.tsx
Comment on lines
+754
to
+759
| // 主题改变时刷新数据 | ||
| useEffect(() => { | ||
| if (selectedDiscoveryChannel === 'topic' && discoverySelectedTopic) { | ||
| refreshChannel('topic', 1, false); | ||
| } | ||
| }, [discoverySelectedTopic, selectedDiscoveryChannel]); |
Contributor
There was a problem hiding this comment.
Reset pagination when the topic changes.
Changing topic fetches page 1 but leaves currentPage unchanged, so switching topics from page 2+ can render an empty slice.
🐛 Proposed fix
useEffect(() => {
if (selectedDiscoveryChannel === 'topic' && discoverySelectedTopic) {
+ setCurrentPage(1);
refreshChannel('topic', 1, false);
}
}, [discoverySelectedTopic, selectedDiscoveryChannel]);📝 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.
Suggested change
| // 主题改变时刷新数据 | |
| useEffect(() => { | |
| if (selectedDiscoveryChannel === 'topic' && discoverySelectedTopic) { | |
| refreshChannel('topic', 1, false); | |
| } | |
| }, [discoverySelectedTopic, selectedDiscoveryChannel]); | |
| // 主题改变时刷新数据 | |
| useEffect(() => { | |
| if (selectedDiscoveryChannel === 'topic' && discoverySelectedTopic) { | |
| setCurrentPage(1); | |
| refreshChannel('topic', 1, false); | |
| } | |
| }, [discoverySelectedTopic, selectedDiscoveryChannel]); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/DiscoveryView.tsx` around lines 754 - 759, When topic changes
while selectedDiscoveryChannel === 'topic', currentPage isn't reset so the view
can show an empty slice; update the effect that calls refreshChannel (the
useEffect watching discoverySelectedTopic and selectedDiscoveryChannel) to also
set currentPage to 1 before/when calling refreshChannel (e.g. call the state
setter for currentPage or invoke whatever pagination state update is used) so
the component and refreshChannel use page 1 consistently when
discoverySelectedTopic changes.
Comment on lines
+872
to
+895
| (result) => { | ||
| if (result.success && result.repo) { | ||
| const updatedRepo: DiscoveryRepo = { | ||
| ...result.repo, | ||
| rank: 0, | ||
| channel: selectedDiscoveryChannel, | ||
| platform: discoveryPlatform, | ||
| ai_summary: result.summary, | ||
| ai_tags: result.tags, | ||
| ai_platforms: result.platforms, | ||
| analyzed_at: new Date().toISOString(), | ||
| analysis_failed: false, | ||
| }; | ||
| updateDiscoveryRepo(updatedRepo); | ||
| } else if (!result.success && result.repo) { | ||
| const failedRepo: DiscoveryRepo = { | ||
| ...result.repo, | ||
| rank: 0, | ||
| channel: selectedDiscoveryChannel, | ||
| platform: discoveryPlatform, | ||
| analyzed_at: new Date().toISOString(), | ||
| analysis_failed: true, | ||
| }; | ||
| updateDiscoveryRepo(failedRepo); |
Contributor
There was a problem hiding this comment.
Preserve discovery metadata when writing analysis results.
These updates overwrite the repo’s existing rank, channel, and platform; after analysis, ranked discovery cards can lose their real rank.
🐛 Proposed fix
if (result.success && result.repo) {
const updatedRepo: DiscoveryRepo = {
- ...result.repo,
- rank: 0,
- channel: selectedDiscoveryChannel,
- platform: discoveryPlatform,
+ ...(result.repo as DiscoveryRepo),
ai_summary: result.summary,
ai_tags: result.tags,
ai_platforms: result.platforms,
analyzed_at: new Date().toISOString(),
@@
} else if (!result.success && result.repo) {
const failedRepo: DiscoveryRepo = {
- ...result.repo,
- rank: 0,
- channel: selectedDiscoveryChannel,
- platform: discoveryPlatform,
+ ...(result.repo as DiscoveryRepo),
analyzed_at: new Date().toISOString(),
analysis_failed: true,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/DiscoveryView.tsx` around lines 872 - 895, The current result
handling overwrites existing discovery metadata (rank, channel, platform) when
constructing updatedRepo/failedRepo; change the logic in the result callback
that builds DiscoveryRepo objects (the branches that call updateDiscoveryRepo)
to preserve existing result.repo.rank, result.repo.channel, and
result.repo.platform when they exist instead of unconditionally setting rank: 0,
channel: selectedDiscoveryChannel, platform: discoveryPlatform; only
set/overwrite AI-specific fields (ai_summary, ai_tags, ai_platforms),
analyzed_at, and analysis_failed, and fall back to the new values only if the
original repo lacks those properties.
Comment on lines
+1166
to
+1217
| <select | ||
| value={discoveryLanguage} | ||
| onChange={(e) => setDiscoveryLanguage(e.target.value as ProgrammingLanguage)} | ||
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | ||
| > | ||
| <option value="All">{t('所有语言', 'All Languages')}</option> | ||
| <option value="JavaScript">JavaScript</option> | ||
| <option value="TypeScript">TypeScript</option> | ||
| <option value="Python">Python</option> | ||
| <option value="Java">Java</option> | ||
| <option value="Kotlin">Kotlin</option> | ||
| <option value="Go">Go</option> | ||
| <option value="Rust">Rust</option> | ||
| <option value="CSharp">C#</option> | ||
| <option value="CPlusPlus">C++</option> | ||
| <option value="C">C</option> | ||
| <option value="Swift">Swift</option> | ||
| <option value="Dart">Dart</option> | ||
| <option value="Ruby">Ruby</option> | ||
| <option value="PHP">PHP</option> | ||
| </select> | ||
|
|
||
| <select | ||
| value={discoverySortBy} | ||
| onChange={(e) => setDiscoverySortBy(e.target.value as SortBy)} | ||
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | ||
| > | ||
| <option value="BestMatch">{t('最佳匹配', 'Best Match')}</option> | ||
| <option value="MostStars">{t('最多Star', 'Most Stars')}</option> | ||
| <option value="MostForks">{t('最多Fork', 'Most Forks')}</option> | ||
| </select> | ||
|
|
||
| <select | ||
| value={discoverySortOrder} | ||
| onChange={(e) => setDiscoverySortOrder(e.target.value as SortOrder)} | ||
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | ||
| > | ||
| <option value="Descending">{t('降序', 'Descending')}</option> | ||
| <option value="Ascending">{t('升序', 'Ascending')}</option> | ||
| </select> | ||
|
|
||
| <select | ||
| value={discoveryPlatform} | ||
| onChange={(e) => setDiscoveryPlatform(e.target.value as DiscoveryPlatform)} | ||
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | ||
| > | ||
| <option value="All">{t('所有平台', 'All Platforms')}</option> | ||
| <option value="Android">Android</option> | ||
| <option value="Macos">macOS</option> | ||
| <option value="Windows">Windows</option> | ||
| <option value="Linux">Linux</option> | ||
| </select> |
Contributor
There was a problem hiding this comment.
Reset search pagination when filters change.
Changing language/sort/order/platform while on page 2+ keeps the old page index, so the next search/refresh can render an empty or stale slice.
🐛 Proposed direction
- onChange={(e) => setDiscoveryLanguage(e.target.value as ProgrammingLanguage)}
+ onChange={(e) => {
+ setCurrentPage(1);
+ setDiscoveryLanguage(e.target.value as ProgrammingLanguage);
+ }}Apply the same reset to the sort, order, and platform selects.
📝 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.
Suggested change
| <select | |
| value={discoveryLanguage} | |
| onChange={(e) => setDiscoveryLanguage(e.target.value as ProgrammingLanguage)} | |
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | |
| > | |
| <option value="All">{t('所有语言', 'All Languages')}</option> | |
| <option value="JavaScript">JavaScript</option> | |
| <option value="TypeScript">TypeScript</option> | |
| <option value="Python">Python</option> | |
| <option value="Java">Java</option> | |
| <option value="Kotlin">Kotlin</option> | |
| <option value="Go">Go</option> | |
| <option value="Rust">Rust</option> | |
| <option value="CSharp">C#</option> | |
| <option value="CPlusPlus">C++</option> | |
| <option value="C">C</option> | |
| <option value="Swift">Swift</option> | |
| <option value="Dart">Dart</option> | |
| <option value="Ruby">Ruby</option> | |
| <option value="PHP">PHP</option> | |
| </select> | |
| <select | |
| value={discoverySortBy} | |
| onChange={(e) => setDiscoverySortBy(e.target.value as SortBy)} | |
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | |
| > | |
| <option value="BestMatch">{t('最佳匹配', 'Best Match')}</option> | |
| <option value="MostStars">{t('最多Star', 'Most Stars')}</option> | |
| <option value="MostForks">{t('最多Fork', 'Most Forks')}</option> | |
| </select> | |
| <select | |
| value={discoverySortOrder} | |
| onChange={(e) => setDiscoverySortOrder(e.target.value as SortOrder)} | |
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | |
| > | |
| <option value="Descending">{t('降序', 'Descending')}</option> | |
| <option value="Ascending">{t('升序', 'Ascending')}</option> | |
| </select> | |
| <select | |
| value={discoveryPlatform} | |
| onChange={(e) => setDiscoveryPlatform(e.target.value as DiscoveryPlatform)} | |
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | |
| > | |
| <option value="All">{t('所有平台', 'All Platforms')}</option> | |
| <option value="Android">Android</option> | |
| <option value="Macos">macOS</option> | |
| <option value="Windows">Windows</option> | |
| <option value="Linux">Linux</option> | |
| </select> | |
| <select | |
| value={discoveryLanguage} | |
| onChange={(e) => { | |
| setCurrentPage(1); | |
| setDiscoveryLanguage(e.target.value as ProgrammingLanguage); | |
| }} | |
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | |
| > | |
| <option value="All">{t('所有语言', 'All Languages')}</option> | |
| <option value="JavaScript">JavaScript</option> | |
| <option value="TypeScript">TypeScript</option> | |
| <option value="Python">Python</option> | |
| <option value="Java">Java</option> | |
| <option value="Kotlin">Kotlin</option> | |
| <option value="Go">Go</option> | |
| <option value="Rust">Rust</option> | |
| <option value="CSharp">C#</option> | |
| <option value="CPlusPlus">C++</option> | |
| <option value="C">C</option> | |
| <option value="Swift">Swift</option> | |
| <option value="Dart">Dart</option> | |
| <option value="Ruby">Ruby</option> | |
| <option value="PHP">PHP</option> | |
| </select> | |
| <select | |
| value={discoverySortBy} | |
| onChange={(e) => { | |
| setCurrentPage(1); | |
| setDiscoverySortBy(e.target.value as SortBy); | |
| }} | |
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | |
| > | |
| <option value="BestMatch">{t('最佳匹配', 'Best Match')}</option> | |
| <option value="MostStars">{t('最多Star', 'Most Stars')}</option> | |
| <option value="MostForks">{t('最多Fork', 'Most Forks')}</option> | |
| </select> | |
| <select | |
| value={discoverySortOrder} | |
| onChange={(e) => { | |
| setCurrentPage(1); | |
| setDiscoverySortOrder(e.target.value as SortOrder); | |
| }} | |
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | |
| > | |
| <option value="Descending">{t('降序', 'Descending')}</option> | |
| <option value="Ascending">{t('升序', 'Ascending')}</option> | |
| </select> | |
| <select | |
| value={discoveryPlatform} | |
| onChange={(e) => { | |
| setCurrentPage(1); | |
| setDiscoveryPlatform(e.target.value as DiscoveryPlatform); | |
| }} | |
| className="px-3 py-2 text-sm rounded-xl border border-gray-200 dark:border-gray-600 bg-white/80 dark:bg-gray-700/80 text-gray-700 dark:text-gray-300 focus:ring-2 focus:ring-blue-500 focus:border-transparent transition-colors" | |
| > | |
| <option value="All">{t('所有平台', 'All Platforms')}</option> | |
| <option value="Android">Android</option> | |
| <option value="Macos">macOS</option> | |
| <option value="Windows">Windows</option> | |
| <option value="Linux">Linux</option> | |
| </select> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/DiscoveryView.tsx` around lines 1166 - 1217, The issue:
changing filters (language/sort/order/platform) doesn't reset pagination,
leaving the page index stale; fix by updating each select's onChange handler
(the handlers that call setDiscoveryLanguage, setDiscoverySortBy,
setDiscoverySortOrder, setDiscoveryPlatform) to also reset the discovery page
state (call setDiscoveryPage(1) or the pagination setter used in this component)
so any filter change returns results from page 1; apply this to all four select
elements (language, sortBy, sortOrder, platform).
…nnel dependency on allRepos and auto-fetch logic
This was referenced Apr 23, 2026
Merged
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
Verification
Notes
Summary by CodeRabbit
New Features
Bug Fixes
Chores