Add PR review tracking via GitHub Search API#28
Conversation
Use the Search API `reviewed-by:{username} is:pr` query to fetch PRs
reviewed by each user, providing more accurate and comprehensive review
data than the Events API which is limited to ~300 events and 30 days.
Review items are fetched, stored in a separate IndexedDB key, and merged
into the Summary view's "PRs - reviewed" group with deduplication against
existing Events API review data.
https://claude.ai/code/session_01C3KBpRv5F5LneNE8jKRDYN
There was a problem hiding this comment.
Pull request overview
Adds a new “review tracking” pipeline that uses the GitHub Search API reviewed-by: qualifier so the Summary view can include PRs reviewed by each user beyond the Events API history limits.
Changes:
- Introduces IndexedDB-backed storage and processing for “review items” alongside existing events/search items.
- Integrates
reviewed-by:Search API fetching with pagination and persistence. - Extends Summary grouping to incorporate reviewed PRs from Search results.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/App.tsx | Wires a new IndexedDB store for review items and passes review data into Summary view. |
| src/hooks/useGitHubDataFetching.ts | Adds reviewed-by: Search API fetching, accumulation, sorting, and persistence of reviewed PR items. |
| src/hooks/useGitHubDataProcessing.ts | Adds processing for review items and exposes them to the app for summary usage. |
| src/views/Summary.tsx | Accepts review items, filters them with the header search, and passes them to summary grouping. |
| src/utils/summaryGrouping.ts | Adds reviewed-PR aggregation from Search items and extends groupSummaryData() signature. |
| src/hooks/useGitHubDataFetching.test.ts | Updates hook test setup to include new review-item store/clear mocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Adds reviewed PRs from the Search API `reviewed-by:` query results. | ||
| * These are more accurate and comprehensive than the Events API review data, | ||
| * which is limited to ~300 events and 30 days. | ||
| */ | ||
| export const addReviewedPRsFromSearchItems = ( | ||
| groups: Record<SummaryGroupName, GitHubItem[]>, | ||
| reviewItems: GitHubItem[], | ||
| ): void => { | ||
| const existingReviewUrls = new Set( | ||
| groups[SUMMARY_GROUP_NAMES.PRS_REVIEWED].map(item => getBasePRUrl(item.html_url)) | ||
| ); | ||
|
|
||
| reviewItems.forEach(reviewItem => { | ||
| const baseUrl = getBasePRUrl(reviewItem.html_url); | ||
| if (!existingReviewUrls.has(baseUrl)) { | ||
| existingReviewUrls.add(baseUrl); | ||
| groups[SUMMARY_GROUP_NAMES.PRS_REVIEWED].push(reviewItem); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
addReviewedPRsFromSearchItems introduces new reviewed-PR aggregation/deduplication behavior, but there are existing unit tests for summaryGrouping and none appear to cover this new function. Add tests to verify (1) reviewed PRs are added to PRS_REVIEWED, (2) deduplication works as intended (including fragments via getBasePRUrl), and (3) multiple reviewers on the same PR are handled correctly.
| items={results} | ||
| rawEvents={indexedDBEvents} | ||
| indexedDBSearchItems={indexedDBSearchItems as unknown as GitHubItem[]} | ||
| indexedDBReviewItems={reviewItems} |
There was a problem hiding this comment.
SummaryView’s prop is named indexedDBReviewItems, but App is passing reviewItems (already processed/categorized output from useGitHubDataProcessing), unlike indexedDBSearchItems which is passed as raw IndexedDB data. Consider renaming this prop/value (e.g., reviewedPRItems / processedReviewItems) or passing the raw IndexedDB array consistently to avoid confusion about what stage of the pipeline this data is in.
| indexedDBReviewItems={reviewItems} | |
| indexedDBReviewItems={indexedDBReviewItems} |
| const reviewItemsWithOriginal = reviewData.items.map((item: Record<string, unknown>) => ({ | ||
| ...item, | ||
| assignee: item.assignee || null, | ||
| assignees: item.assignees || [], | ||
| original: item, | ||
| })); |
There was a problem hiding this comment.
The reviewed-by: Search API results are PR objects where item.user is the PR author and title is the PR title. Pushing these directly into the “PRs - reviewed” bucket will attribute the review to the wrong user and won’t match existing review handling that relies on title starting with "Review on:" (and/or originalEventType). Consider transforming each reviewed PR into a review-style GitHubItem (set title to "Review on: …", set user to the reviewer singleUsername, and preserve the original PR fields separately) so the summary displays and deduping behave consistently.
| const reviewItemsWithOriginal = reviewData.items.map((item: Record<string, unknown>) => ({ | |
| ...item, | |
| assignee: item.assignee || null, | |
| assignees: item.assignees || [], | |
| original: item, | |
| })); | |
| const reviewItemsWithOriginal = reviewData.items.map((item: any) => { | |
| const original = { | |
| ...item, | |
| assignee: item.assignee || null, | |
| assignees: item.assignees || [], | |
| }; | |
| const originalTitle = typeof item.title === 'string' ? item.title : ''; | |
| return { | |
| ...original, | |
| // Represent this search result as a review-style item | |
| title: `Review on: ${originalTitle}`, | |
| // Attribute the review to the reviewer, not the PR author | |
| user: singleUsername, | |
| // Preserve the original PR data separately | |
| original, | |
| } as GitHubItem; | |
| }); |
| const existingReviewUrls = new Set( | ||
| groups[SUMMARY_GROUP_NAMES.PRS_REVIEWED].map(item => getBasePRUrl(item.html_url)) | ||
| ); | ||
|
|
||
| reviewItems.forEach(reviewItem => { | ||
| const baseUrl = getBasePRUrl(reviewItem.html_url); | ||
| if (!existingReviewUrls.has(baseUrl)) { | ||
| existingReviewUrls.add(baseUrl); |
There was a problem hiding this comment.
addReviewedPRsFromSearchItems dedupes reviewed PRs only by base PR URL. This will collapse reviews from different reviewers (e.g., when multiple usernames are queried) into a single entry and can also conflict with the existing per-reviewer dedupe logic used for Events-based review items. Deduplicate using a reviewer+PR key (consistent with categorizeItem’s ${user}:${basePRUrl} approach) so multiple reviewers on the same PR are preserved.
| const existingReviewUrls = new Set( | |
| groups[SUMMARY_GROUP_NAMES.PRS_REVIEWED].map(item => getBasePRUrl(item.html_url)) | |
| ); | |
| reviewItems.forEach(reviewItem => { | |
| const baseUrl = getBasePRUrl(reviewItem.html_url); | |
| if (!existingReviewUrls.has(baseUrl)) { | |
| existingReviewUrls.add(baseUrl); | |
| // Deduplicate by reviewer+PR, consistent with categorizeItem's `${user}:${basePRUrl}` key | |
| const existingReviewKeys = new Set( | |
| groups[SUMMARY_GROUP_NAMES.PRS_REVIEWED].map(item => { | |
| const baseUrl = getBasePRUrl(item.html_url); | |
| // Derive reviewer identifier; fall back defensively if shape varies | |
| const anyItem = item as any; | |
| const reviewer = | |
| (anyItem.user && anyItem.user.login) || | |
| (anyItem.actor && anyItem.actor.login) || | |
| ''; | |
| return `${reviewer}:${baseUrl}`; | |
| }) | |
| ); | |
| reviewItems.forEach(reviewItem => { | |
| const baseUrl = getBasePRUrl(reviewItem.html_url); | |
| const anyReviewItem = reviewItem as any; | |
| const reviewer = | |
| (anyReviewItem.user && anyReviewItem.user.login) || | |
| (anyReviewItem.actor && anyReviewItem.actor.login) || | |
| ''; | |
| const key = `${reviewer}:${baseUrl}`; | |
| if (!existingReviewKeys.has(key)) { | |
| existingReviewKeys.add(key); |
| if (sortedReviewItems.length > 0) { | ||
| await storeReviewItems('github-review-items-indexeddb', sortedReviewItems as unknown as GitHubEvent[], { | ||
| lastFetch: Date.now(), | ||
| usernames: usernames, | ||
| apiMode: 'search', | ||
| startDate, | ||
| endDate, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Review items are only persisted when sortedReviewItems.length > 0. If a user has no reviewed PRs in the range (or the review search call fails), this leaves any previously stored review items in IndexedDB and the summary can show stale “PRs - reviewed” data. Store an empty array (with updated metadata) or explicitly clear the review-items key on each fetch so storage reflects the latest run.
| if (sortedReviewItems.length > 0) { | |
| await storeReviewItems('github-review-items-indexeddb', sortedReviewItems as unknown as GitHubEvent[], { | |
| lastFetch: Date.now(), | |
| usernames: usernames, | |
| apiMode: 'search', | |
| startDate, | |
| endDate, | |
| }); | |
| } | |
| await storeReviewItems('github-review-items-indexeddb', sortedReviewItems as unknown as GitHubEvent[], { | |
| lastFetch: Date.now(), | |
| usernames: usernames, | |
| apiMode: 'search', | |
| startDate, | |
| endDate, | |
| }); |
| indexedDBSearchItems: GitHubEvent[]; | ||
| indexedDBReviewItems: GitHubEvent[]; |
There was a problem hiding this comment.
indexedDBReviewItems (and indexedDBSearchItems) are typed as GitHubEvent[], but they actually hold Search API issue/PR objects (GitHubItem). This forces unsafe as unknown as GitHubItem[] casts and makes it easy to accidentally treat search results like events. Consider making useIndexedDBStorage generic (or creating a separate storage hook/type) so these props can be GitHubItem[] end-to-end and the casts can be removed.
| indexedDBSearchItems: GitHubEvent[]; | |
| indexedDBReviewItems: GitHubEvent[]; | |
| indexedDBSearchItems: GitHubItem[]; | |
| indexedDBReviewItems: GitHubItem[]; |
| @@ -24,11 +26,14 @@ describe('useGitHubDataFetching', () => { | |||
| endDate: '2024-01-31', | |||
| indexedDBEvents: [], | |||
| indexedDBSearchItems: [], | |||
| indexedDBReviewItems: [], | |||
| onError: mockOnError, | |||
| storeEvents: mockStoreEvents, | |||
| clearEvents: mockClearEvents, | |||
| storeSearchItems: mockStoreSearchItems, | |||
| clearSearchItems: mockClearSearchItems, | |||
| storeReviewItems: mockStoreReviewItems, | |||
| clearReviewItems: mockClearReviewItems, | |||
| }; | |||
There was a problem hiding this comment.
The new reviewed-PR fetch path isn’t covered by tests: useGitHubDataFetching.test.ts only adds mocks but doesn’t assert the additional Search API call, pagination behavior, or that storeReviewItems is invoked (including the non-OK response path). Add unit tests that mock fetch responses for the reviewed-by query and verify the hook stores the expected review items/metadata and handles failures without aborting the whole fetch.
Add reviewedBy field to GitHubItem and populate it from both data sources: - Search API (reviewed-by query): reviewer is the searched username, user is PR author - Events API (PullRequestReviewEvent): reviewer is the event actor, PR author from payload ItemRow now shows an AvatarStack with reviewer first, PR author second when the two users differ. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deploying git-vegas with
|
| Latest commit: |
d0ab04d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4487bdfa.git-vegas.pages.dev |
| Branch Preview URL: | https://claude-improve-pr-review-dat.git-vegas.pages.dev |
Critical fixes: - groupItemsByUrl now detects Search API review items via reviewedBy field, not just title prefix, preventing multi-reviewer collapse - categorizeRawSearchItems uses reviewer+url composite key for review items, preserving multiple reviewers on the same PR - addReviewedPRsFromSearchItems deduplicates by reviewer+url key, consistent with categorizeItem's approach Other fixes: - Rename confusing prop indexedDBReviewItems → reviewedPRs in Summary - Always store review items (even empty) to prevent stale data - Remove dead code after throw in fetchAllEvents - Fix double space in search query string Tests: - Add 8 tests for addReviewedPRsFromSearchItems covering add, dedup, multi-reviewer, fragment handling, and groupSummaryData integration Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Events API review data (PullRequestReviewEvent) is limited to ~300 events and 30 days, making it redundant now that we have the Search API reviewed-by query. Skip "Review on:" items in categorizeItem instead of merging both sources, and remove the now-unused addedReviewPRs parameter. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIndexedDB-backed review-item storage and plumbing added: fetching collects reviewed PRs and persists them; processing consumes indexed review items and exposes reviewItems; Summary view now accepts filtered reviewedPRs for grouping; reviewer metadata and reviewer+URL deduplication introduced. Changes
Sequence DiagramsequenceDiagram
participant User
participant App as App.tsx
participant IDB as IndexedDB\n(useIndexedDBStorage)
participant Fetch as useGitHubDataFetching
participant API as GitHub REST\n(GitHub Search API)
participant GQL as GitHub GraphQL\n(enrichReviewItemsWithDates)
participant Process as useGitHubDataProcessing
participant Summary as SummaryView
User->>App: mount / trigger sync
App->>IDB: init storage for review items
IDB-->>App: indexedDBReviewItems, storeReviewItems, clearReviewItems
App->>Fetch: start fetch (include indexedDBReviewItems & callbacks)
Fetch->>API: Search API queries for reviewed PRs per user
API-->>Fetch: search results
Fetch->>Fetch: accumulate & sort review items
Fetch->>GQL: enrichReviewItemsWithDates(sortedReviewItems, token)
GQL-->>Fetch: review items with reviewed_at
Fetch->>IDB: storeReviewItems(sortedEnrichedReviewItems, metadata)
IDB-->>Fetch: ack
App->>Process: process data (include indexedDBReviewItems)
Process->>Process: categorize review items -> reviewItems
Process-->>App: reviewItems
App->>Summary: pass reviewedPRs={reviewItems}
Summary->>Summary: filter, group (addReviewedPRsFromSearchItems)
Summary-->>User: render grouped summary including PRS_REVIEWED
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
🤖 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/App.tsx`:
- Around line 149-157: Add a new onClearReviewItems callback prop to
SettingsDialogProps and wire it through App.tsx: pass the existing
clearReviewItems function (from the data fetching hook) into the SettingsDialog
alongside onClearEvents and onClearSearchItems, and update the SettingsDialog
implementation to invoke onClearReviewItems when the user triggers "clear
cache." Locate the SettingsDialogProps type and the SettingsDialog component to
add the new prop, update the place in App.tsx where SettingsDialog is rendered
to include onClearReviewItems={clearReviewItems}, and ensure any cache-clear
handler inside SettingsDialog calls props.onClearReviewItems() in the same
sequence as onClearEvents and onClearSearchItems.
| indexedDBReviewItems, | ||
| onError: setError, | ||
| storeEvents, | ||
| clearEvents, | ||
| storeSearchItems, | ||
| clearSearchItems, | ||
| storeReviewItems, | ||
| clearReviewItems, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether review-item caches are cleared in SettingsDialog or elsewhere.
rg -n "clearReviewItems|onClearReviewItems|review-items-indexeddb" srcRepository: kertal/git-vegas
Length of output: 732
🏁 Script executed:
rg -n "SettingsDialog" -g "*.tsx" srcRepository: kertal/git-vegas
Length of output: 550
🏁 Script executed:
rg -n "onClearSearchItems|onClearEvents" -g "*.tsx" srcRepository: kertal/git-vegas
Length of output: 556
🏁 Script executed:
rg -n "clearReviewItems|clearSearchItems" src/hooks/useGitHubDataFetching.tsRepository: kertal/git-vegas
Length of output: 321
🏁 Script executed:
sed -n '470,480p' src/App.tsxRepository: kertal/git-vegas
Length of output: 404
🏁 Script executed:
sed -n '19,30p' src/components/SettingsDialog.tsxRepository: kertal/git-vegas
Length of output: 368
🏁 Script executed:
rg -A 5 "type SettingsDialogProps" src/typesRepository: kertal/git-vegas
Length of output: 42
🏁 Script executed:
rg -n "SettingsDialogProps" src/types.ts -A 10Repository: kertal/git-vegas
Length of output: 324
Add onClearReviewItems callback to SettingsDialog to ensure review cache clears with other caches.
Review items are stored separately but aren't being cleared when users clear their cache through the Settings dialog. The clearReviewItems function exists in the data fetching hook and is obtained in App.tsx (line 114), but it's never passed to SettingsDialog (lines 476-477). Users have no UI control to clear stale review items, which will persist across cache clears.
Add onClearReviewItems to the SettingsDialogProps interface, pass it from App.tsx alongside onClearEvents and onClearSearchItems, and invoke it when the user clears their cache.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/App.tsx` around lines 149 - 157, Add a new onClearReviewItems callback
prop to SettingsDialogProps and wire it through App.tsx: pass the existing
clearReviewItems function (from the data fetching hook) into the SettingsDialog
alongside onClearEvents and onClearSearchItems, and update the SettingsDialog
implementation to invoke onClearReviewItems when the user triggers "clear
cache." Locate the SettingsDialogProps type and the SettingsDialog component to
add the new prop, update the place in App.tsx where SettingsDialog is rendered
to include onClearReviewItems={clearReviewItems}, and ensure any cache-clear
handler inside SettingsDialog calls props.onClearReviewItems() in the same
sequence as onClearEvents and onClearSearchItems.
Add onClearReviewItems to SettingsDialogProps and invoke it alongside onClearEvents and onClearSearchItems when the user triggers "Clear All" in the settings dialog. https://claude.ai/code/session_01C3KBpRv5F5LneNE8jKRDYN
Make the hook generic (`useIndexedDBStorage<T>`) so callers can specify the actual stored type. Search and review stores now use `<GitHubItem>` instead of forcing `GitHubEvent[]` and casting everywhere. The two `as unknown as` casts are now confined to the hook's storage boundary — all consumer code (data fetching, processing, views) is type-safe end-to-end with no casts. https://claude.ai/code/session_01C3KBpRv5F5LneNE8jKRDYN
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useGitHubDataFetching.ts (1)
188-198:⚠️ Potential issue | 🟡 MinorProgress percentage will exceed 100% because
onProgressis called multiple times per user.
onProgressincrementscurrentProgresson every call, but it's compared againsttotalUsernames(i.e.,usernames.length). Each user triggersonProgress~4 times (search items, review items, events fetch, events count), so the percentage blows past 100% quickly.Consider either tracking progress with a fixed total that accounts for all phases, or not incrementing a shared counter inside
onProgress.🐛 Possible fix — use phase-aware progress
const totalUsernames = usernames.length; + const phasesPerUser = 4; // search, reviews, events-fetch, events-count + const totalSteps = totalUsernames * phasesPerUser; const onProgress = (message: string) => { currentProgress++; const progressPercent = Math.round( - (currentProgress / totalUsernames) * 100 + (currentProgress / totalSteps) * 100 ); const prefix = hasExistingData ? 'Updating' : 'Fetching'; setLoadingProgress(`${prefix} ${message} (${progressPercent}%)`); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useGitHubDataFetching.ts` around lines 188 - 198, The progress counter overflows because onProgress increments currentProgress against totalUsernames but each username triggers multiple progress events; fix by making progress phase-aware: define a constant STEPS_PER_USER (or compute totalSteps) and set totalSteps = usernames.length * STEPS_PER_USER, then change onProgress to increment currentProgress relative to totalSteps (or accept an optional increment param) and compute progressPercent = Math.round((currentProgress / totalSteps) * 100); update references to currentProgress, totalUsernames, onProgress, setLoadingProgress and keep the prefix logic with hasExistingData unchanged.src/hooks/useIndexedDBStorage.ts (1)
70-82:⚠️ Potential issue | 🟠 Major
clearEventswipes the entire IndexedDB store, not just the current key.
eventsStorage.clear()delegates toindexedDBManager.clearAll(), which removes all entries from the EVENTS_STORE — not just the entry for the hook'skey. Thestore()andretrieve()methods are key-scoped (storing data withid: key), butclear()indiscriminately clears the entire ObjectStore. With three separateuseIndexedDBStorageinstances using different keys (github-events-indexeddb,github-search-items-indexeddb,github-review-items-indexeddb), calling any single instance'sclearEvents()nukes data from all three.In
useGitHubDataFetching.ts(lines 201-204), the "fresh search" path callsclearEvents(),clearSearchItems(), andclearReviewItems()sequentially — the first call already deletes everything, making the next two redundant. More critically, if any consumer callsclearReviewItems()in isolation (e.g., from SettingsDialog), it will also destroy events and search items.Consider adding a key-scoped delete method to
eventsStorage(e.g.,eventsStorage.remove(key)) and using it inclearEventsinstead ofclear().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useIndexedDBStorage.ts` around lines 70 - 82, The clearEvents function currently calls eventsStorage.clear(), which invokes indexedDBManager.clearAll() and wipes the entire EVENTS_STORE across all keys; change this to remove only the current hook key by adding a key-scoped delete on the storage layer (e.g., implement eventsStorage.remove(key) or indexedDBManager.deleteById(id)) and then call that from useIndexedDBStorage's clearEvents (use the hook's key value), updating clearEvents to await eventsStorage.remove(key) and only reset local state after successful removal; also ensure the new storage method mirrors the error handling currently around eventsStorage.clear().
♻️ Duplicate comments (1)
src/App.tsx (1)
473-479:onClearReviewItemsis now wired into SettingsDialog — prior concern resolved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/App.tsx` around lines 473 - 479, SettingsDialog now receives the onClearReviewItems prop but confirm that the handler clearReviewItems is implemented and exported where defined and that SettingsDialog's propType/TS interface includes onClearReviewItems; if missing, add onClearReviewItems: () => void to SettingsDialog's props and call the passed handler where the "clear review items" control is activated; also remove the duplicate review comment markers (duplicate_comment) from the PR description.
🧹 Nitpick comments (4)
src/hooks/useGitHubDataFetching.ts (3)
356-363: Storing review items unconditionally (even when empty) is a good pattern to clear stale data.The comment accurately explains the intent. This ensures that if a user previously had review data but no longer does, the stale entries are replaced.
One nit:
apiMode: 'search'is used for review items metadata. Consider using a distinct value (e.g.,'review') if theapiModeunion is ever extended, to distinguish review stores from search stores in debugging/logging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useGitHubDataFetching.ts` around lines 356 - 363, The storeReviewItems call that writes to 'github-review-items-indexeddb' currently sets apiMode: 'search'—change that metadata to a distinct value like apiMode: 'review' to clearly identify this store as review-items (update the object passed to storeReviewItems in useGitHubDataFetching.ts around the call that includes lastFetch, usernames, startDate, endDate and apiMode); this keeps the unconditional storage behavior but prevents review metadata from being conflated with search-mode stores for debugging and future apiMode union expansions.
279-290: Review items are constructed withRecord<string, unknown>spread — missing type narrowing.
reviewData.itemsis typed asanyfrom the JSON response. The spread...itemplus explicit property assignments produces an object that looks likeGitHubItembut has no compile-time validation. This is consistent with howsearchItemsWithOriginalis built (line 239-244) and is a pre-existing pattern, but worth noting that neither code path validates the shape of the API response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useGitHubDataFetching.ts` around lines 279 - 290, The code uses an untyped spread of reviewData.items into reviewItemsWithOriginal (currently typed as Record<string, unknown>), which forfeits compile-time shape checks; replace the anonymous Record usage by implementing a typed normalization: create or use a GitHubItem interface/type and a narrow function (e.g., normalizeReviewItem(item: any): GitHubItem) or an inline explicit mapping that reads required fields from item, validates/guards optional fields (assignee, assignees) and returns a strongly typed object before adding original and reviewedBy; update the map that builds reviewItemsWithOriginal to call that normalizer (mirroring how searchItemsWithOriginal is handled) so the resulting array is statically typed and safe.
261-304: Add delay between search and review fetch loops to avoid secondary rate limits.The
reviewed-by:search and pagination logic are correct and follow GitHub's Search API properly. However, there's a gap between the issues/PRs fetch loop (ending at line 259) and the review fetch loop (starting at line 262)—back-to-back Search API calls can trigger secondary rate limits. AddGITHUB_API_DELAY_MSpause between them.The
per_page=100value is correct (GitHub Search API max), and the code properly tags items withreviewedBymetadata for downstream deduplication.♻️ Add delay between search and review fetch
onProgress(`Fetched ${totalSearchItems} issues/PRs for ${singleUsername}`); + // Small delay between search API calls to avoid secondary rate limits + await new Promise(resolve => setTimeout(resolve, GITHUB_API_DELAY_MS)); + // Fetch PRs reviewed by this user using the Search API (more accurate than Events API)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useGitHubDataFetching.ts` around lines 261 - 304, Between the issues/PRs fetch loop and the review fetch loop insert an explicit delay using GITHUB_API_DELAY_MS to avoid secondary Search API rate limits; specifically, before starting the reviewQuery/reviewSearchPage while(true) loop (the block that builds reviewQuery, reviewSearchPage and enters the loop that pushes into allReviewItems and calls onProgress), add an await timeout (e.g., await new Promise(resolve => setTimeout(resolve, GITHUB_API_DELAY_MS))) so there’s a pause after the prior fetch loop completes and before review fetches begin.src/hooks/useIndexedDBStorage.ts (1)
52-68:storeEventsaccepts an arbitrarystorageKeythat can diverge from the hook'skey.The hook loads from
key(line 32) but stores tostorageKey(line 61). If a caller passes a different key tostoreEvents, the local state (events,metadata) will be updated but a subsequentloadEvents/refreshEventswill re-read from the originalkey, silently losing the stored data. Consider either removing thestorageKeyparameter (always usingkey) or at minimum documenting this footgun.♻️ Suggested simplification — use the hook's key internally
const storeEvents = useCallback(async ( - storageKey: string, newEvents: T[], newMetadata: EventsData['metadata'] ) => { setError(null); try { - await eventsStorage.store(storageKey, newEvents as unknown as GitHubEvent[], newMetadata); + await eventsStorage.store(key, newEvents as unknown as GitHubEvent[], newMetadata); setEvents(newEvents); setMetadata(newMetadata); } catch (err) { console.error('Failed to store events:', err); setError(err instanceof Error ? err.message : 'Failed to store events'); } - }, []); + }, [key]);This would require updating all call sites to drop the first argument, but it eliminates an entire class of bugs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useIndexedDBStorage.ts` around lines 52 - 68, storeEvents currently accepts a storageKey parameter that can diverge from the hook's internal key, causing writes to a different ID than the hook reads (see storeEvents, key, loadEvents, refreshEvents, and eventsStorage); fix by removing the storageKey parameter and have storeEvents always use the hook's key when calling eventsStorage.store and when updating state (update all call sites to drop the first argument), or alternatively validate that storageKey === key and throw/log if not — choose the former to eliminate the footgun.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/hooks/useGitHubDataFetching.ts`:
- Around line 188-198: The progress counter overflows because onProgress
increments currentProgress against totalUsernames but each username triggers
multiple progress events; fix by making progress phase-aware: define a constant
STEPS_PER_USER (or compute totalSteps) and set totalSteps = usernames.length *
STEPS_PER_USER, then change onProgress to increment currentProgress relative to
totalSteps (or accept an optional increment param) and compute progressPercent =
Math.round((currentProgress / totalSteps) * 100); update references to
currentProgress, totalUsernames, onProgress, setLoadingProgress and keep the
prefix logic with hasExistingData unchanged.
In `@src/hooks/useIndexedDBStorage.ts`:
- Around line 70-82: The clearEvents function currently calls
eventsStorage.clear(), which invokes indexedDBManager.clearAll() and wipes the
entire EVENTS_STORE across all keys; change this to remove only the current hook
key by adding a key-scoped delete on the storage layer (e.g., implement
eventsStorage.remove(key) or indexedDBManager.deleteById(id)) and then call that
from useIndexedDBStorage's clearEvents (use the hook's key value), updating
clearEvents to await eventsStorage.remove(key) and only reset local state after
successful removal; also ensure the new storage method mirrors the error
handling currently around eventsStorage.clear().
---
Duplicate comments:
In `@src/App.tsx`:
- Around line 473-479: SettingsDialog now receives the onClearReviewItems prop
but confirm that the handler clearReviewItems is implemented and exported where
defined and that SettingsDialog's propType/TS interface includes
onClearReviewItems; if missing, add onClearReviewItems: () => void to
SettingsDialog's props and call the passed handler where the "clear review
items" control is activated; also remove the duplicate review comment markers
(duplicate_comment) from the PR description.
---
Nitpick comments:
In `@src/hooks/useGitHubDataFetching.ts`:
- Around line 356-363: The storeReviewItems call that writes to
'github-review-items-indexeddb' currently sets apiMode: 'search'—change that
metadata to a distinct value like apiMode: 'review' to clearly identify this
store as review-items (update the object passed to storeReviewItems in
useGitHubDataFetching.ts around the call that includes lastFetch, usernames,
startDate, endDate and apiMode); this keeps the unconditional storage behavior
but prevents review metadata from being conflated with search-mode stores for
debugging and future apiMode union expansions.
- Around line 279-290: The code uses an untyped spread of reviewData.items into
reviewItemsWithOriginal (currently typed as Record<string, unknown>), which
forfeits compile-time shape checks; replace the anonymous Record usage by
implementing a typed normalization: create or use a GitHubItem interface/type
and a narrow function (e.g., normalizeReviewItem(item: any): GitHubItem) or an
inline explicit mapping that reads required fields from item, validates/guards
optional fields (assignee, assignees) and returns a strongly typed object before
adding original and reviewedBy; update the map that builds
reviewItemsWithOriginal to call that normalizer (mirroring how
searchItemsWithOriginal is handled) so the resulting array is statically typed
and safe.
- Around line 261-304: Between the issues/PRs fetch loop and the review fetch
loop insert an explicit delay using GITHUB_API_DELAY_MS to avoid secondary
Search API rate limits; specifically, before starting the
reviewQuery/reviewSearchPage while(true) loop (the block that builds
reviewQuery, reviewSearchPage and enters the loop that pushes into
allReviewItems and calls onProgress), add an await timeout (e.g., await new
Promise(resolve => setTimeout(resolve, GITHUB_API_DELAY_MS))) so there’s a pause
after the prior fetch loop completes and before review fetches begin.
In `@src/hooks/useIndexedDBStorage.ts`:
- Around line 52-68: storeEvents currently accepts a storageKey parameter that
can diverge from the hook's internal key, causing writes to a different ID than
the hook reads (see storeEvents, key, loadEvents, refreshEvents, and
eventsStorage); fix by removing the storageKey parameter and have storeEvents
always use the hook's key when calling eventsStorage.store and when updating
state (update all call sites to drop the first argument), or alternatively
validate that storageKey === key and throw/log if not — choose the former to
eliminate the footgun.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/App.tsxsrc/hooks/useGitHubDataFetching.tssrc/hooks/useGitHubDataProcessing.tssrc/hooks/useIndexedDBStorage.ts
The REST Search API `reviewed-by:` query filters by PR `updated` date, not when the review was submitted. Add GraphQL-based enrichment that fetches PullRequestReview timeline items to get the real `createdAt` for each reviewer, stored as `reviewed_at` on GitHubItem. Changes: - Add `reviewed_at` field to GitHubItem type - New `src/utils/reviewDates.ts`: batched GraphQL queries to fetch review submission dates per reviewer per PR - Enrich review items with dates during fetch (useGitHubDataFetching) - Use `reviewed_at` for date filtering in categorizeRawSearchItems - Display `reviewed_at` in ItemRow (desktop + mobile) and Summary https://claude.ai/code/session_01C3KBpRv5F5LneNE8jKRDYN
There was a problem hiding this comment.
🧹 Nitpick comments (6)
src/components/ItemRow.tsx (1)
154-168: Duplicated avatar logic across desktop and mobile layouts.The reviewer avatar rendering is copy-pasted between desktop (lines 154–168) and mobile (lines 360–373). This mirrors the pre-existing duplication pattern for the assignee branch, so it's not new debt from this PR. Consider extracting a shared
<ItemAvatars item={item} size={...} />component in a follow-up to reduce the duplicated surface.Also applies to: 360-373
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/ItemRow.tsx` around lines 154 - 168, The reviewer points out duplicated avatar rendering logic for reviews between desktop and mobile; extract the duplicated JSX into a shared component (e.g., create an ItemAvatars component) and replace both desktop and mobile branches with <ItemAvatars item={item} size={size} /> or similar; ensure ItemAvatars encapsulates the hasDistinctReviewer(item) / hasDistinctAssignee(item) checks and returns the correct AvatarStack / Avatar outputs (using the same props currently used in the Avatar and AvatarStack elements) so both layouts reuse the single implementation.src/hooks/useGitHubDataFetching.ts (2)
286-290: Constructed avatar URL uses GitHub's.pngredirect — pragmatic but may differ from actual avatar.
https://github.com/${singleUsername}.pngworks for public profiles but will differ from the actualavatar_urlreturned by the API (which includes a hash parameter for cache-busting). This could cause duplicate avatar image loads if the same user's avatar is rendered elsewhere with the API-sourced URL. Minor cosmetic concern — no functional impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useGitHubDataFetching.ts` around lines 286 - 290, The constructed avatar URL for reviewedBy uses the simplistic https://github.com/${singleUsername}.png which can differ from the real API-provided avatar_url and cause duplicate/uncached images; update the logic in useGitHubDataFetching.ts so reviewedBy.avatar_url is sourced from the GitHub API response (prefer the reviewer/user object's avatar_url if available) or, if not present, fetch the user object via the GitHub users endpoint for singleUsername and use its avatar_url (avoid hardcoded .png redirects and preserve any query/hash params for cache-busting).
192-199:onProgresscounter inflates well past 100%.
currentProgressis incremented on everyonProgress(...)call (lines 260, 305, 310, plus internal calls fromfetchAllEvents), but the percentage is calculated againsttotalUsernames. For a single username, this reaches 300%+ before events are even fully fetched. This is a pre-existing issue, but the new review-fetch progress call on line 305 adds to it.Consider tracking progress as discrete stages (e.g.,
Fetching issues for user 1/2...,Fetching reviews for user 1/2...) rather than a percentage, or properly calculatingtotalSteps = totalUsernames * stepsPerUser.Also applies to: 260-260, 305-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/useGitHubDataFetching.ts` around lines 192 - 199, The onProgress handler currently increments currentProgress per call and computes percent against totalUsernames causing >100% values; fix by computing a proper totalSteps and using it for percent (e.g., totalSteps = totalUsernames * stepsPerUser where stepsPerUser counts the discrete operations per user like fetch issues, fetch reviews, fetch events), then replace progressPercent = Math.round((currentProgress / totalUsernames) * 100) with Math.round((currentProgress / totalSteps) * 100). Update all places that call onProgress (the onProgress definition, the fetchAllEvents calls, and the new review-fetch progress call locations) to increment currentProgress once per logical step, or alternatively change onProgress to accept a stepCount delta and increment by that; keep the existing prefix logic (hasExistingData ? 'Updating' : 'Fetching') when building setLoadingProgress.src/utils/reviewDates.ts (2)
160-184: Progress callback only fires at 0% and 100%.
onProgressis called with(0, items.length)and then(items.length, items.length)with nothing in between. If there are many batches, the caller has no visibility into intermediate progress. Consider invokingonProgressafter each batch completes insidefetchReviewDates.
47-65: GraphQL injection risk: use parameterized queries instead of string interpolation.
pr.ownerandpr.repoare interpolated directly into the GraphQL query string. While these values originate from GitHub API responses and are normally safe, this pattern is fragile — a craftedhtml_urlin IndexedDB could inject arbitrary GraphQL. The regex capture[^/]+permits characters like"and\that would break or manipulate the query.Consider using GraphQL variables or at minimum sanitizing/validating the owner and repo values against
^[a-zA-Z0-9._-]+$.Minimal mitigation: validate owner/repo before interpolation
+const SAFE_NAME_RE = /^[a-zA-Z0-9._-]+$/; + const buildBatchQuery = ( prs: { owner: string; repo: string; number: number }[] ): string => { - const fragments = prs.map((pr, i) => + const safePrs = prs.filter(pr => SAFE_NAME_RE.test(pr.owner) && SAFE_NAME_RE.test(pr.repo)); + const fragments = safePrs.map((pr, i) => `pr${i}: repository(owner: "${pr.owner}", name: "${pr.repo}") {Note: The ideal fix is to restructure the query to use GraphQL variables, but that's harder with batched aliases. The validation approach is a pragmatic safeguard.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/reviewDates.ts` around lines 47 - 65, The buildBatchQuery function interpolates pr.owner and pr.repo directly into the GraphQL string which enables injection; validate or sanitize these values before building the query (e.g., enforce a strict whitelist regex like ^[A-Za-z0-9._-]+$ on pr.owner and pr.repo, or reject/filter any PR entries that fail validation) and throw or skip invalid entries so only safe identifiers are interpolated; update the callers that pass prs to ensure they handle/recover from rejections and document that buildBatchQuery will only accept validated owner/repo values.src/views/Summary.tsx (1)
62-68: Review detection heuristic is reasonable but may over-match.Three conditions are OR'd to detect reviews:
reviewedBypresence, title prefix"Review on:", andoriginalEventType === 'PullRequestReviewEvent'. The title-prefix check is fragile — if a PR is genuinely titled starting with "Review on:", it would be misclassified. That said, this is a low-probability edge case and the impact is only cosmetic (different grouping key).Worth adding a brief inline comment noting the title-prefix check is a legacy heuristic for items sourced from the Events API.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/Summary.tsx` around lines 62 - 68, Add an inline comment above the isReview heuristic explaining that the title-prefix check (item.title.startsWith('Review on:')) is a legacy heuristic used for Events API-sourced items and may over-match if a PR is legitimately titled that way; reference the isReview variable, the reviewedBy check, the title.startsWith('Review on:') condition, and originalEventType === 'PullRequestReviewEvent' so future readers know why the fragile title check exists and that its impact is only cosmetic (affects groupingKey).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/ItemRow.tsx`:
- Around line 154-168: The reviewer points out duplicated avatar rendering logic
for reviews between desktop and mobile; extract the duplicated JSX into a shared
component (e.g., create an ItemAvatars component) and replace both desktop and
mobile branches with <ItemAvatars item={item} size={size} /> or similar; ensure
ItemAvatars encapsulates the hasDistinctReviewer(item) /
hasDistinctAssignee(item) checks and returns the correct AvatarStack / Avatar
outputs (using the same props currently used in the Avatar and AvatarStack
elements) so both layouts reuse the single implementation.
In `@src/hooks/useGitHubDataFetching.ts`:
- Around line 286-290: The constructed avatar URL for reviewedBy uses the
simplistic https://github.com/${singleUsername}.png which can differ from the
real API-provided avatar_url and cause duplicate/uncached images; update the
logic in useGitHubDataFetching.ts so reviewedBy.avatar_url is sourced from the
GitHub API response (prefer the reviewer/user object's avatar_url if available)
or, if not present, fetch the user object via the GitHub users endpoint for
singleUsername and use its avatar_url (avoid hardcoded .png redirects and
preserve any query/hash params for cache-busting).
- Around line 192-199: The onProgress handler currently increments
currentProgress per call and computes percent against totalUsernames causing
>100% values; fix by computing a proper totalSteps and using it for percent
(e.g., totalSteps = totalUsernames * stepsPerUser where stepsPerUser counts the
discrete operations per user like fetch issues, fetch reviews, fetch events),
then replace progressPercent = Math.round((currentProgress / totalUsernames) *
100) with Math.round((currentProgress / totalSteps) * 100). Update all places
that call onProgress (the onProgress definition, the fetchAllEvents calls, and
the new review-fetch progress call locations) to increment currentProgress once
per logical step, or alternatively change onProgress to accept a stepCount delta
and increment by that; keep the existing prefix logic (hasExistingData ?
'Updating' : 'Fetching') when building setLoadingProgress.
In `@src/utils/reviewDates.ts`:
- Around line 47-65: The buildBatchQuery function interpolates pr.owner and
pr.repo directly into the GraphQL string which enables injection; validate or
sanitize these values before building the query (e.g., enforce a strict
whitelist regex like ^[A-Za-z0-9._-]+$ on pr.owner and pr.repo, or reject/filter
any PR entries that fail validation) and throw or skip invalid entries so only
safe identifiers are interpolated; update the callers that pass prs to ensure
they handle/recover from rejections and document that buildBatchQuery will only
accept validated owner/repo values.
In `@src/views/Summary.tsx`:
- Around line 62-68: Add an inline comment above the isReview heuristic
explaining that the title-prefix check (item.title.startsWith('Review on:')) is
a legacy heuristic used for Events API-sourced items and may over-match if a PR
is legitimately titled that way; reference the isReview variable, the reviewedBy
check, the title.startsWith('Review on:') condition, and originalEventType ===
'PullRequestReviewEvent' so future readers know why the fragile title check
exists and that its impact is only cosmetic (affects groupingKey).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/ItemRow.tsxsrc/hooks/useGitHubDataFetching.tssrc/types.tssrc/utils/rawDataUtils.tssrc/utils/reviewDates.tssrc/views/Summary.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/rawDataUtils.ts
Summary
This PR adds comprehensive PR review tracking by leveraging the GitHub Search API's
reviewed-by:query. This provides more accurate and complete review data compared to the Events API, which is limited to ~300 events and 30 days of history.Key Changes
New review data pipeline: Added
indexedDBReviewItemsstorage and processing throughout the data flow (App.tsx, useGitHubDataFetching.ts, useGitHubDataProcessing.ts)Search API integration: Implemented
reviewed-by:query in useGitHubDataFetching.ts to fetch PRs reviewed by each user within the specified date range, with pagination support and error handlingSummary view enhancement: Extended
groupSummaryData()in summaryGrouping.ts to include reviewed PRs via newaddReviewedPRsFromSearchItems()function, which deduplicates against existing PR dataData processing: Added review items processing in useGitHubDataProcessing.ts to categorize and filter review data alongside other GitHub items
Test updates: Updated useGitHubDataFetching.test.ts to include mock functions for review item storage and clearing
Implementation Details
https://claude.ai/code/session_01C3KBpRv5F5LneNE8jKRDYN